From 8708e6372f04f9aaa5967a68477fd6090d119a47 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 30 Jul 2026 11:46:14 -0700 Subject: [PATCH] fix(mobile): fail closed on unreadable pairing credentials --- ...mobile-relay-pairing-journal-store.test.ts | 25 ++++-- mobile/src/transport/pairing-keychain.test.ts | 49 ++++++++++- mobile/src/transport/pairing-keychain.ts | 81 ++++++++++++++++++- 3 files changed, 146 insertions(+), 9 deletions(-) diff --git a/mobile/src/transport/mobile-relay-pairing-journal-store.test.ts b/mobile/src/transport/mobile-relay-pairing-journal-store.test.ts index c62c168c705..cf249a90506 100644 --- a/mobile/src/transport/mobile-relay-pairing-journal-store.test.ts +++ b/mobile/src/transport/mobile-relay-pairing-journal-store.test.ts @@ -32,6 +32,7 @@ import type { PairingOffer } from './types' const now = Date.UTC(2026, 6, 13) const GENERATION_KEY = 'orca:pairing-keychain-generation' +const JOURNAL_PRESENCE_KEY = 'orca:pairing-keychain-presence:orca.mobile-relay.pairing-journal.v1' const offer = { v: 2, endpoint: 'ws://192.168.1.10:6768', @@ -53,6 +54,7 @@ describe('mobile relay pairing journal store', () => { let metadataRaw: string | null let secretRaw: string | null let generationRaw: string | null + let presenceRaw: string | null beforeEach(() => { vi.clearAllMocks() @@ -61,18 +63,31 @@ describe('mobile relay pairing journal store', () => { metadataRaw = null secretRaw = null generationRaw = null - asyncStorage.getItem.mockImplementation(async (key: string) => - key === GENERATION_KEY ? generationRaw : metadataRaw - ) + presenceRaw = null + asyncStorage.getItem.mockImplementation(async (key: string) => { + if (key === GENERATION_KEY) { + return generationRaw + } + if (key === JOURNAL_PRESENCE_KEY) { + return presenceRaw + } + return metadataRaw + }) asyncStorage.setItem.mockImplementation(async (key: string, value: string) => { if (key === GENERATION_KEY) { generationRaw = value + } else if (key === JOURNAL_PRESENCE_KEY) { + presenceRaw = value } else { metadataRaw = value } }) - asyncStorage.removeItem.mockImplementation(async () => { - metadataRaw = null + asyncStorage.removeItem.mockImplementation(async (key: string) => { + if (key === JOURNAL_PRESENCE_KEY) { + presenceRaw = null + } else { + metadataRaw = null + } }) secureStore.getItemAsync.mockImplementation(async () => secretRaw) secureStore.setItemAsync.mockImplementation(async (_key: string, value: string) => { diff --git a/mobile/src/transport/pairing-keychain.test.ts b/mobile/src/transport/pairing-keychain.test.ts index 23c7633a7f7..fda46d0a328 100644 --- a/mobile/src/transport/pairing-keychain.test.ts +++ b/mobile/src/transport/pairing-keychain.test.ts @@ -30,6 +30,7 @@ import { const GENERATION_KEY = 'orca:pairing-keychain-generation' const TOKEN_KEY = 'orca.host-token.host-1782629088232' +const TOKEN_PRESENCE_KEY = `orca:pairing-keychain-presence:${TOKEN_KEY}` // Why: the exact Android failure from #6600 — expo maps a null-message GeneralSecurityException to this. const ENCRYPT_REJECTION = new Error( @@ -139,7 +140,11 @@ describe('pairing keychain', () => { 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')) + asyncStorageMock.setItem.mockImplementation(async (key: string) => { + if (key === GENERATION_KEY) { + throw new Error('storage full') + } + }) secureStoreMock.setItemAsync.mockImplementation( async (_k: string, _v: string, options: Options) => { if (serviceOf(options) === undefined) { @@ -241,6 +246,39 @@ describe('pairing keychain', () => { expect(secureStoreMock.getItemAsync).toHaveBeenCalledTimes(1) }) + it('does not return a stale older value when a recorded current item decrypts as null', async () => { + generationRecord = '1' + let presenceRecord: string | null = null + asyncStorageMock.getItem.mockImplementation(async (key: string) => { + if (key === GENERATION_KEY) { + return generationRecord + } + return key === TOKEN_PRESENCE_KEY ? presenceRecord : null + }) + asyncStorageMock.setItem.mockImplementation(async (key: string, raw: string) => { + if (key === GENERATION_KEY) { + generationRecord = raw + } + if (key === TOKEN_PRESENCE_KEY) { + presenceRecord = raw + } + }) + + await writePairingKeychainItem(TOKEN_KEY, 'rotated-token') + expect(presenceRecord).toBe('1') + expect(asyncStorageMock.setItem.mock.invocationCallOrder[0]).toBeLessThan( + secureStoreMock.setItemAsync.mock.invocationCallOrder[0]! + ) + + resetPairingKeychainForTests() + secureStoreMock.getItemAsync.mockImplementation(async (_k: string, options: Options) => + serviceOf(options) === undefined ? 'legacy-token' : null + ) + + await expect(readPairingKeychainItem(TOKEN_KEY)).rejects.toThrow(/recorded generation/) + expect(secureStoreMock.getItemAsync).toHaveBeenCalledTimes(1) + }) + it('reads an older value while a newly recorded alias has no item yet', async () => { generationRecord = '1:pending' secureStoreMock.getItemAsync.mockImplementation(async (_k: string, options: Options) => @@ -251,7 +289,12 @@ describe('pairing keychain', () => { }) it('probes every bounded service when the generation record is temporarily unreadable', async () => { - asyncStorageMock.getItem.mockRejectedValueOnce(new Error('storage unavailable')) + asyncStorageMock.getItem.mockImplementation(async (key: string) => { + if (key === GENERATION_KEY) { + throw new Error('storage unavailable') + } + return null + }) secureStoreMock.getItemAsync.mockImplementation(async (_k: string, options: Options) => serviceOf(options) === 'orca.pairing.v2' ? 'rotated-token' : null ) @@ -280,6 +323,7 @@ describe('pairing keychain', () => { serviceOf(call[1] as Options) ) expect(services).toEqual(['orca.pairing.v2', 'orca.pairing.v1', undefined]) + expect(asyncStorageMock.removeItem).toHaveBeenCalledWith(TOKEN_PRESENCE_KEY) }) it('reports a partial delete failure after attempting every generation', async () => { @@ -293,6 +337,7 @@ describe('pairing keychain', () => { await expect(deletePairingKeychainItem(TOKEN_KEY)).rejects.toBe(deleteError) expect(secureStoreMock.deleteItemAsync).toHaveBeenCalledTimes(3) + expect(asyncStorageMock.removeItem).not.toHaveBeenCalled() }) it('serializes writes that share the global generation', async () => { diff --git a/mobile/src/transport/pairing-keychain.ts b/mobile/src/transport/pairing-keychain.ts index bb4a21a4a3e..bf3b1810807 100644 --- a/mobile/src/transport/pairing-keychain.ts +++ b/mobile/src/transport/pairing-keychain.ts @@ -8,6 +8,7 @@ const BASE_OPTIONS: SecureStore.SecureStoreOptions = { } const GENERATION_STORAGE_KEY = 'orca:pairing-keychain-generation' +const PRESENCE_STORAGE_PREFIX = 'orca:pairing-keychain-presence:' const SERVICE_PREFIX = 'orca.pairing.v' // Why: every generation adds one read probe per miss; bound pathological recovery cost. const MAX_GENERATION = 8 @@ -33,6 +34,7 @@ type LoadedGeneration = | { generation: 0; pending: false; reliable: false; error: unknown } type GenerationState = { generation: number; pending: boolean } +type PresenceChange = { storageKey: string; previousRaw: string | null } let cachedGeneration: GenerationState | null = null let keychainMutation: Promise = Promise.resolve() @@ -61,6 +63,33 @@ function parseGeneration(raw: string | null): LoadedGeneration { return { generation: parsed, pending, reliable: true } } +function parsePresenceGeneration(raw: string | null): number | null { + if (raw === null) { + return null + } + const parsed = Number(raw) + if ( + !Number.isInteger(parsed) || + parsed < 0 || + parsed > MAX_GENERATION || + String(parsed) !== raw + ) { + throw new Error('pairing keychain presence record is invalid') + } + return parsed +} + +function presenceStorageKey(key: string): string { + return `${PRESENCE_STORAGE_PREFIX}${key}` +} + +async function loadPresenceGeneration(key: string): Promise { + if (Platform.OS !== 'android') { + return null + } + return parsePresenceGeneration(await AsyncStorage.getItem(presenceStorageKey(key))) +} + async function loadGeneration(): Promise { if (cachedGeneration !== null) { return { ...cachedGeneration, reliable: true } @@ -90,8 +119,53 @@ function enqueueKeychainMutation(operation: () => Promise): Promise return mutation } +async function preparePresenceWrite( + key: string, + generation: number +): Promise { + if (Platform.OS !== 'android') { + return null + } + // Why: Expo Android returns null for both absent and undecryptable entries. + const storageKey = presenceStorageKey(key) + const previousRaw = await AsyncStorage.getItem(storageKey) + parsePresenceGeneration(previousRaw) + await AsyncStorage.setItem(storageKey, String(generation)) + return { storageKey, previousRaw } +} + +async function restorePresence(change: PresenceChange | null): Promise { + if (!change) { + return + } + if (change.previousRaw === null) { + await AsyncStorage.removeItem(change.storageKey) + return + } + await AsyncStorage.setItem(change.storageKey, change.previousRaw) +} + +async function setItemAtGeneration(key: string, value: string, generation: number): Promise { + const presenceChange = await preparePresenceWrite(key, generation) + try { + await SecureStore.setItemAsync(key, value, optionsForGeneration(generation)) + } catch (error) { + // Why: a failed rollback leaves reads fail-closed at the attempted generation. + await restorePresence(presenceChange).catch(() => {}) + throw error + } +} + export async function readPairingKeychainItem(key: string): Promise { await keychainMutation + const presenceGeneration = await loadPresenceGeneration(key) + if (presenceGeneration !== null) { + const value = await SecureStore.getItemAsync(key, optionsForGeneration(presenceGeneration)) + if (value === null) { + throw new Error('pairing keychain item is unavailable at its recorded generation') + } + return value + } const loaded = await loadGeneration() const firstCandidate = loaded.reliable ? loaded.generation : MAX_GENERATION for (let candidate = firstCandidate; candidate >= 0; candidate -= 1) { @@ -111,7 +185,7 @@ async function writePairingKeychainItemImpl(key: string, value: string): Promise const generation = loaded.generation let firstError: unknown try { - await SecureStore.setItemAsync(key, value, optionsForGeneration(generation)) + await setItemAtGeneration(key, value, generation) if (loaded.pending) { await commitGeneration(generation, false) } @@ -134,7 +208,7 @@ async function writePairingKeychainItemImpl(key: string, value: string): Promise throw firstError } try { - await SecureStore.setItemAsync(key, value, optionsForGeneration(rotated)) + await setItemAtGeneration(key, value, rotated) await commitGeneration(rotated, false) } catch { throw firstError @@ -167,6 +241,9 @@ async function deletePairingKeychainItemImpl(key: string): Promise { if (firstError !== undefined) { throw firstError } + if (Platform.OS === 'android') { + await AsyncStorage.removeItem(presenceStorageKey(key)) + } } export function deletePairingKeychainItem(key: string): Promise {