feat: 2.0.0 security modernization by enochgroot · Pull Request #25687 · msgpack/msgpack-node

enochgroot · 2026-09-10T16:15:39Z

Summary

2.0.0 security modernization of this native Node.js MessagePack addon.

Keeps the native binding. Vendors msgpack-c c-7.0.2. Requires Node.js 18+. Unpack now fails closed on oversized array/map/str/bin/ext headers instead of allocating them. Pack no longer aborts the process on a throwing getter.

License

This addon remains BSD-3-Clause. Vendored deps/msgpack/ is Boost Software License 1.0.

msgpack-c relicensed Apache-2.0 → BSL-1.0 in 1.3.0 (2015-11-21); see msgpack/msgpack-c#366 and CHANGELOG #386. c-7.0.2 ships Boost headers. This binding does not restate those files as Apache. See LICENSE and SECURITY.md.

Docs / CLI / benchmarks (review follow-up)

README is written as the upstream 2.0.0 tree (no personal-fork branding).
Command Line Utilities restored. bin/json2msgpack and bin/msgpack2json still ship.
Benchmarks restored. npm run bench re-ran on Node v20.20.2; current numbers are in the README. Historical 2011 numbers are gone.

Test plan

npm test — 128 pass, 0 fail
npm run bench
echo '[1, 2, 3]' | ./bin/json2msgpack | ./bin/msgpack2json

Closes #25686

