From 34fa3c5507bad67cc448f6c7dfc076aa74ccf0aa Mon Sep 17 00:00:00 2001 From: Emt-lin <41323133+Emt-lin@users.noreply.github.com> Date: Fri, 15 May 2026 04:57:21 +0800 Subject: [PATCH] feat(keychain): migrate API key storage to Obsidian Keychain (#2364) * feat(keychain): migrate API key storage to Obsidian Keychain Replaces the legacy disk-encryption flow with Obsidian's vault-scoped Keychain (SecretStorage) as the source of truth for API keys. Migration is explicit and opt-in via Advanced Settings; existing vaults stay on disk mode until the user clicks "Migrate to Obsidian Keychain". Architecture - `KeychainService` (singleton) wraps Obsidian's SecretStorage with a per-vault namespace (`copilot-v{8hex}-{field}`) so multiple vaults on one device cannot collide. - `settingsPersistence` is the single owner of load/save: - `loadSettingsWithKeychain()` dispatches by `_keychainOnly` flag. - `doPersist()` writes either keychain (stripped data.json) or disk (plaintext data.json, never new `enc_*`). - Write queue + transaction epoch serialise every persist with dedicated transactions (forgetAllSecrets, migrate). - `settingsSecretTransforms` provides the pure helpers shared by both sides (stripKeychainFields, cleanupLegacyFields, isKeychainOnly, ...). - `encryptionService` shrinks to decrypt-only + base64/sensitive-key helpers; new encryption writes are gone. Migration model - Fresh installs auto-opt-in to keychain mode; existing installs stay disk-mode until the user clicks Migrate. - `migrateDiskSecretsToKeychain()` is a single transaction: write keychain -> strip data.json -> flip `_keychainOnly`. Rolls back the keychain on partial-write or disk-save failure, and re-derives from live settings at transaction end so concurrent edits are preserved. - Undecryptable legacy `enc_*` values are cleared and reported via `MigrationResult.fieldsRequiringReentry`. Persist hardening - Stranded vaults (keychain-only mode on a non-SecretStorage build) preserve `_keychainOnly` and never load plaintext from disk or silently downgrade to disk mode. - `persistHadUndecryptableSecrets` fail-closed guard blocks disk-clear when the last keychain save did not complete safely; a clean disk-mode save lifts the lock so migration retry works. - Destructive flows (clearAllVaultSecrets, forgetAllSecrets) feature- detect `listSecrets()` and refuse before stripping disk on builds where keychain entries cannot be enumerated. UI - New `MigrateConfirmModal` gates migration behind an acknowledgement checkbox surfacing the multi-device trade-off. - API Key Storage panel exposes active / standard / blocked / unavailable / stranded states. - `canClearDiskSecrets()` falls back to in-memory settings so the Migrate / Delete-All CTAs surface immediately after a key is typed. Testing - New suites for keychainService, settingsPersistence, settingsSecretTransforms, MigrateConfirmModal, password-input; encryptionService tests rewritten for the decrypt-only surface. - DESIGN NOTEs lock the trade-offs triaged during review (sparse bootstrap write, rollback enc_* replay, hasEncryptionPrefix guard, unconditional epoch bump, etc.) to prevent future review churn. * docs(keychain): document intentional first-stage migration residue Add a DESIGN NOTE in migrateDiskSecretsToKeychain explaining why keychain residue from a failed first-stage persist is intentionally left uncleaned: it stays dormant in disk-mode and self-heals on the next successful migration or Delete All Keys. * docs(keychain): clarify Reset Settings does not clear keychain secrets Reset Settings rewrites data.json to defaults but does not enumerate or remove Obsidian Keychain entries. The troubleshooting doc still promised Reset would delete all API keys, which is false for keychain-only vaults. - Correct the Reset warning to point users to "Delete All Keys" for erasing keychain secrets - Add a DESIGN NOTE on resetSettings() explaining the omission is intentional for the first-stage migration * style(keychain): emphasize data.json and Keychain labels in migrate modal Add subtle background and accent coloring to the `data.json` and `Obsidian Keychain` inline code labels so the source and destination of the migration read more clearly. --- docs/troubleshooting-and-faq.md | 11 +- .../modals/MigrateConfirmModal.test.tsx | 118 ++ src/components/modals/MigrateConfirmModal.tsx | 156 +++ src/components/ui/password-input.test.tsx | 136 +++ src/components/ui/password-input.tsx | 72 +- src/constants.ts | 1 - src/encryptionService.test.ts | 321 +++-- src/encryptionService.ts | 333 ++++-- src/logFileManager.ts | 9 +- src/main.ts | 37 +- src/services/keychainService.test.ts | 613 ++++++++++ src/services/keychainService.ts | 738 ++++++++++++ src/services/settingsPersistence.test.ts | 1029 +++++++++++++++++ src/services/settingsPersistence.ts | 960 +++++++++++++++ src/services/settingsSecretTransforms.test.ts | 214 ++++ src/services/settingsSecretTransforms.ts | 177 +++ src/settings/model.test.ts | 19 + src/settings/model.ts | 38 +- .../v2/components/AdvancedSettings.tsx | 326 +++++- src/settings/v2/utils/modelActions.ts | 3 + src/types.ts | 18 + src/utils/curlCommand.ts | 15 +- 22 files changed, 5131 insertions(+), 213 deletions(-) create mode 100644 src/components/modals/MigrateConfirmModal.test.tsx create mode 100644 src/components/modals/MigrateConfirmModal.tsx create mode 100644 src/components/ui/password-input.test.tsx create mode 100644 src/services/keychainService.test.ts create mode 100644 src/services/keychainService.ts create mode 100644 src/services/settingsPersistence.test.ts create mode 100644 src/services/settingsPersistence.ts create mode 100644 src/services/settingsSecretTransforms.test.ts create mode 100644 src/services/settingsSecretTransforms.ts 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)