fix(nodeenv): read quoted config values and mirror credentials - #426
Merged
Merged
Conversation
ConfigParser keeps quotes, so the values quoted as in the README (`mirror = 'https://...'`) broke with "unknown url type: 'https". Config._load() now strips one matching pair of quotes. urllib takes user:password@ in a URL for a part of the host, so a mirror behind a login could not be used. main() now cuts it off src_base_url and urlopen() sends it as a Basic Authorization header, only to URLs under the mirror and not along redirects. The password no longer shows up in error messages either. Fixes #321
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #321
What happens
ConfigParserkeeps quotes as part of the value. The README shows the config values quoted (node = 'latest',jobs = '2'), somirror = 'https://...'became the URL'https://...'and failed withunknown url type: 'https.node = 'latest'broke the same way.user:password@in a URL for a part of the host.--mirror=https://user:pass@host/...ended in an uncaughthttp.client.InvalidURL: nonnumeric porttraceback, or in a DNS error when the URL had a port. The request never reached the mirror, and the error message printed the password.What changes
Config._loadstrips one matching pair of quotes from string values.main()cutsuser:password@offsrc_base_urlwith the newsplit_url_auth(), percent-decodes it and keeps it as a BasicAuthorizationheader insrc_auth, the waycertifi_contextis built once.urlopen()adds the header only to URLs under the mirror, so the npm registry does not get it, and as an unredirected header, so a redirect (for example to a presigned storage URL) does not carry it either. Sincesrc_base_urlholds no password any more, the error messages do not show it.This differs from #322 in two points:
install_openerthere is bypassed when_urlopenpassescontext=(--ignore-ssl-certs,--with-certifi), and the quotes were stripped formirroronly.Tests
All six new tests fail on master and pass with the fix:
main()against a local HTTP server that asks for Basic auth, with a plain and a percent-encodeduser:password@;