Vendor msgpack-c c-7.0.2, fail-closed unpack limits, and fix the sbuffer leak on pack throw (nodejs/node#25686). Replace nodeunit with node:test, require Node 18+, and run GitHub Actions on 18/20/22.

Dates and toJSON apply at every nesting level. Numeric object keys are packed instead of dropped. Cycle marks use V8 private symbols so a user key named _msgpack_stack is no longer stripped. Integral doubles outside uint64/int64 range (e.g. 1e30) pack as float64. Pack recursion is capped at 512 so 8000-deep input throws instead of SIGSEGV. The vendored C unpacker is built with MSGPACK_EMBED_STACK_SIZE=512 so advertised unpack depth matches the walker. Stream emits error on unpack throw and treats packed nil as a message. The CLI bins run on Node 18+.

DefineOwnProperty on map keys so a wire __proto__ cannot replace the decoded object's prototype. Property reads during pack go through Nan::TryCatch so a throwing getter or Proxy ownKeys raises a catchable error instead of aborting on ToLocalChecked. Stream snapshots bytes_remaining and advances the buffer before emit('msg'), so a listener that unpacks or throws cannot desync or replay a frame.

pack() ran a JS pre-pass that called toJSON() on any top-level object. Buffer.prototype.toJSON exists, so msgpack.pack(Buffer.from([1,2,3])) emitted a {type:'Buffer',data:[...]} map instead of msgpack bin, and unpack() no longer returned a Buffer. Nested Buffers were unaffected because they went straight to the binding. The pre-pass is redundant: the native JsToMsgpack checks node::Buffer::HasInstance before the generic object path, PackObject applies toJSON at every level, and Dates pack as ISO strings natively. Drop it and forward arguments to the binding unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

NODE_MODULE is not context-aware, so requiring msgpack in the main thread then in a Worker throws "Module did not self-register". Register with NODE_MODULE_CONTEXT_AWARE and keep the sbuffer pool, unpack remainder, and cycle-detection key thread_local so workers do not race the main isolate. Fixes msgpack#60

Packed integer 0 must emit on Stream (msgpack-node#44). A map written by python-msgpack with a bin8 payload must unpack Payload as Buffer (msgpack-node#10). Both already work on 2.0.0; these tests keep them so. Refs msgpack#10 Refs msgpack#44

Adds index.d.ts matching the 2.0.0 runtime: pack(...values) returns Buffer, unpack.bytes_remaining is per-thread, Stream wraps a duplex. Fixes msgpack#39

Address review on PR #2 for worker_threads (msgpack#60): - Mark sbuf_pool thread_local so a worker pack cannot share the pool - Register with NAN_MODULE_WORKER_ENABLED instead of NODE_MODULE_CONTEXT_AWARE - Return non-pooled sbuffers to the pool so it actually fills - Add concurrent pack (workers + main) and sequential pool-reuse tests

…urity/modernize-2.0.0

…ize-2.0.0

lib/ and bin/ reach 100% statements/branches/functions/lines under c8; src/msgpack.cc reaches 95.9% lines and 99.5% branches under gcovr. New tests: - test/coverage-native.test.js walks every MessagePack format family from hand-built wire bytes, including the ones pack() never emits (float32, str8/16/32, bin16/32, array32, map16/32, all eight ext forms, negative fixint); a truncation point for every header and payload; the kMaxBytes, kMaxContainer and kMaxDepth rejections; 0xc1; the pack-side type dispatch (Symbol, BigInt, non-finite numbers, integer edges, undefined, zero- and multi-argument pack); Date and toJSON failure modes with mark cleanup; and a worker nesting 600 packs deep to saturate the thread-local sbuffer pool and reach the "pool is full, free it" arm of ~PackBuffer. - test/cli.test.js covers the exit-1 paths of both CLIs. The pack-failure arm of json2msgpack is reachable from real JSON only through nesting deeper than the 512-level pack cap; every other pack error needs a value JSON.parse cannot produce. Infrastructure: - binding.gyp grows an msgpack_coverage variable, default 0. Only when it is set to 1 does the addon compile and link with --coverage -O0 -g, so npm install and node-gyp rebuild stay uninstrumented. - npm run coverage runs coverage:js (c8, gated at 95% on all four metrics) then coverage:native (scripts/coverage-native.js). The native script rebuilds instrumented, runs the suite, gates on gcovr --fail-under-line 95 --fail-under-branch 95 over src/ excluding deps/, and always rebuilds uninstrumented afterwards -- including when the gate fails. src/msgpack.cc gains comments only. GCOVR_EXCL_BR markers, each with its reason inline, mark branches unreachable without stubbing malloc or V8: allocation-failure arms of msgpack_pack_*, empty-MaybeLocal guards, Skip() calls a preceding CheckBytes has already proved safe, and the post-ScanOne error tail of Unpack. No production code was removed. COVERAGE.md lists all 45 marked lines, the 20 still-uncovered lines, the 2 still-uncovered branches and the un-gated numbers (70.6% raw, 86.3% throw-excluded, 99.5% as shipped).

Adds an ubuntu-latest / Node 20 job that installs gcovr via pip when it is not already present and runs npm run coverage. Both halves are threshold- gated, so the job fails when JS or native coverage drops below 95%. The existing node 18/20/22 x ubuntu/macos test matrix is unchanged.

Acceptance requires the coverage script in the Building section and a pointer to COVERAGE.md for gates and remainder.

Strip personal-fork branding so this reads as the upstream 2.0.0 tree. Document that vendored msgpack-c c-7.0.2 is Boost Software License 1.0 (relicensed from Apache-2.0 in msgpack-c 1.3.0). Restore Command Line Utilities and Benchmarks, refresh the bench runner for Node 18+, and record current numbers.

godsflaw · 2026-09-10T19:01:38Z

-	"description": "A space-efficient object serialization library for node.js",
-	"version": "1.0.3",
-	"homepage": "https://github.com/msgpack/msgpack-node",
-	"contributors": [


you should not remove the contributor list

Restored the contributors list from master in 6892b13 (all 12 entries, names/urls/emails unchanged).

godsflaw

Thanks for the thorough 2.0 security pass — ScanOne limits, PackBuffer RAII, worker-aware thread_locals, and the DefineOwnProperty / CheckedGet hardening all look solid. I verified PackBuffer against #25686: the sbuffer is owned by RAII and is recycled/freed on throw paths, so that leak is fixed.

Requesting a few small fixes before merge:

Restore the contributors list in package.json (please don’t wipe history there).
Fix SECURITY.md: the sbuffer leak is #25686, not nodejs/node#25686.
Add Closes #25686 (or Fixes #25686) to the PR body so the issue closes on merge.

Non-blocking: consider a windows-latest CI cell; and update the Stream comment in lib/msgpack.js — bytesRemaining is thread_local now, not a process-global.

Wire breaks for 2.0 (Buffer→bin, ext reject, unpack caps, Node 18+) look intentional and documented; once the three items above land I’m happy to approve.

Restore the master contributors list in package.json. Point SECURITY.md, src/msgpack.cc, and the leak regression at msgpack#25686 instead of nodejs/node#25686. Correct the Stream comment: bytesRemaining is thread_local. Add windows-latest to the CI matrix. Closes msgpack#25686

enochgroot · 2026-09-12T01:19:32Z

Thanks for the review.

The three merge items are in 6892b13 on security/modernize-2.0.0:

package.json contributors restored from master (all 12 entries).
SECURITY.md (and the other nodejs/node#25686 mentions in this tree) now cite msgpack/msgpack-node#25686.
This PR body includes Closes #25686.

Also took the nits in the same commit: windows-latest added to the CI matrix, and the Stream comment in lib/msgpack.js now says bytesRemaining is thread_local, not process-global.

npm test is still 128 pass / 0 fail.

Pin the Windows matrix to windows-2022 so node-gyp 10/11 (Node 18/20/22) can find Visual Studio. windows-latest currently ships VS 2026, which those node-gyp versions report as unknown version "undefined". Run install and test as one bash command so a failed rebuild cannot continue into npm test. List test files explicitly so npm test works on Windows Node 18/20 (cmd.exe does not expand *, and node --test globs need Node 21+). The Stream test comment now matches thread_local bytesRemaining.

enochgroot · 2026-09-12T01:38:10Z

Follow-up for the Windows CI failures on 6892b13:

Pinned the Windows matrix cell to windows-2022. windows-latest currently ships Visual Studio 2026 (18.x), and the node-gyp 10/11 bundled with Node 18/20/22 reports that as unknown version "undefined", so the native addon never built.
Install and test now run as npm install && npm test under bash, so a failed rebuild is a hard stop on Windows too.
npm test lists the test files explicitly, so it is Windows-safe on Node 18/20 (cmd.exe does not expand *, and node --test glob support is Node 21+).
Corrected the Stream test comment in test/msgpack.test.js: bytesRemaining is thread_local native state, not a process-global C++ value.

Local npm test is still 128 pass / 0 fail.

godsflaw

Re-reviewed tip 5adeb32.

All three merge items look good:

package.json contributors match master (12 entries, including godsflaw).
SECURITY.md / src/msgpack.cc now cite msgpack/msgpack-node#25686 (no remaining nodejs/node#25686).
PR body has Closes #25686.

Also took the nits: Stream comment correctly describes thread_local bytesRemaining, and Windows is in CI as windows-2022 with bash npm install && npm test (sensible pin given VS 2026 / node-gyp). Fork Actions on this SHA is green.

LGTM — approve.

enochgroot and others added 14 commits September 10, 2026 00:02

feat: 2.0.0 security modernization

ee5e5be

Vendor msgpack-c c-7.0.2, fail-closed unpack limits, and fix the sbuffer leak on pack throw (nodejs/node#25686). Replace nodeunit with node:test, require Node 18+, and run GitHub Actions on 18/20/22.

feat: ship TypeScript types for pack, unpack, and Stream

844192d

Adds index.d.ts matching the 2.0.0 runtime: pack(...values) returns Buffer, unpack.bytes_remaining is per-thread, Stream wraps a duplex. Fixes msgpack#39

Merge remote-tracking branch 'origin/test/issue-regressions' into sec…

5578c8a

…urity/modernize-2.0.0

Merge remote-tracking branch 'origin/feat/types' into security/modern…

037e636

…ize-2.0.0

docs: document npm run coverage in README

5d766ca

Acceptance requires the coverage script in the Building section and a pointer to COVERAGE.md for gates and remainder.

enochgroot mentioned this pull request Sep 10, 2026

feat: 2.0.0 security modernization enochgroot/msgpack-node#1

Closed

3 tasks

godsflaw requested changes Sep 10, 2026

View reviewed changes

godsflaw approved these changes Sep 14, 2026

View reviewed changes

Navigation Menu

Uh oh!

feat: 2.0.0 security modernization - #25687

feat: 2.0.0 security modernization#25687
enochgroot wants to merge 16 commits into
msgpack:masterfrom
enochgroot:security/modernize-2.0.0

enochgroot commented Sep 10, 2026 •

edited

Loading

Uh oh!

godsflaw Sep 10, 2026

Uh oh!

enochgroot Sep 12, 2026

Uh oh!

godsflaw left a comment

Uh oh!

enochgroot commented Sep 12, 2026

Uh oh!

enochgroot commented Sep 12, 2026

Uh oh!

godsflaw left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

enochgroot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

License

Docs / CLI / benchmarks (review follow-up)

Test plan

Uh oh!

godsflaw Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

enochgroot Sep 12, 2026

Choose a reason for hiding this comment

Uh oh!

godsflaw left a comment

Choose a reason for hiding this comment

Uh oh!

enochgroot commented Sep 12, 2026

Uh oh!

enochgroot commented Sep 12, 2026

Uh oh!

godsflaw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

enochgroot commented Sep 10, 2026 •

edited

Loading