From 78288e37ed58447d8e46f7bbe29bdf5aa008318a Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Sat, 18 Apr 2026 07:30:32 -0700 Subject: [PATCH] fix(auth): close codex review gaps --- .../.generated/plugin-sdk-api-baseline.sha256 | 4 +- .../codex/src/app-server/auth-bridge.test.ts | 43 ++++++++ .../codex/src/app-server/auth-bridge.ts | 9 +- .../codex/src/app-server/compact.test.ts | 27 +++++ extensions/codex/src/app-server/compact.ts | 13 ++- .../openai/openai-codex-cli-bridge.test.ts | 38 +++++++ extensions/openai/openai-codex-cli-bridge.ts | 9 +- src/agents/auth-profiles/external-cli-sync.ts | 4 +- .../auth-profiles/oauth-manager.test.ts | 95 +++++++++++++++- src/agents/auth-profiles/oauth-manager.ts | 29 ++++- src/agents/cli-runner/prepare.test.ts | 36 ++++++ src/agents/cli-runner/prepare.ts | 34 +++++- src/infra/secret-file.test.ts | 62 ++++++++++- src/infra/secret-file.ts | 104 ++++++++++++++++++ src/plugin-sdk/secret-file-runtime.ts | 3 + 15 files changed, 487 insertions(+), 23 deletions(-) create mode 100644 src/agents/cli-runner/prepare.test.ts diff --git a/docs/.generated/plugin-sdk-api-baseline.sha256 b/docs/.generated/plugin-sdk-api-baseline.sha256 index 97d4b9331f7..f9a41b9dc56 100644 --- a/docs/.generated/plugin-sdk-api-baseline.sha256 +++ b/docs/.generated/plugin-sdk-api-baseline.sha256 @@ -1,2 +1,2 @@ -445130135f0037ca2f0877428d58deedf7a7f50e588af5505c1ba09d346663ae plugin-sdk-api-baseline.json -147f6f63b835a92e24d6c93b91b0e2adbe1b8fb381d3bd45ef1ae63fd9b3386e plugin-sdk-api-baseline.jsonl +57d6ea3acdbaff64d59293bb42224be02ca975c5554afacebb78677642355b6f plugin-sdk-api-baseline.json +faff507780c473574e22e73af63dcdc4f4043cea72fa3475606cd4eaf20c1b17 plugin-sdk-api-baseline.jsonl diff --git a/extensions/codex/src/app-server/auth-bridge.test.ts b/extensions/codex/src/app-server/auth-bridge.test.ts index 09b7925380a..e15979c1f2a 100644 --- a/extensions/codex/src/app-server/auth-bridge.test.ts +++ b/extensions/codex/src/app-server/auth-bridge.test.ts @@ -1,3 +1,4 @@ +import crypto from "node:crypto"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; @@ -15,6 +16,13 @@ let bridgeCodexAppServerStartOptions: typeof import("./auth-bridge.js").bridgeCo describe("bridgeCodexAppServerStartOptions", () => { const tempDirs: string[] = []; + const resolveHashedCodexHome = (agentDir: string, profileId: string) => + path.join( + agentDir, + "harness-auth", + "codex", + crypto.createHash("sha256").update(profileId).digest("hex").slice(0, 16), + ); beforeAll(async () => { ({ bridgeCodexAppServerStartOptions } = await import("./auth-bridge.js")); @@ -74,6 +82,10 @@ describe("bridgeCodexAppServerStartOptions", () => { account_id: "acct-123", }, }); + if (process.platform !== "win32") { + const authStat = await fs.stat(path.join(result.env?.CODEX_HOME ?? "", "auth.json")); + expect(authStat.mode & 0o777).toBe(0o600); + } }); it("leaves start options unchanged when canonical oauth is unavailable", async () => { @@ -97,4 +109,35 @@ describe("bridgeCodexAppServerStartOptions", () => { }), ).resolves.toEqual(startOptions); }); + + it("refuses to overwrite a symlinked auth bridge file", async () => { + const agentDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-codex-app-server-")); + tempDirs.push(agentDir); + mocks.ensureAuthProfileStore.mockReturnValue({ + version: 1, + profiles: { + "openai-codex:default": { + type: "oauth", + provider: "openai-codex", + access: "access-token", + refresh: "refresh-token", + expires: Date.now() + 60_000, + }, + }, + }); + + const codexHome = resolveHashedCodexHome(agentDir, "openai-codex:default"); + await fs.mkdir(codexHome, { recursive: true }); + await fs.symlink(path.join(agentDir, "outside.txt"), path.join(codexHome, "auth.json")); + + await expect( + bridgeCodexAppServerStartOptions({ + startOptions: { + command: "codex", + args: ["app-server"], + }, + agentDir, + }), + ).rejects.toThrow("must not be a symlink"); + }); }); diff --git a/extensions/codex/src/app-server/auth-bridge.ts b/extensions/codex/src/app-server/auth-bridge.ts index 62a84b6ad1b..3883390e270 100644 --- a/extensions/codex/src/app-server/auth-bridge.ts +++ b/extensions/codex/src/app-server/auth-bridge.ts @@ -1,7 +1,7 @@ import crypto from "node:crypto"; -import fs from "node:fs/promises"; import path from "node:path"; import { ensureAuthProfileStore, type OAuthCredential } from "openclaw/plugin-sdk/provider-auth"; +import { writePrivateSecretFileAtomic } from "openclaw/plugin-sdk/secret-file-runtime"; import type { CodexAppServerStartOptions } from "./config.js"; const DEFAULT_CODEX_AUTH_PROFILE_ID = "openai-codex:default"; @@ -60,8 +60,11 @@ export async function bridgeCodexAppServerStartOptions(params: { } const codexHome = resolveCodexBridgeHome(params.agentDir, profileId); - await fs.mkdir(codexHome, { recursive: true }); - await fs.writeFile(path.join(codexHome, "auth.json"), buildCodexAuthFile(credential)); + await writePrivateSecretFileAtomic({ + rootDir: params.agentDir, + filePath: path.join(codexHome, "auth.json"), + content: buildCodexAuthFile(credential), + }); return { ...params.startOptions, diff --git a/extensions/codex/src/app-server/compact.test.ts b/extensions/codex/src/app-server/compact.test.ts index 670bfc0e5d0..6c92f682993 100644 --- a/extensions/codex/src/app-server/compact.test.ts +++ b/extensions/codex/src/app-server/compact.test.ts @@ -137,6 +137,33 @@ describe("maybeCompactCodexAppServerSession", () => { expect(seenAuthProfileId).toBe("openai-codex:work"); }); + + it("fails closed when the persisted binding auth profile disagrees with the runtime request", async () => { + const fake = createFakeCodexClient(); + const factory = vi.fn(async () => fake.client); + __testing.setCodexAppServerClientFactoryForTests(factory); + const sessionFile = path.join(tempDir, "session.jsonl"); + await writeCodexAppServerBinding(sessionFile, { + threadId: "thread-1", + cwd: tempDir, + authProfileId: "openai-codex:binding", + }); + + const result = await maybeCompactCodexAppServerSession({ + sessionId: "session-1", + sessionKey: "agent:main:session-1", + sessionFile, + workspaceDir: tempDir, + authProfileId: "openai-codex:runtime", + }); + + expect(result).toEqual({ + ok: false, + compacted: false, + reason: "auth profile mismatch for session binding", + }); + expect(factory).not.toHaveBeenCalled(); + }); }); function createFakeCodexClient(): { diff --git a/extensions/codex/src/app-server/compact.ts b/extensions/codex/src/app-server/compact.ts index b6d3927ecb7..bbf804e45d5 100644 --- a/extensions/codex/src/app-server/compact.ts +++ b/extensions/codex/src/app-server/compact.ts @@ -38,8 +38,19 @@ export async function maybeCompactCodexAppServerSession( if (!binding?.threadId) { return { ok: false, compacted: false, reason: "no codex app-server thread binding" }; } + const requestedAuthProfileId = params.authProfileId?.trim() || undefined; + if ( + requestedAuthProfileId && + binding.authProfileId && + binding.authProfileId !== requestedAuthProfileId + ) { + return { ok: false, compacted: false, reason: "auth profile mismatch for session binding" }; + } - const client = await clientFactory(appServer.start, binding.authProfileId); + const client = await clientFactory( + appServer.start, + requestedAuthProfileId ?? binding.authProfileId, + ); const waiter = createCodexNativeCompactionWaiter(client, binding.threadId); let completion: CodexNativeCompactionCompletion; try { diff --git a/extensions/openai/openai-codex-cli-bridge.test.ts b/extensions/openai/openai-codex-cli-bridge.test.ts index 673d9cf47af..ba003510dac 100644 --- a/extensions/openai/openai-codex-cli-bridge.test.ts +++ b/extensions/openai/openai-codex-cli-bridge.test.ts @@ -1,3 +1,4 @@ +import crypto from "node:crypto"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; @@ -6,6 +7,13 @@ import { prepareOpenAICodexCliExecution } from "./openai-codex-cli-bridge.js"; describe("prepareOpenAICodexCliExecution", () => { const tempDirs: string[] = []; + const resolveHashedCodexHome = (agentDir: string, profileId: string) => + path.join( + agentDir, + "cli-auth", + "codex", + crypto.createHash("sha256").update(profileId).digest("hex").slice(0, 16), + ); afterEach(async () => { await Promise.all( @@ -52,6 +60,10 @@ describe("prepareOpenAICodexCliExecution", () => { account_id: "acct-123", }, }); + if (process.platform !== "win32") { + const authStat = await fs.stat(path.join(result?.env?.CODEX_HOME ?? "", "auth.json")); + expect(authStat.mode & 0o777).toBe(0o600); + } }); it("returns null when there is no bridgeable canonical oauth credential", async () => { @@ -74,4 +86,30 @@ describe("prepareOpenAICodexCliExecution", () => { }), ).resolves.toBeNull(); }); + + it("refuses to overwrite a symlinked codex cli auth bridge file", async () => { + const agentDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-codex-cli-bridge-")); + tempDirs.push(agentDir); + const codexHome = resolveHashedCodexHome(agentDir, "openai-codex:default"); + await fs.mkdir(codexHome, { recursive: true }); + await fs.symlink(path.join(agentDir, "outside.txt"), path.join(codexHome, "auth.json")); + + await expect( + prepareOpenAICodexCliExecution({ + config: undefined, + workspaceDir: agentDir, + agentDir, + provider: "codex-cli", + modelId: "gpt-5.4", + authProfileId: "openai-codex:default", + authCredential: { + type: "oauth", + provider: "openai-codex", + access: "access-token", + refresh: "refresh-token", + expires: Date.now() + 60_000, + }, + }), + ).rejects.toThrow("must not be a symlink"); + }); }); diff --git a/extensions/openai/openai-codex-cli-bridge.ts b/extensions/openai/openai-codex-cli-bridge.ts index 03460a73853..a77ba1e3abc 100644 --- a/extensions/openai/openai-codex-cli-bridge.ts +++ b/extensions/openai/openai-codex-cli-bridge.ts @@ -1,11 +1,11 @@ import crypto from "node:crypto"; -import fs from "node:fs/promises"; import path from "node:path"; import type { CliBackendPreparedExecution, CliBackendPrepareExecutionContext, } from "openclaw/plugin-sdk/cli-backend"; import type { OAuthCredential } from "openclaw/plugin-sdk/provider-auth"; +import { writePrivateSecretFileAtomic } from "openclaw/plugin-sdk/secret-file-runtime"; const OPENAI_CODEX_PROVIDER_ID = "openai-codex"; const CODEX_AUTH_ENV_CLEAR_KEYS = ["OPENAI_API_KEY"] as const; @@ -60,8 +60,11 @@ export async function prepareOpenAICodexCliExecution( } const codexHome = resolveCodexBridgeHome(ctx.agentDir, ctx.authProfileId); - await fs.mkdir(codexHome, { recursive: true }); - await fs.writeFile(path.join(codexHome, "auth.json"), buildCodexAuthFile(ctx.authCredential)); + await writePrivateSecretFileAtomic({ + rootDir: ctx.agentDir, + filePath: path.join(codexHome, "auth.json"), + content: buildCodexAuthFile(ctx.authCredential), + }); return { env: { diff --git a/src/agents/auth-profiles/external-cli-sync.ts b/src/agents/auth-profiles/external-cli-sync.ts index 302ca45bbbe..c2bdece7059 100644 --- a/src/agents/auth-profiles/external-cli-sync.ts +++ b/src/agents/auth-profiles/external-cli-sync.ts @@ -4,6 +4,7 @@ import { log } from "./constants.js"; import { areOAuthCredentialsEquivalent, hasUsableOAuthCredential, + isSafeToAdoptBootstrapOAuthIdentity, isSafeToOverwriteStoredOAuthIdentity, shouldBootstrapFromExternalCliCredential, shouldReplaceStoredOAuthCredential, @@ -13,6 +14,7 @@ import type { AuthProfileStore, OAuthCredential } from "./types.js"; export { areOAuthCredentialsEquivalent, hasUsableOAuthCredential, + isSafeToAdoptBootstrapOAuthIdentity, isSafeToOverwriteStoredOAuthIdentity, shouldBootstrapFromExternalCliCredential, shouldReplaceStoredOAuthCredential, @@ -139,7 +141,7 @@ export function resolveExternalCliAuthProfiles( } if ( existingOAuth && - !isSafeToOverwriteStoredOAuthIdentity(existingOAuth, creds) && + !isSafeToAdoptBootstrapOAuthIdentity(existingOAuth, creds) && !areOAuthCredentialsEquivalent(existingOAuth, creds) ) { log.warn("refused external cli oauth bootstrap: identity mismatch or missing binding", { diff --git a/src/agents/auth-profiles/oauth-manager.test.ts b/src/agents/auth-profiles/oauth-manager.test.ts index 71a3fbdd746..8dd9c6cf751 100644 --- a/src/agents/auth-profiles/oauth-manager.test.ts +++ b/src/agents/auth-profiles/oauth-manager.test.ts @@ -1,5 +1,14 @@ -import { describe, expect, it } from "vitest"; -import { isSafeToOverwriteStoredOAuthIdentity, OAuthManagerRefreshError } from "./oauth-manager.js"; +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + createOAuthManager, + isSafeToAdoptBootstrapOAuthIdentity, + isSafeToOverwriteStoredOAuthIdentity, + OAuthManagerRefreshError, +} from "./oauth-manager.js"; +import { ensureAuthProfileStore, saveAuthProfileStore } from "./store.js"; import type { AuthProfileStore, OAuthCredential } from "./types.js"; function createCredential(overrides: Partial = {}): OAuthCredential { @@ -13,6 +22,12 @@ function createCredential(overrides: Partial = {}): OAuthCreden }; } +const tempDirs: string[] = []; + +afterEach(async () => { + await Promise.all(tempDirs.splice(0).map((dir) => fs.rm(dir, { recursive: true, force: true }))); +}); + describe("isSafeToOverwriteStoredOAuthIdentity", () => { it("accepts matching account identities", () => { expect( @@ -40,6 +55,22 @@ describe("isSafeToOverwriteStoredOAuthIdentity", () => { ), ).toBe(false); }); + + it("still allows identity-less external bootstrap adoption", () => { + const existing = createCredential({ + access: "expired-local-access", + refresh: "expired-local-refresh", + expires: Date.now() - 60_000, + }); + const incoming = createCredential({ + access: "external-access", + refresh: "external-refresh", + expires: Date.now() + 60_000, + }); + + expect(isSafeToOverwriteStoredOAuthIdentity(existing, incoming)).toBe(false); + expect(isSafeToAdoptBootstrapOAuthIdentity(existing, incoming)).toBe(true); + }); }); describe("OAuthManagerRefreshError", () => { @@ -69,3 +100,63 @@ describe("OAuthManagerRefreshError", () => { expect(serialized).not.toContain("store-refresh"); }); }); + +describe("createOAuthManager", () => { + it("refreshes with the adopted external oauth credential", async () => { + const agentDir = await fs.mkdtemp(path.join(os.tmpdir(), "oauth-manager-refresh-")); + tempDirs.push(agentDir); + const profileId = "minimax-portal:default"; + const localCredential = createCredential({ + provider: "minimax-portal", + access: "stale-local-access", + refresh: "stale-local-refresh", + expires: Date.now() - 60_000, + }); + saveAuthProfileStore( + { + version: 1, + profiles: { + [profileId]: localCredential, + }, + }, + agentDir, + ); + + const manager = createOAuthManager({ + buildApiKey: async (_provider, credential) => credential.access, + refreshCredential: vi.fn(async (credential) => { + expect(credential.refresh).toBe("external-refresh"); + return { + access: "rotated-access", + refresh: "rotated-refresh", + expires: Date.now() + 60_000, + }; + }), + readBootstrapCredential: () => + createCredential({ + provider: "minimax-portal", + access: "expired-external-access", + refresh: "external-refresh", + expires: Date.now() - 30_000, + }), + isRefreshTokenReusedError: () => false, + isSafeToCopyOAuthIdentity: () => true, + }); + + const result = await manager.resolveOAuthAccess({ + store: ensureAuthProfileStore(agentDir), + profileId, + credential: localCredential, + agentDir, + }); + + expect(result).toEqual({ + apiKey: "rotated-access", + credential: expect.objectContaining({ + provider: "minimax-portal", + access: "rotated-access", + refresh: "rotated-refresh", + }), + }); + }); +}); diff --git a/src/agents/auth-profiles/oauth-manager.ts b/src/agents/auth-profiles/oauth-manager.ts index 14532e5d761..48c7f9ddb42 100644 --- a/src/agents/auth-profiles/oauth-manager.ts +++ b/src/agents/auth-profiles/oauth-manager.ts @@ -187,6 +187,25 @@ export function isSafeToOverwriteStoredOAuthIdentity( return hasMatchingOAuthIdentity(existing, incoming); } +export function isSafeToAdoptBootstrapOAuthIdentity( + existing: OAuthCredential | undefined, + incoming: OAuthCredential, +): boolean { + if (!existing || existing.type !== "oauth") { + return true; + } + if (existing.provider !== incoming.provider) { + return false; + } + if (areOAuthCredentialsEquivalent(existing, incoming)) { + return true; + } + if (!hasOAuthIdentity(existing)) { + return true; + } + return hasMatchingOAuthIdentity(existing, incoming); +} + export function shouldBootstrapFromExternalCliCredential(params: { existing: OAuthCredential | undefined; imported: OAuthCredential; @@ -289,7 +308,7 @@ export function resolveEffectiveOAuthCredential(params: { }); return params.credential; } - if (!isSafeToOverwriteStoredOAuthIdentity(params.credential, imported)) { + if (!isSafeToAdoptBootstrapOAuthIdentity(params.credential, imported)) { log.warn("refused external oauth bootstrap credential: identity mismatch or missing binding", { profileId: params.profileId, provider: params.credential.provider, @@ -443,6 +462,7 @@ export function createOAuthManager(adapter: OAuthManagerAdapter) { if (!cred || cred.type !== "oauth") { return null; } + let credentialToRefresh = cred; if (hasUsableOAuthCredential(cred)) { return { @@ -501,7 +521,7 @@ export function createOAuthManager(adapter: OAuthManagerAdapter) { profileId: params.profileId, provider: cred.provider, }); - } else if (!isSafeToOverwriteStoredOAuthIdentity(cred, externallyManaged)) { + } else if (!isSafeToAdoptBootstrapOAuthIdentity(cred, externallyManaged)) { log.warn( "refused external oauth bootstrap credential: identity mismatch or missing binding", { @@ -517,6 +537,7 @@ export function createOAuthManager(adapter: OAuthManagerAdapter) { store.profiles[params.profileId] = { ...externallyManaged }; saveAuthProfileStore(store, params.agentDir); } + credentialToRefresh = externallyManaged; if (hasUsableOAuthCredential(externallyManaged)) { return { apiKey: await adapter.buildApiKey(externallyManaged.provider, externallyManaged), @@ -530,10 +551,10 @@ export function createOAuthManager(adapter: OAuthManagerAdapter) { `refreshOAuthCredential(${cred.provider})`, OAUTH_REFRESH_CALL_TIMEOUT_MS, async () => { - const refreshed = await adapter.refreshCredential(cred); + const refreshed = await adapter.refreshCredential(credentialToRefresh); return refreshed ? ({ - ...cred, + ...credentialToRefresh, ...refreshed, type: "oauth", } satisfies OAuthCredential) diff --git a/src/agents/cli-runner/prepare.test.ts b/src/agents/cli-runner/prepare.test.ts new file mode 100644 index 00000000000..62a008d7204 --- /dev/null +++ b/src/agents/cli-runner/prepare.test.ts @@ -0,0 +1,36 @@ +import { describe, expect, it } from "vitest"; +import { shouldSkipLocalCliCredentialEpoch } from "./prepare.js"; + +describe("shouldSkipLocalCliCredentialEpoch", () => { + it("skips local cli auth only when a profile-owned execution was prepared", () => { + expect( + shouldSkipLocalCliCredentialEpoch({ + authEpochMode: "profile-only", + authProfileId: "openai-codex:default", + authCredential: { + type: "oauth", + provider: "openai-codex", + access: "access-token", + refresh: "refresh-token", + expires: Date.now() + 60_000, + }, + preparedExecution: { + env: { + CODEX_HOME: "/tmp/codex-home", + }, + }, + }), + ).toBe(true); + }); + + it("keeps local cli auth in the epoch when the selected profile has no bridgeable execution", () => { + expect( + shouldSkipLocalCliCredentialEpoch({ + authEpochMode: "profile-only", + authProfileId: "openai-codex:default", + authCredential: undefined, + preparedExecution: null, + }), + ).toBe(false); + }); +}); diff --git a/src/agents/cli-runner/prepare.ts b/src/agents/cli-runner/prepare.ts index 8df69239616..3f06d5f43df 100644 --- a/src/agents/cli-runner/prepare.ts +++ b/src/agents/cli-runner/prepare.ts @@ -3,6 +3,10 @@ import { createMcpLoopbackServerConfig, getActiveMcpLoopbackRuntime, } from "../../gateway/mcp-http.loopback-runtime.js"; +import type { + CliBackendAuthEpochMode, + CliBackendPreparedExecution, +} from "../../plugins/cli-backend.types.js"; import { resolveOpenClawAgentDir } from "../agent-paths.js"; import { resolveSessionAgentIds } from "../agent-scope.js"; import { loadAuthProfileStoreForRuntime } from "../auth-profiles/store.js"; @@ -51,6 +55,20 @@ export function setCliRunnerPrepareTestDeps(overrides: Partial { @@ -88,12 +106,6 @@ export async function prepareCliRunContext( }); authCredential = authStore.profiles[effectiveAuthProfileId]; } - const authEpoch = await resolveCliAuthEpoch({ - provider: params.provider, - authProfileId: effectiveAuthProfileId, - skipLocalCredential: - backendResolved.authEpochMode === "profile-only" && Boolean(effectiveAuthProfileId), - }); const extraSystemPrompt = params.extraSystemPrompt?.trim() ?? ""; const extraSystemPromptHash = hashCliSessionText(extraSystemPrompt); const modelId = (params.model ?? "default").trim() || "default"; @@ -175,6 +187,16 @@ export async function prepareCliRunContext( authProfileId: effectiveAuthProfileId, authCredential, }); + const authEpoch = await resolveCliAuthEpoch({ + provider: params.provider, + authProfileId: effectiveAuthProfileId, + skipLocalCredential: shouldSkipLocalCliCredentialEpoch({ + authEpochMode: backendResolved.authEpochMode, + authProfileId: effectiveAuthProfileId, + authCredential, + preparedExecution, + }), + }); const preparedBackendEnv = preparedExecution?.env && Object.keys(preparedExecution.env).length > 0 ? { ...preparedBackend.env, ...preparedExecution.env } diff --git a/src/infra/secret-file.test.ts b/src/infra/secret-file.test.ts index b2c4ebdffe3..8aea56c52fd 100644 --- a/src/infra/secret-file.test.ts +++ b/src/infra/secret-file.test.ts @@ -1,12 +1,15 @@ -import { mkdir, symlink, writeFile } from "node:fs/promises"; +import { mkdir, stat, symlink, writeFile } from "node:fs/promises"; import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import { createTrackedTempDirs } from "../test-utils/tracked-temp-dirs.js"; import { DEFAULT_SECRET_FILE_MAX_BYTES, loadSecretFileSync, + PRIVATE_SECRET_DIR_MODE, + PRIVATE_SECRET_FILE_MODE, readSecretFileSync, tryReadSecretFileSync, + writePrivateSecretFileAtomic, } from "./secret-file.js"; const tempDirs = createTrackedTempDirs(); @@ -187,3 +190,60 @@ describe("readSecretFileSync", () => { expect(tryReadSecretFileSync(file, label, options)).toBe((expected as () => undefined)()); }); }); + +describe("writePrivateSecretFileAtomic", () => { + it("writes a private file with owner-only permissions", async () => { + const dir = await createTempDir(); + const file = path.join(dir, "nested", "auth.json"); + + await writePrivateSecretFileAtomic({ + rootDir: dir, + filePath: file, + content: '{"ok":true}\n', + }); + + expect(loadSecretFileSync(file, "Gateway password")).toMatchObject({ + ok: true, + secret: '{"ok":true}', + }); + if (process.platform !== "win32") { + const dirStat = await stat(path.dirname(file)); + const fileStat = await stat(file); + expect(dirStat.mode & 0o777).toBe(PRIVATE_SECRET_DIR_MODE); + expect(fileStat.mode & 0o777).toBe(PRIVATE_SECRET_FILE_MODE); + } + }); + + it("rejects symlinked target files", async () => { + const dir = await createTempDir(); + const nestedDir = path.join(dir, "nested"); + const target = path.join(dir, "outside.txt"); + const link = path.join(nestedDir, "auth.json"); + await mkdir(nestedDir); + await writeFile(target, "outside", "utf8"); + await symlink(target, link); + + await expect( + writePrivateSecretFileAtomic({ + rootDir: dir, + filePath: link, + content: '{"ok":true}\n', + }), + ).rejects.toThrow("must not be a symlink"); + }); + + it("rejects symlinked path components", async () => { + const dir = await createTempDir(); + const targetDir = path.join(dir, "outside-dir"); + await mkdir(targetDir); + await symlink(targetDir, path.join(dir, "linked")); + + await expect( + writePrivateSecretFileAtomic({ + rootDir: dir, + filePath: path.join(dir, "linked", "auth.json"), + content: '{"ok":true}\n', + }), + ).rejects.toThrow("must not be a symlink"); + }); +}); diff --git a/src/infra/secret-file.ts b/src/infra/secret-file.ts index 0d10e605ce5..e4d6e337b82 100644 --- a/src/infra/secret-file.ts +++ b/src/infra/secret-file.ts @@ -1,8 +1,13 @@ +import { randomBytes } from "node:crypto"; import fs from "node:fs"; +import fsp from "node:fs/promises"; +import path from "node:path"; import { resolveUserPath } from "../utils.js"; import { openVerifiedFileSync } from "./safe-open-sync.js"; export const DEFAULT_SECRET_FILE_MAX_BYTES = 16 * 1024; +export const PRIVATE_SECRET_DIR_MODE = 0o700; +export const PRIVATE_SECRET_FILE_MODE = 0o600; export type SecretFileReadOptions = { maxBytes?: number; @@ -138,3 +143,102 @@ export function tryReadSecretFileSync( const result = loadSecretFileSync(filePath, label, options); return result.ok ? result.secret : undefined; } + +function assertPathWithinRoot(rootDir: string, targetPath: string): void { + const relative = path.relative(rootDir, targetPath); + if (!relative || relative.startsWith("..") || path.isAbsolute(relative)) { + throw new Error(`Private secret path must stay under ${rootDir}.`); + } +} + +async function ensurePrivateDirectory(rootDir: string, targetDir: string): Promise { + const resolvedRoot = path.resolve(rootDir); + const resolvedTarget = path.resolve(targetDir); + if (resolvedTarget === resolvedRoot) { + await fsp.mkdir(resolvedRoot, { recursive: true, mode: PRIVATE_SECRET_DIR_MODE }); + const rootStat = await fsp.lstat(resolvedRoot); + if (rootStat.isSymbolicLink()) { + throw new Error(`Private secret root ${resolvedRoot} must not be a symlink.`); + } + if (!rootStat.isDirectory()) { + throw new Error(`Private secret root ${resolvedRoot} must be a directory.`); + } + await fsp.chmod(resolvedRoot, PRIVATE_SECRET_DIR_MODE).catch(() => undefined); + return; + } + + assertPathWithinRoot(resolvedRoot, resolvedTarget); + await ensurePrivateDirectory(resolvedRoot, resolvedRoot); + + let current = resolvedRoot; + for (const segment of path + .relative(resolvedRoot, resolvedTarget) + .split(path.sep) + .filter(Boolean)) { + current = path.join(current, segment); + try { + const stat = await fsp.lstat(current); + if (stat.isSymbolicLink()) { + throw new Error(`Private secret directory component ${current} must not be a symlink.`); + } + if (!stat.isDirectory()) { + throw new Error(`Private secret directory component ${current} must be a directory.`); + } + } catch (error) { + if (!error || typeof error !== "object" || !("code" in error) || error.code !== "ENOENT") { + throw error; + } + await fsp.mkdir(current, { mode: PRIVATE_SECRET_DIR_MODE }); + } + await fsp.chmod(current, PRIVATE_SECRET_DIR_MODE).catch(() => undefined); + } +} + +export async function writePrivateSecretFileAtomic(params: { + rootDir: string; + filePath: string; + content: string | Uint8Array; +}): Promise { + const resolvedRoot = path.resolve(params.rootDir); + const resolvedFile = path.resolve(params.filePath); + assertPathWithinRoot(resolvedRoot, resolvedFile); + const parentDir = path.dirname(resolvedFile); + await ensurePrivateDirectory(resolvedRoot, parentDir); + + try { + const stat = await fsp.lstat(resolvedFile); + if (stat.isSymbolicLink()) { + throw new Error(`Private secret file ${resolvedFile} must not be a symlink.`); + } + if (!stat.isFile()) { + throw new Error(`Private secret file ${resolvedFile} must be a regular file.`); + } + } catch (error) { + if (!error || typeof error !== "object" || !("code" in error) || error.code !== "ENOENT") { + throw error; + } + } + + const tempPath = path.join( + parentDir, + `.tmp-${process.pid}-${Date.now()}-${randomBytes(6).toString("hex")}`, + ); + let createdTemp = false; + try { + const handle = await fsp.open(tempPath, "wx", PRIVATE_SECRET_FILE_MODE); + createdTemp = true; + try { + await handle.writeFile(params.content); + } finally { + await handle.close(); + } + await fsp.chmod(tempPath, PRIVATE_SECRET_FILE_MODE).catch(() => undefined); + await fsp.rename(tempPath, resolvedFile); + createdTemp = false; + await fsp.chmod(resolvedFile, PRIVATE_SECRET_FILE_MODE).catch(() => undefined); + } finally { + if (createdTemp) { + await fsp.unlink(tempPath).catch(() => undefined); + } + } +} diff --git a/src/plugin-sdk/secret-file-runtime.ts b/src/plugin-sdk/secret-file-runtime.ts index 07eee264cec..2d7aebf2404 100644 --- a/src/plugin-sdk/secret-file-runtime.ts +++ b/src/plugin-sdk/secret-file-runtime.ts @@ -1,7 +1,10 @@ export { DEFAULT_SECRET_FILE_MAX_BYTES, + PRIVATE_SECRET_DIR_MODE, + PRIVATE_SECRET_FILE_MODE, loadSecretFileSync, readSecretFileSync, + writePrivateSecretFileAtomic, tryReadSecretFileSync, } from "../infra/secret-file.js"; export type { SecretFileReadOptions, SecretFileReadResult } from "../infra/secret-file.js";