Repository navigation
Keep Lockr API routing first-party without polling - #1406
ChristianPavilonis wants to merge 6 commits into
Conversation
Use the dedicated SDK's native API routing instead of polling for and mutating its internal global. Preserve publisher-origin XHR routing under external base URLs with a narrowly scoped API guard, and cover initializer and transport behavior with regression tests.
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Removes the Lockr shim that polled for and mutated the SDK's private identityLockr global, and replaces it with a narrowly scoped XMLHttpRequest.open guard that keeps root-relative /integrations/lockr/api requests on the page origin under an external <base href>. Clean, well-tested change; no blocking issues.
Non-blocking
🤔 thinking
- Guard covers XHR only;
fetch/sendBeaconwould still follow<base href>— see inline atcrates/trusted-server-js/lib/src/integrations/lockr/api_guard.ts:28 - Migration note omits the
rewrite_sdk = false+ legacy SDK case — see inline atdocs/guide/configuration.md:1659
👍 praise
- Tight URL matching with boundary and argument-preservation tests — see inline at
crates/trusted-server-js/lib/src/integrations/lockr/api_guard.ts:24 - Removes private-global polling and mutation — see inline at
crates/trusted-server-js/lib/src/integrations/lockr/index.ts:7
Cross-cutting / body-level findings
- 📌 Static SDK
<script>tags still resolve against an external<base href>— The server-side attribute rewrite incrates/trusted-server-core/src/integrations/lockr.rs:397-405replaces a static Lockr SDKsrcwith root-relative/integrations/lockr/sdk, and its comment calls resolution against<base href>an accepted tradeoff. On external-base pages the SDK itself is therefore requested from the base origin, so the new API guard only helps once the SDK loads. Dynamically inserted tags are already absolute (window.location.origin + proxyPathinshared/script_guard.ts). Worth a follow-up to make the HTML rewrite absolute too, or to document the limitation, so the external-base guarantee holds end to end. - 📝 PR description is stale — "Remaining validation" says "Keep this PR in draft pending those checks", but the PR is not a draft and all remote CI gates (including parity and adapter tests) now pass. Consider updating that caveat.
CI Status
- format-docs: PASS (required)
- format-typescript: PASS (required)
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- vitest: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (actions): PASS
- Analyze (python): PASS
- Analyze (javascript-typescript): PASS
- CLAUDE.md symlink guard: PASS
| /** | ||
| * Keep root-relative Lockr API requests on the page origin, even with an external base URL. | ||
| */ | ||
| export function installLockrApiGuard(): void { | ||
| if (uninstall || typeof window === 'undefined' || typeof window.XMLHttpRequest === 'undefined') { | ||
| return; | ||
| } | ||
|
|
||
| const prototype = window.XMLHttpRequest.prototype; | ||
| const originalOpen = prototype.open; | ||
| const open: typeof originalOpen = function ( | ||
| this: XMLHttpRequest, | ||
| ...args: [ | ||
| method: string, | ||
| url: string | URL, | ||
| async?: boolean, | ||
| username?: string | null, | ||
| password?: string | null, | ||
| ] | ||
| ): void { | ||
| const url = args[1]; | ||
| if (typeof url === 'string' && /^\/integrations\/lockr\/api(?:[/?#]|$)/.test(url)) { | ||
| args[1] = `${window.location.origin}${url}`; | ||
| } | ||
| Reflect.apply(originalOpen, this, args); | ||
| }; |
There was a problem hiding this comment.
🤔 thinking — The external-base fix relies on the current SDK sending every API call through XMLHttpRequest.open. If an SDK update moves any endpoint (page-view beacons are a common candidate) to fetch or navigator.sendBeacon, those requests would silently resolve against <base href> again, with no test or log to catch it. Can you confirm that all current SDK API calls (settings, page-view, generate-tokens, revoke-consent) use XHR? If so, consider recording the dependency in the doc comment:
/**
* Keep root-relative Lockr API requests on the page origin, even with an external base URL.
*
* Only `XMLHttpRequest.open` is guarded: the trust-server SDK issues every API call via XHR.
* Requests made with `fetch` or `navigator.sendBeacon` are not covered and would still
* resolve against a document `<base href>`.
*/| Trusted Server no longer rewrites the SDK API host. Custom `sdk_url` builds must | ||
| route their own API requests through the first-party proxy; builds that depended | ||
| on the removed SDK-host shim need migration. |
There was a problem hiding this comment.
🤔 thinking — This note names only custom sdk_url builds. With rewrite_sdk = false, a page that loads a non-trust-server Lockr SDK (e.g. identity-lockr-v*.js) directly from Lockr keeps that SDK, since the server substitutes the configured sdk_url only when the tag is rewritten. The removed shim used to point that SDK's API host back at the proxy (at least for SDKs exposing identityLockr); now those API calls go straight to Lockr, a third party, with no warning. Consider covering that path in the migration note:
Trusted Server no longer rewrites the SDK API host. Custom `sdk_url` builds, and
pages that load a non-trust-server Lockr SDK directly with `rewrite_sdk = false`,
must route their own API requests through the first-party proxy; builds that
depended on the removed SDK-host shim need migration.| ] | ||
| ): void { | ||
| const url = args[1]; | ||
| if (typeof url === 'string' && /^\/integrations\/lockr\/api(?:[/?#]|$)/.test(url)) { |
There was a problem hiding this comment.
👍 praise — The anchored match ^\/integrations\/lockr\/api(?:[/?#]|$) correctly rejects /apiary, /api-v2, relative, protocol-relative, and absolute URLs, and the tests cover each of those boundaries. Reflect.apply keeps the receiver, sync flag, credentials, and native argument validation intact, and resetApiGuardState won't clobber a later interceptor. Pinning to location.origin also stops an injected <base href> from redirecting identity API traffic.
| import { installLockrApiGuard } from './api_guard'; | ||
| import { installLockrGuard } from './script_guard'; | ||
|
|
||
| // Type definition for Lockr global | ||
| declare const identityLockr: IdentityLockr | undefined; | ||
|
|
||
| interface IdentityLockr { | ||
| host: string; | ||
| app_id: string; | ||
| expiryDateKeys: string[]; | ||
| firstPartyCookies: string[]; | ||
| canRefreshToken: boolean; | ||
| macroDetectionEnabled: boolean; | ||
| iluiMacroDetection: boolean; | ||
| gdprApplies: boolean; | ||
| consentString: string; | ||
| gppString: string; | ||
| ccpaString: string; | ||
| isUTMTagsLoaded: boolean; | ||
| isFirstPartyCookiesLoaded: boolean; | ||
| allowedUTMTags: string[]; | ||
| lockrTrackingID: string; | ||
| panoramaClientId: string; | ||
| writeToDeviceConsentEUID: boolean; | ||
| id5JSEnabled: boolean; | ||
| firstIDPassHEM: boolean; | ||
| panoramaPassHEM: boolean; | ||
| firstIDEnabled: boolean; | ||
| panoramaEnabled: boolean; | ||
| isAdelphicEnabled: boolean; | ||
| os: string; | ||
| browser: string; | ||
| country: string; | ||
| city: string; | ||
| latitude: string; | ||
| longitude: string; | ||
| ip: string; | ||
| hashedUserAgent: string; | ||
| tokenMappings: Record<string, string>; | ||
| tokenSourceMappings: Record<string, string>; | ||
| identitProvidersType: Record<string, string>; | ||
| identityIdEncryptionSalt: string; | ||
| } | ||
|
|
||
| /** | ||
| * Install the Lockr shim to rewrite API endpoints to first-party domain. | ||
| * This function is called after the Lockr SDK has loaded and initialized. | ||
| */ | ||
| function installLockrShim() { | ||
| log.info('Installing Lockr shim - rewriting API host to first-party domain'); | ||
|
|
||
| if (typeof identityLockr === 'undefined' || !identityLockr) { | ||
| log.warn('Lockr shim: identityLockr global not found'); | ||
| return; | ||
| } | ||
|
|
||
| const host = window.location.host; | ||
| const protocol = window.location.protocol === 'https:' ? 'https' : 'http'; | ||
|
|
||
| // Store original host for debugging | ||
| const originalHost = identityLockr.host; | ||
|
|
||
| // Rewrite to first-party domain | ||
| // The Lockr SDK will now make all API calls through our proxy | ||
| identityLockr.host = `${protocol}://${host}/integrations/lockr/api`; | ||
|
|
||
| log.info('Lockr shim installed', { | ||
| originalHost, | ||
| newHost: identityLockr.host, | ||
| appId: identityLockr.app_id, | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Wait for Lockr SDK to be available before installing shim. | ||
| * Polls for SDK availability with a maximum number of attempts. | ||
| * | ||
| * @param callback - Function to call when SDK is available | ||
| * @param maxAttempts - Maximum number of polling attempts (default: 50) | ||
| */ | ||
| function waitForLockrSDK(callback: () => void, maxAttempts = 50) { | ||
| let attempts = 0; | ||
|
|
||
| const check = () => { | ||
| attempts++; | ||
|
|
||
| // Check if identityLockr global exists and is initialized with host | ||
| if (typeof identityLockr !== 'undefined' && identityLockr && identityLockr.host) { | ||
| log.info('Lockr SDK detected, installing shim'); | ||
| callback(); | ||
| } else if (attempts < maxAttempts) { | ||
| // Check again in 50ms | ||
| setTimeout(check, 50); | ||
| } else { | ||
| log.warn('Lockr SDK not detected after', maxAttempts * 50, 'ms'); | ||
| } | ||
| }; | ||
|
|
||
| check(); | ||
| } | ||
|
|
||
| if (typeof window !== 'undefined') { | ||
| installLockrApiGuard(); | ||
| installLockrGuard(); | ||
|
|
||
| waitForLockrSDK(() => installLockrShim()); | ||
| } |
There was a problem hiding this comment.
👍 praise — Nice cleanup: this drops ~100 lines that depended on an undocumented SDK internal (a 50×50 ms polling loop and spurious timeout warnings) in favour of side-effect-free guard installation. The new index.test.ts pins that down (no timers, no warnings, untouched identityLockr.host and public window.lockr).
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The change removes Lockr SDK polling and private-host mutation while preserving first-party API routing for root-relative XHR requests, including pages with an external base URL. No actionable findings.
Validation: all 67 focused Lockr tests passed under Node 24.12.0; checked routing boundaries, argument and receiver forwarding, initializer lifecycle, and existing script interception.
CI Status
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- vitest: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- format-typescript: PASS (required)
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo test: PASS (required)
- cargo fmt: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- format-docs: PASS (required)
- docs-proposal scripts: PASS
- CodeQL: PASS
- prepare integration artifacts: PASS
- Analyze (javascript-typescript): PASS
- CLAUDE.md symlink guard: PASS
- Analyze (actions): PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- Analyze (python): PASS
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Removes the Lockr SDK polling and private-host mutation, and adds an XMLHttpRequest.open guard that keeps root-relative /integrations/lockr/api requests on the page origin under an external <base href>. The guard is correct for the SDK as served today and works in a real browser. Requesting changes for a missing CHANGELOG entry, a receiver assertion that can't fail, and three follow-through items below.
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 findings are in the body because they touch files outside the diff.
Blocking
🔧 wrench
- Receiver check is structural, so it passes for any XHR — see inline at
crates/trusted-server-js/lib/test/integrations/lockr/api_guard.test.ts:82
Cross-cutting / body-level findings
-
🔧 CHANGELOG entry missing — This PR changes browser behavior on every Lockr-enabled page (a global
XMLHttpRequest.prototype.openwrapper) and removes the SDK-host rewrite that the newdocs/guide/configuration.mdparagraph says some setups "need migration" from. Operators readCHANGELOG.mdon upgrade, not the configuration reference, and recent user-facing PRs (#1238, #1223, #1221) each added an[Unreleased]entry.CHANGELOG.mdisn't in the diff, so this can't be a one-click suggestion. Proposed entry under## [Unreleased]/### Changed:- The Lockr browser module no longer polls for the SDK's internal `identityLockr` global or rewrites its API host, which also removes the "Lockr SDK not detected" timeout warning (#1247). It now wraps `XMLHttpRequest.prototype.open` so root-relative `/integrations/lockr/api` requests resolve against the page origin, keeping Lockr API traffic first-party on pages with an external `<base href>`. Setups that relied on the removed host rewrite, such as a custom `sdk_url` build or a non-trust-server Lockr SDK loaded directly with `rewrite_sdk = false`, must now route their API requests through `/integrations/lockr/api` themselves.
-
🔧 Record the SDK's XHR-only dependency in
api_guard.ts— On the open question in dhruv8sh'sapi_guard.ts:28thread: I checked the trust-server SDK served at the defaultsdk_url(identity-lockr-trust-server.js, 118,542 bytes, fetched 2026-10-22). It declaresconst identityLockr = {host: …}at script top level, which confirms the PR description's correction of #1247. All 9 reads ofidentityLockr.hostarexhr.open(method, host + path), and the SDK's string table has nofetchorsendBeaconfragments, so XHR-only coverage handles every API call the SDK makes today. In Chrome, with the builttsjs-lockr.jsloaded ahead of<base href="https://external-base.example/assets/">, a root-relative settings POST stayed on the page origin (200); without the bundle it went toexternal-base.example. The dependency is real but invisible in the code: please add the doc comment proposed in that thread, so an SDK move tofetchorsendBeaconis a documented gap rather than a silent regression. -
🔧 No committed browser coverage for the external-base path — The jsdom tests assert the arguments forwarded to
open, not where the request actually goes, and the external-base proof in the description comes from a local harness that isn't in the repo. The browser integration suite has no Lockr scenario. A browser test that loads the Lockr bundle ahead of an external<base href>and fires a root-relative XHR from a stub SDK would pin the behavior; the probe above (two static pages and a request log) is small enough to adapt. If that's too much for this PR, file it as a follow-up and link it here. -
🔧 Linked Lockr guide documents config that doesn't exist — The new paragraph ends with "See [lockr]", but
docs/guide/integrations/lockr.mddocumentsorganization_idandproject_idfields thatLockrConfigdoesn't have, showsenableddefaulting tofalse(the code defaults totrue), and never mentionsapp_idorsdk_url. Rewriting it is out of scope here, but there's no open issue for it; please file a follow-up and link it from this PR.
CI Status
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- CodeQL: PASS
- Analyze (actions): PASS
- Analyze (rust): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (javascript-typescript): PASS
- cargo test (cross-adapter parity): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- vitest: PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- prepare integration artifacts: PASS
- cargo test: PASS (required)
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (axum native): PASS
- format-docs: PASS (required)
- docs-proposal scripts: PASS
- format-typescript: PASS (required)
- Analyze (javascript-typescript): PASS
- cargo fmt: PASS (required)
- Analyze (python): PASS
- CLAUDE.md symlink guard: PASS
| 'user', | ||
| 'password' | ||
| ); | ||
| expect(open.mock.contexts).toEqual([request]); |
There was a problem hiding this comment.
🔧 wrench — Receiver check is structural, so it passes for any XHR. toEqual compares structure, and two jsdom XMLHttpRequest instances are structurally equal. A mutant that forwards to a fresh instance (Reflect.apply(originalOpen, new XMLHttpRequest(), args)) passes this test on the PR head. An identity check fails that mutant and passes as-is (Lockr tests 67/67, Prettier and ESLint clean).
| expect(open.mock.contexts).toEqual([request]); | |
| expect(open.mock.contexts[0]).toBe(request); |
Summary
XMLHttpRequest.open, including pages with an external<base href>.The issue's diagnosis needed correcting: a top-level lexical
identityLockris accessible to another classic script without being awindowproperty. The old shim does run for the current SDK. Publicwindow.lockrhas methods but no API host, so renaming the polling target would not preserve the old behavior.Changes
crates/trusted-server-js/lib/src/integrations/lockr/index.tscrates/trusted-server-js/lib/src/integrations/lockr/api_guard.tscrates/trusted-server-js/lib/test/integrations/lockr/index.test.tscrates/trusted-server-js/lib/test/integrations/lockr/api_guard.test.tsdocs/guide/configuration.mdThe API guard addresses a regression caught during local browser verification of the initial deletion: under an external document base, the SDK's relative API host resolves to that base origin. The old shim's absolute host prevented that. The guard preserves the destination without restoring polling or depending on an SDK private global.
SDK payload serving, Rust proxy behavior,
fetch, and SDK URL matching are unchanged. Issue #1199 remains separate. Custom SDK builds that relied on mutation of their internal host still need an explicit routing contract; this does not promise generic legacy-SDK adaptation.Closes
Closes #1247
Test plan
api_guard.tscargo fmt --all -- --checkcargo build --locked -p trusted-server-adapter-axum -j 8ts dev proxyplus installed-Chrome harness acceptance, initial load and reloadgit diff --cached --checkandts dev lint domains --stagedThe final local browser run passed in 80.148 seconds with zero artifact capture errors. It verified the exact newly built Lockr module in both served bundles, SDK/settings/page-view HTTP 200 on both navigations, public SDK methods and one ID5 EID source, preserved script interception, and no Lockr detection warnings. The API host itself stays relative; the new XHR guard pins request resolution to the page origin. Raw artifacts remain local and are not attached because they may contain credentials and identity values.
Remaining validation
Full adapter/Rust/parity CI gates have not all been run locally. Keep this PR in draft pending those checks and final review. Local Axum browser verification is Lockr-focused, not a reproduction of the entire production ad stack; the native environment lacks production KV/Geo behavior. The run also captured one page JavaScript error and two existing direct vendor image ORB failures. The original production timeout trigger and generic custom/legacy SDK compatibility remain unverified. No all-vendor-traffic or whole-site regression claim is intended.
Checklist