diff --git a/src/renderer/context/App.test.tsx b/src/renderer/context/App.test.tsx index 0a076d5cf..87ba1498f 100644 --- a/src/renderer/context/App.test.tsx +++ b/src/renderer/context/App.test.tsx @@ -328,7 +328,7 @@ describe('renderer/context/App.tsx', () => { }); }); - describe('migrateAuthTokens (startup)', () => { + describe('persistRotatedAuthTokens (startup)', () => { const refreshAccountSpy = vi .spyOn(authUtils, 'refreshAccount') .mockImplementation(async (account) => account); @@ -387,23 +387,23 @@ describe('renderer/context/App.tsx', () => { expect(tokenChanged).toBe(false); }); - it('re-encrypts plaintext token (legacy migration) when decrypt throws', async () => { + it('does not re-encrypt or persist when decrypt throws', async () => { loadStateSpy.mockReturnValue({ auth: { accounts: [mockGitHubCloudAccount] } as AuthState, settings: mockSettings, }); decryptValueSpy.mockRejectedValue(new Error('not encrypted')); - encryptValueSpy.mockResolvedValue('newly-encrypted'); await act(async () => { renderWithProviders({null}); }); - expect(encryptValueSpy).toHaveBeenCalledWith(mockGitHubCloudAccount.token); - const persistCall = saveStateSpy.mock.calls.find( - ([state]) => (state as { auth: AuthState }).auth.accounts[0]?.token === 'newly-encrypted', + expect(encryptValueSpy).not.toHaveBeenCalled(); + const tokenChanged = saveStateSpy.mock.calls.some( + ([state]) => + (state as { auth: AuthState }).auth.accounts[0]?.token !== mockGitHubCloudAccount.token, ); - expect(persistCall).toBeDefined(); + expect(tokenChanged).toBe(false); }); }); }); diff --git a/src/renderer/context/App.tsx b/src/renderer/context/App.tsx index f9f5ce4ff..1dd21799f 100644 --- a/src/renderer/context/App.tsx +++ b/src/renderer/context/App.tsx @@ -28,7 +28,6 @@ import type { import { addAccount, - getAccountUUID, hasAccounts, isValidHostname, refreshAccount, @@ -64,7 +63,7 @@ import { defaultAuth, defaultSettings } from './defaults'; * coerce `forge` to a registered value, and drop accounts with a * structurally invalid hostname before the renderer ever touches them. */ -function migrateLegacyAuthState(auth: AuthState): AuthState { +function sanitiseAuthState(auth: AuthState): AuthState { return { ...auth, accounts: auth.accounts.flatMap((a) => { @@ -90,9 +89,7 @@ export const AppProvider = ({ children }: { children: ReactNode }) => { const existingState = loadState(); const [auth, setAuth] = useState( - existingState.auth - ? migrateLegacyAuthState({ ...defaultAuth, ...existingState.auth }) - : defaultAuth, + existingState.auth ? sanitiseAuthState({ ...defaultAuth, ...existingState.auth }) : defaultAuth, ); const [settings, setSettings] = useState( @@ -158,45 +155,38 @@ export const AppProvider = ({ children }: { children: ReactNode }) => { persistAuth(updatedAuth); }, [auth, persistAuth]); - // TODO - Remove migration logic in future release - const migrateAuthTokens = useCallback(async () => { - const migratedAccounts = await Promise.all( + /** + * Persist tokens re-encrypted after OS keychain key rotation. + * Tokens are expected to already be encrypted at rest; plaintext tokens + * from pre-encryption releases are no longer migrated. + */ + const persistRotatedAuthTokens = useCallback(async () => { + const rotatedAccounts = await Promise.all( auth.accounts.map(async (account) => { try { const { reEncryptedToken } = await decryptValue(account.token); if (reEncryptedToken) { return { ...account, token: reEncryptedToken as Token }; } - return account; } catch { - const encryptedToken = (await encryptValue(account.token)) as Token; - return { ...account, token: encryptedToken }; + // Leave the account unchanged if decrypt fails (e.g. corrupted ciphertext). } + return account; }), ); - const tokensMigrated = migratedAccounts.some((migratedAccount) => { - const originalAccount = auth.accounts.find( - (account) => getAccountUUID(account) === getAccountUUID(migratedAccount), - ); - - if (!originalAccount) { - return true; - } - - return migratedAccount.token !== originalAccount.token; - }); + const tokensRotated = rotatedAccounts.some( + (account, index) => account.token !== auth.accounts[index]?.token, + ); - if (!tokensMigrated) { + if (!tokensRotated) { return; } - const updatedAuth: AuthState = { + persistAuth({ ...auth, - accounts: migratedAccounts, - }; - - persistAuth(updatedAuth); + accounts: rotatedAccounts, + }); }, [auth, persistAuth]); useEffect(() => { @@ -228,11 +218,11 @@ export const AppProvider = ({ children }: { children: ReactNode }) => { ); /** - * On startup, check if auth tokens need encrypting and refresh all account details + * On startup, persist any keychain-rotated token ciphertext and refresh accounts */ useEffect(() => { void (async () => { - await migrateAuthTokens(); + await persistRotatedAuthTokens(); await refreshAllAccounts(); })(); // oxlint-disable-next-line react/exhaustive-deps -- Run once on startup