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

(v6.x backport) src: allow CLI args in env with NODE_OPTIONS #12677

Closed
wants to merge 9 commits into from
Closed

(v6.x backport) src: allow CLI args in env with NODE_OPTIONS #12677

wants to merge 9 commits into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Apr 26, 2017 •
edited
Loading

Not all CLI options are supported, those that are problematic from a
security or implementation point of view are disallowed, as are ones
that are inappropriate (for example, -e, -p, --i), or that only make
sense when changed with code changes (such as options that change the
javascript syntax or add new APIs).

PR-URL: #12028
Reviewed-By: James M Snell [email protected]
Reviewed-By: Michael Dawson [email protected]
Reviewed-By: Refael Ackermann [email protected]
Reviewed-By: Gibson Fahnestock [email protected]
Reviewed-By: Bradley Farias [email protected]

EDIT: also pushed #13002 and #13172 onto this PR, so it combines all the NODE_OPTIONS PRs. Note that they are all released as of 8.0.0.

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
Affected core subsystem(s)

@sam-github sam-github added lts-agenda semver-minor PRs that contain new features and should be released in the next minor version. labels Apr 26, 2017
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. v6.x windows Issues and PRs related to the Windows platform. labels Apr 26, 2017
@sam-github sam-github mentioned this pull request Apr 26, 2017
Closed
4 tasks
@mscdex mscdex added cli Issues and PRs related to the Node.js command line interface. and removed c++ Issues and PRs that require attention from people who are familiar with C++. lts-watch-v6.x windows Issues and PRs related to the Windows platform. labels Apr 26, 2017
@sam-github sam-github mentioned this pull request May 9, 2017
Closed
@gibfahn
Copy link
Member

gibfahn commented May 10, 2017 •
edited
Loading

@sam-github needs a rebase (but this probably needs to bake in v7.x first, so maybe leave it)

@sam-github
Copy link
Contributor Author

rebased without conflicts

@MylesBorins
Copy link
Contributor

@sam-github SafeGetenv() is now in v6.x-staging

feel free to rework this PR

@sam-github
Copy link
Contributor Author

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

@sam-github
Copy link
Contributor Author

@nodejs/lts, ci was all green, can I land this?

@gibfahn
Copy link
Member

gibfahn commented May 26, 2017

cc/ @MylesBorins I can't remember whether we said it was better to wait till the release goes out and v6.x-staging is rebased.

@MylesBorins
Copy link
Contributor

@gibfahn with this being a minor it likely shouldn't land until our next minor release

This was referenced May 30, 2017
Merged
Merged
@sam-github
Copy link
Contributor Author

@MylesBorins done

mhdawson
mhdawson approved these changes Oct 13, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

MylesBorins pushed a commit that referenced this pull request Oct 16, 2017
@jasnell @MylesBorins
process: add --redirect-warnings command line argument
a10b476 
The --redirect-warnings command line argument allows process warnings
to be written to a specified file rather than printed to stderr.

Also adds an equivalent NODE_REDIRECT_WARNINGS environment variable.

If the specified file cannot be opened or written to for any reason,
the argument is ignored and the warning is printed to stderr.

If the file already exists, it will be appended to.

Backport-PR-URL: #12677
PR-URL: #10116
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Michal Zasso <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 16, 2017
@sam-github @MylesBorins
src: use SafeGetenv() for NODE_REDIRECT_WARNINGS
ef72279 
Mutations of the environment can invalidate pointers to environment
variables, so make `secure_getenv()` copy them out instead of returning
pointers.

This is the part of #11051 that
applies to be11fb4.
This part wasn't backported to 6.x when #11051 was backported
because the semver-minor introduction
of NODE_REDIRECT_WARNINGS hadn't been backported yet. Now that the
env var is backported, this last bit of #11051 is needed.

PR-URL: #12677
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 16, 2017
@sam-github @MylesBorins
src: use a std::vector for preload_modules
69c1329 
A dynamically allocated array was being used, simplify the memory
management by using std::vector.

Backport-PR-URL: #12677
PR-URL: #12241
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 16, 2017
@sam-github @MylesBorins
src: allow CLI args in env with NODE_OPTIONS
2871340 
Not all CLI options are supported, those that are problematic from a
security or implementation point of view are disallowed, as are ones
that are inappropriate (for example, -e, -p, --i), or that only make
sense when changed with code changes (such as options that change the
javascript syntax or add new APIs).

