Skip to content
This repository was archived by the owner on Aug 6, 2026. It is now read-only.

fix(auth): Log out on rejected refresh token instead of stalling startup - #2812

Merged
charlesvien merged 2 commits into
mainfrom
fix/auth-400-refresh-logout
Jun 21, 2026
Merged

fix(auth): Log out on rejected refresh token instead of stalling startup#2812
charlesvien merged 2 commits into
mainfrom
fix/auth-400-refresh-logout

Conversation

@charlesvien

@charlesviencharlesvien commented Jun 20, 2026

Copy link
Copy Markdown
Member

Problem

When a stored session holds a refresh token the server rejects (you logged in against a dev instance, or the token expired or was revoked), the desktop app hung on a blank "Loading..." screen and, 25 seconds later, showed "PostHog is taking longer than expected to start" with Retry and Get support. Restarting did not help, because the dead token was retried on every launch.

Root cause: the OAuth2 token endpoint returns an invalid, expired or revoked refresh token as HTTP 400 (invalid_grant, RFC 6749 section 5.2). Our OAuth client only classified 401/403 as auth errors, so a 400 fell through to unknown_error. That code is non-retryable and does not clear the stored session, so AuthService dropped bootstrap back into the restoring state with bootstrapComplete: false and never recovered. The UI sat on the loading screen until the 25s stall fallback fired.

Console from a failing launch:

(oauth-service) Token refresh failed: 400 Bad Request
Failed to restore stored auth session

Changes

  • packages/core/src/oauth/oauth.ts: classify HTTP 400 as an auth_error alongside 401/403. A rejected refresh token now takes the existing forced-logout path in AuthService.refreshSession: it clears the stored session, publishes anonymous state (bootstrapComplete: true) and routes the user straight to the login screen. No retries, no spinner, no stall screen.
  • packages/core/src/oauth/oauth.test.ts: add a case asserting 400 is classified as auth_error, and repoint the other-4xx case to 404.

Unchanged: 5xx is still retried (server_error), fetch failures stay network_error and other 4xx stay unknown_error. The bootstrap stall fallback is kept as a safety net for a genuinely slow boot; it just no longer triggers for auth failures.

How did you test this?

  • pnpm --filter @posthog/core test: full core suite, 1616 passed. Covers both layers of the fix: oauth.test.ts proves a 400 is classified as auth_error, and auth.test.ts ("does not retry on auth_error and forces logout") proves that an auth_error clears the session and returns anonymous state.
  • pnpm --filter @posthog/core typecheck: clean.
  • biome lint on the two changed files: clean.
  • Pre-commit hook ran repo-wide pnpm typecheck and Biome on commit: passed.

Not done: I did not reproduce the hang in the live Electron app. The coverage above is unit level across the classifier and the logout path.

Automatic notifications

  • Publish to changelog?
  • Alert Sales and Marketing teams?

@charlesvienGraphite App

charlesvien commented Jun 20, 2026

Copy link
Copy Markdown
MemberAuthor

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions

github-actionsBot commented Jun 20, 2026

Copy link
Copy Markdown

React Doctor found no issues in the changed files. 🎉

Reviewed by React Doctor for commit f6a8bfa.

@charlesviencharlesvien changed the title fail fast to logout on invalid refresh tokenfix(auth): Log out on rejected refresh token instead of stalling startupJun 20, 2026
@greptile-apps

greptile-appsBot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Comments Outside Diff (1)

  1. packages/core/src/oauth/oauth.test.ts, line 106-133 (link)

    P2Prefer parameterised tests for auth-error status codes

    Three tests now share identical structure — call refreshToken, assert success: false / errorCode: "auth_error" — differing only in the HTTP status. The project's stated preference is parameterised tests. Collapsing these into an it.each would express the shared intent once, make it trivial to add a new auth-error code, and remove the risk of the three assertions drifting out of sync.

    Context Used: Do not attempt to comment on incorrect alphabetica... (source)

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: packages/core/src/oauth/oauth.test.ts
    Line: 106-133
    Comment:
    **Prefer parameterised tests for auth-error status codes**
    Three tests now share identical structure — call `refreshToken`, assert `success: false` / `errorCode: "auth_error"` — differing only in the HTTP status. The project's stated preference is parameterised tests. Collapsing these into an `it.each` would express the shared intent once, make it trivial to add a new auth-error code, and remove the risk of the three assertions drifting out of sync.
    **Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
    How can I resolve this? If you propose a fix, please make it concise.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---### Issue 1 of 2
