Repository navigation
Conversation
Spec and implementation plan for #1231: a [rewrite] include_domains allowlist for creative asset rewriting, case-insensitive exclude_domains matching, and a single rewrite of link imagesrcset on the inline path. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
dhruv8sh
changed the title
Describe the review fixes: strict include_domains validation with a separate non-ASCII error, binary-first upgrade and rollback docs, the distinct link imagesrcset candidate, the host-in-both-lists test across every handler, and the shared-step CHANGELOG entry with the sign status change. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Spec and plan for #1231's [rewrite] include_domains asset-host allowlist. The design holds up: the code claims check out against 7a0ecb4c, the <link imagesrcset> double rewrite is real (reproduced against lol_html 2.9.0), the shared step is byte-identical with #1249's, and the plan's snippets compile and pass clippy. One blocking issue: the include-list validator doesn't meet its own stated guarantee for some IPv6 and xn-- entries.
7 of the inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch) to apply them. The remaining comments describe the fix in prose because it spans several places or lines outside a single range.
Blocking
🔧 wrench
- Validator's IPv6 check uses Rust's formatter, not
host_str()'s — see inline atdocs/superpowers/plans/2026-10-07-1231-creative-asset-host-allowlist.md:967
Non-blocking
⛏ nitpick
- Step 5 grep can never pass — see inline at
docs/superpowers/plans/2026-10-07-1231-creative-asset-host-allowlist.md:671 - Shared step:
is_host_permittedvisibility contradicts the plan — see inline atdocs/superpowers/specs/2026-10-07-1231-creative-asset-host-allowlist-design.md:312 - Only
test-fastlyruns core tests — see inline atdocs/superpowers/specs/2026-10-07-1231-creative-asset-host-allowlist-design.md:321 rewrite_clicksdoesn't exist onmain— see inline atdocs/superpowers/plans/2026-10-07-1231-creative-asset-host-allowlist.md:1720- Leftover sentence in Task 3 Step 2 — see inline at
docs/superpowers/plans/2026-10-07-1231-creative-asset-host-allowlist.md:876 - Module-doc anchor that doesn't exist — see inline at
docs/superpowers/plans/2026-10-07-1231-creative-asset-host-allowlist.md:1479 - Output-parity and blob-hash wording — see inline at
docs/superpowers/specs/2026-10-07-1231-creative-asset-host-allowlist-design.md:233 - GTM citation points at test code — see inline at
docs/superpowers/specs/2026-10-07-1231-creative-asset-host-allowlist-design.md:94 - Test-plan cases the plan doesn't include — see inline at
docs/superpowers/specs/2026-10-07-1231-creative-asset-host-allowlist-design.md:325
♻️ refactor
link[href]snippet relies on comment placement to pass clippy — see inline atdocs/superpowers/plans/2026-10-07-1231-creative-asset-host-allowlist.md:1593-1613- Task 2 red step can't be observed — see inline at
docs/superpowers/plans/2026-10-07-1231-creative-asset-host-allowlist.md:657
🤔 thinking
- Inert
exclude_domainsentries stay silent — see inline atdocs/superpowers/specs/2026-10-07-1231-creative-asset-host-allowlist-design.md:132
Cross-cutting / body-level findings
- 📌 Trailing-dot hosts bypass
exclude_domains(pre-existing) —https://blocked.example.com./xhashost_str()blocked.example.com., which matches neitherblocked.example.comnor*.example.cominis_host_allowed(proxy.rs:1242-1255), so it is proxied, signed and getsts-ecdespite the exclusion.include_domainsfails safe here (an off-list host stays raw). Not for this PR; worth a follow-up issue (e.g. strip one trailing.before matching). - 👍 Solid design choices — the sign handler's ordering keeps
403reserved forproxy.allowed_domains; the normalizer/policy split is clean and parse-once; the shared step with #1234 is reproducible verbatim; and the plan's red/green expectations line up with the code paths.
CI Status
- format-docs: PASS
- format-typescript: PASS
- CLAUDE.md symlink guard: PASS
- CodeQL: SKIPPED
- cargo fmt, cargo test, cargo test (axum native), cargo test (cross-adapter parity), cargo check/build/test (spin native + wasm32-wasip1), cargo check (cloudflare native + wasm32-unknown-unknown), cargo test (ts CLI, native) (ubuntu-latest / macos-latest), vitest, prepare integration artifacts, Analyze (rust / javascript-typescript / actions / python): PENDING at review time (docs-only change)
| .strip_suffix(']') | ||
| .and_then(|inner| inner.parse::<std::net::Ipv6Addr>().ok()) | ||
| .ok_or("not a bracketed IPv6 address")?; | ||
| if format!("[{address}]") != pattern { |
There was a problem hiding this comment.
🔧 wrench — The IPv6 check compares against Rust's Ipv6Addr Display, not what Url::host_str() renders, so validate_include_domains does not do what spec :133 promises ("rejects every entry that can never equal Url::host_str()"). I ran this function verbatim against url =2.5.8 / validator =0.20.0:
| Entry | Validator | host_str() |
|---|---|---|
[::ffff:192.0.2.1] |
accepts | renders [::ffff:c000:201] — never matches |
[::ffff:c000:201] (the real form) |
rejects ("not in compressed form") | — |
xn--zz.example |
accepts | Url::parse fails (IDNA error) — never matches |
Proposed fix (apply manually — touches the plan's validator and the test lists in both the plan and spec :330-332). Keep the existing syntactic checks for their error reasons, then finish with a canonical round trip through the same parser the matcher depends on:
let host = url::Host::parse(name).map_err(|_| "not a valid URL host")?;
if host.to_string() != name {
return Err("not in the form Url::host_str() renders");
}url::Host's Display brackets IPv6, so apply it to the full pattern for […] entries. Add [::ffff:c000:201] (accept), [::ffff:192.0.2.1] and xn--zz.example (reject) to the test lists.
|
|
||
| Delete `Rewrite::is_excluded` (with its `#[allow(dead_code)]`) and the `test_rewrite_is_excluded` test (`settings.rs:6522-6546`); Task 1's tests cover its cases. | ||
|
|
||
| Run: `git grep -n "to_abs\|is_excluded(" crates/` |
There was a problem hiding this comment.
⛏ nitpick — This grep can never come back empty: it also matches auction/telemetry.rs:1036 (…_default_to_absent_…) and publisher.rs:22620 (…_to_absolute_first_party_urls). The word-bounded form only hits the intended files.
| Run: `git grep -n "to_abs\|is_excluded(" crates/` | |
| Run: `git grep -nw -e to_abs -e is_excluded crates/` |
| Both specs describe the same split, so either implementation can land first: | ||
|
|
||
| - `to_abs` is replaced by `normalize_creative_url(url: &str) -> Option<url::Url>` in `creative.rs`: trimming, protocol-relative, `http(s)` only, no policy. | ||
| - Policy lives on `Rewrite` in `settings.rs` as `should_proxy_asset(&self, host: &str) -> bool` (not excluded and (include list empty or host included)) and `should_wrap_click(&self, host: &str) -> bool` (not excluded), with a private `is_excluded_host`. Both use the existing case-insensitive matchers `proxy::is_host_allowed` and `proxy::is_host_permitted` (made `pub(crate)`): an exact host, or `*.example.com` matching the apex and subdomains. |
There was a problem hiding this comment.
⛏ nitpick — This bullet says the shared split uses is_host_permitted "made pub(crate)" and gives the shared should_proxy_asset the include clause. The plan's Shared step (:49), #1249, and line 317 here all say the shared step is exclude-only and is_host_permitted stays private (importing it would trip -D warnings as unused).
| - Policy lives on `Rewrite` in `settings.rs` as `should_proxy_asset(&self, host: &str) -> bool` (not excluded and (include list empty or host included)) and `should_wrap_click(&self, host: &str) -> bool` (not excluded), with a private `is_excluded_host`. Both use the existing case-insensitive matchers `proxy::is_host_allowed` and `proxy::is_host_permitted` (made `pub(crate)`): an exact host, or `*.example.com` matching the apex and subdomains. | |
| - Policy lives on `Rewrite` in `settings.rs` as `should_proxy_asset(&self, host: &str) -> bool` (not excluded) and `should_wrap_click(&self, host: &str) -> bool` (not excluded), with a private `is_excluded_host`. Both use the existing case-insensitive matcher `proxy::is_host_allowed`: an exact host, or `*.example.com` matching the apex and subdomains. `proxy::is_host_permitted` stays private in the shared step; #1231 makes it `pub(crate)` when it adds the include clause. |
|
|
||
| ## Test plan | ||
|
|
||
| All tests are unit tests in `trusted-server-core` and run under `cargo test-fastly`, `cargo test-axum`, `cargo test-cloudflare` and `cargo test-spin`. Hosts are under `example.com`, `example.net` and `example.org`. |
There was a problem hiding this comment.
⛏ nitpick — Only test-fastly includes -p trusted-server-core; test-axum, test-cloudflare and test-spin are scoped to their adapter crates (.cargo/config.toml:42-60), so they don't run these tests. Native core runs are also much faster than Viceroy and sidestep the "a wasm panic aborts the binary" workaround the plan repeats.
| All tests are unit tests in `trusted-server-core` and run under `cargo test-fastly`, `cargo test-axum`, `cargo test-cloudflare` and `cargo test-spin`. Hosts are under `example.com`, `example.net` and `example.org`. | |
| All tests are unit tests in `trusted-server-core` and run under `cargo test-fastly` (or natively with `cargo test -p trusted-server-core --target <host-triple>`). Hosts are under `example.com`, `example.net` and `example.org`. |
| ::: | ||
| ``` | ||
|
|
||
| The warning box mirrors the `[auction]` one for `rewrite_clicks`. |
There was a problem hiding this comment.
⛏ nitpick — rewrite_clicks doesn't exist on main; it's #1234's [auction] rewrite_clicks. If this lands first the reference dangles. The existing analogue is the rewrite_creatives upgrade/rollback box (configuration.md:1959). Same applies to spec :379 ("mirroring the rewrite_clicks warning") and spec :388 — worth qualifying those as [auction] rewrite_clicks / conditional on #1234 too.
| The warning box mirrors the `[auction]` one for `rewrite_clicks`. | |
| The warning box mirrors the existing `[auction] rewrite_creatives` upgrade and rollback box (and #1234's `[auction] rewrite_clicks` box if that has landed). |
|
|
||
| Run: `cargo test-fastly -- normalize_creative_url proxy_if_abs_respects unparseable_absolute exclude_domains_match_case proxy_sign_rejects_excluded` | ||
|
|
||
| Expected: `error[E0432]: unresolved import super::normalize_creative_url`. `unparseable_absolute_click_url_is_left_byte_identical`, `exclude_domains_match_case_insensitively_in_the_rewrite_pass` and `proxy_sign_rejects_excluded_urls_case_insensitively` use only existing APIs. Against the pre-change code they fail at runtime: the anchor gains `data-tsclick`, the mixed-case-excluded link is wrapped, and sign returns `200`. Run each alone with `--exact <name> --nocapture` to see the panic. |
There was a problem hiding this comment.
♻️ refactor — These red-step failures can't be observed: Step 1 switches the test use super::{…} to normalize_creative_url, so the whole core test binary fails to compile before Step 3. The predicted runtime failures are right by analysis, but no executor will see them. Suggest: add unparseable_absolute_click_url_is_left_byte_identical, exclude_domains_match_case_insensitively_in_the_rewrite_pass and proxy_sign_rejects_excluded_urls_case_insensitively first with the old import, watch them fail, then switch the import.
| Add `Rewrite::normalize(&mut self)` and call it from `Settings::normalize_deserialized` (`settings.rs:2988`), next to `self.proxy.normalize()`. That function runs on every load path: TOML, config blob, and `TrustedServerAppConfig` deserialization used by `ts config push` validation (`config.rs:111-112`). | ||
|
|
||
| - Both lists: trim each entry and lowercase it with `to_ascii_lowercase`. | ||
| - `exclude_domains`: drop empty entries and a bare `*` with a `log::warn!`, as `Proxy::normalize` does. Neither can match a host today, so dropping them changes no behavior, and rejecting them would make an existing blob fail to load after a binary upgrade. |
There was a problem hiding this comment.
🤔 thinking — exclude_domains gets no validation, so an inert entry like https://cdn.example.com, cdn.example.com:443 or [2001:0db8::1] silently never matches — that host keeps being proxied and receives ts-ec, the leak #1231 is about. Rejecting would break existing blobs (agreed), but Rewrite::normalize could run the same check_include_domain and log::warn! once per inert entry, with zero behavior change.
Related: issue #1231 says to reject empty entries and a bare *; dropping with a warning is the better call, but it's worth flagging explicitly as a deviation from the issue (as #1249 does for its own).
|
|
||
| Because no caller reads the original string, every handler in the table gets the allowlist with no per-handler code. Nested HTML and CSS fetched through `/first-party/proxy` run the same pass and pick it up too. | ||
|
|
||
| **Output parity with an empty allowlist.** `build_signed_url_for` already parses the clear URL and signs its `Url` serialization, and `Url::parse(url.as_str())` round-trips, so for every input that parses, the rewritten bytes are identical to today's. The only difference is an absolute-looking value that `Url::parse` rejects (for example `https://exa mple.example/x`). Today `to_abs` accepts it and `build_signed_url_for` hands back the raw string, so the handler rewrites the attribute to its own value and `CssUrlRewriter` re-serializes it as a quoted string. After the change the value is left untouched. No existing fixture contains such a URL; `unparseable_absolute_url_is_left_byte_identical` pins the new behavior. |
There was a problem hiding this comment.
⛏ nitpick — Two precision points:
- Today's parse-failure fallback emits the trimmed,
https:-prefixed string, not "its own value" — e.g.src=" //exa mple.example/x "becomeshttps://exa mple.example/x. Anchors also gaindata-tsclickwith it (the CHANGELOG text says this; this paragraph doesn't). - Line 123's "existing blob hashes do not change" only holds for configs that are already normalized:
TrustedServerAppConfigserializes the normalized settings (config.rs:97-104), so after this changets config pushwrites lowercased/trimmedexclude_domainsfor mixed-case or whitespace entries.proxy.allowed_domainsalready behaves this way, so it's fine — just qualify the sentence.
|
|
||
| `Rewrite::is_excluded` (`settings.rs:647-669`) parses the URL, takes `host_str()` (already lowercase), and compares it against raw entries with `==` and `ends_with`. An entry such as `CDN.example.com` never matches. It still carries a stale `#[allow(dead_code)]` (`settings.rs:647`). | ||
|
|
||
| `proxy::is_host_allowed` (`proxy.rs:1242-1255`) lowercases both sides and enforces a dot boundary: `example.com` matches only itself; `*.example.com` matches `example.com` and any subdomain, not `evil-example.com`. Prebid (`integrations/prebid.rs:918`) and GTM (`integrations/google_tag_manager.rs:1983`) reuse it. `Proxy::normalize` (`settings.rs:1766-1789`) trims and lowercases `allowed_domains`, drops empty entries, and drops a bare `*` with a warning. |
There was a problem hiding this comment.
⛏ nitpick — google_tag_manager.rs:1983 is inside the #[cfg(test)] module. Production GTM reaches the matcher indirectly via .with_allowed_domains(...) (:805) → redirect policy → is_host_permitted. The claim is true in substance; the citation points at test code.
|
|
||
| ### `settings.rs` | ||
|
|
||
| - `should_proxy_asset` table: empty include list proxies any host; exact match; wildcard matches apex and nested subdomain; wildcard does not match `evil-example.com`; off-list host is rejected; a host in both lists is rejected; uppercase host and uppercase entry both match. |
There was a problem hiding this comment.
⛏ nitpick — A few test-plan promises the plan doesn't deliver:
- :325 — an uppercase include entry in
should_proxy_asset; the plan's policy test only pushes lowercase entries, so uppercase is covered only via TOML normalization. Push e.g."*.EXAMPLE.com"directly. - :333 — the port of
test_rewrite_is_excludeddrops its suffix-spoof case (example2.com.fake.com); add something like("example.org.evil.example.net", true)to the Task 1 table. - :339 — lists
video/audio/source src, but the matrix creative has only<video src>.
This branch has not been deployed
3 participants
Summary
[rewrite] include_domainsallowlist for creative asset rewriting. Today every absolute asset URL in a winning creative is proxied, so every third-party asset host receives the EC ID. The allowlist lets operators limit that to hosts they choose. Click-through links are not affected.[rewrite] include_domains.to_abssplit:to_absbecomes a pure normalizer,normalize_creative_url, and host policy moves toRewrite::should_proxy_assetandRewrite::should_wrap_click./first-party/sign: an off-list host gets a502, the same as an excluded host, so the browser falls back to a direct load.exclude_domains: matching becomes case-insensitive. This is a behavior change and gets a CHANGELOG note.<link imagesrcset>was proxied twice.Changes
docs/superpowers/specs/2026-10-07-1231-creative-asset-host-allowlist-design.mddocs/superpowers/plans/2026-10-07-1231-creative-asset-host-allowlist.mdCloses
Part of #1231. This PR adds only the spec and plan. The implementation PR will link the approved spec and close the issue.
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveprettier --config docs/.prettierrc --checkon both files. This PR changes no code, so no Rust or JS gates were run. The plan was checked by implementing it in a local branch, where fmt, all clippy aliases,test-fastly,test-fastly-reuse,test-axum,test-cloudflareandtest-spinpassed. That code is not included here.Checklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)