fix(base-account): parse the SIWE nonce correctly and bind verification to the app domain - #1794
fix(base-account): parse the SIWE nonce correctly and bind verification to the app domain#1794Dusk1e wants to merge 1 commit into
Conversation
🟡 Heimdall Review Status
|
…on to the app domain
The "Authenticate users" guide ships a server example that cannot
authenticate anyone, and a verification step that accepts signatures
minted for other sites.
Nonce extraction used `/at (\w{32})$/`. In an EIP-4361 message the nonce
sits on a `Nonce: <value>` line with `Issued At:` after it, so the regex
never matches and `/auth/verify` answers 400 "Invalid or reused nonce"
for every valid login. Two other pages in this repo already read the
nonce correctly, so the guide was the odd one out. Switching to viem's
`parseSiweMessage` removes the regex entirely.
Verification called `client.verifyMessage`, which only checks that the
signature matches the message. It does not look at the `domain` field,
so a signature a user produced on another site verifies against this
endpoint too. EIP-4361 requires the relying party to check `domain`.
`verifySiweMessage` checks domain, nonce and expiry, and still routes
through `verifyHash`, so ERC-6492 and ERC-1271 signatures from
undeployed Base Accounts keep working.
Verified against a local node with real signatures: a legitimate login
returns 200, a replayed signature 400, a signature minted for another
domain 401, and an unissued nonce 400. Before the change the first case
returned 400 and the third returned 200.
Fixes base#1502
c5f7105 to
32bddba
Compare
Lrifton92
left a comment
There was a problem hiding this comment.
Checked against EIP-4361 and the viem source: the old /at (\w{32})$/ can never match (the SIWE message ends with Issued At: / optional fields, never the nonce), and parseSiweMessage uses Nonce: (?<nonce>[a-zA-Z0-9]+). verifySiweMessage returns a boolean, enforces domain (message.domain !== domain -> false), checks expirationTime/notBefore, and still goes through verifyHash so ERC-6492 signatures keep working. viem/siwe export path is valid. The fix is correct and complete for what it claims — LGTM.
One follow-up worth a look, not blocking: the frontend snippet earlier in this guide still defaults to a client-generated nonce (window.crypto.randomUUID()) with the /auth/nonce fetch commented out. Now that the Express server correctly rejects nonces it did not issue, copying both snippets as-is yields 400 Invalid or reused nonce on every login. Making the fetch the default (or noting that the Express example requires server-issued nonces) would close that gap.
| // 1. Check nonce hasn\'t been reused | ||
| const nonce = message.match(/at (\w{32})$/)?.[1]; | ||
| // 1. Check this server issued the nonce and hasn't seen it before | ||
| const { nonce } = parseSiweMessage(message); |
There was a problem hiding this comment.
Pre-existing, optional: parseSiweMessage(message) throws if message is missing or not a string, which an Express 4 async handler won't catch (hanging request / unhandled rejection). A typeof message !== 'string' guard returning 400 would make the example safer to copy-paste.
|
|
||
| // The domain your app is served from. Anything signed for another | ||
| // domain must not be accepted here. | ||
| const APP_DOMAIN = "yourapp.com"; |
There was a problem hiding this comment.
Nit: worth stating this must equal the message's domain field exactly, i.e. host including port (localhost:3000 in dev), otherwise readers get a puzzling 401 locally.
|
Review Error for Lrifton92 @ 2026-08-15 22:26:16 UTC |
You're right but that part is already covered by #1761 and #1722, so I left it alone to avoid overlapping with them. |
The
Authenticate usersguide ships an Express example that rejects every valid login, and a verification step that accepts signatures a user produced on someone else's site. Both are in code a developer is meant to copy into a production auth endpoint.1. The nonce is never extracted, so
/auth/verifyalways returns 400docs/base-account/guides/authenticate-users.mdx:207reads the nonce with:In an EIP-4361 message the nonce is on its own
Nonce: <value>line andIssued At:comes after it, so nothing sits at the end of the string for$to anchor to. The match is alwaysnull, the guard below it fires, and the endpoint answers400 Invalid or reused noncefor a completely valid login.Using the message format this repo documents at
docs/base-account/reference/core/capabilities/signInWithEthereum.mdx:128:Two other pages in this repo already read the nonce correctly —
signInWithEthereum.mdx:216andframework-integrations/privy/authentication.mdx:227— so this guide was the only one reading it incorrectly. This PR uses viem'sparseSiweMessagerather than another regex.2. Verification is not bound to your domain
Both snippets verified with:
verifyMessageonly answers "does this signature match this message". It never looks at thedomainfield, so a SIWE signature a user was asked to produce onevil.comverifies against your endpoint as well. EIP-4361 requires the relying party to checkdomainagainst its own host (issue #1502).Swapped to
verifySiweMessage, which validatesdomain,nonceand expiry before checking the signature. It still routes throughverifyHashinternally, so ERC-6492 and ERC-1271 signatures from not-yet-deployed Base Accounts keep verifying exactly as before — the guide's own note at line 62 stays accurate.Verification
I ran the proposed server code unmodified against a local node, signing real messages with viem:
400 Invalid or reused nonce200 { ok: true }400400 Invalid or reused nonceevil.com200 { ok: true }401 Invalid signature400400 Invalid or reused nonceRow 1 is the functional bug, row 3 is the security one. Rows 2 and 4 confirm the existing replay protection still behaves.
Relationship to the existing PRs on this file
Five PRs already touch this guide, all opened in May and all still unreviewed. For whoever triages this, here is what each actually changes, checked against viem 2.55.13:
const { isValid } = await verifySiweMessage(...). That function returns aboolean, soisValidisundefinedand the endpoint would answer 401 for every login.nonces.delete(nonce)to after verification, so a nonce survives a failed attempt and two concurrent requests can both pass thehas()check.parseSiweMessage(...).domaincomparison; keepsverifyMessage.noncetoverifySiweMessage, but thatnoncestill comes from the broken regex.Math.random()nonces), no overlap.#1542 covers the same two defects this PR does and is the older submission. The differences here are that the nonce stays an atomic check-and-consume (
nonces.deleteas the guard, as in the current guide), the domain constant does not default tolocalhost:3000, and the behaviour is backed by the run above. If you would rather take #1542, this can be closed — the important thing is that one of them lands, because the guide is currently broken for every reader who copies it.Notes
node scripts/lint-mdx.js docs/base-account/guides/authenticate-users.mdxreports the same 9 errors and 1 warning before and after this change — no new findings introduced.