Backport-PR-URL: #12677
PR-URL: #12028
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 16, 2017
@sam-github @MylesBorins
src: whitelist new options for NODE_OPTIONS
9c0f734 
Add --debug-*, --napi-modules

Remove --prof-process, like -p and -e, it causes node to do something
other than run node js scripts.

Backport-PR-URL: #12677
PR-URL: #13002
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 16, 2017
@sam-github @MylesBorins
src: allow --tls-cipher-list in NODE_OPTIONS
aa5c58c 
Backport-PR-URL: #12677
PR-URL: #13172
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 16, 2017
@danbev @MylesBorins
test: chdir before running test-cli-node-options
03a12cf 
When test-cli-node-options is run it uses the --trace-events-enabled
option which generates a file named node_trace.1.log. This commit
changes the working directory to the test tmp directory to avoid this
file being created in the project root.

Backport-PR-URL: #12677
PR-URL: #12660
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 16, 2017
@danbev @MylesBorins
test: add hasCrypto check to test-cli-node-options
c98baa4 
Currently when configure --without-ssl the test will throw the following
error:
bad option: --use-openssl-ca

This commit checks if crypto was enabled and skips the crypto related
tests if that is the case.

Backport-PR-URL: #12677
PR-URL: #12692
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>/
MylesBorins pushed a commit that referenced this pull request Oct 16, 2017
@sam-github @MylesBorins
src: --abort-on-uncaught-exception in NODE_OPTIONS
a76c2ae 
Allow --abort-on-uncaught-exception in NODE_OPTIONS, its useful to
enable for post-mortem debugging.

Backport-PR-URL: #12677
PR-URL: #13932
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

landed in f35071b...a76c2ae

@sam-github
Copy link
Contributor Author

Lovely, thanks @MylesBorins

@sam-github sam-github deleted the backport-12028-to-v6.x branch October 16, 2017 22:44
@MylesBorins MylesBorins mentioned this pull request Oct 17, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@jasnell @MylesBorins
process: add --redirect-warnings command line argument
16802c0 
The --redirect-warnings command line argument allows process warnings
to be written to a specified file rather than printed to stderr.

Also adds an equivalent NODE_REDIRECT_WARNINGS environment variable.

If the specified file cannot be opened or written to for any reason,
the argument is ignored and the warning is printed to stderr.

If the file already exists, it will be appended to.

Backport-PR-URL: #12677
PR-URL: #10116
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Michal Zasso <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@sam-github @MylesBorins
src: use SafeGetenv() for NODE_REDIRECT_WARNINGS
68f698c 
Mutations of the environment can invalidate pointers to environment
variables, so make `secure_getenv()` copy them out instead of returning
pointers.

This is the part of #11051 that
applies to be11fb4.
This part wasn't backported to 6.x when #11051 was backported
because the semver-minor introduction
of NODE_REDIRECT_WARNINGS hadn't been backported yet. Now that the
env var is backported, this last bit of #11051 is needed.

PR-URL: #12677
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@sam-github @MylesBorins
src: use a std::vector for preload_modules
8f42148 
A dynamically allocated array was being used, simplify the memory
management by using std::vector.

Backport-PR-URL: #12677
PR-URL: #12241
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@sam-github @MylesBorins
src: allow CLI args in env with NODE_OPTIONS
dd6ea89 
Not all CLI options are supported, those that are problematic from a
security or implementation point of view are disallowed, as are ones
that are inappropriate (for example, -e, -p, --i), or that only make
sense when changed with code changes (such as options that change the
javascript syntax or add new APIs).

Backport-PR-URL: #12677
PR-URL: #12028
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@sam-github @MylesBorins
src: whitelist new options for NODE_OPTIONS
12b66e6 
Add --debug-*, --napi-modules

Remove --prof-process, like -p and -e, it causes node to do something
other than run node js scripts.

Backport-PR-URL: #12677
PR-URL: #13002
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@sam-github @MylesBorins
src: allow --tls-cipher-list in NODE_OPTIONS
16f8f9b 
Backport-PR-URL: #12677
PR-URL: #13172
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@danbev @MylesBorins
test: chdir before running test-cli-node-options
ba830f0 
When test-cli-node-options is run it uses the --trace-events-enabled
option which generates a file named node_trace.1.log. This commit
changes the working directory to the test tmp directory to avoid this
file being created in the project root.

Backport-PR-URL: #12677
PR-URL: #12660
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@danbev @MylesBorins
test: add hasCrypto check to test-cli-node-options
dd946c3 
Currently when configure --without-ssl the test will throw the following
error:
bad option: --use-openssl-ca

This commit checks if crypto was enabled and skips the crypto related
tests if that is the case.

Backport-PR-URL: #12677
PR-URL: #12692
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>/
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@sam-github @MylesBorins
src: --abort-on-uncaught-exception in NODE_OPTIONS
cccf5a6 
Allow --abort-on-uncaught-exception in NODE_OPTIONS, its useful to
enable for post-mortem debugging.

Backport-PR-URL: #12677
PR-URL: #13932
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2017
Merged
MylesBorins added a commit that referenced this pull request Nov 6, 2017
@MylesBorins
2017-11-07, Version 6.12.0 'Boron' (LTS)
c6a2ec5 
Notable Changes:

* assert:
  - assert.fail() can now take one or two arguments (Rich Trott)
    #12293
* crypto:
  - add sign/verify support for RSASSA-PSS (Tobias Nießen)
    #11705
* deps:
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu)
    #16691
  - upgrade libuv to 1.15.0 (cjihrig)
    #15745
  - upgrade libuv to 1.14.1 (cjihrig)
    #14866
  - upgrade libuv to 1.13.1 (cjihrig)
    #14117
  - upgrade libuv to 1.12.0 (cjihrig)
    #13306
* fs:
  - Add support for fs.write/fs.writeSync(fd, buffer, cb) and
    fs.write/fs.writeSync(fd, buffer, offset, cb) as documented
    (Andreas Lind) #7856
* inspector:
  - enable --inspect-brk (Refael Ackermann)
    #12615
* process:
  - add --redirect-warnings command line argument (James M Snell)
    #10116
* src:
  - allow CLI args in env with NODE_OPTIONS (Sam Roberts)
    #12028)
  - --abort-on-uncaught-exception in NODE_OPTIONS (Sam Roberts)
    #13932
  - allow --tls-cipher-list in NODE_OPTIONS (Sam Roberts)
    #13172
  - use SafeGetenv() for NODE_REDIRECT_WARNINGS (Sam Roberts)
    #12677
* test:
  - remove common.fail() (Rich Trott)
    #12293

PR-URL: #16263
MylesBorins added a commit that referenced this pull request Nov 7, 2017
@MylesBorins
2017-11-07, Version 6.12.0 'Boron' (LTS)
70dc6d5 
Notable Changes:

* assert:
  - assert.fail() can now take one or two arguments (Rich Trott)
    #12293
* crypto:
  - add sign/verify support for RSASSA-PSS (Tobias Nießen)
    #11705
* deps:
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu)
    #16691
  - upgrade libuv to 1.15.0 (cjihrig)
    #15745
  - upgrade libuv to 1.14.1 (cjihrig)
    #14866
  - upgrade libuv to 1.13.1 (cjihrig)
    #14117
  - upgrade libuv to 1.12.0 (cjihrig)
    #13306
* fs:
  - Add support for fs.write/fs.writeSync(fd, buffer, cb) and
    fs.write/fs.writeSync(fd, buffer, offset, cb) as documented
    (Andreas Lind) #7856
* inspector:
  - enable --inspect-brk (Refael Ackermann)
    #12615
* process:
  - add --redirect-warnings command line argument (James M Snell)
    #10116
* src:
  - allow CLI args in env with NODE_OPTIONS (Sam Roberts)
    #12028)
  - --abort-on-uncaught-exception in NODE_OPTIONS (Sam Roberts)
    #13932
  - allow --tls-cipher-list in NODE_OPTIONS (Sam Roberts)
    #13172
  - use SafeGetenv() for NODE_REDIRECT_WARNINGS (Sam Roberts)
    #12677
* test:
  - remove common.fail() (Rich Trott)
    #12293

PR-URL: #16263
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. cli Issues and PRs related to the Node.js command line interface. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sam-github @gibfahn @MylesBorins @mhdawson @mscdex @nodejs-github-bot @jasnell @danbev