Fix vulnerability in server compilation of ES5/3 style attributes

http://code.google.com/p/google-caja/issues/detail?id=1459

Patch set 4 is to be applied to Caja r4860 for a fix to responsible disclosure clients.

[felix8a, 2012-04-20 23:19:32]

three comments, but all basically the same issue.  other than that looks fine.

http://codereview.appspot.com/6094044/diff/3001/src/com/google/caja/plugin/te...
File src/com/google/caja/plugin/templates/HtmlAttributeRewriter.java (right):

http://codereview.appspot.com/6094044/diff/3001/src/com/google/caja/plugin/te...
src/com/google/caja/plugin/templates/HtmlAttributeRewriter.java:255:
dynamicValue = jsValue;
jsValue here is an array, not a string.  this should always be an array of one
element here, so I think you can fold this into the previous case.

http://codereview.appspot.com/6094044/diff/3001/tests/com/google/caja/plugin/...
File tests/com/google/caja/plugin/es53-test-client-uri-rewriting.js (right):

http://codereview.appspot.com/6094044/diff/3001/tests/com/google/caja/plugin/...
tests/com/google/caja/plugin/es53-test-client-uri-rewriting.js:190:
jsunitPass(testName);
I'd be more comfortable if you also had some type of an end-to-end test.  right
now you're testing that the url gets to the callback, but you're not testing
that the rewritten url ends up in the html correctly.

http://codereview.appspot.com/6094044/diff/3001/tests/com/google/caja/plugin/...
File tests/com/google/caja/plugin/templates/TemplateCompilerTest.java (right):

http://codereview.appspot.com/6094044/diff/3001/tests/com/google/caja/plugin/...
tests/com/google/caja/plugin/templates/TemplateCompilerTest.java:812: + "   
emitter___.setAttr(el___, 'style', ["
note, this result may not actually work, because setAttr expects a string not an
array.

[ihab.awad, 2012-04-23 05:22:57]

http://codereview.appspot.com/6094044/diff/3001/src/com/google/caja/plugin/te...
File src/com/google/caja/plugin/templates/HtmlAttributeRewriter.java (right):

http://codereview.appspot.com/6094044/diff/3001/src/com/google/caja/plugin/te...
src/com/google/caja/plugin/templates/HtmlAttributeRewriter.java:255:
dynamicValue = jsValue;
On 2012/04/20 23:19:32, felix8a wrote:
> jsValue here is an array, not a string.  this should always be an array of one
> element here, so I think you can fold this into the previous case.

Done.

http://codereview.appspot.com/6094044/diff/3001/tests/com/google/caja/plugin/...
File tests/com/google/caja/plugin/es53-test-client-uri-rewriting.js (right):

http://codereview.appspot.com/6094044/diff/3001/tests/com/google/caja/plugin/...
tests/com/google/caja/plugin/es53-test-client-uri-rewriting.js:190:
jsunitPass(testName);
On 2012/04/20 23:19:32, felix8a wrote:
> I'd be more comfortable if you also had some type of an end-to-end test. 
right
> now you're testing that the url gets to the callback, but you're not testing
> that the rewritten url ends up in the html correctly.

Done.

http://codereview.appspot.com/6094044/diff/3001/tests/com/google/caja/plugin/...
File tests/com/google/caja/plugin/templates/TemplateCompilerTest.java (right):

http://codereview.appspot.com/6094044/diff/3001/tests/com/google/caja/plugin/...
tests/com/google/caja/plugin/templates/TemplateCompilerTest.java:812: + "   
emitter___.setAttr(el___, 'style', ["
On 2012/04/20 23:19:32, felix8a wrote:
> note, this result may not actually work, because setAttr expects a string not
an
> array.

Done.

[felix8a, 2012-04-23 05:58:26]

lgtm