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

Respect MacOS proxy settings #1955

Merged

Conversation

jefflloyd
Copy link
Contributor

@jefflloyd jefflloyd commented Aug 26, 2023

This PR adds support to reqwest for respecting the system proxy settings for MacOS. It follows many of the same code paths as #547, which added support for system proxies on Windows. This does add a dependency on a new crate, system-configuration, for communicating with MacOS.

I did a little bit of renaming, mostly changing the word "registry" to "platform" to better communicate that the non-environment-variable proxy settings can come from different locations, depending on your target platform.

I tested this locally via a Rust app I work on (on Ventura 13.5.1), and verified that with these changes (and a local proxy server like Charles active), any requests are forwarded through the proxy. The no proxy case works as well.

src/proxy.rs Outdated Show resolved Hide resolved
src/proxy.rs Outdated Show resolved Hide resolved
src/proxy.rs Outdated Show resolved Hide resolved
src/proxy.rs Outdated Show resolved Hide resolved
src/proxy.rs Outdated Show resolved Hide resolved
src/proxy.rs Outdated Show resolved Hide resolved
src/proxy.rs Outdated Show resolved Hide resolved
src/proxy.rs Outdated Show resolved Hide resolved
src/proxy.rs Outdated Show resolved Hide resolved
src/proxy.rs Outdated Show resolved Hide resolved
src/proxy.rs Outdated Show resolved Hide resolved
src/proxy.rs Show resolved Hide resolved
@jefflloyd jefflloyd marked this pull request as ready for review August 29, 2023 21:58
@seanmonstar
Copy link
Owner

Thanks for the PR! And @alokedesai, thank you too for the review, it's hugely appreciated and you've done a fine job! 🤩

Whenever you feel like the last pieces are tidy, let me know and I'll mergem

@alokedesai
Copy link

Thank you Sean! From one maintainer of Warp to another, your contributions to the Rust networking stack are much appreciated :-)

@jefflloyd
Copy link
Contributor Author

Thanks for taking a look, @seanmonstar ! Is there any way to manually trigger CI for this branch or something? Doesn't appear to be running - once that turns green, I think we'd be good on our side to merge if the code looks good to you.

Also, would this be considered a breaking change? I think I saw that you classified the other Windows change as one, which makes sense - just want to make sure these changes aren't going to disrupt people's usage of reqwest. :)

@seanmonstar
Copy link
Owner

Oh sorry, I forgot, GitHub (for security) doesn't start CI if this is your first contribution to the repo. I've just started it.

Did we say the Windows one was breaking? I don't remember. I could see either way...

@jefflloyd
Copy link
Contributor Author

Thanks :D I think based on this comment you might have made it breaking? I'm actually having a bit of trouble deducing what happened myself 😅

@seanmonstar
Copy link
Owner

Ah, I see. Reading through the changelog (this all happened 4ish years ago 😅), I see that the notes pointed out adding no_proxy() so people could prepare for Client to eventually checking environment variables and system registries by default start in 0.10. So, that change was considered breaking. Though, the notes make me feel like the "breaking change" was that it would look at all.

The much more conservative view would be that any single change that finds more proxies than it did previously is breaking, which would mean any bug fixes to searching HTTPS_PROXY, or adding support for ALL_PROXY, etc.

@jefflloyd
Copy link
Contributor Author

Gotcha, that makes sense. Well, I'll leave the release timing up to you, but I think we're good to go on the code front if all looks OK!

@seanmonstar seanmonstar merged commit 70d100c into seanmonstar:master Aug 31, 2023
31 checks passed
kodiakhq bot pushed a commit to pdylanross/fatigue that referenced this pull request Oct 3, 2023
Bumps reqwest from 0.11.20 to 0.11.21.

Release notes
Sourced from reqwest's releases.

v0.11.21
What's Changed

Add automatically detecting macOS proxy settings.
Add ClientBuilder::tls_info(bool), which will put tls::TlsInfo into the response extensions.
Fix trust-dns resolver from possible hangs.
Fix connect timeout to be split among multiple IP addresses.

New Contributors

@​SpeedReach made their first contribution in seanmonstar/reqwest#1960
@​jefflloyd made their first contribution in seanmonstar/reqwest#1955
@​droe made their first contribution in seanmonstar/reqwest#1938
@​conradludgate made their first contribution in seanmonstar/reqwest#1940




Changelog
Sourced from reqwest's changelog.

v0.11.21

Add automatically detecting macOS proxy settings.
Add ClientBuilder::tls_info(bool), which will put tls::TlsInfo into the response extensions.
Fix trust-dns resolver from possible hangs.
Fix connect timeout to be split among multiple IP addresses.




Commits

d050e60 v0.11.21
2a881fb fix: split connect timeout for multiple IPs (#1940)
17c893f Bump actions/checkout from 3 to 4 (#1968)
10d9d23 Add tls_info / TlsInfo for access to peer's leaf certificate (#1938)
d3d95a5 Optimize TrustDnsResolver (#1967)
0292486 dep: Upgrade trust-dns-resolver from v0.22 to v0.23 (#1965)
70d100c Feature: auto detect MacOS proxy settings (#1955)
34f6c70 Added wasm in the docs. (#1960)
See full diff in compare view




Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

@dependabot rebase will rebase this PR
@dependabot recreate will recreate this PR, overwriting any edits that have been made to it
@dependabot merge will merge this PR after your CI passes on it
@dependabot squash and merge will squash and merge this PR after your CI passes on it
@dependabot cancel merge will cancel a previously requested merge and block automerging
@dependabot reopen will reopen this PR if it is closed
@dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
@dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
@dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
@tnull tnull mentioned this pull request Oct 5, 2023
@littledivy
Copy link
Contributor

Hi @seanmonstar, will you accept a PR making this a (default) feature flag? We want to avoid linking the SystemConfiguration framework on macOS in Deno because dyld framework loading costs startup time.

@seanmonstar
Copy link
Owner

Sure.

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.

4 participants