Add path-based asset backend proxy routing#668
Add path-based asset backend proxy routing#668ChristianPavilonis wants to merge 7 commits intomainfrom
Conversation
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Path-based asset proxy routing closely follows the spec, with a clean separation from the publisher pipeline and a conservative forwarded-header set. Concerns are concentrated around (1) origin_url validation that lets path/query through silently, (2) missing test coverage for spec-stated invariants — most importantly that asset failures must not silently fall back to publisher.origin_url, and (3) one CLAUDE.md doc-comment compliance gap.
Blocking
🔧 wrench
- Missing doc comments on
ProxyAssetRoute(crates/trusted-server-core/src/settings.rs:336-339) origin_urlvalidation accepts URLs with path/query that are silently dropped at runtime (crates/trusted-server-core/src/settings.rs:740-766)- No test that asset-origin failures stop at 502 without falling back to
publisher.origin_url(crates/trusted-server-adapter-fastly/src/route_tests.rs, around theasset_routes_*tests) — spec §9 forbids the fallback; the impl is correct but unpinned by tests.
Non-blocking
🤔 thinking
Strict-Transport-Securityfrom asset origin forwarded unchanged (crates/trusted-server-core/src/proxy.rs:657-663) — same class of risk as upstreamSet-Cookie, which is correctly stripped. Either strip HSTS too or document explicitly.- Spec-listed test gaps: HEAD passthrough, non-GET/HEAD bypass, redirect pass-through, query-string-ignored-for-matching. All small additions given existing helpers.
- String-prefix matching can match non-segment boundaries (
prefix = "/static"matches/staticfile.js). Spec is explicit, but every example uses a trailing/. Worth a sentence in the toml comment and field doc.
♻️ refactor
- Idiomatic method-set check in
route_request(crates/trusted-server-adapter-fastly/src/main.rs:153-156) — replace thematch &method { &Method::GET | &Method::HEAD => ... }withmatches!(...).then(...).flatten(). - Test-stub duplication:
StaticResponseHttpClientcould move intoplatform::test_supportby extendingStubHttpClientwithpush_response_with_headers. - Validation split between
Proxy::normalizeandProxy::prepare_runtimeis inconsistent with the rest ofSettings, which uses#[validate]attributes. Either consolidate or comment the rationale.
🌱 seedling
- WASM heap pressure for large asset bodies (
crates/trusted-server-adapter-fastly/src/platform.rs:223-237→fastly_response_to_platformcallstake_body_bytes();crates/trusted-server-core/src/proxy.rs:657re-buffers viaset_body(Vec<u8>)). Same asPublisherResponse::PassThrough, so not a regression — but this PR is specifically targeting asset traffic, where bodies are routinely larger than HTML. Track as a streaming pass-through follow-up mirroringPublisherResponse::Stream. (Anchored in the body sinceplatform.rsis unchanged in this PR.)
⛏ nitpick
- Validation error wording (
crates/trusted-server-core/src/settings.rs:716,validate_no_trailing_slash): the spec says "must not include a trailing slash"; the error message says "origin_url must not end with '/'". Aligning the wording reads slightly better in operator-facing errors. The function itself is unchanged in this PR but is reached via the newvalidate_proxy_origin_url. (Anchored in the body since the line is outside the diff.) - Redundant
to_ascii_lowercase()ontarget_url.scheme()— already lowercase from URL parsing. See inline comment. Set-Cookiestrip test only verifies a single header is removed; should test multiple. See inline comment.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS (858 tests, 0 failures locally; CI green)
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- browser & integration tests: PASS
- CodeQL / Analyze: PASS
|
Tested proxying, it does work, however proxying to a s3 bucket that requires authentication isn't implemented. |
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Asset-proxy routing closely follows the spec, every prior blocking comment has been addressed with a paired test, and CI is green. Two blockers remain: (1) path_pattern regex is recompiled on every request despite a working OnceLock pattern ten lines up in the same file, and (2) the spec checked in by this PR explicitly lists path rewrite as out of scope but the PR adds path rewrite. Two follow-up question/scope items, plus a handful of non-blockers around header handling and validation tightness.
Blocking
🔧 wrench
path_patternregex recompiled on every request (crates/trusted-server-core/src/settings.rs:435-447) —Handlerten lines up caches viaOnceLock.prepare_runtimealready compiles for fail-fast and throws the result away. Hot-path WASM perf hole for the Cloudinary-style example shipped intrusted-server.toml.- Spec contradicts implementation in the same PR (
docs/superpowers/specs/2026-04-28-multi-backend-asset-proxy-design.md:55-58) — spec says path rewrite and regex matching are out of scope, but the PR adds both. Update the spec or split the rewrite into a follow-up.
❓ question
- Why is the publisher Host-override change in this PR? (
crates/trusted-server-core/src/settings.rs:18-20) — separate publisher-pipeline feature stacked on top of asset-routing. Independent surface, independent rollback risk.
Non-blocking
🤔 thinking
validate_host_header_valueis too permissive (crates/trusted-server-core/src/settings.rs:898-907) — passes spaces, slashes, query strings. Used both as a literalHost:header and to format a URL.Clear-Site-Datanot stripped from asset responses (crates/trusted-server-core/src/proxy.rs:660-661) — same threat class as HSTS / Set-Cookie which are correctly stripped.X-Forwarded-Forlisted but always empty (crates/trusted-server-core/src/proxy.rs:28) —sanitize_forwarded_headersstrips XFF at the edge before routing. Asset CDNs see Trusted Server's IP only.
♻️ refactor
- Dead-code error path in
target_path_for(crates/trusted-server-core/src/settings.rs:459-466) — unreachable.ok_or_else(...)with a misleading error message. from_url_with_first_byte_timeout_and_override_host(crates/trusted-server-core/src/backend.rs:303-318) — 51-character method name; the API is asking for a builder. Out of scope for this PR.
🌱 seedling
- Backend-name collision via
replace(['.', ':'], \"_\")(crates/trusted-server-core/src/backend.rs:144-149) — operator-controlled so practical risk is near-zero, but a hash or non-replacing escape would be more defensive.
⛏ nitpick
matched_asset_routecomputed eagerly (crates/trusted-server-adapter-fastly/src/main.rs:153-155) — only used in the catch-all branch. Move inside the_ =>arm.- Double Host header in publisher.rs (
crates/trusted-server-core/src/publisher.rs:528) —BackendConfig::override_hostis already authoritative; the manualset_headeris redundant. - POST-bypass test could be stronger (
crates/trusted-server-adapter-fastly/src/route_tests.rs:399-410) —assert_ne!(OK)is true for any non-200; pinBAD_GATEWAYlike the other tests.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS (875 tests on this worktree, 0 failures)
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- browser & integration tests: PASS
- CodeQL / Analyze: PASS
| ), | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🔧 wrench — path_pattern regex is recompiled on every request. Regex::new(pattern) runs each time target_path_for is called, so every asset request that hits a path-rewriting route pays the compile cost on a WASM edge runtime. Compare to Handler::compiled_regex ten lines up in this file, which caches via OnceLock<Result<Regex, String>>. prepare_runtime already compiles for fail-fast validation — that compiled regex is just thrown away.
The toml ships a Cloudinary-style example using path_pattern/target_path, so this is the path real publishers will hit first.
Fix — mirror Handler:
#[derive(Debug, Default, Clone, Deserialize, Serialize)]
pub struct ProxyAssetRoute {
pub prefix: String,
pub origin_url: String,
pub path_pattern: Option<String>,
pub target_path: Option<String>,
#[serde(skip, default)]
compiled_pattern: OnceLock<Result<Regex, String>>,
}
impl ProxyAssetRoute {
fn compiled_path_pattern(&self) -> Result<Option<&Regex>, Report<TrustedServerError>> {
let Some(pattern) = self.path_pattern.as_deref() else {
return Ok(None);
};
match self.compiled_pattern.get_or_init(|| Regex::new(pattern).map_err(|e| e.to_string())) {
Ok(regex) => Ok(Some(regex)),
Err(message) => Err(Report::new(TrustedServerError::Configuration {
message: format!("proxy.asset_routes path_pattern `{pattern}` failed to compile: {message}"),
})),
}
}
}| ### Out of scope | ||
|
|
||
| - Regex-based route matching | ||
| - Path rewrite / prefix replacement |
There was a problem hiding this comment.
🔧 wrench — Spec contradicts the implementation in the same PR. This file (added by this PR) lists "Regex-based route matching" and "Path rewrite / prefix replacement" as out of scope, and §3 says "There is no path rewrite in v1." But the PR adds both via path_pattern (regex) and target_path (replacement) on ProxyAssetRoute.
Whichever happened first, they have to agree at merge time — the next person reading the spec to change behavior will get a wrong contract.
Fix — pick one:
-
Update §"Out of scope" to remove "Regex-based route matching" and "Path rewrite / prefix replacement", update §3 to describe the new
path_pattern/target_pathsemantics, and add validation rules to §"Field definitions" and §"Hard validation errors" for the new fields. (Recommended — the rewrite is genuinely useful for the Cloudinary/Raven cases shown intrusted-server.toml.) -
Drop
path_pattern/target_pathfrom this PR and ship them as a follow-up with their own spec amendment.
| pub origin_url: String, | ||
| /// Optional upstream Host header value used when connecting to an origin | ||
| /// whose routing host differs from the backend host. | ||
| pub origin_host_header: Option<String>, |
There was a problem hiding this comment.
❓ question — Why is the publisher origin_host_header change in this PR? The PR title and the spec under docs/superpowers/specs/ are about asset-proxy routing, but commits 126e7fe and 1efc12d added Publisher.origin_host_header, origin_host_header_value(), and origin_rewrite_url() — a separate publisher-path feature with its own validation, normalization, and tests, that touches the consent-aware publisher pipeline.
Is there a coupling to asset-routing I'm missing, or can these be split into a follow-up PR for review isolation? Stacking three independent features (asset routing, path rewrite, publisher Host override) increases blast radius on rollback.
| return Err(err); | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
🤔 thinking — validate_host_header_value only blocks empty + control chars. Spaces, slashes, ?, # all pass. But this value is used in two places that need stricter shape:
- As a literal
Host:header value at publisher.rs:528 — RFC 7230 limits Host tohost[:port]. - To format a URL via
format!("{scheme}://{host_header}")inorigin_rewrite_url— slashes/queries here would silently produce malformed rewrite URLs.
A fat-fingered origin_host_header = "evil.com/path" would pass validation and produce a malformed Host header AND a wrong rewrite URL.
Fix — re-use URL parsing for shape validation:
fn validate_host_header_value(value: &str) -> Result<(), ValidationError> {
let probe = format!("http://{value}");
let parsed = Url::parse(&probe).map_err(|_| ValidationError::new("invalid_host_header"))?;
if parsed.host_str().is_none()
|| !matches!(parsed.path(), "" | "/")
|| parsed.query().is_some()
|| parsed.fragment().is_some()
|| parsed.username() != ""
{
return Err(ValidationError::new("invalid_host_header"));
}
Ok(())
}| // Asset origins must not be able to set first-party cookies or publisher | ||
| // domain transport security policy through this proxy path. | ||
| response.remove_header(header::SET_COOKIE); | ||
| response.remove_header(header::STRICT_TRANSPORT_SECURITY); |
There was a problem hiding this comment.
🤔 thinking — Strip list should also include Clear-Site-Data. You correctly strip Set-Cookie and Strict-Transport-Security because the asset origin can't be allowed to shape the publisher's cookies or transport security. Clear-Site-Data: "*" is the same threat class — a misbehaving image CDN could wipe the publisher's first-party cookies, storage, and cache on every asset response.
response.remove_header(header::SET_COOKIE);
response.remove_header(header::STRICT_TRANSPORT_SECURITY);
response.remove_header("clear-site-data");(Probably worth adding the strip list as a const near ASSET_PROXY_FORWARD_HEADERS so future additions land in one place, and updating §"Security Considerations" / §"Response handling" in the spec to document the deviation from raw pass-through.)
| self.prefix | ||
| ), | ||
| }) | ||
| })?; |
There was a problem hiding this comment.
♻️ refactor — Dead-code error path. We're inside the (Some(_), Some(target_path)) arm, so compiled_path_pattern() cannot return None (it only returns None when self.path_pattern is None). The .ok_or_else(...) is unreachable, and its message — "has a target_path without path_pattern" — doesn't even fit this branch.
(Some(pattern), Some(target_path)) => {
let regex = match self.compiled_pattern.get_or_init(
|| Regex::new(pattern).map_err(|e| e.to_string())
) {
Ok(regex) => regex,
Err(message) => return Err(Report::new(...)),
};
// ...rest unchanged
}| let override_host_suffix = self | ||
| .override_host | ||
| .filter(|host| !host.is_empty()) | ||
| .map(|host| format!("_oh_{}", host.replace(['.', ':'], "_"))) |
There was a problem hiding this comment.
🌱 seedling — host.replace(['.', ':'], "_") collides on inputs that already contain underscores. Two distinct override hosts would map to the same backend name (www.foo.com → www_foo_com, vs. an actual host literally named www_foo_com). Operator-configured, so practical risk is near-zero, but a hash or a non-replacing escape (e.g., percent-encode dots/colons) would be more defensive.
|
|
||
| let matched_asset_route = matches!(method, Method::GET | Method::HEAD) | ||
| .then(|| settings.asset_route_for_path(&path)) | ||
| .flatten(); |
There was a problem hiding this comment.
⛏ nitpick — matched_asset_route is computed for every GET/HEAD request but only consumed in the catch-all _ => arm at line 211. With 0 asset routes (the common case) it's a no-op iter, so cost is negligible — but moving it inside the _ => arm reads more clearly: the asset-route lookup is fallback-path logic, not pre-routing logic.
| origin_host_header | ||
| ); | ||
| // Only advertise encodings the rewrite pipeline can decode and re-encode. | ||
| restrict_accept_encoding(&mut req); |
There was a problem hiding this comment.
⛏ nitpick — Double Host header. BackendConfig::override_host(Some(&origin_host_header)) at line 514 already configures Fastly's authoritative upstream Host, and this line sets it again on the request. Pre-existing pattern, but now that override is configurable via origin_host_header_value(), the manual set_header is redundant — Fastly's Backend::override_host wins at send time.
| Arc::new(NoopBackend), | ||
| Arc::clone(&http_client) as Arc<dyn PlatformHttpClient>, | ||
| ); | ||
|
|
There was a problem hiding this comment.
⛏ nitpick — assert_ne!(resp.get_status(), StatusCode::OK) is true for any non-200 response, including unrelated 500s. The other tests in this file pin the publisher fallback specifically with assert_eq!(StatusCode::BAD_GATEWAY). Same here would be a stronger assertion that POST genuinely falls through to the publisher fallback path, not just "didn't return the asset response".
2 participants
Summary
[[proxy.asset_routes]]path-prefix routing so selected first-party asset paths can proxy to a different backend origin thanpublisher.origin_url.GET/HEAD, uses longest-prefix-wins, and preserves the full incoming path/query while swapping only the origin.Changes
docs/superpowers/specs/2026-04-28-multi-backend-asset-proxy-design.mdcrates/trusted-server-core/src/settings.rsProxyAssetRoute,proxy.asset_routesconfig support, asset-route normalization/validation, duplicate-prefix warnings, and longest-prefix matching helpers.crates/trusted-server-core/src/proxy.rscrates/trusted-server-adapter-fastly/src/main.rscrates/trusted-server-adapter-fastly/src/route_tests.rstrusted-server.toml[[proxy.asset_routes]]configuration shape with a commented example.Closes
Closes #663
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)