From f2169a1235ac149be1d652cf9d96092f6e9955f2 Mon Sep 17 00:00:00 2001 From: Lucas Winther Date: Fri, 30 Oct 2026 20:49:04 +0100 Subject: [PATCH] fix(ai): bind sealed secrets to purpose and row; never drop unreadable secrets Review fix round 1. - Secret box v2 tokens: one HKDF key per purpose (byok, server-api-key, mcp) and AES-GCM AAD bound to the row (user id + provider; server id + field). MCP env and headers are sealed per field. open() refuses v1 tokens; the data upgrade re-seals v1 keys, v1 plaintext MCP values and the draft secretCipher, and marks ai_secrets_v2 only when nothing legacy is left (retried until the previous secret is back). - Unreadable secrets are reported (admin: unreadableSecrets with server and field; user: byok.keyUnreadable), logged at startup, and kept on save unless replaced or cleared (clearApiKey, clearSecrets). The admin and account pages show the warnings. - allowedOrigins and provider resolution no longer open MCP secrets. Co-Authored-By: Claude Opus 5.5 --- packages/feature-ai/src/crypto.ts | 89 +++++++++---- packages/feature-ai/src/feature.ts | 30 +++-- packages/feature-ai/src/index.ts | 2 +- packages/feature-ai/src/mcp/secrets.ts | 119 ++++++++++++++---- packages/feature-ai/src/settings.ts | 85 ++++++++++--- packages/feature-ai/src/upgrade.ts | 85 ++++++++++--- packages/feature-ai/src/web/AccountAiPage.tsx | 7 +- packages/feature-ai/src/web/AdminAiPage.tsx | 22 +++- packages/feature-ai/src/web/components.tsx | 22 ++++ packages/feature-ai/test/crypto.test.ts | 46 +++++-- packages/feature-ai/test/feature.test.ts | 30 +++++ packages/feature-ai/test/settings.test.ts | 95 +++++++++++++- packages/feature-ai/test/upgrade.test.ts | 90 +++++++++++-- packages/feature-ai/test/web.test.tsx | 17 +++ 14 files changed, 612 insertions(+), 127 deletions(-) diff --git a/packages/feature-ai/src/crypto.ts b/packages/feature-ai/src/crypto.ts index 10f6456..a139844 100644 --- a/packages/feature-ai/src/crypto.ts +++ b/packages/feature-ai/src/crypto.ts @@ -1,41 +1,82 @@ /** - * Secret box for API keys at rest: AES-GCM-256 with a key derived by HKDF-SHA-256 from STC_SECRET. - * Token format: `v1..`. + * Secret box for secrets at rest (API keys, MCP env and headers): AES-GCM-256 with keys derived by HKDF-SHA-256 + * from STC_SECRET. + * + * - `v2..` (v1.1): each purpose has its own HKDF key (`info` = + * `stcourses//v2`), and the AES-GCM additional data binds the token to the row it belongs to (for + * example the user id and provider of a BYOK key, or the MCP server id and field). A token copied to another + * row or purpose does not open, so the server cannot be used to decrypt one secret as another. + * - `v1..` (v1): one key for everything (info `aes-gcm-256/v1`), no additional data. Only the v1.1 data + * upgrade opens these, to re-seal them as v2 (`openLegacy`); `open` refuses them. */ +export type SecretPurpose = "byok" | "server-api-key" | "mcp"; + export interface SecretBox { - encrypt(plain: string): Promise; - decrypt(token: string): Promise; + seal(plain: string, purpose: SecretPurpose, aad: string): Promise; + open(token: string, purpose: SecretPurpose, aad: string): Promise; + /** v1 tokens only; for the data upgrade. */ + openLegacy(token: string): Promise; } const enc = new TextEncoder(); const dec = new TextDecoder(); const b64 = (u: Uint8Array) => Buffer.from(u).toString("base64"); -const unb64 = (s: string) => new Uint8Array(Buffer.from(s, "base64")); +const unb64 = (s: string): Uint8Array => new Uint8Array(Buffer.from(s, "base64")); +const SALT = enc.encode("stcourses/byok"); + +export const isLegacyToken = (token: string): boolean => token.startsWith("v1."); + +function deriveKey(secret: string, info: string): Promise { + return (async () => { + const ikm = await crypto.subtle.importKey("raw", enc.encode(secret), "HKDF", false, ["deriveKey"]); + return crypto.subtle.deriveKey({ name: "HKDF", hash: "SHA-256", salt: SALT, info: enc.encode(info) }, ikm, { name: "AES-GCM", length: 256 }, false, ["encrypt", "decrypt"]); + })(); +} + +function parseToken(token: string, version: "v1" | "v2"): { iv: Uint8Array; ct: Uint8Array } { + const [v, iv, ct, extra] = token.split("."); + if (v !== version || !iv || !ct || extra !== undefined) throw new Error("Unrecognised secret format"); + return { iv: unb64(iv), ct: unb64(ct) }; +} + +/** Additional data: purpose and row identity, length-prefixed so no two (purpose, aad) pairs encode alike. */ +const additionalData = (purpose: SecretPurpose, aad: string) => enc.encode(`stc/${purpose.length}:${purpose}/${aad.length}:${aad}`); export function createSecretBox(secret: string): SecretBox { - let keyP: Promise | null = null; - const key = () => - (keyP ??= (async () => { - const ikm = await crypto.subtle.importKey("raw", enc.encode(secret), "HKDF", false, ["deriveKey"]); - return crypto.subtle.deriveKey( - { name: "HKDF", hash: "SHA-256", salt: enc.encode("stcourses/byok"), info: enc.encode("aes-gcm-256/v1") }, - ikm, - { name: "AES-GCM", length: 256 }, - false, - ["encrypt", "decrypt"], - ); - })()); + const keys = new Map>(); + const key = (info: string) => { + let k = keys.get(info); + if (!k) keys.set(info, (k = deriveKey(secret, info))); + return k; + }; return { - async encrypt(plain) { + async seal(plain, purpose, aad) { const iv = crypto.getRandomValues(new Uint8Array(12)); - const ct = new Uint8Array(await crypto.subtle.encrypt({ name: "AES-GCM", iv }, await key(), enc.encode(plain))); - return `v1.${b64(iv)}.${b64(ct)}`; + const ct = new Uint8Array(await crypto.subtle.encrypt({ name: "AES-GCM", iv, additionalData: additionalData(purpose, aad) }, await key(`stcourses/${purpose}/v2`), enc.encode(plain))); + return `v2.${b64(iv)}.${b64(ct)}`; }, - async decrypt(token) { - const [v, iv, ct] = token.split("."); - if (v !== "v1" || !iv || !ct) throw new Error("Unrecognised secret format"); - const plain = await crypto.subtle.decrypt({ name: "AES-GCM", iv: unb64(iv) }, await key(), unb64(ct)); + async open(token, purpose, aad) { + const { iv, ct } = parseToken(token, "v2"); + const plain = await crypto.subtle.decrypt({ name: "AES-GCM", iv, additionalData: additionalData(purpose, aad) }, await key(`stcourses/${purpose}/v2`), ct); return dec.decode(plain); }, + async openLegacy(token) { + const { iv, ct } = parseToken(token, "v1"); + return dec.decode(await crypto.subtle.decrypt({ name: "AES-GCM", iv }, await key("aes-gcm-256/v1"), ct)); + }, }; } + +/** The v1 token format, exactly as v1 wrote it. Only upgrade tests use it, to build v1 data. */ +export async function legacyV1Encrypt(secret: string, plain: string): Promise { + const iv = crypto.getRandomValues(new Uint8Array(12)); + const ct = new Uint8Array(await crypto.subtle.encrypt({ name: "AES-GCM", iv }, await deriveKey(secret, "aes-gcm-256/v1"), enc.encode(plain))); + return `v1.${b64(iv)}.${b64(ct)}`; +} + +/** AAD for a user's own key: user id and provider kind (the user_api_keys row). */ +export const byokAad = (userId: string, provider: string) => `${userId}\n${provider}`; +/** AAD for the admin's provider key (one per server). */ +export const SERVER_KEY_AAD = "server"; +/** AAD for an MCP server's env or headers: server id and field. */ +export const mcpAad = (serverId: string, field: "env" | "headers") => `${serverId}\n${field}`; diff --git a/packages/feature-ai/src/feature.ts b/packages/feature-ai/src/feature.ts index 2803f81..2169bcb 100644 --- a/packages/feature-ai/src/feature.ts +++ b/packages/feature-ai/src/feature.ts @@ -91,6 +91,10 @@ function parseMcp(v: unknown): McpServerConfig[] | "invalid" { if (s.headers && typeof s.headers === "object") cfg.headers = Object.fromEntries(Object.entries(s.headers).map(([k, val]) => [k, String(val)])); } if (Array.isArray(s.courseIds)) cfg.courseIds = s.courseIds.map(String).filter(Boolean); + if (s.clearSecrets !== undefined) { + if (!Array.isArray(s.clearSecrets) || !s.clearSecrets.every((f) => f === "env" || f === "headers")) return "invalid"; + if (s.clearSecrets.length) cfg.clearSecrets = [...new Set(s.clearSecrets as ("env" | "headers")[])]; + } out.push(cfg); } return out; @@ -147,10 +151,16 @@ export function aiFeature(ctx: ServerContext, opts: AiFeatureOptions = {}): Feat const sink = opts.draftSink ?? createFsDraftSink(ctx.config.dataDir); const log = opts.log ?? ((m: string) => console.error(`[ai] ${m}`)); // The v1 data upgrade (mentor toggles, MCP secrets) starts now; every settings call also waits for it. - settings.ready().catch((e) => log(`v1.1 data upgrade failed (retried on next use): ${(e as Error).message}`)); + // Unreadable secrets are kept, never dropped; say so once at startup (the admin page shows the details). + settings.ready() + .then(() => settings.getServer()) + .then((s) => { + if (s.unreadableSecrets.length) log(`${s.unreadableSecrets.length} stored AI secret(s) cannot be decrypted with the current STC_SECRET; they are kept until re-entered or the previous secret is restored`); + }) + .catch((e) => log(`v1.1 data upgrade failed (retried on next use): ${(e as Error).message}`)); // AppConfig.aiAllowedBaseUrls (STC_AI_ALLOWED_BASE_URLS) plus the admin's list in the server AI settings. const configOrigins = parseAllowedOrigins((ctx.config.aiAllowedBaseUrls ?? []).join(",")); - const allowedOrigins = async () => [...new Set([...configOrigins, ...(await settings.getServer()).allowedBaseUrls])]; + const allowedOrigins = async () => [...new Set([...configOrigins, ...(await settings.allowedBaseUrls())])]; /** * Providers for a user's own key never follow redirects, and their errors reach the user only as the HTTP * status plus a fixed sentence (the full text is logged here). Server-scope providers are the admin's own. @@ -172,12 +182,12 @@ export function aiFeature(ctx: ServerContext, opts: AiFeatureOptions = {}): Feat if (key && base) return { source: "byok", cfg: { ...u.byok, baseUrl: base }, key }; } } - const s = await settings.getServer(); - if (!s.provider) return null; - if (s.provider.kind === "anthropic" && !s.provider.hasApiKey) return null; - const key = s.provider.hasApiKey ? await settings.serverApiKey().catch(() => null) : null; - if (s.provider.kind === "anthropic" && !key) return null; - return { source: "server", cfg: s.provider, key }; + const provider = await settings.serverProvider(); + if (!provider) return null; + if (provider.kind === "anthropic" && !provider.hasApiKey) return null; + const key = provider.hasApiKey ? await settings.serverApiKey().catch(() => null) : null; + if (provider.kind === "anthropic" && !key) return null; + return { source: "server", cfg: provider, key }; }; const viewable = async (user: AuthUser | null, courseId: string): Promise => { @@ -378,8 +388,8 @@ export function aiFeature(ctx: ServerContext, opts: AiFeatureOptions = {}): Feat GET: async (_req, rc) => { if (!rc.user) return err(401, "Sign in first."); const u = await settings.getUser(rc.user.id); - const s = await settings.getServer(); - return json({ byok: u.byok, serverProvider: s.provider ? { kind: s.provider.kind, model: s.provider.model ?? null } : null }); + const p = await settings.serverProvider(); + return json({ byok: u.byok, serverProvider: p ? { kind: p.kind, model: p.model ?? null } : null }); }, }, "/api/ai/account/key": { diff --git a/packages/feature-ai/src/index.ts b/packages/feature-ai/src/index.ts index 2582896..d0cae5b 100644 --- a/packages/feature-ai/src/index.ts +++ b/packages/feature-ai/src/index.ts @@ -2,7 +2,7 @@ export { createSecretBox, type SecretBox } from "./crypto"; export { migrationsDirs } from "./migrations"; export * from "./settings"; -export { upgradeAiData, AI_DATA_UPGRADE } from "./upgrade"; +export { upgradeAiData, AI_DATA_UPGRADE, AI_SECRETS_UPGRADE } from "./upgrade"; export * from "./providers"; export * from "./retrieval/course-index"; export { Bm25, tokenize } from "./retrieval/bm25"; diff --git a/packages/feature-ai/src/mcp/secrets.ts b/packages/feature-ai/src/mcp/secrets.ts index 210dc1f..f0ed5d9 100644 --- a/packages/feature-ai/src/mcp/secrets.ts +++ b/packages/feature-ai/src/mcp/secrets.ts @@ -1,12 +1,33 @@ -import type { SecretBox } from "../crypto"; +import { mcpAad, type SecretBox } from "../crypto"; import type { McpServerConfig } from "../settings"; /** - * MCP `env` and `headers` at rest: both move into `secretCipher`, sealed with the same AES-GCM/HKDF box as API - * keys. The admin API only ever sees masked values; server code opens them to start or call an MCP server. + * MCP `env` and `headers` at rest: each field is sealed on its own (`envCipher`, `headersCipher`) with the "mcp" + * purpose key and the server id plus field name as additional data, so a cipher moved to another server, field or + * table does not open. The admin API only ever sees masked values; server code opens them to start or call a server. + * + * Older shapes, re-sealed by the data upgrade (src/upgrade.ts): plaintext `env`/`headers` (v1) and one v1-token + * `secretCipher` holding `{ env, headers }` (the first v1.1 draft). */ -export type StoredMcpServer = Omit & { secretCipher?: string; env?: Record; headers?: Record }; -type Secrets = { env?: Record; headers?: Record }; +export type SecretField = "env" | "headers"; +export const SECRET_FIELDS: SecretField[] = ["env", "headers"]; +const CIPHER: Record = { env: "envCipher", headers: "headersCipher" }; + +export type StoredMcpServer = Omit & { + envCipher?: string; + headersCipher?: string; + /** First v1.1 draft: v1 token of `{ env, headers }`. */ + secretCipher?: string; + /** v1: plaintext. */ + env?: Record; + headers?: Record; +}; + +export interface OpenedMcpServer { + cfg: McpServerConfig; + /** Fields with a stored cipher that cannot be opened (STC_SECRET changed, or a cipher moved from elsewhere). */ + unreadable: SecretField[]; +} export const MASK_PREFIX = "••••"; @@ -23,35 +44,81 @@ export class MaskedSecretError extends Error { } } -const nonEmpty = (r: Record | undefined) => (r && Object.keys(r).length ? r : undefined); +const nonEmpty = (r: Record | undefined) => + (r && typeof r === "object" && !Array.isArray(r) && Object.keys(r).length ? r : undefined); -/** Strips `env`/`headers` from a config and seals them (if any) into `secretCipher`. */ -export async function sealMcpServer(box: SecretBox, cfg: McpServerConfig | StoredMcpServer): Promise { - const { env, headers, secretCipher: _old, ...rest } = cfg as StoredMcpServer; - const secrets: Secrets = {}; - if (nonEmpty(env)) secrets.env = env; - if (nonEmpty(headers)) secrets.headers = headers; - return secrets.env || secrets.headers ? { ...rest, secretCipher: await box.encrypt(JSON.stringify(secrets)) } : rest; +const isRecordOfStrings = (v: unknown): v is Record => + !!v && typeof v === "object" && !Array.isArray(v) && Object.values(v).every((x) => typeof x === "string"); + +/** The non-secret part of a server config (also drops `clearSecrets`, which is a save instruction only). */ +function baseOf(s: McpServerConfig | StoredMcpServer): Omit { + const { env: _e, headers: _h, envCipher: _ec, headersCipher: _hc, secretCipher: _sc, clearSecrets: _cs, ...rest } = s as StoredMcpServer & McpServerConfig; + return rest; +} + +/** Opens a stored server for server code. Unreadable ciphers are reported, never thrown and never discarded. */ +export async function openMcpServer(box: SecretBox, s: StoredMcpServer): Promise { + const cfg: McpServerConfig = { ...baseOf(s) }; + const unreadable: SecretField[] = []; + for (const f of SECRET_FIELDS) { + const token = s[CIPHER[f]]; + if (token) { + try { + const v = JSON.parse(await box.open(token, "mcp", mcpAad(s.id, f))); + if (!isRecordOfStrings(v)) throw new Error("not a record"); + if (nonEmpty(v)) cfg[f] = v; + } catch { unreadable.push(f); } + } else if (nonEmpty(s[f])) cfg[f] = s[f]; // v1 plaintext not yet upgraded + } + // A draft-format cipher the upgrade could not open: report the field this transport uses. + if (s.secretCipher) { + const f: SecretField = s.transport === "stdio" ? "env" : "headers"; + if (!s[CIPHER[f]] && !unreadable.includes(f)) unreadable.push(f); + } + return { cfg, unreadable }; } /** - * The full config for server code. A value that cannot be opened (STC_SECRET changed) is dropped with the server - * still listed, so a broken secret never crashes the assistant. Plaintext left over from v1 is passed through. + * Seals a posted (already unmasked) config. For each field: `clearSecrets` drops it; posted values replace it; with + * nothing posted, a stored cipher that could not be opened is kept as it is, so a temporary STC_SECRET mismatch + * never turns into data loss on the next save. */ -export async function openMcpServer(box: SecretBox, s: StoredMcpServer): Promise { - const { secretCipher, env, headers, ...rest } = s; - let secrets: Secrets = {}; - if (secretCipher) { - try { secrets = JSON.parse(await box.decrypt(secretCipher)) as Secrets; } catch { secrets = {}; } +export async function sealMcpServer(box: SecretBox, cfg: McpServerConfig, previous?: { stored: StoredMcpServer; unreadable: SecretField[] }): Promise { + const out: StoredMcpServer = { ...baseOf(cfg) }; + for (const f of SECRET_FIELDS) { + if (cfg.clearSecrets?.includes(f)) continue; + const values = nonEmpty(cfg[f]); + if (values) out[CIPHER[f]] = await box.seal(JSON.stringify(values), "mcp", mcpAad(cfg.id, f)); + else if (previous?.unreadable.includes(f)) { + if (previous.stored[CIPHER[f]]) out[CIPHER[f]] = previous.stored[CIPHER[f]]; + else if (previous.stored.secretCipher) out.secretCipher = previous.stored.secretCipher; + } } - const out: McpServerConfig = { ...rest }; - const e = nonEmpty(secrets.env) ?? nonEmpty(env); - const h = nonEmpty(secrets.headers) ?? nonEmpty(headers); - if (e) out.env = e; - if (h) out.headers = h; return out; } +/** Re-seals v1 plaintext and a readable draft `secretCipher` into per-field v2 ciphers. `pending`: something legacy stays. */ +export async function upgradeStoredMcpServer(box: SecretBox, s: StoredMcpServer): Promise<{ server: StoredMcpServer; changed: boolean; pending: boolean }> { + let legacy: { env?: Record; headers?: Record } = {}; + let pending = false; + let changed = false; + if (s.secretCipher) { + try { + const v = JSON.parse(await box.openLegacy(s.secretCipher)); + legacy = v && typeof v === "object" ? v : {}; + } catch { pending = true; } + } + const out: StoredMcpServer = { ...baseOf(s) }; + for (const f of SECRET_FIELDS) { + if (s[CIPHER[f]]) { out[CIPHER[f]] = s[CIPHER[f]]; continue; } + const values = nonEmpty(isRecordOfStrings(s[f]) ? s[f] : undefined) ?? nonEmpty(isRecordOfStrings(legacy[f]) ? legacy[f] : undefined); + if (values) out[CIPHER[f]] = await box.seal(JSON.stringify(values), "mcp", mcpAad(s.id, f)); + } + if (pending) out.secretCipher = s.secretCipher; + if (s.env !== undefined || s.headers !== undefined || (s.secretCipher && !pending)) changed = true; + return { server: out, changed, pending }; +} + const maskRecord = (r: Record | undefined) => (r ? Object.fromEntries(Object.entries(r).map(([k, v]) => [k, maskSecret(v)])) : undefined); /** What the admin API returns: keys visible, values masked. */ @@ -68,7 +135,7 @@ export function maskMcpServer(s: McpServerConfig): McpServerConfig { */ export function unmaskMcpServer(posted: McpServerConfig, previous: McpServerConfig[]): McpServerConfig { const prev = previous.find((p) => p.id === posted.id); - const fill = (field: "env" | "headers") => { + const fill = (field: SecretField) => { const r = posted[field]; if (!r) return undefined; return Object.fromEntries(Object.entries(r).map(([k, v]) => { diff --git a/packages/feature-ai/src/settings.ts b/packages/feature-ai/src/settings.ts index c571df4..1e0438a 100644 --- a/packages/feature-ai/src/settings.ts +++ b/packages/feature-ai/src/settings.ts @@ -1,6 +1,6 @@ import type { Db } from "@stc/db"; -import type { SecretBox } from "./crypto"; -import { maskMcpServer, openMcpServer, sealMcpServer, unmaskMcpServer, type StoredMcpServer } from "./mcp/secrets"; +import { byokAad, SERVER_KEY_AAD, type SecretBox } from "./crypto"; +import { maskMcpServer, openMcpServer, sealMcpServer, unmaskMcpServer, type OpenedMcpServer, type SecretField, type StoredMcpServer } from "./mcp/secrets"; import { upgradeAiData } from "./upgrade"; export { maskSecret, MaskedSecretError } from "./mcp/secrets"; @@ -21,20 +21,28 @@ export interface McpServerConfig { headers?: Record; /** Courses this server is offered in. Absent or empty: every course (global). */ courseIds?: string[]; + /** Save only: drop these stored fields, even if they cannot be decrypted. Never stored or returned. */ + clearSecrets?: SecretField[]; } +/** A stored secret the current STC_SECRET cannot open. It is kept until the admin replaces or clears it. */ +export type UnreadableSecret = + | { kind: "api-key" } + | { kind: "mcp"; serverId: string; name: string; field: SecretField }; + /** * `allowedBaseUrls`: origins learners' own keys (BYOK) may be sent to, besides the official endpoints. * `mcpServers` here carry masked `env`/`headers` values (see mcp/secrets.ts); full values: mcpServersWithSecrets(). */ -export interface ServerSettings { provider: PublicProviderConfig | null; mcpServers: McpServerConfig[]; allowedBaseUrls: string[] } +export interface ServerSettings { provider: PublicProviderConfig | null; mcpServers: McpServerConfig[]; allowedBaseUrls: string[]; unreadableSecrets: UnreadableSecret[] } export interface ServerSettingsInput { provider: (ProviderConfig & { apiKey?: string; clearApiKey?: boolean }) | null; mcpServers: McpServerConfig[]; /** Omitted: keep the saved list. */ allowedBaseUrls?: string[]; } -export interface UserSettings { byok: PublicProviderConfig | null; mentorOff: Record } +/** `keyUnreadable`: the saved key cannot be decrypted with the current STC_SECRET; the user must re-enter it. */ +export interface UserSettings { byok: (PublicProviderConfig & { keyUnreadable?: true }) | null; mentorOff: Record } type ServerRow = { provider: ProviderConfig | null; apiKeyCipher: string | null; mcpServers: StoredMcpServer[]; allowedBaseUrls?: string[] }; type UserRow = { byok: ProviderConfig | null }; @@ -84,7 +92,13 @@ export function aiSettingsRepo(db: Db, box: SecretBox) { const { mentorOff: _legacy, ...row } = parse(await read("user", userId), { byok: null } as UserRow & { mentorOff?: unknown }); return row; }; - const openServers = async (r: ServerRow) => Promise.all((r.mcpServers ?? []).map((m) => openMcpServer(box, m))); + const storedServers = (r: ServerRow): StoredMcpServer[] => + (Array.isArray(r.mcpServers) ? r.mcpServers : []).filter((m) => m && typeof m === "object" && typeof m.id === "string"); + const openServers = async (r: ServerRow): Promise => Promise.all(storedServers(r).map((m) => openMcpServer(box, m))); + const openServerKey = async (r: ServerRow): Promise => + r.apiKeyCipher ? box.open(r.apiKeyCipher, "server-api-key", SERVER_KEY_AAD).catch(() => null) : null; + const openUserKey = async (userId: string, k: { provider: string; ciphertext: string }): Promise => + box.open(k.ciphertext, "byok", byokAad(userId, k.provider)).catch(() => null); const keyRow = async (userId: string) => (await db.sql`SELECT provider, ciphertext FROM user_api_keys WHERE user_id = ${userId}`)[0] as { provider: string; ciphertext: string } | undefined; @@ -94,30 +108,60 @@ export function aiSettingsRepo(db: Db, box: SecretBox) { async getServer(): Promise { await ready(); const r = await serverRow(); - const mcpServers = (await openServers(r)).map(maskMcpServer); - return { provider: r.provider ? { ...r.provider, hasApiKey: !!r.apiKeyCipher } : null, mcpServers, allowedBaseUrls: r.allowedBaseUrls ?? [] }; + const opened = await openServers(r); + const unreadableSecrets: UnreadableSecret[] = []; + if (r.provider && r.apiKeyCipher && (await openServerKey(r)) === null) unreadableSecrets.push({ kind: "api-key" }); + for (const o of opened) for (const field of o.unreadable) unreadableSecrets.push({ kind: "mcp", serverId: o.cfg.id, name: o.cfg.name, field }); + return { + provider: r.provider ? { ...r.provider, hasApiKey: !!r.apiKeyCipher } : null, + mcpServers: opened.map((o) => maskMcpServer(o.cfg)), + allowedBaseUrls: r.allowedBaseUrls ?? [], + unreadableSecrets, + }; }, - /** Server code only: MCP servers with their real env and headers. */ + /** The admin provider without opening any secret (for resolving which provider a request uses). */ + async serverProvider(): Promise { + await ready(); + const r = await serverRow(); + return r.provider ? { ...r.provider, hasApiKey: !!r.apiKeyCipher } : null; + }, + /** The admin's BYOK allowlist, without opening any secret. */ + async allowedBaseUrls(): Promise { + await ready(); + return (await serverRow()).allowedBaseUrls ?? []; + }, + /** Server code only: MCP servers with their real env and headers (unreadable fields left out). */ async mcpServersWithSecrets(): Promise { await ready(); - return openServers(await serverRow()); + return (await openServers(await serverRow())).map((o) => o.cfg); }, /** A posted (masked) MCP config with stored values filled in; throws MaskedSecretError for a stray mask. */ async resolveMcpMasks(cfg: McpServerConfig): Promise { await ready(); - return unmaskMcpServer(cfg, await openServers(await serverRow())); + const { clearSecrets: _c, ...rest } = unmaskMcpServer(cfg, (await openServers(await serverRow())).map((o) => o.cfg)); + return rest; }, + /** + * Saves the admin settings. Secrets that cannot be decrypted are never discarded implicitly: the API key stays + * unless replaced or cleared (clearApiKey), and an MCP field stays unless new values are posted or the field is + * listed in that server's clearSecrets. + */ async saveServer(input: ServerSettingsInput): Promise { await ready(); const prev = await serverRow(); let apiKeyCipher: string | null = null; if (input.provider) { - if (input.provider.apiKey) apiKeyCipher = await box.encrypt(input.provider.apiKey); + if (input.provider.apiKey) apiKeyCipher = await box.seal(input.provider.apiKey, "server-api-key", SERVER_KEY_AAD); else if (!input.provider.clearApiKey) apiKeyCipher = prev.apiKeyCipher; } - const previous = await openServers(prev); - const unmasked = (input.mcpServers ?? []).map((m) => unmaskMcpServer(m, previous)); - const mcpServers = await Promise.all(unmasked.map((m) => sealMcpServer(box, m))); + const stored = storedServers(prev); + const opened = await openServers(prev); + const previous = opened.map((o) => o.cfg); + const mcpServers: StoredMcpServer[] = []; + for (const m of input.mcpServers ?? []) { + const i = stored.findIndex((x) => x.id === m.id); + mcpServers.push(await sealMcpServer(box, unmaskMcpServer(m, previous), i >= 0 ? { stored: stored[i]!, unreadable: opened[i]!.unreadable } : undefined)); + } await write("server", "", { provider: input.provider ? pickProvider(input.provider) : null, apiKeyCipher, @@ -128,18 +172,23 @@ export function aiSettingsRepo(db: Db, box: SecretBox) { async serverApiKey(): Promise { await ready(); const r = await serverRow(); - return r.provider && r.apiKeyCipher ? box.decrypt(r.apiKeyCipher) : null; + return r.provider ? openServerKey(r) : null; }, async getUser(userId: string): Promise { await ready(); const r = await userRow(userId); const k = await keyRow(userId); const prefs = (await db.sql`SELECT course_id FROM ai_mentor_prefs WHERE user_id = ${userId} AND off = 1`) as { course_id: string }[]; - return { byok: r.byok && k ? { ...r.byok, hasApiKey: true } : null, mentorOff: Object.fromEntries(prefs.map((p) => [p.course_id, true])) }; + let byok: UserSettings["byok"] = null; + if (r.byok && k) { + byok = { ...r.byok, hasApiKey: true }; + if ((await openUserKey(userId, k)) === null) byok.keyUnreadable = true; + } + return { byok, mentorOff: Object.fromEntries(prefs.map((p) => [p.course_id, true])) }; }, async saveUserKey(userId: string, cfg: ProviderConfig & { apiKey: string }): Promise { await ready(); - const ciphertext = await box.encrypt(cfg.apiKey); + const ciphertext = await box.seal(cfg.apiKey, "byok", byokAad(userId, cfg.kind)); const now = new Date().toISOString(); await db.sql.begin(async (tx) => { await tx`DELETE FROM user_api_keys WHERE user_id = ${userId}`; @@ -157,7 +206,7 @@ export function aiSettingsRepo(db: Db, box: SecretBox) { async userApiKey(userId: string): Promise { await ready(); const k = await keyRow(userId); - return k ? box.decrypt(k.ciphertext) : null; + return k ? openUserKey(userId, k) : null; }, async setMentorOff(userId: string, courseId: string, off: boolean): Promise { await ready(); diff --git a/packages/feature-ai/src/upgrade.ts b/packages/feature-ai/src/upgrade.ts index 1a33600..a5ad34e 100644 --- a/packages/feature-ai/src/upgrade.ts +++ b/packages/feature-ai/src/upgrade.ts @@ -1,9 +1,11 @@ import type { Db } from "@stc/db"; -import type { SecretBox } from "./crypto"; -import { sealMcpServer, type StoredMcpServer } from "./mcp/secrets"; +import { byokAad, isLegacyToken, SERVER_KEY_AAD, type SecretBox } from "./crypto"; +import { upgradeStoredMcpServer, type StoredMcpServer } from "./mcp/secrets"; /** Recorded in schema_migrations once the data upgrade has run (no `.sql` suffix, so no file can share it). */ export const AI_DATA_UPGRADE = "ai_data_v11"; +/** Recorded once no secret is left in a pre-v2 format. */ +export const AI_SECRETS_UPGRADE = "ai_secrets_v2"; type Tx = Bun.TransactionSQL; @@ -31,28 +33,79 @@ async function moveMentorToggles(tx: Tx, now: string) { } } -/** v1 stored MCP `env`/`headers` in plaintext in the server row; seal them like API keys. */ -async function sealMcpSecrets(tx: Tx, box: SecretBox, now: string) { +/** + * Re-seals every secret still in an older format as a v2 token (own HKDF key per purpose, AAD bound to the row): + * BYOK keys, the server API key, and MCP env/headers (v1 plaintext or the draft `secretCipher`). A v1 token that + * does not open (STC_SECRET changed) is left untouched and the step stays pending, so it is retried on every start + * and completes once the previous secret is restored. Returns { changed, pending }. + */ +async function resealSecrets(tx: Tx, box: SecretBox, now: string): Promise<{ changed: boolean; pending: boolean }> { + let changed = false; + let pending = false; + const keys = (await tx`SELECT user_id, provider, ciphertext FROM user_api_keys`) as { user_id: string; provider: string; ciphertext: string }[]; + for (const k of keys) { + if (!isLegacyToken(k.ciphertext)) continue; + let plain: string; + try { plain = await box.openLegacy(k.ciphertext); } catch { pending = true; continue; } + const sealed = await box.seal(plain, "byok", byokAad(k.user_id, k.provider)); + await tx`UPDATE user_api_keys SET ciphertext = ${sealed} WHERE user_id = ${k.user_id} AND provider = ${k.provider}`; + changed = true; + } + const rows = (await tx`SELECT config_json FROM ai_settings WHERE scope = 'server' AND owner_id = ''`) as { config_json: string }[]; const cfg = rows[0] ? parseObject(rows[0].config_json) : null; - if (!cfg || !Array.isArray(cfg.mcpServers)) return; - const servers = cfg.mcpServers as StoredMcpServer[]; - if (!servers.some((s) => s && (s.env || s.headers))) return; - const mcpServers = await Promise.all(servers.map((s) => (s && (s.env || s.headers) ? sealMcpServer(box, s) : s))); - await tx`UPDATE ai_settings SET config_json = ${JSON.stringify({ ...cfg, mcpServers })}, updated_at = ${now} WHERE scope = 'server' AND owner_id = ''`; + if (!cfg) return { changed, pending }; + let rowChanged = false; + if (typeof cfg.apiKeyCipher === "string" && isLegacyToken(cfg.apiKeyCipher)) { + try { + cfg.apiKeyCipher = await box.seal(await box.openLegacy(cfg.apiKeyCipher), "server-api-key", SERVER_KEY_AAD); + rowChanged = true; + } catch { pending = true; } + } + if (Array.isArray(cfg.mcpServers)) { + const servers: unknown[] = []; + for (const s of cfg.mcpServers) { + // Malformed entries are kept as they are (the settings code skips them); they must not stop the upgrade. + if (!s || typeof s !== "object" || typeof (s as StoredMcpServer).id !== "string") { servers.push(s); continue; } + const r = await upgradeStoredMcpServer(box, s as StoredMcpServer); + servers.push(r.server); + if (r.changed) rowChanged = true; + if (r.pending) pending = true; + } + cfg.mcpServers = servers; + } + if (rowChanged) { + await tx`UPDATE ai_settings SET config_json = ${JSON.stringify(cfg)}, updated_at = ${now} WHERE scope = 'server' AND owner_id = ''`; + changed = true; + } + return { changed, pending }; } /** - * Data the v1.1 SQL migrations cannot move portably. Runs after the package's migrations, in one transaction, - * once (marked in schema_migrations). Returns true if it ran now. The settings repo awaits it before first use. + * Data the v1.1 SQL migrations cannot move portably, in one transaction. Each step is recorded in schema_migrations + * when complete (names without `.sql`, so no file can share them): + * - `ai_data_v11`: per-course mentor toggles from config_json to ai_mentor_prefs; + * - `ai_secrets_v2`: every secret re-sealed in the v2 format (marked only when nothing legacy is left). + * Returns true if it changed anything now. The settings repo awaits it before first use. */ export async function upgradeAiData(db: Db, box: SecretBox): Promise { return db.sql.begin(async (tx) => { - if ((await tx`SELECT name FROM schema_migrations WHERE name = ${AI_DATA_UPGRADE}`).length) return false; + const done = new Set(((await tx`SELECT name FROM schema_migrations WHERE name IN (${AI_DATA_UPGRADE}, ${AI_SECRETS_UPGRADE})`) as { name: string }[]).map((r) => r.name)); const now = new Date().toISOString(); - await moveMentorToggles(tx, now); - await sealMcpSecrets(tx, box, now); - await tx`INSERT INTO schema_migrations (name, applied_at) VALUES (${AI_DATA_UPGRADE}, ${now})`; - return true; + let changed = false; + if (!done.has(AI_DATA_UPGRADE)) { + await moveMentorToggles(tx, now); + await tx`INSERT INTO schema_migrations (name, applied_at) VALUES (${AI_DATA_UPGRADE}, ${now})`; + changed = true; + } + if (!done.has(AI_SECRETS_UPGRADE)) { + const r = await resealSecrets(tx, box, now); + if (r.changed) changed = true; + if (!r.pending) { + await tx`INSERT INTO schema_migrations (name, applied_at) VALUES (${AI_SECRETS_UPGRADE}, ${now})`; + changed = true; + } + } + return changed; }); } diff --git a/packages/feature-ai/src/web/AccountAiPage.tsx b/packages/feature-ai/src/web/AccountAiPage.tsx index 137cfe8..79fd713 100644 --- a/packages/feature-ai/src/web/AccountAiPage.tsx +++ b/packages/feature-ai/src/web/AccountAiPage.tsx @@ -1,10 +1,10 @@ import { useEffect, useState } from "react"; import { Pill } from "@stc/ui"; import { api, ApiError } from "./api"; -import { Field, PageHead } from "./components"; +import { Field, PageHead, UNREADABLE_KEY_TEXT } from "./components"; interface Account { - byok: { kind: "anthropic" | "openai"; model?: string; baseUrl?: string; hasApiKey: boolean } | null; + byok: { kind: "anthropic" | "openai"; model?: string; baseUrl?: string; hasApiKey: boolean; keyUnreadable?: boolean } | null; serverProvider: { kind: string; model: string | null } | null; } @@ -51,7 +51,8 @@ export default function AccountAiPage() {

- {acct.byok ?

A {acct.byok.kind === "anthropic" ? "Anthropic" : "OpenAI-compatible"} key is saved{acct.byok.model ? ` (model ${acct.byok.model})` : ""}.

: null} + {acct.byok?.keyUnreadable ?

{UNREADABLE_KEY_TEXT}

: null} + {acct.byok && !acct.byok.keyUnreadable ?

A {acct.byok.kind === "anthropic" ? "Anthropic" : "OpenAI-compatible"} key is saved{acct.byok.model ? ` (model ${acct.byok.model})` : ""}.

: null}
{ e.preventDefault(); save(); }}>
diff --git a/packages/feature-ai/src/web/AdminAiPage.tsx b/packages/feature-ai/src/web/AdminAiPage.tsx index 22b60a4..220527d 100644 --- a/packages/feature-ai/src/web/AdminAiPage.tsx +++ b/packages/feature-ai/src/web/AdminAiPage.tsx @@ -1,14 +1,14 @@ import { useEffect, useState } from "react"; import { Pill } from "@stc/ui"; import { api, ApiError } from "./api"; -import { Field, PageHead } from "./components"; +import { Field, PageHead, UnreadableSecretsWarning, type UnreadableSecretItem } from "./components"; type Kind = "" | "anthropic" | "openai"; interface ProviderOut { kind: "anthropic" | "openai"; model?: string; baseUrl?: string; effort?: string; hasApiKey: boolean } -interface McpServer { id: string; name: string; transport: "stdio" | "http"; command?: string; args?: string[]; env?: Record; url?: string; headers?: Record; courseIds?: string[] } -interface Config { provider: ProviderOut | null; mcpServers: McpServer[]; allowedBaseUrls: string[] } +interface McpServer { id: string; name: string; transport: "stdio" | "http"; command?: string; args?: string[]; env?: Record; url?: string; headers?: Record; courseIds?: string[]; clearSecrets?: ("env" | "headers")[] } +interface Config { provider: ProviderOut | null; mcpServers: McpServer[]; allowedBaseUrls: string[]; unreadableSecrets?: UnreadableSecretItem[] } -type ServerDraft = { id: string; name: string; transport: "stdio" | "http"; command: string; args: string; env: string; url: string; headers?: Record; courses: string; test?: string }; +type ServerDraft = { id: string; name: string; transport: "stdio" | "http"; command: string; args: string; env: string; url: string; headers?: Record; courses: string; test?: string; clearSecrets?: ("env" | "headers")[] }; const toDraft = (s: McpServer): ServerDraft => ({ id: s.id, name: s.name, transport: s.transport, command: s.command ?? "", args: (s.args ?? []).join(" "), env: Object.entries(s.env ?? {}).map(([k, v]) => `${k}=${v}`).join("\n"), url: s.url ?? "", headers: s.headers, courses: (s.courseIds ?? []).join(", "), @@ -19,6 +19,7 @@ const fromDraft = (d: ServerDraft): McpServer => ({ id: d.id, name: d.name || d.id, transport: d.transport, ...(d.transport === "stdio" ? { command: d.command, args: d.args.trim() ? d.args.trim().split(/\s+/) : [], env: parseEnv(d.env) } : { url: d.url, ...(d.headers ? { headers: d.headers } : {}) }), courseIds: d.courses.split(",").map((s) => s.trim()).filter(Boolean), + ...(d.clearSecrets?.length ? { clearSecrets: d.clearSecrets } : {}), }); export default function AdminAiPage() { @@ -80,6 +81,19 @@ export default function AdminAiPage() {

AI is off until you choose a provider here. Learners and authors can also add their own key, which is used only for their own requests. Keys are encrypted on the server and never shown again.

+ { + if (item.kind !== "mcp") return Paste the key again below, or tick "Remove the saved key".; + const i = servers.findIndex((x) => x.id === item.serverId); + const marked = i >= 0 && !!servers[i]!.clearSecrets?.includes(item.field); + if (i < 0) return null; + return marked ? Will be discarded when you save. : ( + setServer(i, { clearSecrets: [...(servers[i]!.clearSecrets ?? []), item.field] })}>Discard stored value + ); + }} + /> + { e.preventDefault(); save(); }}>

Server provider

diff --git a/packages/feature-ai/src/web/components.tsx b/packages/feature-ai/src/web/components.tsx index 3120645..c9c7d47 100644 --- a/packages/feature-ai/src/web/components.tsx +++ b/packages/feature-ai/src/web/components.tsx @@ -75,3 +75,25 @@ export function Markdown({ text }: { text: string }) { } return
{blocks}
; } + +/** A stored secret the server cannot decrypt (mirrors UnreadableSecret in settings.ts). */ +export type UnreadableSecretItem = { kind: "api-key" } | { kind: "mcp"; serverId: string; name: string; field: "env" | "headers" }; + +export const UNREADABLE_SECRETS_TEXT = "Some stored secrets can't be decrypted with the current STC_SECRET. Re-enter them or restore the previous secret."; +export const UNREADABLE_KEY_TEXT = "Your saved key can't be used; please re-enter it."; + +const describeSecret = (s: UnreadableSecretItem) => + s.kind === "api-key" ? "Server provider: API key" : `MCP server "${s.name}" (${s.serverId}): ${s.field === "env" ? "environment" : "headers"}`; + +/** Admin warning listing each affected server and field. Nothing is discarded until the admin replaces or clears it. */ +export function UnreadableSecretsWarning({ items, action }: { items: UnreadableSecretItem[]; action?: (item: UnreadableSecretItem) => ReactNode }) { + if (!items.length) return null; + return ( +
+

{UNREADABLE_SECRETS_TEXT}

+
    + {items.map((s, i) =>
  • {describeSecret(s)}{action ? <> {action(s)} : null}
    • )} +
+
+ ); +} diff --git a/packages/feature-ai/test/crypto.test.ts b/packages/feature-ai/test/crypto.test.ts index 7bea5b8..9dcd487 100644 --- a/packages/feature-ai/test/crypto.test.ts +++ b/packages/feature-ai/test/crypto.test.ts @@ -1,27 +1,51 @@ import { expect, test } from "bun:test"; -import { createSecretBox } from "../src/crypto"; +import { byokAad, createSecretBox, legacyV1Encrypt, mcpAad, SERVER_KEY_AAD } from "../src/crypto"; test("round trips and uses a fresh IV each time", async () => { const box = createSecretBox("s3cret"); - const a = await box.encrypt("sk-ant-123"); - const b = await box.encrypt("sk-ant-123"); + const a = await box.seal("sk-ant-123", "byok", byokAad("u1", "anthropic")); + const b = await box.seal("sk-ant-123", "byok", byokAad("u1", "anthropic")); expect(a).not.toBe(b); expect(a).not.toContain("sk-ant-123"); - expect(a.startsWith("v1.")).toBe(true); - expect(await box.decrypt(a)).toBe("sk-ant-123"); - expect(await box.decrypt(b)).toBe("sk-ant-123"); + expect(a.startsWith("v2.")).toBe(true); + expect(await box.open(a, "byok", byokAad("u1", "anthropic"))).toBe("sk-ant-123"); + expect(await box.open(b, "byok", byokAad("u1", "anthropic"))).toBe("sk-ant-123"); }); test("another secret cannot decrypt", async () => { - const token = await createSecretBox("one").encrypt("key"); - await expect(createSecretBox("two").decrypt(token)).rejects.toThrow(); + const token = await createSecretBox("one").seal("key", "byok", "a"); + await expect(createSecretBox("two").open(token, "byok", "a")).rejects.toThrow(); }); test("a tampered token is rejected", async () => { - const token = await createSecretBox("one").encrypt("key"); + const box = createSecretBox("one"); + const token = await box.seal("key", "byok", "a"); const parts = token.split("."); const ct = Buffer.from(parts[2]!, "base64"); ct[0] = ct[0]! ^ 1; - await expect(createSecretBox("one").decrypt(`${parts[0]}.${parts[1]}.${ct.toString("base64")}`)).rejects.toThrow(); - await expect(createSecretBox("one").decrypt("garbage")).rejects.toThrow(); + await expect(box.open(`${parts[0]}.${parts[1]}.${ct.toString("base64")}`, "byok", "a")).rejects.toThrow(); + await expect(box.open("garbage", "byok", "a")).rejects.toThrow(); +}); + +test("a token opens only for its own purpose and row (domain separation and AAD)", async () => { + const box = createSecretBox("one"); + const byok = await box.seal("sk-user", "byok", byokAad("u1", "anthropic")); + await expect(box.open(byok, "byok", byokAad("u2", "anthropic"))).rejects.toThrow(); // another user + await expect(box.open(byok, "byok", byokAad("u1", "openai"))).rejects.toThrow(); // another provider + await expect(box.open(byok, "server-api-key", SERVER_KEY_AAD)).rejects.toThrow(); // another purpose + await expect(box.open(byok, "mcp", byokAad("u1", "anthropic"))).rejects.toThrow(); // same AAD, other HKDF key + const env = await box.seal("{}", "mcp", mcpAad("fs", "env")); + await expect(box.open(env, "mcp", mcpAad("fs", "headers"))).rejects.toThrow(); + await expect(box.open(env, "mcp", mcpAad("web", "env"))).rejects.toThrow(); + await expect(box.open(env, "byok", mcpAad("fs", "env"))).rejects.toThrow(); +}); + +test("v1 tokens open only through openLegacy, and v2 tokens never do", async () => { + const box = createSecretBox("one"); + const v1 = await legacyV1Encrypt("one", "old-key"); + expect(v1.startsWith("v1.")).toBe(true); + expect(await box.openLegacy(v1)).toBe("old-key"); + await expect(box.open(v1, "byok", byokAad("u1", "anthropic"))).rejects.toThrow(/format/); + await expect(box.openLegacy(await box.seal("x", "byok", "a"))).rejects.toThrow(/format/); + await expect(createSecretBox("two").openLegacy(v1)).rejects.toThrow(); }); diff --git a/packages/feature-ai/test/feature.test.ts b/packages/feature-ai/test/feature.test.ts index 28fbaa9..175f0bd 100644 --- a/packages/feature-ai/test/feature.test.ts +++ b/packages/feature-ai/test/feature.test.ts @@ -283,3 +283,33 @@ forEachDb("the admin API masks MCP secrets, and the test button and chat use the expect(seen.length).toBeGreaterThan(0); expect(seen.filter(Boolean).every((h) => h === "Bearer hdr-SECRET-abcdef")).toBe(true); }); + +forEachDb("after an STC_SECRET change the admin and the user are told, and an unchanged save keeps every secret", async (db) => { + const before = await setup(db, [() => ({ text: "ok" })], {}, { secret: "old-secret" }); + const web = { id: "web", name: "Web", transport: "http", url: "https://mcp.example.com/mcp", headers: { authorization: "Bearer hdr-SECRET-abcdef" } }; + await before.call("PUT", "/api/ai/admin/config", "owner", { provider: { kind: "anthropic", apiKey: "sk-ant-OLD" }, mcpServers: [web] }); + await before.call("PUT", "/api/ai/account/key", "ann", { kind: "anthropic", apiKey: "sk-ann-OLD" }); + const logs: string[] = []; + const { call } = await setup(db, [() => ({ text: "ok" })], { log: (m) => logs.push(m) }, { secret: "new-secret" }); + + const cfg = await (await call("GET", "/api/ai/admin/config", "owner")).json(); + expect(cfg.unreadableSecrets).toEqual([{ kind: "api-key" }, { kind: "mcp", serverId: "web", name: "Web", field: "headers" }]); + expect(cfg.mcpServers[0].headers).toBeUndefined(); + for (let i = 0; i < 100 && !logs.length; i++) await Bun.sleep(5); // the startup check runs in the background + expect(logs.some((l) => l.includes("cannot be decrypted"))).toBe(true); + const acct = await (await call("GET", "/api/ai/account", "ann")).json(); + expect(acct.byok).toEqual({ kind: "anthropic", hasApiKey: true, keyUnreadable: true }); + + // Saving the page as shown (the admin only changes the model) keeps the API key and the MCP header. + expect((await call("PUT", "/api/ai/admin/config", "owner", { provider: { kind: "anthropic", model: "claude-sonnet-5-5" }, mcpServers: cfg.mcpServers })).status).toBe(200); + const back = await setup(db, [() => ({ text: "ok" })], {}, { secret: "old-secret" }); + const restored = await (await back.call("GET", "/api/ai/admin/config", "owner")).json(); + expect(restored.unreadableSecrets).toEqual([]); + expect(restored.mcpServers[0].headers).toEqual({ authorization: "••••cdef" }); + expect((await (await back.call("GET", "/api/ai/account", "ann")).json()).byok.keyUnreadable).toBeUndefined(); + + // An explicit clear is accepted; a malformed one is refused. + expect((await call("PUT", "/api/ai/admin/config", "owner", { provider: null, mcpServers: [{ ...cfg.mcpServers[0], clearSecrets: ["cookies"] }] })).status).toBe(400); + expect((await call("PUT", "/api/ai/admin/config", "owner", { provider: null, mcpServers: [{ ...cfg.mcpServers[0], clearSecrets: ["headers"] }] })).status).toBe(200); + expect((await (await call("GET", "/api/ai/admin/config", "owner")).json()).unreadableSecrets).toEqual([]); +}); diff --git a/packages/feature-ai/test/settings.test.ts b/packages/feature-ai/test/settings.test.ts index 4c8190a..8c588d5 100644 --- a/packages/feature-ai/test/settings.test.ts +++ b/packages/feature-ai/test/settings.test.ts @@ -7,7 +7,7 @@ const box = createSecretBox("test-secret"); forEachDb("server config is empty by default and hides the key", async (db) => { const r = aiSettingsRepo(db, box); - expect(await r.getServer()).toEqual({ provider: null, mcpServers: [], allowedBaseUrls: [] }); + expect(await r.getServer()).toEqual({ provider: null, mcpServers: [], allowedBaseUrls: [], unreadableSecrets: [] }); expect(await r.serverApiKey()).toBeNull(); await r.saveServer({ provider: { kind: "anthropic", model: "claude-opus-5-5", apiKey: "sk-ant-SECRET" }, mcpServers: [] }); @@ -115,9 +115,94 @@ forEachDb("MCP env and headers are encrypted, masked when read and kept when the .toEqual({ id: "fs", name: "Files", transport: "stdio", command: "mcp-fs", env: { FS_TOKEN: "tok-ROTATED-999999" } }); }); -forEachDb("MCP secrets sealed with another STC_SECRET are dropped, not crashed on", async (db) => { - await aiSettingsRepo(db, createSecretBox("old-secret")).saveServer({ provider: null, mcpServers: [{ id: "fs", name: "Files", transport: "stdio", command: "x", env: { T: "tok-SECRET-123456" } }] }); +forEachDb("unreadable MCP secrets are reported, kept on save, and recover when the old STC_SECRET returns", async (db) => { + const old = aiSettingsRepo(db, createSecretBox("old-secret")); + await old.saveServer({ provider: null, mcpServers: [ + { id: "fs", name: "Files", transport: "stdio", command: "x", env: { T: "tok-SECRET-123456" } }, + { id: "web", name: "Web", transport: "http", url: "https://mcp.example.com/mcp", headers: { authorization: "Bearer hdr-SECRET-abcdef" } }, + ] }); + const r = aiSettingsRepo(db, box); // STC_SECRET changed + const shown = await r.getServer(); + expect(shown.mcpServers).toEqual([ + { id: "fs", name: "Files", transport: "stdio", command: "x" }, + { id: "web", name: "Web", transport: "http", url: "https://mcp.example.com/mcp" }, + ]); + expect(shown.unreadableSecrets).toEqual([ + { kind: "mcp", serverId: "fs", name: "Files", field: "env" }, + { kind: "mcp", serverId: "web", name: "Web", field: "headers" }, + ]); + expect(await r.mcpServersWithSecrets()).toEqual(shown.mcpServers); + + // The admin saves the page for another reason: the unreadable ciphers survive. + const before = JSON.parse((await db.sql`SELECT config_json FROM ai_settings WHERE scope = 'server'`)[0].config_json); + await r.saveServer({ provider: { kind: "openai", baseUrl: "http://ollama:11434/v1", model: "llama3" }, mcpServers: shown.mcpServers }); + const after = JSON.parse((await db.sql`SELECT config_json FROM ai_settings WHERE scope = 'server'`)[0].config_json); + expect(after.mcpServers[0].envCipher).toBe(before.mcpServers[0].envCipher); + expect(after.mcpServers[1].headersCipher).toBe(before.mcpServers[1].headersCipher); + expect((await r.getServer()).unreadableSecrets).toHaveLength(2); + + // Restoring the previous secret brings them back. + expect((await aiSettingsRepo(db, createSecretBox("old-secret")).mcpServersWithSecrets()).map((m) => m.env ?? m.headers)).toEqual([ + { T: "tok-SECRET-123456" }, { authorization: "Bearer hdr-SECRET-abcdef" }, + ]); + + // Explicit replace (new values) and explicit clear (clearSecrets) are the only ways to drop them. + await r.saveServer({ provider: null, mcpServers: [ + { id: "fs", name: "Files", transport: "stdio", command: "x", env: { T: "tok-NEW-0000000000" } }, + { id: "web", name: "Web", transport: "http", url: "https://mcp.example.com/mcp", clearSecrets: ["headers"] }, + ] }); + expect(await r.mcpServersWithSecrets()).toEqual([ + { id: "fs", name: "Files", transport: "stdio", command: "x", env: { T: "tok-NEW-0000000000" } }, + { id: "web", name: "Web", transport: "http", url: "https://mcp.example.com/mcp" }, + ]); + expect((await r.getServer()).unreadableSecrets).toEqual([]); + expect((await db.sql`SELECT config_json FROM ai_settings WHERE scope = 'server'`)[0].config_json).not.toContain("clearSecrets"); +}); + +forEachDb("an MCP cipher swapped onto another server or field does not open", async (db) => { const r = aiSettingsRepo(db, box); - expect((await r.getServer()).mcpServers).toEqual([{ id: "fs", name: "Files", transport: "stdio", command: "x" }]); - expect(await r.mcpServersWithSecrets()).toEqual([{ id: "fs", name: "Files", transport: "stdio", command: "x" }]); + await r.saveServer({ provider: null, mcpServers: [ + { id: "a", name: "A", transport: "http", url: "https://a.example.com/mcp", headers: { authorization: "Bearer A-SECRET-111111" } }, + { id: "b", name: "B", transport: "http", url: "https://b.example.com/mcp", headers: { authorization: "Bearer B-SECRET-222222" } }, + { id: "c", name: "C", transport: "stdio", command: "x", env: { T: "C-SECRET-33333333" } }, + ] }); + const row = JSON.parse((await db.sql`SELECT config_json FROM ai_settings WHERE scope = 'server'`)[0].config_json); + // swap a <-> b, and move c's env cipher into a headers slot of c + [row.mcpServers[0].headersCipher, row.mcpServers[1].headersCipher] = [row.mcpServers[1].headersCipher, row.mcpServers[0].headersCipher]; + row.mcpServers[2].headersCipher = row.mcpServers[2].envCipher; + delete row.mcpServers[2].envCipher; + await db.sql`UPDATE ai_settings SET config_json = ${JSON.stringify(row)} WHERE scope = 'server'`; + const full = await r.mcpServersWithSecrets(); + expect(full.every((m) => !m.headers && !m.env)).toBe(true); + expect((await r.getServer()).unreadableSecrets.map((u) => u.kind === "mcp" && `${u.serverId}.${u.field}`)).toEqual(["a.headers", "b.headers", "c.headers"]); +}); + +forEachDb("a BYOK ciphertext copied to another user, or into an MCP slot, does not open", async (db) => { + const r = aiSettingsRepo(db, box); + await r.saveUserKey("u1", { kind: "anthropic", apiKey: "sk-u1-SECRET" }); + await r.saveUserKey("u2", { kind: "anthropic", apiKey: "sk-u2-other" }); + const u1 = (await db.sql`SELECT ciphertext FROM user_api_keys WHERE user_id = 'u1'`)[0].ciphertext as string; + await db.sql`UPDATE user_api_keys SET ciphertext = ${u1} WHERE user_id = 'u2'`; + expect(await r.userApiKey("u2")).toBeNull(); + expect((await r.getUser("u2")).byok).toEqual({ kind: "anthropic", hasApiKey: true, keyUnreadable: true }); + expect(await r.userApiKey("u1")).toBe("sk-u1-SECRET"); + expect((await r.getUser("u1")).byok).toEqual({ kind: "anthropic", hasApiKey: true }); + // into the server API key and an MCP field: no oracle + await r.saveServer({ provider: { kind: "anthropic", apiKey: "sk-server" }, mcpServers: [{ id: "m", name: "M", transport: "http", url: "https://m.example.com/mcp", headers: { x: "y-123456789012" } }] }); + const row = JSON.parse((await db.sql`SELECT config_json FROM ai_settings WHERE scope = 'server'`)[0].config_json); + row.apiKeyCipher = u1; + row.mcpServers[0].headersCipher = u1; + await db.sql`UPDATE ai_settings SET config_json = ${JSON.stringify(row)} WHERE scope = 'server'`; + expect(await r.serverApiKey()).toBeNull(); + expect((await r.mcpServersWithSecrets())[0]!.headers).toBeUndefined(); + expect((await r.getServer()).unreadableSecrets).toEqual([{ kind: "api-key" }, { kind: "mcp", serverId: "m", name: "M", field: "headers" }]); +}); + +forEachDb("an unreadable server API key is reported and kept on save", async (db) => { + await aiSettingsRepo(db, createSecretBox("old-secret")).saveServer({ provider: { kind: "anthropic", apiKey: "sk-ant-OLD" }, mcpServers: [] }); + const r = aiSettingsRepo(db, box); + expect((await r.getServer()).unreadableSecrets).toEqual([{ kind: "api-key" }]); + expect(await r.serverApiKey()).toBeNull(); + await r.saveServer({ provider: { kind: "anthropic", model: "claude-opus-5-5" }, mcpServers: [] }); + expect(await aiSettingsRepo(db, createSecretBox("old-secret")).serverApiKey()).toBe("sk-ant-OLD"); }); diff --git a/packages/feature-ai/test/upgrade.test.ts b/packages/feature-ai/test/upgrade.test.ts index 8526445..89e0b5e 100644 --- a/packages/feature-ai/test/upgrade.test.ts +++ b/packages/feature-ai/test/upgrade.test.ts @@ -2,13 +2,16 @@ import { expect } from "bun:test"; import { migrate, type Db } from "@stc/db"; // Core migrations only: the ai_* tables exactly as v1 shipped them. import { forEachDb as forEachV1Db } from "../../db/test/helpers"; -import { createSecretBox } from "../src/crypto"; +import { byokAad, createSecretBox, legacyV1Encrypt, mcpAad, SERVER_KEY_AAD } from "../src/crypto"; import { migrationsDirs } from "../src/migrations"; import { aiSettingsRepo } from "../src/settings"; -import { upgradeAiData } from "../src/upgrade"; +import { AI_SECRETS_UPGRADE, upgradeAiData } from "../src/upgrade"; const T = "2026-10-01T00:00:00.000Z"; -const box = createSecretBox("test-secret"); +const SECRET = "test-secret"; +const box = createSecretBox(SECRET); +/** A secret as v1 stored it (one key, no additional data). */ +const v1Token = (plain: string, secret = SECRET) => legacyV1Encrypt(secret, plain); const aiMigrations = { "feature-ai": migrationsDirs }; async function seedMessage(db: Db, id: string, conv: string, role: string, content: string) { @@ -25,7 +28,7 @@ forEachV1Db("v1 upgrade: sources rows fold into citations_json on the assistant await seedMessage(db, "0001-c", "c1", "sources", cites); await seedMessage(db, "0001-d", "c1", "user", "And then?"); await seedMessage(db, "0001-e", "c1", "assistant", "Try it."); - // c2: an orphan sources row (no assistant before it) and an assistant whose next row belongs to c1's ids range + // c2: an orphan sources row (no assistant before it), then an assistant with no row after it await seedMessage(db, "0002-a", "c2", "sources", "[]"); await seedMessage(db, "0002-b", "c2", "assistant", "Alone."); @@ -53,7 +56,7 @@ forEachV1Db("v1 upgrade: per-course mentor toggles move from config_json to ai_m await seedSettings(db, "user", "u1", { byok: { kind: "anthropic" }, mentorOff: { sample: true, other: false } }); await seedSettings(db, "user", "u2", { byok: null, mentorOff: {} }); await seedSettings(db, "user", "u3", "{not json"); - await db.sql`INSERT INTO user_api_keys (user_id, provider, ciphertext, created_at) VALUES ('u1', 'anthropic', ${await box.encrypt("sk-u1")}, ${T})`; + await db.sql`INSERT INTO user_api_keys (user_id, provider, ciphertext, created_at) VALUES ('u1', 'anthropic', ${await v1Token("sk-u1")}, ${T})`; expect(await migrate(db, aiMigrations)).toContain("0002_ai_mentor_prefs.sql"); expect(await upgradeAiData(db, box)).toBe(true); @@ -84,7 +87,7 @@ forEachV1Db("v1 upgrade: the settings repo upgrades on first use", async (db) => }); forEachV1Db("v1 upgrade: plaintext MCP env and headers are encrypted at rest", async (db) => { - const apiKeyCipher = await box.encrypt("sk-ant-SERVER"); + const apiKeyCipher = await v1Token("sk-ant-SERVER"); await seedSettings(db, "server", "", { provider: { kind: "anthropic" }, apiKeyCipher, allowedBaseUrls: ["http://ollama:11434"], mcpServers: [ @@ -99,10 +102,14 @@ forEachV1Db("v1 upgrade: plaintext MCP env and headers are encrypted at rest", a const raw = (await db.sql`SELECT config_json FROM ai_settings WHERE scope = 'server'`)[0].config_json as string; expect(raw).not.toContain("PLAIN"); const stored = JSON.parse(raw); - expect(stored.apiKeyCipher).toBe(apiKeyCipher); + expect(stored.apiKeyCipher.startsWith("v2.")).toBe(true); // re-sealed under the v2 scheme + expect(await box.open(stored.apiKeyCipher, "server-api-key", SERVER_KEY_AAD)).toBe("sk-ant-SERVER"); expect(stored.allowedBaseUrls).toEqual(["http://ollama:11434"]); - expect(stored.mcpServers.map((s: { id: string; env?: unknown; headers?: unknown; secretCipher?: string }) => [s.id, !!s.env, !!s.headers, !!s.secretCipher])) - .toEqual([["fs", false, false, true], ["web", false, false, true], ["bare", false, false, false]]); + type S = { id: string; env?: unknown; headers?: unknown; envCipher?: string; headersCipher?: string; secretCipher?: string }; + expect(stored.mcpServers.map((s: S) => [s.id, !!s.env, !!s.headers, !!s.envCipher, !!s.headersCipher, !!s.secretCipher])) + .toEqual([["fs", false, false, true, false, false], ["web", false, false, false, true, false], ["bare", false, false, false, false, false]]); + expect(JSON.parse(await box.open(stored.mcpServers[0].envCipher, "mcp", mcpAad("fs", "env")))).toEqual({ FS_TOKEN: "tok-PLAIN-123456" }); + expect((await db.sql`SELECT name FROM schema_migrations WHERE name = ${AI_SECRETS_UPGRADE}`)).toHaveLength(1); const repo = aiSettingsRepo(db, box); expect(await repo.serverApiKey()).toBe("sk-ant-SERVER"); @@ -114,3 +121,68 @@ forEachV1Db("v1 upgrade: plaintext MCP env and headers are encrypted at rest", a const shown = JSON.stringify(await repo.getServer()); expect(shown).not.toContain("PLAIN"); }); + +forEachV1Db("v1.1 draft upgrade: secretCipher, v1 API keys and BYOK keys are re-sealed as v2, once", async (db) => { + // The state the first v1.1 draft left: mentor step done, MCP secrets in one v1-token secretCipher. + await migrate(db, aiMigrations); + await db.sql`INSERT INTO schema_migrations (name, applied_at) VALUES ('ai_data_v11', ${T})`; + await seedSettings(db, "server", "", { + provider: { kind: "anthropic" }, apiKeyCipher: await v1Token("sk-ant-SERVER"), + mcpServers: [ + { id: "fs", name: "Files", transport: "stdio", command: "x", secretCipher: await v1Token(JSON.stringify({ env: { T: "tok-DRAFT-123456" } })) }, + { id: "web", name: "Web", transport: "http", url: "https://w.example.com/mcp", secretCipher: await v1Token(JSON.stringify({ headers: { authorization: "Bearer DRAFT-abcdef" } })) }, + "not a server", + ], + }); + await db.sql`INSERT INTO user_api_keys (user_id, provider, ciphertext, created_at) VALUES ('u1', 'openai', ${await v1Token("sk-u1")}, ${T})`; + await seedSettings(db, "user", "u1", { byok: { kind: "openai", model: "m" } }); + + expect(await upgradeAiData(db, box)).toBe(true); + const raw = (await db.sql`SELECT config_json FROM ai_settings WHERE scope = 'server'`)[0].config_json as string; + expect(raw).not.toContain("secretCipher"); + expect(raw).not.toContain("\"v1."); + const ct = (await db.sql`SELECT ciphertext FROM user_api_keys WHERE user_id = 'u1'`)[0].ciphertext as string; + expect(ct.startsWith("v2.")).toBe(true); + expect(await box.open(ct, "byok", byokAad("u1", "openai"))).toBe("sk-u1"); + + const repo = aiSettingsRepo(db, box); + expect(await repo.serverApiKey()).toBe("sk-ant-SERVER"); + expect(await repo.userApiKey("u1")).toBe("sk-u1"); + expect(await repo.mcpServersWithSecrets()).toEqual([ + { id: "fs", name: "Files", transport: "stdio", command: "x", env: { T: "tok-DRAFT-123456" } }, + { id: "web", name: "Web", transport: "http", url: "https://w.example.com/mcp", headers: { authorization: "Bearer DRAFT-abcdef" } }, + ]); + expect((await repo.getServer()).unreadableSecrets).toEqual([]); + expect(JSON.parse(raw).mcpServers[2]).toBe("not a server"); // malformed entries are kept, not fatal + + const snapshot = raw; + expect(await upgradeAiData(db, box)).toBe(false); + expect((await db.sql`SELECT config_json FROM ai_settings WHERE scope = 'server'`)[0].config_json).toBe(snapshot); +}); + +forEachV1Db("v1 upgrade with a changed STC_SECRET keeps old tokens, stays pending, and finishes once the secret is restored", async (db) => { + await seedSettings(db, "server", "", { + provider: { kind: "anthropic" }, apiKeyCipher: await v1Token("sk-ant-SERVER", "old-secret"), + mcpServers: [{ id: "fs", name: "Files", transport: "stdio", command: "x", env: { T: "tok-PLAIN-123456" } }], + }); + await db.sql`INSERT INTO user_api_keys (user_id, provider, ciphertext, created_at) VALUES ('u1', 'anthropic', ${await v1Token("sk-u1", "old-secret")}, ${T})`; + await seedSettings(db, "user", "u1", { byok: { kind: "anthropic" } }); + await migrate(db, aiMigrations); + + const repo = aiSettingsRepo(db, box); // the new secret + expect(await repo.userApiKey("u1")).toBeNull(); + expect((await repo.getUser("u1")).byok).toEqual({ kind: "anthropic", hasApiKey: true, keyUnreadable: true }); + expect((await repo.getServer()).unreadableSecrets).toEqual([{ kind: "api-key" }]); + // plaintext MCP values do not need the old secret: sealed already + expect((await repo.mcpServersWithSecrets())[0]!.env).toEqual({ T: "tok-PLAIN-123456" }); + expect((await db.sql`SELECT name FROM schema_migrations WHERE name = ${AI_SECRETS_UPGRADE}`)).toHaveLength(0); + expect(((await db.sql`SELECT ciphertext FROM user_api_keys`)[0].ciphertext as string).startsWith("v1.")).toBe(true); + + // The admin puts the previous secret back: the next start re-seals and completes. + const restored = createSecretBox("old-secret"); + expect(await upgradeAiData(db, restored)).toBe(true); + expect((await db.sql`SELECT name FROM schema_migrations WHERE name = ${AI_SECRETS_UPGRADE}`)).toHaveLength(1); + const again = aiSettingsRepo(db, restored); + expect(await again.userApiKey("u1")).toBe("sk-u1"); + expect(await again.serverApiKey()).toBe("sk-ant-SERVER"); +}); diff --git a/packages/feature-ai/test/web.test.tsx b/packages/feature-ai/test/web.test.tsx index 231dbd3..058a66a 100644 --- a/packages/feature-ai/test/web.test.tsx +++ b/packages/feature-ai/test/web.test.tsx @@ -54,3 +54,20 @@ test("citation links use the learner's registered lesson route", async () => { // matchPath leaves params encoded (the router decodes the pathname before matching). expect(Object.fromEntries(Object.entries(hit?.params ?? {}).map(([k, v]) => [k, decodeURIComponent(v!)]))).toEqual({ courseId: "pure snake", lessonId: "m1-ü/x" }); }); + +test("the unreadable-secrets warnings name what is affected", async () => { + const { UnreadableSecretsWarning, UNREADABLE_SECRETS_TEXT, UNREADABLE_KEY_TEXT } = await import("../src/web/components"); + expect(UNREADABLE_SECRETS_TEXT).toBe("Some stored secrets can't be decrypted with the current STC_SECRET. Re-enter them or restore the previous secret."); + expect(UNREADABLE_KEY_TEXT).toBe("Your saved key can't be used; please re-enter it."); + const html = renderToString(); + expect(html).toContain("Some stored secrets can't be decrypted with the current STC_SECRET."); + expect(html).toContain("Server provider: API key"); + expect(html).toContain("MCP server "Web" (web): headers"); + expect(html).toContain("MCP server "Files" (fs): environment"); + expect(html).toContain('role="alert"'); + expect(renderToString()).toBe(""); + const account = await Bun.file(new URL("../src/web/AccountAiPage.tsx", import.meta.url)).text(); + expect(account).toContain("UNREADABLE_KEY_TEXT"); + const admin = await Bun.file(new URL("../src/web/AdminAiPage.tsx", import.meta.url)).text(); + expect(admin).toContain("