diff --git a/frontend/src/components/prompts/Upload.vue b/frontend/src/components/prompts/Upload.vue index 689f6a6d..5417677c 100644 --- a/frontend/src/components/prompts/Upload.vue +++ b/frontend/src/components/prompts/Upload.vue @@ -38,6 +38,7 @@ import { useRoute } from "vue-router"; import { useLayoutStore } from "@/stores/layout"; import * as upload from "@/utils/upload"; +import buttons from "@/utils/buttons"; const { t } = useI18n(); const route = useRoute(); @@ -66,9 +67,13 @@ const uploadInput = async (event: Event) => { const path = route.path.endsWith("/") ? route.path : route.path + "/"; + // Checking the destination hits the server, so show it is working rather + // than leaving the action looking inert until the upload starts. + buttons.loading("upload"); const conflict = await upload.checkConflict(uploadFiles, path); if (conflict.length > 0) { + buttons.done("upload"); layoutStore.showHover({ prompt: "resolve-conflict", props: { diff --git a/frontend/src/stores/upload.ts b/frontend/src/stores/upload.ts index 53d96ea9..065679fd 100644 --- a/frontend/src/stores/upload.ts +++ b/frontend/src/stores/upload.ts @@ -103,15 +103,21 @@ export const useUploadStore = defineStore("upload", () => { } if (isActiveUploadsOnLimit() && hasPendingUploads()) { - if (!hasActiveUploads()) { - // Update the state in a fixed time interval + if (progressInterval === null) { + // Update the state in a fixed time interval. Guard on the handle, not + // on the active count: this runs again every time the queue drains to + // zero with work still pending, and reassigning would leak the timer. progressInterval = window.setInterval(syncState, 1000); } const upload = nextUpload(); + let succeeded = true; if (upload.type === "dir") { - await api.post(upload.path).catch($showError); + await api.post(upload.path).catch((err) => { + succeeded = false; + $showError(err); + }); } else { const onUpload = (event: ProgressEvent) => { upload.rawProgress.sentBytes = event.loaded; @@ -119,10 +125,13 @@ export const useUploadStore = defineStore("upload", () => { await api .post(upload.path, upload.file!, upload.overwrite, onUpload) - .catch((err) => err.message !== "Upload aborted" && $showError(err)); + .catch((err) => { + succeeded = false; + if (err.message !== "Upload aborted") $showError(err); + }); } - finishUpload(upload); + finishUpload(upload, succeeded); } }; @@ -135,9 +144,18 @@ export const useUploadStore = defineStore("upload", () => { return upload; }; - const finishUpload = (upload: Upload) => { - sentBytes.value += upload.totalBytes - upload.sentBytes; - upload.sentBytes = upload.totalBytes; + const finishUpload = (upload: Upload, succeeded: boolean) => { + if (succeeded) { + sentBytes.value += upload.totalBytes - upload.sentBytes; + upload.sentBytes = upload.totalBytes; + } else { + // Credit only what actually reached the server and drop the rest from the + // total. Counting a failed upload as fully sent makes the bar report 100% + // while nothing was written, which reads as success. + sentBytes.value += upload.rawProgress.sentBytes - upload.sentBytes; + totalBytes.value -= upload.totalBytes - upload.rawProgress.sentBytes; + upload.sentBytes = upload.rawProgress.sentBytes; + } upload.file = null; activeUploads.value.delete(upload); diff --git a/frontend/src/utils/__tests__/check-conflict.test.ts b/frontend/src/utils/__tests__/check-conflict.test.ts index 75f31ef2..fb02494d 100644 --- a/frontend/src/utils/__tests__/check-conflict.test.ts +++ b/frontend/src/utils/__tests__/check-conflict.test.ts @@ -20,6 +20,24 @@ vi.mock("@/stores/layout", () => ({ useLayoutStore: vi.fn() })); vi.mock("@/stores/upload", () => ({ useUploadStore: vi.fn() })); vi.mock("@/utils/url", () => ({ default: {} })); +type ServerEntry = { + path: string; + name: string; + size: number; + modified: string; + isDir: boolean; +}; + +// The destination's direct listing, used for copy/move and for flat uploads. +function mockListing(entries: ServerEntry[]) { + vi.mocked(api.fetch).mockResolvedValue({ items: entries } as Resource); +} + +// The recursive walk of the destination, used only for nested (folder) uploads. +function mockRecursiveListing(entries: ServerEntry[]) { + vi.mocked(api.fetchAll).mockResolvedValue(entries); +} + // A move/copy/drag item carries name (raw) and to (URL-encoded) but no // fullPath - mirroring what Move.vue / Copy.vue / ListingItem.vue build. function moveItem(name: string, dest: string, size = 12) { @@ -40,7 +58,7 @@ describe("checkConflict", () => { }); it("detects a conflict for a plain filename", async () => { - vi.mocked(api.fetchAll).mockResolvedValue([ + mockListing([ { path: "/target/file.txt", name: "file.txt", @@ -59,6 +77,43 @@ describe("checkConflict", () => { expect(conflicts[0].name).toBe("/target/file.txt"); }); + // Regression for #6006: pressing Upload awaited a full server-side recursive + // walk of the destination before a single byte was sent, so the UI sat there + // doing nothing on a large destination. A flat upload can only collide with a + // direct child, which the plain listing already covers. + it("does not walk the destination tree for a flat upload", async () => { + mockListing([]); + + await checkConflict( + [{ name: "file.txt", size: 12, isDir: false }], + "/files/target/" + ); + + expect(api.fetch).toHaveBeenCalledWith("/files/target/"); + expect(api.fetchAll).not.toHaveBeenCalled(); + }); + + // ...but a folder upload lands entries below the destination, so it still + // needs the recursive listing to see them. + it("walks the destination tree for a nested upload", async () => { + mockRecursiveListing([]); + + await checkConflict( + [ + { + name: "file.txt", + size: 12, + isDir: false, + fullPath: "folder/file.txt", + }, + ], + "/files/target/" + ); + + expect(api.fetchAll).toHaveBeenCalledWith("/files/target/"); + expect(api.fetch).not.toHaveBeenCalled(); + }); + // Regression for #5957: names with encodable characters (spaces, "#", // non-ASCII) were keyed by the URL-encoded `to` value and never matched the // server's raw path, so the conflict modal was skipped and the backend @@ -66,7 +121,7 @@ describe("checkConflict", () => { it.each(["my file.txt", "résumé.pdf", "a#b.txt"])( "detects a conflict for %s (encodable characters)", async (name) => { - vi.mocked(api.fetchAll).mockResolvedValue([ + mockListing([ { path: `/target/${name}`, name, @@ -87,7 +142,7 @@ describe("checkConflict", () => { ); it("reports no conflict when the destination has no matching name", async () => { - vi.mocked(api.fetchAll).mockResolvedValue([ + mockListing([ { path: "/target/other.txt", name: "other.txt", @@ -106,7 +161,7 @@ describe("checkConflict", () => { }); it("detects nested conflicts for folder uploads via fullPath", async () => { - vi.mocked(api.fetchAll).mockResolvedValue([ + mockRecursiveListing([ { path: "/target/folder", name: "folder", @@ -143,7 +198,7 @@ describe("checkConflict", () => { // fullPath) and no directory entries. Conflict detection must still find a // nested file even though its parent folder is not in the upload list. it("detects nested conflicts when no directory entries are present", async () => { - vi.mocked(api.fetchAll).mockResolvedValue([ + mockRecursiveListing([ { path: "/target/folder/deep/file.txt", name: "file.txt", @@ -171,24 +226,22 @@ describe("checkConflict", () => { // Copy/move only needs the target directory's direct children. A recursive // walk can make the UI look frozen on large destinations (regression #6005). it("checks only the direct destination listing for copy/move", async () => { - vi.mocked(api.fetch).mockResolvedValue({ - items: [ - { - path: "/target/file.txt", - name: "file.txt", - size: 10, - modified: "2026-06-04T00:00:00Z", - isDir: false, - }, - { - path: "/target/folder", - name: "folder", - size: 0, - modified: "2026-06-04T00:00:00Z", - isDir: true, - }, - ], - } as Resource); + mockListing([ + { + path: "/target/file.txt", + name: "file.txt", + size: 10, + modified: "2026-06-04T00:00:00Z", + isDir: false, + }, + { + path: "/target/folder", + name: "folder", + size: 0, + modified: "2026-06-04T00:00:00Z", + isDir: true, + }, + ]); const items = [ moveItem("file.txt", "/files/target/"), @@ -209,7 +262,7 @@ describe("checkConflict", () => { // Uploads merge into an existing folder, so the directory itself must not be // reported - only the files inside it can conflict. it("ignores a directory conflict for uploads (default)", async () => { - vi.mocked(api.fetchAll).mockResolvedValue([ + mockListing([ { path: "/target/folder", name: "folder", @@ -228,8 +281,8 @@ describe("checkConflict", () => { expect(conflicts).toHaveLength(0); }); - it("returns no conflicts when the recursive listing fails", async () => { - vi.mocked(api.fetchAll).mockRejectedValue(new Error("404")); + it("returns no conflicts when the destination listing fails", async () => { + vi.mocked(api.fetch).mockRejectedValue(new Error("404")); const conflicts = await checkConflict( [moveItem("file.txt", "/files/target/")], @@ -240,11 +293,11 @@ describe("checkConflict", () => { }); // Regression for #5980: a FileBrowser server running on Windows returns - // backslash-separated paths from the recursive listing. Without normalizing - // them, the prefix strip and key lookup never match, so the conflict modal is - // skipped and the backend returns a bare 409. + // backslash-separated paths, Without normalizing them, the prefix strip and + // key lookup never match, so the conflict modal is skipped and the backend + // returns a bare 409. it("detects a conflict for backslash-separated server paths (Windows)", async () => { - vi.mocked(api.fetchAll).mockResolvedValue([ + mockListing([ { path: "\\target\\file.txt", name: "file.txt", @@ -263,7 +316,7 @@ describe("checkConflict", () => { }); it("detects nested conflicts for backslash-separated server paths (Windows)", async () => { - vi.mocked(api.fetchAll).mockResolvedValue([ + mockRecursiveListing([ { path: "\\target\\folder\\nested file.txt", name: "nested file.txt", @@ -293,7 +346,7 @@ describe("checkConflict", () => { ])( "detects conflicts below an encoded destination path with %s", async (_label, serverPath) => { - vi.mocked(api.fetchAll).mockResolvedValue([ + mockListing([ { path: serverPath, name: "file.txt", @@ -314,7 +367,7 @@ describe("checkConflict", () => { ); it("detects nested folder-upload conflicts below an encoded destination path", async () => { - vi.mocked(api.fetchAll).mockResolvedValue([ + mockRecursiveListing([ { path: "\\target\\My SubDir\\folder\\nested file.txt", name: "nested file.txt", @@ -341,7 +394,7 @@ describe("checkConflict", () => { }); it("leaves malformed destination path segments unchanged", async () => { - vi.mocked(api.fetchAll).mockResolvedValue([ + mockListing([ { path: "/target/bad%ZZ/file.txt", name: "file.txt", @@ -360,7 +413,7 @@ describe("checkConflict", () => { }); it("does not match files outside the decoded destination path", async () => { - vi.mocked(api.fetchAll).mockResolvedValue([ + mockRecursiveListing([ { path: "/target/My SubDir/other.txt", name: "other.txt", @@ -369,7 +422,7 @@ describe("checkConflict", () => { isDir: false, }, { - path: "/target/My Other SubDir/file.txt", + path: "/target/My Other SubDir/folder/file.txt", name: "file.txt", size: 10, modified: "2026-06-04T00:00:00Z", @@ -378,7 +431,14 @@ describe("checkConflict", () => { ]); const conflicts = await checkConflict( - [moveItem("file.txt", "/files/target/My%20SubDir/")], + [ + { + name: "file.txt", + size: 12, + isDir: false, + fullPath: "folder/file.txt", + }, + ], "/files/target/My%20SubDir/" ); diff --git a/frontend/src/utils/upload.ts b/frontend/src/utils/upload.ts index d331f7f2..71a2d60b 100644 --- a/frontend/src/utils/upload.ts +++ b/frontend/src/utils/upload.ts @@ -30,9 +30,9 @@ type ServerConflictEntry = { async function fetchConflictEntries( basePath: string, - includeDirectories: boolean + recursive: boolean ): Promise { - if (!includeDirectories) { + if (recursive) { return await api.fetchAll(basePath); } @@ -40,6 +40,14 @@ async function fetchConflictEntries( return destination.items ?? []; } +/** + * Whether any entry lands below the destination rather than directly in it. + * Only then does conflict detection need the whole destination tree. + */ +function hasNestedEntries(files: UploadList): boolean { + return files.some((file) => conflictKey(file).includes("/")); +} + function conflictPath(entry: ServerConflictEntry): string { return entry.path.replace(/\\/g, "/"); } @@ -84,11 +92,14 @@ function buildConflictMap( * server, so the caller can prompt the user to overwrite/rename/skip. * * Directory handling differs by action, hence `includeDirectories`: - * - Upload (false): the destination tree is fetched recursively so nested - * file uploads can be checked; existing folders are silently merged. - * - Copy/move (true): only the destination directory itself is fetched. These - * operations move flat top-level selections, so a same-named direct child is - * the only preflight conflict the backend will reject. + * - Upload (false): existing folders are silently merged, so only files count. + * - Copy/move (true): these move flat top-level selections, so a same-named + * direct child is the only preflight conflict the backend will reject. + * + * The destination tree is only walked recursively when an entry actually lands + * below the destination (a folder upload). Walking it for a flat upload made + * pressing Upload wait on a full server-side walk of the whole subtree before + * a single byte was sent, which reads as a frozen UI on a large destination. * * @param files - flat upload list to check * @param basePath - server destination path (e.g. "/files/uploads/") @@ -101,9 +112,11 @@ export async function checkConflict( ): Promise { if (files.length === 0) return []; + const recursive = !includeDirectories && hasNestedEntries(files); + let serverEntries: ServerConflictEntry[]; try { - serverEntries = await fetchConflictEntries(basePath, includeDirectories); + serverEntries = await fetchConflictEntries(basePath, recursive); } catch { // The destination doesn't exist yet, so nothing can conflict. return []; diff --git a/frontend/src/views/files/FileListing.vue b/frontend/src/views/files/FileListing.vue index 77b41ae3..00901f06 100644 --- a/frontend/src/views/files/FileListing.vue +++ b/frontend/src/views/files/FileListing.vue @@ -348,6 +348,7 @@ import { useLayoutStore } from "@/stores/layout"; import { users, files as api } from "@/api"; import { enableExec } from "@/utils/constants"; import * as upload from "@/utils/upload"; +import buttons from "@/utils/buttons"; import css from "@/utils/css"; import { throttle } from "lodash-es"; import { Base64 } from "js-base64"; @@ -821,11 +822,15 @@ const drop = async (event: DragEvent) => { } } + // Checking the destination hits the server, so show it is working rather + // than leaving the action looking inert until the upload starts. + buttons.loading("upload"); const conflict = await upload.checkConflict(files, path); const preselect = removePrefix(path) + (files[0].fullPath || files[0].name); if (conflict.length > 0) { + buttons.done("upload"); layoutStore.showHover({ prompt: "resolve-conflict", props: { @@ -879,9 +884,14 @@ const uploadInput = async (event: Event) => { } const path = route.path.endsWith("/") ? route.path : route.path + "/"; + + // Checking the destination hits the server, so show it is working rather + // than leaving the action looking inert until the upload starts. + buttons.loading("upload"); const conflict = await upload.checkConflict(uploadFiles, path); if (conflict.length > 0) { + buttons.done("upload"); layoutStore.showHover({ prompt: "resolve-conflict", props: { diff --git a/frontend/src/views/files/__tests__/upload-conflict-resolution.test.ts b/frontend/src/views/files/__tests__/upload-conflict-resolution.test.ts index 3e1ede9c..40c88bb7 100644 --- a/frontend/src/views/files/__tests__/upload-conflict-resolution.test.ts +++ b/frontend/src/views/files/__tests__/upload-conflict-resolution.test.ts @@ -51,6 +51,10 @@ vi.mock("@/utils/upload", () => ({ handleFiles: harness.handleFiles, })); vi.mock("@/utils/css", () => ({ default: vi.fn() })); +// Reaches document.querySelector, which the node test environment lacks. +vi.mock("@/utils/buttons", () => ({ + default: { loading: vi.fn(), done: vi.fn(), success: vi.fn() }, +})); vi.mock("@/components/header/HeaderBar.vue", () => ({ default: {} })); vi.mock("@/components/header/Action.vue", () => ({ default: {} })); vi.mock("@/components/Search.vue", () => ({ default: {} })); diff --git a/frontend/src/views/settings/Global.vue b/frontend/src/views/settings/Global.vue index b638711e..a9b0f45d 100644 --- a/frontend/src/views/settings/Global.vue +++ b/frontend/src/views/settings/Global.vue @@ -262,6 +262,7 @@ const error = ref(null); const originalSettings = ref(null); const settings = ref(null); const debounceTimeout = ref(null); +const pendingChunkSize = ref(null); const commandObject = ref<{ [key: string]: string[] | string; @@ -289,13 +290,30 @@ const formattedChunkSize = computed({ clearTimeout(debounceTimeout.value); } + pendingChunkSize.value = value; + // Set a new timeout to apply the format after a short delay - debounceTimeout.value = window.setTimeout(() => { - if (settings.value) settings.value.tus.chunkSize = parseBytes(value); - }, 1500); + debounceTimeout.value = window.setTimeout(applyChunkSize, 1500); }, }); +// applyChunkSize commits what the user typed. Saving must flush it first: +// otherwise submitting within the debounce window persists the previous value, +// and the setting appears to refuse the change. +const applyChunkSize = () => { + if (debounceTimeout.value) { + clearTimeout(debounceTimeout.value); + debounceTimeout.value = null; + } + + if (pendingChunkSize.value === null) return; + + if (settings.value) { + settings.value.tus.chunkSize = parseBytes(pendingChunkSize.value); + } + pendingChunkSize.value = null; +}; + // Define funcs const capitalize = (name: string, where: string | RegExp = "_") => { if (where === "caps") where = /(?=[A-Z])/; @@ -311,6 +329,7 @@ const capitalize = (name: string, where: string | RegExp = "_") => { const save = async () => { if (settings.value === null) return false; + applyChunkSize(); const newSettings: ISettings = { ...settings.value, shell: @@ -362,8 +381,12 @@ const parseBytes = (input: string) => { const matches = input.match(regex); if (matches) { const size = parseFloat(matches[1].concat(matches[2] || "")); - let unit: keyof SettingsUnit = - matches[3].toUpperCase() as keyof SettingsUnit; + // The unit is optional: a bare number is already a count of bytes. Reading + // it unguarded throws, and the throw happens inside the debounce callback, + // so the setting is silently never updated. + let unit: keyof SettingsUnit = ( + matches[3] ?? "B" + ).toUpperCase() as keyof SettingsUnit; if (!unit.endsWith("B")) { unit += "B"; } diff --git a/http/resource.go b/http/resource.go index 8ad66cc3..f38b3923 100644 --- a/http/resource.go +++ b/http/resource.go @@ -412,6 +412,13 @@ var resourceGetRecursiveHandler = withUser(func(w http.ResponseWriter, r *http.R entries := make([]RecursiveEntry, 0) err = afero.Walk(d.user.Fs, rootPath, func(fPath string, info os.FileInfo, err error) error { + // Walking a large tree is expensive, and every entry is stat'ed through + // the scoped filesystem. Stop as soon as nobody is waiting for the + // answer instead of finishing the walk for a client that has gone. + if ctxErr := r.Context().Err(); ctxErr != nil { + return ctxErr + } + if err != nil { return nil // skip entries we cannot read } @@ -442,6 +449,12 @@ var resourceGetRecursiveHandler = withUser(func(w http.ResponseWriter, r *http.R return nil }) if err != nil { + // Once the request is done there is nobody left to answer, whatever the + // walk reported. Asking for a status here only produces a second header + // write on a connection that is already gone. + if r.Context().Err() != nil { + return 0, err + } return http.StatusInternalServerError, err } diff --git a/http/resource_recursive_test.go b/http/resource_recursive_test.go new file mode 100644 index 00000000..4de6dbe6 --- /dev/null +++ b/http/resource_recursive_test.go @@ -0,0 +1,81 @@ +package fbhttp + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + + "github.com/filebrowser/filebrowser/v2/settings" + "github.com/filebrowser/filebrowser/v2/users" +) + +func recursiveTestHandler(t *testing.T, userScope string) (http.Handler, string) { + t.Helper() + + key := []byte("test-signing-key") + perm := users.Permissions{Create: true, Modify: true, Download: true} + st := scopedUserStorage(t, userScope, perm, key) + + return handle(resourceGetRecursiveHandler, "/api/resources/recursive", st, &settings.Server{}), signToken(t, perm, key) +} + +func TestResourceRecursiveListsTree(t *testing.T) { + userScope := t.TempDir() + if err := os.MkdirAll(filepath.Join(userScope, "target", "nested"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(userScope, "target", "nested", "file.txt"), []byte("hi"), 0o600); err != nil { + t.Fatal(err) + } + + handler, token := recursiveTestHandler(t, userScope) + + req, _ := http.NewRequest(http.MethodGet, "/api/resources/recursive/target", http.NoBody) + req.Header.Set("X-Auth", token) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d body = %q", rec.Code, rec.Body.String()) + } + + var entries []RecursiveEntry + if err := json.Unmarshal(rec.Body.Bytes(), &entries); err != nil { + t.Fatalf("decode response: %v", err) + } + if len(entries) != 2 { + t.Fatalf("got %d entries, want 2: %+v", len(entries), entries) + } +} + +// Walking the tree stats every entry through the scoped filesystem, which is +// expensive on a large destination. When the client has already hung up there +// is nothing to answer, so the walk must stop instead of running to completion +// and then reporting a write failure as a 500. +func TestResourceRecursiveStopsWhenClientDisconnects(t *testing.T) { + userScope := t.TempDir() + if err := os.MkdirAll(filepath.Join(userScope, "target"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(userScope, "target", "file.txt"), []byte("hi"), 0o600); err != nil { + t.Fatal(err) + } + + handler, token := recursiveTestHandler(t, userScope) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + req, _ := http.NewRequestWithContext(ctx, http.MethodGet, "/api/resources/recursive/target", http.NoBody) + req.Header.Set("X-Auth", token) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + if body := rec.Body.String(); body != "" { + t.Fatalf("wrote a response for a disconnected client: %q", body) + } +} diff --git a/http/tus_handlers.go b/http/tus_handlers.go index b046fe61..f31853db 100644 --- a/http/tus_handlers.go +++ b/http/tus_handlers.go @@ -15,6 +15,24 @@ import ( "github.com/spf13/afero" ) +// maxPatchDrainBytes bounds how much of a rejected PATCH body is discarded to +// keep the connection reusable. It comfortably covers a default-sized chunk; +// beyond that the body is not worth reading just to throw away. +const maxPatchDrainBytes = 32 << 20 // 32MB + +// drainRequestBody discards what the client already put on the wire for a +// request the handler answered without reading. net/http only drains 256KiB on +// its own before giving up and closing the connection, and a connection closed +// while the client is still streaming a chunk reaches the browser as a transport +// error instead of the status we replied with. The client then cannot tell a +// conflict from a network fault, retries blindly, and the upload stalls. +func drainRequestBody(r *http.Request) { + if r.Body == nil { + return + } + _, _ = io.Copy(io.Discard, io.LimitReader(r.Body, maxPatchDrainBytes)) +} + // keepUploadActive periodically touches the cache entry to prevent eviction during transfer func keepUploadActive(cache UploadCache, filePath string) func() { stop := make(chan bool) @@ -160,107 +178,121 @@ func tusHeadHandler(cache UploadCache) handleFunc { func tusPatchHandler(cache UploadCache) handleFunc { return withUser(func(w http.ResponseWriter, r *http.Request, d *data) (int, error) { - if !d.user.Perm.Create || !d.Check(r.URL.Path) { - return http.StatusForbidden, nil + status, err := tusPatchUpload(w, r, d, cache) + // A rejected chunk is still a chunk the client is streaming: read what is + // left of it so the answer reaches the client on a connection that stays + // usable, instead of being lost to a reset. + if status >= 400 { + drainRequestBody(r) } - if r.Header.Get("Content-Type") != "application/offset+octet-stream" { - return http.StatusUnsupportedMediaType, nil - } - - uploadOffset, err := getUploadOffset(r) - if err != nil { - return http.StatusBadRequest, fmt.Errorf("invalid upload offset") - } - - file, err := files.NewFileInfo(&files.FileOptions{ - Fs: d.user.Fs, - Path: r.URL.Path, - Modify: d.user.Perm.Modify, - Expand: false, - ReadHeader: d.server.TypeDetectionByHeader, - Checker: d, - }) - - switch { - case errors.Is(err, afero.ErrFileNotFound): - return http.StatusNotFound, nil - case err != nil: - return errToStatus(err), err - } - - uploadLength, err := cache.GetLength(file.RealPath()) - if err != nil { - return http.StatusNotFound, err - } - - if uploadOffset > uploadLength { - return http.StatusBadRequest, fmt.Errorf("upload offset %d exceeds declared length %d", uploadOffset, uploadLength) - } - - // Prevent the upload from being evicted during the transfer - stop := keepUploadActive(cache, file.RealPath()) - defer stop() - - switch { - case file.IsDir: - return http.StatusBadRequest, fmt.Errorf("cannot upload to a directory %s", file.RealPath()) - case file.Size != uploadOffset: - return http.StatusConflict, fmt.Errorf( - "%s file size doesn't match the provided offset: %d", - file.RealPath(), - uploadOffset, - ) - } - - openFile, err := d.user.Fs.OpenFile(r.URL.Path, os.O_WRONLY|os.O_APPEND, d.settings.FileMode) - if err != nil { - return http.StatusInternalServerError, fmt.Errorf("could not open file: %w", err) - } - defer openFile.Close() - - _, err = openFile.Seek(uploadOffset, 0) - if err != nil { - return http.StatusInternalServerError, fmt.Errorf("could not seek file: %w", err) - } - - defer r.Body.Close() - // Enforce the declared Upload-Length: never write more than the bytes - // still expected for this upload. Reading one byte past the remainder - // lets an over-length body be detected and rejected. Without this bound a - // PATCH could stream arbitrary data to disk regardless of the length the - // client declared when the upload was created. - remaining := uploadLength - uploadOffset - bytesWritten, err := io.Copy(openFile, io.LimitReader(r.Body, remaining+1)) - if err != nil { - return http.StatusInternalServerError, fmt.Errorf("could not write to file: %w", err) - } - if bytesWritten > remaining { - // The client sent more than it declared; roll this chunk back so the - // file stays consistent with the tracked offset, and reject it. - if truncErr := openFile.Truncate(uploadOffset); truncErr != nil { - return http.StatusInternalServerError, fmt.Errorf("could not truncate file: %w", truncErr) - } - return http.StatusRequestEntityTooLarge, fmt.Errorf("upload exceeds declared length of %d bytes", uploadLength) - } - - // Sync the file to ensure all data is written to storage - // to prevent file corruption. - if err := openFile.Sync(); err != nil { - return http.StatusInternalServerError, fmt.Errorf("could not sync file: %w", err) - } - - newOffset := uploadOffset + bytesWritten - w.Header().Set("Upload-Offset", strconv.FormatInt(newOffset, 10)) - - if newOffset >= uploadLength { - cache.Complete(file.RealPath()) - _ = d.RunHook(func() error { return nil }, "upload", r.URL.Path, "", d.user) - } - - return http.StatusNoContent, nil + return status, err }) } +func tusPatchUpload(w http.ResponseWriter, r *http.Request, d *data, cache UploadCache) (int, error) { + if !d.user.Perm.Create || !d.Check(r.URL.Path) { + return http.StatusForbidden, nil + } + if r.Header.Get("Content-Type") != "application/offset+octet-stream" { + return http.StatusUnsupportedMediaType, nil + } + + uploadOffset, err := getUploadOffset(r) + if err != nil { + return http.StatusBadRequest, fmt.Errorf("invalid upload offset") + } + + file, err := files.NewFileInfo(&files.FileOptions{ + Fs: d.user.Fs, + Path: r.URL.Path, + Modify: d.user.Perm.Modify, + Expand: false, + ReadHeader: d.server.TypeDetectionByHeader, + Checker: d, + }) + + switch { + case errors.Is(err, afero.ErrFileNotFound): + return http.StatusNotFound, nil + case err != nil: + return errToStatus(err), err + } + + uploadLength, err := cache.GetLength(file.RealPath()) + if err != nil { + return http.StatusNotFound, err + } + + if uploadOffset > uploadLength { + return http.StatusBadRequest, fmt.Errorf("upload offset %d exceeds declared length %d", uploadOffset, uploadLength) + } + + // Prevent the upload from being evicted during the transfer + stop := keepUploadActive(cache, file.RealPath()) + defer stop() + + switch { + case file.IsDir: + return http.StatusBadRequest, fmt.Errorf("cannot upload to a directory %s", file.RealPath()) + case file.Size != uploadOffset: + return http.StatusConflict, fmt.Errorf( + "%s file size doesn't match the provided offset: %d", + file.RealPath(), + uploadOffset, + ) + } + + openFile, err := d.user.Fs.OpenFile(r.URL.Path, os.O_WRONLY|os.O_APPEND, d.settings.FileMode) + if err != nil { + return http.StatusInternalServerError, fmt.Errorf("could not open file: %w", err) + } + defer openFile.Close() + + _, err = openFile.Seek(uploadOffset, 0) + if err != nil { + return http.StatusInternalServerError, fmt.Errorf("could not seek file: %w", err) + } + + // The server closes the request body once the handler returns; closing it + // here would run before the caller gets to drain a rejected chunk, so every + // status raised below this point would still cost the connection. + // + // Enforce the declared Upload-Length: never write more than the bytes + // still expected for this upload. Reading one byte past the remainder + // lets an over-length body be detected and rejected. Without this bound a + // PATCH could stream arbitrary data to disk regardless of the length the + // client declared when the upload was created. + remaining := uploadLength - uploadOffset + bytesWritten, err := io.Copy(openFile, io.LimitReader(r.Body, remaining+1)) + if err != nil { + return http.StatusInternalServerError, fmt.Errorf("could not write to file: %w", err) + } + if bytesWritten > remaining { + // The client sent more than it declared; roll this chunk back so the + // file stays consistent with the tracked offset, and reject it. + if truncErr := openFile.Truncate(uploadOffset); truncErr != nil { + return http.StatusInternalServerError, fmt.Errorf("could not truncate file: %w", truncErr) + } + return http.StatusRequestEntityTooLarge, fmt.Errorf("upload exceeds declared length of %d bytes", uploadLength) + } + + // Sync the file to ensure all data is written to storage + // to prevent file corruption. + if err := openFile.Sync(); err != nil { + return http.StatusInternalServerError, fmt.Errorf("could not sync file: %w", err) + } + + newOffset := uploadOffset + bytesWritten + w.Header().Set("Upload-Offset", strconv.FormatInt(newOffset, 10)) + + if newOffset >= uploadLength { + cache.Complete(file.RealPath()) + _ = d.RunHook(func() error { return nil }, "upload", r.URL.Path, "", d.user) + } + + return http.StatusNoContent, nil +} + func tusDeleteHandler(cache UploadCache) handleFunc { return withUser(func(_ http.ResponseWriter, r *http.Request, d *data) (int, error) { if r.URL.Path == "/" || !d.user.Perm.Delete { diff --git a/http/tus_multichunk_test.go b/http/tus_multichunk_test.go new file mode 100644 index 00000000..17bba14a --- /dev/null +++ b/http/tus_multichunk_test.go @@ -0,0 +1,297 @@ +package fbhttp + +import ( + "bytes" + "io" + "net/http" + "net/http/httptest" + "net/http/httptrace" + "os" + "path/filepath" + "strconv" + "testing" + + "github.com/filebrowser/filebrowser/v2/settings" + "github.com/filebrowser/filebrowser/v2/storage" + "github.com/filebrowser/filebrowser/v2/users" +) + +// newTusTestServer mounts the TUS handlers behind a real HTTP server, under the +// same "/api/tus" prefix used in production, so tests exercise connection +// handling and header round-tripping instead of a bare ResponseRecorder. +func newTusTestServer(t *testing.T, st *storage.Storage, cache UploadCache) *httptest.Server { + t.Helper() + + server := &settings.Server{} + post := handle(tusPostHandler(cache), "/api/tus", st, server) + head := handle(tusHeadHandler(cache), "/api/tus", st, server) + patch := handle(tusPatchHandler(cache), "/api/tus", st, server) + + mux := http.NewServeMux() + mux.HandleFunc("/api/tus/", func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodPost: + post.ServeHTTP(w, r) + case http.MethodHead: + head.ServeHTTP(w, r) + case http.MethodPatch: + patch.ServeHTTP(w, r) + default: + w.WriteHeader(http.StatusMethodNotAllowed) + } + }) + + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + return srv +} + +// tusTestFixture wires a scoped user, an upload cache and a live server. +type tusTestFixture struct { + srv *httptest.Server + client *http.Client + token string + scope string +} + +func newTusTestFixture(t *testing.T) *tusTestFixture { + t.Helper() + + root := t.TempDir() + userScope := filepath.Join(root, "user") + if err := os.MkdirAll(userScope, 0o755); err != nil { + t.Fatal(err) + } + + key := []byte("test-signing-key") + perm := users.Permissions{Create: true, Modify: true} + st := scopedUserStorage(t, userScope, perm, key) + + cache := newMemoryUploadCache() + t.Cleanup(cache.Close) + + srv := newTusTestServer(t, st, cache) + return &tusTestFixture{ + srv: srv, + client: srv.Client(), + token: signToken(t, perm, key), + scope: userScope, + } +} + +// create sends the TUS creation request and returns the absolute upload URL. +func (f *tusTestFixture) create(t *testing.T, name string, length int) string { + t.Helper() + + req, err := http.NewRequest(http.MethodPost, f.srv.URL+"/api/tus/"+name+"?override=false", http.NoBody) + if err != nil { + t.Fatal(err) + } + req.Header.Set("X-Auth", f.token) + req.Header.Set("Upload-Length", strconv.Itoa(length)) + + res, err := f.client.Do(req) + if err != nil { + t.Fatalf("POST create: %v", err) + } + defer res.Body.Close() + body, _ := io.ReadAll(res.Body) + + if res.StatusCode != http.StatusCreated { + t.Fatalf("POST create: status %d body %q", res.StatusCode, body) + } + location := res.Header.Get("Location") + if location == "" { + t.Fatal("POST create: missing Location header") + } + return f.srv.URL + location +} + +// patch sends one chunk and reports the response plus whether the underlying +// connection was reused from the pool. +func (f *tusTestFixture) patch(t *testing.T, url string, offset int, chunk []byte) (*http.Response, bool) { + t.Helper() + + req, err := http.NewRequest(http.MethodPatch, url, bytes.NewReader(chunk)) + if err != nil { + t.Fatal(err) + } + req.Header.Set("X-Auth", f.token) + req.Header.Set("Content-Type", "application/offset+octet-stream") + req.Header.Set("Upload-Offset", strconv.Itoa(offset)) + + var reused bool + trace := &httptrace.ClientTrace{ + GotConn: func(info httptrace.GotConnInfo) { reused = info.Reused }, + } + req = req.WithContext(httptrace.WithClientTrace(req.Context(), trace)) + + res, err := f.client.Do(req) + if err != nil { + t.Fatalf("PATCH at offset %d: %v", offset, err) + } + return res, reused +} + +func testPayload(size int) []byte { + payload := make([]byte, size) + for i := range payload { + payload[i] = byte(i % 251) + } + return payload +} + +// A file larger than the chunk size must upload across several PATCH requests +// over a single reused connection, exactly as the web UI drives it. Each chunk +// has to be acknowledged with 204 and the new Upload-Offset, and the resulting +// file must be byte-identical to the source. +func TestTusMultiChunkUpload(t *testing.T) { + const chunkSize = 64 * 1024 + const fileSize = chunkSize*3 + 1234 + + f := newTusTestFixture(t) + payload := testPayload(fileSize) + uploadURL := f.create(t, "movie.bin", fileSize) + + for offset := 0; offset < fileSize; { + end := min(offset+chunkSize, fileSize) + + res, _ := f.patch(t, uploadURL, offset, payload[offset:end]) + body, _ := io.ReadAll(res.Body) + res.Body.Close() + + if res.StatusCode != http.StatusNoContent { + t.Fatalf("PATCH at offset %d: status %d body %q", offset, res.StatusCode, body) + } + if got, want := res.Header.Get("Upload-Offset"), strconv.Itoa(end); got != want { + t.Fatalf("PATCH at offset %d: Upload-Offset = %q, want %q", offset, got, want) + } + + offset = end + } + + got, err := os.ReadFile(filepath.Join(f.scope, "movie.bin")) + if err != nil { + t.Fatalf("read uploaded file: %v", err) + } + if !bytes.Equal(got, payload) { + t.Fatalf("uploaded file differs from source (got %d bytes, want %d)", len(got), len(payload)) + } +} + +// Between chunks a client may re-synchronise with a HEAD request. It must +// report the bytes already on disk plus the declared total, so the client can +// resume from the right offset. +func TestTusHeadReportsOffsetBetweenChunks(t *testing.T) { + const chunkSize = 32 * 1024 + const fileSize = chunkSize * 2 + + f := newTusTestFixture(t) + payload := testPayload(fileSize) + uploadURL := f.create(t, "resume.bin", fileSize) + + res, _ := f.patch(t, uploadURL, 0, payload[:chunkSize]) + res.Body.Close() + if res.StatusCode != http.StatusNoContent { + t.Fatalf("first PATCH: status %d", res.StatusCode) + } + + req, err := http.NewRequest(http.MethodHead, uploadURL, http.NoBody) + if err != nil { + t.Fatal(err) + } + req.Header.Set("X-Auth", f.token) + + headRes, err := f.client.Do(req) + if err != nil { + t.Fatalf("HEAD: %v", err) + } + defer headRes.Body.Close() + + if headRes.StatusCode != http.StatusOK { + t.Fatalf("HEAD: status %d", headRes.StatusCode) + } + if got, want := headRes.Header.Get("Upload-Offset"), strconv.Itoa(chunkSize); got != want { + t.Fatalf("HEAD Upload-Offset = %q, want %q", got, want) + } + if got, want := headRes.Header.Get("Upload-Length"), strconv.Itoa(fileSize); got != want { + t.Fatalf("HEAD Upload-Length = %q, want %q", got, want) + } +} + +// A PATCH that the handler rejects before reading the body must still leave the +// connection usable. Go abandons draining an unread request body after 256 KiB +// and then closes the socket, which turns a plain 409 into an opaque transport +// error for the client and stalls the upload. Rejecting a chunk-sized body must +// not cost the connection. +func TestTusPatchRejectionKeepsConnectionUsable(t *testing.T) { + const chunkSize = 1024 * 1024 + const fileSize = chunkSize * 2 + + f := newTusTestFixture(t) + payload := testPayload(fileSize) + uploadURL := f.create(t, "conflict.bin", fileSize) + + // Land the first chunk so the file size no longer matches offset 0. + res, _ := f.patch(t, uploadURL, 0, payload[:chunkSize]) + res.Body.Close() + if res.StatusCode != http.StatusNoContent { + t.Fatalf("first PATCH: status %d", res.StatusCode) + } + + // Replaying offset 0 with a full chunk must be answered with a real 409. + res, _ = f.patch(t, uploadURL, 0, payload[:chunkSize]) + body, _ := io.ReadAll(res.Body) + res.Body.Close() + if res.StatusCode != http.StatusConflict { + t.Fatalf("stale-offset PATCH: status %d body %q, want 409", res.StatusCode, body) + } + + // And the connection must survive it, so the client's next attempt is not + // reported as a network failure. + res, reused := f.patch(t, uploadURL, chunkSize, payload[chunkSize:]) + body, _ = io.ReadAll(res.Body) + res.Body.Close() + if res.StatusCode != http.StatusNoContent { + t.Fatalf("PATCH after rejection: status %d body %q", res.StatusCode, body) + } + if !reused { + t.Fatal("connection was dropped by the rejected PATCH instead of being reused") + } + + got, err := os.ReadFile(filepath.Join(f.scope, "conflict.bin")) + if err != nil { + t.Fatalf("read uploaded file: %v", err) + } + if !bytes.Equal(got, payload) { + t.Fatalf("uploaded file differs from source (got %d bytes, want %d)", len(got), len(payload)) + } +} + +// Same requirement for a rejection raised once the write is already underway: +// an over-length body is refused after part of it has been read, and the rest +// still has to be drained for the answer to survive. +func TestTusOverLengthPatchKeepsConnectionUsable(t *testing.T) { + const declared = 1024 + + f := newTusTestFixture(t) + payload := testPayload(4 << 20) + uploadURL := f.create(t, "toolong.bin", declared) + + res, _ := f.patch(t, uploadURL, 0, payload) + body, _ := io.ReadAll(res.Body) + res.Body.Close() + if res.StatusCode != http.StatusRequestEntityTooLarge { + t.Fatalf("over-length PATCH: status %d body %q, want 413", res.StatusCode, body) + } + + res, reused := f.patch(t, uploadURL, 0, payload[:declared]) + body, _ = io.ReadAll(res.Body) + res.Body.Close() + if res.StatusCode != http.StatusNoContent { + t.Fatalf("PATCH after rejection: status %d body %q", res.StatusCode, body) + } + if !reused { + t.Fatal("connection was dropped by the over-length PATCH instead of being reused") + } +} diff --git a/http/utils.go b/http/utils.go index 2d132a6b..4163941b 100644 --- a/http/utils.go +++ b/http/utils.go @@ -65,7 +65,10 @@ func renderJSON(w http.ResponseWriter, _ *http.Request, data interface{}) (int, w.Header().Set("Content-Type", "application/json; charset=utf-8") if _, err := w.Write(marsh); err != nil { - return http.StatusInternalServerError, err + // The status line is already on the wire, so there is no error status + // left to send: a failed write here means the client hung up. Report it + // for the log without asking the caller to write a second header. + return 0, err } return 0, nil