Add a preference to turn on/off insecure password warnings

RESOLVED FIXED in Firefox 44

Status

()

Firefox
Security
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: tanvi, Assigned: tanvi)

Tracking

(Blocks: 1 bug)

44 Branch
Firefox 45
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify ?

Firefox Tracking Flags

(firefox44 fixed, firefox45 fixed)

Details

(Whiteboard: [fxprivacy])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
We should add a pref to enable/disable insecure password warnings in case we decide we need to hold it back from release, but want to keep it in the other channels (especially dev edition).

Updated

2 years ago
Whiteboard: [fxprivacy] → [fxprivacy] [triage]

Updated

2 years ago
Priority: -- → P2
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
(Assignee)

Comment 1

2 years ago
Created attachment 8680901 [details] [diff] [review]
Bug1217156-10-29-15.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc48e884c8ac
Attachment #8680901 - Flags: review?(bgrinstead)
Comment on attachment 8680901 [details] [diff] [review]
Bug1217156-10-29-15.patch

Review of attachment 8680901 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, I'll push to try on aurora and fx-team to make sure we are good there

::: browser/base/content/browser.js
@@ +6929,5 @@
>  
> +  get _hasInsecureLoginForms() {
> +    // checks if the page has been flagged for an insecure login. Also checks
> +    // if the flag to degrade the UI is set to true
> +    return (LoginManagerParent.hasInsecureLoginForms(gBrowser.selectedBrowser) && Services.prefs.getBoolPref("security.insecure_password.ui.enabled"));

Nit - don't need the extra parens, and please wrap this into two lines:

return LoginManagerParent.hasInsecureLoginForms(gBrowser.selectedBrowser) &&
       Services.prefs.getBoolPref("security.insecure_password.ui.enabled");
Attachment #8680901 - Flags: review?(bgrinstead) → review+
fx-team: https://treeherder.mozilla.org/#/jobs?repo=try&revision=385fa1bd69f3
aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25933d00995e
Assignee: nobody → tanvi
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
Created attachment 8680915 [details] [diff] [review]
Bug1217156-10-29-15B.patch

Fixed nits.  Carrying over r+ from bgrins.  Needs check in and uplift.
Attachment #8680901 - Attachment is obsolete: true
Attachment #8680915 - Flags: review+
(Assignee)

Comment 5

2 years ago
(In reply to Tanvi Vyas [:tanvi] from comment #4)
> Needs check in and uplift.

But I'll wait for some try results first.

Comment 6

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/3189c9d88f13
(Assignee)

Comment 7

2 years ago
Comment on attachment 8680915 [details] [diff] [review]
Bug1217156-10-29-15B.patch

Approval Request Comment
[Feature/regressing bug #]: https://bugzilla.mozilla.org/show_bug.cgi?id=1179961
[User impact if declined]: Developers will see insecure password warnings on HTTP pages AND localhost pages, without learn more links.  We need to fix localhost and add learn more before we can expose this to developers
[Describe test coverage new/current, TreeHerder]: There is a test for the feature that is still enabled.  This bug just disables the feature for everything but nightly.
[Risks and why]: Developers will become habituated to the warning since they will see it on localhost.  Which is the opposite of what we want - we want them to fix it.
[String/UUID change made/needed]: None
Attachment #8680915 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/3189c9d88f13
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45

Updated

2 years ago
Iteration: --- → 44.3 - Nov 2
Flags: qe-verify?
Priority: P2 → P1
Comment on attachment 8680915 [details] [diff] [review]
Bug1217156-10-29-15B.patch

Make sense, taking it.
Should be in the first devedition release.
Attachment #8680915 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 10

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/904dd138cbc9
status-firefox44: affected → fixed

Comment 11

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3189c9d88f13
status-b2g-v2.5: --- → fixed

Comment 12

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/904dd138cbc9
status-b2g-v2.5: fixed → ---
(In reply to Carsten Book [:Tomcat] from comment #10)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/904dd138cbc9
>        status-firefox44: affected → fixed

(In reply to Carsten Book [:Tomcat] from comment #12)
> https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/904dd138cbc9
>        status-b2g-v2.5: fixed → ---

Huh? This is the exact same patch. How can it fix the problem on desktop and unfix it on B2G?
Flags: needinfo?(cbook)
(In reply to Tony Mechelynck [:tonymec] from comment #13)
> (In reply to Carsten Book [:Tomcat] from comment #10)
> > https://hg.mozilla.org/releases/mozilla-aurora/rev/904dd138cbc9
> >        status-firefox44: affected → fixed
> 
> (In reply to Carsten Book [:Tomcat] from comment #12)
> > https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/904dd138cbc9
> >        status-b2g-v2.5: fixed → ---
> 
> Huh? This is the exact same patch. How can it fix the problem on desktop and
> unfix it on B2G?

it was a simple problem with the setup of the 2.5 branch. we merged mozilla-central to 2.5 and reverted this. (ended up doing a merge from mozilla-aurora instead of m-c).
Flags: needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.