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

config: accept auth tokens from environment variables #8

Merged
merged 1 commit into from Aug 3, 2018

Conversation

@mkhl
Copy link
Contributor

@mkhl mkhl commented Jul 12, 2018

As discussed on npm.community, the fact that npm registry authentication tokens cannot be defined using environment variables does not seem justified anymore.

The restriction is caused by the config loader translating

  • all _ to -
  • the whole variable name to lowercase

while the credential checker expects a key ending in :_authToken.

As suggested, this change fixes the problem by limiting the translation by the config loader to the part before the first colon.

Closes npm/npm#15565

@mkhl mkhl requested a review from as a code owner Jul 12, 2018
@mkhl
Copy link
Contributor Author

@mkhl mkhl commented Jul 12, 2018

An alternative more limited approach would be to duplicate this block of code that looks for the …:_authToken config key and letting the copy look for …:-authtoken (lowercase and with a hyphen instead of an underscore).

Copy link
Contributor

@zkat zkat left a comment

Yeah, I think we would much rather this be a single special-case for _authToken and nothing else. Could you push that version instead?

As discussed on npm.community[1], the fact that
npm registry authentication tokens
cannot be defined using environment variables
does not seem justified anymore.

The restriction is caused by the config loader translating
* all `_` to `-`
* the whole variable name to lowercase
while the credential checker expects a key ending in `:_authToken`.

This change fixes the problem
by having the credential checker try
a key ending in `:-authtoken` after it tried `:_authToken`.

Closes npm/npm#15565

[1]: https://npm.community/t/cannot-set-npm-config-keys-containing-underscores-registry-auth-tokens-for-example-via-npm-config-environment-variables/233
@mkhl
Copy link
Contributor Author

@mkhl mkhl commented Jul 23, 2018

Of course, here you go.

zkat
zkat approved these changes Jul 30, 2018
Copy link
Contributor

@zkat zkat left a comment

Thanks! This looks great to me! 🎉

@zkat zkat changed the base branch from latest to release-next Jul 30, 2018
@zkat zkat changed the title config: Only translate environment variables up to the first colon config: accept auth tokens from environment variables Jul 30, 2018
@iarna
Copy link
Contributor

@iarna iarna commented Jul 31, 2018

The windows environment is not case sensitive, so we should take care to ensure that this works regardless of the case of the env var. (I've not yet checked to see if this is or isn't addressed yet, I just wanted to make sure this requirement was stated.)

@mkhl
Copy link
Contributor Author

@mkhl mkhl commented Jul 31, 2018

The windows environment is not case sensitive, so we should take care to ensure that this works regardless of the case of the env var.

Yes, thank you for bringing this up, it is one of the problems this change works around:
The config loader translates all env var names to lower case,
then the credential checker looks for one ending in :_authToken
and fails in part due to the uppercase letter in there.

With this change it also tries :-authtoken, matching the behaviour of the config loader,
and the (already present) translation to lower case should ensure that the case of env var names doesn’t matter.

@zkat zkat merged commit 6e9f04b into npm:release-next Aug 3, 2018
2 checks passed
2 checks passed
@travis-ci[bot]
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mkhl mkhl deleted the issue/15565 branch Aug 3, 2018
@zkat zkat mentioned this pull request Aug 15, 2018
4 tasks
@zkat zkat mentioned this pull request Aug 29, 2018
4 tasks
@trevorah
Copy link

@trevorah trevorah commented Jan 29, 2019

This feature is really great, but I can't seem to get it working.
Latest npm:

$ npm --version
6.7.0

env var gets read ok:

$ env "npm_config_//registry.npmjs.org/:_authToken=my-secret-token" npm config ls
; cli configs
metrics-registry = "https://registry.npmjs.org/"
scope = ""
user-agent = "npm/6.7.0 node/v8.12.0 darwin x64"

; environment configs
//registry.npmjs.org/:-authtoken = "my-secret-token"

; userconfig /Users/trevorah/.npmrc
always-auth = true

; builtin config undefined
prefix = "/usr/local"

; node bin location = /usr/local/bin/node
; cwd = /Users/trevorah/Development/something
; HOME = /Users/trevorah
; "npm config ls -l" to show all defaults.

but valid token doesn't get used:

$ env "npm_config_//registry.npmjs.org/:_authToken=my-secret-token" npm whoami
npm ERR! code E401
npm ERR! 401 Unauthorized - GET https://registry.npmjs.org/-/whoami

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/trevorah/.npm/_logs/2019-01-29T16_44_41_403Z-debug.log

