From cffa4e2f6a7406a4777ee3fa05cdbba0939127fe Mon Sep 17 00:00:00 2001 From: miga-heygen Date: Tue, 15 Sep 2026 00:09:35 +0000 Subject: [PATCH 1/6] fix: gsap_callback_dom_measurement no longer taints every caller of a 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 --- packages/lint/src/rules/gsap.test.ts | 150 ++++++++++++++++ packages/lint/src/rules/gsap.ts | 257 ++++++++++++++++++++++++--- 2 files changed, 385 insertions(+), 22 deletions(-) diff --git a/packages/lint/src/rules/gsap.test.ts b/packages/lint/src/rules/gsap.test.ts index 687b36d2e6..fbb3d633f0 100644 --- a/packages/lint/src/rules/gsap.test.ts +++ b/packages/lint/src/rules/gsap.test.ts @@ -2879,6 +2879,156 @@ describe("GSAP seek-order safety rules", () => { const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); expect(finding).toBeUndefined(); }); + + it("gsap_callback_dom_measurement: flags a shared helper's measuring branch but clears its non-measuring branch (boolean)", async () => { + // Distilled from a production report: a shared style-update helper's `if` + // branch measures, its `else` branch only writes style. A caller passing a + // literal `true`/`false` for that branch's own parameter should be judged + // by which branch it actually reaches, not by "does the helper measure ANYWHERE." + const html = ` + +
+
+
+
+ +`; + const result = await lintHyperframeHtml(html); + const findings = result.findings.filter((f) => f.code === "gsap_callback_dom_measurement"); + expect(findings.length).toBe(1); + expect(findings[0]?.selector).toContain("cardA"); + }); + + it("gsap_callback_dom_measurement: resolves a switch/string-mode shared helper's literal branch the same way", async () => { + const html = ` + +
+
+
+
+ +`; + const result = await lintHyperframeHtml(html); + const findings = result.findings.filter((f) => f.code === "gsap_callback_dom_measurement"); + expect(findings.length).toBe(1); + expect(findings[0]?.selector).toContain("cardA"); + }); + + it("gsap_callback_dom_measurement: does NOT flag a bare, uncalled mention of a measuring function's name", async () => { + // A callback that merely references a tainted helper's identifier (no + // invocation) never reaches its measurement — unlike the transitive-call + // propagation between named functions, which already requires a call. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeUndefined(); + }); + + it("gsap_callback_dom_measurement: still conservatively flags a shared-helper call whose branch argument isn't a literal", async () => { + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a comma inside a string argument doesn't misalign later literal arguments", async () => { + // A string argument whose text itself contains a bare "true"/"false" + // token between commas (e.g. "x, true, y") must not let a naive + // (non-string-aware) comma split misread that inner token as a LATER + // param's literal — here `useMeasurement` is really `false` (real + // branch: getBoundingClientRect, must flag), but a split that doesn't + // track string boundaries produces a stray " true" fragment that lands + // on useMeasurement's index and incorrectly clears this call. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); }); describe("SVG draw-on rules", () => { diff --git a/packages/lint/src/rules/gsap.ts b/packages/lint/src/rules/gsap.ts index e1abb94f6a..431da61bb2 100644 --- a/packages/lint/src/rules/gsap.ts +++ b/packages/lint/src/rules/gsap.ts @@ -884,17 +884,73 @@ function collectTimelineVarNames(source: string): string[] { .filter(Boolean); } -// Named function bodies in a script (declarations plus `const f = ...` function +type FunctionSignature = { params: Array; body: string }; + +/** Split `text` on top-level commas — depth-aware, so a comma inside `(...)`/`[...]`/`{...}` doesn't split. */ +function splitTopLevelByComma(text: string): string[] { + const parts: string[] = []; + let depth = 0; + let start = 0; + let inString: '"' | "'" | "`" | null = null; + for (let i = 0; i < text.length; i++) { + const ch = text[i] ?? ""; + const prev = text[i - 1] ?? ""; + if (inString) { + if (ch === inString && prev !== "\\") inString = null; + continue; + } + if (ch === '"' || ch === "'" || ch === "`") { + inString = ch; + continue; + } + if ("({[".includes(ch)) depth++; + else if (")}]".includes(ch)) depth--; + else if (ch === "," && depth === 0) { + parts.push(text.slice(start, i)); + start = i + 1; + } + } + parts.push(text.slice(start)); + return parts; +} + +/** Parameter names of a parameter list (without its parentheses); null per destructured/rest param. */ +function paramNames(paramList: string): Array { + return splitTopLevelByComma(paramList).map(normalizeFirstParam); +} + +// Parameter names for a `const name = ...` prefix already matched by +// `assignPattern` below — mirrors that pattern's own three shapes +// (function expression / parenthesized arrow / bare-identifier arrow), the +// same three shapes `parseFunctionValueSource` also matches independently. +// If a shape is ever added to one, it won't automatically apply to the +// others; an unmatched shape here just yields `[]` (params unresolved), +// which falls back to the old conservative whole-function taint rather than +// silently misbehaving. +function extractAssignedParamNames(prefix: string): Array { + const match = + prefix.match(/function\b[^(]*\(([^)]*)\)/) ?? + prefix.match(/\(([^)]*)\)\s*=>\s*$/) ?? + prefix.match(/([A-Za-z_$][\w$]*)\s*=>\s*$/); + return match ? paramNames(match[1] ?? "") : []; +} + +// Named function signatures in a script (declarations plus `const f = ...` function // expressions and arrows). Expression-bodied arrows keep their single line. -function collectNamedFunctionBodies(source: string): Map { - const bodies = new Map(); +function collectNamedFunctionSignatures(source: string): Map { + const signatures = new Map(); const declPattern = /(?:^|[^.\w$])function\s+([A-Za-z_$][\w$]*)\s*\(/g; let match: RegExpExecArray | null; while ((match = declPattern.exec(source)) !== null) { - const braceIndex = source.indexOf("{", declPattern.lastIndex); + const parenIndex = declPattern.lastIndex - 1; + const paramsWithParens = matchBalanced(source, parenIndex, "(", ")"); + if (!paramsWithParens) continue; + const braceIndex = source.indexOf("{", parenIndex + paramsWithParens.length); if (braceIndex < 0) continue; const body = matchBalanced(source, braceIndex, "{", "}"); - if (body) bodies.set(match[1] ?? "", body); + if (!body) continue; + const params = paramNames(paramsWithParens.slice(1, -1)); + signatures.set(match[1] ?? "", { params, body }); } const assignPattern = /(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*(?:async\s*)?(?:function\b[^{]*|\([^)]*\)\s*=>\s*|[A-Za-z_$][\w$]*\s*=>\s*)/g; @@ -904,39 +960,196 @@ function collectNamedFunctionBodies(source: string): Map { source[bodyStart] === "{" ? matchBalanced(source, bodyStart, "{", "}") : sliceExpression(source, bodyStart); - if (body) bodies.set(match[1] ?? "", body); + if (!body) continue; + const params = extractAssignedParamNames(match[0]); + signatures.set(match[1] ?? "", { params, body }); } - return bodies; + return signatures; +} + +/** Does `text` measure directly, or call an already-tainted function? Requires a call — a bare name mention is not enough. */ +function textReachesMeasurement(text: string, measuring: Set): boolean { + if (CALLBACK_MEASUREMENT_PATTERN.test(text)) return true; + for (const name of measuring) { + if (new RegExp(`\\b${escapeRegExp(name)}\\s*\\(`).test(text)) return true; + } + return false; } // Two-hop closure: functions whose body measures the DOM directly, plus // functions that call one of those (bounded fixpoint — no deep recursion). -function collectMeasuringFunctionNames(bodies: Map): Set { +function collectMeasuringFunctionNames(signatures: Map): Set { const measuring = new Set(); - for (const [name, body] of bodies) { + for (const [name, { body }] of signatures) { if (CALLBACK_MEASUREMENT_PATTERN.test(body)) measuring.add(name); } for (let pass = 0; pass < 3; pass++) { let grew = false; - for (const [name, body] of bodies) { + for (const [name, { body }] of signatures) { if (measuring.has(name)) continue; - for (const measured of measuring) { - if (new RegExp(`\\b${escapeRegExp(measured)}\\s*\\(`).test(body)) { - measuring.add(name); - grew = true; - break; - } - } + if (!textReachesMeasurement(body, measuring)) continue; + measuring.add(name); + grew = true; } if (!grew) break; } return measuring; } -function expressionReachesMeasurement(expression: string, measuring: Set): boolean { +type LiteralArg = { kind: "boolean"; value: boolean } | { kind: "string"; value: string }; + +function parseLiteralArg(argText: string): LiteralArg | null { + const trimmed = argText.trim(); + if (trimmed === "true") return { kind: "boolean", value: true }; + if (trimmed === "false") return { kind: "boolean", value: false }; + const stringMatch = trimmed.match(/^(["'])([^"'`]*)\1$/); + if (stringMatch) return { kind: "string", value: stringMatch[2] ?? "" }; + return null; +} + +/** Body text of the case matching `value` in a `switch (paramName)`, up to the next `case`/`default`. */ +function sliceUntilNextCase(text: string, start: number): string { + let depth = 0; + let inString: '"' | "'" | "`" | null = null; + for (let i = start; i < text.length; i++) { + const ch = text[i] ?? ""; + const prev = text[i - 1] ?? ""; + if (inString) { + if (ch === inString && prev !== "\\") inString = null; + continue; + } + if (ch === '"' || ch === "'" || ch === "`") { + inString = ch; + continue; + } + if ("({[".includes(ch)) depth++; + else if (")}]".includes(ch)) { + if (depth === 0) return text.slice(start, i); + depth--; + } else if (depth === 0 && /^(?:case|default)\b/.test(text.slice(i))) { + return text.slice(start, i); + } + } + return text.slice(start); +} + +// A branch is only decisive when nothing OUTSIDE the branching statement (the +// half-open span `[start, end)` of `body`) measures: an unconditional +// measurement elsewhere in the function applies whichever branch a call took. +function measuresOutsideStatement( + body: string, + start: number, + end: number, + measuring: Set, +): boolean { + return textReachesMeasurement(body.slice(0, start) + body.slice(end), measuring); +} + +// A literal boolean argument resolves `if (paramName) {A} else {B}` (or its +// negation) to exactly one branch. +function resolveBooleanIfElseBranch( + body: string, + paramName: string, + value: boolean, + measuring: Set, +): string | null { + const ifPattern = new RegExp(`\\bif\\s*\\(\\s*(!)?\\s*${escapeRegExp(paramName)}\\s*\\)\\s*\\{`); + const ifMatch = ifPattern.exec(body); + if (!ifMatch) return null; + const thenBraceIndex = ifMatch.index + ifMatch[0].length - 1; + const thenBlock = matchBalanced(body, thenBraceIndex, "{", "}"); + if (!thenBlock) return null; + const elseMatch = /^\s*else\s*\{/.exec(body.slice(thenBraceIndex + thenBlock.length)); + if (!elseMatch) return null; + const elseBraceIndex = thenBraceIndex + thenBlock.length + elseMatch[0].length - 1; + const elseBlock = matchBalanced(body, elseBraceIndex, "{", "}"); + if (!elseBlock) return null; + const statementEnd = elseBraceIndex + elseBlock.length; + if (measuresOutsideStatement(body, ifMatch.index, statementEnd, measuring)) return null; + const conditionHolds = ifMatch[1] === "!" ? !value : value; + return conditionHolds ? thenBlock : elseBlock; +} + +// Same idea for `switch (paramName) { case "value": ... }` — only resolved +// when the literal's case doesn't fall through into the next case's body. +function resolveSwitchCaseBranch( + body: string, + paramName: string, + value: string, + measuring: Set, +): string | null { + const switchPattern = new RegExp(`\\bswitch\\s*\\(\\s*${escapeRegExp(paramName)}\\s*\\)\\s*\\{`); + const switchMatch = switchPattern.exec(body); + if (!switchMatch) return null; + const braceIndex = switchMatch.index + switchMatch[0].length - 1; + const switchBody = matchBalanced(body, braceIndex, "{", "}"); + if (!switchBody) return null; + const casePattern = new RegExp(`case\\s*(["'])${escapeRegExp(value)}\\1\\s*:`); + const caseMatch = casePattern.exec(switchBody); + if (!caseMatch) return null; + const caseBody = sliceUntilNextCase(switchBody, caseMatch.index + caseMatch[0].length); + if (!caseBody.trim()) return null; // fell through with no body of its own — unresolved + const statementEnd = braceIndex + switchBody.length; + if (measuresOutsideStatement(body, switchMatch.index, statementEnd, measuring)) return null; + return caseBody; +} + +// Resolves whether ONE specific call to a tainted helper reaches measurement, +// by substituting any literal argument into the helper's own param-keyed +// branch. Returns null when the call can't be resolved this way (non-literal +// argument, or no recognized branch shape) — callers fall back to the old, +// conservative whole-function taint in that case. +function resolveCallSiteMeasurement( + calleeName: string, + argsText: string, + signatures: Map, + measuring: Set, +): boolean | null { + const signature = signatures.get(calleeName); + if (!signature) return null; + const args = splitTopLevelByComma(argsText); + for (let i = 0; i < signature.params.length; i++) { + const paramName = signature.params[i]; + if (!paramName) continue; + const literal = parseLiteralArg(args[i] ?? ""); + if (!literal) continue; + const branch = + literal.kind === "boolean" + ? resolveBooleanIfElseBranch(signature.body, paramName, literal.value, measuring) + : resolveSwitchCaseBranch(signature.body, paramName, literal.value, measuring); + if (branch !== null) return textReachesMeasurement(branch, measuring); + } + return null; +} + +// Deliberately narrower than the old bare-`\bname\b` check: a callback that +// passes a tainted name BY REFERENCE to something else that invokes it later +// (e.g. `onUpdate: () => setTimeout(measureFn, 0)`) is no longer flagged, +// since there's no actual call syntax in the callback's own text to resolve. +// That's the same "requires a call" standard the taint-propagation closure +// above already applies between named functions — this fix only makes the +// per-callback check consistent with it, at the cost of that narrow +// by-reference shape, in exchange for fixing the reported false positive +// (a bare, uncalled mention was enough to trip the rule). +function expressionReachesMeasurement( + expression: string, + measuring: Set, + signatures: Map, +): boolean { if (CALLBACK_MEASUREMENT_PATTERN.test(expression)) return true; for (const name of measuring) { - if (new RegExp(`\\b${escapeRegExp(name)}\\b`).test(expression)) return true; + const callPattern = new RegExp(`\\b${escapeRegExp(name)}\\s*\\(`, "g"); + let match: RegExpExecArray | null; + while ((match = callPattern.exec(expression)) !== null) { + const parenIndex = match.index + match[0].length - 1; + const argsWithParens = matchBalanced(expression, parenIndex, "(", ")"); + const resolved = argsWithParens + ? resolveCallSiteMeasurement(name, argsWithParens.slice(1, -1), signatures, measuring) + : null; + // null (unresolved) and true (resolved branch measures) both taint this + // callback; only a resolved, provably-safe branch (false) clears it. + if (resolved !== false) return true; + } } return false; } @@ -2222,8 +2435,8 @@ export const gsapRules: LintRule[] = [ for (const script of scripts) { const source = stripJsComments(script.content); if (!/gsap\.timeline/.test(source)) continue; - const bodies = collectNamedFunctionBodies(source); - const measuring = collectMeasuringFunctionNames(bodies); + const signatures = collectNamedFunctionSignatures(source); + const measuring = collectMeasuringFunctionNames(signatures); // A callback argument is hazardous when it is an inline function whose body // reaches a measurement, or a bare reference to a measuring function. Call @@ -2232,7 +2445,7 @@ export const gsapRules: LintRule[] = [ const callbackExpressionHazard = (expression: string): boolean => { const trimmed = expression.trim(); const inline = parseFunctionValueSource(trimmed); - if (inline) return expressionReachesMeasurement(inline.body, measuring); + if (inline) return expressionReachesMeasurement(inline.body, measuring, signatures); if (/^[A-Za-z_$][\w$]*$/.test(trimmed)) return measuring.has(trimmed); return false; }; From 3ea250c664ad9c51b931b2383cf26bec18302e26 Mon Sep 17 00:00:00 2001 From: miga-heygen Date: Tue, 15 Sep 2026 01:22:54 +0000 Subject: [PATCH 2/6] fix: close false-negative gaps in the gsap_callback_dom_measurement call-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 --- packages/lint/src/rules/gsap.test.ts | 420 +++++++++++++++++++++++++++ packages/lint/src/rules/gsap.ts | 89 +++++- 2 files changed, 507 insertions(+), 2 deletions(-) diff --git a/packages/lint/src/rules/gsap.test.ts b/packages/lint/src/rules/gsap.test.ts index fbb3d633f0..d3cd0930b9 100644 --- a/packages/lint/src/rules/gsap.test.ts +++ b/packages/lint/src/rules/gsap.test.ts @@ -3029,6 +3029,399 @@ describe("GSAP seek-order safety rules", () => { const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); expect(finding).toBeDefined(); }); + + it("gsap_callback_dom_measurement: a param reassigned before the branch is not substituted (fails closed)", async () => { + // useMeasurement is flipped before the `if`, so the caller's literal + // `false` no longer reflects what the branch actually tests (it becomes + // `true`, which measures). A naive substitution of the caller's raw + // literal would resolve the (wrong) non-measuring branch and clear this + // — the fix must detect the reassignment and fall back to conservative. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a param shadowed by a block-scoped let/const/var before the branch is not substituted (fails closed)", async () => { + // The `if (useMeasurement)` inside the block reads the block-scoped + // `let useMeasurement` shadow (always undefined/falsy), NOT the outer + // parameter — so its branch is fixed at runtime regardless of the + // caller's argument (always takes the else branch here, which measures). + // Naively substituting the caller's literal `true` for the (wrongly + // assumed to be) same binding would resolve the then-branch + // (non-measuring) and incorrectly clear this call. Deliberately no + // initializer (`let useMeasurement;`, not `= false`): an initializer like + // `= false` would ALSO match the plain-reassignment check on its own + // (`useMeasurement =` looks like a reassignment regardless of the `let` + // in front of it), so it wouldn't isolate the redeclaration guard + // specifically. Unlike an unconditional measurement elsewhere in the + // function, this isn't caught by the "measures outside the statement" + // check either — the only measurement is INSIDE this very if/else. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a param shadowed by a nested arrow's own parameter is not substituted (fails closed)", async () => { + // The IIFE always runs with a fixed `false` — real behavior (always + // measures) is independent of what the caller passes to applyPhaseStyle. + // The only textual `if (useMeasurement)` in the body belongs to the + // arrow's own shadowed parameter; naively substituting the caller's + // literal `true` for it would resolve the non-measuring branch and + // incorrectly clear. (Deliberately an IIFE, not a separately-named + // helper: a named helper gets its own whole-function-taint entry and + // its call site would be caught by the "measures outside the statement" + // guard regardless of shadow detection, which wouldn't isolate this + // guard specifically.) + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a param shadowed by a nested function's own parameter is not substituted (fails closed)", async () => { + // Same idea as the nested-arrow case above, using an IIFE'd `function` + // expression's parameter instead of an arrow's. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a param shadowed by a destructured binding before the branch is not substituted (fails closed)", async () => { + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a fall-through case (no break) is not resolved as just its own body (fails closed)", async () => { + // Case "safe" has no break, so it falls through into case "measure" at + // runtime — the real behavior for mode "safe" includes the measurement. + // Resolving it as just its own (non-measuring) slice would clear it. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a 'break' inside a string literal doesn't look like a real terminator", async () => { + // Case "safe" has no ACTUAL break — `el.setAttribute('data-mode', 'break')` + // just happens to contain the word "break" inside a string argument — so + // it still falls through into case "measure" at runtime. A terminator + // check that isn't string-literal-aware would be fooled by that + // substring into treating "safe" as validly terminated. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a param shadowed by an object method shorthand's own parameter is not substituted (fails closed)", async () => { + // `handlers.run` is an object-literal shorthand method whose OWN + // parameter shadows the outer `useMeasurement` — unlike a `function` + // declaration or an arrow, a shorthand method has neither the `function` + // keyword nor `=>`, so it needs its own shadow-detection pattern. The + // IIFE-style immediate call (rather than a separately named/exported + // helper) keeps this isolated from the "measures outside the statement" + // guard, same reasoning as the nested-arrow/nested-function tests above. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a param shadowed by a catch clause's own parameter is not substituted (fails closed)", async () => { + // The caught error object is always truthy, so `if (useMeasurement)` + // ALWAYS takes the measuring branch here, regardless of what the caller + // passes for the outer `useMeasurement` parameter. Naively substituting + // the caller's literal `false` would resolve the non-measuring else + // branch and incorrectly clear a call that always measures. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a case/default match doesn't fire mid-identifier", async () => { + // "breakcase" contains "case" as a suffix. Without a leading word-boundary + // check, the case-body slice truncates right there — and because the cut + // lands immediately after "break" (the "break" prefix of "breakcase"), + // the truncated fragment spuriously LOOKS like a valid `...break` ending, + // so it isn't caught by the separate no-fallthrough terminator check + // either. The real getBoundingClientRect() call (after the "breakcase" + // declaration) is silently dropped from the analyzed case body. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: an unconditional measurement outside the branch taints every call", async () => { + // getBoundingClientRect() runs unconditionally before the if/else, so + // both branches are effectively measuring regardless of the argument. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: an empty fall-through case body (no statements of its own) is unresolved", async () => { + // case "a" has no body of its own before falling into case "b" — this is + // distinct from the "no break" test above (which has a non-empty body). + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a negated if/else (if (!param)) resolves each literal to the correct branch", async () => { + const html = ` + +
+
+
+
+ +`; + const result = await lintHyperframeHtml(html); + const findings = result.findings.filter((f) => f.code === "gsap_callback_dom_measurement"); + expect(findings.length).toBe(1); + expect(findings[0]?.selector).toContain("cardA"); + }); }); describe("SVG draw-on rules", () => { @@ -3877,3 +4270,30 @@ describe("SVG draw-on rules", () => { }); }); }); + +describe("SCRATCH adversarial probe 3", () => { + it("mid-identifier case match still fires when preceded by a unicode identifier character", async () => { + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); +}); diff --git a/packages/lint/src/rules/gsap.ts b/packages/lint/src/rules/gsap.ts index 431da61bb2..aadaba502d 100644 --- a/packages/lint/src/rules/gsap.ts +++ b/packages/lint/src/rules/gsap.ts @@ -998,6 +998,36 @@ function collectMeasuringFunctionNames(signatures: Map>>=|>>=|&=|\\|=|\\^=|\\|\\|=|&&=|\\?\\?=|\\+\\+|--)` + + `|(?:\\+\\+|--)\\s*${name}\\b`, + ); + const inParamList = `\\([^)]*\\b${name}\\b[^)]*\\)`; + const shadowPattern = new RegExp( + [ + `\\bfunction\\b[^(]*${inParamList}`, // nested function's parameter + `${inParamList}\\s*=>`, // arrow's parenthesized parameter + `\\b${name}\\s*=>`, // arrow's bare single parameter + `\\b(?:let|const|var)\\s+${name}\\b`, // redeclaration + `[{[][^}\\]]*\\b${name}\\b[^}\\]]*[}\\]]\\s*=`, // destructured binding + `\\bcatch\\s*${inParamList}`, // catch clause parameter + // Object/class method shorthand parameter (`run(paramName) {`) — has + // neither `function` nor `=>`, so it needs its own alternative. The + // control-flow keywords are excluded so an EARLIER, unrelated + // if/while/switch/for/catch using the same real (non-shadowed) name + // doesn't force a gratuitous bail. + `\\b(?!if\\b|while\\b|switch\\b|for\\b|catch\\b|function\\b)[A-Za-z_$][\\w$]*\\s*${inParamList}\\s*\\{`, + ].join("|"), + ); + return assignmentPattern.test(before) || shadowPattern.test(before); +} + // A literal boolean argument resolves `if (paramName) {A} else {B}` (or its // negation) to exactly one branch. function resolveBooleanIfElseBranch( @@ -1056,6 +1127,7 @@ function resolveBooleanIfElseBranch( const ifPattern = new RegExp(`\\bif\\s*\\(\\s*(!)?\\s*${escapeRegExp(paramName)}\\s*\\)\\s*\\{`); const ifMatch = ifPattern.exec(body); if (!ifMatch) return null; + if (paramMayBeRebound(body, paramName, ifMatch.index)) return null; const thenBraceIndex = ifMatch.index + ifMatch[0].length - 1; const thenBlock = matchBalanced(body, thenBraceIndex, "{", "}"); if (!thenBlock) return null; @@ -1081,6 +1153,7 @@ function resolveSwitchCaseBranch( const switchPattern = new RegExp(`\\bswitch\\s*\\(\\s*${escapeRegExp(paramName)}\\s*\\)\\s*\\{`); const switchMatch = switchPattern.exec(body); if (!switchMatch) return null; + if (paramMayBeRebound(body, paramName, switchMatch.index)) return null; const braceIndex = switchMatch.index + switchMatch[0].length - 1; const switchBody = matchBalanced(body, braceIndex, "{", "}"); if (!switchBody) return null; @@ -1088,7 +1161,19 @@ function resolveSwitchCaseBranch( const caseMatch = casePattern.exec(switchBody); if (!caseMatch) return null; const caseBody = sliceUntilNextCase(switchBody, caseMatch.index + caseMatch[0].length); - if (!caseBody.trim()) return null; // fell through with no body of its own — unresolved + // Both guards below mean the same thing: the matched case falls through into + // the NEXT case at runtime, which this slice does not include and which may + // measure — so it can't be resolved as "just this case's body." + const trimmedCaseBody = caseBody.trim(); + if (!trimmedCaseBody) return null; // no body of its own + // The trailing `\}?` tolerates one level of the case's own `{ ... }` block + // wrapping (`case "x": { ...; break; }`), which the slice keeps verbatim. + // Masked first so a keyword-shaped SUBSTRING inside a string literal (e.g. + // `el.setAttribute("mode", "break")`) can't be mistaken for a real + // terminator. + if (!/\b(?:break|return|throw)\b[^;{}]*;?\s*\}?\s*$/.test(maskStringLiterals(trimmedCaseBody))) { + return null; + } const statementEnd = braceIndex + switchBody.length; if (measuresOutsideStatement(body, switchMatch.index, statementEnd, measuring)) return null; return caseBody; From 1a8af7759ddf284339ad843e020dde48ae7df6f1 Mon Sep 17 00:00:00 2001 From: miga-heygen Date: Tue, 15 Sep 2026 06:29:56 +0000 Subject: [PATCH 3/6] fix: close remaining false-negative gaps and unify string scanning in 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 --- packages/lint/src/rules/gsap.test.ts | 456 +++++++++++++++++++++++++-- packages/lint/src/rules/gsap.ts | 339 ++++++++++++++------ 2 files changed, 674 insertions(+), 121 deletions(-) diff --git a/packages/lint/src/rules/gsap.test.ts b/packages/lint/src/rules/gsap.test.ts index d3cd0930b9..cd562d518b 100644 --- a/packages/lint/src/rules/gsap.test.ts +++ b/packages/lint/src/rules/gsap.test.ts @@ -3422,6 +3422,435 @@ describe("GSAP seek-order safety rules", () => { expect(findings.length).toBe(1); expect(findings[0]?.selector).toContain("cardA"); }); + + it("gsap_callback_dom_measurement: a break nested inside a conditional does not terminate the case (fails closed)", async () => { + // The `break` only fires when `window.__cond` is true — it does NOT + // unconditionally end case "a", so the case can still fall through into + // case "b"'s measurement. A terminator check that only looks at the + // TRAILING text (not whether the terminator sits at the case's own + // top level) would be fooled by the nested `break` into treating "a" as + // safely terminated. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a case search is depth-aware and doesn't match a same-valued case nested in a sibling case (fails closed)", async () => { + // The REAL case "b" (top-level, measures) sits after case "a", which + // itself contains an unrelated NESTED switch with its own (non-measuring) + // case "b". A depth-unaware search for `case "b":` would match the + // nested one first — the first TEXTUAL occurrence — and wrongly resolve + // using its body instead of the real top-level case's. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a literal with no matching TOP-LEVEL case anywhere in the switch bails closed (every occurrence is nested)", async () => { + // Every textual occurrence of `case "b":` is nested inside case "a"'s own + // inner switch — there is no top-level case "b" at all. findTopLevelCaseMatch + // must reject every candidate and return null, and the caller must treat + // that as unresolved (conservative) rather than as "case not found, so + // nothing to worry about." The measurement lives in case "c" (unreached + // by this call's "b" argument) purely so `alignPanel` enters the + // `measuring` set to begin with — without ANY measurement anywhere in the + // body, the function is never tainted in the first place and this + // resolution path never runs. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a spread argument before a literal is not substituted (fails closed)", async () => { + // `...args` can expand to any number of elements at runtime, so the + // textually-later `false` doesn't reliably bind to `useMeasurement` — + // resolving it anyway (as if positions were unaffected by the spread) + // could substitute the wrong branch. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a rebind inside a loop-wrapped branch is not substituted, even though it's textually after the if (fails closed)", async () => { + // On the first loop iteration `useMeasurement` is false (style only), but + // the else branch flips it to true — so the SECOND iteration's `if` + // measures. A rebind scan limited to the text BEFORE the if misses this, + // since the reassignment sits inside the else branch, textually after. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a param shadowed by a nested function DECLARATION reusing its name is not substituted (fails closed)", async () => { + // `function useMeasurement() {}` re-declares the identifier inside the + // function's own scope — the shadow patterns for nested parameters + // (`function foo(useMeasurement)`) don't cover a nested function whose + // own NAME, not parameter, reuses the outer param. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a default parameter value containing '(' doesn't throw off body extraction for an unrelated helper", async () => { + // `a = "("` puts an unbalanced `(` inside a string default value — + // a matchBalanced that isn't string-literal-aware over-counts depth and + // mis-locates (or fails to find) this function's own body, so it's never + // collected as measuring and a caller invoking it is missed entirely. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: measureFn.call/.apply/.bind() are direct invocations and must still be flagged", async () => { + const html = ` + +
+
+
+
+
+ +`; + const result = await lintHyperframeHtml(html); + const findings = result.findings.filter((f) => f.code === "gsap_callback_dom_measurement"); + expect(findings.length).toBe(3); + }); + + it("gsap_callback_dom_measurement: a named wrapper that reaches measurement only via .call/.apply/.bind is still flagged when passed by reference", async () => { + // Exercises the OTHER call/apply/bind path: not the leaf-callback check in + // expressionReachesMeasurement (covered above), but the two-hop named- + // function taint closure (collectMeasuringFunctionNames -> invokesName), + // where `wrapper` itself measures only indirectly through `.call`. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: measureFn?.() and measureFn?.call(this) (optional chaining) are still flagged (fails closed)", async () => { + const html = ` + +
+
+
+
+ +`; + const result = await lintHyperframeHtml(html); + const findings = result.findings.filter((f) => f.code === "gsap_callback_dom_measurement"); + expect(findings.length).toBe(2); + }); + + it("gsap_callback_dom_measurement: a for-of/for-in reuse of the param as the loop variable is not substituted (fails closed)", async () => { + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: an unrelated post-branch rebind with NO loop anywhere in the function is still conservatively flagged (documented, accepted trade-off)", async () => { + // paramMayBeRebound scans the WHOLE body unconditionally, on purpose: an + // earlier draft tried to narrow this to "only widen the scan when the + // function contains a loop/repetition construct," but two independent + // adversarial reviews each found a real repetition shape that narrower + // gate missed (see git history). Enumerating every way a branch might + // re-execute is an open-ended text-only classification problem, so this + // rule always takes the conservative path instead: an ordinary, + // unrelated reassignment after the branch — even with no loop anywhere + // in the function — still forces a bail to conservative flagging. A + // false positive here is tolerated by design; a false negative is not. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a rebind inside an Array#forEach/map-based repetition is not substituted (fails closed)", async () => { + // paramMayBeRebound's whole-body scan catches this regardless of which + // repetition construct (for/while/do, .forEach/.map, recursion, ...) the + // rebind sits inside — the scan doesn't try to classify the repetition + // shape at all (see the comment above), it just always looks at the + // whole body. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: does NOT flag a by-reference handoff to setTimeout or Array#forEach", async () => { + // Deliberate, documented scope trade: unlike .call/.apply/.bind (direct + // invocations, flagged above), passing a tainted name BY REFERENCE to + // something else that invokes it later has no call syntax in the + // callback's own text to resolve. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeUndefined(); + }); + + it("gsap_callback_dom_measurement: mid-identifier case match still fires when preceded by a unicode identifier character", async () => { + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); }); describe("SVG draw-on rules", () => { @@ -4270,30 +4699,3 @@ describe("SVG draw-on rules", () => { }); }); }); - -describe("SCRATCH adversarial probe 3", () => { - it("mid-identifier case match still fires when preceded by a unicode identifier character", async () => { - const html = ` - -
- -`; - const result = await lintHyperframeHtml(html); - const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); - expect(finding).toBeDefined(); - }); -}); diff --git a/packages/lint/src/rules/gsap.ts b/packages/lint/src/rules/gsap.ts index aadaba502d..7102eb5ffa 100644 --- a/packages/lint/src/rules/gsap.ts +++ b/packages/lint/src/rules/gsap.ts @@ -735,7 +735,50 @@ function targetsShareElement( return false; } -/** Source from the delimiter at `openIndex` to its matching closer, inclusive. */ +/** + * Where the character just consumed sits relative to string/template literals: + * `outside` = ordinary code, `open`/`close` = a literal's own delimiter, + * `content` = inside a literal (escape sequences included). + */ +type StringScanRole = "outside" | "open" | "content" | "close"; + +// Immutable: `advanceStringScan` returns a new state per character, and hands +// back the shared `OUTSIDE_STRING_SCAN_STATE` instance for ordinary code. +type StringScanState = { + readonly inString: '"' | "'" | "`" | null; + readonly escaped: boolean; + /** Role of the character just consumed; `outside` before the scan starts. */ + readonly role: StringScanRole; +}; + +const OUTSIDE_STRING_SCAN_STATE: StringScanState = { + inString: null, + escaped: false, + role: "outside", +}; + +// The single shared implementation of every string-literal-aware scan in this +// file: bracket depth, statement splitting and keyword searches all have to +// ignore text that is really just string content. Escapes are tracked with an +// explicit `escaped` flag rather than "is the previous character a backslash", +// so an escaped backslash right before the closing quote (`"a\\"`, i.e. the +// string `a\`) can't hide the real closing quote behind a literal backslash. +function advanceStringScan(state: StringScanState, ch: string): StringScanState { + if (state.inString) { + if (state.escaped) return { inString: state.inString, escaped: false, role: "content" }; + if (ch === "\\") return { inString: state.inString, escaped: true, role: "content" }; + if (ch === state.inString) return { inString: null, escaped: false, role: "close" }; + return { inString: state.inString, escaped: false, role: "content" }; + } + if (ch === '"' || ch === "'" || ch === "`") return { inString: ch, escaped: false, role: "open" }; + return OUTSIDE_STRING_SCAN_STATE; +} + +/** + * Source from the delimiter at `openIndex` to its matching closer, inclusive. + * String-aware: a `(`/`)`/`{`/`}` inside a string literal (e.g. a default + * value `a = "("`) doesn't count toward depth. + */ function matchBalanced( source: string, openIndex: number, @@ -743,8 +786,11 @@ function matchBalanced( close: string, ): string | null { let depth = 0; + let scan = OUTSIDE_STRING_SCAN_STATE; for (let i = openIndex; i < source.length; i++) { - const ch = source[i]; + const ch = source[i] ?? ""; + scan = advanceStringScan(scan, ch); + if (scan.role !== "outside") continue; if (ch === open) depth++; else if (ch === close) { depth--; @@ -770,19 +816,17 @@ function enclosingObjectLiteral(source: string, index: number): string | null { function objectLiteralHasTopLevelRelativeValue(objectLiteral: string): boolean { let depth = 0; - let inString: '"' | "'" | "`" | null = null; + let scan = OUTSIDE_STRING_SCAN_STATE; for (let i = 0; i < objectLiteral.length; i++) { const ch = objectLiteral[i] ?? ""; - const prev = objectLiteral[i - 1] ?? ""; - if (inString) { - if (ch === inString && prev !== "\\") inString = null; - continue; - } - if (ch === '"' || ch === "'" || ch === "`") { - inString = ch; + scan = advanceStringScan(scan, ch); + if (scan.role === "open") { + // A relative value is always a string (`x: "+=10"`), so the check runs + // at the opening quote of a property value directly inside the literal. if (depth === 1 && /^[+-]=/.test(objectLiteral.slice(i + 1))) return true; continue; } + if (scan.role !== "outside") continue; if (ch === "{" || ch === "(" || ch === "[") depth++; else if (ch === "}" || ch === ")" || ch === "]") depth--; } @@ -808,11 +852,17 @@ function isInsideGsapTweenVars(source: string, index: number, timelineVars: stri return false; } -/** An expression starting at `start`, ending at the first `,` / closer at depth 0. */ +/** + * An expression starting at `start`, ending at the first `,` / closer that sits + * at depth 0 outside any string literal. + */ function sliceExpression(source: string, start: number): string { let depth = 0; + let scan = OUTSIDE_STRING_SCAN_STATE; for (let i = start; i < source.length; i++) { const ch = source[i] ?? ""; + scan = advanceStringScan(scan, ch); + if (scan.role !== "outside") continue; if ("({[".includes(ch)) depth++; else if (")}]".includes(ch)) { if (depth === 0) return source.slice(start, i); @@ -891,18 +941,11 @@ function splitTopLevelByComma(text: string): string[] { const parts: string[] = []; let depth = 0; let start = 0; - let inString: '"' | "'" | "`" | null = null; + let scan = OUTSIDE_STRING_SCAN_STATE; for (let i = 0; i < text.length; i++) { const ch = text[i] ?? ""; - const prev = text[i - 1] ?? ""; - if (inString) { - if (ch === inString && prev !== "\\") inString = null; - continue; - } - if (ch === '"' || ch === "'" || ch === "`") { - inString = ch; - continue; - } + scan = advanceStringScan(scan, ch); + if (scan.role !== "outside") continue; if ("({[".includes(ch)) depth++; else if (")}]".includes(ch)) depth--; else if (ch === "," && depth === 0) { @@ -967,11 +1010,40 @@ function collectNamedFunctionSignatures(source: string): Map): boolean { if (CALLBACK_MEASUREMENT_PATTERN.test(text)) return true; for (const name of measuring) { - if (new RegExp(`\\b${escapeRegExp(name)}\\s*\\(`).test(text)) return true; + if (invokesName(text, name)) return true; } return false; } @@ -998,36 +1070,6 @@ function collectMeasuringFunctionNames(signatures: Map>>=|>>=|&=|\\|=|\\^=|\\|\\|=|&&=|\\?\\?=|\\+\\+|--)` + @@ -1100,11 +1151,17 @@ function paramMayBeRebound(body: string, paramName: string, beforeIndex: number) const shadowPattern = new RegExp( [ `\\bfunction\\b[^(]*${inParamList}`, // nested function's parameter + `\\bfunction\\s+${name}\\s*\\(`, // nested function DECLARATION reusing the name `${inParamList}\\s*=>`, // arrow's parenthesized parameter `\\b${name}\\s*=>`, // arrow's bare single parameter `\\b(?:let|const|var)\\s+${name}\\b`, // redeclaration `[{[][^}\\]]*\\b${name}\\b[^}\\]]*[}\\]]\\s*=`, // destructured binding `\\bcatch\\s*${inParamList}`, // catch clause parameter + // `for (name of ...)` / `for (name in ...)` reusing the param as the + // loop variable — reassigns on every iteration without ever matching + // the assignment or declaration alternatives above (no `=`, no + // let/const/var). + `\\bfor\\s*(?:await\\s+)?\\(\\s*${name}\\s+(?:of|in)\\s+`, // Object/class method shorthand parameter (`run(paramName) {`) — has // neither `function` nor `=>`, so it needs its own alternative. The // control-flow keywords are excluded so an EARLIER, unrelated @@ -1113,7 +1170,7 @@ function paramMayBeRebound(body: string, paramName: string, beforeIndex: number) `\\b(?!if\\b|while\\b|switch\\b|for\\b|catch\\b|function\\b)[A-Za-z_$][\\w$]*\\s*${inParamList}\\s*\\{`, ].join("|"), ); - return assignmentPattern.test(before) || shadowPattern.test(before); + return assignmentPattern.test(body) || shadowPattern.test(body); } // A literal boolean argument resolves `if (paramName) {A} else {B}` (or its @@ -1127,7 +1184,7 @@ function resolveBooleanIfElseBranch( const ifPattern = new RegExp(`\\bif\\s*\\(\\s*(!)?\\s*${escapeRegExp(paramName)}\\s*\\)\\s*\\{`); const ifMatch = ifPattern.exec(body); if (!ifMatch) return null; - if (paramMayBeRebound(body, paramName, ifMatch.index)) return null; + if (paramMayBeRebound(body, paramName)) return null; const thenBraceIndex = ifMatch.index + ifMatch[0].length - 1; const thenBlock = matchBalanced(body, thenBraceIndex, "{", "}"); if (!thenBlock) return null; @@ -1142,6 +1199,94 @@ function resolveBooleanIfElseBranch( return conditionHolds ? thenBlock : elseBlock; } +// Advances `{depth, scan}` by one character — depth only counts brackets found +// outside string literals. `findTopLevelCaseMatch` below drives this one +// character at a time, resuming between regex matches. +type DepthScanState = { depth: number; scan: StringScanState }; +function advanceDepthScan(state: DepthScanState, ch: string): DepthScanState { + const scan = advanceStringScan(state.scan, ch); + if (scan.role !== "outside") return { depth: state.depth, scan }; + let depth = state.depth; + if ("({[".includes(ch)) depth++; + else if (")}]".includes(ch)) depth--; + return { depth, scan }; +} + +// Finds `case "value":` at depth 0 of `switchBody` — i.e. belonging to THIS +// switch, not to a switch/if/object-literal nested inside an earlier case's +// own block. A plain (non-depth-aware) regex `.exec` would match the first +// TEXTUAL occurrence, which can be a same-valued case label nested inside a +// sibling case's inner switch. +function findTopLevelCaseMatch(switchBody: string, value: string): RegExpExecArray | null { + const casePattern = new RegExp(`case\\s*(["'])${escapeRegExp(value)}\\1\\s*:`, "g"); + // Starts at -1, not 0: `switchBody` itself begins with the switch's own + // wrapping `{`, so depth reaches 0 only once we're immediately inside it — + // that's what "top-level" (belonging to THIS switch) means here. + let state: DepthScanState = { depth: -1, scan: OUTSIDE_STRING_SCAN_STATE }; + // Scans forward only: each match resumes where the previous one stopped, so + // every character is still fed to the depth scan exactly once, in order. + let cursor = 0; + let match: RegExpExecArray | null; + while ((match = casePattern.exec(switchBody)) !== null) { + while (cursor < match.index) { + state = advanceDepthScan(state, switchBody[cursor] ?? ""); + cursor++; + } + if (state.depth === 0 && !state.scan.inString) return match; + } + return null; +} + +// Splits `text` into its top-level (depth-0) statements: `;`-terminated +// spans, and `{...}` BLOCK statements (if/for/while/switch/try/a bare block) +// — but not a `(...)`/`[...]` closing back to depth 0, which doesn't end a +// statement (e.g. an `if (x)` condition's own closing paren). +function splitTopLevelStatements(text: string): string[] { + const statements: string[] = []; + const stack: string[] = []; + let scan = OUTSIDE_STRING_SCAN_STATE; + let start = 0; + for (let i = 0; i < text.length; i++) { + const ch = text[i] ?? ""; + scan = advanceStringScan(scan, ch); + if (scan.role !== "outside") continue; + if (ch === "{" || ch === "(" || ch === "[") { + stack.push(ch); + } else if (ch === "}" || ch === ")" || ch === "]") { + const opener = stack.pop(); + if (stack.length === 0 && opener === "{" && ch === "}") { + statements.push(text.slice(start, i + 1)); + start = i + 1; + } + } else if (ch === ";" && stack.length === 0) { + statements.push(text.slice(start, i + 1)); + start = i + 1; + } + } + const rest = text.slice(start).trim(); + if (rest) statements.push(rest); + return statements; +} + +// True when `text`'s own LAST top-level statement is an unconditional +// break/return/throw. Checking the last STATEMENT (rather than the trailing +// text) is what keeps a conditional terminator — `if (x) { break; }`, which +// still falls through when `x` is false — from reading as termination of the +// case itself. When the case body is one whole wrapping block +// (`case "x": { ...; break; }`), the check recurses into that block's own +// last statement. +function lastStatementIsTerminator(text: string): boolean { + const statements = splitTopLevelStatements(text) + .map((s) => s.trim()) + .filter(Boolean); + const last = statements[statements.length - 1]; + if (!last) return false; + if (last.startsWith("{") && last.endsWith("}")) { + return lastStatementIsTerminator(last.slice(1, -1)); + } + return /^(?:break|return|throw)\b/.test(last); +} + // Same idea for `switch (paramName) { case "value": ... }` — only resolved // when the literal's case doesn't fall through into the next case's body. function resolveSwitchCaseBranch( @@ -1153,12 +1298,11 @@ function resolveSwitchCaseBranch( const switchPattern = new RegExp(`\\bswitch\\s*\\(\\s*${escapeRegExp(paramName)}\\s*\\)\\s*\\{`); const switchMatch = switchPattern.exec(body); if (!switchMatch) return null; - if (paramMayBeRebound(body, paramName, switchMatch.index)) return null; + if (paramMayBeRebound(body, paramName)) return null; const braceIndex = switchMatch.index + switchMatch[0].length - 1; const switchBody = matchBalanced(body, braceIndex, "{", "}"); if (!switchBody) return null; - const casePattern = new RegExp(`case\\s*(["'])${escapeRegExp(value)}\\1\\s*:`); - const caseMatch = casePattern.exec(switchBody); + const caseMatch = findTopLevelCaseMatch(switchBody, value); if (!caseMatch) return null; const caseBody = sliceUntilNextCase(switchBody, caseMatch.index + caseMatch[0].length); // Both guards below mean the same thing: the matched case falls through into @@ -1166,14 +1310,7 @@ function resolveSwitchCaseBranch( // measure — so it can't be resolved as "just this case's body." const trimmedCaseBody = caseBody.trim(); if (!trimmedCaseBody) return null; // no body of its own - // The trailing `\}?` tolerates one level of the case's own `{ ... }` block - // wrapping (`case "x": { ...; break; }`), which the slice keeps verbatim. - // Masked first so a keyword-shaped SUBSTRING inside a string literal (e.g. - // `el.setAttribute("mode", "break")`) can't be mistaken for a real - // terminator. - if (!/\b(?:break|return|throw)\b[^;{}]*;?\s*\}?\s*$/.test(maskStringLiterals(trimmedCaseBody))) { - return null; - } + if (!lastStatementIsTerminator(trimmedCaseBody)) return null; const statementEnd = braceIndex + switchBody.length; if (measuresOutsideStatement(body, switchMatch.index, statementEnd, measuring)) return null; return caseBody; @@ -1194,9 +1331,16 @@ function resolveCallSiteMeasurement( if (!signature) return null; const args = splitTopLevelByComma(argsText); for (let i = 0; i < signature.params.length; i++) { + const argText = (args[i] ?? "").trim(); + // A spread argument (`h(...args, false)`) makes every POSITIONAL argument + // from here on unreliable — the spread can expand to any number of + // elements at runtime, so a textually-later literal doesn't necessarily + // bind to the param at its textual index. Positions before the spread are + // unaffected and have already been resolved by the iterations above. + if (argText.startsWith("...")) break; const paramName = signature.params[i]; if (!paramName) continue; - const literal = parseLiteralArg(args[i] ?? ""); + const literal = parseLiteralArg(argText); if (!literal) continue; const branch = literal.kind === "boolean" @@ -1207,15 +1351,17 @@ function resolveCallSiteMeasurement( return null; } -// Deliberately narrower than the old bare-`\bname\b` check: a callback that -// passes a tainted name BY REFERENCE to something else that invokes it later -// (e.g. `onUpdate: () => setTimeout(measureFn, 0)`) is no longer flagged, -// since there's no actual call syntax in the callback's own text to resolve. -// That's the same "requires a call" standard the taint-propagation closure -// above already applies between named functions — this fix only makes the -// per-callback check consistent with it, at the cost of that narrow -// by-reference shape, in exchange for fixing the reported false positive -// (a bare, uncalled mention was enough to trip the rule). +// Requires a CALL, the same standard the taint-propagation closure above +// applies between named functions (see `invokesName`) — a bare, uncalled +// mention of a tainted name is not enough to trip the rule. +// +// Direct invocation forms (`measureFn()`, `measureFn.call(this)`, +// `.apply(...)`, `.bind(this)()`) are flagged: they run the tainted function +// synchronously, right here. A BY-REFERENCE handoff +// (`onUpdate: () => setTimeout(measureFn, 0)`, `[el].forEach(measureFn)`) is a +// deliberate, documented scope trade — there is no call syntax in the +// callback's own text to resolve, because the invocation (and whatever +// arguments it passes) happens inside setTimeout's/forEach's implementation. function expressionReachesMeasurement( expression: string, measuring: Set, @@ -1223,7 +1369,7 @@ function expressionReachesMeasurement( ): boolean { if (CALLBACK_MEASUREMENT_PATTERN.test(expression)) return true; for (const name of measuring) { - const callPattern = new RegExp(`\\b${escapeRegExp(name)}\\s*\\(`, "g"); + const callPattern = new RegExp(directCallPatternSource(name), "g"); let match: RegExpExecArray | null; while ((match = callPattern.exec(expression)) !== null) { const parenIndex = match.index + match[0].length - 1; @@ -1235,6 +1381,11 @@ function expressionReachesMeasurement( // callback; only a resolved, provably-safe branch (false) clears it. if (resolved !== false) return true; } + // `.call`/`.apply`/`.bind` are direct invocations too, but their argument + // shape (a leading `this`, or a further `()` for bind) doesn't match the + // plain-call branch-resolution shape above — always conservative here, + // same as an unresolved plain call would be. + if (invokesNameViaCallApplyBind(expression, name)) return true; } return false; } From a7e212df2b55347394421da8a600b27f3b5cd950 Mon Sep 17 00:00:00 2001 From: miga-heygen Date: Tue, 15 Sep 2026 07:20:10 +0000 Subject: [PATCH 4/6] fix: delegate gsap_callback_dom_measurement's string scanning to the existing regex-aware utility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- packages/lint/src/rules/gsap.test.ts | 128 ++++++++++++++ packages/lint/src/rules/gsap.ts | 240 +++++++++++++-------------- 2 files changed, 247 insertions(+), 121 deletions(-) diff --git a/packages/lint/src/rules/gsap.test.ts b/packages/lint/src/rules/gsap.test.ts index cd562d518b..eab58dbf1c 100644 --- a/packages/lint/src/rules/gsap.test.ts +++ b/packages/lint/src/rules/gsap.test.ts @@ -3640,6 +3640,134 @@ describe("GSAP seek-order safety rules", () => { expect(finding).toBeDefined(); }); + it("gsap_callback_dom_measurement: a regex literal containing a quote (e.g. a sanitizer /'/g) doesn't desync bracket matching", async () => { + // A quote character inside a regex literal (a very common sanitizer + // shape) used to open a string that never closed under the prior + // hand-rolled string scanner, since it only recognized `"`/`'`/`` ` `` + // delimiters and had no concept of regex literals at all — matchBalanced + // then silently failed to find the function's closing brace, dropping it + // from the taint set entirely. Delegating to the shared + // stripJsStringLiterals utility (which already disambiguates regex + // literals from division and blanks their contents) fixes this class of + // bug for every bracket-depth-counting helper in this file at once. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: a case-label-shaped substring inside a template literal at depth 0 is not mistaken for the real case", async () => { + // findTopLevelCaseMatch's label regex runs against the ORIGINAL text (it + // has to read the quoted value literally), so a template literal like + // `` label = `case "b":` `` sitting at the switch's own top level (no + // enclosing brackets of its own) reads as depth 0 — same as the genuine + // `case "b":` label — and bracket depth alone can't tell them apart. + // Requiring the match SITE itself to be unmasked (masked[match.index] + // must still read "c", not a blanked space) rejects the bogus match. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: an unrelated regex/division ambiguity EARLIER in the script doesn't poison masking for a later, unrelated function", async () => { + // stripJsStringLiterals has its own documented fail-safe: if it can't + // resolve a regex-vs-division ambiguity anywhere in the text it scans, it + // returns that ENTIRE input untouched (fully unmasked), not just a + // locally-degraded region. `const ratio = {}/2;` is ordinary, valid JS + // that trips this (a `/` right after `}` defaults to "regex allowed", + // finds no closing `/` before the line ends, and gives up). Naively + // masking the WHOLE script for every matchBalanced/sliceExpression call + // would let that one unrelated line silently disable masking — and thus + // reintroduce the exact "unmasked string content corrupts bracket + // counting" bug this refactor exists to fix — for every OTHER function in + // the same script, including ones with nothing to do with the trigger. + // matchBalanced/sliceExpression now mask only source.slice(fromIndex), + // not the whole script, so a trigger textually BEFORE the region a call + // actually needs can no longer poison it. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + + it("gsap_callback_dom_measurement: an escaped backslash right before a string's closing quote doesn't mis-locate the closing quote", async () => { + // `"a\\"` (the string `a\`) has a literal backslash immediately before + // the real closing quote. Escape tracking based on "is the previous + // character a backslash" misreads this as an escaped closing quote, + // extending the string past its real end and potentially corrupting + // downstream branch resolution. stripJsStringLiterals tracks a true + // escaped flag (odd/even backslash parity) rather than a naive + // prev-char check, so this resolves correctly. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeUndefined(); + }); + it("gsap_callback_dom_measurement: measureFn.call/.apply/.bind() are direct invocations and must still be flagged", async () => { const html = ` diff --git a/packages/lint/src/rules/gsap.ts b/packages/lint/src/rules/gsap.ts index 7102eb5ffa..15f0ab5781 100644 --- a/packages/lint/src/rules/gsap.ts +++ b/packages/lint/src/rules/gsap.ts @@ -39,6 +39,7 @@ import { readDecodedAttr, truncateSnippet, stripJsComments, + stripJsStringLiterals, hasCaptionStyles, WINDOW_TIMELINE_ASSIGN_PATTERN, TIMELINE_REGISTRY_OBJECT_LITERAL_PATTERN, @@ -736,48 +737,48 @@ function targetsShareElement( } /** - * Where the character just consumed sits relative to string/template literals: - * `outside` = ordinary code, `open`/`close` = a literal's own delimiter, - * `content` = inside a literal (escape sequences included). + * The structure-only view of `source`: string, template and regex literal + * CONTENTS blanked to spaces, with delimiters, length and newline positions + * preserved. Every text scanner below shares one shape — mask once, take each + * structural decision (bracket depth, keyword, delimiter) from `masked[i]`, and + * slice the ORIGINAL text, which the preserved length keeps index-aligned 1:1. + * + * The literal awareness itself is delegated, never re-derived here: + * `stripJsStringLiterals` already handles escape sequences and, crucially, + * regex-vs-division disambiguation — so a quote inside a sanitizer regex + * (`/'/g`) cannot open a string that never closes and silently desync every + * depth count downstream. + * + * Memoized on the last input: callers below re-mask the same script once per + * regex match, and `stripJsStringLiterals` is a character-at-a-time scan, so + * re-deriving it per match is quadratic in script size. */ -type StringScanRole = "outside" | "open" | "content" | "close"; - -// Immutable: `advanceStringScan` returns a new state per character, and hands -// back the shared `OUTSIDE_STRING_SCAN_STATE` instance for ordinary code. -type StringScanState = { - readonly inString: '"' | "'" | "`" | null; - readonly escaped: boolean; - /** Role of the character just consumed; `outside` before the scan starts. */ - readonly role: StringScanRole; -}; - -const OUTSIDE_STRING_SCAN_STATE: StringScanState = { - inString: null, - escaped: false, - role: "outside", -}; - -// The single shared implementation of every string-literal-aware scan in this -// file: bracket depth, statement splitting and keyword searches all have to -// ignore text that is really just string content. Escapes are tracked with an -// explicit `escaped` flag rather than "is the previous character a backslash", -// so an escaped backslash right before the closing quote (`"a\\"`, i.e. the -// string `a\`) can't hide the real closing quote behind a literal backslash. -function advanceStringScan(state: StringScanState, ch: string): StringScanState { - if (state.inString) { - if (state.escaped) return { inString: state.inString, escaped: false, role: "content" }; - if (ch === "\\") return { inString: state.inString, escaped: true, role: "content" }; - if (ch === state.inString) return { inString: null, escaped: false, role: "close" }; - return { inString: state.inString, escaped: false, role: "content" }; +let lastMaskedSource: string | null = null; +let lastMaskedResult = ""; +function maskLiterals(source: string): string { + if (source !== lastMaskedSource) { + lastMaskedResult = stripJsStringLiterals(source); + lastMaskedSource = source; } - if (ch === '"' || ch === "'" || ch === "`") return { inString: ch, escaped: false, role: "open" }; - return OUTSIDE_STRING_SCAN_STATE; + return lastMaskedResult; } /** * Source from the delimiter at `openIndex` to its matching closer, inclusive. - * String-aware: a `(`/`)`/`{`/`}` inside a string literal (e.g. a default - * value `a = "("`) doesn't count toward depth. + * Only REAL brackets count toward depth — one inside a string, template or + * regex literal (a default value `a = "("`, a sanitizer regex `/{/g`) has been + * masked out before the scan. + * + * Masks only `source.slice(openIndex)`, not the whole `source`, even though + * every caller already has the full script/body in hand: `stripJsStringLiterals` + * has its own documented fail-safe of returning its ENTIRE input untouched + * (fully unmasked) if regex-vs-division disambiguation goes wrong anywhere + * within it — one unrelated, ordinary line like `const ratio = {}/2;` + * *anywhere later in an unsliced whole script* could otherwise silently + * disable masking for every other call sharing that same source. Slicing to + * `openIndex` first — always a real bracket, itself a valid regex-preceding + * context either way — keeps that blast radius scoped to text this call + * actually needs, not unrelated code before it. */ function matchBalanced( source: string, @@ -785,91 +786,94 @@ function matchBalanced( open: string, close: string, ): string | null { + const relevant = source.slice(openIndex); + const masked = maskLiterals(relevant); let depth = 0; - let scan = OUTSIDE_STRING_SCAN_STATE; - for (let i = openIndex; i < source.length; i++) { - const ch = source[i] ?? ""; - scan = advanceStringScan(scan, ch); - if (scan.role !== "outside") continue; + for (let i = 0; i < relevant.length; i++) { + const ch = masked[i] ?? ""; if (ch === open) depth++; else if (ch === close) { depth--; - if (depth === 0) return source.slice(openIndex, i + 1); + if (depth === 0) return relevant.slice(0, i + 1); } } return null; } -/** The nearest object literal `{...}` enclosing `index` (comment-stripped source). */ -function enclosingObjectLiteral(source: string, index: number): string | null { +/** + * Index of the `{` opening the innermost object literal or block enclosing + * `index`, or -1 when `index` sits inside none. Scans masked text backwards, so + * a brace that is really literal content opens and closes nothing. + */ +function enclosingOpenBraceIndex(masked: string, index: number): number { let depth = 0; for (let i = index; i >= 0; i--) { - const ch = source[i]; + const ch = masked[i]; if (ch === "}") depth++; else if (ch === "{") { - if (depth === 0) return matchBalanced(source, i, "{", "}"); + if (depth === 0) return i; depth--; } } - return null; + return -1; +} + +/** The nearest object literal `{...}` enclosing `index` (comment-stripped source). */ +function enclosingObjectLiteral(source: string, index: number): string | null { + // Masks only the PREFIX up to `index` (text after it is irrelevant to a + // backward search) — same blast-radius reasoning as matchBalanced/ + // sliceExpression above. + const braceIndex = enclosingOpenBraceIndex(maskLiterals(source.slice(0, index + 1)), index); + if (braceIndex < 0) return null; + return matchBalanced(source, braceIndex, "{", "}"); } function objectLiteralHasTopLevelRelativeValue(objectLiteral: string): boolean { + const masked = maskLiterals(objectLiteral); let depth = 0; - let scan = OUTSIDE_STRING_SCAN_STATE; for (let i = 0; i < objectLiteral.length; i++) { - const ch = objectLiteral[i] ?? ""; - scan = advanceStringScan(scan, ch); - if (scan.role === "open") { - // A relative value is always a string (`x: "+=10"`), so the check runs - // at the opening quote of a property value directly inside the literal. + const ch = masked[i] ?? ""; + if (ch === '"' || ch === "'" || ch === "`") { + // Masking blanks literal contents but keeps their delimiters, so every + // quote still standing here is a genuine one. A relative value is always + // a string (`x: "+=10"`), and its contents have to be read back from the + // ORIGINAL text. Testing at closing quotes too is harmless: only an + // OPENING quote can be followed by `+=`/`-=`. if (depth === 1 && /^[+-]=/.test(objectLiteral.slice(i + 1))) return true; - continue; - } - if (scan.role !== "outside") continue; - if (ch === "{" || ch === "(" || ch === "[") depth++; - else if (ch === "}" || ch === ")" || ch === "]") depth--; + } else if ("({[".includes(ch)) depth++; + else if (")}]".includes(ch)) depth--; } return false; } function isInsideGsapTweenVars(source: string, index: number, timelineVars: string[]): boolean { - let depth = 0; - for (let i = index; i >= 0; i--) { - const ch = source[i]; - if (ch === "}") depth++; - else if (ch === "{") { - if (depth === 0) { - const before = source.slice(Math.max(0, i - 240), i).replace(/\s+/g, " "); - const receivers = ["gsap", ...timelineVars].map(escapeRegExp).join("|"); - return new RegExp(`(?:${receivers})\\.(?:set|to|from|fromTo|timeline)\\b[\\s\\S]*$`).test( - before, - ); - } - depth--; - } - } - return false; + const braceIndex = enclosingOpenBraceIndex(maskLiterals(source.slice(0, index + 1)), index); + if (braceIndex < 0) return false; + const before = source.slice(Math.max(0, braceIndex - 240), braceIndex).replace(/\s+/g, " "); + const receivers = ["gsap", ...timelineVars].map(escapeRegExp).join("|"); + return new RegExp(`(?:${receivers})\\.(?:set|to|from|fromTo|timeline)\\b[\\s\\S]*$`).test(before); } /** * An expression starting at `start`, ending at the first `,` / closer that sits - * at depth 0 outside any string literal. + * at depth 0 outside any literal. */ function sliceExpression(source: string, start: number): string { + // Sliced to `source.slice(start)` before masking, not the whole `source` — + // see matchBalanced's comment on why (shrinks stripJsStringLiterals's + // all-or-nothing fail-safe blast radius to text this call actually needs). + const relevant = source.slice(start); + const masked = maskLiterals(relevant); let depth = 0; - let scan = OUTSIDE_STRING_SCAN_STATE; - for (let i = start; i < source.length; i++) { - const ch = source[i] ?? ""; - scan = advanceStringScan(scan, ch); - if (scan.role !== "outside") continue; + for (let i = 0; i < relevant.length; i++) { + const ch = masked[i] ?? ""; if ("({[".includes(ch)) depth++; else if (")}]".includes(ch)) { - if (depth === 0) return source.slice(start, i); + if (depth === 0) return relevant.slice(0, i); depth--; - } else if (ch === "," && depth === 0) return source.slice(start, i); + } else if (ch === "," && depth === 0) return relevant.slice(0, i); } - return source.slice(start); + return relevant; } type ParsedFunctionValue = { firstParam: string | null; body: string }; @@ -938,14 +942,12 @@ type FunctionSignature = { params: Array; body: string }; /** Split `text` on top-level commas — depth-aware, so a comma inside `(...)`/`[...]`/`{...}` doesn't split. */ function splitTopLevelByComma(text: string): string[] { + const masked = maskLiterals(text); const parts: string[] = []; let depth = 0; let start = 0; - let scan = OUTSIDE_STRING_SCAN_STATE; for (let i = 0; i < text.length; i++) { - const ch = text[i] ?? ""; - scan = advanceStringScan(scan, ch); - if (scan.role !== "outside") continue; + const ch = masked[i] ?? ""; if ("({[".includes(ch)) depth++; else if (")}]".includes(ch)) depth--; else if (ch === "," && depth === 0) { @@ -1081,24 +1083,24 @@ function parseLiteralArg(argText: string): LiteralArg | null { /** Body text of the case matching `value` in a `switch (paramName)`, up to the next `case`/`default`. */ function sliceUntilNextCase(text: string, start: number): string { + const masked = maskLiterals(text); let depth = 0; - let scan = OUTSIDE_STRING_SCAN_STATE; for (let i = start; i < text.length; i++) { - const ch = text[i] ?? ""; - const prev = text[i - 1] ?? ""; - scan = advanceStringScan(scan, ch); - if (scan.role !== "outside") continue; + const ch = masked[i] ?? ""; + const prev = masked[i - 1] ?? ""; if ("({[".includes(ch)) depth++; else if (")}]".includes(ch)) { if (depth === 0) return text.slice(start, i); depth--; - } else if (depth === 0 && !/[\w$]/.test(prev) && /^(?:case|default)\b/.test(text.slice(i))) { + } else if (depth === 0 && !/[\w$]/.test(prev) && /^(?:case|default)\b/.test(masked.slice(i))) { // The LEADING word-boundary check (`prev`) matters as much as the // trailing `\b`: without it the "case" inside an identifier such as // "lowercase(" reads as a case label and truncates the body here, // silently dropping the rest — including any measurement — from both // this slice and the caller's outside-the-statement remainder scan - // (which only covers text outside the whole switch statement). + // (which only covers text outside the whole switch statement). Both + // checks read `masked`, so a "case"/"default"-shaped substring inside a + // string or regex literal cannot be mistaken for a real case label. return text.slice(start, i); } } @@ -1199,40 +1201,38 @@ function resolveBooleanIfElseBranch( return conditionHolds ? thenBlock : elseBlock; } -// Advances `{depth, scan}` by one character — depth only counts brackets found -// outside string literals. `findTopLevelCaseMatch` below drives this one -// character at a time, resuming between regex matches. -type DepthScanState = { depth: number; scan: StringScanState }; -function advanceDepthScan(state: DepthScanState, ch: string): DepthScanState { - const scan = advanceStringScan(state.scan, ch); - if (scan.role !== "outside") return { depth: state.depth, scan }; - let depth = state.depth; - if ("({[".includes(ch)) depth++; - else if (")}]".includes(ch)) depth--; - return { depth, scan }; -} - // Finds `case "value":` at depth 0 of `switchBody` — i.e. belonging to THIS // switch, not to a switch/if/object-literal nested inside an earlier case's // own block. A plain (non-depth-aware) regex `.exec` would match the first // TEXTUAL occurrence, which can be a same-valued case label nested inside a -// sibling case's inner switch. +// sibling case's inner switch. The label pattern runs against the ORIGINAL text +// (its own quoted value has to read literally, not blanked), while the depth it +// is judged at comes from `masked` — a literal's bracket-like characters (a +// `{n,m}` regex quantifier, say) must not desync the count. function findTopLevelCaseMatch(switchBody: string, value: string): RegExpExecArray | null { + const masked = maskLiterals(switchBody); const casePattern = new RegExp(`case\\s*(["'])${escapeRegExp(value)}\\1\\s*:`, "g"); // Starts at -1, not 0: `switchBody` itself begins with the switch's own // wrapping `{`, so depth reaches 0 only once we're immediately inside it — // that's what "top-level" (belonging to THIS switch) means here. - let state: DepthScanState = { depth: -1, scan: OUTSIDE_STRING_SCAN_STATE }; - // Scans forward only: each match resumes where the previous one stopped, so - // every character is still fed to the depth scan exactly once, in order. + let depth = -1; + // Forward-only: each match resumes the depth scan where the previous one + // stopped, so every character is counted exactly once, in order. let cursor = 0; let match: RegExpExecArray | null; while ((match = casePattern.exec(switchBody)) !== null) { - while (cursor < match.index) { - state = advanceDepthScan(state, switchBody[cursor] ?? ""); - cursor++; + for (; cursor < match.index; cursor++) { + const ch = masked[cursor] ?? ""; + if ("({[".includes(ch)) depth++; + else if (")}]".includes(ch)) depth--; } - if (state.depth === 0 && !state.scan.inString) return match; + // The match itself must sit OUTSIDE any literal too: masking blanks a + // literal's contents to spaces, so a case-label-shaped substring inside + // a string/template literal (e.g. `` label = `case "b":` ``) reads as + // whitespace at `match.index` in `masked` rather than a real `c` — + // depth alone can't tell the two apart, since a same-level string sitting + // at the switch's own top level doesn't touch bracket depth at all. + if (depth === 0 && masked[match.index] === "c") return match; } return null; } @@ -1242,17 +1242,15 @@ function findTopLevelCaseMatch(switchBody: string, value: string): RegExpExecArr // — but not a `(...)`/`[...]` closing back to depth 0, which doesn't end a // statement (e.g. an `if (x)` condition's own closing paren). function splitTopLevelStatements(text: string): string[] { + const masked = maskLiterals(text); const statements: string[] = []; const stack: string[] = []; - let scan = OUTSIDE_STRING_SCAN_STATE; let start = 0; for (let i = 0; i < text.length; i++) { - const ch = text[i] ?? ""; - scan = advanceStringScan(scan, ch); - if (scan.role !== "outside") continue; - if (ch === "{" || ch === "(" || ch === "[") { + const ch = masked[i] ?? ""; + if ("({[".includes(ch)) { stack.push(ch); - } else if (ch === "}" || ch === ")" || ch === "]") { + } else if (")}]".includes(ch)) { const opener = stack.pop(); if (stack.length === 0 && opener === "{" && ch === "}") { statements.push(text.slice(start, i + 1)); From d3ea149a20d27c0afbee2f0e13dc90557f2c8d0f Mon Sep 17 00:00:00 2001 From: miga-heygen Date: Tue, 15 Sep 2026 17:43:11 +0000 Subject: [PATCH 5/6] fix: recover locally from a regex-vs-division misread instead of masking per call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- packages/lint/src/rules/gsap.test.ts | 47 ++++++--- packages/lint/src/rules/gsap.ts | 149 +++++++++++++++++---------- packages/lint/src/utils.test.ts | 44 +++++++- packages/lint/src/utils.ts | 119 +++++++++++++++++---- 4 files changed, 268 insertions(+), 91 deletions(-) diff --git a/packages/lint/src/rules/gsap.test.ts b/packages/lint/src/rules/gsap.test.ts index eab58dbf1c..c3d49a172c 100644 --- a/packages/lint/src/rules/gsap.test.ts +++ b/packages/lint/src/rules/gsap.test.ts @@ -3703,20 +3703,13 @@ describe("GSAP seek-order safety rules", () => { }); it("gsap_callback_dom_measurement: an unrelated regex/division ambiguity EARLIER in the script doesn't poison masking for a later, unrelated function", async () => { - // stripJsStringLiterals has its own documented fail-safe: if it can't - // resolve a regex-vs-division ambiguity anywhere in the text it scans, it - // returns that ENTIRE input untouched (fully unmasked), not just a - // locally-degraded region. `const ratio = {}/2;` is ordinary, valid JS - // that trips this (a `/` right after `}` defaults to "regex allowed", - // finds no closing `/` before the line ends, and gives up). Naively - // masking the WHOLE script for every matchBalanced/sliceExpression call - // would let that one unrelated line silently disable masking — and thus - // reintroduce the exact "unmasked string content corrupts bracket - // counting" bug this refactor exists to fix — for every OTHER function in - // the same script, including ones with nothing to do with the trigger. - // matchBalanced/sliceExpression now mask only source.slice(fromIndex), - // not the whole script, so a trigger textually BEFORE the region a call - // actually needs can no longer poison it. + // `const ratio = {}/2;` is ordinary, valid JS that trips stripJsStringLiterals's + // regex-vs-division ambiguity: the `/` right after `}` defaults to "regex + // allowed" and then finds no closing `/` before the line ends. That misread is + // recovered LOCALLY (see the scanner's own doc comment), so it must not affect + // masking for any OTHER function in the same script — here one whose real + // closing brace this rule can only find by bracket-depth counting, which needs + // the genuine `}` inside `"clo}se"` to stay masked. const html = `
@@ -3737,6 +3730,32 @@ describe("GSAP seek-order safety rules", () => { expect(finding).toBeDefined(); }); + it("gsap_callback_dom_measurement: an unrelated regex/division ambiguity LATER in the script doesn't poison masking for an earlier function", async () => { + // The mirror of the test above, and the direction that needs the recovery to be + // local rather than merely scoped: matchBalanced scans FORWARD from a function's + // own start index to the end of the script, so a misread sitting after that + // function is inside the span it masks no matter how narrowly the caller slices. + // Only correcting the misread in place keeps it away from `"clo}se"`. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeDefined(); + }); + it("gsap_callback_dom_measurement: an escaped backslash right before a string's closing quote doesn't mis-locate the closing quote", async () => { // `"a\\"` (the string `a\`) has a literal backslash immediately before // the real closing quote. Escape tracking based on "is the previous diff --git a/packages/lint/src/rules/gsap.ts b/packages/lint/src/rules/gsap.ts index 15f0ab5781..36c6734a36 100644 --- a/packages/lint/src/rules/gsap.ts +++ b/packages/lint/src/rules/gsap.ts @@ -745,22 +745,43 @@ function targetsShareElement( * * The literal awareness itself is delegated, never re-derived here: * `stripJsStringLiterals` already handles escape sequences and, crucially, - * regex-vs-division disambiguation — so a quote inside a sanitizer regex - * (`/'/g`) cannot open a string that never closes and silently desync every - * depth count downstream. + * regex-vs-division disambiguation — including recovering locally from a + * misread — so neither a quote inside a sanitizer regex (`/'/g`) nor an + * unrelated `{}/2`-shaped division elsewhere in the script can desync the + * depth counts below. * - * Memoized on the last input: callers below re-mask the same script once per - * regex match, and `stripJsStringLiterals` is a character-at-a-time scan, so - * re-deriving it per match is quadratic in script size. + * Cached by exact input string, since the scanners below mask the SAME text + * repeatedly (once per regex match, once per function signature, ...) and + * `stripJsStringLiterals` walks a character at a time, so re-deriving it per + * call is quadratic in script size. This is a small LRU, not a single slot: + * callers legitimately interleave masking the whole script, one function's + * body, and a small param/arg list within the same pass, and a single-slot + * cache would evict the (expensive) whole-script entry on every (cheap) small + * one — quadratic again, just with a smaller constant. A handful of entries + * comfortably covers that real interleaving without unbounded growth. */ -let lastMaskedSource: string | null = null; -let lastMaskedResult = ""; +const MASK_CACHE_LIMIT = 16; +const maskCache = new Map(); function maskLiterals(source: string): string { - if (source !== lastMaskedSource) { - lastMaskedResult = stripJsStringLiterals(source); - lastMaskedSource = source; + const cached = maskCache.get(source); + if (cached !== undefined) { + // Re-inserting moves this key to the END of the Map's iteration order — + // Maps preserve insertion order — so eviction below (oldest-first) is + // true least-recently-USED, not merely least-recently-inserted. Without + // this, a frequently reused entry (typically the whole script, masked + // once but read on nearly every call) could still be evicted the moment + // enough OTHER distinct strings are masked, even though it was just used. + maskCache.delete(source); + maskCache.set(source, cached); + return cached; } - return lastMaskedResult; + const masked = stripJsStringLiterals(source); + maskCache.set(source, masked); + if (maskCache.size > MASK_CACHE_LIMIT) { + const oldestKey = maskCache.keys().next().value; + if (oldestKey !== undefined) maskCache.delete(oldestKey); + } + return masked; } /** @@ -768,17 +789,6 @@ function maskLiterals(source: string): string { * Only REAL brackets count toward depth — one inside a string, template or * regex literal (a default value `a = "("`, a sanitizer regex `/{/g`) has been * masked out before the scan. - * - * Masks only `source.slice(openIndex)`, not the whole `source`, even though - * every caller already has the full script/body in hand: `stripJsStringLiterals` - * has its own documented fail-safe of returning its ENTIRE input untouched - * (fully unmasked) if regex-vs-division disambiguation goes wrong anywhere - * within it — one unrelated, ordinary line like `const ratio = {}/2;` - * *anywhere later in an unsliced whole script* could otherwise silently - * disable masking for every other call sharing that same source. Slicing to - * `openIndex` first — always a real bracket, itself a valid regex-preceding - * context either way — keeps that blast radius scoped to text this call - * actually needs, not unrelated code before it. */ function matchBalanced( source: string, @@ -786,20 +796,31 @@ function matchBalanced( open: string, close: string, ): string | null { - const relevant = source.slice(openIndex); - const masked = maskLiterals(relevant); + const masked = maskLiterals(source); let depth = 0; - for (let i = 0; i < relevant.length; i++) { + for (let i = openIndex; i < source.length; i++) { const ch = masked[i] ?? ""; if (ch === open) depth++; else if (ch === close) { depth--; - if (depth === 0) return relevant.slice(0, i + 1); + if (depth === 0) return source.slice(openIndex, i + 1); } } return null; } +/** + * +1 for an opening bracket, -1 for a closing one, 0 otherwise — the one + * definition of "which masked characters move nesting depth" that every + * depth-tracking scanner below shares, instead of each re-testing its own copy + * of the `({[` / `)}]` character sets. + */ +function bracketDelta(ch: string): number { + if ("({[".includes(ch)) return 1; + if (")}]".includes(ch)) return -1; + return 0; +} + /** * Index of the `{` opening the innermost object literal or block enclosing * `index`, or -1 when `index` sits inside none. Scans masked text backwards, so @@ -820,10 +841,7 @@ function enclosingOpenBraceIndex(masked: string, index: number): number { /** The nearest object literal `{...}` enclosing `index` (comment-stripped source). */ function enclosingObjectLiteral(source: string, index: number): string | null { - // Masks only the PREFIX up to `index` (text after it is irrelevant to a - // backward search) — same blast-radius reasoning as matchBalanced/ - // sliceExpression above. - const braceIndex = enclosingOpenBraceIndex(maskLiterals(source.slice(0, index + 1)), index); + const braceIndex = enclosingOpenBraceIndex(maskLiterals(source), index); if (braceIndex < 0) return null; return matchBalanced(source, braceIndex, "{", "}"); } @@ -840,14 +858,15 @@ function objectLiteralHasTopLevelRelativeValue(objectLiteral: string): boolean { // ORIGINAL text. Testing at closing quotes too is harmless: only an // OPENING quote can be followed by `+=`/`-=`. if (depth === 1 && /^[+-]=/.test(objectLiteral.slice(i + 1))) return true; - } else if ("({[".includes(ch)) depth++; - else if (")}]".includes(ch)) depth--; + } else { + depth += bracketDelta(ch); + } } return false; } function isInsideGsapTweenVars(source: string, index: number, timelineVars: string[]): boolean { - const braceIndex = enclosingOpenBraceIndex(maskLiterals(source.slice(0, index + 1)), index); + const braceIndex = enclosingOpenBraceIndex(maskLiterals(source), index); if (braceIndex < 0) return false; const before = source.slice(Math.max(0, braceIndex - 240), braceIndex).replace(/\s+/g, " "); const receivers = ["gsap", ...timelineVars].map(escapeRegExp).join("|"); @@ -859,21 +878,18 @@ function isInsideGsapTweenVars(source: string, index: number, timelineVars: stri * at depth 0 outside any literal. */ function sliceExpression(source: string, start: number): string { - // Sliced to `source.slice(start)` before masking, not the whole `source` — - // see matchBalanced's comment on why (shrinks stripJsStringLiterals's - // all-or-nothing fail-safe blast radius to text this call actually needs). - const relevant = source.slice(start); - const masked = maskLiterals(relevant); + const masked = maskLiterals(source); let depth = 0; - for (let i = 0; i < relevant.length; i++) { + for (let i = start; i < source.length; i++) { const ch = masked[i] ?? ""; - if ("({[".includes(ch)) depth++; - else if (")}]".includes(ch)) { - if (depth === 0) return relevant.slice(0, i); + const delta = bracketDelta(ch); + if (delta > 0) depth++; + else if (delta < 0) { + if (depth === 0) return source.slice(start, i); depth--; - } else if (ch === "," && depth === 0) return relevant.slice(0, i); + } else if (ch === "," && depth === 0) return source.slice(start, i); } - return relevant; + return source.slice(start); } type ParsedFunctionValue = { firstParam: string | null; body: string }; @@ -948,11 +964,11 @@ function splitTopLevelByComma(text: string): string[] { let start = 0; for (let i = 0; i < text.length; i++) { const ch = masked[i] ?? ""; - if ("({[".includes(ch)) depth++; - else if (")}]".includes(ch)) depth--; - else if (ch === "," && depth === 0) { + if (ch === "," && depth === 0) { parts.push(text.slice(start, i)); start = i + 1; + } else { + depth += bracketDelta(ch); } } parts.push(text.slice(start)); @@ -1088,8 +1104,9 @@ function sliceUntilNextCase(text: string, start: number): string { for (let i = start; i < text.length; i++) { const ch = masked[i] ?? ""; const prev = masked[i - 1] ?? ""; - if ("({[".includes(ch)) depth++; - else if (")}]".includes(ch)) { + const delta = bracketDelta(ch); + if (delta > 0) depth++; + else if (delta < 0) { if (depth === 0) return text.slice(start, i); depth--; } else if (depth === 0 && !/[\w$]/.test(prev) && /^(?:case|default)\b/.test(masked.slice(i))) { @@ -1223,8 +1240,7 @@ function findTopLevelCaseMatch(switchBody: string, value: string): RegExpExecArr while ((match = casePattern.exec(switchBody)) !== null) { for (; cursor < match.index; cursor++) { const ch = masked[cursor] ?? ""; - if ("({[".includes(ch)) depth++; - else if (")}]".includes(ch)) depth--; + depth += bracketDelta(ch); } // The match itself must sit OUTSIDE any literal too: masking blanks a // literal's contents to spaces, so a case-label-shaped substring inside @@ -1248,9 +1264,10 @@ function splitTopLevelStatements(text: string): string[] { let start = 0; for (let i = 0; i < text.length; i++) { const ch = masked[i] ?? ""; - if ("({[".includes(ch)) { + const delta = bracketDelta(ch); + if (delta > 0) { stack.push(ch); - } else if (")}]".includes(ch)) { + } else if (delta < 0) { const opener = stack.pop(); if (stack.length === 0 && opener === "{" && ch === "}") { statements.push(text.slice(start, i + 1)); @@ -1319,6 +1336,15 @@ function resolveSwitchCaseBranch( // branch. Returns null when the call can't be resolved this way (non-literal // argument, or no recognized branch shape) — callers fall back to the old, // conservative whole-function taint in that case. +// +// Two exotic shapes are known misses, documented rather than handled: (1) a +// measurement reached only through a sibling case LABEL's own expression +// (`case getMeasurement():`) — this resolver reads case BODIES, never case +// expressions; (2) sloppy-mode `arguments[1] = true` aliasing a named +// parameter — `arguments` is a live view onto non-strict parameters, so it can +// flip which literal a parameter holds without the assignment-to-that-name +// this resolver looks for ever appearing. Closing either needs AST-level +// analysis, not text scanning. function resolveCallSiteMeasurement( calleeName: string, argsText: string, @@ -1360,6 +1386,15 @@ function resolveCallSiteMeasurement( // deliberate, documented scope trade — there is no call syntax in the // callback's own text to resolve, because the invocation (and whatever // arguments it passes) happens inside setTimeout's/forEach's implementation. +// +// Known scaling characteristic, pre-existing and out of scope here: this is +// called once per callback call-site, and loops `measuring` (every DOM-touching +// named function in the WHOLE script) on each call — O(call sites × measuring +// names). A script with many call sites, each naming its OWN distinct measuring +// function, scales quadratically; one with many call sites sharing a SMALL, +// shared set of measuring functions (the realistic shape) does not. Not a +// regression from this pass's `maskLiterals` caching change — a follow-up would +// need to invert the loop (index calls by name once, not per callback). function expressionReachesMeasurement( expression: string, measuring: Set, @@ -2714,7 +2749,11 @@ export const gsapRules: LintRule[] = [ const parenIndex = match.index + match[0].length - 1; const argsWithParens = matchBalanced(source, parenIndex, "(", ")"); if (!argsWithParens) continue; - const firstArg = sliceExpression(argsWithParens.slice(1, -1), 0); + // Passes `source` + an absolute index, like every sibling call below — + // never a fresh slice (`argsWithParens.slice(1, -1)`), which would mask + // a brand-new string and evict `source`'s own cached mask, undoing the + // whole point of `maskLiterals`'s single-slot memo. + const firstArg = sliceExpression(source, parenIndex + 1); const site = match[0] + firstArg + ", ...)"; if (callbackExpressionHazard(firstArg)) report(site, site); } diff --git a/packages/lint/src/utils.test.ts b/packages/lint/src/utils.test.ts index 41663ff9d8..cca218a6f9 100644 --- a/packages/lint/src/utils.test.ts +++ b/packages/lint/src/utils.test.ts @@ -51,17 +51,55 @@ describe("stripJsStringLiterals", () => { "foo(/abc\nrequestAnimationFrame(step);", "var of = 2;\nvar r = of /2;\nrequestAnimationFrame(step);", "var q = 1;\nx = /a\\\nrequestAnimationFrame(step);", - ])("falls back to the source when a slash never closes on its line: %j", (src) => { + ])("recovers a slash that never closes on its line without losing %j", (src) => { + // None of these misread spans contain a real string/regex literal of their + // own, so re-walking them as ordinary (division) code reconstructs the input + // exactly — the scan recovers here, it does not give up and return `source` + // the way the mid-literal case below does. expect(scan(src)).toBe(src); expect(findsRaf(src)).toBe(true); }); - it("falls back to the source when a backslash ends a mis-read regex line", () => { + it("recovers a mis-read regex line and still masks a real string literal around it", () => { + // "b in /x...\" misreads as a regex start (`in` is regex-allowed); + // the genuine string literal 'requestAnimationFrame(' sits right after the + // recovered span and must still come back masked — proving real recovery, + // rather than an identity result that only looks like it. const src = "var a = b in /x\\\n y = 'requestAnimationFrame(' / z /;"; - expect(scan(src)).toBe(src); + const out = scan(src); + expect(out).not.toBe(src); + expect(out).toContain("var a = b in /x\\\n y = '"); + expect(out).toContain("' / z /;"); + expect(findsRaf(src)).toBe(false); + }); + + // A misread costs only its own span: the ambiguous `{}/2` division is ordinary, + // valid JS, and must not un-mask an unrelated string carrying bracket-like content + // ANYWHERE else in the same script — on either side of it. + it.each([ + { + label: "before a LATER misread", + src: 'const label = "clo}se"; const ratio = {}/2;\nrequestAnimationFrame(step);', + }, + { + label: "after an EARLIER misread", + src: 'const ratio = {}/2;\nconst label = "clo}se";\nrequestAnimationFrame(step);', + }, + ])("keeps an unrelated string masked $label", ({ src }) => { + const out = scan(src); + expect(out).toContain('const label = " ";'); expect(findsRaf(src)).toBe(true); }); + it("keeps a backslash-escaped quote immediately followed by a real bracket masked as string content", () => { + // If escape-tracking on this branch were ever dropped, the escaped quote + // in `"x\"}"` would misread as the string's REAL closing quote, exposing + // the following `}` as ordinary code instead of masked string content — + // corrupting any bracket-depth count a caller derives from the mask. + const src = 'const s = "x\\"}"; requestAnimationFrame(step);'; + expect(scan(src)).toBe('const s = " "; requestAnimationFrame(step);'); + }); + it("falls back to the source when the scan ends mid-literal", () => { const src = 'const p = "C:\\Users\\demo\\";\nrequestAnimationFrame(step);'; expect(scan(src)).toBe(src); diff --git a/packages/lint/src/utils.ts b/packages/lint/src/utils.ts index ef24a3a09f..b7f259ed04 100644 --- a/packages/lint/src/utils.ts +++ b/packages/lint/src/utils.ts @@ -515,6 +515,15 @@ const REGEX_ALLOWED_KEYWORDS = new Set([ const WORD_CHAR = /[A-Za-z0-9_$]/; +/** A copy of `CodeContext`'s incremental state, so a wrong regex guess can be rewound. */ +type CodeContextSnapshot = { + last: string; + prev: string; + word: string; + wordEnded: boolean; + wordAfterDot: boolean; +}; + /** * Tracks just enough emitted context to tell a regex literal from a division: the last * two significant characters and the trailing identifier. Carried incrementally because @@ -552,45 +561,109 @@ class CodeContext { if ((this.last === "+" || this.last === "-") && this.prev === this.last) return false; return REGEX_ALLOWED_BEFORE.has(this.last); } + + snapshot(): CodeContextSnapshot { + return { + last: this.last, + prev: this.prev, + word: this.word, + wordEnded: this.wordEnded, + wordAfterDot: this.wordAfterDot, + }; + } + + restore(snapshot: CodeContextSnapshot): void { + this.last = snapshot.last; + this.prev = snapshot.prev; + this.word = snapshot.word; + this.wordEnded = snapshot.wordEnded; + this.wordAfterDot = snapshot.wordAfterDot; + } } +/** + * A "/" currently being scanned AS a regex literal, carrying the state to rewind to + * if that guess turns out wrong. Non-null exactly while the scan sits inside a + * candidate regex — the same "value doubles as the mode flag" shape `quote` uses for + * strings below. + */ +type RegexGuess = { index: number; context: CodeContextSnapshot }; + /** * Blanks string, template-literal and regex-literal *contents* (delimiters, length * and newline positions kept) so a rule scanning for an API call does not match one * a composition merely renders as on-screen text. Template `${…}` expressions stay — - * they are code. Returns the source untouched if the scan ends mid-literal, so a - * parse this scanner cannot model degrades to the caller's pre-existing behaviour - * rather than silently blanking real code on an `error`-severity gate. + * they are code. Returns the source untouched if the scan ends mid-literal (an + * unterminated string/template, or a regex still open at end of input), so a parse + * this scanner cannot model degrades to the caller's pre-existing behaviour rather + * than silently blanking real code on an `error`-severity gate. + * + * A candidate "/" is only ever a GUESS at starting a regex literal, since regex and + * division are genuinely ambiguous in text. Real regex literals can't span a line, so + * a "/" whose "regex" runs past a line end was misread, and everything blanked since + * it is ordinary (division) code. The scan then rewinds to that "/" and re-walks the + * span as code (see `recoverFromMisread`), so a misread costs only its own span — + * every literal correctly masked elsewhere in `source`, before it or after it, stays + * masked. */ // fallow-ignore-next-line complexity export function stripJsStringLiterals(source: string): string { let out = ""; + // Content emitted WHILE a regex guess is live, held apart from `out` rather + // than appended to it directly. `out` is built by repeated `+=` (a rope of + // chunks under the hood), so slicing it back on a misread would force it to + // flatten to a plain string proportional to `out`'s ENTIRE length so far, on + // every single misread — quadratic for input with many of them, even though + // each misread's own span is bounded (it can't cross a line). Buffering the + // guess's content separately makes recovery an O(1) discard instead: only + // `regexBuffer` (bounded by one line) is ever thrown away, `out` is never + // touched until a guess is confirmed real. + let regexBuffer = ""; let i = 0; const templateBraces: number[] = []; const ctx = new CodeContext(); let quote: "'" | '"' | "`" | null = null; let escaped = false; - let inRegex = false; + let regexGuess: RegexGuess | null = null; let inRegexClass = false; - let regexMisread = false; + // The one "/" a misread has already proven to be division, so the re-walk reads it + // as ordinary code instead of guessing "regex" at it again. A single slot is enough: + // a rewind only ever returns to the guess it is about to step past, so a resolved + // index can never come round a second time. + let knownDivisionIndex = -1; const blank = (ch: string) => (ch === "\n" || ch === "\r" ? ch : " "); const emit = (text: string) => { - out += text; + if (regexGuess) regexBuffer += text; + else out += text; for (const ch of text) ctx.push(ch); }; + // Undoes a regex guess a line boundary just disproved: discard the buffered + // guess content, rewind `ctx` and `i` to the "/" itself so the span is + // re-walked as code, leaving no regex-scan flag still set. `out` is never + // touched (nothing of the guess ever reached it). `quote`/`templateBraces` + // need no rewinding either — neither can change while inside a regex guess. + const recoverFromMisread = (guess: RegexGuess): void => { + regexBuffer = ""; + ctx.restore(guess.context); + knownDivisionIndex = guess.index; + i = guess.index; + regexGuess = null; + inRegexClass = false; + escaped = false; + }; + while (i < source.length) { const ch = source[i] ?? ""; const next = source[i + 1] ?? ""; - if (inRegex) { + if (regexGuess) { if (escaped) { escaped = false; if (ch === "\n" || ch === "\r") { - inRegex = false; - inRegexClass = false; - regexMisread = true; + recoverFromMisread(regexGuess); + continue; } emit(blank(ch)); } else if (ch === "\\") { @@ -603,14 +676,16 @@ export function stripJsStringLiterals(source: string): string { inRegexClass = false; emit(" "); } else if (ch === "/" && !inRegexClass) { - inRegex = false; + // Confirmed a real regex: fold its buffered content into `out` before + // clearing the guess, so `emit` below (now that `regexGuess` is null) + // appends the closing "/" straight to `out` right after it. + out += regexBuffer; + regexBuffer = ""; + regexGuess = null; emit(ch); } else if (ch === "\n" || ch === "\r") { - inRegex = false; - inRegexClass = false; - escaped = false; - regexMisread = true; - emit(ch); + recoverFromMisread(regexGuess); + continue; } else { emit(" "); } @@ -648,8 +723,14 @@ export function stripJsStringLiterals(source: string): string { continue; } - if (ch === "/" && next !== "/" && next !== "*" && ctx.startsRegexLiteral()) { - inRegex = true; + if ( + ch === "/" && + next !== "/" && + next !== "*" && + i !== knownDivisionIndex && + ctx.startsRegexLiteral() + ) { + regexGuess = { index: i, context: ctx.snapshot() }; emit(ch); i += 1; continue; @@ -674,7 +755,7 @@ export function stripJsStringLiterals(source: string): string { i += 1; } - if (quote !== null || templateBraces.length > 0 || inRegex || regexMisread) return source; + if (quote !== null || templateBraces.length > 0 || regexGuess !== null) return source; return out; } From b128fbf8741744a7c95bdeda38154bf037a3e059 Mon Sep 17 00:00:00 2001 From: miga-heygen Date: Tue, 15 Sep 2026 18:28:03 +0000 Subject: [PATCH 6/6] fix: recover from a regex-vs-division misread that runs to end of input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- packages/lint/src/rules/gsap.test.ts | 31 ++++++++++++++++++++++ packages/lint/src/rules/gsap.ts | 5 ++-- packages/lint/src/utils.test.ts | 21 +++++++++++++++ packages/lint/src/utils.ts | 39 ++++++++++++++++++++-------- 4 files changed, 83 insertions(+), 13 deletions(-) diff --git a/packages/lint/src/rules/gsap.test.ts b/packages/lint/src/rules/gsap.test.ts index c3d49a172c..2724880427 100644 --- a/packages/lint/src/rules/gsap.test.ts +++ b/packages/lint/src/rules/gsap.test.ts @@ -3030,6 +3030,37 @@ describe("GSAP seek-order safety rules", () => { expect(finding).toBeDefined(); }); + it("gsap_callback_dom_measurement: a comma inside an array-literal argument doesn't misalign a later literal argument", async () => { + // An array-literal argument (`[1, 2]`) contains a comma that must NOT be + // treated as a top-level argument separator — bracket-depth tracking has + // to count `[`/`]` just like `(`/`)`/`{`/`}`, or the split misaligns + // every argument after it. Here `useMeasurement` (real value: `true`, + // selecting the SAFE branch) would land one position later than the real + // literal reaches with a broken split, leaving it unresolved and + // conservatively (here, incorrectly) flagged. + const html = ` + +
+ +`; + const result = await lintHyperframeHtml(html); + const finding = result.findings.find((f) => f.code === "gsap_callback_dom_measurement"); + expect(finding).toBeUndefined(); + }); + it("gsap_callback_dom_measurement: a param reassigned before the branch is not substituted (fails closed)", async () => { // useMeasurement is flipped before the `if`, so the caller's literal // `false` no longer reflects what the branch actually tests (it becomes diff --git a/packages/lint/src/rules/gsap.ts b/packages/lint/src/rules/gsap.ts index 36c6734a36..2a8265428f 100644 --- a/packages/lint/src/rules/gsap.ts +++ b/packages/lint/src/rules/gsap.ts @@ -2751,8 +2751,9 @@ export const gsapRules: LintRule[] = [ if (!argsWithParens) continue; // Passes `source` + an absolute index, like every sibling call below — // never a fresh slice (`argsWithParens.slice(1, -1)`), which would mask - // a brand-new string and evict `source`'s own cached mask, undoing the - // whole point of `maskLiterals`'s single-slot memo. + // a brand-new string: an unnecessary `stripJsStringLiterals` scan of the + // whole script on every call, and one more entry competing for space in + // `maskLiterals`'s LRU alongside `source`'s own (already cached) mask. const firstArg = sliceExpression(source, parenIndex + 1); const site = match[0] + firstArg + ", ...)"; if (callbackExpressionHazard(firstArg)) report(site, site); diff --git a/packages/lint/src/utils.test.ts b/packages/lint/src/utils.test.ts index cca218a6f9..c527900412 100644 --- a/packages/lint/src/utils.test.ts +++ b/packages/lint/src/utils.test.ts @@ -91,6 +91,27 @@ describe("stripJsStringLiterals", () => { expect(findsRaf(src)).toBe(true); }); + it("recovers a misread regex that runs all the way to end-of-input with no trailing newline", () => { + // A real regex literal can't extend past EOF either, so a "/" whose guessed + // regex reaches the very end of `source` without a newline in between is + // just as much a misread as one that hits a mid-file line boundary — it + // must get the same local recovery, not the whole-input bail (there is no + // newline left for the OLD newline-only recovery trigger to ever fire on). + const src = 'const label = "clo}se";\nconst ratio = {}/2;'; + expect(scan(src)).toBe('const label = " ";\nconst ratio = {}/2;'); + }); + + it("keeps output length equal to input length across a misread immediately followed by a real regex", () => { + // Guards `recoverFromMisread` actually clearing its buffered guess content: + // if a misread's `regexBuffer` were ever left un-cleared, its stale + // characters would leak into the very next CONFIRMED regex literal's own + // buffered content, inflating the output past the input's length. + const src = "a = {}/2;\nr = /x/;\nrequestAnimationFrame(step);"; + const out = scan(src); + expect(out.length).toBe(src.length); + expect(findsRaf(src)).toBe(true); + }); + it("keeps a backslash-escaped quote immediately followed by a real bracket masked as string content", () => { // If escape-tracking on this branch were ever dropped, the escaped quote // in `"x\"}"` would misread as the string's REAL closing quote, exposing diff --git a/packages/lint/src/utils.ts b/packages/lint/src/utils.ts index b7f259ed04..9a27dd3cdc 100644 --- a/packages/lint/src/utils.ts +++ b/packages/lint/src/utils.ts @@ -594,17 +594,19 @@ type RegexGuess = { index: number; context: CodeContextSnapshot }; * and newline positions kept) so a rule scanning for an API call does not match one * a composition merely renders as on-screen text. Template `${…}` expressions stay — * they are code. Returns the source untouched if the scan ends mid-literal (an - * unterminated string/template, or a regex still open at end of input), so a parse - * this scanner cannot model degrades to the caller's pre-existing behaviour rather - * than silently blanking real code on an `error`-severity gate. + * unterminated string/template), so a parse this scanner cannot model degrades to + * the caller's pre-existing behaviour rather than silently blanking real code on an + * `error`-severity gate. * * A candidate "/" is only ever a GUESS at starting a regex literal, since regex and - * division are genuinely ambiguous in text. Real regex literals can't span a line, so - * a "/" whose "regex" runs past a line end was misread, and everything blanked since - * it is ordinary (division) code. The scan then rewinds to that "/" and re-walks the - * span as code (see `recoverFromMisread`), so a misread costs only its own span — - * every literal correctly masked elsewhere in `source`, before it or after it, stays - * masked. + * division are genuinely ambiguous in text. Real regex literals can't span a line — + * or run past the end of input — so a "/" whose "regex" reaches either was misread, + * and everything blanked since it is ordinary (division) code. The scan then rewinds + * to that "/" and re-walks the span as code (see `recoverFromMisread`), so a misread + * costs only its own span — every literal correctly masked elsewhere in `source`, + * before it or after it, stays masked. Because end-of-input gets the same recovery + * as a line boundary, a still-open guess can never survive to the final fail-safe + * check below — only a genuinely unterminated string/template can. */ // fallow-ignore-next-line complexity export function stripJsStringLiterals(source: string): string { @@ -654,7 +656,18 @@ export function stripJsStringLiterals(source: string): string { escaped = false; }; - while (i < source.length) { + while (true) { + if (i >= source.length) { + // End of input while still guessing a regex is the same misread signal + // as a line boundary — a real regex literal can't extend past EOF + // either — so it gets the identical recovery, not a silent whole-input + // bail: without this, a misread on the source's LAST line (no trailing + // newline) would never be caught, since the newline check below is the + // only other place recovery triggers. + if (!regexGuess) break; + recoverFromMisread(regexGuess); + continue; + } const ch = source[i] ?? ""; const next = source[i + 1] ?? ""; @@ -755,7 +768,11 @@ export function stripJsStringLiterals(source: string): string { i += 1; } - if (quote !== null || templateBraces.length > 0 || regexGuess !== null) return source; + // `regexGuess` can never be non-null here: the only way out of the loop + // above is its `break`, which is itself gated on `!regexGuess` — a guess + // still open at EOF is recovered (and re-walked) exactly like one open at + // a line boundary, never left to fall out of the loop. + if (quote !== null || templateBraces.length > 0) return source; return out; }