Skip to content

Add JavaScript asset proxy integration - #742

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

Add JavaScript asset proxy integration#742
ChristianPavilonis wants to merge 11 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

}

/// One configured JavaScript asset mapping.
#[derive(Debug, Clone, Deserialize, Serialize)]

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.

🔧 wrenchJsAssetProxyAsset has no deny_unknown_fields, and proxy is #[serde(default)] = Enabled. An operator who writes mode = "blocked" (or proxy_mode, block = true) gets the key silently dropped and the asset defaults to enabled — Trusted Server then registers a first-party route and actively proxies the vendor script it was told to remove, and rewrites the publisher's <script src> to point at it.

Inverting a blocking control on a typo is the wrong failure direction, and origin_url being a required field means the other fields already fail loudly; proxy is the one that fails silently. Repo precedent exists in datadome.rs:130,147, prebid.rs:455,467, osano.rs:21, and gpt_diagnostics.rs:36.

Suggested change
#[derive(Debug, Clone, Deserialize, Serialize)]
#[derive(Debug, Clone, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]

Verified in a scratch worktree: cargo fmt --all -- --check, cargo clippy-fastly, trusted-server-core 2291 tests (incl. all 29 js_asset_proxy / config tests), cargo test-axum, cargo test-cloudflare, cargo test-spin, and the parity suite all pass with this applied.

Comment on lines +588 to +593
}

let mut output_lines = Vec::new();
output_lines.extend_from_slice(&lines[..start]);
output_lines.extend(replacement.trim_end_matches('\n').lines());
if end < lines.len() {

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.

♻️ refactor — The forward scan stops at the next [table] header, so every blank line and comment between the last asset entry and that header falls inside the replaced range. In trusted-server.example.toml that range ends with # Proxy behavior and first-party asset routing. Kept active with defaults. — the header doc for [proxy] — which is therefore dropped from every ts audit draft.

Verified by dumping the generated draft from build_draft_config_writes_disabled_js_asset_proxy_candidates: the draft goes straight from proxy = "disabled" to [proxy] with that comment gone. Re-running with the change below restores it with a single blank separator, and all 158 CLI tests still pass.

Suggested change
}
let mut output_lines = Vec::new();
output_lines.extend_from_slice(&lines[..start]);
output_lines.extend(replacement.trim_end_matches('\n').lines());
if end < lines.len() {
}
// Blank lines and comments directly above the next section header document
// that section, not this one, so leave them in the draft.
while end > start + 1 {
let trimmed = lines[end - 1].trim();
if trimmed.is_empty() || trimmed.starts_with('#') {
end -= 1;
} else {
break;
}
}
let mut output_lines = Vec::new();
output_lines.extend_from_slice(&lines[..start]);
output_lines.extend(replacement.trim_end_matches('\n').lines());
if end < lines.len() && !lines[end].trim().is_empty() {

The !lines[end].trim().is_empty() guard avoids emitting a second blank line once the trailing blank is no longer consumed.


[integrations.js_asset_proxy]
enabled = false
cache_ttl_seconds = 3600

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.

♻️ refactordocs/superpowers/specs/2026-06-22-ts-audit-js-asset-proxy-config-design.md states the safe default for this key is "commented guidance — Preserve upstream cache policy until an operator explicitly overrides it", and build_js_asset_proxy_section implements exactly that (with build_draft_config_writes_disabled_js_asset_proxy_candidates asserting the generated draft has no cache_ttl_seconds).

ts config init copies this template verbatim, though, so that path disagrees: it force-overrides every proxied vendor script's upstream Cache-Control — including private or no-store — with public, max-age=3600 the moment an operator flips enabled = true. Every other optional knob in this file is commented out, so this also reads as out of style with its neighbours.

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

Verified: trusted-server-core (2291), cargo test-cloudflare, cargo test-spin, and trusted-server-cli (158) all pass with this applied — the audit path replaces the whole section wholesale, so nothing depends on the key being active.

move |el| {
if let Some(mut href) = el.get_attribute("href") {
let original_href = href.clone();
let element_name = el.tag_name();

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.

🤔 thinkingelement_name is now computed unconditionally in all five attribute handlers ([href], [src], [action], [srcset], [imagesrcset]), but JsAssetProxyIntegration::rewrite is the only reader in the workspace and it inspects element_name solely on the src path. [href] matches every <a> on the page, so a link-heavy article page now pays a String allocation per anchor in the streaming HTML hot path for a field nothing reads.

Not blocking, and I'd rather not hand you a suggestion that passes "" into the four non-src contexts — that would be a landmine for the next rewriter that does read the field. The clean shape is to make the cost conditional, e.g.:

// registry.rs
impl IntegrationRegistry {
    #[must_use]
    pub fn has_attribute_rewriters(&self) -> bool {
        !self.inner.html_rewriters.is_empty()
    }
}

// html_processor.rs, per handler
let element_name = if integrations.has_attribute_rewriters() {
    el.tag_name()
} else {
    String::new()
};

Apply manually — can't be auto-applied as a suggestion because it adds a method in registry.rs and touches five separate hunks in html_processor.rs. Worth weighing before this lands alongside the streaming cutover work.

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