Am I doing something wrong?

EDIT:
something may be messing up the tokens, as it behaves differently if auth is completely missing:

$ npm whoami
npm ERR! code ENEEDAUTH
npm ERR! need auth This command requires you to be logged in.
npm ERR! need auth You need to authorize this machine using `npm adduser`

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/trevorah/.npm/_logs/2019-01-29T16_52_19_586Z-debug.log

@pvdlg
Copy link

@pvdlg pvdlg commented Oct 3, 2019

Another issue is that (at least on OSX with bash) the variable name in invalid:

$ export npm_config_//registry.npmjs.org/:_authToken=my-secret-token

export: `npm_config_//registry.npmjs.org/:_authToken=my-secret-token': not a valid identifier

jasonkarns added a commit to nodenv/node-build that referenced this issue Nov 29, 2019
Npm's CLI is broken because it doesn't respect the auth token being
provided via the environment when publishing despite npm/cli#8

GitHub's node action is broken because it doesn't respect the scope and
registry config as provided via package.json.

And npmjs.org registry is broken because it doesn't support named
tokens, nor tokens that skip OTP. Ergo, in order to publish via github
actions, my user profile 2fa must be downgraded to auth-only and the
package 2fa must be disabled.

OMFG
BYK added a commit to getsentry/craft that referenced this issue Dec 2, 2020
This is a retake on #130. Although npm/cli#8 claims to have support
for `npm_config_//registry.npmjs.org/:_authToken=` usage, my tests
and the reports on the internet says this still doesn't work, even
with the latest npm (7.0.15 at the time).

The only way to pass the token is to have the `authToken` line in
an `.npmrc` file. The quick&dirty way would have been to create one
in the project directory but that may collide with a potentially
pre-existing project `.npmrc`. Trying to merge these seems more
trouble than it is worth:
https://github.com/actions/setup-node/blob/59e61b89511ed136a0b17773f07c349fa5c01e8b/src/authutil.ts
(even worse as you'd need to revert these changes after the fact)

The "better" solution I found is:

1. Create a temporary file as your npmrc
2. Put the token/registry line there
3. Tell npm to use that file as the user config
4. Use the `npm_config_userconfig` for the above to support yarn too

This may still fail for yarn, see yarnpkg/yarn#4568.
BYK added a commit to getsentry/craft that referenced this issue Dec 2, 2020
This is a retake on #130. Although npm/cli#8 claims to have support
for `npm_config_//registry.npmjs.org/:_authToken=` usage, my tests
and the reports on the internet says this still doesn't work, even
with the latest npm (7.0.15 at the time).

The only way to pass the token is to have the `authToken` line in
an `.npmrc` file. The quick&dirty way would have been to create one
in the project directory but that may collide with a potentially
pre-existing project `.npmrc`. Trying to merge these seems more
trouble than it is worth:
https://github.com/actions/setup-node/blob/59e61b89511ed136a0b17773f07c349fa5c01e8b/src/authutil.ts
(even worse as you'd need to revert these changes after the fact)

The "better" solution I found is:

1. Create a temporary file as your npmrc
2. Put the token/registry line there
3. Tell npm to use that file as the user config
4. Use the `npm_config_userconfig` for the above to support yarn too

This may still fail for yarn, see yarnpkg/yarn#4568.
BYK added a commit to getsentry/craft that referenced this issue Dec 3, 2020
This is a retake on #130. Although npm/cli#8 claims to have support
for `npm_config_//registry.npmjs.org/:_authToken=` usage, my tests
and the reports on the internet says this still doesn't work, even
with the latest npm (7.0.15 at the time).

The only way to pass the token is to have the `authToken` line in
an `.npmrc` file. The quick&dirty way would have been to create one
in the project directory but that may collide with a potentially
pre-existing project `.npmrc`. Trying to merge these seems more
trouble than it is worth:
https://github.com/actions/setup-node/blob/59e61b89511ed136a0b17773f07c349fa5c01e8b/src/authutil.ts
(even worse as you'd need to revert these changes after the fact)

The "better" solution I found is:

1. Create a temporary file as your npmrc
2. Put the token/registry line there
3. Tell npm to use that file as the user config
4. Use the `npm_config_userconfig` for the above to support yarn too
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants