Closed Bug 1329628 Opened 7 years ago Closed 7 years ago

Address phishing danger concerns about [Form Autofill]

Categories

(Toolkit :: Form Manager, defect, P3)

Product:
Toolkit
Component:
Form Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: ma1, Assigned: ralin, NeedInfo)

References

(Blocks 1 open bug,
URL
)

Details

(Whiteboard: [form autofill:M3])

Attachments

(2 files, 4 obsolete files)

Bug 1329628 - Part 1. Add a phishing warning text upon footer to show the additional fields that are about to be filled while a profile being selected.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1329628 - Part 2. Add a browser chrome test for phishing warning text.
59 bytes, text/x-review-board-request
MattN
: review+
Details 
Form Autofill profiless can be abused to mount phishing attacks based on automatically filling fields which user cannot see (in analogy with Clickjacking, we may call this "Formjacking").

The "remediation" suggested at the end of https://www.bleepingcomputer.com/news/security/browser-autofill-profiles-can-be-abused-for-phishing-attacks/, i.e.
"the best solution to negate this attack vector would be to disable autofill behavior for hidden form fields", is of course naive at best: as Clickjacking teaches us, there's no shortage of clever and virtually undetectable ways to conceal page elements to a human observer through CSS.

I'm not sure about how to mitigate this feature's dangers without hampering its usability, but maybe at least a warning showing a list of all the data which is about to be automatically filled and asking for confirmation before it actually reaches the input fields is needed.
I guess that it is possible to experiment that with the Test Pilot and the Tracking protection experiment so we can evalutate what is the best solution.
In my personal opinion the hidden field not need auto formfill because they are not usually filled by a person but maybe from javascript or server side so they are only an honeypot (like for spambot) for get personal data.
From this spec: <https://www.dropbox.com/sh/g2vpw7bfegjktbo/AACI-bq0b_0slc0DMXIfbNqDa?dl=0&preview=Form+Autofill-Visual+Spec.pdf>:

Starting from slide 5 -- when a user hovers over an autofill option and the fields "preview" how they will look when filled in (note they turn blue and the text is grey): the text can't be entered in the form in the normal way (i.e., trigger JS events, be readable by JS) so that the info can't be stolen via ajax

Slide 6 -- the form gets auto-filled with the selected profile: before the text is here entered (setting off JS events and being readable by JS and being submittable by user action or scripting) the user should be presented with a pop-up lightbox or doorhanger that shows all the fields that are about to be auto-filled with the option to uncheck individual ones
Here's the UX update about phishing issue: https://mozilla.invisionapp.com/share/AP8TFZ22G#/screens/185446489

At point 3, with additional information at the bottom, user will be informed with which fields are going to be filled. Users are able to determine on whether to fill in or not according to seeing if there's any unexpected fields are about to be filled. We still in early stage of discussion about how to populate and style the result for preview in a proper way. To protect profile from leaking, we might consider something sort of anonymous content which behaves similar to placeholder but won't expose any information (not web-content observable).
Hi Selena, 
This is Juwei, UX designer from form autofill.
Would you kindly review the UX here to see if there's any other security concern? 
Thank you :)
Flags: needinfo?(sdeckelmann)
Matt -- can you help out here?
Flags: needinfo?(sdeckelmann) → needinfo?(MattN+bmo)
Hey Selena, I'm actually tech lead of the team[1] with Juwei and Ray and we wanted to get the opinion of the product security team to ensure that there is agreement that the solution in comment 3 is sufficient for our MVP. I would normally ask Tanvi or Dan about this so maybe I'll redirect to Dan?

I agree with Giorgio that a technical solution to detect only visible fields is likely impossible to get 100% correct and that would be needed if we don't require user interaction in each field (which would be poor UX). It's also expected by the HTML spec to fill type=hidden fields and some sites do rely on that behaviour (e.g. ones that implement custom form widgets and set the real value in <input type=hidden />) so rather than deal with the arms race and compatibility issues of not filling some fields we're choosing to address the issue by informing the user through text in the autocomplete popup about what other data not visible in the dropdown will be filled.

