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

Remove standard http ports from registry URL when writing authToken #62

Merged
merged 1 commit into from
Jun 9, 2022
Merged

Remove standard http ports from registry URL when writing authToken #62

merged 1 commit into from
Jun 9, 2022

Conversation

rhiaxion
Copy link
Contributor

@rhiaxion rhiaxion commented Jun 2, 2022

The latest release of this plugin included an update of the NPM client from v6 to v8, which changed the authentication behaviour of the client.

If a private registry URL contains standard http port 80 or 443 when performing an npm login / adduser command the port number is removed when writing the authToken to the .npmrc file. If the port remains in the authToken registry URL then the token is ignored when running commands like npm whoami or npm publish and results in an error asking to run npm adduser.

Because this plugin writes the .npmrc file directly we need to remove the port from the registry URL before writing.

Example:

# Configure registry
npm config set registry https://private.registry.com:443

Invalid .npmrc authToken

//private.registry.com:443/:_authToken=$TOKEN

Valid .npmrc authToken

//private.registry.com/:_authToken=$TOKEN

Copy link
Contributor

@donny-dont donny-dont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to do due to the change in NPM.

What NPM servers have you tested this on? When I developed the plugin I was using NPM proper and a Sinopia server.

@bradrydzewski
Copy link
Member

out of curiosity, what happens if the npm registry is running on a non-standard port like :3000 ? port 443 and 80 can be inferred based on https or http, so was just wondering how this works with non-standard ports ...

@donny-dont
Copy link
Contributor

I think this might be the relevant commit npm/npm-registry-fetch@cc11cc1

@rhiaxion
Copy link
Contributor Author

rhiaxion commented Jun 8, 2022

What NPM servers have you tested this on?

I'm testing this against Artifactory with a reverse proxy in front, which for some reason is very opinionated about the use of port 443 when resolving dependencies in package-lock.json files, so I explicitly add the port to the registry URL for resolving and publishing.

out of curiosity, what happens if the npm registry is running on a non-standard port like :3000 ? port 443 and 80 can be inferred based on https or http, so was just wondering how this works with non-standard ports ...

You're quite right that the issue relates to use of standard ports. I started a container with plugins/npm and tested npm login against a registry with a standard and non standard port.

  • npm login --registry https://private.registry.com:443/ resulted in //private.registry.com/:_authToken=
  • npm login --registry https://private.registry.com:8443/ resulted in //private.registry.com:8443/:_authToken=

I would assume that port 80 results in the same issue. So it seems the Node URL parsing and token URL matching is not the same for all ports. I'll update this PR to account for port 80 and 443 to match the NPM 8 client behaviour.

@rhiaxion rhiaxion changed the title Remove port from registry URL when writing authToken Remove standard http ports from registry URL when writing authToken Jun 8, 2022
@donny-dont
Copy link
Contributor

Thanks for verifying @rhiaxion ! If @bradrydzewski is happy I think this is good to go.

@donny-dont donny-dont merged commit edd496b into drone-plugins:master Jun 9, 2022
@rhiaxion rhiaxion deleted the sryan/npm8-authtoken-filter-port branch June 10, 2022 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants