From 2eeb2ad7ed1b1106e6eeb95e7fc23da1f858b946 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 16:00:42 +0000 Subject: [PATCH] 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 on 89d4728 showed 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 in 6b4e6c5 made 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> --- lib/test-impact/key.ts | 18 ++++++++++++--- test/test-impact-key.test.ts | 44 ++++++++++++++++++++++++++++++++---- 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/lib/test-impact/key.ts b/lib/test-impact/key.ts index 46289d2e5..76ec75196 100644 --- a/lib/test-impact/key.ts +++ b/lib/test-impact/key.ts @@ -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') }); diff --git a/test/test-impact-key.test.ts b/test/test-impact-key.test.ts index d1d4c42a2..af28bdf51 100644 --- a/test/test-impact-key.test.ts +++ b/test/test-impact-key.test.ts @@ -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 = {}): 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; extra?: string[]; tracedAt?: string; env?: Record } = {}): string { +function traceFrom(repo: FixtureRepo, test: string, opts: { reads?: string[]; lists?: Record; extra?: string[]; tracedAt?: string; env?: Record; 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}/);