From 83b01c2baae1687ccd4b80e712c708cafabad274 Mon Sep 17 00:00:00 2001 From: Julien Neuhart Date: Sat, 5 Sep 2026 09:59:29 +0200 Subject: [PATCH] fix(api): bound downloadFrom requests by the request deadline --- pkg/gotenberg/outbound.go | 30 +++++++++ pkg/gotenberg/outbound_test.go | 57 ++++++++++++++++ pkg/modules/api/context.go | 27 ++++++-- pkg/modules/api/context_test.go | 114 ++++++++++++++++++++++++++++++++ 4 files changed, 224 insertions(+), 4 deletions(-) diff --git a/pkg/gotenberg/outbound.go b/pkg/gotenberg/outbound.go index ffac7b22..8f8643fb 100644 --- a/pkg/gotenberg/outbound.go +++ b/pkg/gotenberg/outbound.go @@ -16,6 +16,7 @@ import ( "time" "github.com/dlclark/regexp2" + "github.com/hashicorp/go-retryablehttp" "golang.org/x/net/http/httpproxy" ) @@ -450,6 +451,16 @@ func (rt *outboundRoundTripper) RoundTrip(req *http.Request) (*http.Response, er // gate this behind their module's opt-in flag. See // https://github.com/gotenberg/gotenberg/issues/1592. func NewOutboundHttpClient(timeout time.Duration, allowList, denyList []*regexp2.Regexp, enableEnvironmentProxy bool, opts ...DecideOption) *http.Client { + // A negative timeout means the caller's budget is already spent, which + // happens when it derives one from a deadline that has passed. [http.Client] + // treats any non-positive Timeout as no deadline at all, so passing it + // through would silently produce an unbounded client. Fail closed instead. + // Zero keeps meaning unbounded: callers that own the connection lifetime + // themselves pass it deliberately. + if timeout < 0 { + timeout = time.Nanosecond + } + base := http.DefaultTransport.(*http.Transport).Clone() var proxyFunc func(*url.URL) (*url.URL, error) @@ -493,6 +504,25 @@ func NewOutboundHttpClient(timeout time.Duration, allowList, denyList []*regexp2 } } +// ClampedBackoff is a [retryablehttp.Backoff] that honors max on every path. +// +// [retryablehttp.DefaultBackoff] returns a Retry-After header from the remote +// verbatim for 429 and 503, and returns it before applying its own max clamp. +// A hostile origin therefore decides how long Gotenberg waits, and the wait is +// not interruptible. Retry-After is still respected here, just never beyond +// the ceiling the caller set. +func ClampedBackoff(min, max time.Duration, attemptNum int, resp *http.Response) time.Duration { + wait := retryablehttp.DefaultBackoff(min, max, attemptNum, resp) + if wait > max { + return max + } + if wait < 0 { + return 0 + } + + return wait +} + // environmentProxyVariables are the variables golang.org/x/net/http/httpproxy // reads, in the casing precedence it applies. var environmentProxyVariables = []string{ diff --git a/pkg/gotenberg/outbound_test.go b/pkg/gotenberg/outbound_test.go index ceec9aee..52791317 100644 --- a/pkg/gotenberg/outbound_test.go +++ b/pkg/gotenberg/outbound_test.go @@ -3,6 +3,7 @@ package gotenberg import ( "context" "errors" + "net/http" "net/netip" "strings" "testing" @@ -640,3 +641,59 @@ func TestDecideOutbound_LegitimateCredentialsStillReachTheHost(t *testing.T) { t.Fatalf("decision.Bypass = false, want true") } } + +func TestClampedBackoff(t *testing.T) { + const ( + min = 1 * time.Second + max = 30 * time.Second + ) + + retryAfter := func(status int, seconds string) *http.Response { + return &http.Response{StatusCode: status, Header: http.Header{"Retry-After": []string{seconds}}} + } + + for _, tc := range []struct { + scenario string + resp *http.Response + want time.Duration + }{ + {"429 with an hour is clamped", retryAfter(http.StatusTooManyRequests, "3600"), max}, + {"429 with a day is clamped", retryAfter(http.StatusTooManyRequests, "86400"), max}, + {"503 with an hour is clamped", retryAfter(http.StatusServiceUnavailable, "3600"), max}, + {"429 under the ceiling is honored", retryAfter(http.StatusTooManyRequests, "5"), 5 * time.Second}, + {"no response falls back to exponential", nil, min}, + } { + t.Run(tc.scenario, func(t *testing.T) { + got := ClampedBackoff(min, max, 0, tc.resp) + if got != tc.want { + t.Fatalf("ClampedBackoff = %s, want %s", got, tc.want) + } + if got > max { + t.Fatalf("ClampedBackoff = %s, which exceeds max %s", got, max) + } + }) + } +} + +// A negative max means the caller's budget is spent. The backoff must not +// return a negative duration, which would make the retry loop spin. +func TestClampedBackoff_NegativeMaxIsNotNegative(t *testing.T) { + got := ClampedBackoff(1*time.Second, -5*time.Second, 0, nil) + if got < 0 { + t.Fatalf("ClampedBackoff = %s, want a non-negative duration", got) + } +} + +func TestNewOutboundHttpClient_NonPositiveTimeout(t *testing.T) { + // Zero stays unbounded: the LibreOffice proxy owns its own lifetime and + // passes it deliberately. + if got := NewOutboundHttpClient(0, nil, nil, false).Timeout; got != 0 { + t.Fatalf("timeout for 0 = %s, want 0", got) + } + + // Negative means an expired budget. http.Client reads any non-positive + // Timeout as no deadline at all, so it must not be passed through. + if got := NewOutboundHttpClient(-5*time.Second, nil, nil, false).Timeout; got <= 0 { + t.Fatalf("timeout for a negative budget = %s, want a positive value so the client fails closed", got) + } +} diff --git a/pkg/modules/api/context.go b/pkg/modules/api/context.go index 87a053ec..cc46832b 100644 --- a/pkg/modules/api/context.go +++ b/pkg/modules/api/context.go @@ -280,7 +280,12 @@ func newContext(echoCtx echo.Context, logger *slog.Logger, fs *gotenberg.FileSys logger.DebugContext(dlCtx, fmt.Sprintf("download file from '%s'", dl.Url)) - req, err := retryablehttp.NewRequest(http.MethodGet, dl.Url, nil) + // The request must carry dlCtx: retryablehttp.NewRequest builds + // on context.Background(), and its wait between attempts is a + // select on the request context, so a contextless request cannot + // be interrupted by --api-timeout (env API_TIMEOUT) or by the + // caller going away. + req, err := retryablehttp.NewRequestWithContext(dlCtx, http.MethodGet, dl.Url, nil) if err != nil { dlSpan.RecordError(err) dlSpan.SetStatus(codes.Error, err.Error()) @@ -303,14 +308,28 @@ func newContext(echoCtx echo.Context, logger *slog.Logger, fs *gotenberg.FileSys } } + // Entries are serialized by the concurrency limit above, so a + // late one can start after the deadline has already passed. + // Fail closed rather than derive a non-positive timeout, which + // [http.Client] reads as no deadline at all. + remaining := time.Until(deadline) + if remaining <= 0 { + dlSpan.RecordError(context.DeadlineExceeded) + dlSpan.SetStatus(codes.Error, context.DeadlineExceeded.Error()) + dlSpan.End() + return fmt.Errorf("download file from '%s': %w", dl.Url, context.DeadlineExceeded) + } + client := &retryablehttp.Client{ - HTTPClient: gotenberg.NewOutboundHttpClient(time.Until(deadline), downloadFromCfg.allowList, downloadFromCfg.denyList, downloadFromCfg.enableEnvironmentProxy, ipOpts...), + HTTPClient: gotenberg.NewOutboundHttpClient(remaining, downloadFromCfg.allowList, downloadFromCfg.denyList, downloadFromCfg.enableEnvironmentProxy, ipOpts...), RetryMax: downloadFromCfg.maxRetry, RetryWaitMin: time.Duration(1) * time.Second, - RetryWaitMax: time.Until(deadline), + RetryWaitMax: remaining, Logger: gotenberg.NewLeveledLogger(logger), CheckRetry: retryablehttp.DefaultRetryPolicy, - Backoff: retryablehttp.DefaultBackoff, + // Not DefaultBackoff: it hands a hostile origin control of + // the wait via Retry-After. + Backoff: gotenberg.ClampedBackoff, } resp, err := client.Do(req) diff --git a/pkg/modules/api/context_test.go b/pkg/modules/api/context_test.go index 308d5108..c205a69d 100644 --- a/pkg/modules/api/context_test.go +++ b/pkg/modules/api/context_test.go @@ -414,3 +414,117 @@ func TestContext_FileCount(t *testing.T) { t.Errorf("expected 3 files, got %d", got) } } + +// A hostile origin must not choose how long Gotenberg waits. +// [retryablehttp.DefaultBackoff] returns a Retry-After header verbatim for 429 +// and 503, and the wait between attempts is a select on the request context. +// Building the request without a context therefore pinned the goroutine, its +// connection, and its working directory for the attacker's chosen duration, +// well past --api-timeout (env API_TIMEOUT). +func TestNewContext_DownloadFromHostileRetryAfterIsBounded(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Retry-After", "3600") + w.WriteHeader(http.StatusTooManyRequests) + })) + defer server.Close() + + payload, err := json.Marshal([]downloadFrom{{Url: server.URL + "/file"}}) + if err != nil { + t.Fatalf("marshal downloadFrom payload: %v", err) + } + + body := new(bytes.Buffer) + writer := multipart.NewWriter(body) + err = writer.WriteField("downloadFrom", string(payload)) + if err != nil { + t.Fatalf("write downloadFrom field: %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)) + + const timeout = 500 * time.Millisecond + + start := time.Now() + _, cancel, err := newContext(echoCtx, logger, fs, timeout, 0, downloadFromConfig{maxRetry: 2}) + elapsed := time.Since(start) + if cancel != nil { + defer cancel() + } + + if err == nil { + t.Fatal("expected newContext to fail against an origin that only answers 429") + } + // Generous: the deadline is 500ms and Retry-After asks for an hour. Any + // value in seconds means the remote is still in control. + if elapsed > 10*time.Second { + t.Fatalf("newContext took %s with Retry-After 3600; --api-timeout must bound it", elapsed) + } +} + +// An entry that starts after the deadline has passed must fail closed. It used +// to derive a negative client timeout, which [http.Client] reads as no +// deadline at all, leaving the download unbounded. +func TestNewContext_DownloadFromExpiredBudgetFailsClosed(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + <-r.Context().Done() + })) + defer server.Close() + + // Two entries, serialized by the concurrency limit, so the second one + // starts once the first has burned the whole budget. + payload, err := json.Marshal([]downloadFrom{ + {Url: server.URL + "/first"}, + {Url: server.URL + "/second"}, + }) + if err != nil { + t.Fatalf("marshal downloadFrom payload: %v", err) + } + + body := new(bytes.Buffer) + writer := multipart.NewWriter(body) + err = writer.WriteField("downloadFrom", string(payload)) + if err != nil { + t.Fatalf("write downloadFrom field: %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)) + + done := make(chan error, 1) + go func() { + _, cancel, err := newContext(echoCtx, logger, fs, 400*time.Millisecond, 0, downloadFromConfig{ + maxRetry: 0, + maxConcurrency: 1, + }) + if cancel != nil { + cancel() + } + done <- err + }() + + select { + case err := <-done: + if err == nil { + t.Fatal("expected newContext to fail against a stalling origin") + } + case <-time.After(15 * time.Second): + t.Fatal("newContext never returned: an entry starting past the deadline built an unbounded client") + } +}