Skip to content

Commit 3dda984

Browse files
melvincarvalhomelvincarvalho
committed
review fix (#573): hash Buffer bodies directly, not via a lossy toString()
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.
1 parent 1fa2266 commit 3dda984

2 files changed

Lines changed: 37 additions & 14 deletions

File tree

‎src/auth/nostr.js‎

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -244,29 +244,35 @@ export async function verifyNostrAuth(request) {
244244
// Validate payload hash if present and request has body
245245
const payloadTag = getTagValue(event, 'payload');
246246
if (payloadTag && request.body) {
247-
let bodyString;
247+
// Hash the EXACT bytes the client signed. NIP-98's `payload` tag is
248+
// sha256(request body) over the wire bytes. crypto.update() accepts a
249+
// string (encoded UTF-8) or a Buffer (raw bytes), so we pass each
250+
// through in its native form — never a lossy conversion.
251+
let bodyData;
248252
if (typeof request.rawBody === 'string') {
249-
// The exact bytes the client signed. NIP-98's `payload` tag is
250-
// sha256(request body) over the wire bytes; the application/json
251-
// parser stashes them here (#565) because by this point request.body
252-
// is already a parsed object and the original bytes are gone.
253-
// Re-serializing the object (the old `else` branch) only matched when
254-
// the client happened to send minified JSON in Node's exact key order
255-
// — pretty-printed or differently-escaped bodies 401'd despite a
256-
// valid signature.
257-
bodyString = request.rawBody;
253+
// application/json raw wire string captured by the parser (#565),
254+
// because by this point request.body is already a parsed object and
255+
// the original bytes are gone. Re-serializing the object (the old
256+
// fallback) only matched when the client happened to send minified
257+
// JSON in Node's exact key order — pretty-printed or differently-
258+
// escaped bodies 401'd despite a valid signature. (JSON is UTF-8 by
259+
// spec, so the captured string round-trips losslessly.)
260+
bodyData = request.rawBody;
258261
} else if (typeof request.body === 'string') {
259-
bodyString = request.body;
262+
bodyData = request.body;
260263
} else if (Buffer.isBuffer(request.body)) {
261-
bodyString = request.body.toString();
264+
// Hash the Buffer DIRECTLY. A .toString() round-trip UTF-8-mangles
265+
// binary / non-UTF-8 bodies (e.g. an image PUT) and would cause
266+
// false mismatches against the raw-byte hash the client signed.
267+
bodyData = request.body;
262268
} else {
263269
// No raw bytes captured (shouldn't happen for HTTP requests:
264270
// application/json sets rawBody, other types stay a Buffer). Keep a
265271
// deterministic fallback rather than throwing.
266-
bodyString = JSON.stringify(request.body);
272+
bodyData = JSON.stringify(request.body);
267273
}
268274

269-
const expectedHash = crypto.createHash('sha256').update(bodyString).digest('hex');
275+
const expectedHash = crypto.createHash('sha256').update(bodyData).digest('hex');
270276
if (payloadTag.toLowerCase() !== expectedHash.toLowerCase()) {
271277
return { webId: null, error: 'Payload hash mismatch' };
272278
}

‎test/nip98-payload-hash.test.js‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,4 +133,21 @@ describe('NIP-98 payload hash uses the raw request bytes (#565)', () => {
133133
assert.notStrictEqual(result.error, 'Payload hash mismatch',
134134
`compact body should still pass; got error: ${result.error}`);
135135
});
136+
137+
it('a binary / non-UTF-8 Buffer body hashes the raw bytes (Copilot review on #573)', async () => {
138+
// Bytes that are NOT valid UTF-8 — a .toString() round-trip would
139+
// mangle them (replacement chars) and break the hash. The client
140+
// signs sha256 over the raw bytes; the server must do the same.
141+
const binary = Buffer.from([0xff, 0xfe, 0x00, 0x01, 0x80, 0xc3, 0x28]);
142+
// Sanity: this fixture genuinely changes bytes under a UTF-8 string
143+
// round-trip, so the old `.toString()` path WOULD have mismatched.
144+
assert.ok(!Buffer.from(binary.toString('utf8'), 'utf8').equals(binary),
145+
'fixture must be lossy under a UTF-8 round-trip');
146+
const token = nip98Token(url, 'PUT', sk, binary);
147+
// Non-JSON body → no rawBody; request.body is the raw Buffer (the `*`
148+
// content-type parser keeps it as a Buffer).
149+
const result = await verifyNostrAuth(mockRequest(token, { rawBody: undefined, body: binary }));
150+
assert.notStrictEqual(result.error, 'Payload hash mismatch',
151+
`binary body must hash raw bytes, not a UTF-8 round-trip; got error: ${result.error}`);
152+
});
136153
});

0 commit comments

Comments
 (0)