[ChristianPavilonis]

Summary

• Emit absolute proxy, click, CSS, srcset, creative-runtime, sign-response and rebuild-response URLs using validated, normalized publisher.public_origin.
• Capture the injected classic script's origin for runtime endpoints and retain absolute click authority across repeated repairs. Preserve existing signing, rewrite policy, legacy inputs, failure behavior and credential modes.
• Refresh onto current main, including its separate click switch and asset-host allowlist. Fix runtime-specific integration origins, foreign-parent browser coverage and the browser CI generator dependency.

Closes #915

Spec

Absolute creative rewrite URLs, with two independent source-backed review rounds recorded in its review log.

Changes

Area	Change
Configuration	Optional publisher.public_origin; normalization at Settings/app-config loading and construction; TOML/JSON/blob and present/absent-leaf CLI overlay coverage.
Creative/proxy output	Shared absolute signed-URL construction and runtime tag generation; absolute rebuild JSON/redirects; existing input parser and original-token validation retained.
Creative runtime	Immutable script-origin endpoint selection; normalized legacy fallback; exact proxy recognition; absolute rebuilt click persistence; source and emitted-IIFE regression tests.
Integration fixtures	Derive public origin from each actual listener, independently of upstream origin; preserve custom Viceroy resources/config; keep the legacy source fixture without the optional key.
Browser/CI	Real first-party and foreign-parent sandbox recovery checks; explicitly build the native config generator on the browser runner.
Documentation	Absolute output examples, configuration normalization, environment-overlay caveat and coordinated rollout/rollback.

GET rebuild routing and absolute-click input support already exist on main; they are not new routes in this PR. Useful compatibility/route tests remain. The inline SSAT/page-bids path keeps its request-origin override and no runtime injection. TSJS source selection and hashing are unchanged. No CORS expansion or signing-format migration.

Compatibility and rollout

An omitted public_origin stays omitted in serialized configuration, but generated general-purpose URLs now use https://{publisher.domain} rather than following the visitor's scheme, host and port. Verify the actual browser-facing endpoint, including local HTTP ports. Old binaries reject the new key, so coordinate binary/config activation and rollback when the fallback cannot serve traffic. CLI environment overrides only replace an existing TOML scalar leaf.

Test plan

Validation uses Rust 1.98.1, Node 24.20.0 and standalone Viceroy 0.21.0. Local artifacts and logs are retained outside the worktree.

• cargo test-fastly && cargo test-fastly-reuse && cargo test-axum && cargo test-cloudflare && cargo test-spin
• Cross-adapter parity tests.
• Focused Settings/app-config, config-generator, generated Cloudflare and actual CLI environment-overlay tests.
• All eight target-matched Clippy aliases, native build-digest test/lint, integration-test lint and wasm-clock lint.
• cargo fmt --all -- --check
• JS build, lint and formatting; full Vitest suite: 52 files, 1,260 tests passed, no type-assertion errors.
• Emitted independent core+creative IIFE regression, including captured origin and repeated click repair/navigation.
• Docs lint, formatting and build; root Markdown formatting.
• Fastly release WASM build.
• Spin release WASM build; native core API documentation build.
• Full real Chromium suites: Next.js 23 passed/1 framework skip; WordPress 13 passed/11 framework skips. Both include first-party and actual foreign-parent signed GET rebuild/click redirect checks, with browser security unchanged.
• Authoritative committed-state URL-host lint: ts dev lint domains --changed-vs origin/main.
• Workflow actionlint, CI Node-pin guard and documentation-proposal script checks.
• Manual fastly compute serve session. Automated Viceroy/Chromium checks cover the runtime path instead.

All local gates above are complete. Native core rustdoc succeeds with 31 link warnings on unchanged main documentation; it is not warning-clean. All 29 GitHub checks passed on 7b98c2510, including browser integration, trace-runtime/browser acceptance, cross-adapter parity, the integration matrix and Fastly EC lifecycle tests. Fresh CI is running on follow-up 6037fa4a2.

Follow-up 6037fa4a2 fixes quickstart/smoke public-origin overlays, corrects GitHub-facing configuration links, and documents static-versus-forwarded origin precedence. Local verification passed all 19 config overlay/store 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. The two new regressions failed before the overlay fix and pass afterward. Full Fastly/Cloudflare/Spin runtime smokes were not rerun.

Review

Four independent final lanes covered correctness, architecture, code smells and style. The missing browser-CI generator and changed-line example-host issues were fixed and re-reviewed. No blocking production findings remain. One nonblocking maintenance suggestion, sharing the two integration-fixture retargeting helpers, is deferred to avoid merging their distinct upstream/timestamp/error contracts in this fix.

Checklist

• Feature spec linked, ending in two review rounds.
• Changes follow AGENTS.md conventions.
• No new production unwrap() calls or plaintext production credentials.
• No new logging API or identity/consent policy.
• New behavior and compatibility cases have tests.

[aram356]

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_origin placeholder escapes the deploy guard: see inline at trusted-server.example.toml:70

