Respect proxy settings when resolving git dependencies

[deivid-rodriguez]

• [x] I'd be willing to implement this feature (contributing guide)

Describe the user story

Hello! Dependabot maintainer here.

We're finally adding support for PNPM in Dependabot. During beta testing, an issue has been reported to us where, in presence of some dependencies coming from git sources, Dependabot is failing to run certain pnpm commands.

After digging, I found that when finding a dependency hosted at GitHub, PNPM is running a HEAD request to the github repository using node-fetch. Our updater environment is on a network where all http(s) requests need to go through a proxy. We set HTTP_PROXY and HTTPS_PROXY for that. However, node-fetch does not respect that and causes the request to fail and as a consequence, leads PNPM into thinking the dependency comes from a private repository and using the ssh protocol for it, eventually failing.

Describe the solution you'd like

I tried a naive patch to verify this was the issue and seems to fix things!

diff --git a/resolving/git-resolver/src/parsePref.ts b/resolving/git-resolver/src/parsePref.ts
index d4f799da8..19f6baa35 100644
--- a/resolving/git-resolver/src/parsePref.ts
+++ b/resolving/git-resolver/src/parsePref.ts
@@ -1,5 +1,5 @@
 import url, { URL } from 'url'
-import { fetch } from '@pnpm/fetch'
+import { fetchWithAgent } from '@pnpm/fetch'

 import git from 'graceful-git'
 import HostedGit from 'hosted-git-info'
@@ -91,7 +91,9 @@ async function fromHostedGit (hosted: any): Promise<HostedPackageSpec> { // esli
           // npm instead tries git ls-remote directly which prompts user for login credentials.

           // HTTP HEAD on https://domain/user/repo, strip out ".git"
-          const response = await fetch(httpsUrl.slice(0, -4), { method: 'HEAD', follow: 0, retry: { retries: 0 } })
+          const httpProxy = process.env['HTTP_PROXY']
+          const httpsProxy = process.env['HTTPS_PROXY']
+          const response = await fetchWithAgent(httpsUrl.slice(0, -4), { agentOptions: { httpsProxy, httpProxy }, method: 'HEAD', follow: 0, retry: { retries: 0 } })
           if (response.ok) {
             fetchSpec = httpsUrl
           }

It would need tests and probably we would want to pass down the config parsed here?

https://github.com/pnpm/pnpm/blob/e2293bd6e6033cd26a7e58e151b2770cfd1699d8/config/config/src/index.ts#L499-L508

But I wanted to open an issue first to see if this makes sense and would be accepted. And to ask for help too since I don't know typescript nor PNPM very well 😃.

Describe the drawbacks of your solution

I can't think of any 🤔, maybe slightly more complicated code to maintain?

Describe alternatives you've considered

I thought of changing the HEAD request that PNPM currently performs to detect private repositories with a plain git ls-remote command like it's done in other cases. There's a note that we can't use that because it hangs on a prompt on private repositories, but I did run into the same issue recently and I got away with GIT_TERMINAL_PROMPT to fix that.

I think that's a valid alternative (since git respects proxy env variables), and it would make the code simpler, but I didn't move on with that because I believe it requires changes to several moving parts since I don't think you can't currently pass env to these git commands? It would also need checking which version of git introduced support for GIT_TERMINAL_PROMPT. If it's too recent, we may not want to use it, or may want to leave a fallback in place.

I'm happy to implement this if it's considered better and if I get some advice!

Thanks for reading :)

[deivid-rodriguez]

I'll try to work on a test case for the above patch and propose a PR, to hopefully make it easier to get some feedback!

[jeffwidman]

Any feedback from maintainers on this?

[alex-page]

We have started to adopt pnpm across multiple repositories and are running into this issue with dependabot. It results in multiple unnecessary fetches, timeouts and failures accessing dependencies from private registries.

@deivid-rodriguez @DetachHead can I support fixing this issue? The approach here looks sound it is just missing tests?

[deivid-rodriguez]

@alex-page Yeah, I think some tests was the only thing missing, plus maybe some answers to the questions I posed:

• Is the approach of reparsing proxy env good enough or should pre parsed proxy configuration be passed around instead?
• Is an approach that switches back to using git ls-remotes better?