From a001b5343ff0499ff06aef0779fc21f06b0ce2c7 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Fri, 17 Apr 2026 13:14:03 -0700 Subject: [PATCH] refactor(auth): make external cli oauth runtime-only --- .../auth-profiles.readonly-sync.test.ts | 34 ++++++-------- src/agents/auth-profiles/external-auth.ts | 8 ++++ src/agents/auth-profiles/external-cli-sync.ts | 38 ++++++++++++++++ .../auth-profiles/external-oauth.test.ts | 44 +++++++++++++++++++ src/agents/auth-profiles/store.ts | 40 ----------------- 5 files changed, 104 insertions(+), 60 deletions(-) diff --git a/src/agents/auth-profiles.readonly-sync.test.ts b/src/agents/auth-profiles.readonly-sync.test.ts index 45f0da9f883..f18bebd3164 100644 --- a/src/agents/auth-profiles.readonly-sync.test.ts +++ b/src/agents/auth-profiles.readonly-sync.test.ts @@ -5,37 +5,34 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { AUTH_STORE_VERSION } from "./auth-profiles/constants.js"; import type { AuthProfileStore } from "./auth-profiles/types.js"; -const mocks = vi.hoisted(() => ({ - syncExternalCliCredentials: vi.fn((store: AuthProfileStore) => { - store.profiles["minimax-portal:default"] = { - type: "oauth", +const resolveExternalAuthProfilesWithPluginsMock = vi.fn(() => [ + { + profileId: "minimax-portal:default", + credential: { + type: "oauth" as const, provider: "minimax-portal", access: "access-token", refresh: "refresh-token", expires: Date.now() + 60_000, - }; - return true; - }), -})); - -vi.mock("./auth-profiles/external-cli-sync.js", () => ({ - syncExternalCliCredentials: mocks.syncExternalCliCredentials, -})); + }, + persistence: "runtime-only" as const, + }, +]); vi.mock("../plugins/provider-runtime.js", () => ({ - resolveExternalAuthProfilesWithPlugins: () => [], + resolveExternalAuthProfilesWithPlugins: resolveExternalAuthProfilesWithPluginsMock, })); let clearRuntimeAuthProfileStoreSnapshots: typeof import("./auth-profiles.js").clearRuntimeAuthProfileStoreSnapshots; let loadAuthProfileStoreForRuntime: typeof import("./auth-profiles.js").loadAuthProfileStoreForRuntime; -describe("auth profiles read-only external CLI sync", () => { +describe("auth profiles read-only external auth overlay", () => { beforeEach(async () => { vi.resetModules(); ({ clearRuntimeAuthProfileStoreSnapshots, loadAuthProfileStoreForRuntime } = await import("./auth-profiles.js")); clearRuntimeAuthProfileStoreSnapshots(); - mocks.syncExternalCliCredentials.mockClear(); + resolveExternalAuthProfilesWithPluginsMock.mockClear(); }); afterEach(() => { @@ -43,7 +40,7 @@ describe("auth profiles read-only external CLI sync", () => { vi.clearAllMocks(); }); - it("syncs external CLI credentials in-memory without writing auth-profiles.json in read-only mode", () => { + it("overlays runtime-only external auth without writing auth-profiles.json in read-only mode", () => { const agentDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-auth-readonly-sync-")); try { const authPath = path.join(agentDir, "auth-profiles.json"); @@ -61,10 +58,7 @@ describe("auth profiles read-only external CLI sync", () => { const loaded = loadAuthProfileStoreForRuntime(agentDir, { readOnly: true }); - expect(mocks.syncExternalCliCredentials).toHaveBeenCalledWith( - expect.any(Object), - expect.objectContaining({ log: false }), - ); + expect(resolveExternalAuthProfilesWithPluginsMock).toHaveBeenCalled(); expect(loaded.profiles["minimax-portal:default"]).toMatchObject({ type: "oauth", provider: "minimax-portal", diff --git a/src/agents/auth-profiles/external-auth.ts b/src/agents/auth-profiles/external-auth.ts index c34ab68ed39..14004b269f9 100644 --- a/src/agents/auth-profiles/external-auth.ts +++ b/src/agents/auth-profiles/external-auth.ts @@ -1,5 +1,6 @@ import type { ProviderExternalAuthProfile } from "../../plugins/provider-external-auth.types.js"; import { resolveExternalAuthProfilesWithPlugins } from "../../plugins/provider-runtime.js"; +import { resolveExternalCliAuthProfiles } from "./external-cli-sync.js"; import type { AuthProfileStore, OAuthCredential } from "./types.js"; type ExternalAuthProfileMap = Map; @@ -48,6 +49,13 @@ function resolveExternalAuthProfileMap(params: { }); const resolved: ExternalAuthProfileMap = new Map(); + for (const profile of resolveExternalCliAuthProfiles(params.store)) { + resolved.set(profile.profileId, { + profileId: profile.profileId, + credential: profile.credential, + persistence: "runtime-only", + }); + } for (const rawProfile of profiles) { const profile = normalizeExternalAuthProfile(rawProfile); if (!profile) { diff --git a/src/agents/auth-profiles/external-cli-sync.ts b/src/agents/auth-profiles/external-cli-sync.ts index 3884a072c62..759b354061c 100644 --- a/src/agents/auth-profiles/external-cli-sync.ts +++ b/src/agents/auth-profiles/external-cli-sync.ts @@ -14,6 +14,11 @@ type ExternalCliSyncOptions = { log?: boolean; }; +export type ExternalCliResolvedProfile = { + profileId: string; + credential: OAuthCredential; +}; + type ExternalCliSyncProvider = { profileId: string; provider: string; @@ -94,6 +99,11 @@ function withExternalCliManager( }; } +function stripExternalCliManager(creds: OAuthCredential): OAuthCredential { + const { managedBy: _managedBy, ...runtimeCredential } = creds; + return runtimeCredential; +} + function resolveExternalCliSyncProvider(params: { profileId?: string; credential?: OAuthCredential; @@ -133,6 +143,34 @@ export function readManagedExternalCliCredential(params: { return withExternalCliManager(creds, provider.managedBy); } +export function resolveExternalCliAuthProfiles( + store: AuthProfileStore, +): ExternalCliResolvedProfile[] { + const profiles: ExternalCliResolvedProfile[] = []; + for (const providerConfig of EXTERNAL_CLI_SYNC_PROVIDERS) { + const creds = providerConfig.readCredentials(); + if (!creds) { + continue; + } + const runtimeCredential = stripExternalCliManager( + withExternalCliManager(creds, providerConfig.managedBy), + ); + const existing = store.profiles[providerConfig.profileId]; + const existingOAuth = existing?.type === "oauth" ? existing : undefined; + if ( + !shouldReplaceStoredOAuthCredential(existingOAuth, runtimeCredential) && + !areOAuthCredentialsEquivalent(existingOAuth, runtimeCredential) + ) { + continue; + } + profiles.push({ + profileId: providerConfig.profileId, + credential: runtimeCredential, + }); + } + return profiles; +} + /** Sync external CLI credentials into the store for a given provider. */ function syncExternalCliCredentialsForProvider( store: AuthProfileStore, diff --git a/src/agents/auth-profiles/external-oauth.test.ts b/src/agents/auth-profiles/external-oauth.test.ts index 161732aea48..d1f6302937c 100644 --- a/src/agents/auth-profiles/external-oauth.test.ts +++ b/src/agents/auth-profiles/external-oauth.test.ts @@ -10,6 +10,14 @@ import type { AuthProfileStore, OAuthCredential } from "./types.js"; const resolveExternalAuthProfilesWithPluginsMock = vi.fn< (params: unknown) => ProviderExternalAuthProfile[] >(() => []); +const { readCodexCliCredentialsCachedMock } = vi.hoisted(() => ({ + readCodexCliCredentialsCachedMock: vi.fn<() => OAuthCredential | null>(() => null), +})); + +vi.mock("../cli-credentials.js", () => ({ + readCodexCliCredentialsCached: readCodexCliCredentialsCachedMock, + readMiniMaxCliCredentialsCached: () => null, +})); function createStore(profiles: AuthProfileStore["profiles"] = {}): AuthProfileStore { return { version: 1, profiles }; @@ -30,6 +38,8 @@ describe("auth external oauth helpers", () => { beforeEach(() => { resolveExternalAuthProfilesWithPluginsMock.mockReset(); resolveExternalAuthProfilesWithPluginsMock.mockReturnValue([]); + readCodexCliCredentialsCachedMock.mockReset(); + readCodexCliCredentialsCachedMock.mockReturnValue(null); __testing.setResolveExternalAuthProfilesForTest(resolveExternalAuthProfilesWithPluginsMock); }); @@ -108,4 +118,38 @@ describe("auth external oauth helpers", () => { expect(shouldPersist).toBe(true); }); + + it("overlays fresher external CLI OAuth credentials without treating them as persisted store state", () => { + readCodexCliCredentialsCachedMock.mockReturnValue( + createCredential({ + access: "fresh-cli-access-token", + refresh: "fresh-cli-refresh-token", + expires: 456, + }), + ); + + const overlaid = overlayExternalOAuthProfiles( + createStore({ + "openai-codex:default": createCredential({ + access: "stale-store-access-token", + refresh: "stale-store-refresh-token", + expires: 123, + }), + }), + ); + + expect(overlaid.profiles["openai-codex:default"]).toMatchObject({ + access: "fresh-cli-access-token", + refresh: "fresh-cli-refresh-token", + expires: 456, + }); + + const shouldPersist = shouldPersistExternalOAuthProfile({ + store: overlaid, + profileId: "openai-codex:default", + credential: overlaid.profiles["openai-codex:default"] as OAuthCredential, + }); + + expect(shouldPersist).toBe(false); + }); }); diff --git a/src/agents/auth-profiles/store.ts b/src/agents/auth-profiles/store.ts index c939de13468..a036e56b1dc 100644 --- a/src/agents/auth-profiles/store.ts +++ b/src/agents/auth-profiles/store.ts @@ -8,7 +8,6 @@ import { log, } from "./constants.js"; import { overlayExternalAuthProfiles, shouldPersistExternalAuthProfile } from "./external-auth.js"; -import { syncExternalCliCredentials } from "./external-cli-sync.js"; import { ensureAuthStoreFile, resolveAuthStatePath, @@ -149,34 +148,9 @@ export async function updateAuthProfileStoreWithLock(params: { } } -function shouldLogAuthStoreTiming(): boolean { - return process.env.OPENCLAW_DEBUG_INGRESS_TIMING === "1"; -} - -function syncExternalCliCredentialsTimed( - store: AuthProfileStore, - options?: Parameters[1], -): boolean { - if (!shouldLogAuthStoreTiming()) { - return syncExternalCliCredentials(store, options); - } - const startMs = Date.now(); - const mutated = syncExternalCliCredentials(store, options); - log.info( - `auth-store stage=external-cli-sync elapsedMs=${Date.now() - startMs} mutated=${mutated}`, - ); - return mutated; -} - -function shouldSyncExternalCliCredentials(options?: { syncExternalCli?: boolean }): boolean { - return options?.syncExternalCli !== false; -} - export function loadAuthProfileStore(): AuthProfileStore { const asStore = loadPersistedAuthProfileStore(); if (asStore) { - // Sync from external CLI tools on every load. - syncExternalCliCredentialsTimed(asStore); return overlayExternalAuthProfiles(asStore); } const legacy = loadLegacyAuthProfileStore(); @@ -186,12 +160,10 @@ export function loadAuthProfileStore(): AuthProfileStore { profiles: {}, }; applyLegacyAuthStore(store, legacy); - syncExternalCliCredentialsTimed(store); return overlayExternalAuthProfiles(store); } const store: AuthProfileStore = { version: AUTH_STORE_VERSION, profiles: {} }; - syncExternalCliCredentialsTimed(store); return overlayExternalAuthProfiles(store); } @@ -216,11 +188,6 @@ function loadAuthProfileStoreForAgent( } const asStore = loadPersistedAuthProfileStore(agentDir); if (asStore) { - // Runtime secret activation must remain read-only: - // sync external CLI credentials in-memory, but never persist while readOnly. - if (shouldSyncExternalCliCredentials(options)) { - syncExternalCliCredentialsTimed(asStore, { log: !readOnly }); - } if (!readOnly) { writeCachedAuthProfileStore({ authPath, @@ -260,10 +227,6 @@ function loadAuthProfileStoreForAgent( } const mergedOAuth = mergeOAuthFileIntoStore(store); - // Keep external CLI credentials visible in runtime even during read-only loads. - if (shouldSyncExternalCliCredentials(options)) { - syncExternalCliCredentialsTimed(store, { log: !readOnly }); - } const forceReadOnly = process.env.OPENCLAW_AUTH_STORE_READONLY === "1"; const shouldWrite = !readOnly && !forceReadOnly && (legacy !== null || mergedOAuth); if (shouldWrite) { @@ -394,9 +357,6 @@ export function saveAuthProfileStore( saveJsonFile(authPath, payload); savePersistedAuthProfileState(store, agentDir); const runtimeStore = cloneAuthProfileStore(store); - if (shouldSyncExternalCliCredentials(options)) { - syncExternalCliCredentialsTimed(runtimeStore, { log: false }); - } writeCachedAuthProfileStore({ authPath, authMtimeMs: readAuthStoreMtimeMs(authPath),