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

src: add env and cli option to disable global search paths #39754

Closed
wants to merge 2 commits into from
Closed

src: add env and cli option to disable global search paths #39754

wants to merge 2 commits into from

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Aug 13, 2021

For embedders it is usually not allowed for Node.js to search modules from global paths like $HOME/.node_modules, otherwise the behavior of the app could be changed because of a global installed module. For example, a global installed electron module should not break all require('electron') calls.

This PR adds an option to disable searching modules from global paths.

Also note that this change will have a merge conflict with #39712.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 13, 2021
@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Aug 13, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@addaleax addaleax added the embedding Issues and PRs related to embedding Node.js in another project. label Aug 13, 2021
aduh95
aduh95 reviewed Aug 13, 2021
View reviewed changes
lib/internal/bootstrap/pre_execution.js Outdated Show resolved Hide resolved
lib/internal/options.js Outdated Show resolved Hide resolved
lib/internal/options.js Outdated Show resolved Hide resolved
@bmeck
Copy link
Member

bmeck commented Aug 13, 2021

i know it might be slightly out of scope, but could we add a CLI option for this or something so that stock installs could test against this behavior?

@zcbenz
Copy link
Contributor Author

zcbenz commented Aug 16, 2021

i know it might be slightly out of scope, but could we add a CLI option for this or something so that stock installs could test against this behavior?

I think it is a good idea, I have added --no-global-search-paths cli option in this PR.

Note that the version number in doc/api/cli.md is left as 16.?.? since I don't know what to fill.

@zcbenz zcbenz changed the title src: add option to disable global search paths src: add env and cli option to disable global search paths Aug 16, 2021
@aduh95 aduh95 mentioned this pull request Aug 16, 2021
Closed
aduh95
aduh95 reviewed Aug 16, 2021
View reviewed changes
doc/api/cli.md Outdated Show resolved Hide resolved
addaleax
addaleax reviewed Aug 16, 2021
View reviewed changes
src/env.cc Outdated Show resolved Hide resolved
@jasnell
Copy link
Member

jasnell commented Aug 17, 2021

Needs a rebase and a new CI run. Removing the author ready label for now.

@jasnell jasnell removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 17, 2021
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 1, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95
Copy link
Contributor

aduh95 commented Sep 16, 2021

@zcbenz Can you please rebase again to fix the git conflict?

@gengjiawen gengjiawen added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 17, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 17, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@targos targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 18, 2021
targos pushed a commit that referenced this pull request Sep 18, 2021
@zcbenz @targos
src: add option to disable global search paths
e4852e3 
PR-URL: #39754
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
targos pushed a commit that referenced this pull request Sep 18, 2021
@zcbenz @targos
src: add --no-global-search-paths cli option
d9791d9 
PR-URL: #39754
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@targos
Copy link
Member

targos commented Sep 18, 2021

Landed in 40c6e83...d9791d9

@targos targos closed this Sep 18, 2021
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
@zcbenz @BethGriggs
src: add option to disable global search paths
a23ea5f 
PR-URL: #39754
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
@zcbenz @BethGriggs
src: add --no-global-search-paths cli option
2083b83 
PR-URL: #39754
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
BethGriggs added a commit that referenced this pull request Sep 21, 2021
@BethGriggs
2021-09-22, Version 16.10.0 (Current)
45a042b 
Notable changes:

crypto:
  * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927
doc:
  * add Ayase-252 to collaborators (Qingyu Deng) #40078
fs:
  * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013
http:
  * (SEMVER-MINOR) limit requests per connection (Artur K) #40082
src:
  * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754
  * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754
  * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926
stream:
  * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067

PR-URL: TODO
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
@zcbenz @BethGriggs
src: add option to disable global search paths
51f9ad4 
PR-URL: #39754
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
@zcbenz @BethGriggs
src: add --no-global-search-paths cli option
9a67296 
PR-URL: #39754
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
BethGriggs added a commit that referenced this pull request Sep 21, 2021
@BethGriggs
2021-09-22, Version 16.10.0 (Current)
10b4fe1 
Notable changes:

crypto:
  * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927
doc:
  * add Ayase-252 to collaborators (Qingyu Deng) #40078
fs:
  * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013
http:
  * (SEMVER-MINOR) limit requests per connection (Artur K) #40082
src:
  * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754
  * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754
  * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926
stream:
  * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067

PR-URL: TODO
@BethGriggs BethGriggs mentioned this pull request Sep 21, 2021
Merged
1 task
BethGriggs added a commit that referenced this pull request Sep 22, 2021
@BethGriggs
2021-09-22, Version 16.10.0 (Current)
aff2a0a 
Notable changes:

crypto:
  * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927
doc:
  * add Ayase-252 to collaborators (Qingyu Deng) #40078
fs:
  * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013
http:
  * (SEMVER-MINOR) limit requests per connection (Artur K) #40082
src:
  * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754
  * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754
  * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926
stream:
  * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067

PR-URL: #40175
BethGriggs added a commit that referenced this pull request Sep 22, 2021
@BethGriggs
2021-09-22, Version 16.10.0 (Current)
8d5675e 
Notable changes:

crypto:
  * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927
doc:
  * add Ayase-252 to collaborators (Qingyu Deng) #40078
fs:
  * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013
http:
  * (SEMVER-MINOR) limit requests per connection (Artur K) #40082
src:
  * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754
  * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754
  * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926
stream:
  * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067

PR-URL: #40175
@ljharb
Copy link
Member

ljharb commented Sep 22, 2021

Any chance the default for this could be switched in a semver-major?

@zcbenz
Copy link
Contributor Author

zcbenz commented Sep 23, 2021

Any chance the default for this could be switched in a semver-major?

It will probably break utilities installed with npm i -g.

Some Linux distributions also provide packages for node modules, and they rely on global search paths to work.
https://packages.ubuntu.com/search?keywords=node&searchon=names&suite=hirsute&section=all

@ljharb
Copy link
Member

ljharb commented Sep 23, 2021

Globally installed modules are pretty community-discouraged, as i think are distro-distributed npm packages, but fair callout.

codebytere added a commit to electron/electron that referenced this pull request Sep 23, 2021
@codebytere
src: add env and cli option to disable global search paths
6f5b56e 
nodejs/node#39754
codebytere added a commit to electron/electron that referenced this pull request Sep 27, 2021
@codebytere
src: add env and cli option to disable global search paths
83c15bc 
nodejs/node#39754
codebytere added a commit to electron/electron that referenced this pull request Sep 27, 2021
@codebytere
src: add env and cli option to disable global search paths
5dbcbcf 
nodejs/node#39754
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@addaleax addaleax addaleax approved these changes

@richardlau richardlau richardlau approved these changes

@tniessen tniessen tniessen approved these changes

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

@fhinkel fhinkel fhinkel 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. c++ Issues and PRs that require attention from people who are familiar with C++. embedding Issues and PRs related to embedding Node.js in another project. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.