1217162 - Implement Contextual Feedback on Insecure Passwords

Status: RESOLVED FIXED

(Firefox :: Security, defect, P1)

Description

[Tanvi Vyas[:tanvi]]

When the user is focused on an insecure username or password field, we should show a contextual message telling the user that the data they are entering is not encrypted.  This is being designed in UX bug https://bugzilla.mozilla.org/show_bug.cgi?id=1217150.  This bug is for the implementation.

Comment 1

[Matthew N. [:MattN]]

Shipping this depends on bug 1217152 and its three dependencies (bug 1289913, bug 1120037, and bug 376668) which IMO are blockers. IMO we should implement the the three shipping blockers (bug 1289913, bug 1120037, and bug 376668) first so they can get testing before we start warning users.

Comment 2

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 1217162 - Implement insecure field warning on username field.;

Review commit: https://reviewboard.mozilla.org/r/84198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84198/

Comment 3

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

Hi Mattn, Tanvi, Mike,

The patch includes a Contextual Feedback content in username's autocomplete panel with HTTP detection, L10N string, and pref for toggling in about:config.

I have some questions about the scope in this bug:
1. How do you think the patch is landed with the username part with L10N changes for 52 first?
Password part could be implemented in another bug probably.

2. IMHO, the patch should include the final L10N string (discussing in bug 1307316).

3. Should this patch include the final visual? Probably I miss a visual spec, and I didn't see it yet.

Could you give this patch a feedback as well?

Thank you!

Comment 4

[Tanvi Vyas[:tanvi]]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #3)
> Comment on attachment 8798780 [details]
> Bug 1217162 - Implement Contextual Feedback on Insecure Passwords.
> 
> Hi Mattn, Tanvi, Mike,
> 
> The patch includes a Contextual Feedback content in username's autocomplete
> panel with HTTP detection, L10N string, and pref for toggling in
> about:config.
> 
Thanks Sean for your patch!

Is this patch built on top of Mike's https://bugzilla.mozilla.org/show_bug.cgi?id=1296638?

Also, is this patch going to be dependent on this change - https://bugzilla.mozilla.org/show_bug.cgi?id=376668#c42 ?  Or will this bug show the drop down on second focus, and let bug 376668 switch to first focus?


> I have some questions about the scope in this bug:
> 1. How do you think the patch is landed with the username part with L10N
> changes for 52 first?
> Password part could be implemented in another bug probably.
> 
What do you mean?  Are you referring to bug 1289913?  We could have this bug just work on the username field and file a second bug for the password field, though I think they would both have to go into the same release (Firefox 52).  Perhaps we could uplift 1289913 if we need to, though that one looks like it has a small string too.

