Skip to content
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

Perform Proxy logic more like cURL #4616

Merged
merged 15 commits into from
May 30, 2024
Merged

Conversation

Steve-Mcl
Copy link
Contributor

@Steve-Mcl Steve-Mcl commented Mar 17, 2024

closes #3904

Types of changes

Introduces a helper function getProxyForUrl that computes the proxy to use based on Env Vars and options provided in accordance with the discussion and linked articles in the issue

Proxy Variables Handling:

  • No proxy variables: The function correctly returns an empty string for various protocols when no proxy variables are set.
  • Invalid URLs: The helper function returns an empty string for invalid URLs.
  • Proxy options:
    • allowUpperCase:false: Prevents *_PROXY values being returned when set to false.
    • favourUpperCase:false: Causes *_PROXY to be used before *_proxy when set to false. The expectation is that lowercase are preferred and get priority
    • includeNpm:false: Does not return npm_config_*_proxy env vars when set to false.
    • legacyMode:false: Processes URLs proxy in node-red <= v3.1 compatibility mode.

Proxy Environment Variables:

  • Handles environment variables like http_proxy, HTTP_PROXY, https_proxy, HTTPS_PROXY, ftp_proxy, ws_proxy, mqtt_proxy, all_proxy, no_proxy, etc.
  • Properly handles different configurations of these variables, including lowercase has priority over uppercase, scheme handling, with/without port, default port numbers (e.g. http:80), IP addresses (v4/v6), host suffix, wildcard matching, and case sensitivity.
  • Takes precedence of some variables over others based on the configurations.

NPM Proxy Configuration:

  • Works with npm_config_http_proxy, npm_config_https_proxy, npm_config_proxy, npm_config_no_proxy.

  • Handles precedence of NPM proxy configurations over environment variables like HTTP_PROXY, HTTPS_PROXY, NO_PROXY.

  • Overall, the "Proxy Helper" function effectively handles various proxy configurations and environment variables, providing consistent and expected results according to the specified configurations.

  • Bugfix (non-breaking change which fixes an issue)

  • New feature (non-breaking change which adds functionality)

Proposed changes

  • HTTP, ws and MQTT nodes will all determine the proxy by calling to this new helper function
  • HTTP node will compute the proxy for dynamic URLs (currently only computed against the nodes edit panel URL field)
  • support overriding with system env var NR_PROXY_MODE (setting to legacy reverts functionality)

TODO

  • Tests & Docs for NR_PROXY_MODE

Checklist

  • I have read the contribution guidelines
  • For non-bugfix PRs, I have discussed this change on the forum/slack team.
  • I have run npm run test to verify the unit tests pass
  • I have added suitable unit tests to cover the new/changed functionality

@Steve-Mcl Steve-Mcl changed the title Proxy logiv dev v4 Perform Proxy logic more like cURL Mar 17, 2024
@knolleary
Copy link
Member

When are the npm config options used? I can't see any reference to them in the original issue other than when you bring them into the discussion and google searches don't really reveal anything about how they expected to be used beyond npm's own use.

@Steve-Mcl
Copy link
Contributor Author

Steve-Mcl commented May 30, 2024

When are the npm config options used? I can't see any reference to them in the original issue other than when you bring them into the discussion and google searches don't really reveal anything about how they expected to be used beyond npm's own use.

If node-red is launched via NPM and the npmrc (or CLI) has specified proxy options, I believe the correct thing to do is respect them. In the interest of getting this in, I could disable the npm proxy parts (there is an option excludeNpm we could default to true

There are a number issues and inconsistencies this covers including:

  • 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
  • no_proxy with protocols that have a default port are not considered
  • Does not consider NPM proxy configuration at all (we could ignore this for now as discussed per above)
  • Doesn't consider https_proxy or HTTPS_PROXY
  • Incorrectly prioritises HTTP_PROXY over http_proxy. HTTP_PROXY is not always supported or recommended
  • doesn't consider all_proxy or ALL_PROXY

So it would be great if we can get it in.

Copy link
Contributor Author

@Steve-Mcl Steve-Mcl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are the places to change to exclude npm proxy options

@Steve-Mcl Steve-Mcl requested a review from knolleary May 30, 2024 10:50
@knolleary
Copy link
Member

One concern I have is how many settings this is added to the settings file - for a topic that has very rarely caused any questions or issues.

Why can we not just have a standard behaviour in terms of upper-case and lower-case? Document which we will use in preference to the other and not expect users to have to understand all of these flags that have been added to the settings file.

@Steve-Mcl
Copy link
Contributor Author

We could be opinionated and expose the finer control at a later if that is an issue.

I mean, we dont really have to add anything at all to the settings file (since it has defaults) but we may wish to keep the mode: "legacy" flag listed out as a formative option.

e.g. settings.js

    // proxyOptions: {
    //     mode: "legacy", // legacy mode is for non-strict previous proxy determination logic (node-red < v4 compatible)
    // },

That way, the user can be least change back to legacy (if needed) and the unlisted options are considered "unsupported"/"subject to change"

Copy link
Member

@knolleary knolleary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay - lets go with what you've got - just remove the entries from the settings file to reduce noise.

@knolleary knolleary merged commit 356e332 into node-red:dev May 30, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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