Draft of a sanitizer for CSS selectors in JavaScript (issue 5649058)

[mikes...@gmail.com]

Reviewers: Jasvir,

Description:
sanitizerSelectors(cssText) performs the same sanitization as
CssRewriter. This CL is just a testfile that contains both the code
under test and the testcases in CssRewriterTest and does not attempt
to move the code into it's final location since I want to discuss
where this should fit as part of this CL.

One issue is whether CssRewriterTest.java should run its tests against
the
JavaScript version. To do that, we would need to

1. either load html-emitter.js and domado.js into Rhino to get the
property
sanitization function in domado.js, OR

2. move the property sanitization out of domado.js so that less needs
to be pulled into Rhino to test equivalence of the java and javascript
CSS rewritings.

I will follow this CL shortly with another that wires
sanitizeSelectors into html-emitter.js.

Please review this at http://codereview.appspot.com/5649058/

Affected files:
M src/com/google/caja/plugin/cssparser.js
MM src/com/google/caja/plugin/domado.js
M src/com/google/caja/plugin/html-emitter.js
M tests/com/google/caja/plugin/CssRewriterTest.java
A tests/com/google/caja/plugin/css-selector-test.html
A tests/com/google/caja/plugin/css-selector-test.js

[jas...@gmail.com]

LGTM

Pulling the selector sanitization functions out of domado seems like the
way to go for your testing question.

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

http://codereview.appspot.com/5649058/diff/1/src/com/google/caja/plugin/html-emitter.js#newcode422
src/com/google/caja/plugin/html-emitter.js:422: // } else if (atIdent
=== '@import') {
For the moment can we leave this uncommented and issue a console warning
here that @imports aren being deliberately elided.

http://codereview.appspot.com/5649058/diff/2001/src/com/google/caja/plugin/cssparser.js
File src/com/google/caja/plugin/cssparser.js (right):

http://codereview.appspot.com/5649058/diff/2001/src/com/google/caja/plugin/cssparser.js#newcode123
src/com/google/caja/plugin/cssparser.js:123: handler.endAtrule(atident);
Are you using the argument anywhere? function endAtrule doesn't seem to
use it.

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

http://codereview.appspot.com/5649058/diff/2001/src/com/google/caja/plugin/html-emitter.js#newcode465
src/com/google/caja/plugin/html-emitter.js:465: || 'head > html');
Clever. It's still possible to construct something that this selector
matches but it's not unsafe. It may be better to avoid this trick and
just hold off emitting the block until you knew if the any selector
would actually match.

http://codereview.appspot.com/5649058/diff/2001/src/com/google/caja/plugin/html-emitter.js#newcode510
src/com/google/caja/plugin/html-emitter.js:510:
document.getElementsByTagName('head')[0].appendChild(
Are we guaranteed gEBTN("head").length > 0 on all supported
browsers/docs?

http://codereview.appspot.com/5649058/diff/2001/tests/com/google/caja/plugin/CssRewriterTest.java
File tests/com/google/caja/plugin/CssRewriterTest.java (right):

http://codereview.appspot.com/5649058/diff/2001/tests/com/google/caja/plugin/CssRewriterTest.java#newcode497
tests/com/google/caja/plugin/CssRewriterTest.java:497:
System.out.println("test:\n\t" + css.replace('\n', '
').replaceAll("[{][^}]*[}]", "\n\t").trim());
cruft

http://codereview.appspot.com/5649058/