Skip to content

Add JavaScript asset proxy integration - #742

Merged
ChristianPavilonis merged 15 commits into
mainfrom
simplify-js-asset-proxy-spec
Sep 4, 2026
Merged

Add JavaScript asset proxy integration#742
ChristianPavilonis merged 15 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 Outdated
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

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/registry.rs
Comment thread crates/trusted-server-cli/src/commands/audit/mod.rs
Comment thread crates/trusted-server-core/src/integrations/js_asset_proxy.rs
Comment thread crates/trusted-server-core/src/proxy.rs
Comment thread crates/trusted-server-cli/src/commands/audit/mod.rs
aram356 added a commit that referenced this pull request Sep 3, 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.

Re-reviewed at 071ece515 against merge-base 0f8b44dc0.

All three blocking findings from the previous round are resolved, and the branch is now mergeable.

Blockers resolved

  • Adapter streaming capability: gated on PlatformHttpClient::supports_streaming_responses() rather than worked around locally, and the same gate was applied to the pre-existing handle_asset_proxy_request path in proxy.rs, which had been calling .with_stream_response() unconditionally.
  • cargo fmt: passes.
  • Merge conflicts: resolved.

Verified locally at this head

  • cargo fmt --all -- --check — pass
  • cargo test-fastly — 2289 passed, 0 failed, including all 24 js_asset_proxy tests
  • cargo test --package trusted-server-cli — pass, all audit and asset-proxy cases green
  • GitHub CI: all checks green

Informational, no action required

  • #[serde(deny_unknown_fields)] on JsAssetProxyAsset is the right call for catching operator typos. Worth keeping in mind for rollout ordering: because app-config is published as a blob envelope and read by whatever version is deployed, a config using a future asset field becomes a hard parse error on older edge nodes rather than an ignored key. Deploy code before config when that field is added.
  • Forcing Content-Type: application/javascript; charset=utf-8 is correct for a JS-only route family. If these routes are ever generalized to other asset types, that constant becomes wrong silently rather than loudly.
  • follow_redirects = false means a vendor that serves its script via a 301/302 returns 502 js-asset-origin-status. This is the documented spec behavior and the safe default; flagging it only as an operator-visible failure mode when onboarding a redirecting vendor.

The three non-blocking items left open from the previous round (conditional revalidation, <link rel=preload> hints, Content-Length preservation) remain reasonable follow-ups rather than merge blockers.

@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.

Posting the three informational notes from my approval as inline threads so they can be tracked and resolved individually. None of these block merge — the approval stands.

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
@ChristianPavilonis
ChristianPavilonis merged commit 22fbbf2 into main Sep 4, 2026
19 checks passed
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