From 17868b8c022bb8f26b9217e115845ad0b5970ee9 Mon Sep 17 00:00:00 2001 From: Julien Neuhart Date: Sat, 5 Sep 2026 10:07:47 +0200 Subject: [PATCH] fix(api): stop upload filenames from failing or silently dropping a request --- pkg/modules/api/context.go | 90 ++++++++++++++---- pkg/modules/api/context_test.go | 163 ++++++++++++++++++++++++++++++++ 2 files changed, 232 insertions(+), 21 deletions(-) diff --git a/pkg/modules/api/context.go b/pkg/modules/api/context.go index 0b6ac15a..6792e171 100644 --- a/pkg/modules/api/context.go +++ b/pkg/modules/api/context.go @@ -458,7 +458,7 @@ func newContext(echoCtx echo.Context, logger *slog.Logger, fs *gotenberg.FileSys // Use a UUID-based name on disk to avoid filesystem // NAME_MAX limits with long filenames. // See: https://github.com/gotenberg/gotenberg/issues/1500. - safeName := uuid.New().String() + filepath.Ext(filename) + safeName := uuid.New().String() + safeExt(filename) path := fmt.Sprintf("%s/%s", ctx.dirPath, safeName) out, err := os.Create(path) @@ -512,18 +512,19 @@ func newContext(echoCtx echo.Context, logger *slog.Logger, fs *gotenberg.FileSys } for _, r := range results { - ctx.files[r.filename] = r.path - ctx.diskToOriginal[r.path] = r.filename + filename := ctx.uniqueFilename(r.filename) + ctx.files[filename] = r.path + ctx.diskToOriginal[r.path] = filename if r.formField != "" { ctx.filesByField[r.formField] = append(ctx.filesByField[r.formField], r.path) } } } - copyToDisk := func(fh *multipart.FileHeader) error { + copyToDisk := func(fh *multipart.FileHeader) (string, error) { in, err := fh.Open() if err != nil { - return fmt.Errorf("open multipart file: %w", err) + return "", fmt.Errorf("open multipart file: %w", err) } defer func() { @@ -546,12 +547,12 @@ func newContext(echoCtx echo.Context, logger *slog.Logger, fs *gotenberg.FileSys // Use a UUID-based name on disk to avoid filesystem // NAME_MAX limits with long filenames. // See: https://github.com/gotenberg/gotenberg/issues/1500. - safeName := uuid.New().String() + filepath.Ext(filename) + safeName := uuid.New().String() + safeExt(filename) path := fmt.Sprintf("%s/%s", ctx.dirPath, safeName) out, err := os.Create(path) if err != nil { - return fmt.Errorf("create local file: %w", err) + return "", fmt.Errorf("create local file: %w", err) } defer func() { err := out.Close() @@ -562,26 +563,26 @@ func newContext(echoCtx echo.Context, logger *slog.Logger, fs *gotenberg.FileSys _, err = io.Copy(out, reader) if err != nil { - return fmt.Errorf("copy multipart file to local file: %w", err) + return "", fmt.Errorf("copy multipart file to local file: %w", err) } + filename = ctx.uniqueFilename(filename) ctx.files[filename] = path ctx.diskToOriginal[path] = filename - return nil + return filename, nil } // Then, copy the form files, if any. for fieldName, files := range form.File { for _, fh := range files { - err = copyToDisk(fh) - if err != nil { - return ctx, cancel, fmt.Errorf("copy to disk: %w", err) + filename, errCopy := copyToDisk(fh) + if errCopy != nil { + return ctx, cancel, fmt.Errorf("copy to disk: %w", errCopy) } - // Track files by field name - filename := sanitizeFilename(fh.Filename) - filePath := ctx.files[filename] - ctx.filesByField[fieldName] = append(ctx.filesByField[fieldName], filePath) + // Track files by field name, under the name copyToDisk actually + // stored, which may be a de-duplicated variant. + ctx.filesByField[fieldName] = append(ctx.filesByField[fieldName], ctx.files[filename]) } } @@ -596,9 +597,9 @@ func newContext(echoCtx echo.Context, logger *slog.Logger, fs *gotenberg.FileSys if symlinkPath == diskPath { continue } - err = os.Symlink(filepath.Base(diskPath), symlinkPath) - if err != nil { - logger.DebugContext(context.Background(), fmt.Sprintf("skip symlink for '%s': %s", originalName, err)) + errSymlink := os.Symlink(filepath.Base(diskPath), symlinkPath) + if errSymlink != nil { + logger.DebugContext(context.Background(), fmt.Sprintf("skip symlink for '%s': %s", originalName, errSymlink)) } } @@ -607,7 +608,10 @@ func newContext(echoCtx echo.Context, logger *slog.Logger, fs *gotenberg.FileSys ctx.Log().DebugContext(ctx, fmt.Sprintf("form files by field: %+v", ctx.filesByField)) ctx.Log().DebugContext(ctx, fmt.Sprintf("total bytes: %d", totalBytesRead.Load())) - return ctx, cancel, err + // Explicitly nil: the best-effort symlink loop above must not decide the + // outcome of the request. Its failure used to escape here as a bare 500, + // non-deterministically, because ctx.files iterates in random order. + return ctx, cancel, nil } // Request returns the [http.Request]. @@ -662,7 +666,7 @@ func (ctx *Context) GeneratePath(extension string) string { // limits but registers the given filename so that [Context.OriginalFilename] // can resolve it. It does not create a file. func (ctx *Context) GeneratePathFromFilename(filename string) string { - safeName := uuid.New().String() + filepath.Ext(filename) + safeName := uuid.New().String() + safeExt(filename) path := fmt.Sprintf("%s/%s", ctx.dirPath, safeName) ctx.diskToOriginal[path] = filename return path @@ -774,6 +778,50 @@ func (ctx *Context) OutputFilename(outputPath string) string { return fmt.Sprintf("%s%s", filename, filepath.Ext(outputPath)) } +// maxDiskExtLength bounds the extension copied onto a UUID-based disk name. +// The UUID stem is 36 characters, so a longer extension risks NAME_MAX, which +// is 255 on ext4 and overlayfs. The untruncated name is kept in +// [Context.diskToOriginal], which never reaches the filesystem. +const maxDiskExtLength = 32 + +// safeExt returns the extension to append to a UUID-based disk name. It drops +// an extension too long to be safe rather than let [os.Create] fail with +// ENAMETOOLONG, which surfaced to the caller as a bare 500. +func safeExt(filename string) string { + ext := filepath.Ext(filename) + if len(ext) > maxDiskExtLength { + return "" + } + + return ext +} + +// uniqueFilename returns filename, or a numbered variant of it when the +// request already carries a file by that name. +// +// Uploads are keyed by their sanitized original filename, so two files sharing +// one name used to collide: the second overwrote the first and only one +// reached the conversion, while both stayed on disk and counted against the +// body limit. Sanitizing strips directories, so "a/doc.pdf" and "b/doc.pdf" +// collide too. +func (ctx *Context) uniqueFilename(filename string) string { + _, exists := ctx.files[filename] + if !exists { + return filename + } + + ext := filepath.Ext(filename) + stem := strings.TrimSuffix(filename, ext) + + for i := 2; ; i++ { + candidate := fmt.Sprintf("%s (%d)%s", stem, i, ext) + _, exists = ctx.files[candidate] + if !exists { + return candidate + } + } +} + // sanitizeFilename strips path separators (including backslashes, which // [filepath.Base] ignores on Linux) and control characters from a // caller-supplied filename, then NFC-normalizes the result. This prevents a diff --git a/pkg/modules/api/context_test.go b/pkg/modules/api/context_test.go index de69c608..ed7f5691 100644 --- a/pkg/modules/api/context_test.go +++ b/pkg/modules/api/context_test.go @@ -669,3 +669,166 @@ func TestContext_OutputFilename_NoHeader(t *testing.T) { t.Fatalf("OutputFilename = %q, want the original filename %q", got, "out.pdf") } } + +func TestSafeExt(t *testing.T) { + for _, tc := range []struct { + scenario string + filename string + want string + }{ + {"ordinary extension", "report.pdf", ".pdf"}, + {"no extension", "report", ""}, + {"at the limit", "a." + strings.Repeat("x", maxDiskExtLength-1), "." + strings.Repeat("x", maxDiskExtLength-1)}, + {"over the limit is dropped", "a." + strings.Repeat("x", 300), ""}, + } { + t.Run(tc.scenario, func(t *testing.T) { + got := safeExt(tc.filename) + if got != tc.want { + t.Fatalf("safeExt(%q) = %q, want %q", tc.filename, got, tc.want) + } + // A UUID stem is 36 characters. The whole disk name must stay + // under NAME_MAX. + if len(got)+36 > 255 { + t.Fatalf("disk name would be %d characters, over NAME_MAX", len(got)+36) + } + }) + } +} + +// An upload whose extension exceeds NAME_MAX used to fail os.Create and return +// a bare 500. The extension is bounded, and the original name survives in +// diskToOriginal. +func TestNewContext_LongExtensionIsAccepted(t *testing.T) { + filename := "invoice." + strings.Repeat("x", 300) + + body := new(bytes.Buffer) + writer := multipart.NewWriter(body) + part, err := writer.CreateFormFile("files", filename) + if err != nil { + t.Fatalf("create multipart file: %v", err) + } + _, err = part.Write([]byte("%PDF-1.4")) + if err != nil { + t.Fatalf("write multipart file: %v", err) + } + err = writer.Close() + if err != nil { + t.Fatalf("close multipart writer: %v", err) + } + + req := httptest.NewRequest(http.MethodPost, "/forms/libreoffice/convert", body) + req.Header.Set("Content-Type", writer.FormDataContentType()) + + echoCtx := echo.New().NewContext(req, httptest.NewRecorder()) + logger := slog.New(slog.DiscardHandler) + fs := gotenberg.NewFileSystem(new(gotenberg.OsMkdirAll)) + + ctx, cancel, err := newContext(echoCtx, logger, fs, 10*time.Second, 0, downloadFromConfig{disable: true}) + if cancel != nil { + defer cancel() + } + if err != nil { + t.Fatalf("newContext returned error for a long extension: %v", err) + } + if got := len(ctx.files); got != 1 { + t.Fatalf("files = %d, want 1", got) + } +} + +// A filename that cannot become a symlink (too long, "..", "/") must not fail +// the request. The symlink loop is best-effort, but its error escaped through +// the shared err variable, and ctx.files iterates randomly, so byte-identical +// requests gave different HTTP outcomes. +func TestNewContext_UnsymlinkableFilenameStillSucceeds(t *testing.T) { + for _, filename := range []string{ + strings.Repeat("a", 300) + ".txt", + "..", + "/", + } { + t.Run(filename[:min(len(filename), 12)], func(t *testing.T) { + body := new(bytes.Buffer) + writer := multipart.NewWriter(body) + part, err := writer.CreateFormFile("files", filename) + if err != nil { + t.Fatalf("create multipart file: %v", err) + } + _, err = part.Write([]byte("%PDF-1.4")) + if err != nil { + t.Fatalf("write multipart file: %v", err) + } + err = writer.Close() + if err != nil { + t.Fatalf("close multipart writer: %v", err) + } + + req := httptest.NewRequest(http.MethodPost, "/forms/libreoffice/convert", body) + req.Header.Set("Content-Type", writer.FormDataContentType()) + + echoCtx := echo.New().NewContext(req, httptest.NewRecorder()) + logger := slog.New(slog.DiscardHandler) + fs := gotenberg.NewFileSystem(new(gotenberg.OsMkdirAll)) + + _, cancel, err := newContext(echoCtx, logger, fs, 10*time.Second, 0, downloadFromConfig{disable: true}) + if cancel != nil { + defer cancel() + } + if err != nil { + t.Fatalf("newContext failed on a best-effort symlink for %q: %v", filename, err) + } + }) + } +} + +// Two uploads sharing a filename must both reach the conversion. The second +// used to overwrite the first in ctx.files, so one file was silently dropped +// while both stayed on disk and counted against the body limit. +func TestNewContext_DuplicateFilenamesAreBothKept(t *testing.T) { + body := new(bytes.Buffer) + writer := multipart.NewWriter(body) + for _, content := range []string{"FIRST", "SECOND"} { + part, err := writer.CreateFormFile("files", "doc.pdf") + if err != nil { + t.Fatalf("create multipart file: %v", err) + } + _, err = part.Write([]byte(content)) + if err != nil { + t.Fatalf("write multipart file: %v", err) + } + } + err := writer.Close() + if err != nil { + t.Fatalf("close multipart writer: %v", err) + } + + req := httptest.NewRequest(http.MethodPost, "/forms/pdfengines/merge", body) + req.Header.Set("Content-Type", writer.FormDataContentType()) + + echoCtx := echo.New().NewContext(req, httptest.NewRecorder()) + logger := slog.New(slog.DiscardHandler) + fs := gotenberg.NewFileSystem(new(gotenberg.OsMkdirAll)) + + ctx, cancel, err := newContext(echoCtx, logger, fs, 10*time.Second, 0, downloadFromConfig{disable: true}) + if err != nil { + t.Fatalf("newContext returned error: %v", err) + } + defer cancel() + + if got := len(ctx.files); got != 2 { + t.Fatalf("ctx.files = %d entries, want 2: a duplicate filename dropped a file", got) + } + if got := len(ctx.filesByField["files"]); got != 2 { + t.Fatalf("filesByField[files] = %d entries, want 2", got) + } + + // The two maps must agree, and both files must be distinct on disk. + seen := make(map[string]struct{}) + for _, path := range ctx.files { + if _, ok := ctx.diskToOriginal[path]; !ok { + t.Fatalf("path %q has no diskToOriginal entry", path) + } + seen[path] = struct{}{} + } + if len(seen) != 2 { + t.Fatalf("distinct disk paths = %d, want 2", len(seen)) + } +}