Record which client failed a token exchange and why - #199
robertjamesprior wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Context, plus a note on priority. This is the instrument for KERNEL-2279, not a fix. Over the last 30 days in prod, 1,009 first-time connections succeeded and 102 failed at the provider exchange, arriving in bursts: 42% of attempts failed on Sep 9, 37% on Aug 19, 0% on most days. Of the 65 distinct source IPs that hit it, only 16 were ever seen completing an exchange afterwards. The event currently records the stage and nothing else, so there is nothing to investigate with. Codex issue #118 is one report of the same failure and it has been open since June for that reason. 33 lines of non-test code. Two things worth more than a skim: reading the Clerk body on the rejection branch (nothing downstream consumes it, but worth confirming), and whether recording This should not jump the CUA-TS cutover. It has been failing quietly since mid-August and another week changes nothing. |
masnwilliams
left a comment
There was a problem hiding this comment.
requesting two narrow fixes before merge:
-
keep the provider error boundary bounded and make the telemetry claim accurate.
invalid_grantcovers expired, revoked, redirect-mismatched, and wrong-client grants under RFC 6749, so the current field does not distinguish the causes named by the PR. either add a privacy-safe reason classifier with regression cases for distinct causes, or scope the field and surrounding names/comments to the coarse provider OAuth code it actually records. -
cover the final analytics sink. the route tests stop at
recordExchange, so they do not prove the three new values reach PostHog under the intended property names. add concrete non-undefined assertions inanalytics.test.ts.
one rollout note, not a reason to expand this diff across repos: #202 has now moved new-client discovery to the Go authorization server, so this instruments retained legacy TypeScript traffic rather than the canonical path. please update the PR scope and link the Go telemetry follow-up so this is not treated as complete current-path coverage.
validated at 849c0512d467a80566795eff3d0a9bb8700aee8a, including a synthetic merge with current main: typecheck passed, 621 tests passed, changed-file formatting passed, and git diff --check passed.
| ): Promise<string | undefined> { | ||
| try { | ||
| const body = (await response.json()) as { error?: unknown }; | ||
| return typeof body.error === "string" ? body.error.slice(0, 64) : undefined; |
There was a problem hiding this comment.
body.error is still an untrusted string. slicing it bounds length, but it does not prevent free text, PII, or high-cardinality values from reaching logs and PostHog. please normalize against a small provider OAuth-code allowlist (with an unknown fallback) rather than recording arbitrary strings. also, invalid_grant itself does not distinguish the expired/revoked/redirect-mismatch cases named above, so the helper comment and test name currently overstate what this value tells us.
| oauth_outcome: exchange.outcome, | ||
| oauth_error_code: exchange.errorCode, | ||
| oauth_provider_status_code: exchange.providerStatusCode, | ||
| oauth_provider_error_code: exchange.providerErrorCode, |
There was a problem hiding this comment.
please extend the existing captureOAuthTokenExchange test with non-undefined clientId, providerStatusCode, and providerErrorCode values and assert these exact PostHog keys. the route tests only verify the object passed into recordExchange, and Bun treats extra undefined properties as absent in the current sink assertion, so a typo or omitted mapping here would still pass.
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
849c051 to
e1b2915
Compare
|
all three addressed. bounded error code. normalized against the RFC 6749 section 5.2 set with an the overclaim. you were right, the sink. scope. PR body now says this instruments the legacy TypeScript path only, and points at KERNEL-2258 scope item 1 for the Go equivalent. 627 tests pass. one related note: the PostHog alert I built for KERNEL-2279 had the same problem, so it is renamed "legacy TypeScript path" and there is now a SigNoz alert on |
|
correction to something I wrote in the PR body and in KERNEL-2279: I said the Go port already fixed the upstream-reason gap. it does not.
also worth flagging before anyone implements KERNEL-2258 scope item 1: the two fields have different cost on the Go side. the provider error code is bounded to six RFC values plus |
Scope
This instruments the retained legacy TypeScript path. Since #202, new-client discovery points at
auth.onkernel.com, so the canonical path is the Go authorization server, which emitskernel.oauth.token_exchangeto SigNoz and never writes this PostHog event.The Go side needs the same two additions and does not have them:
oauthas/telemetry.gocarriesclient_typeonly, with no client id, and no upstream provider code on the metric. That is scope item 1 of KERNEL-2258 and is not folded in here.So this is deliberately partial coverage. It is still worth landing: the legacy path carries every cached client until they re-register, and it is where the failures in KERNEL-2279 were measured.
Why
oauth_token_exchangerecords the stage a failure happened at and nothing about who hit it or what the provider said.oauth_client_typeonly distinguisheskernel_cli,registered_clientandunknown, so every third-party client is one bucket.src/app/token/route.tscollapsed any non-OK Clerk response into a flatinvalid_grantand discarded the body.Over the 30 days to Sep 16, 1,009 new connections succeeded and 102 failed at the provider exchange, in bursts: 42% of attempts on Sep 9, 0% on most days. Of the 65 distinct source IPs that hit it, 16 were ever seen completing an exchange afterwards.
Codex install issue #118 is one report of this same failure, open since June because nobody could tell from the data which client it was or what Clerk said.
What this adds
oauth_client_id. Not a secret and not personal data for a public PKCE client, stable per registered client, already in the request.oauth_provider_status_codeandoauth_provider_error_code, the latter normalized against the RFC 6749 section 5.2 code set with anunknownfallback.The provider's
error_descriptionis never recorded.On what the code does and does not tell you:
invalid_grantcovers expired, revoked, redirect-mismatched and wrong-client grants, so it narrows a failure rather than identifying its cause. An earlier revision of this PR claimed more than that.Testing
bun test: 627 pass, 0 fail. Route tests cover the rejection path, the success path, and an unrecognized provider code falling back tounknownwithout leaking the original string.analytics.test.tsasserts the three values reach PostHog under their exact property names with non-undefined values, since Bun treats extraundefinedproperties as absent.🤖 Generated with Claude Code