Issue 1190: JSON.parse failing on Chrome

Chrome provides native JSON support.  This interacts badly with
revivers and replacers.  I think the solution is not to use revivers,
but to instead sanitize the input and output string.

The problem that we're running into is that methods which memoize state on checking canSetPub cause infinite recursion.

This adds a prepass that filters out bad keys, and fixes revivers and replacers.


Submitted @4005

[MarkM, 2010-03-02 05:39:26]

http://codereview.appspot.com/224092/diff/1/3
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/224092/diff/1/3#newcode875
src/com/google/caja/cajita.js:875: if (opt_memoize !== false) { fastpathSet(obj,
name); }
The parameter in scope here is named "memoize", so why does this seem to work
anyway?

[MikeSamuel, 2010-03-02 05:50:10]

http://codereview.appspot.com/224092/diff/1/3
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/224092/diff/1/3#newcode875
src/com/google/caja/cajita.js:875: if (opt_memoize !== false) { fastpathSet(obj,
name); }
On 2010/03/02 05:39:26, MarkM wrote:
> The parameter in scope here is named "memoize", so why does this seem to work
> anyway?

Hmm.  Because only the property set in directConstructor is hit by my testcase.

[MikeSamuel, 2010-03-04 19:59:33]

I reworked this to avoid the hackiness.  There is now a syntax filter on the
input that filters the JSON and is independent of whether the JSON
implementation is native or not.
I also implemented reviver replacer support.

[MikeSamuel, 2010-03-04 20:11:24]

I tested this on V8 by running

../../v8/shell \
    third_party/js/json_sans_eval/json_sans_eval.js \
    src/com/google/caja/cajita.js /tmp/foo.js

with the input file below and got the result :

CP-1
CP0
{"a":1,"b":2}
CP1
{"x":-1,"y":2} === {"x":-1,"y":2}
,x,y === ,x,y
millis,0,millis,1, == millis,0,millis,1,
true == is array : true
arr.length == 2 : 2
true == Date 0 : true
true == Date 1 : true
0 == 0
1000 == 1000


===========
/tmp/foo.js
===========

print('CP-1');
var x = safeJSON.parse('{"a":1, "b":2, "c___": 3}');
print('CP0');
print(safeJSON.stringify(x));
print('CP1');


(function () {
  var o = { z___: 3 };
  ___.setPub(o, 'x', 1);
  ___.setPub(o, 'y', -2);
  var keysSeen = [];
  function f(key, value) {
    keysSeen.push(key);
    return key === '' ? value : -value;
  }
  print(
      '{\"x\":-1,\"y\":2} === ' + 
      safeJSON.stringify(o, ___.markFuncFreeze(f)));
  print(',x,y === ' + keysSeen);
})();

(function () {
  var s = '[ { \"millis\": 0 }, { \"millis\": 1000 } ]';
  var keysSeen = [];
  function f(key, val) {
    keysSeen.push(key);
    if (typeof val === 'object' && typeof val.millis === 'number') {
      var d = new Date(val.millis);
      d.millis__ = val.millis;
      return d;
    }
    return val;
  }
  var arr = safeJSON.parse(s, ___.markFuncFreeze(f));
  print('millis,0,millis,1, == ' + keysSeen);
  print('true == ' + 'is array : ' + (arr instanceof Array));
  print('arr.length == 2 : ' + arr.length);
  print('true == Date 0 : ' + (arr[0] instanceof Date));
  print('true == Date 1 : ' + (arr[1] instanceof Date));
  print(0 + ' == ' + arr[0].getTime());
  print(1000 + ' == ' + arr[1].getTime());
})();

[MarkM, 2010-03-04 23:56:49]

What's JSON.checkSyntax? Where is it defined?

http://codereview.appspot.com/224092/diff/2008/2010
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/224092/diff/2008/2010#newcode4129
src/com/google/caja/cajita.js:4129: JSON.checkSyntax(text, function (key) {
What's JSON.checkSyntax? Where is it defined?

http://codereview.appspot.com/224092/diff/2008/2010#newcode4133
src/com/google/caja/cajita.js:4133: return len < 2 || '__' !== key.substring(len
- 2);
We already have a RegExp named endsWith__ for this purpose. I vaguely think I
remember it was faster than manually testing this way.

[MikeSamuel, 2010-03-05 00:15:10]

http://codereview.appspot.com/224092/diff/2008/2010
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/224092/diff/2008/2010#newcode4129
src/com/google/caja/cajita.js:4129: JSON.checkSyntax(text, function (key) {
On 2010/03/04 23:56:49, MarkM wrote:
> What's JSON.checkSyntax? Where is it defined?

http://codereview.appspot.com/224092/diff/6001/7003#newcode536

http://codereview.appspot.com/224092/diff/2008/2010#newcode4133
src/com/google/caja/cajita.js:4133: return len < 2 || '__' !== key.substring(len
- 2);
On 2010/03/04 23:56:49, MarkM wrote:
> We already have a RegExp named endsWith__ for this purpose. I vaguely think I
> remember it was faster than manually testing this way.

Done.

[MarkM, 2010-03-05 00:50:01]

http://codereview.appspot.com/224092/diff/6001/7003
File third_party/js/json_sans_eval/json_sans_eval.js (right):

http://codereview.appspot.com/224092/diff/6001/7003#newcode536
third_party/js/json_sans_eval/json_sans_eval.js:536: JSON.checkSyntax = function
checkJson(text, keyFilter) {
Sorry I missed this earlier. Should we be adding this to the real JSON
unconditionally? (Though I don't have a better suggestion of where to put it.)

http://codereview.appspot.com/224092/diff/6001/7003#newcode554
third_party/js/json_sans_eval/json_sans_eval.js:554: var state = 0;
I'm finding it hard to follow your state machine. Perhaps a comment enumerating
the different states and what they mean?

http://codereview.appspot.com/224092/diff/6001/7003#newcode572
third_party/js/json_sans_eval/json_sans_eval.js:572: case 0x27: throw new
Error(t);
First, for consistency with other cases, this should get a

    // '

But why are we switching on char codes rather than characters anyway? If it's
faster, fine. But if not, I'd rather see

    case "\'":
etc.

http://codereview.appspot.com/224092/diff/6001/7003#newcode591
third_party/js/json_sans_eval/json_sans_eval.js:591: return;  // }
For stylistic consistency, the

    // }

should be moved up to the "case 0x7d:" line.

[MikeSamuel, 2010-03-05 01:30:20]

http://codereview.appspot.com/224092/diff/6001/7003
File third_party/js/json_sans_eval/json_sans_eval.js (right):

http://codereview.appspot.com/224092/diff/6001/7003#newcode536
third_party/js/json_sans_eval/json_sans_eval.js:536: JSON.checkSyntax = function
checkJson(text, keyFilter) {
On 2010/03/05 00:50:01, MarkM wrote:
> Sorry I missed this earlier. Should we be adding this to the real JSON
> unconditionally? (Though I don't have a better suggestion of where to put it.)

In the first pass, I had a bunch more whitespace changes that made it hard to
find real changes.

This runs in safeJSON regardless of whether nativeJSON or the JSON.parse defined
below is used, so it needs to be defined independently of whether JSON is
native.

http://codereview.appspot.com/224092/diff/6001/7003#newcode554
third_party/js/json_sans_eval/json_sans_eval.js:554: var state = 0;
On 2010/03/05 00:50:01, MarkM wrote:
> I'm finding it hard to follow your state machine. Perhaps a comment
enumerating
> the different states and what they mean?

Sorry.  Done

http://codereview.appspot.com/224092/diff/6001/7003#newcode572
third_party/js/json_sans_eval/json_sans_eval.js:572: case 0x27: throw new
Error(t);
On 2010/03/05 00:50:01, MarkM wrote:
> First, for consistency with other cases, this should get a
> 
>     // '
> 
> But why are we switching on char codes rather than characters anyway? If it's
> faster, fine. But if not, I'd rather see

It is faster.

>     case "\'":
> etc.

http://codereview.appspot.com/224092/diff/6001/7003#newcode591
third_party/js/json_sans_eval/json_sans_eval.js:591: return;  // }
On 2010/03/05 00:50:01, MarkM wrote:
> For stylistic consistency, the
> 
>     // }
> 
> should be moved up to the "case 0x7d:" line.
> 
> 

Done.

[MarkM, 2010-03-05 05:18:35]

LGTM

http://codereview.appspot.com/224092/diff/13001/6004
File third_party/js/json_sans_eval/json_sans_eval.js (right):

http://codereview.appspot.com/224092/diff/13001/6004#newcode539
third_party/js/json_sans_eval/json_sans_eval.js:539: checkArray();
I'm surprised to see this as the start production. This means you'll accept
input like

    3,5

which isn't valid JSON, which is fine, since this isn't attempting validation.
No change needed; just noting surprise.

http://codereview.appspot.com/224092/diff/13001/6004#newcode540
third_party/js/json_sans_eval/json_sans_eval.js:540: if (i < n) { throw new
Error(); }
Diagnostic? ("Unexpected trailing text" or something?)

http://codereview.appspot.com/224092/diff/13001/6004#newcode566
third_party/js/json_sans_eval/json_sans_eval.js:566: if (!skip && keyFilter &&
!keyFilter(
skip can't possibly be non-zero here, right?

[MarkM, 2010-03-05 14:57:56]

Retracting the previous LGTM. Instead of erasing the associations for forbidden
property names, shouldn't we just throw an Error? This is both safer and simpler
-- no skip variable.

[MikeSamuel, 2010-03-05 17:40:50]

On 2010/03/05 14:57:56, MarkM wrote:
> Retracting the previous LGTM. Instead of erasing the associations for
forbidden
> property names, shouldn't we just throw an Error? This is both safer and
simpler
> -- no skip variable.

Current behavior is to drop invalid keys.  This is consistent what we do with
html sanitizing and css sanitizing as well.

[MikeSamuel, 2010-03-05 21:56:24]

On 2010/03/05 14:57:56, MarkM wrote:
> Retracting the previous LGTM. Instead of erasing the associations for
forbidden
> property names, shouldn't we just throw an Error? This is both safer and
simpler
> -- no skip variable.

done

[MarkM, 2010-03-05 21:58:44]

LGTM