Skip to content

Commit 7145c25

Browse files
committed
fix(permissions): write approval backups atomically and exclusively
A direct wx write of .bak can crash mid-write and leave a torn rollback copy. The next start treats that EEXIST as success and purges live anyway. Write the full backup to a sibling tmp, then link it onto .bak so the name appears complete or not at all, and a pre-existing original still wins.
1 parent fa26639 commit 7145c25

2 files changed

Lines changed: 79 additions & 7 deletions

File tree

‎src/permission/approval-store-migration.test.ts‎

Lines changed: 53 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,14 @@
11
import { afterEach, beforeEach, describe, expect, test } from "bun:test";
2-
import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
2+
import {
3+
chmod,
4+
mkdir,
5+
mkdtemp,
6+
readFile,
7+
rm,
8+
writeFile,
9+
} from "node:fs/promises";
310
import { tmpdir } from "node:os";
4-
import { join } from "node:path";
11+
import { dirname, join } from "node:path";
512
import { generateSessionId, sessionDir } from "../session/index.js";
613
import { loadSeededApprovals } from "../session/runtime-assembly.js";
714
import { normalizeSeededApprovals } from "./authz-grants.js";
@@ -267,4 +274,48 @@ describe("migratePersistedApprovalStores", () => {
267274
});
268275
expect(await readJson(projectStorePath())).toEqual({ approvals: [] });
269276
});
277+
278+
test("EACCES writing the backup leaves live bytes unchanged", async () => {
279+
const path = sessionStorePath();
280+
const original = {
281+
approvals: [
282+
{ tool: "update_plan", pattern: "plan *" },
283+
{ tool: "run_shell", pattern: "npm *" },
284+
],
285+
};
286+
await writeFile(path, JSON.stringify(original));
287+
const liveBytes = await readFile(path, "utf-8");
288+
const dir = dirname(path);
289+
await chmod(dir, 0o555);
290+
try {
291+
const result = await migratePersistedApprovalStores(cwd, sessionId, home);
292+
expect(result.purged).toBe(0);
293+
expect(result.backups).toEqual([]);
294+
expect(await readFile(path, "utf-8")).toBe(liveBytes);
295+
} finally {
296+
await chmod(dir, 0o755);
297+
}
298+
});
299+
300+
test("existing torn .bak plus live update_plan still purges live", async () => {
301+
const path = sessionStorePath();
302+
const original = {
303+
approvals: [
304+
{ tool: "update_plan", pattern: "plan *" },
305+
{ tool: "run_shell", pattern: "npm *" },
306+
],
307+
};
308+
await writeFile(path, JSON.stringify(original));
309+
const torn = '{"approvals":[{"tool":"update_plan"';
310+
await writeFile(backupPath(path), torn);
311+
312+
const result = await migratePersistedApprovalStores(cwd, sessionId, home);
313+
314+
expect(result.purged).toBe(1);
315+
expect(result.backups).toEqual([backupPath(path)]);
316+
expect(await readFile(backupPath(path), "utf-8")).toBe(torn);
317+
expect(await readJson(path)).toEqual({
318+
approvals: [{ tool: "run_shell", pattern: "npm *" }],
319+
});
320+
});
270321
});

‎src/permission/approval-store-migration.ts‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { writeFile } from "node:fs/promises";
1+
import { link, unlink, writeFile } from "node:fs/promises";
22
import { homedir } from "node:os";
33
import { join } from "node:path";
44

@@ -62,6 +62,25 @@ function isFileExistsError(err: unknown): boolean {
6262
);
6363
}
6464

65+
// Exclusive atomic backup: fully write a sibling tmp, then link it onto
66+
// `.bak` so the rollback copy never appears torn. rename would replace a
67+
// pre-existing bak and lose first-original-wins; a direct wx write of `.bak`
68+
// can crash mid-write and leave a truncated file that a later start treats as
69+
// EEXIST success. link is exclusive (EEXIST if the name is taken) and the
70+
// destination inode is complete at the moment it appears.
71+
async function writeBackupExclusive(
72+
backupPath: string,
73+
body: string,
74+
): Promise<void> {
75+
const tmp = `${backupPath}.${process.pid}.tmp`;
76+
try {
77+
await writeFile(tmp, body);
78+
await link(tmp, backupPath);
79+
} finally {
80+
await unlink(tmp).catch(() => undefined);
81+
}
82+
}
83+
6584
// Purge update_plan keys from one approvals file (session, project, or
6685
// global store shape: an `approvals` array plus, for the global file, a
6786
// `providerModels` map of arrays). Backup-then-rewrite: the pre-migration
@@ -74,7 +93,8 @@ function isFileExistsError(err: unknown): boolean {
7493
// propagate. The rewrite goes through chainObjectWrite, so a concurrent grant
7594
// mint to the same file serializes with the migration instead of losing an
7695
// update, and the tmp+rename lands atomically so a reader never sees a torn
77-
// file.
96+
// file. The backup uses the same tmp-then-place atomicity, exclusive so a
97+
// pre-existing `.bak` wins.
7898
export async function migrateApprovalStoreFile(
7999
path: string,
80100
): Promise<ApprovalStoreMigrationFileResult> {
@@ -113,9 +133,10 @@ export async function migrateApprovalStoreFile(
113133
}
114134
if (changed === 0) return undefined;
115135
try {
116-
await writeFile(backupPath, JSON.stringify(current, null, 2), {
117-
flag: "wx",
118-
});
136+
await writeBackupExclusive(
137+
backupPath,
138+
JSON.stringify(current, null, 2),
139+
);
119140
} catch (err) {
120141
if (!isFileExistsError(err)) {
121142
log.warn("Skipping approval-store migration for {path}: {error}", {

0 commit comments

Comments
 (0)