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

Plans for npm 9 #778

Closed
MylesBorins opened this issue Aug 31, 2022 · 71 comments
Closed

Plans for npm 9 #778

MylesBorins opened this issue Aug 31, 2022 · 71 comments

Comments

@MylesBorins
Copy link
Member

Hey all. The npm team is hard at work on getting npm 9 together with the hope that we could have the version of npm included in Node.js 18 prior to LTS. A big part of wanting to do this is to ensure we are in a good place for maintenance of the CLI with 18 LTS being supported until 2025.

We are intending to start cutting release candidates for npm 9 in the next week or two. Is there a certain amount of time prior to the October 25th LTS date that folks would want to see us release a GA of npm 9 to feel comfortable including it in node.js 18?

@mhdawson
Copy link
Member

Are there no breaking changes in npm 9 even though its a major? I think by default our policy is no breaking change once we've cut the major.

@MylesBorins
Copy link
Member Author

@mhdawson the breaking changes can be seen in the link I shared above

npm/statusboard#443

There are a number of them, but none that seem to be extremely disruptive to folks for the mainstay use case of "publish" and "install".

We have prior art of updating npm Semver-Majors during a release line. Node.js 16 was updated from npm 7 -> npm 8 while it was current in v16.11.0 in early october of 2021 (a similar timeline to now). It included a breaking change of dropping support to the require('npm') interface, changing support matrix, and updating dependencies to drop support for node 10.

From a stability stand point I think it is the right thing to do for the ecosystem to ensure that the latest Node.js LTS is using a version of npm that will be getting regular bug fixes and features.

@MylesBorins
Copy link
Member Author

as a caveat perhaps we need to have a broader conversation about versioning of npm in node.js, and our release schedules to try and come up with an agreement for future LTS + Majors to avoid needing to revisit this conversation each year (or major) 😓

@mhdawson
Copy link
Member

mhdawson commented Sep 2, 2022

@nodejs/release I think an opinion from the release team would be good and adding @nodejs/tsc in case as well for awareness.

@mcollina
Copy link
Member

mcollina commented Sep 2, 2022

I find it unfortunate that npm decided to ship a major release the same month Node hit lts.

I think it's time we have this conversation of aligning the releases timelines. I think the best thing is for npm to release a major in March or April, so that there is plenty of time for all bugs to be fixed before then.

My 2 cents: there is a long list of breaking changes that still need to be written on user configs etc likely to cause havoc without some proper testing. I don't want to test in LTS.

Given all the above, I’m -1 in shipping Npm v9 in a Node LTS release.

@mhdawson
Copy link
Member

mhdawson commented Sep 2, 2022

@mcollina I like the suggestion of aligning LTS releases between Node and npm. If there was a major npm release in March or April that would be a great way to get a recent major into the Node.js version that will be the next LTS along with enough backing time to minimize impact/surprises for the ecosystem.

@MylesBorins I know that is probably a separate discussion from the current ask, but do you think that would be possibe in the future?

@MylesBorins
Copy link
Member Author

I'm 100% on board with moving forward with improving coordination for future releases.

For the sake of Node.js 18 and thinking about long term stability and feature compatibility I think it is critical we land npm 9. We are putting together an explanation of the changes and I'd be willing to push back on the npm team to delay some of these changes to a future major release if it would allow us to be aligned on landing npm 9 in node.js 18

@aduh95
Copy link
Contributor

aduh95 commented Sep 2, 2022

I think ideally npm versions should not be coupled with Node.js at all, that would set npm free from doing releases however they please, and Node.js could release LTS versions without having to worry what npm is planning. At some point, we need to ask ourselves if the benefits of bundling a precise version of npm outweigh the downsides. Also, it gives npm an unfair advantage over other package manager clients, and maintains confusion between the Node.js project and the npm one.

I personally would like to see the TSC decide to wind down the bundling of npm with Node.js releases, as to me it appears as the most reasonable and fairest solution.

@kapouer
Copy link

kapouer commented Sep 2, 2022

The open source point of view totally seconds that. Having npm bundled in nodejs never made sense (from that point of view).

@MylesBorins
Copy link
Member Author

Summary of npm9 Breaking Changes

Command Removals

bin

This command is not accurate, and having it available encourages users to rely on an incomplete implementation.

set-script

This has been deprecated in favor of the npm pkg set command.
birthday

