From 8625a4e899eb75e6fcf46d28394334c7fd79fff5 Mon Sep 17 00:00:00 2001 From: Julien Neuhart Date: Fri, 27 Mar 2026 10:52:04 +0100 Subject: [PATCH] fix(webhook/downloadFrom): better default security and DX for allow / deny lists --- pkg/gotenberg/filter.go | 11 ------- pkg/gotenberg/filter_test.go | 42 ------------------------- pkg/modules/api/api.go | 12 +++---- pkg/modules/api/context.go | 7 +---- pkg/modules/webhook/middleware.go | 14 ++------- pkg/modules/webhook/webhook.go | 24 +++++++------- test/integration/features/debug.feature | 32 +++++++++---------- 7 files changed, 37 insertions(+), 105 deletions(-) diff --git a/pkg/gotenberg/filter.go b/pkg/gotenberg/filter.go index f5cfcd4e..217d3303 100644 --- a/pkg/gotenberg/filter.go +++ b/pkg/gotenberg/filter.go @@ -12,17 +12,6 @@ import ( // ErrFiltered happens if a value is filtered by the [FilterDeadline] function. var ErrFiltered = errors.New("value filtered") -// RegexpToSlice wraps a single [regexp2.Regexp] into a slice suitable for -// [FilterDeadline]. If the regexp pattern is empty, it returns nil (meaning no -// filtering). -func RegexpToSlice(r *regexp2.Regexp) []*regexp2.Regexp { - if r == nil || r.String() == "" { - return nil - } - - return []*regexp2.Regexp{r} -} - // 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 diff --git a/pkg/gotenberg/filter_test.go b/pkg/gotenberg/filter_test.go index 20f55b7d..c8ef2965 100644 --- a/pkg/gotenberg/filter_test.go +++ b/pkg/gotenberg/filter_test.go @@ -115,45 +115,3 @@ func TestFilterDeadline(t *testing.T) { }) } } - -func TestRegexpToSlice(t *testing.T) { - for _, tc := range []struct { - scenario string - input *regexp2.Regexp - expectNil bool - expectLen int - }{ - { - scenario: "nil regexp", - input: nil, - expectNil: true, - }, - { - scenario: "empty regexp", - input: regexp2.MustCompile("", 0), - expectNil: true, - }, - { - scenario: "non-empty regexp", - input: regexp2.MustCompile("^file:.*", 0), - expectNil: false, - expectLen: 1, - }, - } { - t.Run(tc.scenario, func(t *testing.T) { - result := RegexpToSlice(tc.input) - - if tc.expectNil && result != nil { - t.Fatalf("expected nil but got: %v", result) - } - - if !tc.expectNil && result == nil { - t.Fatal("expected non-nil but got nil") - } - - if !tc.expectNil && len(result) != tc.expectLen { - t.Fatalf("expected length %d but got %d", tc.expectLen, len(result)) - } - }) - } -} diff --git a/pkg/modules/api/api.go b/pkg/modules/api/api.go index 3863bcac..0eec39ee 100644 --- a/pkg/modules/api/api.go +++ b/pkg/modules/api/api.go @@ -55,8 +55,8 @@ type Api struct { } type downloadFromConfig struct { - allowList *regexp2.Regexp - denyList *regexp2.Regexp + allowList []*regexp2.Regexp + denyList []*regexp2.Regexp maxRetry int disable bool } @@ -192,8 +192,8 @@ func (a *Api) Descriptor() gotenberg.ModuleDescriptor { fs.String("api-root-path", "/", "Set the root path of the API - for service discovery via URL paths") fs.String("api-trace-header", "Gotenberg-Trace", "Set the header name to use for identifying requests") fs.Bool("api-enable-basic-auth", false, "Enable basic authentication - will look for the GOTENBERG_API_BASIC_AUTH_USERNAME and GOTENBERG_API_BASIC_AUTH_PASSWORD environment variables") - fs.String("api-download-from-allow-list", "", "Set the allowed URLs for the download from feature using a regular expression") - fs.String("api-download-from-deny-list", "", "Set the denied URLs for the download from feature using a regular expression") + fs.StringSlice("api-download-from-allow-list", []string{}, "Set the allowed URLs for the download from feature using regular expressions - supports multiple values") + fs.StringSlice("api-download-from-deny-list", []string{}, "Set the denied URLs for the download from feature using regular expressions - supports multiple values") fs.Int("api-download-from-max-retry", 4, "Set the maximum number of retries for the download from feature") fs.Bool("api-disable-download-from", false, "Disable the download from feature") fs.Bool("api-disable-health-check-logging", false, "Disable health check logging") @@ -217,8 +217,8 @@ func (a *Api) Provision(ctx *gotenberg.Context) error { a.rootPath = flags.MustString("api-root-path") a.traceHeader = flags.MustString("api-trace-header") a.downloadFromCfg = downloadFromConfig{ - allowList: flags.MustRegexp("api-download-from-allow-list"), - denyList: flags.MustRegexp("api-download-from-deny-list"), + allowList: flags.MustRegexpSlice("api-download-from-allow-list"), + denyList: flags.MustRegexpSlice("api-download-from-deny-list"), maxRetry: flags.MustInt("api-download-from-max-retry"), disable: flags.MustBool("api-disable-download-from"), } diff --git a/pkg/modules/api/context.go b/pkg/modules/api/context.go index 90d661d7..5c034674 100644 --- a/pkg/modules/api/context.go +++ b/pkg/modules/api/context.go @@ -225,12 +225,7 @@ func newContext(echoCtx echo.Context, logger *zap.Logger, fs *gotenberg.FileSyst ) } - err := gotenberg.FilterDeadline( - gotenberg.RegexpToSlice(downloadFromCfg.allowList), - gotenberg.RegexpToSlice(downloadFromCfg.denyList), - dl.Url, - deadline, - ) + err := gotenberg.FilterDeadline(downloadFromCfg.allowList, downloadFromCfg.denyList, dl.Url, deadline) if err != nil { return fmt.Errorf("filter URL: %w", err) } diff --git a/pkg/modules/webhook/middleware.go b/pkg/modules/webhook/middleware.go index 84de6bf4..8771f042 100644 --- a/pkg/modules/webhook/middleware.go +++ b/pkg/modules/webhook/middleware.go @@ -114,22 +114,12 @@ func webhookMiddleware(w *Webhook) api.Middleware { // Let's check if the webhook URLs are acceptable according to our // allowed/denied lists. - err := gotenberg.FilterDeadline( - gotenberg.RegexpToSlice(w.allowList), - gotenberg.RegexpToSlice(w.denyList), - webhookUrl, - deadline, - ) + err := gotenberg.FilterDeadline(w.allowList, w.denyList, webhookUrl, deadline) if err != nil { return fmt.Errorf("filter webhook URL: %w", err) } - err = gotenberg.FilterDeadline( - gotenberg.RegexpToSlice(w.errorAllowList), - gotenberg.RegexpToSlice(w.errorDenyList), - webhookErrorUrl, - deadline, - ) + err = gotenberg.FilterDeadline(w.errorAllowList, w.errorDenyList, webhookErrorUrl, deadline) if err != nil { return fmt.Errorf("filter webhook error URL: %w", err) } diff --git a/pkg/modules/webhook/webhook.go b/pkg/modules/webhook/webhook.go index dd660ce8..25c4327f 100644 --- a/pkg/modules/webhook/webhook.go +++ b/pkg/modules/webhook/webhook.go @@ -19,10 +19,10 @@ func init() { // to any destinations in an asynchronous fashion. type Webhook struct { enableSyncMode bool - allowList *regexp2.Regexp - denyList *regexp2.Regexp - errorAllowList *regexp2.Regexp - errorDenyList *regexp2.Regexp + allowList []*regexp2.Regexp + denyList []*regexp2.Regexp + errorAllowList []*regexp2.Regexp + errorDenyList []*regexp2.Regexp maxRetry int retryMinWait time.Duration retryMaxWait time.Duration @@ -38,10 +38,10 @@ func (w *Webhook) Descriptor() gotenberg.ModuleDescriptor { FlagSet: func() *flag.FlagSet { fs := flag.NewFlagSet("webhook", flag.ExitOnError) fs.Bool("webhook-enable-sync-mode", false, "Enable synchronous mode for the webhook feature") - fs.String("webhook-allow-list", "", "Set the allowed URLs for the webhook feature using a regular expression") - fs.String("webhook-deny-list", "", "Set the denied URLs for the webhook feature using a regular expression") - fs.String("webhook-error-allow-list", "", "Set the allowed URLs in case of an error for the webhook feature using a regular expression") - fs.String("webhook-error-deny-list", "", "Set the denied URLs in case of an error for the webhook feature using a regular expression") + fs.StringSlice("webhook-allow-list", []string{}, "Set the allowed URLs for the webhook feature using regular expressions - supports multiple values") + fs.StringSlice("webhook-deny-list", []string{}, "Set the denied URLs for the webhook feature using regular expressions - supports multiple values") + fs.StringSlice("webhook-error-allow-list", []string{}, "Set the allowed URLs in case of an error for the webhook feature using regular expressions - supports multiple values") + fs.StringSlice("webhook-error-deny-list", []string{}, "Set the denied URLs in case of an error for the webhook feature using regular expressions - supports multiple values") fs.Int("webhook-max-retry", 4, "Set the maximum number of retries for the webhook feature") fs.Duration("webhook-retry-min-wait", time.Duration(1)*time.Second, "Set the minimum duration to wait before trying to call the webhook again") fs.Duration("webhook-retry-max-wait", time.Duration(30)*time.Second, "Set the maximum duration to wait before trying to call the webhook again") @@ -58,10 +58,10 @@ func (w *Webhook) Descriptor() gotenberg.ModuleDescriptor { func (w *Webhook) Provision(ctx *gotenberg.Context) error { flags := ctx.ParsedFlags() w.enableSyncMode = flags.MustBool("webhook-enable-sync-mode") - w.allowList = flags.MustRegexp("webhook-allow-list") - w.denyList = flags.MustRegexp("webhook-deny-list") - w.errorAllowList = flags.MustRegexp("webhook-error-allow-list") - w.errorDenyList = flags.MustRegexp("webhook-error-deny-list") + w.allowList = flags.MustRegexpSlice("webhook-allow-list") + w.denyList = flags.MustRegexpSlice("webhook-deny-list") + w.errorAllowList = flags.MustRegexpSlice("webhook-error-allow-list") + w.errorDenyList = flags.MustRegexpSlice("webhook-error-deny-list") w.maxRetry = flags.MustInt("webhook-max-retry") w.retryMinWait = flags.MustDuration("webhook-retry-min-wait") w.retryMaxWait = flags.MustDuration("webhook-retry-max-wait") diff --git a/test/integration/features/debug.feature b/test/integration/features/debug.feature index 1ebb55b4..bc561577 100644 --- a/test/integration/features/debug.feature +++ b/test/integration/features/debug.feature @@ -58,8 +58,8 @@ Feature: /debug "api-body-limit": "", "api-disable-download-from": "false", "api-disable-health-check-logging": "false", - "api-download-from-allow-list": "", - "api-download-from-deny-list": "", + "api-download-from-allow-list": "[]", + "api-download-from-deny-list": "[]", "api-download-from-max-retry": "4", "api-enable-basic-auth": "false", "api-enable-debug-route": "true", @@ -73,11 +73,11 @@ Feature: /debug "api-trace-header": "Gotenberg-Trace", "chromium-allow-file-access-from-files": "false", "chromium-allow-insecure-localhost": "false", - "chromium-allow-list": "", + "chromium-allow-list": "[]", "chromium-auto-start": "false", "chromium-clear-cache": "false", "chromium-clear-cookies": "false", - "chromium-deny-list": "^file:(?!//\\/tmp/).*", + "chromium-deny-list": "[^file:(?!//\\/tmp/).*]", "chromium-disable-javascript": "false", "chromium-disable-routes": "false", "chromium-disable-web-security": "false", @@ -114,12 +114,12 @@ Feature: /debug "prometheus-disable-route-logging": "false", "prometheus-namespace": "gotenberg", "prometheus-metrics-path": "/prometheus/metrics", - "webhook-allow-list": "", + "webhook-allow-list": "[]", "webhook-client-timeout": "30s", - "webhook-deny-list": "", + "webhook-deny-list": "[]", "webhook-disable": "false", - "webhook-error-allow-list": "", - "webhook-error-deny-list": "", + "webhook-error-allow-list": "[]", + "webhook-error-deny-list": "[]", "webhook-max-retry": "4", "webhook-retry-max-wait": "30s", "webhook-retry-min-wait": "1s" @@ -180,8 +180,8 @@ Feature: /debug "api-body-limit": "", "api-disable-download-from": "false", "api-disable-health-check-logging": "false", - "api-download-from-allow-list": "", - "api-download-from-deny-list": "", + "api-download-from-allow-list": "[]", + "api-download-from-deny-list": "[]", "api-download-from-max-retry": "4", "api-enable-basic-auth": "false", "api-enable-debug-route": "true", @@ -195,11 +195,11 @@ Feature: /debug "api-trace-header": "Gotenberg-Trace", "chromium-allow-file-access-from-files": "false", "chromium-allow-insecure-localhost": "false", - "chromium-allow-list": "", + "chromium-allow-list": "[]", "chromium-auto-start": "false", "chromium-clear-cache": "false", "chromium-clear-cookies": "false", - "chromium-deny-list": "^file:(?!//\\/tmp/).*", + "chromium-deny-list": "[^file:(?!//\\/tmp/).*]", "chromium-disable-javascript": "false", "chromium-disable-routes": "false", "chromium-disable-web-security": "false", @@ -236,12 +236,12 @@ Feature: /debug "prometheus-disable-route-logging": "false", "prometheus-namespace": "gotenberg", "prometheus-metrics-path": "/prometheus/metrics", - "webhook-allow-list": "", + "webhook-allow-list": "[]", "webhook-client-timeout": "30s", - "webhook-deny-list": "", + "webhook-deny-list": "[]", "webhook-disable": "false", - "webhook-error-allow-list": "", - "webhook-error-deny-list": "", + "webhook-error-allow-list": "[]", + "webhook-error-deny-list": "[]", "webhook-max-retry": "4", "webhook-retry-max-wait": "30s", "webhook-retry-min-wait": "1s"