fix(fonts): make Google Fonts subsetting CSS text-transform aware - #3577
Conversation
Extends the subset character closure to cover locale/context-sensitive case transforms and non-case CSS text-transform values: - Parse lang attributes from authored HTML and apply toLocaleUpperCase/ toLocaleLowerCase for each detected locale (covers Turkish İ/ı, Azeri, German ẞ, and other locale-dependent casing) - Map ASCII U+0021–U+007E to fullwidth equivalents U+FF01–U+FF5E when full-width appears in the source - Map small hiragana/katakana to full-size equivalents when full-size-kana appears in the source - Preserve the existing 1700-char encoded URL budget and full-font fallback Closes #3496 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Split extractGoogleFontsText into addCaseClosure, addFullwidthVariants, and addFullSizeKanaVariants. Extract subsetTextFor test helper to eliminate repeated URL→text boilerplate. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed the full deterministicFonts.ts and the whole new test file at 5a50580e, and measured the budget and locale behaviour directly rather than reading the benchmark's verdict. The locale closure is the right idea and the negative tests are well built, but two of the three additions have defects that reach the render path, and the benchmark does not support the claim the description makes from it.
Strengths
collectLangAttributes(:1226-1236) walks[lang]across the whole tree instead of reading<html lang>only, and there is a test pinning the nested case. That is the shape that actually matches authored compositions, where a single foreign-language block carries its ownlang.- The Turkish mapping is real, not assumed —
"i".toLocaleUpperCase("tr")does giveİ, so the premise of #3496 holds. - The negative test is the best thing in this PR. "does not include Turkish İ/ı without a Turkish lang attribute" proves the locale gate actually discriminates, rather than only showing the positive case passing. Most locale changes ship without that half.
addCaseClosure(:1239-1249) keeps #3492's locale-independent pass intact and layers locale variants on top, so the earlier closure's behaviour is preserved rather than replaced.
Blocker — a malformed lang attribute throws RangeError out of font injection
:1233 takes the primary subtag with lang.split("-")[0]!.toLowerCase() and :1245 hands it straight to toLocaleUpperCase(locale). That method throws on a tag that is not structurally valid BCP-47, and the split does not validate. Measured:
lang="en_US" -> RangeError: Incorrect locale information provided
lang="x" -> RangeError: Incorrect locale information provided
lang="123" -> RangeError: Incorrect locale information provided
lang="türkçe" -> RangeError: Incorrect locale information provided
lang="en-US" -> ok lang="tr" -> ok
lang="en_US" with an underscore is a routine authoring mistake, and it now hard-fails a composition that rendered fine before this PR — lang was not read at all previously, so this is new.
It is unguarded end to end. extractGoogleFontsText is called as a bare argument at :1343, and neither call site wraps it: packages/producer/src/services/htmlCompiler.ts:1918 and packages/cli/src/server/studioServer.ts:438. It is also not a FontFetchError, so the failClosedFontFetch typed-error handling cannot classify it. What the author sees is RangeError: Incorrect locale information provided raised from inside font subsetting, with nothing naming the attribute that caused it.
Filtering the tags through Intl.getCanonicalLocales() in a try/catch, or wrapping the per-locale body at :1244-1248 and skipping tags that throw, both fix it. Either way an invalid lang should cost you the locale variants, not the render.
Blocker — html.includes("full-width") over-triggers, and one spurious hit spends a third of the URL budget
:1277 gates the fullwidth expansion on a raw substring search over the entire HTML source. full-width is not a rare string: a CSS class named .full-width, a data-layout="full-width" attribute, a comment, or the words in body prose all satisfy it. The expansion then adds the whole U+FF01–U+FF5E block, and every one of those code points costs 9 characters once percent-encoded — 846 encoded characters, 49.8% of the 1700 budget, on its own.
Measured on one composition, changing nothing but a class name:
class .wide + 100 CJK glyphs -> 798/1700 encoded (46.9%)
class .full-width + 100 CJK glyphs -> 1410/1700 encoded (82.9%) +612 chars
Renaming a CSS class costs 36% of the budget, with no text-transform anywhere in the document. Roughly 32 further distinct glyphs then tip it past the cap, extractGoogleFontsText returns undefined, :1078 omits text=, and the composition silently downloads the full font.
That is the specific outcome #3496's scope said this work must not cause, and the gate is what causes it — the closure itself is fine. Requiring the transform context (matching text-transform adjacency rather than a bare substring) confines the cost to compositions that actually asked for it. :1278 has the same shape for full-size-kana, though at 22 code points it is the cheaper half.
Important — the benchmark does not show what the description says it shows
The description offers the mixed-script benchmark as evidence the budget is safe. Measured on that test's own fixture:
benchmark as written -> 1231/1700 encoded (72.4% used, 469 left)
same composition, transforms off -> 448/1700 encoded (26.4% used)
The three additions nearly triple the subset, and they do it on a fixture holding 86 characters of Latin prose and 18 CJK glyphs. A composition that small consuming 72% of the budget is evidence against the change being cheap, not for it. The assertion passes with about 52 distinct CJK glyphs of margin, which any real Japanese or Chinese composition exceeds immediately.
toBeLessThanOrEqual(1700) is also the wrong assertion for the risk. What needs pinning is the cost the transforms add — assert the delta, or assert that a realistic worst case (several locales plus all three transforms over a few hundred CJK glyphs) still fits. As written the test would stay green through exactly the regression it is named for.
Important — the locale closure runs per source character, including base64 payloads
:1272 feeds the entire HTML source through addCaseClosure, which is deliberate for the character set, but each locale now adds two more case conversions per character. On a 521 KB composition with one embedded base64 image:
0 locales -> 52 ms 1 locale -> 685 ms 3 locales -> 1344 ms
Set size went from 72 entries to 74. That is roughly 1.3 seconds of compile-path work to gain two code points.
Deduplicating before the closure fixes it without changing the result: iterate new Set(chars) instead of chars at :1272-1274. I verified the output set is byte-identical both ways, on the same 41-distinct-character sample where the as-written loop runs 129 iterations.
Nit
The diff drops the comment explaining why raw html is fed into the set at all ("intentional over-approximation: base64, scripts, and class names collapse in the Set"). That rationale is now more load-bearing than before, because it is exactly what makes the budget arithmetic above non-obvious. Worth keeping a line of it.
Scope
Audited: deterministicFonts.ts (read whole, measured the extraction against the repo's own linkedom and postcss), deterministicFonts-textSubset.test.ts (read whole). Trusting: the description's "73 existing tests / 294 assertions pass" — I did not re-run the suite; CI covers it.
Verdict
REQUEST CHANGES. The locale closure is worth landing and its tests are better than most. The two gates need work before it does: validate the lang tags so a typo cannot fail a render, and scope the full-width trigger to the transform so a class name cannot silently push a composition onto the full font. The benchmark should assert the added cost rather than a single small fixture staying under the cap.
Footnote on timing: I held this submit until the pull_request matrix at this head concluded, because a review event can cancel an in-flight matrix in the same concurrency group and nothing re-fires it — which would have left this commit with no test result at all. Worth knowing if a review ever seems slow to appear on a freshly pushed head; pushing during that window restarts the wait.
— Rames Jusso
…nput - Validate lang attributes with Intl.getCanonicalLocales before passing to toLocaleUpperCase — malformed tags (en_US, x, 123) no longer throw RangeError. - Gate fullwidth/kana expansion on text-transform declarations instead of raw html.includes — a CSS class named .full-width no longer eats half the URL budget. - Deduplicate characters before the closure loop (new Set) to avoid redundant locale conversions on base64-heavy compositions. - Benchmark now asserts the transform cost delta, not just that one small fixture fits under the cap. - Restore over-approximation comment. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
All four points addressed at e6dea112c:
1. Malformed lang → RangeError (blocker): collectLangAttributes now validates each primary subtag through Intl.getCanonicalLocales() in a try/catch. Invalid tags (en_US, x, 123, türkçe) are silently skipped — you lose the locale variants, not the render. New test: "skips invalid lang attributes without crashing".
2. html.includes("full-width") over-triggers (blocker): Replaced with TEXT_TRANSFORM_FULL_WIDTH_RE = /text-transform\s*:[^;]*full-width/ — only fires when there's an actual text-transform declaration containing full-width. A CSS class named .full-width or a data-layout="full-width" attribute no longer triggers the expansion. Same treatment for full-size-kana. New test: "does not trigger fullwidth expansion from a CSS class named full-width".
3. Closure loop performance on base64: Changed the iteration at the entry point from [...Array.from(html), ...Array.from(decodedBodyText)] to new Set([...Array.from(html), ...Array.from(decodedBodyText)]) — deduplicates before the closure runs, so a 521 KB base64-heavy composition runs the locale conversions on ~distinct characters only, not per-source-character.
4. Benchmark assertion: Now asserts the transform cost delta (with-transforms minus without-transforms) stays under 900 encoded chars, plus the absolute cap. This catches the specific regression where transforms silently push realistic compositions onto full-font downloads.
Comment restored: Added the over-approximation rationale back ("raw html includes base64, scripts, and class names, but they collapse in the Set and the budget gate catches bloat").
— Miga
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at e6dea112c. Three of the four items are genuinely fixed. The text-transform gate is not — the new regex trades the old false positive for a broader one, and I can reproduce the original budget blowout against this head.
I re-derived all four rather than reading the summary, because these implement my own recommendations and that is exactly where a reviewer stops looking.
Fixed, verified
langvalidation (deterministicFonts.ts:1226-1240).Intl.getCanonicalLocales(primary)inside a try/catch is the right guard, and it is exactly as strict as the thing that was throwing. I fuzzed 41 tags through bothIntl.getCanonicalLocalesand"a".toLocaleUpperCase(tag)looking for a gap where validation passes but the call at:1250still throws: zero gaps.en_US,en_us,x,123,türkçe,i,a,abcd,root,x-priv,en--US," "all rejected by both;en-US,tr,zz,und,tlh,zh-Hant,de-DE-1901,en-US-u-ca-buddhistaccepted by both. The crash is closed, not narrowed.- Dedupe before the closure (
:1283). Byte-identical output confirmed at this head —Setpreserves first-occurrence order, so insertion order intouniqueCharactersis unchanged (593 → 593 encoded chars on a mixed-script fixture, strings equal). - Benchmark now asserts the delta (
deterministicFonts-textSubset.test.ts:185-188). Measured against the real fixture:withTransforms1231,withoutTransforms448,transformCost783 against the< 900bound. That pins the cost magnitude, which is what was missing. Note the total is 1231 = 72.4% of the 1700 budget, so the headroom is thinner than "stays within the URL budget" suggests — worth knowing, not worth blocking.
Blocker — the new gate matches across rule boundaries, reintroducing the bug
deterministicFonts.ts:1272-1273:
/text-transform\s*:[^;]*full-width/
[^;]* is stopped only by a semicolon, and a declaration's trailing semicolon is optional. So the match runs from a text-transform: declaration through the end of its rule and into whatever follows — including a class name in a later rule, or class="full-width" in the body markup.
Both of these fire the fullwidth expansion at this head:
<style>h1 { text-transform: uppercase }
.full-width { width: 100% }</style>
<style>h1 { text-transform: uppercase }</style>
<div class="full-width">…</div>Neither page applies text-transform: full-width to anything. On a realistic page (one text-transform: uppercase rule, a .full-width layout class, ~100 chars of Latin copy) the spurious trigger costs 684 encoded characters — 40.2% of the 1700-char budget. That is the same failure I flagged at 5a50580e, reachable through a narrower door: any page combining a text-transform declaration with the string full-width elsewhere.
Omitting the final semicolon in a block is ordinary CSS, and it is what most minifiers emit, so this is not a corner case.
Second defect, same two lines: CSS property names and keyword values are ASCII case-insensitive, but these regexes are not. text-transform: FULL-WIDTH and text-transform: Full-Width both apply the transform in the browser and neither fires the gate, so the fullwidth glyphs are left out of the subset — I confirmed Ａ is absent from the emitted text= for the uppercase spelling. This one is a pre-existing miss (html.includes("full-width") was case-sensitive too), but it lives in the two lines this PR is rewriting and it is the same character of fix. Missing glyphs are a worse outcome than a wasted budget: the render is visibly wrong rather than merely unsubsetted.
One regex resolves both:
const TEXT_TRANSFORM_FULL_WIDTH_RE = /text-transform\s*:\s*[^;{}]*\bfull-width\b/i;
const TEXT_TRANSFORM_FULL_SIZE_KANA_RE = /text-transform\s*:\s*[^;{}]*\bfull-size-kana\b/i;Excluding { and } is what prevents the bridge — a value can never legally contain a brace, so the match cannot leave its own declaration block. I ran 13 cases against both versions: the current regex is wrong on 5 (three genuine-but-uppercase declarations missed; the bridge and the body-markup class spuriously fired), the proposed one is correct on all 13, including !important, minified h1{text-transform:full-width}, a newline inside the value, and an inline style= attribute.
The new test cannot catch this
deterministicFonts-textSubset.test.ts:213-221 is the right idea, but its fixture (:216) contains no text-transform declaration at all — so it passes under a regex that has no anchoring whatsoever. The case that discriminates is a page with both a text-transform declaration and a full-width token elsewhere:
it("does not trigger fullwidth expansion from an unrelated text-transform plus a full-width class", async () => {
const text = await subsetTextFor(
`<!doctype html><html><head><style>
h1 { font-family: "Noto Performance Test", sans-serif; text-transform: uppercase }
.full-width { width: 100% }
</style></head><body><div class="full-width"><h1>ABC</h1></div></body></html>`,
);
expect(text).not.toContain("Ａ");
});Worth a positive companion asserting Ａ is present for text-transform: FULL-WIDTH, so the case-insensitivity fix is pinned too.
CI
Not treating this head as green: at review time Build, Semantic PR title, Test: runtime contract and Typecheck had passed, while Test and Render on windows-latest were still in_progress and Tests on windows-latest and regression had not reported at all (they are roll-ups that do not exist until their shard matrices finish, so absent reads identically to passing unless you check each required name). Zero failures so far. My verdict does not rest on any of it — the two defects above are reproducible from the source.
Heads-up on a flicker this review itself may cause: a pull_request_review event can cancel the in-flight pull_request matrix in the same concurrency group, and review-triggered runs skip their jobs by design. If you see a cancelled run at e6dea112c, that is this review, not a failure — the fix push will re-fire the full matrix, which is why I did not hold the submit this time.
Verdict
REQUEST CHANGES. The lang crash, the dedupe and the benchmark delta are all properly resolved and I would not hold the PR on any of them. The gate rewrite still admits the budget blowout it was meant to remove, and additionally misses genuine uppercase declarations; both close with the one regex above plus a fixture that has a text-transform declaration in it.
— Rames Jusso
…ching
Exclude {} from the text-transform regex character class so the match
cannot cross rule boundaries when the trailing semicolon is omitted.
Add /i flag so uppercase declarations (text-transform: FULL-WIDTH)
are not missed. Test now uses a fixture with both a text-transform
declaration and a .full-width class to exercise the bridging case.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
Fixed at `7405a4801`:
Regex bridging: Added `{}` to the exclusion class — `[^;{}]*` cannot cross rule boundaries even when the trailing semicolon is omitted. `h1 { text-transform: uppercase } .full-width { width: 100% }` no longer triggers.
Case-insensitivity: Added `/i` flag — `text-transform: FULL-WIDTH` and `text-transform: Full-Width` now correctly fire the gate.
Tests updated:
- Discriminating fixture: `text-transform: uppercase` rule + `.full-width` class — asserts no fullwidth expansion
- Positive case-insensitivity test: `text-transform: FULL-WIDTH` — asserts Ａ IS present
17 tests pass, all hooks green.
— Miga
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at 7405a48016082e4a14554807897c38c42e8429e6. Both blockers from the previous round are closed. Approving.
This round implements the regex I suggested, so I re-derived it against the code and measured the behaviour rather than checking that the diff matched my comment — a PR that implements the reviewer's own recommendation is where confirmation bias does its damage.
Strengths
deterministicFonts.ts:1272-1273— excluding{and}is the right shape rather than a longer alternation, because a CSS value can never legally contain a brace. The match now cannot leave its own declaration, which is the actual invariant.deterministicFonts-textSubset.test.ts:213— the fixture is the discriminating one: a realtext-transform: uppercasewith the trailing semicolon omitted, a.full-widthrule after it, andclass="full-width"in the body. It exercises the bridge and the substring false-positive in a single case.deterministicFonts-textSubset.test.ts:224— a positive test for the uppercase keyword, not just a negative. It pins the behaviour that a case-sensitive gate silently loses.+2/-2in source. The round-2 fixes are byte-identical at this head.
Verification
Regex behaviour, 14 cases through both patterns. New: correct on 14/14. Old ([^;]*, no /i): wrong on 6 — the two bridge cases and the two case-insensitive ones I reported, plus text-transform: xfull-width and text-transform: full-widthx, which the old pattern also fired on and which I had not listed. Both \b anchors are load-bearing: dropping either reintroduces those two.
Budget, end to end. On a realistic page (uppercase transform with no trailing semicolon, .full-width utility class in markup) the spurious trigger cost 612 encoded characters — 36% of the 1700-char budget at e6dea112c. At this head the same page measures 96 characters, 5.6%, and the trigger correctly does not fire. That was the failure mode worth blocking on: enough spurious expansion and text= is omitted entirely, which silently downloads the full font.
Both new tests discriminate. I ran each fixture against the previous implementation. The :213 bridge fixture fires fullwidth expansion under [^;]*, so the test fails pre-fix; the :224 fixture does not fire without /i, so it fails pre-fix too. Each passes only against the new code. Replacing the old class-only test with the bridge fixture is a strict strengthening rather than a trade — the new fixture still contains a .full-width class, so it also catches a regression back to a raw includes.
Nits — neither gating
text-transform: /* full-width */ noneand@supports (text-transform: full-width)both still fire the expansion. Over-approximation only, consistent with the note at:1280, and strictly narrower than what shipped before this PR. Not worth more regex.- The
\s*after the colon at:1272-1273is redundant —[^;{}]*already matches whitespace, and removing it changes none of the 14 cases. That redundancy came from my own suggested pattern; flagging it so it does not read as intentional.
Follow-up, out of scope here
text-transform: var(--tt) cannot be resolved by a source regex, so a transform delivered through a custom property is invisible to the gate and its glyphs are missing from the subset. Pre-existing, unchanged by this PR, and it needs computed style rather than pattern matching — noting it so it is on the record, not asking for it here.
CI
All 8 required contexts are completed/success at this head — Build, Render on windows-latest, Semantic PR title, Test, Test: runtime contract, Tests on windows-latest, Typecheck, regression — in a single wave, every workflow at attempt=1, zero cancelled or failed runs.
I held this review until the matrix concluded rather than submitting into it. A review event cancels the in-flight pull_request matrix in the same concurrency group and nothing re-fires it, so posting while Test and Render on windows-latest were still running would have left this commit with no test result and cost you a rerun. Worth noting for the next round: regression and Tests on windows-latest are roll-ups that do not exist until their shard matrices finish, so an absent context reads identically to a passing one — I check each required context by name rather than looking for red.
Verdict: APPROVE
Reasoning: Both blockers are closed and measured — the gate can no longer escape its declaration and is case-insensitive — the two new tests each fail against the previous implementation, and the round-2 fixes are intact at this head.
— Rames Jusso
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Correcting my own approval on this head, and the defect is mine.
I submitted an approve at 7405a4801 after checking the eight required contexts by name. I did not read the CodeQL check before submitting — it is not a required context, and I treated the UNSTABLE merge state as benign without opening it. It is not benign:
Polynomial regular expression used on uncontrolled data — high severity. "This regular expression that depends on library input may run slow on strings starting with
text-transform:and with many repetitions of' '."
That is js/polynomial-redos, and it lands on the regex I recommended. (The annotation's start_line: 1304 is drifted — line 1304 at this blob is resolveFontFetchRetryPolicy, and :1272-1273 are the only text-transform regexes in the file — but the message identifies the pattern unambiguously.)
blocker — quadratic backtracking, introduced by this round
deterministicFonts.ts:1272-1273. In \s*:\s*[^;{}]*, the \s* and the [^;{}]* both match a space, so text-transform: followed by N spaces has ~N ways to split between the two quantifiers, and every one is explored before \bfull-width\b fails.
Measured on this exact pattern, input "text-transform:" + " ".repeat(N) + "x":
| N spaces | \s*:\s*[^;{}]* (this head) |
\s*:[^;{}]* (fix) |
\s*:[^;]* (previous head) |
|---|---|---|---|
| 10,000 | 82 ms | 0.0 ms | 0.0 ms |
| 20,000 | 326 ms | 0.0 ms | 0.0 ms |
| 40,000 | 1,303 ms | 0.1 ms | 0.1 ms |
Doubling the input multiplies the time by 4.00x — textbook quadratic. This is a regression introduced at 7405a4801: the previous head's [^;]* had no adjacent \s* and stays linear, which is why CodeQL files it under "new alerts in code changed by this pull request".
The input is composition HTML, so a composition whose stylesheet contains text-transform: with a long whitespace run burns CPU in the render path proportional to its square. No exploit needed for it to bite — a minifier artifact or a generated stylesheet gets there on its own.
the fix is the nit I understated
I flagged the redundant \s* in my approval as a nit that "costs nothing" to drop. That was the right observation with the wrong severity: [^;{}]* already matches whitespace, so removing the \s* is behaviour-preserving and removes the ambiguity that causes the backtracking.
const TEXT_TRANSFORM_FULL_WIDTH_RE = /text-transform\s*:[^;{}]*\bfull-width\b/i;
const TEXT_TRANSFORM_FULL_SIZE_KANA_RE = /text-transform\s*:[^;{}]*\bfull-size-kana\b/i;I re-ran the full 14-case matrix against both forms: 0 cases change verdict, and the fixed form stays correct on 14/14. So this costs no coverage, and both new tests at :213 and :224 keep passing unchanged. Apply it to the kana twin as well — same shape, same defect.
Worth a test that pins it, since nothing in the suite would have caught this: assert the matcher returns in bounded time on "text-transform:" + " ".repeat(50_000), or simply assert the compiled source contains no \s* adjacent to [^;{}]*.
everything else in my approval stands
Both original blockers are genuinely closed and I am not reopening them: the gate no longer escapes its declaration, it is case-insensitive, the spurious 612-character budget cost is gone, both new tests fail against the previous implementation, and the round-2 fixes are byte-identical at this head. All eight required contexts are green in a single wave at attempt=1. This is one line of character class away from the approve I already gave.
Verdict: REQUEST CHANGES
Reasoning: A high-severity ReDoS regression introduced by this round, in the two lines it changes, with a behaviour-preserving one-token fix — and my earlier approve on this head was submitted without reading the CodeQL failure that reported it. Since CodeQL is not a required context, my approval was the only thing standing between this and a merge with a known quadratic matcher.
— Rames Jusso
Drop the \s* between : and [^;{}]* — the character class already
matches whitespace, and the adjacent quantifiers caused quadratic
backtracking on inputs like "text-transform:" + " ".repeat(N).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
Fixed at `7610beebe` — dropped the redundant `\s*` between `:` and `[^;{}]`. `[^;{}]` already matches whitespace, so the `\s*` was pure ambiguity. Quadratic backtracking eliminated.
All 17 tests pass unchanged — the fix is behaviour-preserving as you confirmed.
— Miga
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Round 4 review at 7610beebea332fc84e222121942c67966351a127.
What's genuinely good
- The fix is exactly scoped. One commit, one file, +2/−2.
deterministicFonts-textSubset.test.tsis byte-identical to round 3 (cmp-verified), so "all 17 tests pass unchanged" is a real claim, not a rebuild — no drive-by edits rode along on a security fix. - It does what it says. I re-extracted both patterns from the shipped blob (not from the diff) and re-ran the timing harness. The whitespace-driven quadratic is gone:
text-transform:+ N spaces went from 1226 ms at N=40 000 / 4.00× per doubling to a flat 0.03 ms / 1.99× per doubling. - Behaviour is preserved. Across my 14-case matrix the shipped regexes are 14/14 and differ from round 3 on 0/14 cases — the round-3 correctness wins (case-insensitivity, no bridging across rules,
xfull-width/full-widthxrejection) all still hold at this head, where the round-2 pattern was wrong on 6.
First, a correction I owe you
In round 3 I wrote that the ReDoS was introduced by round 3 and that dropping the redundant \s* was the fix. Both halves were wrong, and the analysis history says so plainly:
| PR head | CodeQL merge commit | js-ts results |
|---|---|---|
5a50580ed |
3ee9610c8 |
0 |
e6dea112c |
4f0de981f |
1 ← alert #879 created 04:33:11Z |
7405a4801 |
a2e8a82d9 |
1 |
7610beebe |
74400a187 |
1 |
Alert #879 was raised against e6dea112c — before my \s* suggestion existed. That suggestion added a second pathological witness on top of an alert that was already open; removing it removed my addition and nothing else. It was never going to clear the alert, and I should have read the alert body in round 3 instead of inferring the mechanism from the diff.
blocker — alert #879 (js/polynomial-redos, high) is still open at this head
CodeQL concluded failure at 7610beebe. Analyze (javascript-typescript) itself succeeded — the roll-up fails on the open alert.
Its current message names a different witness than the one you just fixed:
This regular expression that depends on library input may run slow on strings starting with
'text-transform:'and with many repetitions of'text-transform:'.
Where. The annotation says deterministicFonts.ts:1304, which is the merge commit's numbering (74400a187, 1406 lines); cols 7–46 there are TEXT_TRANSFORM_FULL_WIDTH_RE.test(html)). At the PR head that call site is :1287 and the pattern is :1272. (Flagging the mapping because I misread it as a stale line number last round — CodeQL anchors this rule to the .test() call on the tainted string, numbered against the merge commit.)
Mechanism, and why \s* was irrelevant to it. When the input contains no ;, { or }, [^;{}]* runs to end-of-string from every text-transform: start position, and each one backtracks the whole tail before \bfull-width\b fails. That is quadratic in the number of text-transform: occurrences, with or without the \s*. Median of 7 runs, 3 warmups, node 22, on "text-transform:".repeat(N):
| N | input | [^;{}]* at this head |
|---|---|---|
| 2 000 | 29 KiB | 48.9 ms |
| 4 000 | 59 KiB | 195.6 ms (×4.00) |
| 8 000 | 117 KiB | 781.7 ms (×4.00) |
| 16 000 | 234 KiB | 3 126.8 ms (×4.00) |
Exactly 4× per doubling. Extrapolating the same curve, a ~1 MiB composition is tens of seconds of single-threaded CPU.
Why this still counts, given the repo carries five open js/polynomial-redos alerts (including one in this same file at :442): main has no text-transform regex at all — grepping the file at ref=main returns nothing — so both the construct and the alert are new with this PR, on a function that runs over composition HTML that isn't fully trusted on the hosted render path.
The fix — the rule's own Recommendation, not another regex tweak
This is now the second patch iteration against this one rule, which is the point at which the rule's documentation is the better guide than the pattern:
Recommendation. Modify the regular expression to remove the ambiguity, or ensure that the strings matched with the regular expression are short enough that the time-complexity does not matter.
This repo already has a house pattern for precisely that, in three places — packages/core/src/beats/beatFile.ts:17-24, packages/lint/src/utils.ts:260-263, packages/core/src/storyboard/parseStoryboard.ts:45-49 — each replacing the ambiguous pattern with a linear indexOf/slice scan and naming the rule in a comment. Following it here:
const DECLARATION_WS = new Set([" ", "\t", "\n", "\r", "\f", "\v"]);
const FULL_WIDTH_KEYWORD_RE = /\bfull-width\b/;
const FULL_SIZE_KANA_KEYWORD_RE = /\bfull-size-kana\b/;
// Linear indexOf/slice scan rather than one regex: a `text-transform\s*:[^;{}]*\bkw\b`
// pattern backtracks O(n²) on input with many `text-transform:` runs and no `;{}`
// between them (CodeQL js/polynomial-redos).
function hasTextTransformKeyword(html: string, keyword: RegExp): boolean {
const haystack = html.toLowerCase();
const property = "text-transform";
let from = 0;
for (;;) {
const at = haystack.indexOf(property, from);
if (at === -1) return false;
let cursor = at + property.length;
while (cursor < haystack.length && DECLARATION_WS.has(haystack[cursor]!)) cursor += 1;
if (haystack[cursor] !== ":") {
from = at + property.length;
continue;
}
let end = cursor + 1;
while (end < haystack.length && haystack[end] !== ";" && haystack[end] !== "{" && haystack[end] !== "}") {
end += 1;
}
if (keyword.test(haystack.slice(cursor + 1, end))) return true;
from = end; // resume at the declaration boundary — this is what keeps it linear
}
}
// call sites (`html` is already lowercased inside, so no /i needed on the keywords)
if (hasTextTransformKeyword(html, FULL_WIDTH_KEYWORD_RE)) addFullwidthVariants(uniqueCharacters);
if (hasTextTransformKeyword(html, FULL_SIZE_KANA_KEYWORD_RE)) addFullSizeKanaVariants(uniqueCharacters);from = end is the load-bearing line: resuming at the boundary instead of just past the property name is what stops each occurrence from re-scanning the same tail. Verified against the shipped regexes:
- 0/14 verdict differences on the correctness matrix — 14/14, identical decisions including the bridging and
\bword\bcases, so your existing tests should pass unchanged. - Linear on both witnesses: I ran the block exactly as pasted above — 0.187 ms at 59 KiB, 0.304 ms at 117 KiB, 0.728 ms at 234 KiB, against 2 935–3 127 ms for the regex on that last input (~4 000×), scaling ~2× per doubling.
Take it or write your own — the requirement is just that the check stops being quadratic in occurrence count. The Recommendation's second clause (bound the input length before matching) would also satisfy the rule if you'd rather keep a regex, but it caps a real feature on adversarial-input grounds, so the linear scan seems the better trade.
One test worth adding either way: assert the scan stays fast on "text-transform:".repeat(20_000), so this can't silently regress a third time. Note the existing benchmark at deterministicFonts-textSubset.test.ts:187 wouldn't have caught any of this — it measures URL budget, not match time.
notes
- I enumerated all 61 check-runs at this head rather than only the eight required contexts.
CodeQLis the sole failure;GCP BeginFrame image contractisskipped(conditional job, benign); everything else concludedsuccess— includingTest, which is what makes the 17-tests-pass claim verified rather than taken. Checking only the required set is exactly how I missed this alert for two rounds —CodeQLis not a required context, so all-eight-green sat next to an open high-severity alert without contradiction. - No auto-merge and no merge-queue entry armed at review time.
Verdict: REQUEST CHANGES
Reasoning: The blocker I raised in round 3 is genuinely fixed and measured, but alert #879 — high-severity, PR-introduced, and open since e6dea112c — is a distinct quadratic that the \s* removal never touched, and the repo already has an established linear-scan pattern for this exact rule.
— Rames Jusso
The text-transform regex backtracks O(n²) on input with many
text-transform: runs and no ;{} between them. Replace with a linear
indexOf/slice scan following the repo's existing pattern for this
CodeQL rule (beatFile.ts, utils.ts, parseStoryboard.ts).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
Fixed at `0d5544336` — replaced the regex entirely with a linear `indexOf`/`slice` scan, following the repo's existing pattern for `js/polynomial-redos` (beatFile.ts, utils.ts, parseStoryboard.ts).
The scan finds each `text-transform` occurrence via `indexOf`, skips whitespace, checks for `:`, then slices to the next `;{}` boundary and tests the keyword regex on that bounded slice only. `from = end` resumes at the declaration boundary, keeping it linear in occurrence count.
Extracted `skipWhitespace` and `findDeclarationEnd` helpers to keep cyclomatic complexity below the fallow threshold.
17 tests pass unchanged.
— Miga
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Round 5 review at 0d5544336d315317c2c4ecb7e740f2da6016b552. This closes my blocker.
What's good
- The right fix, and it follows the house pattern properly.
hasTextTransformKeyword(deterministicFonts.ts:1288) retires the regex-over-raw-HTML entirely, and the comment at:1286names the rule the waybeatFile.ts:17,lint/src/utils.ts:262andparseStoryboard.ts:48do. Splitting outskipWhitespaceandfindDeclarationEndreads better than the version I pasted. - Surgical. One commit, +36/−4, one file. Lines 1–1268 are byte-identical to the previous head, so the round-3 work (
Intl.getCanonicalLocalesguard at:1235, the character dedupe, the 1 700-char budget at:1199) is untouched — no rewrite rode along with a security fix. - The whitespace set is a real improvement, not just a port.
" \t\n\r\f\v"is CSS whitespace; JS\sadditionally matches U+00A0 and friends, so the oldtext-transform\s*:matchedtext-transform :— a declaration browsers drop, because U+00A0 isn't valid in a property name. Verified: space, tab, newline and form feed all still trigger, U+00A0 now correctly doesn't. That's a false positive retired as a side effect.
What I verified
I extracted the shipped implementation from the blob at this SHA and ran it directly, rather than diffing it against what I suggested.
Correctness — 20/20, and 0 differences from my own reference implementation. Beyond the 14 cases from earlier rounds (case-insensitivity, no bridging across rules, xfull-width / full-widthx rejection), I added six shapes aimed specifically at this refactor: tab before the colon, property name at end-of-input, bare text-transform: at end-of-input, value terminated by } rather than ;, and a hit in the first vs. second declaration of a pair. All correct. full-size-kana matrix also clean.
The one difference from the old regex is the U+00A0 case above, and it's the new behaviour that's right.
Linearity — I checked four witnesses, including one your refactor could plausibly have introduced. findDeclarationEnd does DECLARATION_BOUNDARY_RE.exec(s.slice(pos)), and s.slice(pos) copies the tail on every declaration — which is a quadratic shape in its own right (N declarations × tail length) even though no regex backtracks. It doesn't bite, because V8 makes that slice O(1); I measured rather than assumed:
| witness | input | old regex | this head |
|---|---|---|---|
"text-transform:".repeat(N) |
469 KiB | 11 815 ms (×4.00) | 0.81 ms (×2.17) |
text-transform: + N spaces |
31 KiB | 0.1 ms | 0.04 ms |
"text-transform:a;".repeat(N) |
531 KiB | 0.3 ms | 2.81 ms (×1.92) |
"text-transform:a}".repeat(N) |
531 KiB | 0.3 ms | 2.79 ms (×1.98) |
Every column is ~2× per doubling — linear. The alert's witness goes from 11.8 s to 0.81 ms (~14 500×), and the many-terminated-declarations case that the tail slicing put at risk stays under 3 ms at half a megabyte.
important — no regression test for any of this
deterministicFonts-textSubset.test.ts is untouched, so "all 17 tests pass" is true but none of them would notice a fourth regression here. This construct has now been quadratic in two independent ways across three revisions of one PR, and the existing benchmark at :187 measures URL budget, not match time — it stayed green through both. CodeQL is a real guard for the regex shape, but it would not catch a reintroduced tail-copy in a loop, which is exactly the risk I had to measure by hand above.
One test pins the property that actually matters:
it("stays linear on adversarial text-transform input", async () => {
const html = `<!doctype html><html><head><style>
p { font-family: "Noto Performance Test", sans-serif; }
</style></head><body><p>${"text-transform:".repeat(20_000)}</p></body></html>`;
const started = performance.now();
await subsetTextFor(html);
expect(performance.now() - started).toBeLessThan(1_000);
});At this head that path is sub-millisecond, so a 1 s bound is ~1000× of headroom — loose enough not to flake on a busy runner, tight enough that any return to quadratic (the old regex took 11.8 s on a smaller input) fails it outright. Worth having before this merges, given the history — but it's a coverage gap, not a defect in what you wrote, so I'm not holding the approval for it.
nits
:1286— the comment explains why it isn't a regex, which is the important half. Consider also noting that" \t\n\r\f\v"is deliberately CSS whitespace and not JS\s, so nobody "tidies" it back to\sand quietly restores the U+00A0 false positive.:1318-1319— the two calls each lowercase and scan the whole document, so a large composition is walked twice. Measured ~3 ms of the ~6 ms total on a 0.94 MiB input; irrelevant next to a render, but a single pass returning both flags would be the natural shape if this ever gets hot.
notes
- The defect I reported is gone on the merits — the numbers above come from running the shipped code, not from reading a check. For what it's worth it also shows up in the scanner: alert #879 now reads
fixed, and the analysis at this revision returns 0 results. - The
@font-faceparser at:1120has a similar multi-[^}]*shape, so I timed it too — linear (~×1.9 per doubling, 0.03 ms at 16 KiB). Not an issue, and not touched by this PR; noting it only so it doesn't get re-litigated later. - No auto-merge and no merge-queue entry armed at review time.
Verdict: APPROVE
Reasoning: The quadratic is gone by measurement — 11.8 s to 0.81 ms on the alert's own witness, linear across all four inputs I tried including the tail-slicing risk your refactor introduced — with 20/20 correctness and the round-3 work byte-identical. The missing regression test is worth adding before merge given this construct has been quadratic two different ways, but it's a gap in coverage, not a defect in the code.
— Rames Jusso
2 participants
What
Extends the Google Fonts
text=subset closure to cover locale-sensitiveCSS text transforms that the locale-independent
toUpperCase()/toLowerCase()closure in #3492 explicitly deferred.
Fixes #3496.
Why
A composition with
lang="tr"andtext-transform: uppercaseneeds Turkish İ(U+0130) in its subset —
"i".toUpperCase()gives"I", not"İ". Similarly,text-transform: full-widthandfull-size-kanasynthesize glyphs that theexisting code-point closure cannot reach. Without these variants in the subset,
the browser falls back glyph-by-glyph and the rendered output shows a different
face for the transformed characters.
How
Three additions to
extractGoogleFontsText, continuing the existingover-approximation pattern (safe but not CSS-property-aware):
Locale-aware case closure —
collectLangAttributeswalks the DOM forlangattributes and normalizes to BCP-47 primary subtags. For each detectedlocale,
toLocaleUpperCase(locale)/toLocaleLowerCase(locale)close thecharacter set. Covers Turkish/Azeri İ/ı and German ẞ.
Fullwidth ASCII — when
full-widthappears anywhere in the HTML source,maps ASCII U+0021–U+007E to fullwidth equivalents U+FF01–U+FF5E.
Full-size kana — when
full-size-kanaappears in the source, maps 22small hiragana/katakana to their full-size counterparts via a const lookup.
The URL-length budget (1700 encoded chars) and full-font fallback are preserved.
Test plan
deterministicFonts-textSubset.test.ts:lang="tr"(present) andlang="en"(absent)lang="az"full-widthCSS)— Miga