Skip to content

fix(nostr): NIP-98 payload hash verifies raw request bytes, not a re-serialization — closes #565#573

Merged
melvincarvalho merged 4 commits into
gh-pagesfrom
issue-565-nip98-raw-body
Jun 16, 2026
Merged

fix(nostr): NIP-98 payload hash verifies raw request bytes, not a re-serialization — closes #565#573
melvincarvalho merged 4 commits into
gh-pagesfrom
issue-565-nip98-raw-body

Conversation

@melvincarvalho

Copy link
Copy Markdown
Contributor

Closes #565.

Bug

src/auth/nostr.js checked the NIP-98 payload tag against JSON.stringify(request.body) — the parsed-then-recompacted body. NIP-98 defines that tag as sha256(request body) over the exact wire bytes, so any client sending pretty-printed JSON (or a different key order / escaping than Node's JSON.stringify) 401'd with Payload hash mismatch despite 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/json parser 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)

  1. server.js overrides the application/json content-type parser to capture the raw bytes as request.rawBody before parsing — the only point they still exist. 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.
  2. nostr.js hashes request.rawBody when present, never a re-serialization.

Scope: only application/json was 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.parse

Replacing the global JSON parser is the blast radius here, so it had to be byte-faithful. Using plain JSON.parse would have dropped the prototype-pollution protection Fastify provides by default — a real security regression. The test asserts a __proto__ payload is still rejected and Object.prototype stays clean.

Tests (test/nip98-payload-hash.test.js, 6 — first direct NIP-98 payload coverage)

  • Parser (via createServer + inject): pretty JSON accepted; malformed → 400 (not 500); __proto__ → 400 + prototype not polluted.
  • Payload hash (unit on verifyNostrAuth): 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, confirming the mirror is faithful.

…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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/json content-type parser to stash the pre-parsed body as request.rawBody while preserving error semantics and prototype-pollution protection via secure-json-parse.
  • Update NIP-98 verification to prefer hashing request.rawBody when 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.

Comment thread src/auth/nostr.js
Comment on lines 260 to 262
} else if (Buffer.isBuffer(request.body)) {
bodyString = request.body.toString();
} else {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Comment thread src/auth/nostr.js Outdated
Comment on lines 244 to 246
// 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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Comment thread src/server.js
Comment on lines +285 to +296
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);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.

@melvincarvalho
melvincarvalho merged commit e31293b into gh-pages Jun 16, 2026
1 check passed
@melvincarvalho
melvincarvalho deleted the issue-565-nip98-raw-body branch June 16, 2026 08:29
melvincarvalho added a commit that referenced this pull request Jun 16, 2026
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NIP-98 payload-hash check hashes re-serialized body, not raw — rejects valid non-minified JSON (401)

2 participants