birthday

This is an easter egg and has been flagged by automated malware tooling.

Breaking Changes to Commands

adduser

This command will no longer be an alias for login.

Config Removals

Bare _auth, _authToken, username, password, email

Bare configs like _auth are confusing and dangerous and will be removed. Instead they will be scoped to a specific registry.

sso-type and auth-type (except for legacy and webauthn)

This removes support for deprecated auth types.

loglevel=timing

This loglevel will be removed and instead timing info will be displayed only with --timing only which will decouple logging from generating timing information.

local, global-style, legacy-bundling

These configs will be replaced with a consolidated --strategy config with the above as values.
Default Config Changes
lockfile-version=3
npm install will create a v3 lockfile by default when creating a new lockfile. These lockfiles are incompatible with npm6.

auth-type=web

Make auth-type=web the default for login and publish for the public registry only.

install-links=true

npm install will install linked dependencies by default

init-private=true

npm init will create a package.json with private: true set by default.

access=public

npm publish will default to access=public for all packages. This is currently the default for non-scoped packages, so this change will now apply to scoped packages as well.

HEAD instead of master for opening links

Commands like npm docs will default to HEAD instead of master as the default ref for opening links.

Other Changes

Error when setting deprecated configs

npm9 will not run npm config set when key is a deprecated config item.

packlist ignore patterns

Two changes here:

Remove shrinkwrap.json as an unignorable file.

npm9 will standardize and document the order of operations of ignoring files from any command that produces a tarball. This is a subtle change but is being treated as breaking change since it will affect the files included in a tarball when multiple ignore types are used. npm/npm-packlist#88 has a full explanation of the specifics of this change.

Folder Permissions

npm will no longer attempt to set folder permissions. This is a complex issue and npm/rfcs#546 has the most up to date information about the specific changes.

Timing Log Files

With timing: true npm will write a process specific timing file similar to how it treats log files instead of appending a newline delimited json file.

engines: ^14.17.0 || ^16.0.0 || >=18.0.0

npm9 will set engines to the above. npm9 will also start to use node@14 specific features and fail when run on npm@12.

stdout and stderr Output

npm9 will standardize output between stdout and stderr. Errors will now be output on stdout when they can possibly be used programmatically by consumers.

Remove Progress Bar

npm9 will remove the progress bar to make it less error prone to receive user input during long running commands. This is not strictly a breaking change since npm does not consider display features to be part of its semver contract, but this change is being made now since it will be a visual change.

Normalize x, x@, & x@*

These specs are all the same but treated and resolved slightly differently. In npm9 they will all be resolved the same. This is a subtle change and is being included in npm9 in order to err on the side of caution. See npm/npm-package-arg#45 for the specific changes in the resolved values.

Remove path support from explain

Only allow for npm explain <package> instead of paths

@MylesBorins
Copy link
Member Author

MylesBorins commented Sep 2, 2022

I think it would make sense to have a separate conversation if we want to talk about unbundling npm from Node.js .

We have to think about the usability concerns of permanently decoupling the two, and I think it would be good to discuss independently

@MylesBorins
Copy link
Member Author

Quick ping on this to @nodejs/releasers and @nodejs/tsc

@BethGriggs
Copy link
Member

Would landing npm@9 in Node.js 19, waiting a couple of months for feedback, and then backporting the update to Node.js 18 during LTS be an alternative option?

I personally think having a good amount of baking time for us to asses impact would be preferable to us rushing to land it in advance of the LTS transition the same month. (As we'll be breaking semver rules anyway, maybe we should do it once we've had more feedback from Node.js 19/Current so we can advise of any anticipated impact?)

@MylesBorins
Copy link
Member Author

Waiting to update is definitely a reasonable solution, I'd still like us to review the proposed breaking changes and flag which changes are going to be "deal breakers" in advance.

@ruyadorno
Copy link
Member

I'm a +1 from the point of view of a releaser. It's going to make it simpler to manage the v18.x release line moving forward, given that there's always extra work involved when managing multiple release lines of npm.

Personally, what I'm more concerned about is limiting the impact of breaking changes. With that in mind the more we can provide configurations that allows for users to opt-in to backwards compatibility the better. Would the npm team be willing to compromise on some of these intended changes (I believe removing bin and tweaking adduser cmds might be somewhat disruptive) in order to reduce any risk involved?

