Proxy from environment variables is ignored if no_proxy or NO_PROXY environment variable is present

[Vovcharaa]

Current Behavior

Proxy is completly ignored in http request node from HTTP_PROXY env variable if NO_PROXY environment variable is present.

Expected Behavior

HTTP_PROXY and NO_PROXY works as expected.

Steps To Reproduce

set HTTP_PROXY and NO_PROXY vars and try to make request with http request node.

Example flow

No response

Environment

• Node-RED version: 3.0.2
• Node.js version: v12.22.12
• npm version: 6.14.16
• Platform/OS: official docker container node:12-alpine
• Browser: Chrome

[Vovcharaa]

Problem seems to be here: https://github.com/node-red/node-red/blob/master/packages/node_modules/%40node-red/nodes/core/network/21-httprequest.js#L519

I have hostname telegram in no_proxy but because of this line request url api.telegram.org is a match for that method.

[Steve-Mcl]

can you please provide an example.

• What did you set NO_PROXY to
• What did you set HTTP_PROXY to
• What was the http request requesting to?

Proxy is completly ignored in http request node

Can you clarify what you mean by "proxy" here. The proxy set in the HTTP Request node? the PROXY setting in ENV var?

A demo flow and ENV settings would be useful to understand if there is really an issue or not.

[Vovcharaa]

Proxy configuration provided with ENV vars (HTTP_PROXY, HTTPS_PROXY and NO_PROXY).
HTTP_PROXY and HTTPS_PROXY is internal company proxy server.
NO_PROXY contains telegram,viber which are hostnames of other containers in docker-compose.
Url used for testing: https://api.telegram.org/bot<token>/getMe
From what I understand if NO_PROXY contains any part of url used in request, proxy is ignored, which is wrong.
Expected NO_PROXY behavior described here (this package used in axios for proxy configuration. Axios working perfectly with same NO_PROXY and HTTP_PROXY env vars)

[Vovcharaa]

@Steve-Mcl Is there anything else i can provide you for fixing this issue?

[knolleary]

I think the issue is now well understood - thanks for the detailed information @Vovcharaa

The fix is relatively straightforward to apply - however we need to consider the impact this will have. We've supported this property for a long time without any issues raised. So either no-one has been using it, or they have used it and might rely on the existing partial-matching approach it uses.

If we change the behaviour to do more strict checking, it will break their flows. That is something we try to avoid - especially in a maintenance release.

Our default position on breaking changes is that they go into the next major release (4.x next April). But when it's a fix, we'd want to find a way to ship it sooner whilst minimising the disruption to users.

Options for us to consider:

• Ship in a maintenance release (3.0.x) and risk breaking users
• Ship in milestone release (3.1.x) and still risk breaking users - but with more ability to highlight the change
• Ship in 4.x next April - as a breaking change that would be our default position - but that doesn't help you today

Another option would be to introduce NO_PROXY_STRICT in 3.0.x/3.1.0 as a new env var that behaves as you expect NO_PROXY to behave. We can then update NO_PROXY as a breaking change in 4.0 to match. That gives you a way to move forward sooner and avoids breaking other users.

[Steve-Mcl]

Another option would be to introduce NO_PROXY_STRICT in 3.0.x/3.1.0 as a new env var that behaves as you expect NO_PROXY to behave. We can then update NO_PROXY as a breaking change in 4.0 to match. That gives you a way to move forward sooner and avoids breaking other users.

I think this is a reasonable direction Nick.

As a slight iteration - perhaps it might be sensible to keep this behaviour beyond v4 as it is well established (in node-red) & simply introduce the "stricktness" via a bool flag like PROXY_STRICT that defaults to false in 3.1.x (i.e. as is now) and default it to true in v4.x? If missing, we default to expected condition based on the version of node-red.

Note to self: double check how the built in proxy works - may have to apply PROXY_STRICT or introduce a checkbox in the proxy UI

[Steve-Mcl]

A quick update for anyone following this thread.

I have this 95% complete but in doing this work a number of issues have been realised. I want to get this out there

Issues with the current *_proxy implementation in Node-RED:

• In the http-request node, the proxy URL is only determined when the node is instantiated - meaning when using a dynamic URL, the proxy is NOT re-evaluated per request
• no_proxy should not be case-sensitive
  • i.e. if no_proxy contains "example.com", then "example.com" and "EXAMPLE.COM" should both be excluded
• no_proxy with protocols that have a default port are not considered
  • i.e. if no_proxy contains "example.com:443", then "https://example.com" should be excluded
  • i.e. if no_proxy contains "example.com:80", then "http://example.com" should be excluded
  • i.e. if no_proxy contains "example.com:1880", then "mqtt://example.com" should be excluded
• Does not consider NPM proxy configuration at all
  • i.e. if npm_config_proxy is set, then it should be used
  • i.e. if npm_config_https_proxy is set, then it should be used
  • i.e. if npm_config_http_proxy is set, then it should be used
  • i.e. if npm_config_no_proxy is set, then it should be used
• Doesn't consider https_proxy or HTTPS_PROXY
  • i.e. if https_proxy is set, then it should be used
  • i.e. if HTTPS_PROXY is set, then it should be used
  • i.e. incorrectly uses HTTP_PROXY 'http://http-proxy' when the url is 'https://example'
