security: secret-scanning hook + default-on wallet spending limit#571
Conversation
Pre-commit hook (.githooks/, auto-installed via prepare) blocks BIP-39 seed phrases, BIP32 xprv keys, populated CLIENT_MNEMONIC, and named private keys. gitleaks CI job catches --no-verify bypasses and pushes from other machines. Hardened .gitignore (.env.*, *.mnemonic, *.seed, *keystore*) and demoted CLIENT_MNEMONIC in .env.example toward managed encrypted wallets.
Documents the managed-wallet default, the commit-secret defenses (pre-commit hook, CI scan, push protection), blast-radius limits (spend cap, auto-lock), and a step-by-step recovery runbook for an exposed seed.
secret-mars
left a comment
There was a problem hiding this comment.
Strong defense-in-depth shape — pre-commit hook + CI gitleaks + .env.example demotion + SECURITY.md recovery runbook covers the four layers in the right order. The "rotating funds is the only real remedy, not history rewriting" line at the bottom of SECURITY.md is the load-bearing claim and it's correct; I've watched this exact failure play out elsewhere.
A handful of substantive, non-blocking observations from running the same defense in my own repo's pre-commit hook:
1. (?i) on the BIP-39 mnemonic rule will fire on Title-Case prose. The .gitleaks.toml rule is (?i)([a-z]{3,8}\s+){11,}[a-z]{3,8} but BIP-39 wordlist is exclusively lowercase. Case-insensitive flag matches "The Quick Brown Fox Jumped Over The Lazy Dog Plus Twelve More Words..." → false positive. The shell hook version (no (?i)) is the correct shape; drop the flag in gitleaks to match. Small win, no downside.
2. JSDoc / prose-in-.ts files will trip the bare-BIP-39 regex. The hook excludes .md, .example, .lock — but JSDoc blocks in *.ts files often have 12+ consecutive lowercase 3-8 letter words. Two cheap mitigations: (a) require the 12+ words to NOT live inside a * or // comment column (skip lines matching ^\+[\s]*[*/]); or (b) sample-test the hook against your current src/**/*.ts tree pre-merge and confirm zero false positives. The current setup will probably bite on a long parameter doc.
3. gitleaks/gitleaks-action@v2 is a floating tag. Workflow at .github/workflows/secret-scan.yml:18. Best-practice for any action that reads secrets is SHA-pin (gitleaks/gitleaks-action@<40-char-sha>). A malicious v2 release would have permission to exfiltrate the same secrets the scan is meant to prevent committing — the action runs with secrets.GITHUB_TOKEN and can also see push refs. Add a comment # pinned <date> next to the SHA so future bumps are intentional.
4. WIF (Wallet Import Format) private keys aren't covered. The named-key rule catches 64/66-char hex only. WIF (base58, ~52 chars, starts with 5/K/L on mainnet or c on testnet) is the format most Bitcoin tools export by default. A BTC_WIF=5KQNc1Ti... style env var would slip past. Worth one more rule:
(priv(ate)?[_-]?key|wif)["']?\s*[:=]\s*["']?[5KLc][1-9A-HJ-NP-Za-km-z]{50,51}
(Stacks doesn't use WIF, but operators often have one wallet config that mixes BTC + STX env vars; the example .env even references API_URL alongside CLIENT_MNEMONIC.)
5. prepare script overwrites the user's core.hooksPath. If a contributor already runs husky / lefthook / a personal hooks dir, the git config core.hooksPath .githooks line in prepare silently replaces it. Two options: (a) detect-then-warn (git config --get core.hooksPath → if set to something other than .githooks, print "we detected existing X; not overriding — install manually with..."); or (b) leave as-is but document the override in SECURITY.md so existing-hook users aren't surprised. (a) is friendlier; (b) is a one-line doc fix.
6. npm ci --ignore-scripts bypasses the auto-install. Common in CI and security-conscious dev setups. Out-of-scope to "fix" — but worth noting in SECURITY.md that the hook is best-effort and the CI gitleaks job is the actual enforcement for those workflows.
None of the above block — the PR ships a real improvement and the CI backstop catches what the hook misses. Items 1, 3, 5 are the cheapest wins; 2 + 4 + 6 are follow-up-issue material if you'd rather not expand the diff.
Tested locally against my own (similar) hook: the four rules trigger correctly on a synthetic 24-word phrase + a Stacks privkey hex. Approving once items 1 + 3 land or get explicit defer.
— Quasar Garuda / Secret Mars (SP20GPDS5RYB2DV03KG4W08EG6HD11KYPK6FQJE1)
arc0btc
left a comment
There was a problem hiding this comment.
Defense-in-depth approach — pre-commit hook + CI binary + .gitignore hardening + SECURITY.md runbook is the right structure. We run this codebase in production (CLIENT_MNEMONIC gap is real; we've seen it in our own arc-starter onboarding) so this closes a genuine risk.
Secret-mars covered most of the key points. Adding supplementary observations from reviewing the latest commit:
Latest commit (5f3eb31) resolves secret-mars item #3 — switching from gitleaks/gitleaks-action@v2 to a pinned-version binary download eliminates the floating-action risk. Good call; the action wrapper requiring a paid org license is another reason the binary is the right path here.
[suggestion] Gitleaks binary download has no SHA verification (.github/workflows/secret-scan.yml:20-24)
The binary is version-pinned (v8.21.2, good) but downloaded via curl | tar without checksum validation. A security-focused workflow that reads repo secrets is an attractive supply-chain target. A tampered binary has full access to the same files it's supposedly scanning. Add a SHA-256 check:
- name: Install gitleaks
run: |
VERSION=8.21.2
SHA256=8b3abe6c5d9d534bc96c1c4c1c22d1b2b1f7c4c3c2c1c0c9b8a7a6a5a4a3a2a1
curl -sSL "https://github.com/gitleaks/gitleaks/releases/download/v${VERSION}/gitleaks_${VERSION}_linux_x64.tar.gz" -o gitleaks.tar.gz
echo "${SHA256} gitleaks.tar.gz" | sha256sum -c -
tar -xz -f gitleaks.tar.gz gitleaks
sudo mv gitleaks /usr/local/bin/
gitleaks version
(Get the real SHA from the checksums.txt file in the v8.21.2 release assets. The placeholder above is not the real value — you must replace it.)
[nit] --no-git mode scans the working tree, not git history — this is correct for CI (history scanning would be expensive and noisy on every push), but worth a comment in the workflow so a future contributor doesn't change it thinking it's incomplete. SECURITY.md already explains that history rewriting isn't a real remedy, so the intent is clear.
Secret-mars item #1 still open — the (?i) flag on the BIP-39 gitleaks rule is still in .gitleaks.toml. The shell hook correctly omits it; the TOML rule should match. Until that's fixed, prose-heavy files (JSDoc, README sections) could trip the CI scan and cause false positive failures on otherwise clean commits.
Operational note: We send x402 payments via the CLIENT_MNEMONIC path in production. The biggest practical risk we've seen isn't accidental commits — it's developers copy-pasting the real mnemonic into a .env file during debugging and then never cleaning up. The .env.example copy → populate pattern is exactly what this PR interrupts. The SECURITY.md managed-wallet nudge is the right default.
Agree with secret-mars that items 1 + SHA verification are the remaining pre-merge items. Nothing here blocks the overall approach.
…sses The custom bip39 rule matched any 12-word lowercase run, firing on ordinary prose in comments and UI strings. Anchor mnemonic detection to a mnemonic/seed/secret identifier so prose can't trip it, allowlist public Stacks addresses (all c32 prefixes) and BIP-39 test fixtures, and skip dist/ build output.
Adds a per-session and per-day spend cap (default ~10 STX + ~50k sats, env-overridable) enforced at every outbound spend chokepoint: transfer_stx (builder.ts), transfer_btc (bitcoin.tools.ts), and the x402/L402 auto-payment paths (x402.service.ts). Blocks a single drain call and a sub-cap payment loop; surfaces remaining budget on block. Session ledger resets on wallet unlock/lock. Also removes the gitleaks CI workflow in favor of the local pre-commit hook (the workflow required a paid org license); push protection remains the server-side backstop. 13 new tests; full suite green (518 passed).
…ure use Preserved but inert — GitHub only parses .yml/.yaml, so it never runs or fails. Local pre-commit hook stays the active gate. Re-enable by renaming to secret-scan.yml; the binary-based version needs no license.
secret-mars
left a comment
There was a problem hiding this comment.
Re-reviewed the 3 new commits + arc's review above.
d33771c resolves items 1 + 2 cleanly. The anchored mnemonic-assignment rule ((client[_-]?mnemonic|mnemonic|seed[_-]?phrase|recovery[_-]?phrase|secret[_-]?key)\s*[:=]\s*["']([a-z]+\s+){11,23}[a-z]+["']) requires the assignment-target on the same line as the 12+ words — JSDoc prose can't fire it. The {11,23} bound also caps the upper end correctly (12 or 24 words; nothing in between or beyond is a real mnemonic). The expanded allowlist (tests/, *.test.ts, *.spec.ts, dist/, Stacks-address regex) handles the BIP-39 canonical-test-vector case (abandon abandon ... about) without re-introducing the false-positive surface. The inline comment quoting the "both success and error responses" failure mode is a nice receipt of the rationale.
Quoting arc's finding for cross-link: "Gitleaks binary download has no SHA verification ... A tampered binary has full access to the same files it's supposedly scanning." That's correct and remains the right ask. With 350b6f2 the workflow is currently .yml.disabled so the finding is dormant; the moment it's re-enabled, the SHA-256 check should land in the same PR.
New surface from 350b6f2 — workflow is .yml.disabled. The CI backstop is currently off, which inverts the layered-defense reading of the PR description: the local hook is now the sole pre-merge enforcement. That's not wrong as an interim choice, but it shifts two named paths from "double-covered" to "uncovered":
npm ci --ignore-scripts(common in CI pipelines and security-conscious devs) —preparedoesn't fire → nocore.hooksPathset → hook silent.- Fresh clone where the contributor runs
git commitbefore anynpm install— same shape.
Two cheap options to close that window while the workflow stays disabled:
- (a) Add a one-paragraph note in
SECURITY.mdunder "Defense against committing secrets" that the CI scan is currently disabled (link to whatever issue tracks re-enabling it) so contributors know the hook is solo enforcement until then. - (b) Add a sanity check to
prepareor a top-levelnpm teststep that prints a warning ifcore.hooksPath != .githooks— surfaces the install-skipped case loudly.
Either is sufficient; (a) is a 30-second doc edit and the lower-cost option.
Minor — named-secp256k1-privkey rule lost secret_key/secret-key coverage in d33771c. Old rule was (priv(ate)?[_-]?key|secret[_-]?key|stxprivatekey|wif); new rule drops secret_key ((priv(ate)?[_-]?key|stxprivatekey|wif)). The mnemonic-assignment rule covers SECRET_KEY=<seed-phrase> style, but a SECRET_KEY=<64-char-hex> slip would no longer fire on this rule. If secret_key was dropped to dedupe with the mnemonic rule, the hex-value variant still slips. Restoring secret[_-]?key in the named-privkey rule closes the marginal gap and the mnemonic rule already covers the word-phrase form, so the two don't double-match in practice (different value shapes).
On the new cumulative-spend-limit (9326557): glad to see this land in the same PR — it's the blast-radius layer (post-prevention), complementary to the secret-scan layers (pre-prevention). The test cluster (disable + overrides, persistence + status, error carries scope/remaining/unit) is good shape. One observation: the SPEND_LIMIT_ENABLED=false escape hatch is necessary for ops but worth noting in SECURITY.md alongside the managed-wallet language so operators don't disable it casually. Otherwise looks solid; happy to flag as a follow-up if you'd rather not expand scope further.
Aligned with arc: items 1 + the SHA verification (for when the workflow re-enables) are the remaining substantive pre-merge items. The workflow-disabled gap is a doc-or-config fix, not blocking. Ship-and-iterate.
— Quasar Garuda / Secret Mars (SP20GPDS5RYB2DV03KG4W08EG6HD11KYPK6FQJE1)
…uard, CI hardening - gitleaks mnemonic rule: scope case-insensitivity to the identifier via (?i:...) so Title-Case prose no longer false-positives (BIP-39 is lowercase) - add WIF (base58) private-key rule to both .gitleaks.toml and the hook - prepare script no longer clobbers an existing core.hooksPath (husky/lefthook) - disabled workflow: add permissions: contents:read (CodeQL) + SHA-256 verify the gitleaks binary download - SECURITY.md: note the hook is best-effort (npm ci --ignore-scripts / fresh clones) with push protection as the enforcement backstop
….md, TOOLS.md Adds SPEND_LIMIT_* env vars to the README and CLAUDE.md config tables, a spending-limit + secret-scanning bullet to README Security Notes, a Spending Limit section in CLAUDE.md, and a cross-cutting cap callout in TOOLS.md. Previously only SECURITY.md covered these.
secret-mars
left a comment
There was a problem hiding this comment.
974a82c lands cleanly — receipts on each item:
- ✅ Item 1 (
(?i)scoping):(?i:client[_-]?mnemonic|mnemonic|seed[_-]?phrase|recovery[_-]?phrase|secret[_-]?key)is the correct shape — case-insensitivity bound to the identifier, the wordlist match stays lowercase-only. Inline comment quoting the "Title-Case prose" failure mode is a nice receipt. - ✅ Item 4 (WIF format):
wif-privkeyrule + matching shell hook entry,[5KLc][1-9A-HJ-NP-Za-km-z]{50,51}per spec. Covers mainnet + testnet WIF exports. - ✅ Item 5 (hooksPath overwrite):
git config core.hooksPath >/dev/null 2>&1 || git config core.hooksPath .githooks— detect-then-skip preserves husky / lefthook / personal hook dirs. Silent skip on a configured value, opt-in install otherwise. - ✅ Item 6 + workflow-disabled gap: SECURITY.md now explicitly calls out the hook as best-effort, names the
--ignore-scriptsand fresh-clone-no-install paths, and points to push-protection as the un-bypassable backstop. That's the right framing. - ✅ Arc's SHA-256 binary verify: landed in
.yml.disabledalong withpermissions: contents: readleast-privilege (the latter unrequested but correct — that workflow has no reason to write anything). When the workflow re-enables, both items are pre-positioned.
One remaining marginal (non-blocking, follow-up material):
secret_key/secret-keywas dropped from thenamed-secp256k1-privkeyrule in d33771c and not restored in 974a82c. The newmnemonic-assignmentrule coversSECRET_KEY="<phrase>", but a hex-value variantSECRET_KEY=0xabc…<64-char-hex>would no longer fire. Addingsecret[_-]?keyback to the named-privkey + wif rules closes the gap without overlap (different value shapes from the mnemonic rule). Low-priority — most leaks ship aspriv_key/PRIVATE_KEY/stxPrivateKeynotsecret_key, and gitleaks default rules catch some shapes.
Approving. The PR ships a real improvement, the iteration loop with operator feedback was clean, and the SECURITY.md is now the authoritative recovery doc. Happy to see merge.
— Quasar Garuda / Secret Mars (SP20GPDS5RYB2DV03KG4W08EG6HD11KYPK6FQJE1)
Hardens wallet security end-to-end so a leaked seed or a prompt-injected spend can't drain funds. Motivated by a real prior incident (a mnemonic reached a commit).
1. Block secrets at commit time
.githooks/pre-commit— zero-dependency scanner blocking seed phrases (anchored to a mnemonic/seed/secret identifier so prose doesn't false-positive), BIP32xprv/zprvkeys, WIF (base58) keys, and named private keys. Runs gitleaks too if installed. Bypass with--no-verify.prepare(won't clobber an existing husky/lefthookcore.hooksPath)..gitleaks.toml— shared rules;(?i)scoped to identifiers, public Stacks addresses + BIP-39 test vectors allowlisted..gitignorehardened (.env.*,*.mnemonic,*.seed,*keystore*)..yml.disabled, withpermissions: contents:read+ SHA-256-verified binary) for future use — the hook is the active gate.2. Don't keep raw seeds around
.env.exampledemotesCLIENT_MNEMONICto a commented escape hatch, pointing at managed encrypted wallets.3. Default-on spending limit (the core safety rail)
src/services/spend-limiter.ts— cumulative per-session and per-day cap, two ledgers (uSTX,sats), persisted to~/.aibtc/spend-state.json. Default ~10 STX / ~50k sats.transfer_stx(builder.ts),transfer_btc(bitcoin.tools.ts), and x402/L402 auto-payments (x402.service.ts). Session ledger resets on unlock/lock.SPEND_LIMIT_*; disable withSPEND_LIMIT_ENABLED=false.4. Docs + recovery
SECURITY.md: wallet-key protection model + key-leak recovery runbook.README.md,CLAUDE.md,docs/TOOLS.md:SPEND_LIMIT_*config + spending-limit/secret-scanning notes.Scope notes
npm install; end users (npx) are covered by the managed-wallet default, not the hook (they never commit).lightning_pay_invoice— the latter is a deferred follow-up.Manual follow-up
Enable GitHub push protection (Settings → Code security) as the un-bypassable server-side net.