Repository navigation
fix: CodeQL alerts (bounded regexes, null-prototype export groups, exact site URL match) - #232
Merged
Merged
Conversation
…TCG groups The patterns that read a number (lengths, line heights, DESIGN.md dimensions, bare HSL and RGB channels) allowed a run of digits to be split between `\d+` and `\d*` in every way, so a failing match on a long value took quadratic time: 25 s for 100,000 digits. They now read the same strings with one way to match. The composite typography pattern and the var() fallback dropped a `\s*` that overlapped the part after it. toDtcg builds its groups with Object.create(null), so a token path with a `__proto__` part is an ordinary key and cannot reach Object.prototype. The site's resolveDocHref turns only the site's own URL, or a path under it, into a site path, not a host that merely starts with it.
… stay linear CodeQL's new alert on this PR: with `[^)]+`, each `var(` in an unclosed run such as `var(---,var(---,(...` scanned to the end of the value for its `)`. 20,000 repeats took 7.4 s; with `[^()]+` they take 0.4 ms. A fallback that holds parentheses had no px value before either, and still has none.
The previous commit's `[^()]+` fallback kept unclosed runs linear but lost values main read inside calc(): calc(2 * var(--a, var(--b))) exported as 16px on main and as the raw string with it. expandVars now scans each var() to its matching parenthesis, so a fallback may hold calc() or another var(), and an unclosed var() ends the scan, so no later `var(` is scanned to the end again. A differential fuzz against a CSS-correct reference (300,009 expressions) finds no value it gets wrong, none main got right that it loses, 54,619 it now reads that main did not, and 9,705 main read wrong that it now reads right. The attack input (`var(---,` + `var(---,(` x 20,000) takes 0.5 ms.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the open CodeQL alerts in the token readers, the token export and the site's docs links. Each regex matches the same strings as before, with the same capture groups. The only difference is that a failing input now has one way to match, not thousands.
Alerts
The numbers are code scanning alert numbers (Security > Code scanning), not issues or pull requests.
ReDoS. Times are for the attack input, before and after. Before means the old regex alone, measured at 10,000 characters where 100,000 did not finish; the growth is quadratic.
LENGTHis now/^(-?(?:\d+(?:\.\d*)?|\.\d+))(px|rem|em)?$/i, so a run of digits can only be split one way.lengthToPxon9x 100,000 +x: 25 s before, 0.6 ms after. End to end,checkon<div className="p-[99…9x]" />(the path from user code, the MCP tool and the Action): 1.1 s at 20,000 digits before, 5 ms after; 6 ms at 100,000 digits.var()is read by the scanner described under chore: release #41, so the\s*before the fallback that overlapped[^)]+is gone.var(---,+ spaces +x: 142 ms at 10,000 before; 0.9 ms at 100,000 after. 20,000 nestedvar(--missing,fallbacks (280,000 characters) take 18 ms: the existing depth limit of 16 bounds how often a fallback is read again.expandVarsno longer uses a regex. It scans eachvar()to its matching parenthesis, so a fallback may holdcalc()or anothervar(), and avar()that is never closed ends the scan, so no latervar(is scanned to the end again.var(---,+var(---,(x 20,000: 7.4 s with a[^)]+or,\s*([^)]+)fallback, 0.5 ms now. A differential fuzz against a CSS-correct reference (300,009 generated expressions) finds no value it gets wrong, none main got right that it loses, 54,619 it now reads that main did not (calc(2 * var(--gap, var(--space-2)))), and 9,705 that main read wrong and it now reads right (calc(var(--gap, var(--space-2)) + 1px)was missing the+ 1px). The changeset says so.BARE_HSLandBARE_RGBread each number as\d+(?:\.\d+)?|\.\d+rather than\d*\.?\d+.isBareHsl/parseColoron9x n: 141 ms at 10,000 before; 0.4 ms at 100,000 after.isBareRgbon9 9 9/+ 9s +!: 235 ms at 10,000 before; 0.6 ms at 100,000 after.PLAIN_LENGTHgets the same change asLENGTH.NUMBERis now/^-?(?:\d+(?:\.\d*)?|\.\d+)$/. On 10,000 digits it went from 144 ms to 0.07 ms.DESIGN_DIMENSIONgets the same change.COMPOSITEdrops the\s*after each:, which overlapped the[^;]+after it.\sis a subset of[^;], and the regex is only used withtest().-:+ 100,000 spaces +;!: 21.7 s before, 0.5 ms after. The exponential case,-::;-:+:;-:x 24 (126 characters), took 3.6 s in a real export before and under 1 ms after.The export test's ten 100,000-character tokens plus the 126-character one go through
toDtcgandtoDesignMdin 34 ms, measured under coverage instrumentation. Before the fix, a single such token took tens of seconds.Prototype pollution:
toDtcgcreates the tree and every group withObject.create(null). A token path with a__proto__part, such as CSS--color-__proto__or--__proto__.polluted, is now an ordinary"__proto__"key in the JSON. Before,--color-__proto__was dropped and--__proto__.pollutedsetObject.prototype.polluted. The JSON for every other token is unchanged.URL check:
resolveDocHrefmakes a link a site path only when it is exactlyhttps://onsystem.vercel.appor starts withhttps://onsystem.vercel.app/. A look-alike host such ashttps://onsystem.vercel.app.example.com/xstays as written; before, it became the broken path.example.com/x. Every docs link today starts with the site URL and/, so the site's output does not change.Not in this PR: #3 js/incomplete-sanitization in action/check.mjs was triaged as a false positive and is handled separately.
Equivalence
The old and new regexes were compared on 400,000 random strings and 400,000 grammar-generated strings (HSL and RGB channels, composites). Matches and capture groups showed 0 differences. For
var()the comparison covered the index, length, name and trimmed fallback.The new tests check that the old valid forms read the same way, in both the DTCG and the DESIGN.md output. They include
1.,.5rem,1.5EM,-30deg, 50%, .5% / .5,22,163,74 / 0.5,var( --missing , 0.5rem )and composites with spaces around:.Tests
var()fallbacks and bare channels read as before, and each 100,000-character attack input finishes in under 100 ms.__proto__token stays in its group, andObject.prototypegets nopollutedproperty.resolveDocHrefon the site URL, a path under it and a look-alike host.Patch changeset: .changeset/long-token-values.md.
Checks run
pnpm lint,pnpm format:checkandpnpm typecheckpnpm test: 1181 passed--coveragepnpm buildandpnpm --filter site generatetest(274 passed),typecheckandlint