Allow 'content' property values in CSS

Allow 'content' property values in CSS

[ihab.awad, 2013-02-12 17:17:36]

Not yet ready for review.

[Jasvir Nagra, 2013-02-12 18:17:36]

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/lang/css/Cs...
File src/com/google/caja/lang/css/CssPropertyPatterns.java (right):

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/lang/css/Cs...
src/com/google/caja/lang/css/CssPropertyPatterns.java:549: .put("z-index",
CssPropBit.QUANTITY)
need to add relevant definitions here for open-quote, string etc.

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/lang/css/cs...
File src/com/google/caja/lang/css/css-extensions-defs.json (right):

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/lang/css/cs...
src/com/google/caja/lang/css/css-extensions-defs.json:163: { "key": "content",
you don't need to specify this here - it's already in css21-defs.

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/lang/css/cs...
src/com/google/caja/lang/css/css-extensions-defs.json:164: "signature": "normal
| none | [ <string>  | open-quote | close-quote | no-open-quote | no-close-quote
]+ ",
you need to tell CssPropertyPatterns what open-quote, close-quote, string etc
mean.  That's why you're getting cssPropBits = 0 right now.

[Jasvir Nagra, 2013-02-12 18:22:55]

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/lang/css/Cs...
File src/com/google/caja/lang/css/CssPropertyPatterns.java (right):

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/lang/css/Cs...
src/com/google/caja/lang/css/CssPropertyPatterns.java:304: return null;
It might be a good time to here and in the rest of the *ToPattern family, to
throw instead of quietly ignoring patterns we don't recognize.  The input to
CssPropertyPatterns is always a file known at compile time - these shouldn't
ever have unrecognized patterns in them.

[ihab.awad, 2013-02-13 05:18:29]

Now ready for review.

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/lang/css/Cs...
File src/com/google/caja/lang/css/CssPropertyPatterns.java (right):

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/lang/css/Cs...
src/com/google/caja/lang/css/CssPropertyPatterns.java:304: return null;
It ends up being a bit of a pain to do since lots of these functions are
intersnakeified and rely on one another to return 'null' at times. I think we
should defer to a different CL.

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/lang/css/Cs...
src/com/google/caja/lang/css/CssPropertyPatterns.java:549: .put("z-index",
CssPropBit.QUANTITY)
On 2013/02/12 18:17:36, Jasvir Nagra wrote:
> need to add relevant definitions here for open-quote, string etc.

Done.

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/lang/css/cs...
File src/com/google/caja/lang/css/css-extensions-defs.json (right):

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/lang/css/cs...
src/com/google/caja/lang/css/css-extensions-defs.json:163: { "key": "content",
On 2013/02/12 18:17:36, Jasvir Nagra wrote:
> you don't need to specify this here - it's already in css21-defs.

I'm a bit confused. The stuff in css21-defs is just the basic stuff per the W3C
spec. We have to override with our own stuff somewhere. My impression is that
the 'signature' here is overriding the default one, making it narrower, because
of what we support. Is there a better place to put this? Should there be?

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/lang/css/cs...
src/com/google/caja/lang/css/css-extensions-defs.json:164: "signature": "normal
| none | [ <string>  | open-quote | close-quote | no-open-quote | no-close-quote
]+ ",
On 2013/02/12 18:17:36, Jasvir Nagra wrote:
> you need to tell CssPropertyPatterns what open-quote, close-quote, string etc
> mean.  That's why you're getting cssPropBits = 0 right now.

Done.

[Jasvir Nagra, 2013-02-13 17:40:07]

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/lang/css/cs...
File src/com/google/caja/lang/css/css-extensions-defs.json (right):

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/lang/css/cs...
src/com/google/caja/lang/css/css-extensions-defs.json:163: { "key": "content",
You have it in the right place - I overlooked attr.

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/parser/Pars...
File src/com/google/caja/parser/ParseTreeNodes.java (right):

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/parser/Pars...
src/com/google/caja/parser/ParseTreeNodes.java:41: ctx.relevantKeys =
Collections.singleton(CssValidator.CSS_PROPERTY_PART_TYPE);
wrap.

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

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/plugin/sani...
src/com/google/caja/plugin/sanitizecss.js:147: // IHAB FOO
cruft

[ihab.awad, 2013-02-13 17:45:29]

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/lang/css/cs...
File src/com/google/caja/lang/css/css-extensions-defs.json (right):

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/lang/css/cs...
src/com/google/caja/lang/css/css-extensions-defs.json:163: { "key": "content",
On 2013/02/13 17:40:07, Jasvir Nagra wrote:
> You have it in the right place - I overlooked attr.

Done.

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/parser/Pars...
File src/com/google/caja/parser/ParseTreeNodes.java (right):

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/parser/Pars...
src/com/google/caja/parser/ParseTreeNodes.java:41: ctx.relevantKeys =
Collections.singleton(CssValidator.CSS_PROPERTY_PART_TYPE);
On 2013/02/13 17:40:07, Jasvir Nagra wrote:
> wrap.

Debug code, was deleted.

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

https://codereview.appspot.com/7308092/diff/1/src/com/google/caja/plugin/sani...
src/com/google/caja/plugin/sanitizecss.js:147: // IHAB FOO
On 2013/02/13 17:40:07, Jasvir Nagra wrote:
> cruft

Debug code, was deleted.

[ihab.awad, 2013-02-13 17:46:10]

Reassigning review to kpreid.

[ihab.awad, 2013-02-13 17:46:14]

Reassigning review to kpreid.

[kpreid2, 2013-02-13 20:26:59]

https://codereview.appspot.com/7308092/diff/6002/src/com/google/caja/lang/css...
File src/com/google/caja/lang/css/css-extensions-defs.json (right):

https://codereview.appspot.com/7308092/diff/6002/src/com/google/caja/lang/css...
src/com/google/caja/lang/css/css-extensions-defs.json:169: "Reject
attr(<identifier>) because it circumvents Domado attribute value
virtualizations",
This is in principle statically determinable in some cases (though it would need
context from the selector). Agreed we don't need it now, though.

https://codereview.appspot.com/7308092/diff/6002/src/com/google/caja/plugin/s...
File src/com/google/caja/plugin/sanitizecss.js (right):

https://codereview.appspot.com/7308092/diff/6002/src/com/google/caja/plugin/s...
src/com/google/caja/plugin/sanitizecss.js:404: return null;
This means that an unknown pseudo-thing now returns null immediately rather than
falling down to the if (start === end) case. Is that an appropriate _pattern_ in
this code?

https://codereview.appspot.com/7308092/diff/6002/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/browser-test-case.js (right):

https://codereview.appspot.com/7308092/diff/6002/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/browser-test-case.js:461: getComputedStyleOnPseudo:
function (tameNode, pseudoElement, styleProp) {
This appears to not handle the pseudo-element in the node.currentStyle case; at
*least* leave a comment to that affect.

Too much duplicated code; please refactor to add an optional parameter to
.getComputedStyle instead.

https://codereview.appspot.com/7308092/diff/6002/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/css-stylesheet-tests.js (right):

https://codereview.appspot.com/7308092/diff/6002/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/css-stylesheet-tests.js:121: +
"text-decoration:underline}"
Test is no longer testing what it is intended to test for per its name.
Suggestion: Change this to the properly-removed example
    content: url(http://example.com/img);
and add another test for that a string value is accepted.

https://codereview.appspot.com/7308092/diff/6002/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-domado-special-guest.html (right):

https://codereview.appspot.com/7308092/diff/6002/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-domado-special-guest.html:660: <p
id="testCssContent" class="testcontainer">testCssContent</p>
I see no reason for this to be in special-guest rather than dom-guest.

https://codereview.appspot.com/7308092/diff/6002/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-domado-special-guest.html:676: return
s.substring(1, s.length - 1);
return /^(['"])(.*)\1$/.exec(s)[2];

[ihab.awad, 2013-02-13 22:20:30]

https://codereview.appspot.com/7308092/diff/6002/src/com/google/caja/lang/css...
File src/com/google/caja/lang/css/css-extensions-defs.json (right):

https://codereview.appspot.com/7308092/diff/6002/src/com/google/caja/lang/css...
src/com/google/caja/lang/css/css-extensions-defs.json:169: "Reject
attr(<identifier>) because it circumvents Domado attribute value
virtualizations",
On 2013/02/13 20:26:59, kpreid2 wrote:
> This is in principle statically determinable in some cases (though it would
need
> context from the selector). Agreed we don't need it now, though.

Done.

https://codereview.appspot.com/7308092/diff/6002/src/com/google/caja/plugin/s...
File src/com/google/caja/plugin/sanitizecss.js (right):

https://codereview.appspot.com/7308092/diff/6002/src/com/google/caja/plugin/s...
src/com/google/caja/plugin/sanitizecss.js:404: return null;
On 2013/02/13 20:26:59, kpreid2 wrote:
> This means that an unknown pseudo-thing now returns null immediately rather
than
> falling down to the if (start === end) case. Is that an appropriate _pattern_
in
> this code?

If (++start) is not executed, then (start !== end) and this function *will*
return null in any case. And note the pre-existing conditional at l.396 which
punts by returning null in a similar "unrecognized" case. But I rewrote (at the
expense of a bit of repetitiveness) and hope it looks clearer that way.

https://codereview.appspot.com/7308092/diff/6002/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/browser-test-case.js (right):

https://codereview.appspot.com/7308092/diff/6002/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/browser-test-case.js:461: getComputedStyleOnPseudo:
function (tameNode, pseudoElement, styleProp) {
On 2013/02/13 20:26:59, kpreid2 wrote:
> This appears to not handle the pseudo-element in the node.currentStyle case;
at
> *least* leave a comment to that affect.
> 
> Too much duplicated code; please refactor to add an optional parameter to
> .getComputedStyle instead.

Ok. So I'm rewriting into a form which fails if a pseudo element is provided and
we're on a browser (ie8--) which doesn't have window.getComputedStyle. This is
similar to what we do in bridal.js:

http://code.google.com/p/google-caja/source/browse/trunk/src/com/google/caja/...

https://codereview.appspot.com/7308092/diff/6002/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/css-stylesheet-tests.js (right):

https://codereview.appspot.com/7308092/diff/6002/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/css-stylesheet-tests.js:121: +
"text-decoration:underline}"
Ah, that's a good catch. I just patched to make it work.

Fixed; see if you like it the new way. :)

https://codereview.appspot.com/7308092/diff/6002/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-domado-special-guest.html (right):

https://codereview.appspot.com/7308092/diff/6002/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-domado-special-guest.html:660: <p
id="testCssContent" class="testcontainer">testCssContent</p>
On 2013/02/13 20:26:59, kpreid2 wrote:
> I see no reason for this to be in special-guest rather than dom-guest.

That's a good point; I don't do any special host-level setup. Changing.

https://codereview.appspot.com/7308092/diff/6002/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-domado-special-guest.html:676: return
s.substring(1, s.length - 1);
On 2013/02/13 20:26:59, kpreid2 wrote:
> return /^(['"])(.*)\1$/.exec(s)[2];

With all due respect, I'm not sure that's clearer. :)

[ihab.awad, 2013-02-13 22:20:39]

@5286

[ihab.awad, 2013-02-13 22:23:03]

Oops, sorry Kevin -- I somehow had in my mind that the changes were lgtm++.
Please look at this and I'll make any necessary changes if there are any.

[kpreid2, 2013-02-13 22:27:48]

LGTM except as noted.

https://codereview.appspot.com/7308092/diff/6002/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-domado-special-guest.html (right):

https://codereview.appspot.com/7308092/diff/6002/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-domado-special-guest.html:676: return
s.substring(1, s.length - 1);
On 2013/02/13 22:20:30, ihab.awad wrote:
> On 2013/02/13 20:26:59, kpreid2 wrote:
> > return /^(['"])(.*)\1$/.exec(s)[2];
> 
> With all due respect, I'm not sure that's clearer. :)

Not clearer -- more paranoid. Please make this change or an equivalent one
rather than ignoring syntax pieces of the input.

[ihab.awad, 2013-02-13 22:36:03]

On 2013/02/13 22:27:48, kpreid2 wrote:
> LGTM except as noted.
> 
>
https://codereview.appspot.com/7308092/diff/6002/tests/com/google/caja/plugin...
> File tests/com/google/caja/plugin/es53-test-domado-special-guest.html (right):
> 
>
https://codereview.appspot.com/7308092/diff/6002/tests/com/google/caja/plugin...
> tests/com/google/caja/plugin/es53-test-domado-special-guest.html:676: return
> s.substring(1, s.length - 1);
> On 2013/02/13 22:20:30, ihab.awad wrote:
> > On 2013/02/13 20:26:59, kpreid2 wrote:
> > > return /^(['"])(.*)\1$/.exec(s)[2];
> > 
> > With all due respect, I'm not sure that's clearer. :)
> 
> Not clearer -- more paranoid. Please make this change or an equivalent one
> rather than ignoring syntax pieces of the input.

Will fix in an upcoming rev asap.