Add JavaScript asset proxy integration - #742
Conversation
ChristianPavilonis
changed the title
6b9389b to
e0d6bf8
Compare
8b56f22 to
753da1f
Compare
7730c4f to
d79e84b
Compare
ee2a692 to
03dd7b8
Compare
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
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 fmtfails: edition-2024 import ordering on threeuselines; 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-treeshows conflicts incrates/trusted-server-core/src/config.rs,crates/trusted-server-core/src/integrations/mod.rs, andtrusted-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_urlis 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-Lengthdropped on a passthrough body (crates/trusted-server-core/src/integrations/js_asset_proxy.rs:334)#[cfg(test)] build_draft_configwrapper (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
| ) -> ProxyRequestConfig<'a> { | ||
| let mut config = ProxyRequestConfig::new(origin_url) | ||
| .with_streaming() | ||
| .with_stream_response() |
There was a problem hiding this comment.
🔧 wrench: build_proxy_config always sets .with_stream_response(), but the Cloudflare adapter hard-rejects that flag (crates/trusted-server-adapter-cloudflare/src/platform.rs:269, "streaming response bodies are not supported on the Cloudflare Workers runtime"). Its guard comment assumes stream-response requests "are not routed to the Cloudflare adapter today"; this integration makes that assumption false, since core integration routes dispatch on every adapter. The result on Cloudflare: proxy_request errors and handle() maps it to 502 X-TS-Error: js-asset-origin-unreachable for every enabled asset request, a guaranteed failure with a misleading diagnostic. Axum and Spin merely ignore the flag and buffer, which degrades gracefully.
Fix: consult a capability probe before setting the flag (precedent: supports_concurrent_fanout() on PlatformHttpClient), e.g. supports_streaming_responses(), and fall back to a buffered send when unsupported. At minimum, distinguish this adapter-capability error from "origin unreachable" and document the Cloudflare limitation.
| use async_trait::async_trait; | ||
| use edgezero_core::body::Body as EdgeBody; | ||
| use error_stack::Report; | ||
| use http::{header, Method, Request, Response, StatusCode}; |
There was a problem hiding this comment.
🔧 wrench: CI cargo fmt fails on this file (reproduced locally). Three use lines need edition-2024 import ordering: line 14 (http::{Method, Request, Response, StatusCode, header}), line 28 (crate::proxy::{ProxyRequestConfig, proxy_request}), and the test import at line 529. One cargo fmt --all fixes it.
| self.config | ||
| .assets | ||
| .iter() | ||
| .find(|asset| asset.origin_url == origin_url) |
There was a problem hiding this comment.
♻️ refactor: the configured origin_url is never normalized. Matching normalizes the script src (lowercased host/scheme, default port stripped) and compares it to the raw configured string, so a hand-written origin_url with an uppercase host or explicit :443 only matches byte-identical HTML; normalized variants silently never match. The duplicate-origin_url validation has the same blind spot: https://cdn.example.com/vendor.js and https://cdn.example.com:443/vendor.js pass as distinct entries. The audit CLI is unaffected because it emits Url::to_string() output.
Fix: normalize origin_url once at build/validation time (store Url::parse(origin_url)?.to_string()), or add a validation error when the parsed-and-serialized form differs from the configured string.
| .collect() | ||
| } | ||
|
|
||
| async fn handle( |
There was a problem hiding this comment.
♻️ refactor: no test drives handle() end-to-end. The 502 mappings are only tested via the private response constructors, and the header policy only via build_proxy_config in isolation, so the spec's verification items ("upstream fetch failure returns 502", "upstream non-success returns 502") are not actually covered at the handler level. StubHttpClient supports exactly this (see proxy_request_forwards_stream_response_flag_to_platform_request in proxy.rs tests): one test asserting asset lookup, proxy call, upstream 404 to 502 js-asset-origin-status, plus one for the unreachable path, would close the gap.
| fn validate(&self) -> Result<(), ValidationErrors> { | ||
| let mut errors = ValidationErrors::new(); | ||
|
|
||
| if !self.path.starts_with('/') { |
There was a problem hiding this comment.
🤔 thinking: path validation permits path = "/". Shadowing publisher paths is the feature's purpose, but / would replace the homepage with a JavaScript payload, and nothing catches that before deploy. Consider rejecting / (and possibly requiring a file-like final segment).
| let content_encoding = parts.headers.get(header::CONTENT_ENCODING).cloned(); | ||
| let etag = parts.headers.get(header::ETAG).cloned(); | ||
| let last_modified = parts.headers.get(header::LAST_MODIFIED).cloned(); | ||
| let upstream_vary = parts |
There was a problem hiding this comment.
⛏ nitpick: headers.get(header::VARY) takes only the first value when the upstream sends repeated Vary headers; get_all plus a join would be faithful. Same applies to Cache-Control below.
| asset: &JsAssetProxyAsset, | ||
| response: Response<EdgeBody>, | ||
| ) -> Response<EdgeBody> { | ||
| let (parts, body) = response.into_parts(); |
There was a problem hiding this comment.
⛏ nitpick: Content-Length is dropped when rebuilding the response. The body is streamed through unchanged, so preserving upstream Content-Length when present would keep length-delimited framing (and download progress) instead of forcing chunked encoding.
| pub(crate) fn builders() -> &'static [IntegrationBuilder] { | ||
| &[ | ||
| IntegrationBuilder { | ||
| id: "js_asset_proxy", |
There was a problem hiding this comment.
🤔 thinking: this entry's position is load-bearing and nothing here says so. rewrite_attribute chains Replace results and short-circuits on RemoveElement, so js_asset_proxy's precedence over native rewriters (GPT etc.) exists only because it is first in this list. The precedence tests would catch a reorder, but a one-line comment on this entry would make the intent local and stop an innocent alphabetization from changing semantics.
| }) | ||
| } | ||
|
|
||
| fn select_js_asset_proxy_candidates( |
There was a problem hiding this comment.
🌱 seedling: dedup keys on the full URL including the query string, so cache-busted script URLs (?v=<hash>, per-session params) produce a new inventory entry on every audit run and stop matching at runtime once the query changes (runtime matching is exact, query included). Worth eventually flagging volatile-query candidates in the generated comments.
| .map_err(|error| report_error(format!("failed to write command output: {error}"))) | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
⛏ nitpick: this #[cfg(test)] wrapper exists only so two older tests avoid constructing a generator. Having the tests call build_draft_config_with_generator directly would remove the test-only production symbol.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
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 fmtrequired 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-Agentcollapses UA-adaptive vendor bundles — see inline atcrates/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
GETis registered for asset paths — see inline atcrates/trusted-server-core/src/integrations/js_asset_proxy.rs:445 X-TS-JS-Asset-Proxymarker is always emitted — see inline atcrates/trusted-server-core/src/integrations/js_asset_proxy.rs:350
Cross-cutting / body-level findings
-
🔧
cargo fmtrequired check is failing — reproduced locally at this head. Threeuselines injs_asset_proxy.rsneed 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 singlecargo fmt --allfixes 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 there —build_proxy_configunconditionally 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_requestsurfaces that as an error, andhandle()maps every error to502withX-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 — gatestream_responseon 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 reportsCONFLICTING;git merge-tree origin/main <head>shows content conflicts incrates/trusted-server-core/src/config.rs,crates/trusted-server-core/src/integrations/mod.rs, andtrusted-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, everyts auditrun fails, not just this integration — the unit test ataudit/mod.rs:997is what guards it. -
👍 Praise — a few things worth calling out: the upstream
Set-Cookieis deliberately dropped and the request-header allowlist is genuinely minimal (build_proxy_config, verified bybuild_proxy_config_forwards_only_asset_header_allowlist); the audit generator emits opaque randomized/assets/<hex>.jspaths fromOsRngrather than mirroring vendor filenames; the precedence tests against the native GPT rewriter cover all three proxy modes; andvalidate_js_asset_proxy_configcorrectly plugs a real hole —IntegrationSettings::get_typedreturns early for explicitly-disabled configs before callingvalidate(), 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
| } | ||
| }; | ||
|
|
||
| if !response.status().is_success() { |
There was a problem hiding this comment.
🔧 wrench — Any upstream redirect becomes a hard 502 and the script silently disappears from the page.
build_proxy_config sets config.follow_redirects = false (line 266), so proxy_with_redirects returns the 3xx response verbatim. This check then sees !is_success() and replaces it with 502 X-TS-Error: js-asset-origin-status. CDN version aliasing is routine (.../lib/latest.js → .../lib/1.2.3/lib.js), and a vendor that starts redirecting turns a working asset into a dead one with no config knob to recover — the operator's only signal is the log::warn! and a broken page.
The same branch is the 304 trap noted separately: once If-None-Match / If-Modified-Since are added to the request allowlist, a conditional revalidation would also land here and become a 502.
Proposed fix (apply manually — this is a policy decision that spans build_proxy_config and this handler, so it can't be a single-range suggestion):
// In build_proxy_config: allow redirects, but only within the asset's own origin.
let mut config = ProxyRequestConfig::new(origin_url)
.with_streaming()
.with_stream_response()
.without_forward_headers();
config.follow_redirects = true;
config.require_https = true;
config.forward_ec_id = false;with allowed_domains bound to the configured asset's host so a redirect can't walk to an unconfigured origin. If keeping follow_redirects = false is deliberate, then the status check should pass 3xx (and 304) through to the browser instead of collapsing them into 502 — the browser following the vendor redirect is a degraded first-party story, but it is not a broken page.
|
|
||
| toml.push_str("[integrations.js_asset_proxy]\n"); | ||
| toml.push_str("enabled = false\n"); | ||
| toml.push_str("cache_ttl_seconds = 3600\n\n"); |
There was a problem hiding this comment.
🤔 thinking — Every audit-generated draft blanket-overrides the upstream cache policy for all candidates.
cache_ttl_seconds = 3600 at the section level feeds resolved_cache_ttl_seconds, which makes finalize_asset_response emit Cache-Control: public, max-age=3600 and discard whatever the vendor sent — including no-store and private. So the moment an operator flips one candidate to "enabled", that vendor's script is cached publicly for an hour regardless of the vendor's own policy, on a config file they were told to review rather than author.
That is the opposite of the documented default: the spec says "When unset, preserve the upstream cache policy" (2026-04-01-js-asset-proxy-design.md:66), and an earlier review comment on this PR asked for exactly that ("Cache TTL should default to inheriting cache headers and not override"). The pass-through path already works — preserves_upstream_cache_control_without_ttl_override covers it — it just isn't what the generator emits.
Emitting the key commented-out keeps it discoverable without making the override the default:
| toml.push_str("cache_ttl_seconds = 3600\n\n"); | |
| toml.push_str("# Uncomment to override upstream cache headers for every asset below.\n"); | |
| toml.push_str("# cache_ttl_seconds = 3600\n\n"); |
(Verified in a scratch worktree at this head: cargo fmt --all -- --check clean on the edited lines, cargo clippy -p trusted-server-cli --all-targets --all-features -- -D warnings clean, and all trusted-server-cli tests pass — including build_draft_config_writes_disabled_js_asset_proxy_candidates, whose "[integrations.js_asset_proxy]\nenabled = false" assertion is unaffected.)
trusted-server.example.toml:107 carries the same cache_ttl_seconds = 3600; that one at least matches the surrounding house style for the other integrations, so I'd leave it unless you want the two to agree.
|
|
||
| config.with_header( | ||
| HEADER_USER_AGENT.clone(), | ||
| http::HeaderValue::from_static("TrustedServer/1.0"), |
There was a problem hiding this comment.
🤔 thinking — The fixed User-Agent collapses UA-adaptive vendor bundles to a single variant, and the cache then pins it.
Some vendor CDNs branch on User-Agent to serve browser-specific builds (transpiled vs modern, polyfill sets). Sending TrustedServer/1.0 for every request means the vendor sees one client and returns one bundle for the whole audience — plausibly the most conservative one, plausibly one that omits a feature-detected path. Combined with Cache-Control: public, max-age=<ttl>, that single variant is then served to every browser for the TTL, and Vary can't help because the real UA never reaches the origin.
Not a defect in itself — a stable UA is a reasonable privacy posture, and the fingerprint reduction is real. Worth deciding explicitly rather than by omission: either forward the client User-Agent (it is already the most-fingerprinted header the browser sends to the vendor anyway, and the vendor would see it on a direct load), or keep it fixed and note the limitation in the spec's header-policy section so operators know not to proxy UA-adaptive assets.
| } | ||
|
|
||
| fn validate_js_asset_proxy_config(settings: &Settings) -> Result<(), Report<TrustedServerError>> { | ||
| let Some(raw_config) = settings.integrations.get("js_asset_proxy") else { |
There was a problem hiding this comment.
♻️ refactor — The integration ID is a bare string literal here, three times in this file (DEPLOY_VALIDATED_INTEGRATION_IDS at line 43, this lookup, and both error messages), while js_asset_proxy.rs already defines JS_ASSET_PROXY_INTEGRATION_ID for exactly this.
The const is private, so this file can't reach it today. Since the ID also has to match the JS-side/registry naming, having a single definition is worth the one-line visibility change:
// crates/trusted-server-core/src/integrations/js_asset_proxy.rs
pub(crate) const JS_ASSET_PROXY_INTEGRATION_ID: &str = "js_asset_proxy";then use it here and at line 43. Apply manually — can't be auto-applied as a suggestion because it touches two files.
| .assets | ||
| .iter() | ||
| .filter(|asset| asset.proxy == JsAssetProxyMode::Enabled) | ||
| .map(|asset| IntegrationEndpoint::new(Method::GET, asset.path.clone())) |
There was a problem hiding this comment.
⛏ nitpick — Only GET is registered, so a HEAD on a configured asset path misses the integration router entirely and falls through to the publisher-origin proxy, which will answer for a path the origin has never heard of.
Shields, health checkers, and some prefetchers do issue HEAD for scripts. Registering Method::HEAD alongside GET (the handler needs no change — the upstream will omit the body) would keep the two consistent.
| let mut finalized = Response::new(body); | ||
| *finalized.status_mut() = status; | ||
| finalized.headers_mut().insert( | ||
| HEADER_X_TS_JS_ASSET_PROXY, |
There was a problem hiding this comment.
📝 note — X-TS-JS-Asset-Proxy: true ships on every asset response, which makes the proxying trivially detectable by anyone probing the first-party paths — including the vendors whose scripts are being fronted, and any anti-adblock logic looking for rehosted tags.
It is clearly useful for debugging, and the paths are opaque, so this is not a leak by itself. If it is only meant for diagnostics, consider gating it the way the auction HTML comments are gated behind [debug] config rather than emitting it unconditionally in production.
6d6f589 to
a82aaf2
Compare
|
@ChristianPavilonis to resolve feedback |
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
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-Modifiedadvertised downstream but conditional requests never forwarded upstream — see inline atcrates/trusted-server-core/src/integrations/js_asset_proxy.rs:288-296
🤔 thinking
Cache-Control: publicon a route that can also mint an EC cookie — see inline atcrates/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 attrusted-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 warningsandcargo test --workspace. PerCLAUDE.md, a workspace-wide clippy trips the Cloudflare adapter's non-wasm32guard; run against this head it exits 101 atcrates/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 against8fc2477and all pass:cargo fmt --all -- --check, all sixclippy-*aliases, all fourtest-*aliases, and the cross-adapter parity suite. Please update the description to theCLAUDE.mdgate list. -
👍 The
supports_streaming_responses()gating inproxy.rsfixes a latent cross-adapter break. Onmain,handle_asset_proxy_requestsetwith_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 atproxy.rs:4265andproxy.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)
| finalized.headers_mut().insert( | ||
| HEADER_X_TS_JS_ASSET_PROXY, | ||
| http::HeaderValue::from_static("true"), | ||
| ); |
There was a problem hiding this comment.
🔧 wrench — Third-party bytes are served from the publisher origin with no content-type protection.
finalize_asset_response copies the upstream Content-Type verbatim (js_asset_proxy.rs:383-387) and sets no X-Content-Type-Options. Every proxy = "enabled" asset is a real, directly navigable first-party GET route (routes(), js_asset_proxy.rs:472-479), so https://publisher.example/assets/<path>.js renders in the publisher's origin, not the vendor's.
A vendor origin that returns a sniffable or HTML-ish body — a CDN compromise, which is precisely the scenario proxy = "blocked" exists to handle, or plain misconfiguration — gets those bytes interpreted against the publisher origin. Before proxying, the same bytes rendered in the vendor's origin. Nothing downstream constrains the type: proxy_request runs with stream_passthrough = true, so finalize_proxied_response_streaming (proxy.rs:661-670) passes the body through untouched.
The repo already has a policy for this exact shape. prebid's external-bundle proxy — the other first-party route that proxies third-party JavaScript — pins Content-Type: application/javascript; charset=utf-8 and sets x-content-type-options: nosniff (prebid.rs:826-833). js_asset_proxy does neither.
nosniff closes the sniffing half:
| finalized.headers_mut().insert( | |
| HEADER_X_TS_JS_ASSET_PROXY, | |
| http::HeaderValue::from_static("true"), | |
| ); | |
| finalized.headers_mut().insert( | |
| HEADER_X_TS_JS_ASSET_PROXY, | |
| http::HeaderValue::from_static("true"), | |
| ); | |
| // Upstream controls these bytes; refuse MIME sniffing so a mislabelled | |
| // body cannot be reinterpreted against the publisher origin. | |
| finalized.headers_mut().insert( | |
| header::X_CONTENT_TYPE_OPTIONS, | |
| http::HeaderValue::from_static("nosniff"), | |
| ); |
To be clear about what this does and does not fix: nosniff does not stop an upstream that explicitly declares text/html. Full parity with prebid means pinning or allowlisting the forwarded Content-Type, which cannot be a one-click suggestion here because successful_response_preserves_body_and_expected_headers (js_asset_proxy.rs:1147-1153) asserts the upstream value is preserved verbatim — that fix needs a test change alongside it. Worth deciding in this PR or tracking as a follow-up.
(Verified in a scratch worktree at 8fc2477: cargo fmt --all -- --check, all six clippy-* aliases, all four test-* aliases, and the cross-adapter parity suite all pass with this suggestion applied alone.)
| for header_name in [ | ||
| &HEADER_ACCEPT, | ||
| &HEADER_ACCEPT_LANGUAGE, | ||
| &HEADER_ACCEPT_ENCODING, | ||
| ] { | ||
| if let Some(value) = req.headers().get(header_name).cloned() { | ||
| config = config.with_header(header_name.clone(), value); | ||
| } | ||
| } |
There was a problem hiding this comment.
♻️ refactor — ETag/Last-Modified are advertised downstream but conditional requests are never forwarded upstream.
The request allowlist here is Accept, Accept-Language, Accept-Encoding with copy_request_headers = false, so If-None-Match and If-Modified-Since are dropped. But finalize_asset_response forwards ETag (js_asset_proxy.rs:406-408) and Last-Modified (js_asset_proxy.rs:409-413) to the browser.
Net effect: once max-age expires, every revalidation re-downloads the full body — the validators Trusted Server advertises can never produce a 304. Correct, but the caching story the spec asks for is only half built.
There's a second-order trap too: if conditional forwarding is added later, if !response.status().is_success() (js_asset_proxy.rs:509) will map the resulting 304 to a 502 js-asset-origin-status. Both parts want fixing together:
// build_proxy_config: extend the allowlist
for header_name in [
&HEADER_ACCEPT,
&HEADER_ACCEPT_LANGUAGE,
&HEADER_ACCEPT_ENCODING,
&header::IF_NONE_MATCH,
&header::IF_MODIFIED_SINCE,
] { /* ... */ }
// handle(): let 304 through instead of mapping it to 502
let status = response.status();
if !status.is_success() && status != StatusCode::NOT_MODIFIED {
// ... existing 502 path
}Apply manually — can't be auto-applied as a suggestion because the fix spans two non-contiguous ranges in this file (:288-296 and :509).
| if let Some(ttl) = self.resolved_cache_ttl_seconds(asset) { | ||
| finalized.headers_mut().insert( | ||
| header::CACHE_CONTROL, | ||
| http::HeaderValue::from_str(&format!("public, max-age={ttl}")) | ||
| .expect("should build JS asset proxy Cache-Control header"), | ||
| ); |
There was a problem hiding this comment.
🤔 thinking — Cache-Control: public on a route that can also mint an EC cookie.
finalize_asset_response sets Cache-Control: public, max-age=<ttl> whenever a TTL is configured, and trusted-server.example.toml:119 ships cache_ttl_seconds = 3600.
Separately, IntegrationRegistry::handle_proxy calls ec_context.generate_if_needed(...) for navigation requests (registry.rs:973-980), and is_navigation_request is true for Sec-Fetch-Dest: document (http_util.rs:73-82). So a first-time visitor who navigates directly to an asset URL gets ec_finalize_response → set_ec_cookie_on_response (ec/finalize.rs:105-107) attaching a per-user Set-Cookie to a response marked public, max-age=3600.
Narrow rather than alarming: subresource <script src> loads aren't navigations, and RFC-compliant shared caches generally refuse to store Set-Cookie responses. But the combination is easy to avoid — skip EC generation for this integration's routes, or don't mark the response public when a cookie was attached. Flagging so it's a deliberate choice rather than an accident.
| [[integrations.js_asset_proxy.assets]] | ||
| path = "/assets/example-vendor-loader.js" | ||
| origin_url = "https://cdn.example.com/vendor-loader.js" | ||
| proxy = "enabled" |
There was a problem hiding this comment.
⛏ nitpick — Sample asset ships as proxy = "enabled", so one edit activates it.
ts config init writes this file verbatim (commands/config/init.rs:51). Both specs standardize the safe default on proxy = "disabled" and describe activation as two deliberate edits (2026-06-22-ts-audit-js-asset-proxy-config-design.md §1 and §7), but the template ships enabled = false at the integration level with the asset already at proxy = "enabled". Flipping the one obvious switch silently registers a live route to a placeholder origin.
Harmless in practice — nothing references that path — but it undercuts the two-step activation the docs promise.
| proxy = "enabled" | |
| proxy = "disabled" |
(Verified in a scratch worktree at 8fc2477: full gate — fmt, all six clippy-*, all four test-*, parity — passes with this applied alone and batched with the other suggestion.)
4 participants
Summary
js_asset_proxyintegration<script src>rewriting, disabled assets, and blocked script removaltrusted-server.tomlRelated
Closes #762
Verification
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspace