Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
addaleax wants to merge 4 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b5bd87c
util: restrict custom inspect function + vm.Context interaction
addaleax Jun 1, 2020
583c1bd
fixup! util: restrict custom inspect function + vm.Context interaction
addaleax Jun 1, 2020
8544be7
fixup! util: restrict custom inspect function + vm.Context interaction
addaleax Jun 6, 2020
7b0b27b
fixup! fixup! util: restrict custom inspect function + vm.Context int…
addaleax Jun 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixup! util: restrict custom inspect function + vm.Context interaction
  • Loading branch information
@addaleax
addaleax committed Jun 6, 2020
commit 8544be7df300e012b2abfbc509a7b8576ab8ea5c
2 changes: 1 addition & 1 deletion lib/internal/util/inspect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ function getUserOptions(ctx, isCrossContext) {
ret.stylize = ObjectSetPrototypeOf((value, flavour) => {
let stylized;
try {
stylized = ctx.stylize(value, flavour);
stylized = `${ctx.stylize(value, flavour)}`;
} catch {}
Comment thread
BridgeAR marked this conversation as resolved.

if (typeof stylized !== 'string') return value;
Copy link
Copy Markdown
Member

@BridgeAR BridgeAR Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@addaleax addaleax Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@BridgeAR BridgeAR Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Member Author

@addaleax addaleax Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@addaleax addaleax Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@BridgeAR BridgeAR Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@addaleax addaleax Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@addaleax addaleax Jun 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I’ve implemented that.

Expand Down