Repository navigation
Match SDK URLs by vendor hostname and path - #1405
ChristianPavilonis wants to merge 5 commits into
Conversation
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Replaces substring matching in the Lockr and Permutive SDK URL matchers (Rust attribute rewriter and browser script guards) with parsed-hostname plus SDK-pathname checks, backed by an 83-case URL corpus shared between Vitest and Rust and an all-six-guard collision test. No correctness, WASM, security, or convention issues found; the remaining comments are small docs/test-readability improvements.
All 6 inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch.
Non-blocking
🏕 camp site
- New
### SDK URL rewritingheading splits Key Components — see inline atdocs/guide/integrations/lockr.md:151
📝 note
- Shorthand-scheme
insertionOwnerrows depend on jsdom'shttp:page — see inline atcrates/trusted-server-js/lib/test/integrations/lockr/url_collision.test.ts:60
⛏ nitpick
- Invisible U+FEFF / U+0085 in fixture — see inline at
crates/trusted-server-js/lib/test/fixtures/sdk-url-ownership.json:406-411 - Fixture path in Rust comments is JS-package-relative — see inline at
crates/trusted-server-core/src/integrations/sdk_url.rs:8,lockr.rs:102,permutive.rs:103
Cross-cutting / body-level findings
- 👍 Shared Rust/JS URL corpus and all-guard collision test — One JSON corpus drives the Rust matchers, the Rust
src/hrefrewrite assertions, and the browser raw-claimant,script.src, nativesetAttribute, and preload/prefetch checks in both normal and Lockr-first dispatch order, so the two implementations can't drift without a failing test. Passing the raw URL to each handler (instead of viascript.src) also catches the GPT-setter double-claim from #1199, and the spec documents the intentional browser-Lockr narrowing and the query/fragment recognition change.
CI Status
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- CodeQL: PASS
- Analyze (javascript-typescript): PASS
- cargo test (axum native): PASS
- Analyze (actions): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- vitest: PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- CLAUDE.md symlink guard: PASS
- cargo test: PASS (required)
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- Analyze (python): PASS
- Analyze (javascript-typescript): PASS
- cargo fmt: PASS (required)
- prepare integration artifacts: PASS
- Analyze (rust): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
| - Route: `/integrations/lockr/api/*` (GET, POST) proxies Lockr API calls | ||
| - Purpose: Keep SDK delivery and identity API traffic on the publisher origin | ||
|
|
||
| ### SDK URL rewriting |
There was a problem hiding this comment.
🏕 camp site — This ### heading is inserted in the middle of ### Key Components, between **Routed Endpoints** and **ID Mapping**, so **ID Mapping** and **Consent Validation** now render (and appear in the TOC) under "SDK URL rewriting". Using the same bold-label style as the other Key Components items keeps that subsection intact (moving the section below Consent Validation would also work, but those lines are outside the diff).
| ### SDK URL rewriting | |
| **SDK URL Rewriting**: |
| } | ||
|
|
||
| function expectedSrc({ url, owner, insertionOwner }: UrlCase): string { | ||
| // The DOM normalizes shorthand schemes before insertion, unlike raw matchers. |
There was a problem hiding this comment.
📝 note — The two shorthand-scheme insertionOwner rows only resolve to vendor URLs because jsdom's page is http://localhost:3000. WHATWG resolution of https:aim.loc.kr/identity-lockr.js against an http: base gives https://aim.loc.kr/identity-lockr.js, but against https://publisher.example.com/page it gives https://publisher.example.com/aim.loc.kr/identity-lockr.js (unowned). Runtime behaviour is fine (on a real HTTPS page the input stays a publisher URL), but this comment — and spec lines 122–123 — state it more generally than it holds, and those rows would flip if the test environment URL ever became https:. Spelling out the dependency:
| // The DOM normalizes shorthand schemes before insertion, unlike raw matchers. | |
| // jsdom's http: page resolves https: shorthand to an absolute vendor URL before insertion; | |
| // on an https: page the same input would resolve to a same-origin path and stay unowned. |
| "url": "https://aim.loc.kr/identity-lockr.js", | ||
| "owner": null | ||
| }, | ||
| { | ||
| "name": "Permutive NEL whitespace", | ||
| "url": " https://cdn.permutive.com/app-web.js", |
There was a problem hiding this comment.
⛏ nitpick — The BOM and NEL rows embed the raw U+FEFF / U+0085 characters, which are invisible in editors and in GitHub's diff (line 411 looks identical to a plain unowned Permutive URL). JSON \u escapes decode to the same strings in both serde_json and Vitest's JSON import, and make the boundary cases reviewable. Only lines 406 and 411 change; the range is one contiguous replacement.
| "url": "https://aim.loc.kr/identity-lockr.js", | |
| "owner": null | |
| }, | |
| { | |
| "name": "Permutive NEL whitespace", | |
| "url": "�https://cdn.permutive.com/app-web.js", | |
| "url": "\ufeffhttps://aim.loc.kr/identity-lockr.js", | |
| "owner": null | |
| }, | |
| { | |
| "name": "Permutive NEL whitespace", | |
| "url": "\u0085https://cdn.permutive.com/app-web.js", |
| /// Parse an HTTP(S) SDK URL with an explicit authority, or a protocol-relative URL. | ||
| /// | ||
| /// Trims ASCII whitespace and rejects relative paths and shorthand schemes. | ||
| /// Browser/Rust ownership cases live in test/fixtures/sdk-url-ownership.json. |
There was a problem hiding this comment.
⛏ nitpick — test/fixtures/sdk-url-ownership.json is relative to crates/trusted-server-js/lib/, not to this crate, so it's hard to find from trusted-server-core. Same pointer in lockr.rs:102 and permutive.rs:103 (separate suggestions there).
| /// Browser/Rust ownership cases live in test/fixtures/sdk-url-ownership.json. | |
| /// Browser/Rust ownership cases live in crates/trusted-server-js/lib/test/fixtures/sdk-url-ownership.json. |
| (lower.contains("aim.loc.kr") || lower.contains("identity.loc.kr")) | ||
| && lower.contains("identity-lockr") | ||
| && lower.ends_with(".js") | ||
| // Keep browser/Rust cases in test/fixtures/sdk-url-ownership.json aligned. |
There was a problem hiding this comment.
⛏ nitpick — Same as sdk_url.rs:8: use the repo-relative fixture path so it resolves from this crate.
| // Keep browser/Rust cases in test/fixtures/sdk-url-ownership.json aligned. | |
| // Keep browser/Rust cases in crates/trusted-server-js/lib/test/fixtures/sdk-url-ownership.json aligned. |
| let lower = url.to_ascii_lowercase(); | ||
| (lower.contains(".edge.permutive.app") || lower.contains("cdn.permutive.com")) | ||
| && lower.ends_with("-web.js") | ||
| // Keep browser/Rust cases in test/fixtures/sdk-url-ownership.json aligned. |
There was a problem hiding this comment.
⛏ nitpick — Same as sdk_url.rs:8: use the repo-relative fixture path so it resolves from this crate.
| // Keep browser/Rust cases in test/fixtures/sdk-url-ownership.json aligned. | |
| // Keep browser/Rust cases in crates/trusted-server-js/lib/test/fixtures/sdk-url-ownership.json aligned. |
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
No actionable findings in this review.
Reviewed all 13 changed files and traced URL parsing and hostname/path boundaries, server attribute rewriting and configuration-selected SDK destinations, browser DOM normalization, GPT assignment interception, all-six-guard ownership and ordering, and preload/prefetch handling. The documented query/fragment removal is intentional and consistent with the existing SDK routes.
Validation: full JS suite passed (2,279 tests across 51 files); focused guard/collision suite passed (1,126 tests); native Lockr and Permutive Rust tests passed (11 and 9); JS bundle build, cargo fmt, and diff whitespace checks passed. Local verification reused installed JS dependencies and freshly built bundles. No full local adapter/clippy matrix or live deployment smoke test was performed.
CI status at submission: no failed checks; several Rust, CLI, formatting, artifact-preparation, and analysis jobs remain pending. Vitest, Cloudflare checks, cross-adapter parity, TypeScript/docs formatting, and completed analysis checks pass. Approval covers the reviewed code; merge remains subject to completion of the required CI gates.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Parsed hostname and pathname matching for Lockr and Permutive is correct in both languages, and the shared corpus plus the six-guard collision test pin it well: 17 of 20 Rust and 15 of 16 JS matcher mutants fail the suite. A 40,000-input Rust/JS differential fuzz, cross-checked in Chrome, found no raw URL where the server claims something the browser would not load as the SDK. The changes requested are a broken pending fixture suggestion, a vacuous href assertion, stale spec text, a missing CHANGELOG entry, an unpinned host boundary, a follow-up for encoded attribute values, and an outdated PR description.
All 5 inline comments carry a one-click GitHub
suggestion: use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. Each was applied alone to4df289f67and checked: Prettier, Vitest and the Rust corpus tests for the fixture;cargo fmt, all eight clippy aliases, the five adapter test aliases and parity for the Rust helper; docs Prettier for the spec. The body-level findings below touch files or lines outside the diff, so they need manual edits.
Blocking
🔧 wrench
- Pending escape suggestion on the BOM/NEL rows breaks the fixture: see inline at
crates/trusted-server-js/lib/test/fixtures/sdk-url-ownership.json:406-411 hrefhalf ofassert_sdk_url_rewritenever reacheshandles_attribute: see inline atcrates/trusted-server-core/src/integrations/sdk_url_test_support.rs:45- Spec names a retired toolchain: see inline at
docs/superpowers/specs/2026-10-16-1199-sdk-url-ownership-design.md:202 - Spec names the reviewing tool and model: see inline at
docs/superpowers/specs/2026-10-16-1199-sdk-url-ownership-design.md:219 - Spec records failures that #1326 already fixed: see inline at
docs/superpowers/specs/2026-10-16-1199-sdk-url-ownership-design.md:250-258
Cross-cutting / body-level findings
-
🔧 CHANGELOG entry missing: This changes operator-visible behavior. The browser Lockr guard stops proxying non-SDK scripts on
aim.loc.kr, and SDK URLs with a query string or fragment start being proxied on the server and in the browser. Recent fixes (#1238, #1221, #1223, #1209) each added an[Unreleased]entry. Proposed under### Fixed(apply manually,CHANGELOG.mdis outside the diff):- Lockr and Permutive SDK URLs are now matched by parsed hostname and SDK pathname in the server attribute rewriter and the browser script guards (#1199). The browser Lockr guard no longer replaces scripts that only mention `aim.loc.kr`, such as a GPT or GTM loader carrying it in the query, a lookalike host, or a non-SDK script served from `aim.loc.kr`. SDK URLs with a query string or fragment are now proxied through `/integrations/lockr/sdk` and `/integrations/permutive/sdk`; the query and fragment are not forwarded upstream.
-
🔧 Lockr's exact-host boundary is unpinned on the suffix side: Replacing the Lockr host check with
host.ends_with("aim.loc.kr") || host.ends_with("identity.loc.kr")inlockr.rspasses every Rust test, and the same change inlockr/script_guard.tspasses every Vitest test. The "Lockr host substring" row (notaim.loc.kr.example.com) checks a prefix, not a suffix. A corpus row can't close this:aim.loc.krandidentity.loc.krare exact-match hosts in the URL-host allowlist, and JSON can't carry anallow-domaincomment. Direct assertions with a trailing suppression can, followinggoogle_tag_manager.rs:1953. Each snippet below kills its mutant and passescargo fmtor Prettier (apply manually, both land outside the diff).In
test_lockr_sdk_url_detectionincrates/trusted-server-core/src/integrations/lockr.rs:// Hosts that only end with a Lockr SDK host are different hosts. assert!( !integration.is_lockr_sdk_url("https://notaim.loc.kr/identity-lockr.js"), // allow-domain: notaim.loc.kr "should require an exact aim.loc.kr hostname" ); assert!( !integration.is_lockr_sdk_url("https://notidentity.loc.kr/identity-lockr.js"), // allow-domain: notidentity.loc.kr "should require an exact identity.loc.kr hostname" );
In the
appendChild interceptionblock ofcrates/trusted-server-js/lib/test/integrations/lockr/script_guard.test.ts:it.each([ 'https://notaim.loc.kr/identity-lockr.js', // allow-domain: notaim.loc.kr 'https://notidentity.loc.kr/identity-lockr.js', // allow-domain: notidentity.loc.kr ])('should require an exact Lockr hostname for %s', (url) => { installLockrGuard(); const script = document.createElement('script'); script.setAttribute('src', url); document.head.appendChild(script); expect(script.getAttribute('src')).toBe(url); script.remove(); });
-
🔧 Server-side SDK recognition is narrower than what the browser loads: The Rust matchers see HTML attribute text with character references still encoded, because lol_html 2.9.0's
get_attribute(html_processor.rs:462) does not decode them; browsers do. Measured with lol_html 2.9.0 against this head:src="https://aim.loc.kr/identity-lockr-trust-server.js"(mustache.js escapes/this way) was rewritten by the old substring matcher and is now missed, so the browser loads the vendor SDK directly. The same applies tocdn.permutive.com. This is a narrow regression from this PR.src="https://static.example.com?@aim.loc.kr/identity-lockr.js"is claimed by both the old and new matchers (userinfo plus anaim.loc.krhost), although the browser loadshttps://static.example.com/?@aim.loc.kr/identity-lockr.js. This predates the PR.
GPT and Sourcepoint already
Url::parsethe same undecoded text, so decoding attribute values once inhtml_processorbefore the rewriters run would fix every integration. Please open a follow-up issue for that.Raw spellings that browsers still resolve to the vendor host also miss on the server: leading C0 controls outside the trimmed set, tab/CR/LF inside the scheme or between the slashes, backslash slashes, and
http:hostshorthand on an https page. The browser guard still claims these on dynamic insertion because it sees the resolvedscript.src. Two more splits come from the URL parsers rather than this code: Node and Chrome accept invalid punycode labels such ashttps://xn--example.edge.permutive.app/ws-web.jsthat the url crate and jsdom reject, and only Chrome decodes a fullwidth％2Ein a host. Please note these accepted fail-open forms under "Input forms" in the spec, so the contract matches the behavior. -
🔧 PR description is out of date: The Changes table omits
crates/trusted-server-core/src/integrations/sdk_url.rs,sdk_url_test_support.rs,mod.rsandcrates/trusted-server-js/lib/src/shared/sdk_url.ts, which the second commit added. The "Known baseline failure" section no longer applies: #1326 fixed it, and the cited test passes at4df289f67.
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
| "url": "https://aim.loc.kr/identity-lockr.js", | ||
| "owner": null | ||
| }, | ||
| { | ||
| "name": "Permutive NEL whitespace", | ||
| "url": " https://cdn.permutive.com/app-web.js", |
There was a problem hiding this comment.
🔧 wrench: Don't commit the earlier escape suggestion on these lines. Its NEL row reads \^E, not \u0085, and \^ is not a JSON escape. With it applied, the Rust corpus loader panics with should parse shared SDK URL ownership corpus: Error("invalid escape", line: 411, column: 14), so both Rust corpus tests lose every row, not just this one. JSON.parse rejects it as well (Bad escaped character in JSON at position 2).
This version makes both boundary characters visible and decodes to the same strings: all 83 rows compare equal after JSON.parse. Applied alone, it passes Prettier, the Lockr Vitest files (1,106 tests), and the Lockr and Permutive Rust tests (20).
| "url": "https://aim.loc.kr/identity-lockr.js", | |
| "owner": null | |
| }, | |
| { | |
| "name": "Permutive NEL whitespace", | |
| "url": "�https://cdn.permutive.com/app-web.js", | |
| "url": "\ufeffhttps://aim.loc.kr/identity-lockr.js", | |
| "owner": null | |
| }, | |
| { | |
| "name": "Permutive NEL whitespace", | |
| "url": "\u0085https://cdn.permutive.com/app-web.js", |
| } else { | ||
| AttributeRewriteAction::Keep | ||
| }; | ||
| for (attribute, element) in [("src", "script"), ("href", "link")] { |
There was a problem hiding this comment.
🔧 wrench: The href iteration repeats the src check. rewrite() ignores the attribute name and context, while the registry routes href through handles_attribute() (registry.rs:1078), which nothing in this helper calls. Changing either integration's handles_attribute to matches!(attribute, "src") still passes all 2,982 core lib tests.
With this assertion, both mutants fail (should route href to the lockr rewriter, should route href to the permutive rewriter). Applied alone, it passes cargo fmt, all eight clippy aliases, the five adapter test aliases, and the parity suite.
| for (attribute, element) in [("src", "script"), ("href", "link")] { | |
| for (attribute, element) in [("src", "script"), ("href", "link")] { | |
| assert!( | |
| integration.handles_attribute(attribute), | |
| "should route {attribute} to the {} rewriter", | |
| integration.integration_id() | |
| ); |
| npx vitest run test/integrations/lockr test/integrations/permutive test/shared/dom_insertion_dispatcher.test.ts | ||
| ``` | ||
|
|
||
| Focused checks from the repository root, using the pinned Rust 1.95 toolchain |
There was a problem hiding this comment.
🔧 wrench: The pinned toolchain has been 1.98.1 since #1123. Dropping the version keeps this sentence true across future bumps.
| Focused checks from the repository root, using the pinned Rust 1.95 toolchain | |
| Focused checks from the repository root, using the pinned Rust toolchain |
|
|
||
| ## Independent review | ||
|
|
||
| A fresh Pi process using `openai-codex/gpt-6.1-sol` reviewed this spec, the full |
There was a problem hiding this comment.
🔧 wrench: This is the only spec under docs/superpowers/specs/ that names the tool and model that reviewed it. The name goes stale and tells a later reader nothing about the design.
| A fresh Pi process using `openai-codex/gpt-6.1-sol` reviewed this spec, the full | |
| A fresh-context reviewer read this spec, the full |
| An additional full native-core run, `cargo test -p trusted-server-core`, had | ||
| 2,940 passing tests and 12 pre-existing legacy settings-test failures. Cargo | ||
| injects `TRUSTED_SERVER__GIT_VERSION`, which the legacy environment settings | ||
| loader rejects as the unknown field `git_version`. This was reproduced in an | ||
| untouched archive of `5bc388625` with | ||
| `cargo test -p trusted-server-core settings::tests::test_origin_host_header_override_env -- --exact`. | ||
| No settings or version-handling changes are included here. The required Fastly | ||
| core suite and focused native integration tests pass. | ||
|
|
There was a problem hiding this comment.
🔧 wrench: #1326 fixed these legacy settings failures, and it is this branch's merge base. At 4df289f67, cargo test -p trusted-server-core --lib settings::tests::test_origin_host_header_override_env -- --exact passes and the full core lib run is 2,982/2,982. This removes the paragraph and its trailing blank line.
| An additional full native-core run, `cargo test -p trusted-server-core`, had | |
| 2,940 passing tests and 12 pre-existing legacy settings-test failures. Cargo | |
| injects `TRUSTED_SERVER__GIT_VERSION`, which the legacy environment settings | |
| loader rejects as the unknown field `git_version`. This was reproduced in an | |
| untouched archive of `5bc388625` with | |
| `cargo test -p trusted-server-core settings::tests::test_origin_host_header_override_env -- --exact`. | |
| No settings or version-handling changes are included here. The required Fastly | |
| core suite and focused native integration tests pass. |
Summary
Changes
crates/trusted-server-core/src/integrations/lockr.rscrates/trusted-server-core/src/integrations/permutive.rscrates/trusted-server-js/lib/src/integrations/lockr/script_guard.tscrates/trusted-server-js/lib/src/integrations/permutive/script_guard.tscrates/trusted-server-js/lib/test/fixtures/sdk-url-ownership.jsoncrates/trusted-server-js/lib/test/integrations/lockr/url_collision.test.tsdocs/guide/integrations/lockr.mddocs/guide/integrations/permutive.mddocs/superpowers/specs/2026-10-16-1199-sdk-url-ownership-design.mdCloses
Closes #1199
Test plan
cargo test-fastly,cargo test-fastly-reuse,cargo test-axum,cargo test-cloudflare,cargo test-spinAGENTS.mdcargo fmt --all -- --checkKnown baseline failure
An additional
cargo test -p trusted-server-corerun had 2,940 passing tests and 12 failing legacy settings tests. Cargo injectsTRUSTED_SERVER__GIT_VERSION, which that legacy environment loader rejects as the unknown fieldgit_version. The same failure was reproduced in an untouched archive of base commit5bc388625with:cargo test -p trusted-server-core settings::tests::test_origin_host_header_override_env -- --exactThis is outside the URL-matching change. No settings/version workaround or test exclusion was added. The required adapter/CI gates and focused native integration tests pass. Existing ignored tests remain ignored.
Checklist
unwrap()calls, logging, dependencies, or dispatcher changes