mirror of
https://github.com/stablyai/orca.git
synced 2026-10-19 16:03:26 +00:00
fix(mobile): record a keychain rotation before storing the token under it
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.
This commit is contained in:
@@ -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) =>
|
||||
|
||||
@@ -55,13 +55,12 @@ async function loadGeneration(): Promise<number> {
|
||||
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<void> {
|
||||
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<string | null> {
|
||||
@@ -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<void> {
|
||||
|
||||
Reference in New Issue
Block a user