GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — doc: NODE_EXTRA_CA_CERTS is ignored if setuid root by bnoordhuis · Pull Request #23770 · nodejs/node

Conversation

@bnoordhuis

@bnoordhuis bnoordhuis commented Oct 19, 2018

Copy link
Copy Markdown
Member

Fixes: #22081

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ChALkeR and dooman87 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@bnoordhuis
doc: NODE_EXTRA_CA_CERTS is ignored if setuid root
c0413f3
Fixes: nodejs#22081
@nodejs-github-bot

nodejs-github-bot commented Oct 19, 2018

Copy link
Copy Markdown
Collaborator

@bnoordhuis build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1306/pipeline

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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 Node.js documentation. labels Oct 19, 2018
cjihrig
cjihrig approved these changes Oct 19, 2018
View reviewed changes
richardlau
richardlau approved these changes Oct 19, 2018
View reviewed changes
jasnell
jasnell approved these changes Oct 19, 2018
View reviewed changes
@vsemozhetbyt

vsemozhetbyt commented Oct 19, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vsemozhetbyt

vsemozhetbyt commented Oct 19, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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/lib/internal/print_help.js

Lines 18 to 19 in e7f710c

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@richardlau

richardlau commented Oct 19, 2018

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bnoordhuis

bnoordhuis commented Oct 19, 2018

Copy link
Copy Markdown
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))
vsemozhetbyt and richardlau reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@richardlau

richardlau commented Oct 20, 2018

Copy link
Copy Markdown
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.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

refack
refack approved these changes Oct 20, 2018
View reviewed changes
@refack

refack commented Oct 20, 2018

Copy link
Copy Markdown
Contributor

$ git grep 'SafeGetenv("'

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@addaleax

addaleax commented Oct 24, 2018

Copy link
Copy Markdown
Member

Landed in cc750b7

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@addaleax addaleax closed this Oct 24, 2018
addaleax pushed a commit that referenced this pull request Oct 24, 2018
@bnoordhuis @addaleax
doc: NODE_EXTRA_CA_CERTS is ignored if setuid root
cc750b7
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
@bnoordhuis @targos
doc: NODE_EXTRA_CA_CERTS is ignored if setuid root
1baba9b
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 targos mentioned this pull request Oct 27, 2018
Release proposal: v11.1.0 #23922
Merged
This was referenced Nov 2, 2018
2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#245
Closed
2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#246
Closed
2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#247
Closed
2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#248
Closed
@richardlau richardlau mentioned this pull request Nov 17, 2018
NODE_PATH not recognized when cap_net_bind_service is set #22648
Closed
@MylesBorins MylesBorins added dont-land-on-v8.x labels Nov 26, 2018
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
@bnoordhuis @MylesBorins
doc: NODE_EXTRA_CA_CERTS is ignored if setuid root
f5c68dc
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
v10.13.1 proposal #24675
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@bnoordhuis @rvagg
doc: NODE_EXTRA_CA_CERTS is ignored if setuid root
b609565
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
@bnoordhuis @MylesBorins
doc: NODE_EXTRA_CA_CERTS is ignored if setuid root
913c491
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
v10.14.2 proposal #24727
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@cjihrig cjihrig cjihrig approved these changes
@richardlau richardlau richardlau approved these changes
+1 more reviewer
@refack refack refack approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

cli Issues and PRs related to the Node.js command-line interface. doc Issues and PRs related to Node.js documentation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@bnoordhuis @nodejs-github-bot @vsemozhetbyt @richardlau @refack @addaleax @jasnell @cjihrig @MylesBorins
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.