replace some minified js with closured js (issue 6257051)

[fel...@gmail.com]

Reviewers: Jasvir, metaweta,

Description:
This replaces caja-minified.js and html-sanitizer-minified.js with
versions compiled using Closure's advanced optimizations option.

This is non-scary because all the relevant tests run fine with the
closured versions.

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

Affected files:
M build.xml
M src/com/google/caja/lang/html/HtmlDefinitions.java
M src/com/google/caja/plugin/caja.js
M src/com/google/caja/plugin/html-sanitizer.js
M src/com/google/caja/plugin/sanitizecss.js
M src/com/google/caja/tools/ClosureCompiler.java
M src/com/google/caja/tools/TransformAntTask.java
M tests/com/google/caja/plugin/GeneralBrowserTest.java
M tests/com/google/caja/plugin/JsHtmlSanitizerTest.java
M tests/com/google/caja/plugin/browser-test-case.html
A + tests/com/google/caja/plugin/html-css-sanitizer-minified-test.html
A + tests/com/google/caja/plugin/html-sanitizer-minified-test.html
M tests/com/google/caja/plugin/test-index.js

[meta...@gmail.com]

I'm unclear on what the criteria are for needing quoted properties as
opposed to dotted properties.

http://codereview.appspot.com/6257051/

[fel...@gmail.com]

On 2012/05/24 23:42:33, metaweta wrote:
> I'm unclear on what the criteria are for needing quoted properties as
opposed to
> dotted properties.

It's about external linkage. When you say o.foo, then closure's
advanced optimization assumes that it can rename .foo to a shorter name,
because it can identify all references to things of the same type as o.
This works fine when Closure is given all the js to compile together.
But when you're compiling a module used by others, then you want to
prevent Closure from doing that renaming.

All the elements in the html4 object are used by random other js code
that isn't compiled together in the same unit. Several of them are used
by URI policies, so container code needs to refer to them. Others are
used by domado, which is not safe to compile yet.

http://codereview.appspot.com/6257051/

[meta...@gmail.com]

lgtm

http://codereview.appspot.com/6257051/

[eri...@gmail.com]

> This is non-scary because all the relevant tests run fine
> with the closured versions.

That's a joke right? (Unless we're assuming that attackers will only
write attacks we're already testing ;).)

http://codereview.appspot.com/6257051/

[fel...@gmail.com]

On 2012/05/25 00:04:01, MarkM wrote:
> > This is non-scary because all the relevant tests run fine
> > with the closured versions.

> That's a joke right? (Unless we're assuming that attackers will only
write
> attacks we're already testing ;).)

heh, yes.

caja-minified.js seems to me unproblematic since it doesn't do much
itself that's security-critical, all the security guarantees of caja are
elsewhere.

html-sanitizer-minified.js is more trouble since it does implement
security guarantees, but many of our tests for that are security tests,
and there are places that are already closure-izing html-sanitizer, so I
think we might as bake that into our build/test process.

If we have serious concerns about the potential of Closure to break
security guarantees, I can maintain the distinction between
foo-minified.js and foo-closured.js, and then it's up to the
container/integrator to make that risk choice, but I think that's asking
too much from container implementers, we should handle that risk
ourselves.


http://codereview.appspot.com/6257051/