From ca859bd6e6e9e921863f7ee2ae1c400282acc7bb Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 29 Jul 2026 16:43:16 -0700 Subject: [PATCH] fix(mobile): record a keychain rotation before storing the token under it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile flagged that a token could be stored under a generation the generation record never captured. `commitGeneration` swallowed the AsyncStorage failure and cached the new generation in memory, so the write succeeded for the rest of the session — but the next launch re-read the old record, and because reads only walk back from the recorded generation they never probed the newer service. The host silently vanished and the user had to re-pair, which is the same class of loss this change set out to fix. Record the rotation first and let a storage failure propagate, so a token is never written under a generation reads won't reach. Advancing the record before the write is safe because reads walk back through every older service; the worst case is one spent generation and one extra probe per miss. --- .../src/transport/host-token-keychain.test.ts | 46 ++++++++++++++++++- mobile/src/transport/host-token-keychain.ts | 19 +++++--- 2 files changed, 56 insertions(+), 9 deletions(-) diff --git a/mobile/src/transport/host-token-keychain.test.ts b/mobile/src/transport/host-token-keychain.test.ts index 04ea606ada0..9e6848f785f 100644 --- a/mobile/src/transport/host-token-keychain.test.ts +++ b/mobile/src/transport/host-token-keychain.test.ts @@ -102,14 +102,56 @@ describe('host token keychain', () => { expect(generationRecord).toBe('1') }) - it('keeps the working service when a rotated write also fails, and surfaces the original error', async () => { + it('surfaces the original error when a rotated write also fails', async () => { secureStoreMock.setItemAsync.mockRejectedValue(ENCRYPT_REJECTION) await expect(writeHostTokenToKeychain(TOKEN_KEY, 'token')).rejects.toBe(ENCRYPT_REJECTION) - // Why: retiring a service on a rotation that didn't help would strand readable tokens for nothing. + }) + + it('never stores a token under a generation it could not durably record', async () => { + // Why: reads only walk back from the recorded generation, so a token written under an + // unrecorded one is silently unreachable after a relaunch and the host vanishes. + asyncStorageMock.setItem.mockRejectedValue(new Error('storage full')) + secureStoreMock.setItemAsync.mockImplementation( + async (_k: string, _v: string, options: Options) => { + if (serviceOf(options) === undefined) { + throw ENCRYPT_REJECTION + } + } + ) + + await expect(writeHostTokenToKeychain(TOKEN_KEY, 'token')).rejects.toBe(ENCRYPT_REJECTION) + + const rotatedWrites = secureStoreMock.setItemAsync.mock.calls.filter( + (call) => serviceOf(call[2] as Options) !== undefined + ) + expect(rotatedWrites).toHaveLength(0) expect(generationRecord).toBeNull() }) + it('records the rotation before storing the token so a relaunch can still find it', async () => { + const order: string[] = [] + asyncStorageMock.setItem.mockImplementation(async (key: string, raw: string) => { + if (key === GENERATION_KEY) { + generationRecord = raw + order.push(`record:${raw}`) + } + }) + secureStoreMock.setItemAsync.mockImplementation( + async (_k: string, _v: string, options: Options) => { + const service = serviceOf(options) + if (service === undefined) { + throw ENCRYPT_REJECTION + } + order.push(`store:${service}`) + } + ) + + await writeHostTokenToKeychain(TOKEN_KEY, 'token') + + expect(order).toEqual(['record:1', 'store:orca.host-tokens.v1']) + }) + it('reads through the rotated service once a rotation has been committed', async () => { generationRecord = '1' secureStoreMock.getItemAsync.mockImplementation(async (_k: string, options: Options) => diff --git a/mobile/src/transport/host-token-keychain.ts b/mobile/src/transport/host-token-keychain.ts index 2ffd9fb9f25..8ed23fb5cfb 100644 --- a/mobile/src/transport/host-token-keychain.ts +++ b/mobile/src/transport/host-token-keychain.ts @@ -55,13 +55,12 @@ async function loadGeneration(): Promise { return cachedGeneration } +// Why: reads only walk back from the recorded generation, so a token stored under a generation +// we failed to record would be unreachable on the next launch. Let the failure propagate and +// cache only after the record is durable, so callers can decline to write under it. async function commitGeneration(generation: number): Promise { + await AsyncStorage.setItem(GENERATION_STORAGE_KEY, String(generation)) cachedGeneration = generation - try { - await AsyncStorage.setItem(GENERATION_STORAGE_KEY, String(generation)) - } catch { - // Why: the token is already written under this generation; losing the record only costs extra read probes. - } } export async function readHostTokenFromKeychain(tokenKey: string): Promise { @@ -103,11 +102,17 @@ export async function writeHostTokenToKeychain(tokenKey: string, token: string): if (rotated > MAX_GENERATION) { throw firstError } - // Why: commit the new generation only after the write under it lands, so a failed rotation doesn't retire a working service. + // Why: record the rotation before storing under it, so the token can never land under a + // generation reads won't probe. Advancing early is safe because reads walk back through every + // older service; the worst case is one spent generation and one extra probe per miss. + try { + await commitGeneration(rotated) + } catch { + throw firstError + } await SecureStore.setItemAsync(tokenKey, token, optionsForGeneration(rotated)).catch(() => { throw firstError }) - await commitGeneration(rotated) } export async function deleteHostTokenFromKeychain(tokenKey: string): Promise {