| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
…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.
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:
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.
Sorry, something went wrong.
| } 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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
…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'.
Sorry, something went wrong.
…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)
| Back | FazBrowse Home | New Git URL |
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)
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)
Full suite: 968/968 — the global JSON-parser override regressed nothing, confirming the mirror is faithful.