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