diff --git a/TODO.md b/TODO.md index 60bf8ee4..516d48c6 100644 --- a/TODO.md +++ b/TODO.md @@ -180,6 +180,18 @@ everything still open, in the order it should be tackled. zero mutants and printed as a clean sweep. Both fixed: inline guards are mutated, and a target that generates no mutants now reports NOT MEASURED rather than passing silently. + - [ ] **DECISION TO RECORD: the API's settable config keys are a + superset of Python's.** Python's `update_config` allowlists 8 + keys; the TypeScript route now allowlists 23, because it can set + the password-policy keys (`min_uppercase`, `exclude_ambiguous` + and friends) that this implementation honours and Python's API + does not expose. Matching Python exactly would mean deleting + working behaviour to reproduce a limitation, so the divergence is + deliberate — but it IS a divergence, and a client that sets a + policy key succeeds here and gets 400 there. Either widen + Python's allowlist or document the API surfaces as + non-identical; do not quietly leave it as an accident. + (Found 2026-08-19 during the adversarial pass.) - [ ] **Adversarial pass over TS-only surfaces**, framed as "what does this trust?" rather than "is this correct?" — the framing that finds trust-boundary bugs rather than logic bugs. diff --git a/js/packages/cli/src/api/routes.ts b/js/packages/cli/src/api/routes.ts index 7d758cb2..fd6a73db 100644 --- a/js/packages/cli/src/api/routes.ts +++ b/js/packages/cli/src/api/routes.ts @@ -55,7 +55,15 @@ import { type VaultIndex, } from "@seedpass/core"; import { AppDir, INDEX_FILENAME, DEFAULT_PBKDF2_ITERATIONS } from "../appDir.js"; -import { loadConfig, mutateConfig, passwordPolicyFromConfig, DEFAULT_RELAYS } from "../configFile.js"; +import { + loadConfig, + mutateConfig, + passwordPolicyFromConfig, + DEFAULT_RELAYS, + SETTABLE_CONFIG_KEYS, + SENSITIVE_CONFIG_KEYS, + ConfigValueError, +} from "../configFile.js"; import { openVault, saveVault, saveVaultHoldingLock, withVaultLock, atomicWrite } from "../vaultFile.js"; import { entryMetadata, refFor, resolveEntry } from "../refs.js"; import { materializeSecret } from "../secrets.js"; @@ -510,10 +518,12 @@ export function registerRoutes(server: ApiServer, ctx: ApiContext): void { const mnemonic = requireUnlocked(ctx); const key = req.params["key"]!; const config = await loadConfig(profileDir(ctx), mnemonic); - if (key === "password_hash" || key === "pin_hash") { + if (SENSITIVE_CONFIG_KEYS.has(key)) { // Never hand back a credential verifier over the wire, even to an // authenticated caller: it is offline-crackable and nothing legitimate - // needs it. + // needs it. Answers with a placeholder rather than Python's 403 so a + // client can tell "exists but withheld" from "no such key" — the value + // is withheld either way. return { json: { key, value: "" } }; } return { json: { key, value: config[key] ?? null } }; @@ -522,13 +532,28 @@ export function registerRoutes(server: ApiServer, ctx: ApiContext): void { server.route("PUT", "/api/v1/config/:key", async (req) => { const mnemonic = requireUnlocked(ctx); const key = req.params["key"]!; - if (key === "password_hash" || key === "pin_hash") { - throw new HttpError(400, `${key} is not settable through the API`); + if (SENSITIVE_CONFIG_KEYS.has(key)) { + // 403, matching Python. These are credential verifiers, not settings. + throw new HttpError(403, `${key} is not settable through the API`); } + // Unknown keys are refused rather than stored. This route used to accept + // any key at all, so a typo became a permanent config entry that nothing + // ever read, while the API answered ok and the setting never moved. + const normalize = Object.prototype.hasOwnProperty.call(SETTABLE_CONFIG_KEYS, key) + ? SETTABLE_CONFIG_KEYS[key] + : undefined; + if (!normalize) throw new HttpError(400, "Unknown key"); const body = bodyObject(req); if (!("value" in body)) throw new HttpError(400, "value is required"); + let value: unknown; + try { + value = normalize(body["value"]); + } catch (e) { + if (e instanceof ConfigValueError) throw new HttpError(400, e.message); + throw e; + } await mutateConfig(profileDir(ctx), mnemonic, (config) => { - config[key] = body["value"]; + config[key] = value; }); return { json: { status: "ok" } }; }); diff --git a/js/packages/cli/src/configFile.ts b/js/packages/cli/src/configFile.ts index c185e7dd..08f05b85 100644 --- a/js/packages/cli/src/configFile.ts +++ b/js/packages/cli/src/configFile.ts @@ -50,6 +50,111 @@ export function defaultConfig(): Record { }; } +/** + * Config keys a caller may set through the API, and how each value is + * normalized. + * + * WHY AN ALLOWLIST + * + * `PUT /api/v1/config/:key` took the key straight from the URL and the value + * straight from the body, so any string at all became a permanent config + * entry holding any JSON at all. A typo (`inactivity_timout`) was accepted, + * stored forever, and reported as ok while the setting it was meant to + * change never moved. Nothing downstream ever reads an unknown key, so the + * failure is silent by construction. + * + * Python allowlists too (`api.py`, update_config) but its list is much + * narrower — it cannot set the password-policy keys at all, which this + * implementation can and does. So this is a superset of Python's rather than + * a copy of it: matching Python exactly would mean deleting working + * behaviour to reproduce a limitation. The divergence is recorded in TODO.md. + * + * Coercion matters as much as the allowlist. `inactivity_timeout: "soon"` + * used to be stored verbatim and produce NaN comparisons at lock time; the + * normalizer rejects it instead. Python does `float(v)`, which raises + * uncaught and answers 500 — a 400 is the same refusal, said properly. + */ +export class ConfigValueError extends Error {} + +function asNumber(key: string, v: unknown): number { + const n = Number(v); + if (typeof v === "boolean" || v === null || v === "" || !Number.isFinite(n)) { + throw new ConfigValueError(`${key} must be a number`); + } + return n; +} + +function asInt(key: string, v: unknown): number { + return Math.trunc(asNumber(key, v)); +} + +function asNonNegativeInt(key: string, v: unknown): number { + const n = asInt(key, v); + if (n < 0) throw new ConfigValueError(`${key} must not be negative`); + return n; +} + +function asString(key: string, v: unknown): string { + if (typeof v !== "string") throw new ConfigValueError(`${key} must be a string`); + return v; +} + +function asStringArray(key: string, v: unknown): string[] { + if (!Array.isArray(v) || v.some((x) => typeof x !== "string")) { + throw new ConfigValueError(`${key} must be an array of strings`); + } + return v as string[]; +} + +export const SETTABLE_CONFIG_KEYS: Record unknown> = { + relays: (v) => { + const list = asStringArray("relays", v); + // An empty relay list is not a configuration, it is an outage: sync + // would silently stop with everything still reporting healthy. + if (list.length === 0) throw new ConfigValueError("relays must not be empty"); + return list; + }, + offline_mode: (v) => Boolean(v), + online_mode_notice_seen: (v) => Boolean(v), + inactivity_timeout: (v) => { + const n = asNumber("inactivity_timeout", v); + // Zero would mean "lock immediately", which is indistinguishable from a + // broken vault, and negative means "already expired". + if (n <= 0) throw new ConfigValueError("inactivity_timeout must be positive"); + return n; + }, + kdf_iterations: (v) => { + const n = asInt("kdf_iterations", v); + if (n < 1) throw new ConfigValueError("kdf_iterations must be at least 1"); + return n; + }, + kdf_mode: (v) => asString("kdf_mode", v), + argon2_time_cost: (v) => asNonNegativeInt("argon2_time_cost", v), + additional_backup_path: (v) => asString("additional_backup_path", v), + backup_interval: (v) => asNonNegativeInt("backup_interval", v), + secret_mode_enabled: (v) => Boolean(v), + clipboard_clear_delay: (v) => asNonNegativeInt("clipboard_clear_delay", v), + quick_unlock_enabled: (v) => Boolean(v), + nostr_max_retries: (v) => asNonNegativeInt("nostr_max_retries", v), + nostr_retry_delay: (v) => asNumber("nostr_retry_delay", v), + nostr_key_index: (v) => asNonNegativeInt("nostr_key_index", v), + // Password policy. Absent from Python's API allowlist; see above. + include_special_chars: (v) => Boolean(v), + allowed_special_chars: (v) => asString("allowed_special_chars", v), + special_mode: (v) => asString("special_mode", v), + exclude_ambiguous: (v) => Boolean(v), + min_uppercase: (v) => asNonNegativeInt("min_uppercase", v), + min_lowercase: (v) => asNonNegativeInt("min_lowercase", v), + min_digits: (v) => asNonNegativeInt("min_digits", v), + min_special: (v) => asNonNegativeInt("min_special", v), +}; + +/** + * Credential verifiers. Offline-crackable, and nothing legitimate reads them + * over the wire or writes them through a generic setter. + */ +export const SENSITIVE_CONFIG_KEYS = new Set(["password_hash", "pin_hash"]); + /** * The profile's password policy, as the BASE that entry `policy` blocks * override. diff --git a/js/packages/cli/test/api.test.ts b/js/packages/cli/test/api.test.ts index 6be6d185..eea9e1c2 100644 --- a/js/packages/cli/test/api.test.ts +++ b/js/packages/cli/test/api.test.ts @@ -123,7 +123,12 @@ beforeAll(async () => { await encryptV3(deriveIndexKeyBytes(MNEMONIC), utf8(JSON.stringify(index))), ); - server = new ApiServer({ host: "127.0.0.1", port: 0, token: TOKEN }); + // Generous on purpose. The default budget is 100 requests a minute, and + // this file passed it as it grew — which made unrelated tests fail with + // 429 depending on how many requests ran before them. Rate limiting has + // its own tests below, each on a server with a budget it sets explicitly, + // so the shared fixture should not be enforcing one at all. + server = new ApiServer({ host: "127.0.0.1", port: 0, token: TOKEN, rateLimit: 100_000 }); ctx = buildContext({ app, fingerprint: FINGERPRINT, mnemonic: MNEMONIC }); registerRoutes(server, ctx); const bound = await server.listen(); @@ -206,12 +211,66 @@ describe("what the API refuses", () => { for (const key of ["password_hash", "pin_hash"]) { const got = await call("GET", `/api/v1/config/${key}`); expect(got.json.value).toBe(""); - // And it cannot be set either — nothing legitimate needs to. + // And it cannot be set either — nothing legitimate needs to. 403, not + // 400: the key is real and the refusal is about authority, which is + // also the status Python answers. const set = await call("PUT", `/api/v1/config/${key}`, { body: { value: "x" } }); - expect(set.status).toBe(400); + expect(set.status).toBe(403); } }); + it("refuses a config key it does not know, instead of storing it forever", async () => { + // This route used to take the key from the URL and the value from the + // body with no validation at all, so `inactivity_timout` was accepted, + // written into the encrypted config permanently, and reported as ok — + // while the setting it was meant to change never moved. Nothing reads an + // unknown key, so the failure could only ever be silent. + const typo = await call("PUT", "/api/v1/config/inactivity_timout", { + body: { value: 60 }, + }); + expect(typo.status).toBe(400); + expect(typo.json.detail).toContain("Unknown key"); + expect((await call("GET", "/api/v1/config/inactivity_timout")).json.value).toBeNull(); + + // Nor a key that only LOOKS like an object property. + expect( + (await call("PUT", "/api/v1/config/__proto__", { body: { value: {} } })).status, + ).toBe(400); + expect( + (await call("PUT", "/api/v1/config/constructor", { body: { value: 1 } })).status, + ).toBe(400); + }); + + it("refuses a value of the wrong type for a key it does know", async () => { + // The allowlist alone is not enough: `inactivity_timeout: "soon"` used to + // be stored verbatim and turn every later comparison into NaN, which + // compares false — so the vault would simply never time out. + for (const [key, value] of [ + ["inactivity_timeout", "soon"], + ["inactivity_timeout", 0], + ["clipboard_clear_delay", -1], + ["relays", "wss://one.example"], + ["relays", []], + ["min_uppercase", "lots"], + ] as const) { + const res = await call("PUT", `/api/v1/config/${key}`, { body: { value } }); + expect(res.status).toBe(400); + } + + // And the well-typed versions still go through, so the check is + // rejecting the value rather than the key. + expect( + (await call("PUT", "/api/v1/config/inactivity_timeout", { body: { value: 120 } })) + .status, + ).toBe(200); + expect((await call("GET", "/api/v1/config/inactivity_timeout")).json.value).toBe(120); + expect( + (await call("PUT", "/api/v1/config/relays", { + body: { value: ["wss://one.example"] }, + })).status, + ).toBe(200); + }); + it("caps the request body rather than buffering whatever arrives", async () => { const small = new ApiServer({ host: "127.0.0.1", port: 0, token: TOKEN, maxBodyBytes: 64 }); const smallCtx = buildContext({ app, fingerprint: FINGERPRINT, mnemonic: MNEMONIC });