镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

console/inspect output for functions is misleading in Hardened JS #55924

Description

@gibson042

Version

v22.11.0

Platform

Linux x86_64

Subsystem

util

What steps will reproduce the bug?

const obj = { m(){} };

console.log(obj); // => { m: [Function: m] }

// Mitigate the assignment override mistake for Function.prototype.constructor,
// allowing the prototype to be frozen.
// cf. https://github.com/endojs/endo/blob/b3f0c567/packages/ses/src/enable-property-overrides.js
{
  const { defineProperty, hasOwn } = Object;
  const BuiltinFunction = Function;
  const BuiltinFunctionPrototype = BuiltinFunction.prototype;
  const constructorKey = "constructor";
  Object.defineProperty(BuiltinFunctionPrototype, constructorKey, {
    get() {
      return BuiltinFunction;
    },
    // A setter is not necessary to trigger this bug, but demonstrates the
    // motivation for defining such accessors.
    set(value) {
      if (this === BuiltinFunctionPrototype) throw TypeError();
      if (hasOwn(this, constructorKey)) {
        this.constructor = value;
      } else {
        const desc = { value, writable: true, enumerable: true, configurable: true };
        defineProperty(this, constructorKey, desc);
      }
    },
  });
}

console.log(obj); // => { m: {} }

How often does it reproduce? Is there a required condition?

always

What is the expected behavior? Why is that the expected behavior?

I expect console.log({ m(){} }) output to always indicate that the value for property "m" is a function, even when Function.prototype.constructor is an accessor that won't be invoked.

What do you see instead?

Defining Function.prototype.constructor as an accessor replaces the useful output with text that makes it look like functions are plain objects.

-{ m: [Function: m] }
+{ m: {} }

Additional information

My proposed fix is updating lib/internal/util/inspect.js formatRaw to privilege the "is function" check over "constructor is Object" while preserving their rendering details:

if (constructor === 'Object') {
if (isArgumentsObject(value)) {
braces[0] = '[Arguments] {';
} else if (tag !== '') {
braces[0] = `${getPrefix(constructor, tag, 'Object')}{`;
}
if (keys.length === 0 && protoProps === undefined) {
return `${braces[0]}}`;
}
} else if (typeof value === 'function') {
base = getFunctionBase(value, constructor, tag);
if (keys.length === 0 && protoProps === undefined)
return ctx.stylize(base, 'special');

     keys = getKeys(value, ctx.showHidden);
     braces = ['{', '}'];
-    if (constructor === 'Object') {
+    if (typeof value === 'function') {
+      base = getFunctionBase(value, constructor, tag);
+      if (keys.length === 0 && protoProps === undefined)
+        return ctx.stylize(base, 'special');
+    } else if (constructor === 'Object') {
       if (isArgumentsObject(value)) {
         braces[0] = '[Arguments] {';
       } else if (tag !== '') {
         braces[0] = `${getPrefix(constructor, tag, 'Object')}{`;
       }
       if (keys.length === 0 && protoProps === undefined) {
         return `${braces[0]}}`;
       }
-    } else if (typeof value === 'function') {
-      base = getFunctionBase(value, constructor, tag);
-      if (keys.length === 0 && protoProps === undefined)
-        return ctx.stylize(base, 'special');
     } else if (isRegExp(value)) {

Doing so will still affect the console/inspect output, but in a way that no longer fails to indicates functionness:

-{ m: [Function: m] }
+{ m: [Function: m] Object }

(although I am also open to tweaking that as well).

Activity

  1. gibson042 commented on Nov 21, 2024

    @gibson042
    ContributorAuthor

    A similar issue also exists one level down:

    const obj = {};
    
    console.log(obj); // => {}
    
    {
      const BultinObjectPrototype = Object.prototype;
      Object.defineProperty(Object.prototype, "constructor", {
        get: () => BuiltinObjectPrototype,
      });
    }
    
    console.log(obj); // => Object <[Object: null prototype] {}> {}

    Failure to establish a constructor name does not imply a null prototype.

  2. added
    utilIssues and PRs related to the built-in util module.
    on Nov 21, 2024
  3. BridgeAR commented on Nov 21, 2024

    @BridgeAR
    Member

    The way the constructor is replaced is tricky. Accessing the getter would trigger side effects and it's as such impossible to identify the constructor properly. We could explore to improve getConstructorName() to detect these as "unknown".

    The suggestion for fixing functions is something we can do. The current order was for performance reasons, while it's not a big overhead to get the type of an argument.

  4. gibson042 commented on Nov 21, 2024

    @gibson042
    ContributorAuthor

    Thanks, that matches my understanding. Would it make sense for me to open a PR, or is there some process between here and there?

  5. juanarbol commented on Dec 7, 2024

    @juanarbol
    Member

    Thanks, that matches my understanding. Would it make sense for me to open a PR, or is there some process between here and there?

    Go for it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    utilIssues and PRs related to the built-in util module.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions