Merge pull request #44 from athombv/fix/parser-robustness

fix(parser): defensive Bitmap copy and Struct prototype-chain guard

Author: RobinBol

lib/DataTypes.js

@@ -103,7 +103,11 @@ class Bitmap {
 
   static fromBuffer(buf, i, len, args) {
     i = i || 0;
-    return new Bitmap(buf.slice(i, i + len), args);
+    // Copy the slice so the resulting Bitmap owns its bytes. `Buffer.slice`
+    // returns a view sharing memory with the source; without the copy, later
+    // mutation of `buf` (e.g. a radio driver reusing a receive buffer) would
+    // silently change the bits of an already-parsed Bitmap.
+    return new Bitmap(Buffer.from(buf.slice(i, i + len)), args);
   }
 
   static toBuffer(buf, i, length, args, v) {

lib/Struct.js

@@ -35,7 +35,20 @@ function Struct(name, defs, opts) {
       constructor(props = {}) {
         // eslint-disable-next-line guard-for-in,no-restricted-syntax
         for (const key in props) {
-          if (!defs[key]) throw new TypeError(`${this.constructor.name}: ${key} is an unexpected property`);
+          // Use hasOwnProperty so prototype-chain keys like `constructor`,
+          // `toString`, etc. are rejected. The previous `!defs[key]` check
+          // walked the prototype chain and would accept `constructor` because
+          // `defs.constructor` resolves to `Object` (truthy).
+          //
+          // Use `new.target.name` for the prefix rather than
+          // `this.constructor.name`: classes constructed via this pattern can
+          // be subclassed, and `new.target` is the actual subclass being
+          // instantiated. Unlike `this.constructor`, `new.target` is a
+          // syntactic binding and cannot be shadowed by a `constructor`
+          // field that the caller may have set earlier in this same loop.
+          if (!Object.prototype.hasOwnProperty.call(defs, key)) {
+            throw new TypeError(`${new.target.name}: ${key} is an unexpected property`);
+          }
           this[key] = props[key];
         }
         // eslint-disable-next-line no-restricted-syntax

test/DataTypes.test.js

@@ -26,5 +26,23 @@ describe('DataType', function() {
       testMap.constructor.toBuffer(buffer, bufferOffset, testMap.length, bits, testMap);
       assert.deepEqual(buffer, expectedBuffer, 'Static toBuffer failed');
     });
+
+    describe('source buffer aliasing', function() {
+      it('should not share memory with the source buffer after fromBuffer', function() {
+        const source = Buffer.from([0xff]);
+        const map = DataTypes.map8('a', 'b', 'c', 'd', 'e', 'f', 'g', 'h')
+          .fromBuffer(source, 0);
+
+        // Snapshot bits, mutate source, read again. If the Bitmap aliases
+        // source memory, the second read observes the mutation.
+        const bitsBefore = map.getBits();
+        source[0] = 0x00;
+        const bitsAfter = map.getBits();
+
+        assert.deepEqual(bitsBefore, bitsAfter,
+          'Bitmap must own its bytes; mutating source must not affect parsed value');
+        assert.deepEqual(bitsAfter, ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h']);
+      });
+    });
   });
 });

test/Struct.test.js

@@ -152,4 +152,64 @@ describe('Struct', function() {
     assert(refData.field6.bit9);
     assert.deepEqual(refData.field6.toArray(), ['bit2', 'bit9']);
   });
+
+  describe('prototype-chain property guard', function() {
+    let S;
+    before(function() {
+      S = Struct('GuardedStruct', { a: DataTypes.uint8 });
+    });
+
+    it('should reject `constructor` as a field name', function() {
+      assert.throws(
+        () => new S({ constructor: 'attacker-controlled' }),
+        /unexpected property/,
+        'constructor is on Object.prototype, not an own property of defs',
+      );
+    });
+
+    it('should reject `toString` as a field name', function() {
+      assert.throws(
+        () => new S({ toString: () => 'pwned' }),
+        /unexpected property/,
+      );
+    });
+
+    it('should still accept declared own-property field names', function() {
+      const instance = new S({ a: 42 });
+      assert.strictEqual(instance.a, 42);
+    });
+
+    it('should still reject undeclared field names', function() {
+      assert.throws(
+        () => new S({ b: 1 }),
+        /unexpected property/,
+      );
+    });
+
+    it('should produce a stable error message even when a `constructor` field is declared', function() {
+      // Edge case: if a struct legitimately declares a `constructor` field
+      // and the caller sets it before an unexpected key is seen, the throw
+      // path must not depend on `this.constructor.name`.
+      const Weird = Struct('WeirdStruct', {
+        constructor: DataTypes.uint8,
+        a: DataTypes.uint8,
+      });
+      assert.throws(
+        () => new Weird({ constructor: 1, badKey: 2 }),
+        err => err instanceof TypeError && /^WeirdStruct: badKey is an unexpected property$/.test(err.message),
+      );
+    });
+
+    it('should report the actual subclass name in the error when subclassed', function() {
+      // Struct-generated classes can be subclassed; the error message should
+      // identify the actual class being instantiated, not the underlying
+      // Struct name. This is what `new.target.name` gives us.
+      const Base = Struct('BaseStruct', { a: DataTypes.uint8 });
+      class Extended extends Base {}
+      assert.throws(
+        () => new Extended({ unexpected: 1 }),
+        err => err instanceof TypeError && /^Extended: unexpected is an unexpected property$/.test(err.message),
+      );
+    });
+  });
 });