Bryan Clark (DevTools PM) [:clarkbw] Reporter
	Description • 15 years ago

I feel like there has to be a dupe out there but I can't find one after a lot of searching.

I often get emails with links for the subject and I have to copy and paste them into the browser.  In my header extension I wrote a function to find links in the subject link and make them clickable / linkified.

Function:
http://hg.mozilla.org/users/clarkbw_gnome.org/ovidiu-header/rev/a363ad16f4f4

Screenshot:
http://hg.mozilla.org/users/clarkbw_gnome.org/ovidiu-header/raw-file/8a8b4a654ea1/release/screenshot-linked-subject.png

I think something like this function would be really useful for the default header implementation.  If I get some time this weekend I'm going to see about adapting my patch to work in the default header view.  I'd like to wait for bug 489609 to land so I don't get bit rotted right away.

Questions:

Is there an existing link detection regex already available?  I used one that I found in a patch in bugzilla. :)

I haven't tried to get the link context menu to work yet but I'm assuming it's possible to use our existing one.

Seems like a reasonable route would be to create a subject specific binding (mail-subject-headerfield) instead of mail-headerfield in mailWidgets.xml that knows how to create linked sub nodes as it finds them in the text.

I was also thinking about doing this for email addresses; perhaps as a separate bug.

	Ben Bucksch (:BenB)
	Comment 1 • 15 years ago

http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/streamconv/public/mozITXTToHTMLConv.idl#111

	Ben Bucksch (:BenB)
	Comment 2 • 15 years ago

i.e.: mozITXTToHTMLConv::findURLInPlaintext() should be exactly what you need.

	Bryan Clark (DevTools PM) [:clarkbw] Reporter
	Comment 3 • 15 years ago

There are some add-ons that already do things like this.  We might try this as an add-on though I think the risk is low enough here that we could probably just land it.  Marking as an add-on idea anyway.

	Jim Porter (:squib) Assignee
	Comment 4 • 15 years ago

Would a generic mail-richheaderfield be better? I could see that being useful for the "Organization" header as well (not that I see that header very often).

	Dan Mosedale (:dmosedale, :dmose)
	Comment 5 • 15 years ago

Jim: sounds reasonable to me, though as you say, it's not a particularly common thing.

	Bryan Clark (DevTools PM) [:clarkbw] Reporter
	Comment 6 • 15 years ago

(In reply to comment #4)
> Would a generic mail-richheaderfield be better? I could see that being useful
> for the "Organization" header as well (not that I see that header very often).

That makes a lot of sense to me.  You're likely much more qualified to take this bug on so feel free to assign it to yourself if you'd like.

	Jim Porter (:squib) Assignee
	Comment 7 • 15 years ago

Since a hypothetical "mail-richheaderfield" would inherit from mail-headerfield, I think I'll wait until the fallout from bug 489609 is resolved.

	Pete Riley
	Comment 8 • 15 years ago

It'd be nice if rss subjects were also linkified (to launch the feed's main url).  I'd like to hide the main url (via userchrome.css) from the header pane to save vertical space.  I'd understand if this is outside the scope of this bug, I just wanted to mention it.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 9 • 15 years ago

Jim: sounds wise. 

Pete: I like that idea a fair bit.

	Bryan Clark (DevTools PM) [:clarkbw] Reporter
	Comment 10 • 15 years ago

(In reply to comment #7)
> Since a hypothetical "mail-richheaderfield" would inherit from
> mail-headerfield, I think I'll wait until the fallout from bug 489609 is
> resolved.

It's go time! :)

	Jim Porter (:squib) Assignee
	Comment 11 • 15 years ago

Here's a fairly simplistic patch that probably gets at least a few things wrong, but the basics are there. The links could use some better styling (I'm stealing the style for mail-urlfield), and maybe a context menu. Also, I'm destroying/recreating nodes every time a new value gets set, which is pretty inefficient.

I probably won't have time to work on this for a few weeks, due to the holidays, so I'm not going to assign the bug to myself. I'll probably have time after that, though.

	Bryan Clark (DevTools PM) [:clarkbw] Reporter
	Comment 12 • 15 years ago

Magnus, do you have time to look at this patch?

It still applies for me and works great.  I'm wondering if we can get it in for 3.1

	Jim Porter (:squib) Assignee
	Comment 14 • 15 years ago

I forgot about this. I just want to mention: I tried to make it so that it caches the DOM nodes when switching messages, but text nodes are handled in strange and mysterious ways (to me, anyway), and all I managed to do was make line-wrapping stop working, so this is probably as close as I can get.

	Magnus Melin [:mkmelin]
	Comment 15 • 15 years ago

Looking forward to seing a finalized patch. Looks basically ok to me.
Nit: use spaces between if and "(" and in places like for (let i=0; i<val.length; i++) - so that it's for (let i = 0; i < val.length; i++)

	Jim Porter (:squib) Assignee
	Comment 16 • 13 years ago

Taking this so I remember to come back to it after cloud files.

	Bryan Clark (DevTools PM) [:clarkbw] Reporter
	Comment 17 • 13 years ago

(In reply to Jim Porter (:squib) from comment #16)
> Taking this so I remember to come back to it after cloud files.

Awesome!  Looking forward to this.

Just wanted to note that I did this for protz's conversation view code as well: https://github.com/protz/GMail-Conversation-View/blob/master/modules/misc.js#L198

	Jim Porter (:squib) Assignee
	Comment 18 • 12 years ago

Ok, I made this work a little better. On a scale from 1 to 10, how crazy/stupid is this patch? The only easy way to get properly linkified URLs for all the various cases out there seems to be to grab an HTML string from mozITXTToHTMLConv. Then, I have to parse that as HTML, iterate over the nodes, and turn them into happy bits of XUL. (I tried just injecting the HTML into the XUL, but it totally broke everything when clicking on links.)

	Ben Bucksch (:BenB)
	Comment 19 • 12 years ago

> grab an HTML string from mozITXTToHTMLConv

Good, thanks.

> I tried just injecting the HTML into the
> XUL, but it totally broke everything when clicking on links.

Maybe you just need to instruct where to open links, similar to message pane. I don't know how, though.

It's a funny idea to convert the HTML to XUL, but you're lacking all security. The input is clearly untrusted, and you're inserting it into chrome XUL with full privileges. This makes it a security hole and gives it crazy/stupid value 10 :). The converter is written with the assumption that the result goes into an untrusted frame with all rights stripped. As absolute minimum, you need to
1. check that the URL scheme is http or https only, nothing else, and
2. make sure that it opens in a content pane.
3. I'd still be unsure, though. It would be better to use a small, seamless iframe (bug 80713).

Please note that this danger is inherent in what we try to do here and completely independent of how you find the URLs.

Have you tried using mozITXTToHTMLConv::findURLInPlaintext() (comment 2) ?

	Ben Bucksch (:BenB)
	Comment 20 • 12 years ago

> Please note that this danger is inherent in what we try to do here and completely independent
> of how you find the URLs.
>
> Have you tried using mozITXTToHTMLConv::findURLInPlaintext() (comment 2) ?

(Please note that despite the proximity, these 2 comments were entirely unrelated. Using findURLInPlaintext() or any other method still leaves the security question to be solved.)

	Jim Porter (:squib) Assignee
	Comment 21 • 12 years ago

(In reply to Ben Bucksch (:BenB) from comment #19)
> It's a funny idea to convert the HTML to XUL, but you're lacking all
> security. The input is clearly untrusted, and you're inserting it into
> chrome XUL with full privileges.

Where's the security hole? I'm only inserting 1) text nodes (which should be safe), and 2) xul:description nodes with some text (which, again, should be safe) and a link. I'm assuming that mozITXTToHTMLConv is safe enough that it's not going to leak or have buffer overruns, so the only potentially unsafe spot is the link. Since that's a pretty small surface area for attacks, it should be straightforward to sanitize.

 This makes it a security hole and gives it
> crazy/stupid value 10 :). The converter is written with the assumption that
> the result goes into an untrusted frame with all rights stripped. As
> absolute minimum, you need to
> 1. check that the URL scheme is http or https only, nothing else, and

We'd want ftp: and mailto: as well, at a minimum.

> 2. make sure that it opens in a content pane.

