From dd76e79443643e8617bc6a935668d8f76c4b25f4 Mon Sep 17 00:00:00 2001 From: Julien Neuhart Date: Tue, 13 Oct 2026 18:13:44 +0200 Subject: [PATCH] fix(api): enforce api-body-limit before parsing multipart requests --- pkg/modules/api/context.go | 29 ++++++++-- pkg/modules/api/context_test.go | 98 +++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 4 deletions(-) diff --git a/pkg/modules/api/context.go b/pkg/modules/api/context.go index ae2a8b9b..91fc12a0 100644 --- a/pkg/modules/api/context.go +++ b/pkg/modules/api/context.go @@ -166,13 +166,17 @@ func newContext(echoCtx *echo.Context, logger *slog.Logger, fs *gotenberg.FileSy // downloaded remotely ("download from" feature). var totalBytesRead atomic.Int64 + bodyLimitErr := func() error { + return WrapError( + fmt.Errorf("body limit reached (> %d)", bodyLimit), + NewSentinelHttpError(http.StatusRequestEntityTooLarge, "The request body exceeds the configured size limit. Increase it with --api-body-limit, or send a smaller request."), + ) + } + addReadBytes := func(n int64) error { newTotal := totalBytesRead.Add(n) if bodyLimit != 0 && newTotal > bodyLimit { - return WrapError( - fmt.Errorf("body limit reached (> %d)", bodyLimit), - NewSentinelHttpError(http.StatusRequestEntityTooLarge, "The request body exceeds the configured size limit. Increase it with --api-body-limit, or send a smaller request."), - ) + return bodyLimitErr() } return nil } @@ -217,8 +221,25 @@ func newContext(echoCtx *echo.Context, logger *slog.Logger, fs *gotenberg.FileSy } }() + // The multipart parser reads the whole request and spools file parts to + // disk before the counter above sees a single byte, so the limit must + // also bound the raw body itself. Content-Length allows an early + // rejection, but it may be absent or lie, hence the reader. + if bodyLimit > 0 { + req := echoCtx.Request() + if req.ContentLength > bodyLimit { + return nil, cancel, fmt.Errorf("check content length: %w", bodyLimitErr()) + } + req.Body = http.MaxBytesReader(echoCtx.Response(), req.Body, bodyLimit) + } + form, err := echoCtx.MultipartForm() if err != nil { + var maxBytesErr *http.MaxBytesError + if errors.As(err, &maxBytesErr) { + return nil, cancel, fmt.Errorf("get multipart form: %w", bodyLimitErr()) + } + if errors.Is(err, http.ErrNotMultipart) { return nil, cancel, WrapError( fmt.Errorf("get multipart form: %w", err), diff --git a/pkg/modules/api/context_test.go b/pkg/modules/api/context_test.go index bce60b55..99808a67 100644 --- a/pkg/modules/api/context_test.go +++ b/pkg/modules/api/context_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "log/slog" "mime/multipart" "net/http" @@ -902,3 +903,100 @@ func TestNewContext_DownloadFromRedirectVerdictStaysGeneric(t *testing.T) { } } } + +type countingReadCloser struct { + r io.Reader + n int64 +} + +func (c *countingReadCloser) Read(p []byte) (int, error) { + n, err := c.r.Read(p) + c.n += int64(n) + return n, err +} + +func (*countingReadCloser) Close() error { return nil } + +// The body limit must stop the multipart parser itself. It used to apply +// only after the parser had read the whole request and spooled file parts +// to disk. +func TestNewContext_BodyLimitBoundsIngress(t *testing.T) { + body := new(bytes.Buffer) + writer := multipart.NewWriter(body) + err := writer.WriteField("foo", "bar") + if err != nil { + t.Fatalf("write multipart field: %v", err) + } + part, err := writer.CreateFormFile("files", "large.bin") + if err != nil { + t.Fatalf("create multipart file: %v", err) + } + _, err = part.Write(bytes.Repeat([]byte("A"), 2<<20)) + if err != nil { + t.Fatalf("write multipart file: %v", err) + } + err = writer.Close() + if err != nil { + t.Fatalf("close multipart writer: %v", err) + } + size := int64(body.Len()) + + for _, tc := range []struct { + scenario string + bodyLimit int64 + knownLength bool + expectErr bool + maxBytesConsumed int64 + }{ + {scenario: "known length over the limit is rejected before reading", bodyLimit: 1024, knownLength: true, expectErr: true, maxBytesConsumed: 0}, + {scenario: "unknown length over the limit stops at the limit", bodyLimit: 1024, knownLength: false, expectErr: true, maxBytesConsumed: 1024 + 1}, + {scenario: "unknown length one byte over the limit", bodyLimit: size - 1, knownLength: false, expectErr: true, maxBytesConsumed: size}, + {scenario: "body exactly at the limit", bodyLimit: size, knownLength: true, expectErr: false}, + {scenario: "body exactly at the limit with unknown length", bodyLimit: size, knownLength: false, expectErr: false}, + {scenario: "no limit", bodyLimit: 0, knownLength: false, expectErr: false}, + } { + t.Run(tc.scenario, func(t *testing.T) { + counted := &countingReadCloser{r: bytes.NewReader(body.Bytes())} + req := httptest.NewRequest(http.MethodPost, "/forms/pdfengines/merge", nil) + req.Body = counted + req.ContentLength = -1 + if tc.knownLength { + req.ContentLength = size + } + 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, tc.bodyLimit, downloadFromConfig{disable: true}) + if cancel != nil { + defer cancel() + } + + if !tc.expectErr { + if err != nil { + t.Fatalf("newContext returned error: %v", err) + } + if counted.n != size { + t.Fatalf("consumed %d bytes, want %d", counted.n, size) + } + return + } + + if err == nil { + t.Fatal("expected the body limit to reject the request") + } + var httpErr HttpError + if !errors.As(err, &httpErr) { + t.Fatalf("expected an HttpError, got: %v", err) + } + if status, _ := httpErr.HttpError(); status != http.StatusRequestEntityTooLarge { + t.Fatalf("status = %d, want %d", status, http.StatusRequestEntityTooLarge) + } + if counted.n > tc.maxBytesConsumed { + t.Fatalf("consumed %d/%d bytes before rejection, want at most %d", counted.n, size, tc.maxBytesConsumed) + } + }) + } +}