From 04e85dfc1d5a688c52662e19e41b7f68675c24e6 Mon Sep 17 00:00:00 2001 From: Codex Date: Tue, 28 Jul 2026 02:04:14 -0600 Subject: [PATCH] fix: preserve encrypted cache during key rotation --- MIGRATION_STATE.md | 14 ++++ docs/architecture.md | 6 ++ src/lib/encryptedPersister.ts | 112 ++++++++++++++++++++++------- src/lib/keyManager.ts | 45 ++++++++++-- tests/keyManager.test.ts | 22 +++++- tests/keyRotationLifecycle.test.ts | 46 +++++------- 6 files changed, 181 insertions(+), 64 deletions(-) diff --git a/MIGRATION_STATE.md b/MIGRATION_STATE.md index 5b2503a..03accc8 100644 --- a/MIGRATION_STATE.md +++ b/MIGRATION_STATE.md @@ -34,6 +34,20 @@ The app uses a **three-layer state architecture** to avoid denormalization and s - **Staleness:** 5-minute staleTime; foreground refetch on app resume - **Sync:** On reconnect, the Sync Engine refetches and overwrites cache with server-authoritative data +#### Encrypted cache key rotation + +React Query persistence is encrypted through `createEncryptedAsyncStoragePersister`. +`KeyManager.initialize()` only ensures a key exists and reports stale keys; it does +not overwrite an existing key by itself. Rotation is coordinated by the encrypted +persister because that layer owns the serialized cache envelope. + +When a key is older than the rotation interval, the persister decrypts the current +`gp1:` envelope with the old key, re-encrypts the same `PersistedClient` under the +new key, writes the rotated envelope back to storage, and only then lets +`KeyManager.rotateKey()` commit the new key. If re-encryption fails or the cache is +unreadable, rotation is deferred and the old key remains active so normal cache +reads are not turned into a silent data-loss event. + ### 2. SQLite (DAL) — Normalized Offline Store - **Owns:** Relational tables for guilds, roles, memberships, user_roles, guild_configs, access_checks diff --git a/docs/architecture.md b/docs/architecture.md index 86c3d23..cb9e3af 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -28,6 +28,12 @@ We use **Expo Router**, which provides file-system based routing similar to Next factory, and cache-coherence mechanisms. This section covers store ownership and the rules contributors need when adding state. +Encrypted React Query persistence owns key-rotation migration. `KeyManager` does +not overwrite an existing stale key during standalone initialization; the encrypted +persister first re-encrypts the stored `gp1:` cache envelope with the candidate key, +then commits that key. If re-encryption cannot complete, rotation is deferred and +the existing cache remains readable. + ### Golden rule Server entity data (guilds, roles, memberships) never goes into Zustand. Zustand holds diff --git a/src/lib/encryptedPersister.ts b/src/lib/encryptedPersister.ts index 81ac586..622b29a 100644 --- a/src/lib/encryptedPersister.ts +++ b/src/lib/encryptedPersister.ts @@ -186,6 +186,7 @@ export function createEncryptedAsyncStoragePersister({ // Set to true if the device-bound key cannot be retrieved; once set we // stop attempting to persist so reads/writes degrade to in-memory only. let memoryOnlyMode = false; + let rotationAttempted = false; async function loadKey(): Promise { if (memoryOnlyMode) { @@ -199,7 +200,18 @@ export function createEncryptedAsyncStoragePersister({ } keyLoadingPromise = (async () => { try { - const hexKey = await keyManager.getOrCreateKey(); + let hexKey = await keyManager.getOrCreateKey(); + if (!rotationAttempted && storage) { + rotationAttempted = true; + const keyInfo = await keyManager.getKeyInfo(); + if (keyInfo?.needsRotation) { + hexKey = await keyManager.rotateKey({ + reencrypt: async ({ oldKey, newKey }) => { + await rotateStoredEnvelope(oldKey, newKey); + }, + }); + } + } cachedKeyBuffer = hexKeyToArrayBuffer(hexKey); return cachedKeyBuffer; } catch (error) { @@ -216,6 +228,38 @@ export function createEncryptedAsyncStoragePersister({ return keyLoadingPromise; } + async function rotateStoredEnvelope(oldKey: string, newKey: string): Promise { + if (!storage) { + return; + } + + const storedString = await storage.getItem(key); + if (!storedString) { + return; + } + + let parsed: unknown; + try { + parsed = JSON.parse(storedString); + } catch { + throw new Error("stored cache is not valid JSON"); + } + + if ( + typeof parsed !== "object" || + parsed === null || + (parsed as Partial).v !== ENVELOPE_MAGIC + ) { + return; + } + + const oldKeyBuffer = hexKeyToArrayBuffer(oldKey); + const newKeyBuffer = hexKeyToArrayBuffer(newKey); + const restored = await decryptEnvelope(parsed as EncryptedEnvelope, oldKeyBuffer); + const rotatedEnvelope = await encryptClient(restored, newKeyBuffer); + await storage.setItem(key, JSON.stringify(rotatedEnvelope)); + } + async function serialize(client: PersistedClient): Promise { const keyBuffer = await loadKey(); if (!keyBuffer) { @@ -226,15 +270,7 @@ export function createEncryptedAsyncStoragePersister({ return ""; } const prunedClient = evictUnboundedData(client, maxAge, maxSize); - const plaintext = JSON.stringify(prunedClient); - const { encrypted, nonce, authTag } = await encryptionService.encrypt(plaintext, keyBuffer); - const envelope: EncryptedEnvelope = { - v: ENVELOPE_MAGIC, - n: bytesToBase64(nonce), - t: bytesToBase64(authTag), - c: bytesToBase64(new Uint8Array(encrypted)), - }; - return JSON.stringify(envelope); + return JSON.stringify(await encryptClient(prunedClient, keyBuffer)); } async function deserialize(storedString: string): Promise { @@ -308,16 +344,30 @@ export function createEncryptedAsyncStoragePersister({ } const plaintext = JSON.stringify(legacyClient); const { encrypted, nonce, authTag } = await encryptionService.encrypt(plaintext, keyBuffer); - const envelope: EncryptedEnvelope = { + const envelope = createEnvelope(encrypted, nonce, authTag); + if (storage) { + await storage.setItem(key, JSON.stringify(envelope)); + } + return true; + } + + async function encryptClient(client: PersistedClient, keyBuffer: ArrayBuffer): Promise { + const plaintext = JSON.stringify(client); + const { encrypted, nonce, authTag } = await encryptionService.encrypt(plaintext, keyBuffer); + return createEnvelope(encrypted, nonce, authTag); + } + + function createEnvelope( + encrypted: ArrayBuffer, + nonce: Uint8Array, + authTag: Uint8Array, + ): EncryptedEnvelope { + return { v: ENVELOPE_MAGIC, n: bytesToBase64(nonce), t: bytesToBase64(authTag), c: bytesToBase64(new Uint8Array(encrypted)), }; - if (storage) { - await storage.setItem(key, JSON.stringify(envelope)); - } - return true; } async function safeClearStoredValue(): Promise { @@ -345,19 +395,8 @@ export function createEncryptedAsyncStoragePersister({ return undefined; } - const nonce = base64ToBytes(envelope.n); - const authTag = base64ToBytes(envelope.t); - const cipherBytes = base64ToBytes(envelope.c); - const cipherBuffer = new ArrayBuffer(cipherBytes.length); - new Uint8Array(cipherBuffer).set(cipherBytes); - try { - const { decrypted } = await encryptionService.decrypt( - cipherBuffer, - nonce, - authTag, - keyBuffer, - ); + const decrypted = await decryptEnvelope(envelope, keyBuffer); if (decrypted && maxAge > 0 && Date.now() - decrypted.timestamp > maxAge) { await safeClearStoredValue(); return undefined; @@ -383,6 +422,25 @@ export function createEncryptedAsyncStoragePersister({ } } + async function decryptEnvelope( + envelope: EncryptedEnvelope, + keyBuffer: ArrayBuffer, + ): Promise { + const nonce = base64ToBytes(envelope.n); + const authTag = base64ToBytes(envelope.t); + const cipherBytes = base64ToBytes(envelope.c); + const cipherBuffer = new ArrayBuffer(cipherBytes.length); + new Uint8Array(cipherBuffer).set(cipherBytes); + + const { decrypted } = await encryptionService.decrypt( + cipherBuffer, + nonce, + authTag, + keyBuffer, + ); + return decrypted; + } + return createAsyncStoragePersister({ storage, key, diff --git a/src/lib/keyManager.ts b/src/lib/keyManager.ts index b4ae330..82c9db7 100644 --- a/src/lib/keyManager.ts +++ b/src/lib/keyManager.ts @@ -39,6 +39,21 @@ export interface KeyInfo { needsRotation: boolean; } +export interface KeyRotationContext { + /** The currently stored key, used to decrypt existing ciphertext. */ + oldKey: string; + /** The candidate replacement key, used to re-encrypt existing ciphertext. */ + newKey: string; +} + +export interface KeyRotationOptions { + /** + * Re-encrypt persisted data before the stored key is overwritten. + * If this callback fails, rotation is deferred and the old key remains active. + */ + reencrypt?: (context: KeyRotationContext) => Promise; +} + export class KeyManagerError extends Error { constructor( message: string, @@ -92,10 +107,11 @@ export class KeyManager { if (!existingKey) { await this.generateAndStoreKey(); } else { - // Check if key needs rotation + // Check if key needs rotation. Actual rotation is coordinated by + // EncryptedPersister so existing encrypted cache can be migrated first. const keyInfo = await this.getKeyInfo(); if (keyInfo?.needsRotation) { - await this.rotateKey(); + console.warn("[KeyManager] Key rotation deferred until encrypted cache migration"); } } } catch (error) { @@ -285,12 +301,27 @@ export class KeyManager { * Rotate the encryption key (generate a new one) * This should be called when the key is due for rotation */ - async rotateKey(): Promise { - // Generate and store new key - const newKey = await this.generateAndStoreKey(); + async rotateKey(options: KeyRotationOptions = {}): Promise { + const oldKey = await this.getKey(); + const newKey = this.generateKey(); + + if (oldKey && options.reencrypt) { + try { + await options.reencrypt({ oldKey, newKey }); + } catch (error) { + console.warn( + "[KeyManager] Key rotation deferred because cache re-encryption failed:", + error instanceof Error ? error.message : String(error), + ); + return oldKey; + } + } else if (oldKey) { + console.warn("[KeyManager] Key rotation deferred because no re-encryption callback was provided"); + return oldKey; + } - // Note: Old encrypted data should be re-encrypted with the new key - // This is handled by the EncryptedPersister during migration + await this.storeKey(newKey); + this.memoryFallbackKey = newKey; console.log("[KeyManager] Key rotated successfully"); return newKey; diff --git a/tests/keyManager.test.ts b/tests/keyManager.test.ts index b73e411..b6c04c3 100644 --- a/tests/keyManager.test.ts +++ b/tests/keyManager.test.ts @@ -141,13 +141,33 @@ describe("KeyManager", () => { }); describe("rotateKey", () => { - it("should generate a new key different from the previous one", async () => { + it("should defer rotation when no re-encryption callback is provided", async () => { vi.mocked(SecureStore.getItemAsync).mockResolvedValue("f".repeat(64)); vi.mocked(SecureStore.setItemAsync).mockResolvedValue(); const km = new KeyManager({ keyId: "test_rotate_1" }); const newKey = await km.rotateKey(); + expect(newKey).toBe("f".repeat(64)); + expect(SecureStore.setItemAsync).not.toHaveBeenCalled(); + }); + + it("should generate and store a new key after re-encryption succeeds", async () => { + vi.mocked(SecureStore.getItemAsync).mockResolvedValue("f".repeat(64)); + vi.mocked(SecureStore.setItemAsync).mockResolvedValue(); + const reencrypt = vi.fn().mockResolvedValue(undefined); + + const km = new KeyManager({ keyId: "test_rotate_2" }); + const newKey = await km.rotateKey({ reencrypt }); expect(newKey).not.toBe("f".repeat(64)); + expect(reencrypt).toHaveBeenCalledWith({ + oldKey: "f".repeat(64), + newKey, + }); + expect(SecureStore.setItemAsync).toHaveBeenCalledWith( + "test_rotate_2", + newKey, + expect.any(Object), + ); }); }); diff --git a/tests/keyRotationLifecycle.test.ts b/tests/keyRotationLifecycle.test.ts index a47f876..bb230fd 100644 --- a/tests/keyRotationLifecycle.test.ts +++ b/tests/keyRotationLifecycle.test.ts @@ -50,7 +50,7 @@ describe("KeyManager Key-Rotation Lifecycle E2E Test", () => { vi.spyOn(KeyManager.prototype, "isSecureStoreAvailable").mockResolvedValue(true); }); - it("should successfully persist under initial key, and then fail decryption (data loss / undefined) after rotation", async () => { + it("should keep previously encrypted offline cache readable after rotation", async () => { // 1. Initialize KeyManager with a test-specific key ID const keyManager = new KeyManager({ keyId: "test_rotation_lifecycle_key" }); const encryptionService = new EncryptionService(); @@ -87,10 +87,12 @@ describe("KeyManager Key-Rotation Lifecycle E2E Test", () => { expect(restoredBeforeRotation).toBeDefined(); expect(restoredBeforeRotation?.clientState).toBeDefined(); - // 3. Trigger KeyManager.rotateKey() which generates and stores a new key + // 3. Mark the key as expired so the next persister session coordinates a rotation. const oldKey = await keyManager.getKey(); - const newKey = await keyManager.rotateKey(); - expect(oldKey).not.toBe(newKey); + secureStoreMock.set( + "test_rotation_lifecycle_key_timestamp", + (Date.now() - 35 * 24 * 60 * 60 * 1000).toString(), + ); // 4. Create a fresh persister instance to simulate a new app session/launch. // This is critical because EncryptedPersister internally caches the key buffer @@ -104,33 +106,19 @@ describe("KeyManager Key-Rotation Lifecycle E2E Test", () => { keyManager, }); - // 5. Attempt to restore/deserialize the previously-encrypted data + // 5. Attempt to restore/deserialize the previously-encrypted data. + // The persister should re-encrypt the envelope before KeyManager commits + // the new key, so a fresh app session remains able to hydrate offline data. const restoredAfterRotation = await persister2.restoreClient(); + const rotatedKey = await keyManager.getKey(); + expect(rotatedKey).toBeDefined(); + expect(rotatedKey).not.toBe(oldKey); + expect(restoredAfterRotation).toBeDefined(); + expect(restoredAfterRotation?.clientState).toBeDefined(); - // --- ASSERTIONS & DOCUMENTATION OF ACTUAL BEHAVIOR --- - // - // Note: The KeyManager's `rotateKey` method states: - // "Note: Old encrypted data should be re-encrypted with the new key. - // This is handled by the EncryptedPersister during migration" - // - // However, the current EncryptedPersister only supports migrating legacy - // plaintext (unencrypted) data to encrypted format, and does not perform - // any key rotation migration. - // - // As a result: - // - Decryption fails because it attempts to decrypt the old ciphertext using the new key. - // - EncryptedPersister treats the decryption failure as a potential tampering event. - // - The persister clears the cached entry from storage to maintain security/integrity. - // - restoreClient() returns `undefined`. - // - // This represents a silent data loss bug during key rotation, which is documented here - // for the companion fix issue to resolve. - - // The restoreClient should return undefined on decryption failure - expect(restoredAfterRotation).toBeUndefined(); - - // The old cache should have been cleared/evicted from storage to avoid corrupted states + // The cache should remain present, now encrypted under the rotated key. const storedAfterRotation = await storage.getItem(PERSISTED_QUERY_CACHE_KEY); - expect(storedAfterRotation).toBeNull(); + expect(storedAfterRotation).toBeDefined(); + expect(storedAfterRotation).toContain("gp1:"); }); });