Closed Bug 1524264 Opened 10 months ago Closed 10 months ago

Move "No Proxy for" outside the radio button and change the case for design guidelines

Categories

(Firefox :: Preferences, defect, P2)

65 Branch
x86_64
Windows 7
defect

Tracking

()

VERIFIED FIXED
Firefox 67
Tracking Status
firefox-esr60 66+ verified
firefox65 --- wontfix
firefox66 + verified
firefox67 + verified

People

(Reporter: caspe15, Assigned: junior)

References

Details

(Keywords: regression)

Attachments

(2 files)

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

Steps to reproduce:

Upgraded from FF 64 to 65.0 (64-bit). No other changes were made. Update occurred automatically when opening the Firefox About dialog.

FF is (and was) configured to use "Use system proxy settings" for the Network Proxy setting. The system is configured with a remote auto config script (wpad.dat). Also attempted manually setting Proxy, No proxy, and Auto-detect, but all fail exactly the same way, acting as if no proxy settings were provided.

IE11 and Chrome with the same settings are working correctly. Same problem is reproducible on all machines tested (Windows 2008R2, Windows 7, Windows 10, Windows 2012R2).

Everything was working correctly before consuming this update. No other changes were made.

Actual results:

Although update seems to have completed successfully, attempts to access sites inside the corporate network fail.

Expected results:

Access to both internal and external web sites should be possible.

It would be very useful if you could find the exact regression range.
https://mozilla.github.io/mozregression/quickstart.html

Has Regression Range: --- → no
Component: Untriaged → Networking: HTTP
Keywords: regression
OS: Unspecified → Windows 7
Product: Firefox → Core
Hardware: Unspecified → x86_64

I was able to correct the issue and mostly understand what causes it. In my FF Network settings I include values for:

Manual Proxy Configuration
No Proxy For
Automatic proxy configuration URL

Because FF stores these values regardless of which configuration chosen, this allowed me to easily switch between different settings as needed.

In v64 and older, when selecting "Use system proxy settings" the values in "No Proxy For" box were seemingly ignored. This was expected behavior in my opinion. When choosing "Use the system settings" I expect FF to use the system settings and not others I've set elsewhere, which is what happened. However, in v65 it appears that even though "Use system proxy settings" is selected, the values in "No proxy for" are also being consumed. This appears to be causing some sort of conflict and causing the issue.

I found the issue by deleting all the network.proxy related entries in prefs.js (e.g. user_pref("network.proxy.XXXX)) and restarted the browser. I then attempted to access a previously failing location and this time it worked. I now entered the exceptions I previously included in the "No proxy for" field (these are the same values set at the system level). I was now able to again reproduce the problem.

So, it appears that the change in behavior is related to using the System settings and having values in the No Proxy For field. Personally, my expectation is that when choosing to use the "System" settings, all other values should be ignored.

If this new behavior is the "expected" behavior, feel free to close this issue, although I would say that I don't agree with the change in behavior.

Thank you for the update. I'll change the summary. Now that there's more to go on, I'll also add the regressionwindow-wanted keyword so hopefully someone can find the regression range based on comment 2. Following is a list of networking "proxy" bugs that were fixed in Firefox 65 and therefore could be suspects: bug 1494364, bug 1219935, bug 1321466, bug 1497994.

Summary: Proxy settings ignored after upgrade to FF 65 → "Use system proxy settings" also applies "No proxy for" settings in Firefox 65

Kershaw, does anything stand out to you?

Flags: needinfo?(kershaw)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #4)

Kershaw, does anything stand out to you?

It looks like that bugs mentioned in comment #3 has nothing to do with this.
I'll try to reproduce this and take a deeper look.

Assign to myself and set to P2 for now.

Assignee: nobody → kershaw
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(kershaw)
Priority: -- → P2
Whiteboard: [necko-triaged]

It looks like this changeset[1] causes this problem.
In nsProtocolProxyService::CanUseProxy, we checked the url against the "No Proxy for" list regardless of proxy type.

[1] https://hg.mozilla.org/mozilla-central/rev/ba6a8e3b071640c50600884501c6266aa0393991

Has Regression Range: no → yes

Kershaw: I've CC'd you on the regressing bug. For the benefit of the reporter, there's some public description at https://nvd.nist.gov/vuln/detail/CVE-2018-18506 .

Perhaps we can address this by fixing bug 1507110? I'd suggest that it would probably not be a great idea to go back to potentially allowing proxying localhost for the system proxy case.

(In reply to :Gijs (he/him) from comment #7)

Kershaw: I've CC'd you on the regressing bug. For the benefit of the reporter, there's some public description at https://nvd.nist.gov/vuln/detail/CVE-2018-18506 .

Perhaps we can address this by fixing bug 1507110? I'd suggest that it would probably not be a great idea to go back to potentially allowing proxying localhost for the system proxy case.

Could you add me in the cc list of bug 1507110?

Another idea: maybe we can bail out at [1] when using system proxy. In this way, we can still avoid proxying localhost and also not checking network.proxy.no_proxies_on.
Or we can choose to honor network.proxy.no_proxies_on for all proxy types. But I am not sure this is what firefox users want.

Honza, what do you think?

[1] https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/netwerk/base/nsProtocolProxyService.cpp#1091

Flags: needinfo?(honzab.moz)

Kershaw, I think bug 1507110 is the correct answer here. Please look at that patch and then feel free to ask more questions. Thanks.

Flags: needinfo?(honzab.moz)

(In reply to Honza Bambas (:mayhemer) from comment #9)

Kershaw, I think bug 1507110 is the correct answer here. Please look at that patch and then feel free to ask more questions. Thanks.

Thanks. I'd like to ask Junior to also take care of this bug.

Assignee: kershaw → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(juhsu)

Anyhow, we need to fix bug 1507110 first.
IMO we should honor "network.proxy.no_proxies_on" for all proxy types to keep the applications flexible.
The firefox UI is already kinda suggest this. We can modify "No Proxy For" no matter what mode we select.

However, I believe we can make things more intuitive, like moving the "No Proxy For" lower.

Flags: needinfo?(juhsu)
Assignee: nobody → juhsu
Status: NEW → ASSIGNED
Component: Networking: HTTP → Preferences
Product: Core → Firefox
Summary: "Use system proxy settings" also applies "No proxy for" settings in Firefox 65 → Move "No proxy for" outside the radio button
Whiteboard: [necko-triaged]

note: I asked to also change the title case of the text after UX pointed out that it doesn't follow our capitalization guidelines.

Summary: Move "No proxy for" outside the radio button → Move "No Proxy for" outside the radio button and changing the case for design guildline
Summary: Move "No Proxy for" outside the radio button and changing the case for design guildline → Move "No Proxy for" outside the radio button and change the case for design guildline
Pushed by juhsu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e957d571c91c
move noproxy textbox lower to hint as a global effect r=ewright,flod
Pushed by juhsu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6e651b1b00b1
move noproxy textbox lower to hint as a global effect r=ewright,flod
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

Please nominate this for Beta approval when you get a chance.

Flags: needinfo?(juhsu)

Comment on attachment 9042117 [details]
Bug 1524264 - move noproxy textbox lower to hint as a global effect

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: None
  • User impact if declined: UI change to make proxy setting intuitive
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): small UI change
  • String changes made/needed: No
Flags: needinfo?(juhsu)
Attachment #9042117 - Flags: approval-mozilla-beta?

Comment on attachment 9042117 [details]
Bug 1524264 - move noproxy textbox lower to hint as a global effect

Small UI change for greater clarity in prefs, makes sense, let's uplift for beta 12.

Attachment #9042117 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment on attachment 9042117 [details]
Bug 1524264 - move noproxy textbox lower to hint as a global effect

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: UI change to make proxy setting intuitive
  • User impact if declined: confusion among enterprise users
  • Fix Landed on Version: 67
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): small UI change
  • String or UUID changes made by this patch:
Attachment #9042117 - Flags: approval-mozilla-esr60?
Whiteboard: [qa-triaged]

Verified fixed on latest Nightly 67.0a1 (2019-02-28) (64-bit) and latest Beta 66.0b12 (64-bit)(buildID: 20190301012442) on Windows 10, Mac OS 10.14 and Ubuntu 16.04.
"No proxy for" is moved lower, outside the radio button area and the title case is also changed.

Comment on attachment 9042117 [details]
Bug 1524264 - move noproxy textbox lower to hint as a global effect

This needs a rebased patch for ESR60.

Flags: needinfo?(juhsu)
Attachment #9042117 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60-
Attachment #9042117 - Flags: approval-mozilla-esr60-

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: UI change to make proxy setting intuitive
  • User impact if declined: confusion among enterprise users
  • Fix Landed on Version: 67
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): small ui change
  • String or UUID changes made by this patch: no
Flags: needinfo?(juhsu)
Attachment #9047900 - Flags: approval-mozilla-esr60?
Comment on attachment 9047900 [details] [diff] [review]
ui_patch_esr60(rebased patch)

Follow-up patch from bug 1503393 to fix a confusing bit of UI. Approved for 60.6esr.
Attachment #9047900 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Summary: Move "No Proxy for" outside the radio button and change the case for design guildline → Move "No Proxy for" outside the radio button and change the case for design guidelines
QA Whiteboard: [qa-triaged]
Whiteboard: [qa-triaged]

Verified - Fixed on latest ESR 60.6.0esr (Build ID: 20190312190806) on Windows 7/10, Mac OS 10.13 and Ubuntu 16.04.
Closing this issue as verified - fixed.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
Duplicate of this bug: 1538776
You need to log in before you can comment on or make changes to this bug.