From 2b24448bd63c2614b05d9569232caeebc4994b47 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> Date: Fri, 17 Jul 2026 10:39:29 -0300 Subject: [PATCH] fix(oauth): resolve Kiro AWS SSO cache client credentials by clientId match (port from 9router#1253) (#7122) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tryAwsSsoCache() only resolved clientId/clientSecret via data.clientIdHash -> .json. Newer kiro-auth-token.json files instead carry a top-level clientId directly, so that lookup silently failed and left clientId/clientSecret null, sending the dashboard's Import Token POST down the non-IDC path. That path (KiroService.validateImportToken -> readCachedClientCredentials) picked a client registration by region + latest-expiry across ALL cached SSO client registrations, ignoring the token's actual clientId — on a machine with multiple stale registrations this returned a mismatched clientId/clientSecret pair, producing 'Bad credentials' on refresh. Fix: resolve clientId/clientSecret by scanning the cache for a registration file whose own clientId matches the token's clientId (falling back to clientIdHash first, then a direct-match scan), and thread an optional clientId hint into readCachedClientCredentials()/validateImportToken() so an exact match always wins over the region/latest-expiry heuristic. Reported-by: Asher (@XCrag) (https://github.com/decolua/9router/issues/1253) --- .../fixes/1253-kiro-sso-cache-clientid.md | 1 + src/app/api/oauth/kiro/auto-import/route.ts | 28 +++ src/app/api/oauth/kiro/import/route.ts | 5 +- src/lib/oauth/services/kiro.ts | 30 ++- ...iro-sso-cache-direct-clientid-1253.test.ts | 211 ++++++++++++++++++ 5 files changed, 270 insertions(+), 5 deletions(-) create mode 100644 changelog.d/fixes/1253-kiro-sso-cache-clientid.md create mode 100644 tests/unit/kiro-sso-cache-direct-clientid-1253.test.ts diff --git a/changelog.d/fixes/1253-kiro-sso-cache-clientid.md b/changelog.d/fixes/1253-kiro-sso-cache-clientid.md new file mode 100644 index 0000000000..091d8a223c --- /dev/null +++ b/changelog.d/fixes/1253-kiro-sso-cache-clientid.md @@ -0,0 +1 @@ +- **fix(oauth):** resolve Kiro AWS SSO cache client credentials by matching the token's own `clientId` (including tokens with a direct `clientId` field instead of `clientIdHash`) instead of a region/latest-expiry guess, fixing spurious "Bad credentials" on refresh when multiple stale SSO client registrations are cached (thanks @XCrag). diff --git a/src/app/api/oauth/kiro/auto-import/route.ts b/src/app/api/oauth/kiro/auto-import/route.ts index c3df55477e..d3db085568 100755 --- a/src/app/api/oauth/kiro/auto-import/route.ts +++ b/src/app/api/oauth/kiro/auto-import/route.ts @@ -343,6 +343,34 @@ async function tryAwsSsoCache(targetProvider: string): Promise<{ } } + // Newer kiro-auth-token.json files omit `clientIdHash` and instead carry + // the OIDC `clientId` directly on the token object (#1253). In that case + // find the client-registration file whose own `clientId` matches the + // token's `clientId`, rather than leaving clientId/clientSecret unset. + // Matching by exact clientId (not region/latest-expiry) avoids picking + // an unrelated stale registration on hosts with multiple cached SSO + // client registrations. + if (!clientId && data.clientId) { + for (const candidateFile of files) { + if (candidateFile === file || !candidateFile.endsWith(".json")) continue; + try { + const candidateContent = await readFile(join(cachePath, candidateFile), "utf-8"); + const candidateData = JSON.parse(candidateContent); + if ( + candidateData.clientId === data.clientId && + typeof candidateData.clientSecret === "string" && + candidateData.clientSecret + ) { + clientId = candidateData.clientId; + clientSecret = candidateData.clientSecret; + break; + } + } catch { + // Skip unreadable/malformed candidate files. + } + } + } + // Read profileArn from Kiro IDE's profile.json. The region is preserved // verbatim by readKiroIdeProfileArn() (#2314) — see its docstring for why. const profileArn: string | null = await readKiroIdeProfileArn(); diff --git a/src/app/api/oauth/kiro/import/route.ts b/src/app/api/oauth/kiro/import/route.ts index 4e58bfcbd3..ffaa4c9842 100755 --- a/src/app/api/oauth/kiro/import/route.ts +++ b/src/app/api/oauth/kiro/import/route.ts @@ -150,8 +150,11 @@ export async function POST(request: Request) { // Validate and refresh token (through proxy if configured). // validateImportToken also calls registerClient() to obtain a per-connection OIDC // client pair so multiple Kiro accounts do not share a single backend session (#2328). + // When only `clientId` is known (no matching secret was found by auto-import), + // forward it as a hint so the AWS SSO cache lookup matches the token's own + // registration instead of guessing via region/latest-expiry (#1253). tokenData = await runWithProxyContext(proxy, () => - kiroService.validateImportToken(refreshToken.trim(), region) + kiroService.validateImportToken(refreshToken.trim(), region, clientId) ); } diff --git a/src/lib/oauth/services/kiro.ts b/src/lib/oauth/services/kiro.ts index 7023ddffa9..e90b0ca1ad 100644 --- a/src/lib/oauth/services/kiro.ts +++ b/src/lib/oauth/services/kiro.ts @@ -321,15 +321,24 @@ export class KiroService { * If that fails or no cached credentials exist, registers a dedicated OIDC client. * If registerClient() also fails, the import falls back to the shared social-auth refresh path. */ - async validateImportToken(refreshToken: string, region: string = "us-east-1") { + async validateImportToken( + refreshToken: string, + region: string = "us-east-1", + clientIdHint?: string + ) { assertValidAwsRegion(region); // Validate token format if (!refreshToken.startsWith("aorAAAAAG")) { throw new Error("Invalid token format. Token should start with aorAAAAAG..."); } - // Try to read cached clientId/clientSecret from AWS SSO cache (Builder ID tokens) - const cachedClient = await this.readCachedClientCredentials(region); + // Try to read cached clientId/clientSecret from AWS SSO cache (Builder ID tokens). + // When the caller knows the token's own clientId (#1253 — e.g. surfaced by + // auto-import from a direct `clientId` field on the token file), pass it + // through so the cache lookup can match it exactly instead of guessing via + // region + latest-expiry, which can silently adopt an unrelated stale + // client registration on hosts with multiple cached SSO sessions. + const cachedClient = await this.readCachedClientCredentials(region, clientIdHint); // Attempt 1: Try Builder ID refresh using cached credentials if (cachedClient) { @@ -397,7 +406,8 @@ export class KiroService { * the OIDC client registration step of the device code flow. */ private async readCachedClientCredentials( - region?: string + region?: string, + clientIdHint?: string ): Promise<{ clientId: string; clientSecret: string } | null> { try { const { readdir, readFile } = await import("fs/promises"); @@ -431,6 +441,18 @@ export class KiroService { } if (candidates.length === 0) return null; + // When the caller knows the token's own clientId (#1253), an exact match + // is authoritative — it identifies the one registration that can actually + // refresh this token, regardless of region or expiry. Falling through to + // the region/latest-expiry heuristic below for an unmatched hint would + // silently adopt an unrelated (and non-working) client pair. + if (clientIdHint) { + const exactMatch = candidates.find((c) => c.clientId === clientIdHint); + if (exactMatch) { + return { clientId: exactMatch.clientId, clientSecret: exactMatch.clientSecret }; + } + } + // A host can cache OIDC client registrations for several SSO sessions; // adopting the wrong pair makes the Builder ID refresh fail. Prefer a // registration whose region matches the requested import region, then — diff --git a/tests/unit/kiro-sso-cache-direct-clientid-1253.test.ts b/tests/unit/kiro-sso-cache-direct-clientid-1253.test.ts new file mode 100644 index 0000000000..0afef36614 --- /dev/null +++ b/tests/unit/kiro-sso-cache-direct-clientid-1253.test.ts @@ -0,0 +1,211 @@ +/** + * TDD for upstream 9router#1253 — Kiro auto-import "Bad credentials" when the + * cached AWS SSO token carries a direct `clientId` field (no `clientIdHash`). + * + * Newer kiro-auth-token.json files omit `clientIdHash` and instead store the + * OIDC `clientId` directly on the token object. Two related bugs combined to + * break refresh for these tokens: + * + * 1. `tryAwsSsoCache()` (auto-import/route.ts) only ever resolved + * clientId/clientSecret via `data.clientIdHash` -> `.json`. When the + * token instead carries a top-level `clientId`, this lookup silently does + * nothing, so the auto-import response comes back with clientId/clientSecret + * both null even though a matching client-registration file exists in the + * same cache dir. + * 2. Because auto-import lost the clientId/clientSecret pair, the dashboard's + * "Import Token" POST is sent as a plain (non-IDC) import, which routes + * through `KiroService.validateImportToken()` -> + * `readCachedClientCredentials()`. That helper scans *all* client + * registration files in `~/.aws/sso/cache` and picks one by + * region + latest-expiry, ignoring the token's own `clientId` entirely. On + * a machine with multiple stale SSO client registrations this can return a + * clientId/clientSecret pair that does not match the token's actual + * clientId, producing "Bad credentials" on refresh. + * + * Fix: both resolution paths must prefer the client-registration file whose + * `clientId` matches the token's own `clientId`, instead of a + * latest-expiry/region heuristic. + */ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +// ── Hermetic DATA_DIR so DB setup / requireLogin does not hit real disk ────── + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-kiro-1253-data-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.JWT_SECRET = process.env.JWT_SECRET || "test-jwt-secret-1253"; +process.env.API_KEY_SECRET = process.env.API_KEY_SECRET || "test-api-key-secret-1253"; + +const core = await import("../../src/lib/db/core.ts"); + +const { GET } = await import("../../src/app/api/oauth/kiro/auto-import/route.ts"); +const { KiroService } = await import("../../src/lib/oauth/services/kiro.ts"); + +const ORIGINAL_HOME = process.env.HOME; +const ORIGINAL_APPDATA = process.env.APPDATA; +const ORIGINAL_FETCH = globalThis.fetch; + +let tmpHome: string; + +test.beforeEach(() => { + tmpHome = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-kiro-1253-")); + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); + process.env.HOME = tmpHome; + delete process.env.APPDATA; + globalThis.fetch = ORIGINAL_FETCH; +}); + +test.afterEach(() => { + process.env.HOME = ORIGINAL_HOME; + if (ORIGINAL_APPDATA !== undefined) { + process.env.APPDATA = ORIGINAL_APPDATA; + } else { + delete process.env.APPDATA; + } + globalThis.fetch = ORIGINAL_FETCH; + if (tmpHome) fs.rmSync(tmpHome, { recursive: true, force: true }); +}); + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +function cacheDirFor(home: string) { + return path.join(home, ".aws/sso/cache"); +} + +function writeJson(dir: string, file: string, data: Record) { + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, file), JSON.stringify(data)); +} + +async function callGet(): Promise<{ status: number; body: Record }> { + const request = new Request("http://localhost/api/oauth/kiro/auto-import"); + const response = await GET(request); + const body = (await response.json()) as Record; + return { status: response.status, body }; +} + +// ── tryAwsSsoCache() (auto-import route) ───────────────────────────────────── + +test("auto-import: resolves clientId/clientSecret from a direct `clientId` field (no clientIdHash) via matching registration file", async () => { + const cacheDir = cacheDirFor(tmpHome); + + // The token file itself: no clientIdHash, only a direct `clientId`. + writeJson(cacheDir, "kiro-auth-token.json", { + accessToken: "aoa-access", + refreshToken: "aorAAAAAGrefresh-token", + clientId: "correct-client-id", + region: "us-east-1", + provider: "BuilderId", + authMethod: "IdC", + }); + + // Two STALE client registration files with a LATER expiresAt than the correct one — + // the old latest-expiry heuristic would wrongly prefer these. + writeJson(cacheDir, "stale-registration-1.json", { + clientId: "stale-client-id-1", + clientSecret: "stale-secret-1", + region: "us-east-1", + expiresAt: "2099-01-01T00:00:00Z", + }); + writeJson(cacheDir, "stale-registration-2.json", { + clientId: "stale-client-id-2", + clientSecret: "stale-secret-2", + region: "us-east-1", + expiresAt: "2098-01-01T00:00:00Z", + }); + + // The registration file that actually matches the token's own clientId, + // deliberately given the OLDEST expiry so the heuristic must lose to the match. + writeJson(cacheDir, "correct-registration.json", { + clientId: "correct-client-id", + clientSecret: "correct-secret", + region: "us-east-1", + expiresAt: "2020-01-01T00:00:00Z", + }); + + const fetchedUrls: string[] = []; + globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { + const u = String(input); + fetchedUrls.push(u); + if (u.includes("oidc.") && u.endsWith("/token")) { + const bodyStr = String(init?.body || "{}"); + const parsed = JSON.parse(bodyStr); + // Refresh must be attempted with the CORRECT client credentials. + assert.equal(parsed.clientId, "correct-client-id"); + assert.equal(parsed.clientSecret, "correct-secret"); + return new Response( + JSON.stringify({ accessToken: "access-refreshed", refreshToken: "aorAAAAAGrefreshed", expiresIn: 3600 }), + { status: 200, headers: { "Content-Type": "application/json" } } + ); + } + throw new Error(`[kiro-1253 test] unexpected fetch to ${u}`); + }) as typeof fetch; + + const { body } = await callGet(); + + assert.equal(body.found, true, `expected found:true, got: ${JSON.stringify(body)}`); + assert.equal( + fetchedUrls.some((u) => u.includes("oidc.") && u.endsWith("/token")), + true, + `expected OIDC refresh to be attempted with resolved client creds, fetched: ${JSON.stringify(fetchedUrls)}` + ); +}); + +// ── KiroService.readCachedClientCredentials() (via validateImportToken) ───── + +test("KiroService.validateImportToken: prefers the client registration matching the token's own clientId over the latest-expiry heuristic", async () => { + const cacheDir = cacheDirFor(tmpHome); + + writeJson(cacheDir, "stale-registration-1.json", { + clientId: "stale-client-id-1", + clientSecret: "stale-secret-1", + region: "us-east-1", + expiresAt: "2099-01-01T00:00:00Z", + }); + writeJson(cacheDir, "correct-registration.json", { + clientId: "correct-client-id", + clientSecret: "correct-secret", + region: "us-east-1", + expiresAt: "2020-01-01T00:00:00Z", + }); + + const fetchedBodies: Record[] = []; + globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { + const u = String(input); + if (u.includes("oidc.") && u.endsWith("/token")) { + const parsed = JSON.parse(String(init?.body || "{}")); + fetchedBodies.push(parsed); + if (parsed.clientId === "correct-client-id" && parsed.clientSecret === "correct-secret") { + return new Response( + JSON.stringify({ accessToken: "ok-access", refreshToken: "aorAAAAAGok", expiresIn: 3600 }), + { status: 200, headers: { "Content-Type": "application/json" } } + ); + } + return new Response(JSON.stringify({ message: "Bad credentials" }), { status: 400 }); + } + throw new Error(`[kiro-1253 test] unexpected fetch to ${u}`); + }) as typeof fetch; + + const kiroService = new KiroService(); + const result = await kiroService.validateImportToken( + "aorAAAAAGrefresh-token", + "us-east-1", + "correct-client-id" + ); + + assert.equal(result.accessToken, "ok-access"); + assert.ok( + fetchedBodies.some( + (b) => b.clientId === "correct-client-id" && b.clientSecret === "correct-secret" + ), + `expected a refresh attempt using the matching client credentials, got: ${JSON.stringify(fetchedBodies)}` + ); +});