[1] https://wiki.mozilla.org/Firefox/Features/Form_Autofill#Team_Members
Flags: needinfo?(MattN+bmo) → needinfo?(dveditz)
Note that we will never fill/preview credit card data at the same time as address data. They are always filled separately so it should be clear when a credit number is getting filled by the user.

(In reply to Daniele "Mte90" Scasciafratte from comment #1)
> In my personal opinion the hidden field not need auto formfill because they
> are not usually filled by a person but maybe from javascript or server side
> so they are only an honeypot (like for spambot) for get personal data.

As you said, some type=hidden fields get filled by JavaScript to provide a value to be submitted with the form and if we didn't fill those type=hidden fields then the form validation/submission will fail. This is often used with custom form controls e.g. a custom <select>-like implementation.

(In reply to Jamie Katz from comment #2)
> From this spec:
> <https://www.dropbox.com/sh/g2vpw7bfegjktbo/AACI-
> bq0b_0slc0DMXIfbNqDa?dl=0&preview=Form+Autofill-Visual+Spec.pdf>:

This isn't loading for me but I assume it's the same as https://mozilla.invisionapp.com/share/MQ8J8K4CE#/screens/222029814 linked from our wiki page.

> Starting from slide 5 -- when a user hovers over an autofill option and the
> fields "preview" how they will look when filled in (note they turn blue and
> the text is grey): the text can't be entered in the form in the normal way
> (i.e., trigger JS events, be readable by JS) so that the info can't be
> stolen via ajax

Right, we are adding internal APIs for this and will use anonymous/shadow content like input@placeholder for this. See bug 1340488 and bug 1340483.

> Slide 6 -- the form gets auto-filled with the selected profile: before the
> text is here entered (setting off JS events and being readable by JS and
> being submittable by user action or scripting) the user should be presented
> with a pop-up lightbox or doorhanger that shows all the fields that are
> about to be auto-filled with the option to uncheck individual ones

This is now shown above the autocomplete footer in Slide 5. We didn't want to hinder the UX with a separate step.
(In reply to Juwei Huang[:juwei] from comment #5)
> Would you kindly review the UX here to see if there's any other security concern? 

My main concern is point 3 where you show "3 categories at most" for the extra fields you're going to fill in. My first concern is that users will interpret that as definitive "We will fill in just this" and not interpret it as "we will fill in this and possibly more". My second concern is that email and phone are way down your list: They would be the first ones bumped off and yet would be potentially of most concern to users and unexpected if they think it's just a name and address field (I'm assuming name, if present in the form, will always be a first or second item).

What happens if the form has a name and a country field, with hidden city? Will the second item be the street address, or the country? From the descriptions it looks like the city would not be given in the 3rd position as "also fill in" because it's lumped under "Address" and the country is already being shown.

If there's no visible street address field (it's hidden) but the drop down says "John Doe 123 Main St." it may not be at all clear to users you are filling in the full address. It will look like the address may be used to distinguish profiles. Might be better to go ahead and list Address (maybe "Complete Address"?) in the "will also fill in" section, as well -- although if you're really limited to three items then there's even more danger of pushing email and phone off the list.

That list can't get too long given the categories you have; I'd be happier if you just let it expand to two lines if it needs to.
Flags: needinfo?(dveditz)
Priority: -- → P3
Whiteboard: [form autofill] → [form autofill:M3]
Assignee: nobody → ralin
Status: NEW → ASSIGNED
just scaffolding the whole patch, still 3 parts are under construction :-)
Some updates about prevent item being clicked or pressed after discussed with Sean. 

Two directions:
1. treat phishing information as one of the result items, and hack the handlers of keypress navigation/mouse click/mouse hover. 
2. add footer container in popup as we discussed in meeting. 

I prefer second way since it highly decreases the possibility of regressing autocomplete, besides, we can have more flexibility for different result types to have their own footer(if they need...). My rough idea is to on the fly bind a custom binding to the footer when _appendCurrentResults()[0], which its binding name is acquired from the controller by like: 
```
footer.style.MozBinding = controller.getFooterStyle();
 ```
but that means we'll need a new method for controller and nsIAutoCompleteResult. 


Second approach seems simpler and more straight to me, but I'm not sure which would really bring more difficulty. Could you give me feedback about this? Thank you Matt :)


[0] https://dxr.mozilla.org/mozilla-central/rev/f81bcc23d37d7bec48f08b19a9327e93c54d37b5/toolkit/content/widgets/autocomplete.xml#1310
Flags: needinfo?(MattN+bmo)
Hi Ray,

I think we should use the approach like the insecure field warning for the footer row since keyboard access makes sense for it:
* https://dxr.mozilla.org/mozilla-central/rev/39d5cc0fda5e16c49a59d29d4ca186a5534cc88b/toolkit/components/passwordmgr/LoginManagerContent.jsm#1439,1442,1482-1484,1527-1529
* https://dxr.mozilla.org/mozilla-central/rev/39d5cc0fda5e16c49a59d29d4ca186a5534cc88b/browser/base/content/browser.css#571-591

That would be done mostly in ProfileAutoCompleteResult for bug 1300995.

Then the warning could be implemented as:
a) part of custom binding content attached to the footer "style" with CSS, or
b) A ::before or ::after pseudo-element
Flags: needinfo?(MattN+bmo)
Depends on: 1300995
Following up now though I think your comments were taken into account just that nobody replied to acknowledge.

