Avoid creating bad URLs when a mitm request has the wrong scheme

[mctofu]

If a mitm request came in like http://host/path this was getting mangled to https://hosthttp://host/path. Instead, we should just swap the scheme to https if the scheme is already defined in the url.

[elazarl]

Can you describe the bug that this fixes?

[mctofu]

We're still investigating this & working on a test but have been observing that the npm cli is not always working with the proxy in mitm mode.

When running npm install [email protected] on a fresh project we've seen these requests to the proxy:

CONNECT registry.npmjs.org:443 HTTP/1.1
GET http://registry.npmjs.org:443/npm HTTP/1.1

This proxy then detects the url does not match the https regex and transforms it to https://registry.npmjs.org:443http://registry.npmjs.org:443/npm. This eventually fails dns resolution later on and closes the connection without returning an error.

Other requests from the npm cli use https instead of http so this doesn't seem to be consistent and we haven't looked to see what the cause is yet.

I'm not sure what the right behavior is in this case. I've observed that mitmproxy v8.1.1 seems to be allowing this by converting the url to https.

[elazarl]

but it shouldn't use GET http://... should it? If it really uses goproxy as a standard HTTP proxy

[mctofu]

but it shouldn't use GET http://... should it? If it really uses goproxy as a standard HTTP proxy

It does seem strange but I'm not positive it's not allowed. I found this request does work successfully when I configure npm to use a squid proxy without mitm handling so registry.npmjs.org is not rejecting the request.

[jeffwidman]

@mctofu any suggestions on next steps here?

[mctofu]

@jeffwidman I think we need to add a test case for this before undrafting.

We could also followup with the npm team on this behavior. I have a vague memory of running into this before.

[ErikPelli]

This issue states that in npm 10.8.3 the problem has been resolved: dependabot/dependabot-core#10623

Can you confirm that now it works and, if so, close this pull request? If it was an NPM problem, we don't need to change anything