mirror of
https://github.com/PR0M3TH3AN/SeedPass.git
synced 2026-11-01 18:32:05 +00:00
Stop the config API accepting any key and any value
Adversarial pass, first finding. `PUT /api/v1/config/:key` took the key from the URL and the value from the body with no validation beyond blocking two credential verifiers. Two ways that goes wrong, both silent: A typo is stored forever. `inactivity_timout` was accepted, written into the encrypted config, and answered ok — while the setting it was meant to change never moved. Nothing downstream reads an unknown key, so this failure can only ever be silent; the caller has no way to find out. A wrong type is stored verbatim. `inactivity_timeout: "soon"` turns every later comparison into NaN, which compares false, so the vault simply never times out. That is a security setting failing open because a string reached a numeric field. There is now an allowlist of 23 keys, each with a normalizer that rejects values it cannot make sense of: negative delays, a zero inactivity timeout (indistinguishable from a broken vault), an empty relay list (not a configuration, an outage), a string where an array belongs. Unknown keys get Python's 400 "Unknown key"; the credential verifiers move from 400 to 403, matching Python, since the key is real and the refusal is about authority. The allowlist is deliberately a SUPERSET of Python's, which cannot set the password-policy keys this implementation honours. Copying Python exactly would have deleted working behaviour to reproduce a limitation. Recorded in TODO.md as a divergence to settle rather than left as an accident. Separately: the shared API test fixture was inheriting the default 100 requests/minute budget and had grown past it, so unrelated tests failed with 429 depending on how many requests ran before them. Rate limiting is tested on its own servers with explicit budgets, so the fixture no longer enforces one. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01Vtx94nXDmNgXEeThUZP4QN
This commit is contained in:
1 parent
d6ffbac312
commit
ae3889bfca
4 files changed
+210
-9
No files matched your search
@@ -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: "<redacted>" } };
|
||||
}
|
||||
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" } };
|
||||
});
|
||||
|
||||
Reference in new issue
Block a user