Four points from the CHANGES_REQUESTED review, plus the test-key concern:
1. Cross-platform test command. `npm test` used inline `TEST_MOCK_DB=1
NODE_OPTIONS=...` which fails on Windows (cmd does not expand inline
env vars). The mock swap is actually done by tsconfig.test.json paths,
so TEST_MOCK_DB was vestigial; the only need is the react-server
condition for `server-only`. Pass `--conditions=react-server` straight
to tsx, which is portable with no env var and no new dependency.
Removed `test:unit`: plain `node --test` cannot run the server test
(needs tsconfig path mocks + react-server), so it was broken, and
`test` now runs the whole suite cross-platform.
2. RPC outage classification. viem 2.55.19's verifyMessage returns false
(rather than throwing) when the RPC transport is unreachable, so an
outage surfaced as 401 "signature does not match" instead of 503.
Added a getChainId liveness probe on the false branch: if the probe
throws, the false came from an outage (503); otherwise it is a genuine
mismatch (401). The nonce is intact either way. The probe only runs on
the failure path, so the happy path keeps its cost.
3. Transport-level outage test through viem itself. The chain mock now
returns a real viem client with an unreachable transport when
transportOutage is set, so the outage path runs through viem's actual
verifyErc6492 path (the same one smart-wallet verification uses) rather
than a hand-rolled stub. Added a unit test asserting 503 + nonce intact.
4. e2e IP-limit test. The IP check used a fresh IP per request, so it
passed even with no limiter. Kept the cross-IP isolation check and
added a single repeated IP past the 30/min limit, asserting it
eventually gets 429.
Test key: replaced the hardcoded TEST_PRIVATE_KEY with a freshly
generated disposable key (never funded, public by design) and labelled
it. The old key is in git history; if it was ever used for real funds
those should be moved, since it is now public.
CodeRabbit review caught two issues:
1. The wallet rate limit added to issueNonce was still abusable. The nonce
endpoint is unauthenticated (no signature), so the launcher check only
confirms the supplied wallet matches the public on-chain launcher
address. An attacker who knows that address passes the launcher check
and spends the wallet bucket. Removed the wallet rate limit from
issueNonce entirely; the IP limit in the route is the only limit on
this endpoint.
2. The db-mock only checked nonce + used_at, not the full predicate set
(chain_id, token, wallet, expires_at). The begin() did not
snapshot/restore on rollback, and the metadata branch could not fail,
so nonce rollback on a failed INSERT was untested. Fixed: the mock now
checks all predicates, snapshots state before the transaction callback
and restores on rejection, and supports a configurable metadata-write
failure.
Updated tests: the nonce DoS test now uses the creator's wallet (the real
attack vector), added predicate-match tests (wrong wallet, wrong chain_id,
expired), and a transaction rollback test.
Hits the real Next.js dev server with a real Postgres DB to verify the
fix end-to-end. Covers: non-creator rejection, creator nonce issuance,
10 non-creator requests not freezing the creator's budget (finding 1),
invalid signature not burning the nonce (finding 2), valid signature
consuming the nonce and writing metadata, nonce reuse rejection, 10
invalid-sig edit requests not freezing the creator's edit budget
(finding 1), non-creator edit rejection, and IP-keyed rate limiting.
RED-GREEN verified: 3 tests fail on the original code for the right
reasons (nonce burned on invalid sig, wallet frozen by attacker, wallet
bucket spent before creator check), all 17 pass after the fix.
beardthelion