Add --bypass-proxy argument to ignore proxy for hosts or IP addresses. #410

otaconix · 2025-03-04T07:54:29Z

I saw #296 as a good first issue and thought I'd have a go at it.

A few things to note:

I named the argument the same as curl's (--noproxy), as that seems to be what you were aiming for. This could potentially be confusing for users, as it's similar to --no-proxy, which undoes any --proxy arguments.
reqwest's NoProxy's applies a wildcard (*) only to hostnames, not IP addresses. curl does both. So there's a little workaround to implicitly add IPv4 and IPv6 full ranges (respectively 0.0.0.0/0 and ::/0).
I wanted to add tests for IPv6 and hostnames, but didn't see a way to do this easily: IPv6 would require testing on a host that has an IPv6 interface (not sure lo has that by default), and for hostnames, I thought I could use --resolve, but that turns the hostname into an IP address, so the hostname never reaches reqwest. If needed, I can take a closer look to see if I can add these tests.

Related issue: ducaale#296

blyxxyz

Thanks!

For the name, maybe --disable-proxy-for? Bit of a mouthful though.

src/cli.rs

tests/cli.rs

otaconix · 2025-03-04T22:25:29Z

For the name, maybe --disable-proxy-for? Bit of a mouthful though.

Some of the ideas I had about the name:

--bypass-proxy
--ignore-proxy
curl's --noproxy

--disable-proxy-for is as good a suggestion as any, though as you say, a bit long.

Is there some "guideline"/design principle that's used to name flags? I originally added support for --proxy, but didn't give much thought to naming back then (I was just scratching my own itch).

blyxxyz · 2025-03-04T23:18:58Z

Most of the flags are inherited from HTTPie, so we don't have a lot of experience coming up with names unfortunately.

--noproxy feels too close to --no-proxy like you said. I don't have a strong opinion about bypass vs ignore vs disable, but I tacked on -for to make it clear that the option takes an argument. --disable-proxy sounds like it could be a binary flag that just toggles off all proxies. I'm not sure how confusing that is in practice.

Uhh, I'll use an LLM. I'll pretend these options already exist and ask it what they do and read what it hallucinates. That hopefully gets close to the intuition of real users. First I ask

What does xh's --proxy option do?

And it doesn't get the syntax quite right but it correctly guesses what the option is for.

Then I ask

Thanks! What about the --disable-proxy option?

And it gives examples where it doesn't take an argument, xh --disable-proxy https://example.com and xh --disable-proxy --auth user:password https://example.com. So it guessed incorrectly.

When I ask it about --disable-proxy-for it gets it exactly right, it even guesses that you can separate the hosts with commas: xh --proxy http://proxy.example.com:8080 --disable-proxy-for internal.example.com,api.local https://example.com.
For --noproxy it also guesses right, probably because it has been trained on lots of curl commands.
(For --no-proxy it guesses that it's a binary flag, so it picks up on the nuance! But it says that --no-proxy can override $http_proxy, which it can't, though maybe it should.)
For --ignore-proxy it incorrectly guesses a binary flag.
For --bypass-proxy it guesses correctly! I guess bypass makes it more clear than the other verbs that you're not completely disabling the proxies. This is a strong candidate.

I also asked it

Thanks! Is there an option for selectively bypassing the proxy for certain hosts?

And it came up with this, which is pretty interesting:
xh --proxy http://proxy.example.com:8080,no-proxy=localhost,no-proxy=127.0.0.1,no-proxy=example.com https://example.com

We have the https: and http: keys in the current syntax, so we could add a no-proxy:/noproxy:/no:/ignore: or something key. There's a pleasing pattern there, http: corresponds to $HTTP_PROXY, https: corresponds to $HTTPS_PROXY, so why not make no: correspond to $NO_PROXY? Then you could write --proxy 'http:http://localhost:8080,no:example.org'.

But maybe it's a little too cute, and I'd first want to check if the syntax for the --proxy option was invented for HTTPie or if it's also used somewhere else.

Finally I asked it

Thanks! I remember there's another option for selectively bypassing the proxy for certain hosts, what's it called?

and it hallucinated --proxy-ignore. Feels a little weird but it does more to suggest that it takes an argument than --ignore-proxy does.

Take all of this with a grain of salt of course.

