Skip to content

Add JavaScript asset proxy integration - #742

Open
ChristianPavilonis wants to merge 13 commits into
mainfrom
simplify-js-asset-proxy-spec
Open

Add JavaScript asset proxy integration#742
ChristianPavilonis wants to merge 13 commits into
mainfrom
simplify-js-asset-proxy-spec

Conversation

@ChristianPavilonis

@ChristianPavilonis ChristianPavilonis commented May 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Add the JS Asset Proxy engineering spec and implement the js_asset_proxy integration
  • Support exact configured first-party GET routes for enabled assets, exact <script src> rewriting, disabled assets, and blocked script removal
  • Proxy configured HTTPS origins with streaming passthrough, no EC forwarding, a small request-header allowlist, fixed User-Agent, and controlled downstream response headers
  • Gate response streaming on adapter support so unsupported runtimes use the buffered fallback
  • Pin proxied asset responses to application/javascript; charset=utf-8 with X-Content-Type-Options: nosniff
  • Generate disabled-by-default JS asset proxy candidates from ts audit

Related

Closes #762

Verification

  • cargo fmt --all -- --check
  • cargo clippy-fastly && cargo clippy-axum && cargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasm
  • cargo test-fastly && cargo test-axum && cargo test-cloudflare && cargo test-spin
  • ./scripts/test-cli.sh
  • cargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity
  • cd crates/trusted-server-js/lib && npx vitest run
  • cd crates/trusted-server-js/lib && npm run format
  • cd docs && npm run format

Comment thread docs/superpowers/specs/2026-04-01-js-asset-proxy-design.md
Comment thread docs/superpowers/specs/2026-04-01-js-asset-proxy-design.md Outdated
Comment thread docs/superpowers/specs/2026-04-01-js-asset-proxy-design.md Outdated
Comment thread docs/superpowers/specs/2026-04-01-js-asset-proxy-design.md Outdated
Comment thread docs/superpowers/specs/2026-04-01-js-asset-proxy-design.md Outdated
Comment thread docs/superpowers/specs/2026-04-01-js-asset-proxy-design.md Outdated
Comment thread docs/superpowers/specs/2026-04-01-js-asset-proxy-design.md
Comment thread docs/superpowers/specs/2026-04-01-js-asset-proxy-design.md
Comment thread docs/superpowers/specs/2026-04-01-js-asset-proxy-design.md
Comment thread docs/superpowers/specs/2026-04-01-js-asset-proxy-design.md
@aram356
aram356 requested review from aram356 and prk-Jr June 8, 2026 15:39
@ChristianPavilonis ChristianPavilonis changed the title Simplify JS asset proxy spec Add JavaScript asset proxy integration Jun 10, 2026
@ChristianPavilonis ChristianPavilonis linked an issue Jun 10, 2026 that may be closed by this pull request
@ChristianPavilonis
ChristianPavilonis marked this pull request as draft June 10, 2026 17:48
@ChristianPavilonis
ChristianPavilonis force-pushed the simplify-js-asset-proxy-spec branch 2 times, most recently from 6b9389b to e0d6bf8 Compare June 22, 2026 21:20
@ChristianPavilonis
ChristianPavilonis force-pushed the simplify-js-asset-proxy-spec branch 2 times, most recently from 8b56f22 to 753da1f Compare July 1, 2026 18:37
@ChristianPavilonis
ChristianPavilonis changed the base branch from main to feature/ts-cli-audit July 1, 2026 19:38
Base automatically changed from feature/ts-cli-audit to main July 1, 2026 21:09
@ChristianPavilonis
ChristianPavilonis force-pushed the simplify-js-asset-proxy-spec branch 2 times, most recently from 7730c4f to d79e84b Compare July 7, 2026 18:06
@ChristianPavilonis
ChristianPavilonis marked this pull request as ready for review July 7, 2026 18:43
@aram356 aram356 mentioned this pull request Jul 7, 2026
3 tasks
@ChristianPavilonis
ChristianPavilonis force-pushed the simplify-js-asset-proxy-spec branch from ee2a692 to 03dd7b8 Compare July 13, 2026 19:48
@aram356 aram356 added this to the 202608 milestone Aug 13, 2026

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Adds the JS Asset Proxy integration (config-driven first-party serving of exact third-party script URLs with enabled/disabled/blocked modes), stream_response plumbing through proxy_request, and ts audit generation of disabled asset-proxy candidates. The design follows the spec closely and the security defaults are right (request-header allowlist only, no EC/Cookie forwarding, Set-Cookie stripped, HTTPS-only origins, opaque generated paths). Blocking items: a guaranteed 502 on the Cloudflare adapter, a CI fmt failure, and merge conflicts with main.

Blocking

🔧 wrench

  • Cloudflare adapter rejects stream_response, so every enabled asset request 502s there: see inline comment (crates/trusted-server-core/src/integrations/js_asset_proxy.rs:264)
  • CI cargo fmt fails: edition-2024 import ordering on three use lines; see inline comment (crates/trusted-server-core/src/integrations/js_asset_proxy.rs:14)
  • Merge conflicts with main: GitHub reports the PR as CONFLICTING; git merge-tree shows conflicts in crates/trusted-server-core/src/config.rs, crates/trusted-server-core/src/integrations/mod.rs, and trusted-server.example.toml. All three are mechanical (registration list, validated-IDs list, sample config), but the branch needs a merge or rebase before landing.

Non-blocking

🤔 thinking

  • builders() ordering is load-bearing but undocumented (crates/trusted-server-core/src/integrations/mod.rs:289)
  • Path validation permits / (crates/trusted-server-core/src/integrations/js_asset_proxy.rs:120)

♻️ refactor

  • Configured origin_url is never normalized, so non-canonical configs silently fail to match (crates/trusted-server-core/src/integrations/js_asset_proxy.rs:244)
  • No test drives IntegrationProxy::handle() end-to-end (crates/trusted-server-core/src/integrations/js_asset_proxy.rs:449)

🌱 seedling

  • Conditional revalidation never 304s at the edge; future allowlist additions would turn upstream 304 into 502 (crates/trusted-server-core/src/integrations/js_asset_proxy.rs:477)
  • <link rel="preload" as="script"> hints for blocked/rewritten assets are untouched (crates/trusted-server-core/src/integrations/js_asset_proxy.rs:496)
  • Audit dedup keys on the full URL including volatile query strings (crates/trusted-server-cli/src/commands/audit/mod.rs:489)

⛏ nitpick

  • headers.get(VARY) takes only the first of repeated headers (crates/trusted-server-core/src/integrations/js_asset_proxy.rs:340)
  • Content-Length dropped on a passthrough body (crates/trusted-server-core/src/integrations/js_asset_proxy.rs:334)
  • #[cfg(test)] build_draft_config wrapper (crates/trusted-server-cli/src/commands/audit/mod.rs:331)

CI Status

  • fmt: FAIL (import ordering; reproduced locally)
  • clippy/check (all adapters): PASS
  • rust tests (fastly, axum, cloudflare, spin, CLI, parity, browser/integration): PASS
  • js tests (vitest): PASS
  • docs/ts format: PASS
  • mergeable: CONFLICTING

Comment thread crates/trusted-server-core/src/integrations/js_asset_proxy.rs
Comment thread crates/trusted-server-core/src/integrations/js_asset_proxy.rs Outdated
Comment thread crates/trusted-server-core/src/integrations/js_asset_proxy.rs
Comment thread crates/trusted-server-core/src/integrations/js_asset_proxy.rs
Comment thread crates/trusted-server-core/src/integrations/js_asset_proxy.rs
Comment thread crates/trusted-server-core/src/integrations/js_asset_proxy.rs Outdated
Comment thread crates/trusted-server-core/src/integrations/js_asset_proxy.rs
Comment thread crates/trusted-server-core/src/integrations/mod.rs Outdated
Comment thread crates/trusted-server-cli/src/commands/audit/mod.rs
Comment thread crates/trusted-server-cli/src/commands/audit/mod.rs Outdated

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

The integration is well-shaped for the existing registry/proxy patterns, the header policy is tight, and the audit-side generator produces a safe disabled-by-default inventory with opaque randomized paths. Four items block: the required cargo fmt check is still red, the always-on stream_response flag is rejected outright by the Cloudflare and Spin adapters, the branch now conflicts with main, and any upstream redirect turns into a hard 502.

Note on overlap: the previous CHANGES_REQUESTED review is pinned to this exact head (6d6f5892) and no commits have landed since, so all of its threads are still open. This pass does not restate them — it confirms the two blocking ones (below) and adds what is new.

1 of the inline comments below carries a one-click GitHub suggestion — use Commit suggestion to apply it as a commit on the PR branch. The remaining comments describe the fix in prose because the change is a design decision or spans multiple files and can't be auto-applied.

Blocking

🔧 wrench

  • Upstream 3xx becomes a hard 502 — see inline at crates/trusted-server-core/src/integrations/js_asset_proxy.rs:477
  • cargo fmt required check is failing — see Cross-cutting below
  • Cloudflare and Spin adapters reject stream_response — see Cross-cutting below
  • Branch conflicts with main — see Cross-cutting below

Non-blocking

