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

Pass environment variables to prebuild-install #329

Closed
wants to merge 1 commit into from

Conversation

0x326
Copy link

@0x326 0x326 commented Jun 13, 2019

Per the prebuild-install documentation, end users should be able to pass environment variables to change where prebuild looks for its binaries. Currently, scripts/prebuild-install.js filters out these variables. This PR passes them on.

@0x326
Copy link
Author

0x326 commented Jun 13, 2019

Also, once this PR is merged, would it be possible to increment the package version by a bug fix and publish to NPM?

@reqshark
Copy link
Member

to change where prebuild looks for its binaries

do we want to pass all the printenv output to prebuild (process.env) or just the ones that matter?

@reqshark
Copy link
Member

would it be possible to increment the package version by a bug fix and publish to NPM?

easier than "simple made easy" by Rich Hickey https://www.youtube.com/watch?v=34_L7t7fD_U

Aren't PASSWORDS or SECRET_KEY credentials just casual properties ("keys") of process.env

Passing.env is objectively simplest but i'm hesitant on the basis of secrecy exposure to outer pkg

@0x326
Copy link
Author

0x326 commented Jun 14, 2019

@reqshark I think we should pass the entirety of process.env for the following reasons:

  1. We don't know which environment variables are important in the eyes of the end user. Though one might suggest that zeromq_binary_host is the only important variable, end users might also be interested in passing HTTP_PROXY or NODE_EXTRA_CA_CERTS.

  2. In general, when a user runs SOME_VAR=value command, he should expect SOME_VAR to be passed to command as well as any child processes spawned from it. If the variable contains sensitive information not intended for command, then he should unset it before running the command.

  3. We ought to mimic the behavior of npm. Since the intended usage of prebuild-install is "install": "prebuild-install || node-gyp rebuild" (and ours is non-standard), we should pass all environment variables to prebuild-install that it would normally receive.

  4. It is not our place to enact a security policy on which variables can pass through. If there exists sensitive environment variables that are passed to npm scripts, then the fix should be in npm itself, not zeromq.

@reqshark
Copy link
Member

ok this change should be included in the next release

@mdsummers
Copy link

Is it possible to get this merged in? It's currently very difficult to include this module as part of a project when running behind a corporate proxy.

@rolftimmermans
Copy link
Member

The latest 6.0 beta release includes all prebuilt binaries in the NPM package and does not need to download anything on supported platforms during install. It would be great if you could try it out! The new version has a new API that addresses some fundamental issues with the previous API, but it does include a compatibility layer that should make upgrading easier. See #189 for the reasoning behind the new API.

I'll leave this open because this change could still be included in the 5.x branch.

@aminya
Copy link
Member

aminya commented Apr 18, 2021

Thanks, but we have switched from prebuild-install to prebuildify

@aminya aminya closed this Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants