1091310 - [Email] Facebook html link/button does not function in email app

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[Jeremiah Lee]

Attachment: facebookbutton_logcat.txt

Description:
When attempting to tap on Facebook confirm link/button in email app it does not function.
   
Repro Steps:
1) Update a Flame device to BuildID: 20141029001202
2) Create a new email account (or use an account that has not signed up for Facebook previously. This step does not need to be done on device)
3) Create new facebook account (This step does not need to be on device)
4) On Homescreen on device, tap on email app
5) Tap on upper left corner to bring upo menu
6) Tap on Settings button
7) Sign in to the email account used to sign up for Facebook
8) In inbox, tap on Facebook confirm email
9) In Facebook email, tap on 'show external images'
10) Attempt to tap on 'Confirm Your Account' button/link
11) Observe
  
Actual:
In email app, Facebook confirm button/link cannot be tapped/does not function.
  
Expected: 
Facebook confirm button in email can be tapped and will bring up screen to open webpage.
  
Environmental Variables:
Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141029001202
Gaia: eb0aab0f13c78c7ac378ad860e865c4b6eaf669f
Gecko: 318019f80a8e
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
  
Notes: Does function in computer web browser
  
Repro frequency: 100%
See attached: video clip (http://youtu.be/9FGQgY9uazc), logcat (facebookbutton_logcat.txt)

Comment 1

[Jeremiah Lee]

ssue is also seen in 2.2.

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141029040208
Gaia: 35e87ac4324f0f3abd93dcc70d61c9f37256a0f5
Gecko: 7e3c85754d32
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 36.0a1 (2.2 Master)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0



Issue is also seen in 2.0.

Device: Flame 2.0 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141029000205
Gaia: 9f5b6f025e528fabfcc068782cb9b492cb51a7f9
Gecko: de8cfd54bf93
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 32.0 (2.0)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

If you could provide a copy of the email that would greatly assist in analysis; since the Facebook account is private-ish (even if it's just for testing), emailing it to [e-mail] and [e-mail] is perhaps best.  See https://wiki.mozilla.org/Gaia/Email/ProvidingEmailsForDebugging for instructions.

Comment 3

[Jeremiah Lee]

Email with attached file has been sent to both email addresses.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Thanks for sending the email!

The basic problem is that the HTML looks like this:
  <a href="confirm-url"><table>
    ...
      <a href="confirm-url"><center><font><span>CONFIRM TEXT</span></center></a>
    ...
  </table></a>

bleach.js tries to apply (its interpretation of) HTML parsing logic and immediately closes the "a" tags.  It thinks "table" and "center" tags are both block tags and "a" is an inline tag.  It sees that the inline tag "a" is on the stack, so it auto-closes the "a" and pops it off.

All the parsing stuff is based on HTML 4.01, which might have had some inconsistencies with reality.  The WHATWG HTML5 spec knows all; https://html.spec.whatwg.org/multipage/syntax.html#parsing-main-inhtml describes the exact correct behaviour.

Of course, since our goal is just to be a whitelist based sanitizer and we are simple and stateless, arguably much of the auto/self-closing doesn't matter to us, so we can probably get away with disabling the logic in question.

A bad situation would be if there was a tag <risky> that we wanted to allow inside an enclosing <safe> but not inside an <evil>, and our ignorance of the <safe>/<evil> allowed us to get confused about what was actually on the stack.  However, we don't currently care about that.  We would largely be okay with a stream parser.  The only case we currently care about state is for snippet generation, and in that case we could arguably just blame the corrupt message (as long as we still honor the standard self-closing tags.)  It's possible new stuff like video/audio tags could change things, but we would have to want to whitelist them before that became an issue.

:jrburke, if you want to grab this, that might be optimal.  If you want super-real testing, you'll need to delete the first blank line from the test email, then you can use something like https://addons.mozilla.org/en-US/thunderbird/addon/importexporttools/ to import the mail into an IMAP account via "tools... importexporttools... import messages"

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Another possibility short of wholesale disabling is to just make sure that we don't apply any closing rules to 'a'.  This might be appropriate for a targeted uplift fix, for one.

Comment 6

[KTucker [:KTucker][Inactive 3/4/2016]]

[Blocking Requested - why for this release]:

Nominating this 2.0? since the user cannot confirm their Facebook account on the phone. This could affect a large number of end users.

Comment 7

[James Burke [:jrburke]]

Attachment: Bleach pull request

The change to bleach for this. I still need to wind this change through gelam and gaia, and then confirm the example message displays correctly, but I believe this is the fix for it, and it is very targeted and small.

Comment 8

[James Burke [:jrburke]]

Tried the fix in local gaia on the test message: it works, but it works best when images are loaded for the message. 

Without the images loaded, the link can be activated, but the activating area is a bit higher up in the message instead of being squarely over the "Confirm your account" blue area. Tapping on the user's email address for example is more squarely in the hit target. 

Looking at the DOM, it looks correct -- the a tags are wrapping pieces appropriately, it just seems to be a layout quirk, perhaps with us not loading the images and what gecko is trying to lay out. It seems to work out correctly when viewing the message in Thunderbird.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8515339 [details] [review]
Bleach pull request

Nice.  Thanks for the excellent comment in bleach.js and the investigation behind it.  Definitely agreed on the minimal fix here.

I like the cleanup of the tests, although it's not immediately obvious why we want to introduce the "ablock" test.  Is the idea to create separate test scripts/dirs for orthogonal aspects of the code or use-cases?  I'm good either way; it's isomorphic from my perspective doing it this way versus making sure the .html files have appropriate prefixes or obvious names.  (No changes required!  Just interested.)

Comment 10

[James Burke [:jrburke]]

The idea on the test was to break it down by functional goal, to match 'unclean' and 'snippets', but ablock was just a placeholder until I figured out the issue, and forgot to revisit it. 

I renamed it to tag-matching.js along with renaming the associated folder, with an a-table-interior.html test as the only one in that group.

I do see where it can get grey for tests that span maybe two different tests, and I can see a prefix also working. Happy to redo it differently if you want a more flattened setup. But would likely do that as a separate commit/bleach ticket. For this ticket, I think what is there now at least fits better with the snippets, unclean sort of separation.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

Makes sense, and arguably superior than a giant soup directory of files that demand perfect hygiene.  Thanks for the explanation!

Comment 12

[James Burke [:jrburke]]

GELAM pull request:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/347

Gaia pull request:
https://github.com/mozilla-b2g/gaia/pull/25712

Once automation runs on them and it looks good, then I'll merge.

Comment 13

[James Burke [:jrburke]]

Merged in Gaia master:
https://github.com/mozilla-b2g/gaia/commit/69a6e9c9d8bdd75d868f31db83008c657a361b2b

from pull request:
https://github.com/mozilla-b2g/gaia/pull/25712

---

Merged in GELAM master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/fa47ac25c45d55d966bd3cbcfc2064a10b2710d1

from pull request:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/347

Comment 14

[Wayne Chang [:wchang]]

[Blocking Requested - why for this release]:

Nominating for 2.1 given where we are with 2.0, it's too late for such change.

Comment 15

[Candice Serran (:cserran)]

Blocking - minimal risk confirmed by engineer and is broken functionality

Comment 16

[James Burke [:jrburke]]

Attachment: Gaia pull request

This is the pull request that was merged into master and that is requesting approval for 2.1, and already r+ via the bleach request, this is the one that just passes the change into Gaia from the bleach.js repo. 

The commit that landed on master:
https://github.com/mozilla-b2g/gaia/commit/69a6e9c9d8bdd75d868f31db83008c657a361b2b

[Approval Request Comment]

[Bug caused by] (feature/regressing bug #):
Long standing bleach.js behavior around wanting to auto-close inline tags if a typically "block" tag was inside it. Those are outdated rules for HTML though.

[User impact] if declined:
User will not be able to tap on "confirm your account" links that contain other elements like tables or what is traditionally considered a block element. Facebook is one of them. Also affects any link that has block elements inside of them.

[Testing completed]:
On flame device on master. A unit test was added to bleach.js too.

[Risk to taking this patch] (and alternatives if risky):
Very low risk. The change just removes the "a" tag from being considered an "inline" element that needs auto-closing if a block element is inside it. Does not affect the scrubbing of what is considered good/bad HTML.

Long term we should probably just remove that auto-close logic altogether, but we wanted to be careful and only fix the main issue: "a" tags closed in a way that they become unclickable.

[String changes made]:
None

Comment 17

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/9607f7e1acc627ea10759aa4a6683a1f7f324112

Comment 18

[sarsenyev]

Verified the issue is fixed on 2.2 and 2.1 Flame
The user is able to confirm Facebook account from email, the confirmation screen appears after tapping the "Confirm your Account"

Device: Flame 2.2 Master
BuildID: 20141105160209
Gaia: 7918024c737c4570cacd784f267e28737ae05dea
Gecko: 2114ef80f6ae
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Device: Flame 2.1
BuildID: 20141106001204
Gaia: 9658b93b412bdcc0f953d668e8c8e68318c99fb8
Gecko: 76880403db44
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0