Closed Bug 1371149 Opened 3 years ago Closed 3 years ago

[Form Autofill] Warn users on credit card fields that are not secure

Categories

(Toolkit :: Form Manager, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: lchang, Assigned: ralin)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [form autofill:M4][ETA:8/2])

Attachments

(2 files)

When users try to autofill credit cards in an insecure website (http), a warning will be shown on the dropdown and temporarily disable the credit card autofill function.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1349681
The bugs were dupes but I realized that we actually need two bugs anyways: one for the warning in the autocomplete dropdown and one for the warning in the address bar. This bug can focus on the autocomplete dropdown warning.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Blocks: 1349681
Status: REOPENED → NEW
Summary: [Form Autofill] Notify user that their connection is not secure when credit card fields are available → [Form Autofill] Warn users on credit card fields that are not secure
See Also: → 46590
Assignee: nobody → ralin
I was thinking of adding a browser test in Part 2, but it turned out I couldn't find an appropriate way to add CC record now. However the main idea of Part 1 patch won't change anyway, I think it's ready to review first. Thanks.
Attachment #8885113 - Flags: review?(selee)
Whiteboard: [form autofill:M4] → [form autofill:M4][ETA:8/2]
Comment on attachment 8885113 [details]
Bug 1371149 - Part 1. Show insecure field in credit card autofill dropdown instead of result when the connection is not secure.

https://reviewboard.mozilla.org/r/155970/#review163920

::: browser/extensions/formautofill/FormAutofillContent.jsm:120
(Diff revision 1)
> +      // TODO: should use pref observer to cache this value in somewhere.
> +      let insecureWarningEnabled = Services.prefs.getBoolPref("security.insecure_field_warning.contextual.enabled", false);

Use `XPCOMUtils.defaultLazyPreferenceGetter`

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:22
(Diff revision 1)
>  
>  this.ProfileAutoCompleteResult = function(searchString,
>                                            focusedFieldName,
>                                            allFieldNames,
>                                            matchingProfiles,
> -                                          {resultCode = null}) {
> +                                          {resultCode = null, isSecure = false, insecureWarningEnabled = true}) {

I think the `insecureWarningEnabled` check could move inside this constructor so it doesn't need to be passed as an argument. You can have a lazy preference getter in global scope that you can refer to inside the result. It just seems not ideal to have to pass the pref in for each consumer and doesn't reduce testability if we use defineLazyPreferenceGetter since that watches pref changes.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:43
(Diff revision 1)
> +    "creditcard" : "address";
> +  this._shouldShowInsecureWarning = !isSecure && this._resultType == "creditcard";
> +
> +  this._popupLabels = [];
> +
> +  // Show only insecure warning for credit card result when the connection is not secure.

Nit: Add a "the": "Show only the insecure…"

s/result/results/

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:46
(Diff revision 1)
> +  this._popupLabels = [];
> +
> +  // Show only insecure warning for credit card result when the connection is not secure.
> +  if (this._shouldShowInsecureWarning) {
> +    if (insecureWarningEnabled) {
> +      this._popupLabels.push({primary: "", secondary: ""});

Could you explain why we don't pass the real string in here?

::: browser/extensions/formautofill/content/formautofill.xml:275
(Diff revision 1)
> +    <!--
> +    <implementation></implementation>
> +    -->

Remove this if we don't end up using it before landing

::: browser/extensions/formautofill/locale/en-US/formautofill.properties:34
(Diff revision 1)
>  # text that is displayed for informing users what categories are about to be filled.
>  # "%S" will be replaced with a list generated from the pre-defined categories.
>  # The text would be e.g. Also fill company, phone, email
>  phishingWarningMessage = Also fill %S
>  phishingWarningMessage2 = Fill %S
> +insecureFieldWarningDescription = Firefox has detected an insecure site. Credit card autofill is temporarily disabled

You shouldn't hardcode "Firefox" in strings ever. You should substitute brandShortName with %S
Attachment #8885113 - Flags: review?(MattN+bmo)
Comment on attachment 8885113 [details]
Bug 1371149 - Part 1. Show insecure field in credit card autofill dropdown instead of result when the connection is not secure.

https://reviewboard.mozilla.org/r/155970/#review163920

> Use `XPCOMUtils.defaultLazyPreferenceGetter`

I meant `defineLazyPreferenceGetter`
Comment on attachment 8885113 [details]
Bug 1371149 - Part 1. Show insecure field in credit card autofill dropdown instead of result when the connection is not secure.

https://reviewboard.mozilla.org/r/155970/#review166016

Thanks for Matt did the first round review.
Hey Ray, do you think this patch should be based on bug 1371131 or not?
If so, please rebase this patch on top of that with the fix of Matt addressed.
I will do the review again. Thank you.
Attachment #8885113 - Flags: review?(selee)
Comment on attachment 8885113 [details]
Bug 1371149 - Part 1. Show insecure field in credit card autofill dropdown instead of result when the connection is not secure.

https://reviewboard.mozilla.org/r/155970/#review163920

Thanks for the review, the issues are addressed in the new revision.
Comment on attachment 8885113 [details]
Bug 1371149 - Part 1. Show insecure field in credit card autofill dropdown instead of result when the connection is not secure.

https://reviewboard.mozilla.org/r/155970/#review168602

::: browser/extensions/formautofill/FormAutofillUtils.jsm:50
(Diff revision 2)
> +  get stringBundle() {
> +    return Services.strings.createBundle("chrome://formautofill/locale/formautofill.properties");
> +  },

Can you use XPCOMUtils.defineLazyGetter at the bottom of the file to define this property on `FormAutofillUtils`

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:43
(Diff revision 2)
>      // The default item that should be entered if none is selected
>      this.defaultIndex = 0;
>      // The reason the search failed
>      this.errorDescription = "";
> +    // The value used to determine whether the connection is secure or not.
> +    // this._isSecure = isSecure;

Delete this leftover line
Comment on attachment 8885113 [details]
Bug 1371149 - Part 1. Show insecure field in credit card autofill dropdown instead of result when the connection is not secure.

https://reviewboard.mozilla.org/r/155970/#review169630

LGTM. Thank you.

::: browser/extensions/formautofill/locales/en-US/formautofill.properties:59
(Diff revision 3)
>  tel = Phone
>  email = Email
>  cancel = Cancel
>  save = Save
>  countryWarningMessage = Autofill is currently available only for US addresses
> +

nit: is this new line necessary?
Attachment #8885113 - Flags: review?(selee) → review+
Comment on attachment 8891674 [details]
Bug 1371149 - Part 2. Add a chrome browser test for form autofill insecure field.

https://reviewboard.mozilla.org/r/162744/#review169624

LGTM. Thank you.

::: browser/extensions/formautofill/test/browser/browser_insecure_form.js:3
(Diff revision 4)
> +"use strict";
> +
> +// const URL = BASE_URL + "autocomplete_basic.html";

nit: is it a leftoever line?

::: browser/extensions/formautofill/test/browser/head.js:10
(Diff revision 4)
> +            getAddresses, saveAddress, removeAddresses, saveCreditCard,
> +            getDisplayedPopupItems */
>  
>  "use strict";
>  
> +// ://example.org/browser/browser/extensions/formautofill/test/browser/autocomplete_creditcard_basic.html"

nit: leftover?
Attachment #8891674 - Flags: review?(selee) → review+
Comment on attachment 8885113 [details]
Bug 1371149 - Part 1. Show insecure field in credit card autofill dropdown instead of result when the connection is not secure.

https://reviewboard.mozilla.org/r/155970/#review169630

Thank you for the reviews :D

> nit: is this new line necessary?

hmm, I'm not sure the best practice of it, but since we've been using newlines to separate different UI conceptually in sections, removing the newline might looks like the entity belongs to dialog. Perhpas, adding namespace prefix is a nice way to go if we got some times to refactor those in the future. (I guess we didn't foresee the growth of locales in the beginning)
Comment on attachment 8885113 [details]
Bug 1371149 - Part 1. Show insecure field in credit card autofill dropdown instead of result when the connection is not secure.

https://reviewboard.mozilla.org/r/155970/#review171016

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:24
(Diff revision 4)
> +
>  this.log = null;
>  FormAutofillUtils.defineLazyLogGetter(this, this.EXPORTED_SYMBOLS[0]);
>  
>  class ProfileAutoCompleteResult {
> -  constructor(searchString, focusedFieldName, allFieldNames, matchingProfiles, {resultCode = null}) {
> +  constructor(searchString, focusedFieldName, allFieldNames, matchingProfiles, {resultCode = null, isSecure = true}) {

Nit: This line is getting long and IMO would be more readable like so:
```js
…, matchingProfiles, {
  resultCode = null,
  isSecure = true,
}) {
```
with the trailing comma so future options don't change blame of existing lines.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:42
(Diff revision 4)
>      this._matchingProfiles = matchingProfiles;
>      // The default item that should be entered if none is selected
>      this.defaultIndex = 0;
>      // The reason the search failed
>      this.errorDescription = "";
> +    // The value used to determine whether the connection is secure or not.

Nit: s/connection/form/ since @action is also considered and there is no "connection" for that when we're computing this.
Attachment #8885113 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8891674 [details]
Bug 1371149 - Part 2. Add a chrome browser test for form autofill insecure field.

https://reviewboard.mozilla.org/r/162744/#review171024

Thanks!
Attachment #8891674 - Flags: review?(MattN+bmo) → review+
Status: NEW → ASSIGNED
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fce47fd69d94
Part 1. Show insecure field in credit card autofill dropdown instead of result when the connection is not secure. r=MattN,seanlee
https://hg.mozilla.org/integration/autoland/rev/4c4c732eef94
Part 2. Add a chrome browser test for form autofill insecure field. r=MattN,seanlee
Keywords: checkin-needed
backed out for test failurs like https://treeherder.mozilla.org/logviewer.html#?job_id=121671031&repo=autoland
Flags: needinfo?(ralin)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/74c153731cbc
Backed out changeset 4c4c732eef94 
https://hg.mozilla.org/integration/autoland/rev/91b30f63e8d8
Backed out changeset fce47fd69d94 for test failures in browser_insecure_form.js | Uncaught exception - popup should be open - timed out after 50 tries
Thank you :Tomcat 

It looks like there's a conflict between https://hg.mozilla.org/mozilla-central/rev/21395e9c310f and this patch. I'll do rebase again to fix the test.
Test is passed now, try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74430c48ccf1ee2a1df34d31d6a4e7bb665b57dc

Thanks.
Flags: needinfo?(ralin)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5c21cf82263e
Part 1. Show insecure field in credit card autofill dropdown instead of result when the connection is not secure. r=MattN,seanlee
https://hg.mozilla.org/integration/autoland/rev/0623467f11ba
Part 2. Add a chrome browser test for form autofill insecure field. r=MattN,seanlee
Keywords: checkin-needed
Sorry for the mess, I initially thought it was a "push race condition" when the first time backed out, but now it occurred another issue that the CSS rule introduced in this bug overrides the original insecure binding of password manager. I haven't came up with a straight way to tell where the result comes from between password manager and form autofill, so we would need more details other than only "originaltype" on the <richlistitem>. I'll file another bug for the changes on m-c and make it ride on 56.
Flags: needinfo?(ralin)
I end up creating our version of insecure warning binding in our extension. The reason is I don't see too much benefits of reusing the original insecure binding, since on one hand we don't have to rewrite item styling, but on the other hand, we need to override unwanted methods and make it fit in a bunch of features that we'll never use. 

The try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=944733f193c648cf5fe75cc2b60875035b5f18f9&selectedJob=121939381
I think the tests are more reliable as our binding has nothing to do with password manager's now, nevertheless, I'll make sure all the related tests are green before marking checkin-needed. Thanks.
ni myself to review the patch again.
Flags: needinfo?(selee)
LGTM! Thanks.
Flags: needinfo?(selee)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/614a07215820
Part 1. Show insecure field in credit card autofill dropdown instead of result when the connection is not secure. r=MattN,seanlee
https://hg.mozilla.org/integration/autoland/rev/36c1b89781b4
Part 2. Add a chrome browser test for form autofill insecure field. r=MattN,seanlee
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/614a07215820
https://hg.mozilla.org/mozilla-central/rev/36c1b89781b4
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.