diff --git a/src/main/handlers/storage.test.ts b/src/main/handlers/storage.test.ts index 9bb1ac4aa..6f0283ea0 100644 --- a/src/main/handlers/storage.test.ts +++ b/src/main/handlers/storage.test.ts @@ -71,18 +71,18 @@ describe('main/handlers/storage.ts', () => { expect(result).toBe(Buffer.from('plain-token').toString('base64')); }); - it('decrypt handler returns the unwrapped plaintext string', async () => { + it('decrypt handler returns the unwrapped token without rotation', async () => { registerStorageHandlers(); const decrypt = getHandler(EVENTS.SAFE_STORAGE_DECRYPT); const ciphertext = Buffer.from('plain-token').toString('base64'); const result = await decrypt({}, ciphertext); - expect(typeof result).toBe('string'); - expect(result).toBe('plain-token'); + expect(result).toEqual({ token: 'plain-token' }); + expect(safeStorage.encryptStringAsync).not.toHaveBeenCalled(); }); - it('decrypt handler unwraps result when shouldReEncrypt is true', async () => { + it('decrypt handler re-encrypts and returns new ciphertext when shouldReEncrypt is true', async () => { vi.mocked(safeStorage.decryptStringAsync).mockResolvedValueOnce({ shouldReEncrypt: true, result: 'rotated-token', @@ -92,8 +92,13 @@ describe('main/handlers/storage.ts', () => { const result = await decrypt({}, 'irrelevant'); - expect(typeof result).toBe('string'); - expect(result).toBe('rotated-token'); + expect(safeStorage.encryptStringAsync).toHaveBeenCalledWith( + 'rotated-token', + ); + expect(result).toEqual({ + token: 'rotated-token', + reEncryptedToken: Buffer.from('rotated-token').toString('base64'), + }); }); it('encrypt → decrypt round-trip preserves the original string', async () => { @@ -102,9 +107,9 @@ describe('main/handlers/storage.ts', () => { const decrypt = getHandler(EVENTS.SAFE_STORAGE_DECRYPT); const ciphertext = (await encrypt({}, 'round-trip-token')) as string; - const plaintext = await decrypt({}, ciphertext); + const result = (await decrypt({}, ciphertext)) as { token: string }; - expect(plaintext).toBe('round-trip-token'); + expect(result.token).toBe('round-trip-token'); }); it('decrypt handler rethrows and logs on safeStorage failure', async () => { diff --git a/src/main/handlers/storage.ts b/src/main/handlers/storage.ts index 14a1ae708..d000f647c 100644 --- a/src/main/handlers/storage.ts +++ b/src/main/handlers/storage.ts @@ -18,14 +18,26 @@ export function registerStorageHandlers(): void { }); /** - * Decrypt a base64-encoded string using Electron's safeStorage and return the decrypted value. + * Decrypt a base64-encoded string using Electron's safeStorage and return the + * decrypted token. When the OS keychain has rotated keys during decryption, + * also re-encrypt the value with the current key and return the new + * ciphertext under `reEncryptedToken` so callers can persist it. */ handleMainEvent(EVENTS.SAFE_STORAGE_DECRYPT, async (_, value: string) => { try { - const { result } = await safeStorage.decryptStringAsync( + const { result, shouldReEncrypt } = await safeStorage.decryptStringAsync( Buffer.from(value, 'base64'), ); - return result; + + if (!shouldReEncrypt) { + return { token: result }; + } + + const reEncrypted = await safeStorage.encryptStringAsync(result); + return { + token: result, + reEncryptedToken: reEncrypted.toString('base64'), + }; } catch (err) { logError( 'main:safe-storage-decrypt', diff --git a/src/renderer/__helpers__/vitest.setup.ts b/src/renderer/__helpers__/vitest.setup.ts index 2c5914c89..04b6e8a94 100644 --- a/src/renderer/__helpers__/vitest.setup.ts +++ b/src/renderer/__helpers__/vitest.setup.ts @@ -39,7 +39,7 @@ window.gitify = { }, twemojiDirectory: vi.fn().mockResolvedValue('/mock/images/assets'), openExternalLink: vi.fn(), - decryptValue: vi.fn().mockResolvedValue('decrypted'), + decryptValue: vi.fn().mockResolvedValue({ token: 'decrypted' }), encryptValue: vi.fn().mockResolvedValue('encrypted'), platform: { isLinux: vi.fn().mockReturnValue(false), diff --git a/src/renderer/context/App.test.tsx b/src/renderer/context/App.test.tsx index 17dd588ae..6aa4c2d57 100644 --- a/src/renderer/context/App.test.tsx +++ b/src/renderer/context/App.test.tsx @@ -23,6 +23,7 @@ import * as authFlows from '../utils/auth/flows'; import * as authUtils from '../utils/auth/utils'; import * as storage from '../utils/core/storage'; import * as notifications from '../utils/notifications/notifications'; +import * as comms from '../utils/system/comms'; import * as tray from '../utils/system/tray'; import { type AppContextState, AppProvider } from './App'; import { defaultSettings } from './defaults'; @@ -312,4 +313,92 @@ describe('renderer/context/App.tsx', () => { ); }); }); + + describe('migrateAuthTokens (startup)', () => { + const refreshAccountSpy = vi + .spyOn(authUtils, 'refreshAccount') + .mockImplementation(async (account) => account); + const decryptValueSpy = vi.spyOn(comms, 'decryptValue'); + const encryptValueSpy = vi.spyOn(comms, 'encryptValue'); + const saveStateSpy = vi + .spyOn(storage, 'saveState') + .mockImplementation(vi.fn()); + const loadStateSpy = vi.spyOn(storage, 'loadState'); + + beforeEach(() => { + vi.useRealTimers(); + saveStateSpy.mockClear(); + decryptValueSpy.mockReset(); + encryptValueSpy.mockReset(); + refreshAccountSpy.mockClear(); + }); + + afterEach(() => { + vi.useFakeTimers(); + }); + + it('persists rotated ciphertext when decryptValue reports a re-encryption', async () => { + loadStateSpy.mockReturnValue({ + auth: { accounts: [mockGitHubCloudAccount] } as AuthState, + settings: mockSettings, + }); + decryptValueSpy.mockResolvedValue({ + token: 'plain-token', + reEncryptedToken: 'rotated-cipher', + }); + + await act(async () => { + renderWithProviders({null}); + }); + + const persistCall = saveStateSpy.mock.calls.find( + ([state]) => + (state as { auth: AuthState }).auth.accounts[0]?.token === + 'rotated-cipher', + ); + expect(persistCall).toBeDefined(); + }); + + it('does not persist when decryptValue returns no rotated ciphertext', async () => { + loadStateSpy.mockReturnValue({ + auth: { accounts: [mockGitHubCloudAccount] } as AuthState, + settings: mockSettings, + }); + decryptValueSpy.mockResolvedValue({ token: 'plain-token' }); + + await act(async () => { + renderWithProviders({null}); + }); + + const tokenChanged = saveStateSpy.mock.calls.some( + ([state]) => + (state as { auth: AuthState }).auth.accounts[0]?.token !== + mockGitHubCloudAccount.token, + ); + expect(tokenChanged).toBe(false); + }); + + it('re-encrypts plaintext token (legacy migration) 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(persistCall).toBeDefined(); + }); + }); }); diff --git a/src/renderer/context/App.tsx b/src/renderer/context/App.tsx index dd3630a42..e6a07ca5c 100644 --- a/src/renderer/context/App.tsx +++ b/src/renderer/context/App.tsx @@ -206,7 +206,10 @@ export const AppProvider = ({ children }: { children: ReactNode }) => { const migratedAccounts = await Promise.all( auth.accounts.map(async (account) => { try { - await decryptValue(account.token); + 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; diff --git a/src/renderer/utils/api/octokit.test.ts b/src/renderer/utils/api/octokit.test.ts index 2a394abb8..a52190c81 100644 --- a/src/renderer/utils/api/octokit.test.ts +++ b/src/renderer/utils/api/octokit.test.ts @@ -16,7 +16,7 @@ describe('renderer/utils/api/octokit.ts', () => { const mockDecryptValue = vi.spyOn(comms, 'decryptValue'); beforeEach(() => { - mockDecryptValue.mockResolvedValue('decrypted-token'); + mockDecryptValue.mockResolvedValue({ token: 'decrypted-token' }); clearOctokitClientCache(); }); diff --git a/src/renderer/utils/api/octokit.ts b/src/renderer/utils/api/octokit.ts index de12ff3a4..c8ef7ad41 100644 --- a/src/renderer/utils/api/octokit.ts +++ b/src/renderer/utils/api/octokit.ts @@ -80,7 +80,7 @@ export async function createOctokitClientUncached( account: Account, type: APIClientType, ): Promise { - const decryptedToken = await decryptValue(account.token); + const { token: decryptedToken } = await decryptValue(account.token); const version = await getAppVersion(); const userAgent = `${APPLICATION.NAME}/${version}`; diff --git a/src/renderer/utils/system/comms.test.ts b/src/renderer/utils/system/comms.test.ts index 7d3cc9105..347d38f7f 100644 --- a/src/renderer/utils/system/comms.test.ts +++ b/src/renderer/utils/system/comms.test.ts @@ -94,7 +94,7 @@ describe('renderer/utils/comms.ts', () => { expect(window.gitify.decryptValue).toHaveBeenCalledTimes(1); expect(window.gitify.decryptValue).toHaveBeenCalledWith('encrypted'); - expect(value).toBe('decrypted'); + expect(value).toEqual({ token: 'decrypted' }); }); }); diff --git a/src/renderer/utils/system/comms.ts b/src/renderer/utils/system/comms.ts index 70e5b8447..b5ac9df2d 100644 --- a/src/renderer/utils/system/comms.ts +++ b/src/renderer/utils/system/comms.ts @@ -1,3 +1,5 @@ +import type { ISafeStorageDecryptResult } from '../../../shared/events'; + import { defaultSettings } from '../../context/defaults'; import { type Link, OpenPreference } from '../../types'; @@ -49,10 +51,15 @@ export async function encryptValue(value: string): Promise { /** * Decrypts a previously encrypted string using the native Electron decryption bridge. * + * Resolves to the decrypted token. When the OS keychain rotated keys during + * decryption, `reEncryptedToken` is also set so callers can persist the new + * ciphertext alongside or in place of the original. + * * @param value - The encrypted string to decrypt. - * @returns Promise resolving to the decrypted plaintext string. */ -export async function decryptValue(value: string): Promise { +export async function decryptValue( + value: string, +): Promise { return await window.gitify.decryptValue(value); } diff --git a/src/shared/events.ts b/src/shared/events.ts index 07832bf2e..b7a951f80 100644 --- a/src/shared/events.ts +++ b/src/shared/events.ts @@ -51,6 +51,19 @@ export interface IOpenExternal { activate: boolean; } +/** + * Result of decrypting a value via Electron's safe storage. + * + * `reEncryptedToken` is set when the OS keychain rotated keys during decrypt + * (signaled by `safeStorage.decryptStringAsync`'s `shouldReEncrypt` flag). + * Callers that persist the original ciphertext should overwrite it with the + * new value so future sessions stay aligned with the active keychain key. + */ +export interface ISafeStorageDecryptResult { + token: string; + reEncryptedToken?: string; +} + /** Shape of a single event contract: a request payload and a response payload. */ type Contract = { request: unknown; response: unknown }; @@ -83,7 +96,10 @@ export type EventContracts = AssertEventCoverage<{ }; [EVENTS.UPDATE_AUTO_LAUNCH]: { request: IAutoLaunch; response: undefined }; [EVENTS.SAFE_STORAGE_ENCRYPT]: { request: string; response: string }; - [EVENTS.SAFE_STORAGE_DECRYPT]: { request: string; response: string }; + [EVENTS.SAFE_STORAGE_DECRYPT]: { + request: string; + response: ISafeStorageDecryptResult; + }; [EVENTS.NOTIFICATION_SOUND_PATH]: { request: undefined; response: string }; [EVENTS.OPEN_EXTERNAL]: { request: IOpenExternal; response: undefined }; [EVENTS.RESET_APP]: { request: undefined; response: undefined };