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

RESOLVED FIXED in Firefox 56

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lchang, Assigned: ralin)

Tracking

(Blocks 2 bugs)

unspecified
mozilla57
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox56 fixed, firefox57 fixed)

Details

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

Attachments

(2 attachments)

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
Last Resolved: 2 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 → ---
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
Assignee

Updated

2 years ago
Assignee: nobody → ralin
Assignee

Comment 4

2 years ago
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.
Assignee

Updated

2 years ago
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 7

2 years ago
mozreview-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/#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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 10

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

2 years ago
mozreview-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

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 18

2 years ago
mozreview-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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 21

2 years ago
mozreview-review-reply
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Status: NEW → ASSIGNED

Comment 27

2 years ago
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)

Comment 29

2 years ago
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
Assignee

Comment 30

2 years ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 33

2 years ago
Test is passed now, try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74430c48ccf1ee2a1df34d31d6a4e7bb665b57dc

Thanks.
Flags: needinfo?(ralin)
Keywords: checkin-needed

Comment 34

2 years ago
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
Assignee

Comment 36

2 years ago
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 39

2 years ago
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 45

2 years ago
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

Comment 46

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/614a07215820
https://hg.mozilla.org/mozilla-central/rev/36c1b89781b4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.