util: prevent leaking inspect internals by BridgeAR · Pull Request #24971 · nodejs/node (original) (raw)

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.

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

Already on GitHub?Sign in to your account

Conversation33 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

BridgeAR

Please have a look at the commit messages.

Refs: #24765

Checklist

@BridgeAR BridgeAR added the semver-major

PRs that contain breaking changes and should be released in the next major version.

label

Dec 11, 2018

@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added encoding

Issues and PRs related to the TextEncoder and TextDecoder APIs.

errors

Issues and PRs related to JavaScript errors originated in Node.js core.

util

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

labels

Dec 11, 2018

@BridgeAR

devsnek

@BridgeAR BridgeAR added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Dec 12, 2018

jdalton

Choose a reason for hiding this comment

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

I've found stylize useful for custom inspection. Before merging this PR we should take some time to consider if it's being removed we should consider exposing it in some way.

@BridgeAR

@jdalton stylize might be an interesting candidate to make public. I just wonder if I would call this API user friendly or not. It can be powerful.

I suggest that I keep stylize accessible so we are still able to figure out in what way this API should be exposed / not exposed. Is that fine?

@BridgeAR BridgeAR removed the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Dec 14, 2018

@jdalton

@BridgeAR

jdalton

@BridgeAR BridgeAR added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Dec 14, 2018

@jdalton

Thanks for the change. FWIW I also found that options.stylize is documented here in a usage example at least.

@Trott

Windows CI failure has a PR to mark it as flaky that should land today.

Other failure is newly observed on that platform. #25029 (comment)

I'll keep monitoring it and re-run it.

@Trott

@bathos

With one additional tweak there may be no remaining way to leak references to a Proxy’s target/handlers objects:

In lib/internal/util/inspect.js at line 578:

