From b39b8c76aab79a677e5893d6fcf3ba440dc43070 Mon Sep 17 00:00:00 2001 From: Julien Neuhart Date: Tue, 8 Sep 2026 18:34:58 +0200 Subject: [PATCH] perf(gotenberg): match allow and deny patterns without recompiling them --- pkg/gotenberg/filter.go | 70 -------------------- pkg/gotenberg/filter_test.go | 117 --------------------------------- pkg/gotenberg/flags.go | 33 +++++++++- pkg/gotenberg/flags_test.go | 33 ++++++++++ pkg/gotenberg/outbound.go | 34 ++++++---- pkg/gotenberg/outbound_test.go | 51 ++++++++++++++ 6 files changed, 136 insertions(+), 202 deletions(-) delete mode 100644 pkg/gotenberg/filter.go delete mode 100644 pkg/gotenberg/filter_test.go diff --git a/pkg/gotenberg/filter.go b/pkg/gotenberg/filter.go deleted file mode 100644 index 217d3303..00000000 --- a/pkg/gotenberg/filter.go +++ /dev/null @@ -1,70 +0,0 @@ -package gotenberg - -import ( - "context" - "errors" - "fmt" - "time" - - "github.com/dlclark/regexp2" -) - -// ErrFiltered happens if a value is filtered by the [FilterDeadline] function. -var ErrFiltered = errors.New("value filtered") - -// FilterDeadline checks if the given value is allowed and not denied according -// to regex patterns. The allowed list uses OR semantics (value must match at -// least one pattern). The denied list uses OR semantics (value is denied if it -// matches any pattern). It returns a [context.DeadlineExceeded] if it takes -// too long to process. -func FilterDeadline(allowed, denied []*regexp2.Regexp, s string, deadline time.Time) error { - if len(allowed) > 0 { - matched := false - - for _, pattern := range allowed { - // FIXME: not ideal to compile everytime, but is there another way to create a clone? - clone := regexp2.MustCompile(pattern.String(), 0) - clone.MatchTimeout = time.Until(deadline) - - ok, err := clone.MatchString(s) - if err != nil { - if time.Now().After(deadline) { - return context.DeadlineExceeded - } - - return fmt.Errorf("'%s' cannot handle '%s': %w", clone.String(), s, err) - } - - if ok { - matched = true - break - } - } - - if !matched { - return fmt.Errorf("'%s' does not match any expression from the allowed list: %w", s, ErrFiltered) - } - } - - if len(denied) > 0 { - for _, pattern := range denied { - clone := regexp2.MustCompile(pattern.String(), 0) - clone.MatchTimeout = time.Until(deadline) - - ok, err := clone.MatchString(s) - if err != nil { - if time.Now().After(deadline) { - return context.DeadlineExceeded - } - - return fmt.Errorf("'%s' cannot handle '%s': %w", clone.String(), s, err) - } - - if ok { - return fmt.Errorf("'%s' matches the expression from the denied list: %w", s, ErrFiltered) - } - } - } - - return nil -} diff --git a/pkg/gotenberg/filter_test.go b/pkg/gotenberg/filter_test.go deleted file mode 100644 index c8ef2965..00000000 --- a/pkg/gotenberg/filter_test.go +++ /dev/null @@ -1,117 +0,0 @@ -package gotenberg - -import ( - "context" - "errors" - "testing" - "time" - - "github.com/dlclark/regexp2" -) - -func TestFilterDeadline(t *testing.T) { - for _, tc := range []struct { - scenario string - allowed []*regexp2.Regexp - denied []*regexp2.Regexp - s string - deadline time.Time - expectError bool - expectedError error - }{ - { - scenario: "DeadlineExceeded (allowed)", - allowed: []*regexp2.Regexp{regexp2.MustCompile("foo", 0)}, - denied: nil, - s: "foo", - deadline: time.Now().Add(time.Duration(-1) * time.Hour), - expectError: true, - expectedError: context.DeadlineExceeded, - }, - { - scenario: "ErrFiltered (allowed, no match)", - allowed: []*regexp2.Regexp{regexp2.MustCompile("foo", 0)}, - denied: nil, - s: "bar", - deadline: time.Now().Add(time.Duration(5) * time.Second), - expectError: true, - expectedError: ErrFiltered, - }, - { - scenario: "DeadlineExceeded (denied)", - allowed: nil, - denied: []*regexp2.Regexp{regexp2.MustCompile("foo", 0)}, - s: "foo", - deadline: time.Now().Add(time.Duration(-1) * time.Hour), - expectError: true, - expectedError: context.DeadlineExceeded, - }, - { - scenario: "ErrFiltered (denied)", - allowed: nil, - denied: []*regexp2.Regexp{regexp2.MustCompile("foo", 0)}, - s: "foo", - deadline: time.Now().Add(time.Duration(5) * time.Second), - expectError: true, - expectedError: ErrFiltered, - }, - { - scenario: "success (empty lists)", - allowed: nil, - denied: nil, - s: "foo", - deadline: time.Now().Add(time.Duration(5) * time.Second), - expectError: false, - }, - { - scenario: "multi-pattern allow list, second matches", - allowed: []*regexp2.Regexp{regexp2.MustCompile("^https://", 0), regexp2.MustCompile("^file:///tmp/", 0)}, - denied: nil, - s: "file:///tmp/abc/index.html", - deadline: time.Now().Add(time.Duration(5) * time.Second), - expectError: false, - }, - { - scenario: "multi-pattern allow list, none matches", - allowed: []*regexp2.Regexp{regexp2.MustCompile("^https://", 0), regexp2.MustCompile("^ftp://", 0)}, - denied: nil, - s: "file:///tmp/abc/index.html", - deadline: time.Now().Add(time.Duration(5) * time.Second), - expectError: true, - expectedError: ErrFiltered, - }, - { - scenario: "multi-pattern deny list, second matches", - allowed: nil, - denied: []*regexp2.Regexp{regexp2.MustCompile("^ftp://", 0), regexp2.MustCompile("^file:.*", 0)}, - s: "file:///etc/passwd", - deadline: time.Now().Add(time.Duration(5) * time.Second), - expectError: true, - expectedError: ErrFiltered, - }, - { - scenario: "https URL passes deny list targeting file://", - allowed: nil, - denied: []*regexp2.Regexp{regexp2.MustCompile("^file:.*", 0)}, - s: "https://example.com", - deadline: time.Now().Add(time.Duration(5) * time.Second), - expectError: false, - }, - } { - t.Run(tc.scenario, func(t *testing.T) { - err := FilterDeadline(tc.allowed, tc.denied, tc.s, tc.deadline) - - 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 tc.expectedError != nil && !errors.Is(err, tc.expectedError) { - t.Fatalf("expected error %v but got: %v", tc.expectedError, err) - } - }) - } -} diff --git a/pkg/gotenberg/flags.go b/pkg/gotenberg/flags.go index dcd7d360..d90b4651 100644 --- a/pkg/gotenberg/flags.go +++ b/pkg/gotenberg/flags.go @@ -207,15 +207,38 @@ func (f *ParsedFlags) MustDeprecatedHumanReadableBytes(deprecated string, newNam return f.MustHumanReadableBytes(newName) } +// PatternMatchTimeout bounds a single match against an operator-supplied +// allow-list or deny-list pattern. +// +// regexp2 backtracks, and the strings matched against these patterns are +// client-controlled: a request URL, a CONNECT host. A pattern that backtracks +// catastrophically would otherwise burn a core for as long as the caller's +// deadline allows, which is --api-timeout (env API_TIMEOUT), 30 seconds by +// default. The ceiling mirrors the one the Chromium module already applies to +// the per-request extraHttpHeaders scope pattern. +// +// [ParsedFlags.MustRegexp] and [ParsedFlags.MustRegexpSlice] stamp this onto +// every pattern they compile, which is how all four production lists are +// 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]. +const PatternMatchTimeout = 250 * time.Millisecond + // MustRegexp returns the regular expression of a flag given by name. // It panics if an error occurs. +// +// The returned expression carries [PatternMatchTimeout] and is safe to match +// on concurrently: callers must not compile a private copy per match. func (f *ParsedFlags) MustRegexp(name string) *regexp2.Regexp { val, err := f.GetString(name) if err != nil { panic(err) } - return regexp2.MustCompile(val, 0) + re := regexp2.MustCompile(val, 0) + re.MatchTimeout = PatternMatchTimeout + + return re } // MustDeprecatedRegexp returns the regular expression of a deprecated flag if @@ -235,6 +258,9 @@ func (f *ParsedFlags) MustDeprecatedRegexp(deprecated string, newName string) *r // // Every allow-list and deny-list in Gotenberg is read through this method, so // it is also where allow-list patterns are audited. See [AuditAllowList]. +// +// The returned expressions carry [PatternMatchTimeout] and are safe to match +// on concurrently: callers must not compile a private copy per match. func (f *ParsedFlags) MustRegexpSlice(name string) []*regexp2.Regexp { vals := f.MustStringSlice(name) @@ -246,7 +272,10 @@ func (f *ParsedFlags) MustRegexpSlice(name string) []*regexp2.Regexp { continue } - regexps = append(regexps, regexp2.MustCompile(val, 0)) + re := regexp2.MustCompile(val, 0) + re.MatchTimeout = PatternMatchTimeout + + regexps = append(regexps, re) } return regexps diff --git a/pkg/gotenberg/flags_test.go b/pkg/gotenberg/flags_test.go index 4ffb64a9..b140a1c1 100644 --- a/pkg/gotenberg/flags_test.go +++ b/pkg/gotenberg/flags_test.go @@ -1054,3 +1054,36 @@ func TestEnvVarName(t *testing.T) { }) } } + +func TestParsedFlags_RegexpMatchTimeout(t *testing.T) { + // [DecideOutbound] matches on these patterns directly instead of compiling + // a private copy per call, so the bound has to come from here. regexp2's + // own default is math.MaxInt64, which it treats as no + // timeout at all, so a pattern built without this stamp runs unbounded + // against a client-controlled string. + fs := flag.NewFlagSet("tests", flag.ContinueOnError) + fs.StringSlice("some-deny-list", []string{`^file:`, `^https?://`}, "") + fs.String("some-pattern", `^file:`, "") + + err := fs.Parse(nil) + if err != nil { + t.Fatalf("expected no error but got: %v", err) + } + + parsedFlags := ParsedFlags{FlagSet: fs} + + regexps := parsedFlags.MustRegexpSlice("some-deny-list") + if len(regexps) != 2 { + t.Fatalf("expected 2 patterns but got %d", len(regexps)) + } + + for _, re := range regexps { + if re.MatchTimeout != PatternMatchTimeout { + t.Fatalf("pattern '%s' has MatchTimeout %s, expected %s", re.String(), re.MatchTimeout, PatternMatchTimeout) + } + } + + if got := parsedFlags.MustRegexp("some-pattern").MatchTimeout; got != PatternMatchTimeout { + t.Fatalf("expected MustRegexp MatchTimeout %s but got %s", PatternMatchTimeout, got) + } +} diff --git a/pkg/gotenberg/outbound.go b/pkg/gotenberg/outbound.go index 5f52e1cb..2a771ba7 100644 --- a/pkg/gotenberg/outbound.go +++ b/pkg/gotenberg/outbound.go @@ -27,6 +27,12 @@ import ( // example [::ffff:127.0.0.1]). var ErrNonPublicIP = errors.New("non-public IP") +// ErrFiltered happens when a value is rejected by an allow-list or a +// deny-list, or when it cannot be validated and [DecideOutbound] fails closed. +// Callers map it to a generic 403: the specific reason stays in the operator +// logs so a client cannot probe the lists. +var ErrFiltered = errors.New("value filtered") + // ErrPublicIP indicates that an outbound URL targets an IP address that is // reachable on the public internet. It is returned when a caller opts // into denying public destinations via [WithDenyPublicIPs]; typical use @@ -291,6 +297,15 @@ func DecideOutbound(ctx context.Context, rawURL string, allowList, denyList []*r opt(&cfg) } + // Each match is bounded by [PatternMatchTimeout] rather than by the + // remaining budget, so an already-spent deadline no longer surfaces from + // the match itself. Schemes that resolve a host still learn about it from + // resolveHost, but a non-matching file:// or data: URL returns before that + // point, so check it here to keep failing closed on every path. + if !time.Now().Before(deadline) { + return OutboundDecision{}, context.DeadlineExceeded + } + parsed, err := url.Parse(rawURL) if err != nil { return OutboundDecision{}, fmt.Errorf("parse URL %q: %w", rawURL, ErrFiltered) @@ -314,15 +329,12 @@ func DecideOutbound(ctx context.Context, rawURL string, allowList, denyList []*r allowMatched := false if len(allowList) > 0 { for _, pattern := range allowList { - clone := regexp2.MustCompile(pattern.String(), 0) - clone.MatchTimeout = time.Until(deadline) - - ok, err := clone.MatchString(normalized) + ok, err := pattern.MatchString(normalized) if err != nil { if time.Now().After(deadline) { return OutboundDecision{}, context.DeadlineExceeded } - return OutboundDecision{}, fmt.Errorf("'%s' cannot handle '%s': %w", clone.String(), normalized, err) + return OutboundDecision{}, fmt.Errorf("'%s' cannot handle '%s': %w", pattern.String(), normalized, err) } if ok { @@ -337,15 +349,12 @@ func DecideOutbound(ctx context.Context, rawURL string, allowList, denyList []*r } for _, pattern := range denyList { - clone := regexp2.MustCompile(pattern.String(), 0) - clone.MatchTimeout = time.Until(deadline) - - ok, err := clone.MatchString(normalized) + ok, err := pattern.MatchString(normalized) if err != nil { if time.Now().After(deadline) { return OutboundDecision{}, context.DeadlineExceeded } - return OutboundDecision{}, fmt.Errorf("'%s' cannot handle '%s': %w", clone.String(), normalized, err) + return OutboundDecision{}, fmt.Errorf("'%s' cannot handle '%s': %w", pattern.String(), normalized, err) } if ok { @@ -392,9 +401,8 @@ func DecideOutbound(ctx context.Context, rawURL string, allowList, denyList []*r } // FilterOutboundURL validates that rawURL is acceptable for an outbound -// request from Gotenberg. It is the URL-aware replacement for -// [FilterDeadline] and should be preferred for any new code that filters -// a URL before issuing or instructing an outbound request. +// request from Gotenberg. Prefer it for any new code that filters a URL +// before issuing or instructing an outbound request. // // The default behavior is permissive: the URL passes as long as it clears // the regex allow-list and deny-list. Callers that need IP-class checks diff --git a/pkg/gotenberg/outbound_test.go b/pkg/gotenberg/outbound_test.go index d4edd067..ec237a00 100644 --- a/pkg/gotenberg/outbound_test.go +++ b/pkg/gotenberg/outbound_test.go @@ -715,3 +715,54 @@ func TestNewOutboundHttpClient_NonPositiveTimeout(t *testing.T) { t.Fatalf("timeout for a negative budget = %s, want a positive value so the client fails closed", got) } } + +func TestDecideOutboundExpiredDeadline(t *testing.T) { + // Patterns are matched under the fixed PatternMatchTimeout rather than + // under the caller's remaining budget, so an expired deadline no longer + // surfaces from the match itself. Every scheme must still fail closed, + // including the ones that return before a host is resolved. + expired := time.Now().Add(-time.Second) + + for _, rawURL := range []string{ + "https://example.com/", + "file:///tmp/foo.html", + "data:text/html,hello", + } { + _, err := DecideOutbound(context.Background(), rawURL, nil, nil, expired) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("DecideOutbound(%q) with an expired deadline = %v, want context.DeadlineExceeded", rawURL, err) + } + } +} + +func TestDecideOutboundBoundsCatastrophicPatterns(t *testing.T) { + // A deny-list pattern that backtracks catastrophically, matched against a + // client-controlled URL. Before PatternMatchTimeout the ceiling was the + // caller's whole budget, so a 30s API_TIMEOUT bought a 30s CPU burn. + // The trailing "!" makes the match fail only after the nested quantifier + // has explored every way to split the run of "a"s. + pattern := regexp2.MustCompile(`^https://example\.com/(a+)+$`, 0) + pattern.MatchTimeout = PatternMatchTimeout + + rawURL := "https://example.com/" + strings.Repeat("a", 40) + "!" + + start := time.Now() + _, err := DecideOutbound( + context.Background(), + rawURL, + nil, + []*regexp2.Regexp{pattern}, + time.Now().Add(30*time.Second), + ) + elapsed := time.Since(start) + + if err == nil { + t.Fatal("expected an error from a catastrophic deny-list pattern") + } + + // 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 { + t.Fatalf("match took %s, want it aborted near PatternMatchTimeout (%s)", elapsed, PatternMatchTimeout) + } +}