diff --git a/services/repository/files/pathutils.go b/services/repository/files/pathutils.go
index c764877cdf..ac095571f7 100644
--- a/services/repository/files/pathutils.go
+++ b/services/repository/files/pathutils.go
@@ -21,25 +21,22 @@ func SanitizePath(inputPath string) (string, error) {
return "", fmt.Errorf("path starts with / : %s", inputPath)
}
- // Make path temporarily absolute to ensure proper cleaning of all components
- temp := "/" + s
-
- // Clean the path (this will properly handle ../.. components when absolute)
- temp = path.Clean(temp)
-
- // Remove the leading / to make it relative again
- s = strings.TrimPrefix(temp, "/")
-
- // If the cleaned path is empty or becomes root, it's invalid
- if s == "" {
- return "", fmt.Errorf("path resolves to root or becomes empty after cleaning")
- }
-
+ // Clean the path
+ s = path.Clean(s)
// Split the path components
pathComponents := strings.Split(s, "/")
// Sanitize each path component
var sanitizedComponents []string
for _, component := range pathComponents {
+ // There is no reason why there should be a path segment with ..
+ if component == ".." {
+ return "", fmt.Errorf("path contains directory traversal: %s", s)
+ }
+ // There is no reason why there should be a path segment with .
+ if component == "." {
+ return "", fmt.Errorf("path contains directory traversal: %s", s)
+ }
+
// Trim whitespace and apply regex sanitization
sanitizedComponent := strings.TrimSpace(fileNameComponentSanitizeRegexp.ReplaceAllString(component, "_"))
diff --git a/templates/repo/editor/commit_form.tmpl b/templates/repo/editor/commit_form.tmpl
index c42eed69a5..637185ad23 100644
--- a/templates/repo/editor/commit_form.tmpl
+++ b/templates/repo/editor/commit_form.tmpl
@@ -75,8 +75,6 @@
-
+
{{ctx.Locale.Tr "repo.editor.cancel"}}
diff --git a/tests/e2e/repo-files.test.e2e.ts b/tests/e2e/repo-files.test.e2e.ts
index 89412f3d90..5254f5c114 100644
--- a/tests/e2e/repo-files.test.e2e.ts
+++ b/tests/e2e/repo-files.test.e2e.ts
@@ -5,102 +5,135 @@
// templates/repo/editor/**
// web_src/js/features/common-global.js
// routers/web/web.go
-// services/repository/files/**
+// services/repository/files/upload.go
// @watch end
import {expect} from '@playwright/test';
-import {test, dynamic_id} from './utils_e2e.ts';
+import {test, dynamic_id, save_visual} from './utils_e2e.ts';
test.use({user: 'user2'});
-
-interface TestCase {
- description: string;
- files: string[];
-}
-
-async function doUpload({page}, testCase: TestCase) {
- await page.goto(`/user2/file-uploads/_upload/main/`);
- const testID = dynamic_id();
- const dropzone = page.getByRole('button', {name: 'Drop files or click here to upload.'});
-
- // create the virtual files
- const dataTransfer = await page.evaluateHandle((testCase: TestCase) => {
- const dt = new DataTransfer();
- for (const filename of testCase.files) {
- dt.items.add(new File([`File content of ${filename}`], filename, {type: 'text/plain'}));
- }
- return dt;
- }, testCase);
- // and drop them to the upload area
- await dropzone.dispatchEvent('drop', {dataTransfer});
-
- await page.getByText('new branch').click();
-
- await page.getByRole('textbox', {name: 'Name the new branch for this'}).fill(testID);
- // ToDo: Potential race condition: We do not currently wait for the upload to complete.
- // See https://codeberg.org/forgejo/forgejo/pulls/6687#issuecomment-5068272 and
- // https://codeberg.org/forgejo/forgejo/issues/5893#issuecomment-5068266 for details.
- // Workaround is to wait (the uploads are just a few bytes and usually complete instantly)
- //
- // eslint-disable-next-line playwright/no-wait-for-timeout
- await page.waitForTimeout(100);
-
- await page.getByRole('button', {name: 'Propose file change'}).click();
-}
-
test.describe('Drag and Drop upload', () => {
- const goodTestCases: TestCase[] = [
- {
- description: 'normal and special characters',
- files: [
- 'dir1/file1.txt',
- 'double/nested/file.txt',
- 'special/äüöÄÜÖß.txt',
- 'special/Ʉ₦ł₵ØĐɆ.txt',
- ],
- },
- {
- description: 'strange paths and spaces',
- files: [
- '..dots.txt',
- '.dots.preserved.txt',
- 'special/S P A C E !.txt',
- ],
- },
- ];
+ test('normal and special characters', async ({page}) => {
+ const response = await page.goto(`/user2/file-uploads/_upload/main/`);
+ expect(response?.status()).toBe(200); // Status OK
- // actual good tests based on definition above
- for (const testCase of goodTestCases) {
- test(`good: ${testCase.description}`, async ({page}) => {
- await doUpload({page}, testCase);
+ const testID = dynamic_id();
+ const dropzone = page.getByRole('button', {name: 'Drop files or click here to upload.'});
- // check that nested file structure is preserved
- for (const filename of testCase.files) {
- await expect(page.locator('#diff-file-boxes').getByRole('link', {name: filename})).toBeVisible();
- }
+ // create the virtual files
+ const dataTransferA = await page.evaluateHandle(() => {
+ const dt = new DataTransfer();
+ // add items in different folders
+ dt.items.add(new File(['Filecontent (dir1/file1.txt)'], 'dir1/file1.txt', {type: 'text/plain'}));
+ dt.items.add(new File(["Another file's content(double / nested / file.txt)"], 'double / nested / file.txt', {type: 'text / plain'}));
+ dt.items.add(new File(['Root file (root_file.txt)'], 'root_file.txt', {type: 'text/plain'}));
+ dt.items.add(new File(['Umlaut test'], 'special/äüöÄÜÖß.txt', {type: 'text/plain'}));
+ dt.items.add(new File(['Unicode test'], 'special/Ʉ₦ł₵ØĐɆ.txt', {type: 'text/plain'}));
+ return dt;
});
- }
+ // and drop them to the upload area
+ await dropzone.dispatchEvent('drop', {dataTransfer: dataTransferA});
- const badTestCases: TestCase[] = [
- {
- description: 'broken path slash in front',
- files: [
- '/special/badfirstslash.txt',
- ],
- },
- {
- description: 'broken path with traversal',
- files: [
- '../baddots.txt',
- ],
- },
- ];
+ await page.getByText('new branch').click();
- // actual bad tests based on definition above
- for (const testCase of badTestCases) {
- test(`bad: ${testCase.description}`, async ({page}) => {
- await doUpload({page}, testCase);
- await expect(page.getByText('Failed to upload files to')).toBeVisible();
+ await page.getByRole('textbox', {name: 'Name the new branch for this'}).fill(testID);
+ await page.getByRole('button', {name: 'Propose file change'}).click();
+
+ // check that nested file structure is preserved
+ await expect(page.getByRole('link', {name: 'dir1/file1.txt'})).toBeVisible();
+ await expect(page.getByRole('link', {name: 'double/nested/file.txt'})).toBeVisible();
+ await expect(page.getByRole('link', {name: 'special/äüöÄÜÖß.txt'})).toBeVisible();
+ await expect(page.getByRole('link', {name: 'special/Ʉ₦ł₵ØĐɆ.txt'})).toBeVisible();
+ // Since this is a file in root, there two links with the same label
+ // we take the on in #diff-file-tree
+ await expect(page.locator('#diff-file-boxes').getByRole('link', {name: 'root_file.txt'})).toBeVisible();
+ });
+
+ test('strange paths and spaces', async ({page}) => {
+ const response = await page.goto(`/user2/file-uploads/_upload/main/`);
+ expect(response?.status()).toBe(200); // Status OK
+
+ const testID = dynamic_id();
+ const dropzone = page.getByRole('button', {name: 'Drop files or click here to upload.'});
+
+ // create the virtual files
+ const dataTransferA = await page.evaluateHandle(() => {
+ const dt = new DataTransfer();
+ // add items in different folders
+ dt.items.add(new File(['1'], '..dots.txt', {type: 'text/plain'}));
+ dt.items.add(new File(['2'], '.dots.vanish.txt', {type: 'text/plain'}));
+ dt.items.add(new File(['3'], 'special/S P A C E !.txt', {type: 'text/plain'}));
+ return dt;
});
- }
+ // and drop them to the upload area
+ await dropzone.dispatchEvent('drop', {dataTransfer: dataTransferA});
+
+ await page.getByText('new branch').click();
+
+ await page.getByRole('textbox', {name: 'Name the new branch for this'}).fill(testID);
+ await page.getByRole('button', {name: 'Propose file change'}).click();
+
+ // check that nested file structure is preserved
+ // Since this is a file in root, there two links with the same label
+ // we take the on in #diff-file-tree
+ await expect(page.locator('#diff-file-boxes').getByRole('link', {name: '.dots.vanish.txt'})).toBeVisible();
+ await expect(page.getByRole('link', {name: 'special/S P A C E !.txt'})).toBeVisible();
+ // Since this is a file in root, there two links with the same label
+ // we take the on in #diff-file-tree
+ await expect(page.locator('#diff-file-boxes').getByRole('link', {name: '..dots.txt'})).toBeVisible();
+ });
+
+ test('broken path slash in front', async ({page}) => {
+ const response = await page.goto(`/user2/file-uploads/_upload/main/`);
+ expect(response?.status()).toBe(200); // Status OK
+
+ const testID = dynamic_id();
+ const dropzone = page.getByRole('button', {name: 'Drop files or click here to upload.'});
+
+ // create the virtual files
+ const dataTransferA = await page.evaluateHandle(() => {
+ const dt = new DataTransfer();
+ // add items in different folders
+ dt.items.add(new File(['1'], '/special/badfirstslash.txt', {type: 'text/plain'}));
+ return dt;
+ });
+ // and drop them to the upload area
+ await dropzone.dispatchEvent('drop', {dataTransfer: dataTransferA});
+
+ await page.getByText('new branch').click();
+
+ await page.getByRole('textbox', {name: 'Name the new branch for this'}).fill(testID);
+ await page.getByRole('button', {name: 'Propose file change'}).click();
+
+ await expect(page.getByText('Failed to upload files to')).toBeVisible();
+
+ await save_visual(page);
+ });
+
+ test('broken path with traversal', async ({page}) => {
+ const response = await page.goto(`/user2/file-uploads/_upload/main/`);
+ expect(response?.status()).toBe(200); // Status OK
+
+ const testID = dynamic_id();
+ const dropzone = page.getByRole('button', {name: 'Drop files or click here to upload.'});
+
+ // create the virtual files
+ const dataTransferA = await page.evaluateHandle(() => {
+ const dt = new DataTransfer();
+ // add items in different folders
+ dt.items.add(new File(['1'], '../baddots.txt', {type: 'text/plain'}));
+ return dt;
+ });
+ // and drop them to the upload area
+ await dropzone.dispatchEvent('drop', {dataTransfer: dataTransferA});
+
+ await page.getByText('new branch').click();
+
+ await page.getByRole('textbox', {name: 'Name the new branch for this'}).fill(testID);
+ await page.getByRole('button', {name: 'Propose file change'}).click();
+
+ await expect(page.getByText('Failed to upload files to')).toBeVisible();
+
+ await save_visual(page);
+ });
});
diff --git a/tests/integration/pathutils_test.go b/tests/integration/pathutils_test.go
deleted file mode 100644
index 6c96cba102..0000000000
--- a/tests/integration/pathutils_test.go
+++ /dev/null
@@ -1,210 +0,0 @@
-// Copyright 2019 The Gitea Authors. All rights reserved.
-// SPDX-License-Identifier: MIT
-package integration
-
-import (
- "testing"
-
- "forgejo.org/services/repository/files"
-
- "github.com/stretchr/testify/assert"
- "github.com/stretchr/testify/require"
-)
-
-func TestSanitizePath(t *testing.T) {
- tests := []struct {
- name string
- input string
- expected string
- expectError bool
- }{
- // Valid paths
- {
- name: "simple valid path",
- input: "folder/file.txt",
- expected: "folder/file.txt",
- },
- {
- name: "single file",
- input: "file.txt",
- expected: "file.txt",
- },
- {
- name: "nested path",
- input: "a/b/c/file.txt",
- expected: "a/b/c/file.txt",
- },
-
- // Path normalization
- {
- name: "backslash to forward slash",
- input: "folder\\file.txt",
- expected: "folder/file.txt",
- },
- {
- name: "mixed separators",
- input: "folder\\subfolder/file.txt",
- expected: "folder/subfolder/file.txt",
- },
- {
- name: "double separators",
- input: "folder//file.txt",
- expected: "folder/file.txt",
- },
- {
- name: "trailing slash",
- input: "folder/file.txt/",
- expected: "folder/file.txt",
- },
- {
- name: "dot segments",
- input: "folder/./file.txt",
- expected: "folder/file.txt",
- },
- {
- name: "parent directory references",
- input: "folder/../other/file.txt",
- expected: "other/file.txt",
- },
-
- // Character sanitization
- {
- name: "illegal characters replaced",
- input: "file.txt",
- expected: "file_name_.txt",
- },
- {
- name: "multiple illegal characters",
- input: "file:name|with?bad*chars.txt",
- expected: "file_name_with_bad_chars.txt",
- },
- {
- name: "quotes in filename",
- input: `file"name.txt`,
- expected: "file_name.txt",
- },
- {
- name: "control characters",
- input: "file\x00\x01name.txt",
- expected: "file__name.txt",
- },
-
- // Whitespace handling
- {
- name: "leading whitespace",
- input: " file.txt",
- expected: "file.txt",
- },
- {
- name: "trailing whitespace",
- input: "file.txt ",
- expected: "file.txt",
- },
- {
- name: "whitespace in path components",
- input: " folder / file.txt ",
- expected: "folder/file.txt",
- },
-
- // Edge cases that should return errors
- {
- name: "path starts with slash",
- input: "/folder/file.txt",
- expectError: true,
- },
- {
- name: "empty string",
- input: "",
- expectError: true,
- },
- {
- name: "only separators",
- input: "///",
- expectError: true,
- },
- {
- name: "only illegal characters",
- input: "<>:\"|?*",
- expected: "_______",
- },
- {
- name: "only whitespace",
- input: " ",
- expectError: true,
- },
- {
- name: "path that resolves to root",
- input: "../..",
- expectError: true,
- },
- {
- name: "path that goes above root",
- input: "folder/../../..",
- expectError: true,
- },
-
- // Complex combinations
- {
- name: "complex path with multiple issues",
- input: "folder\\with:illegal|chars/normal_file.txt",
- expected: "folder/with_illegal_chars/normal_file.txt",
- },
- {
- name: "unicode characters preserved",
- input: "folder/файл.txt",
- expected: "folder/файл.txt",
- },
- {
- name: "dots and extensions",
- input: "file.name.with.dots.txt",
- expected: "file.name.with.dots.txt",
- },
- }
-
- for _, tt := range tests {
- t.Run(tt.name, func(t *testing.T) {
- result, err := files.SanitizePath(tt.input)
-
- if tt.expectError {
- require.Error(t, err, "expected error for input %q", tt.input)
- return
- }
-
- require.NoError(t, err, "unexpected error for input %q", tt.input)
- assert.Equal(t, tt.expected, result, "SanitizePath(%q) should return expected result", tt.input)
- })
- }
-}
-
-// TestSanitizePathErrorMessages tests that error messages are informative
-func TestSanitizePathErrorMessages(t *testing.T) {
- tests := []struct {
- name string
- input string
- expectedError string
- }{
- {
- name: "path starts with slash",
- input: "/test/path",
- expectedError: "path starts with / : /test/path",
- },
- {
- name: "path that resolves to root",
- input: "../..",
- expectedError: "path resolves to root or becomes empty after cleaning",
- },
- {
- name: "empty after sanitization",
- input: "",
- expectedError: "path resolves to root or becomes empty after cleaning",
- },
- }
-
- for _, tt := range tests {
- t.Run(tt.name, func(t *testing.T) {
- _, err := files.SanitizePath(tt.input)
- require.Error(t, err, "expected error for input %q", tt.input)
- assert.Equal(t, tt.expectedError, err.Error(), "error message for %q should match expected", tt.input)
- })
- }
-}