1944213 - https_proxy not recognized on windows

Status: RESOLVED FIXED

(Core :: Networking: Proxy, enhancement, P3)

Description

[Steven Penny]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:128.0) Gecko/20100101 Firefox/128.0

Steps to reproduce:

I tried all these separately:

$env:https_proxy = "res.proxy-seller.com:10000"
$env:https_proxy = "http://res.proxy-seller.com:10000"
$env:HTTPS_PROXY = "res.proxy-seller.com:10000"
$env:HTTPS_PROXY = "http://res.proxy-seller.com:10000"

with the "Use system proxy settings" option selected - then visit this page or similar:

https://httpbingo.org/get

Actual results:

my normal IP is used

Expected results:

should have used proxy IP and/or prompted me for proxy credentials

Comment 1

[BugBot [:suhaib / :marco]]

The Bugbug bot thinks this bug should belong to the 'Core::Networking: Proxy' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Did you also try with the HTTP_PROXY environment variable? Reading bug 911919 comment 5 indicates that formerly Firefox tunneled HTTPS and FTP connections through HTTP. Not sure if something changed here over the years - a peer from the networking team can help here.

Comment 3

[Steven Penny]

same failure with HTTP_PROXY

Comment 4

[Valentin Gosu [:valentin]]

While we have nsUnixSystemProxySettings::GetProxyFromEnvironment for Unix, we don't seem to be looking at the ENV in nsWindowsSystemProxySettings.cpp

@Steven, I assume this is something you expected to work and not a recent regression, right?
Do other browsers or applications support this on Windows?
I'm trying to understand if this is a common expectation and use case.
If we decide to do this, it would be good to move GetProxyFromEnvironment from nsUnixSystemProxySettings to ProxyUtils.cpp and make sure it gets called on all platforms.
I'm happy to review those patches if someone wants to contribute them

Comment 5

[Steven Penny]

@Steven, I assume this is something you expected to work and not a recent regression, right?

correct

Do other browsers or applications support this on Windows?

• https://wikipedia.org/wiki/Go_(programming_language)
• https://wikipedia.org/wiki/Python_(programming_language)
• https://learn.microsoft.com/dotnet/api/system.net.http.httpclient.defaultproxy
• https://wikipedia.org/wiki/CURL

Comment 6

[erosman [:erosman]]

