diff --git a/services/repository/files/pathutils.go b/services/repository/files/pathutils.go
index ac095571f7..c764877cdf 100644
--- a/services/repository/files/pathutils.go
+++ b/services/repository/files/pathutils.go
@@ -21,22 +21,25 @@ func SanitizePath(inputPath string) (string, error) {
return "", fmt.Errorf("path starts with / : %s", inputPath)
}
- // Clean the path
- s = path.Clean(s)
+ // 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")
+ }
+
// 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 637185ad23..c42eed69a5 100644
--- a/templates/repo/editor/commit_form.tmpl
+++ b/templates/repo/editor/commit_form.tmpl
@@ -75,6 +75,8 @@
-
+
{{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 5254f5c114..89412f3d90 100644
--- a/tests/e2e/repo-files.test.e2e.ts
+++ b/tests/e2e/repo-files.test.e2e.ts
@@ -5,135 +5,102 @@
// templates/repo/editor/**
// web_src/js/features/common-global.js
// routers/web/web.go
-// services/repository/files/upload.go
+// services/repository/files/**
// @watch end
import {expect} from '@playwright/test';
-import {test, dynamic_id, save_visual} from './utils_e2e.ts';
+import {test, dynamic_id} 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', () => {
- test('normal and special characters', async ({page}) => {
- const response = await page.goto(`/user2/file-uploads/_upload/main/`);
- expect(response?.status()).toBe(200); // Status OK
+ 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',
+ ],
+ },
+ ];
- const testID = dynamic_id();
- const dropzone = page.getByRole('button', {name: 'Drop files or click here to upload.'});
+ // actual good tests based on definition above
+ for (const testCase of goodTestCases) {
+ test(`good: ${testCase.description}`, async ({page}) => {
+ await doUpload({page}, testCase);
- // 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;
+ // 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();
+ }
});
- // and drop them to the upload area
- await dropzone.dispatchEvent('drop', {dataTransfer: dataTransferA});
+ }
- await page.getByText('new branch').click();
+ const badTestCases: TestCase[] = [
+ {
+ description: 'broken path slash in front',
+ files: [
+ '/special/badfirstslash.txt',
+ ],
+ },
+ {
+ description: 'broken path with traversal',
+ files: [
+ '../baddots.txt',
+ ],
+ },
+ ];
- 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;
+ // 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();
});
- // 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
new file mode 100644
index 0000000000..6c96cba102
--- /dev/null
+++ b/tests/integration/pathutils_test.go
@@ -0,0 +1,210 @@
+// 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)
+ })
+ }
+}