diff --git a/docs/troubleshooting-and-faq.md b/docs/troubleshooting-and-faq.md index 8d0f12f3..061e9f36 100644 --- a/docs/troubleshooting-and-faq.md +++ b/docs/troubleshooting-and-faq.md @@ -189,15 +189,16 @@ If your settings get into a bad state, you can reset: 1. Go to **Settings → Copilot** → find the reset option 2. Or delete the `data.json` file from the plugin folder: `.obsidian/plugins/copilot/data.json` -⚠️ Resetting will delete all your settings including API keys. Back them up first. +⚠️ Resetting clears all your settings. API keys kept in `data.json` (standard storage) are removed, but keys stored in the Obsidian Keychain are **not** — to erase those, use **Settings → Copilot → Advanced → API Key Storage → Delete All Keys**. Back up your keys first. -### API Key Encryption +### API Key Storage -Copilot can encrypt your API keys at rest for added security. +Copilot has two ways to store API keys: -**Enable**: **Settings → Copilot → Advanced → Enable Encryption** +- **Standard storage**: API keys are saved in `data.json` in plain text. Existing vaults stay in this mode until you choose to migrate. +- **Obsidian Keychain**: New installs use this by default. You can also switch an existing vault by going to **Settings → Copilot → Advanced → API Key Storage** and clicking **Migrate to Obsidian Keychain**. After migration, `data.json` no longer contains your API keys. -If you see strange authentication errors after enabling this, try disabling encryption and re-entering your keys. +The Obsidian Keychain is per device. If you sync your vault to another device, you may need to re-enter API keys there. ### Debug Mode and Logs diff --git a/src/components/modals/MigrateConfirmModal.test.tsx b/src/components/modals/MigrateConfirmModal.test.tsx new file mode 100644 index 00000000..57acbeec --- /dev/null +++ b/src/components/modals/MigrateConfirmModal.test.tsx @@ -0,0 +1,118 @@ +/** + * Tests for MigrateConfirmModal — verifies that the Migrate button is gated + * behind the acknowledgement checkbox, that confirm fires the callback, and + * that cancel does not fire the confirm callback. + * + * We exercise the React content component directly so we can assert on + * checkbox/button state without going through Obsidian's Modal lifecycle. + */ + +import React from "react"; +import { act, fireEvent, screen } from "@testing-library/react"; + +// Reason: jsdom doesn't provide Obsidian's `activeDocument` global. Alias it +// to `window.document` so the popout-safe identifier resolves at runtime +// here (lint rule `obsidianmd/prefer-active-doc` enforces this name over +// bare `document`). Scoped to this file rather than jest.setup.js to keep +// the alias out of unrelated suites. +if (typeof (window as { activeDocument?: Document }).activeDocument === "undefined") { + (window as { activeDocument: Document }).activeDocument = window.document; +} + +// Mock obsidian before importing the modal file. +jest.mock("obsidian", () => ({ + App: class App {}, + Modal: class Modal { + app: unknown; + contentEl = activeDocument.createElement("div"); + constructor(app: unknown) { + this.app = app; + } + open() {} + close() {} + }, +})); + +jest.mock("lucide-react", () => ({ + Info: () => , + ShieldCheck: () => , + Smartphone: () => , +})); + +import { MigrateConfirmModal } from "./MigrateConfirmModal"; + +/** + * Render the modal's content by invoking onOpen on an instance so we + * exercise the same render path as production code. + */ +function renderModal(onConfirm: jest.Mock, onCancel?: jest.Mock) { + const app = {} as InstanceType; + const modal = new MigrateConfirmModal(app, onConfirm, onCancel); + // Replace the auto-created contentEl with one inside a DOM container so + // testing-library queries work on it. + const container = activeDocument.createElement("div"); + activeDocument.body.appendChild(container); + // Reason: contentEl is provided by the Obsidian Modal mock; we override it + // so testing-library can discover the rendered DOM under activeDocument.body. + (modal as unknown as { contentEl: HTMLElement }).contentEl = container; + // Reason: React 18's createRoot.render is async — wrap onOpen in act() so + // the rendered DOM is flushed before testing-library queries run. + act(() => { + modal.onOpen(); + }); + return { modal, container }; +} + +describe("MigrateConfirmModal", () => { + afterEach(() => { + activeDocument.body.innerHTML = ""; + }); + + it("disables the Migrate button until the acknowledgement checkbox is checked", () => { + const onConfirm = jest.fn(); + renderModal(onConfirm); + + const migrateButton = screen.getByRole("button", { name: /migrate/i }); + expect((migrateButton as HTMLButtonElement).disabled).toBe(true); + expect(onConfirm).not.toHaveBeenCalled(); + + // Clicking the disabled button must not fire the callback. + fireEvent.click(migrateButton); + expect(onConfirm).not.toHaveBeenCalled(); + }); + + it("enables Migrate after the checkbox is checked and fires onConfirm on click", () => { + const onConfirm = jest.fn(); + renderModal(onConfirm); + + const checkbox = screen.getByRole("checkbox"); + act(() => { + fireEvent.click(checkbox); + }); + + const migrateButton = screen.getByRole("button", { name: /migrate/i }); + expect((migrateButton as HTMLButtonElement).disabled).toBe(false); + + fireEvent.click(migrateButton); + expect(onConfirm).toHaveBeenCalledTimes(1); + }); + + it("fires onCancel and not onConfirm when the user cancels", () => { + const onConfirm = jest.fn(); + const onCancel = jest.fn(); + const { modal } = renderModal(onConfirm, onCancel); + + const cancelButton = screen.getByRole("button", { name: /cancel/i }); + act(() => { + fireEvent.click(cancelButton); + }); + + // The cancel button calls close(); onClose then invokes onCancel. + act(() => { + modal.onClose(); + }); + + expect(onConfirm).not.toHaveBeenCalled(); + expect(onCancel).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/components/modals/MigrateConfirmModal.tsx b/src/components/modals/MigrateConfirmModal.tsx new file mode 100644 index 00000000..427af3fb --- /dev/null +++ b/src/components/modals/MigrateConfirmModal.tsx @@ -0,0 +1,156 @@ +/** + * Modal shown when the user clicks "Migrate to Keychain" in Advanced settings. + * + * Frames migration as a positive security upgrade (ShieldCheck header) while + * still surfacing the multi-device trade-off in a structured card. The + * confirm button is gated behind a checkbox so the implication is read, not + * dismissed by muscle memory. + * + * Visual structure mirrors the original KeychainMigrationModal: + * header icon + heading, plain description, structured card for the + * multi-device note, and a muted tip line. + * + * This is intentionally a dedicated modal (not an extension of ConfirmModal) + * because ConfirmModal exposes a positional constructor that does not handle + * gated confirmation cleanly — see project pattern in NewChatConfirmModal / + * ResetSettingsConfirmModal. + * + * TERMINOLOGY NOTE — "Obsidian Keychain" / "Obsidian's Keychain" is the + * canonical user-facing term. Reasons: + * - Obsidian SecretStorage is vault-scoped and Obsidian-managed; calling it + * "OS Keychain" overpromises an OS-level guarantee the feature does not + * make. + * - It matches what users see in Obsidian's own UI. + * - "Secure Storage" was tried but is too generic and loses the Keychain + * mental model. + * Do NOT change to "OS Keychain", "Secure Storage", or similar without + * discussing the user-facing trade-offs first. + */ + +import { Button } from "@/components/ui/button"; +import { Info, ShieldCheck, Smartphone } from "lucide-react"; +import { App, Modal } from "obsidian"; +import React, { useState } from "react"; +import { createRoot, Root } from "react-dom/client"; + +interface MigrateConfirmContentProps { + onConfirm: () => void; + onCancel: () => void; +} + +/** + * Body of the migration confirmation modal. Renders the structured content + * and gates the Migrate button behind a single acknowledgement checkbox. + */ +function MigrateConfirmContent({ onConfirm, onCancel }: MigrateConfirmContentProps) { + const [acknowledged, setAcknowledged] = useState(false); + + return ( +
+
+ +

Migrate API Keys to Obsidian Keychain

+
+ +

+ Move your API keys from data.json{" "} + to this device's{" "} + Obsidian Keychain.{" "} + data.json will be stripped of API keys after migration. +

+ + {/* Reason: the multi-device implication is the most common source of + confusion after migration — anchor it visually as a card with a + phone icon so users can't miss it. */} +
+ +
+
Using Copilot on multiple devices?
+
+ Each device has its own Obsidian Keychain. Other devices syncing this vault will need to + re-enter their API keys after migration. +
+
+
+ +
+ + Tip: keep an offline backup of your API keys. +
+ + {/* Reason: gating the confirm button forces the user to read the warning + rather than dismissing it by muscle memory. */} + + +
+ + +
+
+ ); +} + +/** + * Obsidian Modal wrapping the migration confirmation UI. Resolves with + * `true` when the user confirms after acknowledging the warning, or + * `false` on cancel / close. + * + * Reason: setTitle is intentionally omitted — the body's ShieldCheck heading + * serves as the title, matching the original KeychainMigrationModal layout + * and avoiding visual duplication. + */ +export class MigrateConfirmModal extends Modal { + private root: Root | null = null; + private confirmed = false; + + constructor( + app: App, + private onConfirm: () => void, + private onCancel?: () => void + ) { + super(app); + } + + onOpen() { + const { contentEl } = this; + this.root = createRoot(contentEl); + + const handleConfirm = () => { + this.confirmed = true; + this.onConfirm(); + this.close(); + }; + + const handleCancel = () => { + this.close(); + }; + + this.root.render(); + } + + onClose() { + if (!this.confirmed) { + this.onCancel?.(); + } + this.root?.unmount(); + this.root = null; + } +} diff --git a/src/components/ui/password-input.test.tsx b/src/components/ui/password-input.test.tsx new file mode 100644 index 00000000..0bd93bd4 --- /dev/null +++ b/src/components/ui/password-input.test.tsx @@ -0,0 +1,136 @@ +/** + * Tests for PasswordInput — pins the "decrypt on first mount only" contract. + * + * Reason: an earlier revision of this component re-ran decryption on every + * prop update whenever `hasEncryptionPrefix(value)` returned true. That + * silently cleared any plaintext API key starting with `enc_*` after the + * user typed it (codex review #3234676133). The first-mount-only guard + * below makes sure typed plaintext is never re-interpreted as ciphertext. + */ + +import React, { useState } from "react"; +import { act, fireEvent, render, screen } from "@testing-library/react"; +import { PasswordInput } from "./password-input"; + +const mockGetDecryptedKey = jest.fn, [string]>(); +const mockHasEncryptionPrefix = jest.fn(); + +jest.mock("@/encryptionService", () => ({ + getDecryptedKey: (value: string) => mockGetDecryptedKey(value), + hasEncryptionPrefix: (value: string) => mockHasEncryptionPrefix(value), +})); + +jest.mock("@/logger", () => ({ logError: jest.fn() })); + +jest.mock("@/utils", () => ({ err2String: (e: unknown) => String(e) })); + +// Reason: shadcn `` doesn't bring useful behavior to these tests — +// mock to a plain so we can drive .value directly. +jest.mock("@/components/ui/input", () => ({ + Input: React.forwardRef>( + function Input(props, ref) { + return ; + } + ), +})); + +// Reason: lucide icons render to nothing useful in tests; stub them out. +jest.mock("lucide-react", () => ({ + Eye: () => , + EyeOff: () => , +})); + +beforeEach(() => { + jest.clearAllMocks(); + // Default: prefix detector treats `enc_*` as encrypted. + mockHasEncryptionPrefix.mockImplementation((v: string) => Boolean(v) && v.startsWith("enc_")); +}); + +describe("PasswordInput", () => { + it("decrypts on first mount when the initial value is encrypted", async () => { + mockGetDecryptedKey.mockResolvedValueOnce("sk-real"); + + let view!: ReturnType; + await act(async () => { + view = render(); + await mockGetDecryptedKey.mock.results[0]?.value; + }); + + const input = view.container.querySelector("input") as HTMLInputElement; + expect(input.value).toBe("sk-real"); + expect(mockGetDecryptedKey).toHaveBeenCalledTimes(1); + }); + + it("shows the decryption-failed message when the initial value cannot be decrypted", async () => { + mockGetDecryptedKey.mockResolvedValueOnce(""); + + let view!: ReturnType; + await act(async () => { + view = render(); + await mockGetDecryptedKey.mock.results[0]?.value; + }); + + const input = view.container.querySelector("input") as HTMLInputElement; + expect(input.value).toBe(""); + expect(screen.getByText(/Unable to decrypt this key/i)).not.toBeNull(); + }); + + it("does NOT decrypt plaintext starting with enc_* once the user has typed it", async () => { + // Reason: regression guard. With the old prefix-anytime logic, the + // controlled prop echo of the user's input would re-enter the effect + // and clear the field via `getDecryptedKey() === ""`. + function Harness() { + const [v, setV] = useState(""); + return ; + } + + const view = render(); + const input = view.container.querySelector("input") as HTMLInputElement; + + await act(async () => { + fireEvent.change(input, { target: { value: "enc_test" } }); + }); + + // Reason: even though `hasEncryptionPrefix("enc_test")` is true, the + // first-mount guard blocks any decrypt — user input survives intact. + expect(input.value).toBe("enc_test"); + expect(mockGetDecryptedKey).not.toHaveBeenCalled(); + expect(screen.queryByText(/Unable to decrypt this key/i)).toBeNull(); + }); + + it("does NOT decrypt when value is initially empty and later set to enc_*-shaped plaintext", async () => { + // Reason: an empty initial render must still consume the first-load + // budget — otherwise pasting `enc_*`-prefixed plaintext later would + // trigger a stale decrypt and clear the input. + let setV!: (s: string) => void; + function Harness() { + const [v, set] = useState(""); + setV = set; + return ; + } + + let view!: ReturnType; + await act(async () => { + view = render(); + }); + const input = view.container.querySelector("input") as HTMLInputElement; + + await act(async () => { + setV("enc_paste"); + }); + + expect(input.value).toBe("enc_paste"); + expect(mockGetDecryptedKey).not.toHaveBeenCalled(); + }); + + it("does NOT decrypt when autoDecrypt is false even on first mount", async () => { + let view!: ReturnType; + await act(async () => { + view = render(); + }); + + const input = view.container.querySelector("input") as HTMLInputElement; + expect(input.value).toBe("enc_payload"); + expect(mockGetDecryptedKey).not.toHaveBeenCalled(); + }); +}); diff --git a/src/components/ui/password-input.tsx b/src/components/ui/password-input.tsx index 21e196e5..76ee0450 100644 --- a/src/components/ui/password-input.tsx +++ b/src/components/ui/password-input.tsx @@ -2,7 +2,7 @@ import React, { useEffect, useRef, useState } from "react"; import { Input } from "@/components/ui/input"; import { cn } from "@/lib/utils"; import { Eye, EyeOff } from "lucide-react"; -import { getDecryptedKey } from "@/encryptionService"; +import { getDecryptedKey, hasEncryptionPrefix } from "@/encryptionService"; import { logError } from "@/logger"; import { err2String } from "@/utils"; @@ -12,37 +12,82 @@ export function PasswordInput({ placeholder, disabled, className, + autoDecrypt = true, }: { value?: string; onChange?: (value: string) => void; placeholder?: string; disabled?: boolean; className?: string; + /** When false, skip auto-decryption of encrypted values. Use for passphrase inputs. */ + autoDecrypt?: boolean; }) { const [showPassword, setShowPassword] = useState(false); + const [decryptionFailed, setDecryptionFailed] = useState(false); const inputRef = useRef(null); - const isFirstLoad = useRef(true); + // Reason: only attempt decryption on the very first mount of a non-empty + // value. After that, simply mirror the prop into the DOM so a user typing a + // plaintext key that happens to start with `enc_` / `enc_web_` / `enc_desk_` + // is never re-interpreted as ciphertext and silently cleared. Matches + // master's pre-PR behavior (`isFirstLoad` ref), with the failure UX added. + const isFirstLoadRef = useRef(true); - // Initialize the input value on first load + // Reason: useEffect cleanup sets `cancelled = true` whenever `value` changes + // or the component unmounts, preventing stale async decrypt results from + // overwriting the input or calling setState after unmount. useEffect(() => { + let cancelled = false; + const processValue = async () => { - if (isFirstLoad.current && value && inputRef.current) { + const inputEl = inputRef.current; + if (!inputEl) return; + + // Reason: only the first non-empty mount can be a legacy encrypted + // value from settings. Subsequent prop updates are echoes of user + // edits (or external resets to plaintext) and must NEVER trigger + // decryption — see the regression flagged in PR review (a plaintext + // value starting with `enc_` would otherwise be cleared). + const isInitialEncryptedValue = + isFirstLoadRef.current && autoDecrypt && value && hasEncryptionPrefix(value); + + if (isInitialEncryptedValue) { try { - inputRef.current.value = await getDecryptedKey(value); + const decrypted = await getDecryptedKey(value); + if (cancelled) return; + // Reason: an encrypted value that decrypts to "" means the current + // device cannot decrypt it. Show an error instead of ciphertext so + // the user knows to re-enter the key. + if (!decrypted) { + inputEl.value = ""; + setDecryptionFailed(true); + } else { + inputEl.value = decrypted; + setDecryptionFailed(false); + } } catch (error) { + if (cancelled) return; logError("Failed to decrypt value:" + err2String(error)); - inputRef.current.value = value; + inputEl.value = ""; + setDecryptionFailed(true); } - isFirstLoad.current = false; + isFirstLoadRef.current = false; } else { - if (inputRef.current) { - inputRef.current.value = value || ""; - } + inputEl.value = value || ""; + setDecryptionFailed(false); + // Reason: lock out future decryption attempts as soon as we have + // ever rendered a non-encrypted (or empty) value. Without this an + // initial empty render followed by a paste of `enc_*`-prefixed + // plaintext would still get decrypted. + if (value !== undefined) isFirstLoadRef.current = false; } }; void processValue(); - }, [value]); + + return () => { + cancelled = true; + }; + }, [value, autoDecrypt]); return (
@@ -82,6 +127,11 @@ export function PasswordInput({ /> )}
+ {decryptionFailed && ( +

+ Unable to decrypt this key on the current device. Please re-enter the key. +

+ )} ); } diff --git a/src/constants.ts b/src/constants.ts index 394d9d1d..d9a37c00 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -942,7 +942,6 @@ export const DEFAULT_SETTINGS: CopilotSettings = { chatNoteContextTags: [], enableIndexSync: true, debug: false, - enableEncryption: false, maxSourceChunks: DEFAULT_MAX_SOURCE_CHUNKS, enableInlineCitations: true, groqApiKey: "", diff --git a/src/encryptionService.test.ts b/src/encryptionService.test.ts index 5de6d3f0..78b6046b 100644 --- a/src/encryptionService.test.ts +++ b/src/encryptionService.test.ts @@ -19,137 +19,272 @@ window.TextEncoder = TextEncoder; window.TextDecoder = TextDecoder as unknown as typeof window.TextDecoder; // Now we can import our modules -import { encryptAllKeys, getDecryptedKey, getEncryptedKey } from "@/encryptionService"; -import { type CopilotSettings } from "@/settings/model"; -import { Platform } from "obsidian"; +import { getDecryptedKey, isEncryptedValue, isSensitiveKey } from "@/encryptionService"; import { Buffer } from "buffer"; -// Mock window.btoa and window.atob for base64 encoding/decoding -window.btoa = jest.fn().mockImplementation((str: string) => Buffer.from(str).toString("base64")); -window.atob = jest.fn().mockImplementation((str: string) => Buffer.from(str, "base64").toString()); +// Mock btoa/atob for base64 encoding/decoding (binary-safe). jsdom's defaults +// are not binary-safe and break the legacy encrypted-payload round-trips below. +window.btoa = jest + .fn() + .mockImplementation((str: string) => Buffer.from(str, "latin1").toString("base64")); +window.atob = jest + .fn() + .mockImplementation((str: string) => Buffer.from(str, "base64").toString("latin1")); + +/** + * Ensure `window.localStorage` is usable for tests. + */ +function ensureTestLocalStorage(): { clear: () => void } { + try { + const storage = window.localStorage; + storage.setItem("__copilot_test__", "1"); + storage.removeItem("__copilot_test__"); + return { clear: () => window.localStorage.clear() }; + } catch { + const data = new Map(); + const shim = { + getItem: jest.fn((key: string) => data.get(key) ?? null), + setItem: jest.fn((key: string, value: string) => { + data.set(key, value); + }), + removeItem: jest.fn((key: string) => { + data.delete(key); + }), + clear: jest.fn(() => data.clear()), + }; + Object.defineProperty(window, "localStorage", { value: shim, configurable: true }); + return { clear: () => data.clear() }; + } +} + +const testLocalStorage = ensureTestLocalStorage(); + +let randomCounter = 1; +Object.defineProperty(window.crypto, "getRandomValues", { + value: jest.fn((arr: Uint8Array) => { + for (let i = 0; i < arr.length; i++) arr[i] = (randomCounter++ & 0xff); + return arr; + }), + configurable: true, +}); const mockSubtle = { + digest: jest.fn().mockImplementation(() => Promise.resolve(new Uint8Array(32).buffer)), importKey: jest.fn().mockResolvedValue("mockCryptoKey"), encrypt: jest.fn().mockImplementation((algorithm, key, data: ArrayBuffer) => { const originalText = new TextDecoder().decode(data); - const encryptedText = `${originalText}_encrypted`; + const iv = (algorithm as { iv?: Uint8Array })?.iv; + const ivTag = iv ? Array.from(iv.slice(0, 4)).join(",") : "noiv"; + const encryptedText = `${originalText}_encrypted_${ivTag}`; return Promise.resolve(new TextEncoder().encode(encryptedText).buffer); }), decrypt: jest.fn().mockImplementation((algorithm, key, data) => { const encryptedText = new TextDecoder().decode(new Uint8Array(data)); - const originalText = encryptedText.replace("_encrypted", ""); + const originalText = encryptedText.replace(/_encrypted_.+$/, ""); return Promise.resolve(new TextEncoder().encode(originalText).buffer); }), }; -// Mock crypto.subtle instead of the entire crypto object Object.defineProperty(window.crypto, "subtle", { value: mockSubtle, configurable: true, }); -describe("Platform-specific Tests", () => { - it("should recognize the platform as desktop", () => { - expect(Platform.isDesktop).toBe(true); // Directly using the mocked value - }); - - // Example of a conditional test based on the platform - it("should only run certain logic on desktop", () => { - if (Platform.isDesktop) { - // Your desktop-specific logic here - expect(true).toBe(true); // Replace with actual assertions - } else { - expect(true).toBe(false); // This line is just for demonstration and should be replaced - } - }); -}); - describe("EncryptionService", () => { beforeEach(() => { - jest.resetModules(); - }); - - describe("getEncryptedKey (deprecated no-op)", () => { - it("should return plaintext key unchanged", async () => { - const apiKey = "testApiKey"; - const result = await getEncryptedKey(apiKey); - expect(result).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(""); - }); + jest.clearAllMocks(); + randomCounter = 1; + testLocalStorage.clear(); + mockElectron.remote.safeStorage.isEncryptionAvailable.mockReturnValue(true); }); describe("getDecryptedKey", () => { - it("should decrypt an encrypted API key", async () => { - const apiKey = "testApiKey"; - const encryptedKey = await getEncryptedKey(apiKey); - const decryptedKey = await getDecryptedKey(encryptedKey); - expect(decryptedKey).toBe(apiKey); - }); - it("should return the original key if it is in plain text", async () => { const apiKey = "testApiKey"; const decryptedKey = await getDecryptedKey(apiKey); expect(decryptedKey).toBe(apiKey); }); - }); - describe("encryptAllKeys (deprecated no-op)", () => { - it("should return settings unchanged regardless of enableEncryption", async () => { - const settings = { - enableEncryption: true, - openAIApiKey: "testApiKey", - cohereApiKey: "anotherTestApiKey", - userSystemPrompt: "shouldBeIgnored", - } as unknown as CopilotSettings; + it("should decrypt CP00 portable payloads", async () => { + mockElectron.remote.safeStorage.isEncryptionAvailable.mockReturnValue(false); - const result = await encryptAllKeys(settings); - expect(result).toBe(settings); + const plaintext = "legacyPortableSecret"; + const iv = new Uint8Array(12); + iv[0] = 1; + + const ciphertext = new Uint8Array( + await crypto.subtle.encrypt( + { name: "AES-GCM", iv }, + "mockCryptoKey" as unknown as CryptoKey, + new TextEncoder().encode(plaintext) + ) + ); + + const cp00 = new Uint8Array([0x43, 0x50, 0x30, 0x30]); // "CP00" + const payload = new Uint8Array(cp00.length + iv.length + ciphertext.length); + payload.set(cp00, 0); + payload.set(iv, cp00.length); + payload.set(ciphertext, cp00.length + iv.length); + + const base64 = Buffer.from(payload).toString("base64"); + const decrypted = await getDecryptedKey(`enc_web_${base64}`); + expect(decrypted).toBe(plaintext); + }); + + it("should decrypt legacy fixed-IV enc_web_ payloads", async () => { + mockElectron.remote.safeStorage.isEncryptionAvailable.mockReturnValue(false); + + const plaintext = "legacyFixedIvSecret"; + const legacyIv = new Uint8Array(12); + const ciphertext = new Uint8Array( + await crypto.subtle.encrypt( + { name: "AES-GCM", iv: legacyIv }, + "mockCryptoKey" as unknown as CryptoKey, + new TextEncoder().encode(plaintext) + ) + ); + + const base64 = Buffer.from(ciphertext).toString("base64"); + const decrypted = await getDecryptedKey(`enc_web_${base64}`); + expect(decrypted).toBe(plaintext); + }); + + it("should decrypt CP01 per-device payloads when localStorage key exists", async () => { + mockElectron.remote.safeStorage.isEncryptionAvailable.mockReturnValue(false); + + // Reason: CP01 uses a per-device AES key stored in localStorage. + // Seed it so getExistingWebCryptoKey() returns a usable key. + const keyBytes = new Uint8Array(32); + window.localStorage.setItem( + "obsidian-copilot:webcrypto-key:v1", + Buffer.from(keyBytes).toString("base64") + ); + + const plaintext = "cp01DeviceSecret"; + const iv = new Uint8Array(12); + iv[0] = 2; + + const ciphertext = new Uint8Array( + await crypto.subtle.encrypt( + { name: "AES-GCM", iv }, + "mockCryptoKey" as unknown as CryptoKey, + new TextEncoder().encode(plaintext) + ) + ); + + const cp01 = new Uint8Array([0x43, 0x50, 0x30, 0x31]); // "CP01" + const payload = new Uint8Array(cp01.length + iv.length + ciphertext.length); + payload.set(cp01, 0); + payload.set(iv, cp01.length); + payload.set(ciphertext, cp01.length + iv.length); + + const base64 = Buffer.from(payload).toString("base64"); + const decrypted = await getDecryptedKey(`enc_web_${base64}`); + expect(decrypted).toBe(plaintext); + }); + + it("should return empty string for CP01 when localStorage key is missing", async () => { + mockElectron.remote.safeStorage.isEncryptionAvailable.mockReturnValue(false); + // Reason: no key in localStorage — CP01 decryption should fail gracefully. + + const cp01 = new Uint8Array([0x43, 0x50, 0x30, 0x31]); // "CP01" + const iv = new Uint8Array(12); + const fakeCiphertext = new Uint8Array([1, 2, 3, 4]); + const payload = new Uint8Array(cp01.length + iv.length + fakeCiphertext.length); + payload.set(cp01, 0); + payload.set(iv, cp01.length); + payload.set(fakeCiphertext, cp01.length + iv.length); + + const base64 = Buffer.from(payload).toString("base64"); + const result = await getDecryptedKey(`enc_web_${base64}`); + expect(result).toBe(""); + }); + + it("should decrypt legacy enc_ prefix payloads via WebCrypto fallback", async () => { + mockElectron.remote.safeStorage.isEncryptionAvailable.mockReturnValue(false); + + const plaintext = "legacyEncPrefixSecret"; + const legacyIv = new Uint8Array(12); + const ciphertext = new Uint8Array( + await crypto.subtle.encrypt( + { name: "AES-GCM", iv: legacyIv }, + "mockCryptoKey" as unknown as CryptoKey, + new TextEncoder().encode(plaintext) + ) + ); + + const base64 = Buffer.from(ciphertext).toString("base64"); + const decrypted = await getDecryptedKey(`enc_${base64}`); + expect(decrypted).toBe(plaintext); }); }); -}); -describe("Cross-platform compatibility", () => { - let originalConsoleError: typeof console.error; + describe("getDecryptedKey — failure returns empty string", () => { + it("should return empty string when safeStorage is unavailable for desktop key", async () => { + mockElectron.remote.safeStorage.isEncryptionAvailable.mockReturnValue(false); + const result = await getDecryptedKey("enc_desk_" + Buffer.from("test").toString("base64")); + expect(result).toBe(""); + }); - beforeEach(() => { - jest.clearAllMocks(); - // Save original console.error - originalConsoleError = console.error; - // Mock console.error to suppress expected encryption fallback messages - console.error = jest.fn(); + it("should return empty string when WebCrypto decryption throws", async () => { + mockElectron.remote.safeStorage.isEncryptionAvailable.mockReturnValue(false); + const originalDecrypt = mockSubtle.decrypt.getMockImplementation(); + mockSubtle.decrypt.mockImplementation(() => { + throw new Error("decrypt boom"); + }); + + const result = await getDecryptedKey("enc_web_" + Buffer.from("garbage").toString("base64")); + expect(result).toBe(""); + + mockSubtle.decrypt.mockImplementation(originalDecrypt); + }); }); - afterEach(() => { - // Restore original console.error - console.error = originalConsoleError; - }); + describe("isEncryptedValue", () => { + it.each(["enc_desk_abc123", "enc_web_abc123", "enc_abc123"])( + 'should return true for encrypted value "%s"', + (val) => { + expect(isEncryptedValue(val)).toBe(true); + } + ); - 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 decryptedKey = await getDecryptedKey(webEncryptedKey); - expect(decryptedKey).toBe("testApiKey"); - expect(mockSubtle.decrypt).toHaveBeenCalled(); + it.each(["sk-abc123", "plaintext-key", ""])( + 'should return false for non-encrypted value "%s"', + (val) => { + expect(isEncryptedValue(val)).toBe(false); + } + ); + }); +}); + +// --------------------------------------------------------------------------- +// isSensitiveKey (pure function — no mocks needed) +// --------------------------------------------------------------------------- + +describe("isSensitiveKey", () => { + it.each([ + "openAIApiKey", + "cohereApiKey", + "anthropicApiKey", + "plusLicenseKey", + "githubCopilotAccessToken", + "githubCopilotToken", + "clientSecret", + "refreshToken", + "apiPassword", + ])('should return true for sensitive key "%s"', (key) => { + expect(isSensitiveKey(key)).toBe(true); + }); + + it.each([ + "temperature", + "userSystemPrompt", + "defaultModelKey", + "activeModels", + "maxTokens", + "githubCopilotTokenExpiresAt", + "openAIOrgId", + ])('should return false for non-sensitive key "%s"', (key) => { + expect(isSensitiveKey(key)).toBe(false); }); }); diff --git a/src/encryptionService.ts b/src/encryptionService.ts index 8462596e..f056b7bb 100644 --- a/src/encryptionService.ts +++ b/src/encryptionService.ts @@ -1,11 +1,26 @@ +/** + * Encryption/decryption service for Copilot secrets. + * + * After the OS Keychain migration, this module only provides: + * - Decryption of legacy encrypted values (for reading legacy encrypted disk values) + * - `isSensitiveKey()` — canonical field identification + * - `isEncryptedValue()` — prefix detection for encrypted strings + * - Base64 utilities + * + * All encryption-write functions have been removed because secrets + * are now written to the OS keychain directly (plaintext). + */ + +// Reason: do NOT import from @/logger here. The logger depends on getSettings(), +// but this module runs during settings loading (before setSettings). +// Use console.* directly for all logging in this file. + // 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"; interface SafeStorage { @@ -16,116 +31,286 @@ interface SafeStorage { let safeStorageInternal: SafeStorage | null = null; +/** + * Safely get Electron `safeStorage` when available. + * + * @returns The Electron safeStorage instance, or `null` when unavailable. + */ function getSafeStorage(): SafeStorage | null { - if (Platform.isDesktop && safeStorageInternal) { + if (!Platform.isDesktopApp && !Platform.isDesktop) return null; + if (safeStorageInternal) return safeStorageInternal; + try { + // eslint-disable-next-line @typescript-eslint/no-require-imports + safeStorageInternal = require("electron")?.remote?.safeStorage as SafeStorage | null; return safeStorageInternal; + } catch { + return null; } - // Dynamically import electron to access safeStorage - // eslint-disable-next-line @typescript-eslint/no-require-imports - safeStorageInternal = require("electron")?.remote?.safeStorage as SafeStorage | null; - return safeStorageInternal; } -// 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. +// Prefixes to distinguish encryption methods const DESKTOP_PREFIX = "enc_desk_"; const WEBCRYPTO_PREFIX = "enc_web_"; -// Keep old prefix for backward compatibility const ENCRYPTION_PREFIX = "enc_"; const DECRYPTION_PREFIX = "dec_"; -// Add these constants for the Web Crypto implementation -const ENCRYPTION_KEY = new TextEncoder().encode("obsidian-copilot-v1"); -const ALGORITHM = { name: "AES-GCM", iv: new Uint8Array(12) }; +/** + * @deprecated getDecryptedKey() now returns "" on failure instead of this sentinel. + * Kept only for backward compatibility with code that may still reference it. + */ +export const DECRYPTION_FAILURE_MESSAGE = "Copilot failed to decrypt API keys!"; -async function getEncryptionKey(): Promise { - return await crypto.subtle.importKey("raw", ENCRYPTION_KEY, ALGORITHM.name, false, [ - "encrypt", - "decrypt", - ]); +/** + * Check whether a value looks like a well-formed encrypted Copilot secret + * (recognized prefix + plausible base64 payload). + * + * Use this when the caller wants strict detection — for example, deciding + * whether to display a value as decrypt-able in a UI. Returns false for + * values whose prefix matches but whose payload is corrupted, so callers + * that must protect such values during persistence should use + * `hasEncryptionPrefix()` instead. + */ +export function isEncryptedValue(value: string): boolean { + if (value.startsWith(DESKTOP_PREFIX)) { + return looksLikeBase64(value.slice(DESKTOP_PREFIX.length)); + } + if (value.startsWith(WEBCRYPTO_PREFIX)) { + return looksLikeBase64(value.slice(WEBCRYPTO_PREFIX.length)); + } + if (value.startsWith(ENCRYPTION_PREFIX)) { + return looksLikeBase64(value.slice(ENCRYPTION_PREFIX.length)); + } + return false; } /** - * 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. + * Permissive sibling of {@link isEncryptedValue}: returns true for any + * value carrying a Copilot encryption prefix, regardless of whether the + * payload still looks like valid base64. + * + * Reason: persistence code must treat "prefix matches but payload is + * corrupted" as encrypted-and-failed. Otherwise the keychain write path + * would treat a corrupted `enc_*` string as plaintext and silently + * overwrite the keychain with junk — and the disk-strip path would drop + * the only remaining copy. See `stripSecretsPreservingFailed`. */ -export async function encryptAllKeys( - settings: Readonly -): Promise> { - return settings; +export function hasEncryptionPrefix(value: string): boolean { + return ( + value.startsWith(DESKTOP_PREFIX) || + value.startsWith(WEBCRYPTO_PREFIX) || + value.startsWith(ENCRYPTION_PREFIX) + ); } /** - * No-op: encryption-at-rest is deprecated. Strips the `dec_` marker if - * present and returns the raw plaintext key. + * Check whether a string is plausibly base64-encoded ciphertext. */ -export async function getEncryptedKey(apiKey: string): Promise { - if (!apiKey) return apiKey; - return isDecrypted(apiKey) ? apiKey.replace(DECRYPTION_PREFIX, "") : apiKey; +function looksLikeBase64(data: string): boolean { + if (!data) return false; + if (data.length % 4 === 1) return false; + return /^[A-Za-z0-9+/]+={0,2}$/.test(data); } -export async function getDecryptedKey(apiKey: string): Promise { - if (!apiKey || isPlainText(apiKey)) { - return apiKey; - } - if (isDecrypted(apiKey)) { - return apiKey.replace(DECRYPTION_PREFIX, ""); - } +// --------------------------------------------------------------------------- +// WebCrypto: decrypt-only support for legacy formats +// --------------------------------------------------------------------------- - // 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); - } +const WEBCRYPTO_IV_LENGTH = 12; +const WEBCRYPTO_KEY_STORAGE_KEY = "obsidian-copilot:webcrypto-key:v1"; +/** Magic header for portable payloads (hardcoded key + random IV). */ +const WEBCRYPTO_PORTABLE_MAGIC = new Uint8Array([0x43, 0x50, 0x30, 0x30]); // "CP00" +/** Magic header for legacy per-device payloads (kept for backward-compat decryption). */ +const WEBCRYPTO_DEVICE_MAGIC = new Uint8Array([0x43, 0x50, 0x30, 0x31]); // "CP01" - if (apiKey.startsWith(WEBCRYPTO_PREFIX)) { - const base64Data = apiKey.replace(WEBCRYPTO_PREFIX, ""); - const key = await getEncryptionKey(); - const encryptedData = base64ToArrayBuffer(base64Data); - const decryptedData = await crypto.subtle.decrypt(ALGORITHM, key, encryptedData); - return new TextDecoder().decode(decryptedData); - } +/** + * Portable AES-GCM key material shared across all devices. + * Reason: derives a 256-bit AES key via SHA-256. + */ +const PORTABLE_WEBCRYPTO_KEY_MATERIAL = new TextEncoder().encode("obsidian-copilot-v1"); +const LEGACY_WEBCRYPTO_IV = new Uint8Array(WEBCRYPTO_IV_LENGTH); - // Legacy support for old enc_ prefix - const base64Data = apiKey.replace(ENCRYPTION_PREFIX, ""); +function assertWebCryptoAvailable(): void { + if (!window.crypto?.subtle) { + throw new Error("WebCrypto API is not available in this environment."); + } +} + +function getLocalStorageSafe(): Storage | null { try { - // Try desktop decryption first + return window.localStorage ?? null; + } catch { + return null; + } +} + +/** + * Read an existing per-device AES-256 key from localStorage. + * Reason: kept for backward-compat decryption of CP01 payloads. + */ +async function getExistingWebCryptoKey(): Promise { + try { + const storage = getLocalStorageSafe(); + if (!storage) return null; + const existing = storage.getItem(WEBCRYPTO_KEY_STORAGE_KEY); + if (!existing) return null; + const raw = base64ToArrayBuffer(existing); + return await crypto.subtle.importKey("raw", raw, "AES-GCM", false, ["encrypt", "decrypt"]); + } catch (error) { + console.warn("Failed to load WebCrypto key from localStorage.", error); + return null; + } +} + +/** + * Import the portable WebCrypto key for CP00 decryption + * and legacy fixed-IV decryption. + */ +async function getPortableWebCryptoKey(): Promise { + const keyBytes = await crypto.subtle.digest("SHA-256", PORTABLE_WEBCRYPTO_KEY_MATERIAL); + return await crypto.subtle.importKey("raw", keyBytes, "AES-GCM", false, ["encrypt", "decrypt"]); +} + +function hasWebCryptoMagic(bytes: Uint8Array, magic: Uint8Array): boolean { + if (bytes.length < magic.length) return false; + for (let i = 0; i < magic.length; i++) { + if (bytes[i] !== magic[i]) return false; + } + return true; +} + +/** + * Decrypt a WEBCRYPTO_PREFIX value. + * Supports portable CP00, legacy per-device CP01, and legacy fixed-IV formats. + */ +async function decryptWebCryptoValue(base64Data: string): Promise { + assertWebCryptoAvailable(); + const raw = new Uint8Array(base64ToArrayBuffer(base64Data)); + + // Portable format: [CP00][IV][CIPHERTEXT] + if (hasWebCryptoMagic(raw, WEBCRYPTO_PORTABLE_MAGIC)) { + const ivStart = WEBCRYPTO_PORTABLE_MAGIC.length; + const ivEnd = ivStart + WEBCRYPTO_IV_LENGTH; + if (raw.length <= ivEnd) throw new Error("Invalid WebCrypto portable payload: too short."); + + const iv = raw.slice(ivStart, ivEnd); + const ciphertext = raw.slice(ivEnd); + const key = await getPortableWebCryptoKey(); + const decrypted = await crypto.subtle.decrypt({ name: "AES-GCM", iv }, key, ciphertext); + return new TextDecoder().decode(decrypted); + } + + // Legacy per-device format: [CP01][IV][CIPHERTEXT] + if (hasWebCryptoMagic(raw, WEBCRYPTO_DEVICE_MAGIC)) { + const ivStart = WEBCRYPTO_DEVICE_MAGIC.length; + const ivEnd = ivStart + WEBCRYPTO_IV_LENGTH; + if (raw.length <= ivEnd) throw new Error("Invalid WebCrypto CP01 payload: too short."); + + const iv = raw.slice(ivStart, ivEnd); + const ciphertext = raw.slice(ivEnd); + const key = await getExistingWebCryptoKey(); + if (!key) throw new Error("Per-device WebCrypto key unavailable for CP01 decryption."); + + const decrypted = await crypto.subtle.decrypt({ name: "AES-GCM", iv }, key, ciphertext); + return new TextDecoder().decode(decrypted); + } + + // Legacy format: ciphertext only, fixed IV + hardcoded key + const key = await getPortableWebCryptoKey(); + const decrypted = await crypto.subtle.decrypt( + { name: "AES-GCM", iv: LEGACY_WEBCRYPTO_IV }, + key, + base64ToArrayBuffer(base64Data) + ); + return new TextDecoder().decode(decrypted); +} + +// --------------------------------------------------------------------------- +// Public API +// --------------------------------------------------------------------------- + +/** + * Check whether a top-level settings key holds a sensitive value. + * This is the single source of truth for sensitive field identification. + */ +export function isSensitiveKey(key: string): boolean { + const lower = key.toLowerCase(); + const normalized = lower.replace(/[_-]/g, ""); + return ( + normalized.includes("apikey") || + lower.endsWith("token") || + lower.endsWith("accesstoken") || + lower.endsWith("secret") || + lower.endsWith("password") || + lower.endsWith("licensekey") + ); +} + +/** + * Decrypt an API key from any supported encryption format. + * + * Returns the plaintext value, or `""` on failure. + * For plaintext input, returns the value as-is. + */ +export async function getDecryptedKey(apiKey: string): Promise { + try { + if (!apiKey || isPlainText(apiKey)) { + return apiKey; + } + if (isDecrypted(apiKey)) { + return apiKey.replace(DECRYPTION_PREFIX, ""); + } + + if (apiKey.startsWith(DESKTOP_PREFIX)) { + const safeStorage = getSafeStorage(); + if (!safeStorage?.isEncryptionAvailable()) { + console.warn("Cannot decrypt desktop-encrypted key: safeStorage unavailable."); + return ""; + } + const base64Data = apiKey.replace(DESKTOP_PREFIX, ""); + const buffer = Buffer.from(base64Data, "base64"); + return safeStorage.decryptString(buffer); + } + + if (apiKey.startsWith(WEBCRYPTO_PREFIX)) { + const base64Data = apiKey.replace(WEBCRYPTO_PREFIX, ""); + return await decryptWebCryptoValue(base64Data); + } + + // Legacy enc_ prefix + const base64Data = apiKey.replace(ENCRYPTION_PREFIX, ""); if (getSafeStorage()?.isEncryptionAvailable()) { try { const buffer = Buffer.from(base64Data, "base64"); return getSafeStorage()!.decryptString(buffer); } catch { - // Silent catch is intentional - if desktop decryption fails, - // it means this key was likely encrypted with Web Crypto. - // We'll fall through to the Web Crypto decryption below. + // Fall through to WebCrypto } } - // Fallback to Web Crypto API - const key = await getEncryptionKey(); - const encryptedData = base64ToArrayBuffer(base64Data); - const decryptedData = await crypto.subtle.decrypt(ALGORITHM, key, encryptedData); - return new TextDecoder().decode(decryptedData); + return await decryptWebCryptoValue(base64Data); } catch (err) { console.error("Decryption failed:", err); - return "Copilot failed to decrypt API keys!"; + return ""; } } +/** + * Decrypt an API key and throw when decryption fails. + * Use in workflows that must not proceed with undecryptable values + * (e.g., configuration export). + */ +export async function getDecryptedKeyOrThrow(apiKey: string): Promise { + const decrypted = await getDecryptedKey(apiKey); + if (!decrypted) { + throw new Error("Failed to decrypt API key."); + } + return decrypted; +} + +// --------------------------------------------------------------------------- +// Internal helpers +// --------------------------------------------------------------------------- + function isPlainText(key: string): boolean { return !key.startsWith(ENCRYPTION_PREFIX) && !key.startsWith(DECRYPTION_PREFIX); } diff --git a/src/logFileManager.ts b/src/logFileManager.ts index a1c03f2c..f4670595 100644 --- a/src/logFileManager.ts +++ b/src/logFileManager.ts @@ -2,6 +2,7 @@ import { err2String } from "@/errorFormat"; import { TFile } from "obsidian"; import { ensureFolderExists } from "@/utils"; import { getSettings } from "@/settings/model"; +import { isSensitiveKey } from "@/encryptionService"; type LogLevel = "INFO" | "WARN" | "ERROR"; @@ -180,12 +181,10 @@ class LogFileManager { const obj = value as Record; for (const [key, val] of Object.entries(obj)) { - // Skip API keys, license keys, and infrastructure identifiers + // Skip sensitive fields (API keys, tokens, secrets, passwords, license keys) + // and infrastructure identifiers that may leak deployment details if ( - /apiKey$/i.test(key) || - /licenseKey$/i.test(key) || - /_api_key$/i.test(key) || - /_license_key$/i.test(key) || + isSensitiveKey(key) || /orgId$/i.test(key) || /instanceName$/i.test(key) || /deploymentName$/i.test(key) || diff --git a/src/main.ts b/src/main.ts index bebe6e6e..90de2034 100644 --- a/src/main.ts +++ b/src/main.ts @@ -23,6 +23,12 @@ import { ChatManager } from "@/core/ChatManager"; import { MessageRepository } from "@/core/MessageRepository"; import { logError, logInfo, logWarn } from "@/logger"; import { logFileManager } from "@/logFileManager"; +import { KeychainService } from "@/services/keychainService"; +import { + persistSettings, + loadSettingsWithKeychain, + flushPersistence, +} from "@/services/settingsPersistence"; import { UserMemoryManager } from "@/memory/UserMemoryManager"; import { clearRecordedPromptPayload } from "@/LLMProviders/chainRunner/utils/promptPayloadRecorder"; import { checkIsPlusUser, refreshSelfHostModeValidation } from "@/plusUtils"; @@ -36,10 +42,8 @@ import { CopilotSettingTab } from "@/settings/SettingsPage"; import { getModelKeyFromModel, getSettings, - sanitizeSettings, setSettings, subscribeToSettingsChange, - type CopilotSettings, } from "@/settings/model"; import { ChatUIState } from "@/state/ChatUIState"; import { VaultDataManager } from "@/state/vaultDataAtoms"; @@ -100,15 +104,23 @@ export default class CopilotPlugin extends Plugin { private lastSelectionSignature?: string; private webSelectionTracker?: WebSelectionTracker; private readonly chatHistoryLastAccessedAtManager = new RecentUsageManager(); - async onload(): Promise { + KeychainService.getInstance(this.app); await this.loadSettings(); this.settingsUnsubscriber = subscribeToSettingsChange((prev, next) => { void (async () => { - // 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); + try { + await persistSettings(next, (data) => this.saveData(data), prev); + } catch (error) { + // Reason: Do NOT rollback memory state on persist failure. + // The writeQueue serializes I/O, so a later setSettings() may already + // be queued. Rolling back memory would create a split where memory is S0 + // but disk ends up at S2 when the later write succeeds. + // Instead, just notify the user — the in-memory state remains current, + // and the next successful persist will reconcile disk with memory. + logError("Failed to persist settings.", error); + new Notice("Copilot failed to save settings. Check logs and try again."); + } registerCommands(this, prev, next); })(); }); @@ -245,6 +257,11 @@ export default class CopilotPlugin extends Plugin { } async onunload() { + // Best-effort flush of pending keychain/data.json writes. + // Reason: onunload() is void in Obsidian's type system, but awaiting here + // is no worse than fire-and-forget, and consistent with the log flush below. + await flushPersistence(); + // Clear all persistent selection highlights before unload // This prevents "stuck" highlights after hot reload (dev environment) this.clearAllPersistentSelectionHighlights(); @@ -617,9 +634,9 @@ export default class CopilotPlugin extends Plugin { } async loadSettings() { - const savedSettings = (await this.loadData()) as CopilotSettings; - const sanitizedSettings = sanitizeSettings(savedSettings); - setSettings(sanitizedSettings); + const rawData = (await this.loadData()) as unknown; + const settings = await loadSettingsWithKeychain(rawData, (d) => this.saveData(d)); + setSettings(settings); } mergeActiveModels( diff --git a/src/services/keychainService.test.ts b/src/services/keychainService.test.ts new file mode 100644 index 00000000..1b411568 --- /dev/null +++ b/src/services/keychainService.test.ts @@ -0,0 +1,613 @@ +jest.mock("obsidian", () => { + class FileSystemAdapter { + private readonly _basePath: string; + constructor(basePath = "/vault/default") { + this._basePath = basePath; + } + getBasePath(): string { + return this._basePath; + } + } + + return { + App: class App {}, + SecretStorage: class SecretStorage {}, + FileSystemAdapter, + Notice: jest.fn(), + }; +}); + +jest.mock("@/utils/hash", () => ({ + md5: jest.fn((value: string) => { + // Reason: deterministic fake hash for test assertions. + // Uses a simple char-code sum to produce reproducible 32-char hex strings. + let sum = 0; + for (let i = 0; i < value.length; i++) sum += value.charCodeAt(i); + return sum.toString(16).padStart(32, "0"); + }), +})); + +jest.mock("@/encryptionService", () => ({ + isSensitiveKey: jest.fn((key: string) => { + const lower = key.toLowerCase(); + const normalized = lower.replace(/[_-]/g, ""); + return ( + normalized.includes("apikey") || + lower.endsWith("token") || + lower.endsWith("accesstoken") || + lower.endsWith("secret") || + lower.endsWith("password") || + lower.endsWith("licensekey") + ); + }), + getDecryptedKey: jest.fn(async (value: string) => value.replace(/^enc_/, "")), +})); + +jest.mock("@/settings/model", () => { + const actual = jest.requireActual("@/settings/model"); + return { + ...actual, + getSettings: jest.fn(), + }; +}); + +jest.mock("@/services/settingsSecretTransforms", () => ({ + MODEL_SECRET_FIELDS: ["apiKey"] as const, + // Reason: stub the canonical secret-field list used by hydrateFromKeychain. + // Keep it minimal so tests targeting a single field don't accidentally + // trigger hydration for every default provider. + TOP_LEVEL_SECRET_FIELDS: ["openAIApiKey"] as const, + stripKeychainFields: jest.fn((settings: Record) => { + const out = { ...settings }; + // Reason: mirror the real isSensitiveKey heuristic for top-level fields + for (const key of Object.keys(out)) { + const lower = key.toLowerCase(); + const normalized = lower.replace(/[_-]/g, ""); + const isSensitive = + normalized.includes("apikey") || + lower.endsWith("token") || + lower.endsWith("accesstoken") || + lower.endsWith("secret") || + lower.endsWith("password") || + lower.endsWith("licensekey"); + if (isSensitive) out[key] = ""; + } + if (Array.isArray(out.activeModels)) { + out.activeModels = (out.activeModels as Array>).map((m) => ({ + ...m, + apiKey: "", + })); + } + if (Array.isArray(out.activeEmbeddingModels)) { + out.activeEmbeddingModels = (out.activeEmbeddingModels as Array>).map( + (m) => ({ ...m, apiKey: "" }) + ); + } + return out; + }), + cleanupLegacyFields: jest.fn((settings: Record) => ({ ...settings })), + // Reason: mirrors the production helper so the stranded-vault guard in + // forgetAllSecrets sees the same truth as the rest of the codebase. + isKeychainOnly: jest.fn((settings: Record) => settings._keychainOnly === true), +})); + +import { FileSystemAdapter, Notice, type App } from "obsidian"; +import { getSettings } from "@/settings/model"; +import type { CopilotSettings } from "@/settings/model"; +import type { CustomModel } from "@/aiParams"; +import { KeychainService, isSecretKey } from "./keychainService"; + +/** Build a lightweight settings object. */ +function makeSettings(overrides: Partial = {}): CopilotSettings { + return { + activeModels: [], + activeEmbeddingModels: [], + ...overrides, + } as unknown as CopilotSettings; +} + +/** Build a minimal custom model. */ +function makeModel(overrides: Partial = {}): CustomModel { + return { + name: "gpt-4", + provider: "openai", + enabled: true, + ...overrides, + }; +} + +/** Fake SecretStorage with controllable Jest spies. */ +function makeSecretStorage() { + return { + getSecret: jest.fn().mockReturnValue(null), + setSecret: jest.fn(), + deleteSecret: jest.fn(), + listSecrets: jest.fn().mockReturnValue([]), + }; +} + +/** Create a FileSystemAdapter mock with a given basePath. */ +function makeAdapter(basePath: string) { + const adapter = new FileSystemAdapter(); + // Reason: override the default basePath from the mock constructor + (adapter as unknown as { _basePath: string })._basePath = basePath; + return adapter; +} + +/** Create a minimal Obsidian app shape for KeychainService. */ +function makeApp(options?: { + basePath?: string; + adapter?: unknown; + /** + * Reason: distinguish "default to fake storage" from "explicitly omit the + * field". Production code reads `app.secretStorage` and falsy means the + * runtime lacks the API entirely — the `??` short-circuit had hidden + * that case in tests. + * - omit the key → falls back to the fake storage (most tests) + * - secretStorage: null → simulate an Obsidian build without SecretStorage + */ + secretStorage?: ReturnType | null; +}) { + const basePath = options?.basePath ?? "/Users/test/MyVault"; + const hasExplicitStorage = options !== undefined && "secretStorage" in options; + const secretStorage = hasExplicitStorage ? options.secretStorage : makeSecretStorage(); + return { + vault: { + adapter: options?.adapter ?? makeAdapter(basePath), + getName: jest.fn().mockReturnValue("MyVault"), + // Reason: any non-empty string works here — the production code resolves + // `app.vault.configDir` rather than hardcoding ".obsidian", and the lint + // rule `obsidianmd/hardcoded-config-path` flags ".obsidian" specifically. + configDir: "test-config-dir", + }, + secretStorage, + } as unknown as App; +} + +beforeEach(() => { + jest.clearAllMocks(); + KeychainService.resetInstance(); +}); + +// --------------------------------------------------------------------------- +// isSecretKey +// --------------------------------------------------------------------------- + +describe("isSecretKey", () => { + it.each(["openAIApiKey", "googleApiKey", "githubCopilotToken", "plusLicenseKey", "myPassword"])( + "returns true for %s", + (key) => { + expect(isSecretKey(key)).toBe(true); + } + ); + + it.each(["temperature", "defaultModelKey", "userId"])("returns false for %s", (key) => { + expect(isSecretKey(key)).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- +// Vault ID generation +// --------------------------------------------------------------------------- + +describe("vault ID generation", () => { + it("produces a deterministic 8-char hex ID from desktop vault path", () => { + const service = KeychainService.getInstance(makeApp({ basePath: "/Users/test/MyVault" })); + const id = service.getVaultId(); + + expect(id).toHaveLength(8); + expect(/^[0-9a-f]{8}$/.test(id)).toBe(true); + }); + + it("produces a stable ID across multiple calls", () => { + const service = KeychainService.getInstance(makeApp({ basePath: "/Users/test/MyVault" })); + expect(service.getVaultId()).toBe(service.getVaultId()); + }); + + it("falls back to vault name + configDir when no base path is available", () => { + const service = KeychainService.getInstance(makeApp({ adapter: {} })); + const id = service.getVaultId(); + + expect(id).toHaveLength(8); + expect(/^[0-9a-f]{8}$/.test(id)).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- +// hydrateFromKeychain — read-only keychain hydration +// --------------------------------------------------------------------------- + +describe("hydrateFromKeychain", () => { + it("honors a keychain tombstone by zeroing the in-memory field", async () => { + const secretStorage = makeSecretStorage(); + secretStorage.getSecret.mockReturnValue(""); + const service = KeychainService.getInstance(makeApp({ secretStorage })); + + const result = await service.hydrateFromKeychain( + makeSettings({ openAIApiKey: "leftover-from-disk" }) + ); + + expect(result.settings.openAIApiKey).toBe(""); + expect(result.hadFailures).toBe(false); + // Reason: hydrateFromKeychain is strictly read-only. + expect(secretStorage.setSecret).not.toHaveBeenCalled(); + }); + + it("replaces the in-memory value with the keychain value when one exists", async () => { + const secretStorage = makeSecretStorage(); + secretStorage.getSecret.mockReturnValue("kc-value"); + const service = KeychainService.getInstance(makeApp({ secretStorage })); + + const result = await service.hydrateFromKeychain( + makeSettings({ openAIApiKey: "stale-disk-value" }) + ); + + expect(result.settings.openAIApiKey).toBe("kc-value"); + expect(secretStorage.setSecret).not.toHaveBeenCalled(); + }); + + it("leaves the field as-is when the keychain has no entry (null)", async () => { + const secretStorage = makeSecretStorage(); + secretStorage.getSecret.mockReturnValue(null); + const service = KeychainService.getInstance(makeApp({ secretStorage })); + + const result = await service.hydrateFromKeychain(makeSettings({ openAIApiKey: "" })); + + expect(result.settings.openAIApiKey).toBe(""); + expect(result.hadFailures).toBe(false); + expect(secretStorage.setSecret).not.toHaveBeenCalled(); + }); + + it("marks hadFailures and skips the field when a keychain read throws", async () => { + const secretStorage = makeSecretStorage(); + secretStorage.getSecret.mockImplementation(() => { + throw new Error("locked"); + }); + const service = KeychainService.getInstance(makeApp({ secretStorage })); + + const result = await service.hydrateFromKeychain(makeSettings({ openAIApiKey: "" })); + + expect(result.hadFailures).toBe(true); + expect(secretStorage.setSecret).not.toHaveBeenCalled(); + }); + + it("hydrates model-level apiKey values from the keychain", async () => { + const secretStorage = makeSecretStorage(); + secretStorage.getSecret.mockReturnValue("kc-model-key"); + const service = KeychainService.getInstance(makeApp({ secretStorage })); + + const result = await service.hydrateFromKeychain( + makeSettings({ + activeModels: [makeModel({ name: "gpt-4", provider: "openai", apiKey: "" })], + }) + ); + + expect(result.settings.activeModels[0].apiKey).toBe("kc-model-key"); + expect(secretStorage.setSecret).not.toHaveBeenCalled(); + }); + + it("hydrates canonical top-level secret fields even when missing from input settings", async () => { + // Reason: covers the partial-settings scenario — data.json from cross-version + // sync, downgrade-then-upgrade, or manual edits may omit some secret fields, + // but a corresponding keychain entry can still exist on this device. Hydrate + // must iterate the canonical field set, not just Object.keys(settings). + const secretStorage = makeSecretStorage(); + secretStorage.getSecret.mockImplementation((id: string) => + id.endsWith("open-a-i-api-key") ? "sk-recovered" : null + ); + const service = KeychainService.getInstance(makeApp({ secretStorage })); + + // Note: makeSettings() intentionally does NOT seed openAIApiKey on the input. + const result = await service.hydrateFromKeychain(makeSettings()); + + expect((result.settings as unknown as Record).openAIApiKey).toBe( + "sk-recovered" + ); + expect(secretStorage.setSecret).not.toHaveBeenCalled(); + }); + + it("still hydrates legacy secret keys present on input but not in DEFAULT_SETTINGS", async () => { + // Reason: deprecated fields may have been removed from DEFAULT_SETTINGS yet + // remain in a user's data.json with a live keychain entry. The union of + // canonical fields + in-memory secret-shaped keys keeps them readable so + // upgrading never silently drops a key. + const secretStorage = makeSecretStorage(); + secretStorage.getSecret.mockImplementation((id: string) => + id.includes("legacy-provider-api-key") ? "legacy-value" : null + ); + const service = KeychainService.getInstance(makeApp({ secretStorage })); + + const result = await service.hydrateFromKeychain( + makeSettings({ legacyProviderApiKey: "" } as unknown as Partial) + ); + + expect((result.settings as unknown as Record).legacyProviderApiKey).toBe( + "legacy-value" + ); + }); +}); + +// --------------------------------------------------------------------------- +// persistSecrets +// --------------------------------------------------------------------------- + +describe("persistSecrets", () => { + it("collects current secrets and tombstones cleared or deleted IDs", () => { + const service = KeychainService.getInstance(makeApp()); + + const current = makeSettings({ + openAIApiKey: "sk-current", + googleApiKey: "", + activeModels: [makeModel({ name: "kept", provider: "openai", apiKey: "chat-secret" })], + activeEmbeddingModels: [], + }); + + const prev = makeSettings({ + openAIApiKey: "sk-prev", + googleApiKey: "g-prev", + activeModels: [ + makeModel({ name: "kept", provider: "openai", apiKey: "chat-prev" }), + makeModel({ name: "deleted", provider: "openai", apiKey: "del-secret" }), + ], + activeEmbeddingModels: [ + makeModel({ name: "del-embed", provider: "openai", apiKey: "embed-secret" }), + ], + }); + + const result = service.persistSecrets(current, prev); + + // Reason: should collect the current openAIApiKey and the kept model's apiKey + const entryIds = result.secretEntries.map(([id]) => id); + expect(entryIds.some((id) => id.includes("open-a-i-api-key"))).toBe(true); + expect(entryIds.some((id) => id.includes("model-api-key-chat"))).toBe(true); + + // Reason: should mark deleted models and cleared googleApiKey for tombstone + expect(result.keychainIdsToDelete.some((id) => id.includes("google-api-key"))).toBe(true); + expect(result.keychainIdsToDelete.some((id) => id.includes("model-api-key-chat"))).toBe(true); + expect(result.keychainIdsToDelete.some((id) => id.includes("model-api-key-embedding"))).toBe( + true + ); + + // Reason: persistSecrets must not mutate the input settings objects + expect(current.openAIApiKey).toBe("sk-current"); + expect(current.activeModels[0].apiKey).toBe("chat-secret"); + expect(prev.openAIApiKey).toBe("sk-prev"); + expect(prev.activeModels[0].apiKey).toBe("chat-prev"); + }); +}); + +// --------------------------------------------------------------------------- +// forgetAllSecrets +// --------------------------------------------------------------------------- + +describe("forgetAllSecrets", () => { + it("clears vault secrets, strips settings, and notifies the user", async () => { + const secretStorage = makeSecretStorage(); + const service = KeychainService.getInstance(makeApp({ secretStorage })); + const vaultId = service.getVaultId(); + + // Reason: listSecrets returns IDs for this vault and one from another vault + secretStorage.listSecrets.mockReturnValue([ + `copilot-v${vaultId}-open-a-i-api-key`, + "copilot-vother000-google-api-key", + ]); + + (getSettings as jest.Mock).mockReturnValue( + makeSettings({ + openAIApiKey: "sk-123", + activeModels: [makeModel({ apiKey: "model-secret" })], + }) + ); + + const saveData = jest.fn().mockResolvedValue(undefined); + const refreshDiskState = jest.fn(); + const syncMemory = jest.fn(); + + await service.forgetAllSecrets(saveData, refreshDiskState, syncMemory); + + // Reason: should only delete entries for THIS vault, not other vaults + expect(secretStorage.deleteSecret).toHaveBeenCalledWith(`copilot-v${vaultId}-open-a-i-api-key`); + expect(secretStorage.deleteSecret).not.toHaveBeenCalledWith("copilot-vother000-google-api-key"); + + // Reason: should save stripped settings to disk with secrets blanked + expect(saveData).toHaveBeenCalled(); + const saved = saveData.mock.calls[0][0] as unknown as Record; + expect(saved._keychainOnly).toBe(true); + expect(saved.openAIApiKey).toBe(""); + const savedModels = saved.activeModels as Array>; + expect(savedModels[0].apiKey).toBe(""); + + expect(refreshDiskState).toHaveBeenCalled(); + expect(syncMemory).toHaveBeenCalled(); + // Reason: synced memory should also have secrets blanked + const synced = syncMemory.mock.calls[0][0] as unknown as Record; + expect(synced.openAIApiKey).toBe(""); + const syncedModels = synced.activeModels as Array>; + expect(syncedModels[0].apiKey).toBe(""); + expect(Notice).toHaveBeenCalledWith( + "All API keys for this vault removed. Please re-enter them." + ); + }); + + it("handles saveData failure gracefully — keychain NOT cleared", async () => { + const secretStorage = makeSecretStorage(); + const service = KeychainService.getInstance(makeApp({ secretStorage })); + secretStorage.listSecrets.mockReturnValue([]); + + (getSettings as jest.Mock).mockReturnValue(makeSettings({ openAIApiKey: "sk-123" })); + + const saveData = jest.fn().mockRejectedValue(new Error("disk write failed")); + const refreshDiskState = jest.fn(); + const syncMemory = jest.fn(); + const onDiskSaveFailed = jest.fn(); + + await service.forgetAllSecrets(saveData, refreshDiskState, syncMemory, onDiskSaveFailed); + + // Reason: disk failed → abort before keychain clear + expect(secretStorage.deleteSecret).not.toHaveBeenCalled(); + expect(syncMemory).not.toHaveBeenCalled(); + expect(onDiskSaveFailed).toHaveBeenCalled(); + expect(refreshDiskState).not.toHaveBeenCalled(); + expect(Notice).toHaveBeenCalledWith( + expect.stringContaining("Failed to remove API keys from data.json") + ); + }); + + it("propagates keychain delete failures after successful disk save so the user can retry", async () => { + const secretStorage = makeSecretStorage(); + const service = KeychainService.getInstance(makeApp({ secretStorage })); + const vaultId = service.getVaultId(); + + const idA = `copilot-v${vaultId}-open-a-i-api-key`; + const idB = `copilot-v${vaultId}-google-api-key`; + secretStorage.listSecrets.mockReturnValue([idA, idB]); + + // Reason: simulate partial failure — first delete succeeds, second throws. + secretStorage.deleteSecret.mockImplementation((id: string) => { + if (id === idB) throw new Error("keychain locked"); + }); + + (getSettings as jest.Mock).mockReturnValue(makeSettings({ openAIApiKey: "sk-123" })); + + const saveData = jest.fn().mockResolvedValue(undefined); + const refreshDiskState = jest.fn(); + const syncMemory = jest.fn(); + const onDiskSaveFailed = jest.fn(); + + await expect( + service.forgetAllSecrets(saveData, refreshDiskState, syncMemory, onDiskSaveFailed) + ).rejects.toThrow(/Failed to clear 1 keychain/); + + // Reason: disk save succeeds first (new ordering), then keychain clear fails. + expect(saveData).toHaveBeenCalled(); + expect(refreshDiskState).toHaveBeenCalled(); + expect(secretStorage.deleteSecret).toHaveBeenCalledWith(idA); + // Reason: memory MUST be synced even on partial keychain failure, otherwise + // the next normal persist would write old secrets back from stale memory. + expect(syncMemory).toHaveBeenCalled(); + expect(onDiskSaveFailed).not.toHaveBeenCalled(); + }); + + it("does NOT flip a disk-mode vault into keychain-only when Secure Storage is unavailable", async () => { + // Reason: on Obsidian builds without `secretStorage`, "Delete All Keys" + // must not write `_keychainOnly: true` to disk. Otherwise the next save + // takes the stranded path and silently strips any newly entered key, + // bricking auth setup on older builds with one click. + const service = KeychainService.getInstance(makeApp({ secretStorage: null })); + expect(service.isAvailable()).toBe(false); + + (getSettings as jest.Mock).mockReturnValue(makeSettings({ openAIApiKey: "sk-disk" })); + + const saveData = jest.fn().mockResolvedValue(undefined); + const refreshDiskState = jest.fn(); + const syncMemory = jest.fn(); + + await service.forgetAllSecrets(saveData, refreshDiskState, syncMemory); + + const saved = saveData.mock.calls[0][0] as unknown as Record; + // Reason: secrets are still cleared from data.json — the user did ask to delete them. + expect(saved.openAIApiKey).toBe(""); + // Reason: but the vault stays in disk mode so future key entries can persist. + expect(saved._keychainOnly).toBeUndefined(); + const synced = syncMemory.mock.calls[0][0] as unknown as Record; + expect(synced._keychainOnly).toBeUndefined(); + }); + + it("refuses to run on a stranded vault and never touches disk/memory", async () => { + // Reason: in a stranded vault (keychain-only + no SecretStorage) we can't + // actually reach the OS keychain to clear its entries. If we still stripped + // disk and cleared memory, the user would see a success Notice but the + // keychain entries would survive — and reappear after upgrading Obsidian + // (or moving to a SecretStorage-capable build). Refuse up-front instead so + // the destructive contract stays honest. + const service = KeychainService.getInstance(makeApp({ secretStorage: null })); + + (getSettings as jest.Mock).mockReturnValue( + makeSettings({ openAIApiKey: "", _keychainOnly: true }) + ); + + const saveData = jest.fn().mockResolvedValue(undefined); + const refreshDiskState = jest.fn(); + const syncMemory = jest.fn(); + + await expect(service.forgetAllSecrets(saveData, refreshDiskState, syncMemory)).rejects.toThrow( + /Secure Storage is unavailable/ + ); + + // Reason: nothing destructive ran — no disk write, no memory sync, no + // refresh of cached state. + expect(saveData).not.toHaveBeenCalled(); + expect(refreshDiskState).not.toHaveBeenCalled(); + expect(syncMemory).not.toHaveBeenCalled(); + }); +}); + +// --------------------------------------------------------------------------- +// clearAllVaultSecrets — partial-failure surface area +// --------------------------------------------------------------------------- + +describe("clearAllVaultSecrets", () => { + it("clears what it can, then throws aggregating the count of failed entries", () => { + const secretStorage = makeSecretStorage(); + const service = KeychainService.getInstance(makeApp({ secretStorage })); + const vaultId = service.getVaultId(); + + const ok = `copilot-v${vaultId}-open-a-i-api-key`; + const bad1 = `copilot-v${vaultId}-google-api-key`; + const bad2 = `copilot-v${vaultId}-cohere-api-key`; + const foreign = "copilot-vother000-anthropic-api-key"; + secretStorage.listSecrets.mockReturnValue([ok, bad1, bad2, foreign]); + + secretStorage.deleteSecret.mockImplementation((id: string) => { + if (id === bad1 || id === bad2) throw new Error("os denied"); + }); + + expect(() => service.clearAllVaultSecrets()).toThrow(/Failed to clear 2 keychain entries/); + + // Reason: the successful delete survives; foreign-vault entry is never touched. + expect(secretStorage.deleteSecret).toHaveBeenCalledWith(ok); + expect(secretStorage.deleteSecret).not.toHaveBeenCalledWith(foreign); + }); + + it("throws without touching deleteSecret when listSecrets is not a function", () => { + // Reason: defensive feature detection — if a future Obsidian build exposes + // secretStorage without listSecrets we cannot enumerate vault entries, so + // we must refuse rather than silently leave residual entries behind. + const secretStorage = makeSecretStorage(); + (secretStorage as unknown as { listSecrets: unknown }).listSecrets = undefined; + const service = KeychainService.getInstance(makeApp({ secretStorage })); + + expect(() => service.clearAllVaultSecrets()).toThrow(/does not support listing entries/); + expect(secretStorage.deleteSecret).not.toHaveBeenCalled(); + }); +}); + +// --------------------------------------------------------------------------- +// forgetAllSecrets — listSecrets feature detection +// --------------------------------------------------------------------------- + +describe("forgetAllSecrets with missing listSecrets", () => { + it("refuses BEFORE stripping disk when keychain is available but listSecrets is missing", async () => { + // Reason: if we cannot enumerate the keychain, stripping disk first would + // leave the user with cleared data.json AND residual keychain entries + // that resurrect on the next hydrate. Refuse up-front instead. + const secretStorage = makeSecretStorage(); + (secretStorage as unknown as { listSecrets: unknown }).listSecrets = undefined; + const service = KeychainService.getInstance(makeApp({ secretStorage })); + + (getSettings as jest.Mock).mockReturnValue(makeSettings({ openAIApiKey: "sk-live" })); + + const saveData = jest.fn().mockResolvedValue(undefined); + const refreshDiskState = jest.fn(); + const syncMemory = jest.fn(); + + await expect(service.forgetAllSecrets(saveData, refreshDiskState, syncMemory)).rejects.toThrow( + /does not support enumerating Keychain entries/ + ); + + expect(saveData).not.toHaveBeenCalled(); + expect(refreshDiskState).not.toHaveBeenCalled(); + expect(syncMemory).not.toHaveBeenCalled(); + expect(secretStorage.deleteSecret).not.toHaveBeenCalled(); + }); +}); diff --git a/src/services/keychainService.ts b/src/services/keychainService.ts new file mode 100644 index 00000000..2086e7de --- /dev/null +++ b/src/services/keychainService.ts @@ -0,0 +1,738 @@ +import { type App, type SecretStorage, FileSystemAdapter } from "obsidian"; +import { type CopilotSettings, getModelKeyFromModel, getSettings } from "@/settings/model"; +import { type CustomModel } from "@/aiParams"; +import { isSensitiveKey } from "@/encryptionService"; +import { + stripKeychainFields, + cleanupLegacyFields, + isKeychainOnly, + MODEL_SECRET_FIELDS, + TOP_LEVEL_SECRET_FIELDS, +} from "@/services/settingsSecretTransforms"; +import { Notice } from "obsidian"; +import { md5 } from "@/utils/hash"; +// Reason: do NOT import logInfo/logWarn/logError here. The logger depends on +// getSettings(), but this module runs during settings loading (before setSettings). +// Use console.* directly for all logging in this file. + +/** + * Fields that are sensitive but don't match the `isSensitiveKey()` heuristic. + * Reason: `isSensitiveKey()` is the canonical source of truth for sensitive fields. + * Add entries here only for fields that are NOT covered by `isSensitiveKey()`. + */ +const EXTRA_SECRET_KEYS: readonly string[] = []; + +type ModelSecretField = (typeof MODEL_SECRET_FIELDS)[number]; + +/** + * Scope distinguishing chat models from embedding models in keychain IDs. + * Reason: `activeModels` and `activeEmbeddingModels` can contain models with + * the same `name|provider` identity but different API keys. Without scope, + * they'd collide in the keychain namespace. + */ +type ModelScope = "chat" | "embedding"; + +/** + * Check whether a settings key should be stored in the OS keychain. + * Combines the heuristic `isSensitiveKey()` with an explicit exception list. + */ +export function isSecretKey(key: string): boolean { + return isSensitiveKey(key) || EXTRA_SECRET_KEYS.includes(key); +} + +// --------------------------------------------------------------------------- +// Vault namespace — isolates keychain entries per vault +// --------------------------------------------------------------------------- + +/** + * Generate a fresh 8-char hex vault ID for first-time use. + * + * Reason: on desktop, MD5 of the filesystem base path gives a deterministic + * seed so the very first run on an existing vault produces a predictable ID. + * On mobile (no basePath), falls back to random bytes from `crypto.getRandomValues` + * — guaranteeing per-vault isolation on the device at the cost of non-determinism + * before `_keychainVaultId` is persisted and synced. When `getRandomValues` is + * absent, a last-resort MD5 of the string `${Date.now()}-${Math.random()}` is used. + * + * Subsequent runs use the persisted `_keychainVaultId` and never re-derive. + */ +function generateVaultId(app: App): string { + const basePath = getVaultBasePath(app); + if (basePath) { + return md5(basePath).slice(0, 8); + } + // Reason: on mobile, basePath is unavailable. Use a random ID to guarantee + // vault isolation — two same-named vaults on one device will NOT share + // keychain entries. The load path persists this ID to data.json immediately, + // so the divergence window is limited to first-run before sync propagates. + // Reason: guard getRandomValues existence — optional chaining on a missing + // method silently returns undefined, leaving the buffer zero-filled and + // collapsing all affected vaults to "00000000". Use `window.crypto` rather + // than `globalThis.crypto` (project rule `obsidianmd/no-global-this`); both + // resolve to the same WebCrypto instance in Obsidian's Electron renderer + // and mobile WebView. + const cryptoApi = window.crypto; + if (typeof cryptoApi?.getRandomValues === "function") { + const bytes = new Uint8Array(4); + cryptoApi.getRandomValues(bytes); + return Array.from(bytes, (b) => b.toString(16).padStart(2, "0")).join(""); + } + return md5(`${Date.now()}-${Math.random()}`).slice(0, 8); +} + +/** Resolve the filesystem base path, or undefined on mobile. */ +function getVaultBasePath(app: App): string | undefined { + const adapter = app.vault.adapter; + if (adapter instanceof FileSystemAdapter) { + return adapter.getBasePath(); + } + const adapterAny = adapter as unknown as { getBasePath?: () => string; basePath?: string }; + if (typeof adapterAny.getBasePath === "function") { + return adapterAny.getBasePath(); + } + if (typeof adapterAny.basePath === "string") { + return adapterAny.basePath; + } + return undefined; +} + +// --------------------------------------------------------------------------- +// Keychain ID helpers +// --------------------------------------------------------------------------- + +/** Max length enforced by Obsidian's SecretStorage API. */ +const MAX_SECRET_ID_LENGTH = 64; + +/** + * Normalize a raw string into a keychain-safe ID segment. + * Reason: SecretStorage IDs must be lowercase alphanumeric with dashes, 64 chars max. + * + * Always appends an 8-char MD5 hash of the raw input to prevent collisions + * between inputs that differ only by punctuation, case, or non-ASCII chars + * (e.g. "foo.bar|openai" vs "foo-bar|openai" would otherwise normalize + * to the same string). + * + * @param raw - The raw string to normalize. + * @param maxLength - Maximum total length of the returned segment (including hash). + * Callers pass the remaining budget after accounting for their prefix. + */ +function normalizeKeychainId(raw: string, maxLength = MAX_SECRET_ID_LENGTH): string { + const normalized = raw + .toLowerCase() + .replace(/[^a-z0-9-]/g, "-") + .replace(/-+/g, "-") + .replace(/^-|-$/g, ""); + const hash = md5(raw).slice(0, 8); + // Reason: reserve 9 chars for "-" + hash, fill the rest with readable prefix. + const prefixBudget = Math.max(0, maxLength - 9); + const prefix = normalized.slice(0, prefixBudget); + return prefix + "-" + hash; +} + +/** + * Convert a camelCase settings key to a vault-namespaced kebab-case keychain ID. + * Format: `copilot-v{8hex}-{kebab-key}`, capped at 64 chars. + * + * Reason: top-level settings keys are short (e.g. "openAIApiKey" → 22 chars total), + * so truncation is extremely unlikely, but we enforce the cap defensively. + * + * DESIGN NOTE — intentionally does NOT run through `normalizeKeychainId()`. + * Every current top-level secret key is plain camelCase (validated by + * `isSecretKey` against a fixed heuristic + EXTRA_SECRET_KEYS list), so the + * regex pipeline above produces a clean kebab-case id. Routing through the + * full normalizer would add a hash suffix to every id and break id stability + * for existing keychain entries. If a future field with unusual characters + * is added to EXTRA_SECRET_KEYS, revisit. Point future reviewers here. + */ +function toKeychainId(vaultId: string, settingsKey: string): string { + const prefix = `copilot-v${vaultId}-`; + const kebab = settingsKey + .replace(/([A-Z])/g, "-$1") + .toLowerCase() + .replace(/^-/, ""); + const id = prefix + kebab; + if (id.length <= MAX_SECRET_ID_LENGTH) return id; + // Reason: hash the full key to preserve uniqueness when truncated. + const hash = md5(settingsKey).slice(0, 8); + return id.slice(0, MAX_SECRET_ID_LENGTH - 9) + "-" + hash; +} + +/** + * Build a keychain ID for a model-level secret. + * Format: `copilot-v{8hex}-model-{field}-{scope}-{normalized}` + * + * Reason: the fixed prefix consumes up to ~28 chars, so `normalizeKeychainId` + * receives the remaining budget to stay within the 64-char SecretStorage limit. + * Reason: include the field name in the segment so that different secret fields + * on the same model (e.g. apiKey vs a future field) get distinct keychain IDs. + */ +function toModelKeychainId( + vaultId: string, + scope: ModelScope, + modelIdentity: string, + field: ModelSecretField +): string { + // Reason: convert camelCase field name to kebab-case for the keychain ID segment. + // e.g. "apiKey" → "api-key" + const kebabField = field.replace(/([A-Z])/g, "-$1").toLowerCase(); + const fieldSegment = `model-${kebabField}`; + const prefix = `copilot-v${vaultId}-${fieldSegment}-${scope}-`; + const budget = MAX_SECRET_ID_LENGTH - prefix.length; + const normalizedModel = normalizeKeychainId(modelIdentity, budget); + return prefix + normalizedModel; +} + +/** Result of a keychain-only hydrate pass. */ +export interface HydrateResult { + settings: CopilotSettings; + /** True if any keychain read failed — caller may need to fall back to disk. */ + hadFailures: boolean; +} + +/** Output of `persistSecrets()` — what to write to keychain and what to clean up. */ +export interface PersistSecretsResult { + /** Entries to write to keychain: `[keychainId, value]` pairs. */ + secretEntries: Array<[string, string]>; + /** Keychain IDs of deleted models to clear. */ + keychainIdsToDelete: string[]; +} + +/** Callback type for Obsidian's saveData. */ +export type SaveDataFn = (data: CopilotSettings) => Promise; + +/** + * Singleton service for reading/writing secrets via Obsidian's SecretStorage (OS Keychain). + * + * Responsibilities: + * - Store and retrieve API keys and tokens in the OS keychain + * - Backfill secrets from data.json to keychain on upgrade (one-time per field) + * - Hydrate in-memory settings with plaintext secrets on startup + * - Extract secrets from settings for persistence + * - Forget all secrets (destructive, user-initiated) + */ +export class KeychainService { + private static instance: KeychainService | null = null; + private app: App; + private vaultId: string; + + private constructor(app: App) { + this.app = app; + // Reason: vaultId starts as a path-derived fallback. The load path + // should call setVaultId() with the persisted _keychainVaultId value + // before any read/write operations to ensure namespace stability. + this.vaultId = generateVaultId(app); + } + + /** Get or create the singleton instance. Must be called with `app` on plugin load. */ + static getInstance(app?: App): KeychainService { + if (!KeychainService.instance) { + if (!app) { + throw new Error("KeychainService must be initialized with app on first call"); + } + KeychainService.instance = new KeychainService(app); + } + return KeychainService.instance; + } + + /** Reset the singleton (for testing). */ + static resetInstance(): void { + KeychainService.instance = null; + } + + /** Whether the OS keychain is available in this Obsidian version. */ + // DESIGN NOTE — intentionally only checks for the `secretStorage` object, + // not individual methods (`getSecret`/`setSecret`/`listSecrets`). Obsidian + // ships SecretStorage as a single API surface (1.11.4); there is no + // released version where the object exists but methods are missing. + // Capability-probing each method would add branching for a partial-API + // world that does not exist. If a future review flags this again, point + // them at this note. + isAvailable(): boolean { + return !!this.app.secretStorage; + } + + /** Get the current vault namespace ID. */ + getVaultId(): string { + return this.vaultId; + } + + /** + * Set the vault namespace ID from persisted settings. + * Reason: called during load to replace the path-derived fallback with the + * stable persisted ID, so vault renames don't orphan keychain entries. + */ + setVaultId(id: string): void { + this.vaultId = id; + } + + /** + * Access SecretStorage with a runtime guard. + * Reason: replaces scattered non-null assertions with a single guard + * that produces a clear error when keychain is unavailable. + */ + private get storage(): SecretStorage { + if (!this.app.secretStorage) { + throw new Error("OS keychain (SecretStorage) is not available."); + } + return this.app.secretStorage; + } + + // --------------------------------------------------------------------------- + // Low-level read/write + // --------------------------------------------------------------------------- + + // Reason: deleteSecret exists at runtime but is not in the official type + // definitions. Prefer real deletion; fall back to empty-string tombstone. + private removeSecret(id: string): void { + if (typeof this.storage.deleteSecret === "function") { + this.storage.deleteSecret(id); + } else { + this.storage.setSecret(id, ""); + } + } + + /** Write a value directly to the keychain using a pre-computed ID. */ + setSecretById(keychainId: string, value: string): void { + this.storage.setSecret(keychainId, value); + } + + /** Delete a keychain entry by its pre-computed ID. */ + deleteSecretById(keychainId: string): void { + this.removeSecret(keychainId); + } + + /** Store a top-level secret in the keychain. */ + setSecret(settingsKey: string, value: string): void { + const id = toKeychainId(this.vaultId, settingsKey); + this.storage.setSecret(id, value); + } + + /** Retrieve a top-level secret from the keychain. Returns `null` if not found. */ + getSecret(settingsKey: string): string | null { + const id = toKeychainId(this.vaultId, settingsKey); + return this.storage.getSecret(id); + } + + /** Store a model-level secret in the keychain. */ + setModelSecret( + scope: ModelScope, + modelIdentity: string, + field: ModelSecretField, + value: string + ): void { + const id = toModelKeychainId(this.vaultId, scope, modelIdentity, field); + this.storage.setSecret(id, value); + } + + /** Retrieve a model-level secret from the keychain. Returns `null` if not found. */ + getModelSecret(scope: ModelScope, modelIdentity: string, field: ModelSecretField): string | null { + const id = toModelKeychainId(this.vaultId, scope, modelIdentity, field); + return this.storage.getSecret(id); + } + + // --------------------------------------------------------------------------- + // hydrateFromKeychain — read-only keychain hydration + // --------------------------------------------------------------------------- + + /** + * Replace each secret field in `settings` with the keychain value for that + * field. This method is strictly read-only — it never writes to the keychain + * and never reads from disk. The simplified opt-in flow performs all keychain + * writes through `persistSecrets()` (normal saves) or + * `migrateDiskSecretsToKeychain()` (one-shot user action). + * + * Per-field logic: + * - keychain `""` (tombstone) → set field to `""` (don't resurrect) + * - keychain has value → use keychain value + * - keychain `null` → leave the field as-is (caller already loaded from disk + * when appropriate; in keychain-only mode `null` simply means "no value") + * + * @param settings - Sanitised settings. Values are replaced in a shallow copy. + * @returns Updated settings with secrets hydrated from the keychain, plus + * whether any keychain read threw (so the caller can surface a warning). + */ + async hydrateFromKeychain(settings: CopilotSettings): Promise { + const hydrated = { ...settings }; + let hadFailures = false; + + // Top-level secrets. + // + // Reason: iterate the union of (a) the canonical default secret fields and + // (b) any secret-shaped keys already on the loaded settings. Hydrating + // strictly from `Object.keys(hydrated)` would skip fields whose entries + // exist in this device's keychain but are missing from `data.json` (e.g. + // partial sync from a downgraded device, schema additions that predate + // the user's last save, or a manually-edited data.json). Including legacy + // keys still present on the settings object preserves support for fields + // that have since been removed from DEFAULT_SETTINGS. + const topLevelKeys = new Set([ + ...TOP_LEVEL_SECRET_FIELDS, + ...Object.keys(hydrated).filter((key) => isSecretKey(key)), + ]); + for (const key of topLevelKeys) { + // Reason: wrap keychain reads in try/catch so a locked/unavailable keychain + // at startup degrades gracefully instead of aborting plugin load. + let keychainValue: string | null; + try { + keychainValue = this.getSecret(key); + } catch (e) { + console.warn(`Keychain read failed for "${key}".`, e); + hadFailures = true; + continue; + } + + if (keychainValue === "") { + // Tombstone — field was explicitly deleted, don't resurrect. + (hydrated as unknown as Record)[key] = ""; + } else if (keychainValue !== null) { + (hydrated as unknown as Record)[key] = keychainValue; + } + // null → leave existing in-memory value untouched. + } + + // Model-level secrets + const modelResult = await this.hydrateModelSecrets("chat", hydrated.activeModels ?? []); + hydrated.activeModels = modelResult.models; + hadFailures = hadFailures || modelResult.hadFailures; + + const embeddingResult = await this.hydrateModelSecrets( + "embedding", + hydrated.activeEmbeddingModels ?? [] + ); + hydrated.activeEmbeddingModels = embeddingResult.models; + hadFailures = hadFailures || embeddingResult.hadFailures; + + if (hadFailures) { + console.warn("Keychain hydrate: some keychain reads failed — values left as-is."); + } + + return { settings: hydrated, hadFailures }; + } + + // --------------------------------------------------------------------------- + // persistSecrets — extract secrets for keychain write during save + // --------------------------------------------------------------------------- + + /** + * Extract secrets from settings for keychain persistence. + * Returns entries to write to keychain and IDs to clean up. + * Does NOT modify the settings object. + */ + persistSecrets(settings: CopilotSettings, prevSettings?: CopilotSettings): PersistSecretsResult { + const secretEntries: Array<[string, string]> = []; + const clearedSecretIds: string[] = []; + + // Collect top-level secrets + for (const key of Object.keys(settings)) { + if (!isSecretKey(key)) continue; + const value = (settings as unknown as Record)[key]; + const id = toKeychainId(this.vaultId, key); + + if (typeof value === "string" && value.length > 0) { + secretEntries.push([id, value]); + } else if (prevSettings) { + const prevValue = (prevSettings as unknown as Record)[key]; + if (typeof prevValue === "string" && prevValue.length > 0) { + clearedSecretIds.push(id); + } + } + } + + // Collect model-level secrets + this.collectModelSecrets( + "chat", + settings.activeModels, + secretEntries, + prevSettings?.activeModels, + clearedSecretIds + ); + this.collectModelSecrets( + "embedding", + settings.activeEmbeddingModels, + secretEntries, + prevSettings?.activeEmbeddingModels, + clearedSecretIds + ); + + // Find deleted models to clean up + const keychainIdsToDelete = [ + ...this.getDeletedModelKeysForScope( + "chat", + prevSettings?.activeModels, + settings.activeModels + ), + ...this.getDeletedModelKeysForScope( + "embedding", + prevSettings?.activeEmbeddingModels, + settings.activeEmbeddingModels + ), + ...clearedSecretIds, + ]; + + return { secretEntries, keychainIdsToDelete }; + } + + // --------------------------------------------------------------------------- + // clearAllVaultSecrets — wipe all keychain entries for this vault + // --------------------------------------------------------------------------- + + /** + * Delete all keychain entries belonging to this vault's namespace. + * + * Reason: uses `removeSecret()` which prefers real deletion via the + * undocumented `deleteSecret()` and falls back to empty-string tombstone. + * Resurrection from data.json is prevented by `_diskSecretsCleared = true` + * (set by the caller after this method succeeds). + */ + clearAllVaultSecrets(): void { + const vaultPrefix = `copilot-v${this.vaultId}-`; + // Reason: defensive feature detection. The destructive flow already + // stripped data.json by the time it reaches us; if `listSecrets()` is + // missing on this Obsidian build we cannot enumerate vault entries and + // would silently leave them behind to resurrect on the next hydrate. + // Surface this as a hard failure so the caller can preserve disk state. + if (typeof this.storage.listSecrets !== "function") { + throw new Error( + "Obsidian Keychain on this build does not support listing entries; " + + "cannot guarantee a complete clear." + ); + } + const allIds = this.storage.listSecrets(); + const failures: string[] = []; + for (const id of allIds) { + if (id.startsWith(vaultPrefix)) { + try { + this.removeSecret(id); + } catch { + failures.push(id); + } + } + } + if (failures.length > 0) { + throw new Error( + `Failed to clear ${failures.length} keychain ` + + `entr${failures.length === 1 ? "y" : "ies"}. Please retry.` + ); + } + } + + // --------------------------------------------------------------------------- + // forgetAllSecrets — destructive user-initiated operation + // --------------------------------------------------------------------------- + + /** + * Erase all secrets from keychain, data.json, and memory. + * + * This is a dedicated transaction — it does NOT use the normal save path + * (which could potentially resurrect old values). + * + * @param saveData - Callback to write data.json. + * @param refreshDiskState - Callback to refresh the cached disk-secret flag after save. + * @param syncMemory - Callback to update in-memory settings without re-entering + * the normal persist path. The caller must suppress the subscriber-triggered + * `persistSettings()` before calling this (via `suppressNextPersistOnce()`). + */ + async forgetAllSecrets( + saveData: SaveDataFn, + refreshDiskState: (data: CopilotSettings) => void, + syncMemory: (data: Partial) => void, + /** When true, the caller should NOT suppress the subscriber-triggered persist. */ + onDiskSaveFailed?: () => void + ): Promise { + // 0. Refuse the operation in a stranded vault (keychain-only mode but + // SecretStorage unavailable on this build). Otherwise we'd strip disk and + // memory while leaving the existing OS keychain entries intact — the user + // would believe their keys are gone, then watch them reappear after + // upgrading Obsidian or opening the vault on a capable build. Refusing + // up-front keeps the destructive intent honest. + const current = getSettings(); + if (isKeychainOnly(current) && !this.isAvailable()) { + throw new Error( + "Cannot delete API keys from the Obsidian Keychain because Secure Storage is " + + "unavailable in this Obsidian build. Update Obsidian to 1.11.4 or later, or open " + + "this vault on a device with Keychain access, then try again." + ); + } + // Reason: if Keychain is available but lacks `listSecrets()`, we cannot + // enumerate vault entries to clear them. Refuse BEFORE stripping disk so + // we don't leave the user with stripped data.json AND residual Keychain + // entries that would resurrect on next hydrate. + if (this.isAvailable() && typeof this.app.secretStorage?.listSecrets !== "function") { + throw new Error( + "Cannot delete all API keys because this Obsidian build does not support " + + "enumerating Keychain entries. Update Obsidian to a newer version and retry." + ); + } + + // 1. Build stripped settings — before touching any durable store. + const stripped = stripKeychainFields(current) as CopilotSettings & { + _keychainOnly?: boolean; + }; + // Reason: only flip the vault into keychain-only mode when Secure Storage + // is actually usable on this build. Without it, the subsequent persist + // path treats the vault as "stranded" and silently strips any newly + // entered API keys from data.json — turning "Delete All Keys" into a + // one-click way to brick auth setup on older Obsidian builds. A vault + // that was already keychain-only stays that way through + // `stripKeychainFields(current)`, so this guard never accidentally + // downgrades a stranded vault. + if (this.isAvailable()) { + stripped._keychainOnly = true; + } + + // 2. Write stripped data.json BEFORE clearing keychain. + // Reason: if a crash occurs after keychain clear but before disk write, + // the next startup sees "keychain empty + disk has secrets" and backfill + // revives the deleted keys. Writing disk first closes that window. + const toSave = cleanupLegacyFields(stripped); + try { + await saveData(toSave); + refreshDiskState(toSave); + } catch (error) { + console.error("forgetAllSecrets: saveData failed — aborting keychain clear", error); + if (onDiskSaveFailed) onDiskSaveFailed(); + new Notice( + "Failed to remove API keys from data.json. Obsidian Keychain was NOT cleared. Please try again." + ); + return; + } + + // 3. Clear keychain AFTER disk is safely stripped. + let keychainError: Error | undefined; + if (this.isAvailable()) { + try { + this.clearAllVaultSecrets(); + } catch (e) { + keychainError = e instanceof Error ? e : new Error(String(e)); + } + } + + // 4. Always sync in-memory state — even on partial keychain failure. + // Reason: disk is already stripped. If we leave old secrets in memory, + // the next normal persist would write them back to keychain/data.json. + syncMemory(stripped); + + if (keychainError) { + // KNOWN LIMITATION: this path emits a Notice and then throws, which the + // UI caller also catches and Notices — producing two Notices with + // slightly conflicting copy. Triggering requires `clearAllVaultSecrets()` + // to throw mid-operation (very rare in practice). A user retry generally + // resolves the residual keychain entries. Restructuring to return a + // result object instead of throw+Notice is a separate UX cleanup, out of + // scope for this PR. + new Notice( + "Some Obsidian Keychain entries could not be removed. " + + "Your keys have been cleared from data.json and memory. Please restart and retry." + ); + throw keychainError; + } + + new Notice("All API keys for this vault removed. Please re-enter them."); + } + + // --------------------------------------------------------------------------- + // Private helpers + // --------------------------------------------------------------------------- + + /** Hydrate model-level secrets for a scope (read-only). */ + private async hydrateModelSecrets( + scope: ModelScope, + models: CustomModel[] + ): Promise<{ models: CustomModel[]; hadFailures: boolean }> { + if (!models?.length) return { models, hadFailures: false }; + + let hadFailures = false; + const result: CustomModel[] = []; + + for (const model of models) { + const identity = getModelKeyFromModel(model); + const copy = { ...model }; + + for (const field of MODEL_SECRET_FIELDS) { + let keychainValue: string | null; + try { + keychainValue = this.getModelSecret(scope, identity, field); + } catch (e) { + console.warn(`Keychain read failed for model "${identity}" field "${field}".`, e); + hadFailures = true; + continue; + } + + if (keychainValue === "") { + (copy as unknown as Record)[field] = ""; + } else if (keychainValue !== null) { + (copy as unknown as Record)[field] = keychainValue; + } + // null → leave existing in-memory value untouched. + } + + result.push(copy); + } + + return { models: result, hadFailures }; + } + + /** Collect model-level secret entries and cleared IDs without modifying models. */ + private collectModelSecrets( + scope: ModelScope, + models: CustomModel[], + secretEntries: Array<[string, string]>, + prevModels?: CustomModel[], + clearedSecretIds?: string[] + ): void { + if (!models?.length) return; + + const prevModelMap = new Map(); + if (prevModels) { + for (const m of prevModels) { + prevModelMap.set(getModelKeyFromModel(m), m); + } + } + + for (const model of models) { + const identity = getModelKeyFromModel(model); + const prevModel = prevModelMap.get(identity); + + for (const field of MODEL_SECRET_FIELDS) { + const value = model[field]; + const id = toModelKeychainId(this.vaultId, scope, identity, field); + + if (typeof value === "string" && value.length > 0) { + secretEntries.push([id, value]); + } else if (prevModel && clearedSecretIds) { + const prevValue = prevModel[field]; + if (typeof prevValue === "string" && prevValue.length > 0) { + clearedSecretIds.push(id); + } + } + } + } + } + + /** Find keychain IDs for models deleted from a specific scope. */ + private getDeletedModelKeysForScope( + scope: ModelScope, + prevModels: CustomModel[] | undefined, + currentModels: CustomModel[] + ): string[] { + if (!prevModels?.length) return []; + + const currentIds = new Set((currentModels ?? []).map(getModelKeyFromModel)); + + return prevModels + .filter((m) => !currentIds.has(getModelKeyFromModel(m))) + .flatMap((m) => { + const identity = getModelKeyFromModel(m); + // Reason: only tombstone models that actually had a secret value. + // Without this guard, importing to a fresh vault creates spurious + // tombstones for default models that never had an API key. + return MODEL_SECRET_FIELDS.flatMap((field) => { + const prevValue = (m as unknown as Record)[field]; + if (typeof prevValue !== "string" || prevValue.length === 0) { + return []; + } + return [toModelKeychainId(this.vaultId, scope, identity, field)]; + }); + }); + } +} diff --git a/src/services/settingsPersistence.test.ts b/src/services/settingsPersistence.test.ts new file mode 100644 index 00000000..5d7e653c --- /dev/null +++ b/src/services/settingsPersistence.test.ts @@ -0,0 +1,1029 @@ +// Reason: structuredClone is missing in some jsdom builds shipped with our +// Jest version. Polyfilled here (not in jest.setup.js) so the lossy +// JSON-fallback only affects this file — other suites get jsdom's real +// implementation when available. +if (typeof window.structuredClone === "undefined") { + window.structuredClone = (val: T): T => JSON.parse(JSON.stringify(val)) as T; +} + +import { DEFAULT_SETTINGS } from "@/constants"; +import type { CustomModel } from "@/aiParams"; +import type { CopilotSettings } from "@/settings/model"; + +/** Match the production secret-key heuristic without importing the real module. */ +function isSensitiveKey(key: string): boolean { + const lower = key.toLowerCase(); + const normalized = lower.replace(/[_-]/g, ""); + return ( + normalized.includes("apikey") || + lower.endsWith("token") || + lower.endsWith("accesstoken") || + lower.endsWith("secret") || + lower.endsWith("password") || + lower.endsWith("licensekey") + ); +} + +/** Build a full settings object while keeping tests compact. */ +function makeSettings(overrides: Partial = {}): CopilotSettings { + return { + ...DEFAULT_SETTINGS, + ...overrides, + }; +} + +/** Build a minimal custom model for persistence tests. */ +function makeModel(overrides: Partial = {}): CustomModel { + return { + name: "gpt-4", + provider: "openai", + enabled: true, + ...overrides, + }; +} + +/** Load a fresh copy of the module with isolated mocks. */ +async function loadModule(overrides?: { + keychain?: Record; + getDecryptedKey?: (value: string) => Promise; +}) { + jest.resetModules(); + + const keychain = { + isAvailable: jest.fn().mockReturnValue(true), + getVaultId: jest.fn().mockReturnValue("vault1234"), + setVaultId: jest.fn(), + hydrateFromKeychain: jest.fn(async (settings: CopilotSettings) => ({ + settings, + hadFailures: false, + })), + persistSecrets: jest.fn().mockReturnValue({ + secretEntries: [], + keychainIdsToDelete: [], + }), + setSecretById: jest.fn(), + deleteSecretById: jest.fn(), + getSecret: jest.fn().mockReturnValue(null), + getModelSecret: jest.fn().mockReturnValue(null), + ...(overrides?.keychain ?? {}), + }; + + jest.doMock("@/services/keychainService", () => ({ + KeychainService: { getInstance: jest.fn(() => keychain) }, + isSecretKey: jest.fn((key: string) => isSensitiveKey(key)), + })); + + jest.doMock("@/encryptionService", () => ({ + isSensitiveKey: jest.fn((key: string) => isSensitiveKey(key)), + getDecryptedKey: jest.fn( + overrides?.getDecryptedKey ?? (async (v: string) => v.replace(/^enc_/, "")) + ), + hasEncryptionPrefix: jest.fn((value: string) => value.startsWith("enc_")), + })); + + jest.doMock("@/logger", () => ({ logWarn: jest.fn() })); + + const mockSettings = { current: makeSettings() }; + jest.doMock("@/settings/model", () => ({ + sanitizeSettings: jest.fn((s: CopilotSettings) => s), + getModelKeyFromModel: jest.fn( + (m: { name: string; provider: string }) => `${m.name}|${m.provider}` + ), + normalizeModelProvider: jest.fn((p: string) => (p === "azure_openai" ? "azure openai" : p)), + getSettings: jest.fn(() => mockSettings.current), + setSettings: jest.fn((s: Partial) => { + mockSettings.current = { ...mockSettings.current, ...s }; + }), + })); + + jest.doMock("@/services/settingsSecretTransforms", () => ({ + MODEL_SECRET_FIELDS: ["apiKey"] as const, + hasPersistedSecrets: jest.fn((rawData: Record) => { + for (const key of Object.keys(rawData)) { + if (!isSensitiveKey(key)) continue; + const value = rawData[key]; + if (typeof value === "string" && value.length > 0) return true; + } + for (const listKey of ["activeModels", "activeEmbeddingModels"] as const) { + const models = rawData[listKey]; + if (!Array.isArray(models)) continue; + for (const model of models) { + if (!model || typeof model !== "object") continue; + const value = (model as Record).apiKey; + if (typeof value === "string" && value.length > 0) return true; + } + } + return false; + }), + stripKeychainFields: jest.fn((s: CopilotSettings) => { + const out = { ...s } as unknown as Record; + for (const key of Object.keys(out)) { + if (isSensitiveKey(key)) out[key] = ""; + } + for (const listKey of ["activeModels", "activeEmbeddingModels"] as const) { + const models = (s as unknown as Record)[listKey]; + if (!Array.isArray(models)) continue; + out[listKey] = models.map((m) => ({ + ...(m as Record), + apiKey: "", + })); + } + return out as unknown as CopilotSettings; + }), + cleanupLegacyFields: jest.fn((s: CopilotSettings) => { + const out = { ...s } as unknown as Record; + delete out.enableEncryption; + delete out._keychainMigrated; + delete out._keychainMigratedAt; + delete out._migrationModalDismissed; + if (out._diskSecretsCleared !== undefined && out._keychainOnly === undefined) { + out._keychainOnly = out._diskSecretsCleared; + } + delete out._diskSecretsCleared; + return out as unknown as CopilotSettings; + }), + isKeychainOnly: jest.fn( + (s: CopilotSettings) => (s as unknown as Record)._keychainOnly === true + ), + })); + + const mod = await import("./settingsPersistence"); + return { mod, keychain, mockSettings }; +} + +afterEach(() => { + jest.useRealTimers(); + jest.restoreAllMocks(); +}); + +// --------------------------------------------------------------------------- +// loadSettingsWithKeychain — mode dispatch and fresh-install promotion +// --------------------------------------------------------------------------- + +describe("loadSettingsWithKeychain", () => { + it("promotes to keychain-only on a truly fresh install (rawData == null)", async () => { + const { mod, keychain } = await loadModule(); + const saveData = jest.fn().mockResolvedValue(undefined); + + const loaded = await mod.loadSettingsWithKeychain(null, saveData); + + expect((loaded as unknown as Record)._keychainOnly).toBe(true); + // Reason: even on fresh install, hydrateFromKeychain still runs so + // tombstones / pre-existing keychain entries are honored. + expect(keychain.hydrateFromKeychain).toHaveBeenCalled(); + }); + + it("does NOT promote to keychain-only when rawData is an empty object", async () => { + // Reason: existing user who manually cleared their keys (`data.json` exists + // but is empty) must stay in disk mode — promoting them would silently flip + // future key entries into keychain-only and violate opt-in. + const { mod } = await loadModule(); + const saveData = jest.fn().mockResolvedValue(undefined); + + const loaded = await mod.loadSettingsWithKeychain({}, saveData); + + expect((loaded as unknown as Record)._keychainOnly).toBeUndefined(); + }); + + it("disk mode load never touches the keychain", async () => { + const { mod, keychain } = await loadModule(); + const saveData = jest.fn().mockResolvedValue(undefined); + + await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + openAIApiKey: "enc_disk_openai", + }), + saveData + ); + + expect(keychain.hydrateFromKeychain).not.toHaveBeenCalled(); + expect(keychain.getSecret).not.toHaveBeenCalled(); + expect(keychain.setSecretById).not.toHaveBeenCalled(); + }); + + it("disk mode decrypts legacy enc_* values for runtime use", async () => { + const { mod } = await loadModule(); + + const loaded = await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + openAIApiKey: "enc_disk_openai", + }), + jest.fn().mockResolvedValue(undefined) + ); + + expect(loaded.openAIApiKey).toBe("disk_openai"); + }); + + it("keychain-only mode reads from keychain and ignores any stale disk secret", async () => { + const { mod, keychain } = await loadModule({ + keychain: { + hydrateFromKeychain: jest.fn(async (s: CopilotSettings) => ({ + settings: { ...s, openAIApiKey: "kc-value" }, + hadFailures: false, + })), + }, + }); + const warnSpy = jest.spyOn(console, "warn").mockImplementation(() => {}); + + const loaded = await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + _keychainOnly: true, + // Reason: stale disk leftover from cross-version sync. + openAIApiKey: "should-be-ignored", + }), + jest.fn().mockResolvedValue(undefined) + ); + + expect(keychain.hydrateFromKeychain).toHaveBeenCalled(); + expect(loaded.openAIApiKey).toBe("kc-value"); + // Reason: scenario H — log a warning when stale disk secrets are observed + // alongside keychain-only mode. + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("disk secrets ignored because keychain-only") + ); + + warnSpy.mockRestore(); + }); + + it("decrypts disk values when keychain is unavailable", async () => { + const { mod } = await loadModule({ + keychain: { isAvailable: jest.fn().mockReturnValue(false) }, + }); + + const loaded = await mod.loadSettingsWithKeychain( + makeSettings({ + openAIApiKey: "enc_disk_openai", + activeModels: [makeModel({ apiKey: "enc_model_key" })], + }), + jest.fn().mockResolvedValue(undefined) + ); + + expect(loaded.openAIApiKey).toBe("disk_openai"); + expect(loaded.activeModels[0].apiKey).toBe("model_key"); + }); + + it("persists a first-run vault ID when raw data does not have one", async () => { + const { mod, keychain } = await loadModule(); + const saveData = jest.fn().mockResolvedValue(undefined); + + const raw = makeSettings({ + _keychainVaultId: undefined, + }); + + await mod.loadSettingsWithKeychain(raw, saveData); + + expect(keychain.getVaultId).toHaveBeenCalled(); + expect(saveData).toHaveBeenCalledWith( + expect.objectContaining({ _keychainVaultId: "vault1234" }) + ); + }); + + it("stranded keychain-only vault never loads plaintext from disk", async () => { + // Reason: codex review #3235563049 — when `_keychainOnly: true` is set + // but SecretStorage is unavailable on this build, the early disk-mode + // bypass used to call `loadSecretsFromDisk()` and surface any stale + // plaintext (cross-version sync, manual edits) for the session. That + // breaks the keychain-only contract enforced everywhere else in this + // module. The fix strips secret fields and preserves `_keychainOnly`. + const { mod } = await loadModule({ + keychain: { isAvailable: jest.fn().mockReturnValue(false) }, + }); + + const loaded = await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + _keychainOnly: true, + openAIApiKey: "sk-disk-leak", + activeModels: [makeModel({ apiKey: "sk-model-leak" })], + }), + jest.fn().mockResolvedValue(undefined) + ); + + const rec = loaded as unknown as Record; + expect(rec._keychainOnly).toBe(true); + expect(loaded.openAIApiKey).toBe(""); + expect(loaded.activeModels[0].apiKey).toBe(""); + }); + + it("migrates legacy _diskSecretsCleared → _keychainOnly via cleanupLegacyFields", async () => { + const { mod } = await loadModule(); + + const loaded = await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + _diskSecretsCleared: true, + } as unknown as Partial), + jest.fn().mockResolvedValue(undefined) + ); + + const rec = loaded as unknown as Record; + expect(rec._keychainOnly).toBe(true); + expect(rec._diskSecretsCleared).toBeUndefined(); + }); +}); + +// --------------------------------------------------------------------------- +// persistSettings — branch dispatch +// --------------------------------------------------------------------------- + +describe("persistSettings", () => { + it("keychain-only branch writes keychain and strips disk", async () => { + const { mod, keychain, mockSettings } = await loadModule(); + const saveData = jest.fn().mockResolvedValue(undefined); + + mockSettings.current = makeSettings({ + openAIApiKey: "sk-123", + _keychainOnly: true, + }); + + keychain.persistSecrets.mockReturnValue({ + secretEntries: [["kc-openai", "sk-123"]], + keychainIdsToDelete: [], + }); + + await mod.persistSettings(mockSettings.current, saveData, mockSettings.current); + await mod.flushPersistence(); + + expect(keychain.setSecretById).toHaveBeenCalledWith("kc-openai", "sk-123"); + const saved = saveData.mock.calls[0][0] as Record; + expect(saved.openAIApiKey).toBe(""); + expect(saved._keychainOnly).toBe(true); + }); + + it("disk branch writes plaintext to disk and never touches the keychain", async () => { + // Reason: scenario "disk mode completely bypasses secretStorage" — important + // for users who never opted in. Even if a stale keychain entry exists, we + // must not read or write it. + const { mod, keychain } = await loadModule(); + const saveData = jest.fn().mockResolvedValue(undefined); + + const current = makeSettings({ openAIApiKey: "sk-plaintext" }); + await mod.persistSettings(current, saveData, current); + await mod.flushPersistence(); + + expect(saveData).toHaveBeenCalledWith( + expect.objectContaining({ openAIApiKey: "sk-plaintext" }) + ); + expect(keychain.setSecretById).not.toHaveBeenCalled(); + expect(keychain.persistSecrets).not.toHaveBeenCalled(); + expect(keychain.getSecret).not.toHaveBeenCalled(); + }); + + it("falls back to plaintext disk save when keychain is unavailable", async () => { + const { mod } = await loadModule({ + keychain: { isAvailable: jest.fn().mockReturnValue(false) }, + }); + const saveData = jest.fn().mockResolvedValue(undefined); + + const current = makeSettings({ openAIApiKey: "sk-fallback" }); + await mod.persistSettings(current, saveData, current); + await mod.flushPersistence(); + + expect(saveData).toHaveBeenCalledWith(expect.objectContaining({ openAIApiKey: "sk-fallback" })); + }); + + it("preserves _keychainOnly and strips secrets when keychain is unavailable", async () => { + // Reason: a keychain-only vault opened on a non-SecretStorage build must + // NOT downgrade to disk mode and write plaintext — that would silently + // orphan the keychain entries on other devices when the vault syncs back. + // Instead, preserve the mode marker and strip secrets so the next device + // with Secure Storage support can resume normally. + const { mod } = await loadModule({ + keychain: { isAvailable: jest.fn().mockReturnValue(false) }, + }); + const saveData = jest.fn().mockResolvedValue(undefined); + + const current = makeSettings({ + openAIApiKey: "sk-fallback", + _keychainOnly: true, + }); + await mod.persistSettings(current, saveData, current); + await mod.flushPersistence(); + + const saved = saveData.mock.calls[0][0] as Record; + expect(saved._keychainOnly).toBe(true); + expect(saved.openAIApiKey).toBe(""); + }); + + it("does not roll memory back when saveData fails", async () => { + const { mod, mockSettings, keychain } = await loadModule(); + mockSettings.current = makeSettings({ + openAIApiKey: "sk-123", + _keychainOnly: true, + }); + + keychain.persistSecrets.mockReturnValue({ + secretEntries: [["kc-openai", "sk-123"]], + keychainIdsToDelete: [], + }); + + const saveData = jest.fn().mockRejectedValue(new Error("disk full")); + + await expect( + mod + .persistSettings(mockSettings.current, saveData, mockSettings.current) + .then(() => mod.flushPersistence()) + ).rejects.toThrow("disk full"); + }); + + it("after migrate, subsequent persist never writes plaintext secret to disk", async () => { + const { mod, keychain, mockSettings } = await loadModule(); + const saveData = jest.fn().mockResolvedValue(undefined); + + mockSettings.current = makeSettings({ + openAIApiKey: "plaintext-still-in-memory", + _keychainOnly: true, + }); + + keychain.persistSecrets.mockReturnValue({ + secretEntries: [["kc-openai", "plaintext-still-in-memory"]], + keychainIdsToDelete: [], + }); + + await mod.persistSettings(mockSettings.current, saveData, mockSettings.current); + await mod.flushPersistence(); + + const saved = saveData.mock.calls[0][0] as Record; + // Reason: even though memory holds the plaintext, the on-disk payload must + // have the secret stripped — this is the keychain-only contract. + expect(saved.openAIApiKey).toBe(""); + }); +}); + +// --------------------------------------------------------------------------- +// migrateDiskSecretsToKeychain +// --------------------------------------------------------------------------- + +describe("migrateDiskSecretsToKeychain", () => { + it("writes keychain, strips disk, and flips memory to _keychainOnly=true", async () => { + const { mod, keychain, mockSettings } = await loadModule(); + mockSettings.current = makeSettings({ + openAIApiKey: "sk-live", + anthropicApiKey: "sk-ant", + }); + + // Seed cached disk-secret state via loadSettingsWithKeychain + await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + openAIApiKey: "sk-live", + }), + jest.fn().mockResolvedValue(undefined) + ); + + keychain.persistSecrets.mockReturnValue({ + secretEntries: [ + ["kc-openai", "sk-live"], + ["kc-anthropic", "sk-ant"], + ], + keychainIdsToDelete: [], + }); + + const saveData = jest.fn().mockResolvedValue(undefined); + await mod.migrateDiskSecretsToKeychain(saveData); + + expect(keychain.setSecretById).toHaveBeenCalledWith("kc-openai", "sk-live"); + expect(keychain.setSecretById).toHaveBeenCalledWith("kc-anthropic", "sk-ant"); + + const saved = saveData.mock.calls[0][0] as Record; + expect(saved._keychainOnly).toBe(true); + expect(saved.openAIApiKey).toBe(""); + expect(saved.anthropicApiKey).toBe(""); + + expect((mockSettings.current as unknown as Record)._keychainOnly).toBe(true); + }); + + it("refuses to migrate when keychain is unavailable", async () => { + const { mod, mockSettings } = await loadModule({ + keychain: { isAvailable: jest.fn().mockReturnValue(false) }, + }); + mockSettings.current = makeSettings({ openAIApiKey: "sk-live" }); + + const saveData = jest.fn().mockResolvedValue(undefined); + await expect(mod.migrateDiskSecretsToKeychain(saveData)).rejects.toThrow( + /Cannot migrate to Obsidian Keychain/ + ); + expect(saveData).not.toHaveBeenCalled(); + }); + + it("partial migration: clears undecryptable enc_* fields and reports them", async () => { + // Reason: a stale device-local crypto key can leave enc_* values that the + // current device cannot decrypt. Migration must not write that ciphertext + // into the keychain (where it would silently break LLM calls). Instead it + // clears those fields and tells the caller exactly which keys to re-enter. + // + // DESIGN NOTE — this asserts the partial-success contract on purpose. Do + // NOT "fix" the implementation to throw on undecryptable fields; the + // multi-device re-entry trade-off is already disclosed in the migration + // modal, and partial-success has identical end state with fewer user + // steps. See the note on `collectUndecryptableFields` in the implementation. + const { mod, keychain, mockSettings } = await loadModule({ + // Reason: simulate decrypt failure for any enc_* value. + getDecryptedKey: async () => "", + }); + mockSettings.current = makeSettings({ + openAIApiKey: "enc_desk_broken", + anthropicApiKey: "sk-ant", + activeModels: [makeModel({ name: "gpt-4", provider: "openai", apiKey: "enc_desk_broken" })], + }); + + await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + openAIApiKey: "enc_desk_broken", + }), + jest.fn().mockResolvedValue(undefined) + ); + + keychain.persistSecrets.mockReturnValue({ + secretEntries: [["kc-anthropic", "sk-ant"]], + keychainIdsToDelete: [], + }); + + const saveData = jest.fn().mockResolvedValue(undefined); + const result = await mod.migrateDiskSecretsToKeychain(saveData); + + expect(result.fieldsRequiringReentry).toEqual( + expect.arrayContaining(["openAIApiKey", "gpt-4 (openai) apiKey"]) + ); + expect(result.fieldsRequiringReentry).toHaveLength(2); + + // Reason: ciphertext must NEVER be written to the keychain, even partially. + expect(keychain.setSecretById).not.toHaveBeenCalledWith(expect.anything(), "enc_desk_broken"); + + const saved = saveData.mock.calls[0][0] as Record; + expect(saved._keychainOnly).toBe(true); + expect(saved.openAIApiKey).toBe(""); + + // Reason: the readable field still got migrated successfully. + expect(keychain.setSecretById).toHaveBeenCalledWith("kc-anthropic", "sk-ant"); + }); + + it("propagates saveData errors and does not flip the memory flag", async () => { + const { mod, keychain, mockSettings } = await loadModule(); + mockSettings.current = makeSettings({ openAIApiKey: "sk-live" }); + + // Seed disk-secret state + await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + openAIApiKey: "sk-live", + }), + jest.fn().mockResolvedValue(undefined) + ); + + keychain.persistSecrets.mockReturnValue({ + secretEntries: [["kc-openai", "sk-live"]], + keychainIdsToDelete: [], + }); + + const saveData = jest.fn().mockRejectedValue(new Error("disk full")); + await expect(mod.migrateDiskSecretsToKeychain(saveData)).rejects.toThrow("disk full"); + + // Reason: memory flag must NOT advance when the transaction failed. + expect( + (mockSettings.current as unknown as Record)._keychainOnly + ).toBeUndefined(); + }); + + it("lifts the fail-closed lock after a rollback-clean transient failure so retry works", async () => { + // Reason: codex review #3234543430 — a transient saveData failure used to + // wedge `persistHadUndecryptableSecrets = true` forever, blocking every + // subsequent migration attempt until Obsidian was restarted. The fix + // resets the lock when the rollback proves the keychain is back in a + // known-good state. This test pins that the user can retry without a + // restart. + const { mod, keychain, mockSettings } = await loadModule(); + mockSettings.current = makeSettings({ openAIApiKey: "sk-live" }); + await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + openAIApiKey: "sk-live", + }), + jest.fn().mockResolvedValue(undefined) + ); + + keychain.persistSecrets.mockReturnValue({ + secretEntries: [["kc-openai", "sk-live"]], + keychainIdsToDelete: [], + }); + + // First attempt: saveData throws. Rollback writes succeed (the default + // setSecretById mock returns undefined), so the lock should be lifted. + const failingSaveData = jest.fn().mockRejectedValue(new Error("transient io")); + await expect(mod.migrateDiskSecretsToKeychain(failingSaveData)).rejects.toThrow("transient io"); + + expect(mod.canClearDiskSecrets(mockSettings.current)).toBe(true); + + // Second attempt: saveData succeeds. Migration completes normally. + const goodSaveData = jest.fn().mockResolvedValue(undefined); + await mod.migrateDiskSecretsToKeychain(goodSaveData); + + const saved = goodSaveData.mock.calls[0][0] as Record; + expect(saved._keychainOnly).toBe(true); + expect((mockSettings.current as unknown as Record)._keychainOnly).toBe(true); + }); + + it("keeps the fail-closed lock when rollback itself fails", async () => { + // Reason: rollback writes the previous settings' secrets back to keychain. + // If those writes also fail, keychain state is unknown — leave the lock + // armed so a subsequent retry doesn't strip disk against a possibly + // corrupt keychain. + const { mod, keychain, mockSettings } = await loadModule(); + mockSettings.current = makeSettings({ openAIApiKey: "sk-live" }); + await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + openAIApiKey: "sk-live", + }), + jest.fn().mockResolvedValue(undefined) + ); + + keychain.persistSecrets.mockReturnValue({ + secretEntries: [["kc-openai", "sk-live"]], + keychainIdsToDelete: [], + }); + // Reason: every keychain write throws — both the forward write and the + // rollback replay. The rollback helper must report failure so the lock + // stays armed. + keychain.setSecretById.mockImplementation(() => { + throw new Error("keychain locked"); + }); + + const saveData = jest.fn().mockResolvedValue(undefined); + await expect(mod.migrateDiskSecretsToKeychain(saveData)).rejects.toThrow("keychain locked"); + + expect(mod.canClearDiskSecrets(mockSettings.current)).toBe(false); + }); + + it("preserves concurrent settings edits committed during the migration await", async () => { + // Reason: codex review surfaced a race in the migration transaction. The + // transaction captured `current` at start, but `setSettings(target)` at + // the end committed that stale snapshot wholesale, silently rolling back + // any unrelated edits (theme toggle, prompt change, etc.) the user made + // while keychain writes + saveData were in flight. The fix re-derives + // the in-memory commit from the latest `getSettings()` snapshot so only + // migration-owned fields (`_keychainOnly: true` plus `enc_*` clears) are + // forced, and concurrent edits survive into memory. + const { mod, keychain, mockSettings } = await loadModule(); + mockSettings.current = makeSettings({ openAIApiKey: "sk-live" }); + await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + openAIApiKey: "sk-live", + }), + jest.fn().mockResolvedValue(undefined) + ); + + keychain.persistSecrets.mockReturnValue({ + secretEntries: [["kc-openai", "sk-live"]], + keychainIdsToDelete: [], + }); + + // Hold the FIRST saveData open so we can simulate a concurrent setSettings + // while the migration transaction is mid-flight. Later saveData calls + // (the reconciliation re-persist) resolve immediately. + let resolveFirstSave!: () => void; + const saveData = jest + .fn() + .mockImplementationOnce( + () => + new Promise((r) => { + resolveFirstSave = r; + }) + ) + .mockResolvedValue(undefined); + + const migration = mod.migrateDiskSecretsToKeychain(saveData); + + // Yield so the transaction reaches its first `await persistSecretsToKeychain`. + await Promise.resolve(); + await Promise.resolve(); + + // Concurrent unrelated edit lands in memory while the transaction awaits. + (mockSettings.current as unknown as Record).temperature = 0.99; + + resolveFirstSave(); + await migration; + await mod.flushPersistence(); + + const final = mockSettings.current as unknown as Record; + expect(final._keychainOnly).toBe(true); + expect(final.temperature).toBe(0.99); // concurrent edit survives in memory + + // Reason: the reconciliation re-persist must also push the concurrent edit + // to disk so a user who closes Obsidian immediately afterwards does not + // lose it. The very last saveData call carries the merged state. + expect(saveData).toHaveBeenCalledTimes(2); + const lastDiskPayload = saveData.mock.calls[saveData.mock.calls.length - 1][0] as Record< + string, + unknown + >; + expect(lastDiskPayload._keychainOnly).toBe(true); + expect(lastDiskPayload.temperature).toBe(0.99); + }); + + it("syncs memory to target when reconciliation persist fails after a successful first save", async () => { + // Reason: codex 3rd review surfaced a split-brain risk. If a concurrent + // edit triggers a reconciliation re-persist and THAT throws, the first + // save has already committed `target` durably to disk + keychain. Memory + // would otherwise stay as the pre-migration disk-mode snapshot, and the + // user's very next settings change would dispatch through + // `persistSecretsToDisk` — silently writing plaintext secrets back into + // `data.json` and undoing the successful migration. The fix forces memory + // to `target` on reconciliation failure so subsequent saves stay on the + // keychain-only path. + const { mod, keychain, mockSettings } = await loadModule(); + mockSettings.current = makeSettings({ openAIApiKey: "sk-live" }); + await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + openAIApiKey: "sk-live", + }), + jest.fn().mockResolvedValue(undefined) + ); + + keychain.persistSecrets.mockReturnValue({ + secretEntries: [["kc-openai", "sk-live"]], + keychainIdsToDelete: [], + }); + + // First save resolves (migration step 1 commits target). Second save + // (reconciliation) rejects. + let resolveFirstSave!: () => void; + const saveData = jest + .fn() + .mockImplementationOnce( + () => + new Promise((r) => { + resolveFirstSave = r; + }) + ) + .mockRejectedValueOnce(new Error("reconcile io")); + + const migration = mod.migrateDiskSecretsToKeychain(saveData); + + await Promise.resolve(); + await Promise.resolve(); + + // Concurrent edit forces the reconciliation pass to run. + (mockSettings.current as unknown as Record).temperature = 0.42; + + resolveFirstSave(); + + await expect(migration).rejects.toThrow("reconcile io"); + await mod.flushPersistence(); + + const final = mockSettings.current as unknown as Record; + // Migration's first save IS durable; memory must match so the next save + // stays on the keychain-only path and does not regress to disk mode. + // (Plaintext stays in memory for runtime LLM use — disk is the stripped + // copy, keychain holds the durable secret.) + expect(final._keychainOnly).toBe(true); + // The concurrent temperature edit is intentionally rolled back so memory + // matches what `target` actually persisted; preserving the migration + // safety invariant takes priority over preserving the concurrent edit. + expect(final.temperature).not.toBe(0.42); + }); + + it("successful disk-mode save lifts the fail-closed lock so migration retry works", async () => { + // Reason: codex review #3235793522 — when a migration attempt suffers a + // double failure (forward keychain write fails AND rollback also fails), + // `persistHadUndecryptableSecrets` stays armed. The conditional reset in + // `persistSecretsToKeychain` only fires when rollback succeeded. Without + // also resetting the lock on successful disk-mode saves, the user was + // trapped until Obsidian restart: `canClearDiskSecrets()` kept returning + // false, so the Migrate flow rejected every retry with "last save did + // not complete safely". The fix clears the lock at the end of a clean + // disk save — disk is the source of truth again, so a fresh migration + // attempt would copy from the known-good baseline. + const { mod, keychain, mockSettings } = await loadModule(); + mockSettings.current = makeSettings({ openAIApiKey: "sk-live" }); + await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + openAIApiKey: "sk-live", + }), + jest.fn().mockResolvedValue(undefined) + ); + + keychain.persistSecrets.mockReturnValue({ + secretEntries: [["kc-openai", "sk-live"]], + keychainIdsToDelete: [], + }); + + // Step 1: force the double-failure so the lock arms. + keychain.setSecretById.mockImplementation(() => { + throw new Error("keychain locked"); + }); + await expect( + mod.migrateDiskSecretsToKeychain(jest.fn().mockResolvedValue(undefined)) + ).rejects.toThrow("keychain locked"); + expect(mod.canClearDiskSecrets(mockSettings.current)).toBe(false); + + // Step 2: the user fixes whatever was wrong and edits any setting → + // settings subscriber triggers a normal disk-mode persist. + keychain.setSecretById.mockReset(); + keychain.setSecretById.mockReturnValue(undefined); + const cleanSave = jest.fn().mockResolvedValue(undefined); + await mod.persistSettings( + mockSettings.current, + cleanSave, + mockSettings.current + ); + await mod.flushPersistence(); + + // Step 3: the lock must be lifted so the next migration attempt can run. + expect(mod.canClearDiskSecrets(mockSettings.current)).toBe(true); + }); + + it("undecryptable enc_* in keychain entries throws BEFORE arming the fail-closed lock", async () => { + // Reason: the defensive `hasEncryptionPrefix` guard rejects writes before + // we touch the keychain or disk, so it must not poison the migration + // retry path. (Today this branch is hard to reach since the migrate path + // calls collectUndecryptableFields() first and throws earlier; this test + // pins the contract for any other caller that goes through + // persistSettings -> persistSecretsToKeychain directly.) + const { mod, keychain, mockSettings } = await loadModule(); + mockSettings.current = makeSettings({ + _keychainOnly: true, + openAIApiKey: "enc_garbled", + }); + await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + openAIApiKey: "enc_garbled", + _keychainOnly: true, + }), + jest.fn().mockResolvedValue(undefined) + ); + + keychain.persistSecrets.mockReturnValue({ + secretEntries: [["kc-openai", "enc_garbled"]], + keychainIdsToDelete: [], + }); + + const saveData = jest.fn().mockResolvedValue(undefined); + await expect( + mod.persistSettings(mockSettings.current, saveData) + ).rejects.toThrow(/undecryptable secrets/); + await mod.flushPersistence(); + + expect(keychain.setSecretById).not.toHaveBeenCalled(); + expect(saveData).not.toHaveBeenCalled(); + // The lock must NOT be left armed by a guard that ran before any write. + // canClearDiskSecrets returns false here for a different reason + // (isKeychainOnly), so we test the lock directly by reading the exposed + // helper through a disk-mode probe. + expect( + mod.canClearDiskSecrets(makeSettings({ openAIApiKey: "sk-live" })) + ).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- +// hasDiskSecretsToMigrate / canClearDiskSecrets +// --------------------------------------------------------------------------- + +describe("hasDiskSecretsToMigrate", () => { + it("returns true when disk has secrets after load", async () => { + const { mod } = await loadModule(); + await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + openAIApiKey: "sk-live", + }), + jest.fn().mockResolvedValue(undefined) + ); + + expect(mod.hasDiskSecretsToMigrate()).toBe(true); + }); + + it("returns false on a fresh install (no disk file)", async () => { + const { mod } = await loadModule(); + await mod.loadSettingsWithKeychain(null, jest.fn().mockResolvedValue(undefined)); + + expect(mod.hasDiskSecretsToMigrate()).toBe(false); + }); +}); + +describe("canClearDiskSecrets", () => { + it("returns true when keychain available, disk has secrets, not yet keychain-only", async () => { + const { mod } = await loadModule(); + await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + openAIApiKey: "sk-live", + }), + jest.fn().mockResolvedValue(undefined) + ); + + expect(mod.canClearDiskSecrets(makeSettings({ openAIApiKey: "sk-live" }))).toBe(true); + }); + + it("returns false once _keychainOnly is true", async () => { + const { mod } = await loadModule(); + await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + openAIApiKey: "sk-live", + }), + jest.fn().mockResolvedValue(undefined) + ); + + expect( + mod.canClearDiskSecrets( + makeSettings({ _keychainOnly: true }) + ) + ).toBe(false); + }); + + it("returns false when keychain is unavailable", async () => { + const { mod } = await loadModule({ + keychain: { isAvailable: jest.fn().mockReturnValue(false) }, + }); + + expect(mod.canClearDiskSecrets(makeSettings({ openAIApiKey: "sk-live" }))).toBe(false); + }); + + it("returns false when disk has no secrets", async () => { + const { mod } = await loadModule(); + await mod.loadSettingsWithKeychain(null, jest.fn().mockResolvedValue(undefined)); + + expect(mod.canClearDiskSecrets(makeSettings())).toBe(false); + }); + + it("returns true when disk has no secrets but the user just typed one into memory", async () => { + // Reason: covers the in-memory fallback. `hasDiskSecretsToMigrate()` only + // refreshes after a persist, so a fresh install where the user just typed + // their first key needs the live-settings presence check to surface the + // "Migrate to Keychain" CTA without waiting for the next save. + const { mod } = await loadModule(); + await mod.loadSettingsWithKeychain(null, jest.fn().mockResolvedValue(undefined)); + + expect(mod.hasDiskSecretsToMigrate()).toBe(false); + expect(mod.canClearDiskSecrets(makeSettings({ openAIApiKey: "sk-live" }))).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- +// Stale-persist guard +// --------------------------------------------------------------------------- + +describe("stale persist guard", () => { + it("skips persist queued during a transaction", async () => { + const { mod } = await loadModule(); + + await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + }), + jest.fn().mockResolvedValue(undefined) + ); + + const saveData = jest.fn().mockResolvedValue(undefined); + + await mod.runPersistenceTransaction(async () => { + const stale = makeSettings({ openAIApiKey: "sk-live" }); + // Reason: fire-and-forget on purpose — the test asserts a persist queued + // mid-transaction is dropped, so we must NOT await it here. + void mod.persistSettings(stale, saveData, stale); + }); + + await mod.flushPersistence(); + + expect(saveData).not.toHaveBeenCalled(); + }); + + it("skips persist queued during a FAILED transaction", async () => { + const { mod } = await loadModule(); + + await mod.loadSettingsWithKeychain( + makeSettings({ + _keychainVaultId: "vault1234", + }), + jest.fn().mockResolvedValue(undefined) + ); + + const saveData = jest.fn().mockResolvedValue(undefined); + + await mod + .runPersistenceTransaction(async () => { + const stale = makeSettings({ openAIApiKey: "sk-live" }); + // Reason: fire-and-forget on purpose — the test asserts a persist queued + // inside a failed transaction is dropped, so we must NOT await it here. + void mod.persistSettings(stale, saveData, stale); + throw new Error("simulated transaction failure"); + }) + .catch(() => { + /* expected */ + }); + + await mod.flushPersistence(); + + expect(saveData).not.toHaveBeenCalled(); + }); +}); diff --git a/src/services/settingsPersistence.ts b/src/services/settingsPersistence.ts new file mode 100644 index 00000000..3ff22982 --- /dev/null +++ b/src/services/settingsPersistence.ts @@ -0,0 +1,960 @@ +/** + * Unified settings persistence layer (simplified opt-in flow). + * + * Two mutually exclusive modes, gated by `settings._keychainOnly`: + * + * 1. **Keychain-only mode** (`_keychainOnly === true`) + * - Secrets live in the OS keychain; data.json is always stripped. + * - Triggered by fresh installs or the user explicitly clicking + * "Migrate to Keychain" in Advanced Settings. + * - Load reads secrets from keychain only; the keychain is the single + * source of truth. + * + * 2. **Disk mode** (`_keychainOnly` is falsy) + * - Secrets stay in data.json. The keychain is never touched. + * - Existing users that did not opt in remain here permanently unless + * they click "Migrate to Keychain". + * - Legacy `enc_*` ciphertext is decrypted at load for runtime use; + * any subsequent save writes the plaintext form to data.json + * (per the maintainer's "never write new encrypted values" rule). + * + * Hard boundaries: + * - `loadSecretsFromDisk` / `persistSecretsToDisk` never touch the keychain. + * - `loadSecretsFromKeychain` / `persistSecretsToKeychain` never read disk. + * - `migrateDiskSecretsToKeychain` is the only function that crosses both + * sides, and it does so as a single transaction. + */ + +import { type CopilotSettings, getSettings, sanitizeSettings, setSettings } from "@/settings/model"; +import { getDecryptedKey, hasEncryptionPrefix } from "@/encryptionService"; +import { KeychainService, isSecretKey } from "@/services/keychainService"; +import { + cleanupLegacyFields, + hasPersistedSecrets, + isKeychainOnly, + MODEL_SECRET_FIELDS, + stripKeychainFields, +} from "@/services/settingsSecretTransforms"; +// Reason: logWarn is safe to import (lazy — only calls getSettings() when invoked), +// but only used in code paths that run AFTER settings are loaded. Functions that +// run DURING settings loading (loadSettingsWithKeychain) use console.* directly. +import { logError, logWarn } from "@/logger"; +import { Notice } from "obsidian"; + +// --------------------------------------------------------------------------- +// Module-global persistence state +// --------------------------------------------------------------------------- + +/** + * Write queue serialising all persistence operations. + * + * Reason: the settings subscriber fires synchronously on every `setSettings()`, + * but keychain + data.json writes are async. Without serialisation, rapid + * successive `setSettings()` calls would race with unpredictable ordering. + * Dedicated transactions (`forgetAllSecrets`, migrate-to-keychain) also run + * through this queue via `runPersistenceTransaction()` to prevent interleaving. + */ +let writeQueue: Promise = Promise.resolve(); + +/** + * Whether the last successful disk state still contains any persisted secrets. + * Used by `hasDiskSecretsToMigrate()` for the UI status check. + */ +let diskHasSecrets = false; + +/** + * Settings snapshot from the last successful `saveData()` call. Used as the + * keychain-diff baseline so rollback restores the previous known-good state. + */ +let lastPersistedSettings: CopilotSettings | undefined; + +/** + * When true, the next `persistSettings()` call is skipped. + * + * Reason: dedicated transactions write data.json themselves, then call + * `setSettings()` to sync memory. That `setSettings()` triggers the subscriber + * which calls `persistSettings()` again — without this guard the subscriber + * would re-enter the normal save path and overwrite the transaction's output. + */ +let suppressNextPersist = false; + +/** + * Monotonic counter incremented by each `runPersistenceTransaction()` to + * invalidate stale persist jobs queued before the transaction committed. + */ +let transactionEpoch = 0; + +/** + * Keychain IDs whose tombstone writes failed in a previous persist cycle. + * Retried at the start of the next `doPersist()` call so delete intent + * survives across saves even when the settings subscriber advances `prev`. + */ +const pendingTombstones = new Set(); + +/** + * Whether the most recent keychain persist attempt left it unsafe to clear + * disk secrets. Fails closed across the whole save cycle. + */ +let persistHadUndecryptableSecrets = false; + +/** Keychain vault IDs are 8 lowercase hex chars. */ +const KEYCHAIN_VAULT_ID_RE = /^[a-f0-9]{8}$/; + +/** Check whether a persisted keychain vault ID has the expected format. */ +function isValidKeychainVaultId(value: unknown): value is string { + return typeof value === "string" && KEYCHAIN_VAULT_ID_RE.test(value); +} + +// --------------------------------------------------------------------------- +// Public state inspectors / refreshers +// --------------------------------------------------------------------------- + +/** Refresh the cached disk-secret presence after a successful save/load. */ +export function refreshDiskHasSecrets(data: CopilotSettings): void { + diskHasSecrets = hasPersistedSecrets(data as unknown as Record); +} + +/** + * Refresh the last known-good settings baseline used by keychain rollback. + * Called by dedicated transactions that bypass `doPersist()`. + */ +export function refreshLastPersistedSettings(data: CopilotSettings): void { + lastPersistedSettings = structuredClone(data); +} + +/** + * Skip the next `persistSettings()` call. Must be called immediately before + * `setSettings()` in dedicated transactions that wrote data.json themselves. + */ +export function suppressNextPersistOnce(): void { + suppressNextPersist = true; +} + +/** + * Whether the data.json on disk still contains any non-empty secret fields. + * Used by the UI to decide whether to surface the "Migrate to Keychain" CTA. + */ +export function hasDiskSecretsToMigrate(): boolean { + return diskHasSecrets; +} + +/** + * Whether it is safe to strip secrets from data.json right now. + * + * All safety gates must pass: + * - Keychain available (otherwise we'd lose the only copy) + * - The most recent keychain persist did not skip any undecryptable secret + * - Not already in keychain-only mode + * - data.json still has secrets to clear + */ +export function canClearDiskSecrets(settings: CopilotSettings): boolean { + const keychain = KeychainService.getInstance(); + if (!keychain.isAvailable()) return false; + if (persistHadUndecryptableSecrets) return false; + if (isKeychainOnly(settings)) return false; + // Reason: in-memory `settings` may already contain a secret the user just + // typed but hasn't saved yet. `diskHasSecrets` only refreshes after persist, + // so a freshly entered key wouldn't enable the Migrate CTA without this + // fallback. Both presence sources mean "there is something to migrate". + return diskHasSecrets || hasPersistedSecrets(settings as unknown as Record); +} + +// --------------------------------------------------------------------------- +// Persistence transaction support +// --------------------------------------------------------------------------- + +/** + * Run a dedicated persistence transaction within the write queue. + * + * Reason: operations that bypass `doPersist` (forgetAllSecrets, migrate) + * must still be serialised with the normal save path to prevent interleaving + * that could restore stripped secrets. + */ +export async function runPersistenceTransaction(task: () => Promise): Promise { + const job = writeQueue.then(async () => { + try { + await task(); + } finally { + // DESIGN NOTE — epoch is bumped unconditionally on failure, even if the + // task failed before touching any persistent store (e.g. an early + // `canClearDiskSecrets()` rejection in migrate). Differentiating "pure + // pre-check failure" from "partial write" would require threading a + // touched-flag through every transaction call site (migrate, forget, + // ...), and the safety cost of letting a stale persist overwrite a + // partially-mutated store is far higher than the worst case here: a + // single queued setting save dropped, recoverable by any subsequent + // settings edit. Triggering this race also requires another + // `setSettings()` to land between the transaction's first await and + // its failure — a very narrow window. Fail-safe wins. + // If a future review flags this again, point them at this note. + transactionEpoch++; + } + }); + writeQueue = job.catch(() => { + /* swallow to unblock next write */ + }); + return job; +} + +/** Wait for all queued persistence operations to complete. */ +export async function flushPersistence(): Promise { + await writeQueue; +} + +// --------------------------------------------------------------------------- +// Load — disk side +// --------------------------------------------------------------------------- + +/** + * Decrypt any `enc_*` encrypted values in `settings` for runtime use. + * + * Boundary: reads ONLY from the in-memory `settings` object (which mirrors + * data.json after sanitize). Never touches the keychain. + * + * Reason: legacy users may still have `enc_*` ciphertext in data.json. Without + * decryption, ciphertext would flow into provider requests. + */ +async function loadSecretsFromDisk(settings: CopilotSettings): Promise { + const hydrated = structuredClone(settings); + const rec = hydrated as unknown as Record; + + for (const key of Object.keys(rec)) { + if (!isSecretKey(key)) continue; + const value = rec[key]; + if (typeof value !== "string" || value.length === 0) continue; + const plaintext = await getDecryptedKey(value); + if (plaintext) rec[key] = plaintext; + } + + for (const listKey of ["activeModels", "activeEmbeddingModels"] as const) { + const models = hydrated[listKey] ?? []; + for (const model of models) { + const modelRec = model as unknown as Record; + for (const field of MODEL_SECRET_FIELDS) { + const value = modelRec[field]; + if (typeof value !== "string" || value.length === 0) continue; + const plaintext = await getDecryptedKey(value); + if (plaintext) modelRec[field] = plaintext; + } + } + } + + return hydrated; +} + +// --------------------------------------------------------------------------- +// Load — keychain side +// --------------------------------------------------------------------------- + +/** + * Replace each secret field in `settings` with the keychain value. + * + * Boundary: read-only against the keychain. Never falls back to disk; if the + * keychain is empty, fields stay empty (the keychain-only contract). + */ +async function loadSecretsFromKeychain(settings: CopilotSettings): Promise { + // Reason: in keychain-only mode, settings as loaded from disk should already + // be stripped. Start from a stripped baseline so any stale disk values that + // crept in (e.g. via cross-version sync) cannot bleed into runtime memory. + const baseline = stripKeychainFields(settings); + // Reason: hadFailures does NOT block persists — empty-diff already protects + // failed fields (empty-vs-empty never writes), so the actual keychain values + // are preserved across the failure. We still surface a one-shot Notice so the + // user knows why model calls may fail this session instead of silently seeing + // "invalid API key" errors. + const { settings: hydrated, hadFailures } = + await KeychainService.getInstance().hydrateFromKeychain(baseline); + if (hadFailures) { + new Notice( + "Some API keys could not be loaded from the Obsidian Keychain. They may be unavailable this session. Restart Obsidian if the issue persists." + ); + } + return hydrated; +} + +// --------------------------------------------------------------------------- +// Public load entry point +// --------------------------------------------------------------------------- + +/** + * Load settings from raw disk data, dispatching to disk or keychain mode + * based on the persisted `_keychainOnly` flag. + * + * Fresh-install detection (the only place new vaults get auto-opted-in): + * `rawData == null` indicates Obsidian had no data.json for this plugin — + * which uniquely identifies a fresh install. An existing vault that happens + * to have no secrets right now (`rawData != null` but empty) is NOT promoted. + */ +export async function loadSettingsWithKeychain( + rawData: unknown, + saveData: (data: CopilotSettings) => Promise +): Promise { + // Reason: capture fresh-install state BEFORE we start mutating anything. + // Obsidian's loadData() returns null when data.json doesn't exist yet. + const isFreshInstall = rawData == null; + + // Reason: sanitize FIRST to normalise model providers (e.g. azure_openai → azure-openai). + let settings = sanitizeSettings(rawData as CopilotSettings); + + // Snapshot raw disk state so the cached `diskHasSecrets` flag is accurate + // regardless of any downstream cleanup that happens to `settings`. + let rawDiskData = structuredClone(rawData ?? {}) as Record; + diskHasSecrets = hasPersistedSecrets(rawDiskData); + + // Reason: cleanupLegacyFields also migrates `_diskSecretsCleared` → + // `_keychainOnly`. Run it BEFORE any code reads `_keychainOnly` so the + // value carries forward correctly from older installs. + settings = cleanupLegacyFields(settings); + + const keychain = KeychainService.getInstance(); + + // ---- Disk mode bypass when keychain isn't available at all. ---- + if (!keychain.isAvailable()) { + // Reason: stranded vault. `_keychainOnly: true` was set on a capable + // build, but SecretStorage isn't available here (older Obsidian, missing + // API). Honour the keychain-only contract: do NOT load disk secrets even + // if `data.json` still carries plaintext (cross-version sync, manual + // edits, or a half-applied migration on another device could put them + // there). Mirrors the keychain-available branch below at line 348-358 + // which also explicitly ignores disk secrets in keychain-only mode. + // Without this guard, a stranded session would silently use stale disk + // plaintext for LLM auth — violating the contract surfaced everywhere + // else in this module. + if (isKeychainOnly(settings)) { + const stripped = stripKeychainFields(settings); + lastPersistedSettings = structuredClone(stripped); + return stripped; + } + const hydrated = await loadSecretsFromDisk(settings); + lastPersistedSettings = structuredClone(hydrated); + return hydrated; + } + + // ---- Fresh-install promotion (the ONLY auto-opt-in path). ---- + // Reason: settings file existed but `_keychainOnly` is undefined → keep + // disk mode. Auto-promoting that case would violate the "until they click, + // nothing changes" rule for users who manually deleted their keys. + if (isFreshInstall) { + (settings as unknown as Record)._keychainOnly = true; + } + + // ---- Vault namespace ID bootstrap (shared by both modes). ---- + if (isValidKeychainVaultId(rawDiskData._keychainVaultId)) { + keychain.setVaultId(rawDiskData._keychainVaultId); + } else { + // First run — persist the generated vaultId immediately to disk. + // Reason: main.ts calls setSettings() before the subscriber is registered, + // so the initial setSettings won't trigger persistSettings. We must write + // the vaultId here to survive a vault rename before the next save. + // Reason: also persist `_keychainOnly` for fresh installs so that the + // mode survives a restart even if the user never edits any setting. + // + // DESIGN NOTE — bootstrap intentionally writes a SPARSE snapshot + // (`rawDiskData` + vaultId + optional `_keychainOnly`), not the full + // sanitized in-memory `settings`. Considered and rejected the "persist + // full settings here" alternative: + // + // - No observable runtime impact from the sparse snapshot. On the next + // startup, `settingsAtom = atom(DEFAULT_SETTINGS)` already holds full + // defaults; `setSettings(loadedSparse)` then merges via + // `{ ...getSettings(), ...loadedSparse }` and + // `mergeAllActiveModelsWithCoreModels()` re-injects built-in models. + // Arrays like `activeModels` are never undefined at runtime. + // - Disk self-heals on the user's very first settings change: the + // settings subscriber calls `persistSettings(next, ...)` with the + // fully-merged in-memory state, replacing the sparse snapshot. + // - Writing full sanitized settings here would persist *computed* + // defaults (built-in model lists, derived keys) into the user's + // `data.json` on first run, making future default changes a + // migration concern rather than a transparent upgrade. + // + // If a future review flags this again, point them at this note. + const vaultId = keychain.getVaultId(); + settings = { ...settings, _keychainVaultId: vaultId }; + try { + const currentDisk: Record = { + ...rawDiskData, + _keychainVaultId: vaultId, + }; + if (isFreshInstall) { + currentDisk._keychainOnly = true; + } + await saveData(currentDisk as unknown as CopilotSettings); + rawDiskData = currentDisk; + diskHasSecrets = hasPersistedSecrets(rawDiskData); + } catch (error) { + // Reason: surface bootstrap failure to the user. Without this Notice the + // failure is invisible — the in-memory `_keychainOnly: true` exists but + // never reaches disk, so the fresh-install promotion is lost if the user + // closes Obsidian before triggering another save. The next save will + // self-heal, but the user should know to confirm by saving once. + logError("Failed to persist initial keychain settings on first run", error); + new Notice("Could not save initial settings. Please save once to confirm Keychain mode."); + } + } + + // ---- Dispatch by `_keychainOnly`. ---- + if (isKeychainOnly(settings)) { + if (diskHasSecrets) { + // Reason: cross-version sync can leave plaintext on disk while the + // device is already keychain-only. Logged for diagnosis only (no Notice + // to avoid distracting users on startup); the disk values are ignored. + console.warn("disk secrets ignored because keychain-only mode is enabled"); + } + const hydrated = await loadSecretsFromKeychain(settings); + lastPersistedSettings = structuredClone(hydrated); + return hydrated; + } + + // ---- Disk mode (existing user, opted-out, or pre-Migrate). ---- + const hydrated = await loadSecretsFromDisk(settings); + lastPersistedSettings = structuredClone(hydrated); + return hydrated; +} + +// --------------------------------------------------------------------------- +// Persist — disk side +// --------------------------------------------------------------------------- + +/** + * Write `settings` to data.json with its in-memory plaintext secrets intact. + * Never touches the keychain. + * + * Note: legacy `enc_*` values are decrypted to plaintext at load time. Any + * subsequent save therefore overwrites the disk copy with plaintext. This is + * intentional and matches the maintainer's "never write new encrypted values" + * requirement. + */ +async function persistSecretsToDisk( + settings: CopilotSettings, + saveData: (data: CopilotSettings) => Promise +): Promise { + const cleaned = cleanupLegacyFields(settings); + await saveData(cleaned); + refreshDiskHasSecrets(cleaned); + // Reason: a successful disk-mode save re-establishes data.json as the + // durable source of truth. If a previous keychain migration's forward + // write and rollback both failed, `persistHadUndecryptableSecrets` would + // otherwise remain armed forever (the conditional reset in + // `persistSecretsToKeychain` only fires when rollback succeeded), leaving + // `canClearDiskSecrets()` permanently false and the Migrate flow stuck + // until the user restarts Obsidian. Clearing the lock here is safe: a + // future migration attempt will read this fresh disk baseline and write + // the keychain from scratch — it does not strip disk against a stale or + // possibly-corrupt keychain. + persistHadUndecryptableSecrets = false; + lastPersistedSettings = structuredClone(settings); +} + +// --------------------------------------------------------------------------- +// Persist — keychain side +// --------------------------------------------------------------------------- + +/** + * Write `settings` secrets to the keychain and persist a stripped data.json. + * Performs partial-write rollback on failure so the keychain matches the + * last known-good state. + * + * Only called for vaults in keychain-only mode. Never reads disk; the caller + * is responsible for providing the previous settings via `prev`. + */ +async function persistSecretsToKeychain( + settings: CopilotSettings, + saveData: (data: CopilotSettings) => Promise, + prev: CopilotSettings | undefined +): Promise { + const keychain = KeychainService.getInstance(); + const cleaned = cleanupLegacyFields(settings); + + const keychainDiffBase = lastPersistedSettings ?? prev; + const { secretEntries, keychainIdsToDelete } = keychain.persistSecrets( + settings, + keychainDiffBase + ); + const rollbackSettings = lastPersistedSettings ?? prev; + const replayedTombstones: string[] = []; + + // Reason: secrets in memory are always plaintext by the time we save + // (load decrypts enc_* on the way in). Writing ciphertext to keychain + // would poison the slot. Defensive guard for unusual paths (e.g. config + // import, external setSettings, cross-version sync) that could smuggle a + // legacy `enc_*` value past the load-time decrypt. Check BEFORE arming + // the fail-closed lock so this guard never poisons the migration retry + // path — the throw here happens before any keychain or disk write. + // + // DESIGN NOTE — `hasEncryptionPrefix(value)` is intentionally the sole + // invalidity check here. Considered and rejected the "decrypt-then-classify" + // alternative (try `getDecryptedKey(value)` and only reject if it returns + // empty): + // + // - The false-positive surface is hypothetical. The known LLM provider + // key formats — `sk-…` (OpenAI), `sk-ant-…` (Anthropic), `AIza…` + // (Google), `ghp_…` (GitHub), `hf_…` (HuggingFace), `csk-…` (Cohere) + // — do not start with `enc_`/`enc_web_`/`enc_desk_`. A user typing a + // plaintext that collides with our prefix would only happen with a + // custom-provider secret deliberately chosen that way. + // - The false-negative surface for "decrypt then accept" is more + // plausible and worse: a corrupted ciphertext that happens to decrypt + // to a non-empty garbage string would be written to keychain as if it + // were a real key, silently breaking auth in a way the fail-closed + // throw catches. + // - The throw is explicit and user-actionable ("Re-enter these keys in + // Settings before saving."), whereas a silent decrypt fallback would + // mask the smuggled-ciphertext case the guard exists to catch. + // + // If a future review flags this again, point them at this note. + const undecryptableIds = secretEntries + .filter(([, value]) => hasEncryptionPrefix(value)) + .map(([id]) => id); + if (undecryptableIds.length > 0) { + throw new Error( + `Refusing to persist: undecryptable secrets in keychain-only mode (${undecryptableIds.join(", ")}). ` + + "Re-enter these keys in Settings before saving." + ); + } + + // Reason: arm the fail-closed lock only once we're about to touch the + // keychain or disk. Any throw inside the try-block leaves the keychain in + // a potentially partial state until rollback finishes. If rollback proves + // the keychain is back to a known-good state we lift the lock; otherwise + // it stays set so the caller can't `clearDiskSecrets()` against a + // possibly-corrupt keychain. + persistHadUndecryptableSecrets = true; + + try { + // Retry any tombstones that failed in a previous cycle so delete intent + // survives across saves. + for (const id of pendingTombstones) { + keychain.setSecretById(id, ""); + replayedTombstones.push(id); + } + + for (const [id, value] of secretEntries) { + keychain.setSecretById(id, value); + } + + for (const id of keychainIdsToDelete) { + try { + keychain.setSecretById(id, ""); + } catch (e) { + // Reason: track for retry on the next save cycle, then abort so the + // caller's error path fires. + pendingTombstones.add(id); + throw e; + } + } + + // Reason: keychain mode → data.json is always stripped of secret fields. + const stripped = stripKeychainFields(cleaned); + (stripped as unknown as Record)._keychainOnly = true; + + await saveData(stripped); + + refreshDiskHasSecrets(stripped); + lastPersistedSettings = structuredClone(settings); + + for (const id of replayedTombstones) { + pendingTombstones.delete(id); + } + } catch (error) { + // Reason: best-effort rollback. If every restore/tombstone write + // succeeded the keychain matches `rollbackSettings` again and it is safe + // to lift the fail-closed lock so the user can retry the migration + // without restarting Obsidian. If rollback itself failed (or there was + // no baseline to restore from) the keychain is in an unknown state and + // the lock must stay set. + let rollbackOk = false; + try { + rollbackOk = await restoreKeychainFromSettings(keychain, rollbackSettings, settings); + } catch (rollbackError) { + logWarn("Failed to roll back keychain after persist failure.", rollbackError); + } + if (rollbackOk) { + persistHadUndecryptableSecrets = false; + } + throw error; + } + + // Reason: full cycle succeeded — narrow the fail-closed guard. + persistHadUndecryptableSecrets = false; +} + +/** + * Collect human-readable labels for every secret field still carrying an + * `enc_*` ciphertext that `loadSecretsFromDisk()` could not decrypt. + * + * Reason: a leftover `enc_*` in memory means the plaintext is lost on this + * device. The migrate path uses this list to (a) clear those fields before + * writing the keychain (so we never store ciphertext as if it were a key) + * and (b) tell the user exactly which keys they need to re-enter. + */ +// DESIGN NOTE — do NOT "simplify" this to fail-closed (throw + force user to +// re-enter before migrating). Considered and rejected: +// +// - End state is identical: in both designs the user re-enters those keys +// and ends up with full keychain coverage. +// - Partial-success requires 2 user steps (Migrate → re-enter in settings). +// Fail-closed requires 3 (try Migrate → re-enter → retry Migrate). +// - The "ciphertext might be recoverable on another device" argument does +// not survive scrutiny: the migration confirm modal already declares +// "Other devices syncing this vault will need to re-enter their API keys +// after migration." Multi-device re-entry is a documented feature, not a +// regression introduced by clearing undecryptable enc_* values. +// +// Keep the clear-and-report flow. If a future review flags this again, point +// them at this note. +function collectUndecryptableFields(settings: CopilotSettings): string[] { + const fields: string[] = []; + const rec = settings as unknown as Record; + + for (const key of Object.keys(rec)) { + if (!isSecretKey(key)) continue; + const value = rec[key]; + if (typeof value === "string" && value.length > 0 && hasEncryptionPrefix(value)) { + fields.push(key); + } + } + for (const listKey of ["activeModels", "activeEmbeddingModels"] as const) { + for (const model of settings[listKey] ?? []) { + const modelRec = model as unknown as Record; + for (const field of MODEL_SECRET_FIELDS) { + const value = modelRec[field]; + if (typeof value === "string" && value.length > 0 && hasEncryptionPrefix(value)) { + // Reason: model entries share the keychain namespace via name|provider, + // so a label like "gpt-4 (openai) apiKey" lets users find which row to edit. + const name = (modelRec.name as string | undefined) ?? "(unnamed)"; + const provider = (modelRec.provider as string | undefined) ?? "?"; + fields.push(`${name} (${provider}) ${field}`); + } + } + } + } + return fields; +} + +/** + * Build a copy of `settings` where every undecryptable `enc_*` secret field + * has been cleared to an empty string. The caller persists the cleaned copy + * to keychain so we never store ciphertext masquerading as a real API key. + * + * Companion to `collectUndecryptableFields()` — they walk the same fields. + */ +function clearUndecryptableSecrets(settings: CopilotSettings): CopilotSettings { + const out = structuredClone(settings); + const rec = out as unknown as Record; + + for (const key of Object.keys(rec)) { + if (!isSecretKey(key)) continue; + const value = rec[key]; + if (typeof value === "string" && value.length > 0 && hasEncryptionPrefix(value)) { + rec[key] = ""; + } + } + for (const listKey of ["activeModels", "activeEmbeddingModels"] as const) { + const models = out[listKey] ?? []; + for (const model of models) { + const modelRec = model as unknown as Record; + for (const field of MODEL_SECRET_FIELDS) { + const value = modelRec[field]; + if (typeof value === "string" && value.length > 0 && hasEncryptionPrefix(value)) { + modelRec[field] = ""; + } + } + } + } + return out; +} + +// --------------------------------------------------------------------------- +// Migration transaction +// --------------------------------------------------------------------------- + +/** + * Result of a `migrateDiskSecretsToKeychain` call. + * + * `fieldsRequiringReentry` lists every secret that could not be decrypted from + * data.json and was therefore cleared instead of migrated. The migration still + * succeeded for the rest; the caller is expected to surface this list to the + * user so they can re-enter the affected keys. + */ +export interface MigrationResult { + fieldsRequiringReentry: string[]; +} + +/** + * One-shot transaction: write current in-memory secrets to keychain, strip + * them from data.json, and flip `_keychainOnly = true` in memory. + * + * Undecryptable legacy `enc_*` values are NOT written to the keychain (they + * are not valid plaintext keys). Instead they are cleared from memory and + * reported in the result so the user can re-enter them. The migration succeeds + * for every other field — see the DESIGN NOTE above `collectUndecryptableFields` + * for why we don't fail closed. + * + * Failure semantics: + * - Partial keychain write → rollback, throw. `_keychainOnly` stays false. + * - Disk save failure → rollback keychain, throw. `_keychainOnly` stays false. + * - Success → memory + disk + keychain all agree on keychain-only mode. + * `fieldsRequiringReentry` may still be non-empty; the caller surfaces it. + */ +export async function migrateDiskSecretsToKeychain( + saveData: (data: CopilotSettings) => Promise +): Promise { + const result: MigrationResult = { fieldsRequiringReentry: [] }; + + await runPersistenceTransaction(async () => { + const current = getSettings(); + if (!canClearDiskSecrets(current)) { + throw new Error( + "Cannot migrate to Obsidian Keychain: it is unavailable, the last save did not complete " + + "safely, or there are no secrets left to migrate. Retry saving settings, then try again." + ); + } + + // Reason: clear undecryptable enc_* values BEFORE persisting so the + // keychain only ever stores real plaintext keys. The cleared fields are + // returned so the UI can prompt the user to re-enter them. + result.fieldsRequiringReentry = collectUndecryptableFields(current); + const sanitized = clearUndecryptableSecrets(current); + + // Step 1+2: persist secrets to keychain and stripped data.json. + // Reason: `persistSecretsToKeychain` is the same code path used by every + // future save in keychain-only mode — using it here keeps the migration + // and steady-state paths in lockstep. + // + // DESIGN NOTE — keychain residue from a failed *first-stage* persist is + // accepted, not cleaned. Scope: this note covers ONLY the + // `persistSecretsToKeychain(target, ...)` call below failing (typically its + // internal `saveData(stripped)` throwing) — NOT the later reconciliation + // pass, which on failure deliberately commits the migrated state. + // When this first-stage call writes the keychain but then throws, its + // rollback restores the keychain to `lastPersistedSettings ?? prev` — which + // for a disk-mode vault still holds the plaintext secrets, so the keychain keeps + // a copy of them. Trigger requires `saveData()` to throw mid-migration + // (disk full / permissions) — uncommon. Impact is benign: the first-stage + // failure leaves the vault disk-mode (`_keychainOnly` never flips in memory + // or on disk), so `loadSettingsWithKeychain` never hydrates from the + // keychain; the residue is dormant and self-heals on the next successful + // Migrate (overwrites the same IDs) or Delete All Keys (enumerate + clear). + // It is not a credential leak — the keychain is at least as protected as + // the data.json copy that already exists. A migration-specific rollback + // branch (snapshot the real keychain baseline / tombstone the IDs this call + // wrote) is complexity not justified by dormant, self-healing residue. + // If a future review flags this again, point them at this note. + const target = { ...sanitized, _keychainOnly: true } as CopilotSettings; + await persistSecretsToKeychain(target, saveData, current); + + // Step 3: reconcile with any concurrent settings edits that landed + // during the await, then sync in-memory state. + // + // Reason: the transaction captured `current` at the top of the callback; + // concurrent `setSettings()` calls during `await persistSecretsToKeychain` + // can advance in-memory state with unrelated user edits (theme toggle, + // custom prompt changes, etc.). The queued persist for that concurrent + // edit is dropped by the `transactionEpoch` guard in + // `runPersistenceTransaction` — necessary to prevent the stale snapshot + // from rolling back the migration's secret stripping — so without an + // explicit reconciliation step those edits would be lost from disk if + // the user closed Obsidian before any subsequent save. Re-derive from + // the latest snapshot, and if it materially differs from `target`, + // persist once more with `target` as the baseline so keychain + disk + // both reflect the final merged state. + const fresh = getSettings(); + const sanitizedFresh = clearUndecryptableSecrets(fresh); + const merged = { ...sanitizedFresh, _keychainOnly: true } as CopilotSettings; + + // DESIGN NOTE — JSON.stringify is intentionally used here despite being + // property-order-sensitive. This comparison only runs once per migration, + // both objects originate from the same sanitize pipeline (so key order is + // stable in practice), and the only consequence of a false-positive is + // one redundant `persistSecretsToKeychain` call. A deepEqual helper would + // add a dependency or ~30 lines of code for no behavioural improvement. + // If a future review flags this again, point them at this note. + if (JSON.stringify(merged) !== JSON.stringify(target)) { + try { + await persistSecretsToKeychain(merged, saveData, target); + } catch (reconcileError) { + // Reason: the first persistSecretsToKeychain already committed + // `target` to disk + keychain durably. If the reconciliation pass + // (which only exists to push concurrent edits) fails, sync memory to + // `target` so subsequent saves see the migrated state. Otherwise + // memory would stay as the pre-migration disk-mode snapshot and the + // very next settings change would dispatch through + // `persistSecretsToDisk` — writing plaintext secrets back to + // `data.json` and silently undoing the successful migration. The + // concurrent edit is lost (we cannot guarantee it landed on disk), + // but the migration's primary contract is preserved. + suppressNextPersistOnce(); + setSettings(target); + throw reconcileError; + } + } + + // Suppress the subscriber-driven persist that `setSettings(merged)` would + // otherwise trigger — the in-transaction persistSecretsToKeychain calls + // already wrote the final state. + suppressNextPersistOnce(); + setSettings(merged); + }); + + return result; +} + +/** + * Backwards-compat alias for the UI's "clear old copies" / "Migrate to Keychain" + * button. New code should call `migrateDiskSecretsToKeychain` directly; this + * export exists so the existing settings page continues to compile while the + * frontend is being refactored to the new API. + * + * @deprecated Use `migrateDiskSecretsToKeychain` instead. + */ +export const clearDiskSecrets = migrateDiskSecretsToKeychain; + +// --------------------------------------------------------------------------- +// Persist queue entry points +// --------------------------------------------------------------------------- + +/** + * Public entry point — queues a save behind the write queue and any pending + * dedicated transactions, then dispatches to disk or keychain mode. + */ +export async function persistSettings( + settings: CopilotSettings, + saveData: (data: CopilotSettings) => Promise, + prevSettings?: CopilotSettings +): Promise { + if (suppressNextPersist) { + suppressNextPersist = false; + return; + } + + const epochAtEnqueue = transactionEpoch; + const job = writeQueue.then(() => { + // Reason: if a transaction committed after this job was queued, the + // captured snapshot is stale and may contain secrets the transaction + // just deleted. Drop the job entirely in that case. + if (epochAtEnqueue !== transactionEpoch) return; + return doPersist(settings, saveData, prevSettings); + }); + writeQueue = job.catch(() => { + /* swallow to unblock next write */ + }); + return job; +} + +/** Core persistence dispatcher, executed inside the write queue. */ +async function doPersist( + settings: CopilotSettings, + saveData: (data: CopilotSettings) => Promise, + prevSettings?: CopilotSettings +): Promise { + const keychain = KeychainService.getInstance(); + + // Reason: when Secure Storage isn't available, the user must still be able + // to save non-secret settings. For a keychain-only vault, preserve the mode + // marker and strip secrets from disk — otherwise a non-SecretStorage build + // (e.g. older Obsidian opening a synced vault) would silently downgrade the + // vault to disk mode and orphan the keychain entries on other devices. + // Disk-mode vaults fall through to the normal plaintext-disk save. + if (!keychain.isAvailable()) { + if (isKeychainOnly(settings)) { + const stripped = stripKeychainFields(cleanupLegacyFields(settings)); + (stripped as unknown as Record)._keychainOnly = true; + await saveData(stripped); + refreshDiskHasSecrets(stripped); + lastPersistedSettings = structuredClone(settings); + return; + } + await persistSecretsToDisk(settings, saveData); + return; + } + + if (isKeychainOnly(settings)) { + await persistSecretsToKeychain(settings, saveData, prevSettings); + return; + } + + await persistSecretsToDisk(settings, saveData); +} + +// --------------------------------------------------------------------------- +// Rollback helpers +// --------------------------------------------------------------------------- + +/** + * Best-effort restore of keychain contents after a failed persist. + * + * Reason: if `saveData()` or a keychain write throws after some secrets were + * already written, the keychain is left in a partial state. Replay the + * previous settings' secrets so the keychain approximates the last + * known-good state. + * + * Returns `true` iff every restore/tombstone write succeeded AND we had a + * baseline to restore from. A `false` return is the caller's signal that + * keychain state is no longer trustworthy — any "safe to clear disk" gate + * must stay closed until a subsequent clean save proves otherwise. + */ +// DESIGN NOTE — rollback intentionally replays `restoreFrom` verbatim, +// including any legacy `enc_*` placeholders the forward migration cleared. +// Codex review #3234868146 flagged that this can write ciphertext-like +// junk into the keychain during a compound failure. Considered and +// rejected the "filter undecryptable enc_* during rollback" fix: +// +// - Trigger chain is narrow: requires (a) legacy `enc_*` that the +// current device cannot decrypt, (b) the user clicking Migrate, (c) +// at least one forward keychain write succeeding, AND (d) a later +// `saveData()` or keychain write throwing. Each step is uncommon on +// mainstream macOS / Windows builds. +// - Observable failure mode is recoverable auth breakage, not credential +// leakage or disk-secret loss. The next read returns `enc_*` ciphertext, +// `getDecryptedKey()` returns `""`, and the provider call fails with a +// visible "invalid key" error — the user re-enters the key as the +// migration modal already instructs. +// - A successful migration retry tombstones the cleared field from the +// keychain (the forward target has the field cleared, prev still has +// `enc_*`), self-healing the poisoned entry without user action beyond +// re-entering the affected key. +// - The forward path is already hard-guarded at +// `persistSecretsToKeychain` (`Refusing to persist: undecryptable +// secrets`), so the much higher-frequency normal-save path cannot +// poison the keychain. Only rollback is exposed. +// +// If a future codex / human review flags this again, point them at this +// note and at the resolved GitHub thread on PR #2364. +async function restoreKeychainFromSettings( + keychain: KeychainService, + restoreFrom: CopilotSettings | undefined, + failedSettings: CopilotSettings +): Promise { + // Reason: no baseline → we cannot prove the keychain matches a known-good + // state. Treat as unsafe so the lock stays closed. + if (!restoreFrom) return false; + + const { secretEntries, keychainIdsToDelete } = keychain.persistSecrets( + restoreFrom, + failedSettings + ); + + let allOk = true; + + for (const [id, value] of secretEntries) { + try { + keychain.setSecretById(id, value); + } catch (error) { + logWarn(`Failed to restore keychain entry "${id}" during rollback.`, error); + allOk = false; + } + } + + for (const id of keychainIdsToDelete) { + try { + keychain.setSecretById(id, ""); + pendingTombstones.delete(id); + } catch (error) { + logWarn(`Failed to write keychain tombstone "${id}" during rollback.`, error); + allOk = false; + } + } + + return allOk; +} diff --git a/src/services/settingsSecretTransforms.test.ts b/src/services/settingsSecretTransforms.test.ts new file mode 100644 index 00000000..b850a644 --- /dev/null +++ b/src/services/settingsSecretTransforms.test.ts @@ -0,0 +1,214 @@ +jest.mock("@/encryptionService", () => ({ + isSensitiveKey: jest.fn((key: string) => { + const lower = key.toLowerCase(); + const normalized = lower.replace(/[_-]/g, ""); + return ( + normalized.includes("apikey") || + lower.endsWith("token") || + lower.endsWith("accesstoken") || + lower.endsWith("secret") || + lower.endsWith("password") || + lower.endsWith("licensekey") + ); + }), +})); + +import type { CopilotSettings } from "@/settings/model"; +import { + cleanupLegacyFields, + hasPersistedSecrets, + stripKeychainFields, +} from "./settingsSecretTransforms"; + +/** Create a lightweight settings object for transform tests. */ +function makeSettings(overrides: Partial = {}): CopilotSettings { + return { + activeModels: [], + activeEmbeddingModels: [], + ...overrides, + } as unknown as CopilotSettings; +} + +/** JSON-safe clone helper for mutation assertions. */ +function clone(value: T): T { + return JSON.parse(JSON.stringify(value)) as T; +} + +describe("hasPersistedSecrets", () => { + it.each([ + { + name: "detects a non-empty top-level sensitive field", + rawData: { openAIApiKey: "sk-123", temperature: 0.7 }, + expected: true, + }, + { + name: "ignores empty top-level secret values", + rawData: { openAIApiKey: "", temperature: 0.7 }, + expected: false, + }, + { + name: "detects model-level apiKey in activeModels", + rawData: { + activeModels: [{ name: "gpt-4", provider: "openai", apiKey: "model-secret" }], + }, + expected: true, + }, + { + name: "detects model-level apiKey in activeEmbeddingModels", + rawData: { + activeEmbeddingModels: [ + { name: "text-embedding-3-small", provider: "openai", apiKey: "embed-secret" }, + ], + }, + expected: true, + }, + { + name: "ignores non-objects and non-secret fields", + rawData: { + googleApiKey: "", + activeModels: [null, "bad-entry", { name: "gpt-4", provider: "openai" }], + activeEmbeddingModels: [{ name: "embed", provider: "openai", apiKey: "" }], + }, + expected: false, + }, + ])("$name", ({ rawData, expected }) => { + const before = clone(rawData); + expect(hasPersistedSecrets(rawData as unknown as Record)).toBe(expected); + // Reason: hasPersistedSecrets should be read-only + expect(rawData).toEqual(before); + }); +}); + +describe("stripKeychainFields", () => { + it("strips top-level and model-level secrets", () => { + const settings = makeSettings({ + openAIApiKey: "sk-123", + googleApiKey: "g-123", + defaultModelKey: "gpt-4|openai", + activeModels: [ + { name: "gpt-4", provider: "openai", apiKey: "chat-secret", enabled: true }, + { name: "claude-3", provider: "anthropic", apiKey: "chat-secret-2", enabled: true }, + ], + activeEmbeddingModels: [ + { name: "text-embed", provider: "openai", apiKey: "embed-secret", enabled: true }, + ], + }); + const before = clone(settings); + + const result = stripKeychainFields(settings); + + expect(result).not.toBe(settings); + expect(result.openAIApiKey).toBe(""); + expect(result.googleApiKey).toBe(""); + expect((result as unknown as Record).defaultModelKey).toBe("gpt-4|openai"); + expect(result.activeModels[0].apiKey).toBe(""); + expect(result.activeModels[1].apiKey).toBe(""); + expect(result.activeEmbeddingModels[0].apiKey).toBe(""); + // Reason: model objects and arrays must be new references to avoid mutation + expect(result.activeModels).not.toBe(settings.activeModels); + expect(result.activeModels[0]).not.toBe(settings.activeModels[0]); + expect(result.activeEmbeddingModels).not.toBe(settings.activeEmbeddingModels); + // Reason: original should be untouched + expect(settings).toEqual(before); + }); + + it("preserves non-secret fields when there is nothing to strip", () => { + const settings = makeSettings({ + temperature: 0.2, + defaultConversationTag: "copilot", + }); + + const result = stripKeychainFields(settings); + + expect((result as unknown as Record).temperature).toBe(0.2); + expect((result as unknown as Record).defaultConversationTag).toBe("copilot"); + }); +}); + +describe("cleanupLegacyFields", () => { + it("returns a shallow copy without mutating the original", () => { + const settings = makeSettings({ openAIApiKey: "sk-123" }); + const before = clone(settings); + + const result = cleanupLegacyFields(settings); + + expect(result).not.toBe(settings); + expect(result.openAIApiKey).toBe("sk-123"); + expect(settings).toEqual(before); + }); + + it("preserves _keychainVaultId and _keychainOnly", () => { + const settings = makeSettings({ + _keychainVaultId: "abc12345", + _keychainOnly: true, + }); + + const result = cleanupLegacyFields(settings); + const rec = result as unknown as Record; + + expect(rec._keychainVaultId).toBe("abc12345"); + expect(rec._keychainOnly).toBe(true); + }); + + it("strips removed migration fields (_keychainMigratedAt, _migrationModalDismissed)", () => { + const settings = makeSettings({ + _keychainMigratedAt: "2026-04-01T00:00:00.000Z", + _migrationModalDismissed: true, + } as unknown as Partial); + + const result = cleanupLegacyFields(settings); + const rec = result as unknown as Record; + + expect(rec._keychainMigratedAt).toBeUndefined(); + expect(rec._migrationModalDismissed).toBeUndefined(); + }); + + it("migrates legacy _diskSecretsCleared → _keychainOnly", () => { + const settings = makeSettings({ + _diskSecretsCleared: true, + } as unknown as Partial); + + const result = cleanupLegacyFields(settings); + const rec = result as unknown as Record; + + expect(rec._keychainOnly).toBe(true); + // Reason: the legacy field name must be dropped so it never resurfaces. + expect(rec._diskSecretsCleared).toBeUndefined(); + }); + + it("prefers an explicit _keychainOnly when both fields are present", () => { + const settings = makeSettings({ + _diskSecretsCleared: true, + _keychainOnly: false, + } as unknown as Partial); + + const result = cleanupLegacyFields(settings); + const rec = result as unknown as Record; + + expect(rec._keychainOnly).toBe(false); + expect(rec._diskSecretsCleared).toBeUndefined(); + }); + + // Reason: the multi-device downgrade story (older plugin version syncing the + // vault while a newer device has flipped `_keychainOnly = true`) relies on + // every persistence path being "spread, then delete known-legacy" rather + // than "rebuild from a whitelist". A future refactor that whitelists + // recognised keys would silently drop `_keychainOnly` on the older device, + // and on the next sync the newer device would lose its keychain-only mode + // without warning. This test pins the behaviour so such a refactor fails + // loudly here instead of in production. + it("preserves unknown future fields (downgrade safety regression)", () => { + const settings = makeSettings({ + _keychainOnly: true, + _someFutureField: "future-value", + anotherUnknownField: 42, + } as unknown as Partial); + + const result = cleanupLegacyFields(settings); + const rec = result as unknown as Record; + + expect(rec._keychainOnly).toBe(true); + expect(rec._someFutureField).toBe("future-value"); + expect(rec.anotherUnknownField).toBe(42); + }); +}); diff --git a/src/services/settingsSecretTransforms.ts b/src/services/settingsSecretTransforms.ts new file mode 100644 index 00000000..65078bf1 --- /dev/null +++ b/src/services/settingsSecretTransforms.ts @@ -0,0 +1,177 @@ +/** + * Pure transform functions for managing sensitive fields in settings. + * + * These functions share a single "keychain-covered field set" definition: + * - Top-level: any key matching `isSensitiveKey()` + * - Model-level: `apiKey` on each CustomModel + * + * Reason: centralising the field-set avoids drift between the callsites + * (hasPersistedSecrets, stripKeychainFields, cleanupLegacyFields). + */ + +import { DEFAULT_SETTINGS } from "@/constants"; +import { type CopilotSettings } from "@/settings/model"; +import { type CustomModel } from "@/aiParams"; +import { isSensitiveKey } from "@/encryptionService"; +// Reason: do NOT import from @/logger here. The logger depends on getSettings(), +// but this module runs during settings loading (before setSettings). + +/** Model-level fields that are managed by the keychain. */ +export const MODEL_SECRET_FIELDS = ["apiKey"] as const; + +/** + * Canonical list of top-level secret field names known at compile time. + * + * Reason: the keychain hydrate path must NOT rely solely on `Object.keys(settings)` + * because cross-version sync, manual edits, or downgrade-then-upgrade cycles can + * leave a `data.json` that is missing fields whose keychain entries still exist on + * this device. Iterating this constant (in addition to the in-memory keys) ensures + * every default secret field is queried even when the in-memory settings object + * does not list it. + * + * Derived from `DEFAULT_SETTINGS` rather than hand-maintained so new secret fields + * added to the default settings automatically flow through here. + */ +export const TOP_LEVEL_SECRET_FIELDS: readonly string[] = Object.freeze( + Object.keys(DEFAULT_SETTINGS as unknown as Record).filter(isSensitiveKey) +); + +/** Helper to cast CopilotSettings to a Record for dynamic key access. */ +function asRecord(obj: CopilotSettings): Record { + return obj as unknown as Record; +} + +// --------------------------------------------------------------------------- +// hasPersistedSecrets +// --------------------------------------------------------------------------- + +/** + * Check whether `data.json` (the raw on-disk data) still contains any + * non-empty sensitive field values. + * + * Used as a condition for showing the migration modal: if the disk already + * has no secrets, there is nothing to clear. + * + * @param rawData - The raw data as loaded from disk (before hydration). + */ +export function hasPersistedSecrets(rawData: Record): boolean { + // Check top-level sensitive fields + for (const key of Object.keys(rawData)) { + if (!isSensitiveKey(key)) continue; + const value = rawData[key]; + if (typeof value === "string" && value.length > 0) return true; + } + + // Check model-level secrets + for (const listKey of ["activeModels", "activeEmbeddingModels"] as const) { + const models = rawData[listKey]; + if (!Array.isArray(models)) continue; + for (const model of models) { + if (!model || typeof model !== "object") continue; + const rec = model as Record; + for (const field of MODEL_SECRET_FIELDS) { + const value = rec[field]; + if (typeof value === "string" && value.length > 0) return true; + } + } + } + + return false; +} + +// --------------------------------------------------------------------------- +// stripKeychainFields +// --------------------------------------------------------------------------- + +/** + * Return a deep copy of `settings` with all keychain-covered fields set to `""`. + * + * Used when `_diskSecretsCleared === true` — the user has confirmed all + * devices are upgraded, so data.json should no longer carry secrets. + * + * DESIGN NOTE: this iterates `Object.keys(settings)` rather than + * `TOP_LEVEL_SECRET_FIELDS`. That is intentional and not an asymmetry bug. + * `hydrateFromKeychain()` writes any non-empty keychain value back onto the + * in-memory settings object, so every secret that exists in memory also has + * its key present on the object — `Object.keys` is guaranteed to hit it. + * When the keychain has no entry, memory holds no secret either, so there is + * nothing to strip. There is no "secret in memory, sparse on disk" state. + * If a future review flags this again, point them at this note. + * + * @param settings - In-memory settings to strip. + */ +export function stripKeychainFields(settings: CopilotSettings): CopilotSettings { + const out = asRecord({ ...settings }); + + // Strip top-level sensitive fields + for (const key of Object.keys(out)) { + if (!isSensitiveKey(key)) continue; + out[key] = ""; + } + + // Strip model-level secrets + out.activeModels = stripModelSecrets(settings.activeModels ?? []); + out.activeEmbeddingModels = stripModelSecrets(settings.activeEmbeddingModels ?? []); + + return out as unknown as CopilotSettings; +} + +/** Set secret fields to `""` on each model, returning new array. */ +function stripModelSecrets(models: CustomModel[]): CustomModel[] { + if (!models?.length) return models; + + return models.map((model) => { + const copy = { ...model } as unknown as Record; + for (const field of MODEL_SECRET_FIELDS) { + copy[field] = ""; + } + return copy as unknown as CustomModel; + }); +} + +// --------------------------------------------------------------------------- +// cleanupLegacyFields +// --------------------------------------------------------------------------- + +/** + * Remove legacy keychain/encryption fields from a settings object and migrate + * older field names forward. + * + * Called on: + * - Load path (after sanitize, before keychain hydrate) + * - Save path (before writing data.json) + * - Configuration file import path (after apply) + * + * Returns a new object — does NOT mutate the input. + */ +export function cleanupLegacyFields(settings: CopilotSettings): CopilotSettings { + const out = asRecord({ ...settings }); + // Reason: these fields are from earlier dev iterations and should not persist. + delete out.enableEncryption; + delete out._keychainMigrated; + // Reason: the simplified opt-in flow no longer uses these transition markers; + // strip them on every cleanup so they never make it back to data.json. + delete out._keychainMigratedAt; + delete out._migrationModalDismissed; + // Reason: `_diskSecretsCleared` was renamed to `_keychainOnly` for clarity. + // Migrate the value forward only when the new field is absent so a later + // explicit set always wins. + if (out._diskSecretsCleared !== undefined && out._keychainOnly === undefined) { + out._keychainOnly = out._diskSecretsCleared; + } + delete out._diskSecretsCleared; + return out as unknown as CopilotSettings; +} + +// --------------------------------------------------------------------------- +// isKeychainOnly +// --------------------------------------------------------------------------- + +/** + * Whether the OS keychain is the single source of truth for secrets in this + * vault. Centralised check so business code does not sprinkle + * `_keychainOnly === true` comparisons throughout the codebase. + */ +export function isKeychainOnly(settings: CopilotSettings): boolean { + return (settings as unknown as Record)._keychainOnly === true; +} diff --git a/src/settings/model.test.ts b/src/settings/model.test.ts index 5b156e54..5672b890 100644 --- a/src/settings/model.test.ts +++ b/src/settings/model.test.ts @@ -382,3 +382,22 @@ describe("getEffectiveUserPrompt - legacy fallback", () => { expect(result).toBe(""); }); }); + +describe("normalizeModelProvider", () => { + it("maps azure_openai to the EmbeddingModelProviders.AZURE_OPENAI value", () => { + const { normalizeModelProvider } = jest.requireActual<{ + normalizeModelProvider: (provider: string) => string; + }>("@/settings/model"); + // Reason: EmbeddingModelProviders.AZURE_OPENAI = "azure openai" (with space) + expect(normalizeModelProvider("azure_openai")).toBe("azure openai"); + }); + + it("passes through already-normalized and unrelated providers", () => { + const { normalizeModelProvider } = jest.requireActual<{ + normalizeModelProvider: (provider: string) => string; + }>("@/settings/model"); + expect(normalizeModelProvider("azure openai")).toBe("azure openai"); + expect(normalizeModelProvider("openai")).toBe("openai"); + expect(normalizeModelProvider("")).toBe(""); + }); +}); diff --git a/src/settings/model.ts b/src/settings/model.ts index e2cc6d7d..b1430fbc 100644 --- a/src/settings/model.ts +++ b/src/settings/model.ts @@ -97,7 +97,8 @@ export interface CopilotSettings { chatNoteContextTags: string[]; enableIndexSync: boolean; debug: boolean; - enableEncryption: boolean; + /** @deprecated Removed — keychain is now the sole encryption mechanism. */ + enableEncryption?: never; maxSourceChunks: number; enableInlineCitations: boolean; qaExclusions: string; @@ -201,6 +202,22 @@ export interface CopilotSettings { autoCompactThreshold: number; /** Folder where converted document markdown files are saved */ convertedDocOutputFolder: string; + /** + * When `true`, the OS keychain is the single source of truth for secrets; + * data.json must never contain plaintext secret values. + * + * Set on: + * - Fresh installs (no prior data.json) when keychain is available + * - User clicking "Migrate to Keychain" in Advanced Settings + * - `forgetAllSecrets` (after stripping disk + clearing keychain) + */ + _keychainOnly?: boolean; + /** + * Stable namespace ID for keychain entries, persisted once on first use. + * Reason: using a persisted ID (instead of deriving from vault path) means + * renaming or moving the vault folder does not orphan keychain entries. + */ + _keychainVaultId?: string; } export const settingsStore = createStore(); @@ -287,6 +304,15 @@ export function getSettings(): Readonly { /** * Resets the settings to the default values. + * + * DESIGN NOTE — does NOT clear secrets from the Obsidian Keychain. Reset only + * rewrites `data.json` to defaults; a keychain-only vault keeps its OS keychain + * entries. "Delete All Keys" (Advanced Settings → API Key Storage, backed by + * `KeychainService.forgetAllSecrets`) is the dedicated path for erasing keychain + * secrets. Wiring that async transaction into this synchronous reset would pull + * the keychain service and its callbacks through `SettingsMainV2`, and is + * intentionally left out of the first-stage migration. + * If a future review flags this again, point them at this note. */ export function resetSettings(): void { const defaultSettingsWithBuiltIns = { @@ -321,6 +347,14 @@ export function useSettingsValue(): Readonly { }); } +/** + * Normalize persisted model provider values so identity keys stay stable across migrations. + * Reason: Legacy data may store "azure_openai" while runtime uses "azure-openai". + */ +export function normalizeModelProvider(provider: string): string { + return provider === "azure_openai" ? EmbeddingModelProviders.AZURE_OPENAI : provider; +} + /** * Sanitizes the settings to ensure they are valid. * Note: This will be better handled by Zod in the future. @@ -351,7 +385,7 @@ export function sanitizeSettings(settings: CopilotSettings): CopilotSettings { settingsToSanitize.activeEmbeddingModels = settingsToSanitize.activeEmbeddingModels.map((m) => { return { ...m, - provider: m.provider === "azure_openai" ? EmbeddingModelProviders.AZURE_OPENAI : m.provider, + provider: normalizeModelProvider(m.provider), }; }); } diff --git a/src/settings/v2/components/AdvancedSettings.tsx b/src/settings/v2/components/AdvancedSettings.tsx index bf07f19d..9b347d38 100644 --- a/src/settings/v2/components/AdvancedSettings.tsx +++ b/src/settings/v2/components/AdvancedSettings.tsx @@ -4,23 +4,96 @@ import { ObsidianNativeSelect } from "@/components/ui/obsidian-native-select"; import { useApp } from "@/context"; import { logFileManager } from "@/logFileManager"; import { flushRecordedPromptPayloadToLog } from "@/LLMProviders/chainRunner/utils/promptPayloadRecorder"; -import { updateSetting, useSettingsValue } from "@/settings/model"; -import { ArrowUpRight, Plus } from "lucide-react"; -import React from "react"; +import { KeychainService } from "@/services/keychainService"; +import { + canClearDiskSecrets, + hasDiskSecretsToMigrate, + migrateDiskSecretsToKeychain, + refreshDiskHasSecrets, + refreshLastPersistedSettings, + runPersistenceTransaction, + suppressNextPersistOnce, +} from "@/services/settingsPersistence"; +import { hasPersistedSecrets, isKeychainOnly } from "@/services/settingsSecretTransforms"; +import { logError } from "@/logger"; +import { + type CopilotSettings, + setSettings, + updateSetting, + useSettingsValue, +} from "@/settings/model"; +import { ArrowUpRight, Info, Plus, ShieldCheck, Trash2, Unlock } from "lucide-react"; +import { ConfirmModal } from "@/components/modals/ConfirmModal"; +import { MigrateConfirmModal } from "@/components/modals/MigrateConfirmModal"; +import { type App, Notice } from "obsidian"; +import React, { useCallback, useState } from "react"; import { getPromptFilePath, SystemPromptAddModal } from "@/system-prompts"; import { useSystemPrompts } from "@/system-prompts/state"; +/** + * Returns a `saveData` callback bound to the loaded Copilot plugin instance. + * + * Reason: settings React components don't have direct access to the plugin, + * so persistence transactions look it up via the Obsidian `App`. Kept as a + * single helper to centralise the `app.plugins` cast (untyped in the Obsidian + * API) and the "plugin not found" guard at every call site. + */ +function getCopilotSaveData(app: App): (data: CopilotSettings) => Promise { + return async (data: CopilotSettings) => { + const { plugins } = app as unknown as { + plugins: { + getPlugin: (id: string) => { saveData: (data: CopilotSettings) => Promise } | null; + }; + }; + const copilotPlugin = plugins.getPlugin("copilot"); + if (!copilotPlugin) throw new Error("Copilot plugin not found"); + await copilotPlugin.saveData(data); + }; +} + export const AdvancedSettings: React.FC = () => { const app = useApp(); const settings = useSettingsValue(); const prompts = useSystemPrompts(); + const [forgetting, setForgetting] = useState(false); + const [migrating, setMigrating] = useState(false); + + const keychainAvailable = KeychainService.getInstance().isAvailable(); + const keychainOnly = isKeychainOnly(settings); + // Reason: `hasDiskSecretsToMigrate()` only refreshes after persist runs, + // so React doesn't re-render when the user types a new key. Union with + // current in-memory `settings` presence so the Migrate / Delete CTAs + // appear immediately. Matches the gate used in `canClearDiskSecrets()`. + const diskHasSecrets = + hasDiskSecretsToMigrate() || + (!keychainOnly && hasPersistedSecrets(settings)); + const canMigrate = canClearDiskSecrets(settings); + + // Reason: a single status string is easier to reason about and debug than + // a constellation of booleans. Order matters — earlier branches short-circuit. + // The mode itself is `_keychainOnly`, not the absence of disk secrets — an + // existing user with no keys configured is still in disk mode and would write + // any newly entered key back to data.json. The "blocked" state is reserved + // for the genuine "has keys but cannot migrate" case so its warning copy + // remains accurate. The "stranded" state is reserved for a keychain-only + // vault opened on a build without SecretStorage — saves silently strip + // secrets (Fix 2), so the user MUST be told their inputs won't persist here. + const storageStatus: "unavailable" | "stranded" | "active" | "standard" | "blocked" = + !keychainAvailable + ? keychainOnly + ? "stranded" + : "unavailable" + : keychainOnly + ? "active" + : diskHasSecrets && !canMigrate + ? "blocked" + : "standard"; // Check if the default system prompt exists in the current prompts list const defaultPromptExists = prompts.some( (prompt) => prompt.title === settings.defaultSystemPromptTitle ); - // Display value: use the default prompt if it exists, otherwise empty string (will show placeholder) const displayValue = defaultPromptExists ? settings.defaultSystemPromptTitle : ""; const handleSelectChange = (value: string) => { @@ -40,11 +113,136 @@ export const AdvancedSettings: React.FC = () => { modal.open(); }; + const handleForgetAllSecrets = useCallback(async () => { + if (forgetting) return; + + // Reason: double-confirm destructive action via project ConfirmModal + const confirmed = await new Promise((resolve) => { + new ConfirmModal( + app, + () => resolve(true), + "This will remove all API keys for this vault from the Obsidian Keychain, data.json, " + + "and memory. You will need to re-enter them.", + "\u26A0\uFE0F Forget All Secrets", + "Remove", + "Cancel", + () => resolve(false) + ).open(); + }); + if (!confirmed) return; + + setForgetting(true); + try { + const keychain = KeychainService.getInstance(); + const saveData = getCopilotSaveData(app); + + // Reason: run inside the persistence queue to prevent interleaving + // with normal saves that could restore old secrets. + let skipSuppress = false; + await runPersistenceTransaction(() => + keychain.forgetAllSecrets( + saveData, + refreshDiskHasSecrets, + (nextSettings) => { + refreshLastPersistedSettings(nextSettings as CopilotSettings); + if (!skipSuppress) { + suppressNextPersistOnce(); + } + setSettings(nextSettings); + }, + // Reason: disk save failed → forgetAllSecrets is a no-op (no memory + // or keychain changes). This callback is reserved for future retry logic. + () => { + skipSuppress = true; + } + ) + ); + } catch (error) { + logError("Failed to forget secrets.", error); + new Notice("Failed to remove API keys. Please try again."); + } finally { + setForgetting(false); + } + }, [app, forgetting]); + + /** + * Move all secrets to the Keychain in a single transaction: + * write keychain → strip data.json → set _keychainOnly=true. + * + * The MigrateConfirmModal gates the action behind a checkbox so the + * multi-device trade-off is acknowledged before any destructive write. + */ + const handleMigrate = useCallback(async () => { + if (migrating) return; + + // DESIGN NOTE — `migrating` only flips true after the confirm modal + // closes, so a fast double-click can open two modals. Intentionally not + // pre-gating because the second confirmed run is already serialized by + // `runPersistenceTransaction` and short-circuited by `canClearDiskSecrets` + // (returns "no secrets left to migrate"), producing a benign error + // Notice. Adding a pre-modal guard adds state for a cosmetic UX wart. + // If a future review flags this again, point them at this note. + const confirmed = await new Promise((resolve) => { + new MigrateConfirmModal( + app, + () => resolve(true), + () => resolve(false) + ).open(); + }); + if (!confirmed) return; + + setMigrating(true); + try { + const saveData = getCopilotSaveData(app); + // Reason: migrateDiskSecretsToKeychain owns the full transaction — + // write keychain, strip disk, flip _keychainOnly, with rollback on + // partial failure. Returns the list of legacy enc_* fields that could + // not be decrypted and were cleared instead of migrated; the user + // needs to re-enter those keys. + const result = await migrateDiskSecretsToKeychain(saveData); + if (result.fieldsRequiringReentry.length > 0) { + const fieldList = result.fieldsRequiringReentry.map((f) => ` • ${f}`).join("\n"); + new Notice( + `API keys moved to Obsidian Keychain.\n\n${result.fieldsRequiringReentry.length} key(s) could not be decrypted and need to be re-entered:\n${fieldList}`, + // Reason: duration=0 keeps the notice up until the user dismisses + // it. The default 10s is too short to read a field list and act on + // it. Do NOT shorten — see TERMINOLOGY/PARTIAL-SUCCESS note below. + 0 + ); + } else { + new Notice("API keys moved to Obsidian Keychain."); + } + } catch (error) { + logError("Failed to migrate secrets to Obsidian Keychain.", error); + new Notice("Failed to migrate to Obsidian Keychain. Please try again."); + } finally { + setMigrating(false); + } + }, [app, migrating]); + + // DESIGN NOTE — do NOT "fix" the migration UX by making it fail closed when + // undecryptable enc_* fields exist: + // - End state is identical (user re-enters the listed keys either way). + // - Partial-success takes 2 user steps (Migrate → re-enter). + // Fail-closed takes 3 (try Migrate → re-enter → retry Migrate). + // - The multi-device "ciphertext recoverability" argument is already + // handled by MigrateConfirmModal's explicit "Other devices will need to + // re-enter their API keys" disclosure. + // See the matching note above `collectUndecryptableFields` in + // settingsPersistence.ts. + // + // TERMINOLOGY NOTE — user-facing copy uses "Obsidian Keychain" / "Obsidian's + // Keychain" (or bare "Keychain" inside a Keychain-context paragraph). It is + // vault-scoped storage managed by Obsidian via SecretStorage. Do NOT change + // to "OS Keychain" (overpromises OS-level guarantees) or "Secure Storage" + // (too generic, loses the Keychain mental model). See the matching note in + // MigrateConfirmModal.tsx. + return (
{/* User System Prompt Section */}
-

User System Prompt

+
User System Prompt
{ {/* Others Section */}
-

Others

+
Others
+ + {/* API Key Storage — five-state UI driven by storageStatus. + Right column stacks pill + buttons vertically. Blocked state reuses + standard's visual shape but disables Migrate and exposes the reason + via the button's tooltip. Stranded state is for keychain-only vaults + opened on a build without SecretStorage: saves silently strip + secrets, so we warn the user and hide the Migrate/Delete buttons. */} + + Update Obsidian to 1.11.4+ to enable the{" "} + Obsidian Keychain. Keys + are stored in data.json. + + ) : storageStatus === "stranded" ? ( + <> + This vault uses the{" "} + Obsidian Keychain, but + this Obsidian build cannot access it. Keys you enter here won't persist. Update + Obsidian to 1.11.4+, or re-enter keys on a supported device. + + ) : storageStatus === "active" ? ( + <> + API keys are stored in this device's{" "} + Obsidian Keychain. + + ) : ( + <> + Your API keys are stored in data.json. Move them to the{" "} + Obsidian Keychain for + stronger protection. + + ) + } + > +
+ {storageStatus === "active" && ( +
+ + Obsidian Keychain +
+ )} + {(storageStatus === "standard" || storageStatus === "blocked") && ( +
+ + Standard Storage +
+ )} + {storageStatus === "unavailable" && ( +
+ + Unavailable +
+ )} + {storageStatus === "stranded" && ( +
+ + Obsidian Keychain unavailable +
+ )} + + {storageStatus === "blocked" && ( + // Reason: visible explanation for the blocked state — disabled + // button `title` alone is unreliable on mobile and assistive tech. +
+ Save your settings once, then try again. +
+ )} + + {(storageStatus === "standard" || storageStatus === "blocked") && diskHasSecrets && ( + + )} + + {/* Reason: only show Delete when secrets actually exist somewhere. + Hiding it in standard/no-secret state prevents accidentally + flipping the vault to keychain-only mode with no real cleanup. + In "stranded" state the action is disabled — this build can't + reach the Obsidian Keychain to actually clear the entries, so + pretending to delete them would let secrets resurface after + upgrading. The button stays visible (rather than hidden) so the + user understands why the action is unavailable here. */} + {(keychainOnly || diskHasSecrets) && ( + + )} +
+ { - updateSetting("debug", checked); - }} + onCheckedChange={(checked) => updateSetting("debug", checked)} /> =1.11.4, but the + // package pinned in package.json (^1.2.5) ships an older `.d.ts` that lacks + // it entirely. Declare the full shape here so the project compiles against + // older obsidian types without bumping the dev dependency (which would also + // shift @codemirror peers and widen this PR's blast radius). `deleteSecret` + // is intentionally optional — it exists at runtime since 1.11.4 but remains + // undocumented, so callers feature-detect it. + interface SecretStorage { + setSecret(id: string, secret: string): void; + getSecret(id: string): string | null; + listSecrets(): string[]; + deleteSecret?(id: string): void; + } + + interface App { + secretStorage?: SecretStorage; + } } export enum PromptSortStrategy { diff --git a/src/utils/curlCommand.ts b/src/utils/curlCommand.ts index 6592bbec..a55532e2 100644 --- a/src/utils/curlCommand.ts +++ b/src/utils/curlCommand.ts @@ -95,7 +95,7 @@ async function resolveApiKeyForCurl( try { const decrypted = (await getDecryptedKey(trimmed))?.trim(); - if (!decrypted || decrypted === "Copilot failed to decrypt API keys!") { + if (!decrypted) { warnings.push("API key could not be decrypted; using placeholder."); return { apiKey: "", warnings }; } @@ -245,9 +245,18 @@ async function buildOpenAICompatibleRequestSpec( Authorization: `Bearer ${apiKeyResolved.apiKey}`, }; - // Add OpenAI org ID if present + // Add OpenAI org ID if present (may be encrypted at rest) if (model.openAIOrgId?.trim()) { - headers["OpenAI-Organization"] = model.openAIOrgId.trim(); + try { + const orgId = (await getDecryptedKey(model.openAIOrgId))?.trim(); + if (orgId) { + headers["OpenAI-Organization"] = orgId; + } else { + warnings.push("OpenAI organization ID could not be decrypted; omitting header."); + } + } catch { + warnings.push("OpenAI organization ID could not be decrypted; omitting header."); + } } // Add OpenRouter-specific headers (see chatModelManager.ts:259-262)