Skip to content

Commit 31d4ea6

Browse files
kevinslinkevinslin
committed
fix(codex): fail closed on layered approval overrides
1 parent a44da4b commit 31d4ea6

3 files changed

Lines changed: 229 additions & 3 deletions

File tree

‎extensions/codex/src/app-server/attempt-startup.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,7 @@ export async function startCodexAttemptThread(params: {
346346
timeoutMs: params.appServer.requestTimeoutMs,
347347
signal,
348348
}),
349+
configCwd: startupExecutionCwd,
349350
appCache: defaultCodexAppInventoryCache,
350351
appCacheKey: pluginAppCacheKey,
351352
}),

‎extensions/codex/src/app-server/plugin-thread-config.test.ts‎

Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,7 @@ describe("Codex plugin thread config", () => {
180180
nextCursor: null,
181181
}),
182182
});
183+
let configReadCount = 0;
183184
const request = vi.fn(async (method: string) => {
184185
if (method === "plugin/list") {
185186
return pluginList([pluginSummary("google-calendar", { installed: true, enabled: true })]);
@@ -192,6 +193,22 @@ describe("Codex plugin thread config", () => {
192193
);
193194
}
194195
if (method === "config/read") {
196+
configReadCount += 1;
197+
if (configReadCount > 1) {
198+
return {
199+
config: {
200+
apps: {
201+
"google-calendar-app": {
202+
tools: {
203+
"calendar/read": {
204+
enabled: false,
205+
},
206+
},
207+
},
208+
},
209+
},
210+
};
211+
}
195212
return {
196213
config: {
197214
apps: {
@@ -250,6 +267,7 @@ describe("Codex plugin thread config", () => {
250267
destructiveApprovalMode: "always",
251268
});
252269
expect(request).toHaveBeenCalledWith("config/read", { includeLayers: false });
270+
expect(request.mock.calls.filter(([method]) => method === "config/read")).toHaveLength(2);
253271
expect(request).toHaveBeenCalledWith("config/value/write", {
254272
keyPath: 'apps."google-calendar-app".tools."calendar/create".approval_mode',
255273
value: null,
@@ -267,6 +285,192 @@ describe("Codex plugin thread config", () => {
267285
});
268286
});
269287

288+
it("omits always policy apps when cwd effective approval overrides remain after cleanup", async () => {
289+
const appCache = new CodexAppInventoryCache();
290+
await appCache.refreshNow({
291+
key: "runtime",
292+
nowMs: 0,
293+
request: async () => ({
294+
data: [appInfo("google-calendar-app", true)],
295+
nextCursor: null,
296+
}),
297+
});
298+
let configReadCount = 0;
299+
const request = vi.fn(async (method: string) => {
300+
if (method === "plugin/list") {
301+
return pluginList([pluginSummary("google-calendar", { installed: true, enabled: true })]);
302+
}
303+
if (method === "plugin/read") {
304+
return pluginDetail(
305+
"google-calendar",
306+
[appSummary("google-calendar-app")],
307+
["google-calendar"],
308+
);
309+
}
310+
if (method === "config/read") {
311+
configReadCount += 1;
312+
return {
313+
config: {
314+
apps: {
315+
"google-calendar-app": {
316+
tools: {
317+
"calendar/create": {
318+
approval_mode: "approve",
319+
source: configReadCount === 1 ? "user" : "project",
320+
},
321+
},
322+
},
323+
},
324+
},
325+
};
326+
}
327+
if (method === "config/value/write") {
328+
return { status: "ok" };
329+
}
330+
throw new Error(`unexpected request ${method}`);
331+
});
332+
333+
const config = await buildCodexPluginThreadConfig({
334+
pluginConfig: {
335+
codexPlugins: {
336+
enabled: true,
337+
allow_destructive_actions: "always",
338+
plugins: {
339+
"google-calendar": {
340+
marketplaceName: CODEX_PLUGINS_MARKETPLACE_NAME,
341+
pluginName: "google-calendar",
342+
},
343+
},
344+
},
345+
},
346+
appCache,
347+
appCacheKey: "runtime",
348+
configCwd: "/repo/project",
349+
nowMs: 1,
350+
request,
351+
});
352+
353+
expect(config.configPatch).toEqual({
354+
apps: {
355+
_default: {
356+
enabled: false,
357+
destructive_enabled: false,
358+
open_world_enabled: false,
359+
},
360+
},
361+
});
362+
expect(config.policyContext.apps).toStrictEqual({});
363+
expect(request).toHaveBeenCalledWith("config/read", {
364+
includeLayers: false,
365+
cwd: "/repo/project",
366+
});
367+
expect(request.mock.calls.filter(([method]) => method === "config/read")).toHaveLength(2);
368+
expect(config.diagnostics).toStrictEqual([
369+
{
370+
code: "approval_overrides_clear_failed",
371+
plugin: {
372+
configKey: "google-calendar",
373+
marketplaceName: CODEX_PLUGINS_MARKETPLACE_NAME,
374+
pluginName: "google-calendar",
375+
enabled: true,
376+
allowDestructiveActions: true,
377+
destructiveApprovalMode: "always",
378+
},
379+
message:
380+
"Could not clear durable Codex app approval overrides for google-calendar-app: effective approval overrides remain for calendar/create",
381+
},
382+
]);
383+
});
384+
385+
it("omits always policy apps when approval override writes are overridden", async () => {
386+
const appCache = new CodexAppInventoryCache();
387+
await appCache.refreshNow({
388+
key: "runtime",
389+
nowMs: 0,
390+
request: async () => ({
391+
data: [appInfo("google-calendar-app", true)],
392+
nextCursor: null,
393+
}),
394+
});
395+
const request = vi.fn(async (method: string) => {
396+
if (method === "plugin/list") {
397+
return pluginList([pluginSummary("google-calendar", { installed: true, enabled: true })]);
398+
}
399+
if (method === "plugin/read") {
400+
return pluginDetail(
401+
"google-calendar",
402+
[appSummary("google-calendar-app")],
403+
["google-calendar"],
404+
);
405+
}
406+
if (method === "config/read") {
407+
return {
408+
config: {
409+
apps: {
410+
"google-calendar-app": {
411+
tools: {
412+
"calendar/create": {
413+
approval_mode: "approve",
414+
},
415+
},
416+
},
417+
},
418+
},
419+
};
420+
}
421+
if (method === "config/value/write") {
422+
return { status: "okOverridden" };
423+
}
424+
throw new Error(`unexpected request ${method}`);
425+
});
426+
427+
const config = await buildCodexPluginThreadConfig({
428+
pluginConfig: {
429+
codexPlugins: {
430+
enabled: true,
431+
allow_destructive_actions: "always",
432+
plugins: {
433+
"google-calendar": {
434+
marketplaceName: CODEX_PLUGINS_MARKETPLACE_NAME,
435+
pluginName: "google-calendar",
436+
},
437+
},
438+
},
439+
},
440+
appCache,
441+
appCacheKey: "runtime",
442+
configCwd: "/repo/project",
443+
nowMs: 1,
444+
request,
445+
});
446+
447+
expect(config.configPatch).toEqual({
448+
apps: {
449+
_default: {
450+
enabled: false,
451+
destructive_enabled: false,
452+
open_world_enabled: false,
453+
},
454+
},
455+
});
456+
expect(config.policyContext.apps).toStrictEqual({});
457+
expect(config.diagnostics).toStrictEqual([
458+
{
459+
code: "approval_overrides_clear_failed",
460+
plugin: {
461+
configKey: "google-calendar",
462+
marketplaceName: CODEX_PLUGINS_MARKETPLACE_NAME,
463+
pluginName: "google-calendar",
464+
enabled: true,
465+
allowDestructiveActions: true,
466+
destructiveApprovalMode: "always",
467+
},
468+
message:
469+
"Could not clear durable Codex app approval overrides for google-calendar-app: approval override for calendar/create is controlled by another config layer",
470+
},
471+
]);
472+
});
473+
270474
it("omits always policy apps when durable approval override cleanup fails", async () => {
271475
const appCache = new CodexAppInventoryCache();
272476
await appCache.refreshNow({

‎extensions/codex/src/app-server/plugin-thread-config.ts‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ export type CodexPluginThreadConfig = {
7272
export type BuildCodexPluginThreadConfigParams = {
7373
pluginConfig?: unknown;
7474
request: CodexPluginRuntimeRequest;
75+
configCwd?: string;
7576
appCache?: CodexAppInventoryCache;
7677
appCacheKey: string;
7778
nowMs?: number;
@@ -254,6 +255,7 @@ export async function buildCodexPluginThreadConfig(
254255
record.policy.destructiveApprovalMode === "always" &&
255256
!(await clearPersistedAppToolApprovalOverrides({
256257
request: params.request,
258+
configCwd: params.configCwd,
257259
plugin: record.policy,
258260
app,
259261
diagnostics,
@@ -380,19 +382,30 @@ function buildPluginAppPolicyContext(
380382

381383
async function clearPersistedAppToolApprovalOverrides(params: {
382384
request: CodexPluginRuntimeRequest;
385+
configCwd?: string;
383386
plugin: ResolvedCodexPluginPolicy;
384387
app: CodexPluginOwnedApp;
385388
diagnostics: CodexPluginThreadConfigDiagnostic[];
386389
}): Promise<boolean> {
387390
try {
388-
for (const toolName of await readPersistedAppToolApprovalOverrideNames(params)) {
389-
await params.request("config/value/write", {
391+
const overrideNames = await readPersistedAppToolApprovalOverrideNames(params);
392+
for (const toolName of overrideNames) {
393+
const response = await params.request("config/value/write", {
390394
keyPath: `apps.${quoteConfigKeyPathSegment(params.app.id)}.tools.${quoteConfigKeyPathSegment(
391395
toolName,
392396
)}.approval_mode`,
393397
value: null,
394398
mergeStrategy: "replace",
395399
});
400+
if (isOverriddenConfigWriteResponse(response)) {
401+
throw new Error(`approval override for ${toolName} is controlled by another config layer`);
402+
}
403+
}
404+
const remainingOverrideNames = await readPersistedAppToolApprovalOverrideNames(params);
405+
if (remainingOverrideNames.length > 0) {
406+
throw new Error(
407+
`effective approval overrides remain for ${remainingOverrideNames.join(", ")}`,
408+
);
396409
}
397410
return true;
398411
} catch (error) {
@@ -409,9 +422,13 @@ async function clearPersistedAppToolApprovalOverrides(params: {
409422

410423
async function readPersistedAppToolApprovalOverrideNames(params: {
411424
request: CodexPluginRuntimeRequest;
425+
configCwd?: string;
412426
app: CodexPluginOwnedApp;
413427
}): Promise<string[]> {
414-
const response = await params.request("config/read", { includeLayers: false });
428+
const response = await params.request("config/read", {
429+
includeLayers: false,
430+
...(params.configCwd ? { cwd: params.configCwd } : {}),
431+
});
415432
const config = isJsonObject(response) ? response.config : undefined;
416433
const appsRoot = isJsonObject(config) ? config.apps : undefined;
417434
const nestedApps = isJsonObject(appsRoot) ? appsRoot.apps : undefined;
@@ -435,6 +452,10 @@ function hasPersistedToolApprovalOverride(value: JsonValue): boolean {
435452
);
436453
}
437454

455+
function isOverriddenConfigWriteResponse(response: unknown): boolean {
456+
return isJsonObject(response) && response.status === "okOverridden";
457+
}
458+
438459
function quoteConfigKeyPathSegment(segment: string): string {
439460
return `"${segment.replace(/["\\]/g, (char) => `\\${char}`)}"`;
440461
}

0 commit comments

Comments
 (0)