Skip to content

fix: only terminate the session on 400 invalid_grant - #57

Merged
germ-mark merged 4 commits into
mainfrom
llm/refresh-terminal-allowlist
Jul 29, 2026
Merged

fix: only terminate the session on 400 invalid_grant#57
germ-mark merged 4 commits into
mainfrom
llm/refresh-terminal-allowlist

Conversation

@germ-mark

Copy link
Copy Markdown
Contributor

Follow-on to #53, which narrowed session termination on token refresh but still defaulted to terminate: only OAuth.Errors.oauthError was inspected, so anything else fell through to return nil and signed the user out.

Two cases that don't confirm the refresh token is dead still terminated the session:

  • Undecodable error bodies — HTML or empty 5xx from a proxy/CDN throws HTTPResponseError.unsuccessful, not oauthError. Probably the most common transient failure in practice.
  • 400 invalid_requestprocessGenericAccessToken remaps this before the oauthError case; per RFC 6749 it means a malformed request, not a revoked grant.

Inverts the catch into an allowlist: only 400 invalid_grant returns nil, everything else propagates. Preserving is now the default for unrecognized errors.

Stacked on #51 — merge after it lands. Based on feat/token-revocation with main merged in, so CI runs the true post-stack state; the error assertions rely on GermConvenience 0.3.0, which that branch already pins.

Tests: three new cases in RefreshErrorTests (HTML 503, empty-body 502, 400 invalid_request), added in a separate commit that fails against the pre-fix code. Full suite green.

🤖 Generated with Claude Code

@changeset-bot

changeset-bot Bot commented Jul 29, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: ba721c5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@germ-network/oauth4swift Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@germ-mark

Copy link
Copy Markdown
Contributor Author

Extended to cover a third case from a parallel review: TokenRefreshOptions.validate documents a thrown error as "unable to resolve the validity - e.g. if an online check is required and the client is currently offline", but the refresh path caught it and nilled the session just the same — so an offline client (or a transient failure in atproto sub-verification) lost its session. Only an explicit false terminates now; a throw propagates.

That also removes the second do/catch entirely, since a failure fetching authServerMetadata should propagate for the same reason.

@germ-mark

Copy link
Copy Markdown
Contributor Author

Self-review follow-up: a validator shared across both flows may throw tokenInvalid (per TokenAuthorizeOptions's convention) rather than return false — that throw now terminates the session like false does, instead of being preserved-and-retried. Also added the missing happy-path test: a valid refresh returns the new access token and updates the stored token state. 19 tests total.

@germ-mark
germ-mark force-pushed the feat/token-revocation branch from e0375d1 to bd6c767 Compare July 29, 2026 08:25
An error occurred while trying to automatically change base from feat/token-revocation to feat/client-auth-protocols July 29, 2026 08:26
@germ-mark
germ-mark changed the base branch from feat/token-revocation to main July 29, 2026 08:35
germ-mark and others added 4 commits July 29, 2026 01:40
Non-OAuth error bodies and 400 invalid_request are not confirmation that
the refresh token is gone. These three cases fail against the current
allowlist, which only preserves the session for structured OAuth errors.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Invert the refresh catch into an allowlist so undecodable error bodies
(proxy HTML, empty 5xx) and invalid_request propagate instead of nilling
the session. Preserving is now the default for anything unrecognized.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TokenRefreshOptions documents a thrown error as "unable to resolve the
validity", e.g. an offline client, but the refresh path nilled the session
for it just the same. Only an explicit false now terminates.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TokenAuthorizeOptions directs validators to throw tokenInvalid, so a
validator shared across both flows may signal invalidity that way rather
than returning false. Without this, that throw would preserve the session
and retry a definitively invalid response indefinitely.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@germ-mark
germ-mark force-pushed the llm/refresh-terminal-allowlist branch from a55b10d to ba721c5 Compare July 29, 2026 08:43
@germ-mark
germ-mark merged commit aa25826 into main Jul 29, 2026
8 checks passed
@germ-mark
germ-mark deleted the llm/refresh-terminal-allowlist branch July 29, 2026 08:48
@github-actions github-actions Bot mentioned this pull request Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant