Last Comment Bug 598105 - Support the HTML5 data-* attributes in the sanitizing fragment sink
: Support the HTML5 data-* attributes in the sanitizing fragment sink
: html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal with 1 vote (vote)
: mozilla2.0b7
Assigned To: :Ehsan Akhgari
: Andrew Overholt [:overholt]
Depends on:
Blocks: CVE-2010-2769 596300
  Show dependency treegraph
Reported: 2010-09-20 12:09 PDT by :Ehsan Akhgari
Modified: 2011-01-03 14:39 PST (History)
6 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (v1) (4.47 KB, patch)
2010-09-20 13:35 PDT, :Ehsan Akhgari
bzbarsky: review+
bzbarsky: approval2.0+
Details | Diff | Splinter Review
Patch (v1.1) (4.91 KB, patch)
2010-09-20 14:06 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch (v1.2) (4.91 KB, patch)
2010-09-20 14:23 PDT, :Ehsan Akhgari
christian: approval1.9.2.11+
christian: approval1.9.1.14+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2010-09-20 12:09:09 PDT
HTML5 allows data-* attributes, and we need to allow them in the sanitizing fragment content sink as well.
Comment 1 :Ehsan Akhgari 2010-09-20 13:35:47 PDT
Created attachment 476901 [details] [diff] [review]
Patch (v1)
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2010-09-20 13:40:59 PDT
Comment on attachment 476901 [details] [diff] [review]
Patch (v1)

>+        if (k.Find(NS_LITERAL_STRING("data-")) != 0) {

  if (!StringBeginsWith(k, NS_LITERAL_STRING("data-"))) }

seems more idiomatic.

Also, instead of nested ifs, would it make sense to make use of "&&" here?  At this point we have three-deep nesting for what is fundamentally just an and on several boolean expressions....

r=me with those nits.
Comment 3 :Ehsan Akhgari 2010-09-20 14:06:22 PDT
Created attachment 476907 [details] [diff] [review]
Patch (v1.1)

With comments addressed.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2010-09-20 14:15:12 PDT
> +    if ((!sAllowedAttributes && !sAllowedAttributes->GetEntry(keyAtom)) &&

That first && should be ||.
Comment 5 :Ehsan Akhgari 2010-09-20 14:23:56 PDT
Created attachment 476912 [details] [diff] [review]
Patch (v1.2)

(In reply to comment #4)
> > +    if ((!sAllowedAttributes && !sAllowedAttributes->GetEntry(keyAtom)) &&
> That first && should be ||.

Of course!
Comment 7 christian 2010-09-22 10:53:45 PDT
Comment on attachment 476912 [details] [diff] [review]
Patch (v1.2)

a=LegNeato for and
Comment 9 christian 2010-09-22 14:15:18 PDT
This caused the tree to go red:

/builds/slave/mozilla-1.9.2-linux/build/content/html/document/src/nsHTMLFragmentContentSink.cpp:1111: error: no matching function for call to 'StringBeginsWith(nsCAutoString&, const nsString&)'

Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2010-09-22 14:22:30 PDT
Oh, on 1.9.2 and 1.9.1 k is probably a UTF-8 string (and hence you need an NS_LITERAL_CSTRING.

I have no idea why this was reopened, though.  This is fixed on trunk.

I assume you backed it out of 1.9.2, right?  So unsetting that flag.  Did you back out of 1.9.1 as well?
Comment 11 christian 2010-09-22 14:26:46 PDT
Sorry, shouldn't have reopened. I have not backed out the patch as I do not have commit privs. Wasn't sure if we generally file new bugs for bustage or just continue on with the existing one.
Comment 12 :Ehsan Akhgari 2010-09-22 14:38:06 PDT
Bustage fix pushed on both branches.

I chose to use |key| instead of |k|, which is the UTF-16 version of the latter.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2010-09-22 14:40:10 PDT
Christian, for bustage we'd generally use the existing bug, since either a bustage fix is pushed (and then we need to say what the changeset for that was) or the patch is backed out (and then whatever bugzilla bit indicates that the patch is checked in needs to be flipped back to the "not checked in" state).

Note You need to log in before you can comment on or make changes to this bug.