fix(crypto): stop republishing the public key on every login (#256)

needsKeySync() never found the stored key, so autoSyncOnLogin() re-uploaded the
browser's local public key on every single login.

Two bugs stacked:

- /api/crypto/public-keys/all returns a bare array of {user_id, public_key}, but
  the check read `data.public_keys[currentUserId]` off it. `data.public_keys` is
  undefined on an array, so the lookup never resolved.
- Even with the right shape it would still have missed: user_public_keys.user_id
  holds the auth user id, while getCurrentUserId() returns the internal users.id.
  Same identity domain drift as IA-040, which is what made five RLS policies dead.

The published key is what everyone else encrypts to. Re-uploading it from a
browser whose keypair had been regenerated silently replaced the good key, and
every message sent afterwards was encrypted to a key the recipient could no
longer decrypt -- surfacing as "ChaCha20-Poly1305 decryption failed: invalid tag"
on load, with the odd message decrypting fine because it predated the swap.

Now the service asks for its own key by internal id and lets the server resolve
the identity domain, so there is one id space and no list to mis-index. Both
failure paths also fail closed: publishing is the direction with consequences, so
an unreadable or errored check leaves the stored key alone instead of assuming
the database has nothing.

This stops the ongoing damage. It does not recover history already encrypted to a
lost keypair -- that needs the key-backup restore path, which is written but has
no callers.

505 tests pass; build clean.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

Author: ralyodio

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;
 		}
 	}
 

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);
+	});
+});