Skip to content

config: Set the default value of the network_http_proxy_uri setting via the http_proxy variable#41209

Merged
jschwe merged 2 commits intoservo:mainfrom
Narfinger:http_proxy_env
Dec 12, 2025
Merged

config: Set the default value of the network_http_proxy_uri setting via the http_proxy variable#41209
jschwe merged 2 commits intoservo:mainfrom
Narfinger:http_proxy_env

Conversation

@Narfinger
Copy link
Copy Markdown
Contributor

@Narfinger Narfinger commented Dec 11, 2025

This parses the env variable "http_proxy" for servoshell and sets the network_http_proxy_uri if it exists to the same value.

Testing: Tested that it correctly picks it up.

@Narfinger Narfinger requested a review from atbrakhi as a code owner December 11, 2025 16:02
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Dec 11, 2025
@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Dec 11, 2025
@atbrakhi atbrakhi added this pull request to the merge queue Dec 11, 2025
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Dec 11, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 11, 2025
@servo-highfive servo-highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Dec 11, 2025
@mrobinson mrobinson changed the title Servoshell: Enable proxy by default from environment variable servoshell: Enable proxy by default from environment variable Dec 11, 2025
mrobinson
mrobinson reviewed Dec 11, 2025
View reviewed changes
mrobinson
mrobinson requested changes Dec 11, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

I believe that handling of the pretty standard http_proxy environment variable should be handled automatically by Servo, allowing the embedder to override the value when necessary.

@servo-highfive servo-highfive added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Dec 11, 2025
@Narfinger
Copy link
Copy Markdown
Contributor Author

Narfinger commented Dec 12, 2025

Ok I moved it to the servo Preferences now. Let me know if there is something else that should change.

@mrobinson mrobinson changed the title servoshell: Enable proxy by default from environment variable config: Set the default value of the network_http_proxy_uri setting via the http_proxy variable Dec 12, 2025
@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Dec 12, 2025
@jschwe
Copy link
Copy Markdown
Member

jschwe commented Dec 12, 2025

Ok I moved it to the servo Preferences now.

Huh, the change was that simple 😆 - Now I feel stupid.

@jschwe jschwe added this pull request to the merge queue Dec 12, 2025
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Dec 12, 2025
Merged via the queue into servo:main with commit fe6b470 Dec 12, 2025
33 checks passed
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Dec 12, 2025
@mrobinson
Copy link
Copy Markdown
Member

mrobinson commented Dec 12, 2025

Ok I moved it to the servo Preferences now.

Huh, the change was that simple 😆 - Now I feel stupid.

No worries. Eventually Servo will need a more complex proxy resolution API allowing the client to override the settings with a URL or a local PAC file, but I guess this is good enough for now. Plus, this should work automatically with anyone embedding Servo. In the future, it is likely we will make it so that the default ProxyConfigurationDelegate (or whatever it is called) will look at the environment variable for the setting.

@Narfinger Narfinger deleted the http_proxy_env branch December 12, 2025 12:59
@treeshateorcs
Copy link
Copy Markdown
Contributor

treeshateorcs commented Dec 15, 2025 •
edited
Loading

This does not read HTTP_PROXY (all caps) or HtTp_PrOxY or w/e. i personally have the variable in all caps. I propose we check both "HTTP_PROXY" and "http_proxy", but not mixed case. Firefox does just that at https://github.com/mozilla-firefox/firefox/blob/bef781bbd7a225c428c2444d7d02e9f6eb327e94/security/sandbox/chromium/base/environment.cc#L29-L42

treeshateorcs added a commit to treeshateorcs/servo that referenced this pull request Dec 15, 2025
@treeshateorcs
Handle uppercase HTTP_PROXY variable
b680790 
follow up to servo#41209

Signed-off-by: tho <[email protected]>
@treeshateorcs treeshateorcs mentioned this pull request Dec 15, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Dec 15, 2025
@treeshateorcs @yezhizhen
prefs: Handle uppercase HTTP_PROXY variable (#41268)
349674f 
follow up to #41209 

#41209 does not read HTTP_PROXY (all caps) or `HtTp_PrOxY" or w/e. i
personally have the variable in all caps. I propose we check both
"HTTP_PROXY" and "http_proxy", but not mixed case. Firefox does just
that at
https://github.com/mozilla-firefox/firefox/blob/bef781bbd7a225c428c2444d7d02e9f6eb327e94/security/sandbox/chromium/base/environment.cc#L29-L42

---------

Signed-off-by: tho <[email protected]>
Signed-off-by: Euclid Ye <[email protected]>
Co-authored-by: Euclid Ye <[email protected]>
@treeshateorcs treeshateorcs mentioned this pull request Dec 15, 2025
Closed
@yezhizhen yezhizhen mentioned this pull request Dec 21, 2025
Closed
@yezhizhen yezhizhen mentioned this pull request Feb 4, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jschwe jschwe jschwe left review comments

@mrobinson mrobinson mrobinson approved these changes

@atbrakhi atbrakhi atbrakhi approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Narfinger @jschwe @mrobinson @treeshateorcs @atbrakhi @servo-highfive