🤔 thinking / ♻️ refactor / ⛏ nitpick / 📝 note

  • Audit-generated drafts override upstream cache headers for every asset — see inline at crates/trusted-server-cli/src/commands/audit/mod.rs:444
  • Fixed User-Agent collapses UA-adaptive vendor bundles — see inline at crates/trusted-server-core/src/integrations/js_asset_proxy.rs:281
  • Integration ID duplicated as a string literal — see inline at crates/trusted-server-core/src/config.rs:138
  • Only GET is registered for asset paths — see inline at crates/trusted-server-core/src/integrations/js_asset_proxy.rs:445
  • X-TS-JS-Asset-Proxy marker is always emitted — see inline at crates/trusted-server-core/src/integrations/js_asset_proxy.rs:350

Cross-cutting / body-level findings

  • 🔧 cargo fmt required check is failing — reproduced locally at this head. Three use lines in js_asset_proxy.rs need edition-2024 import ordering: line 14 (http::{Method, Request, Response, StatusCode, header}), line 25 (crate::proxy::{ProxyRequestConfig, proxy_request}), and the test import at line 526 (crate::html_processor::{HtmlProcessorConfig, create_html_processor}). A single cargo fmt --all fixes all three. This is the only failing check and it is branch-protection required.

  • 🔧 Cloudflare and Spin adapters reject stream_response, so every proxied asset returns 502 therebuild_proxy_config unconditionally sets .with_stream_response() (js_asset_proxy.rs:263). Both adapters treat that flag as an unsupported contract and error out rather than degrade:

    • crates/trusted-server-adapter-cloudflare/src/platform.rs:269 — "streaming response bodies are not supported on the Cloudflare Workers runtime"
    • crates/trusted-server-adapter-spin/src/platform.rs:311 — "Spin outbound HTTP does not support streaming responses"

    proxy_request surfaces that as an error, and handle() maps every error to 502 with X-TS-Error: js-asset-origin-unreachable — so on those runtimes every configured asset is a hard failure, and the response looks like an origin problem rather than an unsupported platform contract. The Cloudflare guard's own comment ("These fields are only set by asset routes, which are not routed to the Cloudflare adapter today") is no longer true, because core integration routes dispatch on every adapter. The Axum adapter has no guard and simply ignores the flag, so it buffers — a third behaviour. The spec's "No adapter entry-point changes are expected if the existing integration registry dispatch is sufficient" (2026-04-01-js-asset-proxy-design.md:297) needs revisiting.

    CI does not catch this: the cross-adapter parity suite passes only because its fixture never enables js_asset_proxy. Whichever way this is resolved — gate stream_response on adapter capability, make the non-Fastly adapters buffer instead of erroring, or document the integration as Fastly-only and fail config validation elsewhere — a parity or per-adapter test that enables one asset would keep it from regressing.

  • 🔧 The branch conflicts with main — GitHub reports CONFLICTING; git merge-tree origin/main <head> shows content conflicts in crates/trusted-server-core/src/config.rs, crates/trusted-server-core/src/integrations/mod.rs, and trusted-server.example.toml. Worth flagging the last one specifically: replace_js_asset_proxy_section (audit/mod.rs:567) searches the embedded example config for a literal [integrations.js_asset_proxy] header and returns a hard CLI error if it is missing. If that header is dropped or renamed while resolving the conflict, every ts audit run fails, not just this integration — the unit test at audit/mod.rs:997 is what guards it.

  • 👍 Praise — a few things worth calling out: the upstream Set-Cookie is deliberately dropped and the request-header allowlist is genuinely minimal (build_proxy_config, verified by build_proxy_config_forwards_only_asset_header_allowlist); the audit generator emits opaque randomized /assets/<hex>.js paths from OsRng rather than mirroring vendor filenames; the precedence tests against the native GPT rewriter cover all three proxy modes; and validate_js_asset_proxy_config correctly plugs a real hole — IntegrationSettings::get_typed returns early for explicitly-disabled configs before calling validate(), so without this deploy-time check an invalid disabled inventory would ship unvalidated.

