console: prevent constructing console methods by Hakerh400 · Pull Request #26096 · nodejs/node

Hakerh400 · 2019-02-14T10:20:03Z

This PR defined all console methods (except the ones intended for internal use) as methods rather than constructible functions. It prevents constructing methods of console instance, except for the global console (opened as separate PR as per #25987 (comment)).

Checklist

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

joyeecheung · 2019-02-14T10:32:53Z

Can you add a test?
Also, I don't think the current patch fixes the issue for the global console, as all console methods are wrapped using consoleCall unconditionally, so to fix the global console, the FunctionTemplate for that method in inspector::Initialize needs to use v8::ConstructorBehavior::kThrow.

node/src/inspector_js_api.cc

Lines 278 to 279 in de2bdfb

 env->SetMethod(target, "consoleCall", InspectorConsoleCall); 

 env->SetMethod(

Hakerh400 · 2019-02-14T10:40:34Z

Can you add a test?

Sure.

Also, I don't think the current patch fixes the issue for the global console

It's mentioned in the PR description: "It prevents constructing methods of console instance, except for the global console". Please read the relevant discussion in #25987. I will open separate PR for ConstructorBehavior::kThrow, since it affect many other things.

joyeecheung · 2019-02-14T11:00:18Z

@Hakerh400

It's mentioned in the PR description: "It prevents constructing methods of console instance, except for the global console".

Sorry for not reading the PR description carefully *_*

I will open separate PR for ConstructorBehavior::kThrow, since it affect many other things.

https://github.com/Hakerh400/node/commit/f084076963c7cdb0dbdcf8ad281c557021f50fc1 affects many other things because the behavior change is done in env-inl.h which gets reused by other internal code - I don't think we can simply replace that wholesale though, that could break a lot of hidden use cases. You can fix the console call without affecting others by manually instantiating a FunctionTemplate in inspector::Initialize instead of using env-SetMethod

Hakerh400 · 2019-02-14T13:20:23Z

Thanks. Applied the suggestion & added a test.

test/parallel/test-console-methods.js

TimothyGu · 2019-02-16T02:12:20Z

src/inspector_js_api.cc

This could probably use FIXED_ONE_BYTE_STRING

maybe an env.h string?

Added FIXED_ONE_BYTE_STRING for consistency with conn_str from inspector::Initialize

joyeecheung

LGTM with a nit

joyeecheung · 2019-02-16T06:21:00Z

lib/internal/console/constructor.js

Can you put this near the end of the file and keep most of the implementation closer to where they used to be to preserve more git blame history?

Added two new lines to align some diffs. All methods are in the same order as before, but console.table pretty much destroys the rest. Not sure if that is fixable.

Ref: #25987

Hakerh400 · 2019-02-28T23:33:59Z

Rebased and resolved the conflict that has just appeared.
BTW, is anything pending here? This PR has been open for two weeks...

addaleax · 2019-03-01T10:10:23Z

@Hakerh400 There wasn’t much activity here because we had a security release being prepared, meaning that e.g. CI only had limited availability.

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

addaleax · 2019-03-01T23:11:33Z

Landed in e9ed6b9, thanks for the PR! 🎉

Ref: #25987 PR-URL: #26096 Refs: #25987 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

addaleax · 2019-03-02T13:39:07Z

Should this be backported to v11.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Hakerh400 · 2019-03-02T22:34:35Z

Should this be backported to v11.x-staging?

To whom the question is directed? If the general opinion is biased towards backporting, I'll open PR.

Ref: #25987 PR-URL: #26096 Refs: #25987 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

BridgeAR · 2019-03-04T23:27:59Z

I backported this directly to the staging branch.

BridgeAR · 2019-03-04T23:29:56Z

@Hakerh400 most of the time the person who originally opened the PR is also the best to backport it and to judge if it makes sense to backport something. But anybody could come along and just do that, so it's a generic question about how people feel about backporting something.

nodejs-github-bot added the console Issues and PRs related to the console subsystem. label Feb 14, 2019

jdalton reviewed Feb 15, 2019

View reviewed changes

test/parallel/test-console-methods.js Outdated Show resolved Hide resolved

TimothyGu reviewed Feb 16, 2019

View reviewed changes

joyeecheung approved these changes Feb 16, 2019

View reviewed changes

gabrielschulhof approved these changes Feb 20, 2019

View reviewed changes

console: prevent constructing console methods

d767f2b

Ref: #25987

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

addaleax closed this Mar 1, 2019

addaleax pushed a commit that referenced this pull request Mar 1, 2019

console: prevent constructing console methods

e9ed6b9

Ref: #25987 PR-URL: #26096 Refs: #25987 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

Hakerh400 deleted the console branch March 2, 2019 12:12

addaleax added the backport-requested-v11.x label Mar 2, 2019

BridgeAR pushed a commit that referenced this pull request Mar 4, 2019

console: prevent constructing console methods

a70bafb

Ref: #25987 PR-URL: #26096 Refs: #25987 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

BridgeAR removed the backport-requested-v11.x label Mar 4, 2019

BridgeAR mentioned this pull request Mar 4, 2019

v11.11.0 proposal #26322

Merged

Hakerh400 mentioned this pull request Mar 16, 2019

src: disallow constructor behaviour for native methods #26700

Closed

3 tasks

Uh oh!

Conversation

Hakerh400 commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

joyeecheung commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hakerh400 commented Feb 14, 2019

Uh oh!

joyeecheung commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hakerh400 commented Feb 14, 2019

Uh oh!

Uh oh!

TimothyGu Feb 16, 2019

Choose a reason for hiding this comment

Uh oh!

devsnek Feb 16, 2019

Choose a reason for hiding this comment

Uh oh!

Hakerh400 Feb 16, 2019

Choose a reason for hiding this comment

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung Feb 16, 2019

Choose a reason for hiding this comment

Uh oh!

Hakerh400 Feb 16, 2019

Choose a reason for hiding this comment

Uh oh!

Hakerh400 commented Feb 28, 2019

Uh oh!

addaleax commented Mar 1, 2019

Uh oh!

addaleax commented Mar 1, 2019

Uh oh!

addaleax commented Mar 2, 2019

Uh oh!

Hakerh400 commented Mar 2, 2019

Uh oh!

BridgeAR commented Mar 4, 2019

Uh oh!

BridgeAR commented Mar 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Hakerh400 commented Feb 14, 2019 •

edited

Loading

joyeecheung commented Feb 14, 2019 •

edited

Loading

joyeecheung commented Feb 14, 2019 •

edited

Loading