Repository navigation
Conversation
With --rewrite-host the proxy sends Host: TO but forwarded the browser's Origin: https://FROM unchanged, so upstream endpoints that verify a same-origin Origin against their own origin rejected proxied requests. Replace a single same-origin Origin with the TO origin (scheme from --upstream-plaintext, non-default port kept) so Origin and Host name the same authority. Cross-site, null, plain-http and duplicated Origin values pass through unchanged.
Derive the first-party origin from the inbound Host so non-443 sessions are rewritten too, and apply the rewrite only to Trusted Server's /_ts endpoints, so publisher and integration requests keep the browser's real Origin. Cover the behavior end to end through the proxy.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Reviewed 4ac34aaa05a36587b303c3bf9d4fc82ab5b6f4a4 against 7a0ecb4cbfa39af3d83f7e1ed2733d7eabdc4cb1. All five changed files and their affected callers and consumers were inspected. One P2 compatibility regression is posted inline.
Validation
cargo test --package trusted-server-cli --target x86_64-unknown-linux-gnu --lib commands::dev::proxy::: 113 passed.cargo test --package trusted-server-cli --target x86_64-unknown-linux-gnu --test proxy_e2e: 33 passed.cargo test --package trusted-server-cli --target x86_64-unknown-linux-gnu: passed, including 607 unit tests and the integration suites; 29 tests remained ignored.cargo fmt --all -- --check,cargo clippy-cli, andcargo build-axum: passed.- Inline Python runtime fixtures: 24 proxy header exchanges passed; the custom Didomi route regression was reproduced through the real Axum adapter against a local Origin-validating vendor fixture.
- All reported CI checks passed. Existing review feedback was empty.
Foreign, opaque, and duplicate Origins remained unchanged in the exercised cases. The trace handlers described in the PR are absent from this revision, so their actual Enable/End acceptance and cookie workflow were not independently verified. No repository files were modified.
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Scoped, well-tested fix: with --rewrite-host, same-origin Origin on /_ts requests is mapped to the TO origin so it agrees with Host. The scheme (http under --upstream-plaintext) and default-port omission line up with how the trace action check canonicalizes its expected origin from the request ingress, and keeping the rewrite out of publisher/integration paths preserves real Origin for vendors that forward it.
1 of the inline comments below carries a one-click GitHub
suggestion.
Non-blocking
- 🌱 Log when
Originis rewritten — see inline atcrates/trusted-server-cli/src/commands/dev/proxy/server.rs:792 - ⛏ Byte-exact
Origin/Hostcomparison — see inline atcrates/trusted-server-cli/src/commands/dev/proxy/server.rs:789
Cross-cutting / body-level findings
- 📝 Depends on unmerged trace endpoints — the
/_ts/trace/enableand/endroutes (and theirOrigincheck intrace/actions.rs) currently live onspec/mobile-ad-render-trace-endpoint, notmain. Worth noting for anyone verifying this againstmain. - 👍 Scoping and tests — restricting the rewrite to the
/_tsnamespace is the right call, and the coverage (non-443 port, missing/duplicatedHost,null/http/foreign/duplicateOrigin,/_tsxand/_ts-foolook-alikes, plus end-to-end through the echo upstream) is thorough.
CI Status
All 22 checks PASS (cargo fmt, cargo test, cargo test (axum native), cargo test (ts CLI, native) on ubuntu/macos, cross-adapter parity, cloudflare/spin checks, vitest, format-typescript, format-docs, integration + browser integration tests, CodeQL / Analyze).
Match POST /_ts/trace/enable and /_ts/trace/end exactly instead of the whole /_ts namespace, so integration routes an operator mounts under /_ts (such as a Didomi proxy_path of _ts/consent) keep the browser's real Origin. Log the rewrite at debug level.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Reviewed fbc7372bb79c8796b8cdbf956ca6408f889f4962 against 182fdf45c8a4e08fac68ea7b0b77f7f59a7284c1, from fix/dev-proxy-rewrite-origin into main. Inspected all five changed files and affected configuration, routing, forwarding, and test consumers.
No actionable new issues found.
Safety proof
-
Only eligible trace actions receive the upstream Origin; publisher/vendor requests retain their browser Origin.
- Highest evidence: Runtime.
- Evidence: All 34 proxy end-to-end tests passed. An additional 36 CLI exchanges exercised plaintext upstreams, Enable/End, query handling, non-default ports, foreign/null/http/duplicate/missing Origins, missing Host, near-miss paths, publisher/vendor paths, and rewrite-host on/off.
- Status: proven for the exercised inputs.
-
Rewritten actions pass Trusted Server authorization without weakening its other controls.
- Highest evidence: Path.
- Evidence: Inspected the declared dependency #1107 at
285c75face3dc23853f053162b20fc7e18734586. Its trace actions compare Origin against trusted ingress metadata and independently require the action header, same-origin Fetch Metadata, and an empty body. Those handlers are absent from this reviewed revision. - Status: unproven.
- Next check: After #1107 is integrated, exercise Enable/End and cookie observation through the proxy; verify foreign Origin and missing Fetch Metadata still return 403.
Validation and review context
cargo test --locked --package trusted-server-cli --target x86_64-unknown-linux-gnu --lib commands::dev::proxy::: 113 passed.cargo test --locked --package trusted-server-cli --target x86_64-unknown-linux-gnu --test proxy_e2e: 34 passed.cargo fmt --all -- --checkandcargo clippy-cli: passed.git diff 182fdf45c8a4e08fac68ea7b0b77f7f59a7284c1...fbc7372bb79c8796b8cdbf956ca6408f889f4962 --check: passed.python3 -with an inline plaintext echo fixture: 36 exchanges passed. Initial certificate validation failed under Python's strict X.509 policy because the unchanged proxy certificate lacks an Authority Key Identifier. The rerun retained CA-chain and hostname verification with strict-policy enforcement disabled.- CI: all 22 checks successful; confirmed the test workflow ran against the locked head.
- Existing feedback: inspected paginated reviews, inline comments, replies, issue comments, and thread resolution states. All three threads are resolved. The earlier custom Didomi-route regression is fixed and covered; no duplicate findings.
- Residual risk: actual trace authorization and browser cookie lifecycle remain unverified pending #1107. Docs formatting was not rerun locally because Prettier is unavailable; its CI check passed.
- No repository files were modified. Head/base revisions were rechecked before submission, and the worktree remained clean.
aram356
left a comment
There was a problem hiding this comment.
Summary
Translating Origin in the proxy is the right layer for this, and it works for --rewrite-host: Enable and End succeed against #1107's real handlers. The same mismatch still breaks the guide's plain-HTTP local setup, and the change depends on routes that aren't on main yet.
1 of the inline comments below carries a one-click GitHub
suggestion; use Commit suggestion to apply it. The other comments describe the fix in prose because it spans several hunks or files.
Blocking
🔧 wrench
- Plaintext upstream without
--rewrite-hoststill gets 403: see inline atcrates/trusted-server-cli/src/commands/dev/proxy/rewrite.rs:193
❓ question
- Merge after #1107?: see Cross-cutting below
Non-blocking
- 🤔 Hardcoded trace routes, and how far this can generalize: see inline at
crates/trusted-server-cli/src/commands/dev/proxy/server.rs:764 - ⛏ The trace actions reject query strings: see inline at
docs/guide/ts-dev-proxy.md:338 - 🏕
Originis missing from the header tables: see Cross-cutting below
Cross-cutting / body-level findings
-
❓ Merge after #1107?
POST /_ts/trace/enableand/enddon't exist onmain, and #1107 is still open (head117373e05). Merged first, the guide paragraph documents endpointsmaindoesn't serve, and nothing checks the hardcoded pair inrequires_upstream_originagainst the real routes. If #1107 renames or moves them during review, this fails closed (403) and no test notices. Can this wait for #1107, or land together with it? -
🏕
Originis missing from the header tables. The spec's §8.3 table (docs/superpowers/specs/2026-06-22-ts-dev-proxy-design.md:361) and the guide's table (docs/guide/ts-dev-proxy.md:356) are where readers look for what the proxy changes, and neither listsOrigin. A spec row could read:| `Origin` | `POST /_ts/trace/enable` and `/end` only: a single same-origin value becomes the upstream's origin (its scheme plus the `Host` sent) when that differs; otherwise unchanged | Trusted Server's trace actions require `Origin` to equal the origin they derive from their own scheme and `Host` |
-
📝 Checked against #1107's handlers. Running #1107's Axum server (
117373e05) behind this branch's proxy (c0982a5e2) with--upstream-plaintext --rewrite-host: Enable returns 200 and sets__Host-ts-console=1, End returns 200 and clears it, and a foreignOriginstill returns 403.
CI Status
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- integration tests: PASS
- prepare integration artifacts: 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
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- vitest: PASS
- format-typescript: PASS (required)
- format-docs: PASS (required)
- CLAUDE.md symlink guard: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (actions): PASS
- Analyze (python): PASS
- Analyze (javascript-typescript): PASS (both runs)
aram356
left a comment
There was a problem hiding this comment.
Summary
Follow-up to my earlier review, after looking at where this fix belongs: the root cause should be fixed in Trusted Server rather than by forging Origin in the proxy. The matching change is requested on #1107.
Blocking
🔧 wrench
- Send an authenticated forwarder header instead of rewriting
Origin: see inline atcrates/trusted-server-cli/src/commands/dev/proxy/server.rs:777
CI Status
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- integration tests: PASS
- prepare integration artifacts: 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
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- vitest: PASS
- format-typescript: PASS (required)
- format-docs: PASS (required)
- CLAUDE.md symlink guard: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (actions): PASS
- Analyze (python): PASS
- Analyze (javascript-typescript): PASS (both runs)
Resolve documentation overlaps while preserving authenticated forwarding, browser authority validation, and Origin handling. Update the configuration guide and example for the available forwarding credential option.
Brings #1107 to b4a20f4, its first revision in this RC that carries the trace implementation rather than only the spec. Integration decisions: - EdgeZero moves to 78439fd4 on spec/pre-dispatch-request-fidelity. It contains v0.0.9 (#1175) and the pre-dispatch ingress hooks #1107 and #1251 pin at 965d482f; no released tag carries both. - scheduleInitialAdInit takes (bids, slots, auctionDiagnostics, traceAuctionTransport). One builder emits both payloads, so the RC's diagnostics-only scripts are unchanged and trace moves to the fourth argument. recordTrustedServerOpportunity likewise takes auctionFacts then traceIdentity; #1107's positional tests and the bootstrap fallback were shifted to match. - The Fastly trace terminal path sends through the timed delivery function without Server-Timing, sandbox counters or access telemetry. - build.rs embeds trace assets from #1214's private bundle directory. The viewer rebuilds byte-identical; the pinned source digest changes only because build-all.mjs gained --out-dir. - The trace projection does not copy the RC's auction evidence fields, so the classification test marks them excluded. - The retained diagnostics toolbar resyncs its filter select after selectRequest. - Shim size guard raised to 55.5 KB for diagnostics plus trace.
Brings #1222 at 514a617. Integration decisions: - RequestInfo::from_request keeps #1251's source order (authenticated forwarder, immutable ingress, runtime origin, Host) and applies #1222's host syntax check to whichever host is selected. It returns BadRequest for a malformed or non-text selected host. Raw Forwarded and X-Forwarded-Host are never selected in the RC, so #1222's tests for those headers now assert they are ignored instead of rejected. - RC-only callers (DataDome protection, Fastly compat, Spin, forwarder, orchestrator and publisher tests) handle the fallible signature. DataDome only reads RequestInfo behind an authenticated origin and keeps its request_host fallback. - The first-party sign path prefers the authenticated forwarder scheme (#1251) and propagates #1222's host error. - The GTM rewriter takes #1222's per-document GtmScriptState, which supersedes main's ScriptTextAccumulator use from #1179; state still lives in IntegrationDocumentState, and #1179's two-document residue tests remain.
| value: args.listen.clone(), | ||
| })); | ||
| } | ||
| if !is_loopback && args.forwarder_secret_file.is_some() { |
There was a problem hiding this comment.
🤔 thinking: the loopback-listener check protects the token on the browser side, but the upstream side has no equivalent. --forwarder-secret-file is accepted with --upstream-plaintext to any TO, and with --insecure, so the token can leave the machine in cleartext or reach an unverified TLS peer. It is also stamped on every mapped upstream, including mappings that aren't Trusted Server.
Reproduced with this branch's ts binary mapped to a LAN address with --upstream-plaintext --forwarder-secret-file: the proxy starts normally and the upstream receives x-ts-forwarder-auth: <token> in the plain-text request head.
Whoever captures the token can make that Trusted Server service trust forged X-Forwarded-Host/X-Forwarded-Proto for any host under publisher.domain, which feeds generated public URLs and trace authorization. --basic-auth has the same exposure today, so this matches existing behavior, but the rest of this PR treats the token as a credential (loopback listener, sensitive header value, trailer stripping).
Refusing every non-loopback plaintext upstream would also block local Docker containers on bridge addresses, so a startup warning, or an explicit opt-in flag, may fit better. A sketch, placed after the --resolve pins are applied:
if forwarder_secret.is_some()
&& (args.upstream_plaintext || args.insecure)
&& rules.0.iter().any(|rule| !upstream_is_loopback(rule, &resolve))
{
log::warn!(
"--forwarder-secret-file token will cross a cleartext or unverified upstream connection"
);
}upstream_is_loopback would check the TO host, or its --resolve pin, for a loopback address. A sentence in the guide's "Authenticated forwarding" section saying the token goes to every mapped upstream would cover the multi-mapping case.
| /// Authentication header shared with Trusted Server's trusted-forwarder configuration. | ||
| pub const FORWARDER_AUTH_HEADER: &str = "x-ts-forwarder-auth"; |
There was a problem hiding this comment.
♻️ refactor: the CLI already depends on trusted-server-core, which exports this same constant as trusted_server_core::forwarder::FORWARDER_AUTH_HEADER ("The default credential header emitted by the development proxy"). Re-exporting it keeps one definition of the wire contract, and the super::config::FORWARDER_AUTH_HEADER call sites in server.rs and upstream/body.rs keep compiling.
| /// Authentication header shared with Trusted Server's trusted-forwarder configuration. | |
| pub const FORWARDER_AUTH_HEADER: &str = "x-ts-forwarder-auth"; | |
| /// Authentication header shared with Trusted Server's trusted-forwarder configuration. | |
| pub use trusted_server_core::forwarder::FORWARDER_AUTH_HEADER; |
Related, and probably for #1107: the server's auth_header is configurable while the CLI always sends this name. An operator who picks another name gets a silent fallback to the transport origin (trace 403s, URLs on TO). The docs say to use x-ts-forwarder-auth; a --forwarder-auth-header flag here, or a fixed name on the server, would remove the trap.
| BasicAuthNonLoopback { value: String }, | ||
| /// The forwarding token file could not be read. | ||
| #[display("cannot read --forwarder-secret-file")] | ||
| ForwarderSecretFile, |
There was a problem hiding this comment.
♻️ refactor: --basic-auth-file reports which file it couldn't read (ConfigError::BasicAuthFile { path }), but this variant drops the path, so a typo prints only "cannot read --forwarder-secret-file". The path isn't secret; only the file's contents are.
/// The forwarding token file could not be read.
#[display("cannot read --forwarder-secret-file `{path}`")]
ForwarderSecretFile { path: String }, let raw = std::fs::read(path)
.map_err(|_| ConfigError::ForwarderSecretFile { path: path.clone() })?;The matches! in forwarder_secret_file_read_failure_is_redacted then needs ConfigError::ForwarderSecretFile { .. }. Apply manually: the variant, its construction at line 365 and the test sit in different hunks. With all three changes, cargo fmt --check, cargo clippy-cli and the config tests pass.
| let addr: SocketAddr = std::env::var(FIXTURE_ADDR) | ||
| .expect("should supply the fixture listener address") | ||
| .parse() | ||
| .expect("should parse the fixture listener address"); |
There was a problem hiding this comment.
⛏ nitpick: cargo test -p trusted-server-cli -- --ignored runs this fixture directly, with no parent and no env vars, so it fails with "should supply the fixture listener address". Returning early makes it a no-op outside the parent's child process.
| let addr: SocketAddr = std::env::var(FIXTURE_ADDR) | |
| .expect("should supply the fixture listener address") | |
| .parse() | |
| .expect("should parse the fixture listener address"); | |
| // When run directly, for example by `cargo test -- --ignored`, there is | |
| // no parent test to serve. | |
| let Ok(addr) = std::env::var(FIXTURE_ADDR) else { | |
| return; | |
| }; | |
| let addr: SocketAddr = addr | |
| .parse() | |
| .expect("should parse the fixture listener address"); |
| path: String, | ||
| } | ||
|
|
||
| /// Parses `host=..;orig=..;fwd=..;origin=..;path=..` echoed by the upstream. |
There was a problem hiding this comment.
⛏ nitpick: the echo now also reports proto and auth.
| /// Parses `host=..;orig=..;fwd=..;origin=..;path=..` echoed by the upstream. | |
| /// Parses `host=..;orig=..;fwd=..;origin=..;proto=..;auth=..;path=..` echoed by the upstream. |
| signs a guessed CONNECT host. Each request must match its own mapping. | ||
| Unmatched blind tunnels and plain HTTP forwarding keep their existing behavior. | ||
|
|
||
| Without the server opt-in, forwarded metadata does not establish a trusted |
There was a problem hiding this comment.
🏕 camp site: the troubleshooting table has no row for the symptom this PR fixes, so someone who hits it has nothing to search for. A row such as:
| Trace **Enable** / **End** returns `403` through the proxy | The upstream compares `Origin` with its own transport origin, which `--rewrite-host` or `--upstream-plaintext` changes. | Configure `[trusted_forwarder]` on the upstream and pass `--forwarder-secret-file`; see [Authenticated forwarding](#authenticated-forwarding). |
Summary
Allow
ts dev proxyto authenticate the browser-facing origin while preserving the browser's original Origin header. This lets trace Enable/End work when the proxy rewrites Host or sends plaintext to its upstream, without forging a trace-specific Origin.Changes
--forwarder-secret-file. Validate a single secret of at least 32 ASCII graphic bytes before listening, redact diagnostics and mark the outgoing credential header sensitive. Credential injection requires a loopback listener.X-Forwarded-Host,X-Forwarded-Proto: httpsandX-TS-Forwarder-Authafter hop-by-hop sanitation. Conflicting or ambiguous authority is rejected; CONNECT fallback is never authenticated.Dependency
Stacked on #1107 (
spec/mobile-ad-render-trace-endpoint), which supplies optional authenticated-forwarder configuration, secret-store resolution and shared public-origin handling. The CLI includes that server branch sots config pushunderstands the new configuration and its secret metadata.Configure the server's authentication header as
x-ts-forwarder-auth; itsshared_secretis a secret-store key, while the CLI file contains the actual matching secret. Deploy the server configuration before using the flag. Keep both PRs open: #1107 owns server trust and trace behavior; this PR owns the CLI companion.Validation
All required local CI gates pass on the stacked branch: all eight lint gates, every adapter suite, parity, JS build/tests and formatting, native build-digest tests/lint, core documentation, native CLI suites and adapter builds including Fastly/Spin release artifacts. Final CLI results: 731 unit tests, 41 proxy wire tests and three actual proxy-to-server regressions pass. Explicit TLS provider setup makes the new shared fixture dependency deterministic; the isolated TLS wire test and serial 111-test proxy unit suite also pass. The actual
ts config validatecommand accepts the forwarder secret-store key reference. The combined regressions use the production Axum HTTP listener behind the real proxy, covering encrypted/plaintext upstreams, rewritten/preserved Host, a public non-default port, Enable/End cookies, rejected Origins/queries/duplicates, missing credentials, server opt-out and transport fallback. Publisher tests check unchanged Origin, stripped credentials, generated public URLs and HTTPS protocol-relative signing.Independent reviewers checked the CLI implementation, found and verified a streaming trailer credential leak fix, and reviewed the real-server fixture. Server ingress and configured identify CORS are covered by the companion PR.
Closes #1250
The new CodeQL alert #204 was independently reviewed and dismissed as a false positive. SHA-256 only creates temporary fixed-size bearer-token digests for comparison; neither digest is stored or exposed. Operator documentation requires cryptographically random token generation and distinguishes minimum length from entropy. This assessment follows CodeQL's guidance for the distinction between password storage and other hashing uses.
Standalone runtime verification: launched the actual
trusted-server-axumserver andts dev proxybinaries, loading server configuration through the normal blob envelope and environment-backed secret store. All 56 live HTTPS requests passed the expected checks across plaintext/TLS upstreams with--rewrite-hoston/off. Enable → separate active-state request → publisher document with active trace context → End → separate inactive-state request succeeded with unchanged Secure/HttpOnly/host-only/SameSite=Lax cookies. Public URLs retain browser HTTPS authority and port; protocol-relative signing uses HTTPS; publisher and custom Didomi routes preserve browser Origin and receive no forwarding credentials. Foreign hosts/Origins, duplicate Origin, action queries, nonempty bodies, missing/wrong credentials, and server opt-out reject without cookie mutation. Browser-to-proxy TLS uses the isolated generated CA; the self-signed local upstream TLS relay uses--insecure. An independent subagent audited the response and recorded upstream evidence. This does not establish physical mobile/browser UI or deployed-runtime acceptance.The standalone run found a TLS-provider startup panic when workspace features enable both providers. The proxy now calls the existing idempotent provider initializer before TLS setup; a fresh-process regression requires a real PAC HTTP response. CI also exposed a redaction-test sentinel collision with a backtrace symbol; the distinct short invalid token retains the complete Report Debug redaction assertion and passes with short/full backtraces.