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

[Bug] Private package credentials should be included when fetching tarballs. #601

Closed
1 task done
phyllisstein opened this issue Nov 24, 2019 · 7 comments · Fixed by #622
Closed
1 task done

[Bug] Private package credentials should be included when fetching tarballs. #601

phyllisstein opened this issue Nov 24, 2019 · 7 comments · Fixed by #622
Labels
bug Something isn't working reproducible This issue can be successfully reproduced

Comments

@phyllisstein
Copy link
Contributor

phyllisstein commented Nov 24, 2019

  • I'd be willing to implement a fix

Describe the Bug

I'm having some trouble fetching good old Font Awesome from their private NPM registry. When I pipe the requests through a proxy, I can see that Berry sends the bearer token assigned to the package scope when it requests the package metadata:

GET /@fortawesome%2fpro-duotone-svg-icons HTTP/1.1
User-Agent: yarn/1.19.1 npm/? node/v13.1.0 darwin x64
Accept: application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*
authorization: Bearer ...
host: npm.fontawesome.com
accept-encoding: gzip, deflate
Connection: close

This yields a 200 OK with a JSON blob. But when Berry initiates a request for the file itself, it does so without the authorization header:

GET /@fortawesome/pro-duotone-svg-icons/-/5.11.2/pro-duotone-svg-icons-5.11.2.tgz HTTP/1.1
user-agent: got/9.6.0 (https://github.com/sindresorhus/got)
accept-encoding: gzip, deflate
Host: npm.fontawesome.com
Connection: close

...and gets 401 Unauthorized.

I believe this is orthogonal to #329 and #334: the hostname is identical, and the responses were not redirects. Attempting the same installation in NPM, I can see that it passes the token along when grabbing the archive:

GET /@fortawesome/pro-duotone-svg-icons/-/5.11.2/pro-duotone-svg-icons-5.11.2.tgz HTTP/1.1
connection: keep-alive
user-agent: npm/6.13.0 node/v13.1.0 darwin x64
npm-in-ci: false
npm-scope:
npm-session: d735b377662ba212
referer: install [REDACTED]
pacote-req-type: tarball
pacote-pkg-id: registry:@fortawesome/pro-duotone-svg-icons@https://npm.fontawesome.com/@fortawesome/pro-duotone-svg-icons/-/5.11.2/pro-duotone-svg-icons-5.11.2.tgz
authorization: Bearer ...
accept: */*
Host: npm.fontawesome.com

The Font Awesome folks are hosting their private repository through Cloudsmith, in case that's relevant.

I'd be happy to work out a fix, if I can! A morning's stumbling around looking for the right request to doctor hasn't done me any good yet, but maybe someone could point me in the right direction.

To Reproduce

const { promises: fs } = require('fs')

// Temporary test token, be gentle. 🙊
const YARNRC_YAML = `
npmRegistries:
  //npm.fontawesome.com:
    npmAuthToken: 565D441C-07C8-4B3E-81A2-CFF2F8D3B7D9
npmScopes:
  fortawesome:
    npmRegistryServer: https://npm.fontawesome.com
`

await fs.writeFile('.yarnrc.yml', YARNRC_YAML)

await expect(packageJsonAndInstall({
  dependencies: {
    '@fortawesome/pro-duotone-svg-icons': '5.11.2',
  },
})).rejects.not.toThrow()

Environment if relevant (please complete the following information):

  • OS: macOS 10.15.1 (19B88)
  • Node v13.1.0
  • Yarn v2.0.0-rc.12

Additional context

Thanks for your help! Excited to see Berry bear fruit.

@phyllisstein phyllisstein added the bug Something isn't working label Nov 24, 2019
@yarnbot
Copy link
Collaborator

yarnbot commented Nov 24, 2019

We couldn't reproduce your issue (all the assertions passed on master).

@yarnbot yarnbot added the unreproducible This issue cannot be reproduced on master label Nov 24, 2019
@phyllisstein
Copy link
Contributor Author

Updated the test case---I had the assertion backwards. 😳

@yarnbot
Copy link
Collaborator

yarnbot commented Nov 24, 2019

This issue reproduces on master:

Error: expect(received).rejects.not.toThrow()

Error name:    "Error"
Error message: "Command failed: /usr/bin/node /github/workspace/scripts/actions/../../packages/yarnpkg-cli/bundles/yarn.js install
"


    at Object.args [as toThrow] (/github/workspace/.yarn/cache/expect-npm-24.8.0-8c7640c562-1.zip/node_modules/expect/build/index.js:242:20)
    at module.exports (evalmachine.<anonymous>:20:17)

@yarnbot yarnbot added reproducible This issue can be successfully reproduced and removed unreproducible This issue cannot be reproduced on master labels Nov 24, 2019
@yarnbot
Copy link
Collaborator

yarnbot commented Nov 24, 2019

This issue reproduces on master:

Error: expect(received).rejects.not.toThrow()

Error name:    "Error"
Error message: "Command failed: /usr/bin/node /github/workspace/scripts/actions/../../packages/yarnpkg-cli/bundles/yarn.js install
"


    at Object.args [as toThrow] (/github/workspace/.yarn/cache/expect-npm-24.8.0-8c7640c562-1.zip/node_modules/expect/build/index.js:242:20)
    at module.exports (evalmachine.<anonymous>:20:17)

@arcanis
Copy link
Member

arcanis commented Nov 25, 2019

Thanks for providing a token - usually those issues are so much harder to solve because we have no repro, it's refreshing to actually get one 😄

I think the problem is caused by an interaction with unconventional registries (ie those that don't respect the /@scope/name/-/name-version.tgz endpoint). Those resolutions go through a different fetcher than the rest of the npm packages, and it doesn't seem to use the npm http utils, which would otherwise set the authorisation header. I think this originally was because the npm http utils add the hostname themselves, but we should simply skip this if the url path is absolute 🤔

Wanna try to make the change?

@crubier
Copy link
Contributor

crubier commented Nov 27, 2019

Same bug here, been blocked for 2 days on it. Trying to push a PR for this now.

@phyllisstein
Copy link
Contributor Author

Forgive me, I got a little distracted. Looks like some good stuff from @crubier and @rtsao, though---thanks for picking up my slack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working reproducible This issue can be successfully reproduced
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants