[FIX]First attribute doesn't win in misplaced content

RESOLVED FIXED in mozilla1.9alpha8

Status

()

defect
P1
critical
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

({fixed1.8.0.14, verified1.8.1.8})

Trunk
mozilla1.9alpha8
x86
Linux
Points:
---
Bug Flags:
blocking1.9 +
wanted1.8.1.x +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: baking on trunk (9/10), URL)

Attachments

(2 attachments)

Problem is also mentioned at http://sla.ckers.org/forum/read.php?2,15336 and http://ha.ckers.org/blog/20070830/overwriting-attributes/

Fix is coming up.  Writing tests now.
Posted patch FixSplinter Review
Makes sure to not reverse the order of tokens when moving them to and from the misplaced content nsDeque.
Attachment #279208 - Flags: superreview?(peterv)
Attachment #279208 - Flags: review?(mrbkap)
(Assignee)

Updated

12 years ago
Flags: blocking1.9?
Priority: -- → P1
Target Milestone: --- → mozilla1.9 M8

Updated

12 years ago
Blocks: xss
Comment on attachment 279208 [details] [diff] [review]
Fix

Index: parser/htmlparser/src/CNavDTD.cpp
+          // XXXbz would it be faster to get the tokens out with ObjectAt and
+          // the PopFront them all?

"then"
Attachment #279208 - Flags: review?(mrbkap) → review+
Comment on attachment 279208 [details] [diff] [review]
Fix

>Index: parser/htmlparser/src/CNavDTD.cpp
>===================================================================

>+          // XXXbz would it be faster to get the tokens out with ObjectAt and
>+          // the PopFront them all?

then PushFront ...?
Attachment #279208 - Flags: superreview?(peterv) → superreview+
Comment on attachment 279208 [details] [diff] [review]
Fix

No, "then PopFront".  Maybe I should say "then PopFront them all from mMisplacedContent".

Requesting approval for trunk and branches.  This is bug apparently being used for content-injection attacks...  Risk is as low as it gets in parser code, which is probably "medium to large", so we might want to bake on the trunk for a week or so before landing on the branches.
Attachment #279208 - Flags: approval1.9?
Attachment #279208 - Flags: approval1.8.1.7?
Attachment #279208 - Flags: approval1.8.0.14?
(In reply to comment #4)
> (From update of attachment 279208 [details] [diff] [review])
> No, "then PopFront".  Maybe I should say "then PopFront them all from
> mMisplacedContent".

Ah, and use PushTokenFront again? (which is what I meant instead of PushFront)
Right.  That approach would be to ObjectAt() and PushTokenFront() starting at the _end_ of the things I want to move, then pop them all off.
You might want to check if this piece of html:
<table><input type="password">
crashes with the patch.
It seems to crash in my debug build, but I might have done something wrong.

It seems to crash at:
    CToken* theAttrToken = theAttrNode.PopAttributeTokenFront();
in PushMisplacedAttributes
Flags: blocking1.8.1.7?
Flags: blocking1.8.0.14?
Doesn't seem to crash over here....
Would be nice to fix on the branch, but probably not a stop-ship. We'll look at approvals after it's landed on the trunk.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.9?
Flags: blocking1.8.1.7?
Flags: blocking1.8.0.14?
Fix landed on the trunk (by manually applying the change to reftest.list as that part of the patch had bit-rotted). Marking FIXED.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Flags: blocking1.9? → blocking1.9+
Resolution: --- → FIXED
It looks like the test file itself didn't land, so I reverted the reftest.list change to fix the orange.
Flags: in-testsuite?
Whiteboard: baking on trunk (9/10)
I checked the test in now that I was able to verify that it passed.
Flags: in-testsuite? → in-testsuite+
Duh, must have forgotten to cvs add that file. Thanks for dealing with it!
Checked in a followup patch addressing the review comments too.
Apparently there is something wrong with my vc build, so you can just ignore comment 7 any further (which you already did).
Comment on attachment 279208 [details] [diff] [review]
Fix

approved for 1.8.1.8 and 1.8.0.14, a=dveditz for release-drivers
Attachment #279208 - Flags: approval1.8.1.8?
Attachment #279208 - Flags: approval1.8.1.8+
Attachment #279208 - Flags: approval1.8.0.14?
Attachment #279208 - Flags: approval1.8.0.14+
Checked in on both branches.
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.8pre) Gecko/20071004 BonEcho/2.0.0.8pre

I now get green text and a checkbox input, while I got red text and a text input with Firefox2.0.0.7, testing:
http://lxr.mozilla.org/seamonkey/source/layout/reftests/bugs/394534-1.html
You need to log in before you can comment on or make changes to this bug.