Skip to content

Use trusted geography for all Didomi SDK requests - #1414

Open
prk-Jr wants to merge 6 commits into
mainfrom
fix-didomi-trusted-geo
Open

prk-Jr wants to merge 6 commits into
mainfrom
fix-didomi-trusted-geo

Conversation

@prk-Jr

@prk-Jr prk-Jr commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Didomi SDK requests now derive X-Geo-Country, X-Geo-Region, and CloudFront-Viewer-Country only from a normalized trusted platform country/region pair. Caller-supplied FastlyGeo-* 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.js paths), SDK POSTs, and API requests bypass the outbound cache and return terminal no-store, private responses. API requests perform no geo lookup.

Closes #1151.

Review fixes

  • Restore origin caching for static SDK assets, with regression coverage across geo options and trusted geo availability. Retain spoofed-header security coverage for those assets.
  • Log omitted SDK geo at debug level, remove the unreachable geo match arm, and correct two stale test names.
  • Add the breaking security release note, trusted client-IP migration link, cache purge guidance, and explicit loader throttling constraints. Update the guide, design, and implementation plan together; remove inaccessible chat approval and stale worktree handoff notes.

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 = true on 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

  • New static SDK regression fails before the cache fix and passes afterward; all 38 focused Didomi tests pass.
  • All eight adapter/CLI/codegen clippy aliases, Rust formatting, native build-digest tests/lint, and WASM clock lint pass.
  • Fastly (Viceroy 0.21.0), reusable-sandbox, Axum, Cloudflare, Spin, and 17 adapter parity tests pass. Host CLI tests and enabled browser fixtures also pass.
  • JS build, 1,211 tests across 50 files, JS/docs/repository Markdown formatting, and documentation proposal checks pass using Node 24.12.0.
  • Added-line domain lint passes; the optional repository-wide audit reports existing violations.
  • Independent review of the complete PR diff found two documentation gaps, both corrected; no further confirmed runtime findings. The committed PR head merges cleanly with current main.

Release producer checks are unavailable because this branch has no scripts/release/ directory. No live Fastly deployment or CDN smoke test was performed.

@prk-Jr prk-Jr self-assigned this Oct 19, 2026

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 dhruv8sh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-store on every non-loader SDK asset goes beyond the shared-cache threat — see inline at crates/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 at crates/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 made no-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 to disabled_geo_proxies_loader_without_geo_query and canonical_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

Comment thread crates/trusted-server-core/src/integrations/didomi.rs Outdated
Comment thread crates/trusted-server-core/src/integrations/didomi.rs
Comment thread crates/trusted-server-core/src/integrations/didomi.rs Outdated
Comment thread crates/trusted-server-core/src/integrations/didomi.rs

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 honoring FastlyGeo-* headers, requires a cache purge, and changes the Didomi SDK cache policy, but none of it reaches the release notes. Suggested ### Security entry, with the cache sentence adjusted to wherever the caching comments land (apply manually; CHANGELOG.md is 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.

Comment thread crates/trusted-server-core/src/integrations/didomi.rs
Comment thread docs/guide/integrations/didomi.md Outdated
Comment thread docs/guide/integrations/didomi.md Outdated
Comment thread docs/superpowers/specs/2026-10-19-didomi-trusted-geo-design.md Outdated

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace caller-supplied Didomi geo headers with trusted platform geo

4 participants