@jasnell
Copy link
Member

jasnell commented Sep 7, 2022

I'd certainly prefer to wait on landing it in 18.

@richardlau
Copy link
Member

If we have to put npm 9 into Node.js 18 then I would prefer to wait and I would also like that as far as possible breaking changes be kept to a minimum (ideally avoided).

We do need to have the separate discussion about aligning releases as the Node.js LTS transition is supposed to be a production-ready stability marker and is not intended to be a point that breaking changes can drop in before (that point is the initial semver major *.0.0 Node.js release).

@MylesBorins
Copy link
Member Author

Would the npm team be willing to compromise on some of these intended changes (I believe removing bin and tweaking adduser cmds might be somewhat disruptive) in order to reduce any risk involved?

We are 100% willing to compromise on any + all proposed breaking changes. I'll surface those two in particular to the team.

I'm personally less concerned about adduser as the existing behavior is confusing and we've had a warning in place. For some changes we can also consider landing breaking changes in v9.0.0, gathering feedback, and reverting if they prove to be disruptive.

Thoughts?

@MylesBorins
Copy link
Member Author

Would it make sense to coordinate a call between the release team + the npm team to review the breaking changes or do folks feel like we can handle this async? I can share a doc for comments if that would work.

@ruyadorno
Copy link
Member

Would landing npm@9 in Node.js 19, waiting a couple of months for feedback, and then backporting the update to Node.js 18 during LTS be an alternative option?

I prefer the original proposition from Myles given that if there's any breaking change (even if they're kept to a minimal as described above) it's safer (and more clearly signalled to users) to land that during the Current release line rather than during LTS.

Would it make sense to coordinate a call between the release team + the npm team to review the breaking changes or do folks feel like we can handle this async? I can share a doc for comments if that would work.

I would be happy to help coordinate a call if there's enough interest and agreement.

@BethGriggs
Copy link
Member

BethGriggs commented Sep 7, 2022

I prefer the original proposition from Myles given that if there's any breaking change (even if they're kept to a minimal as described above) it's safer (and more clearly signalled to users) to land that during the Current release line rather than during LTS.

I disagree with the assessment that it's safer to drop a significant semver-major change into the release line within a few weeks of the LTS date. The expectation from semver is that there are no breaking changes during either Current or LTS. So, I think we'd honour our stability guarantees of LTS more by landing npm@9 in Node.js 19 so we can properly assess the end-user impact and know what workarounds, etc. we need to provide as information to end-users. Our group's assessment of the breaking changes will be good and useful, but there will be inevitable edge cases exposed from real-world usage.

I also am concerned about the timelines of trying to rush it into Node.js 18 considering the expected GA date of October 2022 for npm@9. The LTS transition date is set for 2022-10-25 - by policy, that release will only contain the release commit to promote it to LTS. So going by our schedule, the last release for npm@9 to make will be the one scheduled on 2022-10-04. Is the GA of npm@9 going to be before 2022-10-04? We also try to keep the standard baking time between the last 'Current' release and the LTS transition - so there wouldn't be much, if any, time for an interim release to patch or revert any of the breaking changes before LTS.

@MylesBorins
Copy link
Member Author

MylesBorins commented Sep 7, 2022

In general I think that the case I'm trying to make is that upgrading from npm 8 -> npm 9 should not be considered Semver-Major in the eyes on the Node.js project. The changes we want to land should be considered non-breaking for the primary use cases that npm has for Node.js including installing and publishing packages as well as running scripts.

Quite a few of the breaking changes are being done to improve security posture, others are to clean up rough edge cases and broken APIs.

With this in mind, I think that it would be reasonable to land the release in Node.js 19, gather feedback to confirm it is indeed "non-breaking", if there are things that are breaking we can revert or come up with a reasonable solution. Once we reach that point we can backport the release to npm 18 as a Semver-Minor, which based on discussion should be a bit easier to do 😉.

We are cutting the first r.c. of 9 this week (unless computers / automations want to fight us), and we can start integration testing almost immediately.

The most important thing for us right now is to identify any of the "breaking for npm" changes we want to land that folks would feel would be "breaking for node.js" so we can avoid setting ourselves up for failure.

From a stability + maintainability perspective I think it is incredibly important that we get npm 9 into Node.js 18, this is imho the primary goal of LTS.

