Skip to content

Commit fa26639

Browse files
TheGreatAxiosTheGreatAxios
committed
fix(permissions): harden approval migration call site and writes
Guard the session-start migration call so a future throw degrades to log-and-continue, and route the migration rewrite through the shared chained tmp+rename writer so a concurrent grant mint serializes with the purge instead of losing an update.
1 parent ff9673c commit fa26639

5 files changed

Lines changed: 218 additions & 69 deletions

File tree

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

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { generateSessionId, sessionDir } from "../session/index.js";
66
import { loadSeededApprovals } from "../session/runtime-assembly.js";
77
import { normalizeSeededApprovals } from "./authz-grants.js";
88
import { migratePersistedApprovalStores } from "./approval-store-migration.js";
9+
import { saveGlobalApproval } from "./store.js";
910

1011
let cwd = "";
1112
let home = "";
@@ -167,6 +168,74 @@ describe("migratePersistedApprovalStores", () => {
167168
expect(remaining).toEqual(["manage_tasks", "run_shell"]);
168169
});
169170

171+
test("a grant minted while the migration runs is not lost and the file stays valid", async () => {
172+
await mkdir(join(home, ".corbits"), { recursive: true });
173+
await writeFile(
174+
globalStorePath(),
175+
JSON.stringify({
176+
approvals: [
177+
{ tool: "update_plan", pattern: "plan *" },
178+
{ tool: "run_shell", pattern: "git *" },
179+
],
180+
}),
181+
);
182+
183+
const minted = { tool: "run_shell", pattern: "npm *" };
184+
const [result] = await Promise.all([
185+
migratePersistedApprovalStores(cwd, sessionId, home),
186+
saveGlobalApproval(minted, home),
187+
]);
188+
189+
expect(result.purged).toBe(1);
190+
const final = (await readJson(globalStorePath())) as {
191+
approvals: { tool: string; pattern: string }[];
192+
};
193+
expect(
194+
final.approvals.some((approval) => approval.tool === "update_plan"),
195+
).toBe(false);
196+
expect(final.approvals).toContainEqual({
197+
tool: "run_shell",
198+
pattern: "git *",
199+
});
200+
expect(final.approvals).toContainEqual(minted);
201+
const backup = (await readJson(backupPath(globalStorePath()))) as {
202+
approvals: { tool: string; pattern: string }[];
203+
};
204+
expect(backup.approvals).toContainEqual({
205+
tool: "update_plan",
206+
pattern: "plan *",
207+
});
208+
});
209+
210+
test("a storm of concurrent grants around the migration loses nothing", async () => {
211+
await mkdir(join(home, ".corbits"), { recursive: true });
212+
await writeFile(
213+
globalStorePath(),
214+
JSON.stringify({
215+
approvals: [{ tool: "update_plan", pattern: "plan *" }],
216+
}),
217+
);
218+
219+
const minted = Array.from({ length: 10 }, (_, i) => ({
220+
tool: "run_shell",
221+
pattern: `storm-${i} *`,
222+
}));
223+
await Promise.all([
224+
migratePersistedApprovalStores(cwd, sessionId, home),
225+
...minted.map((approval) => saveGlobalApproval(approval, home)),
226+
]);
227+
228+
const final = (await readJson(globalStorePath())) as {
229+
approvals: { tool: string; pattern: string }[];
230+
};
231+
expect(
232+
final.approvals.some((approval) => approval.tool === "update_plan"),
233+
).toBe(false);
234+
for (const approval of minted) {
235+
expect(final.approvals).toContainEqual(approval);
236+
}
237+
});
238+
170239
test("seed loading purges on-disk update_plan keys while the normalizer still drops them in memory", async () => {
171240
await writeFile(
172241
sessionStorePath(),

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

Lines changed: 67 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
1-
import { mkdir, readFile, writeFile } from "node:fs/promises";
1+
import { writeFile } from "node:fs/promises";
22
import { homedir } from "node:os";
3-
import { dirname, join } from "node:path";
3+
import { join } from "node:path";
44

55
import { getLogger } from "@intx/log";
66

77
import { canonicalGrantTool } from "../agent/canonical-tool-name.js";
88
import { LOG_NAMESPACE_ROOT, SETTINGS_DIR_NAME } from "../branding.js";
99
import { sessionDir } from "../session/index.js";
10+
import { chainObjectWrite } from "./store.js";
1011

1112
const log = getLogger([LOG_NAMESPACE_ROOT, "permission", "approval-migration"]);
1213

@@ -64,75 +65,78 @@ function isFileExistsError(err: unknown): boolean {
6465
// Purge update_plan keys from one approvals file (session, project, or
6566
// global store shape: an `approvals` array plus, for the global file, a
6667
// `providerModels` map of arrays). Backup-then-rewrite: the pre-migration
67-
// bytes are saved to `<path>.bak` first (an existing backup is kept, so the
68+
// state is saved to `<path>.bak` first (an existing backup is kept, so the
6869
// first backup always holds the true original), and a file with nothing to
69-
// left untouched (no backup, byte-identical) so re-runs are no-ops. Entries
70-
// that are not positive update_plan matches are kept verbatim — pure renames
71-
// are never collapsed here; that stays the load-time normalizer's job.
72-
// Missing, unreadable, or corrupt files are no-ops; write failures propagate.
70+
// purge is left untouched (no backup, byte-identical) so re-runs are no-ops.
71+
// Entries that are not positive update_plan matches are kept verbatim — pure
72+
// renames are never collapsed here; that stays the load-time normalizer's
73+
// job. Missing, unreadable, or corrupt files are no-ops; write failures
74+
// propagate. The rewrite goes through chainObjectWrite, so a concurrent grant
75+
// mint to the same file serializes with the migration instead of losing an
76+
// update, and the tmp+rename lands atomically so a reader never sees a torn
77+
// file.
7378
export async function migrateApprovalStoreFile(
7479
path: string,
7580
): Promise<ApprovalStoreMigrationFileResult> {
76-
let raw: string;
77-
try {
78-
raw = await readFile(path, "utf-8");
79-
} catch {
80-
return { path, purged: 0 };
81-
}
82-
let parsed: unknown;
83-
try {
84-
parsed = JSON.parse(raw) as unknown;
85-
} catch {
86-
return { path, purged: 0 };
87-
}
88-
if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) {
89-
return { path, purged: 0 };
90-
}
91-
const record = parsed as Record<string, unknown>;
92-
const next: Record<string, unknown> = { ...record };
93-
let purged = 0;
94-
const onPurge = (): void => {
95-
purged += 1;
96-
};
97-
if (Array.isArray(record.approvals)) {
98-
const { kept, changed } = purgeList(record.approvals, onPurge);
99-
if (changed) next.approvals = kept;
100-
}
101-
const providerModels = record.providerModels;
102-
if (
103-
typeof providerModels === "object" &&
104-
providerModels !== null &&
105-
!Array.isArray(providerModels)
106-
) {
107-
const map = providerModels as Record<string, unknown>;
108-
const nextMap: Record<string, unknown> = {};
109-
let mapChanged = false;
110-
for (const [key, list] of Object.entries(map)) {
111-
const { kept, changed } = purgeList(list, onPurge);
112-
if (changed) {
113-
nextMap[key] = kept;
114-
mapChanged = true;
115-
} else {
116-
nextMap[key] = list;
117-
}
118-
}
119-
if (mapChanged) next.providerModels = nextMap;
120-
}
121-
if (purged === 0) return { path, purged: 0 };
12281
const backupPath = `${path}.bak`;
82+
let purged = 0;
83+
let rewrote = false;
12384
try {
124-
await writeFile(backupPath, raw, { flag: "wx" });
85+
await chainObjectWrite(path, async (current) => {
86+
const next: Record<string, unknown> = { ...current };
87+
let changed = 0;
88+
const onPurge = (): void => {
89+
changed += 1;
90+
};
91+
if (Array.isArray(current.approvals)) {
92+
const { kept, changed: listChanged } = purgeList(
93+
current.approvals,
94+
onPurge,
95+
);
96+
if (listChanged) next.approvals = kept;
97+
}
98+
const providerModels = current.providerModels;
99+
if (
100+
typeof providerModels === "object" &&
101+
providerModels !== null &&
102+
!Array.isArray(providerModels)
103+
) {
104+
const map = providerModels as Record<string, unknown>;
105+
const nextMap: Record<string, unknown> = {};
106+
let mapChanged = false;
107+
for (const [key, list] of Object.entries(map)) {
108+
const { kept, changed: listChanged } = purgeList(list, onPurge);
109+
nextMap[key] = listChanged ? kept : list;
110+
mapChanged = mapChanged || listChanged;
111+
}
112+
if (mapChanged) next.providerModels = nextMap;
113+
}
114+
if (changed === 0) return undefined;
115+
try {
116+
await writeFile(backupPath, JSON.stringify(current, null, 2), {
117+
flag: "wx",
118+
});
119+
} catch (err) {
120+
if (!isFileExistsError(err)) {
121+
log.warn("Skipping approval-store migration for {path}: {error}", {
122+
path,
123+
error: err instanceof Error ? err.message : String(err),
124+
});
125+
return undefined;
126+
}
127+
}
128+
purged = changed;
129+
rewrote = true;
130+
return next;
131+
});
125132
} catch (err) {
126-
if (!isFileExistsError(err)) {
127-
log.warn("Skipping approval-store migration for {path}: {error}", {
128-
path,
129-
error: err instanceof Error ? err.message : String(err),
130-
});
131-
return { path, purged: 0 };
132-
}
133+
log.warn("Skipping approval-store migration for {path}: {error}", {
134+
path,
135+
error: err instanceof Error ? err.message : String(err),
136+
});
137+
return { path, purged: 0 };
133138
}
134-
await mkdir(dirname(path), { recursive: true });
135-
await writeFile(path, JSON.stringify(next, null, 2));
139+
if (!rewrote) return { path, purged: 0 };
136140
return { path, purged, backupPath };
137141
}
138142

‎src/permission/store.ts‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -105,14 +105,21 @@ async function readObjectFile(path: string): Promise<Record<string, unknown>> {
105105
// Serialize the full read-modify-write per path so concurrent grants to the same
106106
// file (a global and a provider-model grant resolving together both touch the
107107
// global file) never lose an update, and rename atomically so a reader never
108-
// observes a torn file.
109-
function chainObjectWrite(
108+
// observes a torn file. Returning undefined from mutate skips the write, which
109+
// keeps migrations that find nothing to purge byte-identical no-ops.
110+
export function chainObjectWrite(
110111
path: string,
111-
mutate: (current: Record<string, unknown>) => Record<string, unknown>,
112+
mutate: (
113+
current: Record<string, unknown>,
114+
) =>
115+
| Record<string, unknown>
116+
| undefined
117+
| Promise<Record<string, unknown> | undefined>,
112118
): Promise<void> {
113119
const tmp = `${path}.${process.pid}.tmp`;
114120
const run = async (): Promise<void> => {
115-
const next = mutate(await readObjectFile(path));
121+
const next = await mutate(await readObjectFile(path));
122+
if (next === undefined) return;
116123
await mkdir(dirname(path), { recursive: true });
117124
await writeFile(tmp, JSON.stringify(next, null, 2));
118125
await rename(tmp, path);

‎src/session/runtime-assembly-migration.test.ts‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
import { afterEach, beforeEach, describe, expect, test } from "bun:test";
2+
import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises";
3+
import { tmpdir } from "node:os";
4+
import { join } from "node:path";
5+
6+
import { withMockedModuleDuring } from "../../tests/helpers/mock-module.js";
7+
import type * as migrationModule from "../permission/approval-store-migration.js";
8+
import { generateSessionId, initSessionDir, sessionDir } from "./index.js";
9+
10+
describe("loadSeededApprovals migration guard", () => {
11+
let cwd = "";
12+
let home = "";
13+
let sessionId = "";
14+
15+
beforeEach(async () => {
16+
cwd = await mkdtemp(join(tmpdir(), "migration-guard-"));
17+
home = await mkdtemp(join(tmpdir(), "migration-guard-home-"));
18+
sessionId = generateSessionId();
19+
await initSessionDir(cwd, sessionId, home);
20+
});
21+
22+
afterEach(async () => {
23+
if (cwd !== "") await rm(cwd, { recursive: true, force: true });
24+
if (home !== "") await rm(home, { recursive: true, force: true });
25+
cwd = "";
26+
home = "";
27+
sessionId = "";
28+
});
29+
30+
test("session start proceeds when the migration throws", async () => {
31+
await mkdir(sessionDir(cwd, sessionId, home), { recursive: true });
32+
await writeFile(
33+
join(sessionDir(cwd, sessionId, home), "permissions.json"),
34+
JSON.stringify({
35+
approvals: [{ tool: "run_shell", pattern: "session npm *" }],
36+
}),
37+
);
38+
39+
let migrationCalls = 0;
40+
const seeded = await withMockedModuleDuring(
41+
import.meta.resolve("../permission/approval-store-migration.js"),
42+
(real: typeof migrationModule) => ({
43+
...real,
44+
migratePersistedApprovalStores: (): Promise<never> => {
45+
migrationCalls += 1;
46+
return Promise.reject(new Error("migration boom"));
47+
},
48+
}),
49+
async () => {
50+
const { loadSeededApprovals } = await import("./runtime-assembly.js");
51+
return loadSeededApprovals(cwd, sessionId, home);
52+
},
53+
);
54+
55+
expect(migrationCalls).toBe(1);
56+
57+
expect(seeded).toContainEqual({
58+
tool: "run_shell",
59+
pattern: "session npm *",
60+
});
61+
});
62+
});

‎src/session/runtime-assembly.ts‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -141,8 +141,15 @@ export async function loadSeededApprovals(
141141
// One-time migration: purge persisted update_plan keys (dropped at load by
142142
// normalizeSeededApprovals but never rewritten) so removing the normalizer
143143
// later cannot resurrect them. Best-effort and idempotent — a clean tree is
144-
// a no-op, and the normalizer stays as defense-in-depth regardless.
145-
await migratePersistedApprovalStores(cwd, sessionId, home);
144+
// a no-op, and the normalizer stays as defense-in-depth regardless. Guarded
145+
// so a future throw can never break session start.
146+
try {
147+
await migratePersistedApprovalStores(cwd, sessionId, home);
148+
} catch (err) {
149+
persistLogger.warn("Skipping approval-store migration: {error}", {
150+
error: err instanceof Error ? err.message : String(err),
151+
});
152+
}
146153
const sessionApprovals = await loadApprovals(cwd, sessionId, home);
147154
const [
148155
projectApprovals,

0 commit comments

Comments
 (0)