fix: harden OAuth token-response parsing and refresh-error classification#181
Open
santhil-cyber wants to merge 1 commit into
Open
fix: harden OAuth token-response parsing and refresh-error classification#181santhil-cyber wants to merge 1 commit into
santhil-cyber wants to merge 1 commit into
Conversation
…tion The token endpoint responses were read with plain `as string` casts and a `(d.expires_in as number) || 3600` fallback. That mostly works, but leaves a few real gaps: - A truthy non-string access_token (a number, an object) passes the presence check and gets mistyped as a string, surfacing later as a confusing downstream failure instead of a clear parse error. - A non-numeric or negative expires_in (e.g. "soon", -5) flows into `Date.now() + expiresIn * 1000`, producing NaN and a RangeError when we build the expiry date. Both token responses are now validated with Zod (the same .safeParse pattern used in auth-config.ts) and expires_in is coerced safely: numeric strings are accepted, anything non-finite or non-positive falls back to the default lifetime. The existing "Token response missing required fields" error and the omitted-expires_in default are preserved. Separately, classifyTerminalRefreshError required both an invalid_grant code and a specific phrase in the error description to flag a dead refresh token. Per RFC 6749 §5.2, invalid_grant on a refresh request already means the token is expired/revoked/reused, so a backend rewording its description would silently break detection. It now classifies on the structured code, keeping the substring checks as a fallback for servers that don't send a clean code. Closes githits-com#180
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tightens how OAuth token-endpoint responses are parsed and how terminal refresh failures are classified, all in
src/services/auth-service.ts. These are edge-case hardening fixes in a path that runs on every authenticated command, not a reproduced production failure.What changed
Token responses validated with Zod.
parseTokenResponse/parseRefreshTokenResponseusedas stringcasts behind a truthiness check, so a truthy non-stringaccess_token(number, object) slipped through and was mistyped. Replaced with.safeParseschemas, matching the existing pattern inauth-config.ts. The"Token response missing required fields"error is preserved.expires_incoerced safely.(d.expires_in as number) || 3600handles a missing value, but a non-numeric string like"soon"becomesNaN, andDate.now() + NaN * 1000throws aRangeErrorwhen building the expiry inlogin.ts. The schema now accepts numeric strings and falls back to the default lifetime for non-finite, zero, or negative values.invalid_grantclassified by code, not wording.classifyTerminalRefreshErrorrequiredinvalid_grantplus a specific phrase in the error description to detect a dead refresh token. Per RFC 6749 §5.2,invalid_granton a refresh request already means the token is expired/revoked/reused, so a reworded description would silently break detection. It now trusts the structured code, keeping the substring matches as a fallback.Tests
Added cases for numeric-string
expires_in, non-positive/non-numeric fallback, rejection of non-stringaccess_token, andinvalid_grantclassification independent of description text.Local: full
bun testgreen (2276 pass),bun run typecheckclean,bun run buildsucceeds,smoke:cli/smoke:mcppass on the unauthenticated path, biome clean.Closes #180