From b5491c3d7cb33663905e8175479ada1b7c08e625 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 24 Oct 2026 20:29:34 +0000 Subject: [PATCH] fix(pregate): a crate name Cargo would reject never reaches the lane's shell Port review (outside voice): quoting alone was not enough, because the lane expands {files} after the runner string is built, so a crate named 'evil{files}' spliced the quoted file list into the quoted name and a file name like tests/$(cmd).rs then ran. A name outside Cargo's grammar (letters, digits, _ and -) now gets no runner: its files are unclaimed and the lane is incomplete, as Cargo itself would refuse the manifest. A valid name, Unicode included, still runs quoted. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/pregate/checks/lanes.ts | 17 ++++++++++++----- test/pregate-rust.test.ts | 34 +++++++++++++++++++++++----------- 2 files changed, 35 insertions(+), 16 deletions(-) diff --git a/lib/pregate/checks/lanes.ts b/lib/pregate/checks/lanes.ts index fd2d7d5c5..b2c3aaca1 100644 --- a/lib/pregate/checks/lanes.ts +++ b/lib/pregate/checks/lanes.ts @@ -94,8 +94,13 @@ export function dartRunner(repoRoot: string, root: string, env: NodeJS.ProcessEn return { runner: `${fvm ? 'fvm ' : ''}${tool} test {files}` }; } -/** `cargo test -p ` for a crate (cargo runs a crate's tests, never a file list), or the missing tool when cargo is not on PATH. The name comes from the repo's Cargo.toml and reaches a shell, so it is quoted like every file and cwd. */ -function cargoRunner(name: string, env: NodeJS.ProcessEnv): { runner: string } | { missing: string } { +/** + * `cargo test -p ` for a crate (cargo runs a crate's tests, never a file list), or the missing tool when cargo is + * not on PATH. The name comes from the repo's Cargo.toml and reaches a shell and the `{files}` expansion: a name outside + * Cargo's grammar (letters, digits, `_`, `-`) gets no runner (null), and a valid one is still quoted like every file and cwd. + */ +function cargoRunner(name: string, env: NodeJS.ProcessEnv): { runner: string } | { missing: string } | null { + if (!/^[\p{ID_Continue}-]+$/u.test(name)) return null; return onPath('cargo', env) ? { runner: `cargo test -p ${quote(name)}` } : { missing: 'cargo' }; } @@ -149,8 +154,8 @@ export interface RunPlan { groups: RunGroup[]; unclaimed: string[]; missing: str * from the crate's workspace root; JS/TS tests owned by a nested package that * declares vitest or jest run from that package; the rest go to the detected * runner from the repo root. Files no runner claims (a tool missing from PATH, - * a `.dart` file outside any package, a `.rs` file outside any crate, no - * detected runner) are `unclaimed`: the lane is `incomplete`, never a pass. + * a `.dart` file outside any package, a `.rs` file outside any crate or in a + * crate whose name Cargo would reject, no detected runner) are `unclaimed`: the lane is `incomplete`, never a pass. */ export function planRuns(ctx: PregateContext, files: readonly string[]): RunPlan { if (ctx.config.runner) return { groups: [{ runner: ctx.config.runner, cwd: '', files: [...files] }], unclaimed: [], missing: [], noPubspec: [] }; @@ -197,7 +202,9 @@ export function planRuns(ctx: PregateContext, files: readonly string[]): RunPlan } for (const [, c] of [...cargo].sort(([a], [b]) => a.localeCompare(b))) { const r = cargoRunner(c.name, ctx.env); - if ('missing' in r) { missing.add(r.missing); unclaimed.push(...c.files.map(f => (c.cwd ? `${c.cwd}/${f}` : f))); } else groups.push({ runner: r.runner, cwd: c.cwd, files: c.files }); + if (r && 'runner' in r) { groups.push({ runner: r.runner, cwd: c.cwd, files: c.files }); continue; } + if (r) missing.add(r.missing); + unclaimed.push(...c.files.map(f => (c.cwd ? `${c.cwd}/${f}` : f))); } if (rest.length) { const runner = detectRunner(ctx, rest); diff --git a/test/pregate-rust.test.ts b/test/pregate-rust.test.ts index ddfe30d7f..c9691e6a1 100644 --- a/test/pregate-rust.test.ts +++ b/test/pregate-rust.test.ts @@ -243,23 +243,35 @@ describe('gstack-pregate on a Rust workspace', () => { expect(red.out).toMatch(/^tests {6}lanes {5}fail .*local lane exited 101 \(1 failed of 2 ran; 1 files; pipefail\)/m); }, 60_000); - // Value: protects=a crate name is data from the repo's Cargo.toml and reaches the lane's shell, so it is quoted like every - // file and cwd; fails_when=`cargo test -p ` is built unquoted and a name with shell syntax runs a command; - // why_new=the crate runner was new; seam=none; no_claim=a real cargo (PATH shim echoes its argv) - test('a crate name with shell syntax reaches cargo as one argument and runs nothing', () => { + // Value: protects=a crate name is data from the repo's Cargo.toml that reaches the lane's shell and the {files} expansion, + // so a name Cargo would reject never runs (its files are unclaimed, the lane incomplete) and a valid one, Unicode + // included, still runs quoted; fails_when=the name is interpolated unchecked (`;touch` runs a command, `{files}` splices + // the quoted file list into the name); why_new=the crate runner was new; seam=none; no_claim=a real cargo (PATH shim) + test('a crate name with shell syntax or {files} never reaches the shell; a valid Unicode name still runs', () => { const repo = rustRepo(); + const lib = '#[cfg(test)]\nmod tests {\n #[test]\n fn t() {}\n}\n'; commit(repo, { 'engine/crates/evil/Cargo.toml': '[package]\nname = "evil;touch pwned-by-crate-name"\nversion = "0.1.0"\n', - 'engine/crates/evil/src/lib.rs': '#[cfg(test)]\nmod tests {\n #[test]\n fn t() {}\n}\n', - }, 'hostile crate'); + 'engine/crates/evil/src/lib.rs': lib, + 'engine/crates/splice/Cargo.toml': '[package]\nname = "evil{files}"\nversion = "0.1.0"\n', + 'engine/crates/splice/src/lib.rs': lib, + 'engine/crates/uni/Cargo.toml': '[package]\nname = "naïve_crate-2"\nversion = "0.1.0"\n', + 'engine/crates/uni/src/lib.rs': lib, + }, 'odd crate names'); repo.git('push', '-q', 'origin', 'HEAD:main'); repo.git('fetch', '-q', 'origin'); - commit(repo, { 'engine/crates/evil/src/lib.rs': '#[cfg(test)]\nmod tests {\n #[test]\n fn t() { }\n}\n' }); - const r = run(repo, cargoShim(repo)); + const env = cargoShim(repo); + const ctx = { repoRoot: repo.work, config: DEFAULT_CONFIG, env } as unknown as PregateContext; + expect(planRuns(ctx, ['engine/crates/evil/src/lib.rs', 'engine/crates/splice/src/lib.rs', 'engine/crates/uni/src/lib.rs'])).toEqual({ + groups: [{ runner: "cargo test -p 'naïve_crate-2'", cwd: 'engine', files: ['crates/uni/src/lib.rs'] }], + unclaimed: ['engine/crates/evil/src/lib.rs', 'engine/crates/splice/src/lib.rs'], missing: [], noPubspec: [], + }); + commit(repo, { 'engine/crates/evil/src/lib.rs': lib.replace('fn t() {}', 'fn t() { }') }); + const r = run(repo, env); expect(fs.existsSync(path.join(repo.work, 'engine', 'pwned-by-crate-name'))).toBe(false); - expect(r.out).toContain("ran: (cd 'engine' && cargo test -p 'evil;touch pwned-by-crate-name')"); - expect(log(r.out)).toContain('ran in engine: test -p evil;touch pwned-by-crate-name'); - expect(r.status).toBe(EXIT.ok); + expect(r.out).not.toContain('cargo test -p'); + expect(r.out).toContain('engine/crates/evil/src/lib.rs'); + expect(r.status).not.toBe(EXIT.ok); }, 60_000); // Value: protects=a crate with no tests still fails closed (no lane), so the crate mapping never invents coverage;