fix(nostr): NIP-98 payload hash verifies raw request bytes, not a re-serialization — closes #565#573
Conversation
…serialization (#565) src/auth/nostr.js hashed JSON.stringify(request.body) — the parsed- then-recompacted body — to check the NIP-98 `payload` tag. NIP-98 defines that tag as sha256 over the EXACT body bytes on the wire, so any client sending pretty-printed (or differently-escaped / different key-order) JSON 401'd with 'Payload hash mismatch' despite a valid signature, URL, method, and timestamp. It silently forced every NIP-98 client to send minified JSON in Node's exact serialization — not a real spec requirement, and the bug only bit Nostr auth (DPoP has no payload check), making valid signatures look broken. Root cause: by the time the auth check runs, Fastify's default application/json parser has already turned the body into an object — the raw bytes are gone, so the re-serialization was the best the code could do, and it only matched by coincidence when the client minified. Fix (two halves): - server.js overrides the application/json content-type parser to capture the raw bytes as request.rawBody before parsing. It otherwise mirrors Fastify 4's defaultJsonParser exactly — empty body → 400, secure-json-parse (the SAME prototype-pollution protection JSS gets today; not plain JSON.parse), 400 on malformed — so no other request path changes. secure-json-parse promoted to a direct dependency for this. - nostr.js hashes request.rawBody when present, never a re-serialization. Other content types are unaffected: they stay Buffers via the '*' parser and already hashed raw bytes correctly; only application/json (parsed to an object) was broken. Tests (test/nip98-payload-hash.test.js, 6): - parser: pretty JSON accepted, malformed → 400, __proto__ → 400 + Object.prototype not polluted (proves secure parsing preserved) - payload hash: a pretty body with rawBody passes AND resolves to the did:nostr identity; the same body WITHOUT rawBody still mismatches (control — proves rawBody is the fix); a compact body still verifies (no regression). Full suite: 968/968 (the global JSON-parser override regressed nothing). Closes #565.
There was a problem hiding this comment.
Pull request overview
Fixes NIP-98 payload-hash verification so Nostr auth validates the SHA-256 hash against the original request body bytes (captured pre-parse) rather than against JSON.stringify(request.body), which could reject valid pretty-printed/non-canonically-serialized JSON.
Changes:
- Override Fastify’s
application/jsoncontent-type parser to stash the pre-parsed body asrequest.rawBodywhile preserving error semantics and prototype-pollution protection viasecure-json-parse. - Update NIP-98 verification to prefer hashing
request.rawBodywhen present. - Add focused regression tests covering pretty JSON acceptance, malformed JSON 400s, prototype-pollution blocking, and NIP-98 payload-hash behavior.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/nip98-payload-hash.test.js | Adds regression coverage for raw-body capture and NIP-98 payload hashing. |
| src/server.js | Captures raw JSON request body bytes via a custom application/json parser and decorates request.rawBody. |
| src/auth/nostr.js | Switches payload hashing to use request.rawBody when available. |
| package.json | Adds secure-json-parse as a direct dependency. |
| package-lock.json | Updates lockfile to include the new dependency and version metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else if (Buffer.isBuffer(request.body)) { | ||
| bodyString = request.body.toString(); | ||
| } else { |
There was a problem hiding this comment.
Good catch — real bug in the pre-existing Buffer branch, fixed in 3dda984. crypto.update() takes a Buffer directly, so I dropped the .toString() round-trip and now pass each body through in its native form (string → UTF-8 encode, Buffer → raw bytes), never a lossy conversion. Added a regression test with a deliberately non-UTF-8 Buffer body (ff fe 00 01 80 c3 28, plus a sanity assert that it genuinely changes under a UTF-8 round-trip) — it verifies against its raw-byte payload hash now and would have mismatched under the old path. JSON's rawBody stays a string, which is lossless since JSON is UTF-8 by spec.
There was a problem hiding this comment.
Done in 09fed94 — this is a distinct issue from the Buffer one (which is fixed), and a real gap: the guard now keys on request.body !== undefined instead of truthiness, so a JSON body that parses to null/false/0/"" is still hash-checked. Fastify leaves request.body undefined only when no body was sent, so genuinely-bodyless requests stay skipped while falsy-but-present bodies are verified. Test: a false body with a wrong payload tag is now rejected (was silently skipped before) and passes with the correct tag.
…ing() Copilot caught a real correctness bug in the pre-existing Buffer branch: `request.body.toString()` UTF-8-decodes the bytes, which mangles binary / non-UTF-8 bodies (an image PUT, etc.) — so the server's hash wouldn't match the raw-byte sha256 a NIP-98 client signed. crypto.update() accepts a Buffer directly, so we now pass the Buffer through unconverted and only ever hash native forms (string → UTF-8, Buffer → raw bytes). New test: a deliberately non-UTF-8 Buffer body (with a sanity check that it's genuinely lossy under a UTF-8 round-trip) verifies against its raw-byte payload hash — would have failed under the old .toString() path. 7/7 in the file; full suite green.
| // Validate payload hash if present and request has body | ||
| const payloadTag = getTagValue(event, 'payload'); | ||
| if (payloadTag && request.body) { |
Copilot pass-2 (separate from the Buffer fix): the `if (payloadTag && request.body)` guard skipped payload validation when a JSON body parsed to a falsy value (null/false/0/""), so a client could sign a payload tag, send a falsy body, and the integrity check silently didn't run. Guard on `request.body !== undefined` instead — Fastify leaves body undefined only when no body was sent — so falsy-but-present bodies are still verified while genuinely bodyless requests stay skipped. New test: a `false` body with a WRONG payload tag is now rejected (was skipped), and with the correct tag passes. 8/8 in file; full suite green.
| if (body === '' || body == null) { | ||
| const err = new Error("Body cannot be empty when content-type is set to 'application/json'"); | ||
| err.statusCode = 400; | ||
| return done(err, undefined); | ||
| } | ||
| let json; | ||
| try { | ||
| json = sjson.parse(body); | ||
| } catch (err) { | ||
| err.statusCode = 400; | ||
| return done(err, undefined); | ||
| } |
There was a problem hiding this comment.
Fixed in 875680c. The empty-body error now sets err.code = 'FST_ERR_CTP_EMPTY_JSON_BODY' (same message + 400) so the error-response shape — which surfaces err.code at server.js:235 — matches Fastify's default parser exactly. The malformed-JSON path already matched: Fastify's default just sets statusCode = 400 on the raw secure-json-parse error without adding an FST code, which is what we do — added a comment so that's clearly intentional, not an omission. Test asserts an empty application/json body returns 400 with code === 'FST_ERR_CTP_EMPTY_JSON_BODY'.
…JSON_BODY code Copilot pass-3: the custom parser's empty-body 400 was a plain Error with no err.code, but JSS's error handler emits code: err.code, so the response shape drifted from Fastify's default (which uses FST_ERR_CTP_EMPTY_JSON_BODY) — undercutting the 'mirrors the default exactly' claim. Set err.code = 'FST_ERR_CTP_EMPTY_JSON_BODY' with the same message/status. The malformed-JSON path already matched (Fastify sets statusCode on the raw parser error without an FST code; so do we) — documented that in a comment so it doesn't look like an oversight. New test: an empty application/json body returns 400 with code === 'FST_ERR_CTP_EMPTY_JSON_BODY'. 9/9 in file; full suite green.
Bundles three fixes merged since 0.0.208: - fix(nostr): NIP-98 payload hash verifies the raw request bytes instead of a re-serialization, so binary/whitespace-sensitive bodies validate (#573, closes #565) - feat(idp): served did:nostr documents use the CID v1.0 @context so Multikey / publicKeyMultibase are actually defined (#569) - feat(cli): `jss start` shifts off a busy port (Vite-style) instead of crashing on EADDRINUSE, and prints an openable banner URL (#574, closes #557)
Closes #565.
Bug
src/auth/nostr.jschecked the NIP-98payloadtag againstJSON.stringify(request.body)— the parsed-then-recompacted body. NIP-98 defines that tag assha256(request body)over the exact wire bytes, so any client sending pretty-printed JSON (or a different key order / escaping than Node'sJSON.stringify) 401'd withPayload hash mismatchdespite a valid signature, URL, method, and timestamp. It silently forced every NIP-98 client toward minified JSON — not a real requirement — and only bit Nostr auth (DPoP has no payload check), making valid signatures look broken.Root cause
By the time the auth preHandler runs, Fastify's default
application/jsonparser has already turned the body into an object — the raw bytes are gone. The re-serialization was the best the old code could do, and it matched only by coincidence when the client happened to minify in Node's exact serialization.Fix (two halves)
server.jsoverrides theapplication/jsoncontent-type parser to capture the raw bytes asrequest.rawBodybefore parsing — the only point they still exist. It otherwise mirrors Fastify 4'sdefaultJsonParserexactly: empty body → 400,secure-json-parse(the same prototype-pollution protection JSS gets today — not plainJSON.parse), 400 on malformed. So no other request path changes.secure-json-parsepromoted to a direct dependency.nostr.jshashesrequest.rawBodywhen present, never a re-serialization.Scope: only
application/jsonwas affected — it's the one type parsed into an object. Turtle, JSON-LD, octet-stream, etc. fall through the*parser as Buffers and already hashed raw bytes correctly.Why mirror Fastify's parser instead of
JSON.parseReplacing the global JSON parser is the blast radius here, so it had to be byte-faithful. Using plain
JSON.parsewould have dropped the prototype-pollution protection Fastify provides by default — a real security regression. The test asserts a__proto__payload is still rejected andObject.prototypestays clean.Tests (
test/nip98-payload-hash.test.js, 6 — first direct NIP-98 payload coverage)createServer+inject): pretty JSON accepted; malformed → 400 (not 500);__proto__→ 400 + prototype not polluted.verifyNostrAuth): a pretty body withrawBodypasses and resolves to thedid:nostr:identity; the same body withoutrawBodystill mismatches (control — provesrawBodyis the fix); a compact body still verifies (no regression).Full suite: 968/968 — the global JSON-parser override regressed nothing, confirming the mirror is faithful.