From e784d28e510d2a5b33b95d24d7ebd76c6aca5985 Mon Sep 17 00:00:00 2001 From: robertjamesprior <83608739+robertjamesprior@users.noreply.github.com> Date: Thu, 17 Sep 2026 00:45:29 +0000 Subject: [PATCH 1/2] Record which client failed a token exchange and why Token exchange telemetry records that a stage failed and nothing about who or why, so a failure that has been breaking first-time connections cannot be attributed to a client or a cause. Add the OAuth client_id to the event, and on a provider rejection record the upstream status and the provider's own error code. The code is what separates an expired authorization code from a redirect mismatch or a revoked client; the free-text description that accompanies it is not recorded. Co-Authored-By: Claude Opus 5 --- src/app/token/route.test.ts | 50 +++++++++++++++++++++++++++++++++++++ src/app/token/route.ts | 23 +++++++++++++++++ src/lib/mcp/analytics.ts | 10 ++++++++ 3 files changed, 83 insertions(+) diff --git a/src/app/token/route.test.ts b/src/app/token/route.test.ts index 2dad6c47..c5f3ea5b 100644 --- a/src/app/token/route.test.ts +++ b/src/app/token/route.test.ts @@ -395,4 +395,54 @@ describe("POST /token", () => { expect(missingRefreshResponse.status).toBe(400); expect(missingRefresh.calls.persisted).toHaveLength(0); }); + + test("records the client and the provider's reason when the exchange is rejected", async () => { + const deps = dependencies({ + clerkStatus: 400, + clerkTokens: { + error: "invalid_grant", + error_description: "Code already redeemed", + }, + }); + + const response = await tokenRequest( + request({ + grant_type: "authorization_code", + client_id: "client_1", + code: "already-used", + code_verifier: "verifier_1", + }), + deps.value, + ); + + expect(response.status).toBe(400); + expect(deps.calls.outcomes[0]).toMatchObject({ + stage: "provider_exchange", + outcome: "error", + clientId: "client_1", + providerStatusCode: 400, + providerErrorCode: "invalid_grant", + }); + // The description can carry anything, so it is deliberately not recorded. + expect(JSON.stringify(deps.calls.outcomes[0])).not.toContain("redeemed"); + }); + + test("records the client on a successful exchange and no provider failure fields", async () => { + const deps = dependencies(); + + await tokenRequest( + request({ + grant_type: "authorization_code", + client_id: "client_1", + code: "code_1", + code_verifier: "verifier_1", + }), + deps.value, + ); + + const outcome = deps.calls.outcomes[0]; + expect(outcome).toMatchObject({ outcome: "success", clientId: "client_1" }); + expect(outcome.providerStatusCode).toBeUndefined(); + expect(outcome.providerErrorCode).toBeUndefined(); + }); }); diff --git a/src/app/token/route.ts b/src/app/token/route.ts index 051bd85e..3bbe0a93 100644 --- a/src/app/token/route.ts +++ b/src/app/token/route.ts @@ -161,6 +161,20 @@ async function oauthErrorCode(response: NextResponse): Promise { return response.status >= 500 ? "server_error" : "invalid_grant"; } +// The provider's own error code is the only thing that separates an expired +// code from a redirect mismatch or a revoked client. Record the code, never the +// free-text description that accompanies it. +async function readProviderErrorCode( + response: Response, +): Promise { + try { + const body = (await response.json()) as { error?: unknown }; + return typeof body.error === "string" ? body.error.slice(0, 64) : undefined; + } catch { + return undefined; + } +} + export async function tokenRequest( request: NextRequest, dependencies: TokenDependencies = tokenDependencies, @@ -172,6 +186,9 @@ export async function tokenRequest( "unknown"; let accessScopeForAnalytics: OAuthTokenExchangeAnalytics["accessScope"] = "unknown"; + let clientIdForAnalytics: string | undefined; + let providerStatusCode: number | undefined; + let providerErrorCode: string | undefined; let stage: OAuthTokenExchangeAnalytics["stage"] = "request_validation"; const finish = ( @@ -182,10 +199,13 @@ export async function tokenRequest( dependencies.recordExchange?.({ grantType: grantTypeForAnalytics, clientType: clientTypeForAnalytics, + ...(clientIdForAnalytics ? { clientId: clientIdForAnalytics } : {}), accessScope: accessScopeForAnalytics, stage, outcome: response.ok ? "success" : "error", ...(errorCode ? { errorCode } : {}), + ...(providerStatusCode ? { providerStatusCode } : {}), + ...(providerErrorCode ? { providerErrorCode } : {}), statusCode: response.status, durationMs: Date.now() - startedAt, }); @@ -229,6 +249,7 @@ export async function tokenRequest( grantTypeForAnalytics = normalizedGrantType(grantType); const { clientId } = clientCredentials(request, body, params); clientTypeForAnalytics = clientType(clientId); + clientIdForAnalytics = clientId || undefined; if (!clientId) { return fail("invalid_request", "Missing required parameter: client_id"); } @@ -296,6 +317,8 @@ export async function tokenRequest( }, ); if (!clerkResponse.ok) { + providerStatusCode = clerkResponse.status; + providerErrorCode = await readProviderErrorCode(clerkResponse); return fail( "invalid_grant", grantType === "refresh_token" diff --git a/src/lib/mcp/analytics.ts b/src/lib/mcp/analytics.ts index 3a366bd8..0eb50255 100644 --- a/src/lib/mcp/analytics.ts +++ b/src/lib/mcp/analytics.ts @@ -39,6 +39,8 @@ const projectToken = process.env.POSTHOG_PROJECT_TOKEN; export type OAuthTokenExchangeAnalytics = { grantType: "authorization_code" | "refresh_token" | "unknown"; clientType: "kernel_cli" | "registered_client" | "unknown"; + /** OAuth client_id, so a failure can be attributed to the client that caused it. */ + clientId?: string; accessScope: "organization" | "project" | "unknown"; stage: | "request_validation" @@ -54,6 +56,11 @@ export type OAuthTokenExchangeAnalytics = { | "invalid_grant" | "unsupported_grant_type" | "server_error"; + /** Upstream status when the provider rejected the exchange. */ + providerStatusCode?: number; + /** Upstream OAuth error code, which is the only thing distinguishing an expired + * code from a redirect mismatch or a revoked client. */ + providerErrorCode?: string; statusCode: number; durationMs: number; }; @@ -541,10 +548,13 @@ export function captureOAuthTokenExchange( const properties = { oauth_grant_type: exchange.grantType, oauth_client_type: exchange.clientType, + oauth_client_id: exchange.clientId, oauth_access_scope: exchange.accessScope, oauth_stage: exchange.stage, oauth_outcome: exchange.outcome, oauth_error_code: exchange.errorCode, + oauth_provider_status_code: exchange.providerStatusCode, + oauth_provider_error_code: exchange.providerErrorCode, http_status_code: exchange.statusCode, duration_ms: exchange.durationMs, }; From e1b2915fcc878530164875b0e6be8fc1499621ec Mon Sep 17 00:00:00 2001 From: robertjamesprior <83608739+robertjamesprior@users.noreply.github.com> Date: Fri, 18 Sep 2026 19:41:37 +0000 Subject: [PATCH 2/2] Bound the provider error code and cover the analytics sink Review feedback. The provider's error field was recorded as an arbitrary bounded-length string, which still lets free text, PII and unbounded cardinality reach logs and analytics. Normalize it against the RFC 6749 section 5.2 code set with an `unknown` fallback. Also correct the claim: `invalid_grant` covers expired, revoked, redirect-mismatched and wrong-client grants, so the value narrows a failure rather than identifying its cause. Comments and the test name said otherwise. The route tests only proved the object handed to recordExchange, and Bun treats extra undefined properties as absent, so a mapping typo would have passed. Assert the three PostHog keys directly. Co-Authored-By: Claude Opus 5 --- src/app/token/route.test.ts | 24 +++++++++++++++++++++++- src/app/token/route.ts | 21 +++++++++++++++++---- src/lib/mcp/analytics.test.ts | 31 +++++++++++++++++++++++++++++++ src/lib/mcp/analytics.ts | 4 ++-- 4 files changed, 73 insertions(+), 7 deletions(-) diff --git a/src/app/token/route.test.ts b/src/app/token/route.test.ts index c5f3ea5b..b28ba8d6 100644 --- a/src/app/token/route.test.ts +++ b/src/app/token/route.test.ts @@ -396,7 +396,7 @@ describe("POST /token", () => { expect(missingRefresh.calls.persisted).toHaveLength(0); }); - test("records the client and the provider's reason when the exchange is rejected", async () => { + test("records the client and a bounded provider error code on rejection", async () => { const deps = dependencies({ clerkStatus: 400, clerkTokens: { @@ -427,6 +427,28 @@ describe("POST /token", () => { expect(JSON.stringify(deps.calls.outcomes[0])).not.toContain("redeemed"); }); + test("records an unrecognized provider error code as unknown", async () => { + const deps = dependencies({ + clerkStatus: 400, + clerkTokens: { error: "user_42_is_rate_limited_until_2026" }, + }); + + await tokenRequest( + request({ + grant_type: "authorization_code", + client_id: "client_1", + code: "rejected", + code_verifier: "verifier_1", + }), + deps.value, + ); + + expect(deps.calls.outcomes[0]).toMatchObject({ + providerErrorCode: "unknown", + }); + expect(JSON.stringify(deps.calls.outcomes[0])).not.toContain("user_42"); + }); + test("records the client on a successful exchange and no provider failure fields", async () => { const deps = dependencies(); diff --git a/src/app/token/route.ts b/src/app/token/route.ts index 3bbe0a93..f395ba52 100644 --- a/src/app/token/route.ts +++ b/src/app/token/route.ts @@ -161,15 +161,28 @@ async function oauthErrorCode(response: NextResponse): Promise { return response.status >= 500 ? "server_error" : "invalid_grant"; } -// The provider's own error code is the only thing that separates an expired -// code from a redirect mismatch or a revoked client. Record the code, never the -// free-text description that accompanies it. +// RFC 6749 section 5.2 token endpoint error codes. The provider's code is a +// coarse signal: `invalid_grant` alone covers expired, revoked, redirect- +// mismatched and wrong-client grants, so it narrows a failure rather than +// identifying it. Anything outside this set is recorded as `unknown`, because +// the field is untrusted input and must not carry free text, PII or unbounded +// cardinality into logs and analytics. The description is never recorded. +const PROVIDER_ERROR_CODES = new Set([ + "invalid_request", + "invalid_client", + "invalid_grant", + "unauthorized_client", + "unsupported_grant_type", + "invalid_scope", +]); + async function readProviderErrorCode( response: Response, ): Promise { try { const body = (await response.json()) as { error?: unknown }; - return typeof body.error === "string" ? body.error.slice(0, 64) : undefined; + if (typeof body.error !== "string") return undefined; + return PROVIDER_ERROR_CODES.has(body.error) ? body.error : "unknown"; } catch { return undefined; } diff --git a/src/lib/mcp/analytics.test.ts b/src/lib/mcp/analytics.test.ts index fd019e5e..39a2c068 100644 --- a/src/lib/mcp/analytics.test.ts +++ b/src/lib/mcp/analytics.test.ts @@ -560,6 +560,37 @@ describe("captureOAuthTokenExchange", () => { expect(JSON.stringify(captured)).not.toContain("refresh_token_hash"); expect(JSON.stringify(captured)).not.toContain("code_verifier"); }); + + test("maps client and provider failure fields onto their PostHog keys", () => { + const captured: unknown[] = []; + const fakePosthog = { + capture: (event: unknown) => captured.push(event), + } as unknown as PostHog; + + captureOAuthTokenExchange( + { + grantType: "authorization_code", + clientType: "registered_client", + clientId: "client_1", + accessScope: "organization", + stage: "provider_exchange", + outcome: "error", + errorCode: "invalid_grant", + providerStatusCode: 400, + providerErrorCode: "invalid_grant", + statusCode: 400, + durationMs: 17, + }, + fakePosthog, + ); + + const properties = (captured[0] as { properties: Record }) + .properties; + + expect(properties.oauth_client_id).toBe("client_1"); + expect(properties.oauth_provider_status_code).toBe(400); + expect(properties.oauth_provider_error_code).toBe("invalid_grant"); + }); }); describe("captureMcpConnectionScopeFailure", () => { diff --git a/src/lib/mcp/analytics.ts b/src/lib/mcp/analytics.ts index 0eb50255..c03778a8 100644 --- a/src/lib/mcp/analytics.ts +++ b/src/lib/mcp/analytics.ts @@ -58,8 +58,8 @@ export type OAuthTokenExchangeAnalytics = { | "server_error"; /** Upstream status when the provider rejected the exchange. */ providerStatusCode?: number; - /** Upstream OAuth error code, which is the only thing distinguishing an expired - * code from a redirect mismatch or a revoked client. */ + /** Coarse RFC 6749 section 5.2 error code from the provider, or `unknown` + * for anything outside that set. Narrows a failure; does not identify it. */ providerErrorCode?: string; statusCode: number; durationMs: number;