Repository navigation
Emit absolute creative rewrite URLs - #917
ChristianPavilonis wants to merge 12 commits into
Conversation
Retain main's sanitization controls, bounded CSS rewriting and opaque-origin click recovery while keeping configured absolute creative URLs. Keep the inline renderer's explicit origin override and both branches' regression coverage. Adapt root-relative test assumptions to absolute signing responses.
aram356
left a comment
There was a problem hiding this comment.
Summary
Adds [publisher].public_origin and emits absolute proxy, click, runtime, sign, and rebuild URLs on it, with the creative runtime capturing its script origin for endpoints. The template's new active public_origin placeholder bypasses the deploy placeholder guard, so an operator following the guard's error message ships a config that points every rewritten creative at ads.example.com. The absent-key fallback and the new currentScript precedence also deserve a second look.
4 of the 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. The remaining comments describe the fix in prose because the change touches multiple files or lines outside the diff and can't be auto-applied.
Blocking
🔧 wrench
- Template
public_originplaceholder escapes the deploy guard: see inline attrusted-server.example.toml:70
Non-blocking
🤔 thinking
- Absent-key fallback moves every existing deployment to
https://{apex}: see inline atcrates/trusted-server-core/src/settings.rs:275 currentScriptprecedence bypasses the pinned-stamp hardening: see inline atcrates/trusted-server-js/lib/src/integrations/creative/first_party.ts:8
♻️ refactor
public_originerrors drop the reason: see inline atcrates/trusted-server-core/src/settings.rs:158- Rebuild
//guard is bypassable and its test is vacuous: see inline atcrates/trusted-server-core/src/proxy.rs:1931 - EC cookie scope constraint is undocumented: see inline at
docs/guide/configuration.md:418 tsjs_unified_script_tagis now dead and duplicated: see inline atcrates/trusted-server-core/src/creative.rs:609
⛏ nitpick
- Port 0 accepted: see inline at
crates/trusted-server-core/src/settings.rs:3845
Cross-cutting / body-level findings
-
♻️ Inline fallback ignores
public_origin(crates/trusted-server-core/src/publisher.rs:6140-6144, outside the diff).build_bid_map_with_auction_idstill falls back toformat!("https://{}", settings.publisher.domain)when the request origin is empty. Withpublic_origin = "https://ads.example.com:8443",domain = "example.com", and an empty request origin, the inlineadmcomes out ashttps://example.com/first-party/proxy?...(verified throughbuild_bid_map(..., "", ...), the branchbuild_bid_map_rewrites_inline_adm_to_absolute_first_party_urlsexercises). Use the configured origin:let base_origin = if request_origin.is_empty() { settings.publisher.effective_public_origin() } else { request_origin.to_owned() };
-
⛏ The rollout caveat is repeated in many places. The "omission keeps old configs deserializable, not the serving page's scheme, host, or port" text, plus upgrade and rollback sequencing, appears in the CHANGELOG, the template,
configuration.md(twice),auction/README.md,creative-processing.md,first-party-proxy.md,auction-orchestration.md,api-reference.md, andtesting.md. Keep the full text inconfiguration.md#public_originand link to it elsewhere, so a later change (for example an opt-in default) edits one place. The newtesting.mdsection is guidance for this one feature's tests rather than the general testing guide, so it fits better in the spec or as test comments.
CI Status
- .github/dependabot.yml: PASS
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS (both runs)
- 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
- CI Node pin guard: 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
- ts dev lint domains (changed lines): PASS
- vitest: PASS
Reject the public-origin template placeholder, warn on the retained domain fallback, preserve validation reasons, and reject port zero.\n\nRemove the redundant rebuild authority guard and unused script-tag helper. Retain script-origin precedence with explicit spoofing-limit coverage, use the configured inline fallback, and centralize rollout and EC-cookie guidance.\n\nRetarget template-based audit generators and test harnesses to their actual public endpoints.
|
Addressed the requested changes in a35fef4 and replied to/resolved all eight inline threads. The two body-level findings are fixed too:
We explicitly reviewed and retained the always-absolute HTTPS/domain fallback and script-origin-first endpoint selection. The CLI now warns about an omitted The placeholder guard also exposed template consumers that only replaced the previous three publisher fields. Audit-generated drafts, CLI test fixtures, EOF runtime fixtures, and the template-cache fixture generator now supply the appropriate public endpoint. EOF fixtures distinguish the runtime listener from the upstream origin. Validation on the revised sources:
Fresh GitHub CI is running for the pushed commit. Requesting re-review from @aram356. |
aram356
left a comment
There was a problem hiding this comment.
Summary
All ten round-1 findings are resolved at a35fef44f. I re-ran the round-1 repros:
- Placeholder guard: with the real
tsbinary,ts config init, then editing onlydomain,cookie_domain, andorigin_url, now failsconfig pushwithInsecure default value for: publisher.public_originand publishes nothing. - Omitted-origin warning: the warning prints once per command.
- Mutation checks: removing each new guard (deploy and runtime placeholder checks, inline fallback, rebuild scheme and path checks, port 0, error reason) fails the new tests, so none of them is vacuous.
The PR can't merge yet: it conflicts with main, and CI has not run on this head.
Blocking
🔧 wrench
- Merge
mainand get CI to run on the head: see the body-level section below
Non-blocking
🌱 seedling
- Warn when
public_originis outside the EC cookie scope: see inline atcrates/trusted-server-core/src/config.rs:126
⛏ nitpick
{#public_origin}anchor doesn't resolve on GitHub: see inline atdocs/guide/configuration.md:414
Cross-cutting / body-level findings
-
🔧 Merge
mainand get CI to run on the head. GitHub reports the PR asdirty(mergeable: false), so thepull_requestworkflows did not run ona35fef44f. Only the two CodeQLAnalyzechecks exist. A trial merge with currentmain(6459e9470) conflicts in:CHANGELOG.mdcrates/trusted-server-integration-tests/src/bin/generate-viceroy-config.rscrates/trusted-server-integration-tests/tests/common/config.rscrates/trusted-server-integration-tests/tests/environments/axum.rscrates/trusted-server-integration-tests/tests/environments/cloudflare.rs
The integration-test conflicts change behavior, not just text. They come from #1107, which:
- added
--bidder-origin-urltogenerate-viceroy-config; - added a
traceparameter andintegration_app_config_envelope_with_trace, used by the newtests/common/trace_boundary.rs, which merges cleanly; - moved port selection into
AxumDevServer::spawn_with_app_config(&envelope)andCloudflareWorkers::spawn_with_readiness(ready).
Those spawn helpers now receive a finished envelope and pick the listener port themselves. The listener
public_origintherefore has to be applied there, after the port is chosen, by retargeting the envelope the wayspawn_with_config_jsonnow does withcloudflare_config_with_public_origin(or a sharedenvelope_with_public_origin). Otherwise the trace runtimes fall back tohttps://localhost. Keep both CLI flags, and both fields inArgs, in the generator. Then re-run the full CI: the parity, integration, and browser jobs cover exactly these files.
CI Status
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- All other checks (cargo fmt, cargo test, format-docs, format-typescript, vitest, integration, browser, parity, lint-domains, and the rest): not run. GitHub does not trigger
pull_requestworkflows while the PR has merge conflicts. - Run locally on
a35fef44f:cargo fmt --check, JS prettier, docs prettier, and Markdown prettier clean;vitest1261/1261 pass.
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Adds publisher.public_origin and emits absolute proxy, click, sign, rebuild, and runtime URLs on it. The code changes look correct, and the round-1 findings are resolved. The PR can't merge yet: it conflicts with main, so CI has not run on this head. Merging main also brings in a second "public origin" concept from #1251, and the merge should settle how the two relate.
Blocking
🔧 wrench
- Merge
mainand get CI to run on the head: see the body-level section below
Non-blocking
🤔 thinking
- Two "public origin" sources after merging #1251: see the body-level section below
Cross-cutting / body-level findings
-
🔧 Merge
mainand get CI to run on the head. This is the same open item as the previous review ona35fef44f. A trial merge with currentmain(d1bb3fa19) still conflicts inCHANGELOG.md,generate-viceroy-config.rs,tests/common/config.rs,tests/environments/axum.rs, andtests/environments/cloudflare.rs. Only the two CodeQLAnalyzechecks exist on this head. As a data point, the core library merges cleanly: with the integration-test conflicts taken frommain's side,cargo test -p trusted-server-core --libpasses (3351 passed). That doesn't exercise the merged integration-test logic, so the parity, integration, and browser jobs still need to run on the merged head. -
🤔 Two "public origin" sources after merging #1251. Since this branch's base,
mainaddedforwarder.rs. When an authenticated[trusted_forwarder]is configured, it derives a per-request public origin, andconfiguration.mdsays that value "supplies URL rewriting". Onmain, the inlinebuild_bid_mappath already takes itsrequest_originfrom it, throughRequestInfo::from_request. This PR adds a staticpublisher.public_originfor the other creative paths and documents "Do not derive it from request headers."With a forwarder configured and
publisher.public_originunset, one deployment then emits two authorities. Say the browser is onhttps://www.publisher.example: inline SSAT creatives usehttps://www.publisher.example, while/auctioncreatives, sign and rebuild responses, and the injected runtime usehttps://publisher.example.configuration.mdwould also use "public origin" for two different things.When resolving the merge, please state the precedence between the two in the
public_originand[trusted_forwarder]docs, or rename one term. Falling back to the forwarder origin would mean threading the request into the rewrite paths. That is probably out of scope here, so documenting the precedence is the minimal fix.
CI Status
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- All other checks: not run. GitHub doesn't trigger
pull_requestworkflows while the PR has merge conflicts.
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Adds publisher.public_origin and emits absolute proxy, click, sign, rebuild and runtime URLs on it. The core and JS changes look correct, and the round-1 fixes hold up. Two things block the merge. First, the template's new placeholder breaks the documented local quickstart and the smoke scripts, which only overlay the three older publisher placeholders. Second, the PR still conflicts with main, so CI has not run on this head.
None of the inline comments carry a one-click
suggestion. The quickstart fix touches.env.dev,docs/guide/getting-started.mdandscripts/smoke-common.sh, which are all outside this diff, so it has to be applied by hand.
Blocking
🔧 wrench
- Template
public_originplaceholder breaks the.env.devquickstart and the smoke scripts: see inline attrusted-server.example.toml:68 - Merge
mainand get CI to run on the head: see the body-level section below
Non-blocking
👍 praise
- Rebuild rejection tests now carry valid tokens: see inline at
crates/trusted-server-core/src/proxy.rs:3281
Cross-cutting / body-level findings
- 🔧 Merge
mainand get CI to run on the head. This is still open from the two earlier reviews ona35fef44f.git merge-treeagainst currentmain(d1bb3fa19) still reports conflicts inCHANGELOG.md,generate-viceroy-config.rs,tests/common/config.rs,tests/environments/axum.rsandtests/environments/cloudflare.rs. GitHub reports the PR asCONFLICTING/DIRTY, and the only checks on this head are the two CodeQLAnalyzeruns. The PR description's "All GitHub checks passed on99d2109c2" refers to the previous head. Please update it once CI has run on the merged head.
CI Status
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- All other checks (cargo fmt, cargo test, clippy, vitest, format-*, integration, browser, parity, lint-domains and the rest): not run. GitHub does not trigger
pull_requestworkflows while the PR has merge conflicts. No checks are marked required on this branch. - Run locally on
a35fef44f:cargo fmt --all -- --check: cleancargo test -p trusted-server-core --lib(native host): 3178 passedcargo test -p trusted-server-cli --test config_env_overlay --test config_store_defaults: 14 + 3 passedvitestwith the pinned Node 24.20.0: 52 files, 1261/1261 passed
Resolve base-branch merge conflicts while preserving absolute creative URLs, trace fixtures, and controlled bidder backends.
Merge main while preserving absolute creative URLs, Lockr routing changes, and both changelog notices.
|
Review status at head 7b98c25. Addressed
Still openThese are not being marked addressed or deferred by this cleanup:
The three actionable inline threads remain unresolved. Review-summary findings cannot be individually resolved, so the completed merge/CI item is recorded here without dismissing anyone's changes-requested review. Re-review has not been requested while the blocking quickstart/smoke item remains open. |
|
Pushed 6037fa4 to address the remaining blocker and the two documentation follow-ups.
Verification: both new regressions failed before the fix and now pass; all 19 CLI config tests, both documented quickstart config-push flows, the full Axum first-success smoke, focused CLI Clippy, ShellCheck/bash syntax, Rust/Markdown formatting, docs lint/build and committed-state URL-host lint pass. Full Fastly, Cloudflare and Spin runtime smokes were not rerun. The optional out-of-cookie-scope warning is deferred to a separate follow-up, not implemented in this PR. Its thread now records that decision and is resolved. The documented cookie-scope guidance remains in place. All inline review threads are now resolved. |
prk-Jr
left a comment
There was a problem hiding this comment.
Both items from my previous review are resolved at 6037fa4a2. The main merges retarget the listener public_origin after port selection for the Axum and Cloudflare runtimes, and all 29 checks pass on this head. The new configuration.md text accurately describes the precedence: build_bid_map prefers the nonempty request origin, which is forwarder-derived when one is authenticated, and falls back to effective_public_origin(); all other creative paths use the static setting. The .env.dev and smoke-script overlays and their CLI regressions look correct.
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Adds publisher.public_origin (validated, normalized, falling back to https://{domain}) and makes generated creative proxy/click/CSS/srcset/runtime/sign/rebuild URLs absolute on it; the creative runtime records its own script origin for endpoints. Well-tested and carefully documented overall, with one blocking issue in JS proxy detection.
Blocking
🔧 wrench
- Double-proxying when the bundle's origin differs from
public_origin— see inline atcrates/trusted-server-js/lib/src/integrations/creative/proxy_sign.ts:10
Non-blocking
🤔 thinking
public_originisn't checked against the cookie scope or for HTTPS — see inline atcrates/trusted-server-core/src/settings.rs:4018- Warning logged from inside
Validate::validate— see inline atcrates/trusted-server-core/src/config.rs:126
♻️ refactor
- Origin choice hidden in
base_path.starts_with('/')— see inline atcrates/trusted-server-core/src/creative.rs:660
⛏ nitpick
- Positive smoke port hard-coded as
BASE_PORT + 4in four scripts — see inline atscripts/smoke-axum.sh:39
👍 praise
- Rebuild endpoint ignores the input's host and tests the tricky inputs — see inline at
crates/trusted-server-core/src/proxy.rs:1938
CI Status
- All 29 reported checks: PASS
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Adds publisher.public_origin (validated, normalized, falling back to https://{domain}) and makes generated creative proxy/click/CSS/srcset/runtime/sign/rebuild URLs absolute on it; the creative runtime records its own script origin for endpoints. Well-tested and carefully documented. One blocking issue in JS proxy detection, plus non-blocking concerns.
Blocking
- 🔧 Double-proxying when the bundle's origin differs from
public_origin— inline atproxy_sign.ts:10
Non-blocking
- 🤔
public_originisn't checked against cookie scope or for HTTPS — inline atsettings.rs:4018 - 🤔 Warning logged from inside
Validate::validate— inline atconfig.rs:126 - ♻️ Origin choice hidden in
base_path.starts_with('/')— inline atcreative.rs:660 - ⛏ Positive smoke port hard-coded as
BASE_PORT + 4in four scripts — inline atscripts/smoke-axum.sh:39 - 📝 Script-origin trust model — honestly documented in
first_party.ts:3-21; earlier creative script can influencedocument.currentScript, but the iframe sandbox remains the boundary andpersistRebuiltClickonly allows http(s). - 👍 Rebuild endpoint ignores input host and tests tricky inputs — inline at
proxy.rs:1938
CI Status
All 29 checks: PASS.
|
Addressed the remaining feedback in 01bcc08 and the EOF fixture follow-up in 8ed42ab.
All 29 GitHub checks pass on The PR is not merged; human re-review is still needed. |
Summary
Emit absolute creative proxy, click, CSS, srcset, runtime, sign-response, and rebuild-response URLs using normalized
publisher.public_origin. Preserve signing, rewrite policy, legacy signed inputs, credential modes, and the inline renderer's request-origin override.Closes #915.
Spec
Absolute creative rewrite URLs at the reviewed revision.
Review follow-up
data-ts-public-origin. The runtime captures this metadata once to recognize the exact/first-party/proxypath on the configured origin as already proxied, even when the bundle loads from a different origin. Sign/rebuild endpoints remain script-origin-first; metadata does not replace the legacy parent stamp.publisher.domain, notcookie_domain, and retains the trailing-dot cookie distinction. Validation itself no longer logs. HTTP and intentional local mismatches remain valid; blob serialization and hashes are unchanged.base_path.starts_with('/')inference or per-resource public-origin resolution. Inline request-origin precedence and encrypted target tokens/query ordering remain covered.Compatibility
An omitted
public_originremains omitted in serialized config but emits general-purpose URLs onhttps://{publisher.domain}. Verify the browser-facing endpoint and coordinate binary/config upgrade and rollback as described under Publisher configuration.No signing-format migration, CORS expansion, cross-origin credential change, or identity/consent policy change. Script-origin and metadata capture are not authenticated against earlier creative scripts; the documented pre-capture currentScript spoofing limitation remains.
Verification on this follow-up
Pinned Rust 1.98.1, Node 24.20.0, and Viceroy 0.21.0.
8ed42ab8e, including Rust analysis, both EOF runtime jobs, integration, browser, trace-runtime acceptance, and parity. The first follow-up exposed an EOF test-harness assumption about byte-identical output across different listener ports. The harness now verifies the exact current origin stamp and permits only that controlled field to change in its otherwise exact output comparison.Plain browser TypeScript checking lacks Node types in the existing browser dependency installation; checking with the installed JS package's Node type roots passes without dependency changes. Default browser suites retain the opt-in mobile trace-runtime skips. Viceroy logs some canceled-response worker panics while browser tests still pass; this follow-up does not claim to fix simulator cancellation handling.
Tracked maintenance
The cookie-scope warning is implemented here rather than deferred. Sharing the two Viceroy fixture retargeting helpers remains separate in #1514, preserving their custom-resource, envelope verification, timestamp, upstream, and error contracts.