Issue 1669: Don't silently drop untranslatable CSS selectors

https://code.google.com/p/google-caja/issues/detail?id=1669

> Our CSS selector filter deletes selectors it does not understand, which cannot be virtualized accurately, or which are not yet properly virtualized (particularly, attribute selectors).

> If this happens to the input to querySelector, then if no selectors remain, a syntax error occurs because we pass the empty string to the browser querySelector. If one or more selectors make it through, the client will receive an unexpectedly shorter list. This behavior is overly inconsistent. Instead:

----

Submitted @r5475

[kpreid2, 2013-07-02 21:55:32]

https://codereview.appspot.com/10892043/diff/1/src/com/google/caja/plugin/san...
File src/com/google/caja/plugin/sanitizecss.js (right):

https://codereview.appspot.com/10892043/diff/1/src/com/google/caja/plugin/san...
src/com/google/caja/plugin/sanitizecss.js:320: *     When a "compound" or
"complex" selector
Why is this not "When a selector..."? Attribute selectors may be untranslatable
and are themselves neither complex nor compound according to the definitions
given.

https://codereview.appspot.com/10892043/diff/1/src/com/google/caja/plugin/san...
src/com/google/caja/plugin/sanitizecss.js:321: *     (see
dev.w3.org/csswg/selectors4/#structure)
Please add http:// so that this is more programmatically recognizable as a URL.

https://codereview.appspot.com/10892043/diff/1/src/com/google/caja/plugin/san...
src/com/google/caja/plugin/sanitizecss.js:359: // affect others.
...except as directed by opt_onUntranslatableSelector.

https://codereview.appspot.com/10892043/diff/1/src/com/google/caja/plugin/san...
src/com/google/caja/plugin/sanitizecss.js:535: // We disallow absolute positions
relative to html.
Is this special case necessary any longer? Mentions of "html" will be either
virtualized or (in a future iframe-using world) entirely correct. If there is a
browser bug this avoids, please mention it.

Also, I notice that elSelector is never assigned anything but '', so this code
is broken — please look at whether this is indicating some further confusion.

[MikeSamuel, 2013-07-03 20:49:08]

https://code.google.com/p/google-caja/issues/detail?id=1669

> Our CSS selector filter deletes selectors it does not understand, which cannot
be virtualized accurately, or which are not yet properly virtualized
(particularly, attribute selectors).

> If this happens to the input to querySelector, then if no selectors remain, a
syntax error occurs because we pass the empty string to the browser
querySelector. If one or more selectors make it through, the client will receive
an unexpectedly shorter list. This behavior is overly inconsistent. Instead:

[MikeSamuel, 2013-07-03 20:49:23]

https://codereview.appspot.com/10892043/diff/1/src/com/google/caja/plugin/san...
File src/com/google/caja/plugin/sanitizecss.js (right):

https://codereview.appspot.com/10892043/diff/1/src/com/google/caja/plugin/san...
src/com/google/caja/plugin/sanitizecss.js:320: *     When a "compound" or
"complex" selector
On 2013/07/02 21:55:32, kpreid2 wrote:
> Why is this not "When a selector..."? Attribute selectors may be
untranslatable
> and are themselves neither complex nor compound according to the definitions
> given.

It was only relevant during an earlier snapshot.  Changed to just "a selector".

https://codereview.appspot.com/10892043/diff/1/src/com/google/caja/plugin/san...
src/com/google/caja/plugin/sanitizecss.js:321: *     (see
dev.w3.org/csswg/selectors4/#structure)
On 2013/07/02 21:55:32, kpreid2 wrote:
> Please add http:// so that this is more programmatically recognizable as a
URL.

Removed entirely since no explanation of "complex" vs "compound" is necessary
here.

https://codereview.appspot.com/10892043/diff/1/src/com/google/caja/plugin/san...
src/com/google/caja/plugin/sanitizecss.js:359: // affect others.
On 2013/07/02 21:55:32, kpreid2 wrote:
> ...except as directed by opt_onUntranslatableSelector.

Done.

https://codereview.appspot.com/10892043/diff/1/src/com/google/caja/plugin/san...
src/com/google/caja/plugin/sanitizecss.js:535: // We disallow absolute positions
relative to html.
On 2013/07/02 21:55:32, kpreid2 wrote:
> Is this special case necessary any longer? Mentions of "html" will be either
> virtualized or (in a future iframe-using world) entirely correct. If there is
a
> browser bug this avoids, please mention it.
> 
> Also, I notice that elSelector is never assigned anything but '', so this code
> is broken — please look at whether this is indicating some further confusion.

Good catch.  We {c,sh}ould replace 'html' and other virtualized elements with
appropriate class references.  I'd rather do that in a separate CL though.
Do we have a list of those elsewhere?

[kpreid2, 2013-07-03 20:58:51]

https://codereview.appspot.com/10892043/diff/1/src/com/google/caja/plugin/san...
File src/com/google/caja/plugin/sanitizecss.js (right):

https://codereview.appspot.com/10892043/diff/1/src/com/google/caja/plugin/san...
src/com/google/caja/plugin/sanitizecss.js:535: // We disallow absolute positions
relative to html.
On 2013/07/03 20:49:23, MikeSamuel wrote:
> Good catch.  We {c,sh}ould replace 'html' and other virtualized elements with
> appropriate class references.  I'd rather do that in a separate CL though.
> Do we have a list of those elsewhere?

That's already handled by the call to tagPolicy — so at this point, 'element'
has already been rewritten. If it wasn't, we would be obviously broken.

And, to reiterate, once we support targeting a real document (such that we don't
virtualize the <html> element), it would be incorrect to reject a 'html >'
selector.

This condition should be deleted.

[MikeSamuel, 2013-07-03 21:31:51]

https://code.google.com/p/google-caja/issues/detail?id=1669

> Our CSS selector filter deletes selectors it does not understand, which cannot
be virtualized accurately, or which are not yet properly virtualized
(particularly, attribute selectors).

> If this happens to the input to querySelector, then if no selectors remain, a
syntax error occurs because we pass the empty string to the browser
querySelector. If one or more selectors make it through, the client will receive
an unexpectedly shorter list. This behavior is overly inconsistent. Instead:

[MikeSamuel, 2013-07-03 21:48:59]

https://code.google.com/p/google-caja/issues/detail?id=1669

> Our CSS selector filter deletes selectors it does not understand, which cannot
be virtualized accurately, or which are not yet properly virtualized
(particularly, attribute selectors).

> If this happens to the input to querySelector, then if no selectors remain, a
syntax error occurs because we pass the empty string to the browser
querySelector. If one or more selectors make it through, the client will receive
an unexpectedly shorter list. This behavior is overly inconsistent. Instead:

[MikeSamuel, 2013-07-03 22:05:39]

https://codereview.appspot.com/10892043/diff/1/src/com/google/caja/plugin/san...
File src/com/google/caja/plugin/sanitizecss.js (right):

https://codereview.appspot.com/10892043/diff/1/src/com/google/caja/plugin/san...
src/com/google/caja/plugin/sanitizecss.js:535: // We disallow absolute positions
relative to html.
On 2013/07/03 20:58:52, kpreid2 wrote:
> On 2013/07/03 20:49:23, MikeSamuel wrote:
> > Good catch.  We {c,sh}ould replace 'html' and other virtualized elements
with
> > appropriate class references.  I'd rather do that in a separate CL though.
> > Do we have a list of those elsewhere?
> 
> That's already handled by the call to tagPolicy — so at this point, 'element'
> has already been rewritten. If it wasn't, we would be obviously broken.
> 
> And, to reiterate, once we support targeting a real document (such that we
don't
> virtualize the <html> element), it would be incorrect to reject a 'html >'
> selector.
> 
> This condition should be deleted.

Done.

https://codereview.appspot.com/10892043/diff/6002/src/com/google/caja/parser/...
File src/com/google/caja/parser/js/ExpressionStmt.java (right):

https://codereview.appspot.com/10892043/diff/6002/src/com/google/caja/parser/...
src/com/google/caja/parser/js/ExpressionStmt.java:67: || startsWithRegex(e)) {
Some of the statements of the form

   if (c) {
     if (/foo/.test(x)) {
       valid = false;
     } else {
       bar = baz;
     }
   } ...

were getting minified to

   if (c)/foo/.test(x)?(valid=false):bar=baz;

which is valid, but which Rhino chokes on, causing JsHtmlSanitizerTest to fail
on a parse error in html-css-sanitizer-minified.js 

This additional change makes our rendered JS Rhino-safe.

[kpreid2, 2013-07-03 22:09:33]

LGTM++

https://codereview.appspot.com/10892043/diff/6002/src/com/google/caja/parser/...
File src/com/google/caja/parser/js/ExpressionStmt.java (right):

https://codereview.appspot.com/10892043/diff/6002/src/com/google/caja/parser/...
src/com/google/caja/parser/js/ExpressionStmt.java:85: // literal.
Blank line after here, or make the // one block instead of three, so it looks
less like only this comment is associated with the code.

[MikeSamuel, 2013-07-04 01:32:23]

https://codereview.appspot.com/10892043/diff/6002/src/com/google/caja/parser/...
File src/com/google/caja/parser/js/ExpressionStmt.java (right):

https://codereview.appspot.com/10892043/diff/6002/src/com/google/caja/parser/...
src/com/google/caja/parser/js/ExpressionStmt.java:85: // literal.
On 2013/07/03 22:09:33, kpreid2 wrote:
> Blank line after here, or make the // one block instead of three, so it looks
> less like only this comment is associated with the code.

Done.

[MikeSamuel, 2013-07-04 01:32:52]

https://code.google.com/p/google-caja/issues/detail?id=1669

> Our CSS selector filter deletes selectors it does not understand, which cannot
be virtualized accurately, or which are not yet properly virtualized
(particularly, attribute selectors).

> If this happens to the input to querySelector, then if no selectors remain, a
syntax error occurs because we pass the empty string to the browser
querySelector. If one or more selectors make it through, the client will receive
an unexpectedly shorter list. This behavior is overly inconsistent. Instead: