Mirrors/filebrowser
1
0
Fork 0
mirror of https://github.com/filebrowser/filebrowser.git synced 2026-07-29 02:30:29 +00:00
Code Issues Projects Releases Wiki Activity

fix: unblock stalled and frozen uploads

Browse source 
A TUS PATCH rejected before its body was read left the client still streaming
a chunk. net/http gives up draining an unread request body after 256KiB and
then closes the connection, so the status we replied with was lost and the
browser saw a bare transport error instead. The client could not tell a
conflict from a network fault, retried blindly, and the upload stalled after
the first chunk — while curl, which reads the response as it writes, still
reported the status and appeared to work. Drain what is left of a rejected
chunk so the answer arrives on a connection that stays usable. The explicit
r.Body.Close() had to go with it: it ran before the drain, and the server
closes the request body itself once the handler returns.

Pressing Upload awaited a recursive listing of the whole destination subtree
before a single byte was sent, with no sign that anything was happening. On a
large destination that reads as a dead button, and the walk kept running after
the client gave up, then reported the failed write as a 500. A flat upload can
only collide with a direct child of the destination, so walk recursively only
for folder uploads, stop the walk once the request is done, and show the action
as busy while the destination is checked.

Along the way: a failed upload was credited as fully sent, so the progress bar
reported 100% when nothing had been written; the progress timer was recreated
without clearing the previous one; and the chunk size field threw on a value
with no unit and raced its own debounce on submit, so it appeared to refuse
every change.

Fixes #6006
Refs #5987
This commit is contained in:
Henrique Dias 2026-07-27 16:27:19 +02:00
parent 9dd7c7d099
commit 0b925cbca9
No known key found for this signature in database
12 changed files with 715 additions and 156 deletions
Download patch file Download diff file Expand all files Collapse all files

5
frontend/src/components/prompts/Upload.vue
View file

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

34
frontend/src/stores/upload.ts
View file

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

134
frontend/src/utils/__tests__/check-conflict.test.ts
View file

@ -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/"
);

29
frontend/src/utils/upload.ts
View file

@ -30,9 +30,9 @@ type ServerConflictEntry = {
async function fetchConflictEntries(
basePath: string,
includeDirectories: boolean
recursive: boolean
): Promise<ServerConflictEntry[]> {
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<ConflictingResource[]> {
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 [];

10
frontend/src/views/files/FileListing.vue
View file

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

4
frontend/src/views/files/__tests__/upload-conflict-resolution.test.ts
View file

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

33
frontend/src/views/settings/Global.vue
View file

@ -262,6 +262,7 @@ const error = ref<StatusError | null>(null);
const originalSettings = ref<ISettings | null>(null);
const settings = ref<ISettings | null>(null);
const debounceTimeout = ref<number | null>(null);
const pendingChunkSize = ref<string | null>(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";
}

13
http/resource.go
View file

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

81
http/resource_recursive_test.go Normal file
View file

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

226
http/tus_handlers.go
View file

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

297
http/tus_multichunk_test.go Normal file
View file

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

5
http/utils.go
View file

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