309012 - STYLE section data appears in message body with Simple HTML

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect, P2)

Description

[Brian Kennelly]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050917 SeaMonkey/1.1a
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050917 SeaMonkey/1.1a

When HTML e-mails has a STYLE section in the header, the style contents are
displayed in the message body with View->Message Body as->Simple HTML.

The last working build was 2005-09-09-07.  

Reproducible: Always

Steps to Reproduce:
1. In Mail/News, choose View->Message Body as->Simple HTML
2. Find (or send to yourself) an HTML email with STYLE data


Actual Results:  
The STYLE contents are displayed at the top of the message body.

Expected Results:  
The STYLE content should not have been displayed.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

I'm taking and confirming this based on code inspection. I'll look at this on
Monday (and add a test for this in the outsink tests).

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Ignore <style>, <script>, <textarea>

I can't actually test this, but it should fix this bug. Basically, the idea is
that if we're not allowing a tag and it appears in the special list of tags,
then we'll ignore all of the contents as well.

Ben, what do you think?

Comment 3

[Ben Bucksch (:BenB)]

+  else if (strchr(gIgnoreContents, aTag))
+    ++mSkipLevel;

Yuck. aTag is an integer, see below. Then you can get rid of the array, too.

I would not skip textarea contents.

+  if (mSkipLevel == 0 && IsAllowedTag(type))

Do

if (type == eHTMLTag_script || type == eHTMLTag_style)
  mSkipLevel++;
else if (IsAllowedTag(type))
  ...
else if (mSkipLevel > 0)
  Write(NS_LITERAL_STRING(" "));

instead.

And please test before the next patch!

Comment 4

[Ben Bucksch (:BenB)]

nonsense, I meant:

if (type == eHTMLTag_script || type == eHTMLTag_style)
  mSkipLevel++;
else if (mSkipLevel > 0)
  Write(NS_LITERAL_STRING(" "));
else if (IsAllowedTag(type))
  ...

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: take 2

This still needs testing, so I'll hold off on asking for reviews until it gets
tested.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 196684 [details] [diff] [review]
take 2

mcsmurf has confirmed that this patch works for him.

I've deliberately kept the ordering the way I had it before so that if someone
ever decides to allow <script> or <style> tags, they can simply enable them in
the pref and not hack the serializer.

Comment 7

[Ben Bucksch (:BenB)]

Your logic is wrong. If you have a <form> (disallowed tag) in a <script>, your
skiplevel will get messed up.

Please test *yourself*, *carefully*, including edge cases.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: take 3

As things stand, it seems that it's impossible to ever give the sanitizing
serializer such a broken DOM. The only entry point that I know is the "Simple
HTML" option for viewing message bodies, which always goes through CNavDTD ->
mozSanitizingSerializer, and if there are _any_ containers inside <style> or
<script>, there's a bug that needs to be fixed there (in the DTD).

That said, I believe this should fix all of the remaining logic problems.

Comment 9

[Hixie (not reading bugmail)]

If at any point a script has access to the DOM in question, then you can end up
with child nodes of <style>, etc.

Comment 10

[Ere Maijala (slow)]

Blake, are you working on this? Personally I think this problem is something that should be fixed before any release.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

I'm simply waiting for review on my patch. This is a trunk-only regression (bug 272702 never touched the branch). I'm sorry it took so long for this fix to go in. If Ben can't review the latest patch, I'll try seeking a new reviewer.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #9)
> If at any point a script has access to the DOM in question, then you can end up
> with child nodes of <style>, etc.

Except, in those cases, we use the fragment content sink. This bug only affects the sanitizing content sink, which is (again!) only instantiated with a CNavDTD DTD. At any rate, the last patch should technically handle all sorts of invalid cases anyway.

Comment 13

[Ere Maijala (slow)]

D'oh, sorry. I somehow missed the r? flag.. 

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 196713 [details] [diff] [review]
take 3

r+sr=jst, looks good.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.