Already done.

> 3. I'd still be unsure, though. It would be better to use a small, seamless
> iframe (bug 80713).

I think this is overkill, since the only things we're inserting that aren't inert text are some urls, and those are easy enough to sanitize (e.g. whitelisting protocols). In fact, the only protocol I can think of that would behave differently in an iframe is javascript:, and we should be safe for that anyway, since all we end up doing is calling nsIMessenger.loadExternalUrl, which - among other things - totally eliminates the window's context.

Assuming nsIMessenger.loadExternalUrl is safe (and we use it* elsewhere for stuff like this), then we should be safe here too, no matter the protocol.

> Have you tried using mozITXTToHTMLConv::findURLInPlaintext() (comment 2) ?

Yes, as the previous patch in this bug shows. It completely falls apart for a lot of cases (email addresses, urls surrounded by angle brackets, etc), and I didn't see much sense in unmunging the result when I could just get something correct from the same interface.

* And the equivalent functions openUILink and openLinkExternally.

	Ben Bucksch (:BenB)
	Comment 22 • 12 years ago

> Where's the security hole? I'm only inserting ... a link

Right. That's the hole. Let's say the URL is <javascript:alert(Components.interfaces)> or data: or whatever, and the user clicks on the title. The URL will run with chrome rights and we're done.

URLs are *the* most dangerous thing in XUL.

> findURLInPlaintext() ... completely falls apart for a lot of cases (email addresses,
> urls surrounded by angle brackets, etc)

Please elaborate.

	Jim Porter (:squib) Assignee
	Comment 23 • 12 years ago

(In reply to Ben Bucksch (:BenB) from comment #22)
> > Where's the security hole? I'm only inserting ... a link
> 
> Right. That's the hole. Let's say the URL is
> <javascript:alert(Components.interfaces)> or data: or whatever, and the user
> clicks on the title. The URL will run with chrome rights and we're done.

javascript: URLs don't do anything at all in this case. data: URLs might work, but they wouldn't be able to do anything that an attachment couldn't already. Still, as I already mentioned, we can easily whitelist protocols if we're not confident that the security already in place is insufficient.

We already use this strategy for opening untrusted URLs in mail-urlfield.

> > findURLInPlaintext() ... completely falls apart for a lot of cases (email addresses,
> > urls surrounded by angle brackets, etc)
> 
> Please elaborate.

The text range breaks for angle-bracketed URLs (it returns a range that extends beyond the ends of the string), email addresses don't get the mailto: protocol prepended, etc. The main problem is that it will (try to) tell us the text that should be linkified, but it doesn't tell us what the link is; since the two aren't always the same, there's a problem.

More to the point, it doesn't actually give us anything we don't get more easily from scanTXT.

	Jim Porter (:squib) Assignee
	Comment 24 • 12 years ago

Oops, s/insufficient/sufficient/; above.

	Ben Bucksch (:BenB)
	Comment 25 • 12 years ago

Just to clarify: I think a protocol whitelist is the absolute minimum protection. I'd still feel uncomfortable.

As for mozITXTToHTMLConv, this is the correct thing to use. Its primary purpose is to process incoming plaintext mail, which is untrusted, so you can assume it's OK.
I was only thinking that findURLsInPlaintext() is the more appropriate function, but your last comment makes sense. Unfortunately, C++ can't really offer a richer API, so your approach to use ScanTXT() indeed seems like the right way.

My only concern is how we insert and activate untrusted URLs in XUL. (I'm glad that links are the only markup you use.)

	Jim Porter (:squib) Assignee
	Comment 26 • 12 years ago

So, after discussing this on IRC, I think we have a pretty good handle on the security aspects* (I'll post a patch soon-ish), but there are two things I'm wondering about (neither of which are security-related):

1) Do we think there will be a significant performance hit from (something like) my patch above if we use mail-richheaderfield for all the "extra" header fields (i.e. the ones you only see when you choose to view all headers)? I'd really like to do this, since it means that links in those headers will be clickable. This makes headers like List-Unsubscribe much more useful.

2) Can we just remove mail-urlfield and use mail-richheaderfield? mail-urlfield is only used once (for Content-Base), which a) I've never actually seen, and b) is a strict subset of the features of mail-richheaderfield. The only downsides I can see are that it would cause a small performance regression, and might introduce an extension compatibility issue. While we could bind <mail-urlfield> to the new rich header field, it would still behave differently. That said, I'm personally ok with breaking compatibility here.

* Basically, do like my current patch, but whitelist it to http[s], ftp[s] and mailto urls, and also use urlSecurityCheck with the principal set to that of the mail body's document. I *think* we could get away with just the latter, but I don't think we even want to link anything but those protocols.

Thoughts?

	Jim Porter (:squib) Assignee
	Comment 27 • 12 years ago

How about this? I think this covers all the potentially-dangerous cases.

	Jim Porter (:squib) Assignee
	Comment 28 • 12 years ago

BenB: feedback ping? (Feel free to redirect this to someone else who you think understands the security concerns if you're too busy.)

	Ben Bucksch (:BenB)
	Comment 29 • 12 years ago

Thanks for the new patch. I still wouldn't do it like that, I still think the whole approach is dangerous, and I'd rather not have this feature at all than risk a security bug. I think the only safe approach is to use an untrusted sandbox (bug 80713).

But I may be overly cautious here, given that the HTML is generated by us. If you really want to go with this, please get approval from dveditz, I can't approve this, esp. the nsIScriptSecurityManager use.

	Ben Bucksch (:BenB)
	Comment 30 • 12 years ago

Comment on attachment 649035 [details] [diff] [review]
Be secure about linking

I leave it to higher authorities.

	Mark Banner (:standard8)
	Comment 31 • 12 years ago

This wants security review, so I'm moving the request to the recently added sec-review flag which hopefully the team will be tracking closer.

	Jim Porter (:squib) Assignee
	Comment 32 • 11 years ago

Ping on sec-review. I'm totally ok with this being WONTFIXed, but it would be nice to have a resolution on this.

	Jim Porter (:squib) Assignee
	Comment 33 • 10 years ago

Time for the annual sec-review ping. :)

Again, if this is deemed too risky, I'm ok with WONTFIXing it, but I'd like to get it out of limbo...

	Daniel Veditz [:dveditz]
	Comment 34 • 10 years ago

This looks like a sane approach: copying only the textContent of nodes except for links, and on links only carrying over the minimal number of attributes. The urlSecurityCheck() might be paranoid since you're filtering on safe schemes to start with, but it can't hurt and might save us if something gets injected that can change the href after you've created the header.

	Jim Porter (:squib) Assignee
	Comment 35 • 10 years ago

Comment on attachment 649035 [details] [diff] [review]
Be secure about linking

Since this has sec-review+ now, asking for a full review. It'd be nice to get this in.

(If this is still worrying, I wouldn't mind adding a pref to disable this for the truly paranoid, but I think this patch is already a bit belt-and-suspenders security-wise.)

	Jim Porter (:squib) Assignee
	Comment 36 • 10 years ago

Comment on attachment 649035 [details] [diff] [review]
Be secure about linking

Oops, I need to de-bitrot this first.

	Jim Porter (:squib) Assignee
	Comment 37 • 9 years ago

Ok, I fixed the bitrot in this patch. This should be pretty safe, since we're being paranoid with URL security (both filtering on protocol and doing a security check with Gecko). We'll also have plenty of time until TB 45 to test this.

	Josiah Bruner [:jsbruner]
	Comment 38 • 9 years ago

Comment on attachment 8597539 [details] [diff] [review]
Fix bitrot

If Blake's doing the review he may as well do the ui-review too. :)

	Blake Winton (:bwinton) (:☕️)
	Comment 39 • 9 years ago

Comment on attachment 8597539 [details] [diff] [review]
Fix bitrot

Review of attachment 8597539 [details] [diff] [review]:
-----------------------------------------------------------------

Okay, ui-review first…
A couple of small problems, but ui-r=me with them fixed.
1) We seem to be stripping spaces when linkifying the subject as seen in https://www.dropbox.com/s/klv6bfrpmostwgz/Screenshot%202015-05-03%2014.32.57.png?dl=0
2) Clicking on a url opens it in (for me) Firefox, but sometimes doesn't seem to focus Firefox, so hitting Cmd-W doesn't close the page.
3) There seems to be no way to open the url from the keyboard, despite letting the urls get focus with tab…
4) (Optional) Cmd-clicking should open the tab in Firefox, but leave the focus on Thunderbird.

