-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fix request authentication for scoped private registry tarballs #1666
Fix request authentication for scoped private registry tarballs #1666
Conversation
Ah, this is actually a dupe of #1561. |
// Try extracting registry from the url, then scoped registry, and default registry | ||
if (packageName.match(/^https?:/)) { | ||
const parts = url.parse(packageName); | ||
return parts.protocol + '//' + parts.host + '/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A registry may not be just a host — it can also include a path e.g. for registries that are part of a larger artifact hosting application, like Nexus or Artifactory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.. What about looping over all available registries that yarn knows (gathered from the config files) and checking which one does this url begin with? Would that work?
Updated the PR with an extra commit that does not assume anything about the registry URL structure (ah.. except that it starts with |
@KidkArolis can confirm that the patch fixed my "invalid tar file" using a private registry with authentication. thanks! |
ha it seems this patch solved |
and on a side note - the dependencies in the generated |
ef7debc
to
d54977e
Compare
I've fixed the linting issues. I'm currently working on a test for this PR. From what I've understood, there are no existing tests that deal with validating authentication headers. My first attempt was to add a test to This is what I'll try instead. I'll modify the |
If you change your mind and decide that a real HTTP server is a more reliable way to test this, I made a mock private registry module for exactly these kinds of cases. |
An http server is a bit too much for a test here, let's mock it. |
Yes, this PR fixes the same issue, but might be in a slightly more generic way in that my PR handles registries that use a path as part of the registry URL. Thats because instead of regexing a URL I compare all known registries against the request URL to see if there's a match. |
OK. I've added a test now. If I remove my fix - the test fails, with the fix, the test passes. As I was working on this fix, I've discovered the following things.
What are your thoughts on these, shall I file as separate issues? And as to this PR, I'm not 100% sure this is the best way to solve this problem, the whole auth business seems quite tricky and brittle. But this fix will enable using And hopefully, the ability to test private, scoped and authenticated packages with the updated |
Any idea if this will land in a release soon? Like @KidkArolis, my company is eager to drop |
To the maintainers - despite long discussions in this thread - this PR is 100% ready, has a test and shouldn't break any existing functionality! |
I installed yarn manually from your branch, but it doesn't seem to be working for me:
I manually checked the manually installed yarn's code, and it has the built patches that are in this PR. Update: That was directly off of your github repo branch. I also just now checked out yarn @ v0.17.2 and merged your branch on top of v0.17.2, built and installed it, and it's giving me the same error. |
I just now changed
|
@KidkArolis there is a conflict with my commit :( but your PR worked well for me before that :) |
@SEAPUNK looking at your
|
000f7a8
to
c9978b7
Compare
Rebased. |
@bestander let's get this in 0.17.3 too :) |
@KidkArolis I'll try it out, give me a few minutes. Either way, shouldn't it work with my current npmrc? That's the .npmrc file npm writes when I log in via |
@SEAPUNK Well, I tried logging in just now with
I would have thought your config would only work if |
Yeah, I don't do the two flags with This is what my config looks like with the unedited
|
@SEAPUNK I think this maybe a different problem from what @KidkArolis is trying to solve with this PR. |
@thomaschaaf Maybe; all I want is to be able to install private npm packages, which I have been unable to all the time. #839 and #1146 got there part of the way, but no cigar just yet. I've been tracking many various yarn issues and PRs, and so far the point I've gotten to was the whole "invalid tar file" problem, which is what I've been hoping this PR would fix, but if it doesn't fix the problem I'm hoping would be fixed, is there an issue or PR to track for the issue I'm having? To be honest, this is all very confusing and frustrating, since there are so many problems revolving around private packages with yarn, so I have no idea what to look for. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks way better, thanks for simplifying the test setup
|
||
beforeEach(request.__resetAuthedRequests); | ||
// flow doesn't like afterEach | ||
// afterEach(request.__resetAuthedRequests); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably can use $FlowFix annotation to force afterEach for flow.
@@ -775,3 +775,25 @@ test.concurrent('install uses OS line endings when lockfile doesn\'t exist', asy | |||
assert(lockfile.indexOf(os.EOL) >= 0); | |||
}); | |||
}); | |||
|
|||
describe('authed registries', (): void => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to split this test into individual describe
?
I think it should work fine along with the others.
@KidkArolis, please ping when it is ready |
Updated in @6be4745. As a sidenote, unfortunately this PR doesn't in itself fix the And as another sidenote, I do think some of this auth logic can be cleaned up further, but it helps to make small steps first, resolve edge cases and then refactor. I think. |
f0b30ef
to
6be4745
Compare
Hmm, shouldn't this be fixed in the latest nightly? I'm on a scoped package on a private registry (Sinopia) and I'm still getting the auth not being set with the tar issue :(
with my .npmrc looking like
|
I'm still facing this in 0.17.9 (which was supposed to include this patch). I'm using nmpo/npme on the server.
|
@chrisdhanaraj @owais this commit is not in 0.17.9. I'm not sure when it's getting released. |
Got it. I was mislead by this: #1859 Thanks for fixing this @KidkArolis. This is a major blocker for a lot of people to start using Yarn. |
@ maintainers: This has been open for 28 days, merged 12 days ago - any chance it could get released so we get real world feedback on whether this fixes all issues with private repos or whether further fixes are needed? |
Sorry, @KidkArolis, I was spread thin testing the 0.17 branch more extensively and addressing some other issues. |
Yeah, that's great. Sorry for nagging... Just eager to start using yarn..
…On Thu, 1 Dec 2016 at 13:00, Konstantin Raev ***@***.***> wrote:
Sorry, @KidkArolis <https://github.com/KidkArolis>, I was spread thin
testing the 0.17 branch more extensively and addressing some other issues.
I plan to do a 0.18 branch cut this weekend, does it work for you?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1666 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AATzWM_jSyqMdwVEzEoiuWhxNGRgMm09ks5rDsT0gaJpZM4Ko-fh>
.
|
@KidkArolis well... 0.18.0 is here and unfortunately the bug is still here for me :( I get a better error message though!
|
I can confirm 0.18.0 works with private scope perfectly. I'm using npmo/npme to host my private NPM registry. Thanks @KidkArolis |
Actually this might be on me! Sorry guys, think our sinopia instance is having issues :) |
Doesn't seem to work for me with 0.18 either, unfortunately. npmrc:
yarn output:
|
@chrisdhanaraj @rexxars how does your ~/.npmrc file looks like? |
Ah, sorry, completely missed that |
I've made a repo that illustrates this problem: https://github.com/rexxars/yarn-private |
Thanks! Will check it out.
…On Tue, 6 Dec 2016 at 10:17, Espen Hovlandsdal ***@***.***> wrote:
I've made a repo that illustrates this problem:
https://github.com/rexxars/yarn-private
Clone it, run npm install, then npm start, which will start a mock
registry.
Now open a terminal to the target folder within the same repo, and run
yarn.
As you'll see, it will use the wrong token for the registry.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1666 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AATzWLrx-V2eJdV76DjpKaEUE0dxwlhkks5rFTYngaJpZM4Ko-fh>
.
|
Since I'm not sure what is causing this bug, I'm just going to mention that I've made an npm package that will find the correct token for a given URL: https://www.npmjs.com/package/registry-auth-token With that, you should be able to just pass the outgoing request url and get back the corresponding token: console.log(getAuthToken('//some.host/registry/deep/path', {recursive: true})) Not sure if that's relevant, but hey. |
@rexxars I think it works if you modify
to
Did you create this config by file or was this produced by npm/yarn? Looking into the exact reason why that trailing slash is so important to the behaviour.. |
Created a PR to discuss/address this edge case, but not 100% yet what the "npm spec" says on this matter. #2168 |
My configuration was created by npm a while ago. If that's all it takes, I think it's worth fixing ;-) |
I'm getting much further in the install process now, but... still KO's near the end; not sure if this just an artifact of our Sinopia instance being misconfigured or something else though
|
@KidkArolis I've updated the test-repository with a newer mock registry which irons out a small inconsistency between how NPM and Yarn URL-encodes (uppercase vs lowercase). Your PR in #2168 fixes the resolving part, but now it seems like the tarball download does not get an Authorization header added. |
@chrisdhanaraj We're having the same issue, also using sinopia:
My .npmrc is:
Note that it's failing for a public, non-scoped package too (which puts it somewhat out of the scope of this ticket but you are the first person I've seen to post a similar 403 error so I bet we have the same thing). |
Summary
Yarn correctly sends authorization headers when resolving metadata, but
when it's time to download the tarballs, it calls registry.request
with a full tarball url. This causes the getRegistry() function to
incorrectly return the DEFAULT_REGISTRY url which means the authorization
headers are no longer sent. The response in this case is a JSON error
object instead of a tarball file. Untaring fails with "invalid tar file"
Test plan
Before this change, I get "invalid tar file" during the "Fetching packages..." stage.
After this change, the packages download sucesfully.
Follow instructions here to reproduce locally: https://github.com/KidkArolis/yarn-scopes-issue.
Any pointers on how to add a test for this are welcome.