Photon and Service improvements

[Jasvir, 2010-09-29 06:20:02]

http://codereview.appspot.com/2206045/diff/3001/build.xml
File build.xml (right):

http://codereview.appspot.com/2206045/diff/3001/build.xml#newcode607
build.xml:607: <!-- Copy the test files to make them available for the script
module
This makes demos depend on tests?

http://codereview.appspot.com/2206045/diff/3001/src/com/google/caja/cajita-mo...
File src/com/google/caja/cajita-module.js (right):

http://codereview.appspot.com/2206045/diff/3001/src/com/google/caja/cajita-mo...
src/com/google/caja/cajita-module.js:193: var jsonpCallbackCount = 0;
Please move this closer to the single place of use.

http://codereview.appspot.com/2206045/diff/1039/src/com/google/caja/plugin/ur...
File src/com/google/caja/plugin/uri.js (right):

http://codereview.appspot.com/2206045/diff/1039/src/com/google/caja/plugin/ur...
src/com/google/caja/plugin/uri.js:381: newPort = Number(newPort);
Indentation is wonky.

For my info, curious what threat you were concerned about here.  Does the next
line not subsume the cast to number?

http://codereview.appspot.com/2206045/diff/1039/src/com/google/caja/service/C...
File src/com/google/caja/service/CajolingService.java (right):

http://codereview.appspot.com/2206045/diff/1039/src/com/google/caja/service/C...
src/com/google/caja/service/CajolingService.java:198:
System.err.println(render(mq));
Please don't print to stdout/err in services.

http://codereview.appspot.com/2206045/diff/1039/src/com/google/caja/service/I...
File src/com/google/caja/service/InnocentHandler.java (left):

http://codereview.appspot.com/2206045/diff/1039/src/com/google/caja/service/I...
src/com/google/caja/service/InnocentHandler.java:1: // Copyright 2008 Google
Inc. All Rights Reserved.
Ah sadness.  This change does eliminate a useful service - previously a host
page that loaded scripts via a script src tag could use the cajoling service to
innocentify scripts by proxying.  Is there any reason the innocent code
transformer cannot be supported via jsonp?

http://codereview.appspot.com/2206045/diff/1039/tests/com/google/caja/plugin/...
File tests/com/google/caja/plugin/DomitaTest.java (right):