…ble-proxy-for

otaconix · 2025-03-05T19:12:47Z

@blyxxyz Can you take a look at the latest changes? And maybe also see if there's anything you'd like to add to the issue I raised at seanmonstar/reqwest#2579 ?

otaconix · 2025-03-05T20:27:38Z

I also asked it

Thanks! Is there an option for selectively bypassing the proxy for certain hosts?

And it came up with this, which is pretty interesting: xh --proxy http://proxy.example.com:8080,no-proxy=localhost,no-proxy=127.0.0.1,no-proxy=example.com https://example.com

We have the https: and http: keys in the current syntax, so we could add a no-proxy:/noproxy:/no:/ignore: or something key. There's a pleasing pattern there, http: corresponds to $HTTP_PROXY, https: corresponds to $HTTPS_PROXY, so why not make no: correspond to $NO_PROXY? Then you could write --proxy 'http:http://localhost:8080,no:example.org'.

But maybe it's a little too cute, and I'd first want to check if the syntax for the --proxy option was invented for HTTPie or if it's also used somewhere else.

This is getting into software archeology, but it kinda looks like httpie's syntax for proxies is a direct mapping of the underlying requests library: https://docs.python-requests.org/en/latest/user/advanced/#proxies

I do like the idea of adding something like no-proxy to the mini-DSL of the --proxy option, since --proxy is repeatable, and reqwest's NoProxy applies to a single proxy (it's just that as currently implemented in this PR, the --ignore-proxy-for is applied to every proxy). I'm just not sure how, if at all, xh will be able to map that to curl, since it doesn't look like curl supports anything like that out of the box.

blyxxyz

Thanks again! A few more notes.

blyxxyz · 2025-03-09T14:09:38Z

src/cli.rs

+    #[test]
+    fn disable_proxy_for_trims_whitespace() {
+        assert_eq!(DisableProxyFor::from("*"), DisableProxyFor::from("  *  "));
+    }


Can you run this via Cli::try_parse_from in case clap one day decides to stop using From<&str> or something?

blyxxyz · 2025-03-12T07:50:50Z

src/cli.rs

+    /// The environment variable "NO_PROXY"/"no_proxy" can also be used, but its completely ignored
+    /// if --disable-proxy-for is passed.
+    #[clap(long, value_name = "no-proxy-list", value_delimiter = ',')]
+    pub disable_proxy_for: Vec<DisableProxyFor>,


After sleeping on it I like your --bypass-proxy naming best, because it's shorter, because --no-bypass-proxy seems clearer than --no-disable-proxy-for, and because the "bypass" terminology is also used by other CLIs like chromium and netsh on Windows.

(Sorry for the churn!)

blyxxyz · 2025-03-12T07:57:08Z

src/cli.rs

+    /// if --disable-proxy-for is passed.
+    #[clap(long, value_name = "no-proxy-list", value_delimiter = ',')]


One effect of doing it this way is that --bypass-proxy=example.com --bypass-proxy=example.org is equivalent to --bypass-proxy=example.com,example.org. This is different from curl, but it might be desirable.

If we want to commit to it then we should have a test for it so it doesn't regress and we should maybe mention it in the docs.

curl's manpage says:

If --noproxy is provided several times, the last set value will be used.

We could say:

--bypass-proxy can be provided several times. The list can be cleared with --no-bypass-proxy.

blyxxyz · 2025-03-12T07:59:54Z

src/cli.rs