Non-blocking

🤔 thinking

• Absent-key fallback moves every existing deployment to https://{apex}: see inline at crates/trusted-server-core/src/settings.rs:275
• currentScript precedence bypasses the pinned-stamp hardening: see inline at crates/trusted-server-js/lib/src/integrations/creative/first_party.ts:8

♻️ refactor

• public_origin errors drop the reason: see inline at crates/trusted-server-core/src/settings.rs:158
• Rebuild // guard is bypassable and its test is vacuous: see inline at crates/trusted-server-core/src/proxy.rs:1931
• EC cookie scope constraint is undocumented: see inline at docs/guide/configuration.md:418
• tsjs_unified_script_tag is now dead and duplicated: see inline at crates/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_id still falls back to format!("https://{}", settings.publisher.domain) when the request origin is empty. With public_origin = "https://ads.example.com:8443", domain = "example.com", and an empty request origin, the inline adm comes out as https://example.com/first-party/proxy?... (verified through build_bid_map(..., "", ...), the branch build_bid_map_rewrites_inline_adm_to_absolute_first_party_urls exercises). 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, and testing.md. Keep the full text in configuration.md#public_origin and link to it elsewhere, so a later change (for example an opt-in default) edits one place. The new testing.md section 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

[ChristianPavilonis]

Addressed the requested changes in a35fef4 and replied to/resolved all eight inline threads.

The two body-level findings are fixed too:

• Inline bid-map rendering now uses the configured public origin when the request origin is empty. A nonempty request origin still wins, and inline creatives still do not inject the runtime.
• The full fallback/upgrade/rollback warning now lives at configuration.md#public_origin, with links elsewhere. Feature-specific testing guidance moved into the spec, and configuration examples stay inside the EC cookie scope.

We explicitly reviewed and retained the always-absolute HTTPS/domain fallback and script-origin-first endpoint selection. The CLI now warns about an omitted public_origin. The runtime documentation/tests explicitly acknowledge pre-capture currentScript spoofing; this change does not claim to fix it. The unused script-tag helper was deleted after confirming there are no external consumers.

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:

• All five Rust adapter test aliases and cross-adapter parity passed.
• All eight Clippy aliases, Rust formatting, native build-digest test/lint, integration lint, and Wasm clock lint passed.
• Full CLI suite passed, including all 14 environment/config tests and real validate/push placeholder and warning regressions.
• JS build/lint/format and all 52 Vitest files passed: 1,261 tests, no type errors.
• Real Chromium focused suites passed all four before/after-spoofing and first-party/foreign-parent cases on both frameworks. Full Next.js: 25 passed, 1 framework skip. Full WordPress: 15 passed, 11 framework skips.
• Native EOF harness tests/lint and actual Axum/Fastly EOF plus auction controls passed.
• Docs lint/format/build, root Markdown formatting, documentation-proposal checks, CI Node-pin guard, and committed-state URL-host lint passed.
• Template-cache inline/ESI fixture generation and real CLI validation passed; the full template-cache runtime harness was not rerun.

Fresh GitHub CI is running for the pushed commit. Requesting re-review from @aram356.

[aram356]

Summary

All ten round-1 findings are resolved at a35fef44f. I re-ran the round-1 repros:

• Placeholder guard: with the real ts binary, ts config init, then editing only domain, cookie_domain, and origin_url, now fails config push with Insecure default value for: publisher.public_origin and 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 main and get CI to run on the head: see the body-level section below

Non-blocking

🌱 seedling

• Warn when public_origin is outside the EC cookie scope: see inline at crates/trusted-server-core/src/config.rs:126

⛏ nitpick

