Reimplement safeJSON using native JSON/json_sans_eval and revivers/replacers.

Replaces the stopgap fix to security issue 978 with a longer term solution.

http://code.google.com/p/google-caja/issues/detail?id=978
    However, our current implementation of JSON.parse omits the
    reviver parameter. So long as the reviver is omitted, it is fine
    to wait until a JSON parse completes to post process with
    something like initializeMap. But once we support the ES3.1 JSON
    API (i.e., approximately the json2.js API), then we need to do our
    postprocessing before any user-provided reviver is run.

    On platforms implementing the ES3.1 JSON directly, can have the
    built in JSON implementation do all this interleaved with parsing,
    in one pass. To do so, we'd provide a tamedJSON whose parse wraps
    the user-provided reviver in one that first applies our own
    postprocess before invoking the user provided reviver.


Submitted @3498

[MarkM, 2009-04-22 05:53:36]

Sorry this took so long.

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

http://codereview.appspot.com/27090/diff/1/3#newcode128
Line 128: * of http://code.google.com/p/json-sans-eval/.
I checked with Ihab. This wasn't spliced into cajita.js for any good reason.
After this CL, it would be good to instead put the real json_sans_eval under
third_party and only leave the wrapping of it here. But don't worry about this
now.

http://codereview.appspot.com/27090/diff/1/3#newcode138
Line 138: if (!this.JSON || !this.JSON.parse
Why refer to "this.JSON" rather than simply "JSON"?

http://codereview.appspot.com/27090/diff/1/3#newcode139
Line 139: || !/\[native code\]/.test('' + this.JSON.parse)) {
I'm uncomfortable sniffing how JSON.parse prints, since no such printing
behavior is standard. Is there any reason not to simply delete this conjunct
from the test?

http://codereview.appspot.com/27090/diff/1/3#newcode303
Line 303: this.JSON = { parse: parse, stringify: this.JSON &&
this.JSON.stringify };
This use of "this.JSON" is bad, as it only works by virtue of the this-coercion
rules of ES3 and ES5-non-strict. It will not work in ES5-strict. Is there any
reason not to simply say "JSON"?

http://codereview.appspot.com/27090/diff/1/3#newcode3263
Line 3263: // 'this' value not produced by JSON.parse in the latest run.
I don't understand this comment.

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

http://codereview.appspot.com/27090/diff/1/2#newcode79
Line 79: + "    hop(safeJSON.parse('{ \"valueOf\": true }'), 'valueOf')); \n"
Hmm. It is somewhat surprising but is indeed the correct behavior. Likewise for
toString below.

[MikeSamuel, 2009-04-24 01:14:43]

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

http://codereview.appspot.com/27090/diff/1/3#newcode128
Line 128: * of http://code.google.com/p/json-sans-eval/.
Removed

http://codereview.appspot.com/27090/diff/1/3#newcode138
Line 138: if (!this.JSON || !this.JSON.parse
On 2009/04/22 05:53:36, MarkM wrote:
> Why refer to "this.JSON" rather than simply "JSON"?

Because I don't want to get an undefined global error when it's undefined.

http://codereview.appspot.com/27090/diff/1/3#newcode139
Line 139: || !/\[native code\]/.test('' + this.JSON.parse)) {
Are we okay with using a, possibly buggy, container's version of json2.js?

http://codereview.appspot.com/27090/diff/1/3#newcode303
Line 303: this.JSON = { parse: parse, stringify: this.JSON &&
this.JSON.stringify };
On 2009/04/22 05:53:36, MarkM wrote:
> This use of "this.JSON" is bad, as it only works by virtue of the
this-coercion
> rules of ES3 and ES5-non-strict. It will not work in ES5-strict. Is there any
> reason not to simply say "JSON"?

Done.

http://codereview.appspot.com/27090/diff/1/3#newcode3263
Line 3263: // 'this' value not produced by JSON.parse in the latest run.
It didn't make sense.  Removed.

[MarkM, 2009-04-24 01:43:53]

On 2009/04/24 01:14:43, MikeSamuel wrote:
> http://codereview.appspot.com/27090/diff/1/3#newcode138
> Line 138: if (!this.JSON || !this.JSON.parse
> On 2009/04/22 05:53:36, MarkM wrote:
> > Why refer to "this.JSON" rather than simply "JSON"?
> 
> Because I don't want to get an undefined global error when it's undefined.

Makes sense. Somehow I always forget that.


> http://codereview.appspot.com/27090/diff/1/3#newcode139
> Line 139: || !/\[native code\]/.test('' + this.JSON.parse)) {
> Are we okay with using a, possibly buggy, container's version of json2.js?

Good point. I agree that this test is the lesser evil.

[MarkM, 2009-04-24 01:44:34]

LGTM

http://codereview.appspot.com/27090/diff/3001/2005
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/27090/diff/3001/2005#newcode25
Line 25: * @provides ___, cajita, funcBind, safeJSON
Given your improvements below, shouldn't funcBind be removed from this list?

http://codereview.appspot.com/27090/diff/3001/2005#newcode66
Line 66: Date.prototype.toISOString = function dateToISOString() {
A nice improvement on the previous pattern. Thanks.

http://codereview.appspot.com/27090/diff/3001/2005#newcode3051
Line 3051: safeJSON = {
Should this be "safeJSON = freeze({"?

http://codereview.appspot.com/27090/diff/3001/2006
File third_party/js/json_sans_eval/json_sans_eval.js (right):

http://codereview.appspot.com/27090/diff/3001/2006#newcode16
Line 16: * Parses a string of well-formed JSON text.
Since this is imported into third_party from a separate project, may I assume it
has already been reviewed as part of that other project, so I only need to
review it here for Caja-compatibility, not correctness?

[MikeSamuel, 2009-04-24 16:01:31]

http://codereview.appspot.com/27090/diff/3001/2005
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/27090/diff/3001/2005#newcode25
Line 25: * @provides ___, cajita, funcBind, safeJSON
On 2009/04/24 01:44:34, MarkM wrote:
> Given your improvements below, shouldn't funcBind be removed from this list?

Done.

http://codereview.appspot.com/27090/diff/3001/2005#newcode3051
Line 3051: safeJSON = {
On 2009/04/24 01:44:34, MarkM wrote:
> Should this be "safeJSON = freeze({"?

Done.

http://codereview.appspot.com/27090/diff/3001/2006
File third_party/js/json_sans_eval/json_sans_eval.js (right):

http://codereview.appspot.com/27090/diff/3001/2006#newcode16
Line 16: * Parses a string of well-formed JSON text.
Yeah.  It's exactly the same as head on the caja project.  Ben looked at those
changes.

[MikeSamuel, 2009-04-24 20:43:22]

Checked the testbed and snapshotted.