From 9a88386e4e8a5fb98bd8b50d7248b47ee565fd03 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 15 Jul 2026 00:55:57 -0700 Subject: [PATCH] fix(mobile): harden host re-pair deduplication --- mobile/src/transport/host-store.test.ts | 86 +++++++++++++++++-- mobile/src/transport/host-store.ts | 77 +++++++++-------- .../mobile-relay-host-overlay-store.test.ts | 21 +++++ .../mobile-relay-host-overlay-store.ts | 24 +++++- .../pre-profile-pairing-coordinator.test.ts | 11 +-- .../pre-profile-pairing-coordinator.ts | 24 ++---- 6 files changed, 181 insertions(+), 62 deletions(-) diff --git a/mobile/src/transport/host-store.test.ts b/mobile/src/transport/host-store.test.ts index 22ec7267785..17246f57eed 100644 --- a/mobile/src/transport/host-store.test.ts +++ b/mobile/src/transport/host-store.test.ts @@ -34,12 +34,13 @@ vi.mock('./host-credential-cleanup', () => ({ })) import { - findStoredHostByPublicKey, loadHosts, MobileRelayUpgradeHostRemovedError, removeHost, renameHost, + resolvePairingHostIdentity, resetHostStoreForTests, + saveHost, saveExistingHostRelayUpgrade, updateLastConnected } from './host-store' @@ -86,6 +87,8 @@ describe('host-store list mutations', () => { asyncStorageMock.setItem.mockImplementation(async (key: string, raw: string) => { if (key === HOSTS_STORAGE_KEY) { storedHostsRaw = raw + } else if (key === OVERLAY_STORAGE_KEY) { + storedOverlayRaw = raw } }) secureStoreMock.getItemAsync.mockImplementation(async (key: string) => @@ -93,16 +96,87 @@ describe('host-store list mutations', () => { ) }) - it('finds a stored host by its pinned public key (STA-1840 re-pair dedup)', async () => { - await expect(findStoredHostByPublicKey(HOST_TWO.publicKeyB64)).resolves.toEqual({ + it('resolves an existing host by pinned key with one durable read', async () => { + await expect(resolvePairingHostIdentity(HOST_TWO.publicKeyB64, 'host-new')).resolves.toEqual({ id: HOST_TWO.id, name: HOST_TWO.name }) + expect(asyncStorageMock.getItem).toHaveBeenCalledOnce() }) - it('returns null when no stored host has the public key, or the key is empty', async () => { - await expect(findStoredHostByPublicKey('unpaired-key')).resolves.toBeNull() - await expect(findStoredHostByPublicKey('')).resolves.toBeNull() + it('names a new host from the same durable read used for identity lookup', async () => { + await expect(resolvePairingHostIdentity('unpaired-key', 'host-new')).resolves.toEqual({ + id: 'host-new', + name: 'Host 3' + }) + expect(asyncStorageMock.getItem).toHaveBeenCalledOnce() + }) + + it('fails closed when durable host identity storage is unreadable', async () => { + asyncStorageMock.getItem.mockRejectedValueOnce(new Error('storage unavailable')) + + await expect(resolvePairingHostIdentity('key-new', 'host-new')).rejects.toThrow(/unreadable/) + expect(asyncStorageMock.setItem).not.toHaveBeenCalled() + }) + + it('collapses already-persisted duplicates when the desktop is re-paired', async () => { + storedHostsRaw = JSON.stringify([ + HOST_ONE, + { ...HOST_TWO, id: 'host-duplicate', publicKeyB64: HOST_ONE.publicKeyB64 } + ]) + + await saveHost({ + ...HOST_ONE, + endpoint: 'ws://127.0.0.1:3', + deviceToken: 'replacement-token' + }) + + expect(JSON.parse(storedHostsRaw)).toEqual([{ ...HOST_ONE, endpoint: 'ws://127.0.0.1:3' }]) + expect(scheduleCleanupMock).toHaveBeenCalledWith('host-duplicate', expect.any(Function)) + }) + + it('clears stale relay state when an existing host is re-paired direct-only', async () => { + const overlay: MobileRelayHostOverlay = { + v: 2, + hostId: HOST_ONE.id, + endpoints: [ + { id: 'direct-primary', kind: 'lan', url: HOST_ONE.endpoint }, + { + id: 'relay-primary', + kind: 'relay', + url: 'wss://relay-c1.onorca.dev/v1/connect/AbCdEf0123_-xyZ9' + } + ], + relayHostId: 'AbCdEf0123_-xyZ9', + relay: { + v: 1, + directorUrl: 'https://relay.onorca.dev', + cellUrl: 'https://relay-c1.onorca.dev', + assignmentEpoch: 7, + relayHostId: 'AbCdEf0123_-xyZ9', + e2eeFraming: 2 + } + } + storedOverlayRaw = JSON.stringify([overlay]) + + await saveHost({ ...HOST_ONE, deviceToken: 'replacement-token' }) + + expect(JSON.parse(storedOverlayRaw!)).toEqual([]) + expect(secureStoreMock.deleteItemAsync).not.toHaveBeenCalled() + }) + + it('does not touch relay storage when saving a new direct-only host', async () => { + await saveHost({ + id: 'host-new', + name: 'New Host', + endpoint: 'ws://127.0.0.1:3', + publicKeyB64: 'key-new', + deviceToken: 'new-token', + lastConnected: 0 + }) + + expect(asyncStorageMock.getItem).not.toHaveBeenCalledWith(OVERLAY_STORAGE_KEY) + expect(secureStoreMock.deleteItemAsync).not.toHaveBeenCalled() }) it('commits the removal when credential cleanup scheduling rejects', async () => { diff --git a/mobile/src/transport/host-store.ts b/mobile/src/transport/host-store.ts index 790eb8dc990..ad2d7ebc51a 100644 --- a/mobile/src/transport/host-store.ts +++ b/mobile/src/transport/host-store.ts @@ -15,6 +15,7 @@ import { import { loadMobileRelayHostOverlayState, removeMobileRelayHostOverlay, + removeMobileRelayHostOverlays, saveMobileRelayHostOverlay } from './mobile-relay-host-overlay-store' import { deleteMobileRelayCredentialBundle } from './mobile-relay-credential-bundle' @@ -182,31 +183,18 @@ async function doLoadHosts(): Promise { return out } -async function loadStoredHosts(): Promise { - try { - return parseStoredHosts(await AsyncStorage.getItem(STORAGE_KEY)) ?? [] - } catch { - return [] - } -} - -// Why: re-pairing the same desktop must merge into its existing card, not mint -// a new host id and duplicate the row (STA-1840). publicKeyB64 is the desktop's -// pinned identity key — stable across re-pairs, unlike id/endpoint/relay — so -// it is the right dedup key. Reads the durable stored list (not loadHosts) so a -// host whose keychain token read is transiently failing still matches and is -// not duplicated. -export async function findStoredHostByPublicKey( - publicKeyB64: string -): Promise<{ id: string; name: string } | null> { - if (!publicKeyB64) { - return null - } - // Why: settle any in-flight save/remove first so a re-pair racing its own - // prior write matches the just-persisted host rather than an empty snapshot. +export async function resolvePairingHostIdentity( + publicKeyB64: string, + newHostId: string +): Promise<{ id: string; name: string }> { + // Why: one durable read both preserves an existing identity and names a new host, + // avoiding duplicate cards and a second serial storage read before connecting. await hostListMutation - const match = (await loadStoredHosts()).find((h) => h.publicKeyB64 === publicKeyB64) - return match ? { id: match.id, name: match.name } : null + const hosts = await readStoredHostsForMutation() + const match = hosts.find((host) => host.publicKeyB64 === publicKeyB64) + return match + ? { id: match.id, name: match.name } + : { id: newHostId, name: getNextHostNameFromHosts(hosts) } } async function readStoredHostsForMutation(): Promise { @@ -261,18 +249,28 @@ export async function saveExistingHostRelayUpgrade(host: HostProfile): Promise { const validated = HostProfileSchema.parse(host) const stored = toStored(validated) + const duplicateHostIds = new Set() + let updatedExistingHost = false await mutateStoredHosts((hosts) => { const index = hosts.findIndex((h) => h.id === stored.id) + for (const candidate of hosts) { + if (candidate.id !== stored.id && candidate.publicKeyB64 === stored.publicKeyB64) { + duplicateHostIds.add(candidate.id) + } + } if (index >= 0) { - const next = hosts.slice() - next[index] = stored - return next + updatedExistingHost = true + // Why: affected installs may already contain duplicate rows; an authoritative + // save is the safe point to collapse them to the preserved host id. + return hosts + .filter(({ id }) => !duplicateHostIds.has(id)) + .map((candidate) => (candidate.id === stored.id ? stored : candidate)) } if (requireExisting) { // Why: an in-flight relay upgrade must not resurrect a host the user removed. throw new MobileRelayUpgradeHostRemovedError('mobile relay upgrade host was removed') } - return [...hosts, stored] + return [...hosts.filter(({ id }) => !duplicateHostIds.has(id)), stored] }) // Why: write metadata BEFORE the keychain token so a crash between the two // leaves orphaned metadata (which loadHosts skips and removeHost can clean @@ -290,6 +288,23 @@ async function persistHost(host: HostProfile, requireExisting: boolean): Promise relay: validated.relay }) } + const overlayRemovalIds = [...duplicateHostIds] + if (!validated.endpoints && updatedExistingHost) { + overlayRemovalIds.push(stored.id) + } + if (overlayRemovalIds.length > 0) { + // Why: reusing an id for direct-only re-pairing must not retain routing + // metadata from the host's previous transport state. + await removeMobileRelayHostOverlays(overlayRemovalIds) + } + for (const duplicateHostId of duplicateHostIds) { + tokenCache.delete(duplicateHostId) + try { + await scheduleHostCredentialCleanup(duplicateHostId, deleteHostCredentials) + } catch { + // Metadata is already deduplicated; orphan-token recovery is best-effort. + } + } } export async function removeHost(hostId: string): Promise { @@ -330,12 +345,6 @@ export async function renameHost(hostId: string, newName: string): Promise }) } -export async function getNextHostName(): Promise { - await hostListMutation - const hosts = await loadStoredHosts() - return getNextHostNameFromHosts(hosts) -} - export async function updateLastConnected(hostId: string): Promise { try { await mutateStoredHosts((hosts) => { diff --git a/mobile/src/transport/mobile-relay-host-overlay-store.test.ts b/mobile/src/transport/mobile-relay-host-overlay-store.test.ts index 8286fe67f5b..812d61ebb1d 100644 --- a/mobile/src/transport/mobile-relay-host-overlay-store.test.ts +++ b/mobile/src/transport/mobile-relay-host-overlay-store.test.ts @@ -9,6 +9,7 @@ vi.mock('@react-native-async-storage/async-storage', () => ({ default: asyncStor import { loadMobileRelayHostOverlays, + removeMobileRelayHostOverlays, resetMobileRelayHostOverlayStoreForTests, saveMobileRelayHostOverlay } from './mobile-relay-host-overlay-store' @@ -77,4 +78,24 @@ describe('mobile relay host overlay store', () => { await expect(saveMobileRelayHostOverlay(OVERLAY)).rejects.toThrow(/unreadable/) expect(asyncStorage.setItem).not.toHaveBeenCalled() }) + + it('removes requested overlays in one storage write', async () => { + const second = { ...OVERLAY, hostId: 'host-2' } + stored = JSON.stringify([OVERLAY, second]) + + await expect(removeMobileRelayHostOverlays(['host-1', 'host-missing'])).resolves.toBeUndefined() + + expect(JSON.parse(stored!)).toEqual([second]) + expect(asyncStorage.getItem).toHaveBeenCalledOnce() + expect(asyncStorage.setItem).toHaveBeenCalledOnce() + }) + + it('skips the storage write when no requested overlay exists', async () => { + stored = JSON.stringify([OVERLAY]) + + await expect(removeMobileRelayHostOverlays(['host-missing'])).resolves.toBeUndefined() + + expect(asyncStorage.getItem).toHaveBeenCalledOnce() + expect(asyncStorage.setItem).not.toHaveBeenCalled() + }) }) diff --git a/mobile/src/transport/mobile-relay-host-overlay-store.ts b/mobile/src/transport/mobile-relay-host-overlay-store.ts index 4dbb50d9f46..7f1519eb5c9 100644 --- a/mobile/src/transport/mobile-relay-host-overlay-store.ts +++ b/mobile/src/transport/mobile-relay-host-overlay-store.ts @@ -40,7 +40,12 @@ async function mutateOverlays( ): Promise { const mutation = overlayMutation.then(async () => { const current = await readOverlaysForMutation() - await AsyncStorage.setItem(OVERLAY_STORAGE_KEY, JSON.stringify(update(current))) + const next = update(current) + // Why: direct-only saves commonly have no overlay to remove; avoid a full + // AsyncStorage write when cleanup leaves the durable list unchanged. + if (next !== current) { + await AsyncStorage.setItem(OVERLAY_STORAGE_KEY, JSON.stringify(next)) + } }) overlayMutation = mutation.catch(() => {}) return mutation @@ -85,7 +90,22 @@ export async function saveMobileRelayHostOverlay(overlay: MobileRelayHostOverlay } export function removeMobileRelayHostOverlay(hostId: string): Promise { - return mutateOverlays((overlays) => overlays.filter((overlay) => overlay.hostId !== hostId)) + return removeMobileRelayHostOverlays([hostId]) +} + +export function removeMobileRelayHostOverlays(hostIds: readonly string[]): Promise { + const targets = new Set(hostIds) + let removed = false + return mutateOverlays((overlays) => { + const next = overlays.filter((overlay) => { + if (!targets.has(overlay.hostId)) { + return true + } + removed = true + return false + }) + return removed ? next : overlays + }) } /** Test-only: drain the module mutation chain between cases. */ diff --git a/mobile/src/transport/pre-profile-pairing-coordinator.test.ts b/mobile/src/transport/pre-profile-pairing-coordinator.test.ts index 8f39e2b36a2..782c02aed50 100644 --- a/mobile/src/transport/pre-profile-pairing-coordinator.test.ts +++ b/mobile/src/transport/pre-profile-pairing-coordinator.test.ts @@ -64,8 +64,10 @@ function dependencies(client: RpcClient, events: string[]) { resolveInviteDirector: vi.fn(async () => { throw new Error('director unavailable') }), - getNextHostName: vi.fn(async () => 'Blue Whale'), - findExistingHost: vi.fn(async (_publicKeyB64: string) => null), + resolveHostIdentity: vi.fn(async (_publicKeyB64: string, hostId: string) => ({ + id: hostId, + name: 'Blue Whale' + })), saveHost: vi.fn(async (_host: HostProfile) => { events.push('save-host') }), @@ -143,8 +145,9 @@ describe('pre-profile pairing coordinator', () => { const events: string[] = [] const client = fakeClient([success({ version: '1.0.0' })]) const deps = dependencies(client, events) - deps.findExistingHost = vi.fn(async (publicKeyB64: string) => { + deps.resolveHostIdentity = vi.fn(async (publicKeyB64: string, newHostId: string) => { expect(publicKeyB64).toBe(directOffer.publicKeyB64) + expect(newHostId).toBe(`host-${now}`) return { id: 'host-existing', name: 'Studio Mac' } }) @@ -163,8 +166,6 @@ describe('pre-profile pairing coordinator', () => { publicKeyB64: directOffer.publicKeyB64, lastConnected: now }) - // Existing host keeps its name — no fresh name minted. - expect(deps.getNextHostName).not.toHaveBeenCalled() }) it('journals before connecting and publishes only after authoritative direct install', async () => { diff --git a/mobile/src/transport/pre-profile-pairing-coordinator.ts b/mobile/src/transport/pre-profile-pairing-coordinator.ts index b66aeeb12e7..82bd989be03 100644 --- a/mobile/src/transport/pre-profile-pairing-coordinator.ts +++ b/mobile/src/transport/pre-profile-pairing-coordinator.ts @@ -6,7 +6,7 @@ import { type MobileRelayEndpoint } from '../../../src/shared/mobile-relay-credential-contract' import { connect, type ConnectOptions } from './rpc-client' -import { findStoredHostByPublicKey, getNextHostName, saveHost } from './host-store' +import { resolvePairingHostIdentity, saveHost } from './host-store' import type { HostProfile, PairingOffer, RpcResponse } from './types' import { createMobileRelayPairingJournal, @@ -39,8 +39,7 @@ type Dependencies = { connectDirect: typeof connect connectRelay: typeof connectMobileRelayForPairing resolveInviteDirector: typeof resolvePairingInviteThroughDirector - getNextHostName: typeof getNextHostName - findExistingHost: typeof findStoredHostByPublicKey + resolveHostIdentity: typeof resolvePairingHostIdentity saveHost: typeof saveHost saveJournal: typeof saveMobileRelayPairingJournal updateJournal: typeof updateMobileRelayPairingJournal @@ -54,8 +53,7 @@ const defaultDependencies: Dependencies = { connectDirect: connect, connectRelay: connectMobileRelayForPairing, resolveInviteDirector: resolvePairingInviteThroughDirector, - getNextHostName, - findExistingHost: findStoredHostByPublicKey, + resolveHostIdentity: resolvePairingHostIdentity, saveHost, saveJournal: saveMobileRelayPairingJournal, updateJournal: updateMobileRelayPairingJournal, @@ -132,16 +130,12 @@ async function runPairing( isDisposed: () => boolean ): Promise<{ hostId: string }> { const now = dependencies.now() - // Why: if this desktop (by its pinned public key) is already paired, reuse - // that host's id and name so the pairing merges into the existing card - // instead of minting a fresh host-${now} and duplicating the row (STA-1840). - // Every downstream write (journal, credential bundle, saveHost) then keys to - // the same id, so persistHost updates in place. A new desktop key falls - // through to a fresh id/name. - const existing = await dependencies.findExistingHost(offer.publicKeyB64) - assertActive(isDisposed) - const hostId = existing?.id ?? `host-${now}` - const hostName = existing?.name ?? (await dependencies.getNextHostName()) + // Why: every pairing artifact must share the preserved host id so re-pairing + // updates one card instead of publishing a second identity (STA-1840). + const { id: hostId, name: hostName } = await dependencies.resolveHostIdentity( + offer.publicKeyB64, + `host-${now}` + ) assertActive(isDisposed) let journal: MobileRelayPairingJournal | null = null if (offer.relay && dependencies.platform !== 'web') {