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

node: --no-dom-globals CLI flag #5853

Closed
wants to merge 1 commit into from
Closed

Conversation

indutny
Copy link
Member

@indutny indutny commented Mar 23, 2016

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

Please provide affected core subsystem(s) (like buffer, cluster, crypto, etc)

Description of change

Introduce --no-dom-globals CLI flag. With this flag set, following
globals won't be exported:

  • setTimeout, clearTimeout, setInterval, clearInterval,
    setImmediate, clearImmediate
  • console

These are provided by the DOM implementation in browser, so the
--no-dom-globals flag may be helpful when embedding node.js within
chromium/webkit.

Inspired-By: atom/node@82e10ce

@indutny
Copy link
Member Author

indutny commented Mar 23, 2016

Hey @zcbenz ! I just decided to land some of the commits that you did in atom/node.

May I ask you what was the reasoning behind atom/node@82e10ce#diff-6ff379484cbabad48301d485db111c08R44 ? Would there be any problem for atom, if we won't hide them too?

@mscdex mscdex added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 23, 2016
@Fishrock123
Copy link
Contributor

Also requires flag docs in doc/api/cli.markdown. It's duplication, but each source requires changes that make it impractical to just generate. :)

@@ -3311,6 +3317,9 @@ static void PrintHelp() {
" --trace-deprecation show stack traces on deprecations\n"
" --throw-deprecation throw an exception anytime a deprecated "
"function is used\n"
" --no-dom-globals do not export setTimeout, console and other\n"
" global methods/object which are provided by "
"the DOM\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Spacing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really. There is no \n on previous line.

@Fishrock123
Copy link
Contributor

Maybe --no-browser-globals would be a better naming?


child.exec(nodejs +
' --no-dom-globals --eval "process.stdout.write(typeof setTimeout)"',
function(err, stdout, stderr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you assert.ifError(err) for completeness. And maybe change equal() to strictEqual().

@rvagg
Copy link
Member

rvagg commented Mar 23, 2016

node --break-everything-that-ever-worked-yolo perhaps?

since the target is embedders, perhaps there's a better way of doing this than using a cmdline flag where its usefulness is much more limited?

@indutny
Copy link
Member Author

indutny commented Mar 23, 2016

@rvagg like what?

@indutny
Copy link
Member Author

indutny commented Mar 23, 2016

@Fishrock123 ack cli.markdown

@indutny
Copy link
Member Author

indutny commented Mar 23, 2016

@jasnell
Copy link
Member

jasnell commented Mar 23, 2016

I tend to agree with @rvagg on this... it seems like there ought to be a better way. If this is for embedders, for instance, then perhaps a programmatic configuration hook that allows the builtins to be switched off or replaced? Not sure I see the value of having this as a command line option.

@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 23, 2016
@rvagg
Copy link
Member

rvagg commented Mar 23, 2016

How about we put it in configure? It's not likely to be something you'd want to turn on and off with the same build, you're either building it for this or not. --without-browser-globals -> node.gyp -> NODE_WITHOUT_BROWSER_GLOBALS=1 -> 💥

@Fishrock123
Copy link
Contributor

cc also @rogerwang of nw.js, since I'm sure they also run into this.

@rogerwang
Copy link
Member

Thanks for CCing. It looks good to me either way.

@jasnell
Copy link
Member

jasnell commented Mar 23, 2016

A configure option is a great alternative. Would love to here from @zcbenz tho as to whether that would address the original issue.

@indutny
Copy link
Member Author

indutny commented Mar 23, 2016

Yep, I agree with configure option. Will commit an update in a bit.

@Fishrock123
Copy link
Contributor

@jasnell Pretty sure it would. I was the one who originally suggested a CLI option.

@indutny
Copy link
Member Author

indutny commented Mar 23, 2016

Ok, pushed an update. PTAL

@jasnell
Copy link
Member

jasnell commented Mar 23, 2016

LGTM but I do think it needs to be clear that running in this mode is not officially supported in core (similarly to how we treat building without openssl)

Introduce `--no-browser-globals` configure flag. With this flag set, following
globals won't be exported:

- `setTimeout`, `clearTimeout`, `setInterval`, `clearInterval`,
  `setImmediate`, `clearImmediate`
- `console`

These are provided by the DOM implementation in browser, so the
`--no-browser-globals` flag may be helpful when embedding node.js within
chromium/webkit.

Inspired-By: atom/node@82e10ce
@indutny
Copy link
Member Author

indutny commented Mar 23, 2016

@indutny
Copy link
Member Author

indutny commented Mar 23, 2016

@jasnell thank you, pushed an update! PTAL

@zcbenz PTAL too, it doesn't hide some other features, and I am not sure why you needed to hide them in atom/node . Thanks!

@jasnell
Copy link
Member

jasnell commented Mar 23, 2016

LGTM

@Fishrock123
Copy link
Contributor

@indutny The other globals don't conflict with browser ones. :)

@indutny
Copy link
Member Author

indutny commented Mar 24, 2016

I know, but the original atom commit has more stuff in removed in it.

@rvagg
Copy link
Member

rvagg commented Mar 24, 2016

lgtm but would really like to have @zcbenz weigh in before merging

@zcbenz
Copy link
Contributor

zcbenz commented Mar 26, 2016

Hey @zcbenz ! I just decided to land some of the commits that you did in atom/node.

This is awesome, thanks for doing this! And this PR looks good to me.

May I ask you what was the reasoning behind atom/node@82e10ce#diff-6ff379484cbabad48301d485db111c08R44 ? Would there be any problem for atom, if we won't hide them too?

In ancient versions of Node.js those lines used to cause troubles, I think it is good to keep them untouched now.

@indutny
Copy link
Member Author

indutny commented Mar 26, 2016

Great, landing then! @zcbenz thank you! I'm going to port some of the other commits from your repo in one way or another, hope it will simplify node.js update process for electron!

@indutny
Copy link
Member Author

indutny commented Mar 26, 2016

Landed in 8363ede, thank you!

@indutny indutny closed this Mar 26, 2016
@indutny indutny deleted the feature/atom-1 branch March 26, 2016 01:04
indutny added a commit that referenced this pull request Mar 26, 2016
Introduce `--no-browser-globals` configure flag. With this flag set, following
globals won't be exported:

- `setTimeout`, `clearTimeout`, `setInterval`, `clearInterval`,
  `setImmediate`, `clearImmediate`
- `console`

These are provided by the DOM implementation in browser, so the
`--no-browser-globals` flag may be helpful when embedding node.js within
chromium/webkit.

Inspired-By: atom/node@82e10ce
PR-URL: #5853
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
Introduce `--no-browser-globals` configure flag. With this flag set, following
globals won't be exported:

- `setTimeout`, `clearTimeout`, `setInterval`, `clearInterval`,
  `setImmediate`, `clearImmediate`
- `console`

These are provided by the DOM implementation in browser, so the
`--no-browser-globals` flag may be helpful when embedding node.js within
chromium/webkit.

Inspired-By: atom/node@82e10ce
PR-URL: #5853
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
evanlucas added a commit that referenced this pull request Mar 31, 2016
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. (Forrest L Norvell)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
Introduce `--no-browser-globals` configure flag. With this flag set, following
globals won't be exported:

- `setTimeout`, `clearTimeout`, `setInterval`, `clearInterval`,
  `setImmediate`, `clearImmediate`
- `console`

These are provided by the DOM implementation in browser, so the
`--no-browser-globals` flag may be helpful when embedding node.js within
chromium/webkit.

Inspired-By: atom/node@82e10ce
PR-URL: #5853
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
evanlucas added a commit that referenced this pull request Mar 31, 2016
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. Fixes a security flaw in the use of
authentication tokens in HTTP requests that would allow an attacker to
set up a server that could collect tokens from users of the command-line
interface. Authentication tokens have previously been sent with every
request made by the CLI for logged-in users, regardless of the destination
of the request. This update fixes this by only including those tokens
for requests made against the registry or registries used for the
current install. (Forrest L Norvell)
[npm#6](npm#6)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)
evanlucas added a commit that referenced this pull request Apr 1, 2016
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. Fixes a security flaw in the use of
authentication tokens in HTTP requests that would allow an attacker to
set up a server that could collect tokens from users of the command-line
interface. Authentication tokens have previously been sent with every
request made by the CLI for logged-in users, regardless of the destination
of the request. This update fixes this by only including those tokens
for requests made against the registry or registries used for the
current install. (Forrest L Norvell)
[npm#6](npm#6)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)
evanlucas added a commit that referenced this pull request Apr 1, 2016
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. Fixes a security flaw in the use of
authentication tokens in HTTP requests that would allow an attacker to
set up a server that could collect tokens from users of the command-line
interface. Authentication tokens have previously been sent with every
request made by the CLI for logged-in users, regardless of the destination
of the request. This update fixes this by only including those tokens
for requests made against the registry or registries used for the
current install. (Forrest L Norvell)
[npm#6](npm#6)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)

PR-URL: #5970
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants