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

doc: NODE_EXTRA_CA_CERTS is ignored if setuid root #23770

Closed
wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

Fixes: #22081

@nodejs-github-bot nodejs-github-bot added cli Issues and PRs related to the Node.js command line interface. doc Issues and PRs related to the documentations. labels Oct 19, 2018
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Oct 19, 2018

Commit title nit. From the guide:

The first line should... be prefixed with the name of the changed subsystem and start with an imperative verb

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Oct 19, 2018

FWIW: just to be sure that this file for the node --help output need not be updated in this case (it has only brief descriptions):

['NODE_EXTRA_CA_CERTS', { helpText: 'path to additional CA certificates ' +
'file' }],

@richardlau
Copy link
Member

NODE_PATH is also similarly affected -- do we know of any others?

@bnoordhuis
Copy link
Member Author

and start with an imperative verb

I know, but I couldn't fit that in 50 columns. :-) (and it's describing the status quo, not changing it.)

do we know of any others?

$ git grep 'SafeGetenv("' src/
src/env.cc:208:  SafeGetenv("NODE_DEBUG_NATIVE", &debug_cats);
src/node.cc:2527:        SafeGetenv("NODE_PENDING_DEPRECATION", &text) && text[0] == '1';
src/node.cc:2534:        SafeGetenv("NODE_PRESERVE_SYMLINKS", &text) && text[0] == '1';
src/node.cc:2540:        SafeGetenv("NODE_PRESERVE_SYMLINKS_MAIN", &text) && text[0] == '1';
src/node.cc:2544:    SafeGetenv("NODE_REDIRECT_WARNINGS",
src/node.cc:2551:    SafeGetenv("OPENSSL_CONF", openssl_config);
src/node.cc:2557:  if (SafeGetenv("NODE_OPTIONS", &node_options)) {
src/node.cc:2589:    SafeGetenv("NODE_ICU_DATA", &per_process_opts->icu_data_dir);
src/node.cc:2985:    if (SafeGetenv("NODE_EXTRA_CA_CERTS", &extra_ca_certs))

@richardlau
Copy link
Member

I think we also expose SafeGetenv on the internal util module (at least that seems to be how NODE_PATH is affected). (Sorry, on a tablet for the rest of the weekend so cannot run grep on the source tree).

Anyway that is more than a handful of the environment variables being affected. Should we similarly add the same sentences to all of them? Or maybe another section with the sentences and a list of affected variables? (Can be done in a follow up PR -- I'm okay for this to land as-is.)

@refack
Copy link
Contributor

refack commented Oct 20, 2018

$ git grep 'SafeGetenv("'

Spin these off to a new bug (might even be a good first issue).

@addaleax
Copy link
Member

Landed in cc750b7

@addaleax addaleax closed this Oct 24, 2018
addaleax pushed a commit that referenced this pull request Oct 24, 2018
Fixes: #22081

PR-URL: #23770
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
targos pushed a commit that referenced this pull request Oct 24, 2018
Fixes: #22081

PR-URL: #23770
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
Fixes: #22081

PR-URL: #23770
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Fixes: #22081

PR-URL: #23770
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Fixes: #22081

PR-URL: #23770
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Issues and PRs related to the Node.js command line interface. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants