Tests for Cajita taming functions

Adds tests and reworks the Rhino test runner to make it a bit easier to
write the tests.

  * Changed RhinoTestBed.runJsUnittestFromHtml to take an HTML tree,
    and to properly find script elements in that HTML using the
    HTML parser, and to cajole script elements that have "use strict,cajita";.
  * Added an HTML test TamingTest.java/taming_test.html.
    This test currently breaks if the IGNORE_KNOWN_FAILURES flag is set to
    false.


Submitted @3348

[MarkM, 2009-03-15 04:15:06]

Some initial reactions. Review not done yet.

http://codereview.appspot.com/28045/diff/1/7
File tests/com/google/caja/parser/quasiliteral/taming_test.html (right):

http://codereview.appspot.com/28045/diff/1/7#newcode35
Line 35: // untrusted code does allows it to retrieve an even number.
What an odd idea.

http://codereview.appspot.com/28045/diff/1/7#newcode36
Line 36: var MyRootNamespace = function () {};
It is even odder (sorry) that you chose to make MyRootNamespace be a function
with visible mutable property. Also, you're freezing it, as you must since it is
a function, but you're not marking it as a constructor or simple-function. This
makes clearer than ever our need to document our taming API. I would like to
claim this combination of decisions is an invalid taming, but there's currently
no basis for that claim.

Would it suit your purposes here equally well for MyRootNamespace to be a
constructed object instead of a function? (Even then, we should question whether
this combination of freeze and mutability is a valid taming.)

http://codereview.appspot.com/28045/diff/1/7#newcode47
Line 47: </script>
Why the separation into separate script blocks?

http://codereview.appspot.com/28045/diff/1/7#newcode59
Line 59: return i < 0 ? i & ~1 : i | 1;
This doesn't work at all on negative numbers. Any reason not to simple "return i
| 1;"?

http://codereview.appspot.com/28045/diff/1/7#newcode70
Line 70: ___.grantCall(MyRootNamespace.SubSpace, 'safeToCall');
Another taming API issue I need to document: Outside of cajita.js, you should
probably not use grantCall by itself. (Yes, perhaps that means cajita.js should
stop exporting it.) Instead, you should use a combination that determines what
value is read if the method property is read rather than called. In this case,
it is safe to treat the value of safeToCall as a simple-function, so you should
probably use grantFunc.

http://codereview.appspot.com/28045/diff/1/7#newcode75
Line 75: </script>
Again, why the separation into separate script blocks?

http://codereview.appspot.com/28045/diff/1/7#newcode114
Line 114: assertTrue('public in', false, publicDataKey in MyRootNamespace);
Why should this be false? AFAICT, it is correct for this test to fail.

http://codereview.appspot.com/28045/diff/1/7#newcode125
Line 125: assertFalse('private in', false, privateDataKey in MyRootNamespace);
Unnecessary double negatives are hard to understand. Is your intention
equivalently captured by 

   assertTrue('private in', true, privateDataKey in MyRootNamespace);

? Again, I think you may have a parity error. "privateDataKey in
MyRootNamespace" should be false.

[MikeSamuel, 2009-03-15 23:40:07]

http://codereview.appspot.com/28045/diff/1/7
File tests/com/google/caja/parser/quasiliteral/taming_test.html (right):

http://codereview.appspot.com/28045/diff/1/7#newcode36
Line 36: var MyRootNamespace = function () {};
On 2009/03/15 04:15:06, MarkM wrote:
> It is even odder (sorry) that you chose to make MyRootNamespace be a function
> with visible mutable property. Also, you're freezing it, as you must since it
is
> a function, but you're not marking it as a constructor or simple-function.
This
> makes clearer than ever our need to document our taming API. I would like to
> claim this combination of decisions is an invalid taming, but there's
currently
> no basis for that claim.

Ok.  My understanding is that functions are used as namespaces in the opensocial
code.

Why is not making it simple or a constructor a violation?
It's used as a namespace, not meant to be called.


> Would it suit your purposes here equally well for MyRootNamespace to be a
> constructed object instead of a function? (Even then, we should question
whether
> this combination of freeze and mutability is a valid taming.)

I think it would be good to test constructed objects as well.

http://codereview.appspot.com/28045/diff/1/7#newcode47
Line 47: </script>
On 2009/03/15 04:15:06, MarkM wrote:
> Why the separation into separate script blocks?

No real reason.

In the opensocial case, the code to tame is defined separately from the taming
decisions since it is most often used without caja.

The container setup is separable from those two.

And the cajoled code has to be separate since it is transformed independently of
the others.

http://codereview.appspot.com/28045/diff/1/7#newcode59
Line 59: return i < 0 ? i & ~1 : i | 1;
On 2009/03/15 04:15:06, MarkM wrote:
> This doesn't work at all on negative numbers. Any reason not to simple "return
i
> | 1;"?

Yep.  Brain fart.

http://codereview.appspot.com/28045/diff/1/7#newcode70
Line 70: ___.grantCall(MyRootNamespace.SubSpace, 'safeToCall');
On 2009/03/15 04:15:06, MarkM wrote:
> Another taming API issue I need to document: Outside of cajita.js, you should
> probably not use grantCall by itself. (Yes, perhaps that means cajita.js
should
> stop exporting it.) Instead, you should use a combination that determines what
> value is read if the method property is read rather than called. In this case,
> it is safe to treat the value of safeToCall as a simple-function, so you
should
> probably use grantFunc.

Will do.

http://codereview.appspot.com/28045/diff/1/7#newcode114
Line 114: assertTrue('public in', false, publicDataKey in MyRootNamespace);
On 2009/03/15 04:15:06, MarkM wrote:
> Why should this be false? AFAICT, it is correct for this test to fail.

This is a typo.  There should only be two parameters.

http://codereview.appspot.com/28045/diff/1/7#newcode125
Line 125: assertFalse('private in', false, privateDataKey in MyRootNamespace);
On 2009/03/15 04:15:06, MarkM wrote:
> Unnecessary double negatives are hard to understand. Is your intention
> equivalently captured by 
> 
>    assertTrue('private in', true, privateDataKey in MyRootNamespace);
> 
> ? Again, I think you may have a parity error. "privateDataKey in
> MyRootNamespace" should be false.

Yep.  There should only be two parameters.

http://codereview.appspot.com/28045/diff/1/7#newcode148
Line 148: assertTrue('mutable in', false, mutableDataKey in MyRootNamespace);
Need to fix.

[MarkM, 2009-03-16 19:45:43]

http://codereview.appspot.com/28045/diff/1/7
File tests/com/google/caja/parser/quasiliteral/taming_test.html (right):

http://codereview.appspot.com/28045/diff/1/7#newcode36
Line 36: var MyRootNamespace = function () {};
On 2009/03/15 23:40:07, MikeSamuel wrote:
> Ok.  My understanding is that functions are used as namespaces in the
opensocial
> code.
> 
> Why is not making it simple or a constructor a violation?
> It's used as a namespace, not meant to be called.

Allowing Caja code to obtain a direct reference to a function it may not call is
equivalent to what we were calling an "outer hull breach" during the security
review. If the outcome of that review had made us sufficiently confident in our
inner hull that we'd be willing to rely on it alone, then this would be fine.
However, the outcome was almost exactly the opposite. Our outer hull (preventing
uncallable functions from being reachable) was quite strong and our inner hull
(preventing reachable uncallable functions from being called) was quite weak.
The difference was great enough that it remains unclear whether the complexity
of the inner hull pays for its complexity or whether we should rely on the outer
hull alone.

Although we have repaired all the identified inner hull breaches identified
during the review, we do not yet have reasons to become confident in it.

[MikeSamuel, 2009-03-17 03:47:30]

Snapshotted and added tests for JSON.  All tests run green now even without
IGNORE_KNOWN_FAILURES.

http://codereview.appspot.com/28045/diff/1/7
File tests/com/google/caja/parser/quasiliteral/taming_test.html (right):

http://codereview.appspot.com/28045/diff/1/7#newcode36
Line 36: var MyRootNamespace = function () {};
It talked to Jas.  I was wrong about the namespace being a function.  It's a
function that is replaced with a JSON object via some currently-not-understood
process.

[MikeSamuel, 2009-03-17 03:52:25]

http://codereview.appspot.com/28045/diff/1021/1037
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/28045/diff/1021/1037#newcode1642
Line 1642: // an enumerable but unreadable property?
Ignore this.  It should not and does not, since readPub returns undefined for
unreadable properties.  The only case an exception will happen is if a getter
exits abnormally.

[MarkM, 2009-03-17 22:49:38]

Modulo one bug and one run-on sentence, LGTM.

http://codereview.appspot.com/28045/diff/1021/1037
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/28045/diff/1021/1037#newcode1043
Line 1043: // TODO(mikesamuel): none of the other grants grant enum, is "Only"
operable?
No. My fault. This is a holdover from when we also had a grantEnum/fastpathEnum
which bundled in a grantRead/fastpathRead. Since we no longer do, we should
simplify the name.

http://codereview.appspot.com/28045/diff/1021/1037#newcode1642
Line 1642: // an enumerable but unreadable property?
On 2009/03/17 03:52:25, MikeSamuel wrote:
> Ignore this.  

ok

http://codereview.appspot.com/28045/diff/1021/1037#newcode3280
Line 3280: // TODO(mikesamuel): should grantCall be exported?
Probably not.

http://codereview.appspot.com/28045/diff/1021/1027
File tests/com/google/caja/parser/quasiliteral/taming_test.html (right):

http://codereview.appspot.com/28045/diff/1021/1027#newcode52
Line 52: // We test that the function is not callable since it is not granted
This is a rather massive run-on sentence. Perhaps you've been reading too much
Hayek? ;)

http://codereview.appspot.com/28045/diff/1021/1027#newcode142
Line 142: var safeToCallName = 'safeToCall';
Why are some of these fooName and others fooKey?

http://codereview.appspot.com/28045/diff/1021/1027#newcode242
Line 242: assertEquals(void 0, MyRootNamespace.JsonObj.not_readable);
This test passes for the wrong reason. "not_readable" should end in a double
underscore to catch what was intended. Please check all occurrences.

[MikeSamuel, 2009-03-19 05:43:45]

http://codereview.appspot.com/28045/diff/1021/1027
File tests/com/google/caja/parser/quasiliteral/taming_test.html (right):

http://codereview.appspot.com/28045/diff/1021/1027#newcode52
Line 52: // We test that the function is not callable since it is not granted
On 2009/03/17 22:49:39, MarkM wrote:
> This is a rather massive run-on sentence. Perhaps you've been reading too much
> Hayek? ;)

Done.

http://codereview.appspot.com/28045/diff/1021/1027#newcode142
Line 142: var safeToCallName = 'safeToCall';
On 2009/03/17 22:49:39, MarkM wrote:
> Why are some of these fooName and others fooKey?

Done.

http://codereview.appspot.com/28045/diff/1021/1027#newcode242
Line 242: assertEquals(void 0, MyRootNamespace.JsonObj.not_readable);
On 2009/03/17 22:49:39, MarkM wrote:
> This test passes for the wrong reason. "not_readable" should end in a double
> underscore to catch what was intended. Please check all occurrences.

Done.

[MarkM, 2009-03-19 07:48:53]

LGTM