A applet-free testbed

A caja testbed that uses the cajoling service instead of an applet to perform the cajoling.
(See http://10.latest.caja.appspot.com for an example of it running).
* Adds a series of examples of the attacks caja prevents and interactions it supports
* Adds gwt libraries needed to use the front-end during development
* The changes to Normalizer are needed to handle the varing availability of java's Normalizer depending on the version of Java and on AppEngine

[Jasvir, 2009-09-10 21:38:48]

Sorry - this is not ready for review yet.

[ihab.awad, 2009-10-09 02:04:47]

http://codereview.appspot.com/116069/diff/2001/2029
File build.xml (right):

http://codereview.appspot.com/116069/diff/2001/2029#newcode58
Line 58: <!-- servlet dir - needs to be called "war" -->
It *needs* to be called "war"? Wow. Please add a little explanation of why --
i.e., is that a requirement of the <dev_appserver> invoked later on...?

http://codereview.appspot.com/116069/diff/2001/2029#newcode65
Line 65: <import
file="${ant.file.appengine-ant-macros}/config/user/ant-macros.xml"/>
Do you mean ${macros.appengine}? This looks like an artifact of development....
And please line wrap.

http://codereview.appspot.com/116069/diff/2001/2029#newcode115
Line 115: <pathelement path="${third_party}/java/gwt/gwt-dev-mac.jar"/>
Will gwt-dev-mac.jar work on (say) Linux?

http://codereview.appspot.com/116069/diff/2001/2029#newcode633
Line 633: <java failonerror="true" fork="true"
classname="com.google.gwt.dev.Compiler">
Please line wrap here & elsewhere to the extent possible.

http://codereview.appspot.com/116069/diff/2001/2029#newcode663
Line 663: todir="${war}/WEB-INF/classes"
Are the third_party jars that the Caja code relies on not also copied into
WEB-INF/lib?

Also, is a WEB-INF/web.xml generated anywhere? How?

http://codereview.appspot.com/116069/diff/2001/2029#newcode675
Line 675: description="Starts the development server.">
Should specify depends="" to build the server?

http://codereview.appspot.com/116069/diff/2001/2018
File src/com/google/caja/demos/playground/client/CajolingService.java (right):

http://codereview.appspot.com/116069/diff/2001/2018#newcode12
Line 12: public interface CajolingService extends RemoteService {
So this caused a name conflict with The Class Formerly Known As CajolingService
[TM]. Hmm.... This class is really the main facade for the Playground to talk
back to its server, so PlaygroundBackendService may not be too specific.

http://codereview.appspot.com/116069/diff/2001/2018#newcode17
Line 17: * @return Returns the cajoled html, js and cajoling messages as an
array
s/Returns//g.

Also, not clear from this comment that you mean:

  return[0] -> html
  return[1] -> js
  return[2 .. (return.length - 1)] -> messages

http://codereview.appspot.com/116069/diff/2001/2016
File src/com/google/caja/demos/playground/client/CajolingServiceAsync.java
(right):

http://codereview.appspot.com/116069/diff/2001/2016#newcode20
Line 20: */
Doc comment format ... and missing @author.

http://codereview.appspot.com/116069/diff/2001/2016#newcode25
Line 25: 
Extra blank line.

http://codereview.appspot.com/116069/diff/2001/2017
File src/com/google/caja/demos/playground/client/CajolingServiceResult.java
(right):

http://codereview.appspot.com/116069/diff/2001/2017#newcode29
Line 29: public CajolingServiceResult(String html, String javascript, String[]
messages) {
Line wrap.

http://codereview.appspot.com/116069/diff/2001/2015
File src/com/google/caja/demos/playground/client/Playground.java (right):

http://codereview.appspot.com/116069/diff/2001/2015#newcode27
Line 27: 
Extra blank line.

http://codereview.appspot.com/116069/diff/2001/2015#newcode28
Line 28: private PlaygroundView gui;
If the view always passed in callbacks (or listened for events, should you need
them), the model would never have to retain an explicit upward pointer to its
view.

http://codereview.appspot.com/116069/diff/2001/2015#newcode34
Line 34: cajolingService.fetch(url, new AsyncCallback<String>() {
Can you push the CPS all the way through, i.e., the view can send the callback
to the model, rather than having the model do direct upcalls to the view?

For a system this small, this is an optional issue....

http://codereview.appspot.com/116069/diff/2001/2015#newcode60
Line 60: gui.setRenderedResult(result[0]);
Did you mean result[1]?

http://codereview.appspot.com/116069/diff/2001/2015#newcode61
Line 61: for (int i=2; i < result.length - 2; i++) {
Spaces around "=".

http://codereview.appspot.com/116069/diff/2001/2019
File src/com/google/caja/demos/playground/client/ui/PlaygroundView.java (right):

http://codereview.appspot.com/116069/diff/2001/2019#newcode57
Line 57: 
Extra blank line.

http://codereview.appspot.com/116069/diff/2001/2019#newcode66
Line 66: public void setVersion(String v) {
Probably unnecessary if you adopt Model/Delegate and pass a callback directly to
the Model.

http://codereview.appspot.com/116069/diff/2001/2019#newcode70
Line 70: public void selectTab(int index) {
At the very least, the indices used here should be named constants somewhere.

http://codereview.appspot.com/116069/diff/2001/2019#newcode78
Line 78: for (Menu menu: Menu.values()) {
Space around ":".

http://codereview.appspot.com/116069/diff/2001/2019#newcode96
Line 96: logoPanel.add(new
Image("http://cajadores.com/demos/testbed/caja_logo_small.png"));
Line wrap.

http://codereview.appspot.com/116069/diff/2001/2019#newcode108
Line 108: public void onFocus(FocusEvent event) {
Don't you wish Java had concise closures? :)

http://codereview.appspot.com/116069/diff/2001/2019#newcode165
Line 165: private DecoratedTabPanel createEditorPanel() {
You do realize are dangerously close to implementing the Eclipse UI in GWT, my
friend.... };->

http://codereview.appspot.com/116069/diff/2001/2019#newcode234
Line 234: public PlaygroundView(Playground controller) {
Oh I see, so you're doing MVC rather than Model/Delegate:

  http://c2.com/cgi/wiki?ModelDelegate

Is MVC more common than Model/Delegate for GWT programming?

In any case, I'm not sure this usage actually fits with classic MVC. There, the
View sends requests to the Controller, the Controller sends commands to the
Model, and the View connects directly to the Model to update its state. The
Controller does not maintain a back-pointer to the View.

And then -- the realization that the Controller is either logic that should be
in the View, or stuff that belongs in the Model, leads us to bundle View and
Controller together and get Model/Delegate.

http://codereview.appspot.com/116069/diff/2001/2019#newcode262
Line 262: System.out.println("Output:<"+prettyPrint(result)+">\n");
Spaces around "+"

http://codereview.appspot.com/116069/diff/2001/2019#newcode268
Line 268: }-*/;
Wow, native. Cool!

http://codereview.appspot.com/116069/diff/2001/2019#newcode270
Line 270: public void setRenderedResult(String result) {
Again, probably not necessary (at least not as a *public* method) if you adopt
Model/Delegate and pass callbacks to the model.

http://codereview.appspot.com/116069/diff/2001/2019#newcode270
Line 270: public void setRenderedResult(String result) {
So ... how do cajita.js and domita.js and whatever else get actually included in
the GWT-served HTML content? Is there a container HTML page that did not get
included in this CL?

http://codereview.appspot.com/116069/diff/2001/2019#newcode282
Line 282: "    var imports = ___.copy(___.sharedImports);\n" +
Continuation indent.

http://codereview.appspot.com/116069/diff/2001/2019#newcode286
Line 286: "    attachDocumentStub('-g___', { rewrite: function() {return null;}
}, imports, gadgetRoot);\n" +
Line wrap.

http://codereview.appspot.com/116069/diff/2001/2019#newcode289
Line 289: htmlAndJs[1].substring(0, htmlAndJs[1].length() - 9);
Perhaps pull the removal of the ending </script> into the regex above, rather
than hiding it here. :)

http://codereview.appspot.com/116069/diff/2001/2014
File src/com/google/caja/demos/playground/server/GWTCajolingServiceImpl.java
(right):

http://codereview.appspot.com/116069/diff/2001/2014#newcode40
Line 40: implements CajolingService {
Indent 2 more spaces.

http://codereview.appspot.com/116069/diff/2001/2014#newcode52
Line 52: + "&mime-type=" + URLEncoder.encode(mimeType, "UTF-8"));
We point URIs to the cajoling service? Can we point them back to the current GWT
server instead?

If that's too much trouble (requiring an entire cajoling service to be built
into the GWT thingey) then please just add a TODO.

http://codereview.appspot.com/116069/diff/2001/2014#newcode119
Line 119: public String fetch(String url) {
Is this used anywhere?

http://codereview.appspot.com/116069/diff/2001/2012
File src/com/google/caja/service/DefaultCajolingService.java (right):

http://codereview.appspot.com/116069/diff/2001/2012#newcode50
Line 50: public class DefaultCajolingService extends HttpServlet {
Not sure about this rename ... see comments on the new CajolingService.

http://codereview.appspot.com/116069/diff/2001/2012#newcode93
Line 93: public void doPost(HttpServletRequest req, HttpServletResponse resp)
Per our discussion, see the doPost I added.

http://codereview.appspot.com/116069/diff/2001/2002
File src/log4j.properties (right):

http://codereview.appspot.com/116069/diff/2001/2002#newcode1
Line 1: # Configure the console as our one appender
This seems identical to the one in third_party/java/appengine/. Was it your
intention to add it directly to src/ as well, or should this be copied in via a
build rule?

http://codereview.appspot.com/116069/diff/2001/2027
File third_party/java/appengine/config/user/ant-macros.xml (right):

http://codereview.appspot.com/116069/diff/2001/2027#newcode1
Line 1: <!--
The third_party/java/appengine/ and third_party/java/gwt/ dirs are missing a
README and LICENSE.

(Re the weird placement of this comment -- it needs to be attached to some text
file line in order for the code review tool to accept it.)

http://codereview.appspot.com/116069/diff/2001/2027#newcode1
Line 1: <!--
I notice gwt-dev-mac.jar. Should there also be other gwt-dev-${platform}.jar
checked in for multiplatform development?

[ihab.awad, 2009-11-25 00:08:01]

I patched this in and tried it; I got --

BUILD FAILED
/google-caja/build.xml:129: The following error occurred while executing this
line:
.../google-caja/third_party/java/appengine/config/user/ant-macros.xml:83:
taskdef class com.google.appengine.tools.enhancer.EnhancerTask cannot be found

[Jasvir Nagra, 2009-11-25 00:43:59]

This is because the patch includes binary checkins to third_party which are
not captured by the code review tool.

On Tue, Nov 24, 2009 at 4:08 PM, <ihab.awad@gmail.com> wrote:

> I patched this in and tried it; I got --
>
> BUILD FAILED
> /google-caja/build.xml:129: The following error occurred while executing
> this line:
> .../google-caja/third_party/java/appengine/config/user/ant-macros.xml:83:
> taskdef class com.google.appengine.tools.enhancer.EnhancerTask cannot be
> found
>
>
>
> http://codereview.appspot.com/116069
>

[Jasvir, 2009-11-26 01:36:58]

Snapshot

* build copies all the necessary libraries to WEB-INF
* myvn creates a .project such that if cajadevs have appengine installed they
can run and debug the playground from within eclipse.

http://codereview.appspot.com/116069/diff/2001/2029
File build.xml (right):

http://codereview.appspot.com/116069/diff/2001/2029#newcode58
build.xml:58: <!-- servlet dir - needs to be called "war" -->
On 2009/10/09 02:04:47, ihab.awad wrote:
> It *needs* to be called "war"? Wow. Please add a little explanation of why --
> i.e., is that a requirement of the <dev_appserver> invoked later on...?

Done.

http://codereview.appspot.com/116069/diff/2001/2018
File src/com/google/caja/demos/playground/client/CajolingService.java (right):

http://codereview.appspot.com/116069/diff/2001/2018#newcode12
src/com/google/caja/demos/playground/client/CajolingService.java:12: public
interface CajolingService extends RemoteService {
Renamed to PlaygroundService.

On 2009/10/09 02:04:47, ihab.awad wrote:
> So this caused a name conflict with The Class Formerly Known As
CajolingService
> [TM]. Hmm.... This class is really the main facade for the Playground to talk
> back to its server, so PlaygroundBackendService may not be too specific.

http://codereview.appspot.com/116069/diff/2001/2018#newcode17
src/com/google/caja/demos/playground/client/CajolingService.java:17: * @return
Returns the cajoled html, js and cajoling messages as an array
On 2009/10/09 02:04:47, ihab.awad wrote:
> s/Returns//g.
> 
> Also, not clear from this comment that you mean:
> 
>   return[0] -> html
>   return[1] -> js
>   return[2 .. (return.length - 1)] -> messages

Done.

http://codereview.appspot.com/116069/diff/2001/2016
File src/com/google/caja/demos/playground/client/CajolingServiceAsync.java
(right):

http://codereview.appspot.com/116069/diff/2001/2016#newcode20
src/com/google/caja/demos/playground/client/CajolingServiceAsync.java:20: */
On 2009/10/09 02:04:47, ihab.awad wrote:
> Doc comment format ... and missing @author.

Done.

http://codereview.appspot.com/116069/diff/2001/2015
File src/com/google/caja/demos/playground/client/Playground.java (right):

http://codereview.appspot.com/116069/diff/2001/2015#newcode61
src/com/google/caja/demos/playground/client/Playground.java:61: for (int i=2; i
< result.length - 2; i++) {
On 2009/10/09 02:04:47, ihab.awad wrote:
> Spaces around "=".

Done.

http://codereview.appspot.com/116069/diff/2001/2019
File src/com/google/caja/demos/playground/client/ui/PlaygroundView.java (right):

http://codereview.appspot.com/116069/diff/2001/2019#newcode57
src/com/google/caja/demos/playground/client/ui/PlaygroundView.java:57: 
On 2009/10/09 02:04:47, ihab.awad wrote:
> Extra blank line.

Done.

http://codereview.appspot.com/116069/diff/2001/2019#newcode70
src/com/google/caja/demos/playground/client/ui/PlaygroundView.java:70: public
void selectTab(int index) {
On 2009/10/09 02:04:47, ihab.awad wrote:
> At the very least, the indices used here should be named constants somewhere.

Done.

http://codereview.appspot.com/116069/diff/2001/2019#newcode78
src/com/google/caja/demos/playground/client/ui/PlaygroundView.java:78: for (Menu
menu: Menu.values()) {
On 2009/10/09 02:04:47, ihab.awad wrote:
> Space around ":".

Done.

http://codereview.appspot.com/116069/diff/2001/2019#newcode96
src/com/google/caja/demos/playground/client/ui/PlaygroundView.java:96:
logoPanel.add(new
Image("http://cajadores.com/demos/testbed/caja_logo_small.png"));
On 2009/10/09 02:04:47, ihab.awad wrote:
> Line wrap.

Done.

http://codereview.appspot.com/116069/diff/2001/2019#newcode108
src/com/google/caja/demos/playground/client/ui/PlaygroundView.java:108: public
void onFocus(FocusEvent event) {
On 2009/10/09 02:04:47, ihab.awad wrote:
> Don't you wish Java had concise closures? :)

Done.

http://codereview.appspot.com/116069/diff/2001/2019#newcode165
src/com/google/caja/demos/playground/client/ui/PlaygroundView.java:165: private
DecoratedTabPanel createEditorPanel() {
On 2009/10/09 02:04:47, ihab.awad wrote:
> You do realize are dangerously close to implementing the Eclipse UI in GWT, my
> friend.... };->
...all in good time :)

http://codereview.appspot.com/116069/diff/2001/2019#newcode262
src/com/google/caja/demos/playground/client/ui/PlaygroundView.java:262:
System.out.println("Output:<"+prettyPrint(result)+">\n");
Debugging.  Removed.

On 2009/10/09 02:04:47, ihab.awad wrote:
> Spaces around "+"

http://codereview.appspot.com/116069/diff/2001/2019#newcode268
src/com/google/caja/demos/playground/client/ui/PlaygroundView.java:268: }-*/;
On 2009/10/09 02:04:47, ihab.awad wrote:
> Wow, native. Cool!

Done.

http://codereview.appspot.com/116069/diff/2001/2019#newcode270
src/com/google/caja/demos/playground/client/ui/PlaygroundView.java:270: public
void setRenderedResult(String result) {
See Playground.html - it was missed from the CL.

On 2009/10/09 02:04:47, ihab.awad wrote:
> So ... how do cajita.js and domita.js and whatever else get actually included
in
> the GWT-served HTML content? Is there a container HTML page that did not get
> included in this CL?

http://codereview.appspot.com/116069/diff/2001/2019#newcode282
src/com/google/caja/demos/playground/client/ui/PlaygroundView.java:282: "    var
imports = ___.copy(___.sharedImports);\n" +
On 2009/10/09 02:04:47, ihab.awad wrote:
> Continuation indent.

Done.

http://codereview.appspot.com/116069/diff/2001/2019#newcode286
src/com/google/caja/demos/playground/client/ui/PlaygroundView.java:286: "   
attachDocumentStub('-g___', { rewrite: function() {return null;} }, imports,
gadgetRoot);\n" +
On 2009/10/09 02:04:47, ihab.awad wrote:
> Line wrap.

Done.

http://codereview.appspot.com/116069/diff/2001/2019#newcode289
src/com/google/caja/demos/playground/client/ui/PlaygroundView.java:289:
htmlAndJs[1].substring(0, htmlAndJs[1].length() - 9);
On 2009/10/09 02:04:47, ihab.awad wrote:
> Perhaps pull the removal of the ending </script> into the regex above, rather
> than hiding it here. :)

Done.

http://codereview.appspot.com/116069/diff/2001/2014
File src/com/google/caja/demos/playground/server/GWTCajolingServiceImpl.java
(right):

http://codereview.appspot.com/116069/diff/2001/2014#newcode40
src/com/google/caja/demos/playground/server/GWTCajolingServiceImpl.java:40:
implements CajolingService {
On 2009/10/09 02:04:47, ihab.awad wrote:
> Indent 2 more spaces.

Done.

http://codereview.appspot.com/116069/diff/2001/2014#newcode52
src/com/google/caja/demos/playground/server/GWTCajolingServiceImpl.java:52: +
"&mime-type=" + URLEncoder.encode(mimeType, "UTF-8"));
On 2009/10/09 02:04:47, ihab.awad wrote:
> We point URIs to the cajoling service? Can we point them back to the current
GWT
> server instead?
> 
> If that's too much trouble (requiring an entire cajoling service to be built
> into the GWT thingey) then please just add a TODO.

Done.

http://codereview.appspot.com/116069/diff/2001/2014#newcode119
src/com/google/caja/demos/playground/server/GWTCajolingServiceImpl.java:119:
public String fetch(String url) {
No.  Elided.

On 2009/10/09 02:04:47, ihab.awad wrote:
> Is this used anywhere?

http://codereview.appspot.com/116069/diff/2001/2012
File src/com/google/caja/service/DefaultCajolingService.java (right):

http://codereview.appspot.com/116069/diff/2001/2012#newcode93
src/com/google/caja/service/DefaultCajolingService.java:93: public void
doPost(HttpServletRequest req, HttpServletResponse resp)
On 2009/10/09 02:04:47, ihab.awad wrote:
> Per our discussion, see the doPost I added.

Done.

http://codereview.appspot.com/116069/diff/2001/2002
File src/log4j.properties (right):

http://codereview.appspot.com/116069/diff/2001/2002#newcode1
src/log4j.properties:1: # Configure the console as our one appender
On 2009/10/09 02:04:47, ihab.awad wrote:
> This seems identical to the one in third_party/java/appengine/. Was it your
> intention to add it directly to src/ as well, or should this be copied in via
a
> build rule?

Removed.

[Jasvir, 2009-12-01 23:54:50]

ping

[ihab.awad, 2009-12-03 22:53:26]

LGTM -- cool! :)

http://codereview.appspot.com/116069/diff/15069/14004
File appengine-web.xml (right):

http://codereview.appspot.com/116069/diff/15069/14004#newcode3
appengine-web.xml:3: <application>caja</application>
Indentation gorp.

http://codereview.appspot.com/116069/diff/15069/14071
File build.xml (right):

http://codereview.appspot.com/116069/diff/15069/14071#newcode761
build.xml:761: <java failonerror="true" fork="true"
classname="com.google.gwt.dev.Compiler">
Line wrap (& some other places too)

http://codereview.appspot.com/116069/diff/15069/14071#newcode797
build.xml:797: <replacevalue>${build.svnVersion}</replacevalue>
Should this task depend on "build.info" then?

http://codereview.appspot.com/116069/diff/15069/14006
File tests/com/google/caja/service/TestHttpServletRequest.java (right):

http://codereview.appspot.com/116069/diff/15069/14006#newcode39
tests/com/google/caja/service/TestHttpServletRequest.java:39: import
javax.servlet.http.HttpServletRequestWrapper;
Unused import

http://codereview.appspot.com/116069/diff/15069/14070
File web.xml (right):

http://codereview.appspot.com/116069/diff/15069/14070#newcode19
web.xml:19: </servlet>
What serves the static files then?

[Jasvir, 2009-12-10 22:57:42]

http://codereview.appspot.com/116069/diff/15069/14071
File build.xml (right):

http://codereview.appspot.com/116069/diff/15069/14071#newcode761
build.xml:761: <java failonerror="true" fork="true"
classname="com.google.gwt.dev.Compiler">
On 2009/12/03 22:53:27, ihab.awad wrote:
> Line wrap (& some other places too)

Done.

http://codereview.appspot.com/116069/diff/15069/14071#newcode797
build.xml:797: <replacevalue>${build.svnVersion}</replacevalue>
Yes it should.  Added.

On 2009/12/03 22:53:27, ihab.awad wrote:
> Should this task depend on "build.info" then?

http://codereview.appspot.com/116069/diff/15069/14006
File tests/com/google/caja/service/TestHttpServletRequest.java (right):

http://codereview.appspot.com/116069/diff/15069/14006#newcode39
tests/com/google/caja/service/TestHttpServletRequest.java:39: import
javax.servlet.http.HttpServletRequestWrapper;
On 2009/12/03 22:53:27, ihab.awad wrote:
> Unused import

Done.

http://codereview.appspot.com/116069/diff/15069/14070
File web.xml (right):

http://codereview.appspot.com/116069/diff/15069/14070#newcode19
web.xml:19: </servlet>
Files in ant-war but outside ant-war/WEB-INF are served statically.

On 2009/12/03 22:53:27, ihab.awad wrote:
> What serves the static files then?