From ebeda4f7f183beb551c66d68128f4998f54054ad Mon Sep 17 00:00:00 2001 From: Austin Kurpuis Date: Tue, 28 Jul 2026 06:24:21 -0700 Subject: [PATCH 1/3] Stop one missed credential write-back from killing a linked account Anthropic ROTATES the refresh token on every refresh, so the instant a run's CLI refreshes, the copy in the store is dead. That makes a missed write-back not a degradation but a KILL: the stored refresh token has been spent, and only a human re-link recovers. `credentialsWriteback.ts` already knew this -- it is the first thing its doc comment says -- but it staked the whole link on a single POST, once, after the turn: - a pod killed, evicted, or hitting activeDeadlineSeconds mid-turn never reached it at all; - it had no retry, and release.yml deploys on every push to main, so a POST arriving during a gateway rollout (ADR 0033 recorded eleven in fourteen hours) simply lost the refresh. One strike, and the user is asked to re-link something they linked hours ago. Observed on PR #170: a run succeeded at 00:38, and the next run 12h later was `authorized` with a credential Anthropic rejected ~20s in -- the orchestrator then correctly invalidated it and logged `verdict:"link-required"`. So persist continuously instead of once. `createCredentialsWritebackWatcher` polls the credentials file during the turn (5s) and POSTs on change, which: - shrinks the exposure window from "the whole turn" to one interval, so a pod that dies mid-turn has already reported the refresh the CLI did seconds in; - IS its own retry -- a tick that fails to store does not advance `lastPersisted`, so the next tick re-sends the same blob; - still flushes at the end, now with bounded retries, for a refresh landing in the last seconds where there is no next tick. `lastPersisted` advances only on a CONFIRMED store, which is what keeps a failure from being mistaken for "already persisted", and re-reads the file after storing so the next comparison is against exactly what was stored. Ticks never stack (`inFlight`), and `stop()` waits out an in-flight tick so the final flush cannot store a blob older than one already sent. When it does run out of attempts it says so loudly, naming the consequence -- the link is stale from that moment and nothing downstream can tell. Also, the diagnostic whose absence made this expensive to find. The decisive question is always "was the credential already dead when we injected it, or did it die during the turn?", and answering it needs the agent pod's logs, which are gone. So the orchestrator's `[authorization]` line -- which outlives the run -- now carries `claudeLoginExpiresAt`, and the agent logs the seeded credential's expiry at startup. Expiry ONLY: `logVerdict` handles the one structure holding every resolved credential for a run, and the new helper is tested to emit no token material. Not fixed here, and the actual cure: every run still rotates a shared refresh token, so concurrent runs for one subject can still invalidate each other. The fix is for the gateway to become the single serialized owner of refresh, handing runs a short-lived credential -- an ADR, not a patch. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NyKcQdvju68R3edo4GXD3T --- .../src/agent/authorization-service.test.ts | 30 +++ .../src/agent/authorization-service.ts | 30 +++ .../src/credentialsWriteback.test.ts | 165 ++++++++++++++++- .../src/credentialsWriteback.ts | 171 ++++++++++++++++++ apps/claude-code-swe-agent/src/index.ts | 32 +++- 5 files changed, 420 insertions(+), 8 deletions(-) diff --git a/apps/agent-orchestrator/src/agent/authorization-service.test.ts b/apps/agent-orchestrator/src/agent/authorization-service.test.ts index c190cda..eab65e9 100644 --- a/apps/agent-orchestrator/src/agent/authorization-service.test.ts +++ b/apps/agent-orchestrator/src/agent/authorization-service.test.ts @@ -386,6 +386,36 @@ describe("AuthorizationService.authorize -- claude-remote write-back grant", () expect(verdict.secretEnv).toContainEqual({ name: "CLAUDE_CREDENTIALS_WRITEBACK_TOKEN", value: "wb-token" }); }); + // The agent's pod and its logs are long gone before anyone asks why a run + // reported an expired credential, so this line is the only durable record of + // whether the credential was already dead at launch (a rotation that went + // unpersisted) or died during the turn. + it("logs the injected claude-remote credential's expiry -- and no token material", async () => { + const expiresAt = Date.UTC(2026, 6, 28, 12, 0, 0); + const blob = JSON.stringify({ claudeAiOauth: { accessToken: "sk-ant-SECRET", refreshToken: "rt-SECRET", expiresAt } }); + const logged: string[] = []; + const spy = vi.spyOn(console, "log").mockImplementation((...args: unknown[]) => { + logged.push(args.map(String).join(" ")); + }); + try { + const svc = new AuthorizationService({ + claudeRemoteGateway: gateway({ "claude-remote": { token: { token: blob } } }), + agentRunTimeoutSeconds: 600, + }); + await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude-remote"] }, + identity: identity({ subject: "openwebui:alice", principal: "github:alice" }), + request: "r", + }); + } finally { + spy.mockRestore(); + } + + const authorized = logged.find((l) => l.includes('"verdict":"authorized"')) ?? ""; + expect(authorized).toContain('"claudeLoginExpiresAt":"2026-07-28T12:00:00.000Z"'); + expect(authorized).not.toContain("SECRET"); + }); + it("launches without write-back rather than failing when no grant is available", async () => { const svc = new AuthorizationService({ claudeRemoteGateway: gateway({ "claude-remote": { token: { token: "{}" } } }), diff --git a/apps/agent-orchestrator/src/agent/authorization-service.ts b/apps/agent-orchestrator/src/agent/authorization-service.ts index 71211b4..59ec747 100644 --- a/apps/agent-orchestrator/src/agent/authorization-service.ts +++ b/apps/agent-orchestrator/src/agent/authorization-service.ts @@ -64,6 +64,29 @@ export const PROVIDER_ENV_VAR: Record = { "claude-remote": "CLAUDE_LOGIN_CREDENTIALS_JSON", }; +/** + * The expiry of the `claude-remote` credentials blob about to be injected, for + * the `authorized` log line -- or `null` when this run carries none, or the + * blob has no readable `expiresAt`. + * + * Expiry ONLY. This function is handed the one structure that holds every + * resolved credential for a run, so it must never return anything derived from + * a token's VALUE (see `logVerdict`'s own warning). + */ +function claudeLoginExpiry(secretEnv: Array<{ name: string; value: string }> | undefined): string | null { + const blob = secretEnv?.find((e) => e.name === PROVIDER_ENV_VAR["claude-remote"])?.value; + if (!blob) return null; + try { + const parsed = JSON.parse(blob) as Record; + const inner = (parsed.claudeAiOauth ?? parsed) as Record; + const raw = inner.expiresAt; + const ms = typeof raw === "number" ? raw : typeof raw === "string" ? Number(raw) : NaN; + return Number.isFinite(ms) ? new Date(ms).toISOString() : null; + } catch { + return null; + } +} + /** * Env vars a `claude-remote` launch carries so the run can persist the * credentials its Claude Code CLI refreshes in-pod (the gateway's @@ -649,6 +672,13 @@ export class AuthorizationService { injecting: (secretEnv ?? []).map((e) => e.name), actorLogin: actorLogin ?? null, principal, + // Expiry ONLY, of the Remote Control credential this run is being handed. + // The agent's own pod (and its logs) are gone long before anyone asks why + // a run reported an expired credential, so this is the one durable record + // of whether it was already dead at launch or died during the turn -- + // which is the difference between a rotation that went unpersisted and a + // credential resolved from the wrong record. + claudeLoginExpiresAt: claudeLoginExpiry(secretEnv), }); return { kind: "authorized", diff --git a/apps/claude-code-swe-agent/src/credentialsWriteback.test.ts b/apps/claude-code-swe-agent/src/credentialsWriteback.test.ts index ba96957..0ed925d 100644 --- a/apps/claude-code-swe-agent/src/credentialsWriteback.test.ts +++ b/apps/claude-code-swe-agent/src/credentialsWriteback.test.ts @@ -2,7 +2,7 @@ import { mkdtempSync, mkdirSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { describe, expect, it, vi } from "vitest"; -import { persistRefreshedCredentials } from "./credentialsWriteback.js"; +import { createCredentialsWritebackWatcher, credentialExpiry, persistRefreshedCredentials } from "./credentialsWriteback.js"; function homeWithCredentials(contents: string | null): string { const home = mkdtempSync(join(tmpdir(), "swe-writeback-")); @@ -104,3 +104,166 @@ describe("persistRefreshedCredentials", () => { expect(fetchImpl).not.toHaveBeenCalled(); }); }); + +describe("credentialExpiry", () => { + it("reads expiresAt from a real credentials shape, and never returns token material", () => { + const at = Date.UTC(2026, 6, 28, 12, 0, 0); + const blob = JSON.stringify({ claudeAiOauth: { accessToken: "secret-token", refreshToken: "r", expiresAt: at } }); + const expiry = credentialExpiry(blob); + expect(expiry).toBe("2026-07-28T12:00:00.000Z"); + expect(expiry).not.toContain("secret-token"); + }); + + it("returns null for junk rather than throwing (it only ever feeds a log line)", () => { + expect(credentialExpiry("not json")).toBeNull(); + expect(credentialExpiry("{}")).toBeNull(); + expect(credentialExpiry(JSON.stringify({ claudeAiOauth: { expiresAt: "nope" } }))).toBeNull(); + }); +}); + +/** + * The watcher exists because a refresh ROTATES the stored refresh token, so a + * refresh this pod fails to report does not degrade the link -- it kills it, + * and only a human re-link recovers. Every test here is a way that used to + * happen. + */ +describe("createCredentialsWritebackWatcher", () => { + const settle = (ms: number): Promise => new Promise((r) => setTimeout(r, ms)); + + it("persists a refresh that happens DURING the turn, without waiting for the turn to end", async () => { + const home = homeWithCredentials(SEEDED); + const fetchImpl = vi.fn().mockResolvedValue({ ok: true, status: 200 } as Response); + const watcher = createCredentialsWritebackWatcher({ + homeDir: home, + url: "https://gateway.example/refresh", + token: "grant-abc", + seeded: SEEDED, + intervalMs: 10, + fetchImpl: fetchImpl as unknown as typeof fetch, + log: () => {}, + }); + + watcher.start(); + // The CLI refreshes mid-turn. A pod killed right here used to lose this. + writeFileSync(join(home, ".claude", ".credentials.json"), REFRESHED); + await settle(60); + + expect(fetchImpl).toHaveBeenCalled(); + expect(JSON.parse(String((fetchImpl.mock.calls[0] as [string, RequestInit])[1].body))).toEqual({ + credentialsJson: REFRESHED, + }); + await watcher.stop(); + }); + + it("stores each refresh once, not on every tick", async () => { + const home = homeWithCredentials(REFRESHED); + const fetchImpl = vi.fn().mockResolvedValue({ ok: true, status: 200 } as Response); + const watcher = createCredentialsWritebackWatcher({ + homeDir: home, + url: "https://gateway.example/refresh", + token: "grant-abc", + seeded: SEEDED, + intervalMs: 10, + fetchImpl: fetchImpl as unknown as typeof fetch, + log: () => {}, + }); + + watcher.start(); + await settle(80); // many ticks + await watcher.stop(); + + // One store for the one refresh: `lastPersisted` advanced, so later ticks + // (and the final flush) see an unchanged file. + expect(fetchImpl).toHaveBeenCalledTimes(1); + }); + + // The gateway restarts on every push to main (release.yml), and a POST that + // landed in that gap used to lose the refresh permanently. + it("keeps retrying a failing gateway on later ticks, and stores once it recovers", async () => { + const home = homeWithCredentials(REFRESHED); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce({ ok: false, status: 503 } as Response) + .mockResolvedValueOnce({ ok: false, status: 503 } as Response) + .mockResolvedValue({ ok: true, status: 200 } as Response); + const watcher = createCredentialsWritebackWatcher({ + homeDir: home, + url: "https://gateway.example/refresh", + token: "grant-abc", + seeded: SEEDED, + intervalMs: 10, + fetchImpl: fetchImpl as unknown as typeof fetch, + log: () => {}, + }); + + watcher.start(); + await settle(120); + const outcome = await watcher.stop(); + + expect(fetchImpl.mock.calls.length).toBeGreaterThanOrEqual(3); + // The blob still got stored -- the whole point. + expect(outcome === "stored" || outcome === "unchanged").toBe(true); + }); + + it("retries the final flush, so a refresh in the turn's last seconds is not lost to one bad POST", async () => { + const home = homeWithCredentials(REFRESHED); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce({ ok: false, status: 502 } as Response) + .mockResolvedValue({ ok: true, status: 200 } as Response); + const watcher = createCredentialsWritebackWatcher({ + homeDir: home, + url: "https://gateway.example/refresh", + token: "grant-abc", + seeded: SEEDED, + fetchImpl: fetchImpl as unknown as typeof fetch, + flushAttempts: 3, + flushRetryDelayMs: 5, + log: () => {}, + }); + + // Never started: this is purely the end-of-turn path. + const outcome = await watcher.stop(); + + expect(outcome).toBe("stored"); + expect(fetchImpl).toHaveBeenCalledTimes(2); + }); + + it("says loudly when it gives up, because the link is dead at that point", async () => { + const logs: string[] = []; + const watcher = createCredentialsWritebackWatcher({ + homeDir: homeWithCredentials(REFRESHED), + url: "https://gateway.example/refresh", + token: "grant-abc", + seeded: SEEDED, + fetchImpl: vi.fn().mockResolvedValue({ ok: false, status: 500 } as Response) as unknown as typeof fetch, + flushAttempts: 2, + flushRetryDelayMs: 1, + log: (m) => logs.push(m), + }); + + const outcome = await watcher.stop(); + + expect(outcome).toBe("failed"); + expect(logs.join("\n")).toMatch(/GAVE UP/); + expect(logs.join("\n")).toMatch(/need re-linking/); + }); + + it("is inert when no grant was injected (a non-claude-remote run)", async () => { + const fetchImpl = vi.fn(); + const watcher = createCredentialsWritebackWatcher({ + homeDir: homeWithCredentials(REFRESHED), + url: "", + token: "", + seeded: SEEDED, + intervalMs: 5, + fetchImpl: fetchImpl as unknown as typeof fetch, + log: () => {}, + }); + + watcher.start(); + await settle(30); + expect(await watcher.stop()).toBe("disabled"); + expect(fetchImpl).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/claude-code-swe-agent/src/credentialsWriteback.ts b/apps/claude-code-swe-agent/src/credentialsWriteback.ts index 4cb8b56..46513ec 100644 --- a/apps/claude-code-swe-agent/src/credentialsWriteback.ts +++ b/apps/claude-code-swe-agent/src/credentialsWriteback.ts @@ -47,6 +47,41 @@ export type CredentialsWritebackOutcome = | "stored" | "failed"; +/** + * Reads just the expiry out of a credentials blob, for logging. Never returns + * or logs token material -- the point is to make "was this credential already + * dead when we injected it?" answerable from a log line, which is the question + * that has repeatedly been unanswerable because the agent pod (and its logs) + * are gone by the time anyone looks. + */ +export function credentialExpiry(blob: string): string | null { + try { + const parsed = JSON.parse(blob) as Record; + const inner = (parsed.claudeAiOauth ?? parsed) as Record; + const raw = inner.expiresAt; + const ms = typeof raw === "number" ? raw : typeof raw === "string" ? Number(raw) : NaN; + if (!Number.isFinite(ms)) return null; + return new Date(ms).toISOString(); + } catch { + return null; + } +} + +/** Outcomes where trying again could plausibly succeed. */ +function isRetryable(outcome: CredentialsWritebackOutcome): boolean { + // `failed` is the gateway being unreachable or erroring -- the case worth + // retrying, and the case that loses credentials today. `malformed` is a read + // that raced the CLI's in-place rewrite, so a re-read is likely to be clean. + return outcome === "failed" || outcome === "malformed"; +} + +function sleep(ms: number): Promise { + return new Promise((resolve) => { + const timer = setTimeout(resolve, ms); + timer.unref?.(); + }); +} + export async function persistRefreshedCredentials( opts: CredentialsWritebackOptions, ): Promise { @@ -93,3 +128,139 @@ export async function persistRefreshedCredentials( return "failed"; } } + +/** Default cadence for re-checking the credentials file during a turn. */ +const DEFAULT_WATCH_INTERVAL_MS = 5_000; +/** Attempts for the final flush, which has no next tick to retry on. */ +const DEFAULT_FLUSH_ATTEMPTS = 4; +/** Delay between those attempts -- long enough to outlast a gateway rollout's gap. */ +const DEFAULT_FLUSH_RETRY_DELAY_MS = 3_000; + +export interface CredentialsWritebackWatcher { + /** Starts polling. Cheap no-op when write-back is disabled. */ + start(): void; + /** + * Stops polling and makes a final, retried attempt to persist anything the + * CLI wrote since the last successful store. Safe to call more than once. + */ + stop(): Promise; +} + +export interface CredentialsWritebackWatcherOptions extends CredentialsWritebackOptions { + /** Poll cadence; injectable for tests. */ + intervalMs?: number; + /** Attempts for the final flush; injectable for tests. */ + flushAttempts?: number; + /** Delay between final-flush attempts; injectable for tests. */ + flushRetryDelayMs?: number; + /** Injectable clock/logger seam for tests. */ + log?: (message: string) => void; +} + +/** + * Persists the run's credentials file CONTINUOUSLY, not once at the end. + * + * Why this is not just a nicety: Anthropic ROTATES the refresh token on every + * refresh, so the moment a run's CLI refreshes, the copy in the store is dead. + * A single missed write-back therefore does not degrade the link, it KILLS it + * -- the stored refresh token has been spent and only a human re-link can + * recover. A write-back that ran once, at the end of the turn, staked the whole + * link on that one POST landing: + * + * - a pod killed, evicted, or hitting `activeDeadlineSeconds` mid-turn never + * reached it at all; + * - it had no retry, and `release.yml` deploys on every push to `main`, so a + * POST arriving during a gateway rollout (ADR 0033 recorded eleven in + * fourteen hours) simply lost the refresh. + * + * Polling shrinks the exposure window from "the whole turn" to `intervalMs`, + * and makes the loop its own retry: a tick that fails to store does not advance + * `lastPersisted`, so the next tick tries the same blob again. The final + * `stop()` flush covers a refresh that lands in the last few seconds, where + * there is no next tick. + */ +export function createCredentialsWritebackWatcher( + opts: CredentialsWritebackWatcherOptions, +): CredentialsWritebackWatcher { + const log = opts.log ?? ((m: string) => console.error(m)); + const enabled = Boolean(opts.url && opts.token); + const intervalMs = opts.intervalMs ?? DEFAULT_WATCH_INTERVAL_MS; + + // Advances only on a CONFIRMED store, so any failure is retried rather than + // being mistaken for "already persisted". + let lastPersisted = opts.seeded; + let timer: ReturnType | undefined; + let inFlight = false; + let stopped = false; + + const attemptOnce = async (): Promise => { + const outcome = await persistRefreshedCredentials({ ...opts, seeded: lastPersisted }); + if (outcome === "stored") { + // Re-read rather than trusting the file to have held still: `seeded` for + // the next comparison must be exactly what we just stored, and + // `persistRefreshedCredentials` is what read it. + try { + lastPersisted = await readFile(join(opts.homeDir, ".claude", ".credentials.json"), "utf8"); + } catch { + // Unreadable now; the next tick's compare against the old value will + // simply try again, which is the safe direction. + } + log( + `[claude-code-swe-agent] credential write-back: stored a refreshed credential (expires ${credentialExpiry(lastPersisted) ?? "unknown"})`, + ); + } else if (isRetryable(outcome)) { + log(`[claude-code-swe-agent] credential write-back: ${outcome} -- will retry`); + } + return outcome; + }; + + const tick = async (): Promise => { + // A slow POST must not have a second one stacked behind it; the next tick + // will pick up whatever this one leaves unpersisted. + if (inFlight || stopped) return; + inFlight = true; + try { + await attemptOnce(); + } finally { + inFlight = false; + } + }; + + return { + start(): void { + if (!enabled || timer) return; + timer = setInterval(() => void tick(), intervalMs); + // Never let the watcher hold the process open past the turn. + timer.unref?.(); + }, + async stop(): Promise { + if (timer) { + clearInterval(timer); + timer = undefined; + } + if (!enabled) return "disabled"; + stopped = true; + // Let an in-flight tick settle so it cannot race the final flush and + // store a blob older than the one it already sent. + while (inFlight) await sleep(10); + stopped = false; + + const attempts = opts.flushAttempts ?? DEFAULT_FLUSH_ATTEMPTS; + const delayMs = opts.flushRetryDelayMs ?? DEFAULT_FLUSH_RETRY_DELAY_MS; + let outcome: CredentialsWritebackOutcome = "unchanged"; + for (let attempt = 1; attempt <= attempts; attempt += 1) { + outcome = await attemptOnce(); + if (!isRetryable(outcome)) return outcome; + if (attempt < attempts) await sleep(delayMs); + } + // Out of attempts on a retryable failure. Say so loudly: the stored + // credential is now dead (its refresh token was spent in-pod) and the + // next run will ask the human to re-link. + log( + `[claude-code-swe-agent] credential write-back GAVE UP after ${attempts} attempts (${outcome}); ` + + `the stored credential's refresh token was rotated in this pod, so the link is now stale and will need re-linking`, + ); + return outcome; + }, + }; +} diff --git a/apps/claude-code-swe-agent/src/index.ts b/apps/claude-code-swe-agent/src/index.ts index cac1cd6..e241d74 100644 --- a/apps/claude-code-swe-agent/src/index.ts +++ b/apps/claude-code-swe-agent/src/index.ts @@ -19,7 +19,7 @@ import { import { extractContinuationToken } from "./continuation.js"; import { decodeSweContinuation, encodeSweContinuation, type SweMarker } from "./marker.js"; import { loadToolConfig } from "./config.js"; -import { persistRefreshedCredentials } from "./credentialsWriteback.js"; +import { createCredentialsWritebackWatcher, credentialExpiry } from "./credentialsWriteback.js"; import { AuthorizationError, finalizeDelegatedWrite, isDelegating, resolveDelegatedToken } from "./identityDelegation.js"; import { clip } from "./security/redact.js"; @@ -61,6 +61,15 @@ async function handler(session: AgentSession): Promise { "the Remote Control turn will likely fail to authenticate unless the init container seeds it before this point.", ); } + // State the expiry of the credential this run was handed, so "was it + // already dead when we injected it?" is answerable from the log instead of + // being reconstructed from when a comment appeared. Expiry only -- never + // any token material. + if (toolConfig.loginCredentialsJson) { + const expiry = credentialExpiry(toolConfig.loginCredentialsJson); + const relative = expiry ? `${Math.round((Date.parse(expiry) - Date.now()) / 60_000)} min from now` : "unknown"; + console.error(`[claude-code-swe-agent] seeded Remote Control credential expires ${expiry ?? "unknown"} (${relative})`); + } // `claude --bg` combined with bypassPermissions refuses to start // ("requires accepting the disclaimer first") unless @@ -194,6 +203,19 @@ async function handler(session: AgentSession): Promise { signal: session.signal, onProgress: (message: string, stage: string) => void session.progress(clip(message, 500), { stage }), }; + // Persist any credential refresh DURING the turn, not only after it. The + // refresh rotates the stored refresh token, so a refresh this pod fails to + // report kills the link outright -- see ./credentialsWriteback.ts. Started + // before the turn so a pod killed mid-turn has already reported whatever the + // CLI refreshed seconds into it. + const writebackWatcher = createCredentialsWritebackWatcher({ + homeDir: toolConfig.homeDir, + url: toolConfig.credentialsWritebackUrl, + token: toolConfig.credentialsWritebackToken, + seeded: toolConfig.loginCredentialsJson, + }); + writebackWatcher.start(); + const outcome = toolConfig.remoteControlEnabled ? await runClaudeTurnRemoteControlled(prompt, { ...runOpts, @@ -208,12 +230,8 @@ async function handler(session: AgentSession): Promise { // Runs on every outcome, including a failed one: the CLI may well have // refreshed its credential before whatever went wrong, and that refresh // invalidated the stored copy either way (see ./credentialsWriteback.ts). - const writeback = await persistRefreshedCredentials({ - homeDir: toolConfig.homeDir, - url: toolConfig.credentialsWritebackUrl, - token: toolConfig.credentialsWritebackToken, - seeded: toolConfig.loginCredentialsJson, - }); + // Retried internally, so a gateway mid-rollout does not cost the link. + const writeback = await writebackWatcher.stop(); if (writeback !== "disabled" && writeback !== "unchanged") { console.error(`[claude-code-swe-agent] credential write-back: ${writeback}`); } From 454fa0eedac1f58a9ec15c52a3a5ef8ef4d848aa Mon Sep 17 00:00:00 2001 From: Austin Kurpuis Date: Tue, 28 Jul 2026 06:59:01 -0700 Subject: [PATCH 2/3] Refresh a stored Remote Control credential in the gateway, before serving it The previous commit made a run's own refresh survive the run. It did not make anything REFRESH -- it only persisted refreshes that a pod happened to perform, which left two holes it could not reach from inside the agent: - two concurrent runs for one subject each refresh from the same stored token, and whichever lands second invalidates the first; - nothing refreshes between runs at all, and a refresh token is not valid forever. The CLI itself names that failure: `refresh_token_expired` and `refresh_token_expires_in` are both strings in the shipped binary. An unexercised link is a link on a timer. Both are fixed by moving refresh to the one component that is single, durable, and able to serialize per subject. `ClaudeCredentialRefresher.ensureFresh` now runs on the `login` read path -- `GET /claude-auth/api/token?mode=login` and `/api/wait`, which is exactly what agent-orchestrator resolves a launch credential through -- so a run is handed a freshly minted access token instead of whatever was left in the store. Serialization is the point, not an optimization: concurrent callers share one in-flight promise per subject, so two simultaneous launches cost one refresh instead of two that rotate each other out. On the endpoint, and on what is actually verified. `POST https://platform.claude.com/v1/oauth/token` is confirmed present in the shipped CLI (v2.1.220), and the client id is lifted from the authorize URL this gateway already hands users, so both are observed rather than assumed. The exact body the service expects is NOT something this repo can prove from outside, and the one thing that could make this worse is a refresh the service ACCEPTS whose result we then lose. So the code is built around that: - anything but a 2xx carrying a usable access_token/expires_in is inert -- `transient`, stored credential untouched, caller served exactly what it would have been served before, retry later; - a 200 we cannot parse is transient too, never "refreshed": claiming success would overwrite a working credential with a broken one; - `invalid_grant`/`refresh_token_expired` is `rejected` and reported, but the record is deliberately NOT deleted -- the orchestrator's re-auth path owns invalidation, and an endpoint hiccup that merely looked like a rejection must not cost a working link; - on success the new blob is persisted and read back BEFORE returning, and if it will not persist we say so loudly and still hand back the live blob, because at that moment it is the only living copy of the credential; - a response that omits a new refresh token carries the old one forward rather than writing `undefined` into the file, and every field the file carried that this code has no business understanding is preserved, so runs get a blob shaped like the one `claude auth login` produces. `GATEWAY_CLAUDE_CREDENTIAL_REFRESH=false` turns it off entirely, and `..._MARGIN_MS` tunes how close to expiry counts as due (30 min default). A blob with no readable expiry is treated as due: an unknown expiry is much more likely to be an expired credential than a healthy one, and a needless refresh costs a round trip while a skipped one costs a re-link. NOT covered, deliberately: a link that goes completely unused still ages out, because refresh-on-read only fires when something reads. A background sweep needs to enumerate stored subjects, and `SecretRecordStore` is documented as having "no listing, no label-selector scan, no O(keyspace) anything" -- so that is a design decision to take on purpose, not to smuggle in here. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NyKcQdvju68R3edo4GXD3T --- .../src/claude-auth/api.ts | 40 ++- .../claude-auth/credential-refresher.test.ts | 246 +++++++++++++ .../src/claude-auth/credential-refresher.ts | 325 ++++++++++++++++++ apps/integration-gateway/src/config.ts | 12 + apps/integration-gateway/src/index.ts | 2 + apps/integration-gateway/src/server.ts | 20 ++ 6 files changed, 643 insertions(+), 2 deletions(-) create mode 100644 apps/integration-gateway/src/claude-auth/credential-refresher.test.ts create mode 100644 apps/integration-gateway/src/claude-auth/credential-refresher.ts diff --git a/apps/integration-gateway/src/claude-auth/api.ts b/apps/integration-gateway/src/claude-auth/api.ts index 09fab1e..e34d76f 100644 --- a/apps/integration-gateway/src/claude-auth/api.ts +++ b/apps/integration-gateway/src/claude-auth/api.ts @@ -103,8 +103,32 @@ export class ClaudeAuthApi { * this is left undefined, never a silent fallback to `setup-token`. */ private readonly loginFlows?: ClaudeLoginFlows, + /** + * Refreshes a stored `login` credential that is at or near expiry before + * it is handed out (see credential-refresher.ts). Optional and additive: + * left undefined, every read below serves the stored blob verbatim, which + * is the behaviour that predates it. + */ + private readonly refresher?: { ensureFresh(subject: string): Promise }, ) {} + /** + * The `login` blob to serve for `subject` -- refreshed first when a refresher + * is configured. Only ever called on the `login` path: a `setup-token` is a + * long-lived API token with no refresh grant, so there is nothing to renew. + * + * Falls back to the stored value if the refresher yields nothing, so a + * refresher fault can never turn an existing link into a 404. + */ + private async freshLoginBlob(subject: string, stored: string | undefined): Promise { + if (!this.refresher) return stored; + try { + return (await this.refresher.ensureFresh(subject)) ?? stored; + } catch { + return stored; + } + } + /** Picks the flows engine for `mode`, or `undefined` if that mode isn't wired up (e.g. `login` before `loginFlows` is configured). */ private flowsFor(mode: ClaudeAuthMode): ClaudeAuthFlows | undefined { return mode === "login" ? this.loginFlows : this.flows; @@ -412,7 +436,13 @@ export class ClaudeAuthApi { sendJson(res, 200, { status: "timeout" }); return; } - sendJson(res, 200, mode === "login" ? { status: "complete", credentialsJson: record.credentialsJson } : { status: "complete", token: record.token }); + sendJson( + res, + 200, + mode === "login" + ? { status: "complete", credentialsJson: await this.freshLoginBlob(subject, record.credentialsJson) } + : { status: "complete", token: record.token }, + ); } private async handleToken(res: ServerResponse, url: URL): Promise { @@ -427,6 +457,12 @@ export class ClaudeAuthApi { res.writeHead(404).end(); return; } - sendJson(res, 200, mode === "login" ? { credentialsJson: record.credentialsJson } : { token: record.token }); + sendJson( + res, + 200, + mode === "login" + ? { credentialsJson: await this.freshLoginBlob(subject, record.credentialsJson) } + : { token: record.token }, + ); } } diff --git a/apps/integration-gateway/src/claude-auth/credential-refresher.test.ts b/apps/integration-gateway/src/claude-auth/credential-refresher.test.ts new file mode 100644 index 0000000..3952b3a --- /dev/null +++ b/apps/integration-gateway/src/claude-auth/credential-refresher.test.ts @@ -0,0 +1,246 @@ +import { describe, expect, it, vi } from "vitest"; +import { + ClaudeCredentialRefresher, + CLAUDE_OAUTH_CLIENT_ID, + CLAUDE_TOKEN_URL, + credentialExpiresAt, + needsRefresh, + refreshCredentialsBlob, +} from "./credential-refresher.js"; +import type { ClaudeTokenRecord } from "./store.js"; + +const HOUR = 3_600_000; + +function blob(overrides: Record = {}): string { + return JSON.stringify({ + claudeAiOauth: { + accessToken: "at-old", + refreshToken: "rt-old", + expiresAt: Date.now() + 8 * HOUR, + scopes: ["user:inference"], + subscriptionType: "max", + ...overrides, + }, + }); +} + +function okResponse(body: Record): Response { + return { ok: true, status: 200, json: async () => body } as unknown as Response; +} + +function errResponse(status: number, text = ""): Response { + return { ok: false, status, text: async () => text } as unknown as Response; +} + +/** In-memory `login`-only store with the read-back behaviour the real one has. */ +function fakeStore(initial?: string) { + const records = new Map(); + if (initial !== undefined) records.set("github:alice", { kind: "login", credentialsJson: initial, createdAt: "t0" }); + return { + records, + get: vi.fn(async (subject: string) => records.get(subject)), + set: vi.fn(async (subject: string, record: ClaudeTokenRecord) => { + records.set(subject, record); + }), + }; +} + +describe("needsRefresh / credentialExpiresAt", () => { + it("leaves a comfortably valid credential alone", () => { + expect(needsRefresh(blob({ expiresAt: Date.now() + 8 * HOUR }))).toBe(false); + }); + + it("refreshes inside the margin, and when already expired", () => { + expect(needsRefresh(blob({ expiresAt: Date.now() + 60_000 }))).toBe(true); + expect(needsRefresh(blob({ expiresAt: Date.now() - HOUR }))).toBe(true); + }); + + // An unknown expiry is far more likely to be an expired credential than a + // healthy one, and a needless refresh costs a round trip while a skipped one + // costs the user a re-link. + it("treats a blob with no readable expiry as due", () => { + expect(needsRefresh(JSON.stringify({ claudeAiOauth: { refreshToken: "rt" } }))).toBe(true); + }); + + it("reports expiry without exposing token material", () => { + const at = Date.UTC(2026, 6, 28, 12, 0, 0); + const expiry = credentialExpiresAt(blob({ expiresAt: at })); + expect(expiry).toBe("2026-07-28T12:00:00.000Z"); + expect(expiry).not.toContain("rt-old"); + }); +}); + +describe("refreshCredentialsBlob", () => { + it("posts the standard refresh grant and merges the new tokens, preserving unrelated fields", async () => { + const fetchImpl = vi.fn().mockResolvedValue( + okResponse({ access_token: "at-new", refresh_token: "rt-new", expires_in: 28800, scope: "user:inference user:profile" }), + ); + const outcome = await refreshCredentialsBlob(blob(), { fetchImpl: fetchImpl as unknown as typeof fetch, now: () => 1_000 }); + + expect(outcome.status).toBe("refreshed"); + const [url, init] = fetchImpl.mock.calls[0] as [string, RequestInit]; + expect(url).toBe(CLAUDE_TOKEN_URL); + const body = new URLSearchParams(String(init.body)); + expect(body.get("grant_type")).toBe("refresh_token"); + expect(body.get("refresh_token")).toBe("rt-old"); + expect(body.get("client_id")).toBe(CLAUDE_OAUTH_CLIENT_ID); + + const merged = JSON.parse(outcome.credentialsJson ?? "{}").claudeAiOauth; + expect(merged.accessToken).toBe("at-new"); + expect(merged.refreshToken).toBe("rt-new"); + expect(merged.expiresAt).toBe(1_000 + 28800 * 1000); + expect(merged.scopes).toEqual(["user:inference", "user:profile"]); + // Fields this code has no business understanding must survive, or runs get + // a blob subtly unlike the one `claude auth login` produces. + expect(merged.subscriptionType).toBe("max"); + }); + + it("carries the old refresh token forward when the response omits a new one", async () => { + const fetchImpl = vi.fn().mockResolvedValue(okResponse({ access_token: "at-new", expires_in: 3600 })); + const outcome = await refreshCredentialsBlob(blob(), { fetchImpl: fetchImpl as unknown as typeof fetch }); + expect(JSON.parse(outcome.credentialsJson ?? "{}").claudeAiOauth.refreshToken).toBe("rt-old"); + }); + + it("calls a spent/expired refresh token a rejection -- retrying cannot help", async () => { + const fetchImpl = vi.fn().mockResolvedValue(errResponse(400, '{"error":"invalid_grant"}')); + expect((await refreshCredentialsBlob(blob(), { fetchImpl: fetchImpl as unknown as typeof fetch })).status).toBe("rejected"); + }); + + it("calls anything else transient, so the stored credential is left alone and retried later", async () => { + for (const res of [errResponse(503), errResponse(500), errResponse(400, '{"error":"server_error"}')]) { + const fetchImpl = vi.fn().mockResolvedValue(res); + expect((await refreshCredentialsBlob(blob(), { fetchImpl: fetchImpl as unknown as typeof fetch })).status).toBe("transient"); + } + const throwing = vi.fn().mockRejectedValue(new Error("ECONNREFUSED")); + expect((await refreshCredentialsBlob(blob(), { fetchImpl: throwing as unknown as typeof fetch })).status).toBe("transient"); + }); + + // A 200 we cannot use must NOT be treated as a refresh: claiming success here + // would overwrite a working credential with a broken one. + it("treats a 200 with no usable tokens as transient, not refreshed", async () => { + const fetchImpl = vi.fn().mockResolvedValue(okResponse({ hello: "world" })); + expect((await refreshCredentialsBlob(blob(), { fetchImpl: fetchImpl as unknown as typeof fetch })).status).toBe("transient"); + }); + + it("does not attempt a blob with no refresh token", async () => { + const fetchImpl = vi.fn(); + const outcome = await refreshCredentialsBlob(JSON.stringify({ claudeAiOauth: { accessToken: "a" } }), { + fetchImpl: fetchImpl as unknown as typeof fetch, + }); + expect(outcome.status).toBe("unrefreshable"); + expect(fetchImpl).not.toHaveBeenCalled(); + }); +}); + +describe("ClaudeCredentialRefresher.ensureFresh", () => { + it("refreshes a near-expiry credential and persists it before returning", async () => { + const store = fakeStore(blob({ expiresAt: Date.now() + 60_000 })); + const fetchImpl = vi.fn().mockResolvedValue(okResponse({ access_token: "at-new", refresh_token: "rt-new", expires_in: 28800 })); + const refresher = new ClaudeCredentialRefresher({ + store, + fetchImpl: fetchImpl as unknown as typeof fetch, + log: () => {}, + }); + + const served = await refresher.ensureFresh("github:alice"); + + expect(JSON.parse(served ?? "{}").claudeAiOauth.accessToken).toBe("at-new"); + // Persisted, because the old refresh token is dead the instant the service + // answered -- this blob is now the only living copy. + expect(store.set).toHaveBeenCalledTimes(1); + expect(JSON.parse(store.records.get("github:alice")?.credentialsJson ?? "{}").claudeAiOauth.refreshToken).toBe("rt-new"); + }); + + it("serves a healthy credential untouched, without calling the token endpoint", async () => { + const store = fakeStore(blob({ expiresAt: Date.now() + 8 * HOUR })); + const fetchImpl = vi.fn(); + const refresher = new ClaudeCredentialRefresher({ store, fetchImpl: fetchImpl as unknown as typeof fetch, log: () => {} }); + + await refresher.ensureFresh("github:alice"); + + expect(fetchImpl).not.toHaveBeenCalled(); + expect(store.set).not.toHaveBeenCalled(); + }); + + // Two concurrent launches for one subject would otherwise each spend the same + // refresh token, and whichever landed second would invalidate the first -- + // the exact rotation race that makes runs fragile, reproduced in the gateway. + it("collapses concurrent callers onto ONE refresh", async () => { + const store = fakeStore(blob({ expiresAt: Date.now() + 60_000 })); + let resolveFetch: ((r: Response) => void) | undefined; + const fetchImpl = vi.fn().mockImplementation(() => new Promise((r) => (resolveFetch = r))); + const refresher = new ClaudeCredentialRefresher({ store, fetchImpl: fetchImpl as unknown as typeof fetch, log: () => {} }); + + const both = Promise.all([refresher.ensureFresh("github:alice"), refresher.ensureFresh("github:alice")]); + await new Promise((r) => setTimeout(r, 5)); + resolveFetch?.(okResponse({ access_token: "at-new", refresh_token: "rt-new", expires_in: 28800 })); + const [a, b] = await both; + + expect(fetchImpl).toHaveBeenCalledTimes(1); + expect(a).toBe(b); + }); + + it("serves the stored credential unchanged when a refresh fails -- never worse off than before", async () => { + const original = blob({ expiresAt: Date.now() + 60_000 }); + const store = fakeStore(original); + const refresher = new ClaudeCredentialRefresher({ + store, + fetchImpl: vi.fn().mockResolvedValue(errResponse(503)) as unknown as typeof fetch, + log: () => {}, + }); + + expect(await refresher.ensureFresh("github:alice")).toBe(original); + expect(store.set).not.toHaveBeenCalled(); + }); + + // Deleting here would let a refresh-endpoint hiccup that merely looked like a + // rejection cost a working link; the orchestrator's re-auth path owns + // invalidation. + it("does not delete a rejected credential -- it reports and leaves it", async () => { + const original = blob({ expiresAt: Date.now() - HOUR }); + const store = fakeStore(original); + const logs: string[] = []; + const refresher = new ClaudeCredentialRefresher({ + store, + fetchImpl: vi.fn().mockResolvedValue(errResponse(400, '{"error":"invalid_grant"}')) as unknown as typeof fetch, + log: (m) => logs.push(m), + }); + + expect(await refresher.ensureFresh("github:alice")).toBe(original); + expect(store.records.has("github:alice")).toBe(true); + expect(logs.join("\n")).toMatch(/rejected/); + }); + + // `set` swallows its own storage errors by design, so a silent drop would be + // indistinguishable from success -- and it is the one outcome that destroys + // the link, because the old refresh token is already spent. + it("shouts, and still serves the live blob, when the refreshed credential will not persist", async () => { + const store = fakeStore(blob({ expiresAt: Date.now() + 60_000 })); + store.set = vi.fn(async () => { + /* silently drops it, exactly as the real store can */ + }); + const logs: string[] = []; + const refresher = new ClaudeCredentialRefresher({ + store, + fetchImpl: vi + .fn() + .mockResolvedValue(okResponse({ access_token: "at-new", refresh_token: "rt-new", expires_in: 28800 })) as unknown as typeof fetch, + log: (m) => logs.push(m), + }); + + const served = await refresher.ensureFresh("github:alice"); + + expect(JSON.parse(served ?? "{}").claudeAiOauth.accessToken).toBe("at-new"); + expect(logs.join("\n")).toMatch(/STORED-BUT-UNVERIFIED/); + expect(logs.join("\n")).toMatch(/need re-linking/); + }); + + it("is inert when disabled, and for a subject with no credential", async () => { + const original = blob({ expiresAt: Date.now() - HOUR }); + const off = new ClaudeCredentialRefresher({ store: fakeStore(original), enabled: false, fetchImpl: vi.fn() as unknown as typeof fetch, log: () => {} }); + expect(await off.ensureFresh("github:alice")).toBe(original); + + const empty = new ClaudeCredentialRefresher({ store: fakeStore(), fetchImpl: vi.fn() as unknown as typeof fetch, log: () => {} }); + expect(await empty.ensureFresh("github:nobody")).toBeUndefined(); + }); +}); diff --git a/apps/integration-gateway/src/claude-auth/credential-refresher.ts b/apps/integration-gateway/src/claude-auth/credential-refresher.ts new file mode 100644 index 0000000..03430c8 --- /dev/null +++ b/apps/integration-gateway/src/claude-auth/credential-refresher.ts @@ -0,0 +1,325 @@ +import type { ClaudeTokenStore } from "./store.js"; + +/** + * Refreshing a stored `login` (Remote Control) credential here, in the gateway, + * rather than leaving it to whatever run happens to pick it up next. + * + * Why this has to exist at all. Anthropic ROTATES the refresh token on every + * refresh, and until now the only thing that ever refreshed was a run's own + * in-pod CLI. That gave the stored credential two ways to die, and the store + * was a passive bystander for both: + * + * 1. a refresh the pod failed to report back killed the stored copy (its + * refresh token had been spent) -- narrowed by the continuous write-back + * in claude-code-swe-agent's `credentialsWriteback.ts`, but still only + * narrowed, because two concurrent runs for one subject can each refresh + * from the same stored token and invalidate the other; + * 2. nothing refreshed it between runs, so a link nobody exercised aged out + * on its own. The CLI itself knows this failure has a name -- + * `refresh_token_expired` and `refresh_token_expires_in` are both strings + * in the shipped binary -- so a refresh token is NOT indefinitely valid + * and an idle link is a link on a timer. + * + * Doing it here fixes both, because the gateway is the one place that is + * single, durable, and able to serialize per subject. + * + * ONE INVARIANT DOMINATES THIS FILE: the instant Anthropic returns new tokens, + * the old refresh token is dead. From that moment the new blob is the only + * living copy of the credential, so it must be persisted (or, failing that, + * still handed to the caller and complained about loudly) rather than dropped. + * Every error path below is written so that a refresh we did not complete + * leaves the stored credential exactly as it was. + */ + +/** Claude Code's OAuth token endpoint. Confirmed present in the shipped CLI binary (v2.1.220). */ +export const CLAUDE_TOKEN_URL = "https://platform.claude.com/v1/oauth/token"; + +/** + * Claude Code's public OAuth client id. Taken from the authorize URL this + * gateway itself hands users (see the `client_id` in the link it posts), so it + * is observed rather than assumed. + */ +export const CLAUDE_OAUTH_CLIENT_ID = "9d1c250a-e61b-44d9-88ed-5944d1962f5e"; + +/** Refresh when the access token expires within this long. */ +export const DEFAULT_REFRESH_MARGIN_MS = 30 * 60_000; + +/** + * The refresh outcome, kept coarse on purpose -- callers only ever need to know + * "is there a newer blob to use" and "did the credential just become + * unrecoverable". + */ +export type RefreshStatus = + | "refreshed" + /** Still comfortably valid; nothing was sent. */ + | "not-needed" + /** No refresh token in the blob, or the blob is not a credentials file. */ + | "unrefreshable" + /** The service rejected the refresh token itself -- the link is dead and needs a human. */ + | "rejected" + /** Network/5xx/unparseable -- the stored credential is untouched and retrying later is right. */ + | "transient"; + +export interface RefreshOutcome { + status: RefreshStatus; + /** Present only on `"refreshed"`: the new blob, which is now the ONLY live copy. */ + credentialsJson?: string; + /** Present only on `"refreshed"`: the new expiry, for logging. */ + expiresAt?: string; + detail?: string; +} + +interface OauthFields { + refreshToken?: string; + expiresAt?: number; +} + +/** The credentials file nests everything under `claudeAiOauth`; tolerate a flat blob too. */ +function readOauth(blob: string): { root: Record; inner: Record } | null { + try { + const root = JSON.parse(blob) as Record; + if (typeof root !== "object" || root === null) return null; + const nested = root.claudeAiOauth; + const inner = typeof nested === "object" && nested !== null ? (nested as Record) : root; + return { root, inner }; + } catch { + return null; + } +} + +function fields(inner: Record): OauthFields { + const rawExpiry = inner.expiresAt; + const expiresAt = + typeof rawExpiry === "number" ? rawExpiry : typeof rawExpiry === "string" ? Number(rawExpiry) : Number.NaN; + return { + refreshToken: typeof inner.refreshToken === "string" && inner.refreshToken ? inner.refreshToken : undefined, + expiresAt: Number.isFinite(expiresAt) ? expiresAt : undefined, + }; +} + +/** The blob's access-token expiry as an ISO string, or null. Never returns token material. */ +export function credentialExpiresAt(blob: string): string | null { + const parsed = readOauth(blob); + if (!parsed) return null; + const { expiresAt } = fields(parsed.inner); + return expiresAt === undefined ? null : new Date(expiresAt).toISOString(); +} + +/** + * True when the access token is inside `marginMs` of expiry (or already past + * it). A blob with no readable expiry is treated as due: an unknown expiry is + * far more likely to be an expired credential than a healthy one, and a + * needless refresh costs one round trip while a skipped one costs a re-link. + */ +export function needsRefresh(blob: string, marginMs = DEFAULT_REFRESH_MARGIN_MS, now = Date.now()): boolean { + const parsed = readOauth(blob); + if (!parsed) return false; // not a credentials file at all -- nothing to do + const { expiresAt } = fields(parsed.inner); + if (expiresAt === undefined) return true; + return expiresAt - now <= marginMs; +} + +/** + * Rebuilds the credentials blob with the new tokens, PRESERVING every other + * field. The file carries things this code has no business understanding + * (subscription type, scopes, account hints); dropping them would hand runs a + * blob subtly unlike the one `claude auth login` produces. + */ +function mergeRefreshed( + blob: string, + next: { accessToken: string; refreshToken: string; expiresAt: number; scopes?: string[] }, +): string { + const parsed = readOauth(blob); + const inner = parsed ? { ...parsed.inner } : {}; + inner.accessToken = next.accessToken; + inner.refreshToken = next.refreshToken; + inner.expiresAt = next.expiresAt; + if (next.scopes && next.scopes.length > 0) inner.scopes = next.scopes; + // Keep the nesting the file actually had. + if (parsed && parsed.root.claudeAiOauth !== undefined) { + return JSON.stringify({ ...parsed.root, claudeAiOauth: inner }); + } + return JSON.stringify(inner); +} + +export interface RefreshDeps { + fetchImpl?: typeof fetch; + tokenUrl?: string; + clientId?: string; + marginMs?: number; + now?: () => number; +} + +/** + * Exchanges the blob's refresh token for a new pair. + * + * The request is the standard OAuth refresh grant against the endpoint and + * client id above. Both of those are observed (binary / our own authorize URL), + * but the exact body this service expects is NOT something this repo can prove + * from the outside, so every non-success path is deliberately inert: on + * anything other than a 2xx carrying a parseable token pair, the caller keeps + * using the credential it already had and the run behaves exactly as it does + * today. The only way this can make things worse is if the service accepts a + * refresh and we then lose the result -- which is why `ClaudeCredentialRefresher` + * persists before returning and shouts if it cannot. + */ +export async function refreshCredentialsBlob(blob: string, deps: RefreshDeps = {}): Promise { + const parsed = readOauth(blob); + if (!parsed) return { status: "unrefreshable", detail: "not a credentials file" }; + const { refreshToken } = fields(parsed.inner); + if (!refreshToken) return { status: "unrefreshable", detail: "blob carries no refreshToken" }; + + const fetchImpl = deps.fetchImpl ?? fetch; + const now = deps.now ?? Date.now; + let res: Response; + try { + res = await fetchImpl(deps.tokenUrl ?? CLAUDE_TOKEN_URL, { + method: "POST", + headers: { "content-type": "application/x-www-form-urlencoded", accept: "application/json" }, + body: new URLSearchParams({ + grant_type: "refresh_token", + refresh_token: refreshToken, + client_id: deps.clientId ?? CLAUDE_OAUTH_CLIENT_ID, + }).toString(), + }); + } catch (err) { + return { status: "transient", detail: err instanceof Error ? err.message : String(err) }; + } + + if (!res.ok) { + let body = ""; + try { + body = (await res.text()).slice(0, 300); + } catch { + /* body is a nicety here, not a requirement */ + } + // `invalid_grant` is the service saying the refresh token is spent or + // expired -- the one case where trying again later cannot help, and the + // case a human has to resolve by re-linking. + const rejected = res.status === 400 && /invalid_grant|refresh_token_expired/i.test(body); + return { status: rejected ? "rejected" : "transient", detail: `HTTP ${res.status}${body ? `: ${body}` : ""}` }; + } + + let payload: Record; + try { + payload = (await res.json()) as Record; + } catch (err) { + return { status: "transient", detail: `unparseable response: ${err instanceof Error ? err.message : String(err)}` }; + } + + const accessToken = typeof payload.access_token === "string" ? payload.access_token : ""; + // A rotating server returns a new refresh token; a non-rotating one omits it + // and the old one stays valid. Carry the old one forward in that case rather + // than writing `undefined` into the file. + const nextRefresh = typeof payload.refresh_token === "string" && payload.refresh_token ? payload.refresh_token : refreshToken; + const expiresIn = typeof payload.expires_in === "number" ? payload.expires_in : Number(payload.expires_in); + if (!accessToken || !Number.isFinite(expiresIn)) { + return { status: "transient", detail: "response carried no usable access_token/expires_in" }; + } + + const expiresAt = now() + Math.floor(expiresIn) * 1000; + const scopes = + typeof payload.scope === "string" && payload.scope.trim() ? payload.scope.trim().split(/\s+/) : undefined; + return { + status: "refreshed", + credentialsJson: mergeRefreshed(blob, { accessToken, refreshToken: nextRefresh, expiresAt, scopes }), + expiresAt: new Date(expiresAt).toISOString(), + }; +} + +export interface ClaudeCredentialRefresherDeps extends RefreshDeps { + store: Pick; + log?: (message: string) => void; + /** Set false to leave stored credentials strictly alone (an operational off switch). */ + enabled?: boolean; +} + +/** + * Keeps one subject's stored `login` credential fresh, serialized per subject. + * + * The serialization is the point, not an optimization: two callers refreshing + * the same credential concurrently would each spend the same refresh token, and + * whichever landed second would invalidate the first -- reproducing, inside the + * gateway, exactly the rotation race that made runs fragile. Sharing one + * in-flight promise per subject makes concurrent launches cost one refresh. + */ +export class ClaudeCredentialRefresher { + private readonly inFlight = new Map>(); + private readonly log: (message: string) => void; + + constructor(private readonly deps: ClaudeCredentialRefresherDeps) { + this.log = deps.log ?? ((m) => console.log(m)); + } + + /** + * Returns the freshest `login` blob for `subject`, refreshing and persisting + * first when it is at or near expiry. Never throws and never returns + * undefined for a credential that exists -- a failed refresh yields the blob + * that was already stored, so the caller is never worse off than before. + */ + async ensureFresh(subject: string): Promise { + const existing = await this.deps.store.get(subject, "login"); + const blob = existing?.credentialsJson; + if (!blob) return undefined; + if (this.deps.enabled === false) return blob; + if (!needsRefresh(blob, this.deps.marginMs ?? DEFAULT_REFRESH_MARGIN_MS, (this.deps.now ?? Date.now)())) { + return blob; + } + + const pending = this.inFlight.get(subject); + if (pending) return pending; + + const attempt = this.refreshAndStore(subject, blob).finally(() => this.inFlight.delete(subject)); + this.inFlight.set(subject, attempt); + return attempt; + } + + private async refreshAndStore(subject: string, blob: string): Promise { + const outcome = await refreshCredentialsBlob(blob, this.deps); + + if (outcome.status !== "refreshed" || !outcome.credentialsJson) { + // Nothing was rotated, so the stored credential is still whatever it was. + // `rejected` is worth saying out loud (a human will have to re-link) but + // deleting the record is deliberately NOT done here: the orchestrator's + // existing re-auth path owns invalidation, and a refresh endpoint hiccup + // that merely looked like a rejection must not cost a working link. + this.log( + `[claude-refresh] ${outcome.status} for ${subject}${outcome.detail ? `: ${outcome.detail}` : ""}; serving the stored credential unchanged`, + ); + return blob; + } + + // From here the OLD refresh token is dead and this blob is the only living + // copy of the credential. Persisting it is not bookkeeping, it is the + // difference between a durable link and a re-link prompt. + try { + await this.deps.store.set(subject, { + kind: "login", + credentialsJson: outcome.credentialsJson, + createdAt: new Date((this.deps.now ?? Date.now)()).toISOString(), + }); + const readBack = await this.deps.store.get(subject, "login"); + if (readBack?.credentialsJson !== outcome.credentialsJson) { + // `set` swallows its own storage errors by design, so a silent drop + // would otherwise be indistinguishable from success -- and it is the + // one outcome that destroys the link. + this.log( + `[claude-refresh] STORED-BUT-UNVERIFIED for ${subject}: the refreshed credential did not read back. ` + + `The previous refresh token is already spent, so the stored credential is now stale and this link will need re-linking.`, + ); + return outcome.credentialsJson; + } + } catch (err) { + this.log( + `[claude-refresh] FAILED TO PERSIST refreshed credential for ${subject} (${err instanceof Error ? err.message : String(err)}). ` + + `The previous refresh token is already spent, so this link will need re-linking.`, + ); + // Still hand back the live blob: this caller can at least succeed, and + // the run's own write-back may yet persist it. + return outcome.credentialsJson; + } + + this.log(`[claude-refresh] refreshed ${subject}'s Remote Control credential (expires ${outcome.expiresAt})`); + return outcome.credentialsJson; + } +} diff --git a/apps/integration-gateway/src/config.ts b/apps/integration-gateway/src/config.ts index 8dc4f7c..0900353 100644 --- a/apps/integration-gateway/src/config.ts +++ b/apps/integration-gateway/src/config.ts @@ -95,6 +95,16 @@ export interface AppConfig { * outlives the spec that caused it and starves whatever triggers next. */ resumeWaitMs: number; + /** + * Off switch for refreshing a stored Remote Control (`login`) credential + * before serving it (claude-auth/credential-refresher.ts). On by default; + * `false` restores the behaviour where only a run's own in-pod CLI ever + * refreshed, which made a link's survival depend on every pod reporting its + * refresh back. + */ + claudeCredentialRefreshEnabled: boolean; + /** How close to expiry a stored credential must be before it is refreshed on read. */ + claudeCredentialRefreshMarginMs: number; /** Public GitHub App client id used to start OAuth Device Flow links (not a secret). */ githubAppClientId: string; /** Base64 (or hex) 32-byte AES-256-GCM key used to encrypt linked GitHub tokens at rest. */ @@ -202,6 +212,8 @@ export function loadConfig(env: NodeJS.ProcessEnv = process.env): AppConfig { pollIntervalMs: num(env.GATEWAY_POLL_INTERVAL_MS, 3_000), pollTimeoutMs: num(env.GATEWAY_POLL_TIMEOUT_MS, 15 * 60 * 1000), resumeWaitMs: num(env.GATEWAY_RESUME_WAIT_MS, 10 * 60 * 1000), + claudeCredentialRefreshEnabled: env.GATEWAY_CLAUDE_CREDENTIAL_REFRESH !== "false", + claudeCredentialRefreshMarginMs: num(env.GATEWAY_CLAUDE_CREDENTIAL_REFRESH_MARGIN_MS, 30 * 60 * 1000), githubAppClientId: env.GITHUB_APP_CLIENT_ID ?? "", identityLinkEncryptionKey: env.IDENTITY_LINK_ENCRYPTION_KEY ?? "", identityLinkToken: env.GATEWAY_IDENTITY_LINK_TOKEN ?? "", diff --git a/apps/integration-gateway/src/index.ts b/apps/integration-gateway/src/index.ts index 1c04d04..0a18889 100644 --- a/apps/integration-gateway/src/index.ts +++ b/apps/integration-gateway/src/index.ts @@ -282,6 +282,8 @@ async function main(): Promise { ...(claudeAuthFlows && claudeTokenStore ? { claudeAuthFlows, claudeAuthStore: claudeTokenStore } : {}), ...(claudeLoginFlows ? { claudeLoginFlows } : {}), resumeWaitMs: config.resumeWaitMs, + claudeCredentialRefreshEnabled: config.claudeCredentialRefreshEnabled, + claudeCredentialRefreshMarginMs: config.claudeCredentialRefreshMarginMs, }); await server.listen(config.httpPort); diff --git a/apps/integration-gateway/src/server.ts b/apps/integration-gateway/src/server.ts index 104693c..004b5b6 100644 --- a/apps/integration-gateway/src/server.ts +++ b/apps/integration-gateway/src/server.ts @@ -8,6 +8,7 @@ import type { ClaudeLoginFlows } from "./claude-auth/pty-login.js"; import type { ClaudeTokenStore } from "./claude-auth/store.js"; import type { IdentityResolver } from "./identity.js"; import type { OrchestratorClient, OrchestratorInvokeResult } from "./orchestrator-client.js"; +import { ClaudeCredentialRefresher } from "./claude-auth/credential-refresher.js"; import { renderSessionPage } from "./session-page.js"; import type { SessionPageStore } from "./session-page-store.js"; import { parseGithubEvent, verifyGithubSignature, WebhookAuthError } from "./webhooks/github.js"; @@ -89,6 +90,15 @@ export interface GatewayServerOptions { * make this reachable once a caller sets it. */ claudeLoginFlows?: ClaudeLoginFlows; + /** + * Off switch for refreshing stored `login` credentials before serving them + * (credential-refresher.ts). Defaults to on. Turning it off restores the + * behaviour where only a run's own CLI ever refreshes -- which is what made + * a link depend on every pod reporting back. + */ + claudeCredentialRefreshEnabled?: boolean; + /** How close to expiry a stored credential must be before it is refreshed on read. */ + claudeCredentialRefreshMarginMs?: number; /** * Session-page feature (issue #81) -- both fields must be set together to * enable it: a "starting work" comment posted right when an @@ -137,6 +147,16 @@ export class GatewayServer { options.identityLinkToken, options.publicBaseUrl, options.claudeLoginFlows, + // Refresh a near-expiry `login` credential before handing it to a + // run, in ONE serialized place. Previously the only thing that ever + // refreshed was a run's own in-pod CLI, which made the stored copy + // dependent on that pod reporting back, and left an unexercised + // link to age out on its own. + new ClaudeCredentialRefresher({ + store: options.claudeAuthStore, + enabled: options.claudeCredentialRefreshEnabled ?? true, + ...(options.claudeCredentialRefreshMarginMs ? { marginMs: options.claudeCredentialRefreshMarginMs } : {}), + }), ) : undefined; } From 770a82a498bc687a3be71a90ddda7d17499e8405 Mon Sep 17 00:00:00 2001 From: Austin Kurpuis Date: Tue, 28 Jul 2026 07:09:58 -0700 Subject: [PATCH 3/3] Renew stored credentials on a schedule, not only when something reads one Refresh-on-read cannot keep an UNUSED link alive: it fires only when something reads, and a refresh token is not valid forever -- the CLI ships `refresh_token_expired` and `refresh_token_expires_in` as strings, so an idle link is on a timer whether or not we act. A user who links today and triggers nothing for long enough is still asked to link again, for no reason they can see. `ClaudeCredentialSweeper` runs in the gateway (the only long-lived single-instance component holding the store) and renews every `login` credential inside a wide margin, hourly by default, with a pass at startup so a deployment neither waits an interval to begin nor waits one to discover the sweep is broken. It refreshes through `ClaudeCredentialRefresher.ensureFresh` rather than directly, so a sweep landing next to a launch collapses onto that subject's single in-flight refresh instead of the two of them rotating each other's token out. Sequential, because there is no hurry and a scheduled burst of token requests is a worse neighbour than a slow loop. The margin is deliberately wider than the on-read one (4h vs 30m): the sweep's job is that a link is never FOUND dead, while the read path's job is only that the blob it is about to hand out works. This needs the one thing the store was built not to do. `SecretRecordStore`'s naming section says "no listing, no label-selector scan, no O(keyspace) anything", and `listRecords` is an explicit exception to it, documented as maintenance-only. The rule is about the READ path, where an exact `get` by computed name is both possible and required; a sweep has no key to compute a name from, so enumerating IS the task -- and `commonLabels` already existed "so an operator -- and a cleanup sweep -- can find them all". The scan is label-selector scoped to one record type, and a record whose plaintext key was never stored is skipped rather than guessed at, since the object name is a one-way hash and acting on an unattributable credential is worse than leaving it alone. Two failure modes are handled as loud rather than quiet, because both would otherwise look exactly like a healthy sweep right up until someone's link expired: a store that cannot enumerate at all makes the sweeper stand down with a message instead of silently never running, and `listSubjects` deliberately does NOT swallow its error the way `get`/`set`/`delete` do -- a list that failed is not an empty store, and reading it as one is how a sweep does nothing forever. No new RBAC: the gateway's Role already grants `list` on Secrets (the API server requires it for the `watch` the pending-link resume uses). Its comment now says the sweep uses it directly too. Tunable via GATEWAY_CLAUDE_CREDENTIAL_SWEEP_INTERVAL_MS / _SWEEP_MARGIN_MS, and disabled wholesale by GATEWAY_CLAUDE_CREDENTIAL_REFRESH=false alongside the read-path refresh. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NyKcQdvju68R3edo4GXD3T --- .../claude-auth/credential-refresher.test.ts | 130 ++++++++++++++++++ .../src/claude-auth/credential-refresher.ts | 118 ++++++++++++++++ .../src/claude-auth/k8s-secret-store.ts | 13 ++ .../src/claude-auth/store.ts | 12 ++ apps/integration-gateway/src/config.ts | 10 ++ .../__fixtures__/fake-secret-api.ts | 25 +++- .../secret-record-store.test.ts | 66 +++++++++ .../credential-store/secret-record-store.ts | 53 +++++++ apps/integration-gateway/src/index.ts | 2 + apps/integration-gateway/src/server.ts | 40 +++++- .../integration-gateway/templates/rbac.yaml | 5 +- 11 files changed, 465 insertions(+), 9 deletions(-) diff --git a/apps/integration-gateway/src/claude-auth/credential-refresher.test.ts b/apps/integration-gateway/src/claude-auth/credential-refresher.test.ts index 3952b3a..4ebf418 100644 --- a/apps/integration-gateway/src/claude-auth/credential-refresher.test.ts +++ b/apps/integration-gateway/src/claude-auth/credential-refresher.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it, vi } from "vitest"; import { + ClaudeCredentialSweeper, ClaudeCredentialRefresher, CLAUDE_OAUTH_CLIENT_ID, CLAUDE_TOKEN_URL, @@ -244,3 +245,132 @@ describe("ClaudeCredentialRefresher.ensureFresh", () => { expect(await empty.ensureFresh("github:nobody")).toBeUndefined(); }); }); + +/** + * The sweep is what keeps an UNUSED link alive. Refresh-on-read cannot: it only + * fires when something reads, and a refresh token is not valid forever (the CLI + * ships `refresh_token_expired`), so a user who links today and triggers nothing + * for long enough would be asked to link again for no visible reason. + */ +describe("ClaudeCredentialSweeper", () => { + function sweepStore(entries: Record) { + const records = new Map( + Object.entries(entries).map(([s, b]) => [s, { kind: "login" as const, credentialsJson: b, createdAt: "t0" }]), + ); + return { + records, + get: vi.fn(async (subject: string) => records.get(subject)), + listSubjects: vi.fn(async () => [...records.keys()]), + }; + } + + it("renews a credential nothing is reading, and leaves healthy ones alone", async () => { + const store = sweepStore({ + "github:stale": blob({ expiresAt: Date.now() + HOUR }), + "github:healthy": blob({ expiresAt: Date.now() + 40 * HOUR }), + }); + const ensureFresh = vi.fn(async () => blob({ expiresAt: Date.now() + 40 * HOUR })); + const sweeper = new ClaudeCredentialSweeper({ + store, + refresher: { ensureFresh }, + marginMs: 4 * HOUR, + log: () => {}, + }); + + const result = await sweeper.sweep(); + + expect(ensureFresh).toHaveBeenCalledTimes(1); + expect(ensureFresh).toHaveBeenCalledWith("github:stale"); + expect(result).toMatchObject({ examined: 2, refreshed: 1 }); + }); + + // The sweep refreshes through the SAME serialized entry point a launch uses, + // so a sweep landing next to a launch costs one refresh, not two that rotate + // each other's token out. + it("renews through the refresher, never by refreshing directly", async () => { + const store = sweepStore({ "github:stale": blob({ expiresAt: Date.now() + HOUR }) }); + const fetchImpl = vi.fn(); + const refresher = new ClaudeCredentialRefresher({ + store: { get: store.get, set: vi.fn() }, + fetchImpl: fetchImpl as unknown as typeof fetch, + log: () => {}, + }); + const spy = vi.spyOn(refresher, "ensureFresh"); + await new ClaudeCredentialSweeper({ store, refresher, marginMs: 4 * HOUR, log: () => {} }).sweep(); + expect(spy).toHaveBeenCalledWith("github:stale"); + }); + + // A list that failed is not an empty store. Reading it as one is how a sweep + // does nothing forever while looking perfectly healthy. + it("reports a failed listing loudly instead of treating it as 'no links'", async () => { + const logs: string[] = []; + const sweeper = new ClaudeCredentialSweeper({ + store: { + get: vi.fn(), + listSubjects: vi.fn(async () => { + throw new Error("API server unreachable"); + }), + }, + refresher: { ensureFresh: vi.fn() }, + log: (m) => logs.push(m), + }); + + const result = await sweeper.sweep(); + + expect(result).toMatchObject({ examined: 0, refreshed: 0, failed: 1 }); + expect(logs.join("\n")).toMatch(/could not list stored credentials/); + expect(logs.join("\n")).toMatch(/API server unreachable/); + }); + + it("stands down audibly when the store cannot enumerate at all", () => { + const logs: string[] = []; + const sweeper = new ClaudeCredentialSweeper({ + store: { get: vi.fn() }, // no listSubjects + refresher: { ensureFresh: vi.fn() }, + log: (m) => logs.push(m), + }); + + sweeper.start(); + + expect(logs.join("\n")).toMatch(/sweep disabled/); + sweeper.stop(); + }); + + it("sweeps once at startup rather than waiting out a full interval", async () => { + const store = sweepStore({ "github:stale": blob({ expiresAt: Date.now() + HOUR }) }); + const ensureFresh = vi.fn(async () => blob({ expiresAt: Date.now() + 40 * HOUR })); + const sweeper = new ClaudeCredentialSweeper({ + store, + refresher: { ensureFresh }, + intervalMs: 60_000, + marginMs: 4 * HOUR, + log: () => {}, + }); + + sweeper.start(); + await new Promise((r) => setTimeout(r, 10)); + sweeper.stop(); + + expect(ensureFresh).toHaveBeenCalledTimes(1); + }); + + it("does not overlap passes", async () => { + const store = sweepStore({ "github:stale": blob({ expiresAt: Date.now() + HOUR }) }); + let release: (() => void) | undefined; + const ensureFresh = vi.fn( + () => new Promise((r) => (release = () => r(blob({ expiresAt: Date.now() + 40 * HOUR })))), + ); + const sweeper = new ClaudeCredentialSweeper({ store, refresher: { ensureFresh }, marginMs: 4 * HOUR, log: () => {} }); + + const first = sweeper.sweep(); + // Wait until the first pass is genuinely mid-refresh before overlapping it. + while (!release) await new Promise((r) => setTimeout(r, 1)); + + const second = await sweeper.sweep(); // while the first is still in flight + expect(second).toMatchObject({ examined: 0 }); + + release(); + await first; + expect(ensureFresh).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/integration-gateway/src/claude-auth/credential-refresher.ts b/apps/integration-gateway/src/claude-auth/credential-refresher.ts index 03430c8..64c4cb2 100644 --- a/apps/integration-gateway/src/claude-auth/credential-refresher.ts +++ b/apps/integration-gateway/src/claude-auth/credential-refresher.ts @@ -323,3 +323,121 @@ export class ClaudeCredentialRefresher { return outcome.credentialsJson; } } + +/** How often the background sweep runs. */ +export const DEFAULT_SWEEP_INTERVAL_MS = 60 * 60_000; + +/** + * How close to expiry a credential must be for the SWEEP to renew it. + * + * Wider than the on-read margin on purpose. The sweep's job is that a link is + * never found dead, so it should act well before anything needs the credential; + * the read path's job is only that the blob it is about to hand out is usable. + */ +export const DEFAULT_SWEEP_MARGIN_MS = 4 * 60 * 60_000; + +export interface CredentialSweeperDeps { + store: Pick; + /** Refreshes one subject -- the same serialized path the read path uses. */ + refresher: Pick; + intervalMs?: number; + marginMs?: number; + log?: (message: string) => void; + now?: () => number; +} + +/** + * Renews stored Remote Control credentials on a timer, independently of whether + * anything is using them. + * + * Refresh-on-read cannot cover this: it only fires when something reads, so a + * link nobody exercises is a link nothing renews. And a refresh token is not + * valid forever -- the CLI ships `refresh_token_expired` and + * `refresh_token_expires_in` as strings, so an idle link is on a timer whether + * or not we act. Left alone, a user who links today and triggers nothing for + * long enough is asked to link again for no reason they can see. + * + * Runs in the gateway because that is the only long-lived, single-instance + * component that holds the store. Reuses `ClaudeCredentialRefresher.ensureFresh` + * rather than refreshing directly, so the sweep and a concurrent launch share + * one in-flight refresh per subject instead of rotating each other's token out. + */ +export class ClaudeCredentialSweeper { + private timer: ReturnType | undefined; + private running = false; + private readonly log: (message: string) => void; + + constructor(private readonly deps: CredentialSweeperDeps) { + this.log = deps.log ?? ((m) => console.log(m)); + } + + start(): void { + if (this.timer) return; + if (!this.deps.store.listSubjects) { + // Standing down is stated, not silent: a sweeper that quietly did nothing + // would look exactly like a working one right up until someone's link + // expired. + this.log("[claude-refresh] sweep disabled: this credential store cannot enumerate subjects"); + return; + } + const intervalMs = this.deps.intervalMs ?? DEFAULT_SWEEP_INTERVAL_MS; + this.timer = setInterval(() => void this.sweep(), intervalMs); + this.timer.unref?.(); + // Sweep once at startup too, so a gateway that restarts often still renews + // (and so a deployment does not wait a full interval to find out the sweep + // is broken). + void this.sweep(); + } + + stop(): void { + if (this.timer) clearInterval(this.timer); + this.timer = undefined; + } + + /** + * One pass. Refreshes every `login` credential inside the sweep margin, + * sequentially -- there is no hurry here, and a burst of parallel token + * requests on a schedule is a worse neighbour than a slow loop. + */ + async sweep(): Promise<{ examined: number; refreshed: number; failed: number }> { + if (this.running) return { examined: 0, refreshed: 0, failed: 0 }; + this.running = true; + const marginMs = this.deps.marginMs ?? DEFAULT_SWEEP_MARGIN_MS; + const now = this.deps.now ?? Date.now; + let examined = 0; + let refreshed = 0; + let failed = 0; + try { + let subjects: string[]; + try { + subjects = (await this.deps.store.listSubjects?.("login")) ?? []; + } catch (err) { + // Deliberately loud: a list that failed is not an empty store, and + // treating it as one is how a sweep does nothing forever. + this.log( + `[claude-refresh] sweep could not list stored credentials: ${err instanceof Error ? err.message : String(err)}`, + ); + return { examined: 0, refreshed: 0, failed: 1 }; + } + + for (const subject of subjects) { + examined += 1; + const record = await this.deps.store.get(subject, "login"); + const blob = record?.credentialsJson; + if (!blob || !needsRefresh(blob, marginMs, now())) continue; + const before = credentialExpiresAt(blob); + const after = credentialExpiresAt((await this.deps.refresher.ensureFresh(subject)) ?? blob); + // `ensureFresh` never throws and reports its own failures; compare + // expiries to know whether this pass actually renewed anything. + if (after && after !== before) refreshed += 1; + else failed += 1; + } + if (refreshed > 0 || failed > 0) { + this.log(`[claude-refresh] sweep: ${examined} examined, ${refreshed} refreshed, ${failed} still due`); + } + return { examined, refreshed, failed }; + } finally { + this.running = false; + } + } +} diff --git a/apps/integration-gateway/src/claude-auth/k8s-secret-store.ts b/apps/integration-gateway/src/claude-auth/k8s-secret-store.ts index f05baf8..c20dc19 100644 --- a/apps/integration-gateway/src/claude-auth/k8s-secret-store.ts +++ b/apps/integration-gateway/src/claude-auth/k8s-secret-store.ts @@ -90,6 +90,19 @@ export class K8sSecretClaudeTokenStore implements ClaudeTokenStore { } } + /** + * Every subject holding a record of `kind`, via a label-selector scan. + * + * Unlike `get`/`set`/`delete` above, this deliberately does NOT swallow its + * error. Those soft-fail because their caller has a sensible response to "no + * answer" (offer a link, skip the write). A sweep that reads a failed list as + * an empty one would silently do nothing forever, which is precisely the + * outcome it exists to prevent -- so the caller is told and can say so. + */ + async listSubjects(kind: ClaudeAuthKind = "setup-token"): Promise { + return (await this.storeFor(kind).listRecords()).map((r) => r.key); + } + async set(subject: string, record: ClaudeTokenRecord): Promise { try { const fields: Record = { kind: record.kind, createdAt: record.createdAt }; diff --git a/apps/integration-gateway/src/claude-auth/store.ts b/apps/integration-gateway/src/claude-auth/store.ts index 3ee16d0..4420a19 100644 --- a/apps/integration-gateway/src/claude-auth/store.ts +++ b/apps/integration-gateway/src/claude-auth/store.ts @@ -51,6 +51,18 @@ export interface ClaudeTokenStore { * credential forever. */ delete(subject: string, kind?: ClaudeAuthKind): Promise; + /** + * Every subject holding a record of `kind` -- for the background refresh + * sweep (claude-auth/credential-refresher.ts), which has no subject to look + * up and so cannot use `get`. + * + * Optional because it is the only method that enumerates: the Secret-backed + * store answers it with a label-selector scan, a deliberate exception to that + * store's own read-path rule, and no in-memory or test double needs it. + * Absent means "this deployment cannot sweep", which the sweeper reports and + * then stands down -- never an empty result silently read as "no links". + */ + listSubjects?(kind?: ClaudeAuthKind): Promise; /** * Moves an existing record from one subject to another, so a credential the * human already authorized keeps working under a new key instead of costing diff --git a/apps/integration-gateway/src/config.ts b/apps/integration-gateway/src/config.ts index 0900353..9d9da0a 100644 --- a/apps/integration-gateway/src/config.ts +++ b/apps/integration-gateway/src/config.ts @@ -105,6 +105,14 @@ export interface AppConfig { claudeCredentialRefreshEnabled: boolean; /** How close to expiry a stored credential must be before it is refreshed on read. */ claudeCredentialRefreshMarginMs: number; + /** + * How often the background sweep renews stored credentials nothing is + * currently reading -- the only thing that keeps an UNUSED link alive, since + * refresh-on-read fires only when something reads. + */ + claudeCredentialSweepIntervalMs: number; + /** How close to expiry a credential must be for the sweep to renew it (wider than the on-read margin). */ + claudeCredentialSweepMarginMs: number; /** Public GitHub App client id used to start OAuth Device Flow links (not a secret). */ githubAppClientId: string; /** Base64 (or hex) 32-byte AES-256-GCM key used to encrypt linked GitHub tokens at rest. */ @@ -214,6 +222,8 @@ export function loadConfig(env: NodeJS.ProcessEnv = process.env): AppConfig { resumeWaitMs: num(env.GATEWAY_RESUME_WAIT_MS, 10 * 60 * 1000), claudeCredentialRefreshEnabled: env.GATEWAY_CLAUDE_CREDENTIAL_REFRESH !== "false", claudeCredentialRefreshMarginMs: num(env.GATEWAY_CLAUDE_CREDENTIAL_REFRESH_MARGIN_MS, 30 * 60 * 1000), + claudeCredentialSweepIntervalMs: num(env.GATEWAY_CLAUDE_CREDENTIAL_SWEEP_INTERVAL_MS, 60 * 60 * 1000), + claudeCredentialSweepMarginMs: num(env.GATEWAY_CLAUDE_CREDENTIAL_SWEEP_MARGIN_MS, 4 * 60 * 60 * 1000), githubAppClientId: env.GITHUB_APP_CLIENT_ID ?? "", identityLinkEncryptionKey: env.IDENTITY_LINK_ENCRYPTION_KEY ?? "", identityLinkToken: env.GATEWAY_IDENTITY_LINK_TOKEN ?? "", diff --git a/apps/integration-gateway/src/credential-store/__fixtures__/fake-secret-api.ts b/apps/integration-gateway/src/credential-store/__fixtures__/fake-secret-api.ts index 8c3e41e..852ef37 100644 --- a/apps/integration-gateway/src/credential-store/__fixtures__/fake-secret-api.ts +++ b/apps/integration-gateway/src/credential-store/__fixtures__/fake-secret-api.ts @@ -29,9 +29,9 @@ export class FakeSecretApi implements SecretApiLike { /** Every call, in order -- lets a test assert create-then-replace rather than two creates. */ readonly calls: string[] = []; /** Set to make the next call of a given kind fail, for the non-404 paths. */ - failWith: { verb: "read" | "create" | "replace" | "delete"; error: unknown } | undefined; + failWith: { verb: "read" | "create" | "replace" | "delete" | "list"; error: unknown } | undefined; - private maybeFail(verb: "read" | "create" | "replace" | "delete"): void { + private maybeFail(verb: "read" | "create" | "replace" | "delete" | "list"): void { if (this.failWith?.verb === verb) { const { error } = this.failWith; this.failWith = undefined; @@ -48,6 +48,27 @@ export class FakeSecretApi implements SecretApiLike { return { metadata: secret.metadata, data }; } + /** + * Filters on the label selector the way the API server does, so a store that + * forgot its selector would visibly scan the whole namespace here rather than + * quietly passing. + */ + async listNamespacedSecret(request: { namespace: string; labelSelector?: string }): Promise<{ items: StoredSecret[] }> { + this.calls.push(`list:${request.labelSelector ?? ""}`); + this.maybeFail("list"); + const required = (request.labelSelector ?? "") + .split(",") + .filter(Boolean) + .map((pair) => pair.split("=") as [string, string]); + const items: StoredSecret[] = []; + for (const [key, secret] of this.secrets) { + if (!key.startsWith(`${request.namespace}/`)) continue; + const labels = secret.metadata.labels ?? {}; + if (required.every(([k, v]) => labels[k] === v)) items.push(secret); + } + return { items }; + } + async readNamespacedSecret(request: { name: string; namespace: string }): Promise { this.calls.push(`read:${request.name}`); this.maybeFail("read"); diff --git a/apps/integration-gateway/src/credential-store/secret-record-store.test.ts b/apps/integration-gateway/src/credential-store/secret-record-store.test.ts index ba20ebd..64359cb 100644 --- a/apps/integration-gateway/src/credential-store/secret-record-store.test.ts +++ b/apps/integration-gateway/src/credential-store/secret-record-store.test.ts @@ -218,3 +218,69 @@ describe("API error classification", () => { expect(isConflictError(new FakeApiError(404, "not found"))).toBe(false); }); }); + +/** + * The one label-selector scan in this store, and a deliberate exception to its + * own "no listing" rule -- see `listRecords`' doc. A maintenance sweep has no + * key to compute a Secret name from, so enumerating IS the task. + */ +describe("SecretRecordStore.listRecords", () => { + it("returns every record of this type with its plaintext key, scoped by label selector", async () => { + const api = new FakeSecretApi(); + const store = makeStore(api); + await store.put("github:alice", { token: "a" }); + await store.put("openwebui:1234", { token: "b" }); + + // A record of a DIFFERENT type in the same namespace must not be returned. + const other = new SecretRecordStore({ + namespace: NS, + namePrefix: "claude-auth-login", + keyLabel: "controller-agent.io/subject", + commonLabels: { "controller-agent.io/credential": "claude-auth" }, + api, + }); + await other.put("github:bob", { credentialsJson: "{}" }); + + const records = await store.listRecords(); + + expect(records.map((r) => r.key).sort()).toEqual(["github:alice", "openwebui:1234"]); + // The bookkeeping field is stripped, exactly as `get` strips it. + expect(records[0]?.fields[RECORD_KEY_FIELD]).toBeUndefined(); + expect(records.find((r) => r.key === "github:alice")?.fields.token).toBe("a"); + expect(api.calls).toContain("list:controller-agent.io/credential=identity-link"); + }); + + // The object name is a one-way hash, so a record without its plaintext key + // cannot be attributed to a subject -- and acting on an unattributable + // credential is worse than leaving it alone. + it("skips a record whose plaintext key was never stored, rather than guessing", async () => { + const api = new FakeSecretApi(); + api.secrets.set(`${NS}/identity-link-github-deadbeefdeadbeef`, { + metadata: { name: "identity-link-github-deadbeefdeadbeef", labels: { "controller-agent.io/credential": "identity-link" } }, + data: { token: Buffer.from("orphan", "utf8").toString("base64") }, + }); + + expect(await makeStore(api).listRecords()).toEqual([]); + }); + + // A failed list is not an empty store; the sweeper's own handling depends on + // this surfacing rather than resolving empty. + it("propagates a listing failure instead of reporting an empty store", async () => { + const api = new FakeSecretApi(); + const store = makeStore(api); + await store.put("github:alice", { token: "a" }); + api.failWith = { verb: "list", error: new FakeApiError(403, "forbidden") }; + + await expect(store.listRecords()).rejects.toThrow(/forbidden/); + }); + + it("reports no records when the API has no list verb at all (an older double or client)", async () => { + const api = new FakeSecretApi(); + const store = makeStore(api); + await store.put("github:alice", { token: "a" }); + // Simulate a SecretApiLike without the optional verb. + (api as { listNamespacedSecret?: unknown }).listNamespacedSecret = undefined; + + expect(await store.listRecords()).toEqual([]); + }); +}); diff --git a/apps/integration-gateway/src/credential-store/secret-record-store.ts b/apps/integration-gateway/src/credential-store/secret-record-store.ts index bd6f465..5a29a85 100644 --- a/apps/integration-gateway/src/credential-store/secret-record-store.ts +++ b/apps/integration-gateway/src/credential-store/secret-record-store.ts @@ -52,6 +52,15 @@ interface SecretLike { */ export interface SecretApiLike { readNamespacedSecret(request: { name: string; namespace: string }): Promise; + /** + * Only ever used by {@link SecretRecordStore.listRecords} -- a maintenance + * sweep, never a lookup. Optional so every existing test double and any + * caller that never sweeps keeps compiling unchanged. + */ + listNamespacedSecret?(request: { + namespace: string; + labelSelector?: string; + }): Promise<{ items?: SecretLike[] }>; createNamespacedSecret(request: { namespace: string; body: unknown }): Promise; replaceNamespacedSecret(request: { name: string; namespace: string; body: unknown }): Promise; deleteNamespacedSecret(request: { name: string; namespace: string }): Promise; @@ -188,6 +197,50 @@ export class SecretRecordStore { return `${this.namePrefix}-${createHash("sha256").update(key).digest("hex").slice(0, 16)}`; } + /** + * Every record of this type, as `{ key, fields }`. + * + * This is the one label-selector scan in this file, and it is a deliberate + * exception to the naming section's "no listing, no label-selector scan, no + * O(keyspace) anything" -- which is a rule about the READ path, where an + * exact `get` by computed name is both possible and required. A maintenance + * sweep has no key to compute a name from; enumerating is the whole task. + * `commonLabels` exists for exactly this ("so an operator -- and a cleanup + * sweep -- can find them all"), and the selector keeps the scan scoped to + * this record type rather than the namespace. + * + * Callers must treat this as maintenance-only: never on a request path, and + * never as a way to answer "does this subject have a record" (that is `get`). + * + * Records whose plaintext key was not stored are skipped rather than guessed + * at -- the object name is a one-way hash, so a record without + * {@link RECORD_KEY_FIELD} cannot be attributed to a subject, and acting on + * an unattributable credential is worse than leaving it alone. + */ + async listRecords(): Promise }>> { + if (!this.api.listNamespacedSecret) return []; + const selector = Object.entries(this.commonLabels) + .map(([k, v]) => `${k}=${v}`) + .join(","); + const page = await this.api.listNamespacedSecret({ + namespace: this.namespace, + ...(selector ? { labelSelector: selector } : {}), + }); + const out: Array<{ key: string; fields: Record }> = []; + for (const secret of page.items ?? []) { + let key = ""; + const fields: Record = {}; + for (const [field, value] of Object.entries(secret.data ?? {})) { + const decoded = Buffer.from(value, "base64").toString("utf8"); + if (field === RECORD_KEY_FIELD) key = decoded; + else fields[field] = decoded; + } + if (!key || Object.keys(fields).length === 0) continue; + out.push({ key, fields }); + } + return out; + } + /** * Reads a record's fields, or `undefined` if there is none. * diff --git a/apps/integration-gateway/src/index.ts b/apps/integration-gateway/src/index.ts index 0a18889..b956cdf 100644 --- a/apps/integration-gateway/src/index.ts +++ b/apps/integration-gateway/src/index.ts @@ -284,6 +284,8 @@ async function main(): Promise { resumeWaitMs: config.resumeWaitMs, claudeCredentialRefreshEnabled: config.claudeCredentialRefreshEnabled, claudeCredentialRefreshMarginMs: config.claudeCredentialRefreshMarginMs, + claudeCredentialSweepIntervalMs: config.claudeCredentialSweepIntervalMs, + claudeCredentialSweepMarginMs: config.claudeCredentialSweepMarginMs, }); await server.listen(config.httpPort); diff --git a/apps/integration-gateway/src/server.ts b/apps/integration-gateway/src/server.ts index 004b5b6..b68a498 100644 --- a/apps/integration-gateway/src/server.ts +++ b/apps/integration-gateway/src/server.ts @@ -8,7 +8,7 @@ import type { ClaudeLoginFlows } from "./claude-auth/pty-login.js"; import type { ClaudeTokenStore } from "./claude-auth/store.js"; import type { IdentityResolver } from "./identity.js"; import type { OrchestratorClient, OrchestratorInvokeResult } from "./orchestrator-client.js"; -import { ClaudeCredentialRefresher } from "./claude-auth/credential-refresher.js"; +import { ClaudeCredentialRefresher, ClaudeCredentialSweeper } from "./claude-auth/credential-refresher.js"; import { renderSessionPage } from "./session-page.js"; import type { SessionPageStore } from "./session-page-store.js"; import { parseGithubEvent, verifyGithubSignature, WebhookAuthError } from "./webhooks/github.js"; @@ -99,6 +99,14 @@ export interface GatewayServerOptions { claudeCredentialRefreshEnabled?: boolean; /** How close to expiry a stored credential must be before it is refreshed on read. */ claudeCredentialRefreshMarginMs?: number; + /** How often the background sweep renews stored credentials nothing is reading. */ + claudeCredentialSweepIntervalMs?: number; + /** + * How close to expiry a credential must be for the SWEEP to renew it -- + * deliberately wider than the on-read margin, since the sweep's job is that a + * link is never found dead rather than that one blob is usable right now. + */ + claudeCredentialSweepMarginMs?: number; /** * Session-page feature (issue #81) -- both fields must be set together to * enable it: a "starting work" comment posted right when an @@ -133,12 +141,20 @@ export class GatewayServer { private server: Server | undefined; private readonly identityLinkApi: IdentityLinkApi | undefined; private readonly claudeAuthApi: ClaudeAuthApi | undefined; + private readonly claudeCredentialSweeper: ClaudeCredentialSweeper | undefined; constructor(private readonly options: GatewayServerOptions) { this.identityLinkApi = options.identityLinkLinker && options.identityLinkToken ? new IdentityLinkApi(options.identityLinkLinker, options.identityLinkToken) : undefined; + const refresher = options.claudeAuthStore + ? new ClaudeCredentialRefresher({ + store: options.claudeAuthStore, + enabled: options.claudeCredentialRefreshEnabled ?? true, + ...(options.claudeCredentialRefreshMarginMs ? { marginMs: options.claudeCredentialRefreshMarginMs } : {}), + }) + : undefined; this.claudeAuthApi = options.claudeAuthFlows && options.claudeAuthStore && options.identityLinkToken && options.publicBaseUrl ? new ClaudeAuthApi( @@ -152,13 +168,23 @@ export class GatewayServer { // refreshed was a run's own in-pod CLI, which made the stored copy // dependent on that pod reporting back, and left an unexercised // link to age out on its own. - new ClaudeCredentialRefresher({ - store: options.claudeAuthStore, - enabled: options.claudeCredentialRefreshEnabled ?? true, - ...(options.claudeCredentialRefreshMarginMs ? { marginMs: options.claudeCredentialRefreshMarginMs } : {}), - }), + refresher, ) : undefined; + // The sweep renews credentials nothing is currently asking for, which is + // the only thing that keeps an UNUSED link alive -- refresh-on-read above + // fires only when something reads. Shares the refresher, so a sweep and a + // concurrent launch collapse onto one refresh per subject rather than + // rotating each other's token out. + this.claudeCredentialSweeper = + refresher && options.claudeAuthStore && (options.claudeCredentialRefreshEnabled ?? true) + ? new ClaudeCredentialSweeper({ + store: options.claudeAuthStore, + refresher, + ...(options.claudeCredentialSweepIntervalMs ? { intervalMs: options.claudeCredentialSweepIntervalMs } : {}), + ...(options.claudeCredentialSweepMarginMs ? { marginMs: options.claudeCredentialSweepMarginMs } : {}), + }) + : undefined; } listen(port: number): Promise { @@ -169,12 +195,14 @@ export class GatewayServer { if (!res.writableEnded) res.writeHead(500).end(); }); }); + this.claudeCredentialSweeper?.start(); this.server.listen(port, resolve); }); } close(): Promise { return new Promise((resolve, reject) => { + this.claudeCredentialSweeper?.stop(); if (!this.server) return resolve(); this.server.close((err) => (err ? reject(err) : resolve())); }); diff --git a/charts/agent-controller/charts/integration-gateway/templates/rbac.yaml b/charts/agent-controller/charts/integration-gateway/templates/rbac.yaml index 6208ea3..1ccf2a1 100644 --- a/charts/agent-controller/charts/integration-gateway/templates/rbac.yaml +++ b/charts/agent-controller/charts/integration-gateway/templates/rbac.yaml @@ -34,7 +34,10 @@ rules: resources: ["secrets"] # `watch` is what lets a pending link resume the moment the user finishes in # their browser instead of on the next poll; `list` is required alongside it - # by the API server for a watch request. + # by the API server for a watch request, and is now also used directly -- + # once per sweep interval, label-selector scoped -- by the background + # credential refresh (claude-auth/credential-refresher.ts), which has no + # subject to compute a Secret name from and so cannot use `get`. verbs: ["get", "list", "watch", "create", "update", "delete"] --- apiVersion: rbac.authorization.k8s.io/v1