diff --git a/src/lib/crypto/key-sync-service.js b/src/lib/crypto/key-sync-service.js index a9ef115..2fa1b44 100644 --- a/src/lib/crypto/key-sync-service.js +++ b/src/lib/crypto/key-sync-service.js @@ -128,31 +128,40 @@ export class KeySyncService { return false; } - // Check if key exists in database by trying to fetch it - // This is a simple check - if the API returns our own key, it's synced - const response = await fetch('/api/crypto/public-keys/all', { - method: 'GET', - credentials: 'include' - }); - - if (!response.ok) { - console.log('🔑 Cannot check database keys, assuming sync needed'); - return true; - } - - const data = await response.json(); const currentUserId = this.getCurrentUserId(); - + if (!currentUserId) { console.log('🔑 No current user ID, cannot determine sync status'); return false; } - // Check if our key exists in the database AND matches the local key. - // Presence alone isn't enough: after switching browsers / rotating keys - // the DB still holds the OLD public key, so a presence-only check would - // skip the sync and leave everyone encrypting to a dead key. - const dbKey = data.public_keys && data.public_keys[currentUserId]; + // Ask for our own key specifically rather than scanning the all-keys list. + // + // The list endpoint returns a bare array of {user_id, public_key} keyed by the + // *auth* user id, while getCurrentUserId() returns the internal `users.id`. The + // previous code read `data.public_keys[currentUserId]` off that array, which is + // undefined twice over -- wrong shape and wrong identity domain -- so this check + // reported "not found" on every single login and re-uploaded the local key each + // time. On a browser whose keypair had been regenerated that silently replaced + // the good published key, and every message anyone sent afterwards was encrypted + // to a key the recipient could no longer decrypt with. + // + // This endpoint takes the internal id and resolves it to auth_user_id server-side, + // so there is one identity domain and no list to mis-index. + const response = await fetch( + `/api/crypto/public-keys?user_id=${encodeURIComponent(currentUserId)}`, + { method: 'GET', credentials: 'include' } + ); + + if (!response.ok) { + // Fail closed: re-uploading is the destructive direction, so an unreadable + // answer must not be treated as "the database has nothing". + console.log('🔑 Cannot check database keys, leaving the published key alone'); + return false; + } + + const data = await response.json(); + const dbKey = data?.public_key ?? null; if (!dbKey) { console.log('🔑 Public key not found in database, sync needed'); @@ -169,8 +178,10 @@ export class KeySyncService { } catch (error) { console.error('🔑 Error checking key sync status:', error); - // If we can't check, assume sync is needed to be safe - return true; + // Publishing is the side with consequences -- it replaces the key everyone + // encrypts to. Not knowing the answer is a reason to leave it alone, not to + // overwrite it. + return false; } } diff --git a/src/lib/crypto/key-sync-service.test.js b/src/lib/crypto/key-sync-service.test.js new file mode 100644 index 0000000..12b2efb --- /dev/null +++ b/src/lib/crypto/key-sync-service.test.js @@ -0,0 +1,86 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + getPublicKey: vi.fn(), + initialize: vi.fn() +})); + +vi.mock('./post-quantum-encryption.js', () => ({ + postQuantumEncryption: { + get isInitialized() { + return true; + }, + initialize: mocks.initialize, + getPublicKey: mocks.getPublicKey + } +})); + +const LOCAL_KEY = 'local-public-key-aaaa'; +const INTERNAL_ID = '4826dea7-225a-45df-a56f-6f380bd74ecf'; + +describe('keySyncService.needsKeySync', () => { + let keySyncService; + + beforeEach(async () => { + vi.resetModules(); + vi.clearAllMocks(); + mocks.getPublicKey.mockResolvedValue(LOCAL_KEY); + // tests/setup.js replaces localStorage with a mock that does not actually store, + // so the value has to be handed back through getItem rather than written. + window.localStorage.getItem.mockReturnValue(JSON.stringify({ id: INTERNAL_ID })); + ({ keySyncService } = await import('./key-sync-service.js')); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + /** @param {{ok?: boolean, body?: any}} res */ + function stubFetch(res) { + const fetchMock = vi.fn().mockResolvedValue({ + ok: res.ok ?? true, + json: async () => res.body + }); + vi.stubGlobal('fetch', fetchMock); + return fetchMock; + } + + // The bug: the old code read `data.public_keys[internalId]` off an endpoint that + // returns an array keyed by auth id, so it reported "not found" every login and + // re-published the local key over whatever was already there. + it('does not re-publish when the stored key already matches', async () => { + const fetchMock = stubFetch({ body: { public_key: LOCAL_KEY } }); + + await expect(keySyncService.needsKeySync()).resolves.toBe(false); + + // Asks for its own key by internal id; the server resolves the identity domain. + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(fetchMock.mock.calls[0][0]).toContain(`user_id=${INTERNAL_ID}`); + expect(fetchMock.mock.calls[0][0]).not.toContain('/all'); + }); + + it('syncs when the database holds no key', async () => { + stubFetch({ body: { public_key: null } }); + await expect(keySyncService.needsKeySync()).resolves.toBe(true); + }); + + it('syncs when the stored key belongs to a different keypair', async () => { + stubFetch({ body: { public_key: 'some-other-key-bbbb' } }); + await expect(keySyncService.needsKeySync()).resolves.toBe(true); + }); + + // Publishing replaces the key everyone encrypts to, so an unknown answer must not + // be treated as "the database has nothing". + it('leaves the published key alone when the check cannot be completed', async () => { + stubFetch({ ok: false, body: {} }); + await expect(keySyncService.needsKeySync()).resolves.toBe(false); + + vi.stubGlobal('fetch', vi.fn().mockRejectedValue(new Error('offline'))); + await expect(keySyncService.needsKeySync()).resolves.toBe(false); + }); + + it('does nothing without local keys', async () => { + mocks.getPublicKey.mockResolvedValue(null); + await expect(keySyncService.needsKeySync()).resolves.toBe(false); + }); +});