From 8723da989d416b954c63b889a78a6c5bc1870626 Mon Sep 17 00:00:00 2001 From: Otto Richter Date: Tue, 10 Jun 2025 14:30:11 +0200 Subject: [PATCH 1/9] Refactor e2e test --- tests/e2e/repo-files.test.e2e.ts | 206 ++++++++++++++----------------- 1 file changed, 93 insertions(+), 113 deletions(-) diff --git a/tests/e2e/repo-files.test.e2e.ts b/tests/e2e/repo-files.test.e2e.ts index 5254f5c114..8edc6f12b8 100644 --- a/tests/e2e/repo-files.test.e2e.ts +++ b/tests/e2e/repo-files.test.e2e.ts @@ -12,128 +12,108 @@ import {expect} from '@playwright/test'; import {test, dynamic_id, save_visual} from './utils_e2e.ts'; test.use({user: 'user2'}); + +interface TestCaseFilenames { + raw: string; + sanitized: string; +} + +interface TestCase { + description: string; + files: string[]; +} + 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: TestCaseFile[] = [ + { + 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}) => { + console.log(testCase); + const response = 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 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; + // 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: dataTransfer}); + + 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 + 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: TestCaseFile[] = [ + { + 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(); + // actual bad tests based on definition above + for (const testCase of badTestCases) { + test(`bad: ${testCase.description}`, async ({page}) => { + const response = 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.'}); - // 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(); - }); + // 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: dataTransfer}); - test('strange paths and spaces', async ({page}) => { - const response = await page.goto(`/user2/file-uploads/_upload/main/`); - expect(response?.status()).toBe(200); // Status OK + await page.getByText('new branch').click(); - const testID = dynamic_id(); - const dropzone = page.getByRole('button', {name: 'Drop files or click here to upload.'}); + await page.getByRole('textbox', {name: 'Name the new branch for this'}).fill(testID); + await page.getByRole('button', {name: 'Propose file change'}).click(); - // 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; + 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); - }); + } }); From 5da912f0a05d6f2a584d94dcd4e06d60b1aa5cb1 Mon Sep 17 00:00:00 2001 From: Otto Richter Date: Tue, 10 Jun 2025 14:33:54 +0200 Subject: [PATCH 2/9] lint fixes --- tests/e2e/repo-files.test.e2e.ts | 44 ++++++++++++++------------------ 1 file changed, 19 insertions(+), 25 deletions(-) diff --git a/tests/e2e/repo-files.test.e2e.ts b/tests/e2e/repo-files.test.e2e.ts index 8edc6f12b8..1259ce4256 100644 --- a/tests/e2e/repo-files.test.e2e.ts +++ b/tests/e2e/repo-files.test.e2e.ts @@ -9,46 +9,40 @@ // @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 TestCaseFilenames { - raw: string; - sanitized: string; -} - interface TestCase { - description: string; - files: string[]; + description: string; + files: string[]; } test.describe('Drag and Drop upload', () => { - const goodTestCases: TestCaseFile[] = [ + const goodTestCases: TestCase[] = [ { description: 'normal and special characters', files: [ 'dir1/file1.txt', 'double/nested/file.txt', 'special/äüöÄÜÖß.txt', - 'special/Ʉ₦ł₵ØĐɆ.txt' - ] + 'special/Ʉ₦ł₵ØĐɆ.txt', + ], }, { description: 'strange paths and spaces', files: [ '..dots.txt', '.dots.preserved.txt', - 'special/S P A C E !.txt' - ] - } + 'special/S P A C E !.txt', + ], + }, ]; // actual good tests based on definition above for (const testCase of goodTestCases) { test(`good: ${testCase.description}`, async ({page}) => { - console.log(testCase); - const response = await page.goto(`/user2/file-uploads/_upload/main/`); + 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.'}); @@ -61,7 +55,7 @@ test.describe('Drag and Drop upload', () => { return dt; }, testCase); // and drop them to the upload area - await dropzone.dispatchEvent('drop', {dataTransfer: dataTransfer}); + await dropzone.dispatchEvent('drop', {dataTransfer}); await page.getByText('new branch').click(); @@ -75,25 +69,25 @@ test.describe('Drag and Drop upload', () => { }); } - const badTestCases: TestCaseFile[] = [ + const badTestCases: TestCase[] = [ { description: 'broken path slash in front', files: [ - '/special/badfirstslash.txt' - ] + '/special/badfirstslash.txt', + ], }, { description: 'broken path with traversal', files: [ - '../baddots.txt' - ] - } + '../baddots.txt', + ], + }, ]; // actual bad tests based on definition above for (const testCase of badTestCases) { test(`bad: ${testCase.description}`, async ({page}) => { - const response = await page.goto(`/user2/file-uploads/_upload/main/`); + 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.'}); @@ -106,7 +100,7 @@ test.describe('Drag and Drop upload', () => { return dt; }, testCase); // and drop them to the upload area - await dropzone.dispatchEvent('drop', {dataTransfer: dataTransfer}); + await dropzone.dispatchEvent('drop', {dataTransfer}); await page.getByText('new branch').click(); From d2753f9845483741b28ebbbb9a624f36f3a3ecd7 Mon Sep 17 00:00:00 2001 From: Otto Richter Date: Tue, 10 Jun 2025 14:37:58 +0200 Subject: [PATCH 3/9] Deduplicate test further --- tests/e2e/repo-files.test.e2e.ts | 63 ++++++++++++-------------------- 1 file changed, 24 insertions(+), 39 deletions(-) diff --git a/tests/e2e/repo-files.test.e2e.ts b/tests/e2e/repo-files.test.e2e.ts index 1259ce4256..147ff68961 100644 --- a/tests/e2e/repo-files.test.e2e.ts +++ b/tests/e2e/repo-files.test.e2e.ts @@ -18,6 +18,28 @@ interface TestCase { 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); + await page.getByRole('button', {name: 'Propose file change'}).click(); +} + test.describe('Drag and Drop upload', () => { const goodTestCases: TestCase[] = [ { @@ -42,25 +64,7 @@ test.describe('Drag and Drop upload', () => { // actual good tests based on definition above for (const testCase of goodTestCases) { test(`good: ${testCase.description}`, async ({page}) => { - 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); - await page.getByRole('button', {name: 'Propose file change'}).click(); + await doUpload({page}, testCase); // check that nested file structure is preserved for (const filename of testCase.files) { @@ -87,26 +91,7 @@ test.describe('Drag and Drop upload', () => { // actual bad tests based on definition above for (const testCase of badTestCases) { test(`bad: ${testCase.description}`, async ({page}) => { - 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); - await page.getByRole('button', {name: 'Propose file change'}).click(); - + await doUpload({page}, testCase); await expect(page.getByText('Failed to upload files to')).toBeVisible(); }); } From 74e5ca6c8ee15bfc6dc04246c5a0a0a299080b87 Mon Sep 17 00:00:00 2001 From: Otto Richter Date: Tue, 10 Jun 2025 15:03:55 +0200 Subject: [PATCH 4/9] Workaround race condition, adjust watch patterns --- tests/e2e/repo-files.test.e2e.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/e2e/repo-files.test.e2e.ts b/tests/e2e/repo-files.test.e2e.ts index 147ff68961..89412f3d90 100644 --- a/tests/e2e/repo-files.test.e2e.ts +++ b/tests/e2e/repo-files.test.e2e.ts @@ -5,7 +5,7 @@ // 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'; @@ -37,6 +37,14 @@ async function doUpload({page}, testCase: TestCase) { 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(); } From 3e664400f84f94197897e3cc81461988c5117315 Mon Sep 17 00:00:00 2001 From: David Rotermund Date: Tue, 10 Jun 2025 15:37:23 +0200 Subject: [PATCH 5/9] Restored the multi line structure in templates/repo/editor/commit_form.tmpl --- templates/repo/editor/commit_form.tmpl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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"}} From 68c860d7170911a55c8ef6f4734022f7064925bd Mon Sep 17 00:00:00 2001 From: David Rotermund Date: Tue, 10 Jun 2025 17:27:04 +0200 Subject: [PATCH 6/9] Remove unnecessary checks in services/repository/files/pathutils.go --- services/repository/files/pathutils.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/services/repository/files/pathutils.go b/services/repository/files/pathutils.go index ac095571f7..d3d5a40b94 100644 --- a/services/repository/files/pathutils.go +++ b/services/repository/files/pathutils.go @@ -28,15 +28,6 @@ func SanitizePath(inputPath string) (string, error) { // 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, "_")) From c920c21f317cbdb1fbc005c4a7c2b1b520867745 Mon Sep 17 00:00:00 2001 From: David Rotermund Date: Tue, 10 Jun 2025 18:16:39 +0200 Subject: [PATCH 7/9] Add pathutils test as well as adding a temporary slash for helping path.Clean --- services/repository/files/pathutils.go | 16 +- tests/integration/pathutils_test.go | 208 +++++++++++++++++++++++++ 2 files changed, 222 insertions(+), 2 deletions(-) create mode 100644 tests/integration/pathutils_test.go diff --git a/services/repository/files/pathutils.go b/services/repository/files/pathutils.go index d3d5a40b94..c764877cdf 100644 --- a/services/repository/files/pathutils.go +++ b/services/repository/files/pathutils.go @@ -21,8 +21,20 @@ 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 diff --git a/tests/integration/pathutils_test.go b/tests/integration/pathutils_test.go new file mode 100644 index 0000000000..6aaf54899d --- /dev/null +++ b/tests/integration/pathutils_test.go @@ -0,0 +1,208 @@ +// 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" +) + +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 { + assert.Error(t, err, "expected error for input %q", tt.input) + return + } + + assert.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) + assert.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) + }) + } +} From b3e64faf8eaa52237a6dade06b2dc25e14260b9a Mon Sep 17 00:00:00 2001 From: David Rotermund Date: Tue, 10 Jun 2025 18:26:08 +0200 Subject: [PATCH 8/9] Linter didn't likes the pathutils_test.go. It wanted require. --- tests/integration/pathutils_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/integration/pathutils_test.go b/tests/integration/pathutils_test.go index 6aaf54899d..f08014434d 100644 --- a/tests/integration/pathutils_test.go +++ b/tests/integration/pathutils_test.go @@ -5,8 +5,10 @@ package integration import ( "testing" - "forgejo.org/services/repository/files" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "forgejo.org/services/repository/files" ) func TestSanitizePath(t *testing.T) { @@ -164,11 +166,11 @@ func TestSanitizePath(t *testing.T) { result, err := files.SanitizePath(tt.input) if tt.expectError { - assert.Error(t, err, "expected error for input %q", tt.input) + require.Error(t, err, "expected error for input %q", tt.input) return } - assert.NoError(t, err, "unexpected error for input %q", tt.input) + 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) }) } @@ -201,7 +203,7 @@ func TestSanitizePathErrorMessages(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { _, err := files.SanitizePath(tt.input) - assert.Error(t, err, "expected error for input %q", 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) }) } From fb9e663660d03ba6f0d4a0f1ddf02a9f59a555ae Mon Sep 17 00:00:00 2001 From: David Rotermund Date: Tue, 10 Jun 2025 18:34:17 +0200 Subject: [PATCH 9/9] Linter didn't likes the pathutils_test.go. Now it want a different order for the imports --- tests/integration/pathutils_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/pathutils_test.go b/tests/integration/pathutils_test.go index f08014434d..6c96cba102 100644 --- a/tests/integration/pathutils_test.go +++ b/tests/integration/pathutils_test.go @@ -5,10 +5,10 @@ package integration import ( "testing" + "forgejo.org/services/repository/files" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - - "forgejo.org/services/repository/files" ) func TestSanitizePath(t *testing.T) {