Skip to content

Fix missing credentials when fetching tarballs #622

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
Merged
merged 5 commits into from
Dec 11, 2019
Merged

Fix missing credentials when fetching tarballs #622

merged 5 commits into from
Dec 11, 2019

Conversation

rtsao
Copy link
Contributor

@rtsao rtsao commented Dec 6, 2019 •
edited
Loading

What's the problem this PR addresses?

Fixes #601

How did you fix it?

  • Use npmHttpUtils.get in NpmHttpFetcher
  • Add support for absolute paths in npmHttpUtils.get

@rtsao rtsao force-pushed the fix-fetch-private branch from 14736a3 to 5a207b5 Compare December 6, 2019 20:49
@arcanis
Copy link
Member

arcanis commented Dec 9, 2019

I'm not sure about this fix - in particular the %2f codepath should probably be kept exclusive to the semver fetcher, in which case there's little point factorizing this code.

Imo, the best fix would be to simply make NpmHttpFetcher use npmHttpUtils.get instead of httpUtils.get.

@rtsao
Copy link
Contributor Author

rtsao commented Dec 9, 2019 •
edited
Loading

I'm not sure about this fix - in particular the %2f codepath should probably be kept exclusive to the semver fetcher, in which case there's little point factorizing this code.

Imo, the best fix would be to simply make NpmHttpFetcher use npmHttpUtils.get instead of httpUtils.get.

Got it, will update accordingly.

@rtsao rtsao force-pushed the fix-fetch-private branch from 5a207b5 to 1f8fa03 Compare December 9, 2019 20:18
@rtsao rtsao force-pushed the fix-fetch-private branch from 1f8fa03 to c666c9c Compare December 9, 2019 20:52
rtsao
rtsao commented Dec 9, 2019
View reviewed changes
packages/acceptance-tests/pkg-tests-core/sources/utils/tests.ts
@@ -233,7 +233,7 @@ export const startPackageServer = (): Promise<string> => {
[version as string]: Object.assign({}, packageVersionEntry!.packageJson, {
dist: {
shasum: await getPackageArchiveHash(name, version),
tarball: localName === `unconventional-tarball`
tarball: (localName === `unconventional-tarball` || localName === `private-unconventional-tarball`)
Copy link
Contributor Author

@rtsao rtsao Dec 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I was also considering something like this (along the same lines of localName.startsWith(`private`)):

Suggested change
tarball: (localName === `unconventional-tarball` || localName === `private-unconventional-tarball`)
tarball: localName.endsWith(`unconventional-tarball`)

Copy link
Member

Choose a reason for hiding this comment

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

It's ok for now - if we need to add more private packages we will just move the whitelist into a set 👍

@arcanis arcanis merged commit 4765a9a into yarnpkg:master Dec 11, 2019
@arcanis
Copy link
Member

arcanis commented Dec 11, 2019

Awesome, thanks a lot @rtsao! 🎉

@arcanis arcanis mentioned this pull request Dec 11, 2019
Closed
@rtsao rtsao mentioned this pull request Dec 13, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arcanis arcanis arcanis left review comments

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

Successfully merging this pull request may close these issues.

[Bug] Private package credentials should be included when fetching tarballs.
2 participants
@rtsao @arcanis