enhance(#3619): ratchet the platform seam with local/no-private-binary-resolution (epic #3411 Phase 3) (#3636)
* chore(#3619): ratchet the platform seam with local/no-private-binary-resolution
Epic #3411 Phase 3, the ratchet. Scope revised with maintainer approval and
recorded on the issue: the epic's literal ask was a rule rejecting a bare-name
spawn outside the seam. Surveyed at ac1b6d679, ~30 such sites exist and none is
a defect — git, gh and npm ship native .exe that CreateProcess resolves unaided,
and the rest are POSIX-only tools. ADR-1703 rules 2 and 3 forbid grandfathering
and escape hatches, so a literal rule would be unsuppressable and would force
rewriting 30 correct calls.
The epic's actual thesis was four private RESOLVERS, not four bare spawns. So
the rule flags re-implementing resolution: reading PATHEXT in any casing from
any object, and a hardcoded list carrying two or more of .exe/.cmd/.bat/.com —
precisely the shapes fallow-runner's candidateNames and gsd-tools' PATHEXT
string had before Phases 1 and 2 deleted them.
Three boundaries were arrived at rather than assumed:
two-or-more a single .endsWith('.cmd') is a classification, not a candidate
set; runtime-hooks-surface derives .cmd shim paths that way
boundary-aware a naive substring test flags .execute and .compacting, caught
on src/host-integration.cts before it could become a false
positive nobody could suppress
suffix-anchored the seam exemption matches src/shell-command-projection.cts
exactly; a substring match would also exempt the dispatch test
file. Case I9 pins it.
PATH scans are deliberately NOT flagged — membership checks (bin/install.js)
are indistinguishable from resolution scans, and an unsound rule in a
zero-escape-hatch architecture is worse than no rule.
To make the ratchet strict with no carve-out, resolveExecutableBinary gained
pathOverride: search THIS PATH, read everything else including PATHEXT from the
ambient environment. resolveFallowBinary now supplies its own search path
without hand-threading PATHEXT, which would itself have been a private read.
The three alternatives were all worse: exempting the file is grandfathering,
exempting the AST shape is a carve-out every future caller must replicate, and
dropping the pass-through would silently ignore a user's real PATHEXT — buying
a lint rule with a correctness regression.
eslint-rules/** is outside the rule's globs rather than exempted, because
portability-vocab.cjs owns the extension set. scripts/**/*.cjs got its own block
so that exclusion does not leave a hole in the ratchet.
Started green with nothing suppressed. Proven able to fail: a fixture with both
signals reports two errors.
Refs #3411
* fix(#3619): close the PATHEXT destructuring evasion and correct two overclaims
Adversarial review found a trivial evasion of the rule's primary signal: the
visitor only handled MemberExpression, so
const { PATHEXT } = process.env
const { PATHEXT: exts } = process.env
const { Pathext } = opts.env
were all unflagged. That is a common idiom, not an exotic bypass. An ObjectPattern
visitor now catches it in every form — renamed, any casing, any receiver, string
keys — while leaving a computed key alone, since it is not statically decidable.
I10-I13 pin the invalid forms and V9/V10 pin PATH and the computed key.
Two overclaims corrected, both mine:
Standards review proved the docs were factually wrong. Both the ADR amendment and
the CONTEXT.md entry asserted that tests/shell-command-projection-dispatch.test.cjs
is still linted by this rule. It is not — the rule's surface is src, gsd-core/bin,
scripts and hooks, and tests/** is deliberately outside it because test setup
legitimately assigns process.env.PATHEXT (fallow-runner's P3 does exactly that).
The suffix-vs-substring distinction is therefore proven by RuleTester case I9
feeding a synthetic filename, NOT by real coverage of that file. Both documents now
say so.
The rule's own docstring claimed the seam exemption matches the seam path
'exactly'. It is a suffix match, so a nested foo/src/shell-command-projection.cts
would also be exempt. Suffix matching is kept — it is how sibling rules resolve
paths and the nested case does not exist — but the docstring now states the
boundary rather than overstating the precision.
The evasion fix was verified by executing eslint against both destructuring forms
in scripts/, not by inspection. Probe: 31/31.
Refs #3411
* chore(#3619): backfill changeset pr number 3636
---------
Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
@@ -635,6 +635,125 @@ describe('resolveExecutableBinary seam options (#3618)', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ─── resolveExecutableBinary pathOverride (#3619, epic #3411 Phase 3) ──────
|
||||
// "Search THIS PATH, but read everything else — PATHEXT included — from the
|
||||
// ambient environment." Added so resolveFallowBinary can hand the seam a
|
||||
// search path without also hand-threading PATHEXT (a private PATHEXT read
|
||||
// is exactly the shape local/no-private-binary-resolution forbids outside
|
||||
// this seam). See .gsd/phase/chore-3619-no-bare-binary-spawn/50-test-matrix.md (O1-O7).
|
||||
|
||||
describe('resolveExecutableBinary pathOverride (#3619)', () => {
|
||||
test('O1: pathOverride omitted — identical to today, env.PATH is used', () => {
|
||||
const dir = createTempDir();
|
||||
try {
|
||||
fs.writeFileSync(path.join(dir, 'foo'), '');
|
||||
const resolved = resolveExecutableBinary('foo', { platform: 'linux', env: { PATH: dir } });
|
||||
assert.equal(resolved, path.join(dir, 'foo'));
|
||||
} finally {
|
||||
cleanup(dir);
|
||||
}
|
||||
});
|
||||
|
||||
test('O2: pathOverride set, binary in the pathOverride dir, env.PATH points elsewhere — resolves via pathOverride, env.PATH ignored', () => {
|
||||
const dirA = createTempDir();
|
||||
const dirElsewhere = createTempDir();
|
||||
try {
|
||||
fs.writeFileSync(path.join(dirA, 'foo'), '');
|
||||
const resolved = resolveExecutableBinary('foo', {
|
||||
platform: 'linux',
|
||||
env: { PATH: dirElsewhere },
|
||||
pathOverride: dirA,
|
||||
});
|
||||
assert.equal(resolved, path.join(dirA, 'foo'));
|
||||
} finally {
|
||||
cleanup(dirA);
|
||||
cleanup(dirElsewhere);
|
||||
}
|
||||
});
|
||||
|
||||
test("O3: pathOverride: '' with a populated env.PATH — null; empty means empty, not a fallback to env.PATH", () => {
|
||||
const dir = createTempDir();
|
||||
try {
|
||||
fs.writeFileSync(path.join(dir, 'foo'), '');
|
||||
const resolved = resolveExecutableBinary('foo', {
|
||||
platform: 'linux',
|
||||
env: { PATH: dir },
|
||||
pathOverride: '',
|
||||
});
|
||||
assert.equal(resolved, null);
|
||||
} finally {
|
||||
cleanup(dir);
|
||||
}
|
||||
});
|
||||
|
||||
test('O4: pathOverride + prependPaths — prepended dirs still come first', () => {
|
||||
const dirPrepend = createTempDir();
|
||||
const dirOverride = createTempDir();
|
||||
try {
|
||||
fs.writeFileSync(path.join(dirPrepend, 'foo'), '');
|
||||
fs.writeFileSync(path.join(dirOverride, 'foo'), '');
|
||||
const resolved = resolveExecutableBinary('foo', {
|
||||
platform: 'linux',
|
||||
pathOverride: dirOverride,
|
||||
prependPaths: [dirPrepend],
|
||||
});
|
||||
assert.equal(resolved, path.join(dirPrepend, 'foo'));
|
||||
assert.notEqual(resolved, path.join(dirOverride, 'foo'));
|
||||
} finally {
|
||||
cleanup(dirPrepend);
|
||||
cleanup(dirOverride);
|
||||
}
|
||||
});
|
||||
|
||||
test('O5: pathOverride set, opts.env omitted, win32 — PATHEXT still comes from the ambient process.env, not DEFAULT_PATHEXT', () => {
|
||||
const dir = createTempDir();
|
||||
const originalPathext = process.env.PATHEXT;
|
||||
try {
|
||||
process.env.PATHEXT = '.XYZ';
|
||||
fs.writeFileSync(path.join(dir, 'foo.XYZ'), '');
|
||||
const resolved = resolveExecutableBinary('foo', { platform: 'win32', pathOverride: dir });
|
||||
assert.equal(resolved, path.join(dir, 'foo.XYZ'));
|
||||
} finally {
|
||||
if (originalPathext === undefined) delete process.env.PATHEXT; else process.env.PATHEXT = originalPathext;
|
||||
cleanup(dir);
|
||||
}
|
||||
});
|
||||
|
||||
test('O6: pathOverride AND env.PATH both set — pathOverride wins (assert WHICH path resolved, not merely that something resolved)', () => {
|
||||
const dirOverride = createTempDir();
|
||||
const dirEnvPath = createTempDir();
|
||||
try {
|
||||
fs.writeFileSync(path.join(dirOverride, 'foo'), '');
|
||||
fs.writeFileSync(path.join(dirEnvPath, 'foo'), '');
|
||||
const resolved = resolveExecutableBinary('foo', {
|
||||
platform: 'linux',
|
||||
env: { PATH: dirEnvPath },
|
||||
pathOverride: dirOverride,
|
||||
});
|
||||
assert.equal(resolved, path.join(dirOverride, 'foo'));
|
||||
assert.notEqual(resolved, path.join(dirEnvPath, 'foo'));
|
||||
} finally {
|
||||
cleanup(dirOverride);
|
||||
cleanup(dirEnvPath);
|
||||
}
|
||||
});
|
||||
|
||||
test('O7: multi-segment pathOverride, binary in the second segment — first-match wins, in order', () => {
|
||||
const dir1 = createTempDir();
|
||||
const dir2 = createTempDir();
|
||||
try {
|
||||
fs.writeFileSync(path.join(dir2, 'foo'), '');
|
||||
const pathOverride = [dir1, dir2].join(path.delimiter);
|
||||
const resolved = resolveExecutableBinary('foo', { platform: 'linux', pathOverride });
|
||||
assert.equal(resolved, path.join(dir2, 'foo'));
|
||||
assert.notEqual(resolved, path.join(dir1, 'foo'));
|
||||
} finally {
|
||||
cleanup(dir1);
|
||||
cleanup(dir2);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
// ─── projectSpawnInvocation (#3411) ─────────────────────────────────────────
|
||||
|
||||
// Mirrors the seam's own `_cmdQuoteToken` (a literal `"` is doubled) so
|
||||
|
||||
Reference in New Issue
Block a user