fix(api): bound downloadFrom requests by the request deadline

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{

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

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)

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