From ac09f73c025e3e0d0c6af22b14774ffa4432e368 Mon Sep 17 00:00:00 2001 From: Erwan Leboucher Date: Sun, 26 Jul 2026 15:33:22 +0200 Subject: [PATCH 1/3] fix(crypto): allow restoring key backup on a verified device --- ...fix-backup-key-recovery-verified-device.md | 5 + src/app/components/BackupRestore.test.tsx | 100 ++++++++++++++++ src/app/components/BackupRestore.tsx | 111 +++++++++++++++++- src/app/components/ManualVerification.tsx | 50 +++----- src/app/components/SecretStorage.tsx | 37 ++++++ src/app/features/settings/devices/Devices.tsx | 6 +- src/app/hooks/useKeyBackup.test.tsx | 66 +++++++++++ src/app/hooks/useKeyBackup.ts | 25 ++++ .../useRestoreBackupOnVerification.test.tsx | 72 ++++++++++++ .../hooks/useRestoreBackupOnVerification.ts | 29 +++-- src/app/state/backupRestore.test.ts | 29 +++++ src/app/state/backupRestore.ts | 8 ++ 12 files changed, 494 insertions(+), 44 deletions(-) create mode 100644 .changeset/fix-backup-key-recovery-verified-device.md create mode 100644 src/app/components/BackupRestore.test.tsx create mode 100644 src/app/hooks/useKeyBackup.test.tsx create mode 100644 src/app/hooks/useRestoreBackupOnVerification.test.tsx create mode 100644 src/app/state/backupRestore.test.ts diff --git a/.changeset/fix-backup-key-recovery-verified-device.md b/.changeset/fix-backup-key-recovery-verified-device.md new file mode 100644 index 0000000000..f6d7a5bee7 --- /dev/null +++ b/.changeset/fix-backup-key-recovery-verified-device.md @@ -0,0 +1,5 @@ +--- +default: patch +--- + +# Fix being unable to restore key backup on a verified device diff --git a/src/app/components/BackupRestore.test.tsx b/src/app/components/BackupRestore.test.tsx new file mode 100644 index 0000000000..de7257b9e0 --- /dev/null +++ b/src/app/components/BackupRestore.test.tsx @@ -0,0 +1,100 @@ +import { render, screen, waitFor } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { TypedEventEmitter } from 'matrix-js-sdk/lib/models/typed-event-emitter'; +import type { CryptoApi, KeyBackupInfo } from '$types/matrix-sdk'; +import type { SecretStorageKeyContent } from '$types/matrix/accountData'; +import { BackupRestoreTile } from './BackupRestore'; + +const emitter = new TypedEventEmitter void>>(); +const mockClient = Object.assign(emitter, { + secretStorage: { checkKey: vi.fn<() => Promise>().mockResolvedValue(true) }, +}); + +vi.mock('$hooks/useMatrixClient', () => ({ + useMatrixClient: () => mockClient, +})); + +vi.mock('$client/secretStorageKeys', () => ({ + storePrivateKey: vi.fn<() => void>(), +})); + +const KEY_ID = 'key-id'; +const KEY_CONTENT = { algorithm: 'm.secret_storage.v1.aes-hmac-sha2' } as SecretStorageKeyContent; +const BACKUP_INFO = { version: '3', count: 42 } as KeyBackupInfo; + +type CryptoOverrides = { + backupInfo?: KeyBackupInfo | null; + backupKey?: Uint8Array | null; +}; + +const createCrypto = ({ backupInfo = BACKUP_INFO, backupKey = null }: CryptoOverrides = {}) => + ({ + getActiveSessionBackupVersion: vi + .fn() + .mockResolvedValue(backupInfo ? '3' : null), + getKeyBackupInfo: vi.fn().mockResolvedValue(backupInfo), + getSessionBackupPrivateKey: vi + .fn() + .mockResolvedValue(backupKey), + isKeyBackupTrusted: vi + .fn() + .mockResolvedValue({ trusted: true, matchesDecryptionKey: true }), + restoreKeyBackup: vi.fn<() => Promise>().mockResolvedValue(undefined), + loadSessionBackupPrivateKeyFromSecretStorage: vi + .fn() + .mockResolvedValue(undefined), + }) as unknown as CryptoApi; + +const recoveryPrompt = () => screen.queryByText(/does not hold the backup decryption key/i); + +describe('BackupRestoreTile', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('offers recovery when a backup exists but its key is not in the crypto store', async () => { + render( + + ); + + await waitFor(() => expect(recoveryPrompt()).not.toBeNull()); + expect(screen.getByText('Recovery Key')).toBeTruthy(); + }); + + it('stays out of the way once the key is cached', async () => { + render( + + ); + + await waitFor(() => expect(screen.getByText('Encryption Backup')).toBeTruthy()); + expect(recoveryPrompt()).toBeNull(); + }); + + it('does not offer recovery when the server has no backup at all', async () => { + render( + + ); + + await waitFor(() => expect(screen.getByText(/No backup present on server/i)).toBeTruthy()); + expect(recoveryPrompt()).toBeNull(); + }); + + it('cannot offer recovery without secret storage configured', async () => { + render(); + + await waitFor(() => expect(screen.getByText('Encryption Backup')).toBeTruthy()); + expect(recoveryPrompt()).toBeNull(); + }); +}); diff --git a/src/app/components/BackupRestore.tsx b/src/app/components/BackupRestore.tsx index a16ed784d8..b668329220 100644 --- a/src/app/components/BackupRestore.tsx +++ b/src/app/components/BackupRestore.tsx @@ -1,12 +1,13 @@ import type { MouseEventHandler } from 'react'; import { useCallback, useState } from 'react'; -import { useAtom } from 'jotai'; +import { useAtom, useAtomValue } from 'jotai'; import type { CryptoApi, KeyBackupInfo } from '$types/matrix-sdk'; import type { RectCords } from 'folds'; import { Badge, Box, Button, + Chip, color, config, IconButton, @@ -18,14 +19,23 @@ import { Text, } from 'folds'; import FocusTrap from 'focus-trap-react'; -import { BackupProgressStatus, backupRestoreProgressAtom } from '$state/backupRestore'; +import type { SecretStorageKeyContent } from '$types/matrix/accountData'; +import { storePrivateKey } from '$client/secretStorageKeys'; +import { + BackupProgressStatus, + backupRestoreErrorAtom, + backupRestoreProgressAtom, + isMissingBackupKeyError, +} from '$state/backupRestore'; import { AsyncStatus, useAsyncCallback } from '$hooks/useAsyncCallback'; import { useKeyBackupInfo, useKeyBackupStatus, useKeyBackupSync, useKeyBackupTrust, + useSessionBackupKeyCached, } from '$hooks/useKeyBackup'; +import { SecretStorageKeyMethod, SecretStorageKeyPrompt } from './SecretStorage'; import { stopPropagation } from '$utils/keyboard'; import { useRestoreBackupOnVerification } from '$hooks/useRestoreBackupOnVerification'; import { @@ -36,6 +46,75 @@ import { } from '$components/icons/phosphor'; import { InfoCard } from './info-card'; +type BackupKeyRecoveryProps = { + crypto: CryptoApi; + secretStorageKeyId: string; + secretStorageKeyContent: SecretStorageKeyContent; +}; +function BackupKeyRecovery({ + crypto, + secretStorageKeyId, + secretStorageKeyContent, +}: BackupKeyRecoveryProps) { + const hasPassphrase = !!secretStorageKeyContent.passphrase; + const [method, setMethod] = useState( + hasPassphrase ? SecretStorageKeyMethod.RecoveryPassphrase : SecretStorageKeyMethod.RecoveryKey + ); + + const [unlockState, unlockBackup] = useAsyncCallback( + useCallback( + async (recoveryKey: Uint8Array) => { + storePrivateKey(secretStorageKeyId, recoveryKey); + // Emits KeyBackupDecryptionKeyCached, which drives the restore. + await crypto.loadSessionBackupPrivateKeyFromSecretStorage(); + }, + [crypto, secretStorageKeyId] + ) + ); + + const otherMethod = + method === SecretStorageKeyMethod.RecoveryPassphrase + ? SecretStorageKeyMethod.RecoveryKey + : SecretStorageKeyMethod.RecoveryPassphrase; + + return ( + + + This device does not hold the backup decryption key. Provide your recovery details to unlock + the backup and restore your message history. + + + {hasPassphrase && ( + + setMethod(otherMethod)} + > + + {otherMethod === SecretStorageKeyMethod.RecoveryPassphrase + ? 'Use Recovery Passphrase' + : 'Use Recovery Key'} + + + + )} + {unlockState.status === AsyncStatus.Error && ( + + {unlockState.error.message} + + )} + + ); +} + type BackupStatusProps = { enabled: boolean; }; @@ -138,15 +217,23 @@ function BackupTrustInfo({ crypto, backupInfo }: BackupTrustInfoProps) { type BackupRestoreTileProps = { crypto: CryptoApi; + secretStorageKeyId?: string; + secretStorageKeyContent?: SecretStorageKeyContent; }; -export function BackupRestoreTile({ crypto }: BackupRestoreTileProps) { +export function BackupRestoreTile({ + crypto, + secretStorageKeyId, + secretStorageKeyContent, +}: BackupRestoreTileProps) { const [restoreProgress, setRestoreProgress] = useAtom(backupRestoreProgressAtom); + const autoRestoreError = useAtomValue(backupRestoreErrorAtom); const restoring = restoreProgress.status === BackupProgressStatus.Fetching || restoreProgress.status === BackupProgressStatus.Loading; const backupEnabled = useKeyBackupStatus(crypto); const backupInfo = useKeyBackupInfo(crypto); + const backupKeyCached = useSessionBackupKeyCached(crypto); const [remainingSession, syncFailure] = useKeyBackupSync(); const [menuCords, setMenuCords] = useState(); @@ -170,6 +257,12 @@ export function BackupRestoreTile({ crypto }: BackupRestoreTileProps) { restoreBackup(); }; + const needsBackupKey = + !!backupInfo && + (backupKeyCached === false || + (restoreState.status === AsyncStatus.Error && isMissingBackupKeyError(restoreState.error)) || + isMissingBackupKeyError(autoRestoreError)); + return ( {restoreState.error.message} )} + {autoRestoreError && ( + + {autoRestoreError} + + )} + {needsBackupKey && secretStorageKeyId && secretStorageKeyContent && ( + + )} ); } diff --git a/src/app/components/ManualVerification.tsx b/src/app/components/ManualVerification.tsx index 5ce90cc6ea..980623c015 100644 --- a/src/app/components/ManualVerification.tsx +++ b/src/app/components/ManualVerification.tsx @@ -11,15 +11,11 @@ import { useMatrixClient } from '$hooks/useMatrixClient'; import { AsyncStatus, useAsyncCallback } from '$hooks/useAsyncCallback'; import { AsyncError } from '$components/AsyncError'; import { SettingTile } from './setting-tile'; -import { SecretStorageRecoveryKey, SecretStorageRecoveryPassphrase } from './SecretStorage'; +import { SecretStorageKeyMethod, SecretStorageKeyPrompt } from './SecretStorage'; -enum ManualVerificationMethod { - RecoveryPassphrase = 'passphrase', - RecoveryKey = 'key', -} type ManualVerificationMethodSwitcherProps = { - value: ManualVerificationMethod; - onChange: (value: ManualVerificationMethod) => void; + value: SecretStorageKeyMethod; + onChange: (value: SecretStorageKeyMethod) => void; }; function ManualVerificationMethodSwitcher({ value, @@ -31,7 +27,7 @@ function ManualVerificationMethodSwitcher({ setMenuCords(evt.currentTarget.getBoundingClientRect()); }; - const handleSelect = (method: ManualVerificationMethod) => { + const handleSelect = (method: SecretStorageKeyMethod) => { setMenuCords(undefined); onChange(method); }; @@ -47,8 +43,8 @@ function ManualVerificationMethodSwitcher({ onClick={handleMenu} > - {value === ManualVerificationMethod.RecoveryPassphrase && 'Recovery Passphrase'} - {value === ManualVerificationMethod.RecoveryKey && 'Recovery Key'} + {value === SecretStorageKeyMethod.RecoveryPassphrase && 'Recovery Passphrase'} + {value === SecretStorageKeyMethod.RecoveryKey && 'Recovery Key'} handleSelect(ManualVerificationMethod.RecoveryPassphrase)} + onClick={() => handleSelect(SecretStorageKeyMethod.RecoveryPassphrase)} > Recovery Passphrase @@ -85,9 +81,9 @@ function ManualVerificationMethodSwitcher({ handleSelect(ManualVerificationMethod.RecoveryKey)} + onClick={() => handleSelect(SecretStorageKeyMethod.RecoveryKey)} > Recovery Key @@ -116,9 +112,7 @@ export function ManualVerificationTile({ const hasPassphrase = !!secretStorageKeyContent.passphrase; const [method, setMethod] = useState( - hasPassphrase - ? ManualVerificationMethod.RecoveryPassphrase - : ManualVerificationMethod.RecoveryKey + hasPassphrase ? SecretStorageKeyMethod.RecoveryPassphrase : SecretStorageKeyMethod.RecoveryKey ); const verifyAndRestoreBackup = useCallback( @@ -163,22 +157,12 @@ export function ManualVerificationTile({ ) : ( - {method === ManualVerificationMethod.RecoveryKey && ( - - )} - {method === ManualVerificationMethod.RecoveryPassphrase && - secretStorageKeyContent.passphrase && ( - - )} + )} diff --git a/src/app/components/SecretStorage.tsx b/src/app/components/SecretStorage.tsx index d408937232..1b63d1087b 100644 --- a/src/app/components/SecretStorage.tsx +++ b/src/app/components/SecretStorage.tsx @@ -13,6 +13,11 @@ import { useAlive } from '$hooks/useAlive'; import { PasswordInput } from './password-input'; import { Button } from '$components/button'; +export enum SecretStorageKeyMethod { + RecoveryPassphrase = 'passphrase', + RecoveryKey = 'key', +} + type SecretStorageRecoveryPassphraseProps = { processing?: boolean; keyContent: SecretStorageKeyContent; @@ -113,6 +118,38 @@ export function SecretStorageRecoveryPassphrase({ ); } +type SecretStorageKeyPromptProps = { + method: SecretStorageKeyMethod; + processing?: boolean; + keyContent: SecretStorageKeyContent; + onDecodedRecoveryKey: (recoveryKey: Uint8Array) => void; +}; +export function SecretStorageKeyPrompt({ + method, + processing, + keyContent, + onDecodedRecoveryKey, +}: SecretStorageKeyPromptProps) { + if (method === SecretStorageKeyMethod.RecoveryPassphrase && keyContent.passphrase) { + return ( + + ); + } + + return ( + + ); +} + type SecretStorageRecoveryKeyProps = { processing?: boolean; keyContent: SecretStorageKeyContent; diff --git a/src/app/features/settings/devices/Devices.tsx b/src/app/features/settings/devices/Devices.tsx index b430b89b19..1286367848 100644 --- a/src/app/features/settings/devices/Devices.tsx +++ b/src/app/features/settings/devices/Devices.tsx @@ -121,7 +121,11 @@ export function Devices({ requestBack, requestClose }: DevicesProps) { /> )} {crypto && verificationStatus === VerificationStatus.Verified && ( - + )} ) : ( diff --git a/src/app/hooks/useKeyBackup.test.tsx b/src/app/hooks/useKeyBackup.test.tsx new file mode 100644 index 0000000000..1bad948cc3 --- /dev/null +++ b/src/app/hooks/useKeyBackup.test.tsx @@ -0,0 +1,66 @@ +import { renderHook, waitFor } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { TypedEventEmitter } from 'matrix-js-sdk/lib/models/typed-event-emitter'; +import type { CryptoApi } from '$types/matrix-sdk'; +import { CryptoEvent } from '$types/matrix-sdk'; +import { useSessionBackupKeyCached } from './useKeyBackup'; + +const emitter = new TypedEventEmitter void>>(); + +vi.mock('$hooks/useMatrixClient', () => ({ + useMatrixClient: () => emitter, +})); + +const createCrypto = (getSessionBackupPrivateKey: CryptoApi['getSessionBackupPrivateKey']) => + ({ getSessionBackupPrivateKey }) as unknown as CryptoApi; + +describe('useSessionBackupKeyCached', () => { + beforeEach(() => { + emitter.removeAllListeners(); + }); + + it('reports true when the key is in the crypto store', async () => { + const crypto = createCrypto( + vi.fn().mockResolvedValue(new Uint8Array([1, 2, 3])) + ); + + const { result } = renderHook(() => useSessionBackupKeyCached(crypto)); + + await waitFor(() => expect(result.current).toBe(true)); + }); + + it('reports false when the store has no key', async () => { + const crypto = createCrypto( + vi.fn().mockResolvedValue(null) + ); + + const { result } = renderHook(() => useSessionBackupKeyCached(crypto)); + + await waitFor(() => expect(result.current).toBe(false)); + }); + + it('reports false rather than hanging when the lookup rejects', async () => { + const crypto = createCrypto( + vi.fn().mockRejectedValue(new Error('store closed')) + ); + + const { result } = renderHook(() => useSessionBackupKeyCached(crypto)); + + await waitFor(() => expect(result.current).toBe(false)); + }); + + it('re-checks once the key gets cached', async () => { + const getKey = vi + .fn() + .mockResolvedValueOnce(null) + .mockResolvedValue(new Uint8Array([1])); + const crypto = createCrypto(getKey); + + const { result } = renderHook(() => useSessionBackupKeyCached(crypto)); + await waitFor(() => expect(result.current).toBe(false)); + + emitter.emit(CryptoEvent.KeyBackupDecryptionKeyCached as never); + + await waitFor(() => expect(result.current).toBe(true)); + }); +}); diff --git a/src/app/hooks/useKeyBackup.ts b/src/app/hooks/useKeyBackup.ts index 2c6eb1736f..4949d5306b 100644 --- a/src/app/hooks/useKeyBackup.ts +++ b/src/app/hooks/useKeyBackup.ts @@ -56,6 +56,31 @@ export const useKeyBackupDecryptionKeyCached = ( useMatrixEvent(mx, CryptoEvent.KeyBackupDecryptionKeyCached, onChange); }; +/** undefined while the first lookup is in flight. */ +export const useSessionBackupKeyCached = (crypto: CryptoApi): boolean | undefined => { + const alive = useAlive(); + const [cached, setCached] = useState(); + + const fetchCached = useCallback(() => { + crypto + .getSessionBackupPrivateKey() + .then((key) => { + if (alive()) setCached(key !== null); + }) + .catch(() => { + if (alive()) setCached(false); + }); + }, [crypto, alive]); + + useEffect(() => { + fetchCached(); + }, [fetchCached]); + + useKeyBackupDecryptionKeyCached(fetchCached); + + return cached; +}; + export const useKeyBackupSync = (): [number, string | undefined] => { const [remaining, setRemaining] = useState(0); const [failure, setFailure] = useState(); diff --git a/src/app/hooks/useRestoreBackupOnVerification.test.tsx b/src/app/hooks/useRestoreBackupOnVerification.test.tsx new file mode 100644 index 0000000000..4a31e8daae --- /dev/null +++ b/src/app/hooks/useRestoreBackupOnVerification.test.tsx @@ -0,0 +1,72 @@ +import { renderHook, waitFor } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { TypedEventEmitter } from 'matrix-js-sdk/lib/models/typed-event-emitter'; +import { Provider, useAtomValue } from 'jotai'; +import type { ReactNode } from 'react'; +import { CryptoEvent } from '$types/matrix-sdk'; +import { backupRestoreErrorAtom } from '$state/backupRestore'; +import { useRestoreBackupOnVerification } from './useRestoreBackupOnVerification'; + +const restoreKeyBackup = vi.fn<() => Promise>(); +const emitter = new TypedEventEmitter void>>(); +const mockClient = Object.assign(emitter, { + getCrypto: () => ({ restoreKeyBackup }), +}); + +vi.mock('$hooks/useMatrixClient', () => ({ + useMatrixClient: () => mockClient, +})); + +vi.mock('@sentry/react', () => ({ + addBreadcrumb: vi.fn<() => void>(), + metrics: { count: vi.fn<() => void>() }, +})); + +function Wrapper({ children }: { children: ReactNode }) { + return {children}; +} + +const useSubject = () => { + useRestoreBackupOnVerification(); + return useAtomValue(backupRestoreErrorAtom); +}; + +const emitKeyCached = () => emitter.emit(CryptoEvent.KeyBackupDecryptionKeyCached as never); + +describe('useRestoreBackupOnVerification', () => { + beforeEach(() => { + restoreKeyBackup.mockReset(); + }); + + it('surfaces a restore failure instead of rejecting silently', async () => { + restoreKeyBackup.mockRejectedValue(new Error('No decryption key found in crypto store')); + + const { result } = renderHook(useSubject, { wrapper: Wrapper }); + emitKeyCached(); + + await waitFor(() => expect(result.current).toBe('No decryption key found in crypto store')); + }); + + it('leaves no error behind on a successful restore', async () => { + restoreKeyBackup.mockResolvedValue(undefined); + + const { result } = renderHook(useSubject, { wrapper: Wrapper }); + emitKeyCached(); + + await waitFor(() => expect(restoreKeyBackup).toHaveBeenCalled()); + expect(result.current).toBeUndefined(); + }); + + it('clears a previous failure when a later restore is attempted', async () => { + restoreKeyBackup.mockRejectedValueOnce(new Error('boom')); + + const { result } = renderHook(useSubject, { wrapper: Wrapper }); + emitKeyCached(); + await waitFor(() => expect(result.current).toBe('boom')); + + restoreKeyBackup.mockResolvedValue(undefined); + emitKeyCached(); + + await waitFor(() => expect(result.current).toBeUndefined()); + }); +}); diff --git a/src/app/hooks/useRestoreBackupOnVerification.ts b/src/app/hooks/useRestoreBackupOnVerification.ts index 8a8cf93d73..a2b3f5c6e8 100644 --- a/src/app/hooks/useRestoreBackupOnVerification.ts +++ b/src/app/hooks/useRestoreBackupOnVerification.ts @@ -1,11 +1,13 @@ import { useSetAtom } from 'jotai'; import { useCallback } from 'react'; -import { backupRestoreProgressAtom } from '$state/backupRestore'; +import * as Sentry from '@sentry/react'; +import { backupRestoreErrorAtom, backupRestoreProgressAtom } from '$state/backupRestore'; import { useMatrixClient } from './useMatrixClient'; import { useKeyBackupDecryptionKeyCached } from './useKeyBackup'; export const useRestoreBackupOnVerification = () => { const setRestoreProgress = useSetAtom(backupRestoreProgressAtom); + const setRestoreError = useSetAtom(backupRestoreErrorAtom); const mx = useMatrixClient(); @@ -14,11 +16,24 @@ export const useRestoreBackupOnVerification = () => { const crypto = mx.getCrypto(); if (!crypto) return; - crypto.restoreKeyBackup({ - progressCallback(progress) { - setRestoreProgress(progress); - }, - }); - }, [mx, setRestoreProgress]) + setRestoreError(undefined); + crypto + .restoreKeyBackup({ + progressCallback(progress) { + setRestoreProgress(progress); + }, + }) + .catch((err: unknown) => { + const message = err instanceof Error ? err.message : String(err); + Sentry.addBreadcrumb({ + category: 'crypto', + message: 'Key backup restore failed', + level: 'error', + data: { error: message }, + }); + Sentry.metrics.count('sable.crypto.key_backup_restore_failures', 1); + setRestoreError(message); + }); + }, [mx, setRestoreProgress, setRestoreError]) ); }; diff --git a/src/app/state/backupRestore.test.ts b/src/app/state/backupRestore.test.ts new file mode 100644 index 0000000000..60faf13e5b --- /dev/null +++ b/src/app/state/backupRestore.test.ts @@ -0,0 +1,29 @@ +import { describe, expect, it } from 'vitest'; +import { isMissingBackupKeyError } from './backupRestore'; + +describe('isMissingBackupKeyError', () => { + it('matches the error the crypto store raises without a cached key', () => { + expect(isMissingBackupKeyError(new Error('No decryption key found in crypto store'))).toBe( + true + ); + }); + + it('matches the same error as a plain string', () => { + expect(isMissingBackupKeyError('No decryption key found in crypto store')).toBe(true); + }); + + it('ignores case', () => { + expect(isMissingBackupKeyError(new Error('NO DECRYPTION KEY FOUND'))).toBe(true); + }); + + it('does not match unrelated restore failures', () => { + expect(isMissingBackupKeyError(new Error('Backup version not found'))).toBe(false); + expect(isMissingBackupKeyError(new Error('M_LIMIT_EXCEEDED'))).toBe(false); + }); + + it('handles absent and non-stringifiable values', () => { + expect(isMissingBackupKeyError(undefined)).toBe(false); + expect(isMissingBackupKeyError(null)).toBe(false); + expect(isMissingBackupKeyError({})).toBe(false); + }); +}); diff --git a/src/app/state/backupRestore.ts b/src/app/state/backupRestore.ts index 8b411ebee1..e8bce791d6 100644 --- a/src/app/state/backupRestore.ts +++ b/src/app/state/backupRestore.ts @@ -33,6 +33,14 @@ const baseBackupRestoreProgressAtom = atom({ status: BackupProgressStatus.Idle, }); +export const backupRestoreErrorAtom = atom(undefined); + +export const isMissingBackupKeyError = (error: unknown): boolean => { + if (error === undefined || error === null) return false; + const message = error instanceof Error ? error.message : typeof error === 'string' ? error : ''; + return message.toLowerCase().includes('no decryption key found'); +}; + export const backupRestoreProgressAtom = atom< IBackupProgress, [ImportRoomKeyProgressData], From d5492982a142b3138e8b671b0dbaa15e7e0b36e2 Mon Sep 17 00:00:00 2001 From: Erwan Leboucher Date: Sun, 26 Jul 2026 16:30:56 +0200 Subject: [PATCH 2/3] fix(crypto): reset per-account crypto state when switching accounts --- src/app/pages/client/ClientRoot.tsx | 10 ++++- src/client/initMatrix.ts | 3 +- src/client/secretStorageKeys.test.ts | 58 ++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 3 deletions(-) create mode 100644 src/client/secretStorageKeys.test.ts diff --git a/src/app/pages/client/ClientRoot.tsx b/src/app/pages/client/ClientRoot.tsx index 25301c7aef..df9dd20894 100644 --- a/src/app/pages/client/ClientRoot.tsx +++ b/src/app/pages/client/ClientRoot.tsx @@ -27,6 +27,7 @@ import { startClient, stopClient, } from '$client/initMatrix'; +import { clearSecretStorageKeys } from '$client/secretStorageKeys'; import { SplashScreen } from '$components/splash-screen'; import { ServerConfigsLoader } from '$components/ServerConfigsLoader'; import { CapabilitiesProvider } from '$hooks/useCapabilities'; @@ -309,9 +310,14 @@ export function ClientRoot({ children }: ClientRootProps) { '— reloading client' ); void pushSessionToSW(activeSession.baseUrl, activeSession.accessToken, activeSession.userId); - if (mx?.clientRunning) { + // Unconditional: stopClient is what stops the crypto backend, and a client + // that never reached clientRunning still holds an open crypto store. + if (mx) { stopClient(mx); } + // The cache is keyed by 4S key id only, so the previous account's key + // would otherwise stay in memory for the next one. + clearSecretStorageKeys(); loadedUserIdRef.current = undefined; setLoadState({ status: AsyncStatus.Idle }); navigate(getHomePath(), { replace: true }); @@ -335,7 +341,7 @@ export function ClientRoot({ children }: ClientRootProps) { useEffect( () => () => { - if (mx?.clientRunning) { + if (mx) { log.log('ClientRoot unmounting — stopping client', mx.getUserId()); stopClient(mx); } diff --git a/src/client/initMatrix.ts b/src/client/initMatrix.ts index b463b31394..144984d83e 100644 --- a/src/client/initMatrix.ts +++ b/src/client/initMatrix.ts @@ -30,7 +30,7 @@ import * as Sentry from '@sentry/react'; import { pushSessionToSW } from '../sw-session'; import { assertAuthMetadataIssuer, createSessionTokenRefresher } from './oidcTokenRefresher'; import { revokeOAuthToken } from './oauthTokenRevocation'; -import { cryptoCallbacks } from './secretStorageKeys'; +import { clearSecretStorageKeys, cryptoCallbacks } from './secretStorageKeys'; import type { SlidingSyncDiagnostics } from './slidingSync'; import { scopeEphemeralExtensions, SlidingSyncManager } from './slidingSync'; import { PresenceSyncManager } from './presenceSync'; @@ -610,6 +610,7 @@ export const logoutClient = async (mx: MatrixClient, session?: Session) => { SlidingSyncSidebarCache.clear(session.userId); clearCachedVersions(session.baseUrl, session.userId); clearCachedUserProfiles(session.userId); + clearSecretStorageKeys(); const storeName: SessionStoreName = getSessionStoreName(session); await mx.clearStores({ cryptoDatabasePrefix: storeName.rustCryptoPrefix }); await deleteDatabase(storeName.sync); diff --git a/src/client/secretStorageKeys.test.ts b/src/client/secretStorageKeys.test.ts new file mode 100644 index 0000000000..c19038c764 --- /dev/null +++ b/src/client/secretStorageKeys.test.ts @@ -0,0 +1,58 @@ +import { beforeEach, describe, expect, it } from 'vitest'; +import { clearSecretStorageKeys, cryptoCallbacks, storePrivateKey } from './secretStorageKeys'; + +const KEY_A = 'key-account-a'; +const KEY_B = 'key-account-b'; + +const ask = (...keyIds: string[]) => + cryptoCallbacks.getSecretStorageKey({ + keys: Object.fromEntries(keyIds.map((id) => [id, {}])), + }); + +describe('secretStorageKeys', () => { + beforeEach(() => { + clearSecretStorageKeys(); + }); + + it('returns a stored key for the requested key id', async () => { + storePrivateKey(KEY_A, new Uint8Array([1, 2, 3])); + + const result = await ask(KEY_A); + + expect(result?.[0]).toBe(KEY_A); + expect(result?.[1]).toEqual(new Uint8Array([1, 2, 3])); + }); + + it('does not answer for a key id it has never seen', async () => { + storePrivateKey(KEY_A, new Uint8Array([1])); + + expect(await ask(KEY_B)).toBeUndefined(); + }); + + it('rejects a non-Uint8Array key', () => { + expect(() => storePrivateKey(KEY_A, 'not-a-key' as unknown as Uint8Array)).toThrow( + 'Unable to store, privateKey is invalid.' + ); + }); + + // The cache is module-level and shared by every client in the tab, so an + // account switch has to empty it or the previous account's 4S key lingers. + it('holds nothing after a switch clears it', async () => { + storePrivateKey(KEY_A, new Uint8Array([1])); + storePrivateKey(KEY_B, new Uint8Array([2])); + + clearSecretStorageKeys(); + + expect(await ask(KEY_A)).toBeUndefined(); + expect(await ask(KEY_B)).toBeUndefined(); + }); + + it('lets the next account store its own key after a clear', async () => { + storePrivateKey(KEY_A, new Uint8Array([1])); + clearSecretStorageKeys(); + storePrivateKey(KEY_B, new Uint8Array([9])); + + expect((await ask(KEY_B))?.[1]).toEqual(new Uint8Array([9])); + expect(await ask(KEY_A)).toBeUndefined(); + }); +}); From d5d7c8b290f5d71a002516621ea9d573fd3f48ee Mon Sep 17 00:00:00 2001 From: Erwan Leboucher Date: Sun, 26 Jul 2026 17:11:18 +0200 Subject: [PATCH 3/3] fix(crypto): base backup recovery on a structural signal and reset restore state per account --- src/app/components/BackupRestore.tsx | 11 +-- src/app/hooks/useKeyBackup.test.tsx | 77 ++++++++++++------- src/app/hooks/useKeyBackup.ts | 40 ++++++---- .../hooks/useRestoreBackupOnVerification.ts | 13 +++- src/app/pages/client/ClientRoot.tsx | 7 +- src/app/state/backupRestore.ts | 7 ++ 6 files changed, 104 insertions(+), 51 deletions(-) diff --git a/src/app/components/BackupRestore.tsx b/src/app/components/BackupRestore.tsx index b668329220..5152779428 100644 --- a/src/app/components/BackupRestore.tsx +++ b/src/app/components/BackupRestore.tsx @@ -33,7 +33,7 @@ import { useKeyBackupStatus, useKeyBackupSync, useKeyBackupTrust, - useSessionBackupKeyCached, + useSessionBackupKeyUsable, } from '$hooks/useKeyBackup'; import { SecretStorageKeyMethod, SecretStorageKeyPrompt } from './SecretStorage'; import { stopPropagation } from '$utils/keyboard'; @@ -233,7 +233,7 @@ export function BackupRestoreTile({ const backupEnabled = useKeyBackupStatus(crypto); const backupInfo = useKeyBackupInfo(crypto); - const backupKeyCached = useSessionBackupKeyCached(crypto); + const backupKeyUsable = useSessionBackupKeyUsable(crypto); const [remainingSession, syncFailure] = useKeyBackupSync(); const [menuCords, setMenuCords] = useState(); @@ -257,11 +257,12 @@ export function BackupRestoreTile({ restoreBackup(); }; + // backupKeyUsable is the structural signal; the error match only covers a + // restore that failed for this reason before the lookup settled. const needsBackupKey = !!backupInfo && - (backupKeyCached === false || - (restoreState.status === AsyncStatus.Error && isMissingBackupKeyError(restoreState.error)) || - isMissingBackupKeyError(autoRestoreError)); + (backupKeyUsable === false || + (restoreState.status === AsyncStatus.Error && isMissingBackupKeyError(restoreState.error))); return ( void>>(); @@ -11,52 +11,75 @@ vi.mock('$hooks/useMatrixClient', () => ({ useMatrixClient: () => emitter, })); -const createCrypto = (getSessionBackupPrivateKey: CryptoApi['getSessionBackupPrivateKey']) => - ({ getSessionBackupPrivateKey }) as unknown as CryptoApi; - -describe('useSessionBackupKeyCached', () => { +type CryptoParts = { + key?: CryptoApi['getSessionBackupPrivateKey']; + version?: CryptoApi['getActiveSessionBackupVersion']; +}; + +const createCrypto = ({ key, version }: CryptoParts) => + ({ + getSessionBackupPrivateKey: + key ?? + vi.fn().mockResolvedValue(new Uint8Array([1, 2, 3])), + getActiveSessionBackupVersion: + version ?? vi.fn().mockResolvedValue('3'), + }) as unknown as CryptoApi; + +describe('useSessionBackupKeyUsable', () => { beforeEach(() => { emitter.removeAllListeners(); }); - it('reports true when the key is in the crypto store', async () => { - const crypto = createCrypto( - vi.fn().mockResolvedValue(new Uint8Array([1, 2, 3])) - ); - - const { result } = renderHook(() => useSessionBackupKeyCached(crypto)); + it('reports true when both the key and the backup version are in the store', async () => { + const { result } = renderHook(() => useSessionBackupKeyUsable(createCrypto({}))); await waitFor(() => expect(result.current).toBe(true)); }); it('reports false when the store has no key', async () => { - const crypto = createCrypto( - vi.fn().mockResolvedValue(null) - ); + const crypto = createCrypto({ + key: vi.fn().mockResolvedValue(null), + }); - const { result } = renderHook(() => useSessionBackupKeyCached(crypto)); + const { result } = renderHook(() => useSessionBackupKeyUsable(crypto)); await waitFor(() => expect(result.current).toBe(false)); }); - it('reports false rather than hanging when the lookup rejects', async () => { - const crypto = createCrypto( - vi.fn().mockRejectedValue(new Error('store closed')) - ); + // restoreKeyBackup raises "No decryption key found in crypto store" for a + // missing version too, which getSessionBackupPrivateKey alone cannot see. + it('reports false when the key is present but the backup version is not', async () => { + const crypto = createCrypto({ + version: vi.fn().mockResolvedValue(null), + }); - const { result } = renderHook(() => useSessionBackupKeyCached(crypto)); + const { result } = renderHook(() => useSessionBackupKeyUsable(crypto)); await waitFor(() => expect(result.current).toBe(false)); }); - it('re-checks once the key gets cached', async () => { - const getKey = vi - .fn() - .mockResolvedValueOnce(null) - .mockResolvedValue(new Uint8Array([1])); - const crypto = createCrypto(getKey); + it('stays unknown rather than false when the lookup rejects', async () => { + const crypto = createCrypto({ + key: vi + .fn() + .mockRejectedValue(new Error('store closed')), + }); + + const { result } = renderHook(() => useSessionBackupKeyUsable(crypto)); - const { result } = renderHook(() => useSessionBackupKeyCached(crypto)); + await waitFor(() => expect(crypto.getSessionBackupPrivateKey).toHaveBeenCalled()); + expect(result.current).toBeUndefined(); + }); + + it('re-checks once the key gets cached', async () => { + const crypto = createCrypto({ + key: vi + .fn() + .mockResolvedValueOnce(null) + .mockResolvedValue(new Uint8Array([1])), + }); + + const { result } = renderHook(() => useSessionBackupKeyUsable(crypto)); await waitFor(() => expect(result.current).toBe(false)); emitter.emit(CryptoEvent.KeyBackupDecryptionKeyCached as never); diff --git a/src/app/hooks/useKeyBackup.ts b/src/app/hooks/useKeyBackup.ts index 4949d5306b..cd2fedfa94 100644 --- a/src/app/hooks/useKeyBackup.ts +++ b/src/app/hooks/useKeyBackup.ts @@ -5,7 +5,7 @@ import type { KeyBackupInfo, } from '$types/matrix-sdk'; import { CryptoEvent } from '$types/matrix-sdk'; -import { useCallback, useEffect, useState } from 'react'; +import { useCallback, useEffect, useRef, useState } from 'react'; import * as Sentry from '@sentry/react'; import { useMatrixClient } from './useMatrixClient'; import { useMatrixEvent } from './useMatrixEvent'; @@ -56,29 +56,39 @@ export const useKeyBackupDecryptionKeyCached = ( useMatrixEvent(mx, CryptoEvent.KeyBackupDecryptionKeyCached, onChange); }; -/** undefined while the first lookup is in flight. */ -export const useSessionBackupKeyCached = (crypto: CryptoApi): boolean | undefined => { +/** + * Whether this device can actually restore from backup. `restoreKeyBackup` + * requires BOTH a decryption key and a backup version in the store — it raises + * the same "No decryption key found in crypto store" for either being absent, + * while `getSessionBackupPrivateKey` only reports on the key. `undefined` while + * unknown (first lookup in flight, or the lookup failed). + */ +export const useSessionBackupKeyUsable = (crypto: CryptoApi): boolean | undefined => { const alive = useAlive(); - const [cached, setCached] = useState(); - - const fetchCached = useCallback(() => { - crypto - .getSessionBackupPrivateKey() - .then((key) => { - if (alive()) setCached(key !== null); + const [usable, setUsable] = useState(); + const requestRef = useRef(0); + + const fetchUsable = useCallback(() => { + requestRef.current += 1; + const request = requestRef.current; + Promise.all([crypto.getSessionBackupPrivateKey(), crypto.getActiveSessionBackupVersion()]) + .then(([key, version]) => { + // A later lookup already answered; this one is stale. + if (alive() && request === requestRef.current) setUsable(key !== null && version !== null); }) .catch(() => { - if (alive()) setCached(false); + if (alive() && request === requestRef.current) setUsable(undefined); }); }, [crypto, alive]); useEffect(() => { - fetchCached(); - }, [fetchCached]); + fetchUsable(); + }, [fetchUsable]); - useKeyBackupDecryptionKeyCached(fetchCached); + useKeyBackupStatusChange(fetchUsable); + useKeyBackupDecryptionKeyCached(fetchUsable); - return cached; + return usable; }; export const useKeyBackupSync = (): [number, string | undefined] => { diff --git a/src/app/hooks/useRestoreBackupOnVerification.ts b/src/app/hooks/useRestoreBackupOnVerification.ts index a2b3f5c6e8..5fc30b2ac1 100644 --- a/src/app/hooks/useRestoreBackupOnVerification.ts +++ b/src/app/hooks/useRestoreBackupOnVerification.ts @@ -1,13 +1,18 @@ import { useSetAtom } from 'jotai'; import { useCallback } from 'react'; import * as Sentry from '@sentry/react'; -import { backupRestoreErrorAtom, backupRestoreProgressAtom } from '$state/backupRestore'; +import { + backupRestoreErrorAtom, + backupRestoreProgressAtom, + resetBackupRestoreAtom, +} from '$state/backupRestore'; import { useMatrixClient } from './useMatrixClient'; import { useKeyBackupDecryptionKeyCached } from './useKeyBackup'; export const useRestoreBackupOnVerification = () => { const setRestoreProgress = useSetAtom(backupRestoreProgressAtom); const setRestoreError = useSetAtom(backupRestoreErrorAtom); + const resetRestore = useSetAtom(resetBackupRestoreAtom); const mx = useMatrixClient(); @@ -16,7 +21,7 @@ export const useRestoreBackupOnVerification = () => { const crypto = mx.getCrypto(); if (!crypto) return; - setRestoreError(undefined); + resetRestore(); crypto .restoreKeyBackup({ progressCallback(progress) { @@ -32,8 +37,10 @@ export const useRestoreBackupOnVerification = () => { data: { error: message }, }); Sentry.metrics.count('sable.crypto.key_backup_restore_failures', 1); + // Progress stays at Fetching if the import threw partway. + resetRestore(); setRestoreError(message); }); - }, [mx, setRestoreProgress, setRestoreError]) + }, [mx, setRestoreProgress, setRestoreError, resetRestore]) ); }; diff --git a/src/app/pages/client/ClientRoot.tsx b/src/app/pages/client/ClientRoot.tsx index df9dd20894..2bbbec9b43 100644 --- a/src/app/pages/client/ClientRoot.tsx +++ b/src/app/pages/client/ClientRoot.tsx @@ -28,6 +28,7 @@ import { stopClient, } from '$client/initMatrix'; import { clearSecretStorageKeys } from '$client/secretStorageKeys'; +import { resetBackupRestoreAtom } from '$state/backupRestore'; import { SplashScreen } from '$components/splash-screen'; import { ServerConfigsLoader } from '$components/ServerConfigsLoader'; import { CapabilitiesProvider } from '$hooks/useCapabilities'; @@ -246,6 +247,7 @@ export function ClientRoot({ children }: ClientRootProps) { const sessions = useAtomValue(sessionsAtom); const [activeSessionId, setActiveSessionId] = useAtom(activeSessionIdAtom); const setSessions = useSetAtom(sessionsAtom); + const resetBackupRestore = useSetAtom(resetBackupRestoreAtom); const activeSession: Session | undefined = sessions.find((s) => s.userId === activeSessionId) ?? sessions[0]; @@ -318,11 +320,14 @@ export function ClientRoot({ children }: ClientRootProps) { // The cache is keyed by 4S key id only, so the previous account's key // would otherwise stay in memory for the next one. clearSecretStorageKeys(); + // Jotai atoms live in the default store for the tab's lifetime, so the + // previous account's restore state would be read as this one's. + resetBackupRestore(); loadedUserIdRef.current = undefined; setLoadState({ status: AsyncStatus.Idle }); navigate(getHomePath(), { replace: true }); } - }, [activeSession, mx, navigate, setLoadState]); + }, [activeSession, mx, navigate, setLoadState, resetBackupRestore]); const handleLogout = useCallback(async () => { if (!mx || !activeSession) return; diff --git a/src/app/state/backupRestore.ts b/src/app/state/backupRestore.ts index e8bce791d6..720e92c97d 100644 --- a/src/app/state/backupRestore.ts +++ b/src/app/state/backupRestore.ts @@ -35,6 +35,13 @@ const baseBackupRestoreProgressAtom = atom({ export const backupRestoreErrorAtom = atom(undefined); +// Progress sticks at Fetching if a restore throws after it starts importing, and +// both atoms outlive an account switch since there is no jotai Provider. +export const resetBackupRestoreAtom = atom(null, (_get, set) => { + set(baseBackupRestoreProgressAtom, { status: BackupProgressStatus.Idle }); + set(backupRestoreErrorAtom, undefined); +}); + export const isMissingBackupKeyError = (error: unknown): boolean => { if (error === undefined || error === null) return false; const message = error instanceof Error ? error.message : typeof error === 'string' ? error : '';