fix: preserve encrypted cache during key rotation

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

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

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<ArrayBuffer | null> {
     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<void> {
+    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<EncryptedEnvelope>).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<string> {
     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<PersistedClient | undefined> {
@@ -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<EncryptedEnvelope> {
+    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<void> {
@@ -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<PersistedClient>(
-        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<PersistedClient> {
+    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<PersistedClient>(
+      cipherBuffer,
+      nonce,
+      authTag,
+      keyBuffer,
+    );
+    return decrypted;
+  }
+
   return createAsyncStoragePersister({
     storage,
     key,

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<void>;
+}
+
 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<string> {
-    // Generate and store new key
-    const newKey = await this.generateAndStoreKey();
+  async rotateKey(options: KeyRotationOptions = {}): Promise<string> {
+    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;

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

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:");
   });
 });