917045 - [B2G][Email] Clicking on email address links in message bodies results in confusing/misleading prompt "Browse to URL?: mailto:"

Status: RESOLVED FIXED

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

Description

[Jeni]

'Browse to URL?: mailto:(email address)' banner displays when selecting a email address in an email message

PreReq: Have an email setup on device.  Received a email with email address in body of the email  

Repro Steps:
1) Updated Buri to Build ID: 20130916040205
2) Tap on Email
3) Open email (with email address in body)
4) Long tap on email address (in body)
5) View what happens

Actual:
When selecting email address from an email message says 'Browse to URL?: mailto:(email address)'

Expected:
User is brought directly to compose email or receives a banner saying 'Send email to: (email address)'

Environmental Variables
Build ID: 20130916040205
Gecko: http://hg.mozilla.org/mozilla-central/rev/c4bcef90cef9
Gaia: a0079597d510ce8ea0b9cbb02c506030510b9eeb
Platform Version: 26.0a1

Repro frequency: 100%
Test Suite Name: Email
UCID: 2895
Link to failed test case: https://moztrap.mozilla.org/manage/case/2895/
See attached picture for example of issue

Comment 1

[Jeni]

Attachment: Screenshot of issue

Comment 2

[Jason Smith [:jsmith]]

UX is probably not going to be okay with this polish regression.

Comment 3

[Lucas Aliaga]

Bug repros on both Leo device 1.1 and Buri device 1.2. Goes as far back as 6/21 for 1.2 build. Contact addresses bring up the "mailto:" banner when tapping on them in the body of emails.

- Broken 1.1 Leo -
Environmental Variables
Build ID: 20130917041200
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/b459ac3ebe24
Gaia: 862b5761825e010e3cc5968c096377e1ea54bbf2
Platform Version: 18.1

- Broken 1.2 Buri -
Environmental Variables
Build ID: 20130621031231
Gecko: http://hg.mozilla.org/mozilla-central/rev/7ba8c86f1a56
Gaia: e2f19420fa6a26c4287588701efaec09a750dba1
Platform Version: 24.0a1

Comment 4

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

To further elaborate on Lucas's comment, this is how things have been implemented since the linkification feature was landed.  Things have never worked like the strict reading of the MozTrap test case suggests.

The bug does make a good point that the UX of what we're doing is not super pleasant for the user, although there are phishing concerns that mean that what the strict reading calls for could have issues since we would potentially hide the e-mail address in favor of the 'display name' of the account.  So the user might hit a hyperlink and not ever know the actual e-mail address of who they are sending the message to.

For the time being, the MozTrap case should probably be amended to describe the implemented behaviour.

Comment 5

[Angela Hubenya]

Test case 2895 has been updated to current design.

Comment 6

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

This is still a thing (for mailto versus normal hyperlink confusion).  Note that bug 1082604 about the annoyance/security issues has most of the interesting discussion on this topic and is fairly relevant because our compose UI is still potentially susceptible to spoofing.

Comment 8

[Teri Wen [:twen]]

Nominate for V3.0 work so we don't forget this.

Comment 9

[GitHub Autolander Bot]

Attachment: [gaia] rsajdok:bug#917045 > mozilla-b2g:master

Comment 10

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

