Skip to content

Commit 8e39bbc

Browse files
claude[bot]claude
andauthored
fix(scripts): close the three conditional import leaks, shrinking KNOWN_IMPORT_UNSAFE 5 -> 2 (#11951)
* fix(scripts): guard qa-rollup's --self-test dispatch against importers `qa-rollup.mjs` exports bindings and tested `--self-test` BEFORE the entry guard, so the branch read the IMPORTER's argv: any tool that imported this module for its exports while carrying `--self-test` in its own argv ran qa-rollup's entire self-test inside itself. The leak is invisible to the two signals a caller usually has. The self-test does not exit on success, so the importer survives with status 0 and finishes its own work; the only trace is 82 bytes of foreign output on the importer's stdout. Measured with a probe that prints a sentinel AFTER the dynamic import and counts bytes that are not the sentinel: before LEAK scripts/qa/qa-rollup.mjs argv --self-test FOREIGN-BYTES=82 after CLEAN scripts/qa/qa-rollup.mjs argv --self-test Guard first, mode second. CLI behaviour is unchanged — `--self-test` output is byte-identical before and after (85 bytes, `cmp` clean), because the guard is true for every direct invocation. `check:entry-guard` names the file STALE once it is inert, so its KNOWN_IMPORT_UNSAFE line goes in this same commit: the ledger shrinks 5 -> 4 and the gate's own count moves to `108 of them inert on import (4 known-unsafe)`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ahemw8RcTgqtxrj15PEZx * fix(scripts): move objectui-range's --help out of module top level `objectui-range.mjs` exports bindings and answered `-h`/`--help` from a bare top-level `if`, so the test read the IMPORTER's argv. An importer carrying either flag got this file's 4666-byte `//` header written to its stdout and then `process.exit(0)`. That is the worst-reading shape in this class: the importer's process ends mid-import with a SUCCESS status, so a caller holding only `result.status` cannot tell it from a clean import. Measured with a probe that prints a sentinel AFTER the dynamic import — the sentinel is what makes the two distinguishable, exit status alone is not: before LEAK argv --help NO-SENTINEL(status=0) FOREIGN-BYTES=4666 before LEAK argv -h NO-SENTINEL(status=0) FOREIGN-BYTES=4666 after CLEAN argv --help after CLEAN argv -h The help text is unchanged, and deliberately so: it is read back out of this file's own `//` lines, so a comment added at column 0 would rewrite it. The new rationale is a `/** */` block for that reason and the column-0 `//` count is still 79. All four CLI surfaces are byte-identical before and after — `--help` 4693 bytes, `-h` 4693, no-args 93 bytes on stderr with status 1, and `--self-test` 1578 bytes, every one `cmp`-clean on both streams. `check:entry-guard` names the file STALE once it is inert, so its KNOWN_IMPORT_UNSAFE line goes in this same commit: the ledger shrinks 4 -> 3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ahemw8RcTgqtxrj15PEZx * fix(scripts): arm ts-parse's census report on first parse, not on import `ts-parse.mjs` registered its `OS_TOOLING_PARSE_CENSUS` exit report from a top-level `if (process.env...)`. This module is a LIBRARY — eleven gates in `scripts/` import it for its exports — so with the env set, that registration ran inside every one of those importers and wrote a line to a process whose only involvement was having loaded it. The entry-point guard is NOT the fix here, and that is the interesting half. As an entrypoint this module parses nothing, so `if (isEntrypoint(...))` would arm the census on the one run that has nothing to count and leave it silent on every run that does — import-safe and permanently empty. The condition had to MOVE, not acquire a guard: it is now armed, once, by the first parse through any of the three doors. One measurable consequence, stated rather than left to be discovered: a process that imports this module and never parses now prints nothing where it used to print `0 parse(s)`. Nothing read that line — `OS_TOOLING_PARSE_CENSUS` appears in no other file in the tree — and a census whose numerator is zero is the case with nothing to report. The self-test pins BOTH directions, so neither the leak nor the over-correction returns unnoticed: a child that parses still gets `[ts-parse census] 1 parse(s)` on stderr, and a child that only imports gets no census line. The child harness grew an optional env argument for it. Measured with a sentinel-after-import probe: before LEAK scripts/ts-parse.mjs env CENSUS=1 FOREIGN-BYTES=102 after CLEAN scripts/ts-parse.mjs env CENSUS=1 CLI: `node scripts/ts-parse.mjs` is byte-identical (45 bytes). `--self-test` is NOT, by design — it reports `30 cases pass` where it reported `28`, and the case count is the only difference in the line. `check:entry-guard` names the file STALE once it is inert, so its KNOWN_IMPORT_UNSAFE line goes in this same commit: the ledger shrinks 3 -> 2 and now holds only the two files fenced by the changesets-v3 epic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ahemw8RcTgqtxrj15PEZx --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 0cbe79a commit 8e39bbc

4 files changed

Lines changed: 91 additions & 23 deletions

File tree

scripts/check-entry-guard.mjs

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -460,9 +460,6 @@ export function importUnsafeStatements(source) {
460460
const KNOWN_IMPORT_UNSAFE = new Set([
461461
'scripts/check-changeset-no-major.mjs',
462462
'scripts/check-empty-changeset.mjs',
463-
'scripts/objectui-range.mjs',
464-
'scripts/qa/qa-rollup.mjs',
465-
'scripts/ts-parse.mjs',
466463
]);
467464

468465
/**

scripts/objectui-range.mjs

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,15 +95,26 @@ const positional = argv.filter(
9595
const JSON_OUT = has('--json');
9696
const SHOW_EXCLUDED = has('--all');
9797

98-
if (has('-h') || has('--help')) {
98+
/**
99+
* Usage text, read back out of this file's own `//` lines so the two cannot
100+
* drift.
101+
*
102+
* It is a FUNCTION rather than a top-level `if` because this module exports
103+
* bindings. As a bare top-level statement the test read the IMPORTER's argv:
104+
* an importer that happened to carry `-h` or `--help` got this header written
105+
* to its stdout and then `process.exit(0)` — its process ended mid-import
106+
* carrying a SUCCESS status, which no caller reading the status alone can
107+
* tell apart from a clean import.
108+
*/
109+
function printHelp() {
99110
console.log(
100111
readFileSync(fileURLToPath(import.meta.url), 'utf8')
101112
.split('\n')
102113
.filter((l) => l.startsWith('//'))
103114
.map((l) => l.slice(3))
104115
.join('\n'),
105116
);
106-
process.exit(0);
117+
return 0;
107118
}
108119

109120
function die(msg) {
@@ -617,5 +628,6 @@ function selfTest() {
617628
}
618629

619630
if (isEntrypoint(import.meta.url)) {
631+
if (has('-h') || has('--help')) process.exit(printHelp());
620632
process.exit(has('--self-test') ? selfTest() : main());
621633
}

scripts/qa/qa-rollup.mjs

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1125,8 +1125,13 @@ async function selfTest() {
11251125
);
11261126
}
11271127

1128-
if (process.argv.includes('--self-test')) {
1129-
await selfTest();
1130-
} else if (isEntrypoint(import.meta.url)) {
1131-
await main(process.argv.slice(2));
1128+
// The guard comes FIRST, then the mode. The other order — `--self-test`
1129+
// tested before `isEntrypoint` — read the IMPORTER's argv: a tool that imports
1130+
// this module for its exports while carrying `--self-test` in its own argv ran
1131+
// this file's self-test inside itself. That leak is invisible to exit status
1132+
// (this branch does not exit on success) and shows only as foreign output on
1133+
// the importer's stdout.
1134+
if (isEntrypoint(import.meta.url)) {
1135+
if (process.argv.includes('--self-test')) await selfTest();
1136+
else await main(process.argv.slice(2));
11321137
}

scripts/ts-parse.mjs

Lines changed: 68 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -130,8 +130,10 @@
130130
*
131131
* With `OS_TOOLING_PARSE_CENSUS` set, this module prints how many parses ran,
132132
* over how many distinct file names, and how many refused, when the process
133-
* exits. It only ADDS observation. There is deliberately no env var that turns
134-
* the refusal off: a guard with a documented bypass is a guard that will be
133+
* exits. It only ADDS observation. The report is armed by the FIRST PARSE, not
134+
* by the import -- see `armCensusReport` for why the entry-point guard is the
135+
* wrong shape for a library. There is deliberately no env var that turns the
136+
* refusal off: a guard with a documented bypass is a guard that will be
135137
* bypassed.
136138
*/
137139

@@ -172,6 +174,45 @@ export function parseCensus() {
172174
};
173175
}
174176

177+
let censusReportArmed = false;
178+
179+
/**
180+
* Arm the exit report ONCE, on the first parse rather than on the import.
181+
*
182+
* This module is a LIBRARY -- eleven gates in `scripts/` import it for its
183+
* exports -- and the report used to be registered by a top-level
184+
* `if (process.env.OS_TOOLING_PARSE_CENSUS)`. That ran inside every one of
185+
* those importers: a module wrote to a process whose only involvement was
186+
* having loaded it.
187+
*
188+
* The entry-point guard is NOT the fix here, and this is the interesting half.
189+
* As an entrypoint this module parses nothing at all, so
190+
* `if (isEntrypoint(...))` would arm the census on the one run that has
191+
* nothing to count and leave it silent on every run that does -- a report that
192+
* is now import-safe and also permanently empty. The condition had to move,
193+
* not acquire a guard.
194+
*
195+
* So the trigger becomes "this module was USED" instead of "this module was
196+
* LOADED", which is the event the number is about anyway. One measurable
197+
* consequence, stated here rather than left to be discovered: a process that
198+
* imports this module and never parses now prints nothing where it used to
199+
* print `0 parse(s)`. Nothing read that line -- `OS_TOOLING_PARSE_CENSUS`
200+
* appears in no other file in the tree -- and a census whose numerator is zero
201+
* is the case with nothing to report. The self-test pins BOTH directions, so
202+
* neither the leak nor the over-correction can come back unnoticed.
203+
*/
204+
function armCensusReport() {
205+
if (censusReportArmed || !process.env.OS_TOOLING_PARSE_CENSUS) return;
206+
censusReportArmed = true;
207+
process.on('exit', () => {
208+
const c = parseCensus();
209+
process.stderr.write(
210+
`[ts-parse census] ${c.parses} parse(s) over ${c.files} distinct file name(s) `
211+
+ `(${c.programs} program(s), ${c.transpiles} transpile(s)); ${c.refusals} refusal(s)\n`,
212+
);
213+
});
214+
}
215+
175216
/**
176217
* The parse errors TypeScript recorded for `sourceFile`, as plain rows.
177218
*
@@ -310,6 +351,7 @@ export function refusalReport(fileName, scriptKind, diagnostics) {
310351
* return: an unparseable source ends the process with {@link EXIT_UNPARSEABLE}.
311352
*/
312353
export function parseSourceFile(fileName, text, scriptKind) {
354+
armCensusReport();
313355
census.parses += 1;
314356
census.files.add(fileName);
315357

@@ -401,6 +443,7 @@ export function transpileRefusalReport(fileName, rows) {
401443
* return: unparseable sources end the process with {@link EXIT_UNPARSEABLE}.
402444
*/
403445
export function createProgramChecked(rootNames, options, host) {
446+
armCensusReport();
404447
const roots = [...rootNames];
405448
census.programs += 1;
406449
census.parses += roots.length;
@@ -441,6 +484,7 @@ export function createProgramChecked(rootNames, options, host) {
441484
* @returns {ts.TranspileOutput} Output emitted from a source that parsed.
442485
*/
443486
export function transpileChecked(fileName, text, transpileOptions = {}) {
487+
armCensusReport();
444488
census.transpiles += 1;
445489
census.parses += 1;
446490
census.files.add(fileName);
@@ -460,16 +504,6 @@ export function transpileChecked(fileName, text, transpileOptions = {}) {
460504
return result;
461505
}
462506

463-
if (process.env.OS_TOOLING_PARSE_CENSUS) {
464-
process.on('exit', () => {
465-
const c = parseCensus();
466-
process.stderr.write(
467-
`[ts-parse census] ${c.parses} parse(s) over ${c.files} distinct file name(s) `
468-
+ `(${c.programs} program(s), ${c.transpiles} transpile(s)); ${c.refusals} refusal(s)\n`,
469-
);
470-
});
471-
}
472-
473507
// ---------------------------------------------------------------------------
474508
// Self-test -- real child processes, because the refusal IS a process exit
475509
// ---------------------------------------------------------------------------
@@ -508,15 +542,18 @@ export function selfTest() {
508542
// exercises the real export through the real module graph rather than a
509543
// re-implementation of it.
510544
const TS_URL = import.meta.resolve('typescript');
511-
const run = (body) => {
545+
const run = (body, env) => {
512546
const probe = join(dir, `probe-${cases.length}-${Math.random().toString(36).slice(2)}.mjs`);
513547
writeFileSync(
514548
probe,
515549
`import ts from ${JSON.stringify(TS_URL)};\n`
516550
+ `import { parseSourceFile, parseCensus } from ${JSON.stringify(pathToFileURL(SELF).href)};\n`
517551
+ `void ts;\n${body}\n`,
518552
);
519-
const r = spawnSync(process.execPath, [probe], { encoding: 'utf8' });
553+
const r = spawnSync(process.execPath, [probe], {
554+
encoding: 'utf8',
555+
env: env === undefined ? process.env : { ...process.env, ...env },
556+
});
520557
rmSync(probe, { force: true });
521558
return { status: r.status, out: (r.stdout || '').trim(), err: r.stderr || '' };
522559
};
@@ -587,6 +624,23 @@ export function selfTest() {
587624
&& counted.out === '{"parses":3,"programs":0,"transpiles":0,"files":2,"refusals":0}',
588625
JSON.stringify(counted));
589626

627+
// -- and the report is armed by the first PARSE, not by the IMPORT. Both
628+
// directions, because only the pair is a claim: a library that writes
629+
// to your stderr because you imported it is the defect, and a census
630+
// that can no longer report is the over-correction. -------------------
631+
const reported = run(
632+
`parseSourceFile('a.ts', 'const a = 1;');\n`,
633+
{ OS_TOOLING_PARSE_CENSUS: '1' },
634+
);
635+
t('with the census env set, a run that PARSED still reports at exit',
636+
reported.status === 0
637+
&& /\[ts-parse census\] 1 parse\(s\) over 1 distinct file name\(s\)/.test(reported.err),
638+
JSON.stringify(reported));
639+
const importedOnly = run(`void parseCensus();\n`, { OS_TOOLING_PARSE_CENSUS: '1' });
640+
t('…and a run that only IMPORTED this module writes no census line at all',
641+
importedOnly.status === 0 && !importedOnly.err.includes('[ts-parse census]'),
642+
JSON.stringify(importedOnly));
643+
590644
// -- ts.createProgram: the syntax lives behind a SECOND call -------------
591645
const PROGRAM_OPTIONS =
592646
`{ noLib: true, skipLibCheck: true, noEmit: true, types: [],`

0 commit comments

Comments
 (0)