Skip to content

Centralize DOM insertion guards#460

Open
prk-Jr wants to merge 8 commits intomainfrom
fix/centralize-dom-insertion-dispatcher
Open

Centralize DOM insertion guards#460
prk-Jr wants to merge 8 commits intomainfrom
fix/centralize-dom-insertion-dispatcher

Conversation

@prk-Jr
Copy link
Collaborator

@prk-Jr prk-Jr commented Mar 7, 2026

Summary

  • Centralize SDK DOM insertion interception behind a single shared dispatcher so integrations no longer stack independent appendChild / insertBefore wrappers.
  • Move GPT onto the shared dispatcher and harden its proxy path so dynamic GPT loads remain first-party without broadening upstream encoding negotiation.
  • Add coordination and reset coverage so SPA re-inits, test runs, and future integrations do not accumulate prototype patches.

Changes

File Change
crates/common/src/integrations/gpt.rs Switched GPT asset fetching to the shared proxy helper, preserved caller Accept-Encoding, updated bootstrap comments, and refreshed proxy tests/fixtures.
crates/js/lib/src/shared/dom_insertion_dispatcher.ts Added the runtime-global DOM insertion dispatcher that owns the single appendChild / insertBefore patch, orders handlers deterministically, and restores baselines safely.
crates/js/lib/src/shared/script_guard.ts Refactored shared script guards to register dispatcher handlers instead of patching DOM prototypes directly.
crates/js/lib/src/integrations/gpt/script_guard.ts Moved GPT Layer 5 onto the shared dispatcher and coordinated reset through handler unregistering.
crates/js/lib/src/integrations/gpt/index.ts Made GPT shim activation robust when the inline enable flag executes before the unified bundle.
crates/js/lib/src/integrations/datadome/script_guard.ts Updated DataDome to the new shared guard API and aligned guard comments with dispatcher behavior.
crates/js/lib/src/integrations/google_tag_manager/script_guard.ts Updated GTM to the new shared guard API with stable integration identity.
crates/js/lib/src/integrations/lockr/nextjs_guard.ts Updated Lockr to the new shared guard API and aligned guard comments with dispatcher behavior.
crates/js/lib/src/integrations/permutive/script_guard.ts Updated Permutive to the new shared guard API with stable integration identity.
crates/js/lib/test/shared/dom_insertion_dispatcher.test.ts Added shared-dispatcher coverage for single patch ownership, ordering, reset coordination, external patch ownership, handler isolation, and repeated install/reset cycles.
crates/js/lib/test/integrations/gpt/index.test.ts Added coverage for GPT auto-install when the enable flag is set before module evaluation.
crates/js/lib/test/integrations/gpt/script_guard.test.ts Updated GPT guard tests for dispatcher-based reset behavior and realistic versioned GPT asset paths.
crates/js/lib/test/integrations/datadome/script_guard.test.ts Updated DataDome guard tests to assert reset restores shared prototype baselines without manual prototype cleanup.
crates/js/lib/test/integrations/google_tag_manager/script_guard.test.ts Updated GTM guard tests to assert reset restores shared prototype baselines without manual prototype cleanup.
crates/js/lib/test/integrations/lockr/nextjs_guard.test.ts Updated Lockr guard tests to assert reset restores shared prototype baselines without manual prototype cleanup.

Closes

Closes #402

Test plan

  • cargo test --workspace
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo fmt --all -- --check
  • JS tests: cd crates/js/lib && npx vitest run
  • JS format: cd crates/js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM build: cargo build --bin trusted-server-fastly --release --target wasm32-wasip1
  • Manual testing via fastly compute serve
  • Other: cd crates/js/lib && npm run build

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses tracing macros (not println!)
  • New code has tests
  • No secrets or credentials committed

@prk-Jr prk-Jr self-assigned this Mar 7, 2026
Copy link
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

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

Summary

Well-executed refactoring that replaces N independent appendChild/insertBefore prototype patches with a single shared dispatcher. The JS design is sound and thoroughly tested. The Rust-side GPT proxy rewrite introduces two behavioral changes that need attention before merge.

Blocking

🔧 wrench

  • Redirect following widens proxy trust boundary: build_proxy_config inherits follow_redirects: true, allowing the GPT proxy to follow up to 4 redirects to any host. The old code was single-hop. GPT static assets don't redirect, so follow_redirects = false is the correct setting. (gpt.rs:128)
  • Error handling semantics changed silently: Old code returned Err on non-2xx upstream status; new code passes through the response body. This is arguably better behavior but is a semantic change that should be documented or reverted. (gpt.rs:160)

Non-blocking

🤔 thinking

  • Vary header normalization dropped: Old vary_with_accept_encoding logic removed; relying on upstream Google CDN to always send correct Vary headers. Low practical risk but removes a defensive layer. (gpt.rs:147)
  • Privacy-scrubbing relies on header set-last-wins: copy_proxy_forward_headers forwards Referer/X-Forwarded-For before with_header overrides blank them. Correct today, fragile if the proxy helper ever appends instead of sets. (gpt.rs:134)
  • Stale dispatcher state replaced without teardown: Version mismatch path overwrites the global without restoring old prototype wrappers. No production risk (single page load), but can stack wrappers in HMR/dev. (dom_insertion_dispatcher.ts:81)

♻️ refactor

  • handle JSDoc ambiguous about node insertion: "Consumed" implies the node might not be inserted, but the dispatcher always inserts. Clarify that return value only controls handler chain short-circuiting. (dom_insertion_dispatcher.ts:28)

👍 praise

  • Single shared prototype patch: The core dispatcher design eliminates wrapper stacking, makes ordering deterministic, and the test coverage (priority ordering, external patch preservation, repeated cycles, error resilience) is thorough.
  • GPT auto-install on pre-set flag: Closes a real race condition between inline bootstrap and bundle evaluation. Both orderings tested.
  • Test cleanup delegated to dispatcher teardown: Tests validate that teardown works rather than masking broken cleanup with manual prototype restoration.

CI Status

  • cargo fmt: PASS
  • cargo test: PASS
  • vitest: PASS
  • format-typescript: PASS
  • format-docs: PASS
  • CodeQL: PASS

Copy link
Collaborator

@ChristianPavilonis ChristianPavilonis left a comment

Choose a reason for hiding this comment

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

Key items to address:

  • Keep proxy redirect handling constrained in crates/common/src/integrations/gpt.rs (do not broadly follow redirects without explicit allow-listing/trust guarantees).
  • Clarify and stabilize GPT proxy error semantics in crates/common/src/integrations/gpt.rs (the non-2xx passthrough change needs either rollback or explicit documentation/tests).
  • Restore explicit cache/header handling in crates/common/src/integrations/gpt.rs (Vary normalization and deterministic privacy header scrubbing should not rely on set order).
  • In crates/js/lib/src/shared/dom_insertion_dispatcher.ts, ensure stale dispatcher replacement does best-effort teardown in version mismatch paths and tighten handle() JSDoc return semantics.

Once these are addressed (or explicitly accepted with rationale), I am happy to re-review.

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.

Multiple global prototype patches stack without coordination

3 participants