Fix scoped pkg auth when using npm private pkgs via npm login --scope #5162

KidkArolis · 2018-01-06T15:43:51Z

Summary

This attempts to squash one of the last remaining auth bugs in yarn (🤞). There currently exists an edge case where if you login to the registry using npm login --scope=@foo, the contents of .npmrc become

@foo:registry=https://registry.npmjs.org/
//registry.npmjs.org/:_authToken=xxxxxx

Installing pkgs from @foo however does not work. That's because the request URL is being made to https://registry.yarnpkg.com and isRequestToRegistry returns false because the args to it are isRequestToRegistry('https://registry.yarnpkg.com/@foo/pkg', 'https://registry.npmjs.org').

This PR attempts to remove isRequestToRegistry entirely, because I believe at this stage it is no longer needed. The only purpose of this function is to make sure that auth tokens for registry X are only sent with requests to registry X. This is now ensured by getAuth function which looks at the request url or package identifier and decides what token to use.

Before this PR, getAuth('http://Y) would sometimes return the auth token for registry.npmjs.org since that's the default registry. Instead, it now returns no token since the request url does not match the registry.

Let me know if I can clarify something further.

Test plan

I've tested this manually using my own npm private account. It would be great to get that e2e test going as discussed here: #4186. I could try working on that next if there's interest, but I'd do that in a separate PR.

I've also added a new unit test for this edge case npm login --scope=@foo. In other words, all existing tests pass (with a small exception - see should add authorization header with token for custom registries with a scoped package test) + I've added a new test to cover for the edge case in question.

Fixes yarnpkg#4157, yarnpkg#4451

buildsize · 2018-01-06T15:46:07Z

This change will decrease the build size from 10.4 MB to 10.4 MB, a decrease of 1.49 KB (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	900.42 KB	900.33 KB	-98 bytes (0%)
yarn-[version].js	3.92 MB	3.92 MB	-273 bytes (0%)
yarn-legacy-[version].js	4.06 MB	4.06 MB	-221 bytes (0%)
yarn-v[version].tar.gz	906.16 KB	905.45 KB	-724 bytes (0%)
yarn_[version]all.deb	669.36 KB	669.16 KB	-210 bytes (0%)

KidkArolis · 2018-01-06T16:46:53Z

__tests__/registries/npm-registry.js

@@ -197,69 +197,31 @@ describe('request', () => {

    npmRegistry.config = {
      '//some.other.registry/:_authToken': 'testScopedAuthToken',
-      '@testScope:registry': '//some.other.registry/',
+      '@testScope:registry': 'https://some.other.registry/',


By default, npm login populates the .npmrc file with the full url, e.g.

@scope:registry=https://some.other.registry/

and not

@scope:registry=//some.other.registry/

I made sure this is not a regression by editing away the https:// in my ~/.npmrc and running npm install which gives me npm ERR! Only absolute URLs are supported

KidkArolis · 2018-01-06T16:51:00Z

__tests__/registries/npm-registry.js

-    );
-
-    npmRegistry.config = {
-      'custom-host-suffix': 'pkgs.host.com',


The custom-host-suffix feature is no longer necessary. If the request is being made to a registry that needs a token - the token will get sent. If the requested registry does no have a token, no token is sent.

Though, looking at this test a bit more now, I might have misunderstood. If someone is using a registry with a full path, e.g. http://pkgs.host.com/bar/baz instead of just http://pkgs.host.com/, removal of this "custom-host-suffix" feature might indeed introduce a regression.

But why not just specify the registry as http://pkgs.host.com/? You basically trust that server.

History shows @soda0289 adding it in #3987.
Do you think the original issue is still fixed?

The custom-host-suffix feature is the only point in this PR that I'm unsure about.

This PR preserves the feature where the authorization token is only sent if it's applicable to that registry (which was one of the intentions of #3987). But I don't look at pathnames.

Fix #3987 made sure that pathname and not just host is also checked when deciding if auth header should be used. But I'm not convinced that's needed. The reasoning is briefly described here: https://github.com/yarnpkg/website/pull/504/files by @lumaxis, but I need to understand that use case better. Are you around to discuss this @lumaxis? In particular how can I setup this VSTS registry to test yarn?

I will dive deeper on this will get back here with my conclusions.

Hey @KidkArolis, thanks for the ping!
Unfortunately, I haven't really been keeping up with the custom-host-suffix feature because for unrelated reasons, we're still using NPM in the projects I'm currently working on.
Looking at one of our internal VSTS package feeds though, it appears that they "fixed" my original issue by simply adding both URLs with the slightly different paths to the snippet they have you copy to your ~/.npmrc:

//team-name.pkgs.visualstudio.com/_packaging/ProjectName/npm/registry/:_authToken=$TOKEN //team-name.pkgs.visualstudio.com/_packaging/ProjectName/npm/:_authToken=$TOKEN

My work in #3231 was really only picking up an existing solution proposed in #2507. Overall, I think my specific problem with VSTS would still be solved if the custom-host-suffix feature were removed. I remember however that other people who were using other private package feeds were looking forward to the feature too. I don't know if they actually ended up using it though.

Does this help you at all?

Thanks for the info!

My conclusion is then that one could still use VSTS if the ~/.npmrc looked something like:

registry=https://team-name.pkgs.visualstudio.com/ //team-name.pkgs.visualstudio.com/:_authToken=$TOKEN

or if using scopes

@foo:registry=https://team-name.pkgs.visualstudio.com/ //team-name.pkgs.visualstudio.com/:_authToken=$TOKEN

With config like that, authorization headers would be sent correctly. So it's good to know this would work for that VSTS case.

Next, I'll look into how exactly npm handles the case when registry config contains a path, I found the relevant bit of code: https://github.com/npm/npm/blob/5e426a78ca02d0044f8dd26e0c5f881217081cbd/lib/config/fetch-opts.js#L46-L67. But haven't had a chance to look mode deeply yet.

KidkArolis · 2018-01-06T16:54:08Z

src/registries/npm-registry.js

    // this.token must be checked to account for publish requests on non-scopped packages
-    if (this.token || (isToRegistry && (alwaysAuth || this.isScopedPackage(packageIdent)))) {
+    if (this.token || alwaysAuth || this.isScopedPackage(packageIdent)) {


The key change is basically:

We stop checking if request isToRegistry (which is a strange and slightly complicated thing to understand, of course request is being made to a registry, what else would it be made to?)

And instead we make use of this.getAuth to figure out if this specific request needs a token or not.

In fact, we could push these 3 conditions (this.token, alwaysAuth, scopedPackage) to getAuth making this method much cleaner. getAuth takes a url and figures out if authorization is required or not. Should I do that?

KidkArolis · 2018-01-06T16:56:02Z

src/registries/npm-registry.js

@@ -271,6 +249,7 @@ export default class NpmRegistry extends Registry {
      if (registry) {
        return String(registry);
      }
+      return '';


This is to make sure that if URL is with a protocol, e.g. https://github.com, we don't "assume" this is registry.npmjs.org. This change is making getRegistry stricter. Since this method is used only for authentication and scope resolving purposes, we basically make sure to be strict and only return registry.npmjs.org if request is really to that registry.

KidkArolis · 2018-01-06T16:57:33Z

src/registries/npm-registry.js

+
+    if (!baseRegistry) {
+      return '';
+    }


Now that we more strictly check what registry the request is for (e.g. don't conclude a request to https://github.com is to known registry), we can conclude that non registry requests don't need auth. This is basically the replacement for the isRequestToRegistry.

KidkArolis · 2018-01-06T16:59:30Z

src/registries/npm-registry.js

+      availableRegistries.push(DEFAULT_REGISTRY);
+    }
+    return availableRegistries;
+  }


DEFAULT_REGISTRY and YARN_REGISTRY are both available registries even if not mentioned in any of the config. Make sure this is reflected as we now rely on this when extracting registry from a request url.

In theory only DEFAULT_REGISTRY should suffice, but I wanted to make sure that if someone sets their default registry for some reason to be the yarn registry, things should work the same.

bestander · 2018-01-08T08:36:01Z

@KidkArolis thank a lot for a PR and your annotations!
I'd leave this for @BYK @arcanis to review it.

Pleas ping here if review gets stuck at any point

TheLudd · 2018-01-08T10:00:50Z

I installed the build version and after that the error in #4451 (comment) was no longer reproducible. I.e. the bug seems to be solved for me. 👍

KidkArolis · 2018-01-08T11:46:02Z

Currently, multiple functions are used to decide if a given request URL/pkg requires authorization. This leads to subtle issues like the on #4451.

The key point of this PR is to simplify the authorization decision by making sure that getAuth(packageIdent) alone decides whether auth should be used for a particular pkg/url.

The only blocker to this PR currently is figuring out whether pathname matters when comparing a URL against a configured registry. E.g. if someone's registry in .npmrc is @foo:registry=https://myregistry.com/with/path, but requests go to https://myregistry.com/some/hash/pkg.tar.gz - the logic in this PR will not send the authorization token. In master branch, however, a token would be sent if custom-host-suffix is provided in the config file.

KidkArolis · 2018-01-08T23:07:03Z

Wow, this is quite deep. Took me a couple hours to write this comment during which I realized twice some issues with this PR. But I think I've figured some things out, just need a break ;).

tl;dr

This PR is not yet ready to be rolled out. I realized that removing isPackageToRegistry is not good, because request() takes both packageName and requestUrl. Both of those need to be properly checked when considering authorization and currently I only look at packageName.

In the meantime, I've split all the changes into 4 independently reviewable commits if you fancy a look.

The longer version

Thing is, this custom-host-suffix feature was a little odd and I was considering removing it. It all has to do with one edge case where a package registry includes a path and tarballs are at a different path.

An example of a registry without any path:

https://registry.npmjs.org/ - root
https://registry.npmjs.org/@pkg/name - meta
https://registry.npmjs.org/@pkg/name/tarball.tar.gz - tarball

An example of a registry with a path (e.g. Artifactory, Nexus, VSTS):

https://artifactory.example.com/artifactory/api/npm/registry/ - root
https://artifactory.example.com/artifactory/api/npm/registry/@pkg/name - meta
https://artifactory.example.com/artifactory/api/packages/@pkg/name.tar.gz - tarball

The issue that custom-host-suffix is trying to solve is to say, even though the registry root is https://artifactory.example.com/artifactory/api/npm/registry/, let's configure custom-host-suffix as artifactory.example.com. This tells yarn to ignore the path difference in isRequestToRegistry check. This then proceeds to getAuth which uses pkgIdent to determine what auth token to use.

I was gonna explain why I thought custom-host-suffix is no longer needed, but been typing this comment for too long.

Conclusion

In conclusion, I'll revisit this PR to make sure:

The bug it's trying to fix is fixed.
Yarn is still secure when it comes to installing pkgs from authed meta with unauthed tarballs (e.g. private npm registry package, but tarball on public cdn).
Registries with paths (like artifactory, nexus, vsts) are supported with custom-host-suffix for backwards compatibility.
Those same registries are supported without needing custom-host-suffix, but instead using some standard existing config. I'm thinking something like setting @foo:registry=artifactory/npm and also @foo-artefacts:registry=artifactory/npm. We could then deprecate custom-host-suffix if we wanted to. But not sure anyone cares about this much detail at this point.

Oh how I wish we could simplify this code further, it seems quite brittle and has been worked on a lot over the years. I think the main reason this might be so complicated is because request can be called in all of the following patterns:

request('@scope/pkg', {})
request('@scope/pkg', {}, '@scope/pkg')
request('https://registry/@scope/pkg.tar.gz', {})
request('https://registry/@scope/pkg.tar.gz', {}, '@scope/pkg')

But I think they're all needed:

Making requests to outside of registry (e.g. tarballs)
Making requests to registry in context of a pkg (e.g. meta)
Making requests to registry out of pkg context (e.g. non pkg related registry APIs)

Anyway, something to think about..

What about npm?

Haven't looked deeply at their auth code yet. But looks like folks at npm are also dealing with similar issues. Here is an issue with recent comments about artifactory installs not working: npm/npm#16528.

awkaiser · 2018-01-09T22:49:38Z

@KidkArolis thank you so much for your attention to detail on this issue 🍻

lumaxis · 2018-01-10T01:39:33Z

Amazing work @KidkArolis ! Your comment is a perfect summary of the problem and will be a tremendous help for anyone trying to understand why we added custom-host-suffix in the first place 🙏

KidkArolis · 2018-01-14T13:37:55Z

I've opened a new PR with a much smaller change to the code (and bigger change to the tests) - #5216.

Closing this for now as it changed the code too much in a direction that wasn't fully correct.

Fix scoped pkg auth when using npm private pkgs via npm login --scope

a05fc91

Fixes yarnpkg#4157, yarnpkg#4451

KidkArolis mentioned this pull request Jan 6, 2018

Yarn writes registry.yarnpkg.com to lockfile when private scoped packages is from registry.npmjs.org #4157

Closed

KidkArolis commented Jan 6, 2018

View reviewed changes

KidkArolis mentioned this pull request Jan 6, 2018

yarn does not honor authentication settings in .npmrc #4451

Closed

KidkArolis force-pushed the fix-scoped-npm-auth branch from c8159cd to c9dae10 Compare January 8, 2018 21:02

KidkArolis added 3 commits January 8, 2018 21:27

Add some more tests and remove unnecessary code

93dd2ab

Refactor authorization flow

9dcbc8f

Readd the custom-host-suffix feature

ea3fba0

KidkArolis force-pushed the fix-scoped-npm-auth branch from c9dae10 to ea3fba0 Compare January 8, 2018 21:36

KidkArolis mentioned this pull request Jan 14, 2018

fix(auth): Fixes authentication conditions and logic with registries #5216

Merged

KidkArolis closed this Jan 14, 2018

renovate bot mentioned this pull request Feb 6, 2018

chore(deps): update jest monorepo packages to ^22.0.0 #5190

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix scoped pkg auth when using npm private pkgs via npm login --scope #5162

Fix scoped pkg auth when using npm private pkgs via npm login --scope #5162

KidkArolis commented Jan 6, 2018

buildsize bot commented Jan 6, 2018 •

edited

KidkArolis Jan 6, 2018

KidkArolis Jan 6, 2018

bestander Jan 8, 2018

KidkArolis Jan 8, 2018

lumaxis Jan 8, 2018

KidkArolis Jan 8, 2018

KidkArolis Jan 6, 2018

KidkArolis Jan 6, 2018

KidkArolis Jan 6, 2018

KidkArolis Jan 6, 2018

bestander commented Jan 8, 2018

TheLudd commented Jan 8, 2018

KidkArolis commented Jan 8, 2018

KidkArolis commented Jan 8, 2018

awkaiser commented Jan 9, 2018

lumaxis commented Jan 10, 2018

KidkArolis commented Jan 14, 2018

Fix scoped pkg auth when using npm private pkgs via npm login --scope #5162

Fix scoped pkg auth when using npm private pkgs via npm login --scope #5162

Conversation

KidkArolis commented Jan 6, 2018

buildsize bot commented Jan 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bestander commented Jan 8, 2018

TheLudd commented Jan 8, 2018

KidkArolis commented Jan 8, 2018

KidkArolis commented Jan 8, 2018

tl;dr

The longer version

Conclusion

What about npm?

awkaiser commented Jan 9, 2018

lumaxis commented Jan 10, 2018

KidkArolis commented Jan 14, 2018

buildsize bot commented Jan 6, 2018 •

edited