fix(cli): close wallet-security gaps, bump CLI to 0.5.0 - #60
Merged
Merged
Conversation
Three defaults could destroy or over-expose a user's key. All three are now explicit choices, which makes this a breaking release. Atomic config writes. saveConfig wrote in place, so a crash, SIGINT, or ENOSPC could truncate config.json. Because `wallet create` discards the BIP-39 mnemonic and declares that file the only copy of the key, a torn write there permanently destroyed funds. It now writes a temp file and renames it, matching the RFQ key storage. Side effect, intentional: a symlink at the config path is replaced rather than written through, closing the write-side gap left by loadConfig's O_NOFOLLOW read. No more silent unlimited approvals. `wallet approve --amount` defaulted to max, so a bare invocation with --yes granted MaxUint256 with no interactive gate. It is now required. `rfq request --ensure-allowance` approves exactly the escrowed reservePrice on a BUY, matching book fill and position close. SHORT RFQs deliberately keep the max default. The settle-time collateral draw is a structure-dependent max-loss figure that is not carried on the request -- params.collateralAmount is hardcoded to 0 by every SDK builder and the send path rejects any nonzero value. Under-approving a SHORT reverts at settlement after a maker has committed, which is worse than a broad allowance. --yes is no longer consent to destroy a key. `wallet create` and `wallet import` exit 2 when the config already holds a key, instead of overwriting it. The old key has no other copy and the replacement's mnemonic is not persisted, so a habitual --yes in a script silently burned funds. Use --force to overwrite deliberately. This matches the existing rule that --reveal-key refuses --yes. Adds tests/approvalDefaults.test.ts. No existing test touched these paths, and an earlier attempt at the SHORT default -- using params.collateralAmount, which is always 0 -- turned --ensure-allowance into a silent no-op and passed the whole suite. The regression test asserts a zero target is the bug. Known gaps, documented in the CHANGELOG: the private key is still stored unencrypted under 0600 perms, and `config set/unset privateKey` still overwrite or delete it with no confirmation. BREAKING CHANGE: `wallet approve` requires --amount; `wallet create/import` reject --yes over an existing key; BUY `rfq request --ensure-allowance` no longer leaves an unlimited allowance behind. Migration steps in the CHANGELOG.
Two follow-ups to the 0.5.0 wallet-security work, both consequences of it. Exit codes. Making `wallet approve --amount` required surfaced a contract violation: Commander exits 1 for every parse failure, but the README documents 1 as a generic runtime error (network, RPC, contract revert) and 2 as a usage error. Automation could not tell a mistyped flag from a reverted transaction. Commander's usage-error codes are now mapped to 2 across the whole command tree -- exitOverride is per-Command and is not inherited, so the walk has to be recursive to reach grandchildren like `wallet approve`. The mapping also covers a group command invoked with no subcommand (`thetanuts wallet`, or bare `thetanuts`), which Commander reports as commander.help with exitCode 1. That is an incomplete invocation, not a runtime failure. Successful --help is a distinct code (commander.helpDisplayed, exitCode 0) and still exits 0, as does --version. Codes the map does not recognise keep Commander's own exit code rather than being coerced, so a new code in a future Commander release fails visibly instead of masquerading as a usage error. Runtime paths that exit 1/3/4/6 from inside action handlers never reach exitOverride and are unaffected. CI. The workflow installed, built, and tested only the root SDK, so every CLI test could fail without blocking a merge -- including the regression test added last commit to catch a silent no-op in the RFQ allowance path. Adds a `cli` job and makes all-checks depend on it. The job builds the root SDK before touching the CLI. The CLI resolves the SDK through `file:..` and the SDK's package entry points at a gitignored dist/, so a fresh checkout has nothing to compile against; without that step the job fails on unresolved types. Verified by running the full sequence in a clean clone. Adds tests/exitCodes.test.ts. Exit status is a process-level contract, so the assertions spawn the CLI as a subprocess -- an in-process call cannot observe it. Covers both directions of the help/helpDisplayed split, which is the pair most likely to regress together.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Triaged the CLI's wallet handling and fixed the three issues that could destroy or over-expose a user's key. All three were unsafe defaults, so making them explicit is a breaking change — hence 0.5.0 rather than a patch.
Scope is
cli/only. The SDK and mcp-server are untouched.Fixes
Atomic config writes (
cli/src/config.ts)saveConfigwrote in place, so a crash,SIGINT, orENOSPCcould truncateconfig.json.wallet creatediscards the BIP-39 mnemonic and tells the user that file is their only copy of the key — a torn write there permanently destroyed funds. Now writes a temp file andrename(2)s it, the same pattern already used by the RFQ key storage.Intentional side effect: a symlink at the config path is now replaced rather than written through, closing the write-side gap left by
loadConfig'sO_NOFOLLOWread. Users who symlink their config from a dotfiles repo will find the link severed.No more silent unlimited approvals (
cli/src/commands/wallet.ts,cli/src/commands/rfq.ts)wallet approve --amountdefaulted tomax, sowallet approve --token USDC --for optionBook --yesgranted MaxUint256 with no interactive gate — the warning went to stderr and--yesauto-passed the confirm.--amountis now required.rfq request --ensure-allowancenow approves exactly the escrowedreservePriceon a BUY, matchingbook fillandposition close.SHORT RFQs deliberately keep the
maxdefault. The settle-time collateral draw is a structure-dependent max-loss figure not carried on the request —params.collateralAmountis hardcoded to0by every SDK builder and the send path rejects any nonzero value. Under-approving a SHORT reverts at settlement after a maker has committed, which is worse than a broad allowance.--approve-amount <n>remains available to cap it.--yesis not consent to destroy a key (cli/src/commands/wallet.ts)wallet createandwallet importnow exit 2 when the config already holds a key. The old key has no other copy on disk and the replacement's mnemonic is not persisted, so a habitual--yesin a script silently burned funds with only a stderr line to show for it.wallet create --forceoverwrites deliberately. Matches the existing rule that--reveal-keyrefuses--yes.Tests
Adds
cli/tests/approvalDefaults.test.ts— the approval-target defaults andsaveConfigatomicity/permissions/symlink behavior.Worth flagging: an earlier attempt at the SHORT default used
params.collateralAmount, which is always0. That made--ensure-allowancea silent no-op ending in a settlement revert — and it passed all 14 existing tests, because none of them touched these paths. The new test asserts that a zero target is the bug.Breaking changes
wallet approverequires--amount--amount <n>, or--amount maxfor the old behaviorwallet create/import --yesover an existing keywallet create --force, or drop--yesand confirm interactivelyrfq request --ensure-allowanceno longer leaves an unlimited allowance--approve-amount maxif a later tx relied on the leftoverKnown gaps — not addressed here
0600perms. That stops other users on the machine, not another process running as you. An encrypted keystore / OS-keychain option is a separate design decision.config set privateKeyandconfig unset privateKeystill overwrite or delete the key with no confirmation.Both are documented in the CHANGELOG rather than left to be discovered.
Verification
npx tsc --noEmitcleannpm test— 15/15npm run buildclean;dist/index.js --version→0.5.0npm pack --dry-runresolves--yeswith exit 2 and leave the key intact on disk;--forcestill overwrites; atomic write verified for perms (600/700), no temp leftovers, and symlink target untouchedNot run:
prepublishOnly'sswap-sdk-dep.cjs. Worth confirming that resolves to an SDK version compatible with these changes beforenpm publish.