fix(wallet): serialize wallet mutations (#762) by Kewe63 · Pull Request #763 · nansen-ai/nansen-cli

Kewe63 · 2026-10-22T17:03:59Z

Summary

Fixes #762.

Concurrent wallet commands previously protected individual JSON writes but not the complete read-check-write state transition. Two first encrypted-wallet creates could both validate against an empty config, both report success, and leave one wallet inaccessible after the last passwordHash write won.

This PR:

adds a dependency-free inter-process wallet mutation lock using atomically published private directories and owner tokens;
serializes local wallet create, default selection, and delete mutations;
serializes only Privy's local persistence boundary, without holding a filesystem lock during network calls;
bounds lock waiting and reports the live owner PID in actionable busy errors;
recovers locks owned by dead processes through exclusive recovery markers and quarantine paths;
treats disappeared paths, recovery-marker unlink races, and owner-token handoffs as retryable contention;
rejects symlink or unexpected lock/metadata types;
rejects Promise callbacks and same-process nested locking immediately;
preserves primary callback errors when release also fails;
reports a successful mutation followed by release failure as committed, preventing accidental deletion of matching remote Privy wallets;
adds a patch changeset and focused multi-process regressions.

No live API, RPC, production credential, funded wallet, or blockchain operation is used by the tests.

Testing

RED against the old production behavior:

npx vitest run src/__tests__/wallet-concurrency.test.js --reporter=verbose
Test Files  1 failed (1)
Tests       2 failed | 1 passed (3)

The failures showed that different-password first creates and duplicate-name creates both incorrectly reported two successes.

Final focused and impacted suite:

npx vitest run \
  src/__tests__/wallet-concurrency.test.js \
  src/__tests__/wallet.test.js \
  src/__tests__/wallet-export-guard.test.js \
  src/__tests__/wallet-signing.test.js \
  src/__tests__/privy.test.js \
  --reporter=dot

Test Files  5 passed (5)
Tests       163 passed (163)

Stale-recovery contention stress:

100/100 rounds passed
12 simultaneous contenders per round

npm run lint
eslint .

git diff --check
# clean

Full local suite before the final PID-identity follow-up:

npm test
Test Files  116 passed | 1 failed (117)
Tests       4999 passed | 16 skipped | 2 failed (5017)

The only failures were pre-existing feedback fixture timeouts reproduced identically on pristine base commit 19b7cfcb562bac2beb85775833fb8b87edfd0037 and addressed separately. All 116 other test files passed. The final lock-remediation follow-up passed the complete 163-test impacted suite; GitHub CI validates the final head.

Compatibility and risk

No CLI commands, options, schemas, or output envelopes change.
Wallet mutations may now wait up to 15 seconds for another local wallet command before returning an actionable busy error.
On Linux, lock ownership includes the kernel process start time and boot ID so a reused PID is recovered safely. Platforms without a dependency-free process-start identity fall back conservatively to a bounded busy error.
If a recovery process dies after publishing its exclusive claim but before quarantine, later mutations fail closed with an actionable interrupted-recovery error; after stopping wallet commands, the stale lock directory must be removed manually rather than risking deletion of a live successor claim.
A process killed after quarantining a stale lock may leave a uniquely named stale directory; it cannot block later mutations or delete a successor lock.
Native Windows behavior is covered by the repository's CI matrix; local development verification was performed under WSL/Linux.

Checklist

Focused and impacted tests pass
npm run lint passes
New concurrency, stale recovery, Privy commit, release failure, and reentrancy paths have tests
No console.log or hardcoded secrets
Error messages are actionable
Patch changeset added
src/schema.json unchanged — no command or option changes
README.md unchanged — no top-level command or category changes

Protect wallet create, default, delete, and Privy local persistence with a bounded inter-process lock so concurrent commands cannot bypass password and default-wallet invariants.\n\nFixes nansen-ai#762

aikido-pr-checks · 2026-10-22T17:04:54Z

+}
+
+function writePrivateJson(filePath, value) {
+  const fd = fs.openSync(filePath, 'wx', 0o600);


Potential file inclusion attack via reading file - high severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.

Show fix

Remediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend!

_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

@AikidoSec ignore: writePrivateJson only receives internal lock metadata paths constructed from the fixed ~/.nansen/wallets/.wallet-mutation.lock directory plus process IDs and cryptographically random tokens. No user-controlled path reaches this helper; creation is exclusive (wx) inside the private wallet directory.

✅ Based on your feedback, we ignored this issue because of the following reason:

writePrivateJson only receives internal lock metadata paths constructed from the fixed ~/.nansen/wallets/.wallet-mutation.lock directory plus process IDs and cryptographically random tokens. No user-controlled path reaches this helper; creation is exclusive (wx) inside the private wallet directory.

aikido-pr-checks · 2026-10-22T17:04:55Z

+    throw new Error(`Wallet mutation lock is invalid: ${label} must be a regular file. Stop other wallet commands, remove ${path.dirname(filePath)}, and retry.`);
+  }
+  try {
+    return JSON.parse(fs.readFileSync(filePath, 'utf8'));


Potential file inclusion attack via reading file - medium severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.

Show fix

Remediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend!

_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

@AikidoSec ignore: readLockJson is only called for fixed owner.json / recovery.json files under the internally constructed wallet mutation lock directory. The parent path is not user input, and the function rejects symlinks and non-regular files before reading.

✅ Based on your feedback, we ignored this issue because of the following reason:

readLockJson is only called for fixed owner.json / recovery.json files under the internally constructed wallet mutation lock directory. The parent path is not user input, and the function rejects symlinks and non-regular files before reading.

aikido-pr-checks · 2026-10-22T17:04:56Z

+  if (stat.isSymbolicLink() || !stat.isDirectory()) {
+    throw new Error(`Wallet mutation lock is invalid: ${lockPath} must be a real directory. Stop other wallet commands, remove it, and retry.`);
+  }
+  return validateLockRecord(readLockJson(path.join(lockPath, 'owner.json'), 'owner metadata'), 'owner metadata');


Potential file inclusion attack via reading file - medium severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.

Show fix

Remediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend!

_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

@AikidoSec ignore: this path is path.join(lockPath, "owner.json"), where lockPath is the fixed .wallet-mutation.lock entry inside the private wallet directory. inspectWalletMutationLock first rejects symlinks and non-directories, and readLockJson rejects symlinks/non-files.

✅ Based on your feedback, we ignored this issue because of the following reason:

this path is path.join(lockPath, "owner.json"), where lockPath is the fixed .wallet-mutation.lock entry inside the private wallet directory. inspectWalletMutationLock first rejects symlinks and non-directories, and readLockJson rejects symlinks/non-files.

aikido-pr-checks · 2026-10-22T17:04:58Z

+function recoverDeadWalletMutationLock(lockPath, owner) {
+  const recoveryPath = path.join(lockPath, 'recovery.json');
+  const recoveryToken = crypto.randomBytes(16).toString('hex');
+  const recoveryCandidatePath = path.join(lockPath, `.recovery-${process.pid}-${recoveryToken}.candidate`);


Potential file inclusion attack via reading file - medium severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.

Show fix

Remediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend!

_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

@AikidoSec ignore: the recovery candidate filename is generated internally from the fixed wallet lock directory, validated numeric process.pid, and a 128-bit random hex token. It contains no request, CLI, wallet-name, or other user-controlled path segment.

✅ Based on your feedback, we ignored this issue because of the following reason:

the recovery candidate filename is generated internally from the fixed wallet lock directory, validated numeric process.pid, and a 128-bit random hex token. It contains no request, CLI, wallet-name, or other user-controlled path segment.

nansen-pr-reviewer · 2026-10-22T17:05:02Z

nansen-pr-reviewer summary

The change adds cross-process serialization and substantially improves stale-lock recovery, including Linux PID-start identity checks and Privy persistence cleanup. However, non-Linux platforms still fall back to PID-only liveness, and lock records without identity remain accepted there, so PID reuse can repeatedly strand wallet mutations.

Findings by Severity

Severity	Count
🟡 Medium	5

Deterministic check: neutral — 3 earlier finding(s) still open (below the blocking bar)

Risk: 4/5 (High) — raised by: complexity: 1432 added lines

Token usage: 41,399 input, 1,910 output, 19,430 cache read | Usage Guide

Cooldown: for the next 10 minutes (counting from when this review finished), new pushes to this PR will not trigger another review — the next push after the window expires will. Need a fresh review sooner? Comment @nansen-pr-reviewer[bot] re-review.

Record Linux process start identity in lock metadata so stale locks remain recoverable after PID reuse, with conservative fallback on other platforms.

nansen-pr-reviewer · 2026-10-22T17:22:30Z

+    if (err.code !== 'EPERM') throw err;
+  }
+  if (!owner.identity) return true;
+  const currentIdentity = linuxProcessIdentity(pid);


🟡 - Medium | PID reuse remains unsafe on non-Linux platforms

The lock owner identity is only populated by linuxProcessIdentity; on macOS and Windows it is always null, and processIsAlive therefore treats any process still occupying the recorded PID as the original owner. If the original owner crashes and the PID is reused, stale locks will be considered live until the 15-second timeout on every mutation, recreating the previously reported stranded-lock behavior for those platforms. Use a platform-appropriate process-start identity or lease, or otherwise avoid accepting PID-only liveness on platforms without identity support.

nansen-pr-reviewer · 2026-10-22T17:33:33Z

+  return currentIdentity !== null && currentIdentity === owner.identity;
+}
+
+function readLockJson(filePath, label) {


🟡 - Medium | medium: PID-only fallback still permits PID reuse for locks without identity

The lock format accepts an absent identity, and processIsAlive treats such records as live whenever the PID exists. This leaves legacy locks or lock records created when identity collection was unavailable vulnerable to PID reuse: a new process occupying the old PID will prevent recovery until the timeout, and repeated wallet mutations can remain blocked. Lock records without a verifiable process-start identity should be treated as stale or handled with an explicit compatibility/lease strategy rather than accepted as live.

nansen-pr-reviewer · 2026-10-22T17:33:33Z

+function sleepSync(ms) {
+  Atomics.wait(LOCK_SLEEP_CELL, 0, 0, ms);
+}
+


🟡 - Medium | medium: PID reuse remains unsafe on non-Linux platforms

linuxProcessIdentity explicitly returns null on macOS and Windows, while processIsAlive accepts any process still occupying the recorded PID as the original owner. If a wallet command crashes and the PID is later reused, the stale lock remains considered live and each mutation waits until the 15-second timeout. A platform-appropriate process-start identity or a lease mechanism is needed before treating the owner as live on these platforms.

Preserve exclusive recovery claims across owner handoffs, bound filesystem retries, distinguish lock directory generations, and retain remote Privy wallets when local rollback is incomplete.

nansen-pr-reviewer · 2026-10-22T18:53:01Z

+  return currentIdentity !== null && currentIdentity === owner.identity;
+}
+
+function readLockJson(filePath, label) {


🟡 - Medium | PID-only lock liveness remains unsafe on non-Linux platforms

On macOS and Windows, linuxProcessIdentity always returns null, while processIsAlive treats a PID that currently exists as the original owner when owner.identity is absent. If the original process crashes and its PID is later reused, the stale lock will be considered live until the wait timeout, and subsequent wallet mutations can repeat that delay. Use a platform-appropriate process-start identity or a lease, and do not accept PID-only liveness on platforms without verifiable identity.

nansen-pr-reviewer · 2026-10-22T18:53:01Z

+    throw new Error('Wallet mutation lock cannot establish the current Linux process identity; refusing unsafe PID-only locking.');
+  }
+  return identity;
+}


🟡 - Medium | Lock records without identity are still accepted as live where identity is unavailable

validateLockRecord explicitly permits identity to be absent on non-Linux platforms, and processIsAlive then accepts any process occupying the recorded PID. This leaves legacy or identity-less lock records vulnerable to PID reuse and preserves the stranded-lock behavior. Such records should be rejected or recovered through an explicit compatibility/lease mechanism rather than treated as live.

aikido-pr-checks · 2026-10-22T18:53:28Z

+    throw new Error(`Wallet mutation lock is invalid: ${lockPath} must be a real directory. Stop other wallet commands, remove it, and retry.`);
+  }
+  try {
+    const owner = validateLockRecord(readLockJson(path.join(lockPath, 'owner.json'), 'owner metadata'), 'owner metadata');


Potential file inclusion attack via reading file - medium severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.

Show fix

Remediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend!

_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

aikido-pr-checks · 2026-10-22T18:53:29Z

+  if (owner.token !== token) {
+    throw new Error('Wallet mutation lock ownership changed before release; refusing to remove another process lock.');
+  }


A timing attack might allow hackers to bruteforce passwords - medium severity
An insecure way to compare passwords to user input might allow hackers to bruteforce passwords.

Show fix

Remediation: Use crypto.timingSafeEqual, a timing-safe string comparison function built into NodeJS

_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

Ahmett101 · 2026-10-22T22:19:57Z

Cross-platform lock-identity follow-up is ready: Kewe63#1. It targets this PR’s head branch and addresses the remaining non-Linux PID-reuse and identity-less-record findings. Direct push to Kewe63:fix/wallet-mutation-lock was denied for the active GitHub account, so merging the follow-up PR will update this PR.

fix(wallet): serialize wallet mutations

01f45b0

Protect wallet create, default, delete, and Privy local persistence with a bounded inter-process lock so concurrent commands cannot bypass password and default-wallet invariants.\n\nFixes nansen-ai#762

aikido-pr-checks Bot reviewed Oct 22, 2026

View reviewed changes

nansen-pr-reviewer Bot reviewed Oct 22, 2026

View reviewed changes

Comment thread src/wallet.js Outdated

fix(wallet): distinguish reused lock-owner PIDs

9e02c13

Record Linux process start identity in lock metadata so stale locks remain recoverable after PID reuse, with conservative fallback on other platforms.

nansen-pr-reviewer Bot reviewed Oct 22, 2026

View reviewed changes

ci: retry native auth cleanup

8f93afd

nansen-pr-reviewer Bot reviewed Oct 22, 2026

View reviewed changes

fix(wallet): harden stale lock recovery

eed52e6

Preserve exclusive recovery claims across owner handoffs, bound filesystem retries, distinguish lock directory generations, and retain remote Privy wallets when local rollback is incomplete.

nansen-pr-reviewer Bot reviewed Oct 22, 2026

View reviewed changes

aikido-pr-checks Bot reviewed Oct 22, 2026

View reviewed changes

Kewe63 mentioned this pull request Oct 22, 2026

Concurrent first encrypted wallet creation can strand one wallet #762

Open

7 tasks done

Ahmett101 mentioned this pull request Oct 22, 2026

fix(wallet): verify lock-owner identity cross-platform Kewe63/nansen-cli#1

Open

Conversation

Kewe63 commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Compatibility and risk

Checklist

Uh oh!

aikido-pr-checks Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

Kewe63 Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

Kewe63 Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

Kewe63 Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

Kewe63 Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nansen-pr-reviewer Bot commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

nansen-pr-reviewer summary

Findings by Severity

Uh oh!

nansen-pr-reviewer Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

nansen-pr-reviewer Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

nansen-pr-reviewer Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

nansen-pr-reviewer Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

nansen-pr-reviewer Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

Ahmett101 commented Oct 22, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Kewe63 commented Oct 22, 2026 •

edited

Loading

nansen-pr-reviewer Bot commented Oct 22, 2026 •

edited

Loading