Add integration tests for proxy functionality

[deepy]

Creating a separate issue for the tests, continuing #84.

We're adding --http, --https-proxy, and --noproxy depending on the Gradle configuration, while this works and is manually tested it'd be much nicer to have automated tests.

[bsautel]

I pushed a first integration test that checks that npm uses the proxy defined for Gradle using MockServer to create a proxy on-the-fly and make assertions regarding its usage.

For now, I could get it work only in a subset of the case we want to support. I encountered multiple issues:

1. I cannot get it work with HTTPS. I added the Mock Server certificate to the npm trusted certificates but it still does not work. Maybe someone can have a look and find a solution.
2. I did not find how to setup proxy authentication with Mock Server, so I cannot test it for now
3. I sounds like the parameters we are adding to tell npm to ignore some hosts do not work, npm returns an error, explaining that the command is invalid. We currently add -c noproxy=registry.npmjs.org:80 for instance.

I disabled these use cases in the where part of the test, but they can easily be enabled to test that.

We also should do the same thing for yarn.

I also discovered that we can use some environment variable when using the requests package. The syntax is very similar to the npm parameters, so I assume npm uses requests to download packages but I am not sure about that. If it's the case, maybe we should have better to define these environment variables instead of adding some CLI args to npm, this would make the proxy work for all request calls using requests (which seems to be very popular even if deprecated). Maybe this will help us get the proxy work for npx also?

[deepy]

Sorry about the delayed response, managed to get a nasty wisdom tooth issue. Good job with the tests!

I guess the environment variable would be the easiest way, the documentation for NPM even mentions that requests will honor it https://docs.npmjs.com/misc/config#https-proxy

The only case I can think against it is that right now if someone see's the full npm/yarn invocation all the args we provide are shown, and if we instead change the environment this might not be entirely obvious. Although since we do have it documented and I anyone who needs to set up the plugin in a restricted environment really ought to at least skim the documentation.

With that said I guess it'd be safe for us to add it to npx no matter how we support the others since npx is turning out to be a pretty special case in many regards :-(

[deepy]

The arg is supposed to be --noproxy, I got mixed up with git, sorry about that!

[bsautel]

Thanks for your answer. Hope you are fine now!

I did the change to use some environment variables instead of the CLI args.

I did some experiments but I still cannot get the HTTP proxy test with ignored host working. It sounds like the NO_PROXY variable is not taken into account, or at least not exactly the same way as the request documentation states.

In the HTTP test, I define this registry:

registry=http://registry.npmjs.org/

When I define this proxy configuration:

systepProps.http.proxyHost=localhost
systepProps.http.proxyPort=12345
systepProps.http.nonProxyHosts=registry.npmjs.org

We translate that to:

HTTP_PROXY=http://localhost:12345
NO_PROXY=registry.npmjs.org:80

This is right according to the request documentation.

But the proxy is used whereas it should not be. I noticed that if I modify the code to remove the port in the NO_PROXY environment variable, it works as expected.

I added the port because in Java we define the ignored hosts for HTTP and HTTPS separately. As they are not separated in Node.js, if I don't add the port, I cannot distinguish the protocol.

Any idea about that? You can reproduce that by uncommenting the line 54 of the NpmProxy_integTest in the proxy-tests branch.

[bsautel]

I discovered that npm uses npm-registry-fetch/ to interact with the npm registry, which relies on make-fetch-happen. Its documentation states that here:

opts.noProxy

If present, should be a comma-separated string or an array of domain extensions that a proxy should not be used for.

This option may also be provided through process.env.NO_PROXY.

request seems to support ports in the NO_PROXY environment variable, but it is not the case for the package that is used by npm.

I changed the code to remove the ports in the value we put in the NO_PROXY. This makes we won't be able to support proxy skipping for only a given protocol. This will not probably happen often. As soon as you ignore a proxy host for a protocol in Java, it will be ignored for all protocols in npm.

[bsautel]

I could write successfully a proxy test for Yarn using HTTP. I had to setup a second proxy which is a reverse proxy that forwards to the registry because Yarn enforces the HTTPS usage when using its registry or the npmjs.org registry. It does not enforce HTTPS for other URLs.

I could not get HTTPS proxy work so I commented lines 95 and 96 of the YarnProxy_integTest file. If someone knows how to get it work, please let me know. I get a self-signed certificate error whereas I configured the certificate authority to use. I also tried to disable SSL check (strict-ssl false) but it does not work.

For npm there is also an issue, it is not a certificate issue but a protocol issue. I am also stuck.

It also sounds like the npm proxy test fails on macOS because it uses its cache whereas it should not (--force). I am going to try to find a solution to this issue.

[deepy]

I mostly develop on a Mac so I can have a look at the tests failing, although looking on the branch and on travis they look green, is there something I need to uncomment to work on that?

npm using two different libraries for http requests is kinda annoying and I guess this is the perfect example of why one shouldn't do that.

[bsautel]

For some strange reason, the latest execution of the test passed on macOS. I don't know why, as far as I know I did not change anything regarding that. 🤔

npm using two different libraries for http requests is kinda annoying and I guess this is the perfect example of why one shouldn't do that.

As far as I know, it uses the make-fetch-happen library to access to the repository. This library does not seem to be related to request. Maybe it does not use request at all?

What do you mean when you say that we should not do that?

The remaining work on this issue is to get the HTTPS proxy tests pass. For now, they don't and I have no idea to fix the issue. For that you have to uncomment the where extra cases in the tests for npm and Yarn.

[deepy]

Sorry I should've clarified, I was complaining about npm doing it, not commenting on how we do it.

I've got my hands mostly full at the moment so it'll be a little while until I can do anything.

[bsautel]

I just merged these changes to the master branch. The tests do not work with HTTPS for now but it is better than nothing. This also changed the way we configure the proxy (using environment variables instead of CLI args).

It remains an issue: the tests seem to be unstable on macOS on GitHub Actions. I ran the build again and this time it worked. Don't know why. It sounds like for some unknown reason npm ignores sometimes the --force flag and does not download the packages. Because of that, the download request is not done and the assertion that checks that the proxy did process the request does not work.

[deepy]

Using the environment variables instead of passing parameters does have one downside. The settings do not appear in npm config ls

We could get around that by using NPM_CONFIG_PROXY and similar, but I think the current approach is better. And really I'm just adding this here so that when this confuses me again in half a year I'll find this comment.