Skip to content

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

@charlesvien charlesvien 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?

charlesvien commented Jun 20, 2026

Copy link
Copy Markdown
Member Author

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

@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown

React Doctor found no issues in the changed files. 🎉

Reviewed by React Doctor for commit f6a8bfa.

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

greptile-apps Bot 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)

    P2 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)

    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 thread packages/core/src/oauth/oauth.ts Outdated
@charlesvien charlesvien changed the base branch from refactor/electron-vite to graphite-base/2812 June 20, 2026 22:55
@charlesvien charlesvien force-pushed the fix/auth-400-refresh-logout branch from a883f15 to 5c85f4e Compare June 20, 2026 22:55
@charlesvien charlesvien changed the base branch from graphite-base/2812 to main June 20, 2026 22:55
@charlesvien charlesvien added the Stamphog This will request an autostamp by stamphog on small changes label Jun 20, 2026

@stamphog stamphog Bot 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.

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

@stamphog stamphog Bot 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.

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

@stamphog stamphog Bot 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.

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

@stamphog stamphog Bot 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.

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

greptile-apps Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

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

@greptile-apps

greptile-apps Bot commented Jun 21, 2026

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
@charlesvien charlesvien added the Create Release This will trigger a new release label Jun 21, 2026
@charlesvien charlesvien merged commit 4ef876f into main Jun 21, 2026
27 checks passed
@charlesvien charlesvien deleted the fix/auth-400-refresh-logout branch June 21, 2026 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Create Release This will trigger a new release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants