diff --git a/scripts/astra-review.js b/scripts/astra-review.js index f4c34177b..f35aa8185 100644 --- a/scripts/astra-review.js +++ b/scripts/astra-review.js @@ -26,7 +26,9 @@ 'use strict'; +const crypto = require('crypto'); const fs = require('fs'); +const path = require('path'); const { collectChanges, describeScope, parseScope } = require('./lib/astra-review/scope'); const { buildPrompt, effectiveVerdict, formatReport } = require('./lib/astra-review/prompt'); const codex = require('./lib/astra-review/codex'); @@ -37,20 +39,31 @@ const EXIT_FAIL = 1; const EXIT_ERROR = 2; /** - * Write the JSON report without following a pre-existing symlink (a - * predictable path could otherwise be pointed at another file) and readable - * only by the current user. + * Write the JSON report atomically: the content goes into a fresh, exclusively + * created 0600 temp file in the same directory, which is then renamed over the + * destination. A pre-existing symlink or world-readable file at the destination + * is replaced, never followed or reused, so there is no check-then-write race. */ function writeReport(file, content, io = fs) { - let existing = null; + const temp = path.join( + path.dirname(file), + `.${path.basename(file)}.${process.pid}.${crypto.randomBytes(6).toString('hex')}.tmp` + ); + const fd = io.openSync(temp, 'wx', REPORT_FILE_MODE); + let published = false; try { - existing = io.lstatSync(file); - } catch (error) { - if (!error || error.code !== 'ENOENT') throw error; + // writeFileSync on a descriptor loops until every byte is written; a bare + // writeSync can return short and would publish a truncated report. + io.writeFileSync(fd, content, 'utf8'); + io.closeSync(fd); + io.renameSync(temp, file); + published = true; + } finally { + if (!published) { + try { io.closeSync(fd); } catch { /* already closed */ } + try { io.unlinkSync(temp); } catch { /* nothing left to clean */ } + } } - if (existing && existing.isSymbolicLink()) throw new Error(`refusing to write report through symlink: ${file}`); - if (existing && !existing.isFile()) throw new Error(`report path is not a regular file: ${file}`); - io.writeFileSync(file, content, { encoding: 'utf8', mode: REPORT_FILE_MODE }); } function usage() { diff --git a/scripts/lib/astra-review/scope.js b/scripts/lib/astra-review/scope.js index 3dd42adce..b62973b82 100644 --- a/scripts/lib/astra-review/scope.js +++ b/scripts/lib/astra-review/scope.js @@ -193,10 +193,15 @@ function readForReview(absolute, root, io) { return io.readFile(resolved); } +/** Git-style (forward-slash) display path, so reports look the same on every OS. */ +function toGitPath(filePath) { + return filePath.split(path.sep).join('/'); +} + function collectFiles(io, files, cwd, root) { const entries = files.map((file) => { const absolute = path.resolve(cwd, file); - const display = path.relative(root, absolute) || file; + const display = toGitPath(path.relative(root, absolute)) || file; return { display, content: readForReview(absolute, root, io) }; }); return { @@ -290,12 +295,13 @@ function safeRealpath(target) { function collectChanges(scope, deps = {}) { const realpath = deps.realpathSync || safeRealpath; - // Canonical paths keep the containment check honest when /tmp is a symlink. - const cwd = realpath(deps.cwd || process.cwd()); + // Canonical, platform-native paths keep the containment check honest when + // /tmp is a symlink or when git reports a forward-slash path on Windows. + const cwd = realpath(path.resolve(deps.cwd || process.cwd())); const runGitIn = deps.runGit || defaultRunGit; // Always work from the repository root so paths are root-relative and // untracked files outside the current subdirectory are not missed. - const root = realpath(runGitIn(['rev-parse', '--show-toplevel'], cwd).trim() || cwd); + const root = realpath(path.resolve(runGitIn(['rev-parse', '--show-toplevel'], cwd).trim() || cwd)); const runGit = (args, options) => runGitIn(args, root, options); const io = { readFile: deps.readFile || ((file) => fs.readFileSync(file, 'utf8')), diff --git a/tests/lib/astra-review-scope.test.js b/tests/lib/astra-review-scope.test.js index d989d6704..15187180c 100644 --- a/tests/lib/astra-review-scope.test.js +++ b/tests/lib/astra-review-scope.test.js @@ -25,6 +25,11 @@ function test(name, fn) { } } +// Git reports slash-separated paths; the filesystem side must be native so +// these fixtures hold on Windows as well as POSIX. +const ROOT = path.resolve('/repo'); +const R = (...parts) => path.join(ROOT, ...parts); +const OUTSIDE = path.resolve('/outside'); const REGULAR_FILE = () => ({ isSymbolicLink: () => false, isFile: () => true }); const enoent = () => { throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); }; const lstatOnly = (present) => (file) => (present.includes(file) ? REGULAR_FILE() : enoent()); @@ -95,7 +100,7 @@ test('collectChanges (uncommitted) merges staged, unstaged, and untracked change assert.ok(result.diff.includes('+staged change')); assert.ok(result.diff.includes('+unstaged change')); assert.ok(result.diff.includes('+brand new')); - assert.strictEqual(result.root, '/repo'); + assert.strictEqual(result.root, ROOT); assert.strictEqual(result.truncated, false); }); @@ -122,10 +127,10 @@ test('collectChanges runs git from the repository root, not the current subdirec return ''; }; - scope.collectChanges({ kind: 'uncommitted', value: null }, { runGit, cwd: '/repo/scripts' }); + scope.collectChanges({ kind: 'uncommitted', value: null }, { runGit, cwd: R('scripts') }); - assert.strictEqual(cwds[0], '/repo/scripts'); - assert.ok(cwds.slice(1).every((cwd) => cwd === '/repo'), `expected /repo, got ${cwds.slice(1)}`); + assert.strictEqual(cwds[0], R('scripts')); + assert.ok(cwds.slice(1).every((cwd) => cwd === ROOT), `expected ${ROOT}, got ${cwds.slice(1)}`); }); test('collectChanges against a base branch diffs merge-base to the working tree, plus untracked files', () => { @@ -177,7 +182,7 @@ test('collectChanges for explicit files reads file contents', () => { const result = scope.collectChanges( { kind: 'files', value: ['one.js', 'two.js'] }, - { runGit: () => '', readFile, cwd: '/repo', lstatSync: REGULAR_FILE } + { runGit: () => '', readFile, cwd: ROOT, lstatSync: REGULAR_FILE } ); assert.deepStrictEqual(result.files, ['one.js', 'two.js']); @@ -192,10 +197,10 @@ test('collectChanges resolves explicit relative paths against the invocation dir const result = scope.collectChanges( { kind: 'files', value: ['AGENTS.md'] }, - { runGit, readFile, cwd: '/repo/docs/zh-CN', lstatSync: REGULAR_FILE } + { runGit, readFile, cwd: R('docs', 'zh-CN'), lstatSync: REGULAR_FILE } ); - assert.deepStrictEqual(read, ['/repo/docs/zh-CN/AGENTS.md']); + assert.deepStrictEqual(read, [R('docs', 'zh-CN', 'AGENTS.md')]); assert.deepStrictEqual(result.files, ['docs/zh-CN/AGENTS.md']); assert.ok(result.diff.includes('=== docs/zh-CN/AGENTS.md ===')); }); @@ -262,7 +267,7 @@ test('collectChanges returns a recovery hint for index and working tree on uncom test('collectChanges returns a recovery hint that reads files directly for explicit files', () => { const result = scope.collectChanges( { kind: 'files', value: ['a.js'] }, - { runGit: () => '', readFile: () => 'x', cwd: '/repo', lstatSync: REGULAR_FILE } + { runGit: () => '', readFile: () => 'x', cwd: ROOT, lstatSync: REGULAR_FILE } ); assert.ok(/read/i.test(result.recovery)); @@ -283,7 +288,7 @@ test('files-from-commit reviews current contents of touched files and keeps stil ]); const readFile = (file) => `now: ${path.basename(file)}`; - const result = scope.collectChanges({ kind: 'files-from-commit', value: 'HEAD' }, { runGit: git.runGit, readFile, cwd: '/repo/sub', lstatSync: lstatOnly(['/repo/kept.js']) }); + const result = scope.collectChanges({ kind: 'files-from-commit', value: 'HEAD' }, { runGit: git.runGit, readFile, cwd: R('sub'), lstatSync: lstatOnly([R('kept.js')]) }); assert.deepStrictEqual(result.files, ['kept.js', 'gone-later.js']); assert.ok(result.diff.includes('now: kept.js')); @@ -299,7 +304,7 @@ test('files-from-commit handles a root commit', () => { [/^diff-tree --no-renames --no-commit-id --root -r --name-only -z a00b000$/, 'init.js\0'], ]); - const result = scope.collectChanges({ kind: 'files-from-commit', value: 'a00b000' }, { runGit: git.runGit, readFile: () => 'x', cwd: '/repo', lstatSync: REGULAR_FILE }); + const result = scope.collectChanges({ kind: 'files-from-commit', value: 'a00b000' }, { runGit: git.runGit, readFile: () => 'x', cwd: ROOT, lstatSync: REGULAR_FILE }); assert.deepStrictEqual(result.files, ['init.js']); }); @@ -314,7 +319,7 @@ test('files-from-commit includes a file the commit deleted once it has been rest const result = scope.collectChanges( { kind: 'files-from-commit', value: 'HEAD' }, - { runGit: git.runGit, readFile: () => 'back', cwd: '/repo', lstatSync: lstatOnly(['/repo/restored.js']) } + { runGit: git.runGit, readFile: () => 'back', cwd: ROOT, lstatSync: lstatOnly([R('restored.js')]) } ); assert.deepStrictEqual(result.files, ['restored.js']); @@ -326,7 +331,7 @@ test('collectFiles serializes symlinks instead of following them', () => { { kind: 'files', value: ['link.js'] }, { runGit: (args) => (args.join(' ') === 'rev-parse --show-toplevel' ? '/repo\n' : ''), - cwd: '/repo', + cwd: ROOT, lstatSync: () => ({ isSymbolicLink: () => true, isFile: () => false }), readlinkSync: () => '/etc/passwd', readFile: () => { readCalled = true; return 'secret'; }, @@ -344,7 +349,7 @@ test('collectFiles refuses paths that resolve outside the repository root', () = { kind: 'files', value: ['../outside.js'] }, { runGit: (args) => (args.join(' ') === 'rev-parse --show-toplevel' ? '/repo\n' : ''), - cwd: '/repo', + cwd: ROOT, lstatSync: () => ({ isSymbolicLink: () => false, isFile: () => true }), readFile: () => 'x', } @@ -359,7 +364,7 @@ test('collectFiles refuses non-regular files such as directories', () => { { kind: 'files', value: ['src'] }, { runGit: (args) => (args.join(' ') === 'rev-parse --show-toplevel' ? '/repo\n' : ''), - cwd: '/repo', + cwd: ROOT, lstatSync: () => ({ isSymbolicLink: () => false, isFile: () => false }), readFile: () => 'x', } @@ -374,8 +379,8 @@ test('collectFiles refuses files under a symlinked parent directory that resolve { kind: 'files', value: ['external/secret.txt'] }, { runGit: (args) => (args.join(' ') === 'rev-parse --show-toplevel' ? '/repo\n' : ''), - cwd: '/repo', - realpathSync: (target) => (target === '/repo/external' ? '/outside' : target), + cwd: ROOT, + realpathSync: (target) => (target === R('external') ? OUTSIDE : target), lstatSync: REGULAR_FILE, readFile: () => 'secret', } @@ -395,7 +400,7 @@ test('files-from-commit keeps unresolved deletions in scope with their deletion const result = scope.collectChanges( { kind: 'files-from-commit', value: 'HEAD' }, - { runGit: git.runGit, readFile: () => 'x', cwd: '/repo', lstatSync: lstatOnly([]) } + { runGit: git.runGit, readFile: () => 'x', cwd: ROOT, lstatSync: lstatOnly([]) } ); assert.deepStrictEqual(result.files, ['removed.js']); @@ -414,7 +419,7 @@ test('files-from-commit labels a file the commit added and the repair removed as const result = scope.collectChanges( { kind: 'files-from-commit', value: 'HEAD' }, - { runGit: git.runGit, readFile: () => 'x', cwd: '/repo', lstatSync: lstatOnly([]) } + { runGit: git.runGit, readFile: () => 'x', cwd: ROOT, lstatSync: lstatOnly([]) } ); assert.deepStrictEqual(result.files, ['added-then-removed.js']); @@ -430,13 +435,13 @@ test('files-from-commit treats ENOTDIR (parent directory replaced by a file) as [/^diff --no-ext-diff --no-textconv parent1 -- config\/settings\.js$/, '-old setting\n'], ]); const lstatSync = (file) => { - if (file === '/repo/config') return REGULAR_FILE(); + if (file === R('config')) return REGULAR_FILE(); throw Object.assign(new Error('ENOTDIR'), { code: 'ENOTDIR' }); }; const result = scope.collectChanges( { kind: 'files-from-commit', value: 'HEAD' }, - { runGit: git.runGit, readFile: () => 'now a file', cwd: '/repo', lstatSync } + { runGit: git.runGit, readFile: () => 'now a file', cwd: ROOT, lstatSync } ); assert.deepStrictEqual(result.files, ['config', 'config/settings.js']); @@ -457,7 +462,7 @@ test('files-from-commit treats a dangling symlink as present and serializes the { kind: 'files-from-commit', value: 'HEAD' }, { runGit: git.runGit, - cwd: '/repo', + cwd: ROOT, lstatSync: () => ({ isSymbolicLink: () => true, isFile: () => false }), readlinkSync: () => 'missing-target.js', readFile: () => { readCalled = true; return 'x'; }, @@ -482,7 +487,7 @@ test('files-from-commit describes a submodule (gitlink) directory instead of fai const result = scope.collectChanges( { kind: 'files-from-commit', value: 'HEAD' }, - { runGit: git.runGit, cwd: '/repo', lstatSync, readFile: () => { throw new Error('must not read a directory'); } } + { runGit: git.runGit, cwd: ROOT, lstatSync, readFile: () => { throw new Error('must not read a directory'); } } ); assert.deepStrictEqual(result.files, ['vendor/lib']); @@ -510,18 +515,18 @@ test('defaultRunGit distinguishes a no-index access failure from a genuine diffe const accessError = () => ({ status: 1, stdout: '', stderr: 'error: Could not access nested-repo/' }); assert.throws( - () => scope.defaultRunGit(['diff', '--no-index', '--', '/dev/null', 'nested-repo/'], '/repo', {}, accessError), + () => scope.defaultRunGit(['diff', '--no-index', '--', '/dev/null', 'nested-repo/'], ROOT, {}, accessError), /Could not access/ ); const realDiff = () => ({ status: 1, stdout: '+x\n', stderr: '' }); - assert.strictEqual(scope.defaultRunGit(['diff', '--no-index', '--', '/dev/null', 'x'], '/repo', {}, realDiff), '+x\n'); + assert.strictEqual(scope.defaultRunGit(['diff', '--no-index', '--', '/dev/null', 'x'], ROOT, {}, realDiff), '+x\n'); }); test('defaultRunGit fails loudly when a file inventory overflows the buffer', () => { const overflow = () => ({ error: Object.assign(new Error('x'), { code: 'ENOBUFS' }), stdout: 'partial.js\0', status: null }); assert.throws( - () => scope.defaultRunGit(['ls-files', '--others', '-z'], '/repo', {}, overflow), + () => scope.defaultRunGit(['ls-files', '--others', '-z'], ROOT, {}, overflow), /exceeded .* inventory would be incomplete/ ); }); @@ -529,7 +534,7 @@ test('defaultRunGit fails loudly when a file inventory overflows the buffer', () test('defaultRunGit returns partial output only for diff payloads marked partialOk', () => { const overflow = () => ({ error: Object.assign(new Error('x'), { code: 'ENOBUFS' }), stdout: '+partial', status: null }); - const out = scope.defaultRunGit(['diff', 'HEAD', '--'], '/repo', { partialOk: true }, overflow); + const out = scope.defaultRunGit(['diff', 'HEAD', '--'], ROOT, { partialOk: true }, overflow); assert.strictEqual(out, '+partial'); }); @@ -537,8 +542,8 @@ test('defaultRunGit returns partial output only for diff payloads marked partial test('defaultRunGit returns empty output for a failed probe when allowFailure is set', () => { const failed = () => ({ status: 128, stdout: '', stderr: 'fatal: bad revision' }); - assert.strictEqual(scope.defaultRunGit(['rev-parse', '-q', '--verify', 'HEAD'], '/repo', { allowFailure: true }, failed), ''); - assert.throws(() => scope.defaultRunGit(['rev-parse', '-q', '--verify', 'HEAD'], '/repo', {}, failed), /bad revision/); + assert.strictEqual(scope.defaultRunGit(['rev-parse', '-q', '--verify', 'HEAD'], ROOT, { allowFailure: true }, failed), ''); + assert.throws(() => scope.defaultRunGit(['rev-parse', '-q', '--verify', 'HEAD'], ROOT, {}, failed), /bad revision/); }); console.log(`\n${passed} passed, ${failed} failed`); diff --git a/tests/scripts/astra-review.test.js b/tests/scripts/astra-review.test.js index d25011cd0..c09f8a0d3 100644 --- a/tests/scripts/astra-review.test.js +++ b/tests/scripts/astra-review.test.js @@ -7,6 +7,8 @@ 'use strict'; const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); const path = require('path'); const { spawnSync } = require('child_process'); const cli = require('../../scripts/astra-review'); @@ -156,26 +158,88 @@ test('runCli passes model, prompt, and timeout through to the reviewer', () => { assert.ok(seen.prompt.includes('git show sha:')); }); -test('writeReport refuses to write through an existing symlink', () => { - const io = { - lstatSync: () => ({ isSymbolicLink: () => true, isFile: () => false }), - writeFileSync: () => { throw new Error('must not write'); }, +function fakeReportIo({ renameFails = false, writeFails = false } = {}) { + const events = []; + return { + events, + openSync: (file, flags, mode) => { events.push(['open', file, flags, mode]); return 7; }, + writeFileSync: (fd, content) => { + events.push(['write', fd, content]); + if (writeFails) throw Object.assign(new Error('ENOSPC'), { code: 'ENOSPC' }); + }, + writeSync: () => { throw new Error('writeSync can return short; use writeFileSync on the descriptor'); }, + closeSync: (fd) => { events.push(['close', fd]); }, + renameSync: (from, to) => { + events.push(['rename', from, to]); + if (renameFails) throw new Error('EXDEV'); + }, + unlinkSync: (file) => { events.push(['unlink', file]); }, }; +} - assert.throws(() => cli.writeReport('/tmp/x.json', '{}', io), /symlink/); +const OUT_DIR = path.resolve('/out'); +const REPORT = path.join(OUT_DIR, 'report.json'); + +test('writeReport creates a private temp file exclusively and renames it over the destination', () => { + const io = fakeReportIo(); + + cli.writeReport(REPORT, '{}', io); + + const open = io.events.find((event) => event[0] === 'open'); + assert.ok(open[1].startsWith(path.join(OUT_DIR, '.report.json.')) && open[1].endsWith('.tmp'), `temp file in same dir: ${open[1]}`); + assert.strictEqual(open[2], 'wx'); + assert.strictEqual(open[3], 0o600); + const rename = io.events.find((event) => event[0] === 'rename'); + assert.strictEqual(rename[1], open[1]); + assert.strictEqual(rename[2], REPORT); + assert.ok(io.events.findIndex((event) => event[0] === 'close') < io.events.indexOf(rename), 'closed before rename'); }); -test('writeReport creates a user-only file when the path is new', () => { - let written = null; - const io = { - lstatSync: () => { throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); }, - writeFileSync: (file, content, options) => { written = { file, content, options }; }, - }; +test('writeReport removes the temp file when the rename fails', () => { + const io = fakeReportIo({ renameFails: true }); - cli.writeReport('/tmp/new.json', '{}', io); + assert.throws(() => cli.writeReport(REPORT, '{}', io), /EXDEV/); + assert.ok(io.events.some((event) => event[0] === 'unlink' && event[1].endsWith('.tmp'))); +}); - assert.strictEqual(written.file, '/tmp/new.json'); - assert.strictEqual(written.options.mode, 0o600); +test('writeReport writes the whole content via the descriptor and cleans up when the write fails', () => { + const ok = fakeReportIo(); + cli.writeReport(REPORT, '{"full":true}', ok); + assert.deepStrictEqual(ok.events.find((event) => event[0] === 'write').slice(1), [7, '{"full":true}']); + + const io = fakeReportIo({ writeFails: true }); + assert.throws(() => cli.writeReport(REPORT, '{}', io), /ENOSPC/); + assert.ok(io.events.some((event) => event[0] === 'close'), 'descriptor closed after failure'); + assert.ok(io.events.some((event) => event[0] === 'unlink' && event[1].endsWith('.tmp')), 'temp file removed'); + assert.ok(!io.events.some((event) => event[0] === 'rename'), 'nothing published'); +}); + +test('writeReport replaces a symlink and a world-readable file with a private regular file (real fs)', () => { + const posix = process.platform !== 'win32'; + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'astra-report-')); + const victim = path.join(dir, 'victim.txt'); + fs.writeFileSync(victim, 'keep me'); + const linked = path.join(dir, 'linked.json'); + let canSymlink = true; + try { + fs.symlinkSync(victim, linked); + } catch { + canSymlink = false; // Windows without symlink privilege: replacement is still checked below + } + const shared = path.join(dir, 'shared.json'); + fs.writeFileSync(shared, 'old', { mode: 0o644 }); + + if (canSymlink) cli.writeReport(linked, '{"a":1}'); + cli.writeReport(shared, '{"b":2}'); + + if (canSymlink) { + assert.strictEqual(fs.readFileSync(victim, 'utf8'), 'keep me', 'symlink target untouched'); + assert.strictEqual(fs.lstatSync(linked).isSymbolicLink(), false, 'symlink replaced by a regular file'); + assert.strictEqual(fs.readFileSync(linked, 'utf8'), '{"a":1}'); + } + assert.strictEqual(fs.readFileSync(shared, 'utf8'), '{"b":2}'); + if (posix) assert.strictEqual((fs.statSync(shared).mode & 0o777), 0o600, 'existing 0644 file becomes 0600'); + assert.deepStrictEqual(fs.readdirSync(dir).filter((name) => name.endsWith('.tmp')), [], 'no temp files left'); }); test('CLI exits 2 with usage on a bad argument', () => {