Linter didn't likes the pathutils_test.go. Now it want a different order for the imports

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, "_"))
 

templates/repo/editor/commit_form.tmpl

@@ -75,6 +75,8 @@
 			</select>
 		</div>
 	</div>
-	<button id="commit-button" type="submit" class="ui primary button">{{if eq .commit_choice "commit-to-new-branch"}}{{ctx.Locale.Tr "repo.editor.propose_file_change"}}{{else}}{{ctx.Locale.Tr "repo.editor.commit_changes"}}{{end}}</button>
+	<button id="commit-button" type="submit" class="ui primary button">
+		{{if eq .commit_choice "commit-to-new-branch"}}{{ctx.Locale.Tr "repo.editor.propose_file_change"}}{{else}}{{ctx.Locale.Tr "repo.editor.commit_changes"}}{{end}}
+	</button>
 	<a class="ui button red" href="{{$.BranchLink}}/{{PathEscapeSegments .TreePath}}">{{ctx.Locale.Tr "repo.editor.cancel"}}</a>
 </div>

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);
-  });
+  }
 });

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<name>.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)
+		})
+	}
+}