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.

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

Jump to bottom

console: prevent constructing console methods #26096

Closed
wants to merge 1 commit into from
Closed

console: prevent constructing console methods #26096

wants to merge 1 commit into from

Conversation

Hakerh400
Copy link
Contributor

@Hakerh400 Hakerh400 commented Feb 14, 2019 •
edited
Loading

Ref: #25987

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

@nodejs-github-bot nodejs-github-bot added the console Issues and PRs related to the console subsystem. label Feb 14, 2019
@joyeecheung
Copy link
Member

joyeecheung commented Feb 14, 2019 •
edited
Loading

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
Copy link
Contributor Author

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
Copy link
Member

joyeecheung commented Feb 14, 2019 •
edited
Loading

@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
Copy link
Contributor Author

Thanks. Applied the suggestion & added a test.

TimothyGu
TimothyGu reviewed Feb 16, 2019
View reviewed changes
src/inspector_js_api.cc Outdated
const v8::NewStringType type = v8::NewStringType::kInternalized;
v8::Local<v8::String> name_string =
v8::String::NewFromUtf8(env->isolate(), "consoleCall", type)
.ToLocalChecked();
Copy link
Member

Choose a reason for hiding this comment

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

This could probably use FIXED_ONE_BYTE_STRING

Copy link
Member

Choose a reason for hiding this comment

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

maybe an env.h string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added FIXED_ONE_BYTE_STRING for consistency with conn_str from inspector::Initialize

joyeecheung
joyeecheung approved these changes Feb 16, 2019
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM with a nit

lib/internal/console/constructor.js Outdated
};

for (const method of Reflect.ownKeys(consoleMethods))
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@Hakerh400
Copy link
Contributor Author

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

@addaleax
Copy link
Member

addaleax commented Mar 1, 2019

@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 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
Copy link
Member

addaleax commented Mar 1, 2019

Landed in e9ed6b9, thanks for the PR! 🎉

@addaleax addaleax closed this Mar 1, 2019
addaleax pushed a commit that referenced this pull request Mar 1, 2019
@Hakerh400 @addaleax
console: prevent constructing console methods
e9ed6b9 
Ref: #25987

PR-URL: #26096
Refs: #25987
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
@Hakerh400 Hakerh400 deleted the console branch March 2, 2019 12:12
@addaleax
Copy link
Member

addaleax commented Mar 2, 2019

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
Copy link
Contributor Author

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.

BridgeAR pushed a commit that referenced this pull request Mar 4, 2019
@Hakerh400 @BridgeAR
console: prevent constructing console methods
a70bafb 
Ref: #25987

PR-URL: #26096
Refs: #25987
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
@BridgeAR
Copy link
Member

BridgeAR commented Mar 4, 2019

I backported this directly to the staging branch.

@BridgeAR
Copy link
Member

BridgeAR commented Mar 4, 2019

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

@BridgeAR BridgeAR mentioned this pull request Mar 4, 2019
Merged
@Hakerh400 Hakerh400 mentioned this pull request Mar 16, 2019
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devsnek devsnek devsnek left review comments

@TimothyGu TimothyGu TimothyGu left review comments

@jdalton jdalton jdalton left review comments

@joyeecheung joyeecheung joyeecheung approved these changes

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. console Issues and PRs related to the console subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@Hakerh400 @joyeecheung @addaleax @BridgeAR @jdalton @gabrielschulhof @TimothyGu @devsnek @nodejs-github-bot