fix(api): stop upload filenames from failing or silently dropping a request

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
+			filename := ctx.uniqueFilename(r.filename)
-			ctx.diskToOriginal[r.path] = 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)
+			filename, errCopy := copyToDisk(fh)
-			if err != nil {
+			if errCopy != nil {
-				return ctx, cancel, fmt.Errorf("copy to disk: %w", err)
+				return ctx, cancel, fmt.Errorf("copy to disk: %w", errCopy)
 			}
-			// Track files by field name
+			// Track files by field name, under the name copyToDisk actually
-			filename := sanitizeFilename(fh.Filename)
+			// stored, which may be a de-duplicated variant.
-			filePath := ctx.files[filename]
+			ctx.filesByField[fieldName] = append(ctx.filesByField[fieldName], ctx.files[filename])
-			ctx.filesByField[fieldName] = append(ctx.filesByField[fieldName], filePath)
 		}
 	}
 
@@ -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)
+		errSymlink := os.Symlink(filepath.Base(diskPath), symlinkPath)
-		if err != nil {
+		if errSymlink != nil {
-			logger.DebugContext(context.Background(), fmt.Sprintf("skip symlink for '%s': %s", originalName, err))
+			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

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