util: prevent leaking inspect internals

[BridgeAR]

Please have a look at the commit messages.

Refs: #24765

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

[nodejs-github-bot]

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1901/pipeline

[BridgeAR]

CI https://ci.nodejs.org/job/node-test-pull-request/19433/ ✔️
CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1679/

[jdalton]

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?

[jdalton]

Sounds good!

[BridgeAR]

@jdalton I rebased and kept stylize as we discussed. PTAL.

@nodejs/tsc PTAL this is semver-major.

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

ok

[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]

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/19540/ ✔️

[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]

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.

[BridgeAR]

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

[addaleax]

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.

[BridgeAR]

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.

[BridgeAR]

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?

[BridgeAR]

Ping

[addaleax]

The first 2 commits LGTM, by the way

[BridgeAR]

I addressed the comments and pushed the fixes in separate commits so it's easier to review the actual change. I rebased while doing that so the commits stay in order how they should be applied / squashed.

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

[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.<anonymous> (/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]

Rebased. I fixed the CI issue (a commit on master changed behavior here, so I addressed that).

@nodejs/tsc PTAL

CI https://ci.nodejs.org/job/node-test-pull-request/19614/ ✔️

[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]

Rebased due to conflicts.

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

[addaleax]

Looks good!

[addaleax]

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

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

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

[BridgeAR]

Landed in be78266, 7b67469 🎉