(In reply to Autolander from comment #9)
> Created attachment 8600680 [details] [review]
> [gaia] rsajdok:bug#917045 > mozilla-b2g:master

(Commenting in the bug since the spoofing/phishing concerns will affect all implementations and comments on pull requests can sorta get lost/be a hassle to review.)

The current patch prompts using "data.to[0]".  This results in us only showing one of potentially many email addresses that are being included.  I don't think this is sufficient to address spoofing concerns given the rest of our compose UI as it currently exists.

For the mailto: case, I think the following might be a better UX for this:

- When a mailto link is clicked on, just directly trigger the compose UI.  The rationale is that the compose UI should already be a good characterization of what mail the user is going to send.  And...

- we ensure that the back button can be immediately hit from the compose UI triggered by mailto links without prompting to create a draft (and without creating a draft!).  This way there's no more clicks than the prompt if the user hadn't wanted to compose a new mail.

- Make the compose UI have a mode where instead of displaying "display names" in the bubbles, we display email addresses.  Or perhaps we even drop the display names in this mode entirely.  We could call it "suspicious mode" or something like that.

- mailto links trigger this "suspicious mode" (both internally an externally triggered.


The specific family of threat I'm concerned about is where the user receives a message from an attacker that purports to be from a non-attacker and uses multiple recipients or display name tricks to make it seems like the user's response is going to a trusted person when it's not (or is additionally going to a bad actor).  For example, I think I'm sending a message to [e-mail], but that was just a display name that looked like an email address, or the first recipient really is "@trusted.domain" but is to an empty/abandoned/blackhole account and there is a cc that is to an account controlled by the attacker, etc.


I do think it's okay for us to address this in multiple steps, but from a security perspective, I would argue that the current solution is preferable to than the current pull request, just because more complex mailto URI's will (hopefully?) look more suspicious because they'll be a load of gobbledygook.

All of this is a :jrburke and UX call, though.

Comment 11

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

well, and security.

Comment 12

[James Burke [:jrburke]]

Comment on attachment 8600680 [details] [review]
[gaia] rsajdok:bug#917045 > mozilla-b2g:master

Agreed with :asuth's analysis that is best to just go directly to compose for mailto: addresses, as it will be more expressive since it can list all the email addresses that will receive the address.

It also matches what I have heard Juwei express before for links in email: where possible, while matching security goals, allow the user to proceed directly to the action. I will ni? for Juwei to confirm.

So in the case of mailto: links, we can meet security goals around spoofing by doing what :asuth outlined.

Although, I think instead of a special flag to compose, it should be enough for us to scrub the "to" addresses to just be strings with the email addresses and not pass the "to" addresses as objects with names. compose.js _loadStateFromComposer will do the right thing if they are just email address strings.

This should also play better if the user does save a draft and want to go back to compose later, the names will stay in email form without us using a name different from the email address.

So Robert, if you want to look at a patch that sanitizes the "to" array before passing to compose and confirming that the back without the user entering any more new info can be done, let's start down that path. That "back without draft save" might need some thought, but we can see how that goes.

It also seems like activity_composer_data.js should also do this scrubbing on to/cc/bcc addresses. A bit unfortunate since user might have some context from the app triggering the activity, but then again we cannot validate the calling app is acting in good faith.

Of course if Juwei indicates wanting a different UX flow we can revisit based on that.

Comment 13

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

(In reply to James Burke [:jrburke] from comment #12)
> Although, I think instead of a special flag to compose, it should be enough
> for us to scrub the "to" addresses to just be strings with the email
> addresses and not pass the "to" addresses as objects with names. compose.js
> _loadStateFromComposer will do the right thing if they are just email
> address strings.
> 
> This should also play better if the user does save a draft and want to go
> back to compose later, the names will stay in email form without us using a
> name different from the email address.

Ah, yeah, I forgot that the activity case sortof seizes control of the composer instance and the compose UI isn't particularly involved in this.  We just want the sanitization to happen once on the way into the compose UI from a "mailto" URI and it should not impact any other compose behaviour.

Note that sanitization should probably be done using MailAPI.parseMailbox() and then dropping the "name" from the object if a non-null result is returned.  (More specifically, the addressparser lib *must* be used, but that wrapper could be handy since it does some normalization already and if we haven't address the group syntax issue in it, we can address it in there too.)

> It also seems like activity_composer_data.js should also do this scrubbing
> on to/cc/bcc addresses. A bit unfortunate since user might have some context
> from the app triggering the activity, but then again we cannot validate the
> calling app is acting in good faith.

In bug 883008 the idea is that the compose UI would resolve email addresses to contacts, which could re-populate names as needed, which could mitigate fallout from this.

I will say from a security perspective I'm less worried about web activities than I am about mailto URIs.  Specifically, any content context can include a URL that will be linkified with high probability.  However, in order to trigger an activity (currently), there needs to be JS code actively running.  If an attacker is in a context where they can execute JS, they already have a lot of spoofing capabilities and side-channel attacks available to them.

Comment 14

[Juwei Huang[:juwei]]

As James said, if the security concern is less worry in this case, then it'd better to direct the link to the compose view.
I like Andrew's idea that tapping on back key will go back to the message directly without asking saving as draft or not. Yet I suggest that once users input texts in the compose view, we should allow users to have chance to save it as a draft.
So the flow would be:

1. Tap on the email link in the message body.
2. Screen brings users to compose window with a back key on top left.
3-1. If users tap on back key immediately without input anything, go back to message directly.
3-2. If users start to input texts on compose window then tap on back key, prompt a dialog to ask users want to save it as draft or delete it (same as ordinary compose view).

Comment 15

[robert.sajdok]

In my opinion, there are too many changes for one commit. It will be much easier for me to split it. I propose 1, 2 points and sanitization email for one solution (commit). 3-1, 3-2 points for the second solution (commit) and even to fill the separate bug on bugzilla. Furthermore, I propose the separate branch proposal for every points of your comments. If you accept my plan I will start to work on it.

Comment 16

[James Burke [:jrburke]]

I like the idea of doing the work in stages, but we can do that in one pull request like so: ask for needinfo? as you go through the steps, and you can have the separate commits in the pull request/branch, so you have checkpoints. Then, at the end, after getting feedback on the final commit in the branch, before asking for final r?, squash the commits to one commit for landing.

I do not think the changes should be very big, and it would be nice to have one atomic commit that contains the whole of the feature. It also helps avoid QA confusion that may lead to opening new bugs if we do not have a cohesive changeset.

Here is how I would break down the work for stages:

A) In message_reader, if a mailto: link, pushCard to compose. For the "to" values, make sure they are just strings of addresses instead of name/address objects. That can all be done in message_reader.

The compose card already has a back button and user can close it out just fine. The thing we need to investigate more is:

B) Make sure back button press does not prompt if the user has not changed the code. Right now compose's _saveNeeded, checkAddressEmpty function checks for any "to" value as triggering the save dialog, where now we want to not pay attention to values that exist as part of the list passed to compose on creation.

That would be enough to close out this bug.

Things that can be separate tasks/bugs:

1) Confirm that mailto: links from a web page in the browser do the right thing, namely make sure that the compose card shows the email addresses and not names. This might involve tracking how query_uri, activity_composer_data and the compose card work together. This part we could break out as a separate ticket if we find an issue, as it would be an existing issue not related to this change.

2) We can continue to use the existing bug 883008 to track upgrading email addresses to known contacts into their names, and that one can come later in its own pull request as it is something that has existed as a separate task for a while.

