Harden the Google Tag Manager proxy's upstream and tag selection - #1109
Open
prk-Jr wants to merge 14 commits into
Open
Harden the Google Tag Manager proxy's upstream and tag selection#1109prk-Jr wants to merge 14 commits into
prk-Jr wants to merge 14 commits into
Conversation
Serve only the configured container from gtm.js: a client-supplied id is dropped and the query is rebuilt, so the id the upstream parses is the one this code validated rather than whatever the client sent alongside it. Accept a gtag/js tag id only when it matches container_id or the new allowed_tag_ids, and answer any other id with a redirect to the upstream so the tag still loads for the visitor without this origin serving it. Upstream returns 200 for tag ids that do not exist, so it cannot be relied on to reject an unknown tag. Pin upstream fetches to https and to the configured hosts so a redirect cannot carry them somewhere else, and drop Set-Cookie, Strict-Transport-Security and Clear-Site-Data from responses that are passed through and served from the publisher's origin. Narrow the URL rewriter to the paths this integration routes, matching the allowlist the attribute rewriter already applied.
…tors Reject an upstream_url that embeds a username or password. The upstream is handed to the browser in the redirect served for an unconfigured tag, so anything embedded in it is disclosed to the client. Match the end of a rewritable URL by requiring that the next byte is not a path character, rather than by naming the terminators. The named set missed the ones nobody thought of: a routed path inside a template literal, ended by a backtick or an interpolation, stopped being rewritten.
Treating anything that is not a path character as the end of the URL still rewrote a longer path that merely starts with a routed one, because a path may continue with a percent-encoded byte or with `:` and `@`. `/collect%58YZ` became a first-party URL with no route behind it. Match on the delimiters that end a URL in the surrounding source instead. The extent of a URL here is set by the language it is embedded in, not by the RFC path grammar, so characters that are legal in a path can still end it. Listing the terminators also fails safe: an unrecognised byte leaves the URL third-party rather than pointing it at a route that does not exist.
A character class cannot tell a delimiter of the surrounding source from a character inside the URL: `;`, `,`, `&`, `$`, `'` and parentheses are all legal in a path. Treating them as terminators rewrote `/collect;matrix` as though it were the routed `/collect`, pointing it at a first-party URL with no route behind it. Anchor the match on the quote that opens the URL instead, and require the matching close, so the path must genuinely end where the routed path ends. A template literal may also be closed by an interpolation. An attribute value carries no surrounding source, so it is matched as a whole string. One consequence is that a fragment can no longer be rewritten on its own, since the delimiter that bounds the URL may be in the next chunk. Text is already reassembled before rewriting, so this only shows up when a chunk boundary falls inside the `google` prefix shared by both markers, which is the trade-off GTM_MIN_PREFIX_LEN already documents. Both outcomes now have a test.
An unlisted tag is not merely served third-party. A redirect does not make the upstream origin satisfy script-src 'self', so a strict policy that does not list googletagmanager.com refuses the redirected script and the tag does not run. Standard gtag installations use a product tag id distinct from the container, so most deployments loading gtag/js have one to add.
is_rewritable_url accepts a fragment, but the delimiter-anchored patterns allowed only a query, so a routed URL ending in `#...` was left third-party. A suffix may now begin with either `?` or `#` and is preserved verbatim. Drop the remaining claims that a redirected tag still loads for the visitor. They contradicted the upgrade note: a redirect does not make the upstream origin satisfy script-src 'self', so a policy that does not list it refuses the script. Name the configured upstream as the origin to allow rather than the default host, since a deployment may point upstream_url elsewhere. Point the rewriter's documentation at the patterns that replaced GTM_URL_PATTERN, which cargo doc reported as an unresolved link.
The template offers an upstream_url override four lines above, so telling an operator to allowlist googletagmanager.com is wrong for anyone who takes it. The paragraph above already states the rule, so the duplicate claim goes rather than being repeated correctly.
Collaborator
Author
|
Closes #1115 |
gtm.js substituted container_id for whatever the request named, so a page loading a second configured container silently got the first one and its tags never fired. Both script paths now share one decision: serve a configured tag as asked for, refuse anything else, and fall back to container_id when the request names no tag at all — upstream answers 200 for anything, so a request without a tag cannot be forwarded as-is. Require upstream_url to be a bare origin. Targets are built by appending to it, so `https://host#f` appended to the fragment and fetched the upstream homepage, which was then re-served as script. Allow the analytics host and its regional endpoints in the proxy pin, since the rewriter points at them and a redirect there was otherwise refused. Rewrite an origin with no path again. A script may hold the origin and concatenate the path later, and leaving it alone sent the beacon direct.
GA4 answers the beacon from regional endpoints, and the allowlist comment said they were permitted, but the pattern was built from the collect host. The regions are siblings of that host rather than subdomains, so the pattern matched nothing they use and every regional redirect failed the allowlist. Put the wildcard on the shared domain, and cover a regional host alongside lookalikes that must still be refused. A test asserted the wildcard had to end with the collect host, which held the narrower pattern in place; it now checks the domain.
Extending the tag allowlist to gtm.js made the container this origin serves a request parameter, which is not what container_id means and not what any operator-facing document describes: the guide, the example config and the endpoint's own note all say a client-supplied id is ignored there. The code was the outlier. Answer gtm.js with container_id and drop any id the request names, so the allowlist widens gtag/js alone, where it is documented. A second container should be a configuration change; if that is wanted on this endpoint it needs a setting of its own rather than one named for tags.
Three doc blocks had collapsed onto one resolver: the routing note that says which endpoint goes where and when the result is absent, the gtag/js allowlist note, and the container note. That left the routing text describing a private helper that never returns an absent result, and the gtag reasoning attached to the endpoint it does not apply to. Put each block on the function it describes.
prk-Jr
marked this pull request as ready for review
September 3, 2026 06:01
aram356
approved these changes
Sep 4, 2026
Comment on lines
+579
to
+584
| fn container_target(&self, base: &str, query: Option<&str>) -> GtmTarget { | ||
| GtmTarget::Proxy(Self::with_query( | ||
| base.to_owned(), | ||
| &Self::canonical_query(query, Some(&self.config.container_id)), | ||
| )) | ||
| } |
Collaborator
There was a problem hiding this comment.
🤔 thinking — is_rewritable_url does not consult the id param, so on a page running two GTM containers both gtm.js?id= URLs are rewritten first-party and the second container's request is answered with the first container's script: container 1 double-fires and container 2 silently never loads. gtag/js handles the analogous case with a redirect that keeps the tag alive third-party; here the substitution leaves no signal anywhere — script_target's redirect arm logs, container_target does not. Pinning is a defensible, documented choice, so no objection to the behavior itself, but a value-free warn gives operators a way to notice the misconfiguration. The suggestion deliberately does not echo the client-controlled id, matching the redirect log's practice.
Suggested change
| fn container_target(&self, base: &str, query: Option<&str>) -> GtmTarget { | |
| GtmTarget::Proxy(Self::with_query( | |
| base.to_owned(), | |
| &Self::canonical_query(query, Some(&self.config.container_id)), | |
| )) | |
| } | |
| fn container_target(&self, base: &str, query: Option<&str>) -> GtmTarget { | |
| if Self::requested_tag_id(query).is_some_and(|tag_id| tag_id != self.config.container_id) { | |
| // The id value is client-controlled, so it is not echoed here. | |
| log::warn!("Ignoring a gtm.js id that differs from the configured container"); | |
| } | |
| GtmTarget::Proxy(Self::with_query( | |
| base.to_owned(), | |
| &Self::canonical_query(query, Some(&self.config.container_id)), | |
| )) | |
| } |
(compile-verified only — cargo fmt --check, cargo clippy-fastly, and cargo check-fastly/check-axum/check-cloudflare pass with this applied; please re-run the matching cargo test-* aliases after applying)
Comment on lines
+83
to
+85
| /// The paths this integration actually routes, mirroring | ||
| /// [`GoogleTagManagerIntegration::is_rewritable_url`]. | ||
| /// The paths this integration routes, plus the empty path. |
Collaborator
There was a problem hiding this comment.
⛏ nitpick — Two stacked summary lines, an editing leftover — and the first one ("mirroring is_rewritable_url") is now inaccurate, since the pattern also routes the empty path which is_rewritable_url does not. Keep the second line only:
Suggested change
| /// The paths this integration actually routes, mirroring | |
| /// [`GoogleTagManagerIntegration::is_rewritable_url`]. | |
| /// The paths this integration routes, plus the empty path. | |
| /// The paths this integration routes, plus the empty path. |
| .ok() | ||
| .and_then(|upstream| upstream.host_str().map(str::to_owned)) | ||
| { | ||
| domains.push(host); |
Collaborator
There was a problem hiding this comment.
🌱 seedling — With the default upstream this list includes www.googletagmanager.com, but an operator pointing upstream_url at their own tagging domain removes it — so a custom upstream that 302s a script fetch to Google fails closed, with no setting to widen the fetch allowlist. Likely acceptable (failing closed is the design, and server-side tagging domains normally serve the script themselves), but worth confirming it is intended. Relatedly, proxy_config_pins_the_hosts_the_upstream_fetch_may_reach only covers the default upstream; a companion test asserting a custom upstream's host lands in the allowlist would pin this.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
is_host_permittedtreats as "any host", while redirects are followed by default.Operator note — read before upgrading
If your pages load
gtag/jswith a measurement id different from your container, add those ids to the newallowed_tag_ids.Nothing breaks if you miss one — the tag is answered with a redirect to Google and still loads for the visitor. But it is then served third-party, so it loses the ad-blocker resilience this integration exists to provide, and a
<link rel="preload">for it pays an extra round trip. Find the ids your pages use:This was confirmed against a live deployment: its pages preload two GA4 ids distinct from the container, and both fell back to the redirect until they were listed.
Behavior
gtm.js?id=<anything>container_id; other params (l,gtm_auth,gtm_preview,gtm_cookies_win) preservedgtag/js?id=<configured>gtag/js?id=<other>private, no-storegtag/js?ID=<a>&id=<b>/collect,/g/collectSet-Cookie/Strict-Transport-Security/Clear-Site-DataTag ids are matched exactly; a different capitalization is treated as unconfigured and redirected.
gtag/jsanswers200for ids that do not exist, so upstream cannot be relied on to reject an unknown tag.Changes
crates/trusted-server-core/src/integrations/google_tag_manager.rsbuild_target_urlreturns aGtmTarget(Proxy/Redirect);canonical_queryrebuilds the query so the validated id is the one sent;tag_id_is_allowedgatesgtag/js;redirect_to_upstreamserves the non-matching caseallowed_tag_idsconfig plusvalidate_allowed_tag_ids;validate_https_upstreamrequires an https URL with a literal (non-wildcard) hostbuild_proxy_configadds.with_https_only()and.with_allowed_domains(...), pinned to the configured upstream and the GA beacon hoststrip_upstream_first_party_headersapplied before any branch, so it covers passthrough and the upstream-error early returnGTM_URL_PATTERNnarrowed to the routed paths with a terminator guard; host literals consolidated onto constantshandlesplit intoempty_response,content_length_rejection,payload_size_status,rewritten_script_responsecrates/trusted-server-core/src/proxy.rsASSET_PROXY_STRIP_RESPONSE_HEADERSrenamed topub(crate) FIRST_PARTY_PASSTHROUGH_STRIP_HEADERSand shared, so the two passthrough paths cannot driftdocs/guide/integrations/google_tag_manager.mdallowed_tag_ids, the https requirement, exact matching, the redirect behavior, and an upgrade notetrusted-server.example.tomlallowed_tag_idswith guidance34 tests added, covering id clamping and parameter pollution (duplicate, case-varied and percent-encoded keys), the allowlist and exact matching, the redirect, host pinning, header stripping through
handle, the rewriter's routed-path and terminator behavior, and the config validators.Closes
Closes #1115
Test plan
cargo test-fastly && cargo test-axum(alsotest-cloudflare,test-spin)cargo clippy-fastly && cargo clippy-axum(alsoclippy-cloudflare,clippy-cloudflare-wasm,clippy-spin-native,clippy-spin-wasm,trusted-server-cli)cargo fmt --all -- --checkcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serve— verified id clamping, the allowlist before and after configuring real tag ids, exact matching, all three parameter-pollution shapes, the redirect chain through headless Chrome, and beacon passthroughChecklist
unwrap()in production codelogmacros (notprintln!)