(In reply to Daniel Veditz [:dveditz] from comment #9)
> (In reply to Juwei Huang[:juwei] from comment #5)
> > Would you kindly review the UX here to see if there's any other security concern? 
> 
> My main concern is point 3 where you show "3 categories at most" for the
> extra fields you're going to fill in. My first concern is that users will
> interpret that as definitive "We will fill in just this" and not interpret
> it as "we will fill in this and possibly more". My second concern is that
> email and phone are way down your list: They would be the first ones bumped
> off and yet would be potentially of most concern to users and unexpected if
> they think it's just a name and address field (I'm assuming name, if present
> in the form, will always be a first or second item).

The spec has since changed to say 4 instead of 3 so I could be wrong about this but I believe that was just stating the maximum number of groups and there wasn't an intention to ever exclude any of the types we will fill. The intention was to always be exhaustive.

> What happens if the form has a name and a country field, with hidden city?
> Will the second item be the street address, or the country? From the
> descriptions it looks like the city would not be given in the 3rd position
> as "also fill in" because it's lumped under "Address" and the country is
> already being shown.

It all depends on which field is focused:
* When the name field is focused: We can/should show the city as the 2nd item in this case as it's the next highest priority field in the form.
* When the country field is focused: We would show the Name as the 2nd item. The spec isn't totally clear if we would also include "address" in the yellow phishing line but I confirmed that it is the intention now.

> If there's no visible street address field (it's hidden) but the drop down
> says "John Doe 123 Main St." it may not be at all clear to users you are
> filling in the full address. It will look like the address may be used to
> distinguish profiles. Might be better to go ahead and list Address (maybe
> "Complete Address"?) in the "will also fill in" section, as well -- although
> if you're really limited to three items then there's even more danger of
> pushing email and phone off the list.

