diff --git a/Cargo.lock b/Cargo.lock index 8da486f8c..018b5189c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5478,6 +5478,7 @@ dependencies = [ "config", "cookie", "criterion", + "cssparser 0.36.0", "derive_more", "ed25519-dalek", "edgezero-core", diff --git a/Cargo.toml b/Cargo.toml index 7faba7553..d6ecdb9ca 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -51,6 +51,7 @@ clap = { version = "4", features = ["derive"] } config = "0.15.19" cookie = "0.18.1" criterion = { version = "0.5", default-features = false, features = ["cargo_bench_support"] } +cssparser = "0.36" derive_more = { version = "2.0", features = ["display", "error"] } directories = "5" ed25519-dalek = { version = "2.2", features = ["rand_core"] } diff --git a/crates/trusted-server-core/Cargo.toml b/crates/trusted-server-core/Cargo.toml index e44d46f77..01780dd39 100644 --- a/crates/trusted-server-core/Cargo.toml +++ b/crates/trusted-server-core/Cargo.toml @@ -19,6 +19,7 @@ bytes = { workspace = true } chacha20poly1305 = { workspace = true } chrono = { workspace = true } cookie = { workspace = true } +cssparser = { workspace = true } derive_more = { workspace = true } ed25519-dalek = { workspace = true } edgezero-core = { workspace = true } diff --git a/crates/trusted-server-core/README.md b/crates/trusted-server-core/README.md index 3049a1115..69575b0d2 100644 --- a/crates/trusted-server-core/README.md +++ b/crates/trusted-server-core/README.md @@ -34,7 +34,7 @@ Additional behavior: Helpers: - `rewrite_creative_html(settings, markup) -> String` — rewrite an HTML fragment -- `rewrite_css_body(settings, css) -> String` — rewrite a CSS body (`url(...)` entries) +- `rewrite_css_body(settings, css) -> Result` — rewrite a CSS body (`url(...)` entries) - `rewrite_srcset(settings, srcset) -> String` — proxy absolute candidates; preserve descriptors (`1x`, `1.5x`, `100w`) - `split_srcset_candidates(srcset) -> Vec<&str>` — robust splitting for commas with/without spaces; avoids splitting the first `data:` mediatype comma diff --git a/crates/trusted-server-core/src/creative.rs b/crates/trusted-server-core/src/creative.rs index a4d641bdf..578d86d68 100644 --- a/crates/trusted-server-core/src/creative.rs +++ b/crates/trusted-server-core/src/creative.rs @@ -31,8 +31,8 @@ //! - `split_srcset_candidates(&str) -> Vec<&str>`: Robust splitting that supports //! commas with or without spaces and avoids splitting the mediatype/data comma //! in a leading `data:` URL. -//! - `rewrite_css_body(&Settings, &str) -> String`: Rewrites url(...) occurrences -//! inside CSS bodies. +//! - `rewrite_css_body(&Settings, &str) -> Result`: +//! Rewrites url(...) occurrences inside CSS bodies. //! //! See the tests in this module for comprehensive cases, including irregular //! spacing, no-space commas, and `data:` handling. @@ -42,6 +42,7 @@ use crate::settings::Settings; use crate::streaming_processor::StreamProcessor; use crate::tsjs; use lol_html::{HtmlRewriter, Settings as HtmlSettings, element, html_content::ContentType, text}; +use std::fmt::{self, Write as _}; use std::io; /// Maximum size of response body that can be buffered for rewriting. @@ -73,64 +74,480 @@ pub(super) fn to_abs(settings: &Settings, u: &str) -> Option { Some(absolute) } -// Helper: rewrite url(...) occurrences inside a CSS style string to first-party proxy. -// `base_origin` is prefixed onto the proxy path — empty for root-relative output, -// `https://` for absolute output (see [`build_proxy_url`]). +/// Maximum number of nested parser scopes [`rewrite_style_urls`] will enter. +/// +/// The walk recurses into blocks, functions and `@import` preludes, and the CSS +/// it reads is supplied by the upstream creative, so an input of nothing but +/// `{` would otherwise decide how deep the stack goes — and an overflow aborts +/// the guest, turning a 2 KB style attribute into a failed response. Measured on +/// `wasm32-wasip1`, the walk survives 1,000 nested blocks and overflows by +/// 1,100, so this leaves room for a stack an order of magnitude smaller than +/// the one measured. It is still far above any real stylesheet, where nesting +/// is a declaration list inside a handful of blocks. Entering this many +/// scopes costs one more stack frame than the bound, since the outermost walk +/// has entered none. +/// +/// An iterative walk would need no bound, but the tokenizer only exposes a +/// block through a closure, so each nested slice would be re-read from its +/// start — quadratic in the input, which [`MAX_REWRITABLE_BODY_SIZE`] allows +/// to be 10 MB. Recursing stays linear and bounds the stack instead. +/// +/// A scope is anything whose contents the walk reads by recursing: a block, a +/// function that may hold a value of its own (`image-set()`, `var()`), an +/// `@import` prelude. Reading the single string argument of a `url()` or +/// `src()` is not one — that grammar is terminal, so it costs no recursion and +/// is not charged a level. Without that exemption the bound would depend on +/// how a URL is spelled, admitting `url(https://…)` where it rejects the +/// identical `url("https://…")`, and rejection discards the whole stylesheet. +const MAX_CSS_NESTING_DEPTH: usize = 64; + +/// 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. +/// +/// `base_origin` is prefixed onto the proxy path — empty for root-relative +/// output, `https://` for absolute output (see [`build_proxy_url`]). +/// +/// Values are read with a CSS tokenizer rather than by scanning for quotes, so +/// the extent of a value comes from the grammar and escapes are already +/// resolved. That matters in both directions: a value whose escapes hide an +/// absolute URL (`url("https://t.example/\70 ixel.gif")`) is still proxied, 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 at. +/// +/// A rewritten reference keeps the shape it was read in — `url()` as `url()`, +/// `src()` as `src()`, 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, so the output is normalized in that respect rather than +/// byte-preserved. Anything not rewritten keeps its original bytes. +/// +/// A URL that only exists after custom-property substitution is out of reach: +/// `--c:"https://t.example/a.png"` used as `image-set(var(--c) 1x)` is a URL to +/// the browser, but the string and its use are separate declarations and +/// resolving one against the other is the cascade's job, not a rewriter's. The +/// inline fallback form, `image-set(var(--c, "https://t.example/a.png") 1x)`, +/// is substituted in place and is rewritten. A `url()` token in a custom +/// property is also rewritten, since it is a URL wherever it lands. +/// +/// CSS nested past [`MAX_CSS_NESTING_DEPTH`] is rejected outright (empty string +/// returned), matching [`MAX_CREATIVE_SIZE`]. The alternative — keeping the +/// rewrite of everything above the cap and passing the deeper bytes through — +/// turns the bound into a way around the rewrite: a `url()` placed below the +/// cap is never inspected and reaches the browser untouched, which is the leak +/// this exists to close. Rejecting costs the styling of CSS no real page +/// produces; passing through would cost the guarantee. +/// +/// This entry point serves markup, where the stylesheet is one part of a +/// document the rest of which is still rewritten, so a rejection drops that +/// part and is reported in the log. A whole CSS response has no such +/// remainder — see [`rewrite_css_body`], which reports the rejection to its +/// caller so the response carries it. pub(super) fn rewrite_style_urls(settings: &Settings, style: &str, base_origin: &str) -> String { - // naive url(...) rewrite for absolute/protocol-relative URLs - let lower = style.to_ascii_lowercase(); - let mut out = String::with_capacity(style.len() + 16); - let mut write_pos = 0_usize; - let mut scan = 0_usize; - while let Some(off) = lower[scan..].find("url(") { - let start = scan + off; - let open = start + 4; // after 'url(' - // write prefix including 'url(' - out.push_str(&style[write_pos..open]); - // find closing ')' - let close = if let Some(c) = lower[open..].find(')') { - open + c - } else { - out.push_str(&style[open..]); - return out; - }; - // trim spaces and quotes - let bytes = style.as_bytes(); - let mut s = open; - while s < close && bytes[s].is_ascii_whitespace() { - s += 1; + drop_if_rejected( + rewrite_style_urls_in_context(settings, style, base_origin, true, MAX_REWRITABLE_BODY_SIZE), + " - "; + let html = r#"
ad
"#; + let out = rewrite_creative_html(&settings, html); + assert!( - out.matches("/first-party/proxy?tsurl=").count() >= 2, - "style block url() not rewritten: {out}" + !out.contains("/first-party/proxy?tsurl="), + "should not treat @import inside an inline declaration as an at-rule: {out}" + ); + assert!( + out.contains("https://tracker.example/text"), + "should preserve the custom-property string: {out}" + ); + } + + #[test] + fn css_output_checks_each_serialized_fragment_before_appending() { + let mut out = super::CssOutput { + value: String::new(), + limit: 8, + }; + assert!( + std::fmt::Write::write_str(&mut out, "123456").is_ok(), + "should fit a prefix" + ); + assert!( + cssparser::serialize_string("x", &mut out).is_err(), + "should stop while serializing a quoted value" + ); + assert_eq!( + out.value, "123456\"x", + "should never append the byte beyond the limit" + ); + assert!( + out.value.capacity() <= out.limit, + "should bound reserved capacity too" + ); + assert!( + std::fmt::Write::write_str(&mut out, "").is_ok(), + "should permit an empty append at the limit" ); } + #[test] + fn css_output_budget_includes_unchanged_trailing_text() { + let limit = 256; + let settings = crate::test_support::tests::create_test_settings(); + let reference = "@import \"https://cdn.example.com/a.css\";"; + super::rewrite_style_urls_in_context(&settings, reference, "", true, limit) + .expect("should fit the rewritten reference before appending the tail"); + let css = format!("{reference}{}", " ".repeat(limit - reference.len())); + assert!( + matches!( + super::rewrite_style_urls_in_context(&settings, &css, "", true, limit), + Err(super::CssRewriteError::OutputTooLarge) + ), + "should reject the expanded output including the unchanged tail" + ); + } + + #[test] + fn css_processor_reports_output_expansion_and_accepts_the_exact_limit() { + let settings = crate::test_support::tests::create_test_settings(); + let plain = " ".repeat(super::MAX_REWRITABLE_BODY_SIZE); + assert_eq!( + super::rewrite_css_body(&settings, &plain) + .expect("should accept the exact limit") + .len(), + plain.len(), + "should preserve unchanged CSS" + ); + let reference = "@import \"https://cdn.example.com/a.css\";"; + let css = format!("{reference}{}", &plain[..plain.len() - reference.len()]); + let mut processor = super::CreativeCssProcessor::new(&settings); + assert!( + super::StreamProcessor::process_chunk(&mut processor, css.as_bytes(), false).is_ok(), + "should accept input within the buffer limit" + ); + assert!( + super::StreamProcessor::process_chunk(&mut processor, &[], true).is_err(), + "should propagate the output refusal" + ); + } + + #[test] + fn inline_css_output_uses_the_same_budget() { + let limit = 256; + let settings = crate::test_support::tests::create_test_settings(); + let reference = "background:image-set(\"https://cdn.example.com/a.png\" 1x);"; + let css = format!("{reference}{}", " ".repeat(limit - reference.len())); + for (allows_import_rules, context) in [(true, "

kept

"; + assert!( + rewrite_creative_html(&settings, html).contains("

kept

"), + "should preserve surrounding markup" + ); + } + + #[test] + fn css_output_expansion_is_rejected_before_the_body_limit() { + let limit = 256; + let settings = crate::test_support::tests::create_test_settings(); + let reference = "@import \"https://cdn.example.com/a.css\";"; + let css = format!("{}{reference}", " ".repeat(limit - reference.len())); + assert!( + super::rewrite_style_urls_in_context(&settings, &css, "", true, limit).is_err(), + "should reject URL expansion beyond the output budget" + ); + } + + #[test] + fn rewrites_style_block_url_variants() { + let settings = crate::test_support::tests::create_test_settings(); + for css in [ + "a{background:url(https://cdn.example.com/a.png)}", + "a{background:image-set(\"https://cdn.example.com/a.png\" 1x)}", + "@font-face{src:src(\"https://cdn.example.com/a.woff2\")}", + "@import \"https://cdn.example.com/a.css\";", + ] { + let out = rewrite_creative_html(&settings, &format!("")); + assert!( + out.contains("/first-party/proxy?tsurl="), + "should rewrite {css}: {out}" + ); + assert!( + out.contains("&tstoken="), + "should preserve signed query separators: {out}" + ); + assert!( + !out.contains("&tstoken="), + "should not escape raw CSS text: {out}" + ); + } + } + #[test] fn rewrite_srcset_w_and_x_descriptors() { let settings = crate::test_support::tests::create_test_settings(); @@ -1589,6 +3030,25 @@ mod tests { assert!(items[1].trim().starts_with("//cdn.example/b.png 2x")); } + #[test] + fn split_srcset_keeps_consecutive_data_url_commas_in_one_candidate() { + let s = "data:text/plain;charset=utf-8,a,b,c 1x, /local/b.png 2x"; + let items = super::split_srcset_candidates(s); + assert_eq!(items.len(), 2, "{items:?}"); + assert_eq!(items[0], "data:text/plain;charset=utf-8,a,b,c 1x"); + } + + #[test] + fn split_srcset_handles_long_data_url_comma_run() { + let mut s = String::from("data:image/png;base64,"); + s.push_str(&",".repeat(100_000)); + s.push_str(" 1x, https://cdn.example/b.png 2x"); + let items = super::split_srcset_candidates(&s); + assert_eq!(items.len(), 2, "{items:?}"); + assert!(items[0].starts_with("data:image/png;base64,")); + assert!(items[1].trim().starts_with("https://cdn.example/b.png")); + } + #[test] fn link_rel_case_and_multi_values_rewritten() { let settings = crate::test_support::tests::create_test_settings(); @@ -1639,7 +3099,7 @@ mod tests { fn rewrite_css_body_direct_smoke() { let settings = crate::test_support::tests::create_test_settings(); let css = ".x{background:url(https://cdn.example/a.png)} .y{mask:url('//cdn.example/b.svg')} .z{background:url(/local.png)}"; - let out = super::rewrite_css_body(&settings, css); + let out = super::rewrite_css_body(&settings, css).expect("should rewrite ordinary CSS"); assert!( out.matches("/first-party/proxy?tsurl=").count() >= 2, "{}", @@ -1648,6 +3108,25 @@ mod tests { assert!(out.contains("url(/local.png)")); } + #[test] + fn css_processor_reports_a_stylesheet_nested_past_the_supported_depth() { + let settings = crate::test_support::tests::create_test_settings(); + // An empty `200` would be indistinguishable from a stylesheet the + // origin served empty, so the rejection has to reach the caller that + // sets the status. + let css = "{".repeat(super::MAX_CSS_NESTING_DEPTH * 40); + let mut processor = CreativeCssProcessor::new(&settings); + + let error = processor + .process_chunk(css.as_bytes(), true) + .expect_err("should report the rejection rather than serve an empty body"); + + assert!( + error.to_string().contains("nested past"), + "should say why the stylesheet was rejected: {error}" + ); + } + #[test] fn rewrites_anchor_click_to_first_party() { let settings = crate::test_support::tests::create_test_settings();