From 70783a01c64cd65a984a2b9909a079a4bcc95138 Mon Sep 17 00:00:00 2001 From: Julien Neuhart Date: Wed, 16 Sep 2026 21:23:43 +0200 Subject: [PATCH] fix(gotenberg): retry pattern matches aborted by a process pause --- pkg/gotenberg/flags.go | 4 + pkg/gotenberg/outbound.go | 15 +++- pkg/gotenberg/outbound_test.go | 7 ++ pkg/gotenberg/pattern.go | 55 +++++++++++++ pkg/gotenberg/pattern_test.go | 99 ++++++++++++++++++++++++ pkg/modules/chromium/events.go | 2 +- pkg/modules/chromium/scopebudget_test.go | 15 ++-- 7 files changed, 187 insertions(+), 10 deletions(-) create mode 100644 pkg/gotenberg/pattern.go create mode 100644 pkg/gotenberg/pattern_test.go diff --git a/pkg/gotenberg/flags.go b/pkg/gotenberg/flags.go index d90b4651..96e7d100 100644 --- a/pkg/gotenberg/flags.go +++ b/pkg/gotenberg/flags.go @@ -222,6 +222,10 @@ func (f *ParsedFlags) MustDeprecatedHumanReadableBytes(deprecated string, newNam // built. Patterns compiled any other way keep regexp2's default of // math.MaxInt64, which it treats as no timeout at all, so a hand-built slice // must set this itself before reaching [DecideOutbound]. +// +// The ceiling is wall-clock. Match through [MatchPattern] rather than calling +// regexp2 directly: a match that never approaches the ceiling still aborts if +// the process loses the CPU at the wrong moment. const PatternMatchTimeout = 250 * time.Millisecond // MustRegexp returns the regular expression of a flag given by name. diff --git a/pkg/gotenberg/outbound.go b/pkg/gotenberg/outbound.go index 4e2a726c..8ff409ad 100644 --- a/pkg/gotenberg/outbound.go +++ b/pkg/gotenberg/outbound.go @@ -336,12 +336,16 @@ func DecideOutbound(ctx context.Context, rawURL string, allowList, denyList []*r allowMatched := false if len(allowList) > 0 { for _, pattern := range allowList { - ok, err := pattern.MatchString(normalized) + ok, err := MatchPattern(pattern, normalized) if err != nil { if time.Now().After(deadline) { return OutboundDecision{}, context.DeadlineExceeded } - return OutboundDecision{}, fmt.Errorf("'%s' cannot handle '%s': %w", pattern.String(), normalized, err) + // The pattern could not be evaluated, so the URL cannot be + // cleared for the IP-check bypass an allow-list match grants. + // Fail closed like an unresolvable host does below, so the + // client gets a generic 403 rather than a 500. + return OutboundDecision{}, fmt.Errorf("'%s' cannot handle '%s': %v: %w", pattern.String(), normalized, err, ErrFiltered) } if ok { @@ -356,12 +360,15 @@ func DecideOutbound(ctx context.Context, rawURL string, allowList, denyList []*r } for _, pattern := range denyList { - ok, err := pattern.MatchString(normalized) + ok, err := MatchPattern(pattern, normalized) if err != nil { if time.Now().After(deadline) { return OutboundDecision{}, context.DeadlineExceeded } - return OutboundDecision{}, fmt.Errorf("'%s' cannot handle '%s': %w", pattern.String(), normalized, err) + // The pattern could not be evaluated, so the URL cannot be proven + // to fall outside the deny-list. Fail closed rather than letting a + // deny-list that never ran pass the request through. + return OutboundDecision{}, fmt.Errorf("'%s' cannot handle '%s': %v: %w", pattern.String(), normalized, err, ErrFiltered) } if ok { diff --git a/pkg/gotenberg/outbound_test.go b/pkg/gotenberg/outbound_test.go index 733b2280..5b79d292 100644 --- a/pkg/gotenberg/outbound_test.go +++ b/pkg/gotenberg/outbound_test.go @@ -793,6 +793,13 @@ func TestDecideOutboundBoundsCatastrophicPatterns(t *testing.T) { t.Fatal("expected an error from a catastrophic deny-list pattern") } + // A deny-list that could not be evaluated cannot clear the URL, so the + // decision fails closed and the client gets a generic 403 rather than a + // 500 naming the pattern. + if !errors.Is(err, ErrFiltered) { + t.Fatalf("expected ErrFiltered from an unevaluable deny-list pattern but got: %v", err) + } + // Generous headroom over the 250ms ceiling, still far below the 30s // deadline the match would otherwise have been allowed to consume. if elapsed > 5*time.Second { diff --git a/pkg/gotenberg/pattern.go b/pkg/gotenberg/pattern.go new file mode 100644 index 00000000..a54de874 --- /dev/null +++ b/pkg/gotenberg/pattern.go @@ -0,0 +1,55 @@ +package gotenberg + +import ( + "github.com/dlclark/regexp2" +) + +// patternMatchAttempts caps how many times [MatchPattern] runs one pattern +// against one string. It is what keeps a pattern that is genuinely out of +// budget from retrying forever: three attempts bound its cost at three +// [PatternMatchTimeout], which is still two orders of magnitude below the +// --api-timeout (env API_TIMEOUT) the ceiling exists to protect. +const patternMatchAttempts = 3 + +// MatchPattern reports whether s matches pattern. It bounds the match by the +// pattern's MatchTimeout without the false timeouts that the bound alone +// produces. +// +// regexp2 does not time a match against [time.Now]. It derives the deadline +// from a process-global clock that a background goroutine advances every +// 100ms, and it tests that deadline on the very first step of the match. +// Anything that stops the whole process, a cgroup CPU-quota throttle or a long +// stop-the-world pause, also stops that goroutine, which then advances the +// clock by the full pause in a single write. A match holding a deadline from +// before that jump aborts whatever work it had done: a 366ns match against a +// short URL reports "match timeout after 250ms". The abort lands on whichever +// match straddles the jump rather than on a match that was slow, which is why +// it fires on an idle instance and against Gotenberg's own file:///tmp/ URLs. +// See https://github.com/gotenberg/gotenberg/issues/1659. +// +// Retrying separates the two cases. Catastrophic backtracking is +// deterministic: the same pattern against the same string exhausts the same +// budget on every attempt, so a genuine runaway still aborts, and costs at +// most patternMatchAttempts ceilings to prove it. A clock-induced abort needs +// the process to lose the CPU inside one specific match, which the next +// attempt does not reproduce. +// +// Elapsed time cannot make that call instead. A match frozen mid-flight +// reports the freeze as its own cost, 806ms against a 250ms ceiling in one +// measured run, so it is indistinguishable by wall clock from a match that +// really did spend its budget. Go exposes no per-goroutine CPU time, and +// process CPU time counts every other request in flight. +func MatchPattern(pattern *regexp2.Regexp, s string) (bool, error) { + var err error + + for range patternMatchAttempts { + var ok bool + + ok, err = pattern.MatchString(s) + if err == nil { + return ok, nil + } + } + + return false, err +} diff --git a/pkg/gotenberg/pattern_test.go b/pkg/gotenberg/pattern_test.go new file mode 100644 index 00000000..f7b61f1e --- /dev/null +++ b/pkg/gotenberg/pattern_test.go @@ -0,0 +1,99 @@ +package gotenberg + +import ( + "strings" + "testing" + "time" + + "github.com/dlclark/regexp2" +) + +// mustPattern compiles a pattern the way the production lists are built. +func mustPattern(t *testing.T, expr string) *regexp2.Regexp { + t.Helper() + + re := regexp2.MustCompile(expr, 0) + re.MatchTimeout = PatternMatchTimeout + + return re +} + +func TestMatchPattern(t *testing.T) { + // The abort [MatchPattern] absorbs cannot be staged here: it needs the + // whole process to lose the CPU around one specific match, which no test + // can schedule. What is testable is the other half of the contract, that + // retrying never turns a genuine runaway into a pass. + // See https://github.com/gotenberg/gotenberg/issues/1659. + for _, tc := range []struct { + scenario string + pattern *regexp2.Regexp + s string + expectMatch bool + expectError bool + }{ + { + scenario: "deny-list match", + pattern: mustPattern(t, `^file:(?!//\/tmp/).*`), + s: "file:///etc/passwd", + expectMatch: true, + }, + { + scenario: "no match against Gotenberg's own working directory", + pattern: mustPattern(t, `^file:(?!//\/tmp/).*`), + s: "file:///tmp/1a2b3c4d/5e6f7a8b/9c0d1e2f.html", + expectMatch: false, + }, + { + scenario: "no match", + pattern: mustPattern(t, `^https://example\.com/`), + s: "https://example.org/", + expectMatch: false, + }, + { + scenario: "catastrophic backtracking still aborts", + pattern: mustPattern(t, `^https://example\.com/(a+)+$`), + s: "https://example.com/" + strings.Repeat("a", 40) + "!", + expectError: true, + }, + } { + t.Run(tc.scenario, func(t *testing.T) { + ok, err := MatchPattern(tc.pattern, tc.s) + + if tc.expectError && err == nil { + t.Fatal("expected an error but got none") + } + + if !tc.expectError && err != nil { + t.Fatalf("expected no error but got: %v", err) + } + + if ok != tc.expectMatch { + t.Fatalf("expected match %t but got %t", tc.expectMatch, ok) + } + }) + } +} + +func TestMatchPatternBoundsCatastrophicPattern(t *testing.T) { + // Proving a runaway is genuine costs one ceiling per attempt, so the + // worst case is patternMatchAttempts of them plus regexp2's clock period + // on each, roughly a second. The bound that matters is the one this + // replaced: before the ceiling existed, the same match was allowed to + // burn a core for the caller's whole 30s budget. + pattern := mustPattern(t, `^https://example\.com/(a+)+$`) + s := "https://example.com/" + strings.Repeat("a", 40) + "!" + + start := time.Now() + _, err := MatchPattern(pattern, s) + elapsed := time.Since(start) + + if err == nil { + t.Fatal("expected an error from a catastrophic pattern") + } + + // Generous headroom over the expected second keeps this stable on a + // loaded CI box while still failing if the bound is gone. + if elapsed > 5*time.Second { + t.Fatalf("match took %s, want at most %d ceilings of %s", elapsed, patternMatchAttempts, PatternMatchTimeout) + } +} diff --git a/pkg/modules/chromium/events.go b/pkg/modules/chromium/events.go index 52e7b2a2..1deb7722 100644 --- a/pkg/modules/chromium/events.go +++ b/pkg/modules/chromium/events.go @@ -206,7 +206,7 @@ func listenForEventRequestPaused(ctx context.Context, logger *slog.Logger, optio } matchStart := time.Now() - ok, err := header.Scope.MatchString(e.Request.URL) + ok, err := gotenberg.MatchPattern(header.Scope, e.Request.URL) budget.consume(time.Since(matchStart)) switch { diff --git a/pkg/modules/chromium/scopebudget_test.go b/pkg/modules/chromium/scopebudget_test.go index bb56e9ea..774dfb14 100644 --- a/pkg/modules/chromium/scopebudget_test.go +++ b/pkg/modules/chromium/scopebudget_test.go @@ -7,6 +7,8 @@ import ( "time" "github.com/dlclark/regexp2" + + "github.com/gotenberg/gotenberg/v8/pkg/gotenberg" ) func TestScopeMatchBudget(t *testing.T) { @@ -90,7 +92,7 @@ func TestScopeMatchBudget_BoundsCatastrophicBacktracking(t *testing.T) { break } matchStart := time.Now() - _, _ = pattern.MatchString(url) + _, _ = gotenberg.MatchPattern(pattern, url) b.consume(time.Since(matchStart)) matched++ } @@ -100,10 +102,13 @@ func TestScopeMatchBudget_BoundsCatastrophicBacktracking(t *testing.T) { t.Errorf("all %d headers were matched, want the budget to stop matching early", headers) } - // Each match is separately capped at extraHttpHeaderScopeMatchTimeout, so - // the worst case is the budget plus one final match that started with the - // last of the credit. Generous slack keeps this stable on a loaded CI box. - ceiling := budget + extraHttpHeaderScopeMatchTimeout + time.Second + // A match that is genuinely out of budget costs a few + // extraHttpHeaderScopeMatchTimeout rather than one: + // [gotenberg.MatchPattern] retries an abort to tell a real runaway from + // one caused by the process losing the CPU. The worst case is the budget + // plus one final match that started with the last of the credit. Generous + // slack keeps this stable on a loaded CI box. + ceiling := budget + 4*extraHttpHeaderScopeMatchTimeout + time.Second if elapsed > ceiling { t.Errorf("matching took %s, want at most %s", elapsed, ceiling) }