Issue 1102: document.write support for DOMita

A simple document.write implementation that uses the sax parser in the HTML sanitizer, and the element/attribute sanitizing code from DOMita's attribute sanitizer.

Submitted @3802

[metaweta, 2009-10-15 02:46:22]

http://codereview.appspot.com/130076/diff/1/7
File src/com/google/caja/plugin/bridal.js (right):

http://codereview.appspot.com/130076/diff/1/7#newcode350
Line 350: ' ', attribs[i], '="', html.escapeAttrib(attribs[i + 1]), '"');
Looks like you're masking the html object, and that the tests probably didn't
catch the mistake.  Since the test is browser-dependent, do we have a way to
test both of these branches?

http://codereview.appspot.com/130076/diff/1/6
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/130076/diff/1/6#newcode799
Line 799: // Swap last attribute name/value pair in place, and reprocess here.
If the value is null/undefined here and you swap them, then the attribName
becomes null/undefined the next time through the loop, which makes attribKey
look like "A::null" or "*::null".  Is that guaranteed to exist in html4.ATTRIBS?
 If not, then it looks like both attribName and attribValue become
null/undefined and you get an infinite loop.

http://codereview.appspot.com/130076/diff/1/5
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/130076/diff/1/5#newcode276
Line 276: if ('script'.toUpperCase() === 'SCRIPT') {
I know we special-case script tags in other places; should this code be shared
more widely?

http://codereview.appspot.com/130076/diff/1/2
File tests/com/google/caja/plugin/domita_test_untrusted.html (right):

http://codereview.appspot.com/130076/diff/1/2#newcode910
Line 910: // to move above it's starting position
its

http://codereview.appspot.com/130076/diff/1/2#newcode1266
Line 1266: if (false)
Is this necessary now that failure is an option?

[MikeSamuel, 2009-10-15 04:57:30]

http://codereview.appspot.com/130076/diff/1/7
File src/com/google/caja/plugin/bridal.js (right):

http://codereview.appspot.com/130076/diff/1/7#newcode350
Line 350: ' ', attribs[i], '="', html.escapeAttrib(attribs[i + 1]), '"');
Good catch.

This branch is exercised on IE.  I think the best way to test it is to get
DomitaTest kicking off to IE.

http://codereview.appspot.com/130076/diff/1/6
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/130076/diff/1/6#newcode799
Line 799: // Swap last attribute name/value pair in place, and reprocess here.
We have subtracted 2 from n, so if it's swapping stuff in from the end, then
there will be no next time through the loop.