+
+        Ok(proxy.no_proxy(
+            reqwest::NoProxy::from_string(&noproxy_comma_delimited)
+                .or_else(reqwest::NoProxy::from_env),


Ideally we'd also apply the * handling to the environment variable.

I'm also OK with deferring that for now since it's annoying to handle here and we haven't heard back from reqwest yet. In that case could you put a code comment?

blyxxyz · 2025-03-12T08:13:27Z

src/cli.rs

+            Proxy::All(url) => reqwest::Proxy::all(url),
+        }?;
+
+        let mut noproxy_comma_delimited = disable_proxy_for.join(",");


This causes a weird effect where --disable-proxy-for='' doesn't override $NO_PROXY but --disable-proxy-for='' --disable-proxy-for='' does override it.

Perhaps:

if disable_proxy_for.is_empty(), always do .no_proxy(reqwest::NoProxy::from_env()).

Otherwise, do from_string() without a fallback.

blyxxyz · 2025-03-12T08:14:56Z

tests/cli.rs

+    let mut proxy_server = server::http(|_| async move {
+        hyper::Response::builder()
+            .status(200)
+            .body("Proxy shouldn't have been used.".into())
+            .unwrap()
+    });
+    let mut actual_server = server::http(|_| async move {
+        hyper::Response::builder()
+            .status(200)
+            .body("".into())
+            .unwrap()
+    });


Suggested change

let mut proxy_server = server::http(|_| async move {

hyper::Response::builder()

.status(200)

.body("Proxy shouldn't have been used.".into())

.unwrap()

});

let mut actual_server = server::http(|_| async move {

hyper::Response::builder()

.status(200)

.body("".into())

.unwrap()

});

let mut proxy_server = server::http(|_| async move {

hyper::Response::builder()

.status(200)

.body("Proxy response".into())

.unwrap()

});

let mut actual_server = server::http(|_| async move {

hyper::Response::builder()

.status(200)

.body("Non-proxy response".into())

.unwrap()

});

blyxxyz · 2025-03-12T08:16:11Z

tests/cli.rs

Can you also test the environment variable?

blyxxyz · 2025-03-12T10:30:35Z

src/cli.rs

+impl Proxy {
+    pub fn into_reqwest_proxy(
+        self,
+        disable_proxy_for: &[DisableProxyFor],


Could you instead (or additionally) have a function to convert &[DisableProxyFor] to NoProxy? Then we only have to process it once.

Bonus points if you do it in Cli::try_parse_from and store the NoProxy as a field in the Cli struct, since then the fully processed IpMatcher and DomainMatcher get printed by --debug.

Add --noproxy argument to ignore proxy for hosts or IP addresses.

fc34e10

Related issue: ducaale#296

blyxxyz reviewed Mar 4, 2025

View reviewed changes

src/cli.rs Show resolved Hide resolved

src/cli.rs Show resolved Hide resolved

src/cli.rs Outdated Show resolved Hide resolved

tests/cli.rs Show resolved Hide resolved

Stefan Zwanenburg added 4 commits March 5, 2025 13:51

--noproxy: Trim whitespace in arg values & add extra tests

51ca27d

Rename --noproxy to --disable-proxy-for

824ed3d

Added link to reqwest issue for its different handling of NoProxy

4e88c91

Handle NO_PROXY/no_proxy env variables & fix long help for --disa…

e694193

…ble-proxy-for

blyxxyz self-requested a review March 5, 2025 19:17

otaconix changed the title ~~Add --noproxy argument to ignore proxy for hosts or IP addresses.~~ Add --disable-proxy-for argument to ignore proxy for hosts or IP addresses. Mar 10, 2025

blyxxyz reviewed Mar 12, 2025

View reviewed changes

otaconix changed the title ~~Add --disable-proxy-for argument to ignore proxy for hosts or IP addresses.~~ Add --bypass-proxy argument to ignore proxy for hosts or IP addresses. Mar 12, 2025

otaconix marked this pull request as draft May 9, 2025 07:36

		/// if --disable-proxy-for is passed.
		#[clap(long, value_name = "no-proxy-list", value_delimiter = ',')]

Add --bypass-proxy argument to ignore proxy for hosts or IP addresses. #410

Are you sure you want to change the base?

Add --bypass-proxy argument to ignore proxy for hosts or IP addresses. #410

Uh oh!

Conversation

otaconix commented Mar 4, 2025

Uh oh!

blyxxyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

otaconix commented Mar 4, 2025

Uh oh!

blyxxyz commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

otaconix commented Mar 5, 2025

Uh oh!

otaconix commented Mar 5, 2025

Uh oh!

blyxxyz left a comment

Choose a reason for hiding this comment

Uh oh!

blyxxyz Mar 9, 2025

Choose a reason for hiding this comment

Uh oh!

blyxxyz Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

blyxxyz Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

blyxxyz Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

blyxxyz Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

blyxxyz Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

blyxxyz Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

blyxxyz Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

blyxxyz commented Mar 4, 2025 •

edited

Loading

blyxxyz Mar 12, 2025 •

edited

Loading