Skip to content

[Med] fix(cli): serialize local-vault writes to prevent TOCTOU silent data loss #713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
katnisscalls99 wants to merge 1 commit into
base: master
Choose a base branch
from
+34 −9
Open

[Med] fix(cli): serialize local-vault writes to prevent TOCTOU silent data loss #713

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 34 additions & 9 deletions packages/cli/src/local-vault.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,22 @@ import { promises as fs, readFileSync, statSync } from 'node:fs';
import path from 'node:path';
import { configDir } from '@profullstack/sh1pt-core';

// In-process serialization lock for read-modify-write operations.
// Prevents concurrent setSecretInLocal / deleteSecretFromLocal calls
// (e.g. from Promise.all over multiple adapter setups) from silently
// overwriting each other's writes. Each mutation acquires this before
// reading, modifies the in-memory snapshot, writes, then releases.
let _vaultLock: Promise<void> = Promise.resolve();
function withVaultLock<T>(fn: () => Promise<T>): Promise<T> {
const next = _vaultLock.then(fn, fn);
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Jun 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 fn is passed directly as the onRejected handler in _vaultLock.then(fn, fn). When the previous lock step rejects, Promise calls fn(rejectionReason) — the rejection reason becomes fn's first positional argument. Since fn is typed () => Promise<T> and JavaScript silently ignores extra arguments, this is harmless at runtime, but it's a subtle coupling: TypeScript allows it only because a zero-parameter function is structurally assignable to a one-parameter function type. The idiomatic form wraps fn in a thunk so the call contract is unambiguous and the intent ("run fn regardless of prior outcome") is immediately clear to the next reader.

Suggested change
const next = _vaultLock.then(fn, fn);
const next = _vaultLock.then(() => fn(), () => fn());

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

// Reset the chain tail so the lock doesn't grow unbounded.
_vaultLock = next.then(
() => {},
() => {},
);
return next;
}

const VAULT_VERSION = 1;

interface LocalVault {
Expand Down Expand Up @@ -76,7 +92,9 @@ async function readVault(): Promise<LocalVault> {
async function writeVault(v: LocalVault): Promise<void> {
const file = localVaultPath();
await fs.mkdir(path.dirname(file), { recursive: true, mode: 0o700 });
const tmp = `${file}.tmp`;
// Include pid + a random suffix so concurrent writers never share a tmp
// filename and stomp each other's in-progress write.
const tmp = `${file}.${process.pid}.${Math.random().toString(36).slice(2)}.tmp`;
await fs.writeFile(tmp, JSON.stringify(v, null, 2) + '\n', { mode: 0o600 });
await fs.rename(tmp, file);
// rename preserves the source mode, but be explicit if the destination
Expand All @@ -85,9 +103,14 @@ async function writeVault(v: LocalVault): Promise<void> {
}
Comment on lines 92 to 103
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Jun 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Orphaned tmp files can accumulate with unique names

The old fixed ${file}.tmp path meant a failed write left exactly one orphan that the next successful write would atomically replace. With the new per-call unique name, every interrupted write (process kill, disk-full, etc.) leaves a permanent secrets.json.<pid>.<random>.tmp in ~/.config/sh1pt/. Over time these accumulate — each is a 0o600 copy of the vault at that snapshot, so there's no secret-exposure risk, but a busy CLI user could end up with many stale files in their config dir. Adding a fs.unlink(tmp).catch(() => {}) in the error path (try/finally around the writeFile+rename pair) would clean them up on failure.


export async function setSecretInLocal(key: string, value: string): Promise<void> {
const v = await readVault();
v.secrets[key] = value;
await writeVault(v);
// Serialize through the in-process lock to prevent concurrent
// read-modify-write races (e.g. Promise.all over multiple adapter setups)
// from silently dropping each other's writes.
return withVaultLock(async () => {
const v = await readVault();
v.secrets[key] = value;
await writeVault(v);
});
}

export async function getSecretFromLocal(key: string): Promise<string | undefined> {
Expand All @@ -96,11 +119,13 @@ export async function getSecretFromLocal(key: string): Promise<string | undefine
}

export async function deleteSecretFromLocal(key: string): Promise<boolean> {
const v = await readVault();
if (!(key in v.secrets)) return false;
delete v.secrets[key];
await writeVault(v);
return true;
return withVaultLock(async () => {
const v = await readVault();
if (!(key in v.secrets)) return false;
delete v.secrets[key];
await writeVault(v);
return true;
});
}

export async function listSecretsLocal(): Promise<Array<{ key: string }>> {
Expand Down