From 7b604da4f14d0d9d9319fbf1928d58b4dbc2d67e Mon Sep 17 00:00:00 2001 From: Julien Neuhart Date: Tue, 13 Oct 2026 18:31:38 +0200 Subject: [PATCH] fix(chromium)!: isolate each conversion in its own browser context --- Makefile | 1 + compose.yaml | 1 + pkg/modules/chromium/browser.go | 81 ++++++++++++++++--- pkg/modules/chromium/chromium.go | 26 +++--- .../features/chromium_convert_url.feature | 63 +++++++++++++-- test/integration/features/debug.feature | 2 + 6 files changed, 148 insertions(+), 26 deletions(-) diff --git a/Makefile b/Makefile index a5008821..73b7b379 100644 --- a/Makefile +++ b/Makefile @@ -66,6 +66,7 @@ CHROMIUM_ALLOW_LIST= CHROMIUM_DENY_LIST=^file:(?!//\/tmp/).* CHROMIUM_DENY_PRIVATE_IPS=false CHROMIUM_DENY_PUBLIC_IPS=false +CHROMIUM_SHARED_BROWSER_CONTEXT=false CHROMIUM_CLEAR_CACHE=false CHROMIUM_CLEAR_COOKIES=false CHROMIUM_DISABLE_JAVASCRIPT=false diff --git a/compose.yaml b/compose.yaml index 19512e83..c2b1a497 100644 --- a/compose.yaml +++ b/compose.yaml @@ -60,6 +60,7 @@ services: - "--chromium-deny-list=${CHROMIUM_DENY_LIST}" - "--chromium-deny-private-ips=${CHROMIUM_DENY_PRIVATE_IPS}" - "--chromium-deny-public-ips=${CHROMIUM_DENY_PUBLIC_IPS}" + - "--chromium-shared-browser-context=${CHROMIUM_SHARED_BROWSER_CONTEXT}" - "--chromium-clear-cache=${CHROMIUM_CLEAR_CACHE}" - "--chromium-clear-cookies=${CHROMIUM_CLEAR_COOKIES}" - "--chromium-disable-javascript=${CHROMIUM_DISABLE_JAVASCRIPT}" diff --git a/pkg/modules/chromium/browser.go b/pkg/modules/chromium/browser.go index 8dbfbbd9..ed523eec 100644 --- a/pkg/modules/chromium/browser.go +++ b/pkg/modules/chromium/browser.go @@ -12,10 +12,12 @@ import ( "time" cdprotobrowser "github.com/chromedp/cdproto/browser" + "github.com/chromedp/cdproto/cdp" "github.com/chromedp/cdproto/fetch" "github.com/chromedp/cdproto/network" "github.com/chromedp/cdproto/page" "github.com/chromedp/cdproto/runtime" + "github.com/chromedp/cdproto/target" "github.com/chromedp/chromedp" "github.com/dlclark/regexp2/v2" "github.com/shirou/gopsutil/v4/process" @@ -62,14 +64,15 @@ type browserArguments struct { hyphenDataDirPath string // Tasks specific. - allowList []*regexp2.Regexp - denyList []*regexp2.Regexp - denyPrivateIPs bool - denyPublicIPs bool - clearCache bool - clearCookies bool - clearStorage bool - disableJavaScript bool + allowList []*regexp2.Regexp + denyList []*regexp2.Regexp + denyPrivateIPs bool + denyPublicIPs bool + sharedBrowserContext bool + clearCache bool + clearCookies bool + clearStorage bool + disableJavaScript bool } type chromiumBrowser struct { @@ -435,6 +438,55 @@ func (b *chromiumBrowser) screenshot(ctx context.Context, logger *slog.Logger, u }) } +// newIsolatedTarget creates a browser context and opens a blank tab in it, so +// cookies, cache, and storage never leak to another conversion, even a +// concurrent one. The caller must dispose the browser context. +// +// chromedp.WithNewBrowserContext does not work here: it opens the tab without +// a new window, which headless Chromium rejects with "no browser is open", and +// it leaks the browser context on that failure. +func (b *chromiumBrowser) newIsolatedTarget(ctx context.Context, logger *slog.Logger) (cdp.BrowserContextID, target.ID, error) { + c := chromedp.FromContext(b.ctx) + if c == nil || c.Browser == nil { + return "", "", errors.New("browser not allocated") + } + executor := cdp.WithExecutor(ctx, c.Browser) + + browserContextID, err := target.CreateBrowserContext().WithDisposeOnDetach(true).Do(executor) + if err != nil { + return "", "", fmt.Errorf("create browser context: %w", err) + } + + targetID, err := target.CreateTarget("about:blank"). + WithBrowserContextID(browserContextID). + WithNewWindow(true). + Do(executor) + if err != nil { + b.disposeBrowserContext(logger, browserContextID) + return "", "", fmt.Errorf("create target: %w", err) + } + + return browserContextID, targetID, nil +} + +// disposeBrowserContext disposes a browser context created by +// [chromiumBrowser.newIsolatedTarget]. It uses a fresh context, as the +// conversion's one may already be done. +func (b *chromiumBrowser) disposeBrowserContext(logger *slog.Logger, browserContextID cdp.BrowserContextID) { + c := chromedp.FromContext(b.ctx) + if c == nil || c.Browser == nil { + return + } + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + err := target.DisposeBrowserContext(browserContextID).Do(cdp.WithExecutor(ctx, c.Browser)) + if err != nil { + logger.ErrorContext(ctx, fmt.Sprintf("dispose browser context: %s", err)) + } +} + func (b *chromiumBrowser) do(ctx context.Context, logger *slog.Logger, url string, options Options, aggregate *networkAggregate, tasks chromedp.Tasks) error { if !b.isStarted.Load() { return errors.New("browser not started, cannot handle tasks") @@ -461,7 +513,18 @@ func (b *chromiumBrowser) do(ctx context.Context, logger *slog.Logger, url strin timeoutCtx, timeoutCancel := context.WithTimeout(b.ctx, time.Until(deadline)) defer timeoutCancel() - taskCtx, taskCancel := chromedp.NewContext(timeoutCtx) + var taskOpts []chromedp.ContextOption + if !b.arguments.sharedBrowserContext { + browserContextID, targetID, err := b.newIsolatedTarget(timeoutCtx, logger) + if err != nil { + return fmt.Errorf("create isolated browser context: %w", err) + } + // Deferred before taskCancel, so it runs after the tab is closed. + defer b.disposeBrowserContext(logger, browserContextID) + taskOpts = append(taskOpts, chromedp.WithTargetID(targetID)) + } + + taskCtx, taskCancel := chromedp.NewContext(timeoutCtx, taskOpts...) defer taskCancel() // Accumulate per-conversion network activity for telemetry. diff --git a/pkg/modules/chromium/chromium.go b/pkg/modules/chromium/chromium.go index 0c5b8829..a8eb7a6d 100644 --- a/pkg/modules/chromium/chromium.go +++ b/pkg/modules/chromium/chromium.go @@ -479,9 +479,10 @@ func (mod *Chromium) Descriptor() gotenberg.ModuleDescriptor { fs.StringSlice("chromium-deny-list", []string{`^file:(?!//\/tmp/).*`}, "Set the denied URLs for Chromium using regular expressions - supports multiple values") fs.Bool("chromium-deny-private-ips", false, "Reject URLs whose host resolves to a non-public IP address (loopback, RFC1918, link-local, unique-local). Enable on deployments that accept untrusted form input to mitigate SSRF against internal services") fs.Bool("chromium-deny-public-ips", false, "Reject URLs whose host resolves to a public IP address. Enable on air-gapped or data-governed deployments to prevent outbound traffic from leaving a private network") - fs.Bool("chromium-clear-cache", false, "Clear Chromium cache between each conversion") - fs.Bool("chromium-clear-cookies", false, "Clear Chromium cookies between each conversion") - fs.Bool("chromium-clear-storage", false, "Clear Chromium local storage between each conversion (session storage is already isolated per conversion)") + fs.Bool("chromium-shared-browser-context", false, "Run every conversion in one shared browser context, so cookies, cache, and storage carry over between conversions. Only enable it when all callers trust each other: a conversion can then reuse cookies another one set") + fs.Bool("chromium-clear-cache", false, "Clear Chromium cache between each conversion - only applies with --chromium-shared-browser-context") + fs.Bool("chromium-clear-cookies", false, "Clear Chromium cookies between each conversion - only applies with --chromium-shared-browser-context") + fs.Bool("chromium-clear-storage", false, "Clear Chromium local storage between each conversion - only applies with --chromium-shared-browser-context") fs.Bool("chromium-disable-javascript", false, "Disable JavaScript") fs.Bool("chromium-disable-routes", false, "Disable the routes") @@ -515,6 +516,8 @@ func (mod *Chromium) Provision(ctx *gotenberg.Context) error { return errors.New("CHROMIUM_HYPHEN_DATA_DIR_PATH environment variable is not set; set it to the absolute path of the Chromium hyphenation data directory (it ships in the Gotenberg image)") } + sharedBrowserContext := flags.MustBool("chromium-shared-browser-context") + mod.args = browserArguments{ binPath: binPath, allowInsecureLocalhost: flags.MustBool("chromium-allow-insecure-localhost"), @@ -527,13 +530,16 @@ func (mod *Chromium) Provision(ctx *gotenberg.Context) error { wsUrlReadTimeout: flags.MustDuration("chromium-start-timeout"), hyphenDataDirPath: hyphenDataDirPath, - allowList: flags.MustRegexpSlice("chromium-allow-list"), - denyList: flags.MustRegexpSlice("chromium-deny-list"), - denyPrivateIPs: flags.MustBool("chromium-deny-private-ips"), - denyPublicIPs: flags.MustBool("chromium-deny-public-ips"), - clearCache: flags.MustBool("chromium-clear-cache"), - clearCookies: flags.MustBool("chromium-clear-cookies"), - clearStorage: flags.MustBool("chromium-clear-storage"), + allowList: flags.MustRegexpSlice("chromium-allow-list"), + denyList: flags.MustRegexpSlice("chromium-deny-list"), + denyPrivateIPs: flags.MustBool("chromium-deny-private-ips"), + denyPublicIPs: flags.MustBool("chromium-deny-public-ips"), + sharedBrowserContext: sharedBrowserContext, + // An isolated conversion starts from an empty browser context that + // is disposed afterward, so there is nothing to clear. + clearCache: sharedBrowserContext && flags.MustBool("chromium-clear-cache"), + clearCookies: sharedBrowserContext && flags.MustBool("chromium-clear-cookies"), + clearStorage: sharedBrowserContext && flags.MustBool("chromium-clear-storage"), disableJavaScript: flags.MustBool("chromium-disable-javascript"), } diff --git a/test/integration/features/chromium_convert_url.feature b/test/integration/features/chromium_convert_url.feature index 3c4c2391..18c338be 100644 --- a/test/integration/features/chromium_convert_url.feature +++ b/test/integration/features/chromium_convert_url.feature @@ -22,10 +22,9 @@ Feature: /forms/chromium/convert/url Page 1 """ - # localStorage is per-origin and shared by the long-lived browser, so without - # clearing it accumulates across same-origin conversions. - # See https://github.com/gotenberg/gotenberg/issues/919. - Scenario: POST /forms/chromium/convert/url (localStorage leaks without clearing) + # Each conversion runs in its own browser context, so localStorage never + # carries over. + Scenario: POST /forms/chromium/convert/url (localStorage isolated per conversion) Given I have a default Gotenberg container Given I have a static server When I make a "POST" request to Gotenberg at the "/forms/chromium/convert/url" endpoint with the following form data and header(s): @@ -37,14 +36,34 @@ Feature: /forms/chromium/convert/url | url | http://host.docker.internal:%d/html/testdata/local-storage-html/index.html | field | | Gotenberg-Output-Filename | foo | header | Then the response status code should be 200 + Then the "foo.pdf" PDF should have content matching "localStorageCount=1" at page 1 + + # With --chromium-shared-browser-context, localStorage is per-origin and + # shared by the long-lived browser, so without clearing it accumulates + # across same-origin conversions. + # See https://github.com/gotenberg/gotenberg/issues/919. + Scenario: POST /forms/chromium/convert/url (localStorage leaks in a shared browser context) + Given I have a Gotenberg container with the following environment variable(s): + | CHROMIUM_SHARED_BROWSER_CONTEXT | true | + Given I have a static server + When I make a "POST" request to Gotenberg at the "/forms/chromium/convert/url" endpoint with the following form data and header(s): + | url | http://host.docker.internal:%d/html/testdata/local-storage-html/index.html | field | + | Gotenberg-Output-Filename | foo | header | + Then the response status code should be 200 + Then the "foo.pdf" PDF should have content matching "localStorageCount=1" at page 1 + When I make a "POST" request to Gotenberg at the "/forms/chromium/convert/url" endpoint with the following form data and header(s): + | url | http://host.docker.internal:%d/html/testdata/local-storage-html/index.html | field | + | Gotenberg-Output-Filename | foo | header | + Then the response status code should be 200 Then the "foo.pdf" PDF should have content matching "localStorageCount=2" at page 1 - # With --chromium-clear-storage every conversion clears the origin's - # localStorage first, so the counter never carries over. + # With --chromium-clear-storage every conversion in a shared browser context + # clears the origin's localStorage first, so the counter never carries over. # See https://github.com/gotenberg/gotenberg/issues/919. Scenario: POST /forms/chromium/convert/url (Clear Storage) Given I have a Gotenberg container with the following environment variable(s): - | CHROMIUM_CLEAR_STORAGE | true | + | CHROMIUM_SHARED_BROWSER_CONTEXT | true | + | CHROMIUM_CLEAR_STORAGE | true | Given I have a static server When I make a "POST" request to Gotenberg at the "/forms/chromium/convert/url" endpoint with the following form data and header(s): | url | http://host.docker.internal:%d/html/testdata/local-storage-html/index.html | field | @@ -210,6 +229,36 @@ Feature: /forms/chromium/convert/url Then the server request cookie "cookie_1" should be "foo" Then the server request cookie "cookie_2" should be "" + # A cookie set by one conversion must not reach the next one. + Scenario: POST /forms/chromium/convert/url (Cookies isolated per conversion) + Given I have a default Gotenberg container + Given I have a static server + When I make a "POST" request to Gotenberg at the "/forms/chromium/convert/url" endpoint with the following form data and header(s): + | url | http://host.docker.internal:%d/html/testdata/page-1-html/index.html | field | + | cookies | [{"name":"cookie_1","value":"foo","domain":"host.docker.internal:%d"}] | field | + Then the response status code should be 200 + Then the server request cookie "cookie_1" should be "foo" + When I make a "POST" request to Gotenberg at the "/forms/chromium/convert/url" endpoint with the following form data and header(s): + | url | http://host.docker.internal:%d/html/testdata/page-1-html/index.html | field | + Then the response status code should be 200 + Then the server request cookie "cookie_1" should be "" + + # With --chromium-shared-browser-context, cookies carry over between + # conversions. + Scenario: POST /forms/chromium/convert/url (Cookies in a shared browser context) + Given I have a Gotenberg container with the following environment variable(s): + | CHROMIUM_SHARED_BROWSER_CONTEXT | true | + Given I have a static server + When I make a "POST" request to Gotenberg at the "/forms/chromium/convert/url" endpoint with the following form data and header(s): + | url | http://host.docker.internal:%d/html/testdata/page-1-html/index.html | field | + | cookies | [{"name":"cookie_1","value":"foo","domain":"host.docker.internal:%d"}] | field | + Then the response status code should be 200 + Then the server request cookie "cookie_1" should be "foo" + When I make a "POST" request to Gotenberg at the "/forms/chromium/convert/url" endpoint with the following form data and header(s): + | url | http://host.docker.internal:%d/html/testdata/page-1-html/index.html | field | + Then the response status code should be 200 + Then the server request cookie "cookie_1" should be "foo" + # See https://github.com/gotenberg/gotenberg/issues/1130. Scenario: POST /forms/chromium/convert/url (case-insensitive sameSite) Given I have a default Gotenberg container diff --git a/test/integration/features/debug.feature b/test/integration/features/debug.feature index cbd67351..16badbe9 100644 --- a/test/integration/features/debug.feature +++ b/test/integration/features/debug.feature @@ -98,6 +98,7 @@ Feature: /debug "chromium-max-queue-size": "0", "chromium-proxy-server": "", "chromium-restart-after": "100", + "chromium-shared-browser-context": "false", "chromium-start-timeout": "20s", "gotenberg-build-debug-data": "true", "gotenberg-graceful-shutdown-duration": "30s", @@ -237,6 +238,7 @@ Feature: /debug "chromium-max-concurrency": "6", "chromium-proxy-server": "", "chromium-restart-after": "100", + "chromium-shared-browser-context": "false", "chromium-start-timeout": "20s", "gotenberg-build-debug-data": "true", "gotenberg-graceful-shutdown-duration": "30s",