[BUG] Setting $GIT_SSH_COMMAND is a bad idea

[ebarzilay-godaddy]

I originally ran into a very hard to trace issue that resulted from using a newer npm. After much head scratching, I figured out that the problem is that npm is setting $GIT_SSH_COMMAND in a way that kills the script. In my case, the culprit was using a git config setting that sets the ssh command, but $GIT_SSH_COMMAND will override that setting. It's essentially the same problem reported in this comment. This is of course related to #2891, but a different problem. There's more relevant pointers in those issues, and I'll add this commit as the specific one that lead to this problem. (Though it looks like there was always an intention to set it.)

Having gone through a bunch of issues in this neighborhood, I see that this is not the first time that it bites someone. But more than that -- IMO it's actually bad to actually try to do the StrictHostKeyChecking tweaking that this thing is doing. Yes, it can be annoying to figure out the ssh-keyscan that is needed to pre-populate known hosts, but since this is potentially a security issue, I'd argue that disabling it is a BAD idea which should be left for people who want to do that. Even more so in a world where ephemeral runners are common, so you're constantly exposed to the risks that come with the disabling. (The original, plain usage of ssh makes it less dangerous, since you'd get the auto-trusted to happen on the first time you ssh to a new host, but then you'd have that persist. This is why disabling this and using it in an ephemeral runner is a bad idea.)

I therefore think that this particular setting should be removed. Or at least do it based on some setting that is not the default, so it's easier to do the ssh tweaking for people who opt into it (and the docs for that option would be a good place to warn people that it works by setting that env var).

As for the problem of existing things breaking, it shouldn't be too difficult to look at stderr, and if there's a message about an unknown host error, then add some text about the new option (whose page should describe the risk and what could be done to avoid it).

[dennishenry]

Opened a PR as an attempt to resolve this: #194

[wraithgar]

Based on the difficulty in tracking down the bug in #193, I think you can appreciate why npm is setting oStrictHostKeyChecking=accept-new. This was the source of countless issues every time the github ssh servers had a new IP address. The solution, outside of using that flag, is also pretty onerous for the average npm user.

So while we can appreciate that this is an issue for folks who rely on custom ssh configs, any solution can not break the experience for the vast majority of npm users. Its addition was a hard earned lesson in user experience.

Also, this commit was likely not the introduction of this issue. As you can see it was just shuffling around an existing implementation that still used that param. That PR was made to stop removing existing values that were not set by npm itself. This was introduced in when git references were defaulted to using ssh in npm 7. There is a longstanding issue here where folks in the comments have listed some workarounds iirc.

The long and the short of it is that git references in npm have always had edge cases and problems. We don't even store checksums in lockfiles anymore because git links aren't idempotent: they can point to branches that may be different every time you install a package. They are discouraged from general use.

[ebarzilay-godaddy]

@wraithgar --

1. Even a configuration option that would default to having this option on would be better than the current state, since currently I just cannot use a local ssh config (as it's overridden anyway by the env var). I had to resort to moving all of my conf into a command-line that is put in the env var.

   Like I said, an option would also make a natural place to mention this behavior, since AFAICT, it is currently not described anywhere.

2. The cause of that, which is the main source of annoyance here, is that setting the env var doesn't compose with anything people have around it. It turns on something that npm wants, at the cost of possibly turning off stuff that I want. (And the overall hassle that this choice leads to is due to this fundamental conflict...)

   I personally believe that this is never a good way to manage things. But doing the above would be a good first step towards at least making it more known and save the headaches for people.

3. As for whether such an option should be on or off by default -- I'm on the side of having it off, because it's a security feature which you're turning off for (almost) everyone. But if you're (collectively) on the other side, then another good step is to do exactly what ssh itself is doing: show a warning the first time an unknown host is blindly trusted -- and since ssh is doing that anyway, then all you need is to let that message through to the npm output. (I didn't actually test it, since my build does set the GH servers, so it might be doing that already.)

[dennishenry]

@wraithgar This was a fair callout. I've modified my PR to now only overwrite if the git config is does not havecore.sshCommand set or if core.askpass is not set. This now handles both the cases if the environment variable is set or the .gitconfig is set. I've updated all tests as well to ensure that this covers all cases.

[dennishenry]

This issue is now resolved in npm version 10.8.2 which has my PR above in it: https://github.com/npm/cli/releases/tag/v10.8.2

@wraithgar I think this can be closed now

[ebarzilay-godaddy]

@dennishenry -- Thanks!!