mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-05 16:34:00 +01:00
fix: refuse unsafe tree paths before materializeTree writes; trace the CLI key test on the real runtime
materializeTree now checks every path before writing: a .., . or .git segment, an empty segment, a backslash, a duplicate path or an entry under a symlink entry refuses the tree, and symlinks are written last. Failing-first tests on89d4728showed a '..' tree writing into os.tmpdir() and a symlink-then-blob tree writing through the link. The key --base CLI test traced under a hard-coded bun 1.4.4 while the CLI keys on the real runtime (CI runs bun 1.4.2), so the runtime check added in6b4e6c5made it ineligible; it now traces on the real runtime at the current time. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
89d4728ecf
commit
2eeb2ad7ed
2 files changed
+55
-7
No files matched your search
+15
-3
@@ -73,6 +73,7 @@ export function readTreeIndex(repoRoot: string, treeish: string): TreeIndex {
|
||||
if (tab < 0) continue;
|
||||
const [mode, type, oid] = rec.slice(0, tab).split(' ');
|
||||
const rel = rec.slice(tab + 1);
|
||||
if (entries.has(rel)) throw new Error(`${treeish}: duplicate tree path ${JSON.stringify(rel)}`);
|
||||
entries.set(rel, { mode: mode!, type: type!, oid: oid! });
|
||||
const parent = path.posix.dirname(rel);
|
||||
children.set(parent, [...(children.get(parent) ?? []), path.posix.basename(rel)]);
|
||||
@@ -108,12 +109,23 @@ export function catText(repoRoot: string, treeish: string, rel: string): string
|
||||
* A tree-ish written to a fresh temp directory blob by blob from `git cat-file --batch`, with a git index loaded by read-tree,
|
||||
* so the import graph (git ls-files) sees exactly that tree's paths and bytes. Nothing applies attributes: `git archive`
|
||||
* honors export-ignore and checkout-index honors working-tree-encoding and filters, and either would let the tree hide
|
||||
* an import from the graph. Submodules are skipped; symlinks become symlinks.
|
||||
* an import from the graph. Submodules are skipped; symlinks become symlinks, written last. Every path is checked before
|
||||
* anything is written: a `..`, `.` or `.git` segment, an empty segment or a backslash, or an entry under a symlink entry,
|
||||
* refuses the tree, so a hand-built tree object cannot write outside the temp dir or into its .git.
|
||||
*/
|
||||
export function materializeTree(repoRoot: string, treeish: string): string {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-test-impact-tree-'));
|
||||
try {
|
||||
const blobs = [...readTreeIndex(repoRoot, treeish).entries].filter(([, e]) => e.type === 'blob');
|
||||
const { entries } = readTreeIndex(repoRoot, treeish);
|
||||
for (const rel of entries.keys()) {
|
||||
const parts = rel.split('/');
|
||||
if (parts.some(p => p === '' || p === '.' || p === '..' || p.toLowerCase() === '.git' || p.includes('\\'))) throw new Error(`${treeish}: unsafe tree path ${JSON.stringify(rel)}`);
|
||||
for (let i = 1; i < parts.length; i++) {
|
||||
const prefix = parts.slice(0, i).join('/');
|
||||
if (entries.get(prefix)?.mode === '120000') throw new Error(`${treeish}: ${JSON.stringify(rel)} is under symlink ${JSON.stringify(prefix)}`);
|
||||
}
|
||||
}
|
||||
const blobs = [...entries].filter(([, e]) => e.type === 'blob').sort(([, a], [, b]) => Number(a.mode === '120000') - Number(b.mode === '120000'));
|
||||
const buf = blobs.length ? git(repoRoot, ['cat-file', '--batch'], blobs.map(([, e]) => e.oid).join('\n') + '\n') : Buffer.alloc(0);
|
||||
let at = 0;
|
||||
for (const [rel, e] of blobs) {
|
||||
@@ -124,7 +136,7 @@ export function materializeTree(repoRoot: string, treeish: string): string {
|
||||
const abs = path.join(dir, rel);
|
||||
fs.mkdirSync(path.dirname(abs), { recursive: true });
|
||||
if (e.mode === '120000') fs.symlinkSync(bytes.toString('utf8'), abs);
|
||||
else fs.writeFileSync(abs, bytes, { mode: e.mode === '100755' ? 0o755 : 0o644 });
|
||||
else fs.writeFileSync(abs, bytes, { mode: e.mode === '100755' ? 0o755 : 0o644, flag: 'wx' });
|
||||
}
|
||||
git(dir, ['init', '-q']);
|
||||
git(repoRoot, ['read-tree', treeish], undefined, { ...process.env, GIT_INDEX_FILE: path.join(dir, '.git', 'index') });
|
||||
|
||||
@@ -6,10 +6,11 @@
|
||||
import { afterEach, describe, expect, test } from 'bun:test';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import * as fs from 'node:fs';
|
||||
import * as os from 'node:os';
|
||||
import * as path from 'node:path';
|
||||
import { bunIsolationArgs } from '../lib/bun-isolation';
|
||||
import { ALWAYS_RUN_PATH } from '../lib/pregate/lane-selection';
|
||||
import { computeKeys, MAX_HOPS, type KeyResult } from '../lib/test-impact/key';
|
||||
import { computeKeys, materializeTree, MAX_HOPS, type KeyResult } from '../lib/test-impact/key';
|
||||
import { TRACE_VERSION, envDigests, parseStrace, readTrace, tracePath, type TraceRecord } from '../lib/test-impact/trace';
|
||||
import { cleanup, commit, makeFixtureRepo, type FixtureRepo } from './helpers/pregate-fixture';
|
||||
|
||||
@@ -28,7 +29,7 @@ function fixture(baseFiles: Record<string, string> = {}): FixtureRepo {
|
||||
}
|
||||
|
||||
/** Write `test`'s trace from strace lines that open `reads`, list `lists` and add `extra` raw calls, through parseStrace. */
|
||||
function traceFrom(repo: FixtureRepo, test: string, opts: { reads?: string[]; lists?: Record<string, string[]>; extra?: string[]; tracedAt?: string; env?: Record<string, string | undefined> } = {}): string {
|
||||
function traceFrom(repo: FixtureRepo, test: string, opts: { reads?: string[]; lists?: Record<string, string[]>; extra?: string[]; tracedAt?: string; env?: Record<string, string | undefined>; runtime?: TraceRecord['runtime'] } = {}): string {
|
||||
const w = repo.work;
|
||||
const lines = [test, ...(opts.reads ?? [])].map(p => `100 openat(AT_FDCWD<${w}>, "${w}/${p}", O_RDONLY|O_CLOEXEC) = 3<${w}/${p}>`);
|
||||
for (const [dir, names] of Object.entries(opts.lists ?? {})) lines.push(`100 getdents64(4<${w}/${dir}>, [${['.', '..', ...names].map(n => `{d_ino=1, d_off=1, d_reclen=24, d_type=DT_REG, d_name="${n}"}`).join(', ')}], 32768) = 96`);
|
||||
@@ -36,7 +37,7 @@ function traceFrom(repo: FixtureRepo, test: string, opts: { reads?: string[]; li
|
||||
const dir = path.join(repo.root, 'traces');
|
||||
fs.mkdirSync(dir, { recursive: true });
|
||||
const set = parseStrace(lines.join('\n'), { repoRoot: w, home: repo.home, tmpDir: path.join(repo.root, 'run'), sharedTmp: [] });
|
||||
const record: TraceRecord = { version: TRACE_VERSION, test, tracedAt: opts.tracedAt ?? '2026-11-01T00:00:00Z', commit: null, exitCode: 0, runtime: RUNTIME, env: envDigests(opts.env ?? {}), ...set };
|
||||
const record: TraceRecord = { version: TRACE_VERSION, test, tracedAt: opts.tracedAt ?? '2026-11-01T00:00:00Z', commit: null, exitCode: 0, runtime: opts.runtime ?? RUNTIME, env: envDigests(opts.env ?? {}), ...set };
|
||||
fs.writeFileSync(tracePath(dir, test), JSON.stringify(record));
|
||||
return dir;
|
||||
}
|
||||
@@ -213,6 +214,41 @@ describe('test-impact key: ineligible', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('test-impact materializeTree: a hostile tree cannot write outside its temp dir', () => {
|
||||
const hashTree = (repo: FixtureRepo, entries: [string, string, string][]) => {
|
||||
const body = Buffer.concat(entries.map(([mode, name, oid]) => Buffer.concat([Buffer.from(`${mode} ${name}\0`), Buffer.from(oid, 'hex')])));
|
||||
const r = spawnSync('git', ['hash-object', '-t', 'tree', '-w', '--literally', '--stdin'], { cwd: repo.work, input: body, encoding: 'utf8', timeout: 60_000 });
|
||||
expect(r.status).toBe(0);
|
||||
return r.stdout.trim();
|
||||
};
|
||||
const attempt = (fn: () => string) => { try { fs.rmSync(fn(), { recursive: true, force: true }); return 'did not throw'; } catch (error) { return String(error); } };
|
||||
const hashBlob = (repo: FixtureRepo, text: string) => spawnSync('git', ['hash-object', '-w', '--stdin'], { cwd: repo.work, input: text, encoding: 'utf8', timeout: 60_000 }).stdout.trim();
|
||||
|
||||
test('a tree entry named .. (or .git) throws and writes nothing outside the temp dir', () => {
|
||||
const repo = fixture();
|
||||
const victim = `gstack-test-impact-victim-${process.pid}-${Date.now()}`;
|
||||
const outside = path.join(os.tmpdir(), victim);
|
||||
const evil = hashTree(repo, [['100644', 'evil.txt', hashBlob(repo, 'pwned\n')]]);
|
||||
const victimTree = hashTree(repo, [['40000', victim, evil]]);
|
||||
try {
|
||||
const thrown = attempt(() => materializeTree(repo.work, hashTree(repo, [['40000', '..', victimTree]])));
|
||||
expect(fs.existsSync(outside)).toBe(false);
|
||||
expect(thrown).toMatch(/unsafe tree path/);
|
||||
expect(() => materializeTree(repo.work, hashTree(repo, [['40000', '.git', hashTree(repo, [['40000', 'hooks', evil]])]]))).toThrow(/unsafe tree path/);
|
||||
} finally { fs.rmSync(outside, { recursive: true, force: true }); }
|
||||
});
|
||||
|
||||
test('a blob under a symlink entry (a -> outside dir, a/x) throws and writes nothing through the link', () => {
|
||||
const repo = fixture();
|
||||
const outside = fs.mkdtempSync(path.join(repo.root, 'outside-'));
|
||||
const sub = hashTree(repo, [['100644', 'x', hashBlob(repo, 'pwned\n')]]);
|
||||
const tree = hashTree(repo, [['40000', 'a', sub], ['120000', 'a', hashBlob(repo, outside)]]);
|
||||
const thrown = attempt(() => materializeTree(repo.work, tree));
|
||||
expect(fs.readdirSync(outside)).toEqual([]);
|
||||
expect(thrown).toMatch(/duplicate tree path|under symlink/);
|
||||
});
|
||||
});
|
||||
|
||||
describe('scripts/test-impact.ts', () => {
|
||||
test('key --base runs the base ref\'s key code, not this checkout\'s, and refuses when the base has none', () => {
|
||||
const repo = fixture();
|
||||
@@ -229,7 +265,7 @@ describe('scripts/test-impact.ts', () => {
|
||||
expect(trusted.out).toContain('key code from main:lib/test-impact/key.ts');
|
||||
expect(trusted.out).toContain('INELIGIBLE test/util.test.js: stub key code from the base ref');
|
||||
|
||||
traceFrom(repo, 'test/util.test.js', { env: { ...process.env, ...repo.env } });
|
||||
traceFrom(repo, 'test/util.test.js', { env: { ...process.env, ...repo.env }, runtime: { platform: process.platform, arch: process.arch, bun: Bun.version }, tracedAt: new Date().toISOString() });
|
||||
const local = cli(repo, ['explain', '--tree', 'HEAD', '--traces', path.join(repo.root, 'traces'), 'test/util.test.js']);
|
||||
expect(local.status).toBe(0);
|
||||
expect(local.out).toMatch(/KEY test\/util\.test\.js [0-9a-f]{12}/);
|
||||
|
||||
Reference in new issue
Block a user