packages/core/src/oauth/oauth.test.ts:106-133
**Prefer parameterised tests for auth-error status codes**
Three tests now share identical structure — call `refreshToken`, assert `success: false` / `errorCode: "auth_error"` — differing only in the HTTP status. The project's stated preference is parameterised tests. Collapsing these into an `it.each` would express the shared intent once, make it trivial to add a new auth-error code, and remove the risk of the three assertions drifting out of sync.
### Issue 2 of 2
packages/core/src/oauth/oauth.ts:222-225
**All 400s classified as auth errors, including non-grant errors**
Per RFC 6749, the token endpoint returns 400 for several error codes beyond `invalid_grant` — notably `invalid_client` (misconfigured `client_id`) and `invalid_request` (malformed body). With this change, any of those would also trigger a logout. For `invalid_client` specifically, the user would be logged out and then fail to log back in with the same broken config, potentially creating an unrecoverable loop. Consider inspecting the response body for the OAuth `error` field (`"invalid_grant"`, `"invalid_token"`) before promoting a 400 to an `auth_error`, so that configuration-level failures are surfaced differently.

Reviews (1): Last reviewed commit: "fail fast to logout on invalid refresh t..." | Re-trigger Greptile

Comment threadpackages/core/src/oauth/oauth.ts Outdated
@charlesvien
charlesvien changed the base branch from refactor/electron-vite to graphite-base/2812June 20, 2026 22:55
@charlesvien
charlesvienforce-pushed the fix/auth-400-refresh-logout branch from a883f15 to 5c85f4eCompareJune 20, 2026 22:55
@charlesvien
charlesvien changed the base branch from graphite-base/2812 to mainJune 20, 2026 22:55
@charlesviencharlesvien added the Stamphog This will request an autostamp by stamphog on small changes label Jun 20, 2026

@stamphogstamphogBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bot raised a valid unresolved concern: treating all HTTP 400s as auth errors is too broad — RFC 6749 returns 400 for invalid_client and invalid_request too, which should not trigger logout. The fix should check the error field in the response body for invalid_grant specifically, not blanket all 400s.

@stamphogstamphogBot removed the Stamphog This will request an autostamp by stamphog on small changes label Jun 20, 2026
@charlesvien
charlesvienforce-pushed the fix/auth-400-refresh-logout branch from 5c85f4e to de89b91CompareJune 20, 2026 23:00
@charlesviencharlesvien added the Stamphog This will request an autostamp by stamphog on small changes label Jun 20, 2026

@stamphogstamphogBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The auth deny-list gate blocked this PR. While the code correctly narrows 400 handling to only invalid_grant/invalid_token (addressing the bot reviewer's concern), auth/token refresh logic requires a human security reviewer to approve — automated gates are configured to require it here.

@stamphogstamphogBot removed the Stamphog This will request an autostamp by stamphog on small changes label Jun 20, 2026
@charlesviencharlesvien added the Stamphog This will request an autostamp by stamphog on small changes label Jun 21, 2026

@stamphogstamphogBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR touches auth token refresh logic and was blocked by the auth deny-list gate, which requires a human security reviewer to approve changes in this area. Request a human security review before re-requesting automation approval.

@stamphogstamphogBot removed the Stamphog This will request an autostamp by stamphog on small changes label Jun 21, 2026
@charlesviencharlesvien added the Stamphog This will request an autostamp by stamphog on small changes label Jun 21, 2026

@stamphogstamphogBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auth token refresh logic is in the deny-list and requires a human security reviewer to approve. The automated gates are configured to block all changes in this area unconditionally.

@stamphogstamphogBot removed the Stamphog This will request an autostamp by stamphog on small changes label Jun 21, 2026
@charlesvien
charlesvienforce-pushed the fix/auth-400-refresh-logout branch from de89b91 to aabb9d8CompareJune 21, 2026 05:23
@greptile-apps

Copy link
Copy Markdown
Contributor

Reviews (2): Last reviewed commit: "fail fast to logout on invalid refresh t..." | Re-trigger Greptile

@greptile-apps

Copy link
Copy Markdown
Contributor

Reviews (3): Last reviewed commit: "extract oauth error parser and cover 400..." | Re-trigger Greptile

@charlesvien
charlesvien enabled auto-merge (squash) June 21, 2026 06:35
@charlesviencharlesvien added the Create Release This will trigger a new release label Jun 21, 2026
@charlesvien
charlesvien merged commit 4ef876f into mainJun 21, 2026
27 checks passed
@charlesvien
charlesvien deleted the fix/auth-400-refresh-logout branch June 21, 2026 08:16
Gilbert09 added a commit that referenced this pull request Jun 22, 2026
…2812)
Mobile had its own OAuth refresh path that threw a generic Error on any
non-ok response, so a transient network/server blip at startup deleted the
tokens and signed the user out.
Classify the refresh failure in `refreshAccessToken` and only sign out when
the refresh token is genuinely dead:
- 401/403, or 400 with `invalid_grant`/`invalid_token` -> auth_error -> sign out
- 5xx -> server_error -> keep session
- network failure -> network_error -> keep session
- other 400s (e.g. invalid_client) -> unknown_error/config -> keep session
`initializeAuth` now keeps the stored session on transient/config failures so
the per-request `authedFetch` retry can recover, and only deletes tokens on a
genuine auth_error.
Ports desktop #2812 to apps/mobile.
Generated-By: PostHog Code
Task-Id: c274cae6-c3d7-46f4-bfe1-96b3514dbc94
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Create ReleaseThis will trigger a new release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@charlesvien@k11kirky