Repository navigation
css: reject out-of-range percentages in color-mix() - #33953
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward spec-mandated range check on color-mix() percentages.
What was reviewed:
- Verified
expect_percentage()returns a unit fraction, so the0.0..=1.0range matches the spec's<percentage [0,100]>. - Checked placement: validation runs before normalization and leaves the existing
p1 + p2 == 0.0check intact; boundary values 0%/100% are covered by tests. - Test matrix covers both operands, percent-before/after-color, multiple color spaces, and the fuzz repro in a subprocess.
Extended reasoning...
Overview
Adds a 6-line range check in parse_color_mix (src/css/values/color.rs) rejecting mix percentages outside [0%, 100%], plus a test block in test/js/bun/css/color.test.ts covering the fuzz repro, invalid inputs across color spaces/operand positions, and boundary acceptance.
Security risks
None. This is CSS color parsing with no I/O, auth, or memory-management surface; the change only tightens input validation to reject previously-accepted garbage per spec.
Level of scrutiny
Low. The fix is a pure input-validation guard with a clear spec citation (css-color-5 §3, WPT color-invalid-color-mix-function.html, WebKit's CSS::Percentage<Range{0,100}>). I confirmed expect_percentage() returns unit_value (a fraction), so !(0.0..=1.0).contains(&p) is the correct bound. The check sits before normalization, so the existing p1 + p2 == 0.0 invalid-value path and the unwrap_or_else(|| 1.0 - other) derivation are unaffected for in-range inputs.
Other factors
Tests are thorough for the size of the change: negative, >100%, just-over-boundary (100.001%), percent before and after the color, both operands, five interpolation spaces, plus explicit 0%/100%/50% acceptance so the guard isn't over-tight. The subprocess crash repro drains stdout/stderr/exited concurrently. A finder flagged the stderr: "" equality in that subprocess test; verifiers ruled it out (bunEnv quiets debug logs and the -e expression is trivial). No prior reviews or outstanding comments.
The color-mix() grammar is <percentage [0,100]>; values outside that range are parse errors. parse_color_mix accepted them anyway, which could drive HSL saturation negative and trip a debug assertion in hsl_to_rgb. Release builds skipped the assert and emitted garbage colors. Found by fuzzing Bun.color().
b62c9d1 to
64edcaf
Compare
There was a problem hiding this comment.
LGTM — the inline nit about stderr: "" is non-blocking.
Extended reasoning...
Overview
7 lines added to parse_color_mix in src/css/values/color.rs to reject mix percentages outside [0%, 100%] before normalization, plus ~46 lines of tests in test/js/bun/css/color.test.ts. The fix is a straightforward range check using is_some_and(|p| !(0.0..=1.0).contains(&p)) on the two optional parsed percentages, returning the same ParserError::invalid_value already used a few lines below for the both-zero case.
Security risks
None. This is input validation on a CSS color string that tightens what is accepted (rejects more, accepts nothing new). No allocation, no unsafe, no FFI, no user-controllable size arithmetic.
Level of scrutiny
Low. The change is small, self-contained, and spec-cited (css-color-5 §3 grammar <percentage [0,100]>, WPT color-invalid-color-mix-function.html, WebKit CSS::Percentage<Range{0,100}>). It sits in the CSS color parser used by Bun.color() and the bundler's CSS pipeline — not a hot path, not memory-safety-sensitive, and the failure mode of a bug here is Bun.color() returning null for a valid input, which the boundary tests (0%, 100%, 50%/50%) guard against.
Other factors
- The check is placed correctly: after both percentages are parsed (so the
None/ omitted case passes through viais_some_and), before normalization (so the raw grammar constraint is enforced, not the derived weight). NaN would also be rejected sinceRangeInclusive::containsreturns false for NaN — harmless here. - Error construction matches the existing file-local pattern (
input.new_custom_error(css::ParserError::invalid_value)) used for thep1 + p2 == 0.0case just below. - Test coverage is thorough: subprocess repro of the original fuzz crash, negative and >100% on both operands in both prefix/postfix positions across five color spaces, plus boundary acceptance tests confirming 0% and 100% still parse. The PR evidence shows the tests fail on main (debug panics, release returns garbage) and pass with the fix on both ASAN debug and release.
- The one finding (asserting
stderr: ""in the subprocess test) is a test-robustness nit against future ASAN noise, not a correctness issue; the ASAN lane already passed this run.
Match the file-local convention (autoAnsi helper): include stderr in the failure diff without requiring it to be empty on success.
There was a problem hiding this comment.
LGTM — the stderr nit from the earlier pass is addressed in 5acf294.
What was reviewed:
- Range check placement in
parse_color_mix: runs after both percentages are parsed and before normalization;is_some_andleaves the omitted-percentage path untouched, and(0.0..=1.0).containscorrectly rejects NaN as well. - Boundary behavior: 0% and 100% remain accepted (inclusive range), matching the css-color-5 grammar and the WPT invalid-color-mix cases cited.
- Tests: reject matrix covers negative / >100% / before-and-after-color / both operands / multiple color spaces; subprocess repro now follows the file-local
autoAnsistderr convention.
Extended reasoning...
Overview
Two files touched. src/css/values/color.rs gains a 4-line guard in parse_color_mix that rejects mix percentages outside [0.0, 1.0] before the normalization step, returning ParserError::invalid_value — the same error already used a few lines below for the p1 + p2 == 0 case. test/js/bun/css/color.test.ts gains a describe block with a subprocess crash-repro test, an 11-case reject matrix, and a 5-case accept matrix for boundary values.
Security risks
None. This is input validation on a CSS color string in a pure parsing path — no I/O, no allocation driven by the untrusted value, no auth or crypto surface. The change strictly tightens accepted input to match the spec grammar, which if anything reduces the reachable state space (previously a negative percentage could drive interpolation into a debug-assert panic).
Level of scrutiny
Low-to-moderate. The Rust change is a single self-contained conditional using an established error-return pattern already present in the same function. It is spec-cited (css-color-5 §3, WPT color-invalid-color-mix-function.html, WebKit's CSS::Percentage<Range{0,100}>). is_some_and correctly skips the check when a percentage was omitted, so the 1.0 - other inference path is unaffected. RangeInclusive::contains on f32 is false for NaN, so a hypothetical NaN percentage would also be rejected rather than passed to interpolation.
Other factors
The PR evidence shows the new tests fail on both debug (assertion panic) and release (garbage output) builds without the fix, and pass on both with it — satisfying the fails-for-the-right-reason requirement. The one review comment I left previously (asserting empty stderr in the subprocess test) was applied in 5acf294 and the thread is resolved; the current diff matches the file's existing autoAnsi convention. No CODEOWNERS on this path, no outstanding human review comments, and the bug-hunting system found no issues.
|
CI note: the only hard failure on builds 71745 and 71815 is |
2 participants
What
parse_color_mixaccepted mix percentages outside[0%, 100%]. With a negative percentage the interpolated HSL saturation goes negative and trips adebug_assert!(saturation >= 0.0 && saturation <= 1.0)inhsl_to_rgb. Release builds skip the assert and emit garbage colors.Repro
bun -e 'Bun.color("color-mix(in hsl,red -9%,color(display-p3 0 0 0)", "lab")'Debug build panics:
Release build returns a bogus value instead of
null:Fix
Per css-color-5 §3 the grammar is
[ <color> && <percentage [0,100]>? ]; values outside that range are parse errors. WPTcolor-invalid-color-mix-function.htmltests exactly this ("Percentages less than 0 are not valid", "Percentages greater than 100 are not valid") and WebKit enforces it viaCSS::Percentage<Range{0, 100}>.Added a range check on each parsed percentage in
parse_color_mixbefore normalization.Tests
Added to
test/js/bun/css/color.test.ts:nulloutput)null0%/100%still parseFound by fuzzing
Bun.color().[review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 2
evidence per changed file