Skip to content

Commit 78288e3

Browse files
vincentkocsteipete
vincentkoc
authored and
steipete
committed
fix(auth): close codex review gaps
1 parent 859eb06 commit 78288e3

15 files changed

Lines changed: 487 additions & 23 deletions

‎docs/.generated/plugin-sdk-api-baseline.sha256‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
445130135f0037ca2f0877428d58deedf7a7f50e588af5505c1ba09d346663ae plugin-sdk-api-baseline.json
2-
147f6f63b835a92e24d6c93b91b0e2adbe1b8fb381d3bd45ef1ae63fd9b3386e plugin-sdk-api-baseline.jsonl
1+
57d6ea3acdbaff64d59293bb42224be02ca975c5554afacebb78677642355b6f plugin-sdk-api-baseline.json
2+
faff507780c473574e22e73af63dcdc4f4043cea72fa3475606cd4eaf20c1b17 plugin-sdk-api-baseline.jsonl

‎extensions/codex/src/app-server/auth-bridge.test.ts‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import crypto from "node:crypto";
12
import fs from "node:fs/promises";
23
import os from "node:os";
34
import path from "node:path";
@@ -15,6 +16,13 @@ let bridgeCodexAppServerStartOptions: typeof import("./auth-bridge.js").bridgeCo
1516

1617
describe("bridgeCodexAppServerStartOptions", () => {
1718
const tempDirs: string[] = [];
19+
const resolveHashedCodexHome = (agentDir: string, profileId: string) =>
20+
path.join(
21+
agentDir,
22+
"harness-auth",
23+
"codex",
24+
crypto.createHash("sha256").update(profileId).digest("hex").slice(0, 16),
25+
);
1826

1927
beforeAll(async () => {
2028
({ bridgeCodexAppServerStartOptions } = await import("./auth-bridge.js"));
@@ -74,6 +82,10 @@ describe("bridgeCodexAppServerStartOptions", () => {
7482
account_id: "acct-123",
7583
},
7684
});
85+
if (process.platform !== "win32") {
86+
const authStat = await fs.stat(path.join(result.env?.CODEX_HOME ?? "", "auth.json"));
87+
expect(authStat.mode & 0o777).toBe(0o600);
88+
}
7789
});
7890

7991
it("leaves start options unchanged when canonical oauth is unavailable", async () => {
@@ -97,4 +109,35 @@ describe("bridgeCodexAppServerStartOptions", () => {
97109
}),
98110
).resolves.toEqual(startOptions);
99111
});
112+
113+
it("refuses to overwrite a symlinked auth bridge file", async () => {
114+
const agentDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-codex-app-server-"));
115+
tempDirs.push(agentDir);
116+
mocks.ensureAuthProfileStore.mockReturnValue({
117+
version: 1,
118+
profiles: {
119+
"openai-codex:default": {
120+
type: "oauth",
121+
provider: "openai-codex",
122+
access: "access-token",
123+
refresh: "refresh-token",
124+
expires: Date.now() + 60_000,
125+
},
126+
},
127+
});
128+
129+
const codexHome = resolveHashedCodexHome(agentDir, "openai-codex:default");
130+
await fs.mkdir(codexHome, { recursive: true });
131+
await fs.symlink(path.join(agentDir, "outside.txt"), path.join(codexHome, "auth.json"));
132+
133+
await expect(
134+
bridgeCodexAppServerStartOptions({
135+
startOptions: {
136+
command: "codex",
137+
args: ["app-server"],
138+
},
139+
agentDir,
140+
}),
141+
).rejects.toThrow("must not be a symlink");
142+
});
100143
});

‎extensions/codex/src/app-server/auth-bridge.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import crypto from "node:crypto";
2-
import fs from "node:fs/promises";
32
import path from "node:path";
43
import { ensureAuthProfileStore, type OAuthCredential } from "openclaw/plugin-sdk/provider-auth";
4+
import { writePrivateSecretFileAtomic } from "openclaw/plugin-sdk/secret-file-runtime";
55
import type { CodexAppServerStartOptions } from "./config.js";
66

77
const DEFAULT_CODEX_AUTH_PROFILE_ID = "openai-codex:default";
@@ -60,8 +60,11 @@ export async function bridgeCodexAppServerStartOptions(params: {
6060
}
6161

6262
const codexHome = resolveCodexBridgeHome(params.agentDir, profileId);
63-
await fs.mkdir(codexHome, { recursive: true });
64-
await fs.writeFile(path.join(codexHome, "auth.json"), buildCodexAuthFile(credential));
63+
await writePrivateSecretFileAtomic({
64+
rootDir: params.agentDir,
65+
filePath: path.join(codexHome, "auth.json"),
66+
content: buildCodexAuthFile(credential),
67+
});
6568

6669
return {
6770
...params.startOptions,

‎extensions/codex/src/app-server/compact.test.ts‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,33 @@ describe("maybeCompactCodexAppServerSession", () => {
137137

138138
expect(seenAuthProfileId).toBe("openai-codex:work");
139139
});
140+
141+
it("fails closed when the persisted binding auth profile disagrees with the runtime request", async () => {
142+
const fake = createFakeCodexClient();
143+
const factory = vi.fn(async () => fake.client);
144+
__testing.setCodexAppServerClientFactoryForTests(factory);
145+
const sessionFile = path.join(tempDir, "session.jsonl");
146+
await writeCodexAppServerBinding(sessionFile, {
147+
threadId: "thread-1",
148+
cwd: tempDir,
149+
authProfileId: "openai-codex:binding",
150+
});
151+
152+
const result = await maybeCompactCodexAppServerSession({
153+
sessionId: "session-1",
154+
sessionKey: "agent:main:session-1",
155+
sessionFile,
156+
workspaceDir: tempDir,
157+
authProfileId: "openai-codex:runtime",
158+
});
159+
160+
expect(result).toEqual({
161+
ok: false,
162+
compacted: false,
163+
reason: "auth profile mismatch for session binding",
164+
});
165+
expect(factory).not.toHaveBeenCalled();
166+
});
140167
});
141168

142169
function createFakeCodexClient(): {

‎extensions/codex/src/app-server/compact.ts‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,19 @@ export async function maybeCompactCodexAppServerSession(
3838
if (!binding?.threadId) {
3939
return { ok: false, compacted: false, reason: "no codex app-server thread binding" };
4040
}
41+
const requestedAuthProfileId = params.authProfileId?.trim() || undefined;
42+
if (
43+
requestedAuthProfileId &&
44+
binding.authProfileId &&
45+
binding.authProfileId !== requestedAuthProfileId
46+
) {
47+
return { ok: false, compacted: false, reason: "auth profile mismatch for session binding" };
48+
}
4149

42-
const client = await clientFactory(appServer.start, binding.authProfileId);
50+
const client = await clientFactory(
51+
appServer.start,
52+
requestedAuthProfileId ?? binding.authProfileId,
53+
);
4354
const waiter = createCodexNativeCompactionWaiter(client, binding.threadId);
4455
let completion: CodexNativeCompactionCompletion;
4556
try {

‎extensions/openai/openai-codex-cli-bridge.test.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import crypto from "node:crypto";
12
import fs from "node:fs/promises";
23
import os from "node:os";
34
import path from "node:path";
@@ -6,6 +7,13 @@ import { prepareOpenAICodexCliExecution } from "./openai-codex-cli-bridge.js";
67

78
describe("prepareOpenAICodexCliExecution", () => {
89
const tempDirs: string[] = [];
10+
const resolveHashedCodexHome = (agentDir: string, profileId: string) =>
11+
path.join(
12+
agentDir,
13+
"cli-auth",
14+
"codex",
15+
crypto.createHash("sha256").update(profileId).digest("hex").slice(0, 16),
16+
);
917

1018
afterEach(async () => {
1119
await Promise.all(
@@ -52,6 +60,10 @@ describe("prepareOpenAICodexCliExecution", () => {
5260
account_id: "acct-123",
5361
},
5462
});
63+
if (process.platform !== "win32") {
64+
const authStat = await fs.stat(path.join(result?.env?.CODEX_HOME ?? "", "auth.json"));
65+
expect(authStat.mode & 0o777).toBe(0o600);
66+
}
5567
});
5668

5769
it("returns null when there is no bridgeable canonical oauth credential", async () => {
@@ -74,4 +86,30 @@ describe("prepareOpenAICodexCliExecution", () => {
7486
}),
7587
).resolves.toBeNull();
7688
});
89+
90+
it("refuses to overwrite a symlinked codex cli auth bridge file", async () => {
91+
const agentDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-codex-cli-bridge-"));
92+
tempDirs.push(agentDir);
93+
const codexHome = resolveHashedCodexHome(agentDir, "openai-codex:default");
94+
await fs.mkdir(codexHome, { recursive: true });
95+
await fs.symlink(path.join(agentDir, "outside.txt"), path.join(codexHome, "auth.json"));
96+
97+
await expect(
98+
prepareOpenAICodexCliExecution({
99+
config: undefined,
100+
workspaceDir: agentDir,
101+
agentDir,
102+
provider: "codex-cli",
103+
modelId: "gpt-5.4",
104+
authProfileId: "openai-codex:default",
105+
authCredential: {
106+
type: "oauth",
107+
provider: "openai-codex",
108+
access: "access-token",
109+
refresh: "refresh-token",
110+
expires: Date.now() + 60_000,
111+
},
112+
}),
113+
).rejects.toThrow("must not be a symlink");
114+
});
77115
});

‎extensions/openai/openai-codex-cli-bridge.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
import crypto from "node:crypto";
2-
import fs from "node:fs/promises";
32
import path from "node:path";
43
import type {
54
CliBackendPreparedExecution,
65
CliBackendPrepareExecutionContext,
76
} from "openclaw/plugin-sdk/cli-backend";
87
import type { OAuthCredential } from "openclaw/plugin-sdk/provider-auth";
8+
import { writePrivateSecretFileAtomic } from "openclaw/plugin-sdk/secret-file-runtime";
99

1010
const OPENAI_CODEX_PROVIDER_ID = "openai-codex";
1111
const CODEX_AUTH_ENV_CLEAR_KEYS = ["OPENAI_API_KEY"] as const;
@@ -60,8 +60,11 @@ export async function prepareOpenAICodexCliExecution(
6060
}
6161

6262
const codexHome = resolveCodexBridgeHome(ctx.agentDir, ctx.authProfileId);
63-
await fs.mkdir(codexHome, { recursive: true });
64-
await fs.writeFile(path.join(codexHome, "auth.json"), buildCodexAuthFile(ctx.authCredential));
63+
await writePrivateSecretFileAtomic({
64+
rootDir: ctx.agentDir,
65+
filePath: path.join(codexHome, "auth.json"),
66+
content: buildCodexAuthFile(ctx.authCredential),
67+
});
6568

6669
return {
6770
env: {

‎src/agents/auth-profiles/external-cli-sync.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { log } from "./constants.js";
44
import {
55
areOAuthCredentialsEquivalent,
66
hasUsableOAuthCredential,
7+
isSafeToAdoptBootstrapOAuthIdentity,
78
isSafeToOverwriteStoredOAuthIdentity,
89
shouldBootstrapFromExternalCliCredential,
910
shouldReplaceStoredOAuthCredential,
@@ -13,6 +14,7 @@ import type { AuthProfileStore, OAuthCredential } from "./types.js";
1314
export {
1415
areOAuthCredentialsEquivalent,
1516
hasUsableOAuthCredential,
17+
isSafeToAdoptBootstrapOAuthIdentity,
1618
isSafeToOverwriteStoredOAuthIdentity,
1719
shouldBootstrapFromExternalCliCredential,
1820
shouldReplaceStoredOAuthCredential,
@@ -139,7 +141,7 @@ export function resolveExternalCliAuthProfiles(
139141
}
140142
if (
141143
existingOAuth &&
142-
!isSafeToOverwriteStoredOAuthIdentity(existingOAuth, creds) &&
144+
!isSafeToAdoptBootstrapOAuthIdentity(existingOAuth, creds) &&
143145
!areOAuthCredentialsEquivalent(existingOAuth, creds)
144146
) {
145147
log.warn("refused external cli oauth bootstrap: identity mismatch or missing binding", {

‎src/agents/auth-profiles/oauth-manager.test.ts‎

Lines changed: 93 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,14 @@
1-
import { describe, expect, it } from "vitest";
2-
import { isSafeToOverwriteStoredOAuthIdentity, OAuthManagerRefreshError } from "./oauth-manager.js";
1+
import fs from "node:fs/promises";
2+
import os from "node:os";
3+
import path from "node:path";
4+
import { afterEach, describe, expect, it, vi } from "vitest";
5+
import {
6+
createOAuthManager,
7+
isSafeToAdoptBootstrapOAuthIdentity,
8+
isSafeToOverwriteStoredOAuthIdentity,
9+
OAuthManagerRefreshError,
10+
} from "./oauth-manager.js";
11+
import { ensureAuthProfileStore, saveAuthProfileStore } from "./store.js";
312
import type { AuthProfileStore, OAuthCredential } from "./types.js";
413

514
function createCredential(overrides: Partial<OAuthCredential> = {}): OAuthCredential {
@@ -13,6 +22,12 @@ function createCredential(overrides: Partial<OAuthCredential> = {}): OAuthCreden
1322
};
1423
}
1524

25+
const tempDirs: string[] = [];
26+
27+
afterEach(async () => {
28+
await Promise.all(tempDirs.splice(0).map((dir) => fs.rm(dir, { recursive: true, force: true })));
29+
});
30+
1631
describe("isSafeToOverwriteStoredOAuthIdentity", () => {
1732
it("accepts matching account identities", () => {
1833
expect(
@@ -40,6 +55,22 @@ describe("isSafeToOverwriteStoredOAuthIdentity", () => {
4055
),
4156
).toBe(false);
4257
});
58+
59+
it("still allows identity-less external bootstrap adoption", () => {
60+
const existing = createCredential({
61+
access: "expired-local-access",
62+
refresh: "expired-local-refresh",
63+
expires: Date.now() - 60_000,
64+
});
65+
const incoming = createCredential({
66+
access: "external-access",
67+
refresh: "external-refresh",
68+
expires: Date.now() + 60_000,
69+
});
70+
71+
expect(isSafeToOverwriteStoredOAuthIdentity(existing, incoming)).toBe(false);
72+
expect(isSafeToAdoptBootstrapOAuthIdentity(existing, incoming)).toBe(true);
73+
});
4374
});
4475

4576
describe("OAuthManagerRefreshError", () => {
@@ -69,3 +100,63 @@ describe("OAuthManagerRefreshError", () => {
69100
expect(serialized).not.toContain("store-refresh");
70101
});
71102
});
103+
104+
describe("createOAuthManager", () => {
105+
it("refreshes with the adopted external oauth credential", async () => {
106+
const agentDir = await fs.mkdtemp(path.join(os.tmpdir(), "oauth-manager-refresh-"));
107+
tempDirs.push(agentDir);
108+
const profileId = "minimax-portal:default";
109+
const localCredential = createCredential({
110+
provider: "minimax-portal",
111+
access: "stale-local-access",
112+
refresh: "stale-local-refresh",
113+
expires: Date.now() - 60_000,
114+
});
115+
saveAuthProfileStore(
116+
{
117+
version: 1,
118+
profiles: {
119+
[profileId]: localCredential,
120+
},
121+
},
122+
agentDir,
123+
);
124+
125+
const manager = createOAuthManager({
126+
buildApiKey: async (_provider, credential) => credential.access,
127+
refreshCredential: vi.fn(async (credential) => {
128+
expect(credential.refresh).toBe("external-refresh");
129+
return {
130+
access: "rotated-access",
131+
refresh: "rotated-refresh",
132+
expires: Date.now() + 60_000,
133+
};
134+
}),
135+
readBootstrapCredential: () =>
136+
createCredential({
137+
provider: "minimax-portal",
138+
access: "expired-external-access",
139+
refresh: "external-refresh",
140+
expires: Date.now() - 30_000,
141+
}),
142+
isRefreshTokenReusedError: () => false,
143+
isSafeToCopyOAuthIdentity: () => true,
144+
});
145+
146+
const result = await manager.resolveOAuthAccess({
147+
store: ensureAuthProfileStore(agentDir),
148+
profileId,
149+
credential: localCredential,
150+
agentDir,
151+
});
152+
153+
expect(result).toEqual({
154+
apiKey: "rotated-access",
155+
credential: expect.objectContaining({
156+
provider: "minimax-portal",
157+
access: "rotated-access",
158+
refresh: "rotated-refresh",
159+
}),
160+
});
161+
});
162+
});

0 commit comments

Comments
 (0)