diff --git a/src/analytics/metrics/types.ts b/src/analytics/metrics/types.ts index 00b258c244..e8dd407236 100644 --- a/src/analytics/metrics/types.ts +++ b/src/analytics/metrics/types.ts @@ -64,6 +64,7 @@ export type Events = { divergence: 'none' | 'legacy-lost' | 'secure-behind' secureAccountsWithCreds: number legacyAccountsWithCreds: number + adoptTokensFromLegacy: number } 'notifications:openApp': { reason: NotificationReason diff --git a/src/state/session/storage/__tests__/boot-test.ts b/src/state/session/storage/__tests__/boot-test.ts index 80acec4379..cecba22135 100644 --- a/src/state/session/storage/__tests__/boot-test.ts +++ b/src/state/session/storage/__tests__/boot-test.ts @@ -76,6 +76,28 @@ const expiredAlice: SessionAccount = { refreshJwt: undefined, accessJwt: undefined, } +const bob: SessionAccount = { + service: 'https://bsky.social', + did: 'did:plc:bob', + handle: 'bob.test', + refreshJwt: 'bob-refresh', + accessJwt: 'bob-access', +} + +/** + * A minimal unsigned JWT carrying an `iat`. `jwt-decode` reads the payload + * without verifying anything, so the signature can be junk. `jti` is there to + * make two tokens of the same age differ as strings. + */ +function jwtWithIat(iat: number, jti = 'a'): string { + const encode = (value: object) => + Buffer.from(JSON.stringify(value)).toString('base64url') + return [ + encode({alg: 'HS256', typ: 'JWT'}), + encode({iat, jti}), + 'not-a-signature', + ].join('.') +} /** An account with usable credentials. */ const withCreds: SessionSnapshot = {accounts: [alice], currentDid: alice.did} @@ -88,6 +110,12 @@ const withoutCreds: SessionSnapshot = { type ExpectedDecision = Omit & { adopts: boolean divergence: SessionStorageBootReport['divergence'] + /** + * The adopted snapshot, when the per-account merge makes it something other + * than the secure snapshot as read. + */ + adopt?: SessionSnapshot + adoptTokensFromLegacy?: number } const rows: { @@ -248,6 +276,8 @@ const rows: { expected: { source: 'secure', adopts: true, + // The merge keeps the credential-less account the blob still lists. + adopt: withoutCreds, backfill: 'none', forceIndex: false, divergence: 'none', @@ -296,6 +326,11 @@ const rows: { describe('decideBootSource', () => { it.each(rows)('$name', ({gateEnabled, secure: read, legacy, expected}) => { + const { + adopt: expectedAdopt, + adoptTokensFromLegacy = 0, + ...expectedCore + } = expected const decision = decideBootSource({gateEnabled, secure: read, legacy}) expect({ @@ -304,10 +339,13 @@ describe('decideBootSource', () => { backfill: decision.backfill, forceIndex: decision.forceIndex, divergence: decision.report.divergence, - }).toEqual(expected) + }).toEqual(expectedCore) expect(decision.adopt).toEqual( - expected.adopts && read.status === 'ok' ? read.snapshot : undefined, + expected.adopts + ? (expectedAdopt ?? (read.status === 'ok' ? read.snapshot : undefined)) + : undefined, ) + expect(decision.report.adoptTokensFromLegacy).toBe(adoptTokensFromLegacy) expect(decision.report.gateEnabled).toBe(gateEnabled) expect(decision.report.secureStatus).toBe(read.status) expect(decision.report.source).toBe(expected.source) @@ -331,6 +369,80 @@ describe('decideBootSource', () => { }) }) +/** + * Adopting the secure store wholesale would lose whatever only the blob holds, + * which is what a single failed mirror write looks like: the store as a whole + * still has credentials, so the whole-store guard sees nothing wrong. + */ +describe('decideBootSource credential merge', () => { + function adoptedBy(secureSnapshot: SessionSnapshot, legacy: SessionSnapshot) { + return decideBootSource({ + gateEnabled: true, + secure: {status: 'ok', snapshot: secureSnapshot}, + legacy, + }) + } + + it('keeps an account the secure store never received', () => { + const legacy: SessionSnapshot = { + accounts: [alice, bob], + currentDid: alice.did, + } + + const decision = adoptedBy(withCreds, legacy) + + expect(decision.source).toBe('secure') + expect(decision.adopt).toEqual(legacy) + expect(decision.report.adoptTokensFromLegacy).toBe(1) + }) + + it('keeps an account only the secure store still holds', () => { + const secureSnapshot: SessionSnapshot = { + accounts: [alice, bob], + currentDid: bob.did, + } + + const decision = adoptedBy(secureSnapshot, withCreds) + + expect(decision.adopt).toEqual(secureSnapshot) + expect(decision.report.adoptTokensFromLegacy).toBe(0) + }) + + it('takes whichever refresh token was issued last', () => { + const older = {...alice, refreshJwt: jwtWithIat(1_000), accessJwt: 'older'} + const newer = {...alice, refreshJwt: jwtWithIat(2_000), accessJwt: 'newer'} + const snapshotOf = (account: SessionAccount): SessionSnapshot => ({ + accounts: [account], + currentDid: alice.did, + }) + + const legacyIsNewer = adoptedBy(snapshotOf(older), snapshotOf(newer)) + expect(legacyIsNewer.adopt).toEqual(snapshotOf(newer)) + expect(legacyIsNewer.report.adoptTokensFromLegacy).toBe(1) + + const secureIsNewer = adoptedBy(snapshotOf(newer), snapshotOf(older)) + expect(secureIsNewer.adopt).toEqual(snapshotOf(newer)) + expect(secureIsNewer.report.adoptTokensFromLegacy).toBe(0) + }) + + it('falls back to the blob when the comparison cannot be made', () => { + const snapshotOf = (account: SessionAccount): SessionSnapshot => ({ + accounts: [account], + currentDid: alice.did, + }) + const undecodable = {...alice, refreshJwt: 'not-a-jwt'} + const decodable = {...alice, refreshJwt: jwtWithIat(1_000)} + const sameAge = {...alice, refreshJwt: jwtWithIat(1_000, 'b')} + + const unreadable = adoptedBy(snapshotOf(undecodable), snapshotOf(decodable)) + expect(unreadable.adopt).toEqual(snapshotOf(decodable)) + expect(unreadable.report.adoptTokensFromLegacy).toBe(1) + + const tied = adoptedBy(snapshotOf(decodable), snapshotOf(sameAge)) + expect(tied.adopt).toEqual(snapshotOf(sameAge)) + }) +}) + describe('initSessionStorage', () => { beforeEach(() => { secure.values.clear() @@ -361,6 +473,47 @@ describe('initSessionStorage', () => { divergence: 'legacy-lost', secureAccountsWithCreds: 1, legacyAccountsWithCreds: 0, + adoptTokensFromLegacy: 0, + }) + }) + + it('adopts the newer blob credential and converges the store on it', () => { + device.set(['sessionSecureStorageReadEnabled'], true) + const stored = { + ...alice, + refreshJwt: jwtWithIat(1_000), + accessJwt: 'stored-access', + } + const refreshed = { + ...alice, + refreshJwt: jwtWithIat(2_000), + accessJwt: 'refreshed-access', + } + writeSessions( + EMPTY_SNAPSHOT, + {accounts: [stored], currentDid: alice.did}, + {forceIndex: true}, + ) + persistedState.session = { + accounts: [refreshed], + currentAccount: refreshed, + } + + initSessionStorage() + + expect(persistedState.session).toEqual({ + accounts: [refreshed], + currentAccount: refreshed, + }) + // The mirror write lands the adopted credential back in the keychain. + expect(readSessions()).toEqual({ + accounts: [refreshed], + currentDid: alice.did, + }) + expect(consumeSessionStorageBootReport()).toMatchObject({ + source: 'secure', + divergence: 'none', + adoptTokensFromLegacy: 1, }) }) diff --git a/src/state/session/storage/__tests__/store-test.ts b/src/state/session/storage/__tests__/store-test.ts index 8f189ecefe..a42d2aec26 100644 --- a/src/state/session/storage/__tests__/store-test.ts +++ b/src/state/session/storage/__tests__/store-test.ts @@ -3,12 +3,23 @@ import {afterEach, beforeEach, describe, expect, it, jest} from '@jest/globals' jest.mock('expo-secure-store', () => { const values = new Map() const writes: [key: string, value: string][] = [] - const control: {failKey?: string} = {} + /* + * `failAfter` lets a key succeed that many times before it starts failing, + * which is how a write is made to fail on its second index write - the one + * that cleans up a journal it already published. + */ + const control: {failKey?: string; failAfter?: number} = {} return { AFTER_FIRST_UNLOCK: 0, getItem: (key: string) => values.get(key) ?? null, setItem: (key: string, value: string) => { - if (key === control.failKey) throw new Error('disk full') + if (key === control.failKey) { + if (control.failAfter) { + control.failAfter -= 1 + } else { + throw new Error('disk full') + } + } writes.push([key, value]) values.set(key, value) }, @@ -49,13 +60,14 @@ jest.mock('#/logger', () => { import {type SessionAccount} from '#/state/session/types' import {accountKeys, SESSION_INDEX_KEY} from '../keys' import {EMPTY_SNAPSHOT, type SessionSnapshot} from '../schema' +import {readSessions} from '../secureStore' import {createSessionStorageStore} from '../store' const {__mock: secure} = jest.requireMock('expo-secure-store') as { __mock: { values: Map writes: [key: string, value: string][] - control: {failKey?: string} + control: {failKey?: string; failAfter?: number} } } const {__listeners: appStateListeners} = jest.requireMock('#/lib/appState') as { @@ -82,6 +94,7 @@ beforeEach(() => { secure.values.clear() secure.writes.length = 0 secure.control.failKey = undefined + secure.control.failAfter = undefined appStateListeners.length = 0 loggedErrors.length = 0 }) @@ -192,6 +205,101 @@ describe('createSessionStorageStore', () => { expect(storedIndex()).toEqual({version: 1, dids: []}) }) + it('drops writes for the rest of the process once a clear succeeds', () => { + const store = createSessionStorageStore() + store.write(active) + + store.clear() + const writesAfterClear = secure.writes.length + + /* + * Clearing storage does not end the session, so a token refresh already in + * flight will still ask for a mirror write. + */ + store.write(active) + + expect(secure.writes.length).toBe(writesAfterClear) + expect(secure.values.get(aliceKeys.refresh)).toBe('') + expect(secure.values.get(aliceKeys.access)).toBe('') + expect(readSessions()).toEqual(EMPTY_SNAPSHOT) + }) + + it('force-publishes a clean index over a journal left by a failed write', () => { + const store = createSessionStorageStore() + store.write(active) + + /* + * Removing alice fails on the final index write: the journaled index that + * names her as retired is already published and her keys are tombstoned. + */ + secure.control.failKey = SESSION_INDEX_KEY + secure.control.failAfter = 1 + store.write(EMPTY_SNAPSHOT) + expect(storedIndex()).toMatchObject({dids: [], retiredDids: [alice.did]}) + expect(secure.values.get(aliceKeys.refresh)).toBe('') + + // alice is back, byte-identical to the snapshot still believed durable. + secure.control.failKey = undefined + secure.control.failAfter = undefined + store.write(active) + + expect(secure.values.get(aliceKeys.refresh)).toBe(alice.refreshJwt) + expect(secure.values.get(aliceKeys.access)).toBe(alice.accessJwt) + expect(storedIndex()).toEqual({ + version: 1, + currentDid: alice.did, + dids: [alice.did], + }) + expect(readSessions()).toEqual(active) + expect(store.getDurable()).toEqual(active) + }) + + it('rewrites credentials after a failure even when the snapshot is unchanged', () => { + const store = createSessionStorageStore() + store.write(active) + + secure.control.failKey = SESSION_INDEX_KEY + store.write({ + accounts: [{...alice, refreshJwt: 'rotated-refresh'}], + currentDid: alice.did, + }) + secure.control.failKey = undefined + + /* + * A throw can land anywhere in the write ordering, so the keychain may hold + * something the baseline does not describe. + */ + secure.values.set(aliceKeys.refresh, 'partially-applied') + store.write(active) + + expect(secure.values.get(aliceKeys.refresh)).toBe(alice.refreshJwt) + expect(secure.values.get(aliceKeys.access)).toBe(alice.accessJwt) + expect(readSessions()).toEqual(active) + }) + + it('tombstones absent credentials when rewriting from an unknown baseline', () => { + const store = createSessionStorageStore() + store.write(active) + + secure.control.failKey = SESSION_INDEX_KEY + store.write({ + accounts: [{...alice, accessJwt: 'rotated-access'}], + currentDid: alice.did, + }) + secure.control.failKey = undefined + + // The user logs out: the account is kept, its credentials are not. + const loggedOut: SessionSnapshot = { + accounts: [{...alice, refreshJwt: undefined, accessJwt: undefined}], + currentDid: undefined, + } + store.write(loggedOut) + + expect(secure.values.get(aliceKeys.refresh)).toBe('') + expect(secure.values.get(aliceKeys.access)).toBe('') + expect(readSessions()).toEqual(loggedOut) + }) + it('retries when the app returns to the foreground', () => { const store = createSessionStorageStore() secure.control.failKey = aliceKeys.access diff --git a/src/state/session/storage/boot.ts b/src/state/session/storage/boot.ts index 02eaf159af..2a1bb9e956 100644 --- a/src/state/session/storage/boot.ts +++ b/src/state/session/storage/boot.ts @@ -1,3 +1,6 @@ +import {jwtDecode} from 'jwt-decode' + +import {type SessionAccount} from '#/state/session/types' import {type SessionSnapshot} from './schema' /** Which store the app boots its session state from. */ @@ -22,11 +25,22 @@ export type SessionStorageBootReport = { divergence: 'none' | 'legacy-lost' | 'secure-behind' secureAccountsWithCreds: number legacyAccountsWithCreds: number + /** + * How many accounts in the adopted snapshot took their credentials from the + * legacy blob, i.e. how often the secure store held a stale or missing token + * that would have been adopted as-is. Always 0 when not booting from the + * secure store. + */ + adoptTokensFromLegacy: number } export type BootDecision = { source: BootSource - /** The snapshot to adopt, set only when booting from the secure store. */ + /** + * The snapshot to adopt, set only when booting from the secure store. It is + * the secure snapshot merged with the legacy blob, not the secure snapshot + * itself - see {@link mergeFreshestCredentials}. + */ adopt: SessionSnapshot | undefined backfill: 'none' | 'secure-from-legacy' /** Publish an index even if the backfill changes nothing. */ @@ -44,6 +58,11 @@ export type BootDecision = { * mirror writes look like - booting from the secure store there would log the * user out. A real logout clears the credentials in both stores, so it does * not trip the guard. + * + * That guard is whole-store, so winning the boot does not mean the secure + * snapshot is adopted verbatim: it is merged per account against the blob by + * {@link mergeFreshestCredentials}, which is what keeps a single failed mirror + * write from dropping an account or reinstating a spent refresh token. */ export function decideBootSource({ gateEnabled, @@ -62,6 +81,10 @@ export function decideBootSource({ gateEnabled && secureSnapshot !== undefined && (secureHasCreds || !legacyHasCreds) + const merged = + bootSecure && secureSnapshot + ? mergeFreshestCredentials(secureSnapshot, legacy) + : undefined let backfill: BootDecision['backfill'] = 'none' let forceIndex = false @@ -90,7 +113,7 @@ export function decideBootSource({ return { source: bootSecure ? 'secure' : 'legacy', - adopt: bootSecure ? secureSnapshot : undefined, + adopt: merged?.snapshot, backfill, forceIndex, report: { @@ -107,10 +130,97 @@ export function decideBootSource({ : 'none', secureAccountsWithCreds: secureSnapshot ? countCreds(secureSnapshot) : 0, legacyAccountsWithCreds: countCreds(legacy), + adoptTokensFromLegacy: merged?.tokensFromLegacy ?? 0, }, } } +/** + * Merge the two stores account by account, taking the freshest credentials for + * each did, so that adopting the secure store cannot lose what only the blob + * holds. A mirror write can fail for one account - a login whose write failed + * and whose in-memory retry died with the process, or a token rotation that + * never landed - while the rest of the store looks perfectly healthy, which is + * exactly what the whole-store guard cannot see. + * + * Ties and unprovable comparisons go to the blob: it is the historical write of + * record, and the secure store is the mirror on trial. + * + * Deliberate asymmetry: this can resurrect a credential the other store + * recorded as revoked, e.g. a logout only one of the two writes captured. That + * is accepted. A token the server has already revoked self-corrects on first + * use with a re-login prompt, while a valid token dropped here logs the user + * out of an account with no way back - the failure this module exists to + * prevent. + */ +function mergeFreshestCredentials( + secure: SessionSnapshot, + legacy: SessionSnapshot, +): {snapshot: SessionSnapshot; tokensFromLegacy: number} { + const legacyByDid = new Map(legacy.accounts.map(a => [a.did, a])) + const secureDids = new Set(secure.accounts.map(a => a.did)) + let tokensFromLegacy = 0 + + const accounts = [ + ...secure.accounts.map(account => { + const other = legacyByDid.get(account.did) + if (!other) return account + if (freshest(account, other) === 'secure') return account + if (other.refreshJwt) tokensFromLegacy += 1 + return other + }), + /* + * Accounts the secure store never received. The blob is the only copy, so + * dropping them here is the multi-account logout this merge exists to stop. + */ + ...legacy.accounts.filter(account => { + if (secureDids.has(account.did)) return false + if (account.refreshJwt) tokensFromLegacy += 1 + return true + }), + ] + + const dids = new Set(accounts.map(account => account.did)) + const currentDid = secure.currentDid ?? legacy.currentDid + return { + snapshot: { + accounts, + currentDid: currentDid && dids.has(currentDid) ? currentDid : undefined, + }, + tokensFromLegacy, + } +} + +/** + * Which store holds the newer refresh token for one account. The caller takes + * the winner's account object whole, so its tokens and descriptor fields stay + * coherent with each other. + */ +function freshest( + secure: SessionAccount, + legacy: SessionAccount, +): 'secure' | 'legacy' { + if (secure.refreshJwt === legacy.refreshJwt) return 'secure' + if (!legacy.refreshJwt) return 'secure' + if (!secure.refreshJwt) return 'legacy' + const secureIssuedAt = issuedAt(secure.refreshJwt) + const legacyIssuedAt = issuedAt(legacy.refreshJwt) + if (secureIssuedAt === undefined || legacyIssuedAt === undefined) { + return 'legacy' + } + return secureIssuedAt > legacyIssuedAt ? 'secure' : 'legacy' +} + +/** A token's `iat` claim, or undefined if it cannot be read. */ +function issuedAt(token: string): number | undefined { + try { + const {iat} = jwtDecode(token) + return typeof iat === 'number' ? iat : undefined + } catch { + return undefined + } +} + function hasCreds(snapshot: SessionSnapshot): boolean { return snapshot.accounts.some(account => Boolean(account.refreshJwt)) } diff --git a/src/state/session/storage/index.ts b/src/state/session/storage/index.ts index 706dc855a1..a41366c572 100644 --- a/src/state/session/storage/index.ts +++ b/src/state/session/storage/index.ts @@ -46,8 +46,9 @@ let bootReport: SessionStorageBootReport | undefined * Call once during bootstrap, after `persisted.init()` has resolved and before * anything reads the session. * - * When the secure store wins, its snapshot is adopted by writing it back into - * the persisted state. `persisted.write` updates its in-memory copy + * When the secure store wins, its snapshot - merged per account with the blob, + * see `decideBootSource` - is adopted by writing it back into the persisted + * state. `persisted.write` updates its in-memory copy * synchronously before awaiting storage, so every existing reader - the * session store constructor, the last-active-account lookup, the expiry rescue * path - sees the adopted data with no seam of its own. It also means the @@ -90,10 +91,17 @@ export function initSessionStorage(): void { legacy, }) + store.setDurable(secure.status === 'ok' ? secure.snapshot : EMPTY_SNAPSHOT) if (decision.adopt) { adopt(decision.adopt) + /* + * The adopted snapshot is a merge, so it can hold credentials the + * keychain does not. The write diffs against the durable snapshot set + * just above - the secure store as it was read - so it no-ops when the + * merge took nothing from the blob, and retries if it fails. + */ + store.write(decision.adopt) } - store.setDurable(secure.status === 'ok' ? secure.snapshot : EMPTY_SNAPSHOT) if (decision.backfill === 'secure-from-legacy') { backfill(legacy, decision, installId) } diff --git a/src/state/session/storage/secureStore.ts b/src/state/session/storage/secureStore.ts index 93b79038be..73b4390cbb 100644 --- a/src/state/session/storage/secureStore.ts +++ b/src/state/session/storage/secureStore.ts @@ -207,10 +207,17 @@ export function writeSessions( for (const account of next.accounts) { const prior = previousByDid.get(account.did) const keys = accountKeys(account.did) - if (prior?.refreshJwt !== account.refreshJwt) { + /* + * The absent-prior case must write, not skip. A caller that distrusts its + * baseline passes an empty `previous` to force a full rewrite, and there + * `undefined !== undefined` would skip an account whose token is absent - + * leaving whatever the keychain still holds under that key to be read back + * as a live credential under an index that names the did. + */ + if (!prior || prior.refreshJwt !== account.refreshJwt) { SecureStore.setItem(keys.refresh, account.refreshJwt ?? '', WRITE_OPTIONS) } - if (prior?.accessJwt !== account.accessJwt) { + if (!prior || prior.accessJwt !== account.accessJwt) { SecureStore.setItem(keys.access, account.accessJwt ?? '', WRITE_OPTIONS) } const descriptor = canonicalJson(toDescriptor(account)) diff --git a/src/state/session/storage/store.ts b/src/state/session/storage/store.ts index 30e8855b2c..2b6f07aeb6 100644 --- a/src/state/session/storage/store.ts +++ b/src/state/session/storage/store.ts @@ -10,9 +10,8 @@ import {eraseSessions, writeSessions} from './secureStore' const RETRY_DELAY = 5_000 /** - * Pending work is a union so the sticky-clear rule reads as one guard: while a - * clear is pending, a write is dropped rather than risk resurrecting a session - * after a requested wipe. + * Pending work is a union: a failed write and a failed clear retry the same + * way, and only the newest of them is ever outstanding. */ type PendingWork = {type: 'write'; snapshot: SessionSnapshot} | {type: 'clear'; dids: string[]} @@ -45,6 +44,14 @@ export type SessionStorageStore = { export function createSessionStorageStore(): SessionStorageStore { let durableSnapshot: SessionSnapshot = EMPTY_SNAPSHOT let pending: PendingWork | undefined + /** + * Sticky: once a clear is requested, every later write is dropped for the + * rest of the process, including after the erase succeeds. Clearing storage + * does not stop the session, so an in-flight token refresh would otherwise + * mirror the credentials straight back into the keychain. The user is told to + * restart the app, and the next boot lifts the latch via `setDurable`. + */ + let clearRequested = false let retryTimer: ReturnType | undefined let retryTriggersAttached = false const maybeOrphanedDids = new Set() @@ -57,15 +64,17 @@ export function createSessionStorageStore(): SessionStorageStore { durableSnapshot = snapshot maybeOrphanedDids.clear() pending = undefined + clearRequested = false cancelRetry() } function write(next: SessionSnapshot) { - if (pending?.type === 'clear') return + if (clearRequested) return persist(next, 'write') } function clear() { + clearRequested = true const dids = [ ...new Set([ ...durableSnapshot.accounts.map(account => account.did), @@ -93,9 +102,26 @@ export function createSessionStorageStore(): SessionStorageStore { operation: SessionStorageErrorOperation, ) { trackMaybeOrphaned(next) + /* + * A pending write means the last attempt threw, and a throw can land + * anywhere in the write ordering - including after a journaled index was + * published. The baseline is then a guess, so it is thrown away: diffing + * from empty rewrites every account in `next` in full, retires everything + * else the store might still name, and force-publishes a clean index over + * any journal the failure left behind. + */ + const rebaseline = pending !== undefined + const previous = rebaseline ? EMPTY_SNAPSHOT : durableSnapshot + const alsoRetire = rebaseline + ? [ + ...durableSnapshot.accounts.map(account => account.did), + ...maybeOrphanedDids, + ] + : [...maybeOrphanedDids] try { - writeSessions(durableSnapshot, next, { - alsoRetire: [...maybeOrphanedDids], + writeSessions(previous, next, { + alsoRetire, + forceIndex: rebaseline, }) durableSnapshot = next maybeOrphanedDids.clear()