http://codereview.appspot.com/130076/diff/1/5
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/130076/diff/1/5#newcode276
Line 276: if ('script'.toUpperCase() === 'SCRIPT') {
On 2009/10/15 02:46:22, metaweta wrote:
> I know we special-case script tags in other places; should this code be shared
> more widely?

This is nothing to do with script tags.  It's just a convenient way to test that
toUpperCase isn't doing a locale sensitive case conversion that mucks up tag
names.

http://codereview.appspot.com/130076/diff/1/2
File tests/com/google/caja/plugin/domita_test_untrusted.html (right):

http://codereview.appspot.com/130076/diff/1/2#newcode1266
Line 1266: if (false)
Failure is an option, but crashing the browser is not.

[MikeSamuel, 2009-10-15 18:47:20]

snapshotted

[felix8a, 2009-10-15 19:39:57]

http://codereview.appspot.com/130076/diff/19/24
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/130076/diff/19/24#newcode799
Line 799: // Swap last attribute name/value pair in place, and reprocess here.
I'm wary that you're changing the order of the attributes, which matters if an
attribute is duplicated, but iirc browsers aren't consistent about what value
gets used when an attr is repeated, so it shouldn't matter.  maybe add a comment
about that.

http://codereview.appspot.com/130076/diff/19/23
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/130076/diff/19/23#newcode220
Line 220: var isLimitClosed = detached[0].parentNode !== limit;
do you mean detached[1] instead of detached[0].parentNode?

http://codereview.appspot.com/130076/diff/19/23#newcode290
Line 290: var eltype = html4.ELEMENTS[tagName];
need hasOwnProperty test here

http://codereview.appspot.com/130076/diff/19/23#newcode291
Line 291: if ((eltype & html4.eflags.UNSAFE) !== 0) { return; }
it looks to me like if a startTag is rejected, the matching endTag will never
find the right tag to close, so it will close all open tags, screwing up the
rest of the structure.  which is ok, but should be documented.

it also looks like this doesn't cope too well with optional-close, like <p>. 
which is ok, but should also be documented.

http://codereview.appspot.com/130076/diff/19/23#newcode351
Line 351: ___.grantRead(tameDoc, 'writeln');
shouldn't this be grantFunc?

[MikeSamuel, 2009-10-15 20:19:52]

http://codereview.appspot.com/130076/diff/19/23
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/130076/diff/19/23#newcode220
Line 220: var isLimitClosed = detached[0].parentNode !== limit;
Good point.  Done

http://codereview.appspot.com/130076/diff/19/23#newcode290
Line 290: var eltype = html4.ELEMENTS[tagName];
On 2009/10/15 19:39:59, felix8a wrote:
> need hasOwnProperty test here

Done.

http://codereview.appspot.com/130076/diff/19/23#newcode291
Line 291: if ((eltype & html4.eflags.UNSAFE) !== 0) { return; }
On 2009/10/15 19:39:59, felix8a wrote:
> it looks to me like if a startTag is rejected, the matching endTag will never
> find the right tag to close, so it will close all open tags, screwing up the
> rest of the structure.  which is ok, but should be documented.

I don't think it will.  endTag never changes the insertionPoint unless it finds
a match.

http://codereview.appspot.com/130076/diff/19/23#newcode291
Line 291: if ((eltype & html4.eflags.UNSAFE) !== 0) { return; }
On 2009/10/15 19:39:59, felix8a wrote:
> it also looks like this doesn't cope too well with optional-close, like <p>. 
> which is ok, but should also be documented.

Fixed.  If the element type is optional, and the top is an element of the same
type, then this will bump up one level.
That should handle <p>'s and <li>'s and <td>'s closing each other, but will not
address the problem of an <h1> closing a <p>.  To do that, we'd need to keep
track of what are block level elements, or allowed containment relationships.

http://codereview.appspot.com/130076/diff/19/23#newcode351
Line 351: ___.grantRead(tameDoc, 'writeln');
On 2009/10/15 19:39:59, felix8a wrote:
> shouldn't this be grantFunc?
> 

Done.

[MikeSamuel, 2009-10-15 20:22:30]

http://codereview.appspot.com/130076/diff/19/24
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/130076/diff/19/24#newcode799
Line 799: // Swap last attribute name/value pair in place, and reprocess here.
On 2009/10/15 19:39:59, felix8a wrote:
> I'm wary that you're changing the order of the attributes, which matters if an
> attribute is duplicated, but iirc browsers aren't consistent about what value
> gets used when an attr is repeated, so it shouldn't matter.  maybe add a
comment
> about that.

Done.

[felix8a, 2009-10-15 20:23:45]

http://codereview.appspot.com/130076/diff/19/23
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/130076/diff/19/23#newcode291
Line 291: if ((eltype & html4.eflags.UNSAFE) !== 0) { return; }
On 2009/10/15 20:19:52, MikeSamuel wrote:
> On 2009/10/15 19:39:59, felix8a wrote:
> > it looks to me like if a startTag is rejected, the matching endTag will
never
> > find the right tag to close, so it will close all open tags, screwing up the
> > rest of the structure.  which is ok, but should be documented.
> 
> I don't think it will.  endTag never changes the insertionPoint unless it
finds
> a match.
> 

oh, right.  I was misreading that.

[felix8a, 2009-10-15 20:33:35]

lgtm

[MikeSamuel, 2009-10-15 20:42:15]

Will wait for feedback from Mike Stay.

[metaweta, 2009-10-15 21:14:41]

On 2009/10/15 20:42:15, MikeSamuel wrote:
> Will wait for feedback from Mike Stay.

LGTM