From 55879eaa1bb674b472c87e4cb96984dd36dadcfe Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:10:20 -0400 Subject: [PATCH] security: durable vault writes, path validation, and safe restore semantics Continues the security-review fixes. Durability and concurrency: - Vault, config, fingerprint-registry and parent-seed writes are now atomic: temp file in the same directory, fsync, rename, then an fsync of the directory. Previously the target was truncated and rewritten in place, so a crash could destroy the vault. - The lock now spans the whole read-modify-write cycle, not just the write. Serializing writes alone still lost changes -- two commands each read the same index, mutated their copy, and wrote in turn. Every mutating command goes through mutateVault(), which holds the lock from open to save. A test runs five concurrent writers and asserts all five entries survive. - 0600 is now enforced on existing files too: writeFile's mode argument only applies on creation, so a loosened vault stayed loosened. - The index is schema-validated before it can reach disk. Path handling: - Profile fingerprints are validated (16 uppercase hex) before being used in any path, and removeProfile additionally checks the resolved directory still sits directly under the app directory. A tampered registry could otherwise have steered a recursive delete elsewhere. - Malformed entries in fingerprints.json are dropped on read. Restore semantics: - `nostr restore` merges remote state into the local vault instead of replacing it. Replacing silently discarded entries created since the last sync; that now requires --replace --yes, and any overwritten vault is copied aside first. - Deltas are bound to the manifest they extend (filtered on the e tag, and re-checked locally), so a relay cannot replay a validly-signed delta from another snapshot lineage into a restore. - Manifest selection breaks created_at ties by relay order, so a same-second republish cannot leave a restore on a stale snapshot. - mergeIndexPayloads gained an index0 option. The default still refuses to merge populated Python atlas state rather than drop it; restore uses preserve-current, keeping the local block verbatim, because Python recomputes canonical views on load and the default would block every restore into a Python-created profile. 193 core + 69 CLI tests green; 36/36 cross-implementation checks green. Co-Authored-By: Claude Fable 5 --- js/packages/cli/src/appDir.ts | 56 ++++-- js/packages/cli/src/configFile.ts | 5 +- js/packages/cli/src/program.ts | 249 +++++++++++++++--------- js/packages/cli/src/vaultFile.ts | 103 +++++++++- js/packages/cli/test/durability.test.ts | 129 ++++++++++++ js/packages/cli/test/nostr.test.ts | 30 ++- js/packages/core/src/index.ts | 1 + js/packages/core/src/sync/merge.ts | 28 ++- js/packages/core/src/sync/syncFlows.ts | 33 +++- 9 files changed, 519 insertions(+), 115 deletions(-) create mode 100644 js/packages/cli/test/durability.test.ts diff --git a/js/packages/cli/src/appDir.ts b/js/packages/cli/src/appDir.ts index 2f9f35b3..7afe753e 100644 --- a/js/packages/cli/src/appDir.ts +++ b/js/packages/cli/src/appDir.ts @@ -8,10 +8,11 @@ * ~/.seedpass//seedpass_config.json.enc index key */ -import { mkdir, readFile, writeFile, rm } from "node:fs/promises"; +import { mkdir, readFile, rm } from "node:fs/promises"; import { existsSync } from "node:fs"; import { homedir } from "node:os"; -import { join } from "node:path"; +import { dirname, join, resolve } from "node:path"; +import { atomicWrite } from "./vaultFile.js"; import process from "node:process"; import { base64url } from "@scure/base"; import { @@ -55,6 +56,25 @@ export function resolveAppDir(override?: string): string { return override ?? process.env["SEEDPASS_APP_DIR"] ?? join(homedir(), ".seedpass"); } +/** + * Fingerprints are 16 uppercase hex characters (SHA-256 of the seed, + * truncated). Validate before any fingerprint reaches a filesystem path: + * these values come from a JSON registry on disk, and a tampered entry + * containing `..` would otherwise steer a recursive delete out of the + * profile directory. + */ +const FINGERPRINT_RE = /^[0-9A-F]{16}$/; + +export function assertValidFingerprint(fingerprint: string): string { + if (!FINGERPRINT_RE.test(fingerprint)) { + throw new Error( + `invalid profile fingerprint ${JSON.stringify(fingerprint)} ` + + `(expected 16 uppercase hex characters)`, + ); + } + return fingerprint; +} + export class AppDir { constructor(public readonly root: string) {} @@ -63,17 +83,20 @@ export class AppDir { } profileDir(fingerprint: string): string { - return join(this.root, fingerprint); + return join(this.root, assertValidFingerprint(fingerprint)); } async readFingerprints(): Promise { try { const raw = JSON.parse(await readFile(this.fingerprintsPath, "utf8")) as Partial; - return { - fingerprints: raw.fingerprints ?? [], - last_used: raw.last_used ?? null, - names: raw.names ?? {}, - }; + const fingerprints = (raw.fingerprints ?? []).filter((f) => + FINGERPRINT_RE.test(String(f)), + ); + const lastUsed = + raw.last_used && FINGERPRINT_RE.test(String(raw.last_used)) + ? String(raw.last_used) + : null; + return { fingerprints, last_used: lastUsed, names: raw.names ?? {} }; } catch { return { fingerprints: [], last_used: null, names: {} }; } @@ -82,7 +105,7 @@ export class AppDir { async writeFingerprints(data: FingerprintsFile): Promise { await mkdir(this.root, { recursive: true }); // indent=4 matches the Python writer - await writeFile(this.fingerprintsPath, JSON.stringify(data, null, 4), { mode: 0o600 }); + await atomicWrite(this.fingerprintsPath, utf8(JSON.stringify(data, null, 4))); } /** Create a profile from a mnemonic; returns its fingerprint. */ @@ -118,15 +141,14 @@ export class AppDir { kdf, ct: Buffer.from(ct).toString("base64"), }); - await writeFile(join(dir, PARENT_SEED_FILENAME), wrapper, { mode: 0o600 }); + await atomicWrite(join(dir, PARENT_SEED_FILENAME), utf8(wrapper)); // empty entries index under the index key const indexKey = deriveIndexKeyBytes(mnemonic); const emptyIndex = { schema_version: 4, entries: {} }; - await writeFile( + await atomicWrite( join(dir, INDEX_FILENAME), await encryptV3(indexKey, utf8(JSON.stringify(emptyIndex))), - { mode: 0o600 }, ); data.fingerprints.push(fingerprint); @@ -137,11 +159,18 @@ export class AppDir { } async removeProfile(fingerprint: string): Promise { + assertValidFingerprint(fingerprint); const data = await this.readFingerprints(); if (!data.fingerprints.includes(fingerprint)) { throw new Error(`no profile ${fingerprint}`); } - await rm(this.profileDir(fingerprint), { recursive: true, force: true }); + const dir = this.profileDir(fingerprint); + // Belt and braces: the resolved path must still sit directly under the + // app directory before anything is deleted recursively. + if (dirname(resolve(dir)) !== resolve(this.root)) { + throw new Error(`refusing to delete ${dir}: outside ${this.root}`); + } + await rm(dir, { recursive: true, force: true }); data.fingerprints = data.fingerprints.filter((f) => f !== fingerprint); delete data.names[fingerprint]; if (data.last_used === fingerprint) { @@ -151,6 +180,7 @@ export class AppDir { } async switchProfile(fingerprint: string): Promise { + assertValidFingerprint(fingerprint); const data = await this.readFingerprints(); if (!data.fingerprints.includes(fingerprint)) { throw new Error(`no profile ${fingerprint}`); diff --git a/js/packages/cli/src/configFile.ts b/js/packages/cli/src/configFile.ts index f62b3771..3bc1791f 100644 --- a/js/packages/cli/src/configFile.ts +++ b/js/packages/cli/src/configFile.ts @@ -3,7 +3,7 @@ * (seedpass_config.json.enc, encrypted under the index key). */ -import { readFile, writeFile } from "node:fs/promises"; +import { readFile } from "node:fs/promises"; import { existsSync } from "node:fs"; import { join } from "node:path"; import { @@ -14,6 +14,7 @@ import { utf8, } from "@seedpass/core"; import { CONFIG_FILENAME } from "./appDir.js"; +import { atomicWrite } from "./vaultFile.js"; export const DEFAULT_RELAYS = [ "wss://relay.snort.social", @@ -69,5 +70,5 @@ export async function saveConfig( ): Promise { const key = deriveIndexKeyBytes(mnemonic); const payload = await encryptV3(key, utf8(JSON.stringify(config))); - await writeFile(join(profileDir, CONFIG_FILENAME), payload, { mode: 0o600 }); + await atomicWrite(join(profileDir, CONFIG_FILENAME), payload); } diff --git a/js/packages/cli/src/program.ts b/js/packages/cli/src/program.ts index e74d864b..866649b3 100644 --- a/js/packages/cli/src/program.ts +++ b/js/packages/cli/src/program.ts @@ -13,7 +13,14 @@ import { entryMetadata, parseRef, resolveEntry } from "./refs.js"; import { materializeSecret } from "./secrets.js"; import { clipboardSink, execSink, parseCommandSpec, stdinSink } from "./sinks.js"; import { capabilities } from "./capabilities.js"; -import { openVault, saveVault, type OpenedVault } from "./vaultFile.js"; +import { + openVault, + saveVault, + saveVaultHoldingLock, + withVaultLock, + atomicWrite, + type OpenedVault, +} from "./vaultFile.js"; import { importBackup, exportBackup, @@ -176,6 +183,30 @@ async function openFromOptions(opts: GlobalOpts): Promise { return openVault(path, await resolveMnemonic(app, opts)); } +/** + * Run a mutation under the vault lock, holding it for the whole + * read-modify-write cycle. + * + * Locking only the write still loses changes: two commands can each read the + * same index, mutate their copy, and write in turn. Everything that changes + * the vault must go through here. + */ +async function mutateVault( + opts: GlobalOpts, + fn: (vault: OpenedVault) => Promise | T, +): Promise { + const app = new AppDir(resolveAppDir(opts.appDir)); + const path = + opts.vault ?? join(app.profileDir(await currentFingerprint(app, opts)), INDEX_FILENAME); + const mnemonic = await resolveMnemonic(app, opts); + return withVaultLock(path, async () => { + const vault = await openVault(path, mnemonic); + const result = await fn(vault); + await saveVaultHoldingLock(vault); + return result; + }); +} + /** * Read/secret access that works in two modes: * - owner mode (mnemonic via env or agent): full local access @@ -366,10 +397,15 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { return cmd; }; - async function finishAdd(vault: OpenedVault, id: string): Promise { - await saveVault(vault); - const e = vault.index.entries[id]!; - io.out(JSON.stringify(entryMetadata(id, e), null, 2)); + /** Create an entry under the lock and print its reference + metadata. */ + async function addEntry( + create: (vault: OpenedVault) => string, + ): Promise { + const row = await mutateVault(program.opts(), (vault) => { + const id = create(vault); + return entryMetadata(id, vault.index.entries[id]!); + }); + io.out(JSON.stringify(row, null, 2)); } const commonOpts = (o: { notes?: string; tags?: string[]; archived?: boolean }) => ({ @@ -390,13 +426,11 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { label: string, o: { length: string; username?: string; url?: string; notes?: string; tags?: string[]; archived?: boolean }, ) => { - const vault = await openFromOptions(program.opts()); - const id = addPasswordEntry(vault.index, label, parseIntOption(o.length, "--length", { min: 8, max: 128 }), { + await addEntry((vault) => addPasswordEntry(vault.index, label, parseIntOption(o.length, "--length", { min: 8, max: 128 }), { ...commonOpts(o), ...(o.username !== undefined && { username: o.username }), ...(o.url !== undefined && { url: o.url }), - }); - await finishAdd(vault, id); + })); }, ); @@ -412,16 +446,16 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { label: string, o: { secret?: string; period: string; digits: string; notes?: string; tags?: string[]; archived?: boolean }, ) => { - const vault = await openFromOptions(program.opts()); const opts = { ...commonOpts(o), period: parseIntOption(o.period, "--period", { min: 1 }), digits: parseIntOption(o.digits, "--digits", { min: 6, max: 10 }), }; - const id = o.secret - ? addTotpImported(vault.index, label, o.secret, opts) - : addTotpDeterministic(vault.index, label, vault.mnemonic, opts); - await finishAdd(vault, id); + await addEntry((vault) => + o.secret + ? addTotpImported(vault.index, label, o.secret, opts) + : addTotpDeterministic(vault.index, label, vault.mnemonic, opts), + ); }, ); @@ -436,9 +470,7 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { value: string, o: { notes?: string; tags?: string[]; archived?: boolean }, ) => { - const vault = await openFromOptions(program.opts()); - const id = addKeyValueEntry(vault.index, label, key, value, commonOpts(o)); - await finishAdd(vault, id); + await addEntry((vault) => addKeyValueEntry(vault.index, label, key, value, commonOpts(o))); }, ); @@ -453,12 +485,10 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { content: string, o: { fileType: string; notes?: string; tags?: string[]; archived?: boolean }, ) => { - const vault = await openFromOptions(program.opts()); - const id = addDocumentEntry(vault.index, label, content, { + await addEntry((vault) => addDocumentEntry(vault.index, label, content, { ...commonOpts(o), fileType: o.fileType, - }); - await finishAdd(vault, id); + })); }, ); @@ -469,41 +499,37 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { .option("--words ", "word count", "24"), ).action( async (label: string, o: { words: string; notes?: string; tags?: string[]; archived?: boolean }) => { - const vault = await openFromOptions(program.opts()); const words = parseIntOption(o.words, "--words"); if (words !== 12 && words !== 18 && words !== 24) { throw new Error("--words must be 12, 18 or 24"); } - const id = addSeedEntry(vault.index, label, { - ...commonOpts(o), - wordCount: words as 12 | 18 | 24, - }); - await finishAdd(vault, id); + await addEntry((vault) => + addSeedEntry(vault.index, label, { + ...commonOpts(o), + wordCount: words as 12 | 18 | 24, + }), + ); }, ); commonAddOptions( add.command("managed-account