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.

Sign up for GitHub

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

Jump to bottom

Fix the scope regex, allow / in addition to %2f in the url #4367

Merged
merged 3 commits into from
Sep 12, 2017
Merged

Fix the scope regex, allow / in addition to %2f in the url #4367

merged 3 commits into from
Sep 12, 2017

Conversation

KidkArolis
Copy link
Contributor

Fixes #4366

@KidkArolis
Copy link
Contributor Author

Will add a test in a bit.

@KidkArolis
Copy link
Contributor Author

Now with tests

@BYK
Copy link
Member

BYK commented Sep 10, 2017

See #4366 (comment)

I don't think this is the correct solution.

@lukeggchapman
Copy link
Contributor

Following on from #4366 (comment)

I think this might be the best solution.
The request method having to handle both:
@scope%2fpkg and https://npm.registry.com/@scope/pkg/-/pkg-0.0.1.tgz
makes it difficult, without handling of package names and path requests more separately.

I've added some more tests and a fix for getRegistry so that when request is called with a path it uses the registry to get auth rather than the scope, it still relies on isScopedPackage (or always-auth) to flag it as needing auth.
@KidkArolis if you merge/rebase this to the latest master I can add them to your PR, depending on what @BYK wants to do.

@lukeggchapman
Copy link
Contributor

Hey @KidkArolis I improved some tests while investigating but couldn't PR against your fork.
Have a look and if you think they're worth while, either cherry-pick or I can add them.
7a01b96

@KidkArolis
Copy link
Contributor Author

@BYK this PR fixes the private registry issue. Is it possible to get this into the next patch release?

Re master...lukeggchapman:more-scope-tests, I'm happy to:

  1. Cherry pick that into this PR, if we think more tests are better.
  2. First get this PR merged, and then have a separate PR improving the tests.

Whatever gets this fixed sooner.. as it's causing a bit of disturbance, all our Codeship builds are failing since Codeship upgraded to the latest yarn..

@lukeggchapman
Copy link
Contributor

Agreed, lets get this in as soon as possible. I can create a seperate PR after.

@BYK
Copy link
Member

BYK commented Sep 12, 2017

@KidkArolis it is my aim to get this into 1.0.2 and release that ASAP.

@BYK BYK added this to the 1.0.2 milestone Sep 12, 2017
@BYK BYK merged commit 939a130 into yarnpkg:master Sep 12, 2017
This was referenced Sep 12, 2017
Closed
Closed
joaolucasl pushed a commit to joaolucasl/yarn that referenced this pull request Oct 27, 2017
@KidkArolis @joaolucasl
Fix: allow / in addition to %2f in the url for scope checks (yarnpkg#…
4f7a9e7 
…4367)

**Summary**

Fixes yarnpkg#4366. NPM registry encodes the `/` in scoped package names for meta look ups but not for tarball download URLs so Yarn was not sending authentication headers for the tarball downloads breaking scoped packages. This patch fixes it.

**Test plan**

Updated tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BYK BYK BYK approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0.2
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@KidkArolis @BYK @lukeggchapman