errors: validate constructor name

determineSpecificType() checks for a usable constructor name with
`'name' in value.constructor`. That accepts an empty name, and the `in`
operator requires its right-hand side to be an object.

Anonymous classes own a `name` that is the empty string, so they produce
messages ending in a dangling "Received an instance of ". A truthy
primitive `constructor` reaches the `in` operator and throws while the
message is being built, so ERR_INVALID_ARG_TYPE is replaced by a
TypeError carrying no `code`. The latter is reachable from untrusted
input, since `constructor` is an ordinary JSON key.

The check changed in #49696, while this function was rewritten as a
switch. That pull request updated test/common's invalidArgTypeHelper to
match its deliberate change to the `function` branch, but left the
helper's `object` branch on the original truthy check, so the two have
disagreed since.

Read `constructor` and its `name` once and use the name only when it is
a non-empty string. Requiring a string also stops meaningless names from
being interpolated: 42 currently yields "an instance of 42", and a
symbol name throws outright. The current check reads `constructor` three
times, which an accessor can observe.

Move the helper to the same check so the two cannot drift apart again.

Signed-off-by: Christian Aurich <christian.aurichzm@gmail.com>
PR-URL: #65607
Refs: #49696
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Author: christianaurichzm

lib/internal/errors.js

@@ -1035,11 +1035,16 @@ function determineSpecificType(value) {
       return `type symbol (${String(value)})`;
     case 'function':
       return `function ${value.name}`;
-    case 'object':
-      if (value.constructor && 'name' in value.constructor) {
-        return `an instance of ${value.constructor.name}`;
+    case 'object': {
+      // `constructor` may be user-controlled: it need not be an object, and
+      // its `name` need not be a non-empty string. Reading either can invoke
+      // an accessor, so read each one once.
+      const name = value.constructor?.name;
+      if (typeof name === 'string' && name !== '') {
+        return `an instance of ${name}`;
       }
       return `${lazyInternalUtilInspect().inspect(value, { depth: -1 })}`;
+    }
     case 'string':
       value.length > 28 && (value = `${StringPrototypeSlice(value, 0, 25)}...`);
       if (StringPrototypeIndexOf(value, "'") === -1) {

test/common/index.js

@@ -855,8 +855,9 @@ function invalidArgTypeHelper(input) {
     return ` Received function ${input.name}`;
   }
   if (typeof input === 'object') {
-    if (input.constructor?.name) {
-      return ` Received an instance of ${input.constructor.name}`;
+    const name = input.constructor?.name;
+    if (typeof name === 'string' && name !== '') {
+      return ` Received an instance of ${name}`;
     }
     return ` Received ${inspect(input, { depth: -1 })}`;
   }

test/parallel/test-error-value-type-detection.mjs

@@ -209,3 +209,58 @@ assert.strictEqual(
   determineSpecificType(new WeakSet()),
   'an instance of WeakSet',
 );
+
+// Anonymous classes have an empty `name`, so the value is inspected instead.
+// `inspect(..., { depth: -1 })` prints this one as `{}` because it has no own
+// properties; the values below carry a `constructor` property of their own and
+// print as `[Object]`.
+assert.strictEqual(
+  determineSpecificType(new (class {})()),
+  '{}',
+);
+
+// `constructor` is an ordinary, user-controlled property that need not be a
+// function. Describing such a value must not throw.
+assert.strictEqual(
+  determineSpecificType(JSON.parse('{"constructor": 5}')),
+  '[Object]',
+);
+
+assert.strictEqual(
+  determineSpecificType({ constructor: { name: '' } }),
+  '[Object]',
+);
+
+// A `constructor.name` that is not a usable string must not be interpolated
+// into the message.
+assert.strictEqual(
+  determineSpecificType({ constructor: { name: Symbol('x') } }),
+  '[Object]',
+);
+
+assert.strictEqual(
+  determineSpecificType({ constructor: { name: 42 } }),
+  '[Object]',
+);
+
+// `constructor` and its `name` are each read once. Both may be accessors, so
+// repeated reads are observable, and a `name` validated by one read and
+// interpolated from another need not be the same value twice.
+let constructorReads = 0;
+let nameReads = 0;
+const named = {
+  get constructor() {
+    constructorReads++;
+    return { get name() { nameReads++; return 'Foo'; } };
+  },
+};
+assert.strictEqual(determineSpecificType(named), 'an instance of Foo');
+assert.strictEqual(constructorReads, 1);
+assert.strictEqual(nameReads, 1);
+
+// Building the error must not fail when `constructor` is a truthy primitive.
+assert.strictEqual(
+  new errorsModule.codes.ERR_INVALID_ARG_TYPE(
+    'arg', 'string', JSON.parse('{"constructor": 5}')).code,
+  'ERR_INVALID_ARG_TYPE',
+);