-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
url: Fix NO_PROXY env var to work properly with --proxy option. #1140
Conversation
@eramoto, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @dfandrich and @rousskov to be potential reviewers. |
It isn't very clear to me exactly how this works (after this patch). Are you saying that the proxy given with
|
@bagder eramoto is saying that NO_PROXY environment variable is ignored when used with --proxy option. So, for example, before this patch
after this patch
@eramoto please tell me if this is not what you meant. |
That's a crystal clear explanation that I understand perfectly. @eramoto, do you think you could also add something to the docs in this vein that lets future users understand the priorities and what overrides what? |
I add the following descriptions to the curl man-page:
Cloud you confirm this commit. |
@@ -2312,6 +2317,16 @@ Sets the proxy server to use if no protocol-specific proxy is set. | |||
.IP "NO_PROXY <comma-separated list of hosts>" | |||
list of host names that shouldn't go through any proxy. If set to a asterisk | |||
\&'*' only, it matches all hosts. | |||
|
|||
Since 7.52.0, this einvironment variable disable the proxy even if specify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling: "environment"
@@ -2312,6 +2317,16 @@ Sets the proxy server to use if no protocol-specific proxy is set. | |||
.IP "NO_PROXY <comma-separated list of hosts>" | |||
list of host names that shouldn't go through any proxy. If set to a asterisk | |||
\&'*' only, it matches all hosts. | |||
|
|||
Since 7.52.0, this einvironment variable disable the proxy even if specify | |||
\fI--porxy\fP option. That is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling:" --proxy"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two spelling errors, apart from that I think it looks ready to merge!
Thank you for your reviews. @bagder I fixed these typos. |
Do we really need the environment variable for NOPROXY to override the command line option |
Because it is better to change to the behavior that are similar to other options, Cloud you wait a few days. @bagder |
I changed as following:
Cloud you check this commits. @bagder |
I thought we agreed that the command line options should override the environment variables? Yet your man page edit says:
|
@bagder |
Hm, yes I think you're right. Thanks. |
@eramoto , Can you please rebase your patch set (and squash what you think is reasonable), and update the release version number mentioned in the docs where this change is included (7.52.2 is now the next possible release number) and I promise to do my best to merge it when done? |
(basically, you can skip changing curl.1 completely, we'll generate that based on the cmdline-opts/* docs anyway) |
The combination of --noproxy option and http_proxy env var works well both for proxied hosts and non-proxied hosts. However, when combining NO_PROXY env var with --proxy option, non-proxied hosts are not reachable while proxied host is OK. This patch allows us to access non-proxied hosts even if using NO_PROXY env var with --proxy option.
If defined CURL_DISABLE_HTTP, detect_proxy() returned NULL. If not defined CURL_DISABLE_HTTP, detect_proxy() checked noproxy list. Thus refactor to set proxy to NULL instead of calling detect_proxy() if define CURL_DISABLE_HTTP, and refactor to call detect_proxy() if not define CURL_DISABLE_HTTP and the host is not in the noproxy list.
Under condition using http_proxy env var, noproxy list was the combination of --noproxy option and NO_PROXY env var previously. Since this commit, --noproxy option overrides NO_PROXY environment variable even if use http_proxy env var.
c211af4
to
59fa2ed
Compare
I rebased and squashed my patch set (, and updated the release version number in the docs). @bagder |
Thanks a lot for your work and patience on this! |
This allows users to specify an override value for the noproxy list, in addition to overriding the proxy url, in proxy_info_from_url(). If no override is given, we should respect the values in the no_proxy/NO_PROXY environment variables, if present. This behavior can be seen in other tools like curl (see curl/curl#1140). I would also tweak this for the python3 module, but its ProxyInfo class doesn't even seem to have a bypass_hosts attribute.
This allows users to specify an override value for the noproxy list, in addition to overriding the proxy url, in proxy_info_from_url(). If no override is given, we should respect the values in the no_proxy/NO_PROXY environment variables, if present. This behavior can be seen in other tools like curl (see curl/curl#1140). I would also tweak this for the python3 module, but its ProxyInfo class doesn't even seem to have a bypass_hosts attribute.
The combination of --noproxy option and http_proxy env var works well
both for proxied hosts and non-proxied hosts.
However, when combining NO_PROXY env var with --proxy option,
non-proxied hosts are not reachable while proxied host is OK.
This patch allows us to access non-proxied hosts even if using NO_PROXY
env var with --proxy option.