I also want to be very clear that once we navigate the npm 9 situation the npm team will work closely with the Node.js team to ensure we have a coordinated release process moving forward that can scale to the unique maintenance requirements of both projects. I think it is reasonable for us to entertain any and all solutions and I look forward to the discussion.

@nlf
Copy link

nlf commented Sep 8, 2022

i think one significant thing we can do here to help us avoid the concern about breaking things is to define what exactly the node team would like to ensure doesn't break. can we work together to determine a set of commands/options/behaviors that should always work? if so, we can codify them as tests in the npm cli itself and we can be certain we don't break that (hopefully) minimal feature set. changes to those tests would give us a strong signal of when a semver-major of npm happens to also require a semver-major of node as well.

@mcollina
Copy link
Member

mcollina commented Sep 8, 2022

Here is a non-exclusive list:

  1. a package-lock.json file from the previous version of npm is left untouched after npm install
  2. a package-lock.json file from the previous version of npm is being kept editable by the previous version of npm shipped with node at x.0.0 mark.
  3. the credentials defined within .npmrc keep working across releases for both install and publish
  4. Installation instructions in READMEs keep working, e.g. npm i -g something is still there.
  5. generically, CI keeps being green...

@GeoffreyBooth
Copy link
Member

  1. a package-lock.json file from the previous version of npm is left untouched after npm install

Even on the same version of npm, successive runs of npm install often cause a package-lock.json to be modified. (This was my feature request at the collaborator summit, a command that does npm ci‘s “install from lockfile” without deleting node_modules first.)

@MylesBorins
Copy link
Member Author

Even on the same version of npm, successive runs of npm install often cause a package-lock.json to be modified.

@GeoffreyBooth I don't believe that should be the case anymore. If there are no conflicts within your package-lock.json running npm install should respect the package-lock and not modify either the package-lock or the package.json

Here is a non-exclusive list

@mcollina this is a great starting point. We'll review the changes with this in mind. We can also think a bit about how we can turn some of these requirements into tests we can have in the Node.js tree!

@mhdawson
Copy link
Member

mhdawson commented Sep 9, 2022

I like the suggestion from @nlf If we had a matrix of npm commands with a checkbox of breaking/non breaking for node-js that would help to discuss as well as to document for the community what they should expect.

@nlf
Copy link

nlf commented Sep 12, 2022

Here is a non-exclusive list:

  1. a package-lock.json file from the previous version of npm is left untouched after npm install

this is the case if the package-lock.json is already lockfileVersion: 2, very old package-lock.json files from npm6 are currently upgraded to a v2 the first time they're consumed in npm7 or greater. npm 6 can still read from these lockfiles, but writing to them from npm6 is likely going to cause some churn the next time the same lockfile is acted on by npm>=7

once a lockfile exists as v2, we will not upgrade it to v3 without being explicitly asked. i think this bar meets this criteria.

  1. a package-lock.json file from the previous version of npm is being kept editable by the previous version of npm shipped with node at x.0.0 mark.

npm>=7 has full support for lockfileVersion 2, support for npm6 and all versions of node that ship with npm6 has already ended, so that's our minimum bar. i think we're good on this criteria too.

  1. the credentials defined within .npmrc keep working across releases for both install and publish

this one we do have a breaking change that will be taking place in npm9. currently we still allow users to define a top level _authToken (or other auth related configs) without scoping them to a specific registry. this is a security risk since those credentials will be passed to whatever default registry is in use. because of this we have deprecated the use of these top level keys in favor of requiring credentials to be scoped to the specific registry they belong to. while breaking due to the security risk involved in not making this change, i think we should consider this to be a good thing to land.

  1. Installation instructions in READMEs keep working, e.g. npm i -g something is still there.

i believe the only significant change here will be the separation of npm login and npm adduser, folks who run npm login and do not have an account will no longer have one created for them, instead they'll be informed they need to run npm adduser or visit the npmjs.com website. since we're at the very least informing the user of next steps and account registration is (hopefully) not being run in non-interactive ways i hope we can call this one acceptable.

  1. generically, CI keeps being green...

this one we will definitely maintain

@BethGriggs
Copy link
Member

support for npm6 and all versions of node that ship with npm6 has already ended,

@nlf, can I just check this statement? Apologies if I am interpreting it out of context, but isn't npm6 still expected to be supported until the EOL of Node.js 14 in April 2023?

