diff --git a/server/index.js b/server/index.js index 9bdaaf95..8b7cbf4a 100644 --- a/server/index.js +++ b/server/index.js @@ -247,7 +247,11 @@ app.patch("/api/sparks/:id", (req, res) => { return res.json({ success: true, spark, hasPassword: true }); } - const spark = registry.updateSpark(req.params.id, body); + // LLM API keys: an llmPorts change is the second bypass besides out-of-band + // sparks.json writes. patchSpark() arms the reconcile on the llmPorts + // own-property (any shape — [] and the legacy scalar are applied by the + // normalizer too) and syncs against the post-normalize ports. + const { spark } = registry.patchSpark(req.params.id, body); // Restart monitor so collectors pick up host/auth/isLocal changes stopMonitor(req.params.id); startMonitor(spark); diff --git a/server/sparks/SparkRegistry.js b/server/sparks/SparkRegistry.js index 699a42ad..2e6d0465 100644 --- a/server/sparks/SparkRegistry.js +++ b/server/sparks/SparkRegistry.js @@ -250,6 +250,40 @@ export class SparkRegistry { this._sparks = []; } } + + // Out-of-band config edits (sparks.json edited directly without resyncing + // secrets) can rename llmPorts underneath stored API keys. Reconcile once + // here — before the registry is handed to anything — and never delete key + // material at load. + this._reconcileLlmApiKeysAtLoad(); + } + + /** + * Load-time LLM key/port reconcile. + * An out-of-band sparks.json edit that renames llmPorts (without going + * through PATCH / PUT llm-ports) leaves keys keyed on ports the spark no + * longer exposes. When exactly one keyed port is orphaned and exactly one + * configured port lacks a key, the rename shape is unambiguous → MOVE the + * key. Any other mismatch shape is warn-only: never prune, never delete + * stored key material at load. + */ + _reconcileLlmApiKeysAtLoad() { + for (const spark of this._sparks) { + const configured = Array.isArray(spark.llmPorts) ? spark.llmPorts : []; + const keyed = this.llmApiKeyPorts(spark.id); + const orphans = keyed.filter((p) => !configured.includes(p)); + const missing = configured.filter((p) => !this.hasLlmApiKey(spark.id, p)); + if (orphans.length === 1 && missing.length === 1) { + this.moveLlmApiKey(spark.id, orphans[0], missing[0]); + console.warn( + `[SparkRegistry] migrated LLM API key for spark ${spark.id}: port ${orphans[0]} -> port ${missing[0]} after out-of-band config change` + ); + } else if (orphans.length > 0 || missing.length > 0) { + console.warn( + `[SparkRegistry] spark ${spark.id} LLM key/port mismatch: keyed=<${orphans.join(", ")}> missing=<${missing.join(", ")}>` + ); + } + } } _save() { @@ -434,6 +468,35 @@ export class SparkRegistry { * @param {number[]} prevPorts * @param {number[]} nextPorts */ + /** + * updateSpark() plus LLM API key reconcile for PATCH /api/sparks/:id. + * + * Armed whenever the patch carries an `llmPorts` own-property — including + * `[]` and the legacy single-value shape — because _normalizeLlmPorts() + * applies those too (empty/invalid -> [LLM_PORT], scalar -> [n]). The sync + * runs against the POST-normalize ports on the stored spark, so it sees the + * same rename the persisted record does, not the raw request body. + * @param {string} id + * @param {object} updates PATCH body + * @returns {{ spark: object, llmPortsSynced: boolean }} + */ + patchSpark(id, updates) { + const body = updates || {}; + const armed = Object.prototype.hasOwnProperty.call(body, "llmPorts"); + let prevPorts = null; + if (armed) { + const existing = this.getSpark(id); + if (!existing) throw new Error(`Spark ${id} not found`); + prevPorts = Array.isArray(existing.llmPorts) ? [...existing.llmPorts] : []; + } + const updated = this.updateSpark(id, body); + const llmPortsSynced = armed && Array.isArray(updated.llmPorts); + if (llmPortsSynced) { + this.syncLlmApiKeysToPorts(id, prevPorts, updated.llmPorts); + } + return { spark: llmPortsSynced ? this.getSpark(id) : updated, llmPortsSynced }; + } + syncLlmApiKeysToPorts(id, prevPorts, nextPorts) { const prev = Array.isArray(prevPorts) ? prevPorts : []; const next = Array.isArray(nextPorts) ? nextPorts : []; diff --git a/server/sparks/__tests__/SparkRegistry.llmApiKeys.test.js b/server/sparks/__tests__/SparkRegistry.llmApiKeys.test.js new file mode 100644 index 00000000..adbfabbb --- /dev/null +++ b/server/sparks/__tests__/SparkRegistry.llmApiKeys.test.js @@ -0,0 +1,181 @@ +/** + * SparkRegistry LLM API key sync guard (follow-up to #25/#26). + * + * An out-of-band sparks.json edit can rename llmPorts without touching the + * encrypted secrets store; PATCH /api/sparks/:id was the other bypass. Result: + * the registry kept an orphaned key on the old port (probe then hit the host + * without auth → 401s). Covered here: + * - load-time reconcile: unambiguous single-port rename MOVES the key + * - load-time reconcile: every other mismatch shape is warn-only and NEVER + * deletes stored key material at load + * - syncLlmApiKeysToPorts: rename move + prune semantics (the rails the + * PATCH llmPorts path and PUT /api/sparks/:id/llm-ports drive) + * + * Run: npm test + */ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +// Env must be set before the first import of config.js (paths are captured as +// consts at module evaluation); each node --test file runs in its own process. +const TMPDIR = fs.mkdtempSync(path.join(os.tmpdir(), "sparkdash-registry-keys-")); +process.env.SPARKS_JSON_PATH = path.join(TMPDIR, "sparks.json"); +process.env.SPARKS_SECRETS_PATH = path.join(TMPDIR, "sparks-secrets.json"); +process.env.SECRETS_KEY_PATH = path.join(TMPDIR, ".secrets-key"); +process.env.LLM_PORT = "8888"; + +const { SparkRegistry } = await import("../SparkRegistry.js"); +const { saveSecrets } = await import("../../secretsStore.js"); +const { LLM_PORT } = await import("../../config.js"); + +/** Synthetic test key only — never real key material. */ +const TEST_KEY = "sk-test-0000"; + +const SPARK_ID = "t"; + +/** + * Seed sparks.json + the encrypted secrets store, then construct the registry + * through its real load path so the load-time reconcile provably runs. + * @param {number[]} llmPorts configured ports + * @param {Record} llmApiKeys port -> key in the secrets store + * @param {{ reset?: boolean }} [opts] reset wipes seeded state first + */ +function loadRegistry(llmPorts, llmApiKeys = {}, { reset = false } = {}) { + if (reset) { + fs.rmSync(process.env.SPARKS_JSON_PATH, { force: true }); + fs.rmSync(process.env.SPARKS_SECRETS_PATH, { force: true }); + } + fs.writeFileSync( + process.env.SPARKS_JSON_PATH, + JSON.stringify({ + sparks: [{ id: SPARK_ID, name: "T", lanIp: "127.0.0.1", llmPorts }], + }) + ); + saveSecrets( + new Map(), + new Map([[SPARK_ID, Object.fromEntries(Object.entries(llmApiKeys))]]) + ); + return new SparkRegistry(); +} + +/** Capture `[SparkRegistry]` warns for the duration of fn(). */ +function captureRegistryWarns(fn) { + const lines = []; + const orig = console.warn; + console.warn = (...args) => { + if (typeof args[0] === "string" && args[0].includes("[SparkRegistry]")) { + lines.push(args[0]); + } + }; + try { + fn(); + } finally { + console.warn = orig; + } + return lines; +} + +// ─── Load-time reconcile ───────────────────────────────── + +test("load reconcile: unambiguous single-port rename moves the key", () => { + const r = loadRegistry([8899], { "8888": TEST_KEY }); + assert.deepEqual(r.llmApiKeyPorts(SPARK_ID), [8899]); + assert.equal(r.hasLlmApiKey(SPARK_ID, 8888), false); + assert.equal(r.getSpark(SPARK_ID).llmApiKeys["8899"], TEST_KEY); +}); + +test("load reconcile: single-port rename warns with migration message", () => { + const warns = captureRegistryWarns(() => loadRegistry([8899], { "8888": TEST_KEY })); + assert.equal(warns.length, 1); + assert.equal( + warns[0], + "[SparkRegistry] migrated LLM API key for spark t: port 8888 -> port 8899 after out-of-band config change" + ); +}); + +test("load reconcile: ambiguous shape never prunes — key on 8888 survives", () => { + const r = loadRegistry([8015, 8899], { "8888": TEST_KEY }); + // No destructive load path: the orphaned key must still exist, unmoved. + assert.equal(r.hasLlmApiKey(SPARK_ID, 8888), true); + assert.deepEqual(r.llmApiKeyPorts(SPARK_ID), [8888]); + assert.equal(r.getSpark(SPARK_ID).llmApiKeys["8888"], TEST_KEY); +}); + +test("load reconcile: ambiguous shape warns listing port numbers only", () => { + const warns = captureRegistryWarns(() => loadRegistry([8015, 8899], { "8888": TEST_KEY })); + assert.equal(warns.length, 1); + assert.equal( + warns[0], + "[SparkRegistry] spark t LLM key/port mismatch: keyed=<8888> missing=<8015, 8899>" + ); + assert.doesNotMatch(warns[0], /sk-test-0000/); +}); + +test("load reconcile: aligned shape stays silent and unchanged", () => { + const warns = captureRegistryWarns(() => loadRegistry([8899], { "8899": TEST_KEY })); + assert.deepEqual(warns, []); + const r = loadRegistry([8899], { "8899": TEST_KEY }); + assert.deepEqual(r.llmApiKeyPorts(SPARK_ID), [8899]); +}); + +// ─── syncLlmApiKeysToPorts (PATCH llmPorts / PUT llm-ports rails) ── + +function registryWithKey() { + const r = loadRegistry([8899], { "8899": TEST_KEY }, { reset: true }); + r.addSpark({ id: "sp", name: "SP", lanIp: "127.0.0.1", llmPorts: [8888] }); + r.setLlmApiKey("sp", 8888, TEST_KEY); + return r; +} + +test("syncLlmApiKeysToPorts: single rename moves the key with value intact", () => { + const r = registryWithKey(); + r.syncLlmApiKeysToPorts("sp", [8888], [8899]); + assert.deepEqual(r.llmApiKeyPorts("sp"), [8899]); + assert.equal(r.getSpark("sp").llmApiKeys["8899"], TEST_KEY); + assert.equal(r.hasLlmApiKey("sp", 8888), false); +}); + +test("syncLlmApiKeysToPorts: no-shape change is a no-op", () => { + const r = registryWithKey(); + r.syncLlmApiKeysToPorts("sp", [8888], [8888]); + assert.equal(r.hasLlmApiKey("sp", 8888), true); + assert.equal(r.getSpark("sp").llmApiKeys["8888"], TEST_KEY); +}); + +test("syncLlmApiKeysToPorts: removed port without rename is pruned", () => { + const r = registryWithKey(); + r.setLlmApiKey("sp", 9001, TEST_KEY); + r.syncLlmApiKeysToPorts("sp", [8888, 9001], [8888]); + assert.deepEqual(r.llmApiKeyPorts("sp"), [8888]); +}); + +// ─── patchSpark (PATCH /api/sparks/:id path) ───────────────────── + +test("patchSpark: llmPorts [] normalizes to the default port and moves the key", () => { + const r = loadRegistry([8899], { "8899": TEST_KEY }, { reset: true }); + const { spark, llmPortsSynced } = r.patchSpark(SPARK_ID, { llmPorts: [] }); + assert.equal(llmPortsSynced, true); + assert.deepEqual(spark.llmPorts, [LLM_PORT]); + assert.deepEqual(r.llmApiKeyPorts(SPARK_ID), [LLM_PORT]); + assert.equal(r.getSpark(SPARK_ID).llmApiKeys[String(LLM_PORT)], TEST_KEY); + assert.equal(r.hasLlmApiKey(SPARK_ID, 8899), false); +}); + +test("patchSpark: legacy scalar llmPorts is applied and the key follows", () => { + const r = loadRegistry([8888], { "8888": TEST_KEY }, { reset: true }); + const { spark } = r.patchSpark(SPARK_ID, { llmPorts: "8899" }); + assert.deepEqual(spark.llmPorts, [8899]); + assert.deepEqual(r.llmApiKeyPorts(SPARK_ID), [8899]); + assert.equal(r.getSpark(SPARK_ID).llmApiKeys["8899"], TEST_KEY); +}); + +test("patchSpark: a body without llmPorts never touches keys", () => { + const r = loadRegistry([8888], { "8888": TEST_KEY }, { reset: true }); + const { llmPortsSynced } = r.patchSpark(SPARK_ID, { name: "renamed" }); + assert.equal(llmPortsSynced, false); + assert.deepEqual(r.llmApiKeyPorts(SPARK_ID), [8888]); + assert.equal(r.getSpark(SPARK_ID).name, "renamed"); +});