Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util,console: guard against overwritten util functions #13011

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented May 13, 2017

Some functions in util and console will happily invoke other functions
in util that have been overridden. The internal use of
these functions is an implementation detail and users should not be able
to indirectly affect them this way.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

util console

Some functions in util and console will happily invoke other functions
in `util` that have been overridden. The internal use of
these functions is an implementation detail and users should not be able
to indirectly affect them this way.
@nodejs-github-bot nodejs-github-bot added console Issues and PRs related to the console subsystem. util Issues and PRs related to the built-in util module. labels May 13, 2017
@mscdex
Copy link
Contributor

mscdex commented May 13, 2017

@addaleax
Copy link
Member

The internal use of these functions is an implementation detail

I don’t think that’s actually true… If I was overriding util.format or util.inspect, I would definitely expect the console.* methods to pick that up, too.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

1 Question
1 Licese comment

@@ -0,0 +1,92 @@
// Copyright Joyent, Inc. and other Node contributors.
Copy link
Contributor

Choose a reason for hiding this comment

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

should it be

Copyright Node.js contributors. All rights reserved.
SPDX-License-Identifier: MIT

As per #10599 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Since most of the test code here is copied nearly verbatim from another test, I think keeping that test's license is appropriate. I added a few lines, but that's it. I made it a separate test to avoid having to wrestle with side effects from this test affecting other tests.

@@ -103,7 +103,7 @@ function write(ignoreErrors, stream, string, errorhandler) {
Console.prototype.log = function log(...args) {
write(this._ignoreErrors,
this._stdout,
`${util.format.apply(null, args)}\n`,
`${format.apply(null, args)}\n`,
Copy link
Contributor

Choose a reason for hiding this comment

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

[question] if args is a restArgs, why not just spread them?

@refack
Copy link
Contributor

refack commented May 13, 2017

RE CI: ignore fails on macOS & freeBSD, we're fixing that (#12964)

I'm -1 on this. IMHO too deep down the slippery slope... (#12960 (comment))
I'll do a variation on this PR, tell me what you think...

@Trott
Copy link
Member Author

Trott commented May 13, 2017

If I was overriding util.format or util.inspect, I would definitely expect the console.* methods to pick that up, too.

@addaleax The docs agree with you. They state that console.* implicitly call into util.inspect().

I'm going to close this, but if anyone thinks it's worth further consideration, please comment and I'll reopen.

/cc @daurnimator just in case this is relevant to somewhat similar issues they've raised elsewhere

@Trott Trott closed this May 13, 2017
@daurnimator
Copy link

@Trott no this change is unrelated to the issues I've raised.

@Trott Trott deleted the simian branch January 13, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants