Restrict numeric whitelisting to nonnegative integers.

* Changed CajitaRewriter.java and Rule.java to whitelist expressions of the
  form @o[@s & -1 <<< 1] instead of @o[+@s]

* Changed cajita.js to check that index > 0 when allowing numeric reads

* Added tests to CajitaRewriterTest and domita_test_untrusted.html to
  check that neither Rhino nor the browser return anything but undefined
  on reading a negative index.

* Updated structural tests in CajitaTest.java

* Fixes bug 1093

[MikeSamuel, 2009-07-31 03:05:28]

In the CL comment you say > 0 instead of >= 0.  Cajita.js uses the correct range
check, but can you fix the CL comment for posterity.

[MikeSamuel, 2009-07-31 03:17:59]

Thanks for getting this out so quickly.

http://codereview.appspot.com/96200/diff/1/6
File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (right):

http://codereview.appspot.com/96200/diff/1/6#newcode1051
Line 1051: synopsis="Recognize that nonnegative integer indexing is inherently
safe.",
Maybe use the term "array index" since that is a term defined in the language
specification that does not include negative integers.

15.4 Array Objects
     Array objects give special treatment to a certain
     class of property names. A property name P (in the
     form of a string value) is an array index if and only
     if ToString(ToUint32(P)) is equal to P and ToUint32(P)
     is not equal to 2**32-1

http://codereview.appspot.com/96200/diff/1/6#newcode1081
Line 1081: Number indexValue = ((NumberLiteral)index).getValue();
Need space after cast.

http://codereview.appspot.com/96200/diff/1/6#newcode1082
Line 1082: if (indexValue.longValue() >= 0 &&
Please operate on either one representation consistently.

I think indexValue.doubleValue() >= 0 would be better since it won't admit NaN.

http://codereview.appspot.com/96200/diff/1/6#newcode1083
Line 1083: indexValue.doubleValue() == Math.floor(indexValue.doubleValue())) {
Long line.  Maybe pull the double value out into a variable since you're using
the double value three times.

http://codereview.appspot.com/96200/diff/1/5
File src/com/google/caja/parser/quasiliteral/Rule.java (right):

http://codereview.appspot.com/96200/diff/1/5#newcode601
Line 601: key = (Expression) QuasiBuilder.substV("@key&-1>>>1", "key", key);
Maybe parenthesize the rushift.  It's correct but less readable and the renderer
will get rid of unnecessary parentheses.

http://codereview.appspot.com/96200/diff/1/2
File tests/com/google/caja/CajitaTest.java (right):

http://codereview.appspot.com/96200/diff/1/2#newcode29
Line 29: "assertEquals((new RegExp('.'))[-1], void 0);");
What is this testing?  Unless we do some gimickry to set RegExp.prototype[5] for
Rhino, this will spuriously pass.

I think Ihab likes these tests to be in the same order as the rules.

http://codereview.appspot.com/96200/diff/1/4
File tests/com/google/caja/plugin/domita_test_untrusted.html (right):

http://codereview.appspot.com/96200/diff/1/4#newcode2480
Line 2480: assertEquals((new RegExp('.'))[-1], void 0);
cool

[metaweta, 2009-07-31 15:56:04]

On 2009/07/31 03:17:59, MikeSamuel wrote:
> Thanks for getting this out so quickly.

Thanks for jumping in to review it.

> http://codereview.appspot.com/96200/diff/1/6
> File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (right):
> 
> http://codereview.appspot.com/96200/diff/1/6#newcode1051
> Line 1051: synopsis="Recognize that nonnegative integer indexing is inherently
> safe.",
> Maybe use the term "array index" since that is a term defined in the language
> specification that does not include negative integers.

Done.

> 
> 15.4 Array Objects
>      Array objects give special treatment to a certain
>      class of property names. A property name P (in the
>      form of a string value) is an array index if and only
>      if ToString(ToUint32(P)) is equal to P and ToUint32(P)
>      is not equal to 2**32-1
> 
> http://codereview.appspot.com/96200/diff/1/6#newcode1081
> Line 1081: Number indexValue = ((NumberLiteral)index).getValue();
> Need space after cast.

Done.

> http://codereview.appspot.com/96200/diff/1/6#newcode1082
> Line 1082: if (indexValue.longValue() >= 0 &&
> Please operate on either one representation consistently.
> 
> I think indexValue.doubleValue() >= 0 would be better since it won't admit
NaN.

Done.

> 
> http://codereview.appspot.com/96200/diff/1/6#newcode1083
> Line 1083: indexValue.doubleValue() == Math.floor(indexValue.doubleValue())) {
> Long line.  Maybe pull the double value out into a variable since you're using
> the double value three times.

Done.

> http://codereview.appspot.com/96200/diff/1/5
> File src/com/google/caja/parser/quasiliteral/Rule.java (right):
> 
> http://codereview.appspot.com/96200/diff/1/5#newcode601
> Line 601: key = (Expression) QuasiBuilder.substV("@key&-1>>>1", "key", key);
> Maybe parenthesize the rushift.  It's correct but less readable and the
renderer
> will get rid of unnecessary parentheses.

Done.

> 
> http://codereview.appspot.com/96200/diff/1/2
> File tests/com/google/caja/CajitaTest.java (right):
> 
> http://codereview.appspot.com/96200/diff/1/2#newcode29
> Line 29: "assertEquals((new RegExp('.'))[-1], void 0);");
> What is this testing?  

Added the same comment from domita_test_untrusted.  Changed it to loop over
negative indices down to -10 and check functions, regexps, and the arguments
object.  

> Unless we do some gimickry to set RegExp.prototype[5] for
> Rhino, this will spuriously pass.

The idea for putting this test into CajitaTest was just to make sure Rhino never
does this in the future.  I guess there are all kinds of things that someone
might do to break stuff, and since even Firefox thinks it's ugly cruft with no
justification, testing this weird case here may not be worthwhile.

> I think Ihab likes these tests to be in the same order as the rules.
> 

If it were in CajitaRewriterTest, that would make sense.  This file just tests
that various things work cajoled in Cajita mode--I can't see that putting it
anywhere else in the file makes more sense.

> http://codereview.appspot.com/96200/diff/1/4
> File tests/com/google/caja/plugin/domita_test_untrusted.html (right):
> 
> http://codereview.appspot.com/96200/diff/1/4#newcode2480
> Line 2480: assertEquals((new RegExp('.'))[-1], void 0);
> cool

[metaweta, 2009-07-31 15:58:44]

On 2009/07/31 15:56:04, metaweta wrote:
> > I think Ihab likes these tests to be in the same order as the rules.
> > 
> 
> If it were in CajitaRewriterTest, that would make sense.  This file just tests
> that various things work cajoled in Cajita mode--I can't see that putting it
> anywhere else in the file makes more sense.

Eek!  This code isn't cajoled, so it makes no sense at all to put this here. 
Removed.

[MikeSamuel, 2009-07-31 22:16:21]

LGTM

http://codereview.appspot.com/96200/diff/18/1017
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/96200/diff/18/1017#newcode1377
Line 1377: if (typeof name === 'number' && name >= 0) { return name in obj; }
Ok.  So this will allow 'Infinity' but not '-Infinity'.

http://codereview.appspot.com/96200/diff/18/1017#newcode1425
Line 1425: //  (+name) === (name & 0x7fffffff)
That was a bad suggestion by me.
It violates the requirement that the string form be the canonical form of the
number.
So 'foo'['00'] would be treated the same as 'foo'['0'] which is incorrect.

Can you add a bit to the comment to that effect.

http://codereview.appspot.com/96200/diff/18/1016
File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (right):

http://codereview.appspot.com/96200/diff/18/1016#newcode1071
Line 1071: reason="Nonnegative integer properties are always readable, we can
pass these"
Long line.

http://codereview.appspot.com/96200/diff/18/1016#newcode1081
Line 1081: double indexValue = ((NumberLiteral) index).getValue().doubleValue();
Long line.

[metaweta, 2009-07-31 23:11:38]

On 2009/07/31 22:16:21, MikeSamuel wrote:
> LGTM
> 
> http://codereview.appspot.com/96200/diff/18/1017
> File src/com/google/caja/cajita.js (right):
> 
> http://codereview.appspot.com/96200/diff/18/1017#newcode1377
> Line 1377: if (typeof name === 'number' && name >= 0) { return name in obj; }
> Ok.  So this will allow 'Infinity' but not '-Infinity'.

Yes.

> http://codereview.appspot.com/96200/diff/18/1017#newcode1425
> Line 1425: //  (+name) === (name & 0x7fffffff)
> That was a bad suggestion by me.
> It violates the requirement that the string form be the canonical form of the
> number.
> So 'foo'['00'] would be treated the same as 'foo'['0'] which is incorrect.
> 
> Can you add a bit to the comment to that effect.

Done.

> http://codereview.appspot.com/96200/diff/18/1016
> File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (right):
> 
> http://codereview.appspot.com/96200/diff/18/1016#newcode1071
> Line 1071: reason="Nonnegative integer properties are always readable, we can
> pass these"
> Long line.

Fixed.

> http://codereview.appspot.com/96200/diff/18/1016#newcode1081
> Line 1081: double indexValue = ((NumberLiteral)
index).getValue().doubleValue();
> Long line.

Fixed.

[metaweta, 2009-07-31 23:33:49]

@3625