feat: 2.0.0 security modernization - #1
enochgroot wants to merge 14 commits into
Conversation
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
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.
| * Distributed under the Boost Software License, Version 1.0. | ||
| * (See accompanying file LICENSE_1_0.txt or copy at | ||
| * http://www.boost.org/LICENSE_1_0.txt) |
There was a problem hiding this comment.
Why did you change these license lines? It was Apache 2.0 and now there is what, some Boost license?
There was a problem hiding this comment.
Kept Boost. zone.h (and the rest of vendored msgpack-c c-7.0.2) 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 msgpack#386. This binding does not restate those headers as Apache. Native first-party code stays BSD-3-Clause (the original node-msgpack license).
| Boost Software License - Version 1.0 - August 17th, 2003 | ||
|
|
||
| Permission is hereby granted, free of charge, to any person or organization | ||
| obtaining a copy of the software and accompanying documentation covered by | ||
| this license (the "Software") to use, reproduce, display, distribute, | ||
| execute, and transmit the Software, and to prepare derivative works of the | ||
| Software, and to permit third-parties to whom the Software is furnished to | ||
| do so, all subject to the following: | ||
|
|
||
| The copyright notices in the Software and this entire statement, including | ||
| the above license grant, this restriction and the following disclaimer, | ||
| must be included in all copies of the Software, in whole or in part, and | ||
| all derivative works of the Software, unless such copies or derivative | ||
| works are solely in the form of machine-executable object code generated by | ||
| a source language processor. | ||
|
|
||
| THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
| IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
| FITNESS FOR A PARTICULAR PURPOSE, TITLE AND NON-INFRINGEMENT. IN NO EVENT | ||
| SHALL THE COPYRIGHT HOLDERS OR ANYONE DISTRIBUTING THE SOFTWARE BE LIABLE | ||
| FOR ANY DAMAGES OR OTHER LIABILITY, WHETHER IN CONTRACT, TORT OR OTHERWISE, | ||
| ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER | ||
| DEALINGS IN THE SOFTWARE. |
There was a problem hiding this comment.
same license question. This seems like a big change without good justification.
There was a problem hiding this comment.
Yes — that file is the current c-7.0.2 LICENSE, Boost Software License 1.0. msgpack-c left Apache-2.0 in 1.3.0 (2015-11-21). Documented in root LICENSE and SECURITY.md so readers do not have to hunt the C changelog.
| and deps/msgpack/NOTICE for further information> | ||
| . MessagePack C library, located in deps/msgpack/. | ||
|
|
||
| Vendored from msgpack-c c-7.0.2, licensed under the Boost Software |
There was a problem hiding this comment.
oh, so the C library license changed?
There was a problem hiding this comment.
Yes. The C library's license changed; this binding's did not. Root LICENSE now states: addon remains BSD-3-Clause; vendored msgpack-c is BSL-1.0 as of 1.3.0 (2015-11-21). Native first-party code is not relicensed to Apache-2.0.
|
|
||
| Note that `node-msgpack` produces and consumes Buffer objects, and a such does | ||
| not incur encoding/decoding overhead when performing I/O with native strings. | ||
| This tree is a security-focused 2.0 fork (`enochgroot/msgpack-node`). |
There was a problem hiding this comment.
This will be merged to upstream master, I would leave mentions of your personal enochgroot repo out of this.
There was a problem hiding this comment.
Removed. README now reads as the upstream 2.0.0 tree — no personal-fork branding. package.json homepage/repository and index.d.ts point at msgpack/msgpack-node.
| `node-msgpack` is currently slower than the built-in `JSON.stringify()` and | ||
| `JSON.parse()` methods. In recent versions of node.js, the JSON functions | ||
| have been heavily optimized. node-msgpack is still more compact, and we are | ||
| currently working performance improvements. Testing shows that, over 500k | ||
| iterations, `msgpack.pack()` is about 5x slower than `JSON.stringify()`, and | ||
| `msgpack.unpack()` is about 3.5x slower than `JSON.parse()`. | ||
|
|
||
| Old performance numbers are below. | ||
|
|
||
| The following tests were performed with 500,000 instances of | ||
| the JavaScript object `{'abcdef' : 1, 'qqq' : 13, '19' : [1, 2, 3, 4]}`: | ||
|
|
||
| * `JSON.stringify()` 7.17 seconds | ||
| * `JSON.parse(JSON.stringify())` 22.18 seconds | ||
| * `msgpack.pack()` 5.80 seconds | ||
| * `msgpack.unpack(msgpack.pack())` 8.62 seconds | ||
|
|
||
| Note that `node-msgpack` produces and consumes Buffer objects, and a such does | ||
| not incur encoding/decoding overhead when performing I/O with native strings. |
There was a problem hiding this comment.
Re-tested. npm run bench on Node v20.20.2, Debian 12, linuxkit aarch64: JSON.stringify 179 ms, JSON.parse(JSON.stringify()) 346 ms, msgpack.pack 1021 ms, msgpack.unpack(msgpack.pack()) 1585 ms (500k iterations of the documented object). Current numbers are in README. The 2011 figures are gone. On this small object V8 JSON is faster; msgpack's win here is payload size.
| As a convenience and for debugging, `bin/json2msgpack` and `bin/msgpack2json` | ||
| are provided to convert JSON data to and from MessagePack data, reading from | ||
| stdin and writing to stdout. |
There was a problem hiding this comment.
No — they are still here. bin/json2msgpack and bin/msgpack2json still ship. Restored the Command Line Utilities section in README with Node 18+ examples. Verified: echo '[1, 2, 3]' | ./bin/json2msgpack | ./bin/msgpack2json.
| To run benchmarks: | ||
| ### License | ||
|
|
||
| ./run_tests test/benchmark/benchmark.js |
There was a problem hiding this comment.
don't remove the benchmarking, update it and document the current benchmarks.
There was a problem hiding this comment.
Restored. README has a Benchmarks section with how to run (npm run bench / node test/benchmark/benchmark.js) and the current numbers. The runner is a Node 18+ script (no nodeunit). test/benchmark/ was not deleted.
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.
|
Superseded: opened against msgpack/msgpack-node:master. Review comments on license, README branding, CLI, and benchmarks are addressed on |
2 participants
Summary
Security-focused 2.0.0 modernization of the Node MessagePack addon on
enochgroot/msgpack-node(fork ofmsgpack/msgpack-node).e17beb371b59459a13b48e166a11e123bda5bf93), C API only (Boost 1.0).unpack.ccontainer caps (MSGPACK_NODE_MAX_CONTAINER= 1e6). The bombdd ff 00 00 00throwsmsgpack unpack limit exceededand does not allocate.pack()throw paths free the buffer (nodejs/node#25686).Breaking for 2.0:
Bufferpacks as MessagePack bin (not raw/str); ext types throw on unpack.See
SECURITY.md.Review round 2 (commit
1b9feae)Fixes the eight regressions called out on the previous review:
toISOString()at every nesting level (not only the top-level wrapper).toJSON()is honored on nested objects, matchingJSON.stringify._msgpack_stackis kept.1e30(integral but outside uint64) packs as float64, not2^64-1.MSGPACK_EMBED_STACK_SIZE=512so 32-deep input no longer dies in the C embed stack.bin/msgpack2jsonandbin/json2msgpackno longer use the removedsys/net.StreamAPIs; they run on Node 18+.msgpack.Streamemitserroron unpack throw (and drops the bomb) instead of throwing from thedatalistener; packednull(0xc0) is emitted as a message, not treated as "incomplete".Test plan
npx node-gyp rebuildon Node 20.20.2 linux arm64npm test— 39 passed, 0 failed (includes nested Date/toJSON, numeric keys, 1e30, 8000-deep pack throw, 32/511-deep unpack, CLI bins, Stream nil + unpack error).github/workflows/ci.yml: Node 18/20/22 × ubuntu-latest + macos-latest)Reviewer
@godsflaw — please review. Collaborator invite requested with maintain so you can merge on this fork.
Coverage (2.0.0)
Independent verification on Node 20 linux/arm64 after landing issue PRs #2/#3/#4 on this branch:
npm test: 128 tests, 0 failc8,lib/+bin/): 100% statements / branches / functions / linesgcovronsrc/msgpack.cconly,--exclude-throw-branches --exclude-unreachable-branches): 95.9% lines (473/493), 99.5% branches (400/402), 100% functions (36/36)CI coverage job (ubuntu-latest, Node 20) fails the PR under 95% for both.
No production logic was deleted.
src/msgpack.cconly gained comments (GCOVR_EXCL_BR_*) on branches that cannot be taken from JS without stubbing the allocator or V8. Full remainder list:COVERAGE.md.Uncovered remainder after those documented exclusions:
Skip()afterCheckBytesalready proved the payload fits (gcovr lines 148 and 575)NAN_MODULE_WORKER_ENABLEDcompile-time#elseon this NAN 2.23.0 treeDo not merge this into
msgpack/msgpack-node. This is the 2.0.0 PR on the fork.