util: restrict custom inspect function + vm.Context interaction #33690

addaleax · 2020-06-01T22:48:48Z

When util.inspect() is called on an object with a custom inspect
function, and that object is from a different vm.Context,
that function will not receive any arguments that access
context-specific data anymore.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

When `util.inspect()` is called on an object with a custom inspect function, and that object is from a different `vm.Context`, that function will not receive any arguments that access context-specific data anymore.

nodejs-github-bot · 2020-06-02T09:37:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31696/

BridgeAR

While it's indeed surprising that the API could in theory pass through the other contexts prototypes it's not something we guard against anywhere else and the VM module is explicitly not a sandbox for security. I wonder if this is really required therefore.

BridgeAR · 2020-06-03T10:44:02Z

lib/internal/util/inspect.js

+    for (const key of ObjectKeys(ret)) {
+      if ((typeof ret[key] === 'object' || typeof ret[key] === 'function') &&
+          ret[key] !== null) {
+        delete ret[key];


What about cloning objects and to set the prototype to the destination's Object prototype? The returned object and stylize function could also be set to the destination's Object and Function prototype.

I’ve thought about that, but it’s not trivial to determine what the Object prototype is in that case.

Something like that should work well enough:

let proto = maybeCustom while(true) { const next = Object.getPrototypeOf(proto) if (next === null) { if (proto === maybeCustom) proto = null break } proto = next }

There is no guarantee that it's actually the Object prototype but it should be. It would also be possible to check for the constructor name (which is also not "reliable") but I would just use this.

I would prefer to have consistent behavior here rather than to take a guess at the correct prototype.

The behavior might actually cause issues inside of the vm context: we return the object prototype in the "regular" context and return a null prototype otherwise. The code inside of the VM might not be aware of that and expect an object as prototype and use e.g., hasOwnProperty. Using this pattern should work in all cases where the user did not actively manipulate the passed through functions prototype and that is very unlikely.

Right, I understand that problem. I still think consistency is more important than the problems that could occur from mis-guessing the prototype.

lib/internal/util/inspect.js

BridgeAR · 2020-06-04T00:17:49Z

lib/internal/util/inspect.js

+        stylized = ctx.stylize(value, flavour);
+      } catch {}
+
+      if (typeof stylized !== 'string') return value;


~~Is this required? The current stylize function does not check for strings and I would keep those aligned (especially, since it's not something supported to do).~~

We should probably only filter objects and functions.

Isn’t the purpose of .stylize() to transform strings into other strings?

Yes but this function is not filtering anything in the "regular context". I would try to keep the behavior aligned (and it's not a supported function anyway).

Yes but this function is not filtering anything in the "regular context". I would try to keep the behavior aligned

I can do this if you feel strongly about it, but imo the function is broken anyway if it does not return a string.

and it's not a supported function anyway.

It’s used in examples in the documentation, so I think it qualifies as officially supported.

(Also, practically speaking, I don’t think anybody is going to override this function with a custom one.)

It’s used in examples in the documentation, so I think it qualifies as officially supported.

Thanks for pointing that out (even though there's no description of what it would do). I was not aware of that.

practically speaking, I don’t think anybody is going to override this function with a custom one.

The guard here would never be triggered with the internal stylize function.

the function is broken anyway if it does not return a string.

Absolutely. It would either trigger an error in such cases or it changes the printed output to the stringified value. It would therefore align the behavior more closely by using my suggestion.

practically speaking, I don’t think anybody is going to override this function with a custom one.

The guard here would never be triggered with the internal stylize function.

See the example above.

the function is broken anyway if it does not return a string.

Absolutely. It would either trigger an error in such cases or it changes the printed output to the stringified value. It would therefore align the behavior more closely by using my suggestion.

I can add stringification here as well, inside the try/catch. If I understand correctly, that is your main concern? I don’t see any issue with that.

Okay, I’ve implemented that.

nodejs-github-bot · 2020-06-06T15:34:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31752/

…eraction

nodejs-github-bot · 2020-06-08T18:43:51Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31782/

nodejs-github-bot · 2020-06-08T21:07:47Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31786/

nodejs-github-bot · 2020-06-11T16:31:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31836/

addaleax · 2020-06-11T19:44:59Z

Landed in 58bae4d

When `util.inspect()` is called on an object with a custom inspect function, and that object is from a different `vm.Context`, that function will not receive any arguments that access context-specific data anymore. PR-URL: #33690 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

util: restrict custom inspect function + vm.Context interaction

b5bd87c

When `util.inspect()` is called on an object with a custom inspect function, and that object is from a different `vm.Context`, that function will not receive any arguments that access context-specific data anymore.

nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Jun 1, 2020

fixup! util: restrict custom inspect function + vm.Context interaction

583c1bd

jasnell approved these changes Jun 1, 2020

View reviewed changes

cjihrig approved these changes Jun 2, 2020

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 2, 2020

BridgeAR reviewed Jun 4, 2020

View reviewed changes

fixup! util: restrict custom inspect function + vm.Context interaction

8544be7

fixup! fixup! util: restrict custom inspect function + vm.Context int…

7b0b27b

…eraction

This was referenced Jun 9, 2020

CI Relability 2020-06-09 JaneaSystems/nodejs-reliability#9

Open

CI Relability 2020-06-10 JaneaSystems/nodejs-reliability#10

Open

addaleax closed this Jun 11, 2020

addaleax deleted the util-inspect-context branch June 11, 2020 19:45

github-actions bot mentioned this pull request Jun 18, 2020

CI Relability 2020-06-18 JaneaSystems/nodejs-reliability#11

Open

18 tasks

MylesBorins mentioned this pull request Jul 14, 2020

v14.6.0 proposal #34371

Merged

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

aduh95 mentioned this pull request Oct 6, 2020

doc: fix util.inspect history #35528

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: restrict custom inspect function + vm.Context interaction #33690

util: restrict custom inspect function + vm.Context interaction #33690

addaleax commented Jun 1, 2020

nodejs-github-bot commented Jun 2, 2020

BridgeAR left a comment

BridgeAR Jun 3, 2020

addaleax Jun 4, 2020

BridgeAR Jun 4, 2020

addaleax Jun 4, 2020

BridgeAR Jun 4, 2020

addaleax Jun 4, 2020

BridgeAR Jun 4, 2020 •

edited

addaleax Jun 4, 2020

BridgeAR Jun 4, 2020

addaleax Jun 4, 2020

addaleax Jun 4, 2020

BridgeAR Jun 4, 2020

addaleax Jun 4, 2020

addaleax Jun 6, 2020

nodejs-github-bot commented Jun 6, 2020

nodejs-github-bot commented Jun 8, 2020

nodejs-github-bot commented Jun 8, 2020

nodejs-github-bot commented Jun 11, 2020

addaleax commented Jun 11, 2020

util: restrict custom inspect function + vm.Context interaction #33690

util: restrict custom inspect function + vm.Context interaction #33690

Conversation

addaleax commented Jun 1, 2020

Checklist

nodejs-github-bot commented Jun 2, 2020

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR Jun 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jun 6, 2020

nodejs-github-bot commented Jun 8, 2020

nodejs-github-bot commented Jun 8, 2020

nodejs-github-bot commented Jun 11, 2020

addaleax commented Jun 11, 2020

BridgeAR Jun 4, 2020 •

edited