(Feel free to hide this tangential comment/query after)

@ljharb
Copy link
Member

ljharb commented Jan 16, 2023

I just published on npm 9 latest with auth-type=legacy, and altho it prompts for OTP, it also gives me the URL for web auth first - so restoring the node 19 default behavior wouldn't actually prevent anyone from using that experience if they wished.

@MylesBorins
Copy link
Member Author

@ljharb the flow for using WebAuthn via that URL requires multiple steps and copying additional numbers from the browser back into the cli to complete the flow. The login flow also requires inputing an email address that is unrelated to the actual authentication (a legacy of couch db).

The new login flow can piggy back on existing web sessions and is significantly streamlined.

The new 2FA auth flow allows for "remember me" when publishing to allow the same session (IP + Token) to be used for publishing multiple packages within a 5 minute period.

Most workflows that are automated should be un-impacted due to still supporting the --otp flag. Any flow requiring manual intervention still works but changes where items are pasted. The new flow supports more options and features.

I recognize that this is frustrating to you personally, but you are advocating to enforce an inferior / less secure flow on all users of Node.js 18 . The flow for WebAuthn using the legacy auth-type is significantly worse than the TOTP flow, which would encourage developers to adopt a less secure method of protecting their account (TOTP + Authenticator based 2FA is phishable and objectively worse than the types of 2FA supported by WebAuthn such as physical key / biometric). There is no loss in behavior and it is relatively straight forward to roll back to the previous behavior with a single line added to an .npmrc.

@ljharb
Copy link
Member

ljharb commented Jan 16, 2023

semver doesn't say "breaking changes except ones that are easy to avoid with configuration". node can certainly decide to ignore semver at will - as it does for some security issues - but it's still a breaking change, and in node 18's case, one that would land inside an LTS line.

@MylesBorins
Copy link
Member Author

@ljharb the purpose of this conversation was to determine which breaking changes to npm were considered breaking changes to node.js. While this was a breaking change to npm I do not believe this is a breaking change to Node.js as it does not meaningfully change behavior. It changes what box a human has to manually type numbers into, any automated solution would work exactly as it had before. This is breaking to npm, and I argue not breaking to Node.js with regards to the set expectations of npm that are not to change.

@ljharb
Copy link
Member

ljharb commented Jan 17, 2023

Given that node often runs on a system that doesn't have a browser at all, I'm not sure I agree, but it's not worth debating further since your mind's made up.

@MylesBorins
Copy link
Member Author

MylesBorins commented Jan 17, 2023

@ljharb on a system without TTY or without a browser it will fall back to legacy

It seems like I might have been mistaken about this, behavior may have changed after the initial implementations. This does not change my opinion, but I am discussing with the team options here for behavior on machines that are non-tty or w/o a browser

@MylesBorins
Copy link
Member Author

Follow up to Ruy's comment

Let’s open a backport PR to explicit our intention of landing the npm9 update in the LTS line

Backport PR is open for npm 9.2.0. The latest version of npm is 9.3.1 which is on main, but it hasn't gone out in a current release yet.

Make sure there’s no potential impact to CI in the issue brought up by Jordan in nodejs/node#45693

As discussed above, there is no impact to CI environments with the change to auth-type. CI environments can continue to use auth tokens (publish, automation, or granular access) to publish from CI.

There is some contention on behavior, but the flow supported in npm 9 is the most secure authentication flow we currently have and I feel strongly that it should be the default experience in LTS.

If everything looks good (citgm seems ok, no new negative feedback from v19) we should land the backport in the upcoming v18 release

We've reviewed CITGM and found no additional failures that would have ecosystem impact. There were only 2 additional failures compared to the CITGM run on 18.13.0. node-gyp is related to process.version being a pre-release. npm is related to internal snapshots and expectations of where the TMPDIR is, this is purely an issue of the npm test suite and the environment configuration of CITGM. The npm team will fix this.


With all of the above I feel we are in a good place to land the backport PR.

@sonnyp
Copy link

sonnyp commented Feb 8, 2023

as a caveat perhaps we need to have a broader conversation about versioning of npm in node.js, and our release schedules to try and come up with an agreement for future LTS + Majors to avoid needing to revisit this conversation each year (or major) sweat

I started one #825

@nschonni
Copy link
Member

nschonni commented Feb 8, 2023