http://codereview.appspot.com/2206045/diff/1039/tests/com/google/caja/plugin/...
tests/com/google/caja/plugin/DomitaTest.java:71: poll(20000, 1000, new Check() {
:(

[ihab.awad, 2010-10-01 20:44:09]

http://codereview.appspot.com/2206045/diff/1039/src/com/google/caja/plugin/ur...
File src/com/google/caja/plugin/uri.js (right):

http://codereview.appspot.com/2206045/diff/1039/src/com/google/caja/plugin/ur...
src/com/google/caja/plugin/uri.js:381: newPort = Number(newPort);
On 2010/09/29 06:20:02, jasvir wrote:
> Indentation is wonky.

Fixed.

> For my info, curious what threat you were concerned about here.  Does
> the next line not subsume the cast to number?

Well ... it _does_ fail safe. :P

The problem is that 'newPort' is idiomatically a string. (newPort & ...) is a
number. Even in the success case, they are _always_ !== to each other.

http://codereview.appspot.com/2206045/diff/1039/src/com/google/caja/service/C...
File src/com/google/caja/service/CajolingService.java (right):

http://codereview.appspot.com/2206045/diff/1039/src/com/google/caja/service/C...
src/com/google/caja/service/CajolingService.java:198:
System.err.println(render(mq));
On 2010/09/29 06:20:02, jasvir wrote:
> Please don't print to stdout/err in services.

That was debug cruft. Bobbitted.

http://codereview.appspot.com/2206045/diff/1039/src/com/google/caja/service/I...
File src/com/google/caja/service/InnocentHandler.java (left):

http://codereview.appspot.com/2206045/diff/1039/src/com/google/caja/service/I...
src/com/google/caja/service/InnocentHandler.java:1: // Copyright 2008 Google
Inc. All Rights Reserved.
On 2010/09/29 06:20:02, jasvir wrote:
> Ah sadness.  This change does eliminate a useful service - previously a host
> page that loaded scripts via a script src tag could use the cajoling service
to
> innocentify scripts by proxying.  Is there any reason the innocent code
> transformer cannot be supported via jsonp?

Ok, reinstating.

http://codereview.appspot.com/2206045/diff/1039/tests/com/google/caja/plugin/...
File tests/com/google/caja/plugin/DomitaTest.java (right):

http://codereview.appspot.com/2206045/diff/1039/tests/com/google/caja/plugin/...
tests/com/google/caja/plugin/DomitaTest.java:71: poll(20000, 1000, new Check() {
On 2010/09/29 06:20:02, jasvir wrote:
> :(

Yeah ... for some reason, stuff now takes a little bit longer. :P

[ihab.awad, 2010-10-01 20:45:10]

Actually, Jas, could you please hang on ... one more detail I need to fix before
you take another look. Thanks! -- I

[ihab.awad, 2010-10-01 21:14:45]

Ok, ready now!

[Jasvir, 2010-10-08 17:37:49]

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/cajita-m...
File src/com/google/caja/cajita-module.js (right):

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/cajita-m...
src/com/google/caja/cajita-module.js:195: var jsonpCallbackCount = 0;
Please move this closer to its single place of use

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/service/...
File src/com/google/caja/service/AbstractCajolingHandler.java (right):

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/service/...
src/com/google/caja/service/AbstractCajolingHandler.java:172:
output.append(jsonpCallback + "(");
This is vulnerable - jsonpCallback is never checked to be just a function
identifier and can be arbitrary js.

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/service/...
File src/com/google/caja/service/CajaArguments.java (right):

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/service/...
src/com/google/caja/service/CajaArguments.java:40:
JSONP_CALLBACK("jsonp-callback"),
We can call this whatever we like but it might be worthwhile calling is
"callback" to be compatible with other gdata feeds..

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/service/...
src/com/google/caja/service/CajaArguments.java:41: 
In this or in a separate CL, alt=json and alt=json-in-script as alternatives to
input-mime-type.

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/service/...
File src/com/google/caja/service/CajolingServlet.java (right):

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/service/...
src/com/google/caja/service/CajolingServlet.java:153:
resp.setStatus(HttpServletResponse.SC_OK);
Ok.  Response is always ok if the cajoler ran and output json contains the
needed errors.

[ihab.awad, 2010-10-10 20:21:37]

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/cajita-m...
File src/com/google/caja/cajita-module.js (right):

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/cajita-m...
src/com/google/caja/cajita-module.js:195: var jsonpCallbackCount = 0;
On 2010/10/08 17:37:49, jasvir wrote:
> Please move this closer to its single place of use

Done.

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/service/...
File src/com/google/caja/service/AbstractCajolingHandler.java (right):

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/service/...
src/com/google/caja/service/AbstractCajolingHandler.java:172:
output.append(jsonpCallback + "(");
On 2010/10/08 17:37:49, jasvir wrote:
> This is vulnerable - jsonpCallback is never checked to be just a function
> identifier and can be arbitrary js.

That is the intention. We are emitting un-cajoled code to a client who wishes to
make their entire page vulnerable to our service. Our only responsibility is to
assure maximum fidelity to whatever it is they give us. Parsing the callback for
correctness would not meaningfully reduce the attack surface, anyway, because
the result would *still* be a piece of code that calls an arbitrary top-level
function which, in the "wrong" hands, is already dangerous enough.

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/service/...
File src/com/google/caja/service/CajaArguments.java (right):

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/service/...
src/com/google/caja/service/CajaArguments.java:40:
JSONP_CALLBACK("jsonp-callback"),
On 2010/10/08 17:37:49, jasvir wrote:
> We can call this whatever we like but it might be worthwhile calling is
> "callback" to be compatible with other gdata feeds..

Done.

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/service/...
src/com/google/caja/service/CajaArguments.java:41: 
On 2010/10/08 17:37:49, jasvir wrote:
> In this or in a separate CL, alt=json and alt=json-in-script as alternatives
to
> input-mime-type.

I assume you mean as an alternative to the now-defunct "output-mime-type"? But
we *always* produce JSON now....

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/service/...
File src/com/google/caja/service/CajolingServlet.java (right):

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/service/...
src/com/google/caja/service/CajolingServlet.java:153:
resp.setStatus(HttpServletResponse.SC_OK);
On 2010/10/08 17:37:49, jasvir wrote:
> Ok.  Response is always ok if the cajoler ran and output json contains the
> needed errors. 

Done.

[ihab.awad, 2010-10-14 23:24:17]

Snapshotted. Now with new de-XSSificationerific goodness.

[Jasvir, 2010-10-18 22:20:21]

LGTM

The changes to make this CL more compatible with Gdata can happen in a separate
change.  I haven't reviewed the photon/ part (other than noting that it is in
fact awesome).

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/service/...
File src/com/google/caja/service/CajaArguments.java (right):

http://codereview.appspot.com/2206045/diff/21001/src/com/google/caja/service/...
src/com/google/caja/service/CajaArguments.java:41: 
No we produce either json or jsonp.  In this CL, the difference depends on
whether a &callback= was defined.  The usual gdata convention is alt=json or
alt=json-in-script decides whether the response is json or jsonp and the
callback=XXX defines what the callback function is.  If the callback function is
unspecified it defaults to gadgets.io.handleScriptLoad (See
http://code.google.com/apis/gdata/javadoc/com/google/gdata/client/GDataProtoc...)

On 2010/10/10 20:21:37, ihab.awad wrote:
> On 2010/10/08 17:37:49, jasvir wrote:
> > In this or in a separate CL, alt=json and alt=json-in-script as alternatives
> to
> > input-mime-type.
> 
> I assume you mean as an alternative to the now-defunct "output-mime-type"? But
> we *always* produce JSON now....

http://codereview.appspot.com/2206045/diff/30001/src/com/google/caja/service/...
File src/com/google/caja/service/AbstractCajolingHandler.java (right):

http://codereview.appspot.com/2206045/diff/30001/src/com/google/caja/service/...
src/com/google/caja/service/AbstractCajolingHandler.java:212: throw new
RuntimeException("Detected XSS attempt; aborting request");
Please throw SomethingWidgeyHappenedErrors for Caja specific runtime exceptions.

[ihab.awad, 2010-10-20 22:06:16]

@4305