Skip to content

Commit 96dba64

Browse files
vitryvitry
committed
fix: preserve artifact handle identity checks
1 parent d38757f commit 96dba64

2 files changed

Lines changed: 94 additions & 10 deletions

File tree

‎scripts/lib/review.mjs‎

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -97,10 +97,15 @@ export async function readResultArtifact({ dataRoot, workspace, artifact }) {
9797
if (await realpath(dirname(path)) !== root) throw artifactError();
9898
const handle = await open(path, constants.O_RDONLY | (constants.O_NOFOLLOW ?? 0));
9999
try {
100-
// Keep the before/after identity check on path stats as well. Mixing
101-
// FileHandle.stat with lstat is inconsistent on Node 22.13 Windows.
102-
const before = pathInfo; const contents = await handle.readFile('utf8'); const after = await lstat(path);
103-
if (after.isSymbolicLink() || before.dev !== after.dev || before.ino !== after.ino) throw artifactError(); return contents;
100+
const before = await handle.stat(); const contents = await handle.readFile('utf8'); const handleAfter = await handle.stat();
101+
const after = await lstat(path); if (after.isSymbolicLink() || !after.isFile() || await realpath(dirname(path)) !== root) throw artifactError();
102+
const pathHandle = await open(path, constants.O_RDONLY | (constants.O_NOFOLLOW ?? 0));
103+
try {
104+
const pathAfter = await pathHandle.stat();
105+
if (!sameFileIdentity(before, handleAfter) || !sameFileIdentity(before, pathAfter)) throw artifactError();
106+
}
107+
finally { await pathHandle.close(); }
108+
return contents;
104109
}
105110
finally { await handle.close(); }
106111
});
@@ -131,14 +136,17 @@ async function writeArtifact({ dataRoot, workspace, directory, jobId, contents }
131136
try { if ((await lstat(path)).isSymbolicLink()) throw artifactError(); } catch (error) { if (errorCode(error) !== 'ENOENT') throw error; }
132137
temporary = join(targetDirectory, `.${basename(path)}.${randomBytes(8).toString('hex')}.tmp`);
133138
handle = await open(temporary, 'wx', 0o600); await handle.writeFile(contents, 'utf8'); await handle.sync();
134-
// Compare the temporary and final paths through the same stat API. On
135-
// Windows, Node 22.13 uses libuv's fast path for lstat while
136-
// FileHandle.stat uses the handle path; their dev/ino pairs can differ
137-
// for the same file even though the rename is correct.
138-
const sourceInfo = await lstat(temporary); await handle.close(); handle = undefined;
139+
// Compare the temporary and final files through FileHandle.stat on both
140+
// sides. Node 22.13 Windows uses different libuv stat paths for lstat
141+
// and fstat, so a path-stat comparison rejects a valid rename. Keeping
142+
// both identities handle-bound preserves the replacement check.
143+
const sourceInfo = await handle.stat(); await handle.close(); handle = undefined;
139144
if (await realpath(targetDirectory) !== targetDirectory) throw artifactError();
140145
await rename(temporary, path); temporary = undefined; const finalInfo = await lstat(path);
141-
if (finalInfo.isSymbolicLink() || !finalInfo.isFile() || finalInfo.dev !== sourceInfo.dev || finalInfo.ino !== sourceInfo.ino || await realpath(dirname(path)) !== targetDirectory) throw artifactError();
146+
if (finalInfo.isSymbolicLink() || !finalInfo.isFile() || await realpath(dirname(path)) !== targetDirectory) throw artifactError();
147+
const finalHandle = await open(path, constants.O_RDONLY | (constants.O_NOFOLLOW ?? 0));
148+
try { if (!sameFileIdentity(sourceInfo, await finalHandle.stat())) throw artifactError(); }
149+
finally { await finalHandle.close(); }
142150
await chmod(path, 0o600); await syncDirectory(targetDirectory); return relative;
143151
});
144152
} catch (error) { await closeFileHandle(handle); if (temporary) await unlink(temporary).catch(() => {}); throw new PluginError('ARTIFACT_WRITE_FAILED', 'Could not durably write the private artifact.', { category: 'storage', remedy: 'Check plugin data storage and retry.', cause: error }); }
@@ -147,6 +155,9 @@ async function writeArtifact({ dataRoot, workspace, directory, jobId, contents }
147155
/** @param {import('node:fs/promises').FileHandle|undefined} handle */
148156
async function closeFileHandle(handle) { await handle?.close().catch(() => {}); }
149157

158+
/** @param {any} left @param {any} right */
159+
function sameFileIdentity(left, right) { return left.dev === right.dev && left.ino === right.ino; }
160+
150161
/** @param {string} storageDirectory @param {string} directory @param {boolean} create */
151162
async function secureArtifactRoot(storageDirectory, directory, create) {
152163
const storageRoot = await realpath(resolve(storageDirectory)); const lexicalRoot = join(storageDirectory, directory); let info;

‎tests/windows-compat.test.mjs‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,3 +92,76 @@ test('artifact identity checks do not mix handle and path stat implementations',
9292
const result = await runNode(source);
9393
assert.equal(result.code, 0, result.stderr || result.stdout);
9494
});
95+
96+
test('artifact writes retain handle-bound identity checks', async () => {
97+
const source = `
98+
import { mkdtemp, open, rm } from 'node:fs/promises';
99+
import { tmpdir } from 'node:os';
100+
import { join } from 'node:path';
101+
import { writeResultArtifact } from ${JSON.stringify(reviewModule)};
102+
const directory = await mkdtemp(join(tmpdir(), 'zcode-write-identity-'));
103+
const probe = await open(join(directory, 'probe'), 'a+');
104+
const prototype = Object.getPrototypeOf(probe);
105+
await probe.close();
106+
const originalStat = prototype.stat;
107+
let statCalls = 0;
108+
prototype.stat = async function patchedStat(...args) {
109+
const stats = await originalStat.call(this, ...args);
110+
statCalls += 1;
111+
if (statCalls !== 2) return stats;
112+
return new Proxy(stats, { get(target, property) {
113+
if (property === 'dev') return target.dev + 1;
114+
if (property === 'ino') return target.ino + 1;
115+
return Reflect.get(target, property);
116+
} });
117+
};
118+
try {
119+
await writeResultArtifact({ dataRoot: directory, workspace: directory, jobId: 'c'.repeat(64), contents: 'done' });
120+
throw new Error('artifact write unexpectedly accepted a destination identity mismatch');
121+
} catch (error) {
122+
if (error?.code !== 'ARTIFACT_WRITE_FAILED') throw error;
123+
} finally {
124+
prototype.stat = originalStat;
125+
await rm(directory, { recursive: true, force: true });
126+
}
127+
`;
128+
const result = await runNode(source);
129+
assert.equal(result.code, 0, result.stderr || result.stdout);
130+
});
131+
132+
test('artifact reads retain handle-bound identity checks', async () => {
133+
const source = `
134+
import { mkdtemp, open, rm } from 'node:fs/promises';
135+
import { tmpdir } from 'node:os';
136+
import { join } from 'node:path';
137+
import { readResultArtifact, writeResultArtifact } from ${JSON.stringify(reviewModule)};
138+
const directory = await mkdtemp(join(tmpdir(), 'zcode-read-identity-'));
139+
const probe = await open(join(directory, 'probe'), 'a+');
140+
const prototype = Object.getPrototypeOf(probe);
141+
await probe.close();
142+
const artifact = await writeResultArtifact({ dataRoot: directory, workspace: directory, jobId: 'd'.repeat(64), contents: 'done' });
143+
const originalStat = prototype.stat;
144+
let statCalls = 0;
145+
prototype.stat = async function patchedStat(...args) {
146+
const stats = await originalStat.call(this, ...args);
147+
statCalls += 1;
148+
if (statCalls !== 2) return stats;
149+
return new Proxy(stats, { get(target, property) {
150+
if (property === 'dev') return target.dev + 1;
151+
if (property === 'ino') return target.ino + 1;
152+
return Reflect.get(target, property);
153+
} });
154+
};
155+
try {
156+
await readResultArtifact({ dataRoot: directory, workspace: directory, artifact });
157+
throw new Error('artifact read unexpectedly accepted a path identity mismatch');
158+
} catch (error) {
159+
if (error?.code !== 'RESULT_READ_FAILED') throw error;
160+
} finally {
161+
prototype.stat = originalStat;
162+
await rm(directory, { recursive: true, force: true });
163+
}
164+
`;
165+
const result = await runNode(source);
166+
assert.equal(result.code, 0, result.stderr || result.stdout);
167+
});

0 commit comments

Comments
 (0)