From a6264998e6f4cb3a42fab7bbde299b0764bb9124 Mon Sep 17 00:00:00 2001 From: David Rotermund Date: Mon, 2 Jun 2025 15:41:06 +0200 Subject: [PATCH] Cleaning up the commit history. Last relevent commits were: Cleaned the comments and used test.describe to nest the tests ; fix the 0ko conflict --- models/repo/upload.go | 11 -- routers/web/repo/editor.go | 1 + routers/web/web.go | 20 +++- services/forms/repo_form.go | 1 + services/repository/files/pathutils.go | 55 ++++++++++ services/repository/files/upload.go | 28 +++-- templates/repo/editor/commit_form.tmpl | 4 +- tests/e2e/declare_repos_test.go | 6 +- tests/e2e/repo-files.test.e2e.ts | 139 +++++++++++++++++++++++++ tests/integration/editor_test.go | 12 ++- tests/integration/empty_repo_test.go | 4 +- web_src/js/features/common-global.js | 8 +- 12 files changed, 258 insertions(+), 31 deletions(-) create mode 100644 services/repository/files/pathutils.go create mode 100644 tests/e2e/repo-files.test.e2e.ts diff --git a/models/repo/upload.go b/models/repo/upload.go index 49152db7fd..a213cb1986 100644 --- a/models/repo/upload.go +++ b/models/repo/upload.go @@ -104,17 +104,6 @@ func GetUploadByUUID(ctx context.Context, uuid string) (*Upload, error) { return upload, nil } -// GetUploadsByUUIDs returns multiple uploads by UUIDS -func GetUploadsByUUIDs(ctx context.Context, uuids []string) ([]*Upload, error) { - if len(uuids) == 0 { - return []*Upload{}, nil - } - - // Silently drop invalid uuids. - uploads := make([]*Upload, 0, len(uuids)) - return uploads, db.GetEngine(ctx).In("uuid", uuids).Find(&uploads) -} - // DeleteUploads deletes multiple uploads func DeleteUploads(ctx context.Context, uploads ...*Upload) (err error) { if len(uploads) == 0 { diff --git a/routers/web/repo/editor.go b/routers/web/repo/editor.go index 5114cc9c05..d40a02ad0e 100644 --- a/routers/web/repo/editor.go +++ b/routers/web/repo/editor.go @@ -758,6 +758,7 @@ func UploadFilePost(ctx *context.Context) { TreePath: form.TreePath, Message: message, Files: form.Files, + FullPaths: form.FullPaths, Signoff: form.Signoff, Author: gitIdentity, Committer: gitIdentity, diff --git a/routers/web/web.go b/routers/web/web.go index f8a13dab7e..6164ef2a55 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -51,6 +51,7 @@ import ( _ "forgejo.org/modules/session" // to registers all internal adapters + "code.forgejo.org/go-chi/binding" "code.forgejo.org/go-chi/captcha" chi_middleware "github.com/go-chi/chi/v5/middleware" "github.com/go-chi/cors" @@ -1259,7 +1260,7 @@ func registerRoutes(m *web.Route) { Post(web.Bind(forms.DeleteRepoFileForm{}), repo.DeleteFilePost) m.Combo("/_upload/*", repo.MustBeAbleToUpload). Get(repo.UploadFile). - Post(web.Bind(forms.UploadRepoFileForm{}), repo.UploadFilePost) + Post(BindUpload(forms.UploadRepoFileForm{}), repo.UploadFilePost) m.Combo("/_diffpatch/*").Get(repo.NewDiffPatch). Post(web.Bind(forms.EditRepoFileForm{}), repo.NewDiffPatchPost) m.Combo("/_cherrypick/{sha:([a-f0-9]{4,64})}/*").Get(repo.CherryPick). @@ -1678,3 +1679,20 @@ func registerRoutes(m *web.Route) { ctx.NotFound("", nil) }) } + +func BindUpload(f forms.UploadRepoFileForm) http.HandlerFunc { + return func(resp http.ResponseWriter, req *http.Request) { + theObj := new(forms.UploadRepoFileForm) // create a new form obj for every request but not use obj directly + data := middleware.GetContextData(req.Context()) + binding.Bind(req, theObj) + files := theObj.Files + var fullpaths []string + for _, fileID := range files { + fullPath := req.Form.Get("files_fullpath[" + fileID + "]") + fullpaths = append(fullpaths, fullPath) + } + theObj.FullPaths = fullpaths + data.GetData()["__form"] = theObj + middleware.AssignForm(theObj, data) + } +} diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index 4a46c9cc5f..dbf3ee8797 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -671,6 +671,7 @@ type UploadRepoFileForm struct { CommitChoice string `binding:"Required;MaxSize(50)"` NewBranchName string `binding:"GitRefName;MaxSize(100)"` Files []string + FullPaths []string CommitMailID int64 `binding:"Required"` Signoff bool } diff --git a/services/repository/files/pathutils.go b/services/repository/files/pathutils.go new file mode 100644 index 0000000000..ac095571f7 --- /dev/null +++ b/services/repository/files/pathutils.go @@ -0,0 +1,55 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT +package files + +import ( + "fmt" + "path" + "regexp" + "strings" +) + +var fileNameComponentSanitizeRegexp = regexp.MustCompile(`(?i)[<>:\"/\\|?*\x{0000}-\x{001F}]|^(con|prn|aux|nul|com\d|lpt\d)$`) + +// SanitizePath cleans and validates a file path +func SanitizePath(inputPath string) (string, error) { + // Normalize path separators + s := strings.ReplaceAll(inputPath, "\\", "/") + + // We don't want a / or \\ as the beginning of a path + if strings.HasPrefix(inputPath, "/") { + return "", fmt.Errorf("path starts with / : %s", inputPath) + } + + // 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, "_")) + + // Skip empty components after sanitization + if sanitizedComponent != "" { + sanitizedComponents = append(sanitizedComponents, sanitizedComponent) + } + } + // Check if we have any components left after sanitization + if len(sanitizedComponents) == 0 { + return "", fmt.Errorf("path became empty after sanitization") + } + // Reconstruct the path + reconstructedPath := path.Join(sanitizedComponents...) + return reconstructedPath, nil +} diff --git a/services/repository/files/upload.go b/services/repository/files/upload.go index 6359087e88..032cb107ad 100644 --- a/services/repository/files/upload.go +++ b/services/repository/files/upload.go @@ -28,6 +28,7 @@ type UploadRepoFileOptions struct { Author *IdentityOptions Committer *IdentityOptions Files []string // In UUID format. + FullPaths []string Signoff bool } @@ -56,16 +57,25 @@ func UploadRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use return nil } - uploads, err := repo_model.GetUploadsByUUIDs(ctx, opts.Files) - if err != nil { - return fmt.Errorf("GetUploadsByUUIDs [uuids: %v]: %w", opts.Files, err) + if len(opts.Files) != len(opts.FullPaths) { + return fmt.Errorf("the length of opts.Files and opts.FullPaths is not the same") } - names := make([]string, len(uploads)) - infos := make([]uploadInfo, len(uploads)) - for i, upload := range uploads { + uploads := make([]*repo_model.Upload, len(opts.Files)) + names := make([]string, len(opts.Files)) + infos := make([]uploadInfo, len(opts.Files)) + var err error + for i := 0; i < len(opts.Files); i++ { + uploads[i], err = repo_model.GetUploadByUUID(ctx, opts.Files[i]) + if err != nil { + return fmt.Errorf("GetUploadByUUID [uuids: %v]: %w", opts.Files[i], err) + } + uploads[i].Name, err = SanitizePath(opts.FullPaths[i]) + if err != nil { + return fmt.Errorf("SanitizePath [path: %v]: %w", opts.FullPaths[i], err) + } // Check file is not lfs locked, will return nil if lock setting not enabled - filepath := path.Join(opts.TreePath, upload.Name) + filepath := path.Join(opts.TreePath, uploads[i].Name) lfsLock, err := git_model.GetTreePathLock(ctx, repo.ID, filepath) if err != nil { return err @@ -78,8 +88,8 @@ func UploadRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use return git_model.ErrLFSFileLocked{RepoID: repo.ID, Path: filepath, UserName: u.Name} } - names[i] = upload.Name - infos[i] = uploadInfo{upload: upload} + names[i] = uploads[i].Name + infos[i] = uploadInfo{upload: uploads[i]} } t, err := NewTemporaryUploadRepository(ctx, repo) 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/declare_repos_test.go b/tests/e2e/declare_repos_test.go index 83ee40c71a..8b1a8536d7 100644 --- a/tests/e2e/declare_repos_test.go +++ b/tests/e2e/declare_repos_test.go @@ -53,6 +53,10 @@ func DeclareGitRepos(t *testing.T) func() { CommitMsg: "Another commit which mentions @user1 in the title\nand @user2 in the text", }, }), + newRepo(t, 2, "file-uploads", []FileChanges{{ + Filename: "README.md", + Versions: []string{"# File upload test\nUse this repo to test various file upload features in new branches."}, + }}), newRepo(t, 2, "unicode-escaping", []FileChanges{{ Filename: "a-file", Versions: []string{"{a}{а}"}, @@ -77,7 +81,7 @@ func newRepo(t *testing.T, userID int64, repoName string, fileChanges []FileChan for _, file := range fileChanges { for i, version := range file.Versions { operation := "update" - if i == 0 { + if i == 0 && file.Filename != "README.md" { operation = "create" } diff --git a/tests/e2e/repo-files.test.e2e.ts b/tests/e2e/repo-files.test.e2e.ts new file mode 100644 index 0000000000..5254f5c114 --- /dev/null +++ b/tests/e2e/repo-files.test.e2e.ts @@ -0,0 +1,139 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +// @watch start +// templates/repo/editor/** +// web_src/js/features/common-global.js +// routers/web/web.go +// services/repository/files/upload.go +// @watch end + +import {expect} from '@playwright/test'; +import {test, dynamic_id, save_visual} from './utils_e2e.ts'; + +test.use({user: 'user2'}); +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 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; + }); + // 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 + 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/editor_test.go b/tests/integration/editor_test.go index b6faf8a118..bc2c4dcb84 100644 --- a/tests/integration/editor_test.go +++ b/tests/integration/editor_test.go @@ -407,22 +407,26 @@ func TestCommitMail(t *testing.T) { file1UUID := uploadFile(t, "upload_file_1", "Uploaded a file!") file2UUID := uploadFile(t, "upload_file_2", "Uploaded another file!") + file1UUIDFullpathKey := fmt.Sprintf("files_fullpath[%s]", file1UUID) + file2UUIDFullpathKey := fmt.Sprintf("files_fullpath[%s]", file2UUID) assertCase(t, caseOpts{ fileName: "upload_file_1", link: "user2/repo1/_upload/master", skipLastCommit: true, base: map[string]string{ - "commit_choice": "direct", - "files": file1UUID, + "commit_choice": "direct", + "files": file1UUID, + file1UUIDFullpathKey: "upload_file_1", }, }, caseOpts{ fileName: "upload_file_2", link: "user2/repo1/_upload/master", skipLastCommit: true, base: map[string]string{ - "commit_choice": "direct", - "files": file2UUID, + "commit_choice": "direct", + "files": file2UUID, + file2UUIDFullpathKey: "upload_file_2", }, }, ) diff --git a/tests/integration/empty_repo_test.go b/tests/integration/empty_repo_test.go index 151f9450bd..ba5db5ded4 100644 --- a/tests/integration/empty_repo_test.go +++ b/tests/integration/empty_repo_test.go @@ -6,6 +6,7 @@ package integration import ( "bytes" "encoding/base64" + "fmt" "io" "mime/multipart" "net/http" @@ -88,11 +89,12 @@ func TestEmptyRepoUploadFile(t *testing.T) { resp = session.MakeRequest(t, req, http.StatusOK) respMap := map[string]string{} require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &respMap)) - + filesFullpathKey := fmt.Sprintf("files_fullpath[%s]", respMap["uuid"]) req = NewRequestWithValues(t, "POST", "/user30/empty/_upload/"+setting.Repository.DefaultBranch, map[string]string{ "_csrf": GetCSRF(t, session, "/user/settings"), "commit_choice": "direct", "files": respMap["uuid"], + filesFullpathKey: "uploaded-file.txt", "tree_path": "", "commit_mail_id": "-1", }) diff --git a/web_src/js/features/common-global.js b/web_src/js/features/common-global.js index 7d553f9692..1aa9edf60f 100644 --- a/web_src/js/features/common-global.js +++ b/web_src/js/features/common-global.js @@ -229,7 +229,8 @@ export function initDropzone(el) { this.on('success', (file, data) => { file.uuid = data.uuid; const $input = $(``).val(data.uuid); - $dropzone.find('.files').append($input); + const $inputPath = $(``); + $dropzone.find('.files').append($input).append($inputPath); // Create a "Copy Link" element, to conveniently copy the image // or file link as Markdown to the clipboard const copyLinkElement = document.createElement('div'); @@ -250,7 +251,12 @@ export function initDropzone(el) { file.previewTemplate.append(copyLinkElement); }); this.on('removedfile', (file) => { + // Remove the hidden input for the file $(`#${file.uuid}`).remove(); + + // Remove the hidden input for files_fullpath + $(`input[name="files_fullpath[${file.uuid}]"]`).remove(); + if ($dropzone.data('remove-url')) { POST($dropzone.data('remove-url'), { data: new URLSearchParams({file: file.uuid}),