• {#public_origin} anchor doesn't resolve on GitHub: see inline at docs/guide/configuration.md:414

Cross-cutting / body-level findings

• 🔧 Merge main and get CI to run on the head. GitHub reports the PR as dirty (mergeable: false), so the pull_request workflows did not run on a35fef44f. Only the two CodeQL Analyze checks exist. A trial merge with current main (6459e9470) conflicts in:

  • CHANGELOG.md
  • crates/trusted-server-integration-tests/src/bin/generate-viceroy-config.rs
  • crates/trusted-server-integration-tests/tests/common/config.rs
  • crates/trusted-server-integration-tests/tests/environments/axum.rs
  • crates/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-url to generate-viceroy-config;
  • added a trace parameter and integration_app_config_envelope_with_trace, used by the new tests/common/trace_boundary.rs, which merges cleanly;
  • moved port selection into AxumDevServer::spawn_with_app_config(&envelope) and CloudflareWorkers::spawn_with_readiness(ready).

  Those spawn helpers now receive a finished envelope and pick the listener port themselves. The listener public_origin therefore has to be applied there, after the port is chosen, by retargeting the envelope the way spawn_with_config_json now does with cloudflare_config_with_public_origin (or a shared envelope_with_public_origin). Otherwise the trace runtimes fall back to https://localhost. Keep both CLI flags, and both fields in Args, 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_request workflows while the PR has merge conflicts.
• Run locally on a35fef44f: cargo fmt --check, JS prettier, docs prettier, and Markdown prettier clean; vitest 1261/1261 pass.

[prk-Jr]

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 main and 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 main and get CI to run on the head. This is the same open item as the previous review on a35fef44f. A trial merge with current main (d1bb3fa19) still conflicts in CHANGELOG.md, generate-viceroy-config.rs, tests/common/config.rs, tests/environments/axum.rs, and tests/environments/cloudflare.rs. Only the two CodeQL Analyze checks exist on this head. As a data point, the core library merges cleanly: with the integration-test conflicts taken from main's side, cargo test -p trusted-server-core --lib passes (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, main added forwarder.rs. When an authenticated [trusted_forwarder] is configured, it derives a per-request public origin, and configuration.md says that value "supplies URL rewriting". On main, the inline build_bid_map path already takes its request_origin from it, through RequestInfo::from_request. This PR adds a static publisher.public_origin for the other creative paths and documents "Do not derive it from request headers."

  With a forwarder configured and publisher.public_origin unset, one deployment then emits two authorities. Say the browser is on https://www.publisher.example: inline SSAT creatives use https://www.publisher.example, while /auction creatives, sign and rebuild responses, and the injected runtime use https://publisher.example. configuration.md would also use "public origin" for two different things.

  When resolving the merge, please state the precedence between the two in the public_origin and [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_request workflows while the PR has merge conflicts.

[dhruv8sh]

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.md and scripts/smoke-common.sh, which are all outside this diff, so it has to be applied by hand.

Blocking

🔧 wrench

• Template public_origin placeholder breaks the .env.dev quickstart and the smoke scripts: see inline at trusted-server.example.toml:68
• Merge main and 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 main and get CI to run on the head. This is still open from the two earlier reviews on a35fef44f. git merge-tree against current main (d1bb3fa19) still reports conflicts in CHANGELOG.md, generate-viceroy-config.rs, tests/common/config.rs, tests/environments/axum.rs and tests/environments/cloudflare.rs. GitHub reports the PR as CONFLICTING/DIRTY, and the only checks on this head are the two CodeQL Analyze runs. The PR description's "All GitHub checks passed on 99d2109c2" 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_request workflows while the PR has merge conflicts. No checks are marked required on this branch.
• Run locally on a35fef44f:
  • cargo fmt --all -- --check: clean
  • cargo test -p trusted-server-core --lib (native host): 3178 passed
  • cargo test -p trusted-server-cli --test config_env_overlay --test config_store_defaults: 14 + 3 passed
  • vitest with the pinned Node 24.20.0: 52 files, 1261/1261 passed

[ChristianPavilonis]

Review status at head 7b98c25.

Addressed

• All ten original findings were handled in a35fef4. The eight inline threads already have replies and are resolved; the two body-level fixes are covered in the previous response. The subsequent review confirmed all ten fixes.
• The merge/CI blocker raised by @aram356, @prk-Jr and @dhruv8sh is addressed by 68d59c9 and 7b98c25. The integration merge preserves trace/custom app-config fixtures and retargets the listener public origin after port selection. The generator retains both public-origin and bidder-origin options. GitHub now reports the PR as mergeable.
• All 29 current-head checks pass, including browser integration, integration, cross-adapter parity, trace-runtime/browser acceptance, Rust/CLI/JS tests, formatting and CodeQL. This supersedes the earlier review snapshots where CI had not run and the description's older-head CI reference.
• The CodeQL logging finding is addressed in fbc861e. Added a reply explaining the fix; that thread was already resolved. Also acknowledged and resolved the rebuild-test praise thread, which requests no further change.

Still open

These are not being marked addressed or deferred by this cleanup:

1. Blocking: quickstart and smoke scripts still need a non-placeholder PUBLIC_ORIGIN overlay.
2. GitHub-facing configuration anchors need correction.
3. Document the precedence and scope of static publisher.public_origin versus the authenticated forwarder's request origin. This is a body-level finding, so it does not appear in the unresolved inline-thread count.
4. Optional: warn when public_origin is outside the EC cookie scope.

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.

[ChristianPavilonis]

Pushed 6037fa4 to address the remaining blocker and the two documentation follow-ups.

• Fixed the quickstart/smoke public-origin overlays and replied to/resolved that inline thread.
• Fixed all three GitHub-facing configuration anchors and replied to/resolved that inline thread.
• Addressed @prk-Jr's body-level origin-precedence finding in both relevant configuration sections. Static publisher.public_origin controls general creative rewriting, sign/rebuild responses and injected runtime URLs. Inline SSAT/page-bids creatives retain their nonempty request-origin override, including an authenticated forwarder origin, and fall back to the static setting only when that origin is empty. This documents existing behavior; no forwarding, credential or CORS policy changed.
• Updated the PR description to distinguish the passing previous-head CI from fresh CI on this follow-up.

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.

Requesting re-review from @aram356, @prk-Jr and @dhruv8sh.

[prk-Jr]

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.