Comment 17

[robert.sajdok]

(In reply to James Burke [:jrburke] from comment #16)
> A) In message_reader, if a mailto: link, pushCard to compose. For the "to"
> values, make sure they are just strings of addresses instead of name/address
> objects. That can all be done in message_reader.
> 
> The compose card already has a back button and user can close it out just
> fine.

I have done it to this step. Check it, please.

Comment 18

[James Burke [:jrburke]]

This is the right top level change, but there is an issue with the `composer[key] = [address];` section: that only grabs the first address in a set of addresses. I did the following and it seems to capture the array nature of the to/cc/bcc, and convert those to be usable arrays:


diff --git a/apps/email/js/cards/message_reader.js b/apps/email/js/cards/message_reader.js
index d8d01fe..8df2ca6 100644
--- a/apps/email/js/cards/message_reader.js
+++ b/apps/email/js/cards/message_reader.js
@@ -772,8 +772,16 @@ return [
               // blindly accept input from the outside, and the queryURI
               // properties match the property names allowed on composer.
               Object.keys(data).forEach(function(key) {
-                var address = model.api.parseMailbox(data[key]).address;
-                composer[key] = [address];
+                if (key === 'to' || key === 'cc' || key === 'bcc') {
+                  composer[key] = data[key].map(function(addr) {
+                    return model.api.parseMailbox(addr).address;
+                  }).filter(function (value) {
+                    // Filter out nulls if address parsing failed.
+                    return value;
+                  });
+                } else {
+                  composer[key] = data[key];
+                }
               });
             }
           }