This is now the plan (see above) and we will never leave off any category in item 3 that will be filled (we just won't repeat item 1).

> That list can't get too long given the categories you have; I'd be happier
> if you just let it expand to two lines if it needs to.

Yep, that's fine with me too.

Does this current spec address your concerns?
Flags: needinfo?(dveditz)
Hi :flod,

Nice to meet you over bugzilla. I got a l10n question about phishing warning text need your suggestion.
Please see the third point in UX spec: https://mozilla.invisionapp.com/share/AP8TFZ22G#/screens/185446489

The phishing warning text varies according to the result and the rest of fields on page. With the 5 categories, it brings 4 combinations of string format (at least 1 category, at most 4 categories):
1. Also fill %s
2. Also fill %s and %s 
3. Also fill %s, %s and %s
4. Also fill %s, %s, %s and %s

and 5 locale entries:
- Address = address
- Name = name
- Company = company
- Phone = phone
- Email = email

For example, if the given categories are "Name", "Company" and "Email", my rough idea is to:
1. choose format 3
2. substitute the %s with the translation: Also fill %s, %s and %s ---> Also fill [Name], [Company] and [Email] ---> Also fill name, company and email. 

I suspect that whether this way could work for all languages? Is there any case might change the conjunction if different following/preceding word is given? i.e. Company "ande" Email, the conjunction "and" changes to "ande" if the preceding word is Company. 
I didn't find many precedents doing this kind of dynamically translating, so I might not consider all the aspects. Could you give me some feedback about this? Thank you.
Flags: needinfo?(francesco.lodolo)
Sadly, building lists dynamically is not something that works great for localization.

Example with Italian, which is a relatively simple language: conjunction is "e" (and), but it becomes "ed" if it clashes with another "e" ("e indirizzo" vs "ed email"). That rule is called "euphonic d", and there's no way to apply it dynamically. You have similar problems if you add an article before the noun.

Out of curiosity, do we control the categories? As in, we only have 5 because we decided that?

Even if the result won't sound as natural, one solution would be to give up the "and", and do something similar to this:

message = Also fill: %S
separator = ,\u0020

Which would result in "Also fill: address, name", etc.
Flags: needinfo?(francesco.lodolo)
(In reply to Francesco Lodolo [:flod] from comment #20)
> Sadly, building lists dynamically is not something that works great for
> localization.
> 
> Example with Italian, which is a relatively simple language: conjunction is
> "e" (and), but it becomes "ed" if it clashes with another "e" ("e indirizzo"
> vs "ed email"). That rule is called "euphonic d", and there's no way to
> apply it dynamically. You have similar problems if you add an article before
> the noun.
Learn that, glad to know in advance. Thanks. 
> 
> Out of curiosity, do we control the categories? As in, we only have 5
> because we decided that?
yep, only 5 categories

> 
> Even if the result won't sound as natural, one solution would be to give up
> the "and", and do something similar to this:
> 
> message = Also fill: %S
> separator = ,\u0020
> 
> Which would result in "Also fill: address, name", etc.
Thanks for the example, it seems fine to me as that's what shown on the spec.

I wonder if we could mix two approaches and let localizers decide. We keep "and" in the string, but add a NOTICE comment above to tell user an alternative(using separator ",") if their language has some rules for conjunction like in Italian?
correction: s/user/the localizers
(In reply to Ray Lin[:ralin] from comment #21)
> I wonder if we could mix two approaches and let localizers decide. We keep
> "and" in the string, but add a NOTICE comment above to tell user an
> alternative(using separator ",") if their language has some rules for
> conjunction like in Italian?

I don't see how we could from a practical point of view. Localizers don't pick their strings, they're exposed to all strings available in English.

We need to chose a solution that works for all locales, not provide alternatives, because that comes with a high risk of failing, based on experience.
(In reply to Francesco Lodolo [:flod] from comment #23)
> (In reply to Ray Lin[:ralin] from comment #21)
> > I wonder if we could mix two approaches and let localizers decide. We keep
> > "and" in the string, but add a NOTICE comment above to tell user an
> > alternative(using separator ",") if their language has some rules for
> > conjunction like in Italian?
> 
> I don't see how we could from a practical point of view. Localizers don't
> pick their strings, they're exposed to all strings available in English.

I don't understand what you're saying above.

> We need to chose a solution that works for all locales, not provide
> alternatives, because that comes with a high risk of failing, based on
> experience.

To double-check, are you saying that we couldn't use an English string like below when showing 4 values:

> # LOCALIZATION NOTE (alsoFill4): %s will be substituted with translations of some of: address, name, company, phone, email.
> #                                Simply use a list if "and" cannot be translated appropriately with all possible substitutions.
> alsoFill4 = Also fill %s, %s, %s, and %s

(likewise for 1-3)?
Flags: needinfo?(francesco.lodolo)
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #24)
> (In reply to Francesco Lodolo [:flod] from comment #23)
> > (In reply to Ray Lin[:ralin] from comment #21)
> > > I wonder if we could mix two approaches and let localizers decide. We keep
> > > "and" in the string, but add a NOTICE comment above to tell user an
> > > alternative(using separator ",") if their language has some rules for
> > > conjunction like in Italian?
> > 
> > I don't see how we could from a practical point of view. Localizers don't
> > pick their strings, they're exposed to all strings available in English.
> 
> I don't understand what you're saying above.

I read "mix two approaches and let localizers decide" as in having both approaches (mine and yours), that's an answer to that scenario. 

On the other hand what you're suggesting is to keep the strings you proposed in the first place and add localization comments to work around them.

As explained, the conjunction is only one part of the problem: in the middle of a sentence I might want to use articles, and that's not possible if nouns change dynamically. In a list that should be easier to avoid. We could suggested to do that in the localization comment.

So:
1. Also fill %s
2. Also fill %s and %s 
3. Also fill %s, %s and %s
4. Also fill %s, %s, %s and %s

Could become in a localization
1. Also fill %s
2. Also fill %s, %s 
3. Also fill %s, %s, %s
4. Also fill %s, %s, %s, %s

Or 
1. Also fill: %s
2. Also fill: %s, %s 
3. Also fill: %s, %s, %s
4. Also fill: %s, %s, %s, %s

I hope you agree that, from a localizer's point of view, it's just a waste of time. Also note that this approach doesn't scale if we decide to support new fields.

> To double-check, are you saying that we couldn't use an English string like
> below when showing 4 values:
> 
> > # LOCALIZATION NOTE (alsoFill4): %s will be substituted with translations of some of: address, name, company, phone, email.
> > #                                Simply use a list if "and" cannot be translated appropriately with all possible substitutions.
> > alsoFill4 = Also fill %s, %s, %s, and %s

We can and at this point it's your decision to make, I just don't think it's a good approach for the reasons already explained.
Flags: needinfo?(francesco.lodolo)
Attachment #8870353 - Attachment is obsolete: true
Attachment #8870354 - Attachment is obsolete: true
Attachment #8870355 - Attachment is obsolete: true
Attachment #8870356 - Attachment is obsolete: true
Comment on attachment 8879498 [details]
Bug 1329628 - Part 2. Add a browser chrome test for phishing warning text.

https://reviewboard.mozilla.org/r/150810/#review157024

::: browser/extensions/formautofill/test/browser/head.js:59
(Diff revision 3)
> +  await sleep(2000);
> +  await BrowserTestUtils.synthesizeKey("VK_DOWN", {}, browser);

We really shouldn't keep adding more uses of setTimeout (aka. sleep). We should use the approach of sending an event after identifying is done, possibly with an executeSoon after.
Comment on attachment 8845205 [details]
Bug 1329628 - Part 1. Add a phishing warning text upon footer to show the additional fields that are about to be filled while a profile being selected.

https://reviewboard.mozilla.org/r/118398/#review157026

I review the whole patch but here are some initial issues

::: browser/extensions/formautofill/content/formautofill.xml:181
(Diff revision 6)
> +                                                                      "phishingWarningMessage" :
> +                                                                      "phishingWarningMessage2");
> +            let sep = this._stringBundle.GetStringFromName("separator");
> +            let categoriesText = categories.map(this._stringBundle.GetStringFromName).join(sep);
> +
> +            this._warningTextBox.textContent = warningTextTmpl.replace("%S", categoriesText);

You should use `formatStringFromName` instead of doing the replacement youself

::: browser/extensions/formautofill/content/formautofill.xml:248
(Diff revision 6)
> -            footerTextBundleKey += "Short";
> +            buttonTextBundleKey += "Short";
>            }
> -          let footerText = this._stringBundle.GetStringFromName(footerTextBundleKey);
> -          this._itemBox.textContent = footerText;
> +          let buttonText = this._stringBundle.GetStringFromName(buttonTextBundleKey);
> +          this._optionButton.textContent = buttonText;
> +
> +          messageManager.addMessageListener("FormAutofill:ShowWarningMessage", this._updateWarningText);

It would perhaps be easier if we had one listener for the popup instead of putting it in the list item? I guess we still haven't forked the popup binding yet though…

::: browser/extensions/formautofill/locale/en-US/formautofill.properties:15
(Diff revision 6)
> +address = address
> +name = name
> +organization = company
> +tel = phone
> +email = email
> +phishingWarningMessage = Also fill %S
> +phishingWarningMessage2 = Fill %S
> +separator = ,\u0020

I think you should have some localization notes to explain:
1) How the data type strings are used
2) What %S is in the two strings
3) What the separator is for

::: browser/extensions/formautofill/locale/en-US/formautofill.properties:20
(Diff revision 6)
> +phishingWarningMessage = Also fill %S
> +phishingWarningMessage2 = Fill %S

There were some concerns in today's security review that these strings could be interpretted as an instruction to the user that they should also fill the listed fields, not that Firefox will fill it. I'm not sure if this string was part of the string review but I tend to agree with them though I don't think we want this too long either…

Firefox will also fill…

::: browser/extensions/formautofill/locale/en-US/formautofill.properties:22
(Diff revision 6)
> +organization = company
> +tel = phone
> +email = email
> +phishingWarningMessage = Also fill %S
> +phishingWarningMessage2 = Fill %S
> +separator = ,\u0020

Nit: fieldNameSeparator would be more clear

::: toolkit/content/widgets/autocomplete.xml:1413
(Diff revision 6)
>              }
>  
>              this._currentIndex++;
>            }
>  
> +          for (let i = matchCount; i <  existingItemsCount; i++) {

Nit: extra space after `<`

::: toolkit/content/widgets/autocomplete.xml:1416
(Diff revision 6)
> +            if (typeof item._onHidden == "function") {
> +              item._onHidden();
> +            }

Just looking at this code it feels a bit unusual to call an `_onHidden` method in the middle of an `_appendCurrentResult` method.
Attachment #8845205 - Flags: review?(MattN+bmo)
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #34)
> Comment on attachment 8879498 [details]
> Bug 1329628 - Part 2. Add a browser chrome test for phishing warning text.
> 
> https://reviewboard.mozilla.org/r/150810/#review157024
> 
> ::: browser/extensions/formautofill/test/browser/head.js:59
> (Diff revision 3)
> > +  await sleep(2000);
> > +  await BrowserTestUtils.synthesizeKey("VK_DOWN", {}, browser);
> 
> We really shouldn't keep adding more uses of setTimeout (aka. sleep). We
> should use the approach of sending an event after identifying is done,
> possibly with an executeSoon after.

I agree. I attempted to eliminate existing setTimeout workaround in bug 1373130, and I should have tried but I didn't use the combination with executeSoon after receiving message. I'll experiment on this approach to see if it's reliable enough.
Comment on attachment 8845205 [details]
Bug 1329628 - Part 1. Add a phishing warning text upon footer to show the additional fields that are about to be filled while a profile being selected.

https://reviewboard.mozilla.org/r/118398/#review157026

Thank you for the review :D, I'll fix it soon.

> It would perhaps be easier if we had one listener for the popup instead of putting it in the list item? I guess we still haven't forked the popup binding yet though…

no, we've considered doing that at very beginning, but had some concerns about how to make it work with existing AutoCompletePopup since currently browser has only one reference to "popup".
With the situation we still sharing the same popup, I think the problem is that we don't have a good point to add/remove our listener so it doesn't has too much different either way.

> There were some concerns in today's security review that these strings could be interpretted as an instruction to the user that they should also fill the listed fields, not that Firefox will fill it. I'm not sure if this string was part of the string review but I tend to agree with them though I don't think we want this too long either…
> 
> Firefox will also fill…

"Firefox will also fill…" looks more clear to me too. If the height is dynamic, I think a bit longer wouldn't be a problem. I can build a version with this string and let :juwei determine if it's fine.

> Just looking at this code it feels a bit unusual to call an `_onHidden` method in the middle of an `_appendCurrentResult` method.

I guess invalidate may be a better place.
Although we do adjust the height dynamically, I still want the string to be short and straigh forward. From ux point of view, once users click on the suggestion to fill the form, I guess they would pretty sure they don't have to fill the fields again:)
 
The string has been reviewed by Michelle already but I can needinfo her again to double check: https://mozilla.invisionapp.com/share/AP8TFZ22G#/185446489_1-6_Autofill
Flags: needinfo?(mheubusch)
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #18)
> Does this current spec address your concerns?

yes
Flags: needinfo?(dveditz)
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #34)
> Comment on attachment 8879498 [details]
> Bug 1329628 - Part 2. Add a browser chrome test for phishing warning text.
> 
> https://reviewboard.mozilla.org/r/150810/#review157024
> 
> ::: browser/extensions/formautofill/test/browser/head.js:59
> (Diff revision 3)
> > +  await sleep(2000);
> > +  await BrowserTestUtils.synthesizeKey("VK_DOWN", {}, browser);
> 
> We really shouldn't keep adding more uses of setTimeout (aka. sleep). We
> should use the approach of sending an event after identifying is done,
> possibly with an executeSoon after.


I've tried to use sending an event with executeSoon, however, the result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=92c36809e6c4defdd2f214e86c1a87d3654614a6 looks like it's still not enough to ensure identifying is done. In this patch, no new setTimeout was added instead it just refactored to a more concise helper function: `openPopupOn`, so maybe this is fine as no more harm to our test, but it's should eventually replaced with a better approach.
Comment on attachment 8845205 [details]
Bug 1329628 - Part 1. Add a phishing warning text upon footer to show the additional fields that are about to be filled while a profile being selected.

https://reviewboard.mozilla.org/r/118398/#review158980

::: browser/extensions/formautofill/FormAutofillUtils.jsm:55
(Diff revision 7)
> +  },
> +
>    getCategoriesFromFieldNames(fieldNames) {
>      let categories = new Set();
>      for (let fieldName of fieldNames) {
>        let info = this._fieldNameInfo[fieldName];

You could re-use `getCategoryFromFieldName` here if you want

::: browser/extensions/formautofill/content/formautofill.xml:116
(Diff revision 7)
> +      <method name="_cleanup">
> +        <body>
> +        <![CDATA[

You were saying you think this should be reverted and put back.

::: browser/extensions/formautofill/content/formautofill.xml:174
(Diff revision 7)
> +            this, "anonid", "autofill-warning"
> +          );
> +
> +          this.allFieldCategories = JSON.parse(this.getAttribute("ac-value")).categories;
> +
> +          this._updateText = (categories = this.allFieldCategories, hasExtraCategories = true) => {

I think it would be useful to have a JSDoc comment here explaining the different results.

::: browser/extensions/formautofill/content/formautofill.xml:184
(Diff revision 7)
> +          this._updateWarningText = ({target, data: {focusedCategory, categories}} = {data: {}}) => {
> +            let hasSelectedAddress = focusedCategory && categories;
> +            // If the length of categories is 1, that means all the fillable fields are in the same
> +            // category. We will change the way to inform user according to this flag.
> +            let hasExtraCategories = hasSelectedAddress && categories.length > 1;

Maybe add a longer comment either as JSDoc or with this to have a numbered list of the three states: selected, none selected, and when the selected category is the same as the only all category.

::: browser/extensions/formautofill/content/formautofill.xml:215
(Diff revision 7)
> +      <method name="_cleanup">
> +        <body>
> +        <![CDATA[
> +          /* global messageManager */
> +
> +          messageManager.removeMessageListener("FormAutofill:ShowWarningMessage", this._updateWarningText);
> +          this.removeAttribute("formautofillattached");
> +          if (this._itemBox) {
> +            this._itemBox.removeAttribute("size");
> +          }
> +        ]]>
> +        </body>
> +      </method>
> +

You told me IRL that we don't need this anymore.

::: browser/extensions/formautofill/content/formautofill.xml:247
(Diff revision 7)
> -            footerTextBundleKey += "Short";
> +            buttonTextBundleKey += "Short";
>            }
> -          let footerText = this._stringBundle.GetStringFromName(footerTextBundleKey);
> -          this._itemBox.textContent = footerText;
> +          let buttonText = this._stringBundle.GetStringFromName(buttonTextBundleKey);
> +          this._optionButton.textContent = buttonText;
> +
> +          messageManager.addMessageListener("FormAutofill:ShowWarningMessage", this._updateWarningText);

Maybe the message should be "…:UpdateWarningMessage" since the warning message is technically already shown and this is about giving the updated categories to show

::: browser/extensions/formautofill/locale/en-US/formautofill.properties:26
(Diff revision 7)
> +fieldNameSeparator = ,\u0020
> +# LOCALIZATION NOTE (phishingWarningMessage, phishingWarningMessage2): The warning
> +# 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

Update this to the latest string

::: browser/extensions/formautofill/skin/shared/autocomplete-item.css:13
(Diff revision 7)
>  
>  xul|richlistitem[originaltype="autofill-profile"][selected="true"] > .autofill-item-box {
>    background-color: #F2F2F2;
>  }
>  
> -xul|richlistitem[originaltype="autofill-footer"][selected="true"] > .autofill-footer {
> +xul|richlistitem[originaltype="autofill-footer"][selected="true"] .autofill-option-button {

Nit: We generally try to avoid the descendant selector since it's much less performant than child selectors. It will make this quite long but don't worry about wrapping it.

::: browser/extensions/formautofill/skin/shared/autocomplete-item.css:100
(Diff revision 7)
> +
> +.autofill-footer > .autofill-warning {
> +  padding: 2.5px 0;
> +  color: #989898;
> +  text-align: center;
> +  background-color: rgba(248, 232, 28, .2);

Nit: remove ths spaces inside the rgba
Attachment #8845205 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8879498 [details]
Bug 1329628 - Part 2. Add a browser chrome test for phishing warning text.

https://reviewboard.mozilla.org/r/150810/#review158988
Attachment #8879498 - Flags: review?(MattN+bmo) → review+
seems there are 3 open issues in part 1 that need to be fixed first before we can push this with autoland
Flags: needinfo?(ralin)
Keywords: checkin-needed
Comment on attachment 8845205 [details]
Bug 1329628 - Part 1. Add a phishing warning text upon footer to show the additional fields that are about to be filled while a profile being selected.

https://reviewboard.mozilla.org/r/118398/#review158980

> Update this to the latest string

I guess you're referring line 26 as that's a new string.
Sorry :Tomcat

Forgot to update & publish mozreview. The issues are closed now. Thanks
Flags: needinfo?(ralin)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05ccce453664
Part 1. Add a phishing warning text upon footer to show the additional fields that are about to be filled while a profile being selected. r=MattN
https://hg.mozilla.org/integration/autoland/rev/9308293036bb
Part 2. Add a browser chrome test for phishing warning text. r=MattN
Keywords: checkin-needed
As per San-Francisco meet up, ni-ing Vance to link us to the phishing specs.
Flags: needinfo?(vchen)
Blocks: 1378072
https://hg.mozilla.org/mozilla-central/rev/05ccce453664
https://hg.mozilla.org/mozilla-central/rev/9308293036bb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1380910
Depends on: 1385785
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: