fix: correctly handle nerf-darted env vars

[jenseng]

Preserve nerf-darted keys verbatim, since the downcasing and dasherizing can mess up both the URIs and the sub-keys themselves.

This will allow you to successfully use env vars to control registry auth, e.g. npm_config_//reg.example/UP_CASE/:_authToken=secret will result in a //reg.example/UP_CASE/:_authToken config

See #64 for more context, but the key details to note are:

1. The current logic can mess up URIs (paths may be case-sensitive, and underscores get converted to dashes)
2. The current logic dasherizes the prefix (e.g. _password becomes -password), which npm-registry-fetch does not handle.
3. The current logic downcases authToken ... although getCredentialsByURI has some handling around authtoken (and the - prefix), it doesn't really matter because npm-registry-fetch does not, so it won't get used.
4. Regarding case-sensitivity and env variables, based on my testing this should be safe/portable because:
   • For Linux/Mac/etc. variables are case-sensitive and you can use unusual characters in names
   • For Windows, although env var accesses are case-insensitive, the original case of the variable name is preserved when iterating or otherwise retrieving the keys, so we can reasonably pull a case-sensitive registry prefix from the key

References

Fixes #64

[jenseng]

Interestingly enough lint works for me locally, appreciate any pointers on where to look for that one, the lines it complains about seem unrelated

[jenseng]

Also let me know if you have any thoughts/recommendations around how much to surface this in the docs.

And of course, env vars for this kind of thing definitely aren't the most user friendly approach (e.g. depending on your shell, you may have to do something like env NPM_CONFIG_//nerf.dart...=... npm install, but based on issues like npm/cli#3985 it seems like there is a need.

[wraithgar]

You will need to run npm update to get the same linting errors locally. There was an update to @npmcli/eslint-config. Now that we're flying without package-locks it's something we have to be in the habit of doing when doing development on these modules.

[jenseng]

I created a separate PR for the eslint fixes, since they're happening on main

[wraithgar]

This PR is on my radar and I plan on getting to it this week. Thank you.

[jenseng]

Thanks @wraithgar!

[wraithgar]

Suggested change

	if (/^\/\/.*?:./.test(key)) {
	// preserve case and dashes within URIs
	let [uri, k] = key.split(':', 2)
	k = this.normalizeKeyFromEnv(k)
	return `${uri}:${k === '_authtoken' ? '_authToken' : k}`
	}
	let [uri, k] = key.split(':')
	if (uri.startsWith('//') && k) {
	// preserve case and dashes within URIs
	k = this.normalizeKeyFromEnv(k)
	return `${uri.slice(2)}:${k === '_authtoken' ? '_authToken' : k}`
	}

This is maybe a 7/10 on the suggestion scale. If you really think the regex is more appropriate here please push back. We tend to default to not adding new regex (and removing existing regex) when possible, given the amount of bugs it's led to in npm the past.

Also, are we intentionally supporting //uri:_authtoken (lower case _authtoken)? If we're not mucking with the case on uris do we still need to do this?

[wraithgar]

disclaimer: I freehand typed that code, it may have linting/syntax errors. You may want to test and commit it locally. I do not care about credit.

[wraithgar]

After sleeping on this I realized that this will eat any urls with ports in them. We probably need to take another stab at this so that doesn't happen.

npm_config_//my.server:4242/foo:_authToken=asdf

This isn't a problem we've had to solve yet because of the way we search through nerf-darted configs right now, (i.e. by url specifically).

lastIndexOf is likely the tool for the job.

if (key.startsWith('//') && key.includes(':')) {
  // preserve case and dashes within URIs
  const uri = key.slice(2, key.lastIndexOf(':'))
  let k = key.slice(key.lastIndexOf(':') + 1)
  k = this.normalizeKeyFromEnv(k)
  return `${uri}:${k === '_authtoken' ? '_authToken' : k}`
}

[jenseng]

Sounds great, I've been offline for a couple days but will push a patch in the next day or so

[jenseng]

re: //uri:_authtoken on that line, it's moreso that I was maintaining the existing "keys are case-insensitive" functionality where possible ... i.e. a nerfed key could have any case coming in (e.g. //uri:_PASSWORD=...), so we normalizeKeyFromEnv the sub-key on the next line. But then _authtoken then needs to be changed to _authToken since that's what various things downstream expect 😅

It's definitely a little weird, so if we're ok with saying that nerfed keys are case sensitive that could simplify this a bit... no need to normalize the sub-key or have special handling for _authToken. WDYT?

[wraithgar]

so if we're ok with saying that nerfed keys are case sensitive

Yes please. It's net new so it doesn't break anything existing.

[wraithgar]

re case sensitivity: Windows is going to be the place to make sure things work as expected. I know you can read from process.env ignoring case, but if you are iterating its keys, does it retain what was originally used to set it?

[jenseng]

Yes, when iterating the keys in Windows you get the original value, e.g. under the Output env and Output env 2 steps here you can see the results of doing for (let key in process.env) console.log(key) under PowerShell and CMD.exe respectively, variable names like mixed_CASE and something_//really-weird.LOL:_ok are preserved

[jenseng]

Ok updated, it's much simpler now 😅

[wraithgar]

That's an understatement!

[wraithgar]

Thanks for your patience here, I ended up doing a deep dive on npm exec last week that took all of my focus. This is great! Just one suggestion and I think we can get this out this week.