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

new feature flag to control whether or not to detect proxy settings from environment by default #685

Merged
merged 2 commits into from
Nov 22, 2023

Conversation

toymil
Copy link
Contributor

@toymil toymil commented Nov 21, 2023

The default behavior of many libs and tools is to pick up proxy environment variables. Being able to config ureq to behave the same way makes implementing this behavior in other software easier.

If the default is false and we have no ways to make it default to true, implementing this behavior in other software would mean one of the following:

  • no qol methods like ureq::get(..), because we need to construct an AgentBuilder and call try_proxy_from_env, then build, every time.
  • construct and config a shared or global Agent, use that throughout the code.

…o detect proxy settings from environment by default
src/agent.rs Outdated
/// The default is `false`, i.e. not detecting proxy from env since this is
/// a potential security risk.
/// The default is `false` without the `proxy-from-env` feature flag, i.e.
/// not detecting proxy from env, to maintain backward compatibility.
Copy link
Owner

Choose a reason for hiding this comment

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

Having automatic proxy detection is a security risk. I want you to rephrase this doc to keep the warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Owner

@algesten algesten left a comment

Choose a reason for hiding this comment

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

Thanks!

@algesten algesten merged commit 89a69a5 into algesten:main Nov 22, 2023
39 of 41 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.

2 participants