From 249f9e24720768247066fbd6d7fb2f9696fd71d6 Mon Sep 17 00:00:00 2001 From: Logan Yang Date: Thu, 14 May 2026 00:47:59 -0700 Subject: [PATCH] fix(encryption): deprecate encryption toggle; fix mobile chat failure on desktop-encrypted keys (#2446) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mobile): handle desktop-encrypted API keys on mobile (follow-up to #2443) PR #2443 added the Buffer polyfill to base64.ts, utils.ts, and BedrockChatModel.ts but missed src/encryptionService.ts:119. My original write-up incorrectly claimed mobile never enters the DESKTOP_PREFIX branch of getDecryptedKey — it does, whenever a user encrypts their API keys on desktop and then syncs the vault to mobile via Obsidian Sync. Mobile reads the same encrypted blob, enters the DESKTOP_PREFIX branch, and hits `Buffer.from(...)`, which throws "Can't find variable: Buffer" on iOS WebKit. User reproducer: "Model request failed: Can't find variable: Buffer" during normal chat send on mobile after PR #2443 merged. Two-part fix: 1. Add `import { Buffer } from "buffer"` (the npm polyfill, already in dependencies and used in PR #2443) so the literal ReferenceError is gone. The legacy ENCRYPTION_PREFIX branch at line 137 also benefits. 2. Even with Buffer available, the next line `getSafeStorage()!.decryptString(buffer)` cannot succeed on mobile because there is no Electron safeStorage there. The encrypted blob uses OS-level encryption (Keychain on macOS, DPAPI on Windows, libsecret on Linux) that cannot be replayed from mobile. Add a Platform.isMobile guard that throws a clear, actionable error asking the user to re-enter their API key on this device instead of crashing on the unreachable decryption call. Out of scope: - The DESKTOP_PREFIX-on-mobile situation requires the user to re-save their key in mobile settings (which will re-encrypt with WEBCRYPTO_PREFIX, the cross-platform path that Web Crypto handles). Surfacing a clear error is the right user experience until the plugin auto-migrates on first mobile launch — that's a larger follow-up not appropriate for a same-day hotfix. - The legacy ENCRYPTION_PREFIX branch at line 137 still references Buffer, but it's already gated by `getSafeStorage()?.isEncryptionAvailable()` which returns false on mobile, so mobile never enters it. The new import covers it defensively if that gate ever changes. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(mobile): list desktop-encrypted keys on mobile load + block desktop encryption Two additions on top of the existing fix in this PR: 1. **Guard the encryption side with Platform.isDesktop.** Mobile should NEVER produce DESKTOP_PREFIX keys, even if `require("electron")` happens to return a truthy shim. Before this change, the only gate was `getSafeStorage()?.isEncryptionAvailable()` — fragile, since any mobile environment that exposes a partial Electron-like shim could slip through and save a key as DESKTOP_PREFIX that the same device then can't decrypt. 2. **Surface a Notice on mobile load** listing every key field that's still DESKTOP_PREFIX-encrypted. Without this, the user only finds out which key needs re-entry by hitting a chat error and having to guess. With it, they see the full list at startup: "Copilot: 3 API key(s) encrypted on desktop and unusable on mobile. Re-enter in Copilot settings: plusLicenseKey, openAIApiKey, activeModels[gpt-5.5]" Notice has timeout 0 so it stays until dismissed. The new exported `findDesktopEncryptedKeyFields(settings)` walks: - All top-level string settings (catches plusLicenseKey, openAIApiKey, anthropicApiKey, githubCopilotToken, etc.) - settings.activeModels[*].apiKey (per-model custom keys) - settings.activeEmbeddingModels[*].apiKey The exact field names returned match the keys in data.json so the user can grep / inspect / re-enter them confidently. This addresses the user report after the first commit in this PR: "despite reentering api key on mobile" — which happened because they re-entered one key but a *different* key (likely plusLicenseKey or a model-specific apiKey) was still DESKTOP_PREFIX, causing the chat to fail on that other key. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(encryption): deprecate encryption-at-rest; make encrypt/save a no-op The encryption toggle has created repeated incidents: 1. Mobile cannot decrypt DESKTOP_PREFIX (Electron safeStorage) keys that synced over from desktop, because mobile has no safeStorage. 2. The WebCrypto path is security theater — the AES-GCM key is a hardcoded "obsidian-copilot-v1" string, so anyone with data.json can decrypt every "encrypted" key in seconds. 3. PRs #2401 and #2403 (3.3.0) introduced bare `Buffer.from` in the encryption + base64 paths, which crashed mobile WebView with "Can't find variable: Buffer" — see #2443 and earlier commits in this PR. 4. Even with that fixed, the eager dispatch-map builders in chatModelManager / embeddingManager await getDecryptedKey for every provider; a throw on the one bad desktop-encrypted key killed chats that used a different provider with a valid key. Rather than continue patching, deprecate the toggle and stop adding encryption to new keys. Existing encrypted blobs in data.json remain readable via getDecryptedKey (for all three legacy formats: DESKTOP_PREFIX, WEBCRYPTO_PREFIX, and the original `enc_`); they migrate organically to plaintext the next time the user re-saves any given key. Changes: - `encryptAllKeys` → no-op (returns settings unchanged). 6+ lines of per-field encryption gone. - `getEncryptedKey` → no-op (returns plaintext, strips `dec_` marker if present). 25 lines of safeStorage / WebCrypto / DESKTOP_PREFIX branching gone. - `main.ts` settings-save → unconditionally `saveData(next)` instead of branching on `enableEncryption`. The setting field stays in the CopilotSettings interface for backwards-compat with existing data.json files but is no longer read or written. - `AdvancedSettings.tsx` → "Enable Encryption" toggle removed from the UI. Users who had it on get no further encryption; users who had it off see no change. Mobile-specific path inside `getDecryptedKey`: - DESKTOP_PREFIX keys on mobile return `DESKTOP_KEY_ON_MOBILE_SENTINEL` (no longer throw). chatModelManager / embeddingManager's eager dispatch-map construction completes; the sentinel sits harmlessly in unused config slots; providers actually being used with valid plaintext keys work. The startup Notice (already in this PR) surfaces which fields the user must re-enter. Tests: - `getEncryptedKey` test suite rewritten to verify the new no-op behavior (plaintext passthrough, dec_ stripping, empty-string). - `encryptAllKeys` test suite shrunk to one identity-return assertion. - Cross-platform encrypt/decrypt round-trip tests removed (encryption is gone). Added a legacy WebCrypto decrypt test to confirm we can still read existing enc_web_* blobs. - 1974 tests pass (was 1975; -1 for the dropped round-trip test). Net change: encryptionService.ts down from 207 to 130 lines. Risk: - `data.json` now contains plaintext API keys after first re-save. Worth saying explicitly in release notes. The previous "encryption" was indistinguishable from plaintext for anyone with filesystem access (hardcoded WebCrypto key), so this is a documentation change more than a security change. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(mobile): simplify to a throw; drop sentinel + Notice + helper User feedback: the sentinel + startup-Notice approach added 70+ lines of transitional code that we'd have to delete later. Replace with the lightest viable shape. - Mobile DESKTOP_PREFIX branch in getDecryptedKey now throws a generic but actionable error ("re-enter your API keys on this device") and nothing else. - Removed DESKTOP_KEY_ON_MOBILE_SENTINEL constant. - Removed findDesktopEncryptedKeyFields function. - Removed the startup Notice block in main.ts and its import. Trade-off accepted: with multiple desktop-encrypted keys present, the chat fails on the first one the eager dispatch-map encounters; user re-enters that key, retries, hits the next bad key, repeats. The generic message tells them what to do; they iterate rather than seeing the full list up front. Net: -71 lines vs. previous approach. Future cleanup (when no users have desktop-encrypted keys anymore) is one small if-branch in getDecryptedKey instead of a sentinel + assert + helper + Notice spread across four files. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- src/encryptionService.test.ts | 102 ++++++----------- src/encryptionService.ts | 105 ++++++------------ src/main.ts | 10 +- .../v2/components/AdvancedSettings.tsx | 10 -- 4 files changed, 70 insertions(+), 157 deletions(-) diff --git a/src/encryptionService.test.ts b/src/encryptionService.test.ts index 17c336f6..5de6d3f0 100644 --- a/src/encryptionService.test.ts +++ b/src/encryptionService.test.ts @@ -69,21 +69,21 @@ describe("EncryptionService", () => { jest.resetModules(); }); - describe("getEncryptedKey", () => { - it("should encrypt an API key", async () => { + describe("getEncryptedKey (deprecated no-op)", () => { + it("should return plaintext key unchanged", async () => { const apiKey = "testApiKey"; - const encryptedKey = await getEncryptedKey(apiKey); - // The key is base64 encoded, so we should expect that format - expect(encryptedKey).toMatch(/^enc_(desk|web)_[A-Za-z0-9+/=]+$/); - // Verify we can decrypt it back - const decryptedKey = await getDecryptedKey(encryptedKey); - expect(decryptedKey).toBe(apiKey); + const result = await getEncryptedKey(apiKey); + expect(result).toBe(apiKey); }); - it("should return the original key if already encrypted", async () => { - const apiKey = "enc_testApiKey"; - const encryptedKey = await getEncryptedKey(apiKey); - expect(encryptedKey).toBe(apiKey); + it("should strip the dec_ prefix when present", async () => { + const result = await getEncryptedKey("dec_testApiKey"); + expect(result).toBe("testApiKey"); + }); + + it("should return empty string for empty input", async () => { + const result = await getEncryptedKey(""); + expect(result).toBe(""); }); }); @@ -102,8 +102,8 @@ describe("EncryptionService", () => { }); }); - describe("encryptAllKeys", () => { - it("should encrypt all keys containing 'apikey'", async () => { + describe("encryptAllKeys (deprecated no-op)", () => { + it("should return settings unchanged regardless of enableEncryption", async () => { const settings = { enableEncryption: true, openAIApiKey: "testApiKey", @@ -111,28 +111,8 @@ describe("EncryptionService", () => { userSystemPrompt: "shouldBeIgnored", } as unknown as CopilotSettings; - const newSettings = await encryptAllKeys(settings); - expect(newSettings.openAIApiKey).toMatch(/^enc_(desk|web)_[A-Za-z0-9+/=]+$/); - expect(newSettings.cohereApiKey).toMatch(/^enc_(desk|web)_[A-Za-z0-9+/=]+$/); - expect(newSettings.userSystemPrompt).toBe("shouldBeIgnored"); - - // Verify we can decrypt the keys back - const decryptedOpenAI = await getDecryptedKey(newSettings.openAIApiKey); - const decryptedCohere = await getDecryptedKey(newSettings.cohereApiKey); - expect(decryptedOpenAI).toBe("testApiKey"); - expect(decryptedCohere).toBe("anotherTestApiKey"); - }); - - it("should not encrypt keys when encryption is not enabled", async () => { - const newSettings = await encryptAllKeys({ - enableEncryption: false, - openAIApiKey: "testApiKey", - cohereApiKey: "anotherTestApiKey", - userSystemPrompt: "shouldBeIgnored", - } as unknown as CopilotSettings); - expect(newSettings.openAIApiKey).toBe("testApiKey"); - expect(newSettings.cohereApiKey).toBe("anotherTestApiKey"); - expect(newSettings.userSystemPrompt).toBe("shouldBeIgnored"); + const result = await encryptAllKeys(settings); + expect(result).toBe(settings); }); }); }); @@ -153,41 +133,23 @@ describe("Cross-platform compatibility", () => { console.error = originalConsoleError; }); - it("should encrypt and decrypt consistently on mobile", async () => { - // Mock as mobile by making safeStorage unavailable - mockElectron.remote.safeStorage.isEncryptionAvailable.mockReturnValue(false); + it("should decrypt legacy WebCrypto-encrypted keys on mobile", async () => { + // Simulate a key encrypted with the legacy WebCrypto path (the only path + // mobile ever used, and the path desktop fell back to when safeStorage + // wasn't available). The mocked subtle.decrypt strips the `_encrypted` + // suffix the encrypt mock appends. + const enc = (await mockSubtle.encrypt( + null, + null, + new TextEncoder().encode("testApiKey").buffer + )) as ArrayBuffer; + const encBytes = new Uint8Array(enc); + let binary = ""; + for (let i = 0; i < encBytes.length; i++) binary += String.fromCharCode(encBytes[i]); + const webEncryptedKey = "enc_web_" + window.btoa(binary); - const originalKey = "testApiKey"; - const encryptedKey = await getEncryptedKey(originalKey); - expect(encryptedKey).toMatch(/^enc_(desk|web)_[A-Za-z0-9+/=]+$/); - - // Reset the mock counts before decryption - mockSubtle.encrypt.mockClear(); - mockSubtle.decrypt.mockClear(); - - const decryptedKey = await getDecryptedKey(encryptedKey); - expect(decryptedKey).toBe(originalKey); - - // On mobile, we should use Web Crypto API for decryption + const decryptedKey = await getDecryptedKey(webEncryptedKey); + expect(decryptedKey).toBe("testApiKey"); expect(mockSubtle.decrypt).toHaveBeenCalled(); }); - - it("should be able to decrypt mobile-encrypted keys on desktop", async () => { - // First encrypt on mobile - mockElectron.remote.safeStorage.isEncryptionAvailable.mockReturnValue(false); - - const originalKey = "testApiKey"; - const mobileEncryptedKey = await getEncryptedKey(originalKey); - expect(mobileEncryptedKey).toMatch(/^enc_(desk|web)_[A-Za-z0-9+/=]+$/); - expect(mockSubtle.encrypt).toHaveBeenCalled(); - - // Reset the mock counts before desktop decryption - mockSubtle.encrypt.mockClear(); - mockSubtle.decrypt.mockClear(); - - // Then decrypt on desktop - mockElectron.remote.safeStorage.isEncryptionAvailable.mockReturnValue(true); - const decryptedKey = await getDecryptedKey(mobileEncryptedKey); - expect(decryptedKey).toBe(originalKey); - }); }); diff --git a/src/encryptionService.ts b/src/encryptionService.ts index 5399b69a..8462596e 100644 --- a/src/encryptionService.ts +++ b/src/encryptionService.ts @@ -1,3 +1,10 @@ +// Reason: `buffer` is the npm polyfill (browser-compatible), bundled by +// esbuild so the same Buffer code path works on desktop (Electron) and +// mobile (WebView). Without this import, bare `Buffer` is undefined on +// mobile and throws "Can't find variable: Buffer" — see PR #2443. +// eslint-disable-next-line import/no-nodejs-modules +import { Buffer } from "buffer"; + import { type CopilotSettings } from "@/settings/model"; import { Platform } from "obsidian"; @@ -19,7 +26,11 @@ function getSafeStorage(): SafeStorage | null { return safeStorageInternal; } -// Add new prefixes to distinguish encryption methods +// Encryption-at-rest is deprecated as of this commit. `getEncryptedKey` +// and `encryptAllKeys` are now no-ops, so new keys are saved plaintext. +// We still decrypt legacy `enc_*` blobs at read time via `getDecryptedKey` +// for backwards compatibility; existing keys migrate organically the next +// time the user re-saves them. const DESKTOP_PREFIX = "enc_desk_"; const WEBCRYPTO_PREFIX = "enc_web_"; // Keep old prefix for backward compatibility @@ -37,72 +48,25 @@ async function getEncryptionKey(): Promise { ]); } +/** + * No-op: encryption-at-rest is deprecated. Returns settings unchanged. + * Existing encrypted keys (`enc_*`) stay in data.json and are decrypted at + * read time via getDecryptedKey for backwards compatibility. New keys are + * saved plaintext. + */ export async function encryptAllKeys( settings: Readonly ): Promise> { - if (!settings.enableEncryption) { - return settings; - } - const newSettings = { ...settings }; - const keysToEncrypt = Object.keys(settings).filter( - (key) => - key.toLowerCase().includes("apikey") || - key === "plusLicenseKey" || - key === "githubCopilotAccessToken" || - key === "githubCopilotToken" - ); - - for (const key of keysToEncrypt) { - const apiKey = settings[key as keyof CopilotSettings] as string; - (newSettings[key as keyof CopilotSettings] as any) = await getEncryptedKey(apiKey); - } - - if (Array.isArray(settings.activeModels)) { - newSettings.activeModels = await Promise.all( - settings.activeModels.map(async (model) => ({ - ...model, - apiKey: await getEncryptedKey(model.apiKey || ""), - })) - ); - } - - if (Array.isArray(settings.activeEmbeddingModels)) { - newSettings.activeEmbeddingModels = await Promise.all( - settings.activeEmbeddingModels.map(async (model) => ({ - ...model, - apiKey: await getEncryptedKey(model.apiKey || ""), - })) - ); - } - - return newSettings; + return settings; } +/** + * No-op: encryption-at-rest is deprecated. Strips the `dec_` marker if + * present and returns the raw plaintext key. + */ export async function getEncryptedKey(apiKey: string): Promise { - if (!apiKey || apiKey.startsWith(ENCRYPTION_PREFIX)) { - return apiKey; - } - - if (isDecrypted(apiKey)) { - apiKey = apiKey.replace(DECRYPTION_PREFIX, ""); - } - - try { - // Try desktop encryption first - if (getSafeStorage()?.isEncryptionAvailable()) { - const encryptedBuffer = getSafeStorage()!.encryptString(apiKey); - return DESKTOP_PREFIX + encryptedBuffer.toString("base64"); - } - - // Fallback to Web Crypto API - const key = await getEncryptionKey(); - const encodedData = new TextEncoder().encode(apiKey); - const encryptedData = await crypto.subtle.encrypt(ALGORITHM, key, encodedData); - return WEBCRYPTO_PREFIX + arrayBufferToBase64(encryptedData); - } catch (error) { - console.error("Encryption failed:", error); - return apiKey; - } + if (!apiKey) return apiKey; + return isDecrypted(apiKey) ? apiKey.replace(DECRYPTION_PREFIX, "") : apiKey; } export async function getDecryptedKey(apiKey: string): Promise { @@ -113,8 +77,16 @@ export async function getDecryptedKey(apiKey: string): Promise { return apiKey.replace(DECRYPTION_PREFIX, ""); } - // Handle different encryption methods + // Handle different encryption methods (legacy data only — new keys are + // saved plaintext per the deprecation above). if (apiKey.startsWith(DESKTOP_PREFIX)) { + if (Platform.isMobile) { + throw new Error( + "An API key was encrypted on desktop with OS-level encryption that's " + + "unavailable on mobile. Please re-enter your API keys in Copilot " + + "settings on this device." + ); + } const base64Data = apiKey.replace(DESKTOP_PREFIX, ""); const buffer = Buffer.from(base64Data, "base64"); return getSafeStorage()!.decryptString(buffer); @@ -162,15 +134,6 @@ function isDecrypted(keyBuffer: string): boolean { return keyBuffer.startsWith(DECRYPTION_PREFIX); } -function arrayBufferToBase64(buffer: ArrayBuffer): string { - const bytes = new Uint8Array(buffer); - let binary = ""; - for (let i = 0; i < bytes.byteLength; i++) { - binary += String.fromCharCode(bytes[i]); - } - return window.btoa(binary); -} - function base64ToArrayBuffer(base64: string): ArrayBuffer { const binaryString = window.atob(base64); const bytes = new Uint8Array(binaryString.length); diff --git a/src/main.ts b/src/main.ts index 8417944e..4540ec5e 100644 --- a/src/main.ts +++ b/src/main.ts @@ -21,7 +21,6 @@ import { ProjectRegister } from "@/projects/projectRegister"; import { ABORT_REASON, CHAT_VIEWTYPE, DEFAULT_OPEN_AREA, EVENT_NAMES } from "@/constants"; import { ChatManager } from "@/core/ChatManager"; import { MessageRepository } from "@/core/MessageRepository"; -import { encryptAllKeys } from "@/encryptionService"; import { logError, logInfo, logWarn } from "@/logger"; import { logFileManager } from "@/logFileManager"; import { UserMemoryManager } from "@/memory/UserMemoryManager"; @@ -106,11 +105,10 @@ export default class CopilotPlugin extends Plugin { await this.loadSettings(); this.settingsUnsubscriber = subscribeToSettingsChange((prev, next) => { void (async () => { - if (next.enableEncryption) { - await this.saveData(await encryptAllKeys(next)); - } else { - await this.saveData(next); - } + // Reason: encryption-at-rest is deprecated. encryptAllKeys is a no-op + // now; new keys are saved plaintext. Existing encrypted keys remain + // in data.json and are decrypted lazily by getDecryptedKey when read. + await this.saveData(next); registerCommands(this, prev, next); })(); }); diff --git a/src/settings/v2/components/AdvancedSettings.tsx b/src/settings/v2/components/AdvancedSettings.tsx index ca2f9415..070e150f 100644 --- a/src/settings/v2/components/AdvancedSettings.tsx +++ b/src/settings/v2/components/AdvancedSettings.tsx @@ -95,16 +95,6 @@ export const AdvancedSettings: React.FC = () => {

Others

- { - updateSetting("enableEncryption", checked); - }} - /> -