Ensure passwords in hosted Git URLs are correctly escaped

[stevenhilder]

What

Hosted Git URLs may be specified in the format protocol:https://username:password@domain/path.

However, when the username and/or password contain certain punctuation characters (notably : or @), this library fails to return the credentials with these characters correctly escaped. The result is that npm either fails to connect because the hostname has been parsed incorrectly, or fails to authenticate with the host because the credentials have been parsed incorrectly.

const HostedGitInfo = require('hosted-git-info');

const username = 'user:n@me';
const password = 'p@ss:word';
const auth = encodeURIComponent(username) + ':' + encodeURIComponent(password);
const url = 'https://' + auth + '@github.com/npm/hosted-git-info.git';
const parsed = HostedGitInfo.fromUrl(url);

                          // expected: user%3Ana%40me:p%40ss%3Aword
console.log(parsed.auth); // actual: user:na@me:p@ss:word

Why

The reason for this is that the "Legacy" API of Node.js' url module does not escape these characters in the auth property of the parsed URL.

See https://nodejs.org/api/url.html#url_percent_encoding_in_urls for details.

How

Node.js' newer, WHATWG-compatible API does escape the username and password properties of the parsed URL. The fix that I propose in this PR is to continue using the Legacy API to minimize code changes and therefore risk of regression, but to use the WHATWG API to return the parsed username and password only when auth is included in the URL.

[isaacs]

This sounds reasonable, but definitely needs a test to be accepted.

My concern is ensuring that this doesn't cause any problems with how pacote et al make the request for the module. Do you have an example where this currently fails in the CLI, so that we can ensure it is fixing a real problem? If it currently doesn't fail in the CLI, then that means it probably will fail with this change (which might still be reasonable, but means that other changes will be required to integrate the version of hosted-git-info/npm-package-arg that includes this change, making it semver-major).

For npm v7, we've been moving towards using the WhatWG-style urls throughout the CLI. I'm assuming that might change the approach here, since escaped auth would be the default? If it turns out to be a semver-major change, we may as well go all the way and use the new parsing style, especially if it's not possible (or just not worth the effort) to pull this in until npm v7 anyway.

[stevenhilder]

Test added, along with fix for edge-cases where only username or password is provided, not both.

Yes, I encountered the problem in the CLI and tracked the error via pacote and npa through to this module.

With a package.json file similar to the following...

{
  "dependencies": {
    "my-dependency": "https://my-username:p%[email protected]/my-username/my-dependency.git"
  }
}

...the CLI fails with an error similar to the following...

npm ERR! Error while executing:
npm ERR! /usr/bin/git ls-remote -h -t https://my-username:p@[email protected]/my-username/my-dependency.git
npm ERR! 
npm ERR! fatal: unable to access 'https://[email protected]/my-username/my-dependency.git/': Could not resolve host: [email protected]
npm ERR! 
npm ERR! exited with error code: 128

With my fix, I am able to run npm install successfully with no impact (that I could find) on unauthenticated URLs or authenticated URLs without special characters. Therefore I believe this is a bugfix, not a semver-major change.

---

For npm v7, bear in mind that the newer URL API doesn't provide an .auth property on parsed URLs. You'll have to build the auth from .username and .password the same way I did here; but yes, it will be escaped by default (including in .href).

[isaacs]

Thanks for looking into that @stevenhilder! It looks like this is reasonable to try to get into the next npm/cli v6 release as a bugfix then.

[darcyclarke]

✅ LGTM; That said, @isaacs are we sure we can pull this in to [email protected] as we're currently on [email protected]? I guess the biggest Q there is whether there were serious breaking changes in [email protected]