fix(desktop): rehydrate persisted native OAuth tokens with a stored-set parser (#73271) - #73273
Conversation
…et parser (NousResearch#73271) Native OAuth sign-in worked during a process lifetime but a cold Desktop restart reported the user as signed out even though a valid encrypted token record was persisted. `_persistNativeTokens` writes `JSON.stringify(tokens)` where `tokens` is an internal camelCase `NativeTokenSet` (`accessToken`, `refreshToken`, `expiresAt`, …). On reload, `_loadNativeTokens` fed that decrypted JSON to `parseTokenResponse`, which reads the raw gateway response shape (snake_case `access_token`, …). The persisted set has no `access_token`, so the parser threw "Gateway token response missing access_token"; the surrounding catch swallowed it and returned null, stranding a signed-in user on the sign-in recovery screen after every restart. Add `parseStoredTokenSet`, the reload counterpart of `parseTokenResponse`, that validates/normalizes a persisted camelCase set, and use it on the load path. Existing on-disk tokens (already camelCase) now rehydrate. Covered by tests that round-trip a persisted set and guard against regressing to the raw parser on the reload path.
|
Closing as a duplicate of #71524 by @Doud-FR, which is the earlier open implementation of the same fix: it adds the same camelCase If useful, the extra regression test here — asserting |
|
@PRATHAMESH75 Thanks for the thoughtful comparison and for closing this in favor of #71524. I also appreciate you pointing out the additional regression guard around Thanks again for taking the time to investigate the issue and for sharing the extra test — it’s genuinely helpful. |
|
Thanks again @PRATHAMESH75 for pointing this out. I reviewed the additional regression guard from #73273 and agreed that it usefully documents the parser boundary, even though the underlying rejection was already indirectly covered. I’ve now added the equivalent test to #71524 in commit 071baf5ca. It verifies that a persisted camelCase token set is rejected by parseTokenResponse() and accepted by parseStoredTokenSet(). All 19 native OAuth tests and the Electron typecheck pass. Thanks for contributing the extra safeguard — it makes the intent of the fix much clearer for future changes. |
Fixes #73271
Problem
Desktop native OAuth sign-in works during a process lifetime, but a cold restart reports the user as signed out even though a valid, encrypted native token record was persisted. The Desktop log shows
failed to load tokens: Gateway token response missing access_token.Root cause
The persist and reload paths disagree on the token payload shape:
_persistNativeTokenswritesJSON.stringify(tokens), wheretokensis an internal camelCaseNativeTokenSet(accessToken,refreshToken,expiresAt,provider,userId)._loadNativeTokensfed that decrypted JSON toparseTokenResponse, which parses the raw gateway response shape (snake_caseaccess_token, …).A persisted set has no
access_token, soparseTokenResponsethrowsGateway token response missing access_token; the surroundingcatchswallows it and returnsnull, stranding a signed-in user on the sign-in recovery screen after every restart.Fix
Add
parseStoredTokenSet— the reload counterpart ofparseTokenResponse— which validates/normalizes a persisted camelCaseNativeTokenSet, and use it on the load path (_loadNativeTokens). The token-response path (parseTokenResponse) is unchanged for the live/auth/native/token+/auth/native/refreshexchanges. Existing on-disk tokens are already camelCase, so they rehydrate without a re-login.Tests
apps/desktop/electron/native-oauth.test.ts:parseStoredTokenSetround-trips a persisted set throughJSON.stringify→JSON.parse(the on-disk encoding) and rehydrates it.parseTokenResponsethrows on a persisted camelCase set — encoding why the reload path must useparseStoredTokenSet.parseTokenResponsecoverage.No token value is exposed to reproduce or fix this; the failure is entirely after safe-storage decryption and before any token use.
electronvitest project: 837 passed.parseStoredTokenSet/parseTokenResponsefiles lint- and typecheck-clean (tsconfig.electron.json).The arm64 fork-Docker CI job is expected to fail on fork PRs and is unrelated to this change.