1378770 - "Sync" section on onboarding tour does not validate email. Invalid emails interact poorly with Firefox Accounts.

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Shane Tomlinson [:stomlinson]]

STR

1. Open a new tab, click on the Firefox head to open the new onboarding tour.
2. Open the "Sync" section of the onboarding tour.
3. Enter an invalid email address, e.g., "shane", click "Next"
4. When Firefox Accounts opens, "Invalid parameter: email" is displayed.

Expected

Email should be recognized as invalid and user is unable to click "Next" in step 3.

Actual

User is able to click "Next", which causes Firefox Accounts to display an error.

Comment 1

[[e-mail]]

Attachment: no way back to account login page

When link with invalid email , it redirect to an about:accounts error page
about:accounts?action=signup&entrypoint=uitour&email=asdsdf

I think if user can click a link and go back to the account login page, it will help user continue the following process.


:rfkelly how do you think?

Comment 2

[Shane Tomlinson [:stomlinson]]

(In reply to Fred Lin [:gasolin] from comment #1)
> Created attachment 8884770 [details]
> no way back to account login page
> 
> When link with invalid email , it redirect to an about:accounts error page
> about:accounts?action=signup&entrypoint=uitour&email=asdsdf
> 
> I think if user can click a link and go back to the account login page, it
> will help user continue the following process.


I am against this approach primarily for UX reasons, but I also have security
concerns.

UX - We already have one scenario in which user input and the ability 
to correct the input are decoupled in time. On FxA, it's possible for a user
to enter an incorrect password, and later an unblock code. Only after the
user enters the unblock code is the password checked, at which point the
user is asked to fix their password. This is confusing. We receive reports 
about it nearly every week.

The earlier user input is validated and corrected, the better, preferably
at point of entry. Having the user enter an invalid email address on one form 
and correct it on another is going to cause confusion.

Security - Email addresses are written to the DOM. Relaxed validation rules 
could in theory lead to an XSS vector. We are strict in what we write to the 
DOM, but defense in depth.

We prohibit OAuth reliers from specifying invalid email addresses. We did so to help manage expectations and to enforce good UX.

Comment 3

[Ryan Feeley [:rfeeley]]

Onboarding tour should ensure that only valid-looking emails are being handed off to us. Shane, can you share was we consider to be valid-looking so that our validation is aligned?

Comment 4

[KM Lee [:rexboy] (inactivate after Sep. 22)]

I'd propose just change the input type to "email" and check
the element's validity.valid to determine if the submit
button is clickable.
Looks like about:account forum verifies its form the same way.
I'm just not sure if there are additional limitation applied
on FxA's server side check.

Comment 5

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Just made some test, there’re still some difference between FxA’s 
checker and html email input; like user@user passes the validation
of email input element but fail on FxA’s checker.
If we want to 100% avoid these situations from onboarding side we'll
still need FxA team's input.

Comment 6

[Shane Tomlinson [:stomlinson]]

(In reply to KM Lee [:rexboy] from comment #5)
> Just made some test, there’re still some difference between FxA’s 
> checker and html email input; like user@user passes the validation
> of email input element but fail on FxA’s checker.
> If we want to 100% avoid these situations from onboarding side we'll
> still need FxA team's input.

Thanks for checking :rexboy, even validating at that level should significantly
reduce confusion.

We are slightly more strict than the HTML5 spec for email validation because
username@one_part_tld is for all intents and purposes an 
unroutable email address. Background FxA issues: [1][2]

We validate emails against IETF rfc5321 [3], with the modification
that emails must contain at least a two part TLD.

The validation regexp we use is [4]:

/^[\w.!#$%&’*+\/=?^`{|}~-]{1,64}@[a-z\d](?:[a-z\d-]{0,253}[a-z\d])?(?:\.[a-z\d](?:[a-z\d-]{0,253}[a-z\d])?)+$/i

[1] - https://github.com/mozilla/fxa-content-server/issues/362
[2] - https://github.com/mozilla/fxa-content-server/issues/2199
[3] - http://tools.ietf.org/html/rfc5321#section-4.5.3.1.1
[4] - https://github.com/mozilla/fxa-content-server/blob/359e45fc8661e8ca3bd6098bea46c3db32449126/app/scripts/lib/validate.js#L41

Comment 7

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Thanks a lot for the reference :stomlinson.
It should be easy to apply the same regex to the form of onboarding.
Let me try to come up a patch then.

Comment 8

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Bug 1378770 - [Onboarding] Sync tour should proceed with only valid Email.

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

Comment 9

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: checker

Made a screenshot for how the validation works.

The popup message came from default email input element so we don't need to do l10n works.

Comment 10

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Validation with the patch

sorry, the screenshot video should be this one.

Comment 11

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Mossop may you take a look on this quick fix?
Due to we're having the same regular expression, the "bad request" screen shouldn't show again.

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 8885638 [details]
Bug 1378770 - [Onboarding] Sync tour should proceed with only valid Email.

https://reviewboard.mozilla.org/r/156492/#review161652

Should we be disabling the button if the input doesn't match the pattern?

Comment 13

[Justin [:JW_SoftvisionQA]]

As QA of this feature I think this should be prioritized higher than a P3 as this will effect the flow of the overall feature.

Comment 14

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8885638 [details]
Bug 1378770 - [Onboarding] Sync tour should proceed with only valid Email.

https://reviewboard.mozilla.org/r/156492/#review161652

I tried that; but seems the validation check is performed as the input losts focus or the button being clicked. If we disabled the button then users don't get the message of incorrect pattern if they don't click anywhere else.

Comment 15

[KM Lee [:rexboy] (inactivate after Sep. 22)]

clearing the ni? since we've converged the solution.

Comment 16

[KM Lee [:rexboy] (inactivate after Sep. 22)]

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17425c055ef8
[Onboarding] Sync tour should proceed with only valid Email. r=mossop

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/17425c055ef8

Comment 19

[Md.Tarikul Islam Oashi]

I have reproduced this bug with nightly 56.0a1 (2017-07-06) on "Linux Mint (64 Bit).

The bug's fix is now verified on Latest Nightly 56.0a1

Build ID 	20170714100217
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0

[Bugday-20170712]