Repository navigation
Match SDK URLs by vendor hostname and path - #1405
ChristianPavilonis wants to merge 8 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
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
…ing regressions, and apply accepted documentation and fixture cleanup.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Round 2 at f3fa6b752. The round-1 fixes landed and hold up: the fixture escapes are correct, the handles_attribute and exact-host assertions now kill the mutants that survived round 1, and the three spec corrections are in. The broader browser-style normalization is sound: 13 of 15 Rust and 10 of 11 JS mutants of the new parsing code fail the suite (each survivor is behaviorally equivalent, apart from the lol_html helper below), and a 59,788-input differential fuzz found no ownership disagreement between Rust and JS, with a Chrome spot check matching jsdom byte for byte. Two round-1 requests (CHANGELOG, PR description) are still open without a reply. The new lol_html helper cannot fail and skips the one corpus row that served HTML gets wrong.
5 of the 7 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 tof3fa6b752and checked (Prettier and docs Prettier, ESLint, Vitest, the Rust corpus tests, and for the two Rust-affecting onescargo fmt, all eight clippy aliases, the five adapter test aliases, and parity). The other two describe the change in prose because it spans several files or needs a decision.
Blocking
🔧 wrench
- lol_html helper can't fail and hides a server-claimed corpus row: see inline at
crates/trusted-server-core/src/integrations/sdk_url_test_support.rs:75 - Server HTML limits are undocumented and untracked: see inline at
docs/superpowers/specs/2026-10-16-1199-sdk-url-ownership-design.md:55 - Internal NUL rejection is stricter than HTML: see inline at
crates/trusted-server-core/src/integrations/sdk_url.rs:12 - Tab/CR/LF removal allocates on every call: see inline at
crates/trusted-server-core/src/integrations/sdk_url.rs:3-17 - CR removal is pinned only by a Lockr row: see inline at
crates/trusted-server-js/lib/test/fixtures/sdk-url-ownership.json:445-448 replaceAllneeds a newer lib than the tsconfig: see inline atcrates/trusted-server-js/lib/test/shared/sdk_url.test.ts:44- Spec records a local timeout note: see inline at
docs/superpowers/specs/2026-10-16-1199-sdk-url-ownership-design.md:278-280
Cross-cutting / body-level findings
-
🔧 CHANGELOG entry still missing: Round 1 asked for this and it is still absent;
mainhas added entries since (#1291, #1252, #1222, #1413). This round also widened what the server recognizes. 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. Server-side recognition applies the browser's URL preprocessing: leading and trailing C0 controls and spaces are trimmed, tab, CR, and LF are removed, and backslashes are accepted as authority separators.
-
🔧 PR description is still out of date: It reports an 83-case corpus, 1,068 collision checks, and 2,279 JS tests; the spec now records 105 cases, 1,376 checks, and 2,605 tests. The Changes table omits
crates/trusted-server-core/src/integrations/sdk_url.rs,sdk_url_test_support.rs,mod.rs,crates/trusted-server-js/lib/src/shared/sdk_url.ts,test/shared/sdk_url.test.ts, andtest/integrations/lockr/script_guard.test.ts. The "Known baseline failure" section no longer applies: #1326 fixed it before round 1.
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)
- HTML EOF runtime (Fastly): PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
| /// # Panics | ||
| /// | ||
| /// Panics if HTML processing fails or the SDK attribute is not replaced. | ||
| pub(super) fn assert_sdk_html_attribute_rewrite( |
There was a problem hiding this comment.
🔧 wrench: This helper can't fail for the current corpus, and the row it skips is one that served HTML gets wrong. lol_html's get_attribute returns the attribute text as written, and the helper only runs for owned rows, none of which contains ", a character reference, or a leading BOM. Passing &case.url instead of &value to rewrite, or skipping the call (if matches → if false), still passes all 20 Lockr and Permutive tests.
lol_html does change one thing: it drops a leading U+FEFF while decoding the value. So "Lockr BOM whitespace", which the corpus marks unowned, is rewritten to /integrations/lockr/sdk in served HTML, while a browser keeps the BOM and resolves a publisher path. Running the helper for every row shows it:
assertion `left == right` failed: should apply expected HTML src rewrite for Lockr BOM whitespace
left: "<script src=\"/integrations/lockr/sdk\"></script>"
right: "<script src=\"\u{feff}https://aim.loc.kr/identity-lockr.js\"></script>"
This predates the PR, but the corpus row and the spec currently claim the opposite. The patch below runs the helper for every row (expecting the markup unchanged for unowned rows) and records that row's server outcome in a serverHtmlOwner field. With it, the Rust corpus tests (20), Vitest (1,432), ESLint, and Prettier pass, and the &case.url mutant now fails on that row. Apply manually: it touches four files.
diff --git a/crates/trusted-server-core/src/integrations/lockr.rs b/crates/trusted-server-core/src/integrations/lockr.rs
--- a/crates/trusted-server-core/src/integrations/lockr.rs
+++ b/crates/trusted-server-core/src/integrations/lockr.rs
@@ -487,13 +487,11 @@ mod tests {
);
assert_sdk_url_rewrite(integration.as_ref(), &case, "/integrations/lockr/sdk");
- if matches {
- assert_sdk_html_attribute_rewrite(
- integration.as_ref(),
- &case,
- "/integrations/lockr/sdk",
- );
- }
+ assert_sdk_html_attribute_rewrite(
+ integration.as_ref(),
+ &case,
+ "/integrations/lockr/sdk",
+ );
}
}
diff --git a/crates/trusted-server-core/src/integrations/permutive.rs b/crates/trusted-server-core/src/integrations/permutive.rs
--- a/crates/trusted-server-core/src/integrations/permutive.rs
+++ b/crates/trusted-server-core/src/integrations/permutive.rs
@@ -513,13 +513,11 @@ mod tests {
);
assert_sdk_url_rewrite(integration.as_ref(), &case, "/integrations/permutive/sdk");
- if matches {
- assert_sdk_html_attribute_rewrite(
- integration.as_ref(),
- &case,
- "/integrations/permutive/sdk",
- );
- }
+ assert_sdk_html_attribute_rewrite(
+ integration.as_ref(),
+ &case,
+ "/integrations/permutive/sdk",
+ );
}
}
diff --git a/crates/trusted-server-core/src/integrations/sdk_url_test_support.rs b/crates/trusted-server-core/src/integrations/sdk_url_test_support.rs
--- a/crates/trusted-server-core/src/integrations/sdk_url_test_support.rs
+++ b/crates/trusted-server-core/src/integrations/sdk_url_test_support.rs
@@ -7,6 +7,7 @@ use super::{AttributeRewriteAction, IntegrationAttributeContext, IntegrationAttr
/// One raw URL and its expected integration owner from the shared corpus.
#[derive(Deserialize)]
+#[serde(rename_all = "camelCase")]
pub(super) struct SdkUrlCase {
/// Descriptive case name for assertion failures.
pub(super) name: String,
@@ -14,6 +15,8 @@ pub(super) struct SdkUrlCase {
pub(super) url: String,
/// Expected integration ID, or none for an unowned URL.
pub(super) owner: Option<String>,
+ /// Owner once lol_html has read the attribute, when that differs from `owner`.
+ pub(super) server_html_owner: Option<String>,
}
/// Load the URL ownership cases used by both Rust and browser tests.
@@ -67,16 +70,18 @@ pub(super) fn assert_sdk_url_rewrite(
}
}
-/// Assert that a matching SDK URL is rewritten after HTML attribute tokenization.
+/// Assert the SDK rewrite of a corpus URL after lol_html reads it from markup.
///
/// # Panics
///
-/// Panics if HTML processing fails or the SDK attribute is not replaced.
+/// Panics if HTML processing fails or the output disagrees with the server-side owner.
pub(super) fn assert_sdk_html_attribute_rewrite(
integration: &dyn IntegrationAttributeRewriter,
case: &SdkUrlCase,
sdk_path: &str,
) {
+ let owner = case.server_html_owner.as_ref().or(case.owner.as_ref());
+ let owned = owner.map(String::as_str) == Some(integration.integration_id());
for (attribute, tag) in [("src", "script"), ("href", "link")] {
let html = format!(
"<{tag} {attribute}=\"{}\"></{tag}>",
@@ -96,23 +101,25 @@ pub(super) fn assert_sdk_html_attribute_rewrite(
let value = element
.get_attribute(attribute)
.expect("should read the SDK URL attribute");
- assert_eq!(
- integration.rewrite(attribute, &value, &context),
- AttributeRewriteAction::Replace(sdk_path.to_string()),
- "should rewrite HTML {attribute} for {}",
- case.name
- );
- element.set_attribute(attribute, sdk_path)?;
+ if let AttributeRewriteAction::Replace(next) =
+ integration.rewrite(attribute, &value, &context)
+ {
+ element.set_attribute(attribute, &next)?;
+ }
Ok(())
})],
..RewriteStrSettings::default()
},
)
.expect("should process SDK URL markup");
+ let expected = if owned {
+ format!("<{tag} {attribute}=\"{sdk_path}\"></{tag}>")
+ } else {
+ html
+ };
assert_eq!(
- output,
- format!("<{tag} {attribute}=\"{sdk_path}\"></{tag}>"),
- "should replace the SDK URL in HTML for {}",
+ output, expected,
+ "should apply expected HTML {attribute} rewrite for {}",
case.name
);
}
diff --git a/crates/trusted-server-js/lib/test/fixtures/sdk-url-ownership.json b/crates/trusted-server-js/lib/test/fixtures/sdk-url-ownership.json
--- a/crates/trusted-server-js/lib/test/fixtures/sdk-url-ownership.json
+++ b/crates/trusted-server-js/lib/test/fixtures/sdk-url-ownership.json
@@ -404,7 +404,8 @@
{
"name": "Lockr BOM whitespace",
"url": "\ufeffhttps://aim.loc.kr/identity-lockr.js",
- "owner": null
+ "owner": null,
+ "serverHtmlOwner": "lockr"
},
{
"name": "Permutive NEL whitespace",|
|
||
| - Reject literal NULs in raw matcher inputs: HTML replaces them with U+FFFD, | ||
| unlike direct URL-property assignment, which can trim them. This prevents raw | ||
| HTML recognition from claiming a URL that the browser would not load as an SDK. |
There was a problem hiding this comment.
🔧 wrench: Lines 53-55 reject NUL because HTML changes it before the browser resolves the URL. Server matching has two more differences of that kind, because it reads lol_html's attribute text, and neither is documented or tracked. Measured with lol_html 2.9.0 against this head:
https://aim.loc.kr/identity-lockr.js(mustache.js escapes/this way) was rewritten by the pre-PR substring matcher and is missed now, so the browser loads the vendor SDK directly.https://static.example.com?@aim.loc.kr/identity-lockr.jsis claimed by both, although the browser loadshttps://static.example.com/?@aim.loc.kr/identity-lockr.js. Accepting backslash authority adds four cases of the same kind, such ashttps:\\aim.loc.kr/#identity-lockr.js.- A leading U+FEFF is dropped (see the comment on
sdk_url_test_support.rs:75).
Please document them as accepted limits, open one follow-up issue for reading decoded attribute values (character references and the leading BOM) in html_processor, which would fix GPT and Sourcepoint too, and link it from here.
| HTML recognition from claiming a URL that the browser would not load as an SDK. | |
| HTML recognition from claiming a URL that the browser would not load as an SDK. | |
| - Server matching reads the attribute text lol_html returns, which differs from | |
| what a browser resolves in two ways. Character references stay encoded, so a | |
| reference in a structural position, such as | |
| `https://aim.loc.kr/identity-lockr.js`, is not recognized | |
| server-side and the browser loads the vendor SDK directly, while one inside | |
| user information, such as | |
| `https://static.example.com?@aim.loc.kr/identity-lockr.js`, is still | |
| claimed although the browser loads another host. A leading U+FEFF is dropped, | |
| so the "Lockr BOM whitespace" corpus input is claimed in served HTML although | |
| a browser resolves it as a publisher path. Both are server HTML limits; fixing | |
| them belongs in the HTML processor for every attribute rewriter and is out of | |
| scope here. |
| /// stay unowned; HTML replaces NULs rather than treating them as URL whitespace. | ||
| /// Browser/Rust ownership cases live in crates/trusted-server-js/lib/test/fixtures/sdk-url-ownership.json. | ||
| pub(super) fn parse_sdk_network_url(url: &str) -> Option<Url> { | ||
| if url.contains('\0') { |
There was a problem hiding this comment.
🔧 wrench: Rejecting a NUL anywhere is stricter than the HTML behavior the doc comment cites. HTML replaces each NUL with U+FFFD, so a leading or trailing NUL never reaches a vendor host, but an internal one can leave both the vendor host and the SDK path intact. For //aim.loc.kr/<NUL>identity-lockr.js in markup, jsdom and Chrome both request https://aim.loc.kr/%EF%BF%BDidentity-lockr.js, while this returns None, so the server lets it load from the vendor. The pre-PR matchers and round 1 claimed these; the corpus only covers a leading and a trailing NUL. Please either mirror HTML on this path (replace NUL with U+FFFD before trimming, which keeps the leading/trailing rows unowned), or say in the spec that an internal NUL stays unowned server-side.
| use url::Url; | ||
|
|
||
| /// Parse an HTTP(S) SDK URL with an explicit authority, or a protocol-relative URL. | ||
| /// | ||
| /// Applies browser C0/space trimming, removes tab/CR/LF, and accepts backslash | ||
| /// authority separators. Literal NULs, relative paths, and base-dependent shorthand | ||
| /// stay unowned; HTML replaces NULs rather than treating them as URL whitespace. | ||
| /// Browser/Rust ownership cases live in crates/trusted-server-js/lib/test/fixtures/sdk-url-ownership.json. | ||
| pub(super) fn parse_sdk_network_url(url: &str) -> Option<Url> { | ||
| if url.contains('\0') { | ||
| return None; | ||
| } | ||
| let value = url | ||
| .trim_matches(|character| character <= '\u{20}') | ||
| .replace(['\t', '\n', '\r'], ""); |
There was a problem hiding this comment.
🔧 wrench: .replace(...) always allocates a new String, even when the input has no tab, CR, or LF. This runs for every src/href attribute once per enabled SDK rewriter, including the common early-None paths such as /static/app.js. The pre-PR matchers also allocated once per call (to_ascii_lowercase), so this isn't a regression, but a Cow keeps the common case borrowed. Applied alone, it passes cargo fmt, all eight clippy aliases, the five adapter test aliases, and the parity suite.
| use url::Url; | |
| /// Parse an HTTP(S) SDK URL with an explicit authority, or a protocol-relative URL. | |
| /// | |
| /// Applies browser C0/space trimming, removes tab/CR/LF, and accepts backslash | |
| /// authority separators. Literal NULs, relative paths, and base-dependent shorthand | |
| /// stay unowned; HTML replaces NULs rather than treating them as URL whitespace. | |
| /// Browser/Rust ownership cases live in crates/trusted-server-js/lib/test/fixtures/sdk-url-ownership.json. | |
| pub(super) fn parse_sdk_network_url(url: &str) -> Option<Url> { | |
| if url.contains('\0') { | |
| return None; | |
| } | |
| let value = url | |
| .trim_matches(|character| character <= '\u{20}') | |
| .replace(['\t', '\n', '\r'], ""); | |
| use std::borrow::Cow; | |
| use url::Url; | |
| /// Parse an HTTP(S) SDK URL with an explicit authority, or a protocol-relative URL. | |
| /// | |
| /// Applies browser C0/space trimming, removes tab/CR/LF, and accepts backslash | |
| /// authority separators. Literal NULs, relative paths, and base-dependent shorthand | |
| /// stay unowned; HTML replaces NULs rather than treating them as URL whitespace. | |
| /// Browser/Rust ownership cases live in crates/trusted-server-js/lib/test/fixtures/sdk-url-ownership.json. | |
| pub(super) fn parse_sdk_network_url(url: &str) -> Option<Url> { | |
| if url.contains('\0') { | |
| return None; | |
| } | |
| let trimmed = url.trim_matches(|character| character <= '\u{20}'); | |
| let value = if trimmed.contains(['\t', '\n', '\r']) { | |
| Cow::Owned(trimmed.replace(['\t', '\n', '\r'], "")) | |
| } else { | |
| Cow::Borrowed(trimmed) | |
| }; |
| "name": "Permutive newline in scheme", | ||
| "url": "ht\ntps://cdn.permutive.com/app-web.js", | ||
| "owner": "permutive" | ||
| }, |
There was a problem hiding this comment.
🔧 wrench: Dropping '\r' from the tab/CR/LF removal in parse_sdk_network_url fails only the Lockr corpus test ("Lockr CR between slashes"). No Permutive row has a CR before the authority, so the Permutive corpus test alone would not notice. With this row, that mutant also fails "Permutive CR in scheme". Applied alone, Prettier, the Lockr Vitest files (1,430 tests), the Rust corpus tests, cargo fmt, all eight clippy aliases, the five adapter test aliases, and the parity suite pass.
| "name": "Permutive newline in scheme", | |
| "url": "ht\ntps://cdn.permutive.com/app-web.js", | |
| "owner": "permutive" | |
| }, | |
| "name": "Permutive newline in scheme", | |
| "url": "ht\ntps://cdn.permutive.com/app-web.js", | |
| "owner": "permutive" | |
| }, | |
| { | |
| "name": "Permutive CR in scheme", | |
| "url": "ht\rtps://cdn.permutive.com/app-web.js", | |
| "owner": "permutive" | |
| }, |
| const template = document.createElement('template'); | ||
| template.innerHTML = `<script src="${raw}"></script>`; | ||
| const script = template.content.querySelector('script')!; | ||
| const attribute = raw.replaceAll('\0', '\ufffd'); |
There was a problem hiding this comment.
🔧 wrench: String.prototype.replaceAll is ES2021, and this library's tsconfig.json sets lib to ES2020, so tsc --noEmit -p . reports TS2550 on this line. It is the only tsc error in this PR's files. tsc isn't a CI gate and the library has other errors, so this is hygiene for new code. split/join builds the same string without a newer lib. Applied alone, Prettier, ESLint, and the 16 tests in this file pass, and the error is gone.
| const attribute = raw.replaceAll('\0', '\ufffd'); | |
| const attribute = raw.split('\0').join('\ufffd'); |
| Default-worker JS runs hit timeouts in core, creative, and Prebid tests locally. | ||
| The full four-worker run passed without changing test timeouts or assertions. | ||
|
|
There was a problem hiding this comment.
🔧 wrench: CI's vitest job runs with default workers and passes, so this reads as if the suite were flaky. It records one local environment rather than the design. This removes the note and its trailing blank line.
| Default-worker JS runs hit timeouts in core, creative, and Prebid tests locally. | |
| The full four-worker run passed without changing test timeouts or assertions. |
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