css: fix exponential backtracking on nested color function values in token lists - #31919
Conversation
|
Ready to act? Review this PR in Change Stack to turn feedback into patch suggestions you can inspect and refine. Warning Review limit reached
More reviews will be available in 3 minutes and 17 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR prevents exponential backtracking in CSS color token-list parsing by introducing monotonic failure tracking. The parser now records failures during nested token-list arguments, allowing functions like ChangesCSS token-list backtracking prevention
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found, but the error-propagation change in parse_into relies on the invariant that a failed token-list sub-parse can never succeed under the generic-function fallback — that's subtle enough to warrant a human sanity check before merging.
Extended reasoning...
Overview
This PR fixes a fuzzer-reported O(2^depth) backtracking DoS in the CSS custom-property parser. It adds a monotonic token_list_parse_failures counter to ParserInput (mirroring the existing math_fn_parse_failures pattern from #31558), wraps TokenList::parse_into to bump it on error, and samples it around the UnresolvedColor::parse attempt so that a failure originating inside a nested token-list argument propagates instead of falling through to a re-parse. It also adds an allocation-free top-level-comma pre-scan to light-dark() so the structural no-comma failure is detected before buffering the first half. The Token::Function arm in parse_into_impl is restructured from an else if ladder to early-continue + match. A new test file pins both the bounded-time behavior at depth 96 and byte-identical output for valid/recovered color values.
Security risks
The fix itself is defensive (closes a CPU/allocation DoS). No new attack surface — no untrusted-input handling beyond what already existed, no auth/crypto/permissions code touched.
Level of scrutiny
Medium-high. The mechanical parts (counter plumbing, pre-scan loop) are straightforward and follow an established in-tree pattern. But the semantic change — propagating an error out of parse_into_impl where it previously fell through to the generic-function fallback — depends on the claim that token-list parsing is context-free: any range of tokens that fails as an rgb/hsl alpha or light-dark half must also fail when re-parsed as plain function arguments at the same depth. I traced the failure modes (is_parse_error tokens, depth limit, nested var/env/url failures) and they do appear to fail identically under the fallback, and the test suite pins unchanged minified output for a representative set of valid and recovered inputs. Still, this is exactly the kind of invariant a maintainer who knows the parser should confirm — a missed case would silently turn previously-recovered declarations into hard errors.
Other factors
The PR description is unusually thorough (repro, root cause, verification across four test suites, before/after timings), test coverage is good (three exponential shapes + the original fuzzer input + 11 pinned-output cases + 3 still-failing cases), and the bug-hunting pass found nothing. The control-flow restructure in the Token::Function arm looks behavior-preserving on the success paths. CI is still building. Given the subtlety of the core invariant, I'm deferring rather than approving.
|
Re the invariant flagged by the review (a failed token-list sub-parse can never succeed under the generic-function fallback), the precise argument: The guard only fires when Its failure sources are:
The structural checks inside One deliberate edge: a depth-cap failure recorded at depth N propagates even though a hypothetical re-parse rooted at a shallower base could succeed. Within a single declaration value the base is fixed (both alternatives count blocks from the same value start), so this only matters across outer-grammar retries, which reset the counter sampling anyway (they sample The output-equivalence tests in token-list-backtracking.test.ts pin the recovered cases (light-dark without a comma still minifies as a plain function, etc.) and the three bad-token cases still fail with the same errors as the unfixed build. |
…token lists TokenList::parse_into tries UnresolvedColor::parse for rgb()/hsl()/ light-dark() and falls back to parsing the arguments as a plain function when the attempt fails. The attempt buffers token-list arguments (the rgb()/hsl() alpha, the light-dark() halves), so when it failed after consuming them the fallback re-parsed the same range. With such functions nested, every level re-buffered the remaining input once per alternative: O(2^depth) time and allocation churn. The unclosed-block-at-EOF short-circuit only covered truncated inputs, not balanced ones. Record token-list parse failures in a counter on ParserInput (same pattern as math_fn_parse_failures) and propagate the error instead of falling through when the counter grew during the attempt: those tokens fail identically under every alternative. light-dark() additionally checks for its top-level comma with a raw scan before buffering anything, since a missing comma is a structural failure the counter cannot see. Outputs for valid and recovered values are unchanged.
36b650f to
acec7d9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/css/token-list-backtracking.test.ts`:
- Around line 86-126: The three independent tests ("original fuzzer input parses
in bounded time and memory", "valid and recovered color function values are
unchanged", and "bad tokens inside color function arguments still fail the
declaration") call native synchronous helpers (minifyTest, _test, prefixTest)
and should be marked concurrent to speed the suite; change their declarations
from test(...) to test.concurrent(...) so Jest runs them in parallel, leaving
test body and assertions unchanged (look for the test(...) blocks with those
exact titles and the minifyTest/_test/prefixTest calls).
- Line 52: The test currently masks stderr via a conditional (stderr:
stderr.includes("error") ? stderr : ""), which allows unexpected non-"error"
output to be ignored; update the assertion in token-list-backtracking.test.ts to
assert stderr is exactly empty (replace that conditional with stderr: "") so the
test fails on any subprocess stderr output (since bunEnv sets
BUN_DEBUG_QUIET_LOGS=1), and keep using the existing stdout and exitCode
variables in the expect object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f6fe14e-7dab-4539-a7b7-348152ccf24b
📒 Files selected for processing (3)
src/css/css_parser.rssrc/css/properties/custom.rstest/js/bun/css/token-list-backtracking.test.ts
There was a problem hiding this comment.
I didn't find any bugs — the counter pattern mirrors math_fn_parse_failures and the comma pre-scan correctly skips nested blocks via at_start_of — but the correctness of propagating instead of recovering rests on the parse_into-determinism invariant you laid out, which is subtle enough that a human should sign off on it.
Extended reasoning...
Overview
This PR fixes a fuzzer-reported O(2^depth) backtracking blowup in the CSS token-list parser. It touches three files: src/css/css_parser.rs adds a token_list_parse_failures: u64 counter to ParserInput (mirroring the existing math_fn_parse_failures from #31558); src/css/properties/custom.rs wraps TokenList::parse_into to bump the counter on error, samples it around the UnresolvedColor::parse attempt to propagate (rather than retry) failures that originated inside a nested token-list argument, and adds a non-allocating top-level-comma pre-scan to light-dark(); test/js/bun/css/token-list-backtracking.test.ts adds bounded-time/memory regression tests, output-equivalence pins, and bad-token error-path checks.
Security risks
None introduced. The change removes a CPU/memory DoS vector (a few KB of nested light-dark( / rgb(1 1 1/ triggering exponential re-buffering). No new input surface, no auth/crypto/permissions.
Level of scrutiny
Medium-high. The mechanical pieces are clean and follow established repo patterns: the counter infrastructure is a copy of math_fn_parse_failures, the comma pre-scan correctly relies on next_including_whitespace_and_comments setting at_start_of so nested blocks are skipped, and reset() restores at_start_of without touching the monotonic counter (which is exactly what the sampling needs). The part that warrants human eyes is the semantic change: propagating an error from inside the UnresolvedColor attempt instead of falling through to the generic-function recovery path. This is sound iff TokenList::parse_into is deterministic for (position, depth) — the author gives a detailed argument (error-token positions are tokenizer-fixed; depth is path-independent block-distance; var/env/url arms use ? with no recovery) and explicitly calls out the one edge (depth-cap at >500 levels). I find the argument convincing, but it's a subtle invariant about error-recovery equivalence in a parser whose output feeds the bundler, and the author themselves found a separate amplification (#31918) while testing this area.
Other factors
All coderabbit comments are resolved (concurrent-test nit applied in 9e00b68; stderr-conditional rebutted with merged precedent from #31642). The CI failures in build #60978 are pre-existing infrastructure issues (musl LTO linking, Windows agent creation, an unrelated bunx.test.ts) and not caused by this change. Test coverage is good: the three exponential shapes, the original fuzzer input, eleven output-equivalence pins, and three bad-token error checks. No CODEOWNERS for src/css/.
|
CI status summary for reviewers: The remaining red lanes (alpine/debian/ubuntu/windows test lanes, exit status 2) are not produced by this change. The same lane set is failing on concurrent builds of unrelated PRs (#31916, #31922, and earlier #31911/#31920), while main's last completed build is green. The lanes that exercise this diff most directly are green:
The branch is rebased on a7839df (green main). The diff touches only src/css/css_parser.rs, src/css/properties/custom.rs, and the new test file, so the cross-platform test-lane failures (which include non-css suites) have no path into this change. Ready for review/merge from my side. |
2 participants
Fixes a fuzzer-reported OOM/DoS class in the CSS parser.
Fuzzer signature:
oom:csswith allocation stacks in<alloc::vec::Vec<bun_css::properties::custom::TokenOrValue> as core::ops::drop::Drop>::drop, diagnosed as error recovery re-buffering rawTokenOrValuelists once per nesting level.Repro
Same blowup with
"rgb(1 1 1/ ".repeat(D)and a bad-string token or an invalidvar()at the bottom. Time and allocation churn multiply by ~2 per added nesting level (measured x16 per +4 levels). Under allocator instrumentation the churn shows up as memory, matching the fuzzer's OOM; in release builds it is a CPU DoS from a few KB of CSS.Cause
TokenList::parse_into(src/css/properties/custom.rs) triesUnresolvedColor::parseforrgb()/hsl()/light-dark()and, when that attempt fails, rewinds and re-parses the arguments as a plain function. The attempt buffers token-list arguments (the rgb/hsl alpha, the light-dark halves), so a late failure makes the fallback re-parse the whole consumed range. When such functions nest, each level runs both the attempt and the fallback over the same suffix, so the work is O(2^depth) in total.#31243 added the unclosed-block-at-EOF short-circuit, which kills this only when the input is truncated mid-block. Balanced inputs (closed parens) with a late failure, or a tokenizer error token mid-stream, never hit it.
Fix
Two parts, in src/css/properties/custom.rs and src/css/css_parser.rs:
ParserInputgains atoken_list_parse_failurescounter (same pattern as the existingmath_fn_parse_failuresused by the atan2 fix, css: fix exponential backtracking in atan2() color parsing #31558), bumped wheneverTokenList::parse_intofails. TheUnresolvedColorcall site samples it around the attempt; if it grew, the failure came from inside a token-list argument, which fails identically under every later alternative, so the error is propagated instead of falling through to a re-parse. A token-list parse is context-free, so re-parsing the same tokens cannot succeed.light-dark()fails on a missing top-level comma only after buffering its first half; that is a structural failure the counter cannot see. It now checks for the comma with a raw scan (no allocation, nested blocks skipped) before parsing, so the failed attempt costs no buffering and the fallback's single pass is the only full parse.Recovery is now a single descent: parse failures propagate immediately, and the only repeated work is the light-dark comma scan, which is allocation-free and bounded by the 500-level nesting cap.
Verification
minifyTest,_test, andprefixTest. Note it did not reproduce a standalone OOM on the reported canary build (1.4.0-canary.1+5ac120ca3, built and tested from that exact commit); the class it was diagnosed as is what the synthetic shapes reproduce.test/js/bun/css/css.test.ts(1093),test/js/bun/css/(only pre-existingcss-fuzz.test.tstimeouts, identical on the unfixed build in the same container),test/bundler/css/(167),test/bundler/esbuild/css.test.ts(53) all pass.test/js/bun/css/token-list-backtracking.test.tsfail on the unfixed build (3 timeouts) and pass with the fix.While testing this I found a separate success-path amplification: nested relative colors with
light-dark()origins produce O(2^depth) output. Not touched here; tracked in #31918.