Repository navigation
Conversation
pr-reviewer Summary for #f12cce8📝 3 findings Review completed. Please address the findings below. Findings by Severity
Review effort: 4/5 (Complex) SummaryThis is a well-structured PR that adds meaningful financial safety guards to x402 auto-payments. The atomic write pattern (temp-file + rename), fail-closed ledger semantics, per-outcome finalization at every branch, and the Findings
|
| } | ||
|
|
||
| function readDailySpendMicros(filePath) { | ||
| const data = JSON.parse(fs.readFileSync(filePath, 'utf8')); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@AikidoSec ignore: path is derived solely from $HOME + UTC date; no attacker-controllable input; local CLI.
There was a problem hiding this comment.
✅ Based on your feedback, we ignored this issue because of the following reason:
path is derived solely from $HOME + UTC date; no attacker-controllable input; local CLI.
- new src/x402-ledger.js: daily and session spend caps, JSONL audit log
at ~/.nansen/x402/payments.jsonl; atomic daily ledger writes; fail-closed
on corrupt ledger; URL sanitisation strips query strings before logging
- evaluatePaymentRequirement() now returns network/asset/payTo/amountRaw
on success so providers can pass full metadata to the ledger
- local wallet, Privy, and WalletConnect providers all call
assertCumulativeSpendAllowed() before signing; signed attempts are
recorded with recordPaymentAttempt(); _x402Retry() finalises each
attempt as accepted/rejected/ambiguous and increments the daily ledger
only for accepted and ambiguous outcomes
- WalletConnect handleX402Payment() now returns { signature, paymentId,
network, asset } instead of a bare string so api.js can thread the
network and asset through to the balance-check and audit calls
- classifyX402Rejection() in api.js surfaces clearer stderr diagnostics
for insufficient-balance rejections
- new env vars: NANSEN_X402_DAILY_MAX_AMOUNT (default $10.00),
NANSEN_X402_SESSION_MAX_AMOUNT (optional per-process cap)
- docs: wallet.js help text and schema.json updated with new env vars
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
15c2aaa to
bb4c241
Compare
- Re-raise fail-closed ledger errors in the WalletConnect payment path so a corrupt ledger surfaces as PAYMENT_REQUIRED instead of a generic "auto-payment failed" message (mirrors the Privy and local-wallet paths). - Only advance the in-memory session spend total after the durable daily ledger write succeeds, so a failed write no longer under-counts the daily cap across restarts. - Fail closed in getLedgerDir when no home directory is resolvable, instead of writing the ledger to a relative path and silently disabling the cap. - Throw a typed X402LedgerError from getDailySpendState on a corrupt ledger. - Document that callers must always finalize a recorded payment attempt. - Update walletconnect-x402 tests for the object return shape and add regression tests for each fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Log an [x402] stderr line before re-throwing a fail-closed cap-check error in the WalletConnect path, matching the local-wallet and Privy providers. - Attribute daily spend to the UTC day the payment was recorded (captured at recordPaymentAttempt), not the day finalization happens to land on, so a payment that finalizes just after midnight still counts against the day the cap check authorized it. - Document the cumulative spend caps (NANSEN_X402_DAILY_MAX_AMOUNT, NANSEN_X402_SESSION_MAX_AMOUNT) and the audit log path in AGENTS.md. - Add regression tests for the stderr-on-throw behavior and the cross-midnight spend attribution. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Log the actionable [x402] cap reason to stderr before throwing in the WalletConnect path. For x402-only users (no API key), api.js rewrites the thrown message to a generic "No API key configured" note, so this stderr line is the only explanation the user gets. - Thread the server x-request-id into every payment audit finalization that has a response (accepted, rejected, ambiguous), so ~/.nansen/x402/payments.jsonl records the primary correlation key to server-side logs. The transport- failure branch has no response and correctly leaves it unset. - Add regression tests for both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| while (true) { | ||
| let fd = null; | ||
| try { | ||
| fd = fs.openSync(lockPath, 'wx', 0o600); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@AikidoSec ignore: false positive. This path is local CLI state under the current user HOME/USERPROFILE (~/.nansen/x402), not remote/API-controlled input. If an attacker can control HOME for the process, they already control where this CLI stores local state; no privilege boundary or backend file inclusion is involved.
There was a problem hiding this comment.
✅ Based on your feedback, we ignored this issue because of the following reason:
false positive. This path is local CLI state under the current user HOME/USERPROFILE (~/.nansen/x402), not remote/API-controlled input. If an attacker can control HOME for the process, they already control where this CLI stores local state; no privilege boundary or backend file inclusion is involved.
Unlink the orphaned temp file if the atomic rename fails, and document the Node.js main-thread assumption behind sleepSync's Atomics.wait. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve conflicts in AGENTS.md, src/api.js, and src/x402.js: - AGENTS.md: keep main's 'Account credits' auth bullet (browser login) alongside the x402 bullet's new spend-cap and audit-log documentation. - src/api.js: layer the failClosedX402 guards onto main's selection-aware payment-failure messaging; drop the now-redundant post-catch data.paymentRequirements assignment (main sets it earlier). - src/x402.js: combine main's missing-wallet-key skip with the cumulative spend-cap check and audit-ledger plumbing, and keep main's clearer per-option skip logging while preserving the failClosedX402 rethrow. - x402.test.js: the yielded payment option now carries a paymentId, so the Solana skip tests assert that field. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
decision.usd / svmDecision.usd are always set once evaluatePaymentRequirement accepts a requirement, so recordPaymentAttempt always returns an id and every yielded Privy payment carries a paymentId that is finalized on its outcome. Note the invariant so a future refactor does not silently drop audit records. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nansen-pr-reviewer summaryThe change adds cumulative daily/session x402 caps, reservations, audit logging, and fail-closed ledger handling. The malformed-ledger issue from the earlier review is addressed and covered by shape-validation tests. Two edge cases remain around audit persistence and releasing reservations when local payment recording fails. Findings by Severity
Deterministic check: success — Found 2 finding(s) within acceptable thresholds Risk: 4/5 (High) — raised by: dependency/lockfile change; complexity: 2743 added lines Token usage: 80,553 input, 3,642 output, 122,987 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 |
- Finalize the x402 attempt on 401/403/429/451. This branch threw without finalizing, so the attempt stayed `signed` in the audit log, leaked its pending entry, and never counted against the daily cap -- despite the branch's own reasoning that the payment may still have settled. Collapse the six duplicated finalize blocks onto one helper so a terminal branch cannot quietly skip finalization again. - Latch fail-closed when a daily ledger write fails. The payment is already transmitted and cannot be unsent, but the daily total now under-counts it, so every later cap check in the process refuses with the amount, the ledger path and the recovery step. - Charge spend to the UTC day whose cap authorized it. The increment used the record time, which lands on the next day when signing crosses midnight -- up to 120s of wallet approval on the WalletConnect path. The cap check now returns authorizedAt and all three providers thread it through; the audit record carries both times. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…esses The cap check read the day's total without a lock and reserved nothing; the lock was taken later, during finalization. Two CLI processes could read the same total, both clear the cap, both sign and transmit, and then serialize increments whose sum exceeds the configured limit. Agents fanning out parallel commands hit this on the normal path, not an exotic one. assertCumulativeSpendAllowed now checks and reserves in one locked read-modify-write, counting settled spend plus budget held by in-flight payments. Finalization settles the reservation on accepted/ambiguous and releases it on rejected. Providers release it when they abandon an attempt before signing -- no key for the chain, a missing Permit2 allowance, a signing failure, a wallet approval the user declines -- so a failed option does not park its amount. A reservation from a process that died expires after 15 minutes, comfortably past the 120s WalletConnect approval window. A malformed reservation entry is treated as a corrupt ledger and refuses to sign, consistent with the rest of the file. The reservations key is omitted when empty, so a settled day's file keeps its existing shape and older readers are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Close two cumulative-cap gaps the review flagged: - The per-session cap was checked against settled spend only, which is incremented at finalization. Two concurrent payments in one process could both clear the cap before either settled and together exceed NANSEN_X402_SESSION_MAX_AMOUNT. The cap check now takes an in-memory session reservation under the same id as the daily file reservation, so in-flight budget counts against the cap and is committed or released at finalization. - The local-wallet payment generator swallowed a consumer interruption injected at the yield, leaving the signed payment's audit entry stuck at 'signed' with its reserved budget held until the TTL. It now finalizes the recorded payment as ambiguous and propagates, matching the Privy generator. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Close two more cumulative-cap gaps the review flagged: - The payment generators handled an exception injected at the yield, but not a consumer that cancels with break/return. iterator.return() resumes the yield with a return completion, which skips catch entirely, so the signed payment stayed recorded as 'signed' and held its budget until the TTL in a process that never learned its outcome. createPaymentSignature() takes exactly that path. Both generators now clean up in finally, via a new finalizeIfPending() that settles an id only if the consumer has not already settled it -- a no-op after an ordinary outcome, so no duplicate audit line and no double-counted spend. Applied to the local generator and to both the EVM and SVM blocks of the Privy generator, which had the same structure. - A daily-ledger write that failed after a payment was transmitted set a fail-closed latch in process memory only. That latch dies with the process and the reservation standing in for the uncounted payment expires after 15 minutes, so the next CLI run reclaimed budget that was genuinely spent and could exceed the daily cap. The failure is now also appended to ~/.nansen/x402/accounting-failure.jsonl, and every cap check on the machine fails closed while that file exists. Recovery is explicit: reconcile the daily total against payments.jsonl, then delete it. The marker write is best-effort by necessity -- if the disk itself is gone it fails too, and the warning then says only this process is blocked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A reservation is written into the ledger file of the day its cap check cleared, but releasePaymentReservation located that file from the current time and all six call sites relied on the default. A payment authorized at 23:59:59 and abandoned a second later had its cleanup open the next day's file, so the real reservation held budget until its 15-minute TTL and refused otherwise-valid payments on the day that authorized them. Threading the day through every caller would leave the same trap for the next one, so the id carries it instead: YYYY-MM-DD-<32 hex>. Commit and release derive the file from the id and fall back to the passed time only for an id without a day prefix, so a caller cannot target the wrong day. mutateDailyLedger now takes `day` (which file) and `at` (the wall clock that decides which reservations have expired) separately. They are one instant for a cap check, but not for cleanup running after midnight on the previous day's file, where pruning by that file's own midnight would keep genuinely expired reservations alive. Both tests fail without the fix: one covers the abandoned-across-midnight release, the other the settled case where the id is the only record of the authorizing day. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s $0 The parser treated any JSON value without totalUsdMicros or totalUsd as a zero-spend day, and coerced a non-object `reservations` to "no holds". So a syntactically valid but malformed spend-YYYY-MM-DD.json could hand back a full day's cap, or drop every in-flight hold and let a concurrent process reuse the same headroom -- the opposite of the documented fail-closed contract for a corrupt ledger. writeDailyLedger always emits totalUsdMicros, so a file without a usable total is not one this CLI wrote. The parser now rejects a non-object ledger, a missing or non-numeric total, and a `reservations` value that is present but not a plain object. Absent `reservations` stays valid: writeDailyLedger omits the key once a day has no holds left. totalUsd is also no longer read through Number(), which maps null, '' and booleans onto real numbers -- a `"totalUsd": null` previously read as a legitimate $0. The legacy numeric totalUsd shape is still accepted. Tests cover thirteen rejected shapes and four that must keep working; twelve of the thirteen pass against the old parser. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| walletLabel: walletLabel || 'local wallet', | ||
| authorizedAt: capCheck.authorizedAt, | ||
| reservationId, | ||
| network: decision.network, |
There was a problem hiding this comment.
🟡 - Medium | Local wallet reservations can leak when recording the signed payment throws
The local-wallet path reserves budget before signing, but recordPaymentAttempt is called outside the try/catch that releases the reservation. If recording throws after the payment payload has been built—for example, if an unexpected ledger-path or timestamp error occurs—the outer generator catches the error as an ordinary option failure and proceeds without calling releasePaymentReservation. The reservation then holds daily/session budget until its TTL, causing subsequent valid payments to be refused.
| commitDailySpend(amountUsd, recordedAt, reservationId); | ||
| // Only advance the in-memory session total once the durable daily file | ||
| // has actually been written. If the write failed (ENOSPC, permissions, | ||
| // corrupt ledger), the daily total is unchanged; advancing session spend |
There was a problem hiding this comment.
🟡 - Medium | Audit-log write failures are silently ignored despite the documented audit guarantee
The implementation deliberately suppresses all failures while writing the initial audit record. A permissions error, disk-full condition, or unavailable home directory can therefore allow a signed/transmitted payment to proceed without a payments.jsonl record, contradicting the documented guarantee that every signed attempt is recorded and making reconciliation against the accounting-failure flow incomplete. The spend ledger may still be updated, but the operator has no reliable audit entry for that payment.
Summary
NANSEN_X402_DAILY_MAX_AMOUNT, default $10.00;NANSEN_X402_SESSION_MAX_AMOUNToptional per-process cap)~/.nansen/x402/payments.jsonlfor signed, accepted, rejected, and ambiguous payment attemptsDetails
src/x402-ledger.js: atomic daily ledger writes (temp-file + rename), JSONL audit log, fail-closed on corrupt ledger, URL sanitisation strips query strings before loggingevaluatePaymentRequirement()return extended withnetwork,asset,payTo,amountRawso providers have full metadata without re-deriving ithandleX402Payment()inwalletconnect-x402.jsnow returns{ signature, paymentId, network, asset }instead of a bare string — also fixes a silent gap wherenetworkwas alwaysnullin the WalletConnect retry path_x402Retry()gains apaymentMetaargument and finalises each attempt at every outcome branch before returning or throwingTesting
npm test(72/72 pass)npm run lint