Compare commits

...

9 commits

4 changed files with 313 additions and 131 deletions

View file

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

View file

@ -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>

View file

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

View file

@ -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)
})
}
}