CI Status

  • cargo fmt: FAIL (required)
  • cargo test: PASS (required)
  • format-docs: PASS (required)
  • format-typescript: PASS (required)
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • cargo test (axum native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo test (ts CLI, native): PASS
  • integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • prepare integration artifacts: PASS
  • browser integration tests: PASS
  • vitest: PASS
  • Analyze (rust): PASS
  • Analyze (actions): PASS
  • Analyze (javascript-typescript): PASS
  • CodeQL: PASS

Comment thread crates/trusted-server-core/src/integrations/js_asset_proxy.rs
Comment thread crates/trusted-server-cli/src/commands/audit/mod.rs Outdated
Comment thread crates/trusted-server-core/src/integrations/js_asset_proxy.rs
Comment thread crates/trusted-server-core/src/config.rs Outdated
Comment thread crates/trusted-server-core/src/integrations/js_asset_proxy.rs
Comment thread crates/trusted-server-core/src/integrations/js_asset_proxy.rs
@ChristianPavilonis
ChristianPavilonis force-pushed the simplify-js-asset-proxy-spec branch from 6d6f589 to a82aaf2 Compare August 20, 2026 13:49
@aram356

aram356 commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

@ChristianPavilonis to resolve feedback

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Adds a js_asset_proxy integration serving explicitly configured third-party JavaScript from exact first-party paths, plus ts audit generation of disabled-by-default candidate entries. The core mechanics are sound: exact-match routing with no prefix or wildcard, follow_redirects = false, a three-header request allowlist with copy_request_headers = false and a fixed User-Agent, no EC forwarding, and a response rebuilt from scratch so Set-Cookie and every other upstream header outside a small allowlist are dropped. I specifically probed SSRF, request/response header leakage, cross-adapter streaming parity, and route shadowing, and found no defect in any of them. One security-hardening gap and three smaller items below.

2 of the inline comments below carry a one-click GitHub suggestion — use Commit suggestion (or Add suggestion to batch) to apply them as commits on the PR branch. The remaining comments describe the fix in prose because the change spans more than one range in the file or needs an accompanying test change.

Blocking

🔧 wrench

  • Proxied third-party bytes are served from the publisher origin with no content-type protection — see inline at crates/trusted-server-core/src/integrations/js_asset_proxy.rs:378-381

Non-blocking

♻️ refactor

  • ETag/Last-Modified advertised downstream but conditional requests never forwarded upstream — see inline at crates/trusted-server-core/src/integrations/js_asset_proxy.rs:288-296

🤔 thinking

  • Cache-Control: public on a route that can also mint an EC cookie — see inline at crates/trusted-server-core/src/integrations/js_asset_proxy.rs:415-420

⛏ nitpick

  • Example config's sample asset is proxy = "enabled", so one edit activates it — see inline at trusted-server.example.toml:124

Cross-cutting / body-level findings

  • 📝 The verification commands in the PR description are not this workspace's gates. The body lists cargo clippy --workspace --all-targets --all-features -- -D warnings and cargo test --workspace. Per CLAUDE.md, a workspace-wide clippy trips the Cloudflare adapter's non-wasm32 guard; run against this head it exits 101 at crates/trusted-server-adapter-cloudflare/src/lib.rs:5, so it cannot have passed as written. No quality problem behind it — I ran the real target-matched gates against 8fc2477 and all pass: cargo fmt --all -- --check, all six clippy-* aliases, all four test-* aliases, and the cross-adapter parity suite. Please update the description to the CLAUDE.md gate list.

  • 👍 The supports_streaming_responses() gating in proxy.rs fixes a latent cross-adapter break. On main, handle_asset_proxy_request set with_stream_response() unconditionally (proxy.rs:1196) — a contract both the Cloudflare (adapter-cloudflare/src/platform.rs:307) and Spin (adapter-spin/src/platform.rs:318) clients hard-reject. Gating it plus the buffered fallback (proxy.rs:1211-1228), covered by new tests at proxy.rs:4265 and proxy.rs:4334, is a real fix beyond this PR's stated scope. Worth calling out in the description since it changes shared proxy behaviour.

CI Status

  • cargo fmt: PASS (required)
  • cargo test: PASS (required)
  • format-typescript: PASS (required)
  • format-docs: PASS (required)
  • cargo test (axum native): PASS
  • cargo test (ts CLI, native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • vitest: PASS
  • integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • browser integration tests: PASS
  • prepare integration artifacts: PASS
  • CodeQL: PASS
  • Analyze (rust): PASS
  • Analyze (actions): PASS
  • Analyze (javascript-typescript): PASS (reported twice, from two workflow runs)

Comment thread crates/trusted-server-core/src/integrations/js_asset_proxy.rs
Comment thread crates/trusted-server-core/src/integrations/js_asset_proxy.rs
Comment thread crates/trusted-server-core/src/integrations/js_asset_proxy.rs
Comment thread trusted-server.example.toml
@aram356 aram356 modified the milestones: 202608, 202609 Aug 31, 2026

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Focused, well-tested integration that follows the existing integration-proxy shape closely and matches its own spec. One blocking finding: the asset config accepts unknown keys, so a misspelled proxy key silently fails open from blocked to enabled. The rest are non-blocking cleanups, one of which is a verified defect in the ts audit draft generator.

3 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 comment describes the fix in prose because it needs a registry-side change and can't be auto-applied.

All three suggestions were verified in an isolated worktree individually and as a batch: cargo fmt --all -- --check, cargo clippy-fastly, trusted-server-core (2291 tests, native host), cargo test-axum, cargo test-cloudflare, cargo test-spin, trusted-server-cli (158 tests, native host), and the cross-adapter parity suite (13 tests) — all green, with no drift between the approved bytes and the post-verification tree.

Blocking

🔧 wrench

  • Asset config accepts unknown keys, so a typo'd proxy silently fails open to enabled — see inline at crates/trusted-server-core/src/integrations/js_asset_proxy.rs:54

Non-blocking

♻️ refactor

  • replace_js_asset_proxy_section swallows the comment block documenting the next section — see inline at crates/trusted-server-cli/src/commands/audit/mod.rs:588-593
  • Example template ships an active cache_ttl_seconds = 3600, contradicting this PR's own documented safe default — see inline at trusted-server.example.toml:167

🤔 thinking

  • el.tag_name() allocates per element in four handlers whose only consumer never reads it — see inline at crates/trusted-server-core/src/html_processor.rs:522

Cross-cutting / body-level findings

  • 🌱 Core named routes silently shadow configured asset pathsIntegrationRegistry route insertion catches integration-vs-integration collisions loudly, which is what the spec asks for (docs/superpowers/specs/2026-04-01-js-asset-proxy-design.md: "an asset path cannot silently shadow another integration endpoint"). But in crates/trusted-server-adapter-fastly/src/app.rs, named routes resolve before state.registry.has_route(&method, &path) in dispatch_fallback, so an asset path colliding with a core route (/auction, /page-bids, /first-party/proxy, /_ts/*) is silently shadowed with no config-time error — the rewritten <script src> just hits the core handler and the asset is never proxied. Low practical risk today because ts audit generates opaque /assets/<24-hex>.js paths, but hand-written config has no guard. Worth a follow-up issue rather than widening this PR.

  • 👍 Streaming capability gate applied at every call sitesupports_streaming_responses() is the right fix for a flag that Cloudflare and Spin hard-reject, and crates/trusted-server-core/src/proxy.rs gates it in proxy_request, send_asset_origin_request, and handle_asset_proxy_request rather than only at the new call site. The buffered-fallback assertions (recorded_stream_response_flags()) pin the behaviour on both branches.

  • 👍 Response is rebuilt from an allowlist, not filteredfinalize_asset_response starts from Response::new(body) and copies only Content-Encoding / ETag / Last-Modified / Vary / Cache-Control, so upstream Set-Cookie, CORS grants, and Content-Type cannot escape by construction. Forcing application/javascript; charset=utf-8 plus nosniff closes the "configured upstream serves an HTML document from the publisher origin" hole cleanly.

  • 👍 The ordering constraint is enforced by tests, not just a comment — the "must remain first" note on js_asset_proxy in builders() is backed by js_asset_proxy_rewriter_takes_precedence_over_native_rewriters, js_asset_proxy_blocking_takes_precedence_over_native_rewriters, and disabled_js_asset_proxy_candidate_allows_native_rewriters, all driving a real IntegrationRegistry. Reordering builders() breaks CI instead of silently changing rewrite precedence.

  • 👍 Deploy validation covers disabled asset inventoryvalidate_js_asset_proxy_config runs regardless of enabled, so audit-generated candidates can't rot into invalid config that only explodes when an operator flips enabled = true. validate_rejects_invalid_disabled_js_asset_proxy_assets locks that in.

CI Status

  • Analyze (actions): PASS
  • Analyze (javascript-typescript): PASS
  • Analyze (javascript-typescript): PASS
  • Analyze (rust): PASS
  • CodeQL: 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): PASS
  • format-docs: PASS (required)
  • format-typescript: PASS (required)
  • integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • prepare integration artifacts: PASS
  • vitest: PASS

Comment thread crates/trusted-server-core/src/integrations/js_asset_proxy.rs
Comment thread crates/trusted-server-cli/src/commands/audit/mod.rs Outdated
Comment thread trusted-server.example.toml Outdated
Comment thread crates/trusted-server-core/src/html_processor.rs

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Verdict: APPROVE — no blocking findings. Everything below is non-blocking: questions about deliberate design choices, one test-coverage gap, and small consistency items. None of it needs to land before merge.

Adds a js_asset_proxy integration that serves explicitly configured third-party JavaScript from first-party paths, plus the stream_response plumbing and adapter capability gate it needs, and ts audit generation of disabled-by-default candidates. The design is tight: exact-match-only routing and rewriting, a three-mode (enabled/disabled/blocked) per-asset switch, a narrow request-header allowlist, and a response built from an allowlist rather than passed through. All 19 CI checks pass. Nothing here blocks merge — the findings below are questions about deliberate design choices, a test-coverage gap, and small consistency items.

2 of the inline comments below carry a one-click GitHub suggestion — use Commit suggestion (or Add suggestion to batch) to apply them as commits on the PR branch. The remaining comments describe the concern in prose because the change spans multiple files or is a question rather than an edit.

Non-blocking

🤔 thinking

  • Conditional-request headers dropped; a 304 would map to 502 — see inline at crates/trusted-server-core/src/integrations/js_asset_proxy.rs:516
  • Fixed User-Agent vs UA-differentiated vendor bundles (and integrity) — see inline at crates/trusted-server-core/src/integrations/js_asset_proxy.rs:303
  • cache_ttl_seconds upgrades private/no-store upstreams to public — see inline at crates/trusted-server-core/src/integrations/js_asset_proxy.rs:360
  • The streaming response path is flag-asserted, never exercised — see inline at crates/trusted-server-core/src/integrations/js_asset_proxy.rs:1398

⛏ nitpick

  • Use the existing without_ec_id() builder — see inline at crates/trusted-server-core/src/integrations/js_asset_proxy.rs:289
  • Header-name constants don't follow the constants.rs convention — see inline at crates/trusted-server-core/src/integrations/js_asset_proxy.rs:32
  • Spec 2's "Related context" paths don't exist — see inline at docs/superpowers/specs/2026-06-22-ts-audit-js-asset-proxy-config-design.md:11

🌱 seedling

  • IntegrationAttributeContext gained a public field — see inline at crates/trusted-server-core/src/integrations/registry.rs:85
  • ts audit mints fresh random paths on every run — see inline at crates/trusted-server-cli/src/commands/audit/mod.rs:127

👍 praise

  • Content-type pinning + nosniff — see inline at crates/trusted-server-core/src/integrations/js_asset_proxy.rs:387
  • The streaming capability gate fixes a real pre-existing bug — see inline at crates/trusted-server-core/src/proxy.rs:1237
  • Injection-safe TOML generation in the audit draft — see inline at crates/trusted-server-cli/src/commands/audit/mod.rs:651

Verified and cleared

Recording these so the next reviewer doesn't re-derive them:

  • Rewriter ordering holds. builders() order is preserved into inner.html_rewriters, and rewrite_attribute chains Replace without short-circuiting, so placing js_asset_proxy first genuinely gives it precedence over the native rewriters. Two tests guard it.
  • Configured paths can't shadow built-in routes. Integration routes dispatch in the EdgeZero fallback, after the tsjs and built-in route arms, so a configured path cannot capture /_ts/admin or /static/tsjs=.
  • No EC cookie lands on a publicly cacheable asset. handle_proxy skips EC generation for non-navigation requests, and enforce_set_cookie_cache_privacy is a second net at response send.
  • The expect() calls in finalize_asset_response are unreachable. Every value handed to HeaderValue::from_str came back through HeaderValue::to_str().ok(), so it is already visible ASCII.
  • The new non-streaming branch in handle_asset_proxy_request is not redundant — it mirrors the pre-existing shape at proxy.rs:1054 on main.
  • The cargo test --workspace / cargo clippy --workspace lines in the spec's Verification block are pre-existing house style across 10+ documents in docs/superpowers/specs/, not something this PR introduced.

CI Status

  • cargo test: PASS (required)
  • cargo fmt: PASS (required)
  • format-docs: PASS (required)
  • format-typescript: PASS (required)
  • 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
  • integration tests: PASS
  • browser integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • prepare integration artifacts: PASS
  • CodeQL: PASS
  • Analyze (rust): PASS
  • Analyze (actions): PASS
  • Analyze (javascript-typescript): PASS

}
};

if !response.status().is_success() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 thinking — Conditional-request headers are dropped, and a 304 would land here as a 502.

finalize_asset_response forwards ETag and Last-Modified downstream, but build_proxy_config forwards only Accept, Accept-Language, and Accept-EncodingIf-None-Match and If-Modified-Since never reach the origin. The browser therefore caches with validators it can present, but every revalidation costs a full origin fetch and a full downstream transfer, where loading the script directly from the vendor CDN would have yielded a 304. For a proxy whose selling point is first-party delivery of vendor JS, that is a real bandwidth and latency regression versus the un-proxied baseline.

There is also an interlock worth recording before someone revisits the allowlist: StatusCode::is_success() is 200–299, so if conditional forwarding is added later without touching this check, an upstream 304 becomes 502 X-TS-Error: js-asset-origin-status and the script silently stops loading on every revalidation.

If the narrow allowlist is deliberate MVP scope — the spec does pin it to those three headers — a line in docs/superpowers/specs/2026-04-01-js-asset-proxy-design.md saying revalidation is intentionally not supported yet would keep the next reader from treating it as an oversight.

Comment on lines +302 to +303
HEADER_USER_AGENT.clone(),
http::HeaderValue::from_static("TrustedServer/1.0"),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 thinking — The fixed User-Agent will change which bytes some origins return.

Plenty of tag and SDK CDNs branch on User-Agent — modern versus ES5 bundles, polyfill services, and vendor-specific device detection. Pinning every upstream fetch to TrustedServer/1.0 means all clients receive whatever the origin serves an unrecognized agent, which is usually the most conservative fallback and occasionally a bot/blocked response.

Two follow-on effects worth confirming:

  • If the publisher's markup carries integrity on the script tag, the attribute rewriter replaces src but leaves the hash in place. When the UA-selected bytes differ from what the publisher hashed, SRI fails and the script never executes — a failure mode that only shows up for assets the operator flips to proxy = "enabled".
  • Because the UA is constant, the response deliberately does not Vary on it, which is correct — but it means an origin that does vary by UA has its variants collapsed into one cached entry.

The spec mandates the fixed UA, so this is a "confirm the tradeoff is understood" note rather than a defect. Documenting it next to the assets[].proxy field would help operators recognize the symptom.

Comment on lines +360 to +362
fn resolved_cache_ttl_seconds(&self, asset: &JsAssetProxyAsset) -> Option<u32> {
asset.cache_ttl_seconds.or(self.config.cache_ttl_seconds)
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 thinking — A configured TTL rewrites private / no-store upstreams into public.

When either cache_ttl_seconds is set, finalize_asset_response emits Cache-Control: public, max-age=<ttl> and discards whatever the origin said. That is the documented behaviour, and the default (preserve upstream) is the safe one — but the override has no floor. A vendor bundle carrying per-user content, which the origin correctly marks private, no-store, becomes shared-cacheable at the edge the moment an operator sets a TTL on it, and every subsequent visitor gets the first visitor's copy.

The operator-facing text does not hint at this. In trusted-server.example.toml the whole guidance is:

# Uncomment to override upstream cache headers for every asset below.
# cache_ttl_seconds = 3600

Two options, in increasing order of cost:

  1. Extend that comment to say the override replaces private / no-store and must only be used for assets the operator knows are identical for every visitor. Note this comment block is duplicated verbatim in three places — here in trusted-server.example.toml, in the ts audit generator's string literal in crates/trusted-server-cli/src/commands/audit/mod.rs, and in the exact-string assertion in crates/trusted-server-cli/src/commands/config/init.rs — so it is a synchronized three-file edit rather than a one-click suggestion.
  2. Refuse the public upgrade when the upstream response is private or no-store, logging at warn instead. That is a behaviour change beyond this PR's scope, so it would want a follow-up issue rather than a late edit here.

.await
.expect("should proxy streaming asset response");
assert_eq!(success.status(), StatusCode::OK);
assert_eq!(streaming_stub.recorded_stream_response_flags(), vec![true]);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 thinking — This asserts the streaming flag, but the streaming body is never exercised.

StubHttpClient::send always builds its response from a Vec<u8>, so it yields EdgeBody::Once no matter what set_streaming_responses_supported(true) reports. This test therefore proves that stream_response: true reached the platform request, and nothing more.

What stays uncovered is the path that actually runs in production on Fastly: an EdgeBody::Stream flowing through finalize_asset_response's into_parts()Response::new(body) reconstruction, out of handle_proxy, and into the EdgeBody::Stream(_) arm of send_edgezero_response in crates/trusted-server-adapter-fastly/src/main.rs:345. Header-only handling of a stream body is exactly where a regression would hide — the reconstruction drops the upstream Content-Length (correct for a stream, and worth keeping that way), and nothing in the suite would notice if that changed.

A stub variant that can yield a Body::Stream would close this, and it would pay for itself for the other integrations that adopt with_stream_response() later. Not a merge blocker — the flag assertion plus the Fastly adapter's own streaming tests cover the halves separately — but the seam between them is untested.

.with_stream_response()
.without_forward_headers();
config.follow_redirects = false;
config.forward_ec_id = false;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpickProxyRequestConfig already has a builder for this.

without_ec_id() exists at crates/trusted-server-core/src/proxy.rs:424 and is what every other caller uses; setting the field directly bypasses it. (follow_redirects on the line above genuinely has no builder, so that one stays as-is.)

Suggested change
config.forward_ec_id = false;
config = config.without_ec_id();

Scratch-verified on this head: cargo fmt --all -- --check, cargo clippy-fastly, and cargo check-fastly && cargo check-axum && cargo check-cloudflare all pass with this applied, with no formatter drift.

#[derive(Debug)]
pub struct IntegrationAttributeContext<'a> {
pub attribute_name: &'a str,
pub element_name: &'a str,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌱 seedling — Adding a public field here is a breaking change for anyone constructing this struct.

IntegrationAttributeContext is a pub struct with all-pub fields and no constructor, so every struct-literal construction outside this crate breaks on a new field. In-tree that cost 12 test-only edits across 8 integration modules in this PR, which is fine — but the same tax lands on any out-of-tree integration, and this is the second context field the rewriter API has needed.

Worth considering before the next one:

#[derive(Debug)]
#[non_exhaustive]
pub struct IntegrationAttributeContext<'a> {
    pub attribute_name: &'a str,
    pub element_name: &'a str,
    // …
}

with a constructor for the required fields. That makes future context additions additive for external callers and turns the in-tree churn into a single call-site change. Follow-up material, not something to hold this PR for.

Comment on lines +124 to +128
impl OpaqueAssetPathGenerator for RandomOpaqueAssetPathGenerator {
fn next_path(&mut self) -> String {
let mut bytes = [0_u8; 12];
rand::rngs::OsRng.fill_bytes(&mut bytes);
format!("/assets/{}.js", lowercase_hex(&bytes))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌱 seedling — Re-running ts audit mints an entirely new path for every asset.

Drawing from OsRng per invocation is the right call for the first draft — the paths are opaque, unguessable, and carry no vendor or filename semantics, exactly as the spec asks. The cost shows up on the second run: an operator who has already curated trusted-server.toml and re-audits the same page gets a completely disjoint set of path values, so there is no way to diff "what's new on this page since last time" against what they already configured. Every candidate looks new.

A follow-up worth tracking: derive the opaque id deterministically from the normalized origin_url under a per-deployment salt, or add a merge mode that reads the existing config and only appends origins it doesn't already carry. Either keeps paths opaque while making re-audits reviewable. Explicitly out of scope for this PR — the spec's non-goals rule out interactive and merge workflows for the first pass.

Comment on lines +384 to +390
// Upstream bytes are served from the publisher origin, so the upstream
// cannot choose a document MIME type or opt into browser MIME sniffing.
finalized.headers_mut().insert(
header::CONTENT_TYPE,
http::HeaderValue::from_static(JS_ASSET_CONTENT_TYPE),
);
finalized.headers_mut().insert(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 praise — Pinning the content type instead of forwarding it is the right instinct, and the comment says why.

Discarding the upstream Content-Type and hard-setting application/javascript; charset=utf-8 plus X-Content-Type-Options: nosniff means a vendor origin that answers a soft-404 with an HTML error page — or gets compromised outright — hands the browser a JavaScript syntax error rather than a document rendered on the publisher's own origin. That closes the one genuinely dangerous capability this feature would otherwise grant an upstream, since these bytes are now same-origin with the publisher.

Combined with finalize_asset_response rebuilding the response from an allowlist rather than passing headers through, Set-Cookie and any upstream CORS grant are dropped structurally rather than by an easily-forgotten strip list. Good shape.

Comment on lines +1237 to +1240
let stream_response = services.http_client().supports_streaming_responses();
if stream_response {
platform_req = platform_req.with_stream_response();
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 praise — This gate fixes a real pre-existing bug, not just groundwork for the new integration.

On main this call site sets with_stream_response() unconditionally. Both non-Fastly adapters that see asset traffic hard-reject that flag rather than degrading:

  • crates/trusted-server-adapter-cloudflare/src/platform.rs:307 — "streaming response bodies are not supported on the Cloudflare Workers runtime"
  • crates/trusted-server-adapter-spin/src/platform.rs:318 — "Spin outbound HTTP does not support streaming responses"

So the asset proxy path was returning a platform error on those runtimes, and the cross-adapter parity suite would not have caught it because the rejection is inside the platform HTTP client rather than in routing. Gating on supports_streaming_responses() — with the default false on the trait so a new adapter fails safe into buffering — is the correct fix, and mirroring the existing shape at proxy.rs:1054 keeps the two asset paths reading the same way.

Comment on lines +644 to +651
fn sanitized_comment_value(value: &str) -> String {
value
.chars()
.map(|ch| if ch.is_control() { ' ' } else { ch })
.collect()
}

fn toml_quoted_string(value: &str) -> String {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 praise — The generated TOML is injection-safe by construction.

ts audit writes attacker-influenced data — third-party script URLs harvested from a page the operator does not control — straight into a config file, so this is the one spot in the CLI where escaping actually matters. toml_quoted_string handles backslash, quote, the three common escapes, and falls through to \uXXXX for anything else in the control range, and sanitized_comment_value collapses control characters so a crafted value cannot terminate a # comment and open a new table. Both are applied at every site that emits a value or a comment.

Worth noting the paths themselves are equally careful: is_valid_generated_asset_path re-validates the generator's output against the /assets/<lowercase-hex>.js shape and hard-errors rather than trusting it, so a future generator swap cannot quietly emit something the core-side path validator would reject at load time. Nice defensive layering for a code path most reviewers would wave through.

aram356 added a commit that referenced this pull request Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JS Asset proxy implementation

4 participants