Match CSS url() quoting rules when rewriting creative styles - #1106
Match CSS url() quoting rules when rewriting creative styles#1106prk-Jr wants to merge 13 commits into
Conversation
Treat a url() value as quoted only when a matching closing quote is present, and end a quoted value at that quote rather than at the first paren, so a value is rewritten the way a browser reads it. Derive srcset candidate state as the scan advances rather than from the candidate prefix at each comma.
The rewriter does not resolve CSS escapes, so a value carrying a backslash cannot be mapped to the resource the page will actually request; proxying the raw bytes points somewhere else. A raw newline, which preprocessing also produces from a carriage return or a form feed, makes the value a bad string the browser discards, so rewriting it proxies a URL that is never fetched. Both are now passed through untouched. Also fold an escaped CRLF into a single escaped newline when locating the end of a quoted string, and end the string at a form feed, so the extent matches what preprocessing produces.
Inserting the resolvability helper above css_string_end left that function's doc comment attached to the new helper.
|
Closes #1114 |
The scan bounded a value by the next quote and paren, so a span could fuse across declarations, a missing paren abandoned the rest of the input, an escape became a URL nobody requests, and a value ending at input was skipped. It also stepped back from a computed index, which slices a multi-byte character and aborts the guest. Reading with a tokenizer supplies both the extent and the resolved value, and leaves a malformed value — which a browser discards anyway — on its original bytes. Cover the other references a browser fetches: src(), a bare string candidate in image-set(), and an @import prelude, which takes one URL and reads later strings as media queries. A string counts as a URL only in those places, so font-family and content keep theirs, and an @import is a rule only where a top-level rule may start — the same token is data inside a declaration, in another prelude, and in a style attribute, which is not a stylesheet. The walk recurses per scope through upstream-supplied CSS, so it is bounded, and CSS past the bound is rejected rather than passed through below it. cssparser already built here as a transitive dependency.
… fix/creative-parser-bounds
A rewritten value went out as url() whatever it arrived as. For src() that changes what the browser does rather than where it points: an engine that ignores src() leaves the declaration inert, so emitting url() starts a request the origin never made. Keep the name and proxy the value. Read a var() fallback in the context around it, so a candidate written image-set(var(--c, "https://…") 1x) is proxied like the plain string it becomes. The propagation is deliberately narrow: substitution applies to declaration values, so the same fallback in a content value or an @import prelude stays untouched. A URL assembled from a separate custom-property declaration is left alone and documented — pairing the two is the cascade's work, not a rewriter's.
Bare-string references still went out wrapped in url(). That is valid in the places they are read, so nothing broke, but it is not valid once the same candidate is substituted into a src() argument, which has to stay a string. Re-emit each reference in the shape it arrived in and replace only the value. That makes the remaining src() gap fixable: src() takes a normal value list, so a var() there is substituted and its fallback is the string the engine ends up with. Walk it and rewrite the fallback where it sits, leaving both calls intact. url() is deliberately excluded, since an engine does not substitute inside it and a fallback there is never requested. The entry-point note claimed everything was re-emitted as url(), which the earlier src() change had already made untrue.
Both resolve to their fallback when the name is not set, so a string written there is a string the engine ends up with. Only var() was followed, which left image-set(env(--x, "https://...")) unproxied - and that is a supported feature reached by an unrecognised name, which is the case the fallback exists for, not a future one. Follow both, and pin that this reaches no further: a string in a gradient nested inside image-set is still not a URL.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
This is a larger change than the description suggests: commits from a8d4e099e onward replace the hand-rolled CSS scanner with a cssparser-based grammar walk, adding url()/src(), image-set() bare-string candidates, @import preludes, and var()/env() fallback following. The token-level, property-agnostic design is the right shape for this problem — it covers URL-bearing properties nobody had to enumerate, while correctly leaving fragment-only references like filter:url(#blur) alone.
Two findings below. The depth cap admits a different nesting level depending on whether a URL is quoted, and the depth rejection is operationally invisible on the wire.
Both are prose-only: the wrench fix restructures a match across three arms and needs a companion test change, so it can't be expressed as a single contiguous suggestion.
Verified locally against the PR head: cargo fmt --all -- --check, clippy-fastly, test-fastly (2301 passed), test-axum, check-cloudflare, check-spin, and the cross-adapter parity suite (13 passed) all pass.
Three things I checked and am explicitly not raising, since each looked like a finding and turned out not to be:
@import layer(base) "url.css"looked like a missed egress leak, but the CSS grammar requires the URL first — that prefix form is invalid and browsers do not fetch it.&becoming&in rewritten<style>output is real, butgit show origin/mainconfirms the handler is byte-identical tomain. Pre-existing, not this PR.- The
cssparserdependency is free:cssparser 0.36.0is already inmain's lockfile vialol_html(Cargo.lock:2911). No new packages, and deleting the hand-rolled scanner makes the shipped WASM code sections roughly 19.7 KB smaller.
I also probed parse-error recovery specifically (bad-url tokens, unterminated strings, stray ), unmatched }, embedded NUL, CDO/CDC) and could not construct an input where a URL after the error point escapes rewriting.
Blocking
🔧 wrench
- Depth cap rejects at a different depth depending on whether the URL is quoted — see inline at
crates/trusted-server-core/src/creative.rs:93
❓ question
- Blanking the whole stylesheet is invisible to the client — see inline at
crates/trusted-server-core/src/creative.rs:159
Non-blocking
📝 note
- PR description no longer matches the change — see the Cross-cutting section below
Cross-cutting / body-level findings
- 📝 PR description no longer matches the change — The Summary and Changes tables describe a
css_string_endhelper and positional quote-scanning, which commita8d4e099edeleted. The "Verification against production CSS" section describes a differential run over a parser version that is no longer what ships, so that evidence no longer covers the code under review. Worth refreshing before merge so the squashed commit message describes what actually landed — and worth re-running the production-CSS differential against the grammar walk, since that check is genuinely valuable and currently attests to superseded code.
CI Status
At the time of review, checks on 441c831 were still running (the head is a fresh merge commit). No failures observed; the findings above are independent of CI.
- CodeQL: SKIPPED
- Analyze (actions): PASS
- Analyze (rust): PENDING
- Analyze (javascript-typescript): PENDING
- cargo test: PENDING (required)
- cargo test (axum native): PENDING
- cargo test (ts CLI, native): PENDING
- cargo test (cross-adapter parity): PENDING
- cargo check (cloudflare native + wasm32-unknown-unknown): PENDING
- cargo check/build/test (spin native + wasm32-wasip1): PENDING
- cargo fmt: PENDING (required)
- format-typescript: PENDING (required)
- format-docs: PENDING (required)
- vitest: PENDING
- prepare integration artifacts: PENDING
Reading the single string argument of `url()` or `src()` opened a parser
scope, so `url("https://…")` was charged a nesting level that the
identical `url(https://…)` was not. At exactly the cap that decided
whether the whole stylesheet survived, which is not a distinction the
constant ever claimed to make. That grammar is terminal and costs no
recursion, so it is no longer charged, and every recursion the walk makes
now runs through one `descend` that no arm can bypass. A form that does
open a scope, such as `image-set()`, still costs a level, and the bound
now follows the grammar rather than how a URL is spelled.
A refused stylesheet on the CSS proxy path was returned as an empty 200,
indistinguishable from a stylesheet the origin legitimately served empty
and attributable only from an edge-side log. `rewrite_css_body` now
reports the refusal to its caller, so the response carries a status the
way the oversized-body path already does. Markup still drops only the
offending `<style>` block or attribute, since the rest of the document is
rewritten either way, and the log names which one it dropped.
|
Both non-blocking items addressed. PR description — rewritten. It described the Production-CSS differential — re-run against the grammar walk, and widened, because the old corpus could not have caught anything. The publisher'''s own Next.js bundles carry 2 Method: two detached worktrees at
The 5 that differ differ only in that this branch emits Stating the gap rather than leaving it implied: the corpus holds 8,258 |
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed PR head 60b5e992c004969836febef3efd5340ceaa85d1c.
The latest commit fixes the inconsistent nesting boundary and now reports rejected CSS responses through the proxy error path. I left two P2 findings inline.
Non-blocking documentation fix: crates/trusted-server-core/README.md:37 still documents rewrite_css_body as returning String; it now returns Result<String, CssRewriteError>.
All checks on this revision pass.
| /// Rewrites URL references inside a CSS string to the first-party proxy. | ||
| /// | ||
| /// Covers every form the browser fetches: `url()` and `src()`, a bare string | ||
| /// candidate in `image-set()`, and an `@import` prelude string. |
There was a problem hiding this comment.
[P2] Preserve signed query separators inside <style>
When the newly covered forms reach the existing <style> handler at line 1263, ContentType::Text HTML-escapes the generated URL's &tstoken= as &tstoken=. A <style> element is raw text, so the browser does not decode that entity before CSS parsing. It requests a URL with an amp;tstoken parameter, and /first-party/proxy rejects it for missing tstoken.
This PR makes the problem affect bare image-set() candidates, src(), and bare @import strings that previously remained direct. Please replace the style text without HTML body escaping and add a rewrite_creative_html test asserting that the result contains &tstoken= and not &tstoken=.
| .map_err(|e| io::Error::other(format!("Invalid UTF-8 in CSS: {e}")))?; | ||
|
|
||
| let rewritten = rewrite_css_body(self.settings, &css); | ||
| let rewritten = rewrite_css_body(self.settings, &css).map_err(io::Error::other)?; |
There was a problem hiding this comment.
[P2] Bound transformed CSS output
The processor caps buffered input at 10 MiB, but rewrite_css_body can expand every short URL into a much longer signed proxy URL and return an output far above that limit. Repeated newly supported image-set("https://...") candidates make a permitted stylesheet a practical WASM memory-exhaustion input.
Checking the length after rewriting would still allow the large allocation. Please enforce the limit while building CssUrlRewriter::out and return an error once the output budget is exceeded. The same bound should cover CSS rewritten inside proxied HTML, where the temporary CSS string is allocated before the outer HTML output limit runs.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed at 60b5e992. Both findings from my previous pass are fixed, and I verified each by assertion rather than from the commit message: the depth bound now admits url(x), url("x"), url('x') and src("x") at the same level, with image-set() still costing one because it genuinely opens a scope; and a refused stylesheet now reaches finalize_proxied_response as an error, which maps to a 502 instead of an empty 200. The descend() chokepoint is a better fix than the one I proposed, since it makes the bound structural rather than something each new arm has to remember.
Not approving, because of one regression in the paths this PR newly covers.
@ChristianPavilonis raised this as P2 and I want to add a reproduction, because the framing matters: I saw the & escaping on my first pass and set it aside as pre-existing on main, which was the wrong call. It is pre-existing for url(), but image-set(), src() and bare @import strings were passed through untouched before this PR, so they loaded correctly. Now they are rewritten into URLs the proxy rejects. Those three forms regress from working to broken, and that is this PR's to fix.
Blocking
🔧 wrench
- Newly covered
<style>forms emit&tstoken=and fail at the proxy —crates/trusted-server-core/src/creative.rs:1263, detailed below
Cross-cutting / body-level findings
- 🔧 Newly covered
<style>forms emit&tstoken=and fail at the proxy —ContentType::TextHTML-escapes the signed URL, and this PR makes that break forms which previously worked.
ContentType::Text entity-escapes &, but a <style> element is raw text — the HTML parser does not decode entities inside it, so the CSS parser sees &tstoken= literally. The browser requests …&tstoken=…, /first-party/proxy sees a parameter named amp;tstoken and no tstoken, and rejects the request.
Confirming @ChristianPavilonis's P2 with a reproduction, and correcting my own earlier read of it. I saw this escaping on my first pass and set it aside as pre-existing on main. That was right for url() — which was already rewritten there and already broken this way — but wrong as a reason to leave it. image-set(), src() and bare @import strings were not rewritten before this PR, so they reached the browser as direct third-party URLs and loaded. Now they are rewritten into URLs the proxy rejects, so they regress from working to broken:
<style> content at 60b5e992 |
emits |
|---|---|
image-set("https://cdn.example/a.png" 1x) |
&tstoken= — asset fails to load |
src("https://cdn.example/f.woff2") |
&tstoken= — font fails to load |
@import "https://cdn.example/x.css"; |
&tstoken= — stylesheet fails to load |
ContentType::Html inserts the replacement without entity-escaping, which is the correct content model for a raw-text element:
text!("style", |t| {
let s = t.as_str();
let rewritten = rewrite_style_urls(settings, s, base_origin);
if rewritten != s {
t.replace(&rewritten, ContentType::Html);
}
Ok(())
}),Verified in a scratch worktree at this head: with that one-line change, all four forms above emit a raw &tstoken= and none emit &tstoken=, and the full cargo test-fastly suite still passes (2302 tests, 0 failures).
On the injection question that ContentType::Html naturally raises — I checked, and it does not open one here. A </style> sequence inside a CSS string terminates the element identically before and after this change, because the HTML tokenizer ends raw text at that literal regardless of how the replacement was inserted; I confirmed the escaping form is byte-identical in a case the rewriter leaves untouched. Guarding creative markup against that is sanitize_creative_html's job, and it strips <style> outright.
Worth pairing with a rewrite_creative_html test asserting the output contains &tstoken= and not &tstoken= — no current test covers the <style> path's query separators, which is why CI is green on this.
- 📝 Output growth bound (@ChristianPavilonis's second P2) not assessed here — I did not evaluate it, so nothing in this review should be read as clearing it. It is plausible on its face: every rewritten reference expands a short URL into a longer signed one, so a stylesheet that passes the 10 MiB input cap can exceed it after rewriting. Worth a reply from the author either way.
CI Status
All checks pass on 60b5e992. Locally against the same commit: cargo fmt --all -- --check, clippy-fastly, test-fastly (2302 passed), test-axum, check-cloudflare, check-spin, and the cross-adapter parity suite (13 passed) are all green. The regression below is not caught by any existing test, which is part of the finding.
- Analyze (actions): 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
| element!("[style]", |el| { | ||
| if let Some(st) = el.get_attribute("style") { | ||
| let rewritten = rewrite_style_urls(settings, &st, base_origin); | ||
| let rewritten = rewrite_style_attribute_urls(settings, &st, base_origin); |
There was a problem hiding this comment.
🔧 wrench — Newly covered <style> forms emit &tstoken= and are rejected by the proxy.
(Anchored here because the line this is about — the <style> handler's t.replace(&rewritten, ContentType::Text) at creative.rs:1263 — sits outside this diff's hunks and GitHub rejects a comment there with "line could not be resolved". This line is its sibling: the attribute handler that calls the same rewriter, and the nearest resolvable anchor.)
ContentType::Text entity-escapes &, but a <style> element is raw text — the HTML parser does not decode entities inside it, so the CSS parser sees &tstoken= literally. The browser requests …&tstoken=…, /first-party/proxy sees a parameter named amp;tstoken and no tstoken, and rejects it.
Confirming @ChristianPavilonis's P2 with a reproduction, and correcting my own earlier read. I saw this escaping on my first pass and set it aside as pre-existing on main. That was right for url() — already rewritten there, already broken this way — but wrong as a reason to drop it. image-set(), src() and bare @import strings were not rewritten before this PR, so they reached the browser direct and loaded. Now they are rewritten into URLs the proxy rejects:
<style> content at 60b5e992 |
emits |
|---|---|
image-set("https://cdn.example/a.png" 1x) |
&tstoken= — asset fails to load |
src("https://cdn.example/f.woff2") |
&tstoken= — font fails to load |
@import "https://cdn.example/x.css"; |
&tstoken= — stylesheet fails to load |
Those three regress from working to broken, which is what makes this the PR's to fix rather than inherited.
ContentType::Html inserts the replacement without entity-escaping, the correct content model for a raw-text element. At creative.rs:1263:
text!("style", |t| {
let s = t.as_str();
let rewritten = rewrite_style_urls(settings, s, base_origin);
if rewritten != s {
t.replace(&rewritten, ContentType::Html);
}
Ok(())
}),Verified in a scratch worktree at this head: with that one-line change all four forms emit a raw &tstoken=, none emit &tstoken=, and cargo test-fastly still passes (2302 tests, 0 failures).
On the injection question ContentType::Html naturally raises — it does not open one here. A </style> inside a CSS string terminates the element identically before and after, because the HTML tokenizer ends raw text at that literal however the replacement was inserted; I confirmed the output is byte-identical in a case the rewriter leaves untouched. Guarding creative markup against that is sanitize_creative_html's job, and it strips <style> outright.
Worth pairing with a rewrite_creative_html test asserting the output contains &tstoken= and not &tstoken= — no current test covers this path's query separators, which is why CI is green on it.
Apply manually — can't be offered as a one-click suggestion, since the line it would replace is the unresolvable 1263 rather than this anchor.
3 participants
Summary
url()rewriter decided the extent of a value by scanning for quote and paren positions. That is not how a browser reads a declaration, so some values were rewritten in ways a browser would not, and others were left unrewritten — most visibly a quoted value containing), which the scanner truncated at the first)afterurl(, leaving the intended URL unproxied.cssparser), so the extent of a value and the resolution of its escapes come from the same rules a browser applies. The walk is property-agnostic: it rewrites URL references wherever they appear rather than from an enumerated list of URL-bearing properties, and it leaves fragment-only references such asfilter: url(#blur)alone.split_srcset_candidatesre-derived per-candidate facts from the whole candidate prefix at each comma. Those facts belong to the candidate, so they are now tracked as the scan advances.Changes
Cargo.toml,crates/trusted-server-core/Cargo.tomlcssparseras a direct dependency. It is already in the lockfile vialol_html, so no new package is vendoredcrates/trusted-server-core/src/creative.rsrewrite_style_urls: replace positional quote scanning with acssparsergrammar walk. Escapes are already resolved when the value is read, and a malformed value — which the tokenizer reports as a bad URL or bad string, exactly what a browser discards — is left untouched rather than guessed atcrates/trusted-server-core/src/creative.rssrc()alongsideurl(), bare-string candidates inimage-set(),@importpreludes in both theurl()and bare-string forms, andvar()/env()fallbacks, which are substituted in placecrates/trusted-server-core/src/creative.rsurl()asurl(),src()assrc(), a bare string as a bare string — because the forms are not interchangeable to a browser. Only the value inside is replaced, and it is re-quoted; anything not rewritten keeps its original bytescrates/trusted-server-core/src/creative.rsMAX_CSS_NESTING_DEPTH: the CSS is supplied by the upstream creative, and a stack overflow aborts the guest, so the recursion the walk performs is bounded. The bound counts scopes the walk recurses into, not how a URL is spelled, sourl(x)andurl("x")are admitted at the same depth. Nesting past it discards the whole stylesheet rather than passing the deeper bytes through, which would turn the bound into a way around the rewritecrates/trusted-server-core/src/creative.rsrewrite_css_bodyreturnsResult. A stylesheet the depth bound refuses previously left the CSS proxy path serving an empty200, indistinguishable from a stylesheet the origin legitimately served empty. It now reports the refusal sofinalize_proxied_responsesets a status, matching the oversized-body path. The markup path still drops only the offending<style>block or attribute, and the log names whichcrates/trusted-server-core/src/creative.rssplit_srcset_candidates: derive candidate scheme and whitespace state as the scan advances instead of from the candidate prefix at each comma; corrected the doc note, which described behavior the function did not havecrates/trusted-server-core/src/creative.rssrc(),image-set(),@importand fallback coverage; the depth boundary in every spelling of a URL; and the multi-commadata:srcset caseBehavior
url("https://cdn.example/a)b.png")url("https://t.example/\70 ixel.gif")src("https://cdn.example/a.woff2")src()image-set("https://cdn.example/a.png" 1x)@import "https://cdn.example/x.css";image-set(var(--c, "https://cdn.example/a.png") 1x)--c:"https://cdn.example/a.png"used asimage-set(var(--c) 1x)filter:url(#blur)url('/local/a.png")url("https://cdn.example/a+ newline +b)srcset="data:...;base64,,,,, 1x, /b.png 2x"MAX_CSS_NESTING_DEPTH200Verification against production CSS
Ran
rewrite_css_bodyfrommainand from this branch over an identical corpus of 51 real stylesheets (1.75 MB): 20 captured from the configured publisher origin, plus 31 widely-used public stylesheets fromfonts.googleapis.com,cdnjs.cloudflare.comandcdn.jsdelivr.net. The public ones are there because the publisher's own bundles carry almost nourl()at all, so a corpus limited to them cannot exercise the rewrite at all — which is what the earlier, smaller run of this check ended up showing.mainvs branchThe 5 differing files differ only in that this branch emits
url("…")wheremainemittedurl(…); the value inside is identical, and quoting is the documented normalization. Across the 6 files that contain absolute references, both versions proxy the same 183 targets — none newly rewritten, none newly missed. That includes a real@import url("https://…")and 165 unquoted absoluteurl()references.Corpus coverage, for what this does and does not attest to: 8,258
var(, 165 unquoted absoluteurl(, 63 quotedurl(", 2url(', 2 protocol-relativeurl(//, 1@import. No stylesheet in the sample usedimage-set()orsrc(), so those paths rest on unit tests rather than on this differential.Closes
Closes #1114
Test plan
cargo test-fastly(2302 passed),cargo test-axum,cargo test-cloudflare,cargo test-spincargo clippy-fastly && cargo clippy-axum && cargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasmcargo fmt --all -- --checkmainand branch over a 51-file production CSS corpus (above)Checklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)