Other than those, and the things mentioned below, I'm pretty happy, so r=me.  If you think that you needed to add too much code to fix the nits above, then feel free to re-request review.

Thanks!

::: mail/base/content/contentAreaClick.js
@@ +142,5 @@
> + * @param aFlags
> + *        Flags to be passed to checkLoadURIStr. If undefined,
> + *        nsIScriptSecurityManager.STANDARD will be passed.
> + */
> +function urlSecurityCheck(aURL, aPrincipal, aFlags)

So, why can't we use the version of this that lives in BrowserUtils?
  var temp={};
  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
  XPCOMUtils.defineLazyModuleGetter(temp,"BrowserUtils","resource://gre/modules/BrowserUtils.jsm");
  temp.BrowserUtils.urlSecurityCheck
pasted in the error console seems to give us the function…

::: mail/base/content/msgHdrViewOverlay.js
@@ +1016,5 @@
>      newLabelNode.setAttribute("control", idName);
>      newRowNode.appendChild(newLabelNode);
>  
>      // Create and append the new header value.
> +    newHeaderNode = document.createElement("mail-richheaderfield");

Do we want to do the extra processing to linkify the field for all the extra header fields, or only the subject?

	Jim Porter (:squib) Assignee
	Comment 40 • 9 years ago

(In reply to Blake Winton (:bwinton) from comment #39)
> Comment on attachment 8597539 [details] [diff] [review]
> Fix bitrot
> 
> Review of attachment 8597539 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Okay, ui-review first…
> A couple of small problems, but ui-r=me with them fixed.
> 1) We seem to be stripping spaces when linkifying the subject as seen in
> https://www.dropbox.com/s/klv6bfrpmostwgz/Screenshot%202015-05-03%2014.32.57.
> png?dl=0

Weird. This only seems to happen if the text between links is entirely whitespace. I fixed it, though.

> 2) Clicking on a url opens it in (for me) Firefox, but sometimes doesn't
> seem to focus Firefox, so hitting Cmd-W doesn't close the page.

I'm not sure there's anything I can do about that (or at least, I sure don't know how). I'm just using the same function we use in a bunch of other places for opening URLs in an external application.

> 3) There seems to be no way to open the url from the keyboard, despite
> letting the urls get focus with tab…

Hm, it turns out we have this issue with nearly all the header fields. Maybe I should file a followup.

> 4) (Optional) Cmd-clicking should open the tab in Firefox, but leave the
> focus on Thunderbird.

I have no idea how to do this either, unfortunately, although it seems to work (somewhat inconsistently) in the message body.

> ::: mail/base/content/contentAreaClick.js
> @@ +142,5 @@
> > + * @param aFlags
> > + *        Flags to be passed to checkLoadURIStr. If undefined,
> > + *        nsIScriptSecurityManager.STANDARD will be passed.
> > + */
> > +function urlSecurityCheck(aURL, aPrincipal, aFlags)
> 
> So, why can't we use the version of this that lives in BrowserUtils?
>   var temp={};
>   Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>  
> XPCOMUtils.defineLazyModuleGetter(temp,"BrowserUtils","resource://gre/
> modules/BrowserUtils.jsm");
>   temp.BrowserUtils.urlSecurityCheck
> pasted in the error console seems to give us the function…

Is that allowed? I didn't think we wanted to import browser-related stuff into Thunderbird.

> 
> ::: mail/base/content/msgHdrViewOverlay.js
> @@ +1016,5 @@
> >      newLabelNode.setAttribute("control", idName);
> >      newRowNode.appendChild(newLabelNode);
> >  
> >      // Create and append the new header value.
> > +    newHeaderNode = document.createElement("mail-richheaderfield");
> 
> Do we want to do the extra processing to linkify the field for all the extra
> header fields, or only the subject?

The main reason I chose to do this was for fields like List-Unsubscribe. Since those fields are shown in "All Headers" mode, I thought it would be nice if we could make those clickable.

	Jim Porter (:squib) Assignee
	Comment 41 • 9 years ago

Ok, I think I fixed all the issues. I also added a special path for mailto: links so they don't get loaded externally. This helps if, for instance, Thunderbird isn't your OS's default handler for mailto: URLs.

We could also give mailto: links a special context menu, but that might be better in a followup...

	Blake Winton (:bwinton) (:☕️)
	Comment 42 • 9 years ago

(In reply to Jim Porter (:squib) from comment #40)
> > 2) Clicking on a url opens it in (for me) Firefox, but sometimes doesn't
> > seem to focus Firefox, so hitting Cmd-W doesn't close the page.
> I'm not sure there's anything I can do about that (or at least, I sure don't
> know how). I'm just using the same function we use in a bunch of other
> places for opening URLs in an external application.

Okay, I think we can let it slide, then.

> > 3) There seems to be no way to open the url from the keyboard, despite
> > letting the urls get focus with tab…
> Hm, it turns out we have this issue with nearly all the header fields. Maybe
> I should file a followup.

Sounds good!

> > 4) (Optional) Cmd-clicking should open the tab in Firefox, but leave the
> > focus on Thunderbird.
> I have no idea how to do this either, unfortunately, although it seems to
> work (somewhat inconsistently) in the message body.

I think it's a check in the click-handling code, but it you wanted to file a followup, I would accept that.

> > ::: mail/base/content/contentAreaClick.js
> > @@ +142,5 @@
> > > +function urlSecurityCheck(aURL, aPrincipal, aFlags)
> > So, why can't we use the version of this that lives in BrowserUtils?
> Is that allowed? I didn't think we wanted to import browser-related stuff
> into Thunderbird.

Well, it's called BrowserUtils, but it lives in toolkit/modules, so I think we're probably safe.  ;)

> > ::: mail/base/content/msgHdrViewOverlay.js
> > @@ +1016,5 @@
> > >      newLabelNode.setAttribute("control", idName);
> > >      newRowNode.appendChild(newLabelNode);
> > >  
> > >      // Create and append the new header value.
> > > +    newHeaderNode = document.createElement("mail-richheaderfield");
> > 
> > Do we want to do the extra processing to linkify the field for all the extra
> > header fields, or only the subject?
> 
> The main reason I chose to do this was for fields like List-Unsubscribe.
> Since those fields are shown in "All Headers" mode, I thought it would be
> nice if we could make those clickable.

Hmm.  Yeah, sounds reasonable.  :)

New review coming soon!

Thanks,
Blake.

	Blake Winton (:bwinton) (:☕️)
	Comment 43 • 9 years ago

Comment on attachment 8600726 [details] [diff] [review]
Address review comments

Review of attachment 8600726 [details] [diff] [review]:
-----------------------------------------------------------------

Alright, looks good.  Thanks!  :D

	Magnus Melin [:mkmelin]
	Comment 44 • 9 years ago

Comment on attachment 8600726 [details] [diff] [review]
Address review comments

Review of attachment 8600726 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/mailWidgets.xml
@@ +609,5 @@
> +          let doc = document.getElementById("messagepane").contentDocument;
> +          urlSecurityCheck(uri, doc.nodePrincipal);
> +
> +          if (uri.scheme == "mailto") {
> +            MailServices.compose.OpenComposeWindowWithURI(null, uri);

would be nice to pass in the identity too.

	Jim Porter (:squib) Assignee
	Comment 45 • 9 years ago

(In reply to Magnus Melin from comment #44)
> Comment on attachment 8600726 [details] [diff] [review]
> Address review comments
> 
> Review of attachment 8600726 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/base/content/mailWidgets.xml
> @@ +609,5 @@
> > +          let doc = document.getElementById("messagepane").contentDocument;
> > +          urlSecurityCheck(uri, doc.nodePrincipal);
> > +
> > +          if (uri.scheme == "mailto") {
> > +            MailServices.compose.OpenComposeWindowWithURI(null, uri);
> 
> would be nice to pass in the identity too.

Fair point. I've also been thinking that I should make the context menu for email addresses include some more options, like "Copy Email Address".