if (value[Symbol.iterator]) { noIterator = false; if (Array.isArray(value)) { //...

value here is possibly a target or handlers object. This appears to be the only place where these values may be still be passed into a function which could be overwritten. I think that can be avoided by grabbing a reference to isArray at module eval time:

const { isArray } = Array;

// ...

if (isArray(value)) {

(I know bnoordhuis said the objective isn’t to prevent malicious tampering, but this seems like a reasonably small adjustment with no apparent downside.)

addaleax

if (inspectDefaultOptions[key] === undefined &&
key !== 'stylize' &&
!(key in inspectDefaultOptions)) {
throw new ERR_INVALID_OPT_KEY(key);

Choose a reason for hiding this comment

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

This particular change seems unrelated, and as far as I can tell, it would mean that when we introduce a new option in the future, that would break on older versions of Node.js as opposed to being silently ignored? If my understanding is correct, I’m -1 on this.

Choose a reason for hiding this comment

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

Without this, the seen array could be overridden (just as well as the indentationLvl and budget). That's not intended. I could also prevent this otherwise but this seemed the most straight forward way and I believe it will also uncover bugs in user implementation with typos.

Therefore I consider this something pretty important.

Using "newer" options in older Node.js versions that have this validation would indeed throw but I don't think this is bad at all. IMO if code is written for a specific Node.js version it should be upwards compatible but not downwards compatible.

Every userland module normally supports a version range from a specific point on (e.g., => 6.x). That's just the same here: it's a feature that can only be used for new enough Node.js versions (it would be => 12.x).

Choose a reason for hiding this comment

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

Therefore I consider this something pretty important.

Then maybe we could do something like you did below, to filter out specific properties that we care about? Or would that be too slow?

IMO if code is written for a specific Node.js version it should be upwards compatible but not downwards compatible.

Yes, I think that’s the part that I disagree with – this PR would mean that we artificially reduce the range of supported versions…

@nodejs/tsc Any opinions? I don’t want this to be blocked just on me.

Choose a reason for hiding this comment

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

Then maybe we could do something like you did below, to filter out specific properties that we care about? Or would that be too slow?

This would prevent the main aspect which I would like to get with it: finding bugs in user implementations by finding typos. Otherwise they would be silently ignored as well. The performance is not that critical here.

this PR would mean that we artificially reduce the range of supported version

I don't think that it's artificially reducing the supported version range. Either something fully works in a version or not. Silently ignoring something could result in unwanted behavior. And isn't this what semver-major is for?
I consider it a breaking change feature.

Choose a reason for hiding this comment

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

After gathering the general feedback that it's probably to late to change this in general, I am uncertain how to address this properly...

I see the following options:

  1. Stop accepting only specific options. Either by silently ignoring specific properties or by throwing an error. This would be inconsistent and seems "random": any other property will be passed through to the user in the custom inspection function.
  2. Split the user options and internal options completely from each other. This requires a significantly bigger code change and an extra argument to be passed around in all functions calls that have something to do with util.inspect. There would also be a performance penalty in case we do not pass through the original options argument from the user but a new options object which is a clone from the current one without the internal properties and even without this, there might be a performance penalty.
  3. For this specific API we keep my implementation and forbid all unknown options but this is an exception which would only apply to util.inspect().

@addaleax @joyeecheung does either of you have another idea or suggestion?

Choose a reason for hiding this comment

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

Ping

addaleax

Choose a reason for hiding this comment

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

The first 2 commits LGTM, by the way

@BridgeAR

@BridgeAR BridgeAR removed the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Dec 16, 2018

@Trott

Relevant CI failures:

04:00:44 not ok 1999 parallel/test-util-inspect 04:00:44 --- 04:00:44 duration_ms: 0.322 04:00:44 severity: fail 04:00:44 exitcode: 1 04:00:44 stack: |- 04:00:44 internal/util/inspect.js:191 04:00:44 throw new ERR_INVALID_OPT_KEY(key); 04:00:44 ^ 04:00:44
04:00:44 TypeError [ERR_INVALID_OPT_KEY]: "budget" is an unknown options key 04:00:44 at inspect (internal/util/inspect.js:191:17) 04:00:44 at getConstructorName (internal/util/inspect.js:350:14) 04:00:44 at formatRaw (internal/util/inspect.js:560:23) 04:00:44 at formatValue (internal/util/inspect.js:554:10) 04:00:44 at Object.inspect (internal/util/inspect.js:199:10) 04:00:44 at Object. (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/test/parallel/test-util-inspect.js:1764:27) 04:00:44 at Module._compile (internal/modules/cjs/loader.js:718:30) 04:00:44 at Object.Module._extensions..js (internal/modules/cjs/loader.js:729:10) 04:00:44 at Module.load (internal/modules/cjs/loader.js:617:32) 04:00:44 at tryModuleLoad (internal/modules/cjs/loader.js:560:12) 04:00:44 ...

@BridgeAR

@joyeecheung

If we are specifically checking the validity of options passed into util.inspect, why this particular method alone? This is similar to the question I have in #24267
I am not opposed to the idea, but I think it would be surprising for users to run into this error in one API but not the others, so unless we have a plan about doing this for all our APIs (and promote that plan in change logs/documentation/some official channels), then for consistency reasons I am -1 on landing changes to on particular API without any further plans for other APIs.

@BridgeAR

addaleax

Choose a reason for hiding this comment

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

Looks good!

@addaleax

@addaleax

CI is green.

I don’t know if this has changed enough since @devsnek’s last review that it would require a re-review?

@BridgeAR

@addaleax there were three commits earlier. I removed one of them and I changed that the style function is still going to be exposed. Other changes should not exist. @devsnek please confirm your LG nevertheless.

This still requires another review from @nodejs/tsc as it's semver-major.

@BridgeAR BridgeAR added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Feb 20, 2019

@Trott

This still requires another review from @nodejs/tsc as it's semver-major.

It seems all right to me and I'll give it an official 👍 if no one else on TSC steps up to do so (or offer an objection). I've been refraining because I feel like others would have a better understanding of the ramifications here. Seems OK to me, though.

EDIT: Awaiting @devsnek confirming their LGTM...

devsnek

Trott

BridgeAR added a commit to BridgeAR/node that referenced this pull request

Feb 28, 2019

@BridgeAR

This makes sure the internal stylize function is not used to render anything and instead just uses the regular inspect function in case of reaching the maximum depth level.

PR-URL: nodejs#24971 Refs: nodejs#24765 Reviewed-By: Gus Caplan me@gus.host Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Rich Trott rtrott@gmail.com

BridgeAR added a commit to BridgeAR/node that referenced this pull request

Feb 28, 2019

@BridgeAR

This prevents leaking of the internal inspect() properties when using a custom inspect function.

It also aligns the indentation to the way it was in v8.0.0 since that changed unintentionally. All strings returned by the custom inspect function will now be indented appropriately to the current depth.

PR-URL: nodejs#24971 Refs: nodejs#24765 Reviewed-By: Gus Caplan me@gus.host Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Rich Trott rtrott@gmail.com

@BridgeAR

BethGriggs added a commit that referenced this pull request

Apr 22, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

This was referenced

Apr 23, 2019

@BridgeAR BridgeAR deleted the prevent-inspet-leakage branch

January 20, 2020 11:53

Labels

author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

encoding

Issues and PRs related to the TextEncoder and TextDecoder APIs.

errors

Issues and PRs related to JavaScript errors originated in Node.js core.

semver-major

PRs that contain breaking changes and should be released in the next major version.

util

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