New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proxy from environment variables is ignored if no_proxy or NO_PROXY environment variable is present #3904
Comments
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 |
can you please provide an example.
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. |
Proxy configuration provided with ENV vars (HTTP_PROXY, HTTPS_PROXY and NO_PROXY). |
@Steve-Mcl Is there anything else i can provide you for fixing this issue? |
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:
Another option would be to introduce |
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 Note to self: double check how the built in proxy works - may have to apply |
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:
The proposed implementation is based on the following sources:
The proposed implementation proposal follows the following rules:
Proposed means of use (open to discussion)
|
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
All tests pass. (2) Re-enable disabled parts of proxy testsCurrently, all lines After removing the comment marks, the tests now fail:
PROBLEM: This suggests the HTTP Request node is NOT hitting the proxy (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")
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 proxyUsing "express" and "http-proxy-middleware", I rewrote the proxy endpoints in the tests Now the proxied endpoint gets called, adds the |
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) |
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
The text was updated successfully, but these errors were encountered: