193929 - HTML sanitizer should allow attached images

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Ere Maijala (slow)]

Currently messages filtered using Simple HTML don't display even the images that
are part of the message. Ben confirmed that this is not the intended behavior. 

I'm creating a patch to allow the src tag if the scheme is cid.

Comment 1

[Ere Maijala (slow)]

Attachment: Patch to enable img src with scheme cid

This patch enables img tag's src attribute if the scheme is cid.

Comment 2

[Ere Maijala (slow)]

Comment on attachment 114824 [details] [diff] [review]
Patch to enable img src with scheme cid

Ben, could you check this? It's just a small extension to the list of allowed
attributes (plus removal of few old debug blocks, if you don't mind :) )

Comment 3

[Ben Bucksch (:BenB)]

Comment on attachment 114824 [details] [diff] [review]
Patch to enable img src with scheme cid

Thanks for looking into that, Ere.

However, I don't think that's the right way to fix this.
1. What you are doing is probably slow. For *every* attribute (in allowed tags)
which has been denied, you are searching for a colon, if you find it, construct
another string and check again, if that is allowed. Also, the pref snytax you
use for that might be needed later for another case (attr[fullvalue], not
attr[scheme]).

You have to hardcode knowledge about URLs anyways (the colon), so I would just
hardcode this, like adding a function IsAllowedEmbeddedDoc(anURL) (would
currently true for cid:, false otherwise) and then hardcoding knowledge about
img src. I would check that *after* the attribute has been allowed, so that
users have the option to entirely remove the src attribute of image. You could
then safely allow the attribute of URLs for stylesheets, embedded plugins etc.
and (only) if the user configured the tag/attribute pair as allowed and the URL
is inline, it would work.

So, I am imagining:
If the tag and attribute is allowed (code as in CVS), check for a list of
tag/attribute pairs (probably hardcoded) known to contain URLs (*not* including
<a href>). If it is one of them, check IsAllowedEmbeededDoc(). If true, allow
attribute and value. If false, ignore attribute and value.

2. I would not search for a color etc., but leave that to netwerk. Create an
URI (nsIIOService->NewURI), get its scheme (->GetScheme(), IIRC), compare it
with your list of accepted URL schemes (probably hardcoded, currently only
"cid").

3. Please leave that debug statement in, it's DEBUG_BenB, so it should not
hurt, but might prove valueable in debugging the code.

Comment 4

[Ben Bucksch (:BenB)]

> compare it with your list of accepted URL schemes (probably hardcoded,
> currently only "cid").

If you want, you could maybe make that configurable, in another comma-delimited
pref. That would be useful, if we ever use the sanitzer for the browser, but I
don't know, if I or anybody else ever takes the time to implement that, so it's
not very important.

Comment 5

[Ben Bucksch (:BenB)]

> I would check that *after* the attribute has been allowed

Ah, just hook up at SanitizeAttributeValue() IMHO.

E.g. before |return NS_OK;|, check, if aTag is img (or link or whatever) &&
attr_name is src (or ref or whatever). If the URL scheme is none of the allowed
ones, then return NS_ERROR_ILLEGAL_VALUE, otherwise go on (i.e. bump into
|return NS_OK;| in the end).

Comment 6

[Ere Maijala (slow)]

Comment on attachment 114824 [details] [diff] [review]
Patch to enable img src with scheme cid

Ben, thanks for the input. Will do better next time :)

Comment 7

[Ere Maijala (slow)]

Attachment: v2 Patch to enable inline images

Ben, is this anything like you were thinking about?

Comment 8

[Ben Bucksch (:BenB)]

Yes, that looks good :-). The old code wasn't "bad", though :).

I haven't thoroughly checked the code, though (running etc.)

Are you sure that you need the case insensitive comparisons (maybe the attribute
and schemes are always delivered lower case)?

Comment 9

[Ere Maijala (slow)]

At least it comes to the sanitizer just as it's written. Having said that, it
seems that we have a bug somewhere that makes images with uppercase CID scheme
to not load completely. Using original HTML, it seems that it gets the image
dimensions but doesn't show it. Uppercase HTTP works correctly. I'll try to find
this bug too.

Comment 10

[Ere Maijala (slow)]

A correction to my previous comment. Images with CID scheme don't load at all.
The dimensions are of course from the message itself.

According to RFC 1738:
For resiliency, programs interpreting URLs should treat upper case letters as
equivalent to lower case in scheme names (e.g., allow "HTTP" as well as "http").

Comment 11

[Ere Maijala (slow)]

Attachment: v2.1 Patch to enable inline images

Added a fix for upper/mixedcase CID handling.

Comment 12

[Ben Bucksch (:BenB)]

ok, great, don't worry about that casing. just wanted to make sure :).

I still didn't run it yet, but will do so, on Windows and Linux.

Comment 13

[Ben Bucksch (:BenB)]

+    nsCOMPtr<nsIIOService> ioService;
+    ioService = do_GetIOService(&rv);

Make that
nsCOMPtr<nsIIOService> ioService = do_GetIOService(&rv);

+    // Check img src scheme

Make that
/* Allow only cid: URLs for <img src="...">, i.e. images embedded in
   multipart/related MIME messages */

I'll attach a new patch with those changes. With that, r=BenB. Thanks for the
nice code.

I couldn't compile and run it yet, because I have to recompile all of Mozilla
:-(. I'll notify you about how it went.

Comment 14

[Ben Bucksch (:BenB)]

Attachment: Patch, v2.2

Please let ducarroz review the libmime part, he is the module owner for that

Comment 15

[Ben Bucksch (:BenB)]

It works great on Linux trunk. Thanks :-).

It doesn't work on the 1.0 branch with Beonex Communicator patches (Linux and
Windows). It compiles and runs, but the fix has no effect. That code is probably
too old, so don't worry. At least we know that your code doesn't break VC++.

r=BenB for the complete v2.2. You'll still need review from ducarroz, because he
is the module owner for libmime. Plus superreview of course.

Comment 16

[Jean-Francois Ducarroz]

Comment on attachment 115105 [details] [diff] [review]
Patch, v2.2

R=ducarroz

Comment 17

[Ere Maijala (slow)]

Comment on attachment 115105 [details] [diff] [review]
Patch, v2.2

Jst, would you be able to sr this? Thanks.

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 115105 [details] [diff] [review]
Patch, v2.2

sr=jst

Comment 19

[Ere Maijala (slow)]

Fix checked in to trunk.

Comment 20

[Ben Bucksch (:BenB)]

*** Bug 138249 has been marked as a duplicate of this bug. ***