fix: gsap_callback_dom_measurement no longer taints every caller of a shared helper - #3949
Open
miga-heygen wants to merge 6 commits into
Open
miga-heygen wants to merge 6 commits into
miga-heygen wants to merge 6 commits into
Conversation
… shared helper The rule marked a shared helper's name as "measuring" if any branch anywhere in its body reached a DOM measurement, then flagged every caller identically regardless of which branch that call actually took. A call passing a literal boolean/string argument that gates a recognized if/else or switch/case shape inside the helper is now resolved to the single branch it reaches; anything unresolvable (non-literal argument, unrecognized shape, measurement reachable outside the matched branch) falls back to the old conservative whole-function taint, so no new false negative is introduced. Also fixes a related inconsistency: the per-callback check required no call syntax at all (a bare, uncalled mention of a measuring function's name was enough), unlike the stricter call-requiring closure already used to propagate taint between named functions. Standardizing on "requires a call" fixes that bare-mention false positive, at the cost of no longer flagging a callback that passes a tainted name by reference to something that invokes it later. Co-Authored-By: Miguel Angel <miguel.sierra@heygen.com>
…all-site resolver A quality-review pass on the branch-resolution follow-up found several ways the literal-argument resolver could substitute a caller's argument for a parameter that no longer held it at the matched if/switch, or treat a fall-through case as fully resolved: - The parameter could be reassigned (`m = !m`, `m++`) or shadowed (a nested function/arrow/method/catch parameter, or a let/const/var/destructured declaration reusing the name) before the matched branch. A new paramMayBeRebound() check bails to the old conservative whole-function taint whenever any of these are detected ahead of the branch. - A switch case without a terminating break/return/throw falls through into the next case at runtime, which wasn't accounted for. The terminator check is string-literal-aware, so a keyword-shaped substring inside a string argument (e.g. "break") can't be mistaken for a real terminator. - The case/default boundary scan lacked a leading word-boundary check, so it could match mid-identifier (e.g. "case" inside "lowercase") and truncate a case body early, silently dropping real code that follows. Each fix was verified with a real revert-and-restore: temporarily disabling just that guard reproduces the false negative, restoring it clears the regression. 14 new tests cover the reassignment, five shadowing shapes, the fall-through and mid-identifier truncation cases, plus three previously correct-but-untested guards. Co-Authored-By: Miguel Angel <miguel.sierra@heygen.com>
… gsap_callback_dom_measurement
Quality-review pass on the branch-resolution follow-up found four more ways
the literal-argument resolver could resolve incorrectly, plus a structural
duplication and a dropped behavior:
- The switch fall-through terminator check used a trailing regex a
conditionally-nested break (e.g. `if (x) { break; }`) could fool into
reading as unconditional termination. Replaced with a statement-level
check that recurses into the case's own last top-level statement.
- The case-label search wasn't depth-aware, so a same-valued case nested
inside a sibling case's own inner switch could be matched instead of the
real top-level case. Replaced with a depth-tracking scan.
- A spread argument made every later positional argument's binding
unreliable; the resolver now bails for every param at or after a spread.
- paramMayBeRebound only scanned the text before the matched branch, missing
a rebind that appears textually after it but still matters whenever the
branch can re-execute. An interim attempt narrowed this to "only widen
when a loop-like construct is present" for precision, but two independent
adversarial reviews each found a real repetition shape (Array#forEach/map,
among others) that heuristic missed. Reverted to unconditionally scanning
the whole body: a false positive is tolerated by this rule's design, a
false negative is not. Also added for-of/for-in param-name reuse to the
rebind check.
- .call/.apply/.bind and their optional-chained forms are direct
invocations and must be flagged like a plain call; restored for both the
leaf-callback check and the two-hop named-function taint closure (the
latter had zero prior test coverage for this path).
- Four independently hand-rolled string-literal scanning state machines are
now one shared advanceStringScan primitive.
Every fix verified with a real revert-and-restore: temporarily disabling
just that guard reproduces the false negative (or, for the loop-detection
reversion, the false positive it was meant to avoid), restoring it clears
the regression. Test count: 220 -> 225.
Known limitation (pre-existing, not introduced here): the shared scanner
doesn't recognize /regex/ literals as opaque, so a bracket-like character
inside a regex (e.g. a {n,m} quantifier) can desync bracket-depth counting.
Flagging as a follow-up rather than fixing here.
Co-Authored-By: Miguel Angel <miguel.sierra@heygen.com>
…existing regex-aware utility Quality re-review found the prior round's own hand-rolled string-scanning primitive had a real, confirmed regression: it recognized only "/'/` delimiters and had no concept of regex literals at all, so a quote inside a regex (e.g. a sanitizer `/'/g`) opened a "string" that never closed, corrupting bracket-depth counting and silently dropping the affected function from the taint set. `packages/lint/src/utils.ts` already exports `stripJsStringLiterals`, a length-preserving blanker that correctly handles string/template/regex literals including regex-vs-division disambiguation. Deleted the entire hand-rolled advanceStringScan/StringScanState/DepthScanState mechanism and converted every structural scanner (matchBalanced, enclosingObjectLiteral, objectLiteralHasTopLevelRelativeValue, isInsideGsapTweenVars, sliceExpression, splitTopLevelByComma, sliceUntilNextCase, findTopLevelCaseMatch, splitTopLevelStatements) to the same pattern: mask once via a local maskLiterals() (memoized on last input), take structural decisions from the masked text, slice/return the original. Two more bugs surfaced and were fixed during this conversion, both verified with a real revert-and-restore: - findTopLevelCaseMatch's depth check alone wasn't enough — a case-label- shaped substring sitting inside a string/template literal at the switch's own top-level bracket depth (e.g. a template literal containing literal text that reads like a case label) wasn't excluded, since a same-depth string doesn't touch bracket counting at all. Now also requires the match site itself to still be unmasked. - stripJsStringLiterals has its own documented fail-safe: if it can't resolve a regex-vs-division ambiguity ANYWHERE in what it scans, it returns the ENTIRE input untouched, fully unmasked. Masking the whole script (as originally converted) meant one unrelated, ordinary line elsewhere in the same script could silently disable masking for every other function sharing that source. matchBalanced/sliceExpression and the backward brace scanners now mask only the slice of text a given call actually needs, shrinking that blast radius to text scoped to the call rather than the whole script. Test count: 225 -> 229. Full lint package suite (676 tests across 16 files) still green, confirming no regression in the other rules that reuse these shared scanners. Co-Authored-By: Miguel Angel <miguel.sierra@heygen.com>
…ing per call stripJsStringLiterals's own fail-safe (bail the entire input unmasked on an unresolved regex/division ambiguity) forced gsap.ts to mask only a per-call slice to limit its blast radius, which reintroduced two problems: a forward scanner masking to end-of-script still let an ambiguity after the point it starts from poison that call, and the narrowed slicing defeated gsap.ts's memoization (~99% cache-miss, ~50% slower on a 410-component registry lint pass under CI). Fixed at the root: a misread now rewinds and re-walks its own line-bounded span as ordinary code instead of bailing the whole input, so masking correctly covers text on either side of a misread. gsap.ts's scanners now mask the whole script/body they already hold again, backed by a small LRU (not a single slot) since legitimate calls interleave masking the script, a function body, and small argument lists. Two more bugs found by adversarial review were fixed before shipping: the initial recovery implementation sliced the accumulated output buffer on every misread (quadratic for many-misread input, fixed by buffering tentative content separately), and one call site still pre-sliced before masking (defeating the cache for its whole loop). Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com>
The local-recovery fix only triggered on a line boundary (\n/\r) inside a
guessed regex literal. A guess that ran all the way to end-of-input with no
trailing newline (the last line of a script) never hit that trigger, so it
fell through to the old whole-input bail after all — reproduced both
directly and end-to-end (a helper's string content unmasked because an
unrelated {}/2-shaped division sat on the script's last, newline-less line).
Fixed by treating end-of-input exactly like a line boundary: reaching it
while still holding an open guess now runs the identical recovery instead of
exiting the scan loop. Every exit from the scan now requires the guess to
already be resolved, so the function's fail-safe no longer needs to check
for one — only a genuinely unterminated string/template literal still can.
Also added a length-preservation regression test across a misread
immediately followed by a real regex, and an array-literal-call-argument
fixture, both verified (via a temporary reverted copy) to actually fail
against the specific regression they guard.
Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com>
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.
Summary
gsap_callback_dom_measurementflags GSAP animation callbacks that synchronously measure the DOM (getBoundingClientRect/getTotalLength/getComputedStyle/etc.) — during a deterministic seek-based render, measurement inside these callbacks reads seek-order-dependent, potentially stale layout. The rule works by static text analysis of inline<script>content (no AST), so it has to reimplement string-literal-aware scanning, bracket balancing, and literal call-site/branch resolution by hand. This PR is six commits of hardening that resolver, each verified with a real revert-and-restore (temporarily disable just the one guard, confirm the targeted regression test goes red, restore, confirm green):Commit 1 — resolve individual call sites instead of tainting every caller of a shared helper. A helper like
applyPhaseStyle(el, useMeasurement)withif (useMeasurement) { /* measures */ } else { /* style only */ }used to get every caller flagged the instant one call site anywhere passedtrue. Now, when a call passes a literal boolean/string argument that gates a recognizedif/elseorswitch/caseshape, only the branch that literal actually reaches is evaluated; anything unresolvable falls back to the previous conservative whole-function taint. Also standardized the per-callback check to require an actual call (a bare, uncalled mention of a measuring function's name no longer trips the rule), matching the call-requiring standard already used for named-function taint propagation.Commit 2 — closed gaps in that resolver found on review: the literal-argument substitution could go stale if the parameter was reassigned or shadowed before the matched branch (
paramMayBeRebound); a switch case without a terminatingbreak/return/throwfalls through into the next case at runtime, which wasn't accounted for; the case/default boundary scan lacked a leading word-boundary check and could match mid-identifier (e.g."case"inside"lowercase"), silently truncating a case body.Commit 3 — a further quality pass found the above guards were still incomplete, plus a structural duplication and a dropped behavior:
break(e.g.if (x) { break; }) could fool into reading as unconditional termination — replaced with a statement-level check (lastStatementIsTerminator) that recurses into the case's own last top-level statement.casenested inside a sibling case's own inner switch could be matched instead of the real top-level case — replaced with a depth-tracking scan (findTopLevelCaseMatch)....spreadargument in a call's argument list made every later positional argument's binding unreliable (the spread can expand to any runtime length) —resolveCallSiteMeasurementnow bails for every param at or after a spread.paramMayBeReboundonly scanned the text before the matched branch, missing a rebind that appears textually after it but still matters whenever the branch can re-execute (a loop, anArray#forEach/mapcallback, recursion, ...). An interim attempt narrowed the scan to "only widen when a loop-like construct is present," trading a little precision back — two independent adversarial review passes each found a real repetition shape that narrower heuristic missed (Array#forEach/.map, among others). Enumerating every way a branch might re-execute via text pattern matching turned out to be an open-ended, unwinnable classification problem, so the scan now unconditionally covers the whole body — a false positive (an unrelated post-branch reassignment with no repetition anywhere still forces a conservative bail) is tolerated by design; a false negative is not. Also added afor (name of ...)/for (name in ...)alternative to the rebind check — reusing a parameter as a bare loop variable reassigns it every iteration without matching any of the existing assignment/declaration patterns..call(...)/.apply(...)/.bind(...)()and their optional-chained forms (?.call(...),measureFn?.()) are direct, synchronous invocations and must be flagged like a plain call; they'd been dropped somewhere along the way. Restored for both the leaf-callback check and the two-hop named-function taint closure — the latter had zero test coverage for this path, which a regression test now closes.Commit 4 — the commit-3 string scanner (hand-rolled, aware only of
"/'/`delimiters) had no concept of regex literals at all: a quote inside a regex (e.g. a sanitizer/'/g) opened a "string" that never closed, corrupting bracket-depth counting and silently dropping the affected function from the taint set. Rather than teach the hand-rolled scanner about regexes too, deleted it entirely and delegated to an existing, already-correct utility in this package (stripJsStringLiterals, inutils.ts) that already handles string/template/regex literals — including regex-vs-division disambiguation — for every structural scanner in the file (matchBalanced,sliceExpression,splitTopLevelByComma,sliceUntilNextCase,findTopLevelCaseMatch,splitTopLevelStatements, and the two backward brace-scanners used elsewhere in the file). Two more bugs surfaced during this conversion and were fixed, both verified with a real revert-and-restore:findTopLevelCaseMatch's depth check alone wasn't sufficient — a case-label-shaped substring sitting inside a string/template literal at the switch's own top-level bracket depth (since a same-depth string doesn't touch bracket counting at all) could be mistaken for the real case label. Now also requires the match site itself to still be unmasked.stripJsStringLiteralshas its own documented fail-safe: if it can't resolve a regex-vs-division ambiguity ANYWHERE in what it scans, it returns the entire input untouched, fully unmasked. Masking the whole script naively would let one unrelated, ordinary line elsewhere in the same script (e.g.const ratio = {}/2;) silently disable masking for every other function sharing that source. Commit 4 mitigated this by masking only the slice of text a given call actually needs rather than the whole script — which shrank, but didn't eliminate, the blast radius, and (per commit 5) also reintroduced a real performance regression.Commit 5 — commit 4's per-call slice narrowing had two problems, both found on the next review pass:
matchBalanced/sliceExpression) masks from its own start index to the end of the script — so an ambiguity textually after the point a call starts scanning from, but still inside the region it has to traverse, could still poison that call's masking. A probe confirmed this in practice: a helper containing a real}inside a string, followed later by an unrelated{}/2-shaped division, silently dropped the finding.stripJsStringLiterals(an O(n) char-at-a-time scan) re-ran on a large, overlapping re-sliced substring on nearly every call instead of once per script — confirmed via profiling a 410-component registry lint pass: ~99% cache-miss rate against the single-slot memo, ~28M characters re-scanned, wall time up roughly 50% (a required CI job's timeout budget onregistryComponents.test.ts).Root-caused and fixed both by changing
stripJsStringLiteralsitself instead of continuing to work around it ingsap.ts: a candidate/is only ever a guess at opening a regex literal (regex-vs-division is genuinely ambiguous in text). The guess is now provisional — the function snapshots enough state to undo it before committing, and if a line boundary later proves the guess wrong (a real regex literal can't span a line), it rewinds to that/and re-walks the misread span as ordinary code instead of bailing the whole input. A misread now costs only its own line-bounded span; every literal correctly masked elsewhere in the same script, before or after it, stays masked. An initial version of this recovery rewound by slicing the accumulated output buffer on every misread, which is itself quadratic for adversarial many-misread input (the output buffer is a growing string, and slicing it forces a flatten proportional to its entire length so far) — caught by a second adversarial review pass and fixed by buffering a misread's tentative content separately and discarding it in O(1) instead.With the root cause fixed,
gsap.ts's scanners were reverted to masking the whole script/body they already hold (no more per-call slicing), backed by a small 16-entry LRU cache (not a single slot) inmaskLiterals, since legitimate call patterns interleave masking the whole script, one function's body, and small param/argument lists within processing of one script. The same review pass also caught one remaining call site that still pre-sliced before masking (silently defeating the cache for every call in that loop) — fixed to pass the shared source text and an index instead.Also this commit: added a fixture with the ambiguity placed after the helper (closing the correctness gap above, mirroring the existing before-helper fixture), a fixture for an escaped quote sitting immediately before a real bracket character (guards a specific escape-tracking regression), documented two known, narrow, out-of-scope gaps in the call-site resolver (a measurement reached only via a sibling
caselabel's own value expression, and sloppy-modearguments[1] = truealiasing a named parameter — neither trackable by a text-only resolver without much deeper analysis), and collapsed several duplicated inline bracket-depth-counting character-set checks into one sharedbracketDeltahelper.Commit 6 — commit 5's local recovery still had one gap: it only triggered when a misread guess hit a line boundary (
\n/\r). A guessed regex that ran all the way to the end of input with no trailing newline — the last line of a script — never hit that trigger, so it fell through to the old whole-input bail after all. Reproduced directly ('const l = "clo}se";\nconst ratio = {}/2;', no trailing newline, returned completely unmasked) and end-to-end (the same shape immediately before</script>, no newline in between, produced no finding). Fixed by treating end-of-input exactly like a line boundary: reaching it while still holding an open guess now runs the identical recovery instead of exiting the scan loop. Since every exit from the scan now requires the guess to already be resolved, the function's own fail-safe check no longer needs to test for one — only a genuinely unterminated string/template literal can still trigger it.Also this commit: an EOF-no-trailing-newline fixture, a length-preservation regression test across a misread immediately followed by a real confirmed regex (guards
recoverFromMisreadactually clearing its buffered content — verified this test does fail if that clear is dropped), and an array-literal-call-argument fixture (guardsbracketDeltacorrectly counting[/]when splitting call arguments — verified this test does fail if that handling is dropped), plus a stale comment fix.Test plan
gsap.test.ts: 231/231 passing.utils.test.ts: 38/38 passing (new coverage for the local-recovery mechanism: reconstruction when nothing is lost, real masking of a literal adjacent to a misread, an unrelated string staying masked whether the ambiguity sits before or after it, the escaped-quote-before-bracket case, end-of-input with no trailing newline, and output-length preservation across a misread immediately followed by a real regex).packages/lintpackage suite: 644/644 passing. Four suites (hevcPreviewLint.test.ts,hevcPreviewLint.windowsHide.test.ts,project.test.ts,snippetFragment.test.ts) fail to even load in this specific sandbox on anode:pathposix-named-export interop error inside@hyperframes/parsers— confirmed environment-specific (does not reproduce elsewhere) and unrelated to this diff (none of the four import anything this PR touches).bun run test(not vitest watch, freshly rebuilt package) onregistryComponents.test.ts(410 components): ~6.9-7.3s after this commit, versus ~6.4-6.5s before commit 4 ever introduced the regression and ~9.3-10.5s at commit 4/head before commit 5's fix.tsc --noEmit,oxlint,oxfmt --checkclean on all four touched files.🤖 Generated with Claude Code