If I understand the issue correctly, an HTTP proxy server (http://res.proxy-seller.com10000) is set under HTTPS proxy setting, and when going to an HTTPS site https://httpbingo.org/get, the request is not proxied.

If that is the case, unfortunately, that is current behavior, regardless of the operating system.

See also: Bug 1804693, Bug 1947229

Comment 7

[yooughtul]

Hi Valentin [:valentin],
I’m a new contributor and would love to work on this good first bug (Bug 1944213 - https_proxy not recognized on Windows).
From the previous comments, I understand the core fix is to migrate the GetProxyFromEnvironment function from nsUnixSystemProxySettings to the cross-platform ProxyUtils.cpp, then ensure it’s called in Windows’ nsWindowsSystemProxySettings.cpp. To avoid going off-track, I’d appreciate a little more context on the code structure:
Could you share the exact file paths for nsUnixSystemProxySettings.cpp, ProxyUtils.cpp, and nsWindowsSystemProxySettings.cpp in the Mozilla Central repo? (e.g., under network/proxy/ or another directory)
In nsWindowsSystemProxySettings.cpp, is there a specific existing function (like GetSystemProxySettings) where I should add the call to the migrated GetProxyFromEnvironment?
Are there any existing test cases for proxy environment variables (e.g., in netwerk/test/) that I should reference or extend to cover Windows?
Are there any Mozilla-specific coding conventions I should follow for this migration (e.g., function naming, error handling in cross-platform code) that aren’t in the general contributor guide?
Thanks for your help—I want to make sure the patch aligns with the team’s expectations!
Best,yooughtul

Comment 8

[Valentin Gosu [:valentin]]

Similar to how nsUnixSystemProxySettings::GetProxyForURI first checks if there's a system-wide proxy then fallsback to calling GetProxyFromEnvironment, nsWindowsSystemProxySettings::GetProxyForURI should do the same.

Regarding testing, you can probably use TestProtocolProxyService.cpp. Use PR_SetEnv to set the environment variables then call nsISystemProxySettings::getProxyForURI to check that the env variable is properly consulted.

Thanks!

Comment 9

[yooughtul]

Hi Valentin [:valentin],
First, thank you very much for your earlier guidance. Your suggestions—"making nsWindowsSystemProxySettings::GetProxyForURI follow the Unix-style logic (check system-wide proxy first, then fall back to GetProxyFromEnvironment)" and "writing tests in TestProtocolProxyService.cpp"—provided critical direction for my Bug fix work. I have now completed the core tasks, and I’m sharing the progress and issues encountered below. ​
​
​
Completed Work ​
(1) Code Modifications Implementation ​

1. Cross-Platform Function Migration: Moved the GetProxyFromEnvironment function (originally only in nsUnixSystemProxySettings.cpp) to ProxyUtils.cpp to ensure it is callable on the Windows platform, enabling function reuse and cross-platform consistency. ​
2. Windows Proxy Logic Adaptation: Updated the nsWindowsSystemProxySettings::GetProxyForURI method to strictly follow the Unix-style logic: ​
   Step 1: Prioritize checking system-wide proxy configurations; ​
   Step 2: If no system-wide proxy is detected, automatically fall back to calling GetProxyFromEnvironment to retrieve proxy settings from environment variables. This aligns the proxy logic between Windows and Unix platforms. ​
   ​
   ​
   Test Case Writing ​
   Based on the TestProtocolProxyService.cpp file, new test cases have been added with the following design: ​
3. Core Test Logic: Use the PR_SetEnv interface to set proxy environment variables for different scenarios (covering lowercase/uppercase formats of http_proxy/https_proxy, and formats with/without the http:// prefix). ​
4. Verification Method: Call the nsISystemProxySettings::getProxyForURI interface and check if the returned result matches the set environment variables. This verifies the correctness of the logic for "properly recognizing proxy configurations from environment variables." ​
   ​
   ​
   Issues with Test Execution ​
   (1) Problem Phenomenon ​
   After completing the code and test writing, I confirmed the code has compiled successfully (no compilation errors). However, I encountered issues when running the tests: ​
   After querying available commands via ./mach help and ./mach gtest help, I executed the following two commands, but neither detected the new test cases I added: ​
5. ./mach gtest TestProtocolProxyService​
6. ./mach cppunitest TestProtocolProxyService ​
   However, the tests they conducted did not include any of my test cases.​
   ​
   To successfully verify the correctness of the test cases, I would like to ask you the following questions: ​
7. Correctness of Test Commands: Are there dedicated ./mach test commands for the "Networking: Proxy" component (instead of using the general gtest/cppunitest commands)? ​
8. Test Case Recognition Rules: Do new test cases need to follow specific naming conventions (e.g., more precise function prefixes, belonging to specific test suites) to be recognized by gtest/cppunitest commands? ​
9. Supplementation of Execution Parameters: Do I need to add additional parameters to the command (e.g., specifying the exact path of the test case or the name of the test suite) to trigger the execution of the newly added tests? ​
   ​
   Thank you again for your help—I look forward to your further guidance! ​
   ​
   Yooughtul

Comment 10

[Valentin Gosu [:valentin]]

Hi Yooughtul,

If you post your patch using moz-phab I'll be able to give you much better feedback.

Tests are generally run with ./mach gtest TestProtocolProxyService​.* or ./mach gtest "TestProtocolProxyService​.*" depending on your platform. I'm asuming you defined your test something like TEST(TestProtocolProxyService, GetProxyFromEnvironment) or something like that.

Again, feel free to post your patch and I can give more precise feedback.
Thanks!

Comment 11

[yooughtul]

Attachment: Bug 1944213 - Migrate Unix proxy env var logic to Windows + add TestProtocolProxyService test. r?valentin

Comment 12

[yooughtul]

Hi Valentin [:valentin],
Thanks again for your earlier guidance! I wanted to follow up on the tests—when I ran the command ./mach gtest TestProtocolProxyService​* , the output showed
"
17:26:18 Currently running suites: TestProtocolProxyService.
17:26:18 Ran 0 test (s) from 0 test (s) (0 disabled) with 0 failure (s).
",
it seems no test suites were actually executed.
I suspect this might mean the test I added (named along the lines of TEST(TestProtocolProxyService, GetProxyFromEnvironment)) isn’t being detected properly. Would you have any thoughts on what might be causing this, or steps I should take to troubleshoot?
Let me know if you need more details from the full output—I’m happy to share! Thanks again for your help.

Comment 13

[yooughtul]

Attachment: Bug 1944213 - Migrate Unix proxy env var logic to Windows + add TestProtocolProxyService test. r + fix format error?valentin

Comment 14

[yooughtul]

Hi Valentin [:valentin],
Hope you're doing well! I'm reaching out for a quick help regarding my submitted revision D274429 (linked to Bug 1944213).

Five days ago, I submitted the second version of the patch that addresses the bug, and I distinctly remember updating the revision status to remove the "Work In Progress (WIP)" flag at that time. However, when I checked again recently (since I haven't received review feedback yet), I noticed the revision is still marked as WIP. I’ve gone through the Phabricator interface multiple times but can’t find the option to disable the WIP status—could you guide me on how to resolve this?

The patch migrates Unix-style proxy environment variable logic to Windows and includes a test case (TestProtocolProxyService) along with a minor formatting fix. I want to ensure the revision is in a "Ready for Review" state so you can proceed with the review.

Could you please let me know where I can adjust the WIP status, or help me mark it as ready if needed? Thank you so much for your assistance!

Best regards,
Yooughtul
Revision Link: https://phabricator.services.mozilla.com/D274429
Bug Link: https://bugzilla.mozilla.org/show_bug.cgi?id=1944213

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Yooughtul, are you using moz-phab to push the patch? If yes, you need to use the --no-wip argument to get rid of the WIP flag, which is the default for the command.

Comment 16

[yooughtul]

Attachment: WIP: Bug 1944213 - Migrate Unix proxy env var logic to Windows + add TestProtocolProxyService test. r + fix format error?valentin

Comment 17

[yooughtul]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #15)

Yooughtul, are you using moz-phab to push the patch? If yes, you need to use the --no-wip argument to get rid of the WIP flag, which is the default for the command.

Hi Henrik [whimboo],
Thanks so much for the quick tip! I followed your suggestion and used the --no-wip argument with moz-phab, and it worked perfectly—successfully pushed the patch without the WIP flag. Really appreciate your help saving me from troubleshooting this further!
Let me know if there’s anything else I should keep in mind for future patches.
Best regards,
Yooughtul

Comment 18

[Valentin Gosu [:valentin]]

Sorry for the late review. It is indeed important that the patch is not WIP, otherwise I don't get notifications for it.
I've just submitted some feedback for it. Let me know if you need any help.

Comment 19

[yooughtul]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #18)

Sorry for the late review. It is indeed important that the patch is not WIP, otherwise I don't get notifications for it.
I've just submitted some feedback for it. Let me know if you need any help.

Thank you for the review!
I have addressed the feedback points you mentioned:

1. TestProtocolProxyService.cpp:

   • Removed the #include "nsWindowsSystemProxySettings.h" to prevent build failures on non-Windows platforms. I am now using the nsISystemProxySettings.h interface instead.
   • Simplified the PR_SetEnv calls by passing string literals directly and removed the unnecessary intermediate variables.

2. nsWindowsSystemProxySettings.cpp:

   • Moved the GetProxyFromEnvironment call to occur before ReadInternetOption. This ensures environment variables take precedence and avoids potential overhead from the Windows API if an env var is set.

3. Config files:

   • Reverted the accidental changes to .gitignore and .hgignore.

I've verified the tests locally. Please let me know if there is anything else that needs adjustment.
Thanks again for your help!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/934c10f8994d
https://hg.mozilla.org/integration/autoland/rev/9e3221ccea1b
Migrate Unix proxy env var logic to Windows + add TestProtocolProxyService test. r=valentin,necko-reviewers

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/63c227351d72
https://hg.mozilla.org/integration/autoland/rev/c74e379ac9df
Revert "Bug 1944213 - Migrate Unix proxy env var logic to Windows + add TestProtocolProxyService test. r=valentin,necko-reviewers" for causing Gtest failures

Comment 22

[Sandor Molnar[:smolnar]]

Backed out for causing Gtest failures @ TestProtocolProxyService.Proxy_Env_Vars

Backout link: https://hg.mozilla.org/integration/autoland/rev/c74e379ac9df6dfb825ad44c42501d1ec1a91025

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | TestProtocolProxyService.Proxy_Env_Vars | Value of: result.Find(expected) != kNotFound

also this build bustage

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/95420b7ba8e9
https://hg.mozilla.org/integration/autoland/rev/700e79e51f4b
Migrate Unix proxy env var logic to Windows + add TestProtocolProxyService test. r=valentin,necko-reviewers

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7ecaf34aa52d
https://hg.mozilla.org/integration/autoland/rev/24d2b8a40b6b
Revert "Bug 1944213 - Migrate Unix proxy env var logic to Windows + add TestProtocolProxyService test. r=valentin,necko-reviewers" for causing non-unified build bustages on TestProtocolProxyService

Comment 25

[Cosmin Sabou [:CosminS]]

Backed out for causing non-unified build bustages on TestProtocolProxyService

Push with failures

Failure log

Backout link

Comment 26

[yooughtul]

I've tried to fix the issue where the new test case caused build failures and test failures on non-Windows platforms.
The error code will never be executed and the gtest failures occurred because TestProtocolProxyService.Windows_Proxy_Env_Vars was being compiled on all platforms, but it was intended to test Windows-specific proxy behavior. I have updated the patch to wrap this test case with #if defined(XP_WIN) to ensure it only runs on Windows builds.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/f6a8ccb71089
https://hg.mozilla.org/integration/autoland/rev/9cb64ed79860
Migrate Unix proxy env var logic to Windows + add TestProtocolProxyService test. r=valentin,necko-reviewers

Comment 28

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/9cb64ed79860