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

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P3)

Description

[Luke Chang [:lchang] (inactive)]

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.

Comment 2

[Matthew N. [:MattN]]

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.

Comment 3

[Ray Lin[:ralin]]

Attachment: Bug 1371149 - Part 1. Show insecure field in credit card autofill dropdown instead of result when the connection is not secure.

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

Comment 4

[Ray Lin[: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.

Comment 5

[Matthew N. [:MattN]]

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

Comment 6

[Matthew N. [:MattN]]

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

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

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.

Comment 8

[Ray Lin[:ralin]]

Attachment: Bug 1371149 - Part 2. Add a chrome browser test for form autofill insecure field.

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

Comment 9

[Ray Lin[:ralin]]

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.

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

Comment 10

[Ray Lin[:ralin]]

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 11

[Ray Lin[:ralin]]

Comment on attachment 8891674 [details]
Bug 1371149 - Part 2. Add a chrome browser test for form autofill insecure field.

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

Comment 12

[Ray Lin[:ralin]]

Comment on attachment 8891674 [details]
Bug 1371149 - Part 2. Add a chrome browser test for form autofill insecure field.

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

Comment 13

[Ray Lin[:ralin]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=852da72407a826dc7168f7e93ad04f576fddf9b6

Comment 14

[Matthew N. [:MattN]]

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 15

[Ray Lin[:ralin]]

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.

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

Comment 16

[Ray Lin[:ralin]]

Comment on attachment 8891674 [details]
Bug 1371149 - Part 2. Add a chrome browser test for form autofill insecure field.

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

Comment 17

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

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?

Comment 18

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

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?

Comment 19

[Ray Lin[:ralin]]

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.

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

Comment 20

[Ray Lin[:ralin]]

Comment on attachment 8891674 [details]
Bug 1371149 - Part 2. Add a chrome browser test for form autofill insecure field.

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

Comment 21

[Ray Lin[:ralin]]

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 22

[Matthew N. [:MattN]]

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.

Comment 23

[Matthew N. [:MattN]]

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!

Comment 24

[Ray Lin[:ralin]]

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.

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

Comment 25

[Ray Lin[:ralin]]

Comment on attachment 8891674 [details]
Bug 1371149 - Part 2. Add a chrome browser test for form autofill insecure field.

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

Comment 26

[Ray Lin[:ralin]]

Thanks :D

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=808daca3025c004b33a59b65f64a578ba2799e0b

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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

Comment 28

[Carsten Book [:Tomcat]]

backed out for test failurs like https://treeherder.mozilla.org/logviewer.html#?job_id=121671031&repo=autoland

Comment 29

[Pulsebot]

Backout by [e-mail]:
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

Comment 30

[Ray Lin[:ralin]]

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 31

[Ray Lin[:ralin]]

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.

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

Comment 32

[Ray Lin[:ralin]]

Comment on attachment 8891674 [details]
Bug 1371149 - Part 2. Add a chrome browser test for form autofill insecure field.

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

Comment 33

[Ray Lin[:ralin]]

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

Thanks.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
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

Comment 35

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

Backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=121720976&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/0c8a661bac10

Comment 36

[Ray Lin[:ralin]]

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.

Comment 37

[Ray Lin[:ralin]]

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.

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

Comment 38

[Ray Lin[:ralin]]

Comment on attachment 8891674 [details]
Bug 1371149 - Part 2. Add a chrome browser test for form autofill insecure field.

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

Comment 39

[Ray Lin[: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.

Comment 40

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

ni myself to review the patch again.

Comment 41

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

LGTM! Thanks.

Comment 42

[Ray Lin[:ralin]]

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.

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

Comment 43

[Ray Lin[:ralin]]

Comment on attachment 8891674 [details]
Bug 1371149 - Part 2. Add a chrome browser test for form autofill insecure field.

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

Comment 44

[Ray Lin[:ralin]]

Thank you Sean!!

Rebased and the try result looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=daf8ca4ce0c0c5b06cfd7e398148d14aa4d7875c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable

Comment 45

[Pulsebot]

Pushed by [e-mail]:
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

Comment 46

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

https://hg.mozilla.org/mozilla-central/rev/614a07215820
https://hg.mozilla.org/mozilla-central/rev/36c1b89781b4

Comment 47

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-beta/rev/41d5c3ecf24e
https://hg.mozilla.org/releases/mozilla-beta/rev/6390a4954deb