From 4e62cf5b03781847fec3e402f22a6c4bd42b3aeb Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Thu, 20 Aug 2026 06:23:53 -0400 Subject: [PATCH 01/10] Stop the GitHub App credential refresh storm and tell the truth about auth state The App user credential died because every ADE process refreshed the same rotating refresh token with no coordination. GitHub revoked the token by reuse detection, and the retry loop (no backoff, no failure memory) then hit the OAuth endpoint 100k times in 5 hours, which GitHub answered with 429 on the whole OAuth host - blocking the device-flow re-auth too. Transport: a typed GitHubOAuthError now carries status, oauth error code, and retry-after. Refresh inspects HTTP-200 error bodies, so bad_refresh_token is no longer reported as "did not return a token". Coordination: one refresh coordinator per credential store per process, plus a cross-process lease and backoff ledger persisted inside the credential record. The record to POST is captured inside the atomic lease acquisition. Transient failures back off 60s..1h (retry-after honored, worst case 6 POSTs/hour). A rejected refresh token marks the credential dead and is never retried; the record is kept so the UI can say "re-authorize" honestly. A cleared store no longer resurrects from an instance's memory copy. Sharing: github.appUserToken.v1 is now file-backed, so the desktop, the brain, and the CLI read one record; a routed desktop store plus a one-time adoption migrates copies stranded in the Electron-only store without ever overwriting the shared file. Status model: GitHubAppUserAuthStatus carries credentialState (missing/authorized/blocked/needs_reauth), refreshBlockedUntil, and lastRefreshError. The UI judges by the refresh token, so a stale 8-hour access token no longer renders "Authorization expired". Blocked shows "Paused until " with no re-auth CTA; the repo axis says it is waiting on the account instead of parroting the relay's 401; device-flow rate limits render as plain language; and the App panel gains a Disconnect control (clearAppUserAuth previously had no UI caller at all). Also: the automation ingress cooldown now applies while signed in, and the headless CLI caches App-credential resolution for the same 30s window as the desktop. Co-Authored-By: Claude Fable 5 --- .../src/headlessLinearServices.test.ts | 45 + apps/ade-cli/src/headlessLinearServices.ts | 114 ++- .../credentials/credentialStore.test.ts | 82 ++ .../services/credentials/credentialStore.ts | 171 ++++ apps/desktop/src/main/main.ts | 56 +- .../automationIngressService.test.ts | 44 + .../automations/automationIngressService.ts | 38 +- .../services/github/githubAppUserAuth.test.ts | 125 +++ .../main/services/github/githubAppUserAuth.ts | 115 ++- .../github/githubAppUserAuthService.test.ts | 352 ++++++++ .../github/githubAppUserAuthService.ts | 824 ++++++++++++++++-- .../main/services/github/githubRateLimit.ts | 34 +- .../main/services/github/githubRelayConfig.ts | 46 +- .../services/github/githubService.test.ts | 57 +- .../src/main/services/github/githubService.ts | 48 +- apps/desktop/src/renderer/browserMock.ts | 9 + .../app/IntegrationBannerHost.test.tsx | 3 + .../components/app/IntegrationBannerHost.tsx | 4 +- .../github/GitHubAppInstallPanel.test.tsx | 86 +- .../github/GitHubAppInstallPanel.tsx | 186 ++-- .../components/settings/GitHubSection.tsx | 54 +- .../lib/githubIntegrationStatus.test.ts | 227 ++++- .../renderer/lib/githubIntegrationStatus.ts | 236 ++++- .../src/renderer/webclient/adapter/misc.ts | 21 +- .../src/shared/githubOperationCredential.ts | 10 + apps/desktop/src/shared/types/git.ts | 18 + 26 files changed, 2755 insertions(+), 250 deletions(-) create mode 100644 apps/desktop/src/main/services/github/githubAppUserAuth.test.ts create mode 100644 apps/desktop/src/main/services/github/githubAppUserAuthService.test.ts diff --git a/apps/ade-cli/src/headlessLinearServices.test.ts b/apps/ade-cli/src/headlessLinearServices.test.ts index 9f33d0c391..087cfb5474 100644 --- a/apps/ade-cli/src/headlessLinearServices.test.ts +++ b/apps/ade-cli/src/headlessLinearServices.test.ts @@ -1775,6 +1775,51 @@ describe("headlessLinearServices", () => { } }); + it("resolves the GitHub App credential once per window instead of per request", async () => { + const environment = isolateHeadlessGithubAuth("ade-headless-github-app-window-", { + emptyGhConfig: true, + }); + // An access token past its life, so every resolution that is not cached + // costs a refresh POST — the traffic that rate-limited GitHub for the user. + new EncryptedFileCredentialStore().setSync("github.appUserToken.v1", JSON.stringify({ + accessToken: "ghu_stale_app_token", + tokenType: "bearer", + scope: null, + expiresAt: new Date(Date.now() - 60_000).toISOString(), + refreshToken: "ghr_live_refresh_token", + refreshTokenExpiresAt: new Date(Date.now() + 180 * 24 * 3_600_000).toISOString(), + userLogin: "octocat", + updatedAt: new Date().toISOString(), + })); + const requestedUrls: string[] = []; + globalThis.fetch = vi.fn(async (input: RequestInfo | URL) => { + requestedUrls.push(String(input)); + return new Response(JSON.stringify({ login: "octocat" }), { + status: 200, + headers: { "content-type": "application/json", "x-oauth-scopes": "" }, + }); + }) as unknown as typeof fetch; + const githubService = createHeadlessGitHubService( + "/tmp/ade-project", + { debug() {}, info() {}, warn() {}, error() {} } as any, + { + ghAuthTokenProvider: () => ({ token: null, ghCliPath: null, ghAuthError: null }), + }, + ); + + try { + await githubService.getStatus(); + await githubService.getAppInstallationStatus({ owner: "acme", name: "repo" }); + await githubService.getStatus(); + + const refreshPosts = requestedUrls + .filter((url) => url === "https://github.com/login/oauth/access_token"); + expect(refreshPosts).toHaveLength(1); + } finally { + environment.restore(); + } + }); + it("drops a cached headless writer when that credential disappears", async () => { const environment = isolateHeadlessGithubAuth("ade-headless-github-cache-"); const machineCredentialStore = new EncryptedFileCredentialStore(); diff --git a/apps/ade-cli/src/headlessLinearServices.ts b/apps/ade-cli/src/headlessLinearServices.ts index 2e4cef6e42..aa54100c17 100644 --- a/apps/ade-cli/src/headlessLinearServices.ts +++ b/apps/ade-cli/src/headlessLinearServices.ts @@ -53,7 +53,10 @@ import { fetchGitHubAppInstallationStatus, type GitHubRelaySecretReader, } from "../../desktop/src/main/services/github/githubRelayConfig"; -import { createGitHubAppUserAuthService } from "../../desktop/src/main/services/github/githubAppUserAuthService"; +import { + classifyAppUserAuthFailure, + createGitHubAppUserAuthService, +} from "../../desktop/src/main/services/github/githubAppUserAuthService"; import { requestGithubRawWithCredentialFallback, type GithubRawRequestArgs, @@ -75,6 +78,7 @@ import { createPrService as createPrServiceImpl } from "../../desktop/src/main/s import { createAutomationSecretService as createAutomationSecretServiceImpl } from "../../desktop/src/main/services/automations/automationSecretService"; import { EncryptedFileCredentialStore } from "./services/credentials/credentialStore"; import { + GITHUB_CREDENTIAL_INVENTORY_CACHE_TTL_MS, evaluateGithubCredentialCapabilities, githubOperationCredentialCandidates, githubOperationCredentialPrecedence, @@ -747,11 +751,24 @@ export function createHeadlessGitHubService( promise: Promise; } | null = null; + type AppCredentialLookup = { + token: string | null; + failure: ReturnType | null; + status: GitHubAppUserAuthStatus; + }; + let appCredentialCache: { + expiresAt: number; + promise: Promise; + } | null = null; + const invalidateStatusCache = (): void => { cachedStatus = null; cachedAt = 0; cachedStatusBinding = null; statusLookupGeneration += 1; + // A re-authorization or a sign-out just replaced the App credential this + // cache holds, so it cannot outlive the status it fed. + appCredentialCache = null; }; const noteCredentialStoreReadState = (unreadable: boolean): boolean => { @@ -802,6 +819,54 @@ export function createHeadlessGitHubService( return read.value; }; + const buildAppCredentialAsync = async (): Promise => { + const status = appUserAuth.getAuthStatus(); + if (!status.tokenStored) return { token: null, failure: null, status }; + try { + return { token: await appUserAuth.getValidTokenForRelay(), failure: null, status }; + } catch (error: unknown) { + const failure = classifyAppUserAuthFailure(error); + logger.warn("github.app_user_token_unavailable", { + error: failure.described.message, + kind: failure.authFailure.kind, + credentialState: failure.described.credentialState, + status: failure.described.status, + oauthError: failure.described.oauthError, + retryAt: failure.authFailure.retryAt, + }); + return { token: null, failure, status }; + } + }; + + /** + * The App credential is resolved at most once per TTL window, matching the + * desktop service's inventory cache. + * + * Every status read, PR call and relay poll builds an inventory, and building + * one asks for a GitHub App token — which on a stale access token is a refresh + * POST. The headless twin had no such window, which is why the brain, not the + * desktop app, drove most of the refresh traffic. Only the App lookup is + * cached: the `gh` CLI and PAT reads keep answering live, so signing out of + * `gh` still demotes the write credential immediately. + */ + const readAppCredentialAsync = async (): Promise => { + const now = Date.now(); + if (appCredentialCache && appCredentialCache.expiresAt > now) { + return await appCredentialCache.promise; + } + const promise = buildAppCredentialAsync(); + appCredentialCache = { + expiresAt: now + GITHUB_CREDENTIAL_INVENTORY_CACHE_TTL_MS, + promise, + }; + try { + return await promise; + } catch (error) { + if (appCredentialCache?.promise === promise) appCredentialCache = null; + throw error; + } + }; + const readCredentialInventoryAsync = async (): Promise => { const patToken = await readStoredPatTokenAsync(); const patTokenStored = Boolean(patToken); @@ -811,24 +876,11 @@ export function createHeadlessGitHubService( // below would otherwise hand this inventory somebody else's outcome. const storeUnreadableForThisRead = credentialStoreUnreadable; const environmentToken = envToken("ADE_GITHUB_TOKEN", "GITHUB_TOKEN", "GH_TOKEN"); - const appStatus = appUserAuth.getAuthStatus(); const [appResult, gh] = await Promise.all([ - appStatus.tokenStored - ? appUserAuth.getValidTokenForRelay() - .then((token) => ({ token, failure: null })) - .catch((error: unknown) => { - const message = error instanceof Error ? error.message : String(error); - const failure = classifyGitHubAuthFailure({ message }); - logger.warn("github.app_user_token_refresh_failed", { - error: message, - kind: failure.authFailure.kind, - retryAt: failure.authFailure.retryAt, - }); - return { token: null, failure }; - }) - : Promise.resolve({ token: null, failure: null }), + readAppCredentialAsync(), Promise.resolve(options.ghAuthTokenProvider?.() ?? ghAuthTokenAsync()), ]); + const appStatus = appResult.status; const appToken = appResult.token; const candidates: HeadlessGitHubTokenCandidate[] = []; if (environmentToken) { @@ -876,7 +928,11 @@ export function createHeadlessGitHubService( candidates, availableSources: new Set(candidates.map((candidate) => candidate.source)), failures: appResult.failure - ? [{ source: "app", ...appResult.failure }] + ? [{ + source: "app" as const, + authFailure: appResult.failure.authFailure, + rateLimit: appResult.failure.rateLimit, + }] : [], appTokenStored: appToken != null || appStatus.tokenStored, patTokenStored, @@ -2037,7 +2093,20 @@ export function createHeadlessGitHubService( const owner = args.owner?.trim(); const name = args.name?.trim(); const repo = owner && name ? { owner, name } : await detectGitHubRepoAsync(projectRoot); - const githubAppUserToken = await appUserAuth.getValidTokenForRelay().catch(() => null); + // Same reason as the desktop service: the reason the token is missing is + // what the repo axis is allowed to say, so it cannot be swallowed. + const appUserToken = await appUserAuth.getValidTokenForRelay() + .then((token) => ({ token, failure: null })) + .catch((error: unknown) => { + const failure = classifyAppUserAuthFailure(error); + logger.warn("github.app_installation_status_auth_unavailable", { + error: failure.described.message, + kind: failure.authFailure.kind, + credentialState: failure.described.credentialState, + retryAt: failure.authFailure.retryAt, + }); + return { token: null, failure }; + }); const accountAccessToken = options.getAccountAccessToken ? await options.getAccountAccessToken().catch(() => null) : null; @@ -2045,7 +2114,14 @@ export function createHeadlessGitHubService( repo, secretReader: options.githubRelaySecretReader, forceRefresh: args.forceRefresh === true, - githubAppUserToken, + githubAppUserToken: appUserToken.token, + appUserAuthFailure: appUserToken.failure + ? { + message: appUserToken.failure.described.message, + credentialState: appUserToken.failure.described.credentialState, + retryAt: appUserToken.failure.authFailure.retryAt, + } + : null, accountAccessToken, auditLog: appUserAuth.auditLog, }); diff --git a/apps/ade-cli/src/services/credentials/credentialStore.test.ts b/apps/ade-cli/src/services/credentials/credentialStore.test.ts index ecb1c4dc3f..e4383faa49 100644 --- a/apps/ade-cli/src/services/credentials/credentialStore.test.ts +++ b/apps/ade-cli/src/services/credentials/credentialStore.test.ts @@ -10,7 +10,9 @@ import { EncryptedFileCredentialStore, KeytarCredentialStore, CREDENTIAL_STORE_LOCK_TIMEOUT_MS, + adoptFileBackedCredentials, createDefaultCredentialStore, + createRoutedCredentialStore, inspectCredentialStoreHealth, isFileBackedCredentialKey, readCredentialStoreQuarantine, @@ -22,6 +24,7 @@ import { DEFAULT_REFRESH_ROTATION_WAIT_MS, } from "../account/accountAuthService"; import { BOOTSTRAP_TOKEN_KEY } from "../sync/brainProjectActionsSyncHandler"; +import { GITHUB_APP_USER_TOKEN_KEY } from "../../../../desktop/src/main/services/github/githubAppUserAuthService"; import { createMacKeychainMaterialResolver, resolveMacKeychainMaterialOutcome, @@ -909,6 +912,82 @@ describe("ElectronSafeStorageCredentialStore", () => { expect(raw).toContain("ADE_SAFE_STORAGE_CREDENTIALS_V1"); }); + it("leaves the GitHub App user token where the brain and the CLI read it", () => { + // Two copies of this record means two processes refreshing one rotating + // refresh token, and GitHub answers a reused refresh token by revoking the + // credential. + const brainWrite = new EncryptedFileCredentialStore({ secretsDir: tempDir }); + brainWrite.setSync("github.appUserToken.v1", JSON.stringify({ accessToken: "ghu_brain" })); + + const desktopStore = new ElectronSafeStorageCredentialStore({ secretsDir: tempDir, safeStorage }); + desktopStore.getSync("linear.token.v1"); + + const brainRead = new EncryptedFileCredentialStore({ secretsDir: tempDir }); + expect(brainRead.getSync("github.appUserToken.v1")).toContain("ghu_brain"); + const safeFile = fs.readFileSync(path.join(tempDir, "credentials.safe.enc"), "utf8"); + expect(safeFile).not.toContain("ghu_brain"); + }); + + it("routes file-backed keys to the shared file and adopts ones stranded in safeStorage", () => { + const fileStore = new EncryptedFileCredentialStore({ secretsDir: tempDir }); + const primary = new ElectronSafeStorageCredentialStore({ secretsDir: tempDir, safeStorage }); + // What an older build left behind: the App token sealed in the file only the + // desktop app can open. + fs.writeFileSync( + path.join(tempDir, "credentials.safe.enc"), + Buffer.concat([ + Buffer.from("ADE_SAFE_STORAGE_CREDENTIALS_V1\n"), + safeStorage.encryptString(JSON.stringify({ + "github.appUserToken.v1": "stranded-app-token", + "linear.token.v1": "lin_secret", + })), + ]), + ); + + const adoption = adoptFileBackedCredentials({ primary, fileStore, identity: tempDir }); + const routed = createRoutedCredentialStore({ primary, fileStore }); + + expect(adoption.adopted).toContain("github.appUserToken.v1"); + expect(fileStore.getSync("github.appUserToken.v1")).toBe("stranded-app-token"); + expect(primary.getSync("github.appUserToken.v1")).toBeNull(); + expect(routed.getSync("github.appUserToken.v1")).toBe("stranded-app-token"); + // Everything else keeps going to the Electron-only store. + expect(routed.getSync("linear.token.v1")).toBe("lin_secret"); + expect(fileStore.getSync("linear.token.v1")).toBeNull(); + + // The read state answers about the file the read actually went to, so an + // unreadable sibling cannot make a good credential look unreadable. + routed.getSync("linear.token.v1"); + expect(routed.getLastReadState?.()).toBe("available"); + + // A write through the routed store reaches the brain, and so does the + // atomic single-key update the refresh ledger runs on. + routed.setSync("github.appUserToken.v1", "renewed-app-token"); + routed.updateKeySync?.("github.appUserToken.v1", (current) => `${current}+ledger`); + expect(new EncryptedFileCredentialStore({ secretsDir: tempDir }) + .getSync("github.appUserToken.v1")).toBe("renewed-app-token+ledger"); + }); + + it("never overwrites the shared file's credential with a stale safeStorage copy", () => { + const fileStore = new EncryptedFileCredentialStore({ secretsDir: tempDir }); + fileStore.setSync("github.appUserToken.v1", "fresh-from-brain"); + const primary = new ElectronSafeStorageCredentialStore({ secretsDir: tempDir, safeStorage }); + fs.writeFileSync( + path.join(tempDir, "credentials.safe.enc"), + Buffer.concat([ + Buffer.from("ADE_SAFE_STORAGE_CREDENTIALS_V1\n"), + safeStorage.encryptString(JSON.stringify({ "github.appUserToken.v1": "stale-from-june" })), + ]), + ); + + const adoption = adoptFileBackedCredentials({ primary, fileStore, identity: tempDir }); + + expect(adoption.adopted).toEqual([]); + expect(adoption.pruned).toContain("github.appUserToken.v1"); + expect(fileStore.getSync("github.appUserToken.v1")).toBe("fresh-from-brain"); + expect(primary.getSync("github.appUserToken.v1")).toBeNull(); + }); + it("leaves the brain-readable account session in the legacy file store", () => { // The ADE brain (com.ade.runtime) and the CLI cannot read the Electron-only // safeStorage file. Migrating the account session into it and deleting the @@ -1079,6 +1158,9 @@ describe("ElectronSafeStorageCredentialStore", () => { // Asserted against the real constant: renaming it in the sync handler must // fail here instead of silently moving the token into safeStorage. expect(isFileBackedCredentialKey(BOOTSTRAP_TOKEN_KEY)).toBe(true); + // Same rationale, one incident later: two copies of the GitHub App token + // means two processes refreshing one rotating refresh token. + expect(isFileBackedCredentialKey(GITHUB_APP_USER_TOKEN_KEY)).toBe(true); expect(isFileBackedCredentialKey("linear.token.v1")).toBe(false); }); diff --git a/apps/ade-cli/src/services/credentials/credentialStore.ts b/apps/ade-cli/src/services/credentials/credentialStore.ts index 86d4e50f21..4c48f9aab8 100644 --- a/apps/ade-cli/src/services/credentials/credentialStore.ts +++ b/apps/ade-cli/src/services/credentials/credentialStore.ts @@ -119,6 +119,23 @@ export type SyncCredentialStore = CredentialStore & { deleteSync(key: string): void; /** Atomically update the complete synchronous store when supported. */ updateSync?(updater: (values: Record) => boolean | void): void; + /** + * Atomically update ONE key when supported. + * + * Distinct from `updateSync` because a store that routes keys to different + * files cannot answer a whole-map updater, while it can always say which file + * one key lives in. Return `undefined` to write nothing, `null` to delete. + */ + updateKeySync?( + key: string, + mutator: (current: string | null) => string | null | undefined, + ): void; + /** + * A stable name for the storage behind this store, equal for every store + * object over the same file. Callers that coordinate across instances (the + * GitHub App token refresh) key their process-wide state on it. + */ + credentialStoreIdentity?(): string; /** Best-effort cross-process notification that persisted credentials changed. */ onDidChange?(listener: () => void): () => void; /** Result of the most recent synchronous credential-file read. */ @@ -199,6 +216,7 @@ const SAFE_STORAGE_FILE_MAGIC = Buffer.from("ADE_SAFE_STORAGE_CREDENTIALS_V1\n") * app is signed in. Keep the literals in sync with: * - ACCOUNT_SESSION_CREDENTIAL_KEY (services/account/accountAuthService.ts) * - BOOTSTRAP_TOKEN_KEY (services/sync/brainProjectActionsSyncHandler.ts) + * - GITHUB_APP_USER_TOKEN_KEY (desktop services/github/githubAppUserAuthService.ts) * They are duplicated here rather than imported to keep this module free of * service-layer dependencies; credentialStore.test.ts asserts they match. */ @@ -210,6 +228,12 @@ const FILE_BACKED_CREDENTIAL_KEYS: readonly string[] = [ // the process pair the journal exists to coordinate. "account.session.rotation.v1", "sync.bootstrapToken.v1", + // The GitHub App user token carries a rotating refresh token and, next to it, + // the refresh ledger every ADE process coordinates through. Two copies of that + // record means two processes refreshing the same rotating token, which GitHub + // answers by revoking the credential — so it has to live in the one file the + // app, the brain and the CLI all read. + "github.appUserToken.v1", ]; export function isFileBackedCredentialKey(key: string): boolean { @@ -856,6 +880,34 @@ export class EncryptedFileCredentialStore implements SyncCredentialStore { }); } + updateKeySync( + key: string, + mutator: (current: string | null) => string | null | undefined, + ): void { + const normalized = normalizeKey(key); + this.updateSync((values) => { + const next = mutator(values[normalized] ?? null); + if (next === undefined) return false; + if (next === null) { + if (!(normalized in values)) return false; + delete values[normalized]; + return true; + } + const trimmed = next.trim(); + if (!trimmed.length) { + if (!(normalized in values)) return false; + delete values[normalized]; + return true; + } + values[normalized] = trimmed; + return true; + }); + } + + credentialStoreIdentity(): string { + return this.credentialsPath; + } + /** * What the "Can't read your sign-in" surface actually runs. * @@ -1580,6 +1632,125 @@ export class ElectronSafeStorageCredentialStore implements SyncCredentialStore { } } +/** + * One store that keeps file-backed keys in the shared machine file and + * everything else in `primary`. + * + * The desktop app's own store is Electron-only, and the migration that keeps + * file-backed keys OUT of it only runs once. That left the reader with nowhere + * to look: `account.session.v1` and the GitHub App token stayed in + * `credentials.json.enc` while every desktop read went to `credentials.safe.enc` + * and answered "not connected". Routing per key is what makes "the brain and the + * app share this credential" true for readers, not just for writers. + */ +export function createRoutedCredentialStore(args: { + primary: SyncCredentialStore; + fileStore: SyncCredentialStore; +}): SyncCredentialStore { + const storeFor = (key: string): SyncCredentialStore => + isFileBackedCredentialKey(normalizeKey(key)) ? args.fileStore : args.primary; + // `getLastReadState()` describes the store's MOST RECENT read, and a caller + // asks it immediately after the read it means. Answering from the wrong file + // is how a readable credential gets reported as "can't read your sign-in". + let lastReadStore: SyncCredentialStore = args.primary; + const readStoreFor = (key: string): SyncCredentialStore => { + lastReadStore = storeFor(key); + return lastReadStore; + }; + return { + get: async (key) => readStoreFor(key).get(key), + set: async (key, value) => storeFor(key).set(key, value), + delete: async (key) => storeFor(key).delete(key), + getSync: (key) => readStoreFor(key).getSync(key), + setSync: (key, value) => storeFor(key).setSync(key, value), + deleteSync: (key) => storeFor(key).deleteSync(key), + // A whole-map updater cannot be split across two files, so it keeps the + // meaning it had before routing existed: it updates the primary store. + updateSync: args.primary.updateSync?.bind(args.primary), + updateKeySync: (key, mutator) => { + const store = storeFor(key); + if (store.updateKeySync) { + store.updateKeySync(key, mutator); + return; + } + const next = mutator(store.getSync(key)); + if (next === undefined) return; + if (next === null) store.deleteSync(key); + else store.setSync(key, next); + }, + credentialStoreIdentity: () => args.fileStore.credentialStoreIdentity?.() + ?? args.primary.credentialStoreIdentity?.() + ?? "ade.routed-credential-store", + onDidChange: (listener) => { + const unsubscribes = [ + args.primary.onDidChange?.(listener), + args.fileStore.onDidChange?.(listener), + ]; + return () => { + for (const unsubscribe of unsubscribes) unsubscribe?.(); + }; + }, + getLastReadState: () => lastReadStore.getLastReadState?.() ?? "missing", + getLastReadFailureReason: () => lastReadStore.getLastReadFailureReason?.() ?? null, + }; +} + +/** + * Moves file-backed credentials a previous build left in the Electron-only + * store back into the shared machine file. + * + * Runs once per secrets directory per process. Never overwrites: the shared file + * is where the brain writes, so a value there is at least as fresh as the + * desktop copy — and the desktop copy of the GitHub App token is exactly the + * stale one whose refresh token GitHub has already rotated away. A value is only + * removed from the Electron-only store once the shared file holds one. + */ +const adoptedSecretsDirs = new Set(); + +export function adoptFileBackedCredentials(args: { + primary: SyncCredentialStore; + fileStore: SyncCredentialStore; + identity: string; +}): { adopted: string[]; pruned: string[] } { + const adopted: string[] = []; + const pruned: string[] = []; + if (adoptedSecretsDirs.has(args.identity)) return { adopted, pruned }; + adoptedSecretsDirs.add(args.identity); + for (const key of FILE_BACKED_CREDENTIAL_KEYS) { + let stranded: string | null = null; + try { + stranded = args.primary.getSync(key); + } catch { + // An unreadable Electron store has nothing to adopt, and saying so is the + // job of `getLastReadState`, not of this migration. + return { adopted, pruned }; + } + if (!stranded?.trim()) continue; + try { + if (args.fileStore.updateKeySync) { + // Atomic: a brain write racing this adoption must win over the stale + // desktop copy, and check-then-set leaves a window where it would not. + let wrote = false; + args.fileStore.updateKeySync(key, (current) => { + if (current?.trim()) return undefined; + wrote = true; + return stranded; + }); + if (wrote) adopted.push(key); + } else if (!args.fileStore.getSync(key)?.trim()) { + args.fileStore.setSync(key, stranded); + adopted.push(key); + } + args.primary.deleteSync(key); + pruned.push(key); + } catch { + // Best effort: leaving the duplicate behind is survivable, losing the + // credential is not. + } + } + return { adopted, pruned }; +} + type KeytarModule = { getPassword(service: string, account: string): Promise; setPassword(service: string, account: string, password: string): Promise; diff --git a/apps/desktop/src/main/main.ts b/apps/desktop/src/main/main.ts index 69a2353158..72b793b613 100644 --- a/apps/desktop/src/main/main.ts +++ b/apps/desktop/src/main/main.ts @@ -250,6 +250,8 @@ import { } from "../../../ade-cli/src/services/account/sharedAccountAuthService"; import { installRuntimeService, uninstallRuntimeService } from "../../../ade-cli/src/serviceManager"; import { + adoptFileBackedCredentials, + createRoutedCredentialStore, ElectronSafeStorageCredentialStore, EncryptedFileCredentialStore, isElectronSafeStorageCredentialFile, @@ -637,13 +639,20 @@ function createDesktopCredentialStore(secretsDir: string): SyncCredentialStore { const legacyStore = new EncryptedFileCredentialStore({ secretsDir }); const safeCredentialsPath = path.join(secretsDir, "credentials.safe.enc"); const legacyCredentialsPath = path.join(secretsDir, "credentials.json.enc"); + // Credentials the ADE brain and the CLI co-own live in the shared machine + // file, so the desktop app must READ them there too — the Electron-only store + // cannot see them, and a build that wrote one into it signs the brain out. + const routeFileBackedKeys = (primary: SyncCredentialStore): SyncCredentialStore => { + adoptFileBackedCredentials({ primary, fileStore: legacyStore, identity: secretsDir }); + return createRoutedCredentialStore({ primary, fileStore: legacyStore }); + }; try { if (safeStorage.isEncryptionAvailable()) { - return new ElectronSafeStorageCredentialStore({ + return routeFileBackedKeys(new ElectronSafeStorageCredentialStore({ secretsDir, safeStorage, legacyStore, - }); + })); } } catch { // Fall through to the file store when Electron cannot reach the OS keychain. @@ -653,26 +662,31 @@ function createDesktopCredentialStore(secretsDir: string): SyncCredentialStore { || isElectronSafeStorageCredentialFile(legacyCredentialsPath) ) { const message = "Electron safeStorage is unavailable; unlock the OS credential store to read ADE credentials."; - return { - get: async () => { - throw new Error(message); - }, - set: async () => { - throw new Error(message); - }, - delete: async () => { - throw new Error(message); - }, - getSync: () => { - throw new Error(message); - }, - setSync: () => { - throw new Error(message); - }, - deleteSync: () => { - throw new Error(message); + // The shared file needs no keychain, so the credentials the brain co-owns + // stay reachable even while the Electron-only ones are locked away. + return createRoutedCredentialStore({ + primary: { + get: async () => { + throw new Error(message); + }, + set: async () => { + throw new Error(message); + }, + delete: async () => { + throw new Error(message); + }, + getSync: () => { + throw new Error(message); + }, + setSync: () => { + throw new Error(message); + }, + deleteSync: () => { + throw new Error(message); + }, }, - }; + fileStore: legacyStore, + }); } return legacyStore; } diff --git a/apps/desktop/src/main/services/automations/automationIngressService.test.ts b/apps/desktop/src/main/services/automations/automationIngressService.test.ts index 90a1b6f7d4..a425c38d88 100644 --- a/apps/desktop/src/main/services/automations/automationIngressService.test.ts +++ b/apps/desktop/src/main/services/automations/automationIngressService.test.ts @@ -432,6 +432,50 @@ describe("automationIngressService", () => { expect(logger.warn).not.toHaveBeenCalled(); }); + it("cools down a broken GitHub App credential even while signed in", async () => { + vi.useFakeTimers(); + const logger = makeLogger(); + const webSockets = makeWebSocketHarness(); + const fetchSpy = vi.spyOn(globalThis, "fetch").mockResolvedValue(new Response( + JSON.stringify({ ok: true, events: [] }), + { status: 200, headers: { "content-type": "application/json" } }, + )); + const getAppUserTokenForRelay = vi.fn(async () => { + throw new Error("GitHub paused ADE's authorization renewal until 2026-08-20T13:00:00.000Z."); + }); + + service = createAutomationIngressService({ + logger: logger as never, + automationService: null, + prService: { ingestGithubWebhook: vi.fn() } as never, + secretService: { + getSecret: () => null, + } as never, + githubService: { + detectRepo: vi.fn(async () => ({ owner: "arul28", name: "ADE" })), + getAppUserTokenForRelay, + }, + getAccountAccessToken: async () => "clerk-account-token", + listRules: () => [], + ingressCursorStore: { + get: () => null, + set: () => {}, + }, + pollIntervalMs: GITHUB_RELAY_MIN_POLL_INTERVAL_MS, + webSocketFactory: webSockets.factory, + }); + + await service.start(); + + // The account token still carries the poll, so the relay is not disabled... + expect(fetchSpy).toHaveBeenCalled(); + // ...but the broken GitHub credential is recorded, which is what starts the + // cooldown the signed-in path never had. + expect(logger.info).toHaveBeenCalledWith("automations.github_relay_auth_pending", expect.objectContaining({ + error: expect.stringContaining("paused ADE's authorization renewal"), + })); + }); + it("can read GitHub relay config from runtime environment variables", async () => { const previousApiBase = process.env.ADE_GITHUB_RELAY_API_BASE_URL; const previousProjectId = process.env.ADE_GITHUB_RELAY_REMOTE_PROJECT_ID; diff --git a/apps/desktop/src/main/services/automations/automationIngressService.ts b/apps/desktop/src/main/services/automations/automationIngressService.ts index 2479f77aeb..bd76974b1c 100644 --- a/apps/desktop/src/main/services/automations/automationIngressService.ts +++ b/apps/desktop/src/main/services/automations/automationIngressService.ts @@ -773,13 +773,26 @@ export function createAutomationIngressService(args: AutomationIngressServiceArg }); }; - const enterHostedAuthPending = (message: string): void => { + /** + * Records that the GitHub App credential is unusable, and paces the next + * attempt. + * + * Separate from `enterHostedAuthPending` because a signed-in machine can still + * poll the relay with its account token, so the failure must not disable the + * subscription — but it must still start the cooldown, or the poll loop asks + * for the same broken credential every thirty seconds for as long as the app + * runs. + */ + const noteHostedAuthFailure = (message: string): void => { hostedAuthPendingUntilMs = Date.now() + HOSTED_RELAY_AUTH_PENDING_RETRY_MS; + if (hostedAuthPendingLogged) return; + hostedAuthPendingLogged = true; + args.logger.info("automations.github_relay_auth_pending", { error: message }); + }; + + const enterHostedAuthPending = (message: string): void => { + noteHostedAuthFailure(message); disableRelaySubscription(); - if (!hostedAuthPendingLogged) { - hostedAuthPendingLogged = true; - args.logger.info("automations.github_relay_auth_pending", { error: message }); - } updateGithubRelayStatus({ healthy: false, status: "disabled", @@ -880,10 +893,15 @@ export function createAutomationIngressService(args: AutomationIngressServiceArg ))) ?? "").trim() || null; } catch (error) { if (error instanceof GithubRelayPollSupersededError) throw error; + const message = error instanceof Error ? error.message : String(error); if (!accountAccessToken) { - enterHostedAuthPending(error instanceof Error ? error.message : String(error)); + enterHostedAuthPending(message); return; } + // Signed in, so the poll can still run on the account token — but the + // GitHub credential is broken, and asking it again in thirty seconds + // is what turned one dead token into a hundred thousand attempts. + noteHostedAuthFailure(message); } } const hostedAuth = useLegacyProjectRoute @@ -893,8 +911,12 @@ export function createAutomationIngressService(args: AutomationIngressServiceArg enterHostedAuthPending(hostedAuth.error); return; } - hostedAuthPendingUntilMs = 0; - hostedAuthPendingLogged = false; + if (hostedAuth && !hostedAuth.ok) { + noteHostedAuthFailure(hostedAuth.error); + } else { + hostedAuthPendingUntilMs = 0; + hostedAuthPendingLogged = false; + } const authToken = useLegacyProjectRoute ? legacyAuthToken : hostedAuth?.ok diff --git a/apps/desktop/src/main/services/github/githubAppUserAuth.test.ts b/apps/desktop/src/main/services/github/githubAppUserAuth.test.ts new file mode 100644 index 0000000000..1c7d400cd8 --- /dev/null +++ b/apps/desktop/src/main/services/github/githubAppUserAuth.test.ts @@ -0,0 +1,125 @@ +import { describe, expect, it } from "vitest"; +import { + pollGitHubAppDeviceFlow, + refreshGitHubAppUserToken, + startGitHubAppDeviceFlow, +} from "./githubAppUserAuth"; + +type OAuthErrorShape = { + name?: string; + status?: number | null; + oauthError?: string | null; + errorDescription?: string | null; + retryAfterSec?: number | null; +}; + +function jsonResponse( + body: unknown, + init: { status?: number; headers?: Record } = {}, +): Response { + return new Response(JSON.stringify(body), { + status: init.status ?? 200, + headers: { "content-type": "application/json", ...(init.headers ?? {}) }, + }); +} + +async function captureError(run: () => Promise): Promise { + try { + await run(); + } catch (error) { + return error as OAuthErrorShape & { message: string }; + } + throw new Error("Expected the call to reject."); +} + +describe("refreshGitHubAppUserToken", () => { + it("rejects an HTTP-200 OAuth error body as a definitive dead-token failure", async () => { + // GitHub answers a rejected refresh token with HTTP 200 and an error body, + // which is why "did the response parse" is not the same question as "did the + // refresh work". + const fetchImpl = async (): Promise => jsonResponse({ + error: "bad_refresh_token", + error_description: "The refresh token passed is incorrect or expired.", + error_uri: "https://docs.github.com/apps", + }); + + const error = await captureError(() => refreshGitHubAppUserToken({ + refreshToken: "ghr_dead", + fetchImpl: fetchImpl as unknown as typeof fetch, + userAgent: "ade-test", + })); + + expect(error.name).toBe("GitHubOAuthError"); + expect(error.oauthError).toBe("bad_refresh_token"); + expect(error.status).toBe(200); + expect(error.message).toContain("refresh token"); + }); + + it("carries the status and retry-after of a rate-limited refresh", async () => { + const fetchImpl = async (): Promise => jsonResponse( + { error: "too_many_requests", error_description: "You have exceeded a secondary rate limit." }, + { status: 429, headers: { "retry-after": "120" } }, + ); + + const error = await captureError(() => refreshGitHubAppUserToken({ + refreshToken: "ghr_live", + fetchImpl: fetchImpl as unknown as typeof fetch, + userAgent: "ade-test", + })); + + expect(error.name).toBe("GitHubOAuthError"); + expect(error.status).toBe(429); + expect(error.retryAfterSec).toBe(120); + expect(error.oauthError).toBe("too_many_requests"); + }); + + it("returns the rotated refresh token when GitHub rotates it", async () => { + const fetchImpl = async (): Promise => jsonResponse({ + access_token: "ghu_new", + token_type: "bearer", + expires_in: 28_800, + refresh_token: "ghr_rotated", + refresh_token_expires_in: 15_811_200, + }); + + const record = await refreshGitHubAppUserToken({ + refreshToken: "ghr_old", + fetchImpl: fetchImpl as unknown as typeof fetch, + userAgent: "ade-test", + }); + + expect(record.accessToken).toBe("ghu_new"); + expect(record.refreshToken).toBe("ghr_rotated"); + }); +}); + +describe("device flow transport", () => { + it("carries the status and retry-after of a rate-limited device-code request", async () => { + const fetchImpl = async (): Promise => jsonResponse( + { error: "too_many_requests" }, + { status: 429, headers: { "retry-after": "60" } }, + ); + + const error = await captureError(() => startGitHubAppDeviceFlow({ + fetchImpl: fetchImpl as unknown as typeof fetch, + userAgent: "ade-test", + })); + + expect(error.name).toBe("GitHubOAuthError"); + expect(error.status).toBe(429); + expect(error.retryAfterSec).toBe(60); + }); + + it("keeps reporting HTTP-200 device-flow errors as poll results", async () => { + const fetchImpl = async (): Promise => jsonResponse({ error: "authorization_pending" }); + + const result = await pollGitHubAppDeviceFlow({ + deviceCode: "device", + intervalSec: 5, + fetchImpl: fetchImpl as unknown as typeof fetch, + userAgent: "ade-test", + }); + + expect(result.status).toBe("pending"); + }); +}); diff --git a/apps/desktop/src/main/services/github/githubAppUserAuth.ts b/apps/desktop/src/main/services/github/githubAppUserAuth.ts index 395cba593c..a756753bde 100644 --- a/apps/desktop/src/main/services/github/githubAppUserAuth.ts +++ b/apps/desktop/src/main/services/github/githubAppUserAuth.ts @@ -40,6 +40,61 @@ export type GitHubAppDevicePollResult = type FetchImpl = typeof fetch; +/** Which GitHub OAuth endpoint produced a failure. */ +export type GitHubOAuthEndpoint = "device_code" | "token"; + +/** + * A GitHub OAuth failure with the parts a caller has to act on. + * + * GitHub answers a rejected refresh token with HTTP 200 and an `error` field, so + * the transport cannot report failure by status alone; and it answers a + * rate-limited client with 429 plus `retry-after`, which is the only honest + * source for how long to wait. A bare `Error` carrying a string threw both away, + * which is how a dead refresh token became an unbounded retry loop. + */ +export class GitHubOAuthError extends Error { + constructor( + message: string, + readonly endpoint: GitHubOAuthEndpoint, + readonly status: number, + readonly oauthError: string | null, + readonly errorDescription: string | null, + readonly retryAfterSec: number | null, + ) { + super(message); + this.name = "GitHubOAuthError"; + } +} + +/** + * OAuth error codes that mean "this credential will never work again". + * + * Everything outside this set is treated as transient, because retrying a + * transient failure costs a request while giving up on a live credential costs + * the user their connection. + */ +const DEFINITIVE_OAUTH_ERRORS: ReadonlySet = new Set([ + "bad_refresh_token", + "incorrect_client_credentials", + "invalid_grant", + "unauthorized_client", + "unsupported_grant_type", +]); + +export function isDefinitiveGitHubOAuthError(oauthError: string | null | undefined): boolean { + return typeof oauthError === "string" && DEFINITIVE_OAUTH_ERRORS.has(oauthError.trim()); +} + +function parseRetryAfterSeconds(response: Response): number | null { + const raw = response.headers.get("retry-after")?.trim(); + if (!raw) return null; + const seconds = Number(raw); + if (Number.isFinite(seconds)) return Math.max(0, Math.trunc(seconds)); + const at = Date.parse(raw); + if (!Number.isFinite(at)) return null; + return Math.max(0, Math.ceil((at - Date.now()) / 1000)); +} + function readString(source: Record, key: string): string { const value = source[key]; return typeof value === "string" ? value.trim() : ""; @@ -64,9 +119,10 @@ function parseJsonRecord(value: unknown): Record { async function postGitHubOAuthForm(args: { fetchImpl: FetchImpl; url: string; + endpoint: GitHubOAuthEndpoint; userAgent: string; body: Record; -}): Promise> { +}): Promise<{ payload: Record; status: number; retryAfterSec: number | null }> { const response = await args.fetchImpl(args.url, { method: "POST", headers: { @@ -77,13 +133,25 @@ async function postGitHubOAuthForm(args: { body: new URLSearchParams(args.body).toString(), }); const payload = parseJsonRecord(await response.json().catch(() => ({}))); + const retryAfterSec = parseRetryAfterSeconds(response); if (!response.ok) { - const message = readString(payload, "error_description") - || readString(payload, "error") - || `GitHub OAuth request failed (${response.status})`; - throw new Error(message); + const oauthError = readString(payload, "error") || null; + const errorDescription = readString(payload, "error_description") || null; + throw new GitHubOAuthError( + errorDescription + || oauthError + || `GitHub OAuth request failed (${response.status})`, + args.endpoint, + response.status, + oauthError, + errorDescription, + retryAfterSec, + ); } - return payload; + // An OK response keeps its payload: the device flow reads `error` itself to + // tell "still waiting" from "denied", and only the refresh path treats an + // error field as a failure. + return { payload, status: response.status, retryAfterSec }; } export async function startGitHubAppDeviceFlow(args: { @@ -92,9 +160,10 @@ export async function startGitHubAppDeviceFlow(args: { userAgent: string; }): Promise { const clientId = args.clientId?.trim() || ADE_GITHUB_APP_CLIENT_ID; - const payload = await postGitHubOAuthForm({ + const { payload } = await postGitHubOAuthForm({ fetchImpl: args.fetchImpl ?? fetch, url: GITHUB_DEVICE_CODE_URL, + endpoint: "device_code", userAgent: args.userAgent, body: { client_id: clientId }, }); @@ -125,9 +194,10 @@ export async function pollGitHubAppDeviceFlow(args: { fetchUserLogin?: (accessToken: string) => Promise; }): Promise { const clientId = args.clientId?.trim() || ADE_GITHUB_APP_CLIENT_ID; - const payload = await postGitHubOAuthForm({ + const { payload } = await postGitHubOAuthForm({ fetchImpl: args.fetchImpl ?? fetch, url: GITHUB_OAUTH_TOKEN_URL, + endpoint: "token", userAgent: args.userAgent, body: { client_id: clientId, @@ -182,9 +252,10 @@ export async function refreshGitHubAppUserToken(args: { fetchUserLogin?: (accessToken: string) => Promise; }): Promise { const clientId = args.clientId?.trim() || ADE_GITHUB_APP_CLIENT_ID; - const payload = await postGitHubOAuthForm({ + const { payload, status, retryAfterSec } = await postGitHubOAuthForm({ fetchImpl: args.fetchImpl ?? fetch, url: GITHUB_OAUTH_TOKEN_URL, + endpoint: "token", userAgent: args.userAgent, body: { client_id: clientId, @@ -192,8 +263,32 @@ export async function refreshGitHubAppUserToken(args: { refresh_token: args.refreshToken, }, }); + // GitHub reports a rejected refresh token as HTTP 200 with an error body. It + // has to be read here, or a dead credential looks exactly like a malformed + // response and gets retried forever. + const oauthError = readString(payload, "error"); + if (oauthError) { + const errorDescription = readString(payload, "error_description") || null; + throw new GitHubOAuthError( + errorDescription || `GitHub rejected the refresh token (${oauthError}).`, + "token", + status, + oauthError, + errorDescription, + retryAfterSec, + ); + } const accessToken = readString(payload, "access_token"); - if (!accessToken) throw new Error("GitHub did not return a refreshed user access token."); + if (!accessToken) { + throw new GitHubOAuthError( + "GitHub did not return a refreshed user access token.", + "token", + status, + null, + null, + retryAfterSec, + ); + } const userLogin = args.fetchUserLogin ? await args.fetchUserLogin(accessToken).catch(() => null) : null; return { accessToken, diff --git a/apps/desktop/src/main/services/github/githubAppUserAuthService.test.ts b/apps/desktop/src/main/services/github/githubAppUserAuthService.test.ts new file mode 100644 index 0000000000..dca0803bdb --- /dev/null +++ b/apps/desktop/src/main/services/github/githubAppUserAuthService.test.ts @@ -0,0 +1,352 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + createGitHubAppUserAuthService, + resetGitHubAppUserAuthCoordinatorsForTests, +} from "./githubAppUserAuthService"; + +const TOKEN_KEY = "github.appUserToken.v1"; + +type StoredValues = Record; + +/** + * A stand-in for EncryptedFileCredentialStore: several store objects may share + * one `values` map, which is how the real machine file is shared by the desktop + * app, the ADE brain and the CLI. `updateKeySync` is the atomic read-modify-write + * those processes serialize their writes through. + */ +function createFakeStore(values: StoredValues) { + return { + getSync: (key: string): string | null => values[key] ?? null, + setSync: (key: string, value: string): void => { + values[key] = value; + }, + deleteSync: (key: string): void => { + delete values[key]; + }, + updateKeySync: ( + key: string, + mutator: (current: string | null) => string | null | undefined, + ): void => { + const next = mutator(values[key] ?? null); + if (next === undefined) return; + if (next === null) delete values[key]; + else values[key] = next; + }, + }; +} + +function createLogger() { + return { info: vi.fn(), warn: vi.fn() }; +} + +function storedRecord(values: StoredValues): Record | null { + const raw = values[TOKEN_KEY]; + return raw ? JSON.parse(raw) as Record : null; +} + +function writeRecord(values: StoredValues, patch: Record = {}): void { + values[TOKEN_KEY] = JSON.stringify({ + accessToken: "ghu_old", + tokenType: "bearer", + scope: null, + // Already past its life, so every call has to refresh. + expiresAt: new Date(Date.now() - 60_000).toISOString(), + refreshToken: "ghr_live", + refreshTokenExpiresAt: new Date(Date.now() + 180 * 24 * 3_600_000).toISOString(), + userLogin: "octocat", + updatedAt: new Date().toISOString(), + ...patch, + }); +} + +function jsonResponse( + body: unknown, + init: { status?: number; headers?: Record } = {}, +): Response { + return new Response(JSON.stringify(body), { + status: init.status ?? 200, + headers: { "content-type": "application/json", ...(init.headers ?? {}) }, + }); +} + +const TOKEN_URL = "https://github.com/login/oauth/access_token"; + +function tokenPostCount(fetchImpl: { mock: { calls: unknown[][] } }): number { + return fetchImpl.mock.calls.filter((call) => String(call[0]) === TOKEN_URL).length; +} + +/** A refresh endpoint that rotates the token and kills the one it replaced. */ +function createRotatingRefreshEndpoint() { + let live = "ghr_live"; + let rotations = 0; + return { + get rotations() { + return rotations; + }, + respond(body: string | null): Response { + const sent = new URLSearchParams(body ?? "").get("refresh_token") ?? ""; + if (sent !== live) { + return jsonResponse({ + error: "bad_refresh_token", + error_description: "The refresh token passed is incorrect or expired.", + }); + } + rotations += 1; + live = `ghr_rotated_${rotations}`; + return jsonResponse({ + access_token: `ghu_fresh_${rotations}`, + token_type: "bearer", + expires_in: 28_800, + refresh_token: live, + refresh_token_expires_in: 15_811_200, + }); + }, + }; +} + +let clockNowMs = Date.parse("2026-08-20T12:00:00.000Z"); +const now = (): number => clockNowMs; +const advance = (ms: number): void => { + clockNowMs += ms; +}; +// Yields to the event loop without spending wall-clock time, so a coordinator +// that waits for a peer's write still makes progress inside a test. +const sleep = (): Promise => new Promise((resolve) => setTimeout(resolve, 0)); + +let identityCounter = 0; +function nextIdentity(): string { + identityCounter += 1; + return `test-store-${identityCounter}`; +} + +beforeEach(() => { + clockNowMs = Date.parse("2026-08-20T12:00:00.000Z"); + vi.spyOn(Date, "now").mockImplementation(() => clockNowMs); + resetGitHubAppUserAuthCoordinatorsForTests(); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe("app user token refresh", () => { + it("stops retrying a refresh token GitHub has rejected", async () => { + const values: StoredValues = {}; + writeRecord(values); + const fetchImpl = vi.fn(async () => jsonResponse({ + error: "bad_refresh_token", + error_description: "The refresh token passed is incorrect or expired.", + })); + const service = createGitHubAppUserAuthService({ + credentialStore: createFakeStore(values), + logger: createLogger(), + fetchImpl: fetchImpl as unknown as typeof fetch, + userAgent: "ade-test", + storeIdentity: nextIdentity(), + now, + sleep, + }); + + await expect(service.getValidTokenForRelay()).rejects.toThrow(); + await expect(service.getValidTokenForRelay()).rejects.toThrow(); + await expect(service.getValidTokenForRelay()).rejects.toThrow(); + + expect(fetchImpl).toHaveBeenCalledTimes(1); + // The record survives so the UI can say "re-authorize" instead of "never + // connected", which is the difference between a fixable state and a mystery. + expect(storedRecord(values)).not.toBeNull(); + expect(service.getAuthStatus().credentialState).toBe("needs_reauth"); + }); + + it("pauses refreshes for the retry-after window GitHub asked for", async () => { + const values: StoredValues = {}; + writeRecord(values); + const fetchImpl = vi.fn(async () => jsonResponse( + { error: "too_many_requests" }, + { status: 429, headers: { "retry-after": "120" } }, + )); + const service = createGitHubAppUserAuthService({ + credentialStore: createFakeStore(values), + logger: createLogger(), + fetchImpl: fetchImpl as unknown as typeof fetch, + userAgent: "ade-test", + storeIdentity: nextIdentity(), + now, + sleep, + }); + + await expect(service.getValidTokenForRelay()).rejects.toThrow(); + const status = service.getAuthStatus(); + expect(status.credentialState).toBe("blocked"); + expect(status.lastRefreshError?.kind).toBe("rate_limited"); + expect(Date.parse(status.refreshBlockedUntil ?? "")).toBe(clockNowMs + 120_000); + + advance(119_000); + await expect(service.getValidTokenForRelay()).rejects.toThrow(); + expect(fetchImpl).toHaveBeenCalledTimes(1); + + advance(2_000); + await expect(service.getValidTokenForRelay()).rejects.toThrow(); + expect(fetchImpl).toHaveBeenCalledTimes(2); + }); + + it("keeps a whole hour of demand under eight refresh requests", async () => { + const values: StoredValues = {}; + writeRecord(values); + const fetchImpl = vi.fn(async () => jsonResponse( + { error: "server_error" }, + { status: 503 }, + )); + const service = createGitHubAppUserAuthService({ + credentialStore: createFakeStore(values), + logger: createLogger(), + fetchImpl: fetchImpl as unknown as typeof fetch, + userAgent: "ade-test", + storeIdentity: nextIdentity(), + now, + sleep, + }); + + // The relay poller asks every 30 seconds; an hour of that is 120 demands. + for (let attempt = 0; attempt < 120; attempt += 1) { + await expect(service.getValidTokenForRelay()).rejects.toThrow(); + advance(30_000); + } + + expect(fetchImpl.mock.calls.length).toBeLessThanOrEqual(8); + expect(fetchImpl.mock.calls.length).toBeGreaterThan(1); + }); + + it("sends one refresh for every service instance in a process", async () => { + const values: StoredValues = {}; + writeRecord(values); + const endpoint = createRotatingRefreshEndpoint(); + const fetchImpl = vi.fn(async (_url: string, init?: RequestInit) => endpoint.respond( + typeof init?.body === "string" ? init.body : null, + )); + const identity = nextIdentity(); + const store = createFakeStore(values); + const build = () => createGitHubAppUserAuthService({ + credentialStore: store, + logger: createLogger(), + fetchImpl: fetchImpl as unknown as typeof fetch, + userAgent: "ade-test", + storeIdentity: identity, + now, + sleep, + }); + // One service per project scope is what the desktop app and the brain both + // build, and they all read the same credential record. + const services = [build(), build(), build(), build()]; + + const tokens = await Promise.all(services.map((service) => service.getValidTokenForRelay())); + + expect(endpoint.rotations).toBe(1); + expect(tokenPostCount(fetchImpl)).toBe(1); + expect(new Set(tokens)).toEqual(new Set(["ghu_fresh_1"])); + }); + + it("makes a second process wait for the refresh a peer is already running", async () => { + const values: StoredValues = {}; + writeRecord(values); + const endpoint = createRotatingRefreshEndpoint(); + let releaseWinner: () => void = () => undefined; + const winnerReleased = new Promise((resolve) => { + releaseWinner = resolve; + }); + const fetchImpl = vi.fn(async (_url: string, init?: RequestInit) => { + const body = typeof init?.body === "string" ? init.body : null; + await winnerReleased; + return endpoint.respond(body); + }); + const build = (identity: string) => createGitHubAppUserAuthService({ + // A separate store object per process: same file, separate memory. + credentialStore: createFakeStore(values), + logger: createLogger(), + fetchImpl: fetchImpl as unknown as typeof fetch, + userAgent: "ade-test", + storeIdentity: identity, + now, + sleep, + }); + + const brain = build(nextIdentity()); + const desktop = build(nextIdentity()); + const winner = brain.getValidTokenForRelay(); + // Let the winner take the lease before the peer looks at the record. + await Promise.resolve(); + const loser = desktop.getValidTokenForRelay(); + releaseWinner(); + + await expect(winner).resolves.toBe("ghu_fresh_1"); + await expect(loser).resolves.toBe("ghu_fresh_1"); + // A second POST would have carried the refresh token the first one just + // rotated away, and GitHub answers that by killing the credential. + expect(tokenPostCount(fetchImpl)).toBe(1); + expect(endpoint.rotations).toBe(1); + }); + + it("does not resurrect a credential another instance cleared", async () => { + const values: StoredValues = {}; + writeRecord(values, { expiresAt: new Date(clockNowMs + 3_600_000).toISOString() }); + const store = createFakeStore(values); + const fetchImpl = vi.fn(async () => jsonResponse({})); + const service = createGitHubAppUserAuthService({ + credentialStore: store, + logger: createLogger(), + fetchImpl: fetchImpl as unknown as typeof fetch, + userAgent: "ade-test", + storeIdentity: nextIdentity(), + now, + sleep, + }); + + expect(service.getStoredTokenForHealth()).toBe("ghu_old"); + // Another process signed out; the store is the truth, not this instance's + // last read of it. + delete values[TOKEN_KEY]; + + expect(service.getStoredTokenForHealth()).toBeNull(); + expect(service.getAuthStatus().tokenStored).toBe(false); + expect(service.getAuthStatus().credentialState).toBe("missing"); + }); +}); + +describe("app user auth status", () => { + it("reports a stale access token with a live refresh token as authorized", async () => { + const values: StoredValues = {}; + writeRecord(values); + const service = createGitHubAppUserAuthService({ + credentialStore: createFakeStore(values), + logger: createLogger(), + fetchImpl: (async () => jsonResponse({})) as unknown as typeof fetch, + userAgent: "ade-test", + storeIdentity: nextIdentity(), + now, + sleep, + }); + + const status = service.getAuthStatus(); + expect(status.credentialState).toBe("authorized"); + expect(status.refreshBlockedUntil).toBeNull(); + expect(status.lastRefreshError).toBeNull(); + }); + + it("reports an expired refresh token as needing re-authorization", async () => { + const values: StoredValues = {}; + writeRecord(values, { + refreshTokenExpiresAt: new Date(clockNowMs - 3_600_000).toISOString(), + }); + const service = createGitHubAppUserAuthService({ + credentialStore: createFakeStore(values), + logger: createLogger(), + fetchImpl: (async () => jsonResponse({})) as unknown as typeof fetch, + userAgent: "ade-test", + storeIdentity: nextIdentity(), + now, + sleep, + }); + + expect(service.getAuthStatus().credentialState).toBe("needs_reauth"); + }); +}); diff --git a/apps/desktop/src/main/services/github/githubAppUserAuthService.ts b/apps/desktop/src/main/services/github/githubAppUserAuthService.ts index 0c5ae5b46a..229bbe3d00 100644 --- a/apps/desktop/src/main/services/github/githubAppUserAuthService.ts +++ b/apps/desktop/src/main/services/github/githubAppUserAuthService.ts @@ -2,10 +2,17 @@ import { randomUUID } from "node:crypto"; import type { GitHubAppDeviceAuthPollResult, GitHubAppDeviceAuthStartResult, + GitHubAppUserAuthCredentialState, + GitHubAppUserAuthRefreshError, GitHubAppUserAuthStatus, + GitHubAuthFailure, + GitHubRateLimitState, } from "../../../shared/types"; +import { classifyGitHubAuthFailure } from "./githubRateLimit"; import { ADE_GITHUB_APP_CLIENT_ID, + GitHubOAuthError, + isDefinitiveGitHubOAuthError, type GitHubAppDeviceCode, type GitHubAppUserTokenRecord, pollGitHubAppDeviceFlow, @@ -19,14 +26,35 @@ import { import { GITHUB_REST_API_VERSION } from "./githubApiVersion"; import { asString } from "../shared/utils"; -const GITHUB_APP_USER_TOKEN_KEY = "github.appUserToken.v1"; +export const GITHUB_APP_USER_TOKEN_KEY = "github.appUserToken.v1"; const GITHUB_APP_USER_TOKEN_REFRESH_SKEW_MS = 2 * 60_000; const MAX_PENDING_DEVICE_AUTH_SESSIONS = 5; +/** How long one process may hold the right to run the refresh POST. */ +const REFRESH_LEASE_MS = 60_000; +const REFRESH_BACKOFF_BASE_MS = 60_000; +const REFRESH_BACKOFF_MAX_MS = 60 * 60_000; +const LEASE_POLL_INTERVAL_MS = 250; +/** Bounds the wait for a peer's refresh at roughly three seconds. */ +const LEASE_POLL_MAX_ATTEMPTS = 12; export type GitHubAppUserAuthCredentialStore = { getSync(key: string): string | null | undefined; setSync(key: string, value: string): void; deleteSync(key: string): void; + /** + * Atomic read-modify-write of ONE key, when the store supports it. + * + * The refresh ledger below is shared by every ADE process on the machine, so + * "read, decide, write" has to be one step or two processes can both conclude + * they hold the refresh lease. Return `undefined` to write nothing, `null` to + * delete the key. + */ + updateKeySync?( + key: string, + mutator: (current: string | null) => string | null | undefined, + ): void; + /** A stable name for the underlying storage, shared by every store over it. */ + credentialStoreIdentity?(): string; }; type GitHubAppDeviceAuthSession = GitHubAppDeviceCode & { @@ -39,11 +67,309 @@ type GitHubAppUserAuthLogger = { warn(message: string, meta?: Record): void; }; +type RefreshFailureKind = GitHubAppUserAuthRefreshError["kind"]; + +/** + * The persistent memory of how the last refresh went, stored INSIDE the + * credential record so every process that reads the credential also reads the + * backoff that applies to it. + * + * Without this the only coordination was a per-instance in-flight promise, and + * a machine runs many instances: one per project scope in the desktop app, one + * per project scope in the brain, plus the CLI. Each of them independently + * POSTed the same refresh token, GitHub's rotation-reuse detection revoked the + * credential, and the resulting dead token was retried by all of them until + * GitHub rate-limited the whole OAuth host. + */ +type RefreshLedger = { + /** No refresh may be attempted before this instant. */ + notBeforeAt: string | null; + consecutiveFailures: number; + /** GitHub rejected the refresh token itself; only re-authorization fixes it. */ + dead: boolean; + leaseUntil: string | null; + leaseHolder: string | null; + /** Bumped on every successful refresh, for log correlation across processes. */ + generation: number; + lastFailure: { + kind: RefreshFailureKind; + message: string; + status: number | null; + oauthError: string | null; + retryAfterSec: number | null; + at: string; + } | null; +}; + +type StoredAppUserAuth = { + token: GitHubAppUserTokenRecord | null; + refresh: RefreshLedger; +}; + +type RefreshFailure = { + kind: RefreshFailureKind; + message: string; + status: number | null; + oauthError: string | null; + retryAfterSec: number | null; + dead: boolean; +}; + +/** + * Why ADE cannot hand out an App user token right now, in terms the UI can + * render without guessing. + */ +export class GitHubAppUserAuthError extends Error { + constructor( + message: string, + readonly credentialState: Exclude, + readonly retryAt: string | null, + readonly failureKind: RefreshFailureKind | null, + readonly status: number | null, + readonly oauthError: string | null, + ) { + super(message); + this.name = "GitHubAppUserAuthError"; + } +} + +/** + * The parts of a failed token lookup a caller needs to classify it, whether the + * failure came from this service or straight from the OAuth transport. + * + * Duck-typed for the same reason `githubAuthFailureKindOf` is: the desktop + * service and the headless twin both call this, and an `instanceof` check + * answers wrong across module instances. + */ +export function describeGitHubAppUserAuthFailure(error: unknown): { + message: string; + status: number | null; + oauthError: string | null; + retryAt: string | null; + credentialState: GitHubAppUserAuthCredentialState | null; + failureKind: RefreshFailureKind | null; +} { + const candidate = error as Partial & Partial | null; + return { + message: error instanceof Error ? error.message : String(error), + status: typeof candidate?.status === "number" ? candidate.status : null, + oauthError: typeof candidate?.oauthError === "string" ? candidate.oauthError : null, + retryAt: typeof candidate?.retryAt === "string" ? candidate.retryAt : null, + credentialState: typeof candidate?.credentialState === "string" + ? candidate.credentialState + : null, + failureKind: typeof candidate?.failureKind === "string" ? candidate.failureKind : null, + }; +} + +/** + * Turns a failed App user token lookup into the credential-failure shape the + * GitHub status surfaces already speak, with the OAuth details that decide the + * kind carried across instead of being flattened into a message string. + */ +export function classifyAppUserAuthFailure(error: unknown): { + authFailure: GitHubAuthFailure; + rateLimit: GitHubRateLimitState | null; + described: ReturnType; +} { + const described = describeGitHubAppUserAuthFailure(error); + const classified = classifyGitHubAuthFailure({ + status: described.status ?? undefined, + message: described.message, + oauthError: described.oauthError, + retryAt: described.retryAt, + }); + if (described.credentialState === "needs_reauth") { + // The refresh token is gone, expired, or rejected. Only re-authorization + // helps, so it must never be reported as a rate limit that will pass. + return { + described, + rateLimit: classified.rateLimit, + authFailure: { kind: "invalid_token", message: described.message, retryAt: null }, + }; + } + return { ...classified, described }; +} + +/** + * One refresh at a time per credential store, for every service instance in the + * process. Keyed by the storage the instances share, NOT by the instance. + */ +type StoreCoordinator = { inFlight: Promise | null }; + +const storeCoordinators = new Map(); +const unnamedStoreIdentities = new WeakMap(); +let anonymousStoreCounter = 0; + +/** + * The name every service instance over one storage must agree on. + * + * A store that cannot name itself falls back to per-OBJECT identity rather than + * a shared constant: two unrelated stores must not share a coordinator, and the + * real stores all report a path. + */ +function resolveStoreIdentity( + store: GitHubAppUserAuthCredentialStore | null | undefined, + declared: string | null | undefined, +): string { + const explicit = declared?.trim(); + if (explicit) return explicit; + if (!store) return `ade.memory-credential-store.${(anonymousStoreCounter += 1)}`; + const named = store.credentialStoreIdentity?.().trim(); + if (named) return named; + const existing = unnamedStoreIdentities.get(store); + if (existing) return existing; + const assigned = `ade.unnamed-credential-store.${(anonymousStoreCounter += 1)}`; + unnamedStoreIdentities.set(store, assigned); + return assigned; +} + +function coordinatorFor(identity: string): StoreCoordinator { + let coordinator = storeCoordinators.get(identity); + if (!coordinator) { + coordinator = { inFlight: null }; + storeCoordinators.set(identity, coordinator); + } + return coordinator; +} + +/** Drops the process-wide coordinators so one test cannot leak into the next. */ +export function resetGitHubAppUserAuthCoordinatorsForTests(): void { + storeCoordinators.clear(); +} + +function emptyLedger(): RefreshLedger { + return { + notBeforeAt: null, + consecutiveFailures: 0, + dead: false, + leaseUntil: null, + leaseHolder: null, + generation: 0, + lastFailure: null, + }; +} + +function readIsoAfter(iso: string | null | undefined, cutoffMs: number): boolean { + if (!iso) return false; + const time = Date.parse(iso); + return Number.isFinite(time) && time > cutoffMs; +} + +function trimmedOrNull(value: unknown): string | null { + return typeof value === "string" && value.trim() ? value.trim() : null; +} + +function parseLedger(value: unknown): RefreshLedger { + const base = emptyLedger(); + if (!value || typeof value !== "object" || Array.isArray(value)) return base; + const raw = value as Record; + const failure = raw.lastFailure && typeof raw.lastFailure === "object" && !Array.isArray(raw.lastFailure) + ? raw.lastFailure as Record + : null; + const kind = trimmedOrNull(failure?.kind); + return { + notBeforeAt: trimmedOrNull(raw.notBeforeAt), + consecutiveFailures: typeof raw.consecutiveFailures === "number" && Number.isFinite(raw.consecutiveFailures) + ? Math.max(0, Math.trunc(raw.consecutiveFailures)) + : 0, + dead: raw.dead === true, + leaseUntil: trimmedOrNull(raw.leaseUntil), + leaseHolder: trimmedOrNull(raw.leaseHolder), + generation: typeof raw.generation === "number" && Number.isFinite(raw.generation) + ? Math.max(0, Math.trunc(raw.generation)) + : 0, + lastFailure: failure && kind + ? { + kind: kind as RefreshFailureKind, + message: trimmedOrNull(failure.message) ?? "GitHub refused to renew the ADE GitHub App authorization.", + status: typeof failure.status === "number" && Number.isFinite(failure.status) + ? Math.trunc(failure.status) + : null, + oauthError: trimmedOrNull(failure.oauthError), + retryAfterSec: typeof failure.retryAfterSec === "number" && Number.isFinite(failure.retryAfterSec) + ? Math.max(0, Math.trunc(failure.retryAfterSec)) + : null, + at: trimmedOrNull(failure.at) ?? new Date().toISOString(), + } + : null, + }; +} + +function parseStoredAppUserAuth(raw: string): StoredAppUserAuth { + const parsed = JSON.parse(raw) as Record; + const accessToken = trimmedOrNull(parsed.accessToken); + const refresh = parseLedger(parsed.refresh); + if (!accessToken) return { token: null, refresh }; + return { + token: { + accessToken, + tokenType: trimmedOrNull(parsed.tokenType) ?? "bearer", + scope: trimmedOrNull(parsed.scope), + expiresAt: trimmedOrNull(parsed.expiresAt), + refreshToken: trimmedOrNull(parsed.refreshToken), + refreshTokenExpiresAt: trimmedOrNull(parsed.refreshTokenExpiresAt), + userLogin: trimmedOrNull(parsed.userLogin), + updatedAt: trimmedOrNull(parsed.updatedAt) ?? new Date().toISOString(), + }, + refresh, + }; +} + +function serializeStoredAppUserAuth(stored: StoredAppUserAuth): string | null { + if (!stored.token) return null; + return JSON.stringify({ ...stored.token, refresh: stored.refresh }); +} + +function classifyRefreshFailure(error: unknown): RefreshFailure { + const message = error instanceof Error ? error.message : String(error); + if (error instanceof GitHubOAuthError || (error as { name?: string })?.name === "GitHubOAuthError") { + const oauth = error as GitHubOAuthError; + const status = typeof oauth.status === "number" ? oauth.status : null; + const oauthError = oauth.oauthError ?? null; + const retryAfterSec = oauth.retryAfterSec ?? null; + if (isDefinitiveGitHubOAuthError(oauthError)) { + return { kind: "dead_token", message, status, oauthError, retryAfterSec, dead: true }; + } + if (status === 429 || oauthError === "too_many_requests" || retryAfterSec != null) { + return { kind: "rate_limited", message, status, oauthError, retryAfterSec, dead: false }; + } + if (status != null && status >= 500) { + return { kind: "outage", message, status, oauthError, retryAfterSec, dead: false }; + } + if (status === 400 || status === 401 || status === 403) { + // GitHub reports a rejected credential as HTTP 200 with an error body, so + // a 4xx here is the client credentials or the grant itself being refused. + // Retrying cannot change either. + return { kind: "dead_token", message, status, oauthError, retryAfterSec, dead: true }; + } + return { kind: "unknown", message, status, oauthError, retryAfterSec, dead: false }; + } + return { kind: "network", message, status: null, oauthError: null, retryAfterSec: null, dead: false }; +} + +function refreshBackoffMs(failure: RefreshFailure, consecutiveFailures: number): number { + const exponential = Math.min( + REFRESH_BACKOFF_BASE_MS * 2 ** Math.max(0, consecutiveFailures - 1), + REFRESH_BACKOFF_MAX_MS, + ); + const requested = failure.retryAfterSec != null ? failure.retryAfterSec * 1_000 : 0; + return Math.max(exponential, requested); +} + export function createGitHubAppUserAuthService(args: { credentialStore?: GitHubAppUserAuthCredentialStore | null; logger: GitHubAppUserAuthLogger; fetchImpl: (input: string, init?: RequestInit) => Promise; userAgent: string; + /** + * Names the storage this service shares with its siblings. Defaults to what + * the store reports, so every service instance over one credential file lands + * on one coordinator. + */ + storeIdentity?: string | null; + now?: () => number; + sleep?: (ms: number) => Promise; }): { getAuthStatus(patch?: Partial): GitHubAppUserAuthStatus; startDeviceAuth(): Promise; @@ -54,19 +380,29 @@ export function createGitHubAppUserAuthService(args: { auditLog: GitHubRelayAuthAuditLog; } { const appDeviceAuthSessions = new Map(); - let appUserTokenMemory: GitHubAppUserTokenRecord | null = null; - let refreshInFlight: Promise | null = null; + const now = args.now ?? (() => Date.now()); + const sleep = args.sleep ?? ((ms: number) => new Promise((resolve) => { + const timer = setTimeout(resolve, ms); + timer.unref?.(); + })); + const leaseHolderId = randomUUID(); + // Only serves a service built WITHOUT a credential store. A store that reads + // empty is a cleared credential, not a cache miss: falling back to memory + // there resurrected credentials another process had signed out. + let appUserTokenMemory: StoredAppUserAuth | null = null; // Bumped whenever stored auth is replaced or cleared outside the refresh // path, so an in-flight refresh cannot overwrite the newer auth state. let authEpoch = 0; + const storeIdentity = resolveStoreIdentity(args.credentialStore, args.storeIdentity); + const auditLog = createGitHubRelayAuthAuditLog(args.logger.info.bind(args.logger)); const pruneExpiredDeviceAuthSessions = (requestedSessionId?: string): boolean => { - const now = Date.now(); + const nowMs = now(); let requestedExpired = false; for (const [sessionId, session] of appDeviceAuthSessions.entries()) { - if (Date.parse(session.expiresAt) <= now) { + if (Date.parse(session.expiresAt) <= nowMs) { appDeviceAuthSessions.delete(sessionId); if (sessionId === requestedSessionId) requestedExpired = true; } @@ -74,41 +410,87 @@ export function createGitHubAppUserAuthService(args: { return requestedExpired; }; - const readAppUserTokenRecord = (): GitHubAppUserTokenRecord | null => { + const readStoredAuth = (): StoredAppUserAuth => { + if (!args.credentialStore) return appUserTokenMemory ?? { token: null, refresh: emptyLedger() }; try { - const raw = args.credentialStore?.getSync(GITHUB_APP_USER_TOKEN_KEY)?.trim() || ""; - if (!raw) return appUserTokenMemory; - const parsed = JSON.parse(raw) as Partial; - if (typeof parsed.accessToken !== "string" || !parsed.accessToken.trim()) return null; - return { - accessToken: parsed.accessToken.trim(), - tokenType: typeof parsed.tokenType === "string" && parsed.tokenType.trim() ? parsed.tokenType.trim() : "bearer", - scope: typeof parsed.scope === "string" && parsed.scope.trim() ? parsed.scope.trim() : null, - expiresAt: typeof parsed.expiresAt === "string" && parsed.expiresAt.trim() ? parsed.expiresAt.trim() : null, - refreshToken: typeof parsed.refreshToken === "string" && parsed.refreshToken.trim() ? parsed.refreshToken.trim() : null, - refreshTokenExpiresAt: - typeof parsed.refreshTokenExpiresAt === "string" && parsed.refreshTokenExpiresAt.trim() - ? parsed.refreshTokenExpiresAt.trim() - : null, - userLogin: typeof parsed.userLogin === "string" && parsed.userLogin.trim() ? parsed.userLogin.trim() : null, - updatedAt: typeof parsed.updatedAt === "string" && parsed.updatedAt.trim() ? parsed.updatedAt.trim() : new Date().toISOString(), - }; + const raw = args.credentialStore.getSync(GITHUB_APP_USER_TOKEN_KEY)?.trim() || ""; + if (!raw) return { token: null, refresh: emptyLedger() }; + return parseStoredAppUserAuth(raw); } catch { args.logger.warn("github.app_user_token_read_failed", { error: "failed to parse stored app user token", }); - return null; + return { token: null, refresh: emptyLedger() }; + } + }; + + const readAppUserTokenRecord = (): GitHubAppUserTokenRecord | null => readStoredAuth().token; + + const writeStoredAuth = (stored: StoredAppUserAuth | null): void => { + appUserTokenMemory = stored; + if (!args.credentialStore) return; + try { + const serialized = stored ? serializeStoredAppUserAuth(stored) : null; + if (serialized) args.credentialStore.setSync(GITHUB_APP_USER_TOKEN_KEY, serialized); + else args.credentialStore.deleteSync(GITHUB_APP_USER_TOKEN_KEY); + } catch (error) { + args.logger.warn("github.app_user_token_write_failed", { + error: error instanceof Error ? error.message : String(error), + }); + throw error; } }; const persistAppUserTokenRecord = (record: GitHubAppUserTokenRecord | null): void => { - appUserTokenMemory = record; + writeStoredAuth(record ? { token: record, refresh: emptyLedger() } : null); + }; + + /** + * Applies `mutator` to the stored credential as one step, and returns what the + * mutator decided. A store without `updateKeySync` degrades to a plain + * read-modify-write: still correct inside one process, and the only stores + * without it are process-local ones. + */ + const updateStoredAuth = ( + mutator: (stored: StoredAppUserAuth) => { next: StoredAppUserAuth | null | undefined; result: T }, + ): T => { + let result!: T; + const parseCurrent = (current: string | null): StoredAppUserAuth => { + if (!current?.trim()) return { token: null, refresh: emptyLedger() }; + try { + return parseStoredAppUserAuth(current); + } catch { + // Unparseable is reported as absent, and every mutator declines to write + // over an absent record — so a corrupt value is left for a reader to + // report rather than overwritten from inside a refresh. + return { token: null, refresh: emptyLedger() }; + } + }; + const applyRaw = (current: string | null): string | null | undefined => { + const stored = parseCurrent(current); + const decision = mutator(stored); + result = decision.result; + if (decision.next === undefined) return undefined; + if (decision.next === null) return null; + appUserTokenMemory = decision.next; + return serializeStoredAppUserAuth(decision.next); + }; + const store = args.credentialStore; + if (!store) { + const decision = mutator(appUserTokenMemory ?? { token: null, refresh: emptyLedger() }); + if (decision.next !== undefined) appUserTokenMemory = decision.next; + return decision.result; + } try { - if (record) { - args.credentialStore?.setSync(GITHUB_APP_USER_TOKEN_KEY, JSON.stringify(record)); - } else { - args.credentialStore?.deleteSync(GITHUB_APP_USER_TOKEN_KEY); + if (store.updateKeySync) { + store.updateKeySync(GITHUB_APP_USER_TOKEN_KEY, applyRaw); + return result; } + const next = applyRaw(store.getSync(GITHUB_APP_USER_TOKEN_KEY)?.trim() || null); + if (next === undefined) return result; + if (next === null) store.deleteSync(GITHUB_APP_USER_TOKEN_KEY); + else store.setSync(GITHUB_APP_USER_TOKEN_KEY, next); + return result; } catch (error) { args.logger.warn("github.app_user_token_write_failed", { error: error instanceof Error ? error.message : String(error), @@ -117,26 +499,47 @@ export function createGitHubAppUserAuthService(args: { } }; + const refreshTokenUnusable = (record: GitHubAppUserTokenRecord): boolean => { + // A missing refreshTokenExpiresAt is unknown, not expired — attempt the + // refresh instead of writing off a possibly-valid credential. + if (!record.refreshToken) return true; + return record.refreshTokenExpiresAt != null + && !readIsoAfter(record.refreshTokenExpiresAt, now()); + }; + + const credentialStateOf = (stored: StoredAppUserAuth): GitHubAppUserAuthCredentialState => { + if (!stored.token?.accessToken) return "missing"; + if (stored.refresh.dead || refreshTokenUnusable(stored.token)) return "needs_reauth"; + if (readIsoAfter(stored.refresh.notBeforeAt, now())) return "blocked"; + return "authorized"; + }; + const appUserAuthStatus = (patch: Partial = {}): GitHubAppUserAuthStatus => { - const record = readAppUserTokenRecord(); + const stored = readStoredAuth(); + const credentialState = credentialStateOf(stored); + const failure = stored.refresh.lastFailure; return { configured: true, - tokenStored: Boolean(record?.accessToken), - userLogin: record?.userLogin ?? null, - expiresAt: record?.expiresAt ?? null, - refreshTokenExpiresAt: record?.refreshTokenExpiresAt ?? null, - checkedAt: new Date().toISOString(), + tokenStored: Boolean(stored.token?.accessToken), + userLogin: stored.token?.userLogin ?? null, + expiresAt: stored.token?.expiresAt ?? null, + refreshTokenExpiresAt: stored.token?.refreshTokenExpiresAt ?? null, + credentialState, + refreshBlockedUntil: credentialState === "blocked" ? stored.refresh.notBeforeAt : null, + lastRefreshError: failure + ? { + kind: failure.kind, + message: failure.message, + status: failure.status, + at: failure.at, + } + : null, + checkedAt: new Date(now()).toISOString(), error: null, ...patch, }; }; - const isIsoAfter = (iso: string | null, cutoffMs: number): boolean => { - if (!iso) return false; - const time = Date.parse(iso); - return Number.isFinite(time) && time > cutoffMs; - }; - const fetchAppUserLogin = async (accessToken: string): Promise => { const response = await args.fetchImpl("https://api.github.com/user", { method: "GET", @@ -152,45 +555,254 @@ export function createGitHubAppUserAuthService(args: { return asString(payload.login).trim() || null; }; - const getValidAppUserTokenForRelay = async (): Promise => { - const record = readAppUserTokenRecord(); - if (!record?.accessToken) { - throw new Error("Authorize the ADE GitHub App with GitHub before using the hosted relay."); - } - const refreshCutoff = Date.now() + GITHUB_APP_USER_TOKEN_REFRESH_SKEW_MS; - if (!record.expiresAt || isIsoAfter(record.expiresAt, refreshCutoff)) { - return record.accessToken; - } - // A missing refreshTokenExpiresAt is unknown, not expired — attempt the - // refresh instead of deleting a possibly-valid credential. - const refreshTokenDead = record.refreshTokenExpiresAt != null - && !isIsoAfter(record.refreshTokenExpiresAt, Date.now()); - if (!record.refreshToken || refreshTokenDead) { - persistAppUserTokenRecord(null); - throw new Error("ADE GitHub App authorization expired. Re-authorize ADE with GitHub."); + const missingAuthError = (): GitHubAppUserAuthError => new GitHubAppUserAuthError( + "Authorize the ADE GitHub App with GitHub before using the hosted relay.", + "missing", + null, + null, + null, + null, + ); + + const needsReauthError = (failure: RefreshLedger["lastFailure"]): GitHubAppUserAuthError => + new GitHubAppUserAuthError( + "ADE GitHub App authorization expired. Re-authorize ADE with GitHub.", + "needs_reauth", + null, + failure?.kind ?? null, + failure?.status ?? null, + failure?.oauthError ?? null, + ); + + const blockedError = ( + retryAt: string | null, + failure: RefreshLedger["lastFailure"], + ): GitHubAppUserAuthError => new GitHubAppUserAuthError( + // The deadline rides on `retryAt` rather than inside the sentence: callers + // that show it to a person format it, and callers that log it read the field. + "GitHub paused ADE's authorization renewal. ADE retries on its own.", + "blocked", + retryAt, + failure?.kind ?? null, + failure?.status ?? null, + failure?.oauthError ?? null, + ); + + const isAccessTokenFresh = (record: GitHubAppUserTokenRecord): boolean => { + const refreshCutoff = now() + GITHUB_APP_USER_TOKEN_REFRESH_SKEW_MS; + return !record.expiresAt || readIsoAfter(record.expiresAt, refreshCutoff); + }; + + /** + * Takes the refresh lease, or reports who already holds it. + * + * The record to POST is captured INSIDE this atomic update. Capturing it from + * an earlier read leaves a window where a peer finishes its refresh, rotates + * the token, and releases the lease — and the old refresh token gets POSTed + * anyway, which GitHub answers by revoking the credential. For the same + * reason a token found already fresh here is returned instead of leased. + */ + const acquireRefreshLease = (): { + acquired: boolean; + leaseUntil: string | null; + record: GitHubAppUserTokenRecord | null; + alreadyFresh: boolean; + } => + updateStoredAuth<{ + acquired: boolean; + leaseUntil: string | null; + record: GitHubAppUserTokenRecord | null; + alreadyFresh: boolean; + }>((stored) => { + if (!stored.token) { + return { next: undefined, result: { acquired: false, leaseUntil: null, record: null, alreadyFresh: false } }; + } + if (isAccessTokenFresh(stored.token)) { + return { + next: undefined, + result: { acquired: false, leaseUntil: null, record: stored.token, alreadyFresh: true }, + }; + } + const held = readIsoAfter(stored.refresh.leaseUntil, now()) + && stored.refresh.leaseHolder !== leaseHolderId; + if (held) { + return { + next: undefined, + result: { acquired: false, leaseUntil: stored.refresh.leaseUntil, record: null, alreadyFresh: false }, + }; + } + const leaseUntil = new Date(now() + REFRESH_LEASE_MS).toISOString(); + return { + next: { + token: stored.token, + refresh: { ...stored.refresh, leaseUntil, leaseHolder: leaseHolderId }, + }, + result: { acquired: true, leaseUntil, record: stored.token, alreadyFresh: false }, + }; + }); + + const persistRefreshSuccess = (refreshed: GitHubAppUserTokenRecord): number => + updateStoredAuth((stored) => { + // A record that vanished while the POST was in flight was cleared by a + // sign-out. Writing the refreshed token back would undo it. + if (!stored.token) return { next: undefined, result: 0 }; + const generation = stored.refresh.generation + 1; + return { + next: { token: refreshed, refresh: { ...emptyLedger(), generation } }, + result: generation, + }; + }); + + type PersistedRefreshFailure = { + notBeforeAt: string | null; + backoffMs: number; + consecutiveFailures: number; + }; + + const persistRefreshFailure = (failure: RefreshFailure): PersistedRefreshFailure => + updateStoredAuth((stored) => { + if (!stored.token) { + return { next: undefined, result: { notBeforeAt: null, backoffMs: 0, consecutiveFailures: 0 } }; } - const epochAtJoin = authEpoch; - if (!refreshInFlight) { - refreshInFlight = refreshGitHubAppUserToken({ + const consecutiveFailures = stored.refresh.consecutiveFailures + 1; + const backoffMs = refreshBackoffMs(failure, consecutiveFailures); + const notBeforeAt = new Date(now() + backoffMs).toISOString(); + return { + next: { + // The credential itself is kept even when it is dead: the UI can only + // say "re-authorize" if it can still see that a credential is there. + token: stored.token, + refresh: { + ...stored.refresh, + notBeforeAt, + consecutiveFailures, + dead: stored.refresh.dead || failure.dead, + leaseUntil: null, + leaseHolder: null, + lastFailure: { + kind: failure.kind, + message: failure.message, + status: failure.status, + oauthError: failure.oauthError, + retryAfterSec: failure.retryAfterSec, + at: new Date(now()).toISOString(), + }, + }, + }, + result: { notBeforeAt, backoffMs, consecutiveFailures }, + }; + }); + + const runRefreshPost = async (record: GitHubAppUserTokenRecord): Promise => { + const epochAtStart = authEpoch; + try { + const refreshed = await refreshGitHubAppUserToken({ clientId: ADE_GITHUB_APP_CLIENT_ID, - refreshToken: record.refreshToken, + refreshToken: record.refreshToken!, fetchImpl: (input, init) => args.fetchImpl(String(input), init), userAgent: args.userAgent, fetchUserLogin: fetchAppUserLogin, - }).then((refreshed) => { - if (authEpoch === epochAtJoin) persistAppUserTokenRecord(refreshed); - return refreshed; - }).finally(() => { - refreshInFlight = null; }); + if (authEpoch !== epochAtStart) { + // Auth was cleared or replaced while this POST was in flight; the store + // is the truth, not this result. + const current = readStoredAuth(); + if (!current.token) throw missingAuthError(); + return current.token; + } + const generation = persistRefreshSuccess(refreshed); + args.logger.info("github.app_user_token_refresh_succeeded", { + generation, + userLogin: refreshed.userLogin, + expiresAt: refreshed.expiresAt, + refreshTokenRotated: refreshed.refreshToken !== record.refreshToken, + }); + return refreshed; + } catch (error) { + if (error instanceof GitHubAppUserAuthError) throw error; + const failure = classifyRefreshFailure(error); + const persisted = persistRefreshFailure(failure); + args.logger.warn("github.app_user_token_refresh_failed", { + error: failure.message, + kind: failure.kind, + status: failure.status, + oauthError: failure.oauthError, + retryAfterSec: failure.retryAfterSec, + backoffMs: persisted.backoffMs, + consecutiveFailures: persisted.consecutiveFailures, + dead: failure.dead, + }); + throw failure.dead + ? needsReauthError({ + kind: failure.kind, + message: failure.message, + status: failure.status, + oauthError: failure.oauthError, + retryAfterSec: failure.retryAfterSec, + at: new Date(now()).toISOString(), + }) + : blockedError(persisted.notBeforeAt, { + kind: failure.kind, + message: failure.message, + status: failure.status, + oauthError: failure.oauthError, + retryAfterSec: failure.retryAfterSec, + at: new Date(now()).toISOString(), + }); } - const refreshed = await refreshInFlight; - if (authEpoch !== epochAtJoin) { - // Auth state changed while refreshing (cleared or re-authorized). - // Resolve against the current state instead of the stale refresh. - return getValidAppUserTokenForRelay(); + }; + + /** + * The one refresh body, run under this process's coordinator and the on-disk + * lease. Every gate here is checked against a FRESH read of the store, so a + * peer's outcome is honoured instead of raced. + */ + const refreshUnderLease = async (): Promise => { + for (let attempt = 0; attempt <= LEASE_POLL_MAX_ATTEMPTS; attempt += 1) { + const stored = readStoredAuth(); + if (!stored.token?.accessToken) throw missingAuthError(); + if (isAccessTokenFresh(stored.token)) return stored.token; + if (stored.refresh.dead) throw needsReauthError(stored.refresh.lastFailure); + if (refreshTokenUnusable(stored.token)) throw needsReauthError(stored.refresh.lastFailure); + if (readIsoAfter(stored.refresh.notBeforeAt, now())) { + throw blockedError(stored.refresh.notBeforeAt, stored.refresh.lastFailure); + } + const lease = acquireRefreshLease(); + if (lease.alreadyFresh && lease.record) return lease.record; + if (lease.acquired && lease.record) return await runRefreshPost(lease.record); + if (lease.acquired) throw missingAuthError(); + // A peer is mid-refresh. Never POST the same refresh token behind it: the + // peer may already have rotated it, and GitHub answers a reused refresh + // token by revoking the credential outright. + if (attempt === LEASE_POLL_MAX_ATTEMPTS) { + throw blockedError(lease.leaseUntil, stored.refresh.lastFailure); + } + await sleep(LEASE_POLL_INTERVAL_MS); + } + throw blockedError(null, readStoredAuth().refresh.lastFailure); + }; + + const getValidAppUserTokenForRelay = async (): Promise => { + const stored = readStoredAuth(); + if (!stored.token?.accessToken) throw missingAuthError(); + if (isAccessTokenFresh(stored.token)) return stored.token.accessToken; + if (stored.refresh.dead) throw needsReauthError(stored.refresh.lastFailure); + if (readIsoAfter(stored.refresh.notBeforeAt, now())) { + throw blockedError(stored.refresh.notBeforeAt, stored.refresh.lastFailure); + } + const coordinator = coordinatorFor(storeIdentity); + if (!coordinator.inFlight) { + coordinator.inFlight = refreshUnderLease(); + // Nothing is awaiting this copy, and an unhandled rejection here would be + // reported before the real awaiters attach. + coordinator.inFlight.catch(() => undefined); + } + const inFlight = coordinator.inFlight; + try { + return (await inFlight).accessToken; + } finally { + if (coordinator.inFlight === inFlight) coordinator.inFlight = null; } - return refreshed.accessToken; }; const startDeviceAuth = async (): Promise => { @@ -202,11 +814,30 @@ export function createGitHubAppUserAuthService(args: { if (!oldest) break; appDeviceAuthSessions.delete(oldest); } - const device = await startGitHubAppDeviceFlow({ - clientId: ADE_GITHUB_APP_CLIENT_ID, - fetchImpl: (input, init) => args.fetchImpl(String(input), init), - userAgent: args.userAgent, - }); + let device: GitHubAppDeviceCode; + try { + device = await startGitHubAppDeviceFlow({ + clientId: ADE_GITHUB_APP_CLIENT_ID, + fetchImpl: (input, init) => args.fetchImpl(String(input), init), + userAgent: args.userAgent, + }); + } catch (error) { + const described = describeGitHubAppUserAuthFailure(error); + args.logger.warn("github.app_user_device_start_failed", { + error: described.message, + status: described.status, + oauthError: described.oauthError, + }); + // Re-authorizing is what a user reaches for when the credential broke, and + // it goes to the same host that is throttling ADE. Say that, rather than + // handing back a status code. + if (described.status === 429) { + throw new Error( + "GitHub is rate-limiting ADE's sign-in requests right now. Try again in a few minutes.", + ); + } + throw error; + } const sessionId = randomUUID(); appDeviceAuthSessions.set(sessionId, { ...device, sessionId }); return { @@ -238,14 +869,37 @@ export function createGitHubAppUserAuthService(args: { authStatus: appUserAuthStatus(), }; } - const result = await pollGitHubAppDeviceFlow({ - clientId: ADE_GITHUB_APP_CLIENT_ID, - deviceCode: session.deviceCode, - intervalSec: session.intervalSec, - fetchImpl: (input, init) => args.fetchImpl(String(input), init), - userAgent: args.userAgent, - fetchUserLogin: fetchAppUserLogin, - }); + let result: Awaited>; + try { + result = await pollGitHubAppDeviceFlow({ + clientId: ADE_GITHUB_APP_CLIENT_ID, + deviceCode: session.deviceCode, + intervalSec: session.intervalSec, + fetchImpl: (input, init) => args.fetchImpl(String(input), init), + userAgent: args.userAgent, + fetchUserLogin: fetchAppUserLogin, + }); + } catch (error) { + // A transport failure is a poll RESULT, not a thrown IPC error: the caller + // is a polling UI, and it can only pace itself if it is told what + // happened. A 429 here is GitHub throttling ADE, not a denied user. + const described = describeGitHubAppUserAuthFailure(error); + const rateLimited = described.status === 429; + const message = rateLimited + ? "GitHub is rate-limiting ADE's sign-in requests right now. Try again in a few minutes." + : described.message; + args.logger.warn("github.app_user_device_poll_failed", { + error: described.message, + status: described.status, + oauthError: described.oauthError, + }); + return { + status: "error", + intervalSec: null, + message, + authStatus: appUserAuthStatus({ error: message }), + }; + } if (result.status === "pending" || result.status === "slow_down") { session.intervalSec = result.intervalSec; appDeviceAuthSessions.set(session.sessionId, session); diff --git a/apps/desktop/src/main/services/github/githubRateLimit.ts b/apps/desktop/src/main/services/github/githubRateLimit.ts index 46d1050d98..cdc8cbbfb0 100644 --- a/apps/desktop/src/main/services/github/githubRateLimit.ts +++ b/apps/desktop/src/main/services/github/githubRateLimit.ts @@ -1,5 +1,6 @@ import type { GitHubAuthFailure, GitHubRateLimitState } from "../../../shared/types"; import { isGithubServiceUnavailable } from "../../../shared/githubServiceHealth"; +import { isDefinitiveGitHubOAuthError } from "./githubAppUserAuth"; export class GitHubRateLimitError extends Error { constructor( @@ -59,11 +60,27 @@ export function classifyGitHubAuthFailure(args: { status?: number; message: string; headers?: Pick; + /** + * The OAuth `error` code, when the failure came from GitHub's OAuth endpoints. + * Those answer a rejected refresh token with HTTP 200, so the status alone + * cannot tell a dead credential from a healthy response. + */ + oauthError?: string | null; + /** + * A deadline the caller already knows — the refresh backoff, typically. It + * wins over anything derived from headers, because it is the instant ADE will + * actually try again. + */ + retryAt?: string | null; }): { authFailure: GitHubAuthFailure; rateLimit: GitHubRateLimitState | null } { const message = args.message.trim() || "GitHub token validation failed."; const rateLimit = args.headers ? readGitHubRateLimitState(args.headers) : null; + const retryAfterSeconds = args.headers ? parseHeaderInteger(args.headers.get("retry-after")) : null; + const knownRetryAt = args.retryAt?.trim() || null; const rateLimited = args.status === 429 + || retryAfterSeconds != null + || args.oauthError === "too_many_requests" || rateLimit?.remaining === 0 || /rate limit|too many requests|abuse detection/i.test(message); if (rateLimited) { @@ -72,11 +89,16 @@ export function classifyGitHubAuthFailure(args: { authFailure: { kind: "rate_limited", message, - retryAt: args.headers ? rateLimitRetryAt(args.headers, rateLimit) : rateLimit?.resetAt ?? null, + retryAt: knownRetryAt + ?? (args.headers ? rateLimitRetryAt(args.headers, rateLimit) : rateLimit?.resetAt ?? null), }, }; } - if (args.status === 401 || /bad credentials|invalid token|requires authentication/i.test(message)) { + if ( + args.status === 401 + || isDefinitiveGitHubOAuthError(args.oauthError) + || /bad credentials|invalid token|requires authentication/i.test(message) + ) { return { rateLimit, authFailure: { @@ -92,7 +114,7 @@ export function classifyGitHubAuthFailure(args: { authFailure: { kind: "permission_denied", message, - retryAt: null, + retryAt: knownRetryAt, }, }; } @@ -105,7 +127,7 @@ export function classifyGitHubAuthFailure(args: { authFailure: { kind: "service_unavailable", message, - retryAt: null, + retryAt: knownRetryAt, }, }; } @@ -115,7 +137,7 @@ export function classifyGitHubAuthFailure(args: { authFailure: { kind: "network", message, - retryAt: null, + retryAt: knownRetryAt, }, }; } @@ -124,7 +146,7 @@ export function classifyGitHubAuthFailure(args: { authFailure: { kind: "unknown", message, - retryAt: null, + retryAt: knownRetryAt, }, }; } diff --git a/apps/desktop/src/main/services/github/githubRelayConfig.ts b/apps/desktop/src/main/services/github/githubRelayConfig.ts index 22cefad74e..3314cd9a5e 100644 --- a/apps/desktop/src/main/services/github/githubRelayConfig.ts +++ b/apps/desktop/src/main/services/github/githubRelayConfig.ts @@ -1,5 +1,9 @@ import { createHmac } from "node:crypto"; -import type { GitHubAppInstallationStatus, GitHubRepoRef } from "../../../shared/types"; +import type { + GitHubAppInstallationStatus, + GitHubAppUserAuthCredentialState, + GitHubRepoRef, +} from "../../../shared/types"; export const ADE_GITHUB_APP_DISPLAY_NAME = "ADE"; export const ADE_GITHUB_APP_SLUG = "ade-for-github"; @@ -199,12 +203,39 @@ function normalizeRelayStatusPayload( }); } +/** + * Why ADE has no GitHub App user token for this check. + * + * Carried through so the repo axis can name the account problem instead of + * reporting the relay's 401 — which reads as "this repository is broken" when + * the truth is "ADE's own authorization is not usable right now". + */ +export type GitHubAppUserAuthUnavailable = { + message: string; + credentialState: GitHubAppUserAuthCredentialState | null; + retryAt: string | null; +}; + +function appUserAuthUnavailableCopy(failure: GitHubAppUserAuthUnavailable): string { + if (failure.credentialState === "blocked") { + // The deadline itself travels as `retryAt` on the auth status, where a + // surface can format it as a time a person reads. A raw timestamp in a + // sentence is a log line, not a message. + return "Waiting on GitHub authorization. GitHub paused ADE's renewal; ADE retries on its own."; + } + if (failure.credentialState === "needs_reauth") { + return "ADE's GitHub authorization expired. Re-authorize ADE with GitHub."; + } + return failure.message; +} + export async function fetchGitHubAppInstallationStatus(args: { repo: GitHubRepoRef | null; secretReader?: GitHubRelaySecretReader | null; fetchImpl?: typeof fetch; forceRefresh?: boolean; githubAppUserToken?: string | null; + appUserAuthFailure?: GitHubAppUserAuthUnavailable | null; accountAccessToken?: string | null; auditLog?: GitHubRelayAuthAuditLog | null; }): Promise { @@ -236,11 +267,14 @@ export async function fetchGitHubAppInstallationStatus(args: { const hostedAuth = useLegacyProjectRoute ? null : resolveHostedGitHubRelayAuthToken({ githubAppUserToken }); + const appUserAuthFailure = args.appUserAuthFailure ?? null; if (hostedAuth && !hostedAuth.ok && !accountAccessToken) { return baseStatus(args.repo, { relayConfigured: true, state: "error", - error: hostedAuth.error, + error: appUserAuthFailure + ? appUserAuthUnavailableCopy(appUserAuthFailure) + : hostedAuth.error, }); } const authToken = useLegacyProjectRoute @@ -275,9 +309,15 @@ export async function fetchGitHubAppInstallationStatus(args: { }); const payload = await response.json().catch(() => ({})); if (!response.ok) { - const message = payload && typeof payload === "object" && "error" in payload + const relayMessage = payload && typeof payload === "object" && "error" in payload ? String((payload as { error?: unknown }).error) : `GitHub App relay status check failed (${response.status})`; + // A 401 with no usable App user token is the account problem the relay + // sees from the outside. Report the account problem, not the relay's + // wording for it. + const message = response.status === 401 && appUserAuthFailure + ? appUserAuthUnavailableCopy(appUserAuthFailure) + : relayMessage; return baseStatus(args.repo, { relayConfigured: true, state: "error", diff --git a/apps/desktop/src/main/services/github/githubService.test.ts b/apps/desktop/src/main/services/github/githubService.test.ts index e2c3019e5b..bffe05574f 100644 --- a/apps/desktop/src/main/services/github/githubService.test.ts +++ b/apps/desktop/src/main/services/github/githubService.test.ts @@ -1538,6 +1538,31 @@ describe("githubService.getStatus", () => { ); }); + it("reports a throttled GitHub App refresh as rate limited with the retry time", async () => { + stubOriginRemote(); + const credentialStore = new MemoryCredentialStore(); + credentialStore.setSync("github.appUserToken.v1", JSON.stringify({ + accessToken: "ghu_expiring_app_token", + tokenType: "bearer", + scope: null, + expiresAt: new Date(Date.now() - 60_000).toISOString(), + refreshToken: "ghr_refresh_token", + refreshTokenExpiresAt: new Date(Date.now() + 180 * 24 * 3_600_000).toISOString(), + userLogin: "alice", + updatedAt: new Date().toISOString(), + })); + mockFetch.mockResolvedValueOnce(jsonResponse(429, { error: "too_many_requests" }, { "retry-after": "120" })); + + const status = await makeService({ credentialStore }).getStatus(); + + expect(status.authFailure?.kind).toBe("rate_limited"); + // The deadline is ADE's own backoff, which is what actually gates the next + // attempt — a cosmetic retryAt would tell the user to wait for nothing. + const retryAtMs = Date.parse(status.authFailure?.retryAt ?? ""); + expect(retryAtMs).toBeGreaterThan(Date.now()); + expect(retryAtMs).toBeLessThanOrEqual(Date.now() + 121_000); + }); + it("reports a GitHub App refresh failure instead of treating authorization as missing", async () => { stubOriginRemote(); const credentialStore = new MemoryCredentialStore(); @@ -1564,7 +1589,10 @@ describe("githubService.getStatus", () => { connected: false, authFailure: { kind: "invalid_token", - message: "Bad credentials", + // GitHub's own wording ("Bad credentials") is kept for the log and the + // stored refresh ledger; what reaches a status surface is the sentence + // that says what to do about it. + message: "ADE GitHub App authorization expired. Re-authorize ADE with GitHub.", retryAt: null, }, credentialStates: expect.arrayContaining([ @@ -2826,6 +2854,33 @@ describe("githubService.getAppInstallationStatus", () => { expect(mockFetch).not.toHaveBeenCalled(); }); + it("names the account problem instead of the relay's 401 when authorization is paused", async () => { + const credentialStore = new MemoryCredentialStore(); + credentialStore.setSync("github.appUserToken.v1", JSON.stringify({ + accessToken: "ghu_stale_app_token", + tokenType: "bearer", + scope: null, + expiresAt: new Date(Date.now() - 60_000).toISOString(), + refreshToken: "ghr_refresh_token", + refreshTokenExpiresAt: new Date(Date.now() + 180 * 24 * 3_600_000).toISOString(), + userLogin: "alice", + updatedAt: new Date().toISOString(), + })); + // GitHub throttles the refresh, then the relay answers the token-less status + // request with its own "GitHub auth token is required". + mockFetch.mockResolvedValueOnce(jsonResponse(429, { error: "too_many_requests" }, { "retry-after": "120" })); + mockFetch.mockResolvedValueOnce(jsonResponse(401, { ok: false, error: "GitHub auth token is required" })); + + const status = await makeService({ + credentialStore, + getAccountAccessToken: async () => "clerk-account-token", + }).getAppInstallationStatus({ owner: "acme", name: "repo" }); + + expect(status.state).toBe("error"); + expect(status.error).not.toBe("GitHub auth token is required"); + expect(status.error).toContain("Waiting on GitHub authorization"); + }); + it("does not use the user's existing GitHub token for hosted relay checks", async () => { process.env.ADE_GITHUB_TOKEN = "ghp_user_token"; diff --git a/apps/desktop/src/main/services/github/githubService.ts b/apps/desktop/src/main/services/github/githubService.ts index 564bccbfc9..63e5ac14dd 100644 --- a/apps/desktop/src/main/services/github/githubService.ts +++ b/apps/desktop/src/main/services/github/githubService.ts @@ -24,6 +24,7 @@ import { parseGithubRemoteUrl } from "../../../shared/githubRemote"; import { parseGitHubScopeHeaders } from "../../../shared/githubScopes"; import type { SyncCredentialStore } from "../../../../../ade-cli/src/services/credentials/credentialStore"; import { + GITHUB_CREDENTIAL_INVENTORY_CACHE_TTL_MS, evaluateGithubCredentialCapabilities, githubOperationCredentialCandidates, githubOperationCredentialPrecedence, @@ -41,7 +42,10 @@ import { import { createGithubConditionalRequestCache } from "../../../shared/githubConditionalRequestCache"; import { mergePathEntries, resolveExecutableFromKnownLocations } from "../ai/cliExecutableResolver"; import { fetchGitHubAppInstallationStatus, type GitHubRelaySecretReader } from "./githubRelayConfig"; -import { createGitHubAppUserAuthService } from "./githubAppUserAuthService"; +import { + classifyAppUserAuthFailure, + createGitHubAppUserAuthService, +} from "./githubAppUserAuthService"; import { GITHUB_REST_API_VERSION } from "./githubApiVersion"; import { readCredentialWithState } from "./credentialReadState"; import { @@ -87,7 +91,6 @@ const MACHINE_TOKEN_KEY = "github.token.v1"; const GITHUB_API_TIMEOUT_MS = 20_000; export const GITHUB_API_BODY_TIMEOUT_MS = 30_000; const GH_AUTH_TOKEN_CACHE_TTL_MS = 30_000; -const GITHUB_CREDENTIAL_INVENTORY_CACHE_TTL_MS = 30_000; const GH_HOSTS_TOKEN_CACHE_MAX_ENTRIES = 32; const GITHUB_STATUS_FAILURE_COOLDOWN_MS = 30_000; const execFileAsync = promisify(execFile); @@ -863,11 +866,13 @@ export function createGithubService({ ? appUserAuth.getValidTokenForRelay() .then((token) => ({ token, failure: null })) .catch((error: unknown) => { - const message = error instanceof Error ? error.message : String(error); - const failure = classifyGitHubAuthFailure({ message }); - logger.warn("github.app_user_token_refresh_failed", { - error: message, + const failure = classifyAppUserAuthFailure(error); + logger.warn("github.app_user_token_unavailable", { + error: failure.described.message, kind: failure.authFailure.kind, + credentialState: failure.described.credentialState, + status: failure.described.status, + oauthError: failure.described.oauthError, retryAt: failure.authFailure.retryAt, }); return { token: null, failure }; @@ -919,7 +924,11 @@ export function createGithubService({ candidates, availableSources: new Set(candidates.map((candidate) => candidate.source)), failures: appResult.failure - ? [{ source: "app", ...appResult.failure }] + ? [{ + source: "app" as const, + authFailure: appResult.failure.authFailure, + rateLimit: appResult.failure.rateLimit, + }] : [], appTokenStored, patTokenStored, @@ -2075,7 +2084,21 @@ export function createGithubService({ const owner = args.owner?.trim(); const name = args.name?.trim(); const repo = owner && name ? { owner, name } : await detectRepo(); - const githubAppUserToken = await appUserAuth.getValidTokenForRelay().catch(() => null); + // The reason this token is unavailable decides what the repo axis may say. + // Swallowing it is how "ADE's authorization is paused" reached the user as + // the relay's own "GitHub auth token is required" 401. + const appUserToken = await appUserAuth.getValidTokenForRelay() + .then((token) => ({ token, failure: null })) + .catch((error: unknown) => { + const failure = classifyAppUserAuthFailure(error); + logger.warn("github.app_installation_status_auth_unavailable", { + error: failure.described.message, + kind: failure.authFailure.kind, + credentialState: failure.described.credentialState, + retryAt: failure.authFailure.retryAt, + }); + return { token: null, failure }; + }); const accountAccessToken = getAccountAccessToken ? await getAccountAccessToken().catch(() => null) : null; @@ -2083,7 +2106,14 @@ export function createGithubService({ repo, secretReader: githubRelaySecretReader, forceRefresh: args.forceRefresh === true, - githubAppUserToken, + githubAppUserToken: appUserToken.token, + appUserAuthFailure: appUserToken.failure + ? { + message: appUserToken.failure.described.message, + credentialState: appUserToken.failure.described.credentialState, + retryAt: appUserToken.failure.authFailure.retryAt, + } + : null, accountAccessToken, auditLog: appUserAuth.auditLog, }); diff --git a/apps/desktop/src/renderer/browserMock.ts b/apps/desktop/src/renderer/browserMock.ts index c4258271bd..e8cd92494c 100644 --- a/apps/desktop/src/renderer/browserMock.ts +++ b/apps/desktop/src/renderer/browserMock.ts @@ -5938,6 +5938,9 @@ if (typeof window !== "undefined" && shouldInstallBrowserMock(window)) { userLogin: "arul", expiresAt: new Date(Date.now() + 8 * 60 * 60 * 1000).toISOString(), refreshTokenExpiresAt: new Date(Date.now() + 180 * 24 * 60 * 60 * 1000).toISOString(), + credentialState: "authorized", + refreshBlockedUntil: null, + lastRefreshError: null, checkedAt: new Date().toISOString(), error: null, }), @@ -5959,6 +5962,9 @@ if (typeof window !== "undefined" && shouldInstallBrowserMock(window)) { userLogin: "arul", expiresAt: new Date(Date.now() + 8 * 60 * 60 * 1000).toISOString(), refreshTokenExpiresAt: new Date(Date.now() + 180 * 24 * 60 * 60 * 1000).toISOString(), + credentialState: "authorized", + refreshBlockedUntil: null, + lastRefreshError: null, checkedAt: new Date().toISOString(), error: null, }, @@ -5969,6 +5975,9 @@ if (typeof window !== "undefined" && shouldInstallBrowserMock(window)) { userLogin: null, expiresAt: null, refreshTokenExpiresAt: null, + credentialState: "missing", + refreshBlockedUntil: null, + lastRefreshError: null, checkedAt: new Date().toISOString(), error: null, }), diff --git a/apps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsx b/apps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsx index 0bde218543..fe28fab61a 100644 --- a/apps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsx +++ b/apps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsx @@ -71,6 +71,9 @@ function makeAuth(overrides: Partial = {}): GitHubAppUs userLogin: "octocat", expiresAt: null, refreshTokenExpiresAt: null, + credentialState: "authorized", + refreshBlockedUntil: null, + lastRefreshError: null, checkedAt: "2026-07-01T00:00:00.000Z", error: null, ...overrides, diff --git a/apps/desktop/src/renderer/components/app/IntegrationBannerHost.tsx b/apps/desktop/src/renderer/components/app/IntegrationBannerHost.tsx index 1a693f10ff..8bd8c907e5 100644 --- a/apps/desktop/src/renderer/components/app/IntegrationBannerHost.tsx +++ b/apps/desktop/src/renderer/components/app/IntegrationBannerHost.tsx @@ -250,7 +250,7 @@ export function IntegrationBannerHost({ if (!currentProjectRoot) return; if (appStatusLoaded && loadedRoot === currentProjectRoot) { const account = deriveGithubAccountAuthState(appAuth); - const repo = deriveGithubRepoConnectionState(appInstall); + const repo = deriveGithubRepoConnectionState(appInstall, account); if (account === "valid") clearDismissal("github-app-account"); if (repo === "connected") { const repoKey = appInstall?.repo ? `${appInstall.repo.owner}/${appInstall.repo.name}` : currentProjectRoot; @@ -334,7 +334,7 @@ export function IntegrationBannerHost({ && (!rawAuth || typeof rawAuth.configured === "boolean"); if (!githubSuppressed && appStatusLoaded && currentProjectRoot && loadedRoot === currentProjectRoot && githubAppStatusSupported) { const account = deriveGithubAccountAuthState(appAuth); - const repo = deriveGithubRepoConnectionState(appInstall); + const repo = deriveGithubRepoConnectionState(appInstall, account); const block = deriveGithubRealtimeBlock(account, repo); if (block?.kind === "account") { const copy = githubAccountIssueCopy(block.account); diff --git a/apps/desktop/src/renderer/components/github/GitHubAppInstallPanel.test.tsx b/apps/desktop/src/renderer/components/github/GitHubAppInstallPanel.test.tsx index d3e957ae67..4bad588be6 100644 --- a/apps/desktop/src/renderer/components/github/GitHubAppInstallPanel.test.tsx +++ b/apps/desktop/src/renderer/components/github/GitHubAppInstallPanel.test.tsx @@ -2,10 +2,48 @@ import React from "react"; import { afterEach, describe, expect, it, vi } from "vitest"; -import { cleanup, render, screen } from "@testing-library/react"; -import type { GitHubAppInstallationStatus } from "../../../shared/types"; +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import type { GitHubAppInstallationStatus, GitHubAppUserAuthStatus } from "../../../shared/types"; import { GitHubAppInstallPanel } from "./GitHubAppInstallPanel"; +function makeAppAuth(overrides: Partial = {}): GitHubAppUserAuthStatus { + return { + configured: true, + tokenStored: true, + userLogin: "arul28", + expiresAt: new Date(Date.now() - 2 * 60 * 60 * 1000).toISOString(), + refreshTokenExpiresAt: new Date(Date.now() + 180 * 24 * 60 * 60 * 1000).toISOString(), + credentialState: "authorized", + refreshBlockedUntil: null, + lastRefreshError: null, + checkedAt: new Date().toISOString(), + error: null, + ...overrides, + }; +} + +function installedStatus(): GitHubAppInstallationStatus { + return { + repo: { owner: "arul28", name: "ADE" }, + appName: "ADE", + appSlug: "ade-for-github", + installUrl: "https://github.com/apps/ade-for-github/installations/new", + manageUrl: "https://github.com/settings/installations", + relayConfigured: true, + installed: true, + state: "configured", + installationId: 1, + repositorySelection: "all", + lastSeenAt: null, + webhookEvents: ["pull_request"], + missingWebhookEvents: [], + webhookState: "active", + webhookLastSeenAt: null, + checkedAt: new Date().toISOString(), + error: null, + }; +} + describe("GitHubAppInstallPanel", () => { const originalAde = window.ade; @@ -49,4 +87,48 @@ describe("GitHubAppInstallPanel", () => { expect(screen.queryByText(/still authorized/i)).toBeNull(); expect(screen.queryByText(/re-authorizing is not needed/i)).toBeNull(); }); + + // The device flow polls the same OAuth host that is refusing the renewals, so + // offering the button here is what kept the account locked out. + it("offers no re-authorize button while GitHub has renewals paused", async () => { + const startAppUserDeviceAuth = vi.fn(); + window.ade = { + github: { + getAppInstallationStatus: vi.fn(async () => installedStatus()), + getAppUserAuthStatus: vi.fn(async () => makeAppAuth({ + credentialState: "blocked", + refreshBlockedUntil: new Date(Date.now() + 30 * 60 * 1000).toISOString(), + lastRefreshError: { kind: "rate_limited", message: "429", status: 429, at: new Date().toISOString() }, + })), + startAppUserDeviceAuth, + }, + } as unknown as typeof window.ade; + + render(); + + expect(await screen.findByText(/^Paused until /)).toBeTruthy(); + expect(screen.queryByRole("button", { name: /re-authorize/i })).toBeNull(); + expect(screen.queryByText(/authorization expired/i)).toBeNull(); + expect(startAppUserDeviceAuth).not.toHaveBeenCalled(); + }); + + it("clears the stored authorization only after the disconnect is confirmed", async () => { + const clearAppUserAuth = vi.fn(async () => makeAppAuth({ tokenStored: false, credentialState: "missing" })); + window.ade = { + github: { + getAppInstallationStatus: vi.fn(async () => installedStatus()), + getAppUserAuthStatus: vi.fn(async () => makeAppAuth()), + clearAppUserAuth, + }, + } as unknown as typeof window.ade; + + render(); + + fireEvent.click(await screen.findByRole("button", { name: "Disconnect" })); + expect(clearAppUserAuth).not.toHaveBeenCalled(); + + fireEvent.click(screen.getByRole("button", { name: "Confirm disconnect" })); + expect(clearAppUserAuth).toHaveBeenCalledTimes(1); + expect(await screen.findByText("Not authorized")).toBeTruthy(); + }); }); diff --git a/apps/desktop/src/renderer/components/github/GitHubAppInstallPanel.tsx b/apps/desktop/src/renderer/components/github/GitHubAppInstallPanel.tsx index 95ade73a84..ccc7d00788 100644 --- a/apps/desktop/src/renderer/components/github/GitHubAppInstallPanel.tsx +++ b/apps/desktop/src/renderer/components/github/GitHubAppInstallPanel.tsx @@ -11,11 +11,14 @@ import { COLORS, MONO_FONT, SANS_FONT, cardStyle, inlineBadge, outlineButton, pr import { deriveGithubAccountAuthState, deriveGithubRepoConnectionState, - githubAccountIssueCopy, + describeGithubAccountAxis, githubRepoIssueCopy, + isGithubAuthorizationPausedMessage, isGithubRateLimitMessage, isGithubRealtimeHealthy, isGithubRepoAccessPending, + type GithubAccountAuthState, + type GithubAccountAxisTone, } from "../../lib/githubIntegrationStatus"; import { isGithubServiceUnavailable } from "../../../shared/githubServiceHealth"; @@ -38,6 +41,8 @@ export function GitHubAppInstallPanel({ variant = "settings" }: GitHubAppInstall const [deviceCodeCopied, setDeviceCodeCopied] = useState(false); const [loading, setLoading] = useState(false); const [authLoading, setAuthLoading] = useState(false); + const [disconnectArmed, setDisconnectArmed] = useState(false); + const [disconnecting, setDisconnecting] = useState(false); const autoRenewCountRef = useRef(0); const copyFeedbackTimeoutRef = useRef(null); const appAuthRef = useRef(null); @@ -122,12 +127,28 @@ export function GitHubAppInstallPanel({ variant = "settings" }: GitHubAppInstall setDeviceSession(session); openExternalUrl(session.verificationUriComplete ?? session.verificationUri); } catch (error) { - setDeviceMessage(error instanceof Error ? error.message : String(error)); + setDeviceMessage(deviceAuthErrorCopy(error)); } finally { setAuthLoading(false); } }, []); + const disconnectAppAuthorization = useCallback(async () => { + if (!window.ade?.github?.clearAppUserAuth) return; + setDisconnecting(true); + try { + const next = await window.ade.github.clearAppUserAuth(); + setAppAuth(next ?? null); + setDeviceSession(null); + setDeviceMessage("ADE's GitHub authorization was removed on this machine."); + } catch (error) { + setDeviceMessage(deviceAuthErrorCopy(error)); + } finally { + setDisconnecting(false); + setDisconnectArmed(false); + } + }, []); + const copyDeviceCode = useCallback(async () => { if (!deviceSession) return; try { @@ -156,14 +177,14 @@ export function GitHubAppInstallPanel({ variant = "settings" }: GitHubAppInstall result = { status: "error", intervalSec: null, - message: error instanceof Error ? error.message : String(error), + message: deviceAuthErrorCopy(error), authStatus: appAuthRef.current, }; } if (cancelled || !result) return; setAppAuth(result.authStatus); if (result.status === "pending" || result.status === "slow_down") { - setDeviceMessage(result.message); + setDeviceMessage(deviceAuthMessageCopy(result.message)); setDeviceSession({ ...deviceSession, intervalSec: result.intervalSec ?? deviceSession.intervalSec }); return; } @@ -182,12 +203,12 @@ export function GitHubAppInstallPanel({ variant = "settings" }: GitHubAppInstall } catch (error) { if (cancelled) return; setDeviceSession(null); - setDeviceMessage(error instanceof Error ? error.message : String(error)); + setDeviceMessage(deviceAuthErrorCopy(error)); } return; } setDeviceSession(null); - setDeviceMessage(result.message); + setDeviceMessage(deviceAuthMessageCopy(result.message)); if (result.status === "authorized") { autoRenewCountRef.current = 0; setDeviceMessage("GitHub authorization is complete. Checking repository access..."); @@ -227,8 +248,8 @@ export function GitHubAppInstallPanel({ variant = "settings" }: GitHubAppInstall // Two independent axes, derived by the shared helper so Settings and the // banner can never disagree. const accountState = deriveGithubAccountAuthState(appAuth); - const repoState = deriveGithubRepoConnectionState(status); - const appAuthorized = accountState !== "missing"; + const repoState = deriveGithubRepoConnectionState(status, accountState); + const appAuthorized = accountState === "valid"; const repoLabel = status?.repo ? `${status.repo.owner}/${status.repo.name}` : null; const healthy = isGithubRealtimeHealthy(accountState, repoState); // `appAuth === null` = not fetched yet (distinct from a fetched "no token"). @@ -238,19 +259,57 @@ export function GitHubAppInstallPanel({ variant = "settings" }: GitHubAppInstall const primaryBtnStyle = primaryButton(compact ? compactPrimaryButtonStyle : undefined); const authBusy = authLoading || Boolean(deviceSession); - const accountCta = ( + const account = describeGithubAccountAxis(accountChecking ? "checking" : accountState, appAuth); + // No button in the states ADE recovers from on its own. Offering one while + // GitHub has the account paused is what sent users back through the device + // flow against the very endpoint that was refusing them. + const accountCta = account.cta ? ( - ); - - const account = accountView(accountState, appAuth, accountChecking); + ) : null; + + // Unobtrusive by design: a plain muted control, and only where there is + // something to disconnect. Two clicks, because clearing the credential stops + // real-time updates until the user goes through the device flow again. + const showDisconnect = accountState !== "missing" + && !accountChecking + && !deviceSession + // Hosts without the clear action (the web client, older brains) must not + // show a control that silently does nothing. + && typeof window.ade?.github?.clearAppUserAuth === "function"; + const disconnectControl = showDisconnect ? ( + disconnectArmed ? ( + <> + + + + ) : ( + + ) + ) : null; const recheckButton = (primaryStyle: boolean) => (