Fix Sourcepoint HTML asset rewriting#793
Conversation
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Solid, well-tested change. Adds root-relative HTML asset rewriting so Sourcepoint privacy-manager iframes load their CSS/JS through the first-party proxy, plus two default-off, documented debug flags (debug_clear_state_on_load, debug_force_banner_visible). Security hygiene is strong: SSRF-pinned cdn_origin, cookie allowlist, 5 MB body cap, and Accept-Encoding: identity preflight. Every new branch is exercised by a test.
No blocking findings — all comments below are non-blocking observations and follow-up suggestions. Verdict: comment.
None of the findings are expressed as one-click GitHub suggestion blocks: the debug-tooling items are design judgment calls (better left to the author than auto-patched with invented constants), and the one refactor spans two diff hunks so it can't anchor to a single range.
Non-blocking
🤔 thinking / ♻️ refactor / 📝 note
- HTML rewrite preflight asymmetry — see inline at
crates/trusted-server-core/src/integrations/sourcepoint.rs:998 debug_clear_statematcher misses allowlisted cookies — see inline atcrates/trusted-server-core/src/integrations/sourcepoint.rs:654- HTML asset regex also matches inline JS /
data-*— see inline atcrates/trusted-server-core/src/integrations/sourcepoint.rs:121 debug_force_bannerleaves a permanent MutationObserver — see inline atcrates/trusted-server-core/src/integrations/sourcepoint.rs:719- New TS type fields are never read — see inline at
crates/js/lib/src/integrations/sourcepoint/index.ts:9
Cross-cutting / body-level findings
- ♻️ Duplicate body-guard logic between the JS and HTML rewrite branches — the content-length cap check +
collect_response_bounded+String::from_utf8decode block atsourcepoint.rs:937-989andsourcepoint.rs:1002-1054is ~50 lines of near-identical code. Extracting aread_rewritable_body(response, path, forwarded_cookies)helper that returns either the decoded(parts, String)or the pass-throughResponsewould remove the duplication. Apply manually — it spans two hunks and restructures the early returns, so it can't be a single-range suggestion. - 🌱 Compile-time gating for the debug flags — both flags are runtime config, so a misconfigured
trusted-server.tomlcan ship the debug scripts to production despite the doc warning. A#[cfg(debug_assertions)]or env gate would make accidental production enablement impossible. Future consideration, not for this PR. - 👍 Praise — SSRF pinning of
cdn_originto the exact host with path/query/fragment rejection (and the rationale comment) is excellent; redirectLocationrewriting, cookie-safety cache policy, andVary: Accept-Encodingstripping are all correct and tested; debug-script ordering (clear-state before force-visible, both relative to the_sp_trap) is explicitly covered.
CI Status
- cargo fmt: PASS
- Analyze (rust): PASS
- cargo test: PASS
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- CodeQL: PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
- integration tests: PASS
- integration tests (EdgeZero entry point): PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
| if method == Method::GET | ||
| && response.status() == StatusCode::OK | ||
| && self.config.rewrite_sdk | ||
| && Self::is_html_response(&response) |
There was a problem hiding this comment.
🤔 thinking — HTML rewrite fires on is_html_response (Content-Type), but the Accept-Encoding: identity preflight that makes the body readable fires on is_likely_html_path (.html / .htm only, see line 862-865). A privacy-manager served from an extensionless directory path (e.g. /us_pm/) that returns text/html would arrive gzip-compressed → String::from_utf8 fails → silent pass-through with no rewrite.
The PR's documented target path (/us_pm/index.html) ends in .html, so it works today; this only bites extensionless entry points. There's no corruption (header + body stay consistent on the utf8-fail path), just a silent no-op. Worth a follow-up — either decompress the body or broaden the path heuristic so the preflight set matches the rewrite set.
| "if(window.console&&console.warn)console.warn(\"Sourcepoint debug_clear_state_on_load enabled; clearing Sourcepoint consent state\");", | ||
| "function m(n){", | ||
| "n=String(n||\"\").toLowerCase();", | ||
| "return n.indexOf(\"_sp\")!==-1||n.indexOf(\"sp_\")!==-1||n.indexOf(\"sourcepoint\")!==-1||n.indexOf(\"consent\")!==-1||n.indexOf(\"gpp\")!==-1||n.indexOf(\"usnat\")!==-1||n.indexOf(\"privacy\")!==-1||n.indexOf(\"ccpa\")!==-1;", |
There was a problem hiding this comment.
🤔 thinking — This substring matcher (_sp / sp_ / sourcepoint / consent / gpp / usnat / privacy / ccpa) does not match dnsDisplayed, signedLspa, or globalcmpUUID — all three are in SOURCEPOINT_COOKIE_ALLOWLIST. A tool that "clears Sourcepoint consent state" therefore leaves those cookies/keys behind. Conversely it does match Trusted Server's own __gpp / _ts_gpp_src (likely intended for a full reset).
Not prescribing an exact token set — which names count as "Sourcepoint state" is your call — but flagging the drift between this matcher and the allowlist so the debug reset is intentional rather than accidental. Debug-only, so non-blocking.
| /// those paths must keep flowing through `/integrations/sourcepoint/cdn` rather | ||
| /// than resolving against the publisher origin root. | ||
| static SP_ROOT_RELATIVE_ASSET_ATTR_PATTERN: LazyLock<Regex> = LazyLock::new(|| { | ||
| Regex::new(r#"(?i)(\b(?:src|href)\s*=\s*)(['"])(/[^/'"][^'"]*)(['"])"#) |
There was a problem hiding this comment.
🤔 thinking — This pattern matches any src= / href= in the response body, including inline-script assignments like el.src="/foo" and attributes like data-src="/foo" (the \b lets src match right after data-). For the constrained Sourcepoint privacy-manager HTML that's acceptable, but it's the usual fragility tradeoff of regex-based HTML rewriting — a non-asset data-href or a JS string literal could be rewritten unintentionally.
Protocol-relative (//…) and fragment (#…) values are correctly excluded by (/[^/'"]…), and that's covered by tests. Note only — no change required for this PR.
| "for(var i=0;i<muts.length;i++){", | ||
| "for(var j=0;j<muts[i].addedNodes.length;j++)scan(muts[i].addedNodes[j]);", | ||
| "}", | ||
| "}).observe(document.documentElement,{childList:true,subtree:true});", |
There was a problem hiding this comment.
📝 note — The polling loop self-terminates after 80 × 250 ms (~20 s), but this MutationObserver on document.documentElement with subtree: true runs for the lifetime of the page. Since it's debug-only that's tolerable, but consider disconnecting it when the polling window ends (e.g. call observer.disconnect() in the same branch as clearInterval(timer)) so the debug flag doesn't leave a permanent whole-document observer running.
| debugForceBannerVisible?: boolean; | ||
| debugClearStateOnLoad?: boolean; |
There was a problem hiding this comment.
🤔 thinking — debugForceBannerVisible and debugClearStateOnLoad are added to the __tsjs_sourcepoint window type, but no TypeScript reads them — the entire debug behavior is implemented by the Rust-injected inline scripts. That's fine as config-shape documentation, but worth confirming it's intentional rather than a half-wired feature (e.g. behavior that was meant to live in script_guard.ts but ended up Rust-side). If they're purely documentation, a short comment saying so would prevent a future reader from assuming the TS layer consumes them.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Fixes the Sourcepoint privacy-manager iframe 404s by rewriting root-relative src/href asset URLs in proxied CMP HTML through /integrations/sourcepoint/cdn/..., plus two debug-only injected scripts and docs. The change is well-tested and reuses the existing JS-rewrite safety envelope (bounded read, missing-Content-Length skip, non-UTF-8 passthrough, cookie-safe caching). All CI is green.
Requesting changes to address the findings below before merge. None are correctness-critical, but they cover robustness (over-broad rewrite, extensionless paths), duplication, and two dead/scope notes.
Non-blocking
🤔 thinking / ♻️ refactor / 📝 note
- Over-broad root-relative
href/srcrewrite — see inline atsourcepoint.rs:121 - Duplicate rewrite boilerplate between JS and HTML blocks — see inline at
sourcepoint.rs:995 - Extensionless HTML paths silently skip rewriting — see inline at
sourcepoint.rs:863 - New TS debug type fields are never read — see inline at
index.ts:8 - Absolute CDN URLs in HTML are not first-partied — see inline at
sourcepoint.rs:521
CI Status
- integration tests (EdgeZero entry point): PASS
- browser integration tests: PASS
- integration tests: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- vitest: PASS
- Analyze (javascript-typescript): PASS
- cargo test: PASS (required)
- format-docs: PASS (required)
- prepare integration artifacts: PASS
- format-typescript: PASS (required)
- cargo fmt: PASS (required)
- Analyze (actions): PASS
| /// those paths must keep flowing through `/integrations/sourcepoint/cdn` rather | ||
| /// than resolving against the publisher origin root. | ||
| static SP_ROOT_RELATIVE_ASSET_ATTR_PATTERN: LazyLock<Regex> = LazyLock::new(|| { | ||
| Regex::new(r#"(?i)(\b(?:src|href)\s*=\s*)(['"])(/[^/'"][^'"]*)(['"])"#) |
There was a problem hiding this comment.
🤔 thinking — This pattern matches every root-relative src/href in the CMP document, not just assets. A privacy-manager navigation link like <a href="/privacy-policy"> becomes <a href="/integrations/sourcepoint/cdn/privacy-policy">, which 404s through the proxy. The rewrites_html_root_relative_assets_to_cdn_proxy_prefix test only exercises <a href="#privacy"> (a fragment, correctly skipped) — it never asserts the root-relative <a> case, which is exactly the risky one.
Real-world impact is low (CMP HTML rarely uses root-relative anchors), so this isn't blocking, but consider either scoping the match to asset contexts (<link> / <script>), or at minimum adding a test that pins the behavior for a root-relative <a href> so a future reader knows it's intentional.
Apply manually — the right fix is a judgment call, not a single-line rewrite.
| // Rewrite root-relative asset URLs inside Sourcepoint HTML apps so | ||
| // privacy-manager iframes can load their CSS and JavaScript through the | ||
| // first-party CDN proxy. | ||
| if method == Method::GET |
There was a problem hiding this comment.
♻️ refactor — This HTML block is a near-verbatim copy of the JavaScript rewrite block just above (the Content-Length match arms, collect_response_bounded, and the String::from_utf8 passthrough fallback are identical). Extracting a shared helper — e.g. read_rewritable_body(response, path) -> Option<String> that returns the decoded body or None when the response should pass through untouched — would remove ~45 duplicated lines and keep the two paths from drifting.
Apply manually — spans two blocks and adds a helper, so it can't be a single-hunk suggestion.
| // API responses, CSS) keep the client's original Accept-Encoding for | ||
| // efficiency. | ||
| if self.config.rewrite_sdk | ||
| && (Self::is_likely_javascript_path(target_path) |
There was a problem hiding this comment.
🤔 thinking — is_likely_html_path only matches .html/.htm, so a privacy-manager served at a directory path (e.g. /us_pm/) won't get Accept-Encoding: identity here. The upstream then returns a gzipped body, is_html_response is still true (Content-Type text/html), String::from_utf8 fails on the gzip bytes, and the response passes through unmodified — the asset-404 bug this PR fixes would silently reappear for that URL shape.
Fine if Sourcepoint always uses an .html suffix (as this PR assumes). Worth a code comment noting the assumption, or broadening the detection if extensionless app URLs are possible.
| type SourcepointWindow = Window & { | ||
| __tsjs_sourcepoint?: { | ||
| rewriteSdk?: boolean; | ||
| debugForceBannerVisible?: boolean; |
There was a problem hiding this comment.
📝 note — These two fields are added to the SourcepointWindow type and the Rust config script now serializes both into window.__tsjs_sourcepoint, but no TS code reads them — the only consumer (shouldInstallSourcepointGuard) reads rewriteSdk only. Both debug behaviors are implemented entirely by the Rust-injected inline scripts. Keeping the type in sync with the emitted JSON is defensible as documentation; flagging so it's a conscious choice rather than a leftover from a planned-but-unwired TS consumer.
| /// `/us_pm/index.html`, but the HTML points at root-relative assets such as | ||
| /// `/PrivacyManagerUS.b9d1f.css`. Without rewriting, browsers request those | ||
| /// assets from the publisher origin root and receive 404s. | ||
| fn rewrite_html_content(content: &str) -> String { |
There was a problem hiding this comment.
📝 note — rewrite_html_content intentionally handles only root-relative paths, so an absolute https://cdn.privacy-mgmt.com/foo.css inside the app HTML would load directly from the third-party CDN (works, but not first-party). The JS-body rewriter covers absolute URLs for .js; the HTML path has no equivalent. In scope for this PR (which targets root-relative 404s) — just noting the residual gap for follow-up.
|
Superseded by #822 |
3 participants
Summary
Changes
crates/trusted-server-core/src/integrations/sourcepoint.rssrc/hrefroot-relative assets to/integrations/sourcepoint/cdn/...; adds debug-onlydebug_clear_state_on_loadanddebug_force_banner_visibleconfig and injector scripts; adds unit coverage.crates/js/lib/src/integrations/sourcepoint/index.tsdocs/guide/integrations/sourcepoint.mdCloses
Closes #792
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serve/integrations/sourcepoint/cdn/...; verified debug flags expose config and force-created banner containers visible locally.Checklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)