Repository navigation
Conversation
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Approved. Using trusted platform geography across Didomi SDK requests and limiting caching to canonical notice loaders addresses the spoofed-header and geo-cache concerns. The compatibility and rollout changes are documented clearly.
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Solid fix for #1151: every Didomi SDK request now derives X-Geo-Country / X-Geo-Region / CloudFront-Viewer-Country only from normalized trusted platform geo (or omits the whole group), and only canonical notice loaders keep outbound caching. The trust boundary is correct and the regression matrix is thorough. Remaining comments are non-blocking: one design question about browser-side no-store on static SDK assets, a small observability addition, and two nits.
1 of the inline comments below carries a one-click GitHub
suggestion— use Commit suggestion to apply it as a commit on the PR branch. The remaining comments describe the change in prose because it spans several call sites or lines outside the diff and can't be auto-applied.
Non-blocking
🤔 thinking / 🏕 camp site / ⛏ nitpick
- 🤔 Browser
no-storeon every non-loader SDK asset goes beyond the shared-cache threat — see inline atcrates/trusted-server-core/src/integrations/didomi.rs:502 - 🏕 Log when a non-canonical SDK request omits geo headers — see inline at
crates/trusted-server-core/src/integrations/didomi.rs:447 - ⛏
Ok(None)arm in the canonical-loader match is unreachable — see inline atcrates/trusted-server-core/src/integrations/didomi.rs:435
👍 praise
- Non-canonical SDK regression matrix — see inline at
crates/trusted-server-core/src/integrations/didomi.rs:971
Cross-cutting / body-level findings
- ⛏ Two untouched test names no longer describe the behavior they cover —
disabled_geo_preserves_existing_loader_behavior(didomi.rs:919) still passes because it only asserts the upstream URL, but the disabled-option loader no longer keeps its existing behavior: it now bypasses the outbound cache and is madeno-store, private.sdk_responses_preserve_origin_cache_metadata(didomi.rs:1351) now only holds for canonical loaders; non-canonical SDK responses have their cache metadata stripped. Consider renaming them todisabled_geo_proxies_loader_without_geo_queryandcanonical_loader_responses_preserve_origin_cache_metadata. (Lines are outside the diff, so no inline suggestion.)
CI Status
- integration tests: PASS
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- CodeQL: PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (cross-adapter parity): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (rust): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (axum native): PASS
- cargo test: PASS (required)
- vitest: PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- cargo fmt: PASS (required)
- docs-proposal scripts: PASS
- Analyze (javascript-typescript): PASS
- CLAUDE.md symlink guard: PASS
- prepare integration artifacts: PASS
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
aram356
left a comment
There was a problem hiding this comment.
Summary
The trust boundary is right: every Didomi geo header now comes from platform geo or is omitted, and caller-supplied headers never reach Didomi. The cache change goes too far. Under the default config, every SDK file bypasses the edge cache and is no-store in the browser, including static chunks Didomi marks cacheable for a year. Didomi's reverse-proxy contract says it throttles and disables proxies that do this.
1 of the inline comments below carries a one-click GitHub
suggestion. Use Commit suggestion to apply it as a commit on the PR branch. The remaining comments describe the fix in prose because it spans several files or lines outside the diff and can't be auto-applied.
Blocking
🔧 wrench
- Static SDK files lose all caching, which breaks Didomi's reverse-proxy contract — see inline at
crates/trusted-server-core/src/integrations/didomi.rs:492 - Point front-door deployments at trusted client-IP forwarding — see inline at
docs/guide/integrations/didomi.md:207 - No CHANGELOG entry for a breaking security change — see body-level findings below
❓ question
- How do geo-less loaders stay within Didomi's query-frequency rule? — see inline at
docs/guide/integrations/didomi.md:219
Non-blocking
⛏ nitpick
- Session notes in committed docs — see inline at
docs/superpowers/specs/2026-10-19-didomi-trusted-geo-design.md:53
Cross-cutting / body-level findings
-
🔧 No CHANGELOG entry for a breaking security change —
CHANGELOG.md[Unreleased]records operator-facing compatibility changes as Breaking entries and security fixes under### Security. This PR stops honoringFastlyGeo-*headers, requires a cache purge, and changes the Didomi SDK cache policy, but none of it reaches the release notes. Suggested### Securityentry, with the cache sentence adjusted to wherever the caching comments land (apply manually;CHANGELOG.mdis outside the diff):- Didomi SDK requests now derive `X-Geo-Country`, `X-Geo-Region`, and `CloudFront-Viewer-Country` only from trusted platform geo (#1151). Caller-supplied `FastlyGeo-CountryCode` and `FastlyGeo-Region` headers were previously copied into them, letting a caller choose the geography Didomi used. **Breaking** for deployments that injected those headers: configure CDN-fronted client IP instead. Without a complete trusted country/region pair (always on Cloudflare, Axum, and Spin) the group is omitted. Geo-less loader requests bypass the outbound cache and return `no-store, private`; purge cached Didomi SDK entries before rollout.
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- Analyze (rust): PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- CLAUDE.md symlink guard: PASS
- CodeQL: PASS
- docs-proposal scripts: PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
These checks ran before the Rust 1.98.1 upgrade (#1123) landed on main. Re-verified locally on this head merged with main at f9281917a: the merge is clean, and 36/36 Didomi tests and cargo clippy-fastly pass.
Summary
Didomi SDK requests now derive
X-Geo-Country,X-Geo-Region, andCloudFront-Viewer-Countryonly from a normalized trusted platform country/region pair. Caller-suppliedFastlyGeo-*and destination geo headers cannot select Didomi's geography; unavailable or invalid trusted geo omits the complete header group outside canonical loaders.Canonical notice loaders retain their redirects, fail-closed geo validation, and origin caching. Other SDK GETs, including static core/UI/text files, retain normal outbound caching and all origin cache metadata. Noncanonical loaders (including root and nested
/loader.jspaths), SDK POSTs, and API requests bypass the outbound cache and return terminalno-store, privateresponses. API requests perform no geo lookup.Closes #1151.
Review fixes
Deployment constraints
Noncanonical loaders still contact Didomi for every request and risk automatic throttling/disablement under its reverse-proxy requirements. For production Fastly use, validate complete trusted geo coverage and enable
geo_query_parameters = trueon the supported notice-loader path. Cloudflare, Axum, and Spin currently have no compliant loader-caching mode through this proxy; disable the integration and restore direct Didomi SDK delivery instead. CDN-fronted deployments must configure trusted client-IP forwarding.Purge previously cached SDK entries before rollout. Fresh browser entries require expiry or a cache-busting URL change.
Validation
Release producer checks are unavailable because this branch has no
scripts/release/directory. No live Fastly deployment or CDN smoke test was performed.