mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-02 15:04:37 +01:00
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>
This commit is contained in:
1 parent
c32bd6cd10
commit
b5491c3d7c
2 files changed
+35
-16
No files matched your search
@@ -94,8 +94,13 @@ export function dartRunner(repoRoot: string, root: string, env: NodeJS.ProcessEn
|
||||
return { runner: `${fvm ? 'fvm ' : ''}${tool} test {files}` };
|
||||
}
|
||||
|
||||
/** `cargo test -p <name>` 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 <name>` 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);
|
||||
|
||||
+23
-11
@@ -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 <name>` 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;
|
||||
|
||||
Reference in new issue
Block a user