mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-08-01 12:52:11 +03:00
fix(oauth): resolve Kiro AWS SSO cache client credentials by clientId match (port from 9router#1253) (#7122)
tryAwsSsoCache() only resolved clientId/clientSecret via data.clientIdHash -> <hash>.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)
This commit is contained in:
committed by
GitHub
parent
f9e95a12db
commit
2b24448bd6
1
changelog.d/fixes/1253-kiro-sso-cache-clientid.md
Normal file
1
changelog.d/fixes/1253-kiro-sso-cache-clientid.md
Normal file
@@ -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).
|
||||
@@ -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();
|
||||
|
||||
@@ -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)
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -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 —
|
||||
|
||||
211
tests/unit/kiro-sso-cache-direct-clientid-1253.test.ts
Normal file
211
tests/unit/kiro-sso-cache-direct-clientid-1253.test.ts
Normal file
@@ -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` -> `<hash>.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<string, unknown>) {
|
||||
fs.mkdirSync(dir, { recursive: true });
|
||||
fs.writeFileSync(path.join(dir, file), JSON.stringify(data));
|
||||
}
|
||||
|
||||
async function callGet(): Promise<{ status: number; body: Record<string, unknown> }> {
|
||||
const request = new Request("http://localhost/api/oauth/kiro/auto-import");
|
||||
const response = await GET(request);
|
||||
const body = (await response.json()) as Record<string, unknown>;
|
||||
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<string, unknown>[] = [];
|
||||
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)}`
|
||||
);
|
||||
});
|
||||
Reference in New Issue
Block a user