Skip to content

Commit 2e810f6

Browse files
committed
fix: stop a regex literal from blinding the live-caller scan
Three holes in the guard the previous commit rewrote, all found by reviewing that commit rather than the branch. The masker had no regex-literal awareness, and this suite is full of patterns like /rel=["']modulepreload["']/ whose quote characters it read as string delimiters. That desyncs the mask for the REST OF THE FILE, so every live call below such a line vanished from the scan. Eighteen test files carry the shape, including the app-boot tests. Demonstrated end to end: injecting a live fetch after the regex on blog-smoke.test.js:115 left the guard fully green, while the identical line above it redded. So the scan could report clean because it had gone blind, which is the same class of hole the previous commit was written to close. Telling a regex from a division needs the preceding token, so the usual heuristic goes in, with character classes and escapes handled and division still reading as division. The host lookup used a 200-character raw window, which crosses statements. A `fetch(localUrl)` followed two lines later by an assertion naming a jspm url read as a live call, and so did a comment mentioning one. It reads the call's actual argument list now, via the same paren matcher the guard ranges use. And the allowlist's `live` flag was read as merely falsy, so an entry added without the key skipped the *.live.test.* requirement while still collecting a whole-file exemption. It must be an explicit boolean now. Each of the three ships with the counterfactual that reproduces it, since the previous round's lesson was that this guard's own tests were the thing not being checked.
1 parent 8c735e0 commit 2e810f6

2 files changed

Lines changed: 140 additions & 19 deletions

File tree

test/fixtures/live-caller-scan.mjs

Lines changed: 93 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -42,24 +42,58 @@ export const LIVE_ENTRY_POINTS = ['pinAll', 'updatePinned', 'auditPinned', 'find
4242
const GUARDS = ['withMockedFetch', 'withJspmDouble'];
4343

4444
/**
45-
* Blank out comments, strings, and template literals, replacing each with
46-
* same-length filler so every index still lines up with the original source.
45+
* Blank out comments, strings, template literals, and REGEX literals, replacing
46+
* each with same-length filler so every index still lines up with the original
47+
* source.
4748
*
4849
* Preserving offsets is what lets the guarded-range scan below run on the
4950
* masked text and still report positions in the real file. Blanking strings
5051
* matters for two different reasons: a host named inside a mock's expected-url
5152
* string is not a call, and an unbalanced parenthesis inside a string would
52-
* otherwise wreck the brace matching.
53+
* otherwise wreck the paren matching.
54+
*
55+
* Regex literals are the subtle one, and skipping them is not a rounding
56+
* error: this suite is full of patterns like
57+
* `/rel=["']modulepreload["']/`, whose quote characters would otherwise be
58+
* read as string delimiters. That desyncs the mask for the REST OF THE FILE,
59+
* so every live call below such a line silently disappears from the scan.
60+
* Eighteen test files here carry that shape, including the app-boot tests, so
61+
* a masker without this is a guard that reports clean because it went blind.
62+
*
63+
* Telling a regex from a division needs the preceding token, since `/` is
64+
* both. The usual heuristic applies: a regex may start where a VALUE may not
65+
* have just ended, so after an operator, an opening bracket, a comma, a
66+
* semicolon, or a keyword like `return`, but not after an identifier, a
67+
* number, or a closing bracket.
5368
*
5469
* @param {string} src
5570
* @returns {string}
5671
*/
72+
/** Keywords after which a `/` opens a regex rather than dividing. */
73+
const REGEX_PRECEDING_WORDS = new Set([
74+
'return', 'typeof', 'instanceof', 'in', 'of', 'new', 'delete', 'void',
75+
'throw', 'case', 'do', 'else', 'yield', 'await',
76+
]);
77+
5778
export function maskLiterals(src) {
5879
const out = src.split('');
5980
let i = 0;
6081
const blank = (from, to) => {
6182
for (let k = from; k < to && k < out.length; k++) if (out[k] !== '\n') out[k] = ' ';
6283
};
84+
85+
/** The last token character that was not whitespace and not masked away. */
86+
let prev = '';
87+
/** The identifier immediately before `prev`, when `prev` ends a word. */
88+
let prevWord = '';
89+
90+
const regexCanStartHere = () => {
91+
if (!prev) return true;
92+
if (/[)\]}]/.test(prev)) return false;
93+
if (/[A-Za-z0-9_$]/.test(prev)) return REGEX_PRECEDING_WORDS.has(prevWord);
94+
return true;
95+
};
96+
6397
while (i < src.length) {
6498
const c = src[i];
6599
const next = src[i + 1];
@@ -73,6 +107,25 @@ export function maskLiterals(src) {
73107
const stop = end === -1 ? src.length : end;
74108
blank(i, stop); i = stop; continue;
75109
}
110+
if (c === '/' && regexCanStartHere()) {
111+
// Scan to the closing delimiter, honouring escapes and character
112+
// classes (a `/` inside `[...]` does not end the literal).
113+
let j = i + 1;
114+
let inClass = false;
115+
let closed = false;
116+
while (j < src.length && src[j] !== '\n') {
117+
const d = src[j];
118+
if (d === '\\') { j += 2; continue; }
119+
if (d === '[') inClass = true;
120+
else if (d === ']') inClass = false;
121+
else if (d === '/' && !inClass) { closed = true; break; }
122+
j++;
123+
}
124+
if (closed) {
125+
blank(i + 1, j); prev = '/'; prevWord = ''; i = j + 1; continue;
126+
}
127+
// An unterminated `/` was a division after all. Fall through.
128+
}
76129
if (c === '"' || c === "'" || c === '`') {
77130
let j = i + 1;
78131
while (j < src.length) {
@@ -82,13 +135,37 @@ export function maskLiterals(src) {
82135
}
83136
// Blank the CONTENTS but keep the quotes, so a masked string is still
84137
// recognisably a string and cannot merge with the token beside it.
85-
blank(i + 1, j); i = j + 1; continue;
138+
blank(i + 1, j); prev = c; prevWord = ''; i = j + 1; continue;
139+
}
140+
if (!/\s/.test(c)) {
141+
if (/[A-Za-z0-9_$]/.test(c)) {
142+
prevWord = /[A-Za-z0-9_$]/.test(prev) ? prevWord + c : c;
143+
} else {
144+
prevWord = '';
145+
}
146+
prev = c;
86147
}
87148
i++;
88149
}
89150
return out.join('');
90151
}
91152

153+
/**
154+
* Index of the `)` matching the `(` at `openIdx`, or -1.
155+
* @param {string} masked @param {number} openIdx
156+
*/
157+
function matchParen(masked, openIdx) {
158+
let depth = 0;
159+
for (let k = openIdx; k < masked.length; k++) {
160+
if (masked[k] === '(') depth++;
161+
else if (masked[k] === ')') {
162+
depth--;
163+
if (depth === 0) return k;
164+
}
165+
}
166+
return -1;
167+
}
168+
92169
/**
93170
* Character ranges covered by a `withMockedFetch(` / `withJspmDouble(` call,
94171
* from its opening parenthesis to its match.
@@ -103,14 +180,8 @@ export function guardedRanges(masked) {
103180
const re = new RegExp(`\\b${guard}\\s*\\(`, 'g');
104181
for (const m of masked.matchAll(re)) {
105182
const open = m.index + m[0].length - 1;
106-
let depth = 0;
107-
for (let k = open; k < masked.length; k++) {
108-
if (masked[k] === '(') depth++;
109-
else if (masked[k] === ')') {
110-
depth--;
111-
if (depth === 0) { ranges.push([open, k]); break; }
112-
}
113-
}
183+
const close = matchParen(masked, open);
184+
if (close !== -1) ranges.push([open, close]);
114185
}
115186
}
116187
return ranges;
@@ -139,13 +210,17 @@ export function findLiveCallers(src) {
139210
const found = [];
140211

141212
// A host literal has to be read from the ORIGINAL source, since masking
142-
// blanks string contents. Only its position is taken from the mask, and a
143-
// `fetch(` whose argument names a live host is what counts; the same host in
144-
// an assertion or an importmap fixture is inert, and the suite is full of
145-
// those on purpose.
213+
// blanks string contents. Only the BOUNDS come from the mask, and they are
214+
// the call's actual argument list rather than a fixed character window: a
215+
// window crosses statement boundaries, so `fetch(localUrl)` followed two
216+
// lines later by an assertion naming a jspm url read as a live call. The
217+
// same host in an assertion, a comment, or an importmap fixture is inert,
218+
// and this suite is full of those on purpose.
146219
for (const m of masked.matchAll(/\bfetch\s*\(/g)) {
147-
const argStart = m.index + m[0].length;
148-
const arg = src.slice(argStart, argStart + 200);
220+
const open = m.index + m[0].length - 1;
221+
const close = matchParen(masked, open);
222+
if (close === -1) continue;
223+
const arg = src.slice(open + 1, close);
149224
const host = LIVE_HOSTS.find((h) => arg.includes(h));
150225
if (host && !guarded(m.index)) found.push({ kind: 'host', what: host, line: lineAt(m.index) });
151226
}

test/repo-health/live-cdn-callers.test.mjs

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,11 +105,17 @@ for (const pkg of readdirSync(join(ROOT, 'packages'), { withFileTypes: true }))
105105

106106
const rel = (f) => f.slice(ROOT.length + 1).split(sep).join('/');
107107

108-
test('every allowlisted LIVE caller is a *.live.test.* file that exists', () => {
108+
test('every allowlist entry declares itself and, if live, is a *.live.test.* file', () => {
109109
for (const entry of LIVE_CALLERS) {
110110
assert.ok(files.some((f) => rel(f) === entry.file),
111111
`${entry.file} is allowlisted but no such test file exists`);
112112
assert.ok(entry.why.length > 40, `${entry.file} needs a real reason, not a placeholder`);
113+
// `live` is REQUIRED and must be boolean. Reading it as merely falsy would
114+
// let an entry added without the key skip the marker check below while
115+
// still collecting a whole-file scan exemption, which is the quietest
116+
// possible way to reopen the hole this guard exists to close.
117+
assert.equal(typeof entry.live, 'boolean',
118+
`${entry.file} must state \`live: true\` or \`live: false\` explicitly`);
113119
if (entry.live) {
114120
assert.ok(entry.file.includes(LIVE_MARKER),
115121
`${entry.file} is allowlisted as live but the runners only skip *.live.test.* files`);
@@ -180,6 +186,46 @@ test('counterfactual: the scan does not fire on the shapes that are genuinely sa
180186
`), []);
181187
});
182188

189+
test('counterfactual: a regex literal carrying a quote does not blind the scan', () => {
190+
// The subtlest failure this guard can have, and it went undetected until a
191+
// review reproduced it. A pattern like /rel=["']modulepreload["']/ has quote
192+
// characters in it; a masker without regex awareness reads the first one as
193+
// a string opener and desyncs for the REST OF THE FILE, so every live call
194+
// below that line silently vanishes from the scan. Eighteen test files here
195+
// carry that shape, including the app-boot tests.
196+
const afterARegex = [
197+
'const re = /<link[^>]+rel=["\']modulepreload["\']/g;',
198+
'const leak = await fetch("https://api.jspm.io/generate", { method: "POST" });',
199+
].join('\n');
200+
assert.deepEqual(findLiveCallers(afterARegex).map((h) => h.what), ['api.jspm.io'],
201+
'a live call after a quote-bearing regex must still be seen');
202+
203+
// The other half: a `/` that is division must NOT be read as a regex, or the
204+
// mask desyncs the other way and starts swallowing real code.
205+
const withDivision = [
206+
'const half = total / 2;',
207+
'const other = count / 4;',
208+
'await fetch("https://api.jspm.io/generate");',
209+
].join('\n');
210+
assert.deepEqual(findLiveCallers(withDivision).map((h) => h.what), ['api.jspm.io']);
211+
});
212+
213+
test('counterfactual: a host named near, but not inside, a fetch call is inert', () => {
214+
// The scan reads the call's actual argument list, not a character window. A
215+
// window crossed statement boundaries, so a local fetch followed by an
216+
// assertion naming a jspm url read as a live call.
217+
assert.deepEqual(findLiveCallers([
218+
'const r = await fetch(localUrl);',
219+
'assert.equal(r.status, 200);',
220+
'assert.equal(map.dayjs, "https://ga.jspm.io/npm:dayjs@1.11.20/index.js");',
221+
].join('\n')), []);
222+
223+
assert.deepEqual(findLiveCallers([
224+
'await fetch(baseUrl);',
225+
'// TODO: point this at the double instead of ga.jspm.io one day.',
226+
].join('\n')), []);
227+
});
228+
183229
test('both runners drop live files unless the network is explicitly required', () => {
184230
// The policy above is only worth anything because the runners enforce it, so
185231
// assert the enforcement rather than trusting it. A refactor that renames

0 commit comments

Comments
 (0)