Repository navigation
Conversation
jevansnyc
left a comment
jevansnyc
left a comment
There was a problem hiding this comment.
Review of the design spec. No code changes here, so this is internal consistency plus whether the stated contracts hold against what is in the repo today (gpt_diagnostics.rs, publisher.rs, auth.rs, prebid_eids.rs, and the TS diagnostics store/types).
Seven issues inline. The first three change the design rather than the wording:
- The
ts-auc-token shape does not match the existing producer, so correlation never joins. TraceGptDiagnosticsV1as specified cannot satisfy acceptance criterion 8.- Trace paths terminate ahead of authentication, which carves an exemption out of the
^/_tsnamespace thatauth.rssays should not exist.
The remaining four are bounded-scope corrections to the cookie lifetime claim, the TSJS gate, a capture_status gap, and redaction consistency.
Mechanical checks came back clean: cookie names (ts-ec, ts-eids, ts-tester, __Host-ts-console), the 8 KiB ts-eids cap (MAX_EIDS_COOKIE_BYTES), and the callback-issue reason values all match what the spec assumes.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
A design-only PR adding a single 1852-line spec for a /_ts/trace mobile
diagnostics endpoint. The document is unusually well-grounded: field names,
constants, and several non-obvious hazards (the AuctionRequest.id leak, the
JA4/H2 fingerprint exclusion, bootstrap-fallback argument tolerance, the
unpopulated asn) are verbatim correct against the code. All seven findings
from the previous round are genuinely resolved in e3f371f8c, and I re-verified
each rather than re-raising it.
The blocking findings below are places where the spec mandates behavior the
pinned platform cannot express, or where it assumes adapter defaults that do the
opposite of what sections 8, 12.3, and 13 require. Because this is a design
document, each one is cheaper to fix now than after it becomes four
implementation PRs.
6 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 change in prose because the fix is a new
validation hook or spans more than one contiguous range.
Blocking
🔧 wrench
- Two-second request-body deadline is unimplementable — see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:494 HEAD /_ts/traceis proxied to the publisher origin — see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:468- Router-level 405 carries no
Allowand no hardening headers — see
inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:506 - The mandated configuration validation cannot fire — see
Cross-cutting below
Non-blocking
🤔 thinking / 📝 note
AuctionSlot.extpresented as an existing type — see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:908- Slot tokens described as existing; verbatim-comparison rule conflicts with
normalizedAuctionId— see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:819 - Inherited and new cookie caps are presented as one list — see inline
at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:654 - CSP blocks the favicon, and leaves
blob:and inline styles unaddressed
— see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:1326 - Auth contract overstates rule composition; existing JA4 route is an
auth-bypass precedent — see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:1364 - Container-nesting cap has zero headroom — see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:1092 sourceenum diverges from the existingAuctionSource— see inline
at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:841- Deprecated
/__ts/page-bidsalias is uncovered — see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:996 - GPT projection prose is not a usable allowlist — see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:715
Cross-cutting / body-level findings
-
🔧 The configuration validation this spec mandates cannot fire as a
validatorrule. Section 8 (line 452) requires that
“trace_page_enabled = truerequiresenabled = true; invalid
combinations fail configuration validation”, and section 14.1 (line 1433)
requires a test that configuration “rejects trace-page enablement without
GPT diagnostics”.One correction first, in fairness to the design: adding
trace_page_enabled
to TOML today is already a hard error on both the enabled and disabled
paths, becausevalidate_disabled_schema
(crates/trusted-server-core/src/settings.rs:247-253) forgives only errors
beginning"missing field ", and an unknown-field error is not forgiven.
That part is correctly fail-closed.The real gap is narrower but still real. Once
trace_page_enabledis a
legitimate field,IntegrationSettings::get_typedreturns before validation:// crates/trusted-server-core/src/settings.rs:348-350 if !config.is_enabled() { return Ok(None); } config.validate().map_err(|err| { /* :352 */ })?;
So the exact combination the spec wants rejected —
enabledomitted (serde
defaultfalse) together withtrace_page_enabled = true— resolves to
Ok(None)and a#[validate(schema(...))]rule never runs. Note
#[validate(schema(...))]does otherwise work in this crate
(settings.rs:2737), so the failure mode is silent rather than obvious.The precedent that fits is
validate_js_asset_proxy_config
(crates/trusted-server-core/src/config.rs:280-299): it reads the raw JSON
and callsvalidate()outside the enabled gate, runs from both the deploy
(config.rs:250) and runtime (config.rs:271) paths, and is proven by
validate_rejects_invalid_disabled_js_asset_proxy_assets
(config.rs:1424). Naming that pattern here would keep an implementer from
writing a rule that never fires.On sourcing: the “invalid enabled config must not be silently
logged-and-disabled” rule is not actually inAGENTS.mdor
CONTRIBUTING.md. Its canonical statement is a HIGH-severity finding in
docs/superpowers/specs/2026-03-11-production-readiness-report-design.md:217-233.
If this spec relies on it as normative, that is worth making explicit, since
it was never promoted into the contributor docs. -
📝 Section 12.4's cache invariant is already implemented, in two
places on Fastly. The spec asks that “tests must prove that late
response-header handlers cannot make traced content publicly cacheable”
(line 1355). That guarantee exists today:
apply_response_headers_with_cache_privacy
(crates/trusted-server-core/src/response_privacy.rs:163-172) skips operator
response_headersentries forCache-Controland edge-cache header names
whenever the response is already uncacheable. On Fastly there is a second
layer after it —apply_terminal_response_effects
(crates/trusted-server-adapter-fastly/src/main.rs:371-392) re-runs the
privacy guards, because EC finalize and filter effects can addSet-Cookie
later; the regression test is
late_filter_effects_cannot_make_an_assembled_response_public. Citing both
would let the implementation plan reuse the mechanism instead of rebuilding
it. Note also thatapply_finalize_headersis terminal on Axum, Cloudflare,
and Spin but not on Fastly. -
📝 A reserved-namespace classifier already exists, at the fallback
boundary. Section 8 requires rejecting trailing slashes, extra segments,
repeated separators, encoded separators, and ambiguous dot segments beneath
the reserved namespace (lines 509-514).deny_admin_diagnostic_fallback
(crates/trusted-server-core/src/ec/admin.rs:179-196) already solves that
shape for/_ts/admin, including a bounded percent-decode-to-fixed-point
withMAX_PERCENT_DECODE_ROUNDS = 4. It runs first inside each adapter's
fallback rather than at the front door, but it is the pattern to lift
forward rather than re-derive. -
📝 Adapter-parity caveat for section 8. Several existing
/_ts/*
routes (/_ts/api/v1/*,/_ts/set-tester,/_ts/clear-tester,
/_ts/debug/ja4) are registered only on Fastly. Section 8 requires the trace
namespace on all four adapters, so trace routing cannot follow that
precedent — worth stating, since it affects the sequencing in section 17.
CI Status
All checks passing on e3f371f8c.
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
- cargo fmt: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- CLAUDE.md symlink guard: PASS
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Second review pass, against head 7a82a944a (base main). The previous pass
raised twelve findings; 14c274cba addresses all twelve, and I verified each
fix against the code at this head rather than taking the replies at face value.
Two are worth calling out as substantively verified rather than merely
annotated. The new section 9.3 allowlist table is genuinely exhaustive: diffing
it mechanically against GptDiagnosticsRequestCycle gives 30 real members, 28
allowlisted, and exactly adManager and previousCreativeId excluded, with no
listed name that does not exist on the real type; the coverage keys, counters,
metadata, binding, and durations rows all match their interfaces exactly.
And the container-nesting cap moved 8 to 10, which restores two levels of
headroom over the deepest legitimate path (gpt_diagnostics.slots[].requests[].requestedSizes[][w], level 8).
One new blocking finding, introduced by the body-handling rewrite itself: the
replacement mechanism cannot produce the 413 the same bullet mandates, cites
a precedent that uses a different API, and misreads an empty body on the one
adapter that streams it. Details inline.
1 of the inline comments below carries a one-click GitHub
suggestion.
The other two are prose: one names a trigger condition, one corrects an
explanation.
Blocking
🔧 wrench
- Body-emptiness mechanism cannot return
413and misreads streamed bodies
— see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:516
Non-blocking
📝 note / ⛏ nitpick
- Blob revoke trigger is undefined — see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:1432 - Favicon suppression is attributed to CSP rather than the
<link>element
— see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:1428
Cross-cutting / body-level findings
-
📝 Prior-round findings verified as resolved. Recorded so the next
pass does not re-litigate them: the unimplementable two-second body deadline
and one-byte read are gone; configuration validation now specifies a
raw-config hook modelled onvalidate_js_asset_proxy_config
(crates/trusted-server-core/src/config.rs:280-299) and correctly explains
why a schema validator never fires for a disabled integration;HEADnow
requires explicit registration on all four adapters, citing
dispatch_head_on_named_get_route_falls_through_to_publisher_fallback; the
405 contract now requires the trace responder to supplyAllowand the
section 12.3 hardening itself rather than inheriting a router error; the CSP
gainedimg-src data:and a class-toggle-only styling rule; the auth section
now states first-match-wins and that the fail-closed backstop covers only
/_ts/admin; the deprecated/__ts/page-bidsalias is covered, and the TSJS
retry it refers to is real (crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1675-1692);
and the slot-token, cookie-cap,AuctionSlot.ext, andsource-enum
paragraphs now distinguish new design from existing code. I re-verified the
underlying code claims at this head. -
📝 One claim in the rewrite is accurate and load-bearing enough to
keep. The statement that Spin buffers the body while Axum buffers only JSON
checks out against the pinned dependency: Spin reads the full body into
Body::Onceunconditionally (edgezero-adapter-spin/src/request.rs:74-80),
and Axum branches on content type
(edgezero-adapter-axum/src/request.rs:21-35). That asymmetry is what drives
the blocking finding above, so it is worth keeping the sentence even after
the mechanism changes.
CI Status
No failing checks. Several are still running against the merge commit pushed
shortly before this review; they are recorded as pending rather than treated as
findings.
- cargo fmt: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- CLAUDE.md symlink guard: PASS
- Analyze (actions): PASS
- Analyze (rust): PENDING
- Analyze (javascript-typescript): PENDING
- integration tests: PENDING
- browser integration tests: PENDING
- CodeQL: SKIPPED
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Third review pass, against head c4d664aab (base main). Commit 155d3b09f
resolves all three findings from the previous round, and I verified each fix
against the pinned edgezero v0.0.8 source rather than against the replies.
The body-validation rewrite is the substantive one. It now matches both Body
variants explicitly, requires a clean EOF to prove a streamed body empty, and
names both traps from the last round — telling implementers not to copy
into_bytes().unwrap_or_default() and not to propagate
into_bytes_bounded errors as the response. I checked that this is actually
implementable: Body is a public, non-#[non_exhaustive] enum
(edgezero-core/src/body.rs:14-17), core already matches both variants in the
exact prescribed shape (crates/trusted-server-core/src/publisher.rs:201-211),
and async handlers are available on all four adapters, with Fastly driving them
under block_on. The favicon causality and the blob-revoke timing are both
correct now.
Two findings remain. Neither is a defect in the new text; both are places where
the spec is not yet self-contained. Since this document is the contract four
implementation PRs will be built from, closing them here is cheaper than
discovering them during implementation.
Both inline comments carry a one-click GitHub
suggestion.
Blocking
🔧 wrench
- Section 13 omits every body-validation status code — see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:1516
Non-blocking
📌 out of scope
- Deferred-cleanup rule diverges from the console export that ships today
— see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:253
Cross-cutting / body-level findings
-
📝 Prior-round findings verified as resolved, recorded so a fourth
pass does not re-litigate them. The body-validation mechanism no longer
depends on an API that cannot produce the mandated413, and it no longer
inherits theinto_bytes().unwrap_or_default()behavior that would read a
non-empty streamed body as empty — which matters, because a bodiless
fetch()POST carries noContent-Typeand Axum streams exactly that case
(edgezero-adapter-axum/src/request.rs:24-35, pinned by its own tests at
:97-118and:167-181). Favicon suppression is now attributed to
<link rel="icon">withimg-src data:only permitting the load. Blob
cleanup is now a 1000 mssetTimeoutwith per-download scheduling, and the
accompanying test lines are implementable:vi.useFakeTimersis already used
across nine JS test files, andcreateObjectURL/revokeObjectURLare
already mocked atapi.test.ts:540-551. -
📝 One concern investigated and dismissed, noted so it does not
resurface as a finding later. The 1000 ms revoke timer raises the question of
what happens if the document is torn down before it fires. No specified flow
navigates during a pending download: the section 6.2 storage-failure download
happens on the publisher page, which that section says "remains in place",
and the section 6.3 download happens on/_ts/trace, whose own
Copy/Share/Download and clear actions do not navigate. Line 1729's "same-tab
navigation occurs only after a successful write" governs the earlier handoff,
before any download exists. If a document were torn down anyway, the object
URL is reclaimed with it. No change needed.
CI Status
All 20 checks passing on c4d664aab.
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
- cargo fmt: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- CLAUDE.md symlink guard: PASS
prk-Jr
changed the title
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
When Cookie field multiplicity is not preserved, any comma anywhere in the header made all four trace cookie results unavailable. Google Identity's JSON-valued g_state cookie contains commas and is set on every page load of publishers that use One Tap, so tracing could never activate for those visitors. A comma is now ambiguous only when it could change a reserved result: inside a reserved cookie's own segment, where it could truncate the value, or directly before a reserved name, where it could hide an occurrence inside another cookie's value. Commas inside unrelated values cannot do either. The replacement-character rule is unchanged. Update the four fixtures that relied on the old rule to use a comma that still borders the session cookie, and align the spec and operator guide.
The trace page listed every retained field in one long column, so a reader on a phone had to scroll through many screens of label/value pairs, raw float timings and Unavailable rows to learn whether an ad slot filled. Open the report with a What happened summary: a headline, a short reading computed only from retained fields, count tiles and fill chips. A Needs attention list links to empty, fill-unknown, incomplete or creative-failure slots. Each GPT slot gets a card with its fill state, a proportional response/render/viewable timing bar, its exact server-slot link and a collapsed All details disclosure. Server auctions stay expanded; request, cookie and coverage sections collapse. Unavailable fields fold behind N fields unavailable, timings round to one decimal and capture times read as UTC. The summary never names an auction winner, counts only bidder-role bids so mediator echoes are not double counted, names the outcome of auctions that did not complete and applies no latency thresholds. Timing bars use stylesheet flex-weight classes, not inline styles, so the strict style-src 'self' CSP holds, and the exact milliseconds stay in text and in the bar's accessible label. Light and dark themes follow the system setting. Split the 680-line viewer into a pure, unit-tested summary model (view-model.ts), DOM primitives (report-dom.ts), detail sections (report-details.ts) and the page layout (report-view.ts). Rebuild the v1 trace assets and manifest, drop the setup shell's duplicated instruction, and describe the page in spec section 6.3.
Fastly main skips client-IP sanitization when the native request path is a trace path, and the router hook later classifies the converted URI. Add a regression test that runs encoded, dot-segment, doubled-slash, case and lookalike paths through both and asserts they agree, so a future conversion change cannot route unsanitized forwarded headers into ordinary dispatch. Drop the repeated activation value from the bounded-lifetime test; only 1 and true activate diagnostics.
Adopt the reusable-sandbox Fastly entry point from main (#1179). Trace ingress and trace-path classification are captured first in each handle_request call, before the health shortcut and any native mutation, and are passed into edgezero_main. Trace terminal responses are sent without sandbox counters so their hardened headers stay exact. Pin EdgeZero to 965d482f, the pre-dispatch fidelity branch merged with EdgeZero main, so the reusable application lifecycles and the ingress hook are both available. Re-resolve only the EdgeZero lock entries from main's lockfile. Keep both the trace regression and reusable-sandbox CI steps.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
This review covers one design point, not the whole PR: which origin the trace actions trust when a proxy in front of Trusted Server rewrites Host.
Blocking
🔧 wrench
- Trust the browser-facing origin from an authenticated forwarder: see inline at
crates/trusted-server-core/src/trace/actions.rs:77
CI Status
- trace runtime and browser acceptance: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser 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)
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Reviewed cf36844be0f15380bfbda8ba3c2f4e3a5f43187d against da31a215e62a8201610de367ee06d572708952df, covering all 152 changed files and affected callers and consumers. The default-off trace workflow, browser-local report, and authenticated public-origin forwarding have no verified production defect from this review. Approval includes one non-blocking inline finding.
Validation
Focused local checks passed: 1,290 JavaScript tests, 12 asset and pinned-Prebid artifact tests, 91 core trace tests, 100 broader trace-filtered core tests, both explicitly selected emitted-script tests, 7 forwarder tests, and 27 adapter-parity tests. These core filters overlap. Rust formatting, diff whitespace, and ESLint on 70 changed source/test/build files also passed.
Authentication and rejected-action controls were exercised without cookie mutation. Missing, malformed, and throwing diagnostic observations preserved ordinary advertising in the tested paths. Inspected CI run 37910480115 passed real runtime boundaries and all-four-adapter browser workflows; its tested merge tree matches the reviewed head tree.
Existing feedback was checked to avoid duplicate findings. Physical mobile-device, actual session-restoration, and deployed HTTPS/CDN/CSP acceptance remain unverified here. Full runtime/browser suites were not rerun locally. The reviewed checkout remains unchanged.
| target.__tsjs_trace_active = active; | ||
| installGptStub(); | ||
| installGptDiagnosticsRuntime(target); | ||
| const root = shadow.mock.results.at(-1)?.value as ShadowRoot | undefined; |
There was a problem hiding this comment.
P3 / Low: New composition tests exceed the configured JavaScript library level
This call and the equivalent call at line 129 use Array.at(), but tsconfig.json declares ES2020 libraries. npx tsc --noEmit reports TS2550 at both new locations. Checking the exact base with the same dependencies confirms these two diagnostics are new; the other compiler failures predate this PR.
Both new tests therefore fail to type-check under the declared configuration. Vitest still passes because its type-check gate selects only types.test.ts files and ignores other source errors. This is non-blocking and does not establish a production defect.
Use indexed access here and at line 129, retaining the latter's ShadowRoot assertion:
| const root = shadow.mock.results.at(-1)?.value as ShadowRoot | undefined; | |
| const root = shadow.mock.results[shadow.mock.results.length - 1]?.value as | |
| | ShadowRoot | |
| | undefined; |
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Adds the default-off mobile trace workflow and the opt-in [trusted_forwarder] origin authentication. The forwarder boundary is careful — frozen first decision, constant-time digest comparison, publisher-domain bound, credentials and raw forwarding fields stripped before every preflight return — but the trust-model change it brings to Axum and Cloudflare isn't recorded for operators.
2 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 findings describe the fix in prose because the change touches files outside the diff.
Blocking
🔧 wrench
- Record the forwarding trust change and
[trusted_forwarder]in CHANGELOG — see inline atCHANGELOG.md:32
Non-blocking
♻️ refactor
- Pin the Spin tarball digest — see inline at
.github/workflows/integration-tests.yml:211
👍 praise
- Forwarder boundary design — see inline at
crates/trusted-server-core/src/forwarder.rs:63
Cross-cutting / body-level findings
-
🤔
ts dev proxyguide describes forwarding behaviour this PR removes —docs/guide/ts-dev-proxy.mdlines 289–295, 315–316 and 323–335 (and the comment atcrates/trusted-server-cli/src/commands/dev/proxy/server.rs:713-720) still say core prefersX-Forwarded-Hostand that only Fastly/Spin strip it. After this PR every adapter ignores and strips it unless[trusted_forwarder]authenticates it. #1251 rewrites these, but it's stacked on this branch; if #1107 merges first,maindocuments behaviour the code no longer has. Either land both together or apply a minimal correction here, e.g. replacing lines 289–295 with:The proxy always sends
X-Forwarded-Host: <FROM>(the production hostname). Trusted Server ignores and strips unauthenticatedForwarded/X-Forwarded-*fields on every adapter; it usesX-Forwarded-Host/X-Forwarded-Protofor first-party URL rewriting only when the upstream configures[trusted_forwarder]and the proxy sends the matching credential. Without that, first-party URLs follow the upstream's ownHostand transport scheme.Apply manually — the file is outside this PR's diff.
CI Status
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- trace runtime and browser acceptance: PASS
- integration tests: PASS
- prepare integration artifacts: PASS
- CodeQL: PASS
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS (both runs)
- Analyze (rust): PASS
- Analyze (python): PASS
- cargo fmt: PASS (required)
- format-docs: PASS (required)
- format-typescript: 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
- CLAUDE.md symlink guard: PASS
|
|
||
| ### Security | ||
|
|
||
| - Reserve the application-visible `/_ts/trace*` prefix locally on every adapter, including when mobile tracing is disabled. Existing authentication runs first; disabled or unknown trace routes then return a local `404` instead of forwarding to the publisher. |
There was a problem hiding this comment.
🔧 wrench — Record the forwarding trust change and the new [trusted_forwarder] section
This PR changes what decides the public host and scheme on every adapter, and the changelog doesn't say so:
RequestInfo::from_requestno longer reads unauthenticatedForwarded/X-Forwarded-Host/X-Forwarded-Proto(the fallbacks andtest_request_info_x_forwarded_host_precedence/test_request_info_chained_proxy_scenarioare removed inhttp_util.rs), andprepare_trusted_forwarderstrips all three before routing on every adapter.- On
main, Fastly and Spin already stripped them, but Axum and Cloudflare did not, so a client-suppliedX-Forwarded-Hoststeered first-party URL rewriting there. Closing that is a security fix worth recording. - The operator-visible side: Axum's EdgeZero ingress origin is always
http://<Host>, so an Axum service behind a TLS-terminating proxy — includingts dev proxyagainst a local Axum — now emitshttp://first-party URLs unless[trusted_forwarder]is configured. [trusted_forwarder]is a new top-level section with a secret-store reference, but### Addedonly mentionstrace_page_enabled.
Suggested entry under ### Security, covering both the fix and the opt-in:
| - Reserve the application-visible `/_ts/trace*` prefix locally on every adapter, including when mobile tracing is disabled. Existing authentication runs first; disabled or unknown trace routes then return a local `404` instead of forwarding to the publisher. | |
| - Reserve the application-visible `/_ts/trace*` prefix locally on every adapter, including when mobile tracing is disabled. Existing authentication runs first; disabled or unknown trace routes then return a local `404` instead of forwarding to the publisher. | |
| - Stop deriving the public host and scheme from unauthenticated `Forwarded`, `X-Forwarded-Host` and `X-Forwarded-Proto` fields on every adapter, and strip those fields before routing. Fastly and Spin already removed them; Axum and Cloudflare previously let a client-supplied value steer first-party URL rewriting. A deployment behind a TLS-terminating or Host-rewriting proxy, including `ts dev proxy` against Axum, must now configure the opt-in `[trusted_forwarder]` section, which authenticates one publisher-domain-bounded `X-Forwarded-Host`/`X-Forwarded-Proto` pair with a secret-store-resolved shared secret; without it, first-party URLs use the transport host and scheme. |
| run: | | ||
| trace_spin_dir="$RUNNER_TEMP/trace-spin" | ||
| mkdir -p "$trace_spin_dir" | ||
| curl --fail --location --silent --show-error https://github.com/spinframework/spin/releases/download/v4.0.0/spin-v4.0.0-linux-amd64.tar.gz --output "$trace_spin_dir/spin.tar.gz" |
There was a problem hiding this comment.
♻️ refactor — Pin the Spin tarball digest
This step is named "Install the verified Spin runtime", but nothing verifies the download, and the acceptance job then executes that binary. Pinning the SHA-256 from Spin's checksums-v4.0.0.txt release asset makes a replaced or corrupted tarball fail closed:
| curl --fail --location --silent --show-error https://github.com/spinframework/spin/releases/download/v4.0.0/spin-v4.0.0-linux-amd64.tar.gz --output "$trace_spin_dir/spin.tar.gz" | |
| curl --fail --location --silent --show-error https://github.com/spinframework/spin/releases/download/v4.0.0/spin-v4.0.0-linux-amd64.tar.gz --output "$trace_spin_dir/spin.tar.gz" | |
| echo "e705c9bfd9484a9175f392a116856680862a40f31d1a85618bed331f34ffccfa $trace_spin_dir/spin.tar.gz" | sha256sum --check --strict |
| /// ```ignore | ||
| /// prepare_trusted_forwarder(&mut request, &settings); | ||
| /// ``` | ||
| pub fn prepare_trusted_forwarder(request: &mut Request<Body>, settings: &Settings) { |
There was a problem hiding this comment.
👍 praise — Freezing the first forwarding decision in a typed extension, and stripping the credential plus raw Forwarded/X-Forwarded-* fields before every preflight return, keeps the secret out of publisher and vendor requests on all four adapters. The rejection matrix in capture_rejects_unauthenticated_ambiguous_and_malformed_forwarding (lookalike suffixes, folded lists, ports, whitespace) is the right coverage for this boundary.
This branch has not been deployed
6 participants
Summary
Add a default-off, same-tab mobile ad-rendering trace workflow with deliberate Enable/End controls, bounded browser-local capture, a consolidated viewer and JSON export. Preserve ordinary advertising when transport, correlation, storage or browser APIs are unavailable.
Changes
Authenticated forwarding review corrections
An optional
[trusted_forwarder]section authenticates oneX-Forwarded-Host/X-Forwarded-Protopair using the configured authentication header and a secret-store-resolved shared secret. The credential requires at least 32 ASCII graphic bytes; comparison uses constant-time fixed-size digests. Public hosts must match the configured publisher domain or a proper subdomain, with validated ports. Missing, malformed, duplicate or foreign metadata falls back to transport facts.The authenticated public origin is separate from
RequestIngress: browser Origin must match the public origin, while received URI/Host still must match immutable transport ingress. Forwarding never upgrades request-target provenance, CookieHealth, header fidelity or TLS evidence. Preparation freezes acceptance or rejection and strips default/configured credentials and raw forwarding fields before all trace and ordinary routing paths.Fastly captures forwarding before native sanitation; the shared pre-dispatch hook handles all adapters. Public HTML, Flight/SPA URLs, cache keys, protocol-relative signing and DataDome public fields use the same effective origin. Backend targets, configured publisher identity, identify CORS and actual TLS facts keep their existing rules. Spin retains its trusted runtime URL in separate typed metadata when ingress has no origin.
The CLI companion is #1251. It supplies credentials with
--forwarder-secret-fileand preserves browser Origin, including publisher/vendor requests. Deploy this server configuration first; the CLI PR is stacked on this branch so its configuration schema and secret metadata include this option.Dependencies and release acceptance
The branch pins immutable EdgeZero revision
965d482fe1066279ea409a2e629aa13c49de98d0from stackpop/edgezero#403, which remains open. Repin to its merged revision or release when that dependency lands.Trace remains disabled by default. Physical iOS Safari/Android Chrome, actual browser session restoration, deployed HTTPS cookie workflows and operator staging/privacy/CDN/CSP acceptance remain release gates in the existing operator guide and implementation plan. Authenticated forwarding supports HTTPS offload without weakening cookie attributes; local wire tests do not replace deployed browser acceptance.
Validation
All required local CI gates pass on server commit
92dd20341, including eight adapter/CLI/codegen lint gates, all adapter suites, parity and formatting; native core tests pass 3,057/3,057 and parity passes 27/27. JS build and 1,757 tests pass. Native build-digest tests/lint, core documentation, native CLI tests, Fastly/Axum/Cloudflare builds and Fastly/Spin release artifacts pass. The CLI suite was rerun sequentially after a shared-artifact rustdoc interruption and passed. The companion PR adds three passing actual proxy-to-server regressions. Regression coverage includes strict configuration/secret resolution, authentication and origin parsing, all-adapter trace/public-origin behavior, credential removal, immutable cookie/fidelity evidence, Spin runtime metadata, configured identify CORS, public ports, snapshots and cache partitioning. Independent implementation and architecture reviewers checked each corrective surface; actionable findings were fixed and reviewed again.Closes #1050
Closes #1108
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.