• Incorrectly prioritises HTTP_PROXY over http_proxy. HTTP_PROXY is not always supported or recommended
  • i.e. if HTTP_PROXY and http_proxy are both set, then http_proxy should be used
  • Use lowercase form. HTTP_PROXY is not always supported or recommended
• doesn't consider all_proxy or ALL_PROXY
  • i.e. if all_proxy is set, then it should be used
  • i.e. if ALL_PROXY is set, then it should be used

The proposed implementation is based on the following sources:

• https://about.gitlab.com/blog/2021/01/27/we-need-to-talk-no-proxy/ (GitLab)
• https://www.npmjs.com/package/proxy-from-env (MIT License)

The proposed implementation proposal follows the following rules:

• Support the following PROTOCOL_proxys
  • i.e. http_proxy, https_proxy, mqtt_proxy, ws_proxy, wss_proxy, mqtt_proxy, mqtts_proxy
• Support all_proxy
  • i.e. if all_proxy is set, then all URLs will be proxied
• Use comma-separated hostname[:port] values for no_proxy.
  • no_proxy should contain a comma-separated list of domain extensions proxy should not be used for
  • Each value may include optional whitespace.
  • port is optional and is inferred if the protocol has a default port (supports http, https, mqtt, ws, wss, mqtt, mqtts)
  • Use * to match all hosts
• Support .example (host suffix)
• Support sub.example (host sub domain)
• Upper case forms of *_PROXY are supported but not recommended
• Lower case forms of *_proxy will take precedence over upper case forms
• Does not perform DNS lookups or use regular expressions
• Does not perform validation on the *_proxy urls
• Does not support CIDR block matching
• Support IPv6 matching

Proposed means of use (open to discussion)

• Presence of env var NODE_RED_PROXY_MODE with value of 'strict' or 'legacy'
• Presence of settings.js entry proxyMode with value of 'strict' or 'legacy'

[Steve-Mcl]

Further testing has revealed more worrying issues with proxying.

TLDR;

As far as I can see, the HTTP Request node doesn't work with all proxies. Additionally, it seems the tests are flawed and the proxying is not ever tested.

@knolleary @hardillb I know you guys had some issues with tests and proxy when implementing GOT, could you please comment on my findings? Am I missing something?

Long version...

Tests and modifications were carried out on both Windows 11 + nodeJS 16 AND alpine linux + nodeJS 18

(1) Run tests (as is) master 3.0.2

mocha test/nodes/core/network/21-httprequest_spec.js

All tests pass.

(2) Re-enable disabled parts of proxy tests

Currently, all lines msg.payload.headers.should.have.property('x-testproxy-header','foobar'); are disabled in 21-httprequest_spec.js.

After removing the comment marks, the tests now fail:

 1) HTTP Request Node
       protocol
         should use http_proxy:
     AssertionError: expected Object {
  'user-agent': 'got (https://github.com/sindresorhus/got)',
  'content-length': '3',
  host: 'localhost:10236',
  connection: 'keep-alive'
} to have property 'x-testproxy-header

PROBLEM: This suggests the HTTP Request node is NOT hitting the proxy
Since the tests complete when the header test is disabled, it suggests the proxy is not used. It is not altogether proof that the proxy is not working just that the onProxyReq was not called.

(3) changing the agent in http-request to ensure proxy is "hit"

Changing the proxy agents to use "http-proxy-agent" and "https-proxy-agent" packages (instead of "hpagent")

  1) HTTP Request Node
       protocol
         should use http_proxy:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/steve/github/node-red/test/nodes/core/network/21-httprequest_tests_enabled_spec.js)

This suggests the HTTP Request Node Agent is now calling to the proxy IP:PORT but in-fact the proxy lib being used in the tests doesnt actually work!

(4) Modify test _spec to use different proxy

Using "express" and "http-proxy-middleware", I rewrote the proxy endpoints in the tests

Now the proxied endpoint gets called, adds the x-testproxy-header header, calls to the proxied IP:HOST - all tests passing.

[hardillb]

It's been a while, but I think the problem is as you found that the simulated proxy used in the tests was what didn't work, not that GOT didn't work with proxies.

[Steve-Mcl]

It's been a while, but I think the problem is as you found that the simulated proxy used in the tests was what didn't work, not that GOT didn't work with proxies.

I (kinda) agree but if I am being honest, that is only due to lack of complaints. But the other part of me suspects that may be down to using proxies is far less common for most users. I am just a little uncomfortable changing stuff in an area where the test dont work correctly.

The short of it:

In the efforts to be as close as possible to a "standard" (i.e. more like cURL - see previous post about "standards") while remaining compatible with legacy - I cannot confirm everything works unless I update the agent to "http-proxy-agent" and "https-proxy-agent" packages (instead of "hpagent") & include fixed up tests.

For a bit of a comfort blanket: http(s)-proxy-agent averages over 120M downloads per month (hpagent averages around 3M/mon) : https://npmcharts.com/compare/hpagent,http-proxy-agent,https-proxy-agent?interval=30

So I will raise a PR with the modifications I made to get all proxy tests working property along with this feature (and additional related tests for this feature). I will include a detailed description of reasons for choices & info that should be useful for DOCS once PR vetted & accepted (in principle)