I tested this out with this test page: http://jrburke.com/temp/mailto.html and copy/pasting that HTML into an email.

Comment 19

[robert.sajdok]

(In reply to James Burke [:jrburke] from comment #16)
> I like the idea of doing the work in stages, but we can do that in one pull
> request like so: ask for needinfo? as you go through the steps, and you can
> have the separate commits in the pull request/branch, so you have
> checkpoints. Then, at the end, after getting feedback on the final commit in
> the branch, before asking for final r?, squash the commits to one commit for
> landing.
> 
> I do not think the changes should be very big, and it would be nice to have
> one atomic commit that contains the whole of the feature. It also helps
> avoid QA confusion that may lead to opening new bugs if we do not have a
> cohesive changeset.
> 
> Here is how I would break down the work for stages:
> 
> A) In message_reader, if a mailto: link, pushCard to compose. For the "to"
> values, make sure they are just strings of addresses instead of name/address
> objects. That can all be done in message_reader.
> 
> The compose card already has a back button and user can close it out just
> fine. The thing we need to investigate more is:
> 
> B) Make sure back button press does not prompt if the user has not changed
> the code. Right now compose's _saveNeeded, checkAddressEmpty function checks
> for any "to" value as triggering the save dialog, where now we want to not
> pay attention to values that exist as part of the list passed to compose on
> creation.
> 
> That would be enough to close out this bug.
> 
I have done it to this step. Check it, please.

Comment 20

[James Burke [:jrburke]]

Thanks, left some comments in the pull request.

Comment 21

[robert.sajdok]

Check the new version, please.

Comment 22

[James Burke [:jrburke]]

Comments in pull request. Getting close for this first part!

Comment 23

[robert.sajdok]

Check the new version, please.

Comment 24

[James Burke [:jrburke]]

This first step is looking good. One very small nit about not needing to use `self` in one case, but I think this is clear to proceed to the next step. You can keep this first step commit separate from the next step commit to help review of the next step easier.

A note, I will be away from keyboard for most of the time in the next two weeks, starting in another day or so, so my response times will be very delayed, maybe even up to a week or so.

Comment 25

[James Burke [:jrburke]]

Comment on attachment 8600680 [details] [review]
[gaia] rsajdok:bug#917045 > mozilla-b2g:master

Apologies for the very long review time, the aforementioned "away from keyboard" mention for me turned out to be pretty much no keyboard access. I have a window today of access, but then will enter another window for the rest of this week where I will mostly be away.

This latest change to remove the `self` from the saveNeeded looks good, thanks. All ready for the next step.

Comment 27

[robert.sajdok]

You said that it would be enough to close this bug in comment 16. Why did not you merge it to the master?

Comment 28

[James Burke [:jrburke]]

Comment on attachment 8600680 [details] [review]
[gaia] rsajdok:bug#917045 > mozilla-b2g:master

I am very sorry about that. I had it in my head that the work was going to happen in two stages, but looking at comment 16, both were done in this commit! So I just did not bother to go back and confirm the landing criteria.

This is a great improvement, thank you for waiting as I was away on vacation and suffered from bad cache state in my head.

Comment 29

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/98191d8c0b83b6cbb9685811f532b20a2e072750