From 2af67d05ac2f86d72129a7f73ce62a473df85f90 Mon Sep 17 00:00:00 2001 From: TurtleWolfe Date: Mon, 4 May 2026 15:15:50 +0000 Subject: [PATCH] =?UTF-8?q?fix(messaging):=20batch=208=20=E2=80=94=20non-e?= =?UTF-8?q?xtractable=20in-memory=20ECDH=20key,=20polling=20cache,=20stale?= =?UTF-8?q?=20cursor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security: KeyDerivationService.importPrivateKey now imports the in-memory private key with extractable=false and returns the public-only JWK alongside (computed via noble-curves at the same site). Closes the XSS exfiltration path where a script with a handle to derivedKeys.privateKey could call exportKey('jwk', ...) and exfiltrate raw key material. Batch 7c had already made the IndexedDB-stored copy non-extractable; this completes the picture for the in-memory copy. The asNonExtractablePrivate helper in key-service.ts is removed — it was a workaround for the previously-extractable in-memory key, now redundant. EncryptionService.generateKeyPair is marked @deprecated. It stays extractable because unit-test fixtures need to round-trip JWK to assert non-extractable storage behavior. Production paths use KeyDerivationService.deriveKeyPair exclusively. Perf: getMessageHistory reuses the module-level sharedSecretCache across polling ticks. Polling fires every 10s; ECDH derivation per tick was wasted work since the counterparty key doesn't change between polls. Cache invalidates when the sender's CryptoKey reference changes (post-rotation / post-re-derive). UX: ConversationView pagination uses cursorRef to read the latest cursor without forcing a useCallback rebuild. The previous closure captured cursor=null at hook construction, so every \"load more\" click silently re-fetched page 1. Cleanup: console.{log,error} → logger.{debug,error} in production paths. payment-service.ts intent type narrowed from \`as any\`. Test update: key-derivation.test.ts:109 asserted extractable=true; inverted to extractable=false (now the security contract) and renamed to 'should produce non-extractable private key for XSS resistance'. Verification: - pnpm run type-check: clean - pnpm run lint: clean - pnpm test: 3237/3237 pass Refs: batch 7c (5ba2323), code-review on 6407372..a62df98 Co-Authored-By: Claude Opus 4.7 (1M context) --- .../ConversationView/ConversationView.tsx | 35 ++++--- .../__tests__/key-derivation.test.ts | 10 +- src/lib/messaging/encryption.ts | 16 +++- src/lib/messaging/key-derivation.ts | 56 +++++++---- src/lib/payments/payment-service.ts | 2 +- src/services/messaging/key-service.ts | 96 +++++++++---------- src/services/messaging/message-service.ts | 86 ++++++++++------- 7 files changed, 173 insertions(+), 128 deletions(-) diff --git a/src/components/organisms/ConversationView/ConversationView.tsx b/src/components/organisms/ConversationView/ConversationView.tsx index fbc1711a..4c699295 100644 --- a/src/components/organisms/ConversationView/ConversationView.tsx +++ b/src/components/organisms/ConversationView/ConversationView.tsx @@ -1,6 +1,6 @@ 'use client'; -import React, { useState, useEffect, useCallback } from 'react'; +import React, { useState, useEffect, useCallback, useRef } from 'react'; import ChatWindow from '@/components/organisms/ChatWindow'; import ErrorBoundary from '@/components/ErrorBoundary'; import { @@ -54,6 +54,12 @@ export default function ConversationView({ const [sending, setSending] = useState(false); const [hasMore, setHasMore] = useState(false); const [cursor, setCursor] = useState(null); + // Mirror cursor into a ref so loadMessages can read the latest value + // without depending on `cursor` (which would trigger a reload on every + // cursor change). loadMore=true paths must read cursorRef.current to + // avoid the stale-closure bug where pagination always re-fetched from + // the start because the useCallback closure captured cursor=null. + const cursorRef = useRef(null); const [error, setError] = useState(null); const [participantName, setParticipantName] = useState('Unknown User'); @@ -137,7 +143,7 @@ export default function ConversationView({ const result = await messageService.getMessageHistory( conversationId, - loadMore ? cursor : null, + loadMore ? cursorRef.current : null, 50 ); @@ -187,6 +193,7 @@ export default function ConversationView({ setHasMore(result.has_more); setCursor(result.cursor); + cursorRef.current = result.cursor; } catch (err: unknown) { setError( err instanceof Error ? err.message : 'Failed to load messages' @@ -195,10 +202,10 @@ export default function ConversationView({ setLoading(false); } }, - // cursor is read fresh via closure for the loadMore=true path but we - // deliberately don't depend on it — a cursor change alone shouldn't - // trigger a reload, only a conversationId change should. - // eslint-disable-next-line react-hooks/exhaustive-deps + // cursor is read via cursorRef.current — depending on the cursor state + // would trigger a reload on every page-load (cursor changes after each + // call), so we use a ref to read the latest value without invalidating + // the callback identity. [conversationId] ); @@ -248,16 +255,14 @@ export default function ConversationView({ if (result.queued) { // Offline OR send-failed-and-queued. Show the optimistic bubble. - console.log( - '[ConversationView] message queued (offline/retry):', - result.message.id - ); + logger.debug('message queued (offline/retry)', { + id: result.message.id, + }); addPending(result.message.id, content); } else { - console.log( - '[ConversationView] message sent, appending optimistic:', - result.message.id - ); + logger.debug('message sent, appending optimistic', { + id: result.message.id, + }); // Optimistically append the sent message so it appears immediately. // Supabase free tier can have read-after-write latency — an immediate // loadMessages() query may return stale results (empty if first message). @@ -285,7 +290,7 @@ export default function ConversationView({ err instanceof Error ? err.message : 'Failed to send message. Please try again.'; - console.error('[ConversationView] sendMessage failed:', msg, err); + logger.error('sendMessage failed', { message: msg, error: err }); setError(msg); } finally { setSending(false); diff --git a/src/lib/messaging/__tests__/key-derivation.test.ts b/src/lib/messaging/__tests__/key-derivation.test.ts index 3e52e83b..669dd8c6 100644 --- a/src/lib/messaging/__tests__/key-derivation.test.ts +++ b/src/lib/messaging/__tests__/key-derivation.test.ts @@ -106,13 +106,19 @@ describe('KeyDerivationService', () => { expect(keyPair.publicKeyJwk.y).toBeDefined(); }); - it('should produce extractable private key for operations', async () => { + it('should produce non-extractable private key for XSS resistance', async () => { const password = 'TestPassword123!'; const salt = service.generateSalt(); const keyPair = await service.deriveKeyPair({ password, salt }); - expect(keyPair.privateKey.extractable).toBe(true); + // Batch 8 security contract: KeyDerivationService.importPrivateKey + // imports the in-memory ECDH private key with extractable=false so a + // script holding a reference to derivedKeys.privateKey cannot + // exfiltrate raw key material via exportKey('jwk', ...). Operations + // (deriveKey/deriveBits for ECDH) still work because they don't + // require extractability. + expect(keyPair.privateKey.extractable).toBe(false); }); it('should return base64-encoded salt in result', async () => { diff --git a/src/lib/messaging/encryption.ts b/src/lib/messaging/encryption.ts index eb6e8955..dd3d98c3 100644 --- a/src/lib/messaging/encryption.ts +++ b/src/lib/messaging/encryption.ts @@ -26,10 +26,18 @@ const logger = createLogger('messaging:encryption'); export class EncryptionService { /** - * Generate an ECDH P-256 key pair for asymmetric encryption - * Task: T046 + * Generate an ECDH P-256 key pair for asymmetric encryption. * - * @returns Promise - Public and private CryptoKey objects + * @deprecated Production messaging derives keys via `KeyDerivationService` + * (Argon2id from password). This method exists ONLY for unit-test fixtures + * that need a quick extractable keypair to round-trip JWK and assert + * non-extractable storage behavior. Do NOT call from production code: + * the returned private key is extractable, so XSS reading a reference to + * it can call exportKey('jwk', ...) and exfiltrate raw key material. If + * you need a key pair in production, use KeyDerivationService.deriveKeyPair + * which returns a non-extractable private key. + * + * @returns Promise - Public and private CryptoKey objects (extractable) * @throws EncryptionError if Web Crypto API is unavailable */ async generateKeyPair(): Promise { @@ -43,7 +51,7 @@ export class EncryptionService { name: CRYPTO_PARAMS.ALGORITHM, namedCurve: CRYPTO_PARAMS.CURVE, }, - true, // extractable + true, // extractable — see @deprecated note above ['deriveBits', 'deriveKey'] ); diff --git a/src/lib/messaging/key-derivation.ts b/src/lib/messaging/key-derivation.ts index 3b0cbd9b..e13a9a56 100644 --- a/src/lib/messaging/key-derivation.ts +++ b/src/lib/messaging/key-derivation.ts @@ -116,21 +116,16 @@ export class KeyDerivationService { // Step 2: Reduce seed to valid P-256 scalar const privateKeyScalar = reduceScalar(seed); - // Step 3: Import as ECDH private key - const privateKey = await this.importPrivateKey(privateKeyScalar); + // Step 3: Import as non-extractable ECDH private key. importPrivateKey + // also returns the public-only JWK (computed from the same private + // scalar via noble-curves), so we do NOT need to exportKey() the + // private key — that would require importing extractable, which + // would let XSS exfiltrate raw key material. + const { privateKey, publicKeyJwk } = + await this.importPrivateKey(privateKeyScalar); - // Step 4: Export and reimport to get public key - const privateKeyJwk = await crypto.subtle.exportKey('jwk', privateKey); - - // Create public-only JWK (remove d parameter) - const publicKeyJwk: JsonWebKey = { - kty: privateKeyJwk.kty, - crv: privateKeyJwk.crv, - x: privateKeyJwk.x, - y: privateKeyJwk.y, - }; - - // Import public key + // Step 4: Import the public-only JWK as a separate CryptoKey + // (extractable=true is fine here; public keys aren't sensitive). const publicKey = await crypto.subtle.importKey( 'jwk', publicKeyJwk, @@ -220,13 +215,18 @@ export class KeyDerivationService { } /** - * Import raw private key bytes as CryptoKey - * Uses JWK format for better browser compatibility + * Import raw private key bytes as CryptoKey + computed public-key JWK. + * Uses JWK format for better browser compatibility. + * + * Returns the public-only JWK (x, y, no d) alongside the imported private + * CryptoKey so the caller doesn't need to round-trip the private key + * through exportKey() — that would require importing it as extractable + * (which defeats the XSS-resistance guarantee on the in-memory copy). * @private */ private async importPrivateKey( privateKeyBytes: Uint8Array - ): Promise { + ): Promise<{ privateKey: CryptoKey; publicKeyJwk: JsonWebKey }> { try { // Always use the JWK path with noble-curves to compute the public key. // The PKCS#8 path used to be the primary, but Firefox/WebKit reject the @@ -257,17 +257,33 @@ export class KeyDerivationService { y, }; - // Import the JWK as a CryptoKey (works in all browsers) - return await crypto.subtle.importKey( + // Import the JWK as a NON-EXTRACTABLE CryptoKey. + // The in-memory private key only ever needs deriveKey/deriveBits for + // ECDH; we never need exportKey() on it. Importing non-extractable + // closes the XSS exfiltration path: a script with a handle to + // keyManagementService.derivedKeys.privateKey can still USE the key + // for ECDH from the same origin, but cannot extract raw key material + // for offline decryption of stolen ciphertext. + const privateKey = await crypto.subtle.importKey( 'jwk', jwk, { name: CRYPTO_PARAMS.ALGORITHM, namedCurve: CRYPTO_PARAMS.CURVE, }, - true, + false, ['deriveKey', 'deriveBits'] ); + + // Public-only JWK (no `d`) for the caller to attach to keyPair.publicKeyJwk. + const publicKeyJwk: JsonWebKey = { + kty: 'EC', + crv: CRYPTO_PARAMS.CURVE, + x, + y, + }; + + return { privateKey, publicKeyJwk }; } catch (error) { if (error instanceof KeyDerivationError) { throw error; diff --git a/src/lib/payments/payment-service.ts b/src/lib/payments/payment-service.ts index ea426dc2..5b5c5ab0 100644 --- a/src/lib/payments/payment-service.ts +++ b/src/lib/payments/payment-service.ts @@ -294,7 +294,7 @@ export async function getPaymentHistory( status: item.status as PaymentActivity['status'], charged_amount: item.charged_amount ?? 0, charged_currency: item.charged_currency as Currency, - customer_email: (item.intent as any).customer_email, + customer_email: (item.intent as { customer_email: string }).customer_email, webhook_verified: item.webhook_verified, created_at: item.created_at, })); diff --git a/src/services/messaging/key-service.ts b/src/services/messaging/key-service.ts index 26b07ef4..daa67f2f 100644 --- a/src/services/messaging/key-service.ts +++ b/src/services/messaging/key-service.ts @@ -37,23 +37,12 @@ import { const logger = createLogger('messaging:keys'); -/** - * Re-import an extractable ECDH CryptoKey as a non-extractable copy suitable - * for IndexedDB storage. XSS reading the row gets a handle but cannot export - * raw key material. - */ -async function asNonExtractablePrivate( - extractable: CryptoKey -): Promise { - const jwk = await crypto.subtle.exportKey('jwk', extractable); - return crypto.subtle.importKey( - 'jwk', - jwk, - { name: 'ECDH', namedCurve: 'P-256' }, - false, - ['deriveBits', 'deriveKey'] - ); -} +// Note: previously this module had an asNonExtractablePrivate() helper that +// re-imported the in-memory private CryptoKey as non-extractable before +// writing it to IndexedDB. That step was a workaround — KeyDerivationService +// now imports the in-memory private key as non-extractable from the start +// (see src/lib/messaging/key-derivation.ts importPrivateKey). The keyPair +// object can be passed directly to encryptionService.storePrivateKey(). export class KeyManagementService { /** In-memory storage for derived keys (cleared on logout) */ @@ -131,16 +120,16 @@ export class KeyManagementService { } // Step 4: Store keypair in memory + IndexedDB. - // The IndexedDB copy is a NON-EXTRACTABLE re-import — XSS reading the - // row cannot exportKey() the raw material. The in-memory copy stays - // extractable so the public half can still be exported on demand - // (e.g. for verification fingerprints). + // The private key is non-extractable both in memory (per + // KeyDerivationService.importPrivateKey) and in IndexedDB. The public + // CryptoKey is extractable but that's fine — public keys aren't + // sensitive, and verifyPublicKey works on the JWK form + // (keyPair.publicKeyJwk) without needing exportKey on the CryptoKey. this.derivedKeys = keyPair; try { - const nonExtractablePrivate = await asNonExtractablePrivate( - keyPair.privateKey - ); - await encryptionService.storePrivateKey(user.id, nonExtractablePrivate); + // keyPair.privateKey is already non-extractable (see + // KeyDerivationService.importPrivateKey). + await encryptionService.storePrivateKey(user.id, keyPair.privateKey); } catch (err) { logger.warn('Could not populate IndexedDB after initializeKeys()', { error: err, @@ -237,9 +226,14 @@ export class KeyManagementService { const storedKey = data.public_key as unknown as JsonWebKey; const derivedFingerprint = keyPair.publicKeyJwk?.x?.slice(0, 8) ?? 'null'; const storedFingerprint = storedKey?.x?.slice(0, 8) ?? 'null'; - console.log( - `[deriveKeys] userId=${user.id.slice(0, 8)} derived=${derivedFingerprint} stored=${storedFingerprint} match=${derivedFingerprint === storedFingerprint}` - ); + // logger.debug is suppressed in production; safe for diagnostic output + // that includes user-id prefixes + public-key fingerprints. + logger.debug('deriveKeys fingerprint compare', { + userIdPrefix: user.id.slice(0, 8), + derived: derivedFingerprint, + stored: storedFingerprint, + match: derivedFingerprint === storedFingerprint, + }); const isMatch = this.keyDerivationService.verifyPublicKey( keyPair.publicKeyJwk, @@ -253,10 +247,9 @@ export class KeyManagementService { // Step 4: Store keypair in memory + IndexedDB (non-extractable copy). this.derivedKeys = keyPair; try { - const nonExtractablePrivate = await asNonExtractablePrivate( - keyPair.privateKey - ); - await encryptionService.storePrivateKey(user.id, nonExtractablePrivate); + // keyPair.privateKey is already non-extractable (see + // KeyDerivationService.importPrivateKey). + await encryptionService.storePrivateKey(user.id, keyPair.privateKey); } catch (err) { logger.warn('Could not populate IndexedDB after deriveKeys()', { error: err, @@ -340,9 +333,11 @@ export class KeyManagementService { salt: data.encryption_salt, }; const fingerprint = publicKeyJwk?.x?.slice(0, 8) ?? 'null'; - console.log( - `[restoreKeysFromCache] userId=${currentUserId.slice(0, 8)} pubKey=${fingerprint} source=IndexedDB` - ); + logger.debug('restoreKeysFromCache', { + userIdPrefix: currentUserId.slice(0, 8), + pubKey: fingerprint, + source: 'IndexedDB', + }); logger.info('Restored keys from IndexedDB', { userId: currentUserId }); return true; } @@ -492,9 +487,6 @@ export class KeyManagementService { authError: authError?.message, hasUser: !!user, }); - console.warn( - `[hasKeys] auth failed: error=${authError?.message || 'none'}, user=${!!user}` - ); return false; } @@ -522,9 +514,6 @@ export class KeyManagementService { hasData: data !== null, errorCode: error?.code, }); - console.warn( - `[hasKeys] query: userId=${user.id.slice(0, 8)}, hasData=${data !== null}, err=${error?.code || 'none'}` - ); return data !== null; } catch (error) { if (error instanceof ConnectionError) { @@ -653,10 +642,9 @@ export class KeyManagementService { // Update in-memory keys + IndexedDB (non-extractable copy). this.derivedKeys = keyPair; try { - const nonExtractablePrivate = await asNonExtractablePrivate( - keyPair.privateKey - ); - await encryptionService.storePrivateKey(user.id, nonExtractablePrivate); + // keyPair.privateKey is already non-extractable (see + // KeyDerivationService.importPrivateKey). + await encryptionService.storePrivateKey(user.id, keyPair.privateKey); } catch (err) { logger.warn('Could not populate IndexedDB after rotateKeys()', { error: err, @@ -746,9 +734,10 @@ export class KeyManagementService { const cached = this.publicKeyCache.get(userId); if (cached) { const fingerprint = cached?.x?.slice(0, 8) ?? 'null'; - console.log( - `[getUserPublicKey] userId=${userId.slice(0, 8)} key=${fingerprint} source=offline-cache` - ); + logger.debug('getUserPublicKey from offline cache', { + userIdPrefix: userId.slice(0, 8), + key: fingerprint, + }); return cached; } } @@ -780,15 +769,18 @@ export class KeyManagementService { } const publicKey = (data?.public_key as unknown as JsonWebKey) ?? null; - // Log key fingerprint for E2E debugging (x-coordinate of ECDH public key) + // Log key fingerprint at debug level for E2E diagnostics; suppressed + // in production builds. const fingerprint = publicKey?.x?.slice(0, 8) ?? 'null'; const source = typeof navigator !== 'undefined' && !navigator.onLine ? 'offline-cache' : 'db-fresh'; - console.log( - `[getUserPublicKey] userId=${userId.slice(0, 8)} key=${fingerprint} source=${source}` - ); + logger.debug('getUserPublicKey result', { + userIdPrefix: userId.slice(0, 8), + key: fingerprint, + source, + }); if (publicKey) { this.publicKeyCache.set(userId, publicKey); } diff --git a/src/services/messaging/message-service.ts b/src/services/messaging/message-service.ts index 9990d385..65e17a06 100644 --- a/src/services/messaging/message-service.ts +++ b/src/services/messaging/message-service.ts @@ -178,7 +178,8 @@ export class MessageService { const result = await supabase.auth.getSession(); session = result.data?.session; authError = result.error; - console.log(`[sendMessage] getSession attempt ${attempt + 1}/3:`, { + logger.debug('sendMessage getSession attempt', { + attempt: attempt + 1, hasSession: !!session, hasUser: !!session?.user, hasToken: !!session?.access_token, @@ -191,15 +192,15 @@ export class MessageService { const user = session?.user; if (authError || !user) { - // Log localStorage keys for debugging + // Surface localStorage auth-key state at debug level for diagnostics + // when the retry budget is exhausted. Never includes token values. if (typeof window !== 'undefined') { const authKeys = Object.keys(localStorage).filter( (k) => k.includes('auth') || k.includes('sb-') ); - console.error( - '[sendMessage] AUTH FAILED. localStorage auth keys:', - authKeys - ); + logger.error('sendMessage AUTH FAILED', { + localStorageAuthKeys: authKeys, + }); } throw new AuthenticationError('You must be signed in to send messages'); } @@ -300,9 +301,11 @@ export class MessageService { sharedSecretCache.set(recipientId, sharedSecret); } - console.log( - `[sendMessage] sharedSecret source=${sharedSecretSource} recipientId=${recipientId.slice(0, 8)} online=${navigator.onLine}` - ); + logger.debug('sendMessage sharedSecret', { + source: sharedSecretSource, + recipientIdPrefix: recipientId.slice(0, 8), + online: navigator.onLine, + }); // Encrypt message content const encrypted = await encryptionService.encryptMessage( @@ -768,30 +771,46 @@ export class MessageService { }; } - logger.debug('Importing other user public key via crypto.subtle'); - const otherPublicKeyCrypto = await crypto.subtle.importKey( - 'jwk', - otherPublicKey, - { - name: 'ECDH', - namedCurve: 'P-256', - }, - false, - [] - ); - logger.debug('Other user public key imported successfully'); + // Reuse the module-level sharedSecretCache so a polling loop or a + // refresh of the same conversation doesn't re-run ECDH derivation + // every call. ECDH is computationally cheap individually but compounds + // at ~6/min under polling. Invalidate the cache when the sender's + // private key identity changes (e.g. after key rotation / re-derive). + if (cachedSenderPrivateKey !== currentKeys.privateKey) { + sharedSecretCache.clear(); + cachedSenderPrivateKey = currentKeys.privateKey; + } + let sharedSecret = sharedSecretCache.get(otherParticipantId) ?? null; + if (!sharedSecret) { + logger.debug('Importing other user public key via crypto.subtle'); + const otherPublicKeyCrypto = await crypto.subtle.importKey( + 'jwk', + otherPublicKey, + { + name: 'ECDH', + namedCurve: 'P-256', + }, + false, + [] + ); + logger.debug('Other user public key imported successfully'); - // Derive shared secret using our derived private key - const sharedSecret = await encryptionService.deriveSharedSecret( - currentKeys.privateKey, - otherPublicKeyCrypto - ); + // Derive shared secret using our derived private key + sharedSecret = await encryptionService.deriveSharedSecret( + currentKeys.privateKey, + otherPublicKeyCrypto + ); + sharedSecretCache.set(otherParticipantId, sharedSecret); + } - // Log key fingerprints for E2E debugging (x-coordinate of ECDH public keys) + // Log key fingerprints for E2E diagnostics (debug-suppressed in prod) const otherKeyFingerprint = otherPublicKey?.x?.slice(0, 8) ?? 'null'; - console.log( - `[getMessageHistory] decrypt: myId=${user.id.slice(0, 8)} otherId=${otherParticipantId.slice(0, 8)} otherPubKey=${otherKeyFingerprint} msgCount=${messagesToDecrypt.length}` - ); + logger.debug('getMessageHistory decrypt', { + myIdPrefix: user.id.slice(0, 8), + otherIdPrefix: otherParticipantId.slice(0, 8), + otherPubKey: otherKeyFingerprint, + msgCount: messagesToDecrypt.length, + }); // Decrypt all messages logger.debug('Decrypting messages', { @@ -829,11 +848,10 @@ export class MessageService { 'Unknown', }; } catch (decryptError) { - // Log the decryption failure with details (but not sensitive data) + // Log the decryption failure with details (but not sensitive data). + // Single logger.error call below is sufficient — the previous + // duplicate console.error was redundant noise in production. const err = decryptError as Error; - console.error( - `[getMessageHistory] DECRYPTION FAILED: msgId=${msg.id.slice(0, 8)} sender=${msg.sender_id.slice(0, 8)} error=${err.message}` - ); logger.error('Decryption FAILED for message', { messageId: msg.id, errorName: String(err.name || 'unknown'),