> 2. IMHO, the patch should include the final L10N string (discussing in bug
> 1307316).
Yes, that sounds good.  The final string is: This connection is not secure. Logins entered here could be compromised.
(As seen in https://bugzilla.mozilla.org/show_bug.cgi?id=1307316#c10)

> 
> 3. Should this patch include the final visual? Probably I miss a visual
> spec, and I didn't see it yet.
> 
The visual to use here is https://bug1217150.bmoattachments.org/attachment.cgi?id=8791926.  Some description is here: https://bugzilla.mozilla.org/show_bug.cgi?id=1217150#c28

If there is any part of the design that is unclear, please do ask!

Comment 5

[Tanvi Vyas[:tanvi]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

https://reviewboard.mozilla.org/r/84198/#review83292

::: browser/app/profile/firefox.js:1236
(Diff revision 1)
>  #else
>  pref("security.insecure_password.ui.enabled", false);
>  #endif
>  
> +#ifdef RELEASE_BUILD
> +pref("security.contextualFeedback.enabled", false);

I'm glad you created a separate preference for this!  That way we can keep the url bar warning and the contextual warning separate, in case we have to turn off one but not th eother.

Can we add the "insecure_password" prefix.  Maybe:
security.insecure_password.contextualFeedback.enabled

We can also change this to #ifdef EARLY_BETA_OR_EARLIER, then it is set to true.  else it is set to false.  We can turn this on in early beta, and that might actually help with some user testing.

::: toolkit/components/satchel/AutoCompletePopup.jsm:95
(Diff revision 1)
>    ],
>  
>    init: function() {
> +    Preferences.observe(PREF_CONTEXTUAL_FEEDBACK_ENABLED, this._observePref, this);
> +
> +    this.contextualFeedbackEnabled =

The contextualFeedback is specific to logins.  While AutoCompletePopup is not, so maybe we should change the name of this to include login or password.

I'll leave this up to Matt.
* loginFeedback
* insecureLoginFeedback
* something else?

::: toolkit/content/browser-content.js:1459
(Diff revision 1)
>  
>      let rect = BrowserUtils.getElementBoundingScreenRect(element);
>      let window = element.ownerDocument.defaultView;
>      let dir = window.getComputedStyle(element).direction;
>      let results = this.getResultsFromController(input);
> +    let isHttp = element.ownerGlobal.location.protocol === "http:";

The value of isHttp should come from "isSecureContext".  For example, see bug 1269568 which Matt recently landed.  Note that we need to make sure we are checking isSecureContext on the right frame.  If the password field is in an iframe, then we want to check isSecureContext on the iframe.

Comment 6

[Matthew N. [:MattN]]

I have a lot of the same questions as Tanvi so it would be useful to have them answered before giving feedback.

(In reply to Tanvi Vyas [:tanvi] from comment #5)
> ::: browser/app/profile/firefox.js:1236
> (Diff revision 1)
> >  #else
> >  pref("security.insecure_password.ui.enabled", false);
> >  #endif
> >  
> > +#ifdef RELEASE_BUILD
> > +pref("security.contextualFeedback.enabled", false);
> 
> I'm glad you created a separate preference for this!  That way we can keep
> the url bar warning and the contextual warning separate, in case we have to
> turn off one but not th eother.

Agreed.

> Can we add the "insecure_password" prefix.  Maybe:
> security.insecure_password.contextualFeedback.enabled

I agree with having both prefs under the same prefix though note that you're mixing snake case and camelCase in your suggestion. I also think that we might as well change the ".ui.enabled" to be less broad to reduce confusion: e.g.:
security.insecure_field_warning.urlbar.enabled
security.insecure_field_warning.contextual.enabled
(I'm not including "login" or "password" since we should add credit card support eventually too).

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

https://reviewboard.mozilla.org/r/84198/#review83292

> The contextualFeedback is specific to logins.  While AutoCompletePopup is not, so maybe we should change the name of this to include login or password.
> 
> I'll leave this up to Matt.
> * loginFeedback
> * insecureLoginFeedback
> * something else?

Since we should eventually also do this on credit card fields too, I don't think we need to include login/passwords in the name but I agree with mentioning "secur…" e.g.  (though the problem of putting this stuff not specific to insecure fields in AutoCompletePopup.jsm will go away if we build on bug 1296638)

> The value of isHttp should come from "isSecureContext".  For example, see bug 1269568 which Matt recently landed.  Note that we need to make sure we are checking isSecureContext on the right frame.  If the password field is in an iframe, then we want to check isSecureContext on the iframe.

Well, isSecureContext doesn't take the form action into account so you would want to use the helper that johannh is refactoring out in bug 1302474: InsecurePasswordUtils.isFormSecure.

For now you can use isSecureContext but we need to have this code use that function eventually (perhaps in a follow-up depending on which lands first or whether the last of that and this bug to land makes the change).

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

https://reviewboard.mozilla.org/r/84198/#review84032

Thanks for taking this Sean. I would like the approach of building on bug 1296638 to be pursued rather than adding this logic to AutoCompletePopup.jsm. If that doesn't work well then we can come back to an approach like this. I'm glad this approach isn't too complicated though.

Basically we would use getStyleAt to control the CSS rules you added instead of the custom class (.ac-contextual-feedback-area)

::: toolkit/locales/en-US/chrome/global/autocomplete.dtd:5
(Diff revision 1)
> +<!-- LOCALIZATION NOTE : FILE This file contains the entities needed to -->
> +<!-- LOCALIZATION NOTE : FILE use the Autocomplete. -->

The sentence in this comment would need rephrasing though I think when building on bug 1296638 this could be moved to an existing file maybe.

Comment 9

[Sean Lee [:seanlee][:weilonge]]

Hey Tanvi, thanks for the feedback! Please see my comment below.

(In reply to Tanvi Vyas [:tanvi] from comment #4)
> (In reply to Sean Lee [:seanlee][:weilonge] from comment #3)
> > Comment on attachment 8798780 [details]
> > Bug 1217162 - Implement Contextual Feedback on Insecure Passwords.
> > 
> > Hi Mattn, Tanvi, Mike,
> > 
> > The patch includes a Contextual Feedback content in username's autocomplete
> > panel with HTTP detection, L10N string, and pref for toggling in
> > about:config.
> > 
> Thanks Sean for your patch!
> 
> Is this patch built on top of Mike's
> https://bugzilla.mozilla.org/show_bug.cgi?id=1296638?
No, it's not yet. I targeted to provide a basic warning box with preference toggling in this bug.
Since bug 1296638 is a big patch, shall this patch be built on that?
However, I am fine to build this patch on bug 1296638 if needed.

> 
> Also, is this patch going to be dependent on this change -
> https://bugzilla.mozilla.org/show_bug.cgi?id=376668#c42 ?  Or will this bug
> show the drop down on second focus, and let bug 376668 switch to first focus?
Just the same answer with the above, though I have no idea when the patch can be landed.

For the above two questions about dependency, my concern is only for schedule 52.
If this patch should depend on bug 1296638 and bug 376668, we should remain sufficient time for this patch. 

> 
> 
> > I have some questions about the scope in this bug:
> > 1. How do you think the patch is landed with the username part with L10N
> > changes for 52 first?
> > Password part could be implemented in another bug probably.
> > 
> What do you mean?  Are you referring to bug 1289913?  We could have this bug
> just work on the username field and file a second bug for the password
> field, though I think they would both have to go into the same release
> (Firefox 52).  Perhaps we could uplift 1289913 if we need to, though that
> one looks like it has a small string too.
Thanks for stating my thoughts more clearly.
As far as I know, both password and username use the same L10N string.
I suppose it's fine to include username part only in this bug.

> 
> > 2. IMHO, the patch should include the final L10N string (discussing in bug
> > 1307316).
> Yes, that sounds good.  The final string is: This connection is not secure.
> Logins entered here could be compromised.
> (As seen in https://bugzilla.mozilla.org/show_bug.cgi?id=1307316#c10)
Thanks! I will include it into the following patch.

> 
> > 
> > 3. Should this patch include the final visual? Probably I miss a visual
> > spec, and I didn't see it yet.
> > 
> The visual to use here is
> https://bug1217150.bmoattachments.org/attachment.cgi?id=8791926.  Some
> description is here: https://bugzilla.mozilla.org/show_bug.cgi?id=1217150#c28
I just wonder if there is a more specific description for the dimension, color, size, etc.
> 
> If there is any part of the design that is unclear, please do ask!

Comment 10

[Sean Lee [:seanlee][:weilonge]]

Hi Mattn, Tanvi,

I leave a comment to answer the questions in comment 4.
I think these bugs might block this one, so I summarize my understanding for each bug to come out the dependency and possible solution/risk as far as I know:
1. bug 1289913 - It could be high priority, and this could be uplifted anyway if we land the string changes earlier.
2. bug 1296638 - The contextual warning <xul:hbox id="contextual-feedback-area"> is at the same level with <xul:tree>. I suppose bug 1296638 won't affect this bug even they both change autocomplete.xml. Could you help to point out the dependency?  
3. bug 376668 - do we need the implementation in bug 376668 to finish this bug? this looks like an exist issue, and :jkt is working on that.

If the dependency is confirmed, that's helpful for me to start the implementation.
I am fine to wait for these patches landed first if the schedule is considered.
Please let me know if I misunderstand something. Thanks a lot.

Comment 11

[Ash Grigas]

Sean,
Here are some additional details about the design:
background color for gray part of dropdown: 000000 with 5% opacity

gray line color: D8D8D8, weight: 1px stroke

gray text color: 858585 same font size as we use for the username/password

learn more link color: 0073aa

Let me know if this isn't clear.

The dimensions of the dropdown should be the same as our current username/password drop down except the height of the gray area which should be the height of the text shown plus 4-5 px padding on the left, right and bottom.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

Clearing feedback request - seanlee, MattN and I talked last night on IRC, and we have a way forward with a different approach (using a richlistitem once bug 1296638 is merged).

Comment 13

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84198/diff/1-2/

Comment 14

[Sean Lee [:seanlee][:weilonge]]

Thanks a lot for MattN's great help!!
We come out a new design to show the contextual warning in a richlistitem, and it can handle the case with no username.
I merged the preference part and L10N into the new design, and there are some issues should be fixed:
1. Handle the click behavior on the contextual warning.
2. Refine the css rules.
3. Apply the final visual design (comment 11, Thanks, Ash!)

Comment 15

[Tanvi Vyas[:tanvi]]

Also clearing my feedback request, as I emailed Sean about dependencies and Matt also spoke with him.

Marking this a P1, since it has to go into Firefox 52.

Comment 16

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84198/diff/2-3/

Comment 17

[Sean Lee [:seanlee][:weilonge]]

The new patch includes "Learn More" label which is placed at the bottom of description in a vbox.

Comment 18

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84198/diff/3-4/

Comment 19

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

Hi MattN,

The latest patch includes the following changes:
1. "Learn More" label which is placed at the bottom of description in a vbox. vbox.ac-title is changed from hbox.ac-title.
2. "unselectable" field is for the contextual warning richlistitem which indicates the item is unselectable and should not been filled to the input box. the mouseup event [1] is triggered at autocomplete-richlistbox, so I use a field to fix this.

Could you give it a feedback? Thank you!

[1 ]http://searchfox.org/mozilla-central/source/toolkit/content/widgets/autocomplete.xml#2317

Comment 20

[Sean Lee [:seanlee][:weilonge]]

Hey MattN,

The patch in attachment 8798780 [details] doesn't work when a user uses keyboard to select a richlistitem.
I am tracing the keyboard part and trying to find a solution for it.

Here is the flow for mouse part:
1. When mouseup event is triggered [1], "FormAutoComplete:HandleEnter" will be sent[2].
2. The handler of "FormAutoComplete:HandleEnter"[3] invokes handleEnter relative codes, and the event "DOMAutoComplete"[4] will be sent.
3. "DOMAutoComplete" handler, LoginManagerContent.onUsernameInput, will fill username and password field.

As far as I know, keyboard flow will do the same step 3, but step 1 and 2.

Could you help to answer the keyboard part? Thanks.

[1] http://searchfox.org/mozilla-central/rev/e3e8571c5378ac92663d4f583ccc4ad0a3019716/toolkit/content/widgets/autocomplete.xml#2328
[2] http://searchfox.org/mozilla-central/rev/e3e8571c5378ac92663d4f583ccc4ad0a3019716/toolkit/components/satchel/AutoCompletePopup.jsm#279
[3] http://searchfox.org/mozilla-central/rev/e3e8571c5378ac92663d4f583ccc4ad0a3019716/toolkit/content/browser-content.js#1464
[4] http://searchfox.org/mozilla-central/rev/e3e8571c5378ac92663d4f583ccc4ad0a3019716/toolkit/components/satchel/nsFormFillController.cpp#583

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

https://reviewboard.mozilla.org/r/84198/#review87818

I'll review the `unselectable` logic now.

::: browser/app/profile/firefox.js:1232
(Diff revision 4)
> +#ifdef EARLY_BETA_OR_EARLIER
> +pref("security.contextualFeedback.enabled", false);
> +#else
> +pref("security.contextualFeedback.enabled", true);

As mentioned before, this pref name needs to mention something about "field" otherwise I would have no idea what the pref is for.

::: browser/app/profile/firefox.js:1232
(Diff revision 4)
> +#ifdef EARLY_BETA_OR_EARLIER
> +pref("security.contextualFeedback.enabled", false);
> +#else
> +pref("security.contextualFeedback.enabled", true);
> +#endif

I don't understand why we would put the EARLY_BETA_OR_EARLIER ifdef. We want this in 52 so either we should have it as Nightly only until it's ready or have no ifdef with a value depending on how much follow-up work there is.

::: browser/base/content/browser.css:492
(Diff revision 4)
> +  -moz-binding: url("chrome://global/content/bindings/autocomplete.xml#autocomplete-richlistitem-insecure-field");
> +  height: auto;
> +}
> +
> +#PopupAutoComplete > richlistbox > richlistitem[originaltype="insecureStyle"] > .ac-site-icon {
> +  display: -moz-box;

display: initial

::: browser/base/content/browser.css:505
(Diff revision 4)
> +#PopupAutoComplete > richlistbox > richlistitem[originaltype="insecureStyle"] > .ac-title > label {
> +  margin-inline-start: 0;
> +}
> +
> +#PopupAutoComplete > richlistbox > richlistitem[originaltype="insecureStyle"] > .ac-url {
> +  /*display: -moz-box;*/

Leftover

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:310
(Diff revision 4)
>                          rect: aRect,
> +                        insecure: !win.isSecureContext,
>                          remote: remote };

I also think we should rename this property to `isSecure` to avoid potential double-negatives in the future.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:311
(Diff revision 4)
>      let messageData = { formOrigin: formOrigin,
>                          actionOrigin: actionOrigin,
>                          searchString: aSearchString,
>                          previousResult: previousResult,
>                          rect: aRect,
> +                        insecure: !win.isSecureContext,

Now that bug 1302474 has landed you should use `InsecurePasswordUtils.isFormSecure(form)` instead.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1207
(Diff revision 4)
>      return this._getPasswordOrigin(uriString, true);
>    },
>  };
>  
>  // nsIAutoCompleteResult implementation
> -function UserAutoCompleteResult (aSearchString, matchingLogins, messageManager) {
> +function UserAutoCompleteResult (aSearchString, matchingLogins, messageManager, insecure) {

Can you make the third argument an object:

`{messageManager, isSecure}` since messageManager is optional. There are only 3 callers IIRC.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1221
(Diff revision 4)
>        return  1;
>  
>      return 0;
>    }
>  
> +  let contextualFeedbackEnabled =

"contextualFeedbackEnabled" isn't meaningful in this context. Please rename this.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1257
(Diff revision 4)
>    defaultIndex : -1,
>    errorDescription : "",
>    matchCount : 0,
>  
>    getValueAt(index) {
> -    if (index < 0 || index >= this.logins.length)
> +    if (index < 0 || index >= (this.logins.length + this._showContextualWarning))

Could we use `this.matchCount` here instead?

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1261
(Diff revision 4)
>    getValueAt(index) {
> -    if (index < 0 || index >= this.logins.length)
> +    if (index < 0 || index >= (this.logins.length + this._showContextualWarning))
>        throw new Error("Index out of range.");
>  
> -    return this.logins[index].username;
> +    if (this._showContextualWarning && index === 0) {
> +      return "This connection is not secure. Logins entered here could be compromised.";

This needs to move to a properties file

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1275
(Diff revision 4)
>    getStyleAt(index) {
> +    if (index === 0 && this._showContextualWarning) {
> +      return "insecureStyle";

Nit: the "Style" suffix isn't necessary

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1276
(Diff revision 4)
>    getCommentAt(index) {
>      return "";
>    },
>  
>    getStyleAt(index) {
> +    if (index === 0 && this._showContextualWarning) {

Nit: We normally prefer `==`. In this case `index` wil always be a Number.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1283
(Diff revision 4)
> +    }
>      return "";
>    },
>  
>    getImageAt(index) {
> +    if (index === 0 && this._showContextualWarning) {

`==` here too

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1284
(Diff revision 4)
>      return "";
>    },
>  
>    getImageAt(index) {
> +    if (index === 0 && this._showContextualWarning) {
> +      return "chrome://browser/skin/connection-mixed-active-loaded.svg#icon";

Nit: It would be ideal to move this to CSS actually, if it's already setup for that. It looks like you already have some CSS in this patch that is similar.

::: toolkit/content/widgets/autocomplete.xml:1476
(Diff revision 4)
> +        <xul:description class="ac-text-overflow-container">
> +          <xul:description anonid="title-text"
> +                           class="ac-title-text"
> +                           xbl:inherits="selected"/>
> +        </xul:description>
> +        <xul:label align="left" class="text-link" href="https://support.mozilla.org/en-US/kb/insecure-password-warning-firefox?as=u&amp;utm_source=inproduct" value="&autocomplete.contextualFeedback.learnMore;"/>

Hmm… why does the string and URL have to be duplicated?

::: toolkit/content/widgets/autocomplete.xml:1500
(Diff revision 4)
> +                align="center"
> +                xbl:inherits="selected,actiontype">
> +        <xul:description class="ac-text-overflow-container">
> +          <xul:description anonid="url-text"
> +                           class="ac-url-text"
> +                           href="https://support.mozilla.org/en-US/kb/insecure-password-warning-firefox?as=u&amp;utm_source=inproduct"

This URL shouldn't be hard-coded, especially not an en-US link. There is a helper to generate a support link where you just pass "insecure-password-warning-firefox"

::: toolkit/locales/en-US/chrome/global/autocomplete.dtd:8
(Diff revision 4)
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<!-- LOCALIZATION NOTE : FILE This file contains the entities needed to -->
> +<!-- LOCALIZATION NOTE : FILE use the Autocomplete. -->
> +
> +<!ENTITY autocomplete.contextualFeedback.description "This connection is not secure. Logins entered here could be compromised.">

This is unused

::: toolkit/locales/en-US/chrome/global/autocomplete.dtd:9
(Diff revision 4)
> +
> +<!-- LOCALIZATION NOTE : FILE This file contains the entities needed to -->
> +<!-- LOCALIZATION NOTE : FILE use the Autocomplete. -->
> +
> +<!ENTITY autocomplete.contextualFeedback.description "This connection is not secure. Logins entered here could be compromised.">
> +<!ENTITY autocomplete.contextualFeedback.learnMore "Learn More">

"autocomplete.contextualFeedback" doesn't tell me much about what this string is for.

::: toolkit/themes/windows/global/autocomplete.css:180
(Diff revision 4)
>  
>  toolbarpaletteitem > toolbaritem > * > textbox > hbox > hbox > html|*.textbox-input {
>    visibility: hidden;
>  }
>  
> +/* ::::: contextual feedback ::::: */

As I said before "Contextual feedback" isn't a meaningful description. Something like "Insecure form  field warning" would be much better.

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

https://reviewboard.mozilla.org/r/84198/#review87834

Please mark issues in ReviewBoard as resolved when you fix them.

::: browser/app/profile/firefox.js:1232
(Diff revision 4)
> +#ifdef EARLY_BETA_OR_EARLIER
> +pref("security.contextualFeedback.enabled", false);
> +#else

This was also backwards btw.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1256
(Diff revision 4)
>    getValueAt(index) {
> -    if (index < 0 || index >= this.logins.length)
> +    if (index < 0 || index >= (this.logins.length + this._showContextualWarning))
>        throw new Error("Index out of range.");
>  
> -    return this.logins[index].username;
> +    if (this._showContextualWarning && index === 0) {
> +      return "This connection is not secure. Logins entered here could be compromised.";

GetValueAt(0) needs to return "" in this case to hit https://dxr.mozilla.org/mozilla-central/rev/f9f3cc95d7282f1fd83f66dd74acbcdbfe821915/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1550-1554

We need to go back to how we had it when we did pair programming so getLabelAt and getValueAt have different behaviour. The value of the warning row should be "" but the label should be the visible text.

Please add an automated test to ensure that the field value isn't modified if the warning row is selected.

::: toolkit/components/satchel/AutoCompletePopup.jsm:57
(Diff revision 4)
> -  handleEnter: function(aIsPopupSelection) {
> -    AutoCompletePopup.handleEnter(aIsPopupSelection);
> +  handleEnter: function(aIsPopupSelection, isUnselectable) {
> +    AutoCompletePopup.handleEnter(aIsPopupSelection, isUnselectable);
>    },

We don't need a new concept of unselectable as the autocomplete controller already special cases an empty string value "" for this: https://dxr.mozilla.org/mozilla-central/rev/f9f3cc95d7282f1fd83f66dd74acbcdbfe821915/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1550-1554

That was the function I was wanting you to look at on IRC the other day when I mentioned looking for where the observer notifications and events were dispatched.

::: toolkit/content/widgets/autocomplete.xml:1458
(Diff revision 4)
> +  <binding id="autocomplete-richlistitem-insecure-field" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-richlistitem">
> +    <content align="center"

Your binding isn't picking up the styling to invert the text for selected rows. We will probably want to add a rule for the Learn More link too and maybe even invert the icon. See https://i.imgur.com/rRBhKVH.png

Comment 23

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84198/diff/4-5/

Comment 24

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

https://reviewboard.mozilla.org/r/84198/#review87818

> Hmm… why does the string and URL have to be duplicated?

The URL in xul:label is the correct one in the current patch. I know it should not be hard-coded here, but I can't find a way to override it in js/jsm. I will try to resolve this if needed.
Besides, do you think it's acceptable to place "Learn More" string in autocomplate.dtd rather than autocomplete.properties?
I gusee that it should not be hard-coded as well.

> This URL shouldn't be hard-coded, especially not an en-US link. There is a helper to generate a support link where you just pass "insecure-password-warning-firefox"

could you show me where the helper is?

Comment 25

[Sean Lee [:seanlee][:weilonge]]

Hi MattN,

Thanks a lot for addressing these issues. They are almost resolved except "Learn More" link part.
There are some tasks remain to do, and some of them need your suggestions:
1. "Learn More" link doesn't work after clicking it. I guess it's relative to a mouse-up event [1], and a click behavior for link should not be filtered. I don't have an implementation so far.

2. "Learn More" DOM structure and L10N string. I am not sure if it's an acceptable design for the current Learn More link. Please suggest if there is any concern.

3. mochitest. I am trying to implement a draft.

3. Popup height is not extend to the correct dimension when the pop-up is shown at first time. I guess it's relative to <method name="adjustHeight">[2], and I am figuring out the root-cause.

4. There is a right margin/padding at right side of description. I have no idea the solution yet.

5. I suppose 1, 2, and 3 seem the must for this patch, so 4 and 5 could be resolved in a follow-up bug since we need to land the string first.

[1] http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/toolkit/content/widgets/autocomplete.xml#2320
[2] http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/toolkit/content/widgets/autocomplete.xml#1219

Comment 26

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

https://reviewboard.mozilla.org/r/84198/#review87834

> Your binding isn't picking up the styling to invert the text for selected rows. We will probably want to add a rule for the Learn More link too and maybe even invert the icon. See https://i.imgur.com/rRBhKVH.png

After adding `background-color: #F6F6F6` to `#PopupAutoComplete > richlistbox > richlistitem[originaltype="insecureWarning"]`, the selected warning message won't be in a inverted style.
I suppose the description is not clickable, so it should not be in a selected style when mouse-hover.
However, "Learn More" link should be in a selected style, and it's given class="text-link".

Comment 27

[Sean Lee [:seanlee][:weilonge]]

test_basic_form_autocomplete.html[1] is a good example for insecure form field mochitest, and I will start from it.
[1] http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/toolkit/components/passwordmgr/test/mochitest/test_basic_form_autocomplete.html

Comment 28

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84198/diff/5-6/

Comment 29

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

Hi MattN,

In the latest patch, I add a test which is copied from test_basic_form_autocomplete.html[1].
I suppose all tasks could be helpful for the popup with insecure form field warning, so I remain them and resolve them one by one. That's why you see some tasks are disabled. I will fix them later.

Please give this patch a feedback. Thank you.

[1] http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/toolkit/components/passwordmgr/test/mochitest/test_basic_form_autocomplete.html

Comment 30

[Sean Lee [:seanlee][:weilonge]]

BTW, I will fix all tasks if you think it's worth. HTTPS case will be considered as well but later.

Comment 31

[Matthew N. [:MattN]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

https://reviewboard.mozilla.org/r/84198/#review87818

> The URL in xul:label is the correct one in the current patch. I know it should not be hard-coded here, but I can't find a way to override it in js/jsm. I will try to resolve this if needed.
> Besides, do you think it's acceptable to place "Learn More" string in autocomplate.dtd rather than autocomplete.properties?
> I gusee that it should not be hard-coded as well.

I think we should move the learn more and insecureFieldWarningDescription strings to passwordmgr.properties to keep them together and with the other password manager strings. You can use the same pattern as <property name="_stringBundle"> in this file.

> could you show me where the helper is?

I thought there was one but it seems like most consumers use `Services.urlFormatter.formatURLPref("app.support.baseURL") + "insecure-password-warning-firefox";`. You can probably do that in the constructor of the binding actually as I think it's okay to hard-code the identifier.

Comment 32

[Matthew N. [:MattN]]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #25)
> There are some tasks remain to do, and some of them need your suggestions:
> 1. "Learn More" link doesn't work after clicking it. I guess it's relative
> to a mouse-up event [1], and a click behavior for link should not be
> filtered. I don't have an implementation so far.

Hmm… I think we should land the string but have the link hidden for now and fix this in a follow-up. I was thinking maybe we could listen to FormAutoComplete:HandleEnter and check the `selectedIndex` but didn't see an obvious solution at first glance.

> 2. "Learn More" DOM structure and L10N string. I am not sure if it's an
> acceptable design for the current Learn More link. Please suggest if there
> is any concern.

See comment 31.

> 3. mochitest. I am trying to implement a draft.

Please use `hg copy` when copying with modifications, especially for large or non-trivial copies.

> 3. Popup height is not extend to the correct dimension when the pop-up is
> shown at first time. I guess it's relative to <method
> name="adjustHeight">[2], and I am figuring out the root-cause.

I did notice that as well. I agree this doesn't block landing.

> 4. There is a right margin/padding at right side of description. I have no
> idea the solution yet.

OK, I agree this doesn't block landing too.

(In reply to Sean Lee [:seanlee][:weilonge] from comment #26)
> > Your binding isn't picking up the styling to invert the text for selected rows. We will probably want to add a rule for the Learn More link too and maybe even invert the icon. See https://i.imgur.com/rRBhKVH.png
> 
> After adding `background-color: #F6F6F6` to `#PopupAutoComplete >
> richlistbox > richlistitem[originaltype="insecureWarning"]`, the selected
> warning message won't be in a inverted style.
> I suppose the description is not clickable, so it should not be in a
> selected style when mouse-hover.
> However, "Learn More" link should be in a selected style, and it's given
> class="text-link".

IMO clicking or hitting enter on the row should activate the "Learn More" link so I actually think the selection styling needs to remain but we need to have the text properly inverting when selected.

Comment 33

[Matthew N. [:MattN]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

https://reviewboard.mozilla.org/r/84198/#review88312

::: browser/app/profile/firefox.js:1230
(Diff revision 6)
> +#ifdef RELEASE_OR_BETA
> +pref("security.insecure_field_warning.contextual.enabled", false);
> +#else
> +pref("security.insecure_field_warning.contextual.enabled", true);
> +#endif

I still don't understand why we need to use `#ifdef RELEASE_OR_BETA` at all?

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1220
(Diff revision 6)
>      return this._getPasswordOrigin(uriString, true);
>    },
>  };
>  
>  // nsIAutoCompleteResult implementation
> -function UserAutoCompleteResult (aSearchString, matchingLogins, messageManager) {
> +function UserAutoCompleteResult (aSearchString, matchingLogins, options) {

I was suggesting you use the newer syntax for the options argument so it's unnamed and self-documenting: 
`function UserAutoCompleteResult (aSearchString, matchingLogins, {isSecure, messageManager}) {`

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:273
(Diff revision 6)
>      // autocomplete popup here because the autocomplete protocol hasn't
>      // been e10s-ized yet. In the non-e10s case, our caller is responsible
>      // for showing the autocomplete popup (via the regular
>      // nsAutoCompleteController).
>      if (remote) {
> -      let results = new UserAutoCompleteResult(searchString, matchingLogins);
> +      let results = new UserAutoCompleteResult(searchString, matchingLogins, {isSecure: isSecure});

Nit: `{isSecure}` is sufficient

::: toolkit/components/passwordmgr/nsLoginManager.js:490
(Diff revision 6)
> +    let form = LoginFormFactory.createFromField(aElement);
> +    let isSecure = InsecurePasswordUtils.isFormSecure(form);
> +
>      if (!this._remember) {
>        setTimeout(function() {
> -        aCallback.onSearchCompletion(new UserAutoCompleteResult(aSearchString, []));
> +        aCallback.onSearchCompletion(new UserAutoCompleteResult(aSearchString, [], {isSecure: isSecure}));

`{isSecure}`

::: toolkit/components/passwordmgr/nsLoginManager.js:519
(Diff revision 6)
> +                                   messageManager: messageManager,
> +                                   isSecure: isSecure

You don't need to specify object values if there is a local variable with that name which you're using as the value:
```{
messageManager,
isSecure,
}```

::: toolkit/components/passwordmgr/test/mochitest/test_insecure_form_field_autocomplete.html:1
(Diff revision 6)
> +<!DOCTYPE HTML>

Please make this file an `hg copy` of the original so I can review only what changed.

Comment 34

[Matthew N. [:MattN]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

Getting close to landing.

Comment 35

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84198/diff/6-7/

Comment 36

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

https://reviewboard.mozilla.org/r/84198/#review88312

> I still don't understand why we need to use `#ifdef RELEASE_OR_BETA` at all?

Since there are some follow-up bugs should be implemented fisrt, then the flag can be configured to "true" later.
I've already marked it as "false" without any #ifdef.

Comment 37

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

https://reviewboard.mozilla.org/r/84198/#review87834

> After adding `background-color: #F6F6F6` to `#PopupAutoComplete > richlistbox > richlistitem[originaltype="insecureWarning"]`, the selected warning message won't be in a inverted style.
> I suppose the description is not clickable, so it should not be in a selected style when mouse-hover.
> However, "Learn More" link should be in a selected style, and it's given class="text-link".

This issue can be fixed in a follow-up bug, so I mark it as "Fixed" here.

Comment 38

[Sean Lee [:seanlee][:weilonge]]

Hi MattN,

The latest patch are with a lot of fixes and using "hg copy" for the test. Please help to review it. Thanks.

Comment 39

[Sean Lee [:seanlee][:weilonge]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8036032b001a

Comment 40

[Matthew N. [:MattN]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

https://reviewboard.mozilla.org/r/84198/#review89918

Tests look good and this would have been an "r+ with changes" except for the `index--` issue and the support URL building.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1317
(Diff revision 7)
> +    }
> +    index--;

Isn't the `index--` only needed if `this._showInsecureFieldWarning` is true?

::: toolkit/components/passwordmgr/nsLoginManager.js:518
(Diff revision 7)
> -                                 new UserAutoCompleteResult(aSearchString, logins, messageManager);
> +                                 new UserAutoCompleteResult(aSearchString, logins, {
> +                                   messageManager, isSecure
> +                                 });

One property per line please with trailing commas

::: toolkit/components/passwordmgr/test/mochitest/test_insecure_form_field_autocomplete.html:762
(Diff revision 7)
>    shownPromise = promiseACShown();
>    sendChar("A");
>    let results = yield shownPromise;
>  
>    checkACForm("form9userAAB", "");
> -  checkArrayValues(results, ["form9userAAB"], "Check dropdown is updated after inserting 'A'");
> +  checkArrayValues(results, ["This connection is not secure. Logins entered here could be compromised.","form9userAAB"], "Check dropdown is updated after inserting 'A'");

Nit: missing space after the comma. You should probably put a line break before "Check dropdown"

::: toolkit/content/widgets/autocomplete.xml:1512
(Diff revision 7)
> +    </content>
> +    <implementation>
> +      <constructor><![CDATA[
> +        let learnMoreLink = document.getElementById("learnMoreLink");
> +        learnMoreLink.setAttribute("value", this._stringBundle.GetStringFromName("insecureFieldWarningLearnMore"));
> +        learnMoreLink.setAttribute("href", "https://support.mozilla.org/kb/insecure-password-warning-firefox?as=u&utm_source=inproduct");

Please use the helper I mentioned, don't build the URL yourself.

Comment 41

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

https://reviewboard.mozilla.org/r/84198/#review89918

> Please use the helper I mentioned, don't build the URL yourself.

I found any "https://support.mozilla.org/kb/*" link is hardcode [1], and this is why I did not change it with Services.urlFormatter.formatURLPref.

"en-US" in the URL is removed to prevent the wrong L10N configuration. I suppose this needs to add this line like [1] in firefox.js

pref("app.support.baseURL", "https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/");

(I did reply here, but it was not posted correctly. I think I forgot to click the post button.)

[1] http://searchfox.org/mozilla-central/search?q=https%3A%2F%2Fsupport.mozilla.org%2Fkb&case=false&regexp=false&path=
[2] http://searchfox.org/mozilla-central/source/browser/app/profile/firefox.js#889

Comment 42

[Matthew N. [:MattN]]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #41)
> Comment on attachment 8798780 [details]
> Bug 1217162 - Implement insecure field warning on username field.;
> 
> https://reviewboard.mozilla.org/r/84198/#review89918
> 
> > Please use the helper I mentioned, don't build the URL yourself.
> 
> I found any "https://support.mozilla.org/kb/*" link is hardcode [1], and
> this is why I did not change it with Services.urlFormatter.formatURLPref.

See the 44 results at https://dxr.mozilla.org/mozilla-central/search?q=formatURLPref(%22app.support.baseURL%22) which are doing things correctly.

I forgot that SUMO needs to have the identifier setup manually so I will ask Joni for this. We don't need to block landing on the URL working though.

Hi Joni, can you setup https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/insecure-form-field-warning to point to the https://support.mozilla.org/kb/insecure-password-warning-firefox article. The reason for "insecure-form-field-warning" instead of mentioning passwords/login is because we will eventually want to show a warning on credit card fields too and we could use the same article for that (perhaps with a different anchor).

Comment 43

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84198/diff/7-8/

Comment 44

[Sean Lee [:seanlee][:weilonge]]

Hi MattN, all issues you mentioned on mozreview are resolved. Please review them again. Thank you!

Comment 45

[Matthew N. [:MattN]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

https://reviewboard.mozilla.org/r/84198/#review89962

It would have been nice to actually test the string of the warning but at least you're testing the existing so I guess it's okay for now.

::: toolkit/content/widgets/autocomplete.xml:1512
(Diff revisions 7 - 8)
>      </content>
>      <implementation>
>        <constructor><![CDATA[
>          let learnMoreLink = document.getElementById("learnMoreLink");
>          learnMoreLink.setAttribute("value", this._stringBundle.GetStringFromName("insecureFieldWarningLearnMore"));
> -        learnMoreLink.setAttribute("href", "https://support.mozilla.org/kb/insecure-password-warning-firefox?as=u&utm_source=inproduct");
> +        learnMoreLink.setAttribute("href", Services.urlFormatter.formatURLPref("app.support.baseURL") + "insecure-password-warning-firefox");

This should be "insecure-form-field-warning" now

Comment 46

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8798780 [details]
Bug 1217162 - Implement insecure field warning on username field.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84198/diff/8-9/

Comment 47

[Sean Lee [:seanlee][:weilonge]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=264039f29159

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e315a1721ee
Implement insecure field warning on username field.; r=MattN

Comment 49

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3e315a1721ee

Comment 50

[Joni Chan]

Thanks for letting me know, @MattN. I've set up that link so you can use it now.

Is the text change for 52? If so, I'll make a note to update the article to reflect credit card fields too.

Comment 51

[Tanvi Vyas[:tanvi]]

(In reply to Joni Savage ("need info" me) from comment #50)
> Thanks for letting me know, @MattN. I've set up that link so you can use it
> now.
> 
> Is the text change for 52? If so, I'll make a note to update the article to
> reflect credit card fields too.

Hi Joni,

We aren't supporting credit card fields yet but may in the future.  So no need to add credit card text for 52.  There are some other enhancements to the UI we are making in Firefox 52 though, so we may need to update the article to reflect that.  I will contact you about that once Firefox 52 goes to Aurora and I have a better picture of what changes landed and which ones didn't.  Thanks!