diff --git a/pkg/modules/api/context.go b/pkg/modules/api/context.go index 4f11bfdd..0b6ac15a 100644 --- a/pkg/modules/api/context.go +++ b/pkg/modules/api/context.go @@ -51,6 +51,13 @@ type Context struct { outputPaths []string cancelled bool + // outputFilename is the sanitized Gotenberg-Output-Filename header, + // snapshotted while the [echo.Context] is still live. Echo returns that + // context to a pool as soon as the handler returns, and an asynchronous + // conversion outlives it, so reading the header from the pooled store later + // yields whichever request happens to own it by then. + outputFilename string + logger *slog.Logger echoCtx echo.Context mkdirAll gotenberg.MkdirAll @@ -159,14 +166,18 @@ func newContext(echoCtx echo.Context, logger *slog.Logger, fs *gotenberg.FileSys return nil } + // Snapshot now, while echoCtx still belongs to this request. + outputFilename, _ := echoCtx.Get("outputFilename").(string) + ctx := &Context{ - outputPaths: make([]string, 0), - cancelled: false, - logger: logger, - echoCtx: echoCtx, - mkdirAll: new(gotenberg.OsMkdirAll), - pathRename: new(gotenberg.OsPathRename), - Context: processCtx, + outputPaths: make([]string, 0), + cancelled: false, + outputFilename: outputFilename, + logger: logger, + echoCtx: echoCtx, + mkdirAll: new(gotenberg.OsMkdirAll), + pathRename: new(gotenberg.OsPathRename), + Context: processCtx, } // A custom cancel function which removes the context's working directory @@ -754,12 +765,12 @@ func (ctx *Context) BuildOutputFile() (string, error) { // OutputFilename returns the filename based on the given output path or the // "Gotenberg-Output-Filename" header's value. func (ctx *Context) OutputFilename(outputPath string) string { - filename := ctx.echoCtx.Get("outputFilename").(string) - - if filename == "" { + if ctx.outputFilename == "" { return ctx.OriginalFilename(outputPath) } + filename := ctx.outputFilename + return fmt.Sprintf("%s%s", filename, filepath.Ext(outputPath)) } diff --git a/pkg/modules/api/context_test.go b/pkg/modules/api/context_test.go index 064a739c..de69c608 100644 --- a/pkg/modules/api/context_test.go +++ b/pkg/modules/api/context_test.go @@ -602,3 +602,70 @@ func TestDecodeDownloadFrom_StopsBeforeMaterializingTheArray(t *testing.T) { } t.Logf("allocated %d KiB decoding a %d-entry array with a limit of 1000", allocated>>10, entries) } + +// An asynchronous conversion outlives the [echo.Context]. Echo returns that +// context to a sync.Pool as soon as the handler returns, and +// outputFilenameMiddleware runs in srv.Pre on every request, including +// /health, so a later request overwrites the store. Reading the output +// filename from it after the fact returned another caller's value. +func TestContext_OutputFilename_SurvivesEchoContextRecycling(t *testing.T) { + body := new(bytes.Buffer) + writer := multipart.NewWriter(body) + 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()) + // What outputFilenameMiddleware does for this request. + echoCtx.Set("outputFilename", "victim") + + 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() + + // Echo recycles the context and another request claims the store. + echoCtx.Set("outputFilename", "attacker-controlled") + + if got := ctx.OutputFilename("/tmp/out.pdf"); got != "victim.pdf" { + t.Fatalf("OutputFilename = %q, want %q", got, "victim.pdf") + } +} + +// A recycled context has a nil store, so the previous unguarded type assertion +// could panic. The snapshot must tolerate an absent value. +func TestContext_OutputFilename_NoHeader(t *testing.T) { + body := new(bytes.Buffer) + writer := multipart.NewWriter(body) + 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()) + + // No Set call at all: the store holds nothing for "outputFilename". + 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 := ctx.OutputFilename("/tmp/out.pdf"); got != "out.pdf" { + t.Fatalf("OutputFilename = %q, want the original filename %q", got, "out.pdf") + } +}