[Med] fix(cli): serialize local-vault writes to prevent TOCTOU silent data loss by katnisscalls99 · Pull Request #713 · profullstack/sh1pt

katnisscalls99 · 2026-06-06T14:47:30Z

setSecretInLocal() and deleteSecretFromLocal() both do read-modify-write on secrets.json with no concurrency protection. If two adapter setup() calls run concurrently (e.g. via Promise.all, or a background heartbeat racing a foreground write), both snapshot the same vault, both write, and the last rename wins — silently dropping the other caller's secret.

Two fixes:

In-process serialization: add a promise-chain lock (withVaultLock) that serializes every read-modify-write. The lock adds no latency for the common single-caller case and prevents the race entirely for the parallel-setup pattern sh1pt is explicitly designed for.
Unique tmp filename: the previous code used a fixed ${file}.tmp path, so two concurrent writeVault() calls in different async tasks could write to the same tmp and stomp each other mid-write. The new pattern appends pid + random suffix so each writer gets its own tmp file and the rename is independent.

Severity: Medium

…loss setSecretInLocal() and deleteSecretFromLocal() both do read-modify-write on secrets.json with no concurrency protection. If two adapter setup() calls run concurrently (e.g. via Promise.all, or a background heartbeat racing a foreground write), both snapshot the same vault, both write, and the last rename wins — silently dropping the other caller's secret. Two fixes: 1. In-process serialization: add a promise-chain lock (withVaultLock) that serializes every read-modify-write. The lock adds no latency for the common single-caller case and prevents the race entirely for the parallel-setup pattern sh1pt is explicitly designed for. 2. Unique tmp filename: the previous code used a fixed `${file}.tmp` path, so two concurrent writeVault() calls in different async tasks could write to the same tmp and stomp each other mid-write. The new pattern appends pid + random suffix so each writer gets its own tmp file and the rename is independent. The vault lock is purely in-process. For multi-process isolation (e.g. two concurrent CLI invocations), a proper flock(2) would be needed; that is left as a follow-up as it requires an additional npm dep or N-API shim.

greptile-apps · 2026-06-06T14:50:53Z

Greptile Summary

This PR addresses a TOCTOU race in local-vault.ts where concurrent setSecretInLocal / deleteSecretFromLocal calls (e.g. Promise.all over adapter setups) would snapshot the same vault, write independently, and silently drop one writer's secrets.

In-process mutex: A promise-chain lock (withVaultLock) is added at module scope. Every read-modify-write goes through the lock so the next caller's read can't begin until the previous caller's rename is complete. The serialization logic is correct for Node.js's single-threaded event loop.
Unique tmp filename: writeVault now appends <pid>.<random> to the tmp path so two concurrent writers (before the lock catches them, or from separate processes) never share a tmp file and stomp mid-write.

Confidence Score: 4/5

Safe to merge; the lock correctly serializes all mutating vault operations in the Node.js single-threaded model, and the unique tmp path eliminates the concurrent-writer stomp.

The promise-chain mutex logic is sound — callers queue through _vaultLock and each fn waits for the full async read-modify-write of the previous caller before starting. The unique tmp naming is equally correct. Two small quality gaps remain: fn is passed directly as an onRejected handler (works but is idiomatic-JS surprising), and orphaned tmp files from failed writes now accumulate indefinitely rather than being overwritten by the next attempt.

Only packages/cli/src/local-vault.ts changed; the orphaned-tmp cleanup path in writeVault is worth a second look before shipping.

Important Files Changed

Filename	Overview
packages/cli/src/local-vault.ts	Adds a promise-chain mutex (`withVaultLock`) to serialize read-modify-write operations in `setSecretInLocal` and `deleteSecretFromLocal`, and switches the tmp-file path to a unique per-call name with pid+random suffix. The lock logic is correct for the single-process case; two minor style/cleanup concerns noted.

Sequence Diagram

sequenceDiagram
    participant A as Caller A (adapter 1)
    participant B as Caller B (adapter 2)
    participant L as withVaultLock (_vaultLock chain)
    participant FS as File System

    Note over A,B: Promise.all([setSecretInLocal(k1,v1), setSecretInLocal(k2,v2)])

    A->>L: enqueue fn_A (lock acquired, _vaultLock → tail_A)
    B->>L: enqueue fn_B (waits on tail_A, _vaultLock → tail_B)

    L->>A: fn_A starts
    A->>FS: "readVault() → {secrets:{}}"
    A->>A: "secrets[k1] = v1"
    A->>FS: writeFile(secrets.json.pid.r1.tmp)
    A->>FS: rename(tmp → secrets.json)
    A-->>L: fn_A resolves → tail_A settles

    L->>B: fn_B starts
    B->>FS: "readVault() → {secrets:{k1:v1}}"
    B->>B: "secrets[k2] = v2"
    B->>FS: writeFile(secrets.json.pid.r2.tmp)
    B->>FS: rename(tmp → secrets.json)
    B-->>L: fn_B resolves → tail_B settles

    Note over FS: Final vault contains both k1 and k2

_{Reviews (1): Last reviewed commit: "fix(cli): serialize local-vault writes t..." | Re-trigger Greptile}

greptile-apps · 2026-06-06T14:50:58Z

+// 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);


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!

greptile-apps · 2026-06-06T14:50:59Z

@@ -85,9 +103,14 @@ async function writeVault(v: LocalVault): Promise<void> {
 }


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.

github-actions · 2026-06-06T16:09:51Z