Got one issue report for a breaking change so far nodejs/docker-node#1842

@mhdawson
Copy link
Member

A report of a problem npm team is working on - nodejs/node#46542

@BethGriggs
Copy link
Member

BethGriggs commented Feb 10, 2023

I've heard reports of a team being fairly impacted by the _auth changes. I believe they were using artifactory and Travis. I don't have any more details today, but have asked if they can share more.

One did say that they believed that change should have been considered breaking in the context of Node.js. Or, at least, we could have provided a warning in a prior changelog or blog post indicating when this would happen.

I've also seen a few cases where users are opting to downgrade their version of Node.js 18 to workaround issues they've hit from npm@9 in the latest version. This is perhaps just until they get time to debug or fix. But, it is less than ideal timing considering teams are likely to be under pressure to upgrade to the security release next Tuesday. I think timing of upcoming security releases is something for us to bear in mind in future.

@MylesBorins
Copy link
Member Author

@BethGriggs I'm following up with the team on the Auth-Type breakage. From the original RRFCthis behavior was only supposed to change for the public registry... 3rd party registries were not supposed to be impacted so we need to take a look at how that behavior didn't land and attempt to fix it asap. This was specifically so it would be non-breaking to folks in regards to day-to-day workflows.

Almost every other issue seems like it stemmed from the install-links change which is already reverted.

@MylesBorins
Copy link
Member Author

As a quick update, and to summarize in one place, the issues related to install-links is fixed in npm 9.4.2 and there is a PR to Node.js core to update the version of npm. If you are running into this issue with your projects an need a "quick fix" today you can include a local .npmrc in your project root to manually disable install links. once you update to the newer version of npm this will be a noop.

# .npmrc
# manually set install-links to false
install-links=false

For those who are running into issues with auth-type you can manually set the auth-type to legacy at either a project or system level in a .npmrc to fall back to the flow of npm 8. We are investigating why this is not working as expected with 3rd party registries and hope to have an update soon.

# .npmrc
# manually set auth-type to legacy
auth-type=legacy

@MylesBorins
Copy link
Member Author

I dug more into the auth-type issue... there was a regression when internals were refactored, this is unrelated to any semver-major changes.

Originally npm login and npm adduser would fallback from auth-type=web to auth-type=legacy if the registry being authenticated with did not support the new auth-type.

I've submitted a PR that fixes the regression and will aim to get something landed in a timely fashion and another release of npm cut. Since it's late on Friday it may not get picked up until Monday. It still needs review and tests, but I think that it is most likely going to work as a solution.

@MylesBorins
Copy link
Member Author

FYI npm 9.5.0 is now released and includes the fix for --auth-type fallback. I will work with the Node.js release team to get a release out with this version after the security release has gotten out

@jyboudreau
Copy link

jyboudreau commented Feb 23, 2023

this one we do have a breaking change that will be taking place in npm9. currently we still allow users to define a top level _authToken (or other auth related configs) without scoping them to a specific registry. this is a security risk since those credentials will be passed to whatever default registry is in use. because of this we have deprecated the use of these top level keys in favor of requiring credentials to be scoped to the specific registry they belong to. while breaking due to the security risk involved in not making this change, i think we should consider this to be a good thing to land.

I agree this is a good thing to land, however, I'm concerned with all those folks that rely on this. Have you got statistics on how widespread this usage is?

Just wanted to leave a note that we were impacted by this. Specifically users of Artifactory relying on a REST API endpoint for generating an NPM token via the REST API may be impacted as the documentation from Artifactory recommends the following:

Getting .npmrc entries directly from Artifactory

Run the following command to retrieve these strings directly from Artifactory:

$ curl -uadmin:<CREDENTIAL> http://<ARTIFACTORY_SERVER_DOMAIN>:8081/artifactory/api/npm/auth
Where <CREDENTIAL> is your Artifactory password or APIKey.

Here is an example of the response:

_auth = YWRtaW46e0RFU2VkZX1uOFRaaXh1Y0t3bHN4c2RCTVIwNjF3PT0=
email = myemail@email.com
always-auth = true

@MylesBorins
Copy link
Member Author

MylesBorins commented Feb 23, 2023

@jyboudreau this seems like something artifactory needs to update in their product. The suggestion of using _auth in your local .npmrc is an insecure practice, that is why we have removed the functionality to npm. It seems like artifcatory could give a registry scoped token e.g. //npm.pkg.github.com/:_authToken in the response instead of a generic _auth token. As mentioned above the unscoped _auth token would be passed to any default registry in use and has resulted in many private access tokens being leaked to the npm public registry unintentionally.

edit: I want to recognize that getting broken here is not ideal, but in this particular case the change was done for security reasons and I would hope that if provided that information JFrog would prioritize a fix

@jyboudreau
Copy link

this seems like something artifactory needs to update in their product [...]

Very much agreed on all fronts.

My intention was mostly to raise awareness that this type of change could cause impact and to help release decisions like this in the future. I'm not very familiar with the release cycle of NPM and Node and it was surprising to me to learn that a tool shipping with Node could ship a major breaking version within a minor release cycle of Node, especially on a LTS version.

In my case, our CI was simply using the node-18 official docker image which was updated to latest LTS with these breaking changes. This brings an uncomfortable question of whether teams should clamp down their node version, potentially missing non-breaking security fixes.

@MylesBorins
Copy link
Member Author

@jyboudreau I appreciate you bringing it up!

We definitely don't want people to have to clamp their node versions. The intention was to ensure that breaking changes made to npm would not be considered breaking to Node.js. We went through ample testing and a waterfall release to try and catch everything, but sometimes things are not caught.

IMHO I don't see the breakages that have come from the npm update as significantly different than other bugs that sometimes land on LTS. In the specific instance of _auth changes, which were done for security, it is inline with the Node.js LTS policy of allowing a breaking change as a Semver Minor in LTS if it is for valid security reasons.

Please keep the feedback coming if you have more. We see it as a bigger ecosystem risk if the version of npm in LTS is not regularly updated, which would be the likely outcome if it isn't on the latest major. The npm team doesn't have the capacity today to manage multiple release lines.

@joshuaharley
Copy link

I will say that this has caused us to need to pin our Node images to 18.13 which includes the older, still working npm 8. A working config of (in 8.19.2)

registry=https://nexus.internal.corp/repository/npm/
_auth=${NPM_TOKEN_NEXUS}
email=${NPM_EMAIL_NEXUS}

no longer works in docker after being converted with npm config fix as

registry=https://nexus.internal.corp/repository/npm/
//nexus.internal.corp/repository/npm/:_auth=${NPM_TOKEN_NEXUS}
email=${NPM_EMAIL_NEXUS}

(after putting back the environment variables as the fix command put the real values in) does not work in 18.14 docker image with npm 9.5.0. It fails with a 401 unauthorized. After spending a few days with it in our CI we gave up and pinned it to 18.14 in hopes that npm will either revert or we eventually find some way to debug what is being sent and why its invalid.

@MylesBorins
Copy link
Member Author

@joshuaharley I think it should be

registry=https://nexus.internal.corp/repository/npm/
//nexus.internal.corp/repository/npm/:_authToken=${NPM_TOKEN_NEXUS}
email=${NPM_EMAIL_NEXUS}

Keep in mind that this change was done because an unscoped _auth was sending your token to every registry, including the public registry, if requests ended up going there due to a local .npmrc. We had identified a large number of 3rd party tokens being sent to us via such requests, which is the reason for the change.

Had we not been able to land npm 9 in LTS we would have backported this change to npm 8, which is entirely within the scope of the Node.js LTS release process of allowing Semver-Major changes in a Semver-Minor for security reasons.

It is unfortunate that Nexus (and artifactory) have documented insecure practices in using their 3rd party registries

@MylesBorins
Copy link
Member Author

@joshuaharley I do think you've identified a bug with npm config fix though! I'll raise this to the team

@joshuaharley
Copy link

Keep in mind that this change was done because an unscoped _auth was sending your token to every registry, including the public registry, if requests ended up going there due to a local .npmrc. We had identified a large number of 3rd party tokens being sent to us via such requests, which is the reason for the change.

I totally understand the reasoning behind it I think the frustrating part was that it did happen in a point release and we weren't prepared to be able to work with the update to get it working. I had tried changing _auth to _authToken manually but that did not seem to work either as that expects a longer lived NPM token (I think?) where the _auth environment variable is the base 64 encoding of the username:password (same used for basic auth).

The one thing I have not been able to try yet is splitting the base64 pair back in to a username and password variable and setting the :username and :_password values instead.

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

No branches or pull requests