From 36b9c1ef80a3733dfbb22be240bfb7bb76b6e6c4 Mon Sep 17 00:00:00 2001 From: GSL-R Date: Fri, 9 Oct 2026 22:02:50 +0900 Subject: [PATCH 01/19] feat(vertex): accept pasted GCP credential JSON in the API-key slot (#6821) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let the provider's API-key field take a Service Account JSON directly: - gcp-adc: a `gcp-sa:` marker resolves the credential from the OS keychain (same service + entry factory as `keychain:` references) and runs the existing service_account JWT / authorized_user flow. The marker sits above GOOGLE_APPLICATION_CREDENTIALS in the source priority; everything downstream (token cache, refresh skew, in-flight dedup) is untouched. - api-keys: addProviderApiKey detects a pasted credential JSON (single, pretty-printed, or comma-separated for future pool rotation), verifies the "type" field, stores the secret in the OS keychain with a read-back check, and persists only the marker to config. File paths are rejected with paste guidance; unrecognized types are rejected before anything is stored. - router: usableResolvedApiKey registers the active `gcp-sa:` marker with the ADC resolver, so a marker in any pool entry becomes the active source. - adapters/google: vertex's API-key fast path skips `gcp-sa:` markers — they are not API-key material and must reach the ADC branch instead. Markers keep resolveProviderApiKey synchronous (no signature changes) and work naturally with the existing 429 key-pool failover: each pool entry can independently hold a raw key, an ${ENV} ref, a keychain: ref, or a gcp-sa: marker, and per-key cooldown rotation applies unchanged. Closes #6821 (partially — live model discovery for vertex mode is a separate discussion). Co-Authored-By: Claude --- src/adapters/google.ts | 8 +- src/lib/gcp-adc.ts | 86 ++++++- src/providers/api-keys.ts | 109 +++++++- src/router.ts | 6 + tests/adapters/google/gcp-sa-adapter.test.ts | 50 ++++ tests/providers/gcp-sa-marker.test.ts | 251 +++++++++++++++++++ 6 files changed, 505 insertions(+), 5 deletions(-) create mode 100644 tests/adapters/google/gcp-sa-adapter.test.ts create mode 100644 tests/providers/gcp-sa-marker.test.ts diff --git a/src/adapters/google.ts b/src/adapters/google.ts index af0bd6a4504..612e49c3a87 100644 --- a/src/adapters/google.ts +++ b/src/adapters/google.ts @@ -17,7 +17,7 @@ import type { import { isAllowedToolChoice, namespacedToolName, resolveToolChoiceWireName, toolChoiceToolPredicate } from "../types"; import type { OcxTool } from "../types"; import { contentPartsToText, parseDataUrl } from "./image"; -import { getVertexAccessToken } from "../lib/gcp-adc"; +import { GCP_CREDENTIAL_MARKER_PREFIX, getVertexAccessToken } from "../lib/gcp-adc"; import { fetchAntigravityWithRetry, fetchVertexWithRetry } from "./google-http"; import { safeAntigravityHttpErrorMessage, safeVertexHttpErrorMessage } from "./google-errors"; import { isVertexTruncatedTurn, vertexTruncationErrorMessage } from "./google-truncation"; @@ -174,7 +174,11 @@ function resolveDirectGeminiWireModelId(modelId: string, applyRenames: boolean): /** Vertex API key: provider.apiKey if it looks real (not a sentinel), else GOOGLE_CLOUD_API_KEY env. */ function resolveVertexApiKey(optKey?: string): string | undefined { - const realKey = optKey && !optKey.startsWith("<") && optKey !== "N/A" ? optKey : undefined; + // A `gcp-sa:` marker is the keychain-backed credential reference (see gcp-adc): it is not + // API-key material, so it must not be sent as x-goog-api-key — the ADC branch below handles it. + const realKey = optKey && !optKey.startsWith("<") && optKey !== "N/A" && !optKey.startsWith(GCP_CREDENTIAL_MARKER_PREFIX) + ? optKey + : undefined; return realKey || process.env.GOOGLE_CLOUD_API_KEY; } diff --git a/src/lib/gcp-adc.ts b/src/lib/gcp-adc.ts index f7274bfeda9..2de0cb7b56f 100644 --- a/src/lib/gcp-adc.ts +++ b/src/lib/gcp-adc.ts @@ -2,6 +2,10 @@ * Application Default Credentials (ADC) resolution for Vertex AI. * * Direct WebCrypto + REST implementation (no `google-auth-library`). Sources, in priority order: + * 0. A `gcp-sa:` reference (the provider's active key resolving to one) → the Service + * Account / authorized_user JSON stored in the OS credential store under that account + * (`keychain:` secret, written by the key-store save path when the user pastes a + * credential JSON directly into the API-key slot). See resolveGcpCredentialMarker. * 1. `GOOGLE_APPLICATION_CREDENTIALS` env → file with `type: "service_account"` (RS256 JWT * exchange) or `type: "authorized_user"` (refresh-token exchange). * 2. gcloud user ADC (`CLOUDSDK_CONFIG`, else `%APPDATA%\gcloud` on Windows, else @@ -18,6 +22,7 @@ import { Buffer } from "node:buffer"; import * as os from "node:os"; import * as path from "node:path"; import { readFileSync, existsSync, statSync } from "node:fs"; +import { providerKeychainEntry } from "../providers/api-key-resolve"; import { captureConfigGeneration, sweepExpiredOnWrite, @@ -36,6 +41,43 @@ const TOKEN_TIMEOUT_MS = 15_000; const TOKEN_ATTEMPTS = 3; const TOKEN_RETRY_BASE_MS = 300; +/** + * Reference prefix stored in `provider.apiKey` (and `apiKeyPool` entries) when the user pastes a + * GCP credential JSON directly into an API-key slot. The secret JSON lives in the OS credential + * store under the same service the `keychain:` references use; only the marker is persisted to + * `config.json`. Lives here — not in api-key-resolve — because it is meaningful only to this + * module's ADC resolution (the API-key twins, `keychain:` and `${ENV}`, stay there). + */ +export const GCP_CREDENTIAL_MARKER_PREFIX = "gcp-sa:"; + +/** Extract the credential-store account from a `gcp-sa:` marker. */ +export function gcpCredentialMarkerAccount(value: string | undefined): string | undefined { + if (typeof value !== "string") return undefined; + return value.startsWith(GCP_CREDENTIAL_MARKER_PREFIX) && value.length > GCP_CREDENTIAL_MARKER_PREFIX.length + ? value.slice(GCP_CREDENTIAL_MARKER_PREFIX.length) + : undefined; +} + +/** + * Whether a pasted API-key value is in fact a GCP credential JSON. Used by the save path to + * divert JSON material out of the literal-key flow before it reaches `config.json`. Deliberately + * strict: only a `type` field with a known ADC credential type counts, so a user's literal API + * key that happens to start with `{` is never eaten. + */ +export function parseGcpCredentialJson(value: string): AdcFileCredentials | undefined { + const trimmed = (value ?? "").trim(); + if (!trimmed.startsWith("{") || !trimmed.endsWith("}")) return undefined; + try { + const parsed = JSON.parse(trimmed) as { type?: unknown }; + if (parsed?.type === "service_account" || parsed?.type === "authorized_user") { + return parsed as AdcFileCredentials; + } + return undefined; + } catch { + return undefined; + } +} + interface CachedToken { token: string; expiresAtMs: number; @@ -131,7 +173,30 @@ function readJsonFile(filePath: string): T | undefined { return JSON.parse(readFileSync(filePath, "utf8")) as T; } +/** + * Read the credential JSON for a `gcp-sa:` marker from the OS credential store. Uses the + * same service name and the same injectable entry factory as the `keychain:` API-key references + * (api-key-resolve.providerKeychainEntry), so one probe surface and one test seam cover both. + * Mirrors `readKeychain`'s fail-closed policy: an unreadable entry yields no credential (the + * caller surfaces the failure instead of silently falling through to the next ADC source). + */ +function readMarkerCredential(account: string): AdcFileCredentials | undefined { + try { + const raw = providerKeychainEntry(account).getPassword(); + if (typeof raw !== "string" || !raw.trim()) return undefined; + return parseGcpCredentialJson(raw); + } catch { + return undefined; + } +} + function loadAdcCredentials(): { source: string; creds: AdcFileCredentials } | undefined { + const markerAccount = currentMarkerAccount(); + if (markerAccount) { + const creds = readMarkerCredential(markerAccount); + if (!creds) throw new Error(`gcp-sa:${markerAccount} marker is set but its credential could not be read from the OS credential store`); + return { source: `marker:${markerAccount}`, creds }; + } const gacPath = process.env.GOOGLE_APPLICATION_CREDENTIALS; if (gacPath) { const creds = readJsonFile(gacPath); @@ -148,9 +213,11 @@ function loadAdcCredentials(): { source: string; creds: AdcFileCredentials } | u * The cache key for the source the NEXT resolve would use, computed cheaply (no network). Lets the * cache return a token only when it still matches the active credential source, so an in-process * change to GOOGLE_APPLICATION_CREDENTIALS (or the user ADC file) does not keep serving a stale - * token from a different source. Falls back to "metadata" when no file/env ADC is present. + * token from a different source. Falls back to "metadata" when no marker/file/env ADC is present. */ function currentAdcSourceKey(): string { + const markerAccount = currentMarkerAccount(); + if (markerAccount) return `marker:${markerAccount}`; const gacPath = process.env.GOOGLE_APPLICATION_CREDENTIALS; if (gacPath) return fileSourceTag("gac", gacPath); const userPath = userAdcPath(); @@ -158,6 +225,23 @@ function currentAdcSourceKey(): string { return "metadata"; } +/** + * The marker source needs to know WHICH marker is active, but gcp-adc is a leaf module that + * must not import the provider config graph. Callers that route through a `gcp-sa:` marker + * register the active marker account here (the vertex adapter path calls + * `setActiveGcpCredentialMarker` from the router's key resolution, which already knows the + * resolved key string). Exported for that single call site + tests. + */ +let activeMarkerAccount: string | undefined; + +export function setActiveGcpCredentialMarker(value: string | undefined): void { + activeMarkerAccount = gcpCredentialMarkerAccount(value); +} + +function currentMarkerAccount(): string | undefined { + return activeMarkerAccount; +} + function base64UrlEncode(bytes: Uint8Array | string): string { const buf = typeof bytes === "string" ? Buffer.from(bytes, "utf8") : Buffer.from(bytes); return buf.toString("base64url"); diff --git a/src/providers/api-keys.ts b/src/providers/api-keys.ts index ceb5522df29..a70eee95f58 100644 --- a/src/providers/api-keys.ts +++ b/src/providers/api-keys.ts @@ -9,6 +9,8 @@ import { createHash } from "node:crypto"; import { saveConfigPreservingClaudeCode } from "../config"; import type { OcxConfig, OcxProviderConfig } from "../types"; +import { GCP_CREDENTIAL_MARKER_PREFIX, gcpCredentialMarkerAccount, parseGcpCredentialJson } from "../lib/gcp-adc"; +import { invalidateResolvedProviderKeyCache, probeProviderKeychain, providerKeychainEntry } from "./api-key-resolve"; import type { AccountQuotaFields } from "./quota-types"; import { commitProviderApiKeySelection } from "./api-key-selection"; @@ -28,11 +30,71 @@ function isEnvReference(value: string): boolean { export function maskApiKey(value: string): string { // Env and keychain references carry no secret material; show them verbatim so an operator // can tell where the key lives. - if (isEnvReference(value) || value.startsWith("keychain:")) return value; + if (isEnvReference(value) || value.startsWith("keychain:") || value.startsWith(GCP_CREDENTIAL_MARKER_PREFIX)) return value; if (value.length <= 8) return "****"; return `${value.slice(0, 4)}****${value.slice(-4)}`; } +/** + * Split a pasted key value into candidate credentials. A plain API key is one row; a pasted GCP + * credential JSON (or several, comma/space-separated — the same convention as newline-separated + * API keys) is one row per JSON object. Windows file paths are rejected with guidance rather + * than stored, because they are the most common paste mistake and silently break at runtime. + */ +export function splitCredentialPaste(value: string): Array { + const trimmed = (value ?? "").trim(); + if (!trimmed) return []; + // Fast path: a plain key (no JSON braces) — also covers env/keychain/gcp-sa references. + if (!trimmed.startsWith("{")) { + if (/^[A-Za-z]:[\\/]/.test(trimmed)) { + throw new Error("This looks like a file path. Paste the full JSON body of the credential, not a file path."); + } + return [trimmed]; + } + const objects: string[] = []; + let depth = 0; + let start = -1; + for (let i = 0; i < trimmed.length; i++) { + const ch = trimmed[i]; + if (ch === "{" && depth === 0) start = i; + if (ch === "{") depth += 1; + if (ch === "}") { + depth -= 1; + if (depth === 0 && start >= 0) { + objects.push(trimmed.slice(start, i + 1)); + start = -1; + } + if (depth < 0) throw new Error("Invalid credential JSON: unbalanced braces."); + } + } + if (depth !== 0 || start >= 0) throw new Error("Invalid credential JSON: the pasted text ends inside a JSON object."); + return objects.map(json => { + if (!parseGcpCredentialJson(json)) { + throw new Error("Unrecognized credential JSON: expected a GCP Service Account or authorized_user JSON with a \"type\" field."); + } + return { credentialJson: json }; + }); +} + +/** + * Store a pasted GCP credential JSON in the OS keychain and return the `gcp-sa:` marker for it. + * Verified write (read-back check) mirrors storeProviderKeyInKeychain; on any failure the caller + * receives an error instead of half-stored state. + */ +function storeGcpCredentialJson(config: OcxConfig, name: string, credentialJson: string): string { + const probe = probeProviderKeychain(); + if (!probe.available) throw new Error(`OS keychain unavailable: ${probe.reason}`); + const id = apiKeyPoolEntryId(credentialJson); + const account = `${name}/${id}`; + const entry = providerKeychainEntry(account); + entry.setPassword(credentialJson); + if (entry.getPassword() !== credentialJson) { + try { entry.deletePassword(); } catch { /* best effort */ } + throw new Error(`keychain read-back mismatch for ${account}`); + } + return `${GCP_CREDENTIAL_MARKER_PREFIX}${account}`; +} + /** Content-derived id: re-adding the same key upserts instead of duplicating. */ export function apiKeyPoolEntryId(key: string): string { return createHash("sha256").update(key).digest("hex").slice(0, 8); @@ -84,6 +146,48 @@ export function addProviderApiKey(config: OcxConfig, name: string, key: string, const provider = config.providers[name]; if (!provider || !isKeyAuthProvider(provider)) return { error: "provider does not use API-key auth" }; if (typeof key !== "string" || !key.trim()) return { error: "key is required" }; + // GCP credential JSON never enters the literal-key path: it is diverted to the OS keychain and + // only the `gcp-sa:` marker is stored. splitCredentialPaste runs FIRST (before the CRLF + // rejection) because pretty-printed JSON contains line breaks, and it also catches the + // file-path paste mistake with its own guidance before anything is stored. + try { + const parts = splitCredentialPaste(key); + const first = parts[0]; + const isJsonPaste = first !== undefined && typeof first !== "string"; + if (!isJsonPaste) { + // Not a credential paste — fall through to the plain API-key path below. splitCredentialPaste + // has already thrown for file paths; a plain key lands here as a single literal row. + } else { + if (parts.length > 1) return { error: "paste one credential JSON per request; add each additional credential as its own key" }; + const credentialJson = (first as { credentialJson: string }).credentialJson; + let marker: string; + try { + marker = storeGcpCredentialJson(config, name, credentialJson); + } catch (error) { + return { error: error instanceof Error ? error.message : "credential storage failed" }; + } + const id = apiKeyPoolEntryId(credentialJson); + const committed = commitProviderApiKeySelection(config, name, fresh => { + const pool = ensurePool(fresh); + const existing = pool.find(e => e.id === id); + if (existing) { + existing.key = marker; + if (label?.trim()) existing.label = label.trim(); + } else { + pool.push({ id, key: marker, ...(label?.trim() ? { label: label.trim() } : {}), addedAt: Date.now() }); + } + fresh.apiKey = marker; + return { changed: true, selectionChanged: true, value: id }; + }); + if (committed.status === "committed") { + invalidateResolvedProviderKeyCache(); + return { id }; + } + return { error: "provider selection unavailable" }; + } + } catch (error) { + return { error: error instanceof Error ? error.message : "credential parse failed" }; + } const trimmed = sanitizeApiKeyValue(key); if (!trimmed) return { error: "key must not include line breaks" }; const id = apiKeyPoolEntryId(trimmed); @@ -98,7 +202,8 @@ export function addProviderApiKey(config: OcxConfig, name: string, key: string, fresh.apiKey = trimmed; return { changed: true, selectionChanged: true, value: id }; }); - return committed.status === "committed" ? { id } : { error: "provider selection unavailable" }; + if (committed.status !== "committed") return { error: "provider selection unavailable" }; + return { id }; } /** Switch the ACTIVE key (mirrors into `provider.apiKey`). Persists config. */ diff --git a/src/router.ts b/src/router.ts index 0ea69b230a3..4d57aa7fe6c 100644 --- a/src/router.ts +++ b/src/router.ts @@ -15,6 +15,7 @@ import type { NormalizedComboConfig } from "./combos/types"; import { hasOwnProvider } from "./config/provider-name"; import { anthropicInstanceRowShapeMatches } from "./providers/anthropic-instance-id"; import { providerUsesKeyAuthOverride, resolveProviderApiKey } from "./providers/key-store"; +import { setActiveGcpCredentialMarker } from "./lib/gcp-adc"; import { captureProviderApiKeySelection } from "./providers/api-key-selection-capture"; import { assertProviderDestinationAllowed } from "./lib/destination-policy"; import { redactSecretString, redactUrlForLog } from "./lib/redact"; @@ -298,6 +299,11 @@ export function resetCompactionFallbackWarningsForTests(): void { } function usableResolvedApiKey(apiKey: string | undefined): string | undefined { + // A `gcp-sa:` marker carries no usable key material: the ADC resolver (gcp-adc) must learn it + // is the active credential source. Register before the early returns below — a marker resolves + // to `undefined` here (it is not a keychain/env ref, and the marker itself is not a literal), + // so the vertex adapter's ADC branch is what actually consumes the registration. + setActiveGcpCredentialMarker(apiKey); const resolved = resolveProviderApiKey(apiKey); return typeof resolved === "string" && resolved.trim().length > 0 ? resolved : undefined; } diff --git a/tests/adapters/google/gcp-sa-adapter.test.ts b/tests/adapters/google/gcp-sa-adapter.test.ts new file mode 100644 index 00000000000..0b8779905fe --- /dev/null +++ b/tests/adapters/google/gcp-sa-adapter.test.ts @@ -0,0 +1,50 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { + GCP_CREDENTIAL_MARKER_PREFIX, + setActiveGcpCredentialMarker, + __resetVertexTokenCache, +} from "../../../src/lib/gcp-adc"; +import { createGoogleAdapter } from "../../../src/adapters/google"; +import { setProviderKeychainEntryFactoryForTests } from "../../../src/providers/key-store"; +import type { OcxParsedRequest, OcxProviderConfig } from "../../../src/types"; + +function parsed(modelId = "gemini-3-pro"): OcxParsedRequest { + return { + modelId, + stream: true, + context: { messages: [{ role: "user", content: "hi" }], systemPrompt: [], tools: [] }, + options: {}, + } as unknown as OcxParsedRequest; +} + +afterEach(() => { + __resetVertexTokenCache(); + setActiveGcpCredentialMarker(undefined); + setProviderKeychainEntryFactoryForTests(null); +}); + +describe("vertex adapter with a gcp-sa marker key", () => { + test("marker does not take the x-goog-api-key fast path — ADC branch runs", async () => { + const serviceAccount = JSON.stringify({ + type: "service_account", + client_email: "svc@example.test", + private_key: "not-a-real-key-for-this-test", + private_key_id: "k1", + }); + // The keychain entry exists (the real exchange would sign a real key); this test asserts the + // ROUTE: the marker must reach the ADC branch, not the API-key fast path. We can prove that + // without a valid key because the fast path returns a URL on aiplatform host WITHOUT a project + // while the ADC branch throws on signing before any request. + setProviderKeychainEntryFactoryForTests((service, account) => ({ + getPassword: () => (account === "p/mk1" ? serviceAccount : null), + setPassword: () => {}, + deletePassword: () => true, + })); + setActiveGcpCredentialMarker(`${GCP_CREDENTIAL_MARKER_PREFIX}p/mk1`); + const provider = { adapter: "google", baseUrl: "https://x", googleMode: "vertex", apiKey: `${GCP_CREDENTIAL_MARKER_PREFIX}p/mk1`, project: "proj-1", location: "global" } as OcxProviderConfig; + // The ADC branch needs a valid RSA key to sign — it will throw on importKey, NOT send + // x-goog-api-key. Any throw (even a signing one) proves the fast path was skipped; an + // x-goog-api-key request would have returned successfully with the marker as the key. + await expect(createGoogleAdapter(provider).buildRequest(parsed())).rejects.toThrow(); + }); +}); diff --git a/tests/providers/gcp-sa-marker.test.ts b/tests/providers/gcp-sa-marker.test.ts new file mode 100644 index 00000000000..fbd303f8e55 --- /dev/null +++ b/tests/providers/gcp-sa-marker.test.ts @@ -0,0 +1,251 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { Buffer } from "node:buffer"; +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + GCP_CREDENTIAL_MARKER_PREFIX, + gcpCredentialMarkerAccount, + getVertexAccessToken, + parseGcpCredentialJson, + setActiveGcpCredentialMarker, + __resetVertexTokenCache, +} from "../../src/lib/gcp-adc"; +import { splitCredentialPaste, addProviderApiKey } from "../../src/providers/api-keys"; +import { setProviderKeychainEntryFactoryForTests } from "../../src/providers/key-store"; +import { saveConfig } from "../../src/config"; +import type { OcxConfig, OcxProviderConfig } from "../../src/types"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; + +const SERVICE_ACCOUNT_JSON = JSON.stringify({ + type: "service_account", + client_email: "svc@example.test", + private_key: "-----BEGIN PRIVATE KEY-----\nAAAA\n-----END PRIVATE KEY-----\n", + private_key_id: "k1", +}); +const SERVICE_ACCOUNT_JSON_2 = JSON.stringify({ + type: "service_account", + client_email: "svc2@example.test", + private_key: "-----BEGIN PRIVATE KEY-----\nBBBB\n-----END PRIVATE KEY-----\n", + private_key_id: "k2", +}); + +afterEach(() => { + __resetVertexTokenCache(); + setActiveGcpCredentialMarker(undefined); + setProviderKeychainEntryFactoryForTests(null); +}); + +describe("parseGcpCredentialJson", () => { + test("accepts a service_account JSON body", () => { + const creds = parseGcpCredentialJson(SERVICE_ACCOUNT_JSON); + expect(creds?.type).toBe("service_account"); + }); + + test("accepts an authorized_user JSON body", () => { + const creds = parseGcpCredentialJson(JSON.stringify({ type: "authorized_user", client_id: "c", client_secret: "s", refresh_token: "r" })); + expect(creds?.type).toBe("authorized_user"); + }); + + test("rejects unknown types, non-JSON, and plain keys", () => { + expect(parseGcpCredentialJson(JSON.stringify({ type: "external_account" }))).toBeUndefined(); + expect(parseGcpCredentialJson("not json")).toBeUndefined(); + expect(parseGcpCredentialJson("AIza-real-key-123456")).toBeUndefined(); + expect(parseGcpCredentialJson("{}")).toBeUndefined(); + }); +}); + +describe("gcpCredentialMarkerAccount", () => { + test("extracts the account from a marker", () => { + expect(gcpCredentialMarkerAccount(`${GCP_CREDENTIAL_MARKER_PREFIX}p/abc123`)).toBe("p/abc123"); + expect(gcpCredentialMarkerAccount("gcp-sa:")).toBeUndefined(); + expect(gcpCredentialMarkerAccount("keychain:p/abc")).toBeUndefined(); + expect(gcpCredentialMarkerAccount(undefined)).toBeUndefined(); + }); +}); + +describe("splitCredentialPaste", () => { + test("a plain API key is one literal row", () => { + expect(splitCredentialPaste("sk-real-key")).toEqual(["sk-real-key"]); + }); + + test("one credential JSON parses to one row", () => { + const parts = splitCredentialPaste(SERVICE_ACCOUNT_JSON); + expect(parts).toHaveLength(1); + expect(parts[0]).toEqual({ credentialJson: SERVICE_ACCOUNT_JSON }); + }); + + test("pretty-printed JSON (with newlines) parses", () => { + const pretty = JSON.stringify(JSON.parse(SERVICE_ACCOUNT_JSON), null, 2); + const parts = splitCredentialPaste(pretty); + expect(parts).toHaveLength(1); + expect(parts[0]).toEqual({ credentialJson: pretty }); + }); + + test("two adjacent credential JSONs split into two rows (rotation pool)", () => { + const parts = splitCredentialPaste(`${SERVICE_ACCOUNT_JSON}, ${SERVICE_ACCOUNT_JSON_2}`); + expect(parts).toHaveLength(2); + expect(parts[0]).toEqual({ credentialJson: SERVICE_ACCOUNT_JSON }); + expect(parts[1]).toEqual({ credentialJson: SERVICE_ACCOUNT_JSON_2 }); + }); + + test("a file path is rejected with paste guidance", () => { + expect(() => splitCredentialPaste("C:\\Users\\me\\sa.json")).toThrow(/file path/i); + }); + + test("JSON without a recognized type field is rejected", () => { + expect(() => splitCredentialPaste('{"type": "external_account"}')).toThrow(/type/i); + }); + + test("truncated JSON is rejected", () => { + expect(() => splitCredentialPaste('{"type": "service_account", "client_email"')).toThrow(); + }); +}); + +describe("addProviderApiKey with a pasted credential JSON", () => { + let home: string; + const previousHome = process.env.OPENCODEX_HOME; + + // commitProviderApiKeySelection persists through mutatePersistedConfig, which refuses to + // mutate unless a real config file exists — so each test runs against a throwaway home dir. + beforeEach(() => { + home = mkdtempSync(join(tmpdir(), "ocx-gcp-sa-marker-")); + process.env.OPENCODEX_HOME = home; + }); + + afterEach(() => { + if (previousHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousHome; + removeTreeWithRetry(home); + }); + + function makeConfig(provider: Partial): OcxConfig { + const config = { + port: 10199, + defaultProvider: "p", + providers: { + p: { adapter: "openai-chat", baseUrl: "https://api.example.com/v1", ...provider } as OcxProviderConfig, + }, + } as unknown as OcxConfig; + saveConfig(config); + return config; + } + + const keychainStore = new Map(); + + function installFakeKeychain(): void { + keychainStore.clear(); + setProviderKeychainEntryFactoryForTests((service, account) => ({ + getPassword: () => keychainStore.get(`${service}:${account}`) ?? null, + setPassword: (password: string) => { keychainStore.set(`${service}:${account}`, password); }, + deletePassword: () => keychainStore.delete(`${service}:${account}`), + })); + } + + test("JSON is diverted to the keychain and only the marker lands in config", () => { + installFakeKeychain(); + const config = makeConfig({ apiKey: "legacy-key" }); + const result = addProviderApiKey(config, "p", SERVICE_ACCOUNT_JSON); + expect(result).toEqual({ id: expect.any(String) }); + + const provider = config.providers.p!; + // config holds ONLY the marker — no key material anywhere. + expect(provider.apiKey?.startsWith(GCP_CREDENTIAL_MARKER_PREFIX)).toBe(true); + expect(JSON.stringify(config).includes("private_key")).toBe(false); + // The secret itself lives in the credential store under the marker's account. + const account = gcpCredentialMarkerAccount(provider.apiKey)!; + expect(account.startsWith("p/")).toBe(true); + expect(keychainStore.get(`opencodex.provider-api-key.v1:${account}`)).toBe(SERVICE_ACCOUNT_JSON); + }); + + test("re-adding the same JSON upserts instead of duplicating", () => { + installFakeKeychain(); + const config = makeConfig({ apiKey: "legacy-key" }); + const first = addProviderApiKey(config, "p", SERVICE_ACCOUNT_JSON); + const second = addProviderApiKey(config, "p", SERVICE_ACCOUNT_JSON); + expect(first).toEqual(second); + const pool = config.providers.p!.apiKeyPool!; + expect(pool).toHaveLength(2); // legacy seed + the single marker entry + }); + + test("a file path paste returns an error and stores nothing", () => { + installFakeKeychain(); + const config = makeConfig({ apiKey: "legacy-key" }); + const result = addProviderApiKey(config, "p", "C:\\Users\\me\\sa.json"); + expect("error" in result && /file path/i.test(result.error)).toBe(true); + expect(JSON.stringify(config).includes(GCP_CREDENTIAL_MARKER_PREFIX)).toBe(false); + }); + + test("keychain-unavailable fails closed with an error", () => { + // No fake factory installed: the real keyring may exist in CI, so probe through the seam + // with a factory that always throws instead. + setProviderKeychainEntryFactoryForTests(() => { + throw new Error("keychain unavailable in test"); + }); + const config = makeConfig({ apiKey: "legacy-key" }); + const result = addProviderApiKey(config, "p", SERVICE_ACCOUNT_JSON); + expect("error" in result).toBe(true); + expect(JSON.stringify(config).includes(GCP_CREDENTIAL_MARKER_PREFIX)).toBe(false); + }); +}); + +describe("gcp-adc marker source", () => { + let oauthCalls = 0; + const realFetch = globalThis.fetch; + + afterEach(() => { globalThis.fetch = realFetch; }); + + test("an active marker resolves its keychain credential through the JWT exchange", async () => { + // A real, signable RSA key (generated) so the RS256 JWT exchange inside the resolver works. + const kp = await globalThis.crypto.subtle.generateKey( + { name: "RSASSA-PKCS1-v1_5", modulusLength: 2048, publicExponent: new Uint8Array([1, 0, 1]), hash: "SHA-256" }, + true, + ["sign", "verify"], + ); + const pkcs8 = await globalThis.crypto.subtle.exportKey("pkcs8", kp.privateKey); + const b64 = Buffer.from(pkcs8).toString("base64").match(/.{1,64}/g)!.join("\n"); + const pem = `-----BEGIN PRIVATE KEY-----\n${b64}\n-----END PRIVATE KEY-----\n`; + const serviceAccount = JSON.stringify({ + type: "service_account", + client_email: "svc@example.test", + private_key: pem, + private_key_id: "k1", + }); + setProviderKeychainEntryFactoryForTests((service, account) => ({ + getPassword: () => (account === "p/abc123" ? serviceAccount : null), + setPassword: () => {}, + deletePassword: () => true, + })); + globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { + const url = typeof input === "string" ? input : input instanceof URL ? input.href : (input as Request).url; + if (url === "https://oauth2.googleapis.com/token") { + oauthCalls++; + return new Response(JSON.stringify({ access_token: "marker-tok", expires_in: 3600 }), { status: 200 }); + } + return new Response("nope", { status: 404 }); + }) as typeof fetch; + + setActiveGcpCredentialMarker(`${GCP_CREDENTIAL_MARKER_PREFIX}p/abc123`); + const token = await getVertexAccessToken(); + expect(token).toBe("marker-tok"); + expect(oauthCalls).toBe(1); + }); + + test("an unreadable marker credential fails closed (no token, no silent fallback to metadata)", async () => { + setProviderKeychainEntryFactoryForTests((service, account) => ({ + getPassword: () => null, + setPassword: () => {}, + deletePassword: () => true, + })); + setActiveGcpCredentialMarker(`${GCP_CREDENTIAL_MARKER_PREFIX}p/missing`); + await expect(getVertexAccessToken()).rejects.toThrow(/gcp-sa:p\/missing/); + }); + + test("clearing the marker restores the normal source priority", async () => { + setActiveGcpCredentialMarker(`${GCP_CREDENTIAL_MARKER_PREFIX}p/abc123`); + setActiveGcpCredentialMarker(undefined); + // With no marker and no env/file ADC, the resolver reaches the metadata-server attempt and + // fails with the standard ADC guidance — proving the marker is no longer the active source. + await expect(getVertexAccessToken()).rejects.toThrow(/Application Default Credentials/); + }); +}); From 369f7f2002ceef1023c9bb3e50be863fd79a8714 Mon Sep 17 00:00:00 2001 From: GSL-R Date: Fri, 9 Oct 2026 22:22:10 +0900 Subject: [PATCH 02/19] refactor(vertex): pass the marker account per request; review fixes (#6842) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address CodeRabbit findings: - gcp-adc: remove the process-global activeMarkerAccount/registration (a module-level marker would race across concurrent requests for different providers and could resolve one call with another provider's credential). The marker account now travels per request via getVertexAccessToken(options.markerAccount) and flows through currentAdcSourceKey/loadAdcCredentials as an argument. The reconcile sweep passes no marker (no request context) — marker entries are pruned by expiry instead. - adapters/google: pass gcpCredentialMarkerAccount(provider.apiKey) per request. router: drop the registration; the routed provider config carries the marker verbatim. - api-keys: delete a just-written keychain entry ONLY when the write created it — on a failed commit, an upsert must keep the credential the previous reference still names. - tests: narrow the adapter assertion to the ADC-shaped request (URL + Bearer header + no x-goog-api-key, with a stubbed fetch), and isolate the marker-source tests from host ADC state (empty CLOUDSDK_CONFIG, no GOOGLE_APPLICATION_CREDENTIALS, 404 fetch stub). Co-Authored-By: Claude --- src/adapters/google.ts | 8 ++- src/lib/gcp-adc.ts | 43 +++++-------- src/providers/api-keys.ts | 19 +++++- src/router.ts | 11 ++-- tests/adapters/google/gcp-sa-adapter.test.ts | 67 +++++++++++++------- tests/providers/gcp-sa-marker.test.ts | 59 +++++++++++------ 6 files changed, 126 insertions(+), 81 deletions(-) diff --git a/src/adapters/google.ts b/src/adapters/google.ts index 612e49c3a87..7ec7e3eb357 100644 --- a/src/adapters/google.ts +++ b/src/adapters/google.ts @@ -17,7 +17,7 @@ import type { import { isAllowedToolChoice, namespacedToolName, resolveToolChoiceWireName, toolChoiceToolPredicate } from "../types"; import type { OcxTool } from "../types"; import { contentPartsToText, parseDataUrl } from "./image"; -import { GCP_CREDENTIAL_MARKER_PREFIX, getVertexAccessToken } from "../lib/gcp-adc"; +import { GCP_CREDENTIAL_MARKER_PREFIX, gcpCredentialMarkerAccount, getVertexAccessToken } from "../lib/gcp-adc"; import { fetchAntigravityWithRetry, fetchVertexWithRetry } from "./google-http"; import { safeAntigravityHttpErrorMessage, safeVertexHttpErrorMessage } from "./google-errors"; import { isVertexTruncatedTurn, vertexTruncationErrorMessage } from "./google-truncation"; @@ -1201,7 +1201,11 @@ export function createGoogleAdapter(provider: OcxProviderConfig): ProviderAdapte if (locationError) throw new Error(locationError); const host = location === "global" ? "aiplatform.googleapis.com" : `${location}-aiplatform.googleapis.com`; const url = `https://${host}/v1/projects/${project}/locations/${location}/publishers/google/models/${parsed.modelId}:${method}${streamParam}`; - const token = await getVertexAccessToken(); + // The marker account travels per-request as an argument (never process-global state): a + // module-level "active marker" written at routing time would race across concurrent + // requests for different providers and could resolve one call with another's credential. + const markerAccount = gcpCredentialMarkerAccount(provider.apiKey); + const token = await getVertexAccessToken({ markerAccount }); headers["Authorization"] = `Bearer ${token}`; return { url, method: "POST", headers, body: JSON.stringify(compiled.body) }; } diff --git a/src/lib/gcp-adc.ts b/src/lib/gcp-adc.ts index 2de0cb7b56f..c5f76335818 100644 --- a/src/lib/gcp-adc.ts +++ b/src/lib/gcp-adc.ts @@ -123,7 +123,10 @@ export function sweepExpiredGcpAdcTokens(now = Date.now()): number { export function reconcileGcpAdcTokens(context: GenerationContext): number { if (context.generation <= lastReconciledGeneration) return 0; - const currentSource = currentAdcSourceKey(); + // The periodic sweep registration has no request context, so no marker is active here: markers + // travel per-request via getVertexAccessToken(options.markerAccount), and a marker-token cache + // entry whose account no longer resolves is simply pruned by this sweep when it expires. + const currentSource = currentAdcSourceKey(undefined); let removed = 0; for (const source of tokenCache.keys()) { if (source === currentSource) continue; @@ -190,8 +193,7 @@ function readMarkerCredential(account: string): AdcFileCredentials | undefined { } } -function loadAdcCredentials(): { source: string; creds: AdcFileCredentials } | undefined { - const markerAccount = currentMarkerAccount(); +function loadAdcCredentials(markerAccount: string | undefined): { source: string; creds: AdcFileCredentials } | undefined { if (markerAccount) { const creds = readMarkerCredential(markerAccount); if (!creds) throw new Error(`gcp-sa:${markerAccount} marker is set but its credential could not be read from the OS credential store`); @@ -215,8 +217,7 @@ function loadAdcCredentials(): { source: string; creds: AdcFileCredentials } | u * change to GOOGLE_APPLICATION_CREDENTIALS (or the user ADC file) does not keep serving a stale * token from a different source. Falls back to "metadata" when no marker/file/env ADC is present. */ -function currentAdcSourceKey(): string { - const markerAccount = currentMarkerAccount(); +function currentAdcSourceKey(markerAccount: string | undefined): string { if (markerAccount) return `marker:${markerAccount}`; const gacPath = process.env.GOOGLE_APPLICATION_CREDENTIALS; if (gacPath) return fileSourceTag("gac", gacPath); @@ -225,23 +226,6 @@ function currentAdcSourceKey(): string { return "metadata"; } -/** - * The marker source needs to know WHICH marker is active, but gcp-adc is a leaf module that - * must not import the provider config graph. Callers that route through a `gcp-sa:` marker - * register the active marker account here (the vertex adapter path calls - * `setActiveGcpCredentialMarker` from the router's key resolution, which already knows the - * resolved key string). Exported for that single call site + tests. - */ -let activeMarkerAccount: string | undefined; - -export function setActiveGcpCredentialMarker(value: string | undefined): void { - activeMarkerAccount = gcpCredentialMarkerAccount(value); -} - -function currentMarkerAccount(): string | undefined { - return activeMarkerAccount; -} - function base64UrlEncode(bytes: Uint8Array | string): string { const buf = typeof bytes === "string" ? Buffer.from(bytes, "utf8") : Buffer.from(bytes); return buf.toString("base64url"); @@ -363,8 +347,8 @@ async function fetchMetadataToken(signal: AbortSignal | undefined, fetchImpl: Fe } } -async function resolveAccessTokenUncached(signal: AbortSignal | undefined, fetchImpl: FetchImpl): Promise<{ source: string; token: TokenResponse }> { - const adc = loadAdcCredentials(); +async function resolveAccessTokenUncached(markerAccount: string | undefined, signal: AbortSignal | undefined, fetchImpl: FetchImpl): Promise<{ source: string; token: TokenResponse }> { + const adc = loadAdcCredentials(markerAccount); if (adc) { const token = adc.creds.type === "service_account" ? await exchangeJwtForToken(adc.creds, signal, fetchImpl) @@ -379,14 +363,19 @@ async function resolveAccessTokenUncached(signal: AbortSignal | undefined, fetch } /** Returns a Bearer access token for the `Authorization` header on Vertex AI calls (cached + refreshed). */ -export async function getVertexAccessToken(options?: { signal?: AbortSignal; fetch?: FetchImpl }): Promise { +export async function getVertexAccessToken(options?: { signal?: AbortSignal; fetch?: FetchImpl; markerAccount?: string }): Promise { + // The marker travels as an argument, never as process-global state: a module-level "active + // marker" written by the router would race across concurrent requests for different providers + // and could resolve one provider's Vertex call with another provider's service account. The + // adapter passes `gcpCredentialMarkerAccount(provider.apiKey)` per request instead. + const markerAccount = options?.markerAccount; const fetchImpl = options?.fetch ?? globalThis.fetch.bind(globalThis); const skew = getRefreshSkewMs(); const now = Date.now(); // Only serve a cached token that matches the source the next resolve would actually use; prune // expired or now-stale (different-source) entries so a credential-source change is honored. - const expectedSource = currentAdcSourceKey(); + const expectedSource = currentAdcSourceKey(markerAccount); const writerGeneration = captureConfigGeneration(); for (const [source, cached] of tokenCache) { if (source === expectedSource && cached.expiresAtMs - skew > now) return cached.token; @@ -401,7 +390,7 @@ export async function getVertexAccessToken(options?: { signal?: AbortSignal; fet const promise = (async () => { try { - const { source, token } = await resolveAccessTokenUncached(options?.signal, fetchImpl); + const { source, token } = await resolveAccessTokenUncached(markerAccount, options?.signal, fetchImpl); const expiresAtMs = Date.now() + Math.max(0, token.expires_in * 1000); if (writerGeneration >= lastReconciledGeneration || liveSources.has(source)) { tokenCache.set(source, { token: token.access_token, expiresAtMs }); diff --git a/src/providers/api-keys.ts b/src/providers/api-keys.ts index a70eee95f58..c9a3e76bb47 100644 --- a/src/providers/api-keys.ts +++ b/src/providers/api-keys.ts @@ -85,7 +85,7 @@ function storeGcpCredentialJson(config: OcxConfig, name: string, credentialJson: const probe = probeProviderKeychain(); if (!probe.available) throw new Error(`OS keychain unavailable: ${probe.reason}`); const id = apiKeyPoolEntryId(credentialJson); - const account = `${name}/${id}`; + const account = gcpCredentialStoreAccount(name, id); const entry = providerKeychainEntry(account); entry.setPassword(credentialJson); if (entry.getPassword() !== credentialJson) { @@ -95,6 +95,11 @@ function storeGcpCredentialJson(config: OcxConfig, name: string, credentialJson: return `${GCP_CREDENTIAL_MARKER_PREFIX}${account}`; } +/** The credential-store account a marker names for `name`/`id` (write + cleanup share it). */ +function gcpCredentialStoreAccount(name: string, id: string): string { + return `${name}/${id}`; +} + /** Content-derived id: re-adding the same key upserts instead of duplicating. */ export function apiKeyPoolEntryId(key: string): string { return createHash("sha256").update(key).digest("hex").slice(0, 8); @@ -160,13 +165,17 @@ export function addProviderApiKey(config: OcxConfig, name: string, key: string, } else { if (parts.length > 1) return { error: "paste one credential JSON per request; add each additional credential as its own key" }; const credentialJson = (first as { credentialJson: string }).credentialJson; + const id = apiKeyPoolEntryId(credentialJson); + const account = gcpCredentialStoreAccount(name, id); + // Track whether this write CREATED the keychain entry: an upsert over an existing + // reference must not delete the previously-referenced credential on a failed commit. + const existedBefore = providerKeychainEntry(account).getPassword() !== null; let marker: string; try { marker = storeGcpCredentialJson(config, name, credentialJson); } catch (error) { return { error: error instanceof Error ? error.message : "credential storage failed" }; } - const id = apiKeyPoolEntryId(credentialJson); const committed = commitProviderApiKeySelection(config, name, fresh => { const pool = ensurePool(fresh); const existing = pool.find(e => e.id === id); @@ -183,6 +192,12 @@ export function addProviderApiKey(config: OcxConfig, name: string, key: string, invalidateResolvedProviderKeyCache(); return { id }; } + // Failed commit: the just-written keychain entry has no config reference pointing at it. + // Delete it ONLY when this write created it — an upsert must keep the credential the old + // reference still names. + if (!existedBefore) { + try { providerKeychainEntry(account).deletePassword(); } catch { /* best effort */ } + } return { error: "provider selection unavailable" }; } } catch (error) { diff --git a/src/router.ts b/src/router.ts index 4d57aa7fe6c..6a6572d56e0 100644 --- a/src/router.ts +++ b/src/router.ts @@ -15,7 +15,6 @@ import type { NormalizedComboConfig } from "./combos/types"; import { hasOwnProvider } from "./config/provider-name"; import { anthropicInstanceRowShapeMatches } from "./providers/anthropic-instance-id"; import { providerUsesKeyAuthOverride, resolveProviderApiKey } from "./providers/key-store"; -import { setActiveGcpCredentialMarker } from "./lib/gcp-adc"; import { captureProviderApiKeySelection } from "./providers/api-key-selection-capture"; import { assertProviderDestinationAllowed } from "./lib/destination-policy"; import { redactSecretString, redactUrlForLog } from "./lib/redact"; @@ -299,11 +298,11 @@ export function resetCompactionFallbackWarningsForTests(): void { } function usableResolvedApiKey(apiKey: string | undefined): string | undefined { - // A `gcp-sa:` marker carries no usable key material: the ADC resolver (gcp-adc) must learn it - // is the active credential source. Register before the early returns below — a marker resolves - // to `undefined` here (it is not a keychain/env ref, and the marker itself is not a literal), - // so the vertex adapter's ADC branch is what actually consumes the registration. - setActiveGcpCredentialMarker(apiKey); + // A `gcp-sa:` marker carries no usable key material: the ADC resolver reads it later, when the + // vertex adapter asks for a token. The marker travels on the routed provider config (a marker + // resolves to `undefined` here — it is not a keychain/env ref, and the marker itself is not a + // literal), so `routedProviderConfig` preserves it verbatim and the adapter passes + // `gcpCredentialMarkerAccount(provider.apiKey)` per request — no process-global state. const resolved = resolveProviderApiKey(apiKey); return typeof resolved === "string" && resolved.trim().length > 0 ? resolved : undefined; } diff --git a/tests/adapters/google/gcp-sa-adapter.test.ts b/tests/adapters/google/gcp-sa-adapter.test.ts index 0b8779905fe..e71a2270074 100644 --- a/tests/adapters/google/gcp-sa-adapter.test.ts +++ b/tests/adapters/google/gcp-sa-adapter.test.ts @@ -1,9 +1,6 @@ import { afterEach, describe, expect, test } from "bun:test"; -import { - GCP_CREDENTIAL_MARKER_PREFIX, - setActiveGcpCredentialMarker, - __resetVertexTokenCache, -} from "../../../src/lib/gcp-adc"; +import { Buffer } from "node:buffer"; +import { __resetVertexTokenCache } from "../../../src/lib/gcp-adc"; import { createGoogleAdapter } from "../../../src/adapters/google"; import { setProviderKeychainEntryFactoryForTests } from "../../../src/providers/key-store"; import type { OcxParsedRequest, OcxProviderConfig } from "../../../src/types"; @@ -19,32 +16,56 @@ function parsed(modelId = "gemini-3-pro"): OcxParsedRequest { afterEach(() => { __resetVertexTokenCache(); - setActiveGcpCredentialMarker(undefined); setProviderKeychainEntryFactoryForTests(null); }); describe("vertex adapter with a gcp-sa marker key", () => { - test("marker does not take the x-goog-api-key fast path — ADC branch runs", async () => { - const serviceAccount = JSON.stringify({ - type: "service_account", - client_email: "svc@example.test", - private_key: "not-a-real-key-for-this-test", - private_key_id: "k1", - }); - // The keychain entry exists (the real exchange would sign a real key); this test asserts the - // ROUTE: the marker must reach the ADC branch, not the API-key fast path. We can prove that - // without a valid key because the fast path returns a URL on aiplatform host WITHOUT a project - // while the ADC branch throws on signing before any request. + test("marker resolves through the ADC branch (never the x-goog-api-key fast path)", async () => { + // A real, signable RSA key so buildRequest reaches the actual signing step of the ADC branch. + const kp = await globalViewGenerateKey(); + const pkcs8 = await globalThis.crypto.subtle.exportKey("pkcs8", kp.privateKey); + const b64 = Buffer.from(pkcs8).toString("base64").match(/.{1,64}/g)!.join("\n"); + const pem = `-----BEGIN PRIVATE KEY-----\n${b64}\n-----END PRIVATE KEY-----\n`; + const serviceAccount = JSON.stringify({ type: "service_account", client_email: "svc@example.test", private_key: pem, private_key_id: "k1" }); + + // Stub fetch so the JWT exchange succeeds and NO network call carries x-goog-api-key. + const realFetch = globalThis.fetch; + globalThis.fetch = (async () => + new Response(JSON.stringify({ access_token: "tok", expires_in: 3600 }), { status: 200 })) as typeof fetch; + + // The marker in a keychain-backed store; the adapter passes its account to the ADC branch per + // request (gcpCredentialMarkerAccount(provider.apiKey)) — no process-global registration. setProviderKeychainEntryFactoryForTests((service, account) => ({ getPassword: () => (account === "p/mk1" ? serviceAccount : null), setPassword: () => {}, deletePassword: () => true, })); - setActiveGcpCredentialMarker(`${GCP_CREDENTIAL_MARKER_PREFIX}p/mk1`); - const provider = { adapter: "google", baseUrl: "https://x", googleMode: "vertex", apiKey: `${GCP_CREDENTIAL_MARKER_PREFIX}p/mk1`, project: "proj-1", location: "global" } as OcxProviderConfig; - // The ADC branch needs a valid RSA key to sign — it will throw on importKey, NOT send - // x-goog-api-key. Any throw (even a signing one) proves the fast path was skipped; an - // x-goog-api-key request would have returned successfully with the marker as the key. - await expect(createGoogleAdapter(provider).buildRequest(parsed())).rejects.toThrow(); + const provider = { + adapter: "google", + baseUrl: "https://x", + googleMode: "vertex", + apiKey: "gcp-sa:p/mk1", + project: "proj-1", + location: "global", + } as OcxProviderConfig; + + try { + const req = await createGoogleAdapter(provider).buildRequest(parsed()); + // The produced request must NOT be the API-key fast path (which would carry the marker + // itself as x-goog-api-key); it must be the ADC Bearer request. + expect(req.url).toContain("/v1/projects/proj-1/locations/global/publishers/google/models/"); + expect(req.headers["x-goog-api-key"]).toBeUndefined(); + expect((req.headers["Authorization"] ?? "").startsWith("Bearer tok")).toBe(true); + } finally { + globalThis.fetch = realFetch; + } }); }); + +async function globalViewGenerateKey(): Promise { + return globalThis.crypto.subtle.generateKey( + { name: "RSASSA-PKCS1-v1_5", modulusLength: 2048, publicExponent: new Uint8Array([1, 0, 1]), hash: "SHA-256" }, + true, + ["sign", "verify"], + ) as Promise; +} diff --git a/tests/providers/gcp-sa-marker.test.ts b/tests/providers/gcp-sa-marker.test.ts index fbd303f8e55..0c530505b2e 100644 --- a/tests/providers/gcp-sa-marker.test.ts +++ b/tests/providers/gcp-sa-marker.test.ts @@ -1,6 +1,6 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { Buffer } from "node:buffer"; -import { mkdtempSync } from "node:fs"; +import { mkdtempSync, mkdirSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -8,7 +8,6 @@ import { gcpCredentialMarkerAccount, getVertexAccessToken, parseGcpCredentialJson, - setActiveGcpCredentialMarker, __resetVertexTokenCache, } from "../../src/lib/gcp-adc"; import { splitCredentialPaste, addProviderApiKey } from "../../src/providers/api-keys"; @@ -32,7 +31,6 @@ const SERVICE_ACCOUNT_JSON_2 = JSON.stringify({ afterEach(() => { __resetVertexTokenCache(); - setActiveGcpCredentialMarker(undefined); setProviderKeychainEntryFactoryForTests(null); }); @@ -192,10 +190,38 @@ describe("addProviderApiKey with a pasted credential JSON", () => { describe("gcp-adc marker source", () => { let oauthCalls = 0; const realFetch = globalThis.fetch; + const prevEnv: Record = {}; - afterEach(() => { globalThis.fetch = realFetch; }); + afterEach(() => { + globalThis.fetch = realFetch; + for (const [k, v] of Object.entries(prevEnv)) { + if (v === undefined) delete process.env[k]; + else process.env[k] = v; + delete prevEnv[k]; + } + }); + + // The marker-account tests pass the account per request (options.markerAccount) — no + // process-global registration anywhere. Host ADC state is fully isolated: an empty + // CLOUDSDK_CONFIG dir, no GOOGLE_APPLICATION_CREDENTIALS, and a 404 fetching stub, so + // no test can make a real network call or depend on the developer's gcloud login. + function isolateHostAdc(): void { + prevEnv.GOOGLE_APPLICATION_CREDENTIALS ??= process.env.GOOGLE_APPLICATION_CREDENTIALS; + delete process.env.GOOGLE_APPLICATION_CREDENTIALS; + prevEnv.CLOUDSDK_CONFIG ??= process.env.CLOUDSDK_CONFIG; + process.env.CLOUDSDK_CONFIG = join(tmpdir(), `ocx-gcp-sa-isolated-${Date.now()}`); + mkdirSync(process.env.CLOUDSDK_CONFIG, { recursive: true }); + globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { + const url = typeof input === "string" ? input : input instanceof URL ? input.href : (input as Request).url; + if (url === "https://oauth2.googleapis.com/token") { + oauthCalls++; + return new Response(JSON.stringify({ access_token: "marker-tok", expires_in: 3600 }), { status: 200 }); + } + return new Response("nope", { status: 404 }); + }) as typeof fetch; + } - test("an active marker resolves its keychain credential through the JWT exchange", async () => { + test("a marker account resolves its keychain credential through the JWT exchange", async () => { // A real, signable RSA key (generated) so the RS256 JWT exchange inside the resolver works. const kp = await globalThis.crypto.subtle.generateKey( { name: "RSASSA-PKCS1-v1_5", modulusLength: 2048, publicExponent: new Uint8Array([1, 0, 1]), hash: "SHA-256" }, @@ -216,17 +242,9 @@ describe("gcp-adc marker source", () => { setPassword: () => {}, deletePassword: () => true, })); - globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { - const url = typeof input === "string" ? input : input instanceof URL ? input.href : (input as Request).url; - if (url === "https://oauth2.googleapis.com/token") { - oauthCalls++; - return new Response(JSON.stringify({ access_token: "marker-tok", expires_in: 3600 }), { status: 200 }); - } - return new Response("nope", { status: 404 }); - }) as typeof fetch; + isolateHostAdc(); - setActiveGcpCredentialMarker(`${GCP_CREDENTIAL_MARKER_PREFIX}p/abc123`); - const token = await getVertexAccessToken(); + const token = await getVertexAccessToken({ markerAccount: "p/abc123" }); expect(token).toBe("marker-tok"); expect(oauthCalls).toBe(1); }); @@ -237,15 +255,14 @@ describe("gcp-adc marker source", () => { setPassword: () => {}, deletePassword: () => true, })); - setActiveGcpCredentialMarker(`${GCP_CREDENTIAL_MARKER_PREFIX}p/missing`); - await expect(getVertexAccessToken()).rejects.toThrow(/gcp-sa:p\/missing/); + isolateHostAdc(); + await expect(getVertexAccessToken({ markerAccount: "p/missing" })).rejects.toThrow(/gcp-sa:p\/missing/); }); - test("clearing the marker restores the normal source priority", async () => { - setActiveGcpCredentialMarker(`${GCP_CREDENTIAL_MARKER_PREFIX}p/abc123`); - setActiveGcpCredentialMarker(undefined); + test("no marker account falls through to the normal source priority (isolated: fails with ADC guidance)", async () => { + isolateHostAdc(); // With no marker and no env/file ADC, the resolver reaches the metadata-server attempt and - // fails with the standard ADC guidance — proving the marker is no longer the active source. + // fails with the standard ADC guidance — proving the no-marker path is unchanged. await expect(getVertexAccessToken()).rejects.toThrow(/Application Default Credentials/); }); }); From 12a0adc1b6f6ee810c4e8bc7dbcacb97188824a5 Mon Sep 17 00:00:00 2001 From: GSL-R Date: Fri, 9 Oct 2026 23:19:24 +0900 Subject: [PATCH 03/19] fix(vertex): preserve the marker through routing; harden paste handling (#6842) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address CodeRabbit round-2 findings: - router: usableResolvedApiKey returns gcp-sa: marker values UNCHANGED — replacing a marker with undefined (it resolves to no key material) would silently drop it from the routed config and send the vertex adapter to host ADC instead of the configured service account. - api-keys: storeGcpCredentialJson takes existedBefore and deletes the keychain entry after a read-back mismatch only when the entry did not exist before the write — a re-paste targets an account an existing marker already names, and deleting it would destroy the live credential. - api-keys: splitCredentialPaste honors the parseGcpCredentialJson contract (a single-line brace-leading literal is never eaten — it falls back to a plain key row), tracks JSON string/escape state in the brace scanner (a } inside a string field no longer splits early), and rejects non-separator content before/between objects instead of discarding it. - tests: router-level pass-through test (routedProviderConfig preserves the marker), end-to-end test (routed config feeds buildRequest, ADC branch wins), literal-key fallback + gap-rejection tests. Markers keep resolveProviderApiKey synchronous. Co-Authored-By: Claude --- src/providers/api-keys.ts | 60 +++++++++- src/router.ts | 12 +- tests/adapters/google/gcp-sa-adapter.test.ts | 112 ++++++++++++++++++- tests/providers/gcp-sa-marker.test.ts | 17 ++- 4 files changed, 184 insertions(+), 17 deletions(-) diff --git a/src/providers/api-keys.ts b/src/providers/api-keys.ts index c9a3e76bb47..fe88712b6e1 100644 --- a/src/providers/api-keys.ts +++ b/src/providers/api-keys.ts @@ -40,6 +40,13 @@ export function maskApiKey(value: string): string { * credential JSON (or several, comma/space-separated — the same convention as newline-separated * API keys) is one row per JSON object. Windows file paths are rejected with guidance rather * than stored, because they are the most common paste mistake and silently break at runtime. + * + * Honors the parseGcpCredentialJson contract "a literal key that starts with `{` is never eaten": + * a brace-leading value that is NOT recognizable credential JSON falls back to a plain literal + * row (preserving the old single-line behavior), and only a multi-line or multi-object paste that + * fails credential validation rejects with guidance. The brace scanner tracks JSON string and + * escape state so a `}` inside a string field does not split the object early; any non-separator + * text between objects is rejected rather than silently discarded. */ export function splitCredentialPaste(value: string): Array { const trimmed = (value ?? "").trim(); @@ -51,24 +58,62 @@ export function splitCredentialPaste(value: string): Array = []; const objects: string[] = []; let depth = 0; let start = -1; + let inString = false; + let escaped = false; for (let i = 0; i < trimmed.length; i++) { const ch = trimmed[i]; - if (ch === "{" && depth === 0) start = i; + if (inString) { + if (escaped) escaped = false; + else if (ch === "\\") escaped = true; + else if (ch === '"') inString = false; + continue; + } + if (ch === '"') { inString = true; continue; } + if (ch === "{" && depth === 0) { + start = i; + } if (ch === "{") depth += 1; if (ch === "}") { depth -= 1; if (depth === 0 && start >= 0) { objects.push(trimmed.slice(start, i + 1)); + spans.push({ start, end: i }); start = -1; } if (depth < 0) throw new Error("Invalid credential JSON: unbalanced braces."); } } if (depth !== 0 || start >= 0) throw new Error("Invalid credential JSON: the pasted text ends inside a JSON object."); - return objects.map(json => { + // Single unrecognized object on a single line = a plain key starting with `{` (never eaten). + // Multi-line or multi-object pastes that fail validation get the guidance error instead. + const singleLine = !/[\r\n]/.test(trimmed); + if (objects.length === 1 && singleLine && spans[0]!.end === trimmed.length - 1 && !parseGcpCredentialJson(objects[0]!)) { + return [trimmed]; + } + // Non-separator content BETWEEN objects is rejected, not silently discarded: everything + // between one object's close and the next object's open (and before the first) must be a + // separator character. + for (let i = 0; i < spans.length; i++) { + if (i === 0) { + if (!/^[\s,]*$/.test(trimmed.slice(0, spans[0]!.start))) { + throw new Error("Unrecognized credential JSON: unexpected content before the first JSON object. Paste credential JSON(s) separated by commas or newlines, or a plain API key."); + } + continue; + } + const gap = trimmed.slice(spans[i - 1]!.end + 1, spans[i]!.start); + if (!/^[\s,]*$/.test(gap)) { + throw new Error("Unrecognized credential JSON: unexpected content between JSON objects. Paste credential JSON(s) separated by commas or newlines, or a plain API key."); + } + } + return objects.map((json) => { if (!parseGcpCredentialJson(json)) { throw new Error("Unrecognized credential JSON: expected a GCP Service Account or authorized_user JSON with a \"type\" field."); } @@ -81,7 +126,7 @@ export function splitCredentialPaste(value: string): Array 0 ? resolved : undefined; } diff --git a/tests/adapters/google/gcp-sa-adapter.test.ts b/tests/adapters/google/gcp-sa-adapter.test.ts index e71a2270074..d4b7840a846 100644 --- a/tests/adapters/google/gcp-sa-adapter.test.ts +++ b/tests/adapters/google/gcp-sa-adapter.test.ts @@ -1,9 +1,15 @@ -import { afterEach, describe, expect, test } from "bun:test"; +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { Buffer } from "node:buffer"; -import { __resetVertexTokenCache } from "../../../src/lib/gcp-adc"; +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { __resetVertexTokenCache, gcpCredentialMarkerAccount } from "../../../src/lib/gcp-adc"; import { createGoogleAdapter } from "../../../src/adapters/google"; +import { routedProviderConfig } from "../../../src/router"; import { setProviderKeychainEntryFactoryForTests } from "../../../src/providers/key-store"; -import type { OcxParsedRequest, OcxProviderConfig } from "../../../src/types"; +import { saveConfig } from "../../../src/config"; +import type { OcxConfig, OcxParsedRequest, OcxProviderConfig } from "../../../src/types"; +import { removeTreeWithRetry } from "../../helpers/remove-tree"; function parsed(modelId = "gemini-3-pro"): OcxParsedRequest { return { @@ -22,7 +28,7 @@ afterEach(() => { describe("vertex adapter with a gcp-sa marker key", () => { test("marker resolves through the ADC branch (never the x-goog-api-key fast path)", async () => { // A real, signable RSA key so buildRequest reaches the actual signing step of the ADC branch. - const kp = await globalViewGenerateKey(); + const kp = await generateKeyPair(); const pkcs8 = await globalThis.crypto.subtle.exportKey("pkcs8", kp.privateKey); const b64 = Buffer.from(pkcs8).toString("base64").match(/.{1,64}/g)!.join("\n"); const pem = `-----BEGIN PRIVATE KEY-----\n${b64}\n-----END PRIVATE KEY-----\n`; @@ -62,7 +68,103 @@ describe("vertex adapter with a gcp-sa marker key", () => { }); }); -async function globalViewGenerateKey(): Promise { +describe("router preserves the gcp-sa marker", () => { + let home: string; + const previousHome = process.env.OPENCODEX_HOME; + + // routedProviderConfig reads the committed config through mutatePersistedConfig-dependent + // helpers, so tests run against a throwaway OPENCODEX_HOME with a real config file. + beforeEach(() => { + home = mkdtempSync(join(tmpdir(), "ocx-gcp-sa-routing-")); + process.env.OPENCODEX_HOME = home; + saveConfig({ + port: 10199, + defaultProvider: "google-vertex", + providers: { + "google-vertex": { adapter: "google", baseUrl: "https://aiplatform.googleapis.com", googleMode: "vertex" } as OcxProviderConfig, + }, + } as unknown as OcxConfig); + }); + + afterEach(() => { + if (previousHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousHome; + removeTreeWithRetry(home); + }); + + test("routedProviderConfig preserves a gcp-sa marker in apiKey (usableResolvedApiKey pass-through)", () => { + const provider = { adapter: "google", baseUrl: "https://aiplatform.googleapis.com", googleMode: "vertex", apiKey: "gcp-sa:p/abcd1234" } as OcxProviderConfig; + const routed = routedProviderConfig("google-vertex", provider); + expect(routed.apiKey).toBe("gcp-sa:p/abcd1234"); + expect(gcpCredentialMarkerAccount(routed.apiKey)).toBe("p/abcd1234"); + }); + + test("a non-marker key still resolves through the normal path (no regression)", () => { + const provider = { adapter: "google", baseUrl: "https://aiplatform.googleapis.com", googleMode: "vertex", apiKey: "${OCX_ROUTER_TEST_KEY}" } as OcxProviderConfig; + process.env.OCX_ROUTER_TEST_KEY = "resolved-literal"; + try { + const routed = routedProviderConfig("google-vertex", provider); + expect(routed.apiKey).toBe("resolved-literal"); + } finally { + delete process.env.OCX_ROUTER_TEST_KEY; + } + }); +}); + +describe("routed marker reaches the ADC request path (router + adapter, end to end)", () => { + let home: string; + const previousHome = process.env.OPENCODEX_HOME; + + beforeEach(() => { + home = mkdtempSync(join(tmpdir(), "ocx-gcp-sa-e2e-")); + process.env.OPENCODEX_HOME = home; + saveConfig({ + port: 10199, + defaultProvider: "google-vertex", + providers: { + "google-vertex": { adapter: "google", baseUrl: "https://aiplatform.googleapis.com", googleMode: "vertex" } as OcxProviderConfig, + }, + } as unknown as OcxConfig); + }); + + afterEach(() => { + if (previousHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousHome; + removeTreeWithRetry(home); + __resetVertexTokenCache(); + setProviderKeychainEntryFactoryForTests(null); + }); + + test("routedProviderConfig output feeds buildRequest and the ADC branch wins", async () => { + const kp = await generateKeyPair(); + const pkcs8 = await globalThis.crypto.subtle.exportKey("pkcs8", kp.privateKey); + const b64 = Buffer.from(pkcs8).toString("base64").match(/.{1,64}/g)!.join("\n"); + const pem = `-----BEGIN PRIVATE KEY-----\n${b64}\n-----END PRIVATE KEY-----\n`; + const serviceAccount = JSON.stringify({ type: "service_account", client_email: "svc@example.test", private_key: pem, private_key_id: "k1" }); + + const realFetch = globalThis.fetch; + globalThis.fetch = (async () => + new Response(JSON.stringify({ access_token: "routed-tok", expires_in: 3600 }), { status: 200 })) as typeof fetch; + setProviderKeychainEntryFactoryForTests((service, account) => ({ + getPassword: () => (account === "p/mk1" ? serviceAccount : null), + setPassword: () => {}, + deletePassword: () => true, + })); + + try { + const provider = { adapter: "google", baseUrl: "https://aiplatform.googleapis.com", googleMode: "vertex", apiKey: "gcp-sa:p/mk1", project: "proj-1", location: "global" } as OcxProviderConfig; + const routed = routedProviderConfig("google-vertex", provider); + const req = await createGoogleAdapter(routed).buildRequest(parsed()); + expect(req.url).toContain("/v1/projects/proj-1/locations/global/publishers/google/models/"); + expect(req.headers["x-goog-api-key"]).toBeUndefined(); + expect((req.headers["Authorization"] ?? "").startsWith("Bearer routed-tok")).toBe(true); + } finally { + globalThis.fetch = realFetch; + } + }); +}); + +async function generateKeyPair(): Promise { return globalThis.crypto.subtle.generateKey( { name: "RSASSA-PKCS1-v1_5", modulusLength: 2048, publicExponent: new Uint8Array([1, 0, 1]), hash: "SHA-256" }, true, diff --git a/tests/providers/gcp-sa-marker.test.ts b/tests/providers/gcp-sa-marker.test.ts index 0c530505b2e..543ae4386ab 100644 --- a/tests/providers/gcp-sa-marker.test.ts +++ b/tests/providers/gcp-sa-marker.test.ts @@ -91,8 +91,21 @@ describe("splitCredentialPaste", () => { expect(() => splitCredentialPaste("C:\\Users\\me\\sa.json")).toThrow(/file path/i); }); - test("JSON without a recognized type field is rejected", () => { - expect(() => splitCredentialPaste('{"type": "external_account"}')).toThrow(/type/i); + test("a single-line brace-leading NON-credential literal falls back to a plain key (never eaten)", () => { + // parseGcpCredentialJson contract: a literal API key starting with `{` is never eaten. + expect(splitCredentialPaste('{"type": "external_account"}')).toEqual(['{"type": "external_account"}']); + expect(splitCredentialPaste("{abc:def}")).toEqual(["{abc:def}"]); + }); + + test("a MULTILINE JSON without a recognized type field is rejected with guidance", () => { + // Multi-line braces are not a plausible literal key — guidance error. + const multiline = JSON.stringify({ type: "external_account", token_url: "https://sts.example" }, null, 2); + expect(() => splitCredentialPaste(multiline)).toThrow(/type/i); + }); + + test("text between JSON objects is rejected, not silently discarded", () => { + const sa1 = JSON.parse(SERVICE_ACCOUNT_JSON); + expect(() => splitCredentialPaste(`${JSON.stringify(sa1)} garbage ${JSON.stringify(sa1)}`)).toThrow(/unexpected content|Paste credential/i); }); test("truncated JSON is rejected", () => { From 857ed8cea70d833063c245d46aad804d99cfc884 Mon Sep 17 00:00:00 2001 From: GSL-R Date: Sat, 10 Oct 2026 16:11:00 +0900 Subject: [PATCH 04/19] docs: attest review readiness checklist (4/4 ticked) From 8c025fd295c467c1622e7e2e27a370303f0ec118 Mon Sep 17 00:00:00 2001 From: GSL-R Date: Sat, 10 Oct 2026 17:14:43 +0900 Subject: [PATCH 05/19] docs: attest readiness 4/4; trigger gate re-evaluation From 0f0ef85821e20e33df51e4e29d551b98a4c8fd7d Mon Sep 17 00:00:00 2001 From: GSL-R Date: Sat, 10 Oct 2026 17:45:23 +0900 Subject: [PATCH 06/19] fix(gcp-adc): suppress the GOOGLE_CLOUD_API_KEY fallback for named markers; strict paste validation (#6842) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Prana review findings: - adapters/google: a `gcp-sa:` marker means the operator named ONE credential explicitly — the GOOGLE_CLOUD_API_KEY env fallback no longer fires. Without this guard, a marker + env key would silently route the provider's inference through the ambient env key instead of the configured service account. - providers/api-keys (capture): a parseable JSON object with an unrecognized "type" field (e.g. external_account) is REJECTED with guidance, not saved as a plaintext literal key — saving it would send it to the wire verbatim. Non-JSON brace values ({abc:def}) keep their key-compat behavior. - providers/api-keys (tail): content after the LAST object's close up to EOF is validated like the mid-gaps — trailing garbage is rejected, not silently kept. (Covers captureTrailing otherwise.) Registrations: per-finding regression tests added (explicit-credential-wins for markers, JSON-with-unknown-type rejection, tail-gap rejection). Co-Authored-By: Claude --- src/adapters/google.ts | 6 ++- src/providers/api-keys.ts | 22 +++++--- tests/adapters/google/gcp-sa-adapter.test.ts | 57 ++++++++++++++++++++ tests/providers/gcp-sa-marker.test.ts | 42 +++++++++++++-- 4 files changed, 117 insertions(+), 10 deletions(-) diff --git a/src/adapters/google.ts b/src/adapters/google.ts index 7ec7e3eb357..2ae2e8f8e92 100644 --- a/src/adapters/google.ts +++ b/src/adapters/google.ts @@ -176,7 +176,11 @@ function resolveDirectGeminiWireModelId(modelId: string, applyRenames: boolean): function resolveVertexApiKey(optKey?: string): string | undefined { // A `gcp-sa:` marker is the keychain-backed credential reference (see gcp-adc): it is not // API-key material, so it must not be sent as x-goog-api-key — the ADC branch below handles it. - const realKey = optKey && !optKey.startsWith("<") && optKey !== "N/A" && !optKey.startsWith(GCP_CREDENTIAL_MARKER_PREFIX) + // AND the marker means the operator named one credential explicitly: the GOOGLE_CLOUD_API_KEY + // env fallback must NOT fire (it would silently route this provider's inference through the + // ambient env key instead of the configured service account). + if (optKey && optKey.startsWith(GCP_CREDENTIAL_MARKER_PREFIX)) return undefined; + const realKey = optKey && !optKey.startsWith("<") && optKey !== "N/A" ? optKey : undefined; return realKey || process.env.GOOGLE_CLOUD_API_KEY; diff --git a/src/providers/api-keys.ts b/src/providers/api-keys.ts index fe88712b6e1..b6d8c236134 100644 --- a/src/providers/api-keys.ts +++ b/src/providers/api-keys.ts @@ -92,15 +92,19 @@ export function splitCredentialPaste(value: string): Array= 0) throw new Error("Invalid credential JSON: the pasted text ends inside a JSON object."); - // Single unrecognized object on a single line = a plain key starting with `{` (never eaten). - // Multi-line or multi-object pastes that fail validation get the guidance error instead. + // Single unrecognized object on a single line = a plain key starting with `{` (never eaten) — + // BUT only when it is NOT real JSON: a parseable JSON object with an unrecognized `type` + // (e.g. `external_account`) is a credential the user believes is valid, and saving it as a + // plaintext literal key would send it to the wire. Distinguish by parse success, not shape. const singleLine = !/[\r\n]/.test(trimmed); - if (objects.length === 1 && singleLine && spans[0]!.end === trimmed.length - 1 && !parseGcpCredentialJson(objects[0]!)) { + const isRealJson = (() => { + try { JSON.parse(objects[0]!); return true; } catch { return false; } + })(); + if (objects.length === 1 && singleLine && spans[0]!.end === trimmed.length - 1 && !parseGcpCredentialJson(objects[0]!) && !isRealJson) { return [trimmed]; } - // Non-separator content BETWEEN objects is rejected, not silently discarded: everything - // between one object's close and the next object's open (and before the first) must be a - // separator character. + // Non-separator content BETWEEN and AFTER objects is rejected, not silently discarded: + // everything between/after one object's close must be a separator character. for (let i = 0; i < spans.length; i++) { if (i === 0) { if (!/^[\s,]*$/.test(trimmed.slice(0, spans[0]!.start))) { @@ -113,6 +117,12 @@ export function splitCredentialPaste(value: string): Array garbage`) is rejected rather than silently kept. + const tailGap = trimmed.slice(spans[spans.length - 1]!.end + 1); + if (!/^[\s,]*$/.test(tailGap)) { + throw new Error("Unrecognized credential JSON: unexpected content after the last JSON object. Paste credential JSON(s) separated by commas or newlines, or a plain API key."); + } return objects.map((json) => { if (!parseGcpCredentialJson(json)) { throw new Error("Unrecognized credential JSON: expected a GCP Service Account or authorized_user JSON with a \"type\" field."); diff --git a/tests/adapters/google/gcp-sa-adapter.test.ts b/tests/adapters/google/gcp-sa-adapter.test.ts index d4b7840a846..29265adb52c 100644 --- a/tests/adapters/google/gcp-sa-adapter.test.ts +++ b/tests/adapters/google/gcp-sa-adapter.test.ts @@ -162,6 +162,63 @@ describe("routed marker reaches the ADC request path (router + adapter, end to e globalThis.fetch = realFetch; } }); + + test("marker suppresses the GOOGLE_CLOUD_API_KEY env fallback (explicit credential wins)", async () => { + // A marker names ONE credential explicitly — the ambient env fallback must not silently + // route this provider's inference through the env key instead of the named service account. + // Without this guard, a marker + env key would send the ENV key as x-goog-api-key (review + // finding: fallback fires after the marker skips the fast path). + const prevEnvKey = process.env.GOOGLE_CLOUD_API_KEY; + process.env.GOOGLE_CLOUD_API_KEY = "ambient-env-key-should-not-be-used"; + const realFetch = globalThis.fetch; + globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { + const url = typeof input === "string" ? input : input instanceof URL ? input.href : (input as Request).url; + if (url === "https://oauth2.googleapis.com/token") { + return new Response(JSON.stringify({ access_token: "env-fallback-tok", expires_in: 3600 }), { status: 200 }); + } + return new Response("nope", { status: 404 }); + }) as typeof fetch; + const sawEnvKey = { value: false }; + const spyFetch = globalThis.fetch; + globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { + const url = typeof input === "string" ? input : input instanceof URL ? input.href : (input as Request).url; + if (url.includes("oauth2")) { + return new Response(JSON.stringify({ access_token: "adc-tok", expires_in: 3600 }), { status: 200 }); + } + const headers = (init?.headers ?? {}) as Record; + if (headers["x-goog-api-key"] === "ambient-env-key-should-not-be-used") sawEnvKey.value = true; + return new Response("nope", { status: 404 }); + }) as typeof fetch; + + try { + // Keychain serves the marker credential; env key is set. The adapter must use ADC + // (marker-named credential), never the env key. + const kp = await globalThis.crypto.subtle.generateKey( + { name: "RSASSA-PKCS1-v1_5", modulusLength: 2048, publicExponent: new Uint8Array([1, 0, 1]), hash: "SHA-256" }, + true, ["sign", "verify"], + ); + const pkcs8 = await globalThis.crypto.subtle.exportKey("pkcs8", kp.privateKey); + const b64 = Buffer.from(pkcs8).toString("base64").match(/.{1,64}/g)!.join("\n"); + const pem = `-----BEGIN PRIVATE KEY-----\n${b64}\n-----END PRIVATE KEY-----\n`; + const serviceAccount = JSON.stringify({ type: "service_account", client_email: "svc@example.test", private_key: pem, private_key_id: "k1" }); + setProviderKeychainEntryFactoryForTests((service, account) => ({ + getPassword: () => (account === "p/mk1" ? serviceAccount : null), + setPassword: () => {}, + deletePassword: () => true, + })); + + const provider = { adapter: "google", baseUrl: "https://x", googleMode: "vertex", apiKey: "gcp-sa:p/mk1", project: "proj-1", location: "global" } as OcxProviderConfig; + const req = await createGoogleAdapter(provider).buildRequest(parsed()); + // ADC branch: Bearer + no env key on the fast path + expect(req.headers["x-goog-api-key"]).toBeUndefined(); + expect((req.headers["Authorization"] ?? "").startsWith("Bearer adc-tok")).toBe(true); + expect(sawEnvKey.value).toBe(false); + } finally { + globalThis.fetch = realFetch; + if (prevEnvKey === undefined) delete process.env.GOOGLE_CLOUD_API_KEY; + else process.env.GOOGLE_CLOUD_API_KEY = prevEnvKey; + } + }); }); async function generateKeyPair(): Promise { diff --git a/tests/providers/gcp-sa-marker.test.ts b/tests/providers/gcp-sa-marker.test.ts index 543ae4386ab..0e33bc4a9ca 100644 --- a/tests/providers/gcp-sa-marker.test.ts +++ b/tests/providers/gcp-sa-marker.test.ts @@ -91,12 +91,19 @@ describe("splitCredentialPaste", () => { expect(() => splitCredentialPaste("C:\\Users\\me\\sa.json")).toThrow(/file path/i); }); - test("a single-line brace-leading NON-credential literal falls back to a plain key (never eaten)", () => { - // parseGcpCredentialJson contract: a literal API key starting with `{` is never eaten. - expect(splitCredentialPaste('{"type": "external_account"}')).toEqual(['{"type": "external_account"}']); + test("a single-line brace-leading NON-JSON literal falls back to a plain key (never eaten)", () => { + // parseGcpCredentialJson contract: a literal key starting with `{` is never eaten — + // but only for NON-JSON values. Prana review: a parseable JSON with an unrecognized type + // must be rejected, not saved as plaintext (it would go to the wire as a key). expect(splitCredentialPaste("{abc:def}")).toEqual(["{abc:def}"]); }); + test("a single-line PARSEABLE JSON with an unrecognized type is REJECTED (prana finding 2)", () => { + // `external_account` is a real JSON object — saving it as a plaintext literal key would + // send it verbatim to the wire. Reject with guidance instead. + expect(() => splitCredentialPaste('{"type": "external_account"}')).toThrow(); + }); + test("a MULTILINE JSON without a recognized type field is rejected with guidance", () => { // Multi-line braces are not a plausible literal key — guidance error. const multiline = JSON.stringify({ type: "external_account", token_url: "https://sts.example" }, null, 2); @@ -278,4 +285,33 @@ describe("gcp-adc marker source", () => { // fails with the standard ADC guidance — proving the no-marker path is unchanged. await expect(getVertexAccessToken()).rejects.toThrow(/Application Default Credentials/); }); + + test("a marker does NOT fall back to GOOGLE_CLOUD_API_KEY when ADC resolution succeeds via keychain", async () => { + // Prana review: with a marker set, the vertex adapter must suppress the env fast path — + // the keychain credential wins over the ambient env key (request-per-request ownership). + const kp = await globalThis.crypto.subtle.generateKey( + { name: "RSASSA-PKCS1-v1_5", modulusLength: 2048, publicExponent: new Uint8Array([1, 0, 1]), hash: "SHA-256" }, + true, + ["sign", "verify"], + ); + const pkcs8 = await globalThis.crypto.subtle.exportKey("pkcs8", kp.privateKey); + const b64 = Buffer.from(pkcs8).toString("base64").match(/.{1,64}/g)!.join("\n"); + const pem = `-----BEGIN PRIVATE KEY-----\n${b64}\n-----END PRIVATE KEY-----\n`; + const serviceAccount = JSON.stringify({ + type: "service_account", + client_email: "svc@example.test", + private_key: pem, + private_key_id: "k1", + }); + setProviderKeychainEntryFactoryForTests((service, account) => ({ + getPassword: () => (account === "p/own" ? serviceAccount : null), + setPassword: () => {}, + deletePassword: () => true, + })); + isolateHostAdc(); + oauthCalls = 0; // this describe's counter is shared across tests — reset for this probe + const tok = await getVertexAccessToken({ markerAccount: "p/own" }); + expect(tok).toBe("marker-tok"); + expect(oauthCalls).toBe(1); + }); }); From f87521b4da7ae7be5d3fce40d4d609471bad4750 Mon Sep 17 00:00:00 2001 From: GSL-R Date: Sat, 10 Oct 2026 18:02:47 +0900 Subject: [PATCH 07/19] test(vertex): cover trailing credential paste validation --- tests/providers/gcp-sa-marker.test.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/providers/gcp-sa-marker.test.ts b/tests/providers/gcp-sa-marker.test.ts index 0e33bc4a9ca..eeb268fd107 100644 --- a/tests/providers/gcp-sa-marker.test.ts +++ b/tests/providers/gcp-sa-marker.test.ts @@ -115,6 +115,16 @@ describe("splitCredentialPaste", () => { expect(() => splitCredentialPaste(`${JSON.stringify(sa1)} garbage ${JSON.stringify(sa1)}`)).toThrow(/unexpected content|Paste credential/i); }); + test("text after the last credential JSON is rejected", () => { + for (const input of [SERVICE_ACCOUNT_JSON, `${SERVICE_ACCOUNT_JSON}, ${SERVICE_ACCOUNT_JSON_2}`]) { + expect(() => splitCredentialPaste(`${input} trailing-garbage`)).toThrow(/after the last JSON object/i); + } + }); + + test("separators after the last credential JSON remain valid", () => { + expect(splitCredentialPaste(`${SERVICE_ACCOUNT_JSON}, \n\t`)).toEqual([{ credentialJson: SERVICE_ACCOUNT_JSON }]); + }); + test("truncated JSON is rejected", () => { expect(() => splitCredentialPaste('{"type": "service_account", "client_email"')).toThrow(); }); From c3defe84c5c6a83535c373dd1ac23585dde06276 Mon Sep 17 00:00:00 2001 From: GSL-R Date: Sat, 10 Oct 2026 18:37:42 +0900 Subject: [PATCH 08/19] fix(gcp-adc): validate markers and clean up credential lifecycle --- .../docs/reference/configuration/providers.md | 9 ++ src/cli/provider.ts | 4 +- src/lib/gcp-adc.ts | 10 +- src/providers/api-keys.ts | 75 +++++---- src/providers/gcp-credential-cleanup.ts | 31 ++++ src/server/management/provider-routes.ts | 3 + structure/cli-management.md | 3 + structure/config.md | 2 +- structure/gui-and-management-api.md | 2 +- structure/providers-and-adapters.md | 2 +- structure/providers/google.md | 9 ++ tests/adapters/google/gcp-sa-adapter.test.ts | 19 +++ tests/providers/gcp-sa-marker.test.ts | 153 +++++++++++++++++- 13 files changed, 270 insertions(+), 52 deletions(-) create mode 100644 src/providers/gcp-credential-cleanup.ts diff --git a/docs-site/src/content/docs/reference/configuration/providers.md b/docs-site/src/content/docs/reference/configuration/providers.md index 929c568e852..3e0425620df 100644 --- a/docs-site/src/content/docs/reference/configuration/providers.md +++ b/docs-site/src/content/docs/reference/configuration/providers.md @@ -1412,6 +1412,15 @@ model key. ## Storing keys in the OS keychain +For Vertex, the API-key input also accepts complete GCP `service_account` or `authorized_user` +credential JSON. Paste the JSON contents, not a Windows, Unix, or relative file path. Multiple +JSON objects separated by commas or whitespace are stored together as separate pool entries; +the first becomes active. Configuration contains only `gcp-sa:` references, never the +credential JSON. Empty marker accounts are invalid and do not fall back to host credentials. +Removing a key or provider deletes its unreferenced GCP credentials after the configuration is +saved; a credential referenced by another provider is retained. If OS-store cleanup fails, a +warning requests manual inspection without undoing the committed configuration removal. + By default a provider's `apiKey` and `apiKeyPool` sit in `config.json` (mode 0600, atomic writes). If you would rather keep the key material out of the file, move it into the OS credential store: diff --git a/src/cli/provider.ts b/src/cli/provider.ts index da3902f76d1..9b47d221621 100644 --- a/src/cli/provider.ts +++ b/src/cli/provider.ts @@ -15,6 +15,7 @@ import { getProviderRegistryEntry, PROVIDER_REGISTRY } from "../providers/regist import { providerConfigSeed } from "../providers/derive"; import { assertAnthropicInstanceLoginConfig } from "../oauth/store-anthropic-instance"; import { dropProviderCustomModels } from "../providers/provider-id-rewrite"; +import { cleanupRemovedGcpCredentials, providerGcpCredentialMarkers } from "../providers/gcp-credential-cleanup"; import type { OcxProviderConfig } from "../types"; import { findLiveProxy } from "../server/proxy-liveness"; import { syncModelsToCodex } from "../codex/sync"; @@ -440,10 +441,11 @@ function handleRemove(args: string[]): void { process.exit(1); } + const removedCredentials = providerGcpCredentialMarkers(config.providers[name]!); delete config.providers[name]; const droppedCustomModels = dropProviderCustomModels(config, name); validateAndSave(config); - + cleanupRemovedGcpCredentials(removedCredentials); if (wantsJson) { console.log(JSON.stringify({ diff --git a/src/lib/gcp-adc.ts b/src/lib/gcp-adc.ts index c5f76335818..481fc035717 100644 --- a/src/lib/gcp-adc.ts +++ b/src/lib/gcp-adc.ts @@ -50,12 +50,12 @@ const TOKEN_RETRY_BASE_MS = 300; */ export const GCP_CREDENTIAL_MARKER_PREFIX = "gcp-sa:"; -/** Extract the credential-store account from a `gcp-sa:` marker. */ +/** Extract a marker account; malformed explicit markers never opt into host ADC. */ export function gcpCredentialMarkerAccount(value: string | undefined): string | undefined { - if (typeof value !== "string") return undefined; - return value.startsWith(GCP_CREDENTIAL_MARKER_PREFIX) && value.length > GCP_CREDENTIAL_MARKER_PREFIX.length - ? value.slice(GCP_CREDENTIAL_MARKER_PREFIX.length) - : undefined; + if (typeof value !== "string" || !value.startsWith(GCP_CREDENTIAL_MARKER_PREFIX)) return undefined; + const account = value.slice(GCP_CREDENTIAL_MARKER_PREFIX.length); + if (!account.trim()) throw new Error("GCP credential marker requires a non-empty account"); + return account; } /** diff --git a/src/providers/api-keys.ts b/src/providers/api-keys.ts index b6d8c236134..8517c424660 100644 --- a/src/providers/api-keys.ts +++ b/src/providers/api-keys.ts @@ -13,6 +13,7 @@ import { GCP_CREDENTIAL_MARKER_PREFIX, gcpCredentialMarkerAccount, parseGcpCrede import { invalidateResolvedProviderKeyCache, probeProviderKeychain, providerKeychainEntry } from "./api-key-resolve"; import type { AccountQuotaFields } from "./quota-types"; import { commitProviderApiKeySelection } from "./api-key-selection"; +import { cleanupRemovedGcpCredentials } from "./gcp-credential-cleanup"; export interface ProviderApiKeyInfo extends AccountQuotaFields { id: string; @@ -38,7 +39,7 @@ export function maskApiKey(value: string): string { /** * Split a pasted key value into candidate credentials. A plain API key is one row; a pasted GCP * credential JSON (or several, comma/space-separated — the same convention as newline-separated - * API keys) is one row per JSON object. Windows file paths are rejected with guidance rather + * API keys) is one row per JSON object. File paths are rejected with guidance rather * than stored, because they are the most common paste mistake and silently break at runtime. * * Honors the parseGcpCredentialJson contract "a literal key that starts with `{` is never eaten": @@ -53,7 +54,7 @@ export function splitCredentialPaste(value: string): Array 1) return { error: "paste one credential JSON per request; add each additional credential as its own key" }; - const credentialJson = (first as { credentialJson: string }).credentialJson; - const id = apiKeyPoolEntryId(credentialJson); - const account = gcpCredentialStoreAccount(name, id); - // Track whether this write CREATED the keychain entry: an upsert over an existing - // reference must not delete the previously-referenced credential on a failed commit. - const existedBefore = providerKeychainEntry(account).getPassword() !== null; - let marker: string; + const stored: Array<{ id: string; marker: string }> = []; + const created: string[] = []; try { - marker = storeGcpCredentialJson(config, name, credentialJson, existedBefore); + for (const part of parts) { + const credentialJson = (part as { credentialJson: string }).credentialJson; + const id = apiKeyPoolEntryId(credentialJson); + const account = gcpCredentialStoreAccount(name, id); + const existedBefore = providerKeychainEntry(account).getPassword() !== null; + if (!existedBefore) created.push(`${GCP_CREDENTIAL_MARKER_PREFIX}${account}`); + stored.push({ id, marker: storeGcpCredentialJson(config, name, credentialJson, existedBefore) }); + } + const committed = commitProviderApiKeySelection(config, name, fresh => { + const pool = ensurePool(fresh); + for (const { id, marker } of stored) { + const existing = pool.find(e => e.id === id); + if (existing) { + existing.key = marker; + if (label?.trim()) existing.label = label.trim(); + } else { + pool.push({ id, key: marker, ...(label?.trim() ? { label: label.trim() } : {}), addedAt: Date.now() }); + } + } + fresh.apiKey = stored[0]!.marker; + return { changed: true, selectionChanged: true, value: stored[0]!.id }; + }); + if (committed.status === "committed") { + invalidateResolvedProviderKeyCache(); + return { id: committed.value }; + } + cleanupRemovedGcpCredentials(created); + return { error: "provider selection unavailable" }; } catch (error) { + cleanupRemovedGcpCredentials(created); return { error: error instanceof Error ? error.message : "credential storage failed" }; } - const committed = commitProviderApiKeySelection(config, name, fresh => { - const pool = ensurePool(fresh); - const existing = pool.find(e => e.id === id); - if (existing) { - existing.key = marker; - if (label?.trim()) existing.label = label.trim(); - } else { - pool.push({ id, key: marker, ...(label?.trim() ? { label: label.trim() } : {}), addedAt: Date.now() }); - } - fresh.apiKey = marker; - return { changed: true, selectionChanged: true, value: id }; - }); - if (committed.status === "committed") { - invalidateResolvedProviderKeyCache(); - return { id }; - } - // Failed commit: the just-written keychain entry has no config reference pointing at it. - // Delete it ONLY when this write created it — an upsert must keep the credential the old - // reference still names. - if (!existedBefore) { - try { providerKeychainEntry(account).deletePassword(); } catch { /* best effort */ } - } - return { error: "provider selection unavailable" }; } } catch (error) { return { error: error instanceof Error ? error.message : "credential parse failed" }; @@ -309,11 +309,13 @@ export function setProviderApiKeyLabel(config: OcxConfig, name: string, id: stri /** Remove one key; removing the active one promotes the first remaining. Persists config. */ export function removeProviderApiKey(config: OcxConfig, name: string, id: string): boolean { + let removed: string | undefined; const committed = commitProviderApiKeySelection(config, name, provider => { const pool = provider.apiKeyPool?.length ? provider.apiKeyPool : provider.apiKey ? [{ id: apiKeyPoolEntryId(provider.apiKey), key: provider.apiKey }] : []; const entry = pool.find(e => e.id === id); if (!entry) return { changed: false, value: false }; + removed = entry.key; provider.apiKeyPool = pool.filter(e => e.id !== id); if (provider.apiKey === entry.key) { const next = provider.apiKeyPool[0]; @@ -323,5 +325,8 @@ export function removeProviderApiKey(config: OcxConfig, name: string, id: string if (provider.apiKeyPool.length === 0) delete provider.apiKeyPool; return { changed: true, value: true }; }); - return committed.status === "committed" && committed.value; + if (committed.status !== "committed" || !committed.value) return false; + if (removed?.startsWith(GCP_CREDENTIAL_MARKER_PREFIX)) cleanupRemovedGcpCredentials([removed]); + invalidateResolvedProviderKeyCache(); + return true; } diff --git a/src/providers/gcp-credential-cleanup.ts b/src/providers/gcp-credential-cleanup.ts new file mode 100644 index 00000000000..2805b0fd598 --- /dev/null +++ b/src/providers/gcp-credential-cleanup.ts @@ -0,0 +1,31 @@ +import { mutatePersistedConfig } from "../config"; +import { GCP_CREDENTIAL_MARKER_PREFIX, gcpCredentialMarkerAccount } from "../lib/gcp-adc"; +import type { OcxProviderConfig } from "../types"; +import { providerKeychainEntry } from "./api-key-resolve"; + +/** Snapshot credential references before removing a key or provider. */ +export function providerGcpCredentialMarkers(provider: OcxProviderConfig): string[] { + return [...new Set([provider.apiKey, ...(provider.apiKeyPool ?? []).map(entry => entry.key)] + .filter((key): key is string => typeof key === "string" && key.startsWith(GCP_CREDENTIAL_MARKER_PREFIX)))]; +} + +/** After a successful config commit, remove only credentials no persisted provider references. */ +export function cleanupRemovedGcpCredentials(markers: readonly string[]): void { + if (!markers.length) return; + try { + // Re-read under the config mutation lock: another provider may share a marker, or a newer + // commit may have reintroduced it. Never decide deletion from the caller's stale snapshot. + const outcome = mutatePersistedConfig(fresh => { + const retained = new Set(Object.values(fresh.providers).flatMap(providerGcpCredentialMarkers)); + for (const marker of new Set(markers)) { + if (retained.has(marker)) continue; + const account = gcpCredentialMarkerAccount(marker); + if (account) providerKeychainEntry(account).deletePassword(); + } + return { changed: false, value: true }; + }); + if (outcome.status === "unavailable") throw new Error("configuration unavailable"); + } catch { + console.warn("GCP credential cleanup incomplete; inspect the OS credential store. Configuration removal remains committed."); + } +} diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index ee1733d2b2b..8b10ad0c564 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -1,6 +1,7 @@ import { contextTierRecordConfigError, modelCapabilitiesConfigError, mergeModelCapabilities } from "../../config/provider-validation"; import { DECLARABLE_HOSTED_TOOL_TYPES } from "../../responses/hosted-tool-policy"; import { randomUUID } from "node:crypto"; +import { cleanupRemovedGcpCredentials, providerGcpCredentialMarkers } from "../../providers/gcp-credential-cleanup"; import { readFileSync } from "node:fs"; import { isDeepStrictEqual } from "node:util"; import type { CatalogModel } from "../../codex/catalog"; @@ -1848,11 +1849,13 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise` references in one pool transaction in `src/providers/api-keys.ts`; post-commit deletion follows [Vertex credential markers](providers/google.md#vertex-credential-markers). Quota activation reuses the existing next-reset fields without adding a polling configuration key. See the [quota activation contract](providers/openai-tiers.md#public-provider-contract). `src/types/config.ts` defines `anthropicAccountPool.routes` as ordered model rules. `src/oauth/anthropic-model-routes.ts` validates bounded names, full case-sensitive globs and stored account IDs; `src/config/diagnostics.ts` rejects malformed candidate writes. Load retains a malformed hand edit so unrelated providers survive, while the enabled Anthropic selector refuses it locally before dispatch. Saved rules remain inert when the pool is disabled; affinity is process-local. Request logs use only the rule’s 1-based `route:#` position, never its configured name. Native function-result injection follows [the separate opt-in control contract](transports/streaming-health.md#experimental-native-function-result-injection); this surface does not infer upstream support or alter its defaults. diff --git a/structure/gui-and-management-api.md b/structure/gui-and-management-api.md index ab8e8032e3b..2a7fb5249d6 100644 --- a/structure/gui-and-management-api.md +++ b/structure/gui-and-management-api.md @@ -1,5 +1,5 @@ # GUI And Management API - +Provider DELETE in `src/server/management/provider-routes.ts` runs [GCP credential cleanup](providers/google.md#vertex-credential-markers) only after saving; failed saves preserve secrets and other persisted references retain them. Anthropic OAuth account DTOs include `autoSwitchThresholdOverride` (integer or null), `autoSwitchThreshold` (pool default) and `effectiveAutoSwitchThreshold`. The dedicated `PUT /api/oauth/accounts/auto-switch` accepts `{ provider: "anthropic", accountId, threshold }`; diff --git a/structure/providers-and-adapters.md b/structure/providers-and-adapters.md index 2092b6951e6..0289c036c17 100644 --- a/structure/providers-and-adapters.md +++ b/structure/providers-and-adapters.md @@ -1,5 +1,5 @@ # Providers And Adapters - +GCP JSON storage in `src/providers/api-keys.ts` and post-commit cleanup in `src/providers/gcp-credential-cleanup.ts` follow [Vertex credential markers](providers/google.md#vertex-credential-markers). Configured spend ceilings bind adapter inference executors to the [seed and physical-start contract](transports/responses-spend.md#historical-pool-continuity-and-rollback). Zed completion and auth-recovery replays claim separately; token acquisition stays outside inference accounting. Claude CLI, CodeBuddy and Qoder each count one invocation toward the frozen request limit, seed before spawn and retain a producer through completion. Child-internal retries/tool turns are not separately counted; reported actual usage settles in full, including amounts above the seed estimate. Devin combines only [consecutive same-ID tool results](adapters/registry.md#devin-consecutive-tool-results), using linear accumulation while preserving intervening message slots, image parts and error markers without mutating the parsed request. Anthropic account pause, model routes, quota labels, and the separate `anthropic2` pool (its helpers, quota, reset grants and management surfaces) follow the [Anthropic account-pool contract](providers/anthropic-account-pool.md). Devin Messages follows the [per-turn output ordering contract](clients/claude-desktop.md#devin-messages-output-ordering), preserving late signatures before text/tools without changing Responses or Chat ordering. diff --git a/structure/providers/google.md b/structure/providers/google.md index 7e81d31afc2..325336b15f7 100644 --- a/structure/providers/google.md +++ b/structure/providers/google.md @@ -1,5 +1,14 @@ # Google Provider +## Vertex credential markers + +`src/lib/gcp-adc.ts` resolves an explicit `gcp-sa:` through the OS credential store. +An empty or whitespace-only account is rejected before token resolution; it never selects host ADC. +`src/providers/api-keys.ts` stores pasted credential JSON as markers, including multiple JSON objects +in one atomic pool update. Failed batches remove only newly created, unreferenced secrets. +`src/providers/gcp-credential-cleanup.ts` cleans removed markers after config persistence, under the +config mutation lock, retaining credentials still referenced by any persisted provider. + ## Google thought-text visibility boundary Google-family parts with `thought: true` stay separate from assistant output. After a CCA diff --git a/tests/adapters/google/gcp-sa-adapter.test.ts b/tests/adapters/google/gcp-sa-adapter.test.ts index 29265adb52c..b2eb1e08ffe 100644 --- a/tests/adapters/google/gcp-sa-adapter.test.ts +++ b/tests/adapters/google/gcp-sa-adapter.test.ts @@ -26,6 +26,25 @@ afterEach(() => { }); describe("vertex adapter with a gcp-sa marker key", () => { + test("an empty marker fails before host ADC or any token fetch", async () => { + const realFetch = globalThis.fetch; + const previousKey = process.env.GOOGLE_CLOUD_API_KEY; + let calls = 0; + process.env.GOOGLE_CLOUD_API_KEY = "fixture-env-key"; + globalThis.fetch = (async () => { calls++; return new Response("fixture"); }) as typeof fetch; + try { + for (const apiKey of ["gcp-sa:", "gcp-sa: "]) { + const provider = { adapter: "google", googleMode: "vertex", baseUrl: "https://aiplatform.googleapis.com", project: "fixture", location: "global", apiKey } as OcxProviderConfig; + await expect(createGoogleAdapter(provider).buildRequest(parsed())).rejects.toThrow(/non-empty account/); + } + expect(calls).toBe(0); + } finally { + globalThis.fetch = realFetch; + if (previousKey === undefined) delete process.env.GOOGLE_CLOUD_API_KEY; + else process.env.GOOGLE_CLOUD_API_KEY = previousKey; + } + }); + test("marker resolves through the ADC branch (never the x-goog-api-key fast path)", async () => { // A real, signable RSA key so buildRequest reaches the actual signing step of the ADC branch. const kp = await generateKeyPair(); diff --git a/tests/providers/gcp-sa-marker.test.ts b/tests/providers/gcp-sa-marker.test.ts index eeb268fd107..749acab2215 100644 --- a/tests/providers/gcp-sa-marker.test.ts +++ b/tests/providers/gcp-sa-marker.test.ts @@ -10,9 +10,10 @@ import { parseGcpCredentialJson, __resetVertexTokenCache, } from "../../src/lib/gcp-adc"; -import { splitCredentialPaste, addProviderApiKey } from "../../src/providers/api-keys"; +import { splitCredentialPaste, addProviderApiKey, removeProviderApiKey } from "../../src/providers/api-keys"; +import { cleanupRemovedGcpCredentials, providerGcpCredentialMarkers } from "../../src/providers/gcp-credential-cleanup"; import { setProviderKeychainEntryFactoryForTests } from "../../src/providers/key-store"; -import { saveConfig } from "../../src/config"; +import { loadConfig, saveConfig } from "../../src/config"; import type { OcxConfig, OcxProviderConfig } from "../../src/types"; import { removeTreeWithRetry } from "../helpers/remove-tree"; @@ -56,7 +57,8 @@ describe("parseGcpCredentialJson", () => { describe("gcpCredentialMarkerAccount", () => { test("extracts the account from a marker", () => { expect(gcpCredentialMarkerAccount(`${GCP_CREDENTIAL_MARKER_PREFIX}p/abc123`)).toBe("p/abc123"); - expect(gcpCredentialMarkerAccount("gcp-sa:")).toBeUndefined(); + expect(() => gcpCredentialMarkerAccount("gcp-sa:")).toThrow(/non-empty account/); + expect(() => gcpCredentialMarkerAccount("gcp-sa: ")).toThrow(/non-empty account/); expect(gcpCredentialMarkerAccount("keychain:p/abc")).toBeUndefined(); expect(gcpCredentialMarkerAccount(undefined)).toBeUndefined(); }); @@ -87,8 +89,10 @@ describe("splitCredentialPaste", () => { expect(parts[1]).toEqual({ credentialJson: SERVICE_ACCOUNT_JSON_2 }); }); - test("a file path is rejected with paste guidance", () => { - expect(() => splitCredentialPaste("C:\\Users\\me\\sa.json")).toThrow(/file path/i); + test("a file path is rejected with paste guidance", () => { + for (const path of ["C:\\Users\\me\\sa.json", "/tmp/sa.json", "./sa.json", "../sa.json", "~/sa.json", "credentials/sa.json", "sa.json", "\\\\host\\share\\sa.json"]) { + expect(() => splitCredentialPaste(path)).toThrow(/file path/i); + } }); test("a single-line brace-leading NON-JSON literal falls back to a plain key (never eaten)", () => { @@ -204,7 +208,7 @@ describe("addProviderApiKey with a pasted credential JSON", () => { expect(JSON.stringify(config).includes(GCP_CREDENTIAL_MARKER_PREFIX)).toBe(false); }); - test("keychain-unavailable fails closed with an error", () => { + test("keychain-unavailable fails closed with an error", () => { // No fake factory installed: the real keyring may exist in CI, so probe through the seam // with a factory that always throws instead. setProviderKeychainEntryFactoryForTests(() => { @@ -213,8 +217,141 @@ describe("addProviderApiKey with a pasted credential JSON", () => { const config = makeConfig({ apiKey: "legacy-key" }); const result = addProviderApiKey(config, "p", SERVICE_ACCOUNT_JSON); expect("error" in result).toBe(true); - expect(JSON.stringify(config).includes(GCP_CREDENTIAL_MARKER_PREFIX)).toBe(false); - }); + expect(JSON.stringify(config).includes(GCP_CREDENTIAL_MARKER_PREFIX)).toBe(false); + }); + + test("multiple JSON credentials commit together, deduplicate and select the first", () => { + installFakeKeychain(); + const config = makeConfig({ apiKey: "legacy-key" }); + const pasted = `${SERVICE_ACCOUNT_JSON}, ${SERVICE_ACCOUNT_JSON_2}, ${SERVICE_ACCOUNT_JSON}`; + expect(addProviderApiKey(config, "p", pasted)).toEqual({ id: expect.any(String) }); + const pool = config.providers.p!.apiKeyPool!; + expect(pool).toHaveLength(3); + expect(config.providers.p!.apiKey).toBe(pool[1]!.key); + expect([...keychainStore.values()].sort()).toEqual([SERVICE_ACCOUNT_JSON, SERVICE_ACCOUNT_JSON_2].sort()); + expect(JSON.stringify(config)).not.toContain("private_key"); + expect(addProviderApiKey(config, "p", pasted)).toEqual({ id: pool[1]!.id }); + expect(config.providers.p!.apiKeyPool).toHaveLength(3); + }); + + test("a failed second write rolls back new credentials but preserves existing upserts", () => { + installFakeKeychain(); + const config = makeConfig({ apiKey: "legacy-key" }); + addProviderApiKey(config, "p", SERVICE_ACCOUNT_JSON); + const before = JSON.stringify(config); + setProviderKeychainEntryFactoryForTests((service, account) => ({ + getPassword: () => keychainStore.get(`${service}:${account}`) ?? null, + setPassword: value => { if (value === SERVICE_ACCOUNT_JSON_2) throw new Error("fixture write failed"); keychainStore.set(`${service}:${account}`, value); }, + deletePassword: () => keychainStore.delete(`${service}:${account}`), + })); + expect(addProviderApiKey(config, "p", `${SERVICE_ACCOUNT_JSON}, ${SERVICE_ACCOUNT_JSON_2}`)).toHaveProperty("error"); + expect(JSON.stringify(config)).toBe(before); + expect([...keychainStore.values()]).toEqual([SERVICE_ACCOUNT_JSON]); + }); + + test("a failed batch config commit removes new secrets and preserves existing credentials", () => { + installFakeKeychain(); + const config = makeConfig({ apiKey: "legacy-key" }); + addProviderApiKey(config, "p", SERVICE_ACCOUNT_JSON); + const removed = config.providers.p!; + config.providers.fallback = { adapter: "openai-chat", baseUrl: "https://api.example.test/v1", apiKey: "fixture" }; + config.defaultProvider = "fallback"; + delete config.providers.p; + saveConfig(config); + config.providers.p = removed; + expect(addProviderApiKey(config, "p", `${SERVICE_ACCOUNT_JSON}, ${SERVICE_ACCOUNT_JSON_2}`)).toHaveProperty("error"); + expect([...keychainStore.values()]).toEqual([SERVICE_ACCOUNT_JSON]); + }); + + test("a later write failure removes every newly created secret without a partial pool", () => { + installFakeKeychain(); + const config = makeConfig({ apiKey: "legacy-key" }); + const before = JSON.stringify(config); + setProviderKeychainEntryFactoryForTests((service, account) => ({ + getPassword: () => keychainStore.get(`${service}:${account}`) ?? null, + setPassword: value => { if (value === SERVICE_ACCOUNT_JSON_2) throw new Error("fixture write failed"); keychainStore.set(`${service}:${account}`, value); }, + deletePassword: () => keychainStore.delete(`${service}:${account}`), + })); + expect(addProviderApiKey(config, "p", `${SERVICE_ACCOUNT_JSON}, ${SERVICE_ACCOUNT_JSON_2}`)).toHaveProperty("error"); + expect(JSON.stringify(config)).toBe(before); + expect(keychainStore.size).toBe(0); + }); + + test("a refused removal leaves the credential intact", () => { + installFakeKeychain(); + const config = makeConfig({ apiKey: "legacy-key" }); + const result = addProviderApiKey(config, "p", SERVICE_ACCOUNT_JSON); + if ("error" in result) throw new Error(result.error); + const provider = config.providers.p!; + config.providers.fallback = { adapter: "openai-chat", baseUrl: "https://api.example.test/v1", apiKey: "fixture" }; + config.defaultProvider = "fallback"; + delete config.providers.p; + saveConfig(config); + config.providers.p = provider; + expect(removeProviderApiKey(config, "p", result.id)).toBe(false); + expect([...keychainStore.values()]).toEqual([SERVICE_ACCOUNT_JSON]); + }); + + test("key removal deletes its secret only after successful config removal", () => { + installFakeKeychain(); + const config = makeConfig({ apiKey: "legacy-key" }); + const result = addProviderApiKey(config, "p", SERVICE_ACCOUNT_JSON); + if ("error" in result) throw new Error(result.error); + expect(removeProviderApiKey(config, "p", "missing")).toBe(false); + expect([...keychainStore.values()]).toEqual([SERVICE_ACCOUNT_JSON]); + expect(removeProviderApiKey(config, "p", result.id)).toBe(true); + expect(keychainStore.size).toBe(0); + expect(config.providers.p!.apiKey).toBe("legacy-key"); + }); + + test("provider cleanup preserves shared markers until their last reference is removed", () => { + installFakeKeychain(); + const config = makeConfig({ apiKey: "legacy-key" }); + addProviderApiKey(config, "p", SERVICE_ACCOUNT_JSON); + const markers = providerGcpCredentialMarkers(config.providers.p!); + config.providers.other = { ...config.providers.p!, apiKeyPool: undefined }; + config.defaultProvider = "other"; + delete config.providers.p; + saveConfig(config); + cleanupRemovedGcpCredentials(markers); + expect([...keychainStore.values()]).toEqual([SERVICE_ACCOUNT_JSON]); + config.providers.fallback = { adapter: "openai-chat", baseUrl: "https://api.example.test/v1", apiKey: "fixture" }; + config.defaultProvider = "fallback"; + delete config.providers.other; + saveConfig(config); + cleanupRemovedGcpCredentials(markers); + expect(keychainStore.size).toBe(0); + }); + + test("CLI provider removal cleans all removed GCP credentials", async () => { + installFakeKeychain(); + const config = makeConfig({ apiKey: "legacy-key" }); + addProviderApiKey(config, "p", `${SERVICE_ACCOUNT_JSON}, ${SERVICE_ACCOUNT_JSON_2}`); + config.providers.fallback = { adapter: "openai-chat", baseUrl: "https://api.example.test/v1", apiKey: "fixture" }; + config.defaultProvider = "fallback"; + saveConfig(config); + const { handleProviderCommand } = await import("../../src/cli/provider"); + await handleProviderCommand(["remove", "p", "--json"]); + expect(loadConfig().providers.p).toBeUndefined(); + expect(keychainStore.size).toBe(0); + }); + + test("management provider removal cleans all removed GCP credentials", async () => { + installFakeKeychain(); + const config = makeConfig({ apiKey: "legacy-key" }); + addProviderApiKey(config, "p", `${SERVICE_ACCOUNT_JSON}, ${SERVICE_ACCOUNT_JSON_2}`); + config.providers.fallback = { adapter: "openai-chat", baseUrl: "https://api.example.test/v1", apiKey: "fixture" }; + saveConfig(config); + const { handleProviderRoutes } = await import("../../src/server/management/provider-routes"); + const req = new Request("http://localhost/api/providers?name=p", { method: "DELETE" }); + const response = await handleProviderRoutes({ + req, url: new URL(req.url), config, deps: {}, + convergeCodexCatalog: async () => ({ converged: true }), + } as unknown as import("../../src/server/management/context").ManagementContext); + expect(response?.status).toBe(200); + expect(loadConfig().providers.p).toBeUndefined(); + expect(keychainStore.size).toBe(0); + }); }); describe("gcp-adc marker source", () => { From 2905bf5dd8e4da52a6f17c74364f15d06a0e2dac Mon Sep 17 00:00:00 2001 From: GSL-R Date: Sun, 11 Oct 2026 17:29:30 +0900 Subject: [PATCH 09/19] fix(gcp-adc): clean credentials after provider batch and overwrite commits --- src/server/management/provider-routes.ts | 11 +++++- structure/gui-and-management-api.md | 2 +- tests/providers/gcp-sa-marker.test.ts | 46 +++++++++++++++++++++++- 3 files changed, 56 insertions(+), 3 deletions(-) diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index 8b10ad0c564..67cf26a55dc 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -1073,6 +1073,7 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise(persisted => { if (!isDeepStrictEqual(providerEditorConfigDTO(persisted), baselineResult.value)) { return { @@ -1095,6 +1096,7 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise { validationError = anthropicInstancePublicationError(name, candidate, config.providers[name]); if (validationError) return; @@ -1375,6 +1378,11 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise provider ? providerGcpCredentialMarkers(provider) : []); config.providers[name] = candidate; if (body.setDefault === true) config.defaultProvider = name; (deps.saveConfigPreservingClaudeCode ?? save)(config); @@ -1388,6 +1396,7 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise { expect(keychainStore.size).toBe(0); }); + test.each(["PUT", "POST"])("management %s cleans replaced markers only after commit", async method => { + installFakeKeychain(); + const config = makeConfig({ apiKey: "legacy-key" }); + addProviderApiKey(config, "p", `${SERVICE_ACCOUNT_JSON}, ${SERVICE_ACCOUNT_JSON_2}`); + config.providers.fallback = { adapter: "openai-chat", baseUrl: "https://api.example.test/v1", apiKey: "fixture" }; + saveConfig(config); + const resolver = spyOn(destinationPolicy, "providerDestinationResolvedError").mockResolvedValue(null); + try { + const { handleProviderRoutes } = await import("../../src/server/management/provider-routes"); + const request = async (body: unknown, failSave = false) => { + const req = new Request("http://localhost/api/providers", { + method, headers: { "Content-Type": "application/json" }, body: JSON.stringify(body), + }); + return handleProviderRoutes({ req, url: new URL(req.url), config, + deps: { saveConfigPreservingClaudeCode: (next: OcxConfig) => { + if (failSave) throw new Error("fixture save refused"); + saveConfig(next); + } }, + convergeCodexCatalog: async () => ({ converged: true }), + syncClaudeAgentDefsBestEffort: async () => {}, + } as unknown as import("../../src/server/management/context").ManagementContext); + }; + const replacement = { adapter: "openai-chat", baseUrl: "https://replacement.example.test/v1", apiKey: "replacement" }; + const baseline = providerEditorConfigDTO(loadConfig()); + const next = structuredClone(baseline); + delete next.providers.p; + next.defaultProvider = "fallback"; + const body = method === "PUT" ? { baseline, next } : { name: "p", provider: replacement }; + if (method === "PUT") { + const refused = await request({ baseline: { ...baseline, defaultProvider: "fallback" }, next }); + expect(refused?.status).toBe(409); + } else { + await expect(request(body, true)).rejects.toThrow("fixture save refused"); + } + expect(keychainStore.size).toBe(2); + expect((await request(body))?.status).toBe(200); + expect(keychainStore.size).toBe(0); + if (method === "PUT") expect(loadConfig().providers.p).toBeUndefined(); + else expect(loadConfig().providers.p!.apiKey).toBe("replacement"); + } finally { resolver.mockRestore(); } + }); + test("management provider removal cleans all removed GCP credentials", async () => { installFakeKeychain(); const config = makeConfig({ apiKey: "legacy-key" }); From 950260ff29792d613e4609f032cc12b004cc6039 Mon Sep 17 00:00:00 2001 From: GSL-R Date: Mon, 12 Oct 2026 01:22:54 +0900 Subject: [PATCH 10/19] fix(management): include durable provider markers in DELETE cleanup Co-Authored-By: Claude --- src/server/management/provider-routes.ts | 5 ++++- structure/config.md | 2 -- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index 67cf26a55dc..0efa6cf3506 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -1858,7 +1858,10 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise provider ? providerGcpCredentialMarkers(provider) : []); delete config.providers[name]; const { dropProviderCustomModels } = await import("../../providers/provider-id-rewrite"); const droppedCustomModels = dropProviderCustomModels(config, name); diff --git a/structure/config.md b/structure/config.md index 827331d24c8..3606d1d826b 100644 --- a/structure/config.md +++ b/structure/config.md @@ -1,7 +1,5 @@ # Config Surface - Pasted GCP JSON persists only as `gcp-sa:` references in one pool transaction in `src/providers/api-keys.ts`; post-commit deletion follows [Vertex credential markers](providers/google.md#vertex-credential-markers). - Quota activation reuses the existing next-reset fields without adding a polling configuration key. See the [quota activation contract](providers/openai-tiers.md#public-provider-contract). `src/types/config.ts` defines `anthropicAccountPool.routes` as ordered model rules. `src/oauth/anthropic-model-routes.ts` validates bounded names, full case-sensitive globs and stored account IDs; `src/config/diagnostics.ts` rejects malformed candidate writes. Load retains a malformed hand edit so unrelated providers survive, while the enabled Anthropic selector refuses it locally before dispatch. Saved rules remain inert when the pool is disabled; affinity is process-local. Request logs use only the rule’s 1-based `route:#` position, never its configured name. `src/cli/hub-disable.ts` removes interdependent hub/loopback/role/hostname settings through `mutatePersistedConfig` and `validateConfigCandidate` in one commit. Top-level deletion intent uses `src/config/rebase-provenance.ts` and authoritative disk provenance in `src/config/live-reconcile.ts` to preserve removed hub bindings through later live saves, retaining the actual open socket identity separately from the desired next-start hostname. `src/config/api-key-reconcile.ts` merges key rows by stable ID under the existing save lock; either side's revocation defeats stale edits while disjoint names and rotations merge. `tests/config/hub-disable-live-save.test.ts` exercises disable followed by live key creation, rename, rotation, unrelated settings and repeated saves. Saved API keys have no immutable enrollment marker; explicit exact-ID revocation preserves all unselected keys. Native function-result injection follows [the separate opt-in control contract](transports/streaming-health.md#experimental-native-function-result-injection); this surface does not infer upstream support or alter its defaults. From da9d2fc602d5d81c3e97916da8f4f49690b92998 Mon Sep 17 00:00:00 2001 From: GSL-R Date: Mon, 12 Oct 2026 01:41:44 +0900 Subject: [PATCH 11/19] fix(gcp-adc): validate required fields in parseGcpCredentialJson before persistence Co-Authored-By: Claude --- src/lib/gcp-adc.ts | 14 +++++++++++--- tests/providers/gcp-sa-marker.test.ts | 14 ++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/lib/gcp-adc.ts b/src/lib/gcp-adc.ts index 481fc035717..aa053184ed0 100644 --- a/src/lib/gcp-adc.ts +++ b/src/lib/gcp-adc.ts @@ -68,9 +68,17 @@ export function parseGcpCredentialJson(value: string): AdcFileCredentials | unde const trimmed = (value ?? "").trim(); if (!trimmed.startsWith("{") || !trimmed.endsWith("}")) return undefined; try { - const parsed = JSON.parse(trimmed) as { type?: unknown }; - if (parsed?.type === "service_account" || parsed?.type === "authorized_user") { - return parsed as AdcFileCredentials; + const parsed = JSON.parse(trimmed) as Record; + if (parsed?.type === "service_account") { + if (typeof parsed.client_email !== "string" || !parsed.client_email.trim()) return undefined; + if (typeof parsed.private_key !== "string" || !parsed.private_key.trim()) return undefined; + return parsed as unknown as AdcFileCredentials; + } + if (parsed?.type === "authorized_user") { + if (typeof parsed.client_id !== "string" || !parsed.client_id.trim()) return undefined; + if (typeof parsed.client_secret !== "string" || !parsed.client_secret.trim()) return undefined; + if (typeof parsed.refresh_token !== "string" || !parsed.refresh_token.trim()) return undefined; + return parsed as unknown as AdcFileCredentials; } return undefined; } catch { diff --git a/tests/providers/gcp-sa-marker.test.ts b/tests/providers/gcp-sa-marker.test.ts index 398d3de9abd..fa5ffac462f 100644 --- a/tests/providers/gcp-sa-marker.test.ts +++ b/tests/providers/gcp-sa-marker.test.ts @@ -54,6 +54,20 @@ describe("parseGcpCredentialJson", () => { expect(parseGcpCredentialJson("AIza-real-key-123456")).toBeUndefined(); expect(parseGcpCredentialJson("{}")).toBeUndefined(); }); + + test("rejects incomplete service_account missing required fields", () => { + expect(parseGcpCredentialJson(JSON.stringify({ type: "service_account" }))).toBeUndefined(); + expect(parseGcpCredentialJson(JSON.stringify({ type: "service_account", client_email: "svc@test" }))).toBeUndefined(); + expect(parseGcpCredentialJson(JSON.stringify({ type: "service_account", private_key: "k" }))).toBeUndefined(); + expect(parseGcpCredentialJson(JSON.stringify({ type: "service_account", client_email: " ", private_key: "k" }))).toBeUndefined(); + }); + + test("rejects incomplete authorized_user missing required fields", () => { + expect(parseGcpCredentialJson(JSON.stringify({ type: "authorized_user" }))).toBeUndefined(); + expect(parseGcpCredentialJson(JSON.stringify({ type: "authorized_user", client_id: "c", client_secret: "s" }))).toBeUndefined(); + expect(parseGcpCredentialJson(JSON.stringify({ type: "authorized_user", client_id: "c", refresh_token: "r" }))).toBeUndefined(); + expect(parseGcpCredentialJson(JSON.stringify({ type: "authorized_user", client_id: "c", client_secret: "s", refresh_token: " " }))).toBeUndefined(); + }); }); describe("gcpCredentialMarkerAccount", () => { From f919a6a7fe3e1093fdc78ea3e84d495d751a39d4 Mon Sep 17 00:00:00 2001 From: GSL-R <174104911+GSL-R@users.noreply.github.com> Date: Mon, 12 Oct 2026 03:00:35 +0900 Subject: [PATCH 12/19] style(management): keep GCP cleanup within file-size gate --- src/server/management/provider-routes.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index 0efa6cf3506..0c646645e30 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -1378,10 +1378,8 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise provider ? providerGcpCredentialMarkers(provider) : []); config.providers[name] = candidate; if (body.setDefault === true) config.defaultProvider = name; @@ -1859,8 +1857,7 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise provider ? providerGcpCredentialMarkers(provider) : []); delete config.providers[name]; const { dropProviderCustomModels } = await import("../../providers/provider-id-rewrite"); From 1f4397767e4b2455fff0607e1884ac20d497bea0 Mon Sep 17 00:00:00 2001 From: JUN Date: Mon, 12 Oct 2026 05:28:46 +0900 Subject: [PATCH 13/19] fix(vertex): reject credential JSON before generic provider persistence --- .../docs/reference/configuration/providers.md | 11 +- scripts/test-layout/layout.json | 2 +- src/cli/provider.ts | 7 + src/providers/api-keys.ts | 24 +++- src/server/management/provider-routes.ts | 10 +- structure/providers/google.md | 5 + .../google}/gcp-sa-marker.test.ts | 28 ++-- tests/fixtures/test-layout-expected.json | 4 + .../management-gcp-credential-input.test.ts | 121 ++++++++++++++++++ 9 files changed, 187 insertions(+), 25 deletions(-) rename tests/{providers => adapters/google}/gcp-sa-marker.test.ts (96%) create mode 100644 tests/server/management-gcp-credential-input.test.ts diff --git a/docs-site/src/content/docs/reference/configuration/providers.md b/docs-site/src/content/docs/reference/configuration/providers.md index 6aebfc03f30..cfbfb6e6d2a 100644 --- a/docs-site/src/content/docs/reference/configuration/providers.md +++ b/docs-site/src/content/docs/reference/configuration/providers.md @@ -1412,11 +1412,16 @@ model key. ## Storing keys in the OS keychain -For Vertex, the API-key input also accepts complete GCP `service_account` or `authorized_user` -credential JSON. Paste the JSON contents, not a Windows, Unix, or relative file path. Multiple +For Vertex, the dedicated API-key pool input (`POST /api/providers/keys`) accepts complete GCP +`service_account` or `authorized_user` credential JSON after the provider has been created. +The provider create/edit form and `ocx provider add --api-key` refuse credential JSON before +saving: create the provider without a key, then add the JSON through its key pool. Paste the +JSON contents, not a Windows, Unix, or relative file path. Multiple JSON objects separated by commas or whitespace are stored together as separate pool entries; the first becomes active. Configuration contains only `gcp-sa:` references, never the -credential JSON. Empty marker accounts are invalid and do not fall back to host credentials. +credential JSON. Mixed JSON/literal-key bulk pastes are rejected as a whole; add literal keys +separately. Storage failures leave the configuration unchanged and report a fixed error without +echoing credential material. Empty marker accounts are invalid and do not fall back to host credentials. Removing a key or provider deletes its unreferenced GCP credentials after the configuration is saved; a credential referenced by another provider is retained. If OS-store cleanup fails, a warning requests manual inspection without undoing the committed configuration removal. diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index b11337aa855..5b5ee62053c 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -1,7 +1,7 @@ { "version": 1, "root": "tests", - "explicit": { + "explicit": {"gcp-sa-marker.test.ts": "adapters/google", "gcp-sa-adapter.test.ts": "adapters/google", "management-gcp-credential-input.test.ts": "server", "messages-request-id-headers.test.ts": "claude-integration", "messages-request-id-endpoint.test.ts": "claude-integration", "anthropic-revoked-token.test.ts": "adapters/anthropic", diff --git a/src/cli/provider.ts b/src/cli/provider.ts index 9b47d221621..1988d3f3397 100644 --- a/src/cli/provider.ts +++ b/src/cli/provider.ts @@ -16,6 +16,7 @@ import { providerConfigSeed } from "../providers/derive"; import { assertAnthropicInstanceLoginConfig } from "../oauth/store-anthropic-instance"; import { dropProviderCustomModels } from "../providers/provider-id-rewrite"; import { cleanupRemovedGcpCredentials, providerGcpCredentialMarkers } from "../providers/gcp-credential-cleanup"; +import { providerCredentialInputError } from "../providers/api-keys"; import type { OcxProviderConfig } from "../types"; import { findLiveProxy } from "../server/proxy-liveness"; import { syncModelsToCodex } from "../codex/sync"; @@ -250,6 +251,12 @@ async function handleAdd(args: string[], deps: ProviderCommandDeps): Promise garbage`) is rejected rather than silently kept. const tailGap = trimmed.slice(spans[spans.length - 1]!.end + 1); if (!/^[\s,]*$/.test(tailGap)) { - throw new Error("Unrecognized credential JSON: unexpected content after the last JSON object. Paste credential JSON(s) separated by commas or newlines, or a plain API key."); + throw new Error("Unrecognized credential JSON: unexpected content after the last JSON object. Mixing JSON and literal keys is not supported; add them separately."); } return objects.map((json) => { if (!parseGcpCredentialJson(json)) { @@ -132,6 +135,23 @@ export function splitCredentialPaste(value: string): Array entry && typeof entry === "object" ? entry.key : undefined) : [])]; + try { + for (const value of values) { + if (typeof value !== "string") continue; + if (splitCredentialPaste(value).some(part => typeof part !== "string")) { + return "Credential JSON must be added through the provider API-key pool endpoint (/api/providers/keys), after creating the provider without a key. Nothing was saved."; + } + } + } catch (error) { + return error instanceof Error ? error.message : "Invalid credential paste"; + } + return undefined; +} + /** * Store a pasted GCP credential JSON in the OS keychain and return the `gcp-sa:` marker for it. * Verified write (read-back check) mirrors storeProviderKeyInKeychain; on any failure the caller @@ -257,7 +277,7 @@ export function addProviderApiKey(config: OcxConfig, name: string, key: string, return { error: "provider selection unavailable" }; } catch (error) { cleanupRemovedGcpCredentials(created); - return { error: error instanceof Error ? error.message : "credential storage failed" }; + return { error: "credential storage failed; configuration was not changed" }; } } } catch (error) { diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index 0c646645e30..8ea658acc03 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -2,6 +2,7 @@ import { contextTierRecordConfigError, modelCapabilitiesConfigError, mergeModelC import { DECLARABLE_HOSTED_TOOL_TYPES } from "../../responses/hosted-tool-policy"; import { randomUUID } from "node:crypto"; import { cleanupRemovedGcpCredentials, providerGcpCredentialMarkers } from "../../providers/gcp-credential-cleanup"; +import { providerCredentialInputError } from "../../providers/api-keys"; import { readFileSync } from "node:fs"; import { isDeepStrictEqual } from "node:util"; import type { CatalogModel } from "../../codex/catalog"; @@ -1140,16 +1141,15 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise { config.providers.fallback = { adapter: "openai-chat", baseUrl: "https://api.example.test/v1", apiKey: "fixture" }; config.defaultProvider = "fallback"; saveConfig(config); - const { handleProviderCommand } = await import("../../src/cli/provider"); + const { handleProviderCommand } = await import("../../../src/cli/provider"); await handleProviderCommand(["remove", "p", "--json"]); expect(loadConfig().providers.p).toBeUndefined(); expect(keychainStore.size).toBe(0); @@ -360,7 +360,7 @@ describe("addProviderApiKey with a pasted credential JSON", () => { saveConfig(config); const resolver = spyOn(destinationPolicy, "providerDestinationResolvedError").mockResolvedValue(null); try { - const { handleProviderRoutes } = await import("../../src/server/management/provider-routes"); + const { handleProviderRoutes } = await import("../../../src/server/management/provider-routes"); const request = async (body: unknown, failSave = false) => { const req = new Request("http://localhost/api/providers", { method, headers: { "Content-Type": "application/json" }, body: JSON.stringify(body), @@ -372,7 +372,7 @@ describe("addProviderApiKey with a pasted credential JSON", () => { } }, convergeCodexCatalog: async () => ({ converged: true }), syncClaudeAgentDefsBestEffort: async () => {}, - } as unknown as import("../../src/server/management/context").ManagementContext); + } as unknown as import("../../../src/server/management/context").ManagementContext); }; const replacement = { adapter: "openai-chat", baseUrl: "https://replacement.example.test/v1", apiKey: "replacement" }; const baseline = providerEditorConfigDTO(loadConfig()); @@ -400,12 +400,12 @@ describe("addProviderApiKey with a pasted credential JSON", () => { addProviderApiKey(config, "p", `${SERVICE_ACCOUNT_JSON}, ${SERVICE_ACCOUNT_JSON_2}`); config.providers.fallback = { adapter: "openai-chat", baseUrl: "https://api.example.test/v1", apiKey: "fixture" }; saveConfig(config); - const { handleProviderRoutes } = await import("../../src/server/management/provider-routes"); + const { handleProviderRoutes } = await import("../../../src/server/management/provider-routes"); const req = new Request("http://localhost/api/providers?name=p", { method: "DELETE" }); const response = await handleProviderRoutes({ req, url: new URL(req.url), config, deps: {}, convergeCodexCatalog: async () => ({ converged: true }), - } as unknown as import("../../src/server/management/context").ManagementContext); + } as unknown as import("../../../src/server/management/context").ManagementContext); expect(response?.status).toBe(200); expect(loadConfig().providers.p).toBeUndefined(); expect(keychainStore.size).toBe(0); diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index 3da23ddfff2..63d4a705629 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -1,4 +1,8 @@ { + "gcp-sa-marker.test.ts": "adapters/google", + "gcp-sa-adapter.test.ts": "adapters/google", + "management-gcp-credential-input.test.ts": "server", + "messages-request-id-headers.test.ts": "claude-integration", "messages-request-id-endpoint.test.ts": "claude-integration", "anthropic-revoked-token.test.ts": "adapters/anthropic", diff --git a/tests/server/management-gcp-credential-input.test.ts b/tests/server/management-gcp-credential-input.test.ts new file mode 100644 index 00000000000..540dd9398f1 --- /dev/null +++ b/tests/server/management-gcp-credential-input.test.ts @@ -0,0 +1,121 @@ +import { afterEach, beforeEach, expect, mock, spyOn, test } from "bun:test"; +import { mkdtempSync, readFileSync, readdirSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { saveConfig, loadConfig } from "../../src/config"; +import { addProviderApiKey } from "../../src/providers/api-keys"; +import { setProviderKeychainEntryFactoryForTests } from "../../src/providers/key-store"; +import { handleProviderRoutes } from "../../src/server/management/provider-routes"; +import { handleProviderCommand } from "../../src/cli/provider"; +import * as destinationPolicy from "../../src/lib/destination-policy"; +import type { OcxConfig } from "../../src/types"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; + +const credential = JSON.stringify({ type: "authorized_user", client_id: "test-client", client_secret: "fixture-secret", refresh_token: "fixture-refresh" }); +let home: string; +let previousHome: string | undefined; +let config: OcxConfig; +const secrets = new Map(); + +beforeEach(() => { + previousHome = process.env.OPENCODEX_HOME; + home = mkdtempSync(join(tmpdir(), "ocx-gcp-input-")); + process.env.OPENCODEX_HOME = home; + secrets.clear(); + setProviderKeychainEntryFactoryForTests((service, account) => ({ + getPassword: () => secrets.get(`${service}:${account}`) ?? null, + setPassword: value => { secrets.set(`${service}:${account}`, value); }, + deletePassword: () => secrets.delete(`${service}:${account}`), + })); + spyOn(destinationPolicy, "providerDestinationResolvedError").mockResolvedValue(null); + config = { port: 10199, defaultProvider: "relay", providers: { + relay: { adapter: "openai-chat", baseUrl: "https://relay.example/v1", apiKey: "old-key" }, + } }; + saveConfig(config); +}); + +afterEach(() => { + mock.restore(); + setProviderKeychainEntryFactoryForTests(null); + if (previousHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousHome; + removeTreeWithRetry(home); +}); + +function assertNoPlaintext() { + for (const file of readdirSync(home).filter(file => file.startsWith("config.json"))) { + const content = readFileSync(join(home, file), "utf8"); + expect(content).not.toContain("fixture-secret"); + expect(content).not.toContain("fixture-refresh"); + } +} + +test.each([ + { name: "new-provider", values: { apiKey: credential } }, + { name: "relay", values: { apiKey: credential } }, + { name: "relay", values: { apiKey: credential, apiKeyPool: [{ id: "raw", key: credential }] } }, + { name: "relay", values: { apiKey: "plain-key", apiKeyPool: [{ id: "raw", key: credential }] } }, + { name: "relay", values: { apiKey: `plain-key, ${credential}` } }, +])("provider create/overwrite refuses raw JSON before saving: %j", async ({ name, values }) => { + const before = structuredClone(config); + const url = new URL("http://127.0.0.1/api/providers"); + const response = await handleProviderRoutes({ + req: new Request(url, { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ + name, provider: { adapter: "openai-chat", baseUrl: "https://relay.example/v1", ...values }, + }) }), url, config, deps: {}, version: "test", trustedLoopbackIngress: true, guiSessionIssuance: null, + convergeCodexCatalog: async () => ({ status: "committed", changed: false, degraded: false, notices: [] }), + syncClaudeAgentDefsBestEffort: async () => {}, + }); + expect(response?.status).toBe(400); + expect(await response!.json()).toMatchObject({ error: expect.stringMatching(/API-key|credential|JSON/i) }); + expect(config).toEqual(before); + expect(loadConfig().providers).toEqual(before.providers); + expect(secrets.size).toBe(0); + assertNoPlaintext(); +}); + +test.each([`plain-key, ${credential}`, `${credential}, plain-key`])("mixed key/JSON pool paste is entirely rejected", paste => { + const before = structuredClone(config); + const result = addProviderApiKey(config, "relay", paste); + expect(result).toMatchObject({ error: expect.stringMatching(/Mixing|separate/i) }); + expect(config).toEqual(before); + expect(secrets.size).toBe(0); + assertNoPlaintext(); +}); + +test("dedicated pool paste persists markers only, including subsequent backups", () => { + expect(addProviderApiKey(config, "relay", credential)).toHaveProperty("id"); + expect(config.providers.relay!.apiKey).toStartWith("gcp-sa:"); + saveConfig(config); + expect([...secrets.values()]).toEqual([credential]); + assertNoPlaintext(); +}); + +test("CLI provider add refuses credential JSON before config or backups change", async () => { + const before = readFileSync(join(home, "config.json"), "utf8"); + const previousExitCode = process.exitCode; + const output = spyOn(console, "error").mockImplementation(() => {}); + try { + await handleProviderCommand(["add", "relay", "--force", "--adapter", "openai-chat", "--base-url", "https://relay.example/v1", "--api-key", credential]); + expect(process.exitCode).toBe(2); + expect(output.mock.calls.flat().join(" ")).toContain("API-key pool endpoint"); + expect(output.mock.calls.flat().join(" ")).not.toContain("fixture-secret"); + expect(readFileSync(join(home, "config.json"), "utf8")).toBe(before); + expect(secrets.size).toBe(0); + assertNoPlaintext(); + } finally { process.exitCode = previousExitCode ?? 0; } +}); + +test("OS-store failures propagate without echoing credential material or committing a pool", () => { + const before = structuredClone(config); + setProviderKeychainEntryFactoryForTests(() => ({ + getPassword: () => null, + setPassword: () => { throw new Error(`store failed: ${credential}`); }, + deletePassword: () => {}, + })); + const result = addProviderApiKey(config, "relay", credential); + expect(result).toMatchObject({ error: expect.stringContaining("credential storage failed") }); + expect(JSON.stringify(result)).not.toContain("fixture-secret"); + expect(config).toEqual(before); + assertNoPlaintext(); +}); From 93825e8626318ebfef201cd0222e73616f81034e Mon Sep 17 00:00:00 2001 From: JUN Date: Mon, 12 Oct 2026 05:42:27 +0900 Subject: [PATCH 14/19] fix(vertex): preserve GCP keychain payloads across key migrations --- .../docs/reference/configuration/providers.md | 3 ++ src/lib/gcp-adc.ts | 7 ++- src/providers/api-key-resolve.ts | 4 +- src/providers/gcp-credential-cleanup.ts | 12 +++-- src/providers/key-store.ts | 9 +++- structure/providers/google.md | 4 ++ .../management-gcp-credential-input.test.ts | 44 ++++++++++++++++++- 7 files changed, 70 insertions(+), 13 deletions(-) diff --git a/docs-site/src/content/docs/reference/configuration/providers.md b/docs-site/src/content/docs/reference/configuration/providers.md index cfbfb6e6d2a..dca0f083e5b 100644 --- a/docs-site/src/content/docs/reference/configuration/providers.md +++ b/docs-site/src/content/docs/reference/configuration/providers.md @@ -1425,6 +1425,9 @@ echoing credential material. Empty marker accounts are invalid and do not fall b Removing a key or provider deletes its unreferenced GCP credentials after the configuration is saved; a credential referenced by another provider is retained. If OS-store cleanup fails, a warning requests manual inspection without undoing the committed configuration removal. +Ordinary keychain store/restore actions leave GCP markers and JSON in the OS store. Restoring +credential JSON through a `keychain:` alias is refused, and cleanup preserves accounts still +referenced through either marker spelling. By default a provider's `apiKey` and `apiKeyPool` sit in `config.json` (mode 0600, atomic writes). If you would rather keep the key material out of the file, move it into the OS credential store: diff --git a/src/lib/gcp-adc.ts b/src/lib/gcp-adc.ts index aa053184ed0..05050e694f1 100644 --- a/src/lib/gcp-adc.ts +++ b/src/lib/gcp-adc.ts @@ -22,7 +22,7 @@ import { Buffer } from "node:buffer"; import * as os from "node:os"; import * as path from "node:path"; import { readFileSync, existsSync, statSync } from "node:fs"; -import { providerKeychainEntry } from "../providers/api-key-resolve"; +import { GCP_CREDENTIAL_MARKER_PREFIX, providerKeychainEntry } from "../providers/api-key-resolve"; import { captureConfigGeneration, sweepExpiredOnWrite, @@ -45,10 +45,9 @@ const TOKEN_RETRY_BASE_MS = 300; * Reference prefix stored in `provider.apiKey` (and `apiKeyPool` entries) when the user pastes a * GCP credential JSON directly into an API-key slot. The secret JSON lives in the OS credential * store under the same service the `keychain:` references use; only the marker is persisted to - * `config.json`. Lives here — not in api-key-resolve — because it is meaningful only to this - * module's ADC resolution (the API-key twins, `keychain:` and `${ENV}`, stay there). + * `config.json`. The leaf reference prefix also identifies already-stored material for migration. */ -export const GCP_CREDENTIAL_MARKER_PREFIX = "gcp-sa:"; +export { GCP_CREDENTIAL_MARKER_PREFIX } from "../providers/api-key-resolve"; /** Extract a marker account; malformed explicit markers never opt into host ADC. */ export function gcpCredentialMarkerAccount(value: string | undefined): string | undefined { diff --git a/src/providers/api-key-resolve.ts b/src/providers/api-key-resolve.ts index eeaa0f4d028..00a787d284e 100644 --- a/src/providers/api-key-resolve.ts +++ b/src/providers/api-key-resolve.ts @@ -21,6 +21,8 @@ import { loadKeyringBinding } from "../lib/keyring-native"; import type { OcxProviderConfig } from "../types"; export const KEYCHAIN_REFERENCE_PREFIX = "keychain:"; +/** Stored GCP JSON uses the same OS service, but must never be restored as a literal API key. */ +export const GCP_CREDENTIAL_MARKER_PREFIX = "gcp-sa:"; export const PROVIDER_KEYCHAIN_SERVICE = "opencodex.provider-api-key.v1"; export interface ProviderKeychainEntry { @@ -125,7 +127,7 @@ export type ProviderKeyStoreKind = "keychain" | "env" | "file" | "none"; export function providerKeyStoreKind(provider: Pick | undefined): ProviderKeyStoreKind { const key = provider?.apiKey; if (!key) return "none"; - if (isKeychainReference(key)) return "keychain"; + if (key.startsWith(GCP_CREDENTIAL_MARKER_PREFIX) || isKeychainReference(key)) return "keychain"; if (/^\$\{?\w+\}?$/.test(key)) return "env"; return "file"; } diff --git a/src/providers/gcp-credential-cleanup.ts b/src/providers/gcp-credential-cleanup.ts index 2805b0fd598..309eb0ecc51 100644 --- a/src/providers/gcp-credential-cleanup.ts +++ b/src/providers/gcp-credential-cleanup.ts @@ -1,7 +1,7 @@ import { mutatePersistedConfig } from "../config"; import { GCP_CREDENTIAL_MARKER_PREFIX, gcpCredentialMarkerAccount } from "../lib/gcp-adc"; import type { OcxProviderConfig } from "../types"; -import { providerKeychainEntry } from "./api-key-resolve"; +import { isKeychainReference, keychainAccount, providerKeychainEntry } from "./api-key-resolve"; /** Snapshot credential references before removing a key or provider. */ export function providerGcpCredentialMarkers(provider: OcxProviderConfig): string[] { @@ -16,11 +16,15 @@ export function cleanupRemovedGcpCredentials(markers: readonly string[]): void { // Re-read under the config mutation lock: another provider may share a marker, or a newer // commit may have reintroduced it. Never decide deletion from the caller's stale snapshot. const outcome = mutatePersistedConfig(fresh => { - const retained = new Set(Object.values(fresh.providers).flatMap(providerGcpCredentialMarkers)); + const retained = new Set(Object.values(fresh.providers).flatMap(provider => + [provider.apiKey, ...(provider.apiKeyPool ?? []).map(entry => entry.key)] + .flatMap(key => { + const account = isKeychainReference(key) ? keychainAccount(key) : gcpCredentialMarkerAccount(key); + return account === undefined ? [] : [account]; + }))); for (const marker of new Set(markers)) { - if (retained.has(marker)) continue; const account = gcpCredentialMarkerAccount(marker); - if (account) providerKeychainEntry(account).deletePassword(); + if (account && !retained.has(account)) providerKeychainEntry(account).deletePassword(); } return { changed: false, value: true }; }); diff --git a/src/providers/key-store.ts b/src/providers/key-store.ts index 428b018213f..20272343319 100644 --- a/src/providers/key-store.ts +++ b/src/providers/key-store.ts @@ -1,8 +1,10 @@ import { resolveEnvValue, saveConfigPreservingClaudeCode } from "../config"; import type { OcxConfig, OcxProviderConfig } from "../types"; import type { ProviderRegistryEntry } from "./registry"; +import { parseGcpCredentialJson } from "../lib/gcp-adc"; import { KEYCHAIN_REFERENCE_PREFIX, + GCP_CREDENTIAL_MARKER_PREFIX, invalidateResolvedProviderKeyCache, isKeychainReference, keychainAccount, @@ -79,7 +81,7 @@ export function storeProviderKeyInKeychain(config: OcxConfig, name: string): { o const pool = provider.apiKeyPool ?? []; try { for (const entry of pool) { - if (isKeychainReference(entry.key)) continue; + if (entry.key.startsWith(GCP_CREDENTIAL_MARKER_PREFIX) || isKeychainReference(entry.key)) continue; const secret = resolveEnvValue(entry.key); if (!secret) continue; // unresolved env reference stays as-is const account = `${name}/${entry.id}`; @@ -87,7 +89,7 @@ export function storeProviderKeyInKeychain(config: OcxConfig, name: string): { o written.push(account); planned.push(() => { entry.key = `${KEYCHAIN_REFERENCE_PREFIX}${account}`; }); } - if (provider.apiKey && !isKeychainReference(provider.apiKey)) { + if (provider.apiKey && !provider.apiKey.startsWith(GCP_CREDENTIAL_MARKER_PREFIX) && !isKeychainReference(provider.apiKey)) { const active = pool.find(e => e.key === provider.apiKey || (isKeychainReference(e.key) && false)); const secret = resolveEnvValue(provider.apiKey); if (secret) { @@ -138,6 +140,9 @@ export function restoreProviderKeyFromKeychain(config: OcxConfig, name: string): let value: string | null = null; try { value = providerKeychainEntry(account).getPassword(); } catch { value = null; } if (!value) return { ok: false, error: `OS keychain has no readable secret for ${ref}; config left unchanged`, status: 503 }; + if (parseGcpCredentialJson(value)) { + return { ok: false, error: "GCP credentials must remain in the OS store; use a gcp-sa reference. Config left unchanged", status: 400 }; + } resolved.set(account, value); } for (const entry of pool) { diff --git a/structure/providers/google.md b/structure/providers/google.md index 3e28a980afe..7004bd29a71 100644 --- a/structure/providers/google.md +++ b/structure/providers/google.md @@ -13,6 +13,10 @@ credential material. `src/server/management/provider-routes.ts` and `src/cli/pro this admission through `providerCredentialInputError` in `src/providers/api-keys.ts`. `src/providers/gcp-credential-cleanup.ts` cleans removed markers after config persistence, under the config mutation lock, retaining credentials still referenced by any persisted provider. +Reference retention compares OS account identities across both `gcp-sa:` and `keychain:` +spellings. `src/providers/key-store.ts` leaves GCP markers stored during ordinary keychain +store/restore and refuses to restore GCP JSON through a keychain alias; the shared classification +in `src/providers/api-key-resolve.ts` reports those markers as OS-stored credentials. ## Google thought-text visibility boundary diff --git a/tests/server/management-gcp-credential-input.test.ts b/tests/server/management-gcp-credential-input.test.ts index 540dd9398f1..2e7e6f86e7c 100644 --- a/tests/server/management-gcp-credential-input.test.ts +++ b/tests/server/management-gcp-credential-input.test.ts @@ -3,8 +3,8 @@ import { mkdtempSync, readFileSync, readdirSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { saveConfig, loadConfig } from "../../src/config"; -import { addProviderApiKey } from "../../src/providers/api-keys"; -import { setProviderKeychainEntryFactoryForTests } from "../../src/providers/key-store"; +import { addProviderApiKey, removeProviderApiKey } from "../../src/providers/api-keys"; +import { setProviderKeychainEntryFactoryForTests, storeProviderKeyInKeychain, restoreProviderKeyFromKeychain, providerKeyStoreKind } from "../../src/providers/key-store"; import { handleProviderRoutes } from "../../src/server/management/provider-routes"; import { handleProviderCommand } from "../../src/cli/provider"; import * as destinationPolicy from "../../src/lib/destination-policy"; @@ -119,3 +119,43 @@ test("OS-store failures propagate without echoing credential material or committ expect(config).toEqual(before); assertNoPlaintext(); }); + +test("keychain store/restore leaves GCP markers and their stored JSON intact in mixed pools", () => { + expect(addProviderApiKey(config, "relay", credential)).toHaveProperty("id"); + const marker = config.providers.relay!.apiKey!; + const account = marker.slice("gcp-sa:".length); + expect(providerKeyStoreKind(config.providers.relay)).toBe("keychain"); + expect(storeProviderKeyInKeychain(config, "relay")).toEqual({ ok: true, moved: 1 }); + expect(config.providers.relay!.apiKey).toBe(marker); + expect(secrets.get(`opencodex.provider-api-key.v1:${account}`)).toBe(credential); + expect(restoreProviderKeyFromKeychain(config, "relay")).toEqual({ ok: true, restored: 1 }); + expect(config.providers.relay!.apiKey).toBe(marker); + expect(config.providers.relay!.apiKeyPool!.some(row => row.key === marker)).toBe(true); + expect(secrets.get(`opencodex.provider-api-key.v1:${account}`)).toBe(credential); + assertNoPlaintext(); +}); + +test("cleanup retains the same OS account referenced by another provider through keychain syntax", () => { + const result = addProviderApiKey(config, "relay", credential); + if ("error" in result) throw new Error(result.error); + const account = config.providers.relay!.apiKey!.slice("gcp-sa:".length); + config.providers.shared = { adapter: "openai-chat", baseUrl: "https://shared.example/v1", apiKey: `keychain:${account}` }; + saveConfig(config); + expect(removeProviderApiKey(config, "relay", result.id)).toBe(true); + expect(secrets.get(`opencodex.provider-api-key.v1:${account}`)).toBe(credential); + expect(loadConfig().providers.shared!.apiKey).toBe(`keychain:${account}`); + assertNoPlaintext(); +}); + +test("restore never publishes stored credential JSON through a keychain alias", () => { + expect(addProviderApiKey(config, "relay", credential)).toHaveProperty("id"); + const marker = config.providers.relay!.apiKey!; + const account = marker.slice("gcp-sa:".length); + config.providers.relay!.apiKey = `keychain:${account}`; + saveConfig(config); + const before = structuredClone(config); + expect(restoreProviderKeyFromKeychain(config, "relay")).toMatchObject({ ok: false, status: 400 }); + expect(config).toEqual(before); + expect(secrets.get(`opencodex.provider-api-key.v1:${account}`)).toBe(credential); + assertNoPlaintext(); +}); From 8d36e31d4f68c9ee6a5a1ac3fb67f1d7312a6bb0 Mon Sep 17 00:00:00 2001 From: JUN Date: Mon, 12 Oct 2026 05:49:58 +0900 Subject: [PATCH 15/19] fix(vertex): serialize credential writes with marker commits and cleanup --- .../docs/reference/configuration/providers.md | 2 + src/providers/api-keys.ts | 19 +++--- structure/providers/google.md | 4 +- .../management-gcp-credential-input.test.ts | 59 +++++++++++++++++++ 4 files changed, 75 insertions(+), 9 deletions(-) diff --git a/docs-site/src/content/docs/reference/configuration/providers.md b/docs-site/src/content/docs/reference/configuration/providers.md index dca0f083e5b..e69855439b6 100644 --- a/docs-site/src/content/docs/reference/configuration/providers.md +++ b/docs-site/src/content/docs/reference/configuration/providers.md @@ -1428,6 +1428,8 @@ warning requests manual inspection without undoing the committed configuration r Ordinary keychain store/restore actions leave GCP markers and JSON in the OS store. Restoring credential JSON through a `keychain:` alias is refused, and cleanup preserves accounts still referenced through either marker spelling. +Credential writes and marker persistence share the cleanup transaction lock, so concurrent +removal cannot delete the stored JSON between read-back and the marker commit. By default a provider's `apiKey` and `apiKeyPool` sit in `config.json` (mode 0600, atomic writes). If you would rather keep the key material out of the file, move it into the OS credential store: diff --git a/src/providers/api-keys.ts b/src/providers/api-keys.ts index a4171013397..9ad35d5ae32 100644 --- a/src/providers/api-keys.ts +++ b/src/providers/api-keys.ts @@ -247,15 +247,18 @@ export function addProviderApiKey(config: OcxConfig, name: string, key: string, const stored: Array<{ id: string; marker: string }> = []; const created: string[] = []; try { - for (const part of parts) { - const credentialJson = (part as { credentialJson: string }).credentialJson; - const id = apiKeyPoolEntryId(credentialJson); - const account = gcpCredentialStoreAccount(name, id); - const existedBefore = providerKeychainEntry(account).getPassword() !== null; - if (!existedBefore) created.push(`${GCP_CREDENTIAL_MARKER_PREFIX}${account}`); - stored.push({ id, marker: storeGcpCredentialJson(config, name, credentialJson, existedBefore) }); - } const committed = commitProviderApiKeySelection(config, name, fresh => { + // Hold the same mutation lock as cleanup from the verified OS write through marker + // persistence. Re-run idempotent writes on rebase so a removed payload is restored. + stored.length = 0; + for (const part of parts) { + const credentialJson = (part as { credentialJson: string }).credentialJson; + const id = apiKeyPoolEntryId(credentialJson); + const account = gcpCredentialStoreAccount(name, id); + const existedBefore = providerKeychainEntry(account).getPassword() !== null; + if (!existedBefore) created.push(`${GCP_CREDENTIAL_MARKER_PREFIX}${account}`); + stored.push({ id, marker: storeGcpCredentialJson(config, name, credentialJson, existedBefore) }); + } const pool = ensurePool(fresh); for (const { id, marker } of stored) { const existing = pool.find(e => e.id === id); diff --git a/structure/providers/google.md b/structure/providers/google.md index 7004bd29a71..399d9c0f311 100644 --- a/structure/providers/google.md +++ b/structure/providers/google.md @@ -5,7 +5,9 @@ `src/lib/gcp-adc.ts` resolves an explicit `gcp-sa:` through the OS credential store. An empty or whitespace-only account is rejected before token resolution; it never selects host ADC. `src/providers/api-keys.ts` stores pasted credential JSON as markers, including multiple JSON objects -in one atomic pool update. Failed batches remove only newly created, unreferenced secrets. +in one atomic pool update. Verified OS writes and marker persistence share the cleanup mutation +lock. Rebased callbacks repeat idempotent writes before committing; failed batches remove only +newly created, unreferenced secrets. The dedicated key-pool endpoint owns JSON ingestion. Provider POST/create/overwrite and CLI add reject raw JSON in active keys or submitted pool entries before saving; mixed JSON/literal-key bulk pastes are rejected before any store writes. OS-store failures return a fixed error without diff --git a/tests/server/management-gcp-credential-input.test.ts b/tests/server/management-gcp-credential-input.test.ts index 2e7e6f86e7c..833c7104390 100644 --- a/tests/server/management-gcp-credential-input.test.ts +++ b/tests/server/management-gcp-credential-input.test.ts @@ -159,3 +159,62 @@ test("restore never publishes stored credential JSON through a keychain alias", expect(secrets.get(`opencodex.provider-api-key.v1:${account}`)).toBe(credential); assertNoPlaintext(); }); + +test("credential re-add rebases and restores its payload after an overlapping last-reference removal", () => { + const original = addProviderApiKey(config, "relay", credential); + if ("error" in original) throw new Error(original.error); + const marker = config.providers.relay!.apiKey!; + const account = marker.slice("gcp-sa:".length); + let written = false; + let injectRemoval = true; + let removed = false; + setProviderKeychainEntryFactoryForTests((service, key) => ({ + getPassword: () => { + const value = secrets.get(`${service}:${key}`) ?? null; + if (key === account && written && injectRemoval) { + injectRemoval = false; + removed = removeProviderApiKey(loadConfig(), "relay", original.id); + } + return value; + }, + setPassword: value => { secrets.set(`${service}:${key}`, value); if (key === account) written = true; }, + deletePassword: () => secrets.delete(`${service}:${key}`), + })); + expect(addProviderApiKey(config, "relay", credential)).toEqual(original); + expect(removed).toBe(true); + expect(loadConfig().providers.relay!.apiKey).toBe(marker); + expect(secrets.get(`opencodex.provider-api-key.v1:${account}`)).toBe(credential); + assertNoPlaintext(); +}); + +test("a separate process cannot acquire the cleanup transaction during credential write/read-back", () => { + const original = addProviderApiKey(config, "relay", credential); + if ("error" in original) throw new Error(original.error); + const account = config.providers.relay!.apiKey!.slice("gcp-sa:".length); + let checked = false; + let blocked = false; + setProviderKeychainEntryFactoryForTests((service, key) => ({ + getPassword: () => secrets.get(`${service}:${key}`) ?? null, + setPassword: value => { + secrets.set(`${service}:${key}`, value); + if (key !== account || checked) return; + checked = true; + const child = Bun.spawnSync([process.execPath, "-e", ` + import { Database } from "bun:sqlite"; + const db = new Database(process.argv[1]); + let blocked = false; + try { db.exec("PRAGMA busy_timeout = 0; BEGIN IMMEDIATE"); db.exec("ROLLBACK"); } + catch (error) { blocked = error.code === "SQLITE_BUSY"; } + finally { db.close(); } + console.log(JSON.stringify({ blocked })); + `, join(home, "config-mutation.sqlite")], { stdout: "pipe", stderr: "pipe" }); + expect(child.exitCode).toBe(0); + blocked = JSON.parse(child.stdout.toString()).blocked; + }, + deletePassword: () => secrets.delete(`${service}:${key}`), + })); + expect(addProviderApiKey(config, "relay", credential)).toEqual(original); + expect(checked).toBe(true); + expect(blocked).toBe(true); + assertNoPlaintext(); +}); From 686b6ba01458933e56524f8a4fab57178493bf0e Mon Sep 17 00:00:00 2001 From: GSL-R <174104911+GSL-R@users.noreply.github.com> Date: Tue, 13 Oct 2026 01:38:31 +0900 Subject: [PATCH 16/19] fix(cli): clean removed GCP credentials after provider overwrite --- .../docs/reference/configuration/providers.md | 2 +- src/cli/provider.ts | 4 ++ structure/providers/google.md | 4 +- .../management-gcp-credential-input.test.ts | 51 +++++++++++++++++++ 4 files changed, 58 insertions(+), 3 deletions(-) diff --git a/docs-site/src/content/docs/reference/configuration/providers.md b/docs-site/src/content/docs/reference/configuration/providers.md index e69855439b6..7c2f0936b39 100644 --- a/docs-site/src/content/docs/reference/configuration/providers.md +++ b/docs-site/src/content/docs/reference/configuration/providers.md @@ -1422,7 +1422,7 @@ the first becomes active. Configuration contains only `gcp-sa:` referen credential JSON. Mixed JSON/literal-key bulk pastes are rejected as a whole; add literal keys separately. Storage failures leave the configuration unchanged and report a fixed error without echoing credential material. Empty marker accounts are invalid and do not fall back to host credentials. -Removing a key or provider deletes its unreferenced GCP credentials after the configuration is +Removing a key or provider, or replacing a provider with `ocx provider add --force`, deletes its unreferenced GCP credentials after the configuration is saved; a credential referenced by another provider is retained. If OS-store cleanup fails, a warning requests manual inspection without undoing the committed configuration removal. Ordinary keychain store/restore actions leave GCP markers and JSON in the OS store. Restoring diff --git a/src/cli/provider.ts b/src/cli/provider.ts index 1988d3f3397..871ab15eac8 100644 --- a/src/cli/provider.ts +++ b/src/cli/provider.ts @@ -336,12 +336,15 @@ async function handleAdd(args: string[], deps: ProviderCommandDeps): Promise { if (provConfig.anthropicOAuthInstance) { try { assertAnthropicInstanceLoginConfig(config, name); } catch { markerCollision = true; return; } } initializeProviderModelSelection(name, provConfig, existingProvider, config); + const replaced = config.providers[name]; + removedCredentials = replaced ? providerGcpCredentialMarkers(replaced) : []; config.providers[name] = provConfig; if (setDefault) config.defaultProvider = name; validateAndSave(config); @@ -352,6 +355,7 @@ async function handleAdd(args: string[], deps: ProviderCommandDeps): Promise { + expect(addProviderApiKey(config, "relay", credential)).toHaveProperty("id"); + const retainedMarker = config.providers.relay!.apiKey!; + const account = retainedMarker.slice("gcp-sa:".length); + expect(addProviderApiKey(config, "relay", JSON.stringify({ ...JSON.parse(credential), client_id: "second-client" }))).toHaveProperty("id"); + if (shared !== "none") config.providers.shared = { + adapter: "openai-chat", baseUrl: "https://shared.example/v1", apiKey: `${shared}:${account}`, + }; + saveConfig(config); + expect(secrets.size).toBe(2); + let deletions = 0; + setProviderKeychainEntryFactoryForTests((service, key) => ({ + getPassword: () => secrets.get(`${service}:${key}`) ?? null, + setPassword: value => { secrets.set(`${service}:${key}`, value); }, + deletePassword: () => { + expect(loadConfig().providers.relay!.apiKey).toBe("replacement-key"); + deletions += 1; + return secrets.delete(`${service}:${key}`); + }, + })); + spyOn(console, "log").mockImplementation(() => {}); + await handleProviderCommand(overwriteArgs); + expect(loadConfig().providers.relay!.apiKey).toBe("replacement-key"); + expect(deletions).toBe(shared === "none" ? 2 : 1); + expect(secrets.size).toBe(shared === "none" ? 0 : 1); + if (shared !== "none") expect(secrets.get(`opencodex.provider-api-key.v1:${account}`)).toBe(credential); + assertNoPlaintext(); +}); + +test.each(["save failure", "refused paste"])("CLI overwrite preserves credentials on %s", async failure => { + expect(addProviderApiKey(config, "relay", credential)).toHaveProperty("id"); + const before = readFileSync(join(home, "config.json"), "utf8"); + const previousExitCode = process.exitCode; + try { + if (failure === "save failure") { + spyOn(configModule, "saveConfig").mockImplementation(() => { throw new Error("synthetic save failure"); }); + await expect(handleProviderCommand(overwriteArgs)).rejects.toThrow("synthetic save failure"); + } else { + spyOn(console, "error").mockImplementation(() => {}); + await handleProviderCommand([...overwriteArgs.slice(0, -1), credential]); + expect(process.exitCode).toBe(2); + } + expect(readFileSync(join(home, "config.json"), "utf8")).toBe(before); + expect([...secrets.values()]).toEqual([credential]); + assertNoPlaintext(); + } finally { process.exitCode = previousExitCode ?? 0; } +}); + test("keychain store/restore leaves GCP markers and their stored JSON intact in mixed pools", () => { expect(addProviderApiKey(config, "relay", credential)).toHaveProperty("id"); const marker = config.providers.relay!.apiKey!; From c81a6f7f042a6964da9abc425687892c21b34259 Mon Sep 17 00:00:00 2001 From: GSL-R <174104911+GSL-R@users.noreply.github.com> Date: Tue, 13 Oct 2026 02:00:47 +0900 Subject: [PATCH 17/19] fix(cli): collect durable GCP markers under overwrite lock --- src/cli/provider.ts | 8 ++-- structure/providers/google.md | 2 +- .../management-gcp-credential-input.test.ts | 38 +++++++++++++++++++ 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/src/cli/provider.ts b/src/cli/provider.ts index 871ab15eac8..77d5e23d9c1 100644 --- a/src/cli/provider.ts +++ b/src/cli/provider.ts @@ -8,7 +8,7 @@ * show Show provider config details (secrets masked) * set-default Change the default provider */ -import { hasOwnProvider, isValidProviderName, loadConfig, sanitizeModelCostsForDisplay, saveConfig, validateConfigCandidate, withConfigMutationLockSync } from "../config"; +import { hasOwnProvider, isValidProviderName, loadConfig, readConfigAdmissionSnapshot, sanitizeModelCostsForDisplay, saveConfig, validateConfigCandidate, withConfigMutationLockSync } from "../config"; import { apiKeyTransportConfigError, modelCapabilitiesConfigError, mergeModelCapabilities } from "../config/provider-validation"; import { hasHelpFlag, printSubcommandUsage } from "./help"; import { getProviderRegistryEntry, PROVIDER_REGISTRY } from "../providers/registry"; @@ -343,8 +343,10 @@ async function handleAdd(args: string[], deps: ProviderCommandDeps): Promise provider ? providerGcpCredentialMarkers(provider) : []); config.providers[name] = provConfig; if (setDefault) config.defaultProvider = name; validateAndSave(config); diff --git a/structure/providers/google.md b/structure/providers/google.md index 8e06af9338b..869696370c7 100644 --- a/structure/providers/google.md +++ b/structure/providers/google.md @@ -14,7 +14,7 @@ bulk pastes are rejected before any store writes. OS-store failures return a fix credential material. `src/server/management/provider-routes.ts` and `src/cli/provider.ts` enforce this admission through `providerCredentialInputError` in `src/providers/api-keys.ts`. `src/providers/gcp-credential-cleanup.ts` cleans removed markers after config persistence, including CLI `provider add --force` overwrites, under the -config mutation lock, retaining credentials still referenced by any persisted provider. Failed saves and refused CLI additions do not clean credentials. +config mutation lock, retaining credentials still referenced by any persisted provider. CLI overwrite captures both live and latest durable rows under that lock. Failed saves and refused CLI additions do not clean credentials. Reference retention compares OS account identities across both `gcp-sa:` and `keychain:` spellings. `src/providers/key-store.ts` leaves GCP markers stored during ordinary keychain store/restore and refuses to restore GCP JSON through a keychain alias; the shared classification diff --git a/tests/server/management-gcp-credential-input.test.ts b/tests/server/management-gcp-credential-input.test.ts index 154bd7c97c5..4e04a939988 100644 --- a/tests/server/management-gcp-credential-input.test.ts +++ b/tests/server/management-gcp-credential-input.test.ts @@ -123,6 +123,44 @@ test("OS-store failures propagate without echoing credential material or committ const overwriteArgs = ["add", "relay", "--force", "--adapter", "openai-chat", "--base-url", "https://relay.example/v1", "--api-key", "replacement-key"]; +test.each(["none", "gcp-sa", "keychain"])("CLI overwrite includes a newer durable row, retaining %s shared references", async shared => { + expect(addProviderApiKey(config, "relay", credential)).toHaveProperty("id"); + const lateCredential = JSON.stringify({ ...JSON.parse(credential), client_id: "late-client" }); + const lock = configModule.withConfigMutationLockSync; + let wrote = false, lateAccount = ""; + if (shared !== "none") { + const originalRow = structuredClone(config.providers.relay!); + const added = addProviderApiKey(config, "relay", lateCredential); + if ("error" in added) throw new Error(added.error); + lateAccount = config.providers.relay!.apiKeyPool!.find(entry => entry.id === added.id)!.key.slice("gcp-sa:".length); + config.providers.relay = originalRow; + config.providers.shared = { adapter: "openai-chat", baseUrl: "https://shared.example/v1", apiKey: `${shared}:${lateAccount}` }; + saveConfig(config); + } + spyOn(configModule, "withConfigMutationLockSync").mockImplementation((fn: () => T, dir?: string): T => { + if (!wrote) { + wrote = true; + const fresh = loadConfig(); + fresh.providers.relay!.apiKey = undefined; + fresh.providers.relay!.apiKeyPool = undefined; + expect(addProviderApiKey(fresh, "relay", lateCredential)).toHaveProperty("id"); + lateAccount = fresh.providers.relay!.apiKey!.slice("gcp-sa:".length); + saveConfig(fresh); + } + return lock(fn, dir); + }); + spyOn(console, "log").mockImplementation(() => {}); + await handleProviderCommand(overwriteArgs); + expect(wrote).toBe(true); + const saved = loadConfig(); + expect(saved.providers.relay!.apiKey).toBe("replacement-key"); + expect(saved.providers.relay!.apiKeyPool).toBeUndefined(); + if (shared !== "none") expect(saved.providers.shared!.apiKey).toBe(`${shared}:${lateAccount}`); + expect(secrets.size).toBe(shared === "none" ? 0 : 1); + if (shared !== "none") expect(secrets.get(`opencodex.provider-api-key.v1:${lateAccount}`)).toBe(lateCredential); + assertNoPlaintext(); +}); + test.each(["none", "gcp-sa", "keychain"])("CLI overwrite cleans old pool credentials after commit, preserving %s shared references", async shared => { expect(addProviderApiKey(config, "relay", credential)).toHaveProperty("id"); const retainedMarker = config.providers.relay!.apiKey!; From dc6e85b8152350da0620a86f559562bef51d40ee Mon Sep 17 00:00:00 2001 From: GSL-R <174104911+GSL-R@users.noreply.github.com> Date: Tue, 13 Oct 2026 02:08:42 +0900 Subject: [PATCH 18/19] fix(cli): validate raw keys before registry auth ignores them --- src/cli/provider.ts | 6 ++++ structure/providers/google.md | 2 +- .../management-gcp-credential-input.test.ts | 33 +++++++++++++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/cli/provider.ts b/src/cli/provider.ts index 77d5e23d9c1..24cd728b028 100644 --- a/src/cli/provider.ts +++ b/src/cli/provider.ts @@ -225,6 +225,12 @@ async function handleAdd(args: string[], deps: ProviderCommandDeps): Promise [credential, + JSON.stringify({ type: "service_account", client_email: "fixture@example.test", private_key: "synthetic-test-key" }), + JSON.stringify({ type: "external_account", client_secret: "fixture-secret" }), +].map(input => ({ name, input }))))("CLI refuses credential JSON before ignoring registry key (%j)", async ({ name, input }) => { + const before = readFileSync(join(home, "config.json"), "utf8"); + const files = readdirSync(home).filter(file => file.startsWith("config.json")); + const previousExitCode = process.exitCode; + const error = spyOn(console, "error").mockImplementation(() => {}); + const warning = spyOn(console, "warn").mockImplementation(() => {}); + try { + process.exitCode = 0; + await handleProviderCommand(["add", name, "--api-key", input]); + expect(process.exitCode).toBe(2); + expect(error).toHaveBeenCalled(); + expect(error.mock.calls.flat().join(" ")).not.toContain("fixture-secret"); + expect(error.mock.calls.flat().join(" ")).not.toContain("synthetic-test-key"); + expect(warning).not.toHaveBeenCalled(); + expect(readFileSync(join(home, "config.json"), "utf8")).toBe(before); + expect(readdirSync(home).filter(file => file.startsWith("config.json"))).toEqual(files); + expect(secrets.size).toBe(0); + } finally { process.exitCode = previousExitCode ?? 0; } +}); + +test.each(["openai", "cursor"])("CLI still ignores ordinary keys for registry provider %s", async name => { + const warning = spyOn(console, "warn").mockImplementation(() => {}); + spyOn(console, "log").mockImplementation(() => {}); + await handleProviderCommand(["add", name, "--api-key", "synthetic-unused-key"]); + expect(warning.mock.calls.flat().join(" ")).toContain("--api-key is ignored"); + expect(loadConfig().providers[name]).toBeDefined(); + expect(loadConfig().providers[name]!.apiKey).not.toBe("synthetic-unused-key"); + expect(secrets.size).toBe(0); +}); + const overwriteArgs = ["add", "relay", "--force", "--adapter", "openai-chat", "--base-url", "https://relay.example/v1", "--api-key", "replacement-key"]; test.each(["none", "gcp-sa", "keychain"])("CLI overwrite includes a newer durable row, retaining %s shared references", async shared => { From 200e2d94b93b92d2b683d0be63716ff5693dbe7c Mon Sep 17 00:00:00 2001 From: GSL-R <174104911+GSL-R@users.noreply.github.com> Date: Tue, 13 Oct 2026 16:22:28 +0900 Subject: [PATCH 19/19] fix(gcp-adc): isolate blank markers during credential cleanup --- src/providers/gcp-credential-cleanup.ts | 9 ++++- structure/providers-and-adapters.md | 2 +- structure/providers/google.md | 1 + .../management-gcp-credential-input.test.ts | 39 ++++++++++++++++++- 4 files changed, 47 insertions(+), 4 deletions(-) diff --git a/src/providers/gcp-credential-cleanup.ts b/src/providers/gcp-credential-cleanup.ts index a9bdd8c3554..97cae4e15ce 100644 --- a/src/providers/gcp-credential-cleanup.ts +++ b/src/providers/gcp-credential-cleanup.ts @@ -17,6 +17,11 @@ export function snapshotProviderGcpCredentialMarkers(name: string, provider: Ocx return [provider, durableProvider].flatMap(row => row ? providerGcpCredentialMarkers(row) : []); } +function cleanupGcpCredentialAccount(key: string | undefined): string | undefined { + if (key?.startsWith(GCP_CREDENTIAL_MARKER_PREFIX) && !key.slice(GCP_CREDENTIAL_MARKER_PREFIX.length).trim()) return undefined; + return gcpCredentialMarkerAccount(key); +} + /** After a successful config commit, remove only credentials no persisted provider references. */ export function cleanupRemovedGcpCredentials(markers: readonly string[]): void { if (!markers.length) return; @@ -27,11 +32,11 @@ export function cleanupRemovedGcpCredentials(markers: readonly string[]): void { const retained = new Set(Object.values(fresh.providers).flatMap(provider => [provider.apiKey, ...(provider.apiKeyPool ?? []).map(entry => entry.key)] .flatMap(key => { - const account = isKeychainReference(key) ? keychainAccount(key) : gcpCredentialMarkerAccount(key); + const account = isKeychainReference(key) ? keychainAccount(key) : cleanupGcpCredentialAccount(key); return account === undefined ? [] : [account]; }))); for (const marker of new Set(markers)) { - const account = gcpCredentialMarkerAccount(marker); + const account = cleanupGcpCredentialAccount(marker); if (account && !retained.has(account)) providerKeychainEntry(account).deletePassword(); } return { changed: false, value: true }; diff --git a/structure/providers-and-adapters.md b/structure/providers-and-adapters.md index 4757ae5f154..dbe7f85d69f 100644 --- a/structure/providers-and-adapters.md +++ b/structure/providers-and-adapters.md @@ -1,5 +1,5 @@ # Providers And Adapters -GCP JSON storage in `src/providers/api-keys.ts` and durable-row snapshot/post-commit cleanup in `src/providers/gcp-credential-cleanup.ts` follow [Vertex credential markers](providers/google.md#vertex-credential-markers). +GCP JSON storage in `src/providers/api-keys.ts` and durable-row snapshot/post-commit cleanup in `src/providers/gcp-credential-cleanup.ts` follow [Vertex credential markers](providers/google.md#vertex-credential-markers); malformed blank marker accounts do not block cleanup of valid accounts. Configured spend ceilings bind adapter inference executors to the [seed and physical-start contract](transports/responses-spend.md#historical-pool-continuity-and-rollback). Zed completion and auth-recovery replays claim separately; token acquisition stays outside inference accounting. Claude CLI, CodeBuddy and Qoder each count one invocation toward the frozen request limit, seed before spawn and retain a producer through completion. Child-internal retries/tool turns are not separately counted; reported actual usage settles in full, including amounts above the seed estimate. Devin combines only [consecutive same-ID tool results](adapters/registry.md#devin-consecutive-tool-results), using linear accumulation while preserving intervening message slots, image parts and error markers without mutating the parsed request. Anthropic account pause, model routes, quota labels, and the separate `anthropic2` pool (its helpers, quota, reset grants and management surfaces) follow the [Anthropic account-pool contract](providers/anthropic-account-pool.md). Devin Messages follows the [per-turn output ordering contract](clients/claude-desktop.md#devin-messages-output-ordering), preserving late signatures before text/tools without changing Responses or Chat ordering. diff --git a/structure/providers/google.md b/structure/providers/google.md index 537af13cce7..aade9992443 100644 --- a/structure/providers/google.md +++ b/structure/providers/google.md @@ -4,6 +4,7 @@ `src/lib/gcp-adc.ts` resolves an explicit `gcp-sa:` through the OS credential store. An empty or whitespace-only account is rejected before token resolution; it never selects host ADC. +Post-commit cleanup skips empty or whitespace-only GCP marker accounts in both persisted references and captured removal candidates, so one malformed marker cannot block valid-account retention or cleanup. Request-time marker validation remains strict. `src/providers/api-keys.ts` stores pasted credential JSON as markers, including multiple JSON objects in one atomic pool update. Verified OS writes and marker persistence share the cleanup mutation lock. Rebased callbacks repeat idempotent writes before committing; failed batches remove only diff --git a/tests/server/management-gcp-credential-input.test.ts b/tests/server/management-gcp-credential-input.test.ts index 2c32da4196e..12018a08bef 100644 --- a/tests/server/management-gcp-credential-input.test.ts +++ b/tests/server/management-gcp-credential-input.test.ts @@ -5,7 +5,7 @@ import { join } from "node:path"; import { saveConfig, loadConfig } from "../../src/config"; import * as configModule from "../../src/config"; import { addProviderApiKey, removeProviderApiKey } from "../../src/providers/api-keys"; -import { snapshotProviderGcpCredentialMarkers } from "../../src/providers/gcp-credential-cleanup"; +import { cleanupRemovedGcpCredentials, snapshotProviderGcpCredentialMarkers } from "../../src/providers/gcp-credential-cleanup"; import { setProviderKeychainEntryFactoryForTests, storeProviderKeyInKeychain, restoreProviderKeyFromKeychain, providerKeyStoreKind } from "../../src/providers/key-store"; import { handleProviderRoutes } from "../../src/server/management/provider-routes"; import { handleProviderCommand } from "../../src/cli/provider"; @@ -69,6 +69,43 @@ test.each(["", "{not-json"])("management credential snapshot ignores an invalid expect(secrets.size).toBe(0); }); +test.each(["gcp-sa:", "gcp-sa: "])("cleanup skips malformed persisted markers without blocking valid cleanup: %j", malformed => { + expect(addProviderApiKey(config, "relay", credential)).toHaveProperty("id"); + const retained = config.providers.relay!.apiKey!; + config.providers.removed = { adapter: "openai-chat", baseUrl: "https://relay.example/v1" }; + saveConfig(config); + expect(addProviderApiKey(config, "removed", JSON.stringify({ ...JSON.parse(credential), client_id: "removed-client" }))).toHaveProperty("id"); + const removed = config.providers.removed!.apiKey!; + delete config.providers.removed; + config.providers.unrelated = { adapter: "openai-chat", baseUrl: "https://relay.example/v1", apiKey: malformed, + apiKeyPool: [{ id: "bad", key: malformed }] }; + saveConfig(config); + const warn = spyOn(console, "warn").mockImplementation(() => {}); + cleanupRemovedGcpCredentials([removed, retained]); + expect(secrets.size).toBe(1); + expect([...secrets.values()]).toEqual([credential]); + expect(loadConfig().providers.unrelated!.apiKey).toBe(malformed); + expect(warn).not.toHaveBeenCalled(); +}); + +test.each(["gcp-sa:", "gcp-sa: "])("cleanup skips malformed captured markers and preserves keychain aliases: %j", malformed => { + expect(addProviderApiKey(config, "relay", credential)).toHaveProperty("id"); + const retained = config.providers.relay!.apiKey!; + config.providers.removed = { adapter: "openai-chat", baseUrl: "https://relay.example/v1" }; + saveConfig(config); + expect(addProviderApiKey(config, "removed", JSON.stringify({ ...JSON.parse(credential), client_id: "removed-client" }))).toHaveProperty("id"); + const removed = config.providers.removed!.apiKey!; + delete config.providers.removed; + config.providers.relay!.apiKey = `keychain:${retained.slice("gcp-sa:".length)}`; + delete config.providers.relay!.apiKeyPool; + saveConfig(config); + const warn = spyOn(console, "warn").mockImplementation(() => {}); + cleanupRemovedGcpCredentials([malformed, removed, retained]); + expect(secrets.size).toBe(1); + expect([...secrets.values()]).toEqual([credential]); + expect(warn).not.toHaveBeenCalled(); +}); + test.each([ { name: "new-provider", values: { apiKey: credential } }, { name: "relay", values: { apiKey: credential } },