Do not rely on the global JSON objects on a page

[scharf]

If you go to a cnn video, open the console and type in

JSON.stringify([1,2])

the result is

"\"[1,2]\""

And not as you expect "[1,2]". In fact it puts any array into strings even if they are nested:

JSON.stringify({"x":[1,2]})
"{\"x\":\"[1,2]\"}"

This changes the data sent to the server in unexpected ways....

==> do not rely on the global JSON object

[csillag]

We have met this problem here: hypothesis/h#608
See also here: mozilla/jschannel#17

[nickstenning]

Oh this is horrible. Thanks for the report, @scharf.

[tilgovi]

Prototype has since changed so that they do not export a broken Array.prototype.toJSON anymore. I'm not sure we should really do anything here. It seems we'd have to include our own, private json2.js patched to ignore toJSON methods. That's a lot of nonsense. Thoughts?

[nickstenning]

This is a difficult one. We can't possibly predict all the ways in which people could monkeypatch (and break) native APIs on websites, but we could easily detect and fix this one.

Options:

1. Ignore this problem as it's been fixed upstream (but there are no guarantees that this fix is deployed)
2. Notify the user of the problem somehow, but don't do anything about it.
3. Package and ship our own JSON polyfill and use it when we detect a broken implementation.
4. Detect a broken implementation, and dynamically load a polyfill from a CDN, blocking Annotator's use of JSON until it's loaded.

4 strikes me as the "right" option, given how rare this problem is, but it obviously has a significantly higher implementation cost than any other option. 3 fixes the problem for everyone, but adds N KiB for everyone. 2 seems like the worst of all worlds...

[aron]

@nickstenning but as far as I can see there is nothing wrong with the JSON object on this page. The problem lies with the fact that the Array.prototype.toJSON method has been patched.

delete Array.prototype.toJSON;
JSON.stringify([1,2]);
//=> "[1,2]"

This is a lot harder to detect unless you want to try to validate every object type, and update their toJSON methods. So perhaps option 2 is actually better?

[nickstenning]

The problem lies with the fact that the Array.prototype.toJSON method has been patched.

Ugh, javascript -- (╯°□°）╯︵ ┻━┻

You're right, of course, and that does make this basically impossible to fix for the bookmarklet form of Annotator, short of doing delete Array.prototype.toJSON, which I suppose could be construed as rather antisocial.

[tilgovi]

So we can definitely just warn and I think that's all we should do.
Maybe add it to the unsupported plugin?

[csillag]

You're right, of course, and that does make this basically impossible to fix for the bookmarklet
form of Annotator, short of doing delete Array.prototype.toJSON, which I suppose could be
construed as rather antisocial.

Sorry for entering so late to this discussion, but I think we have already (sort of) solved this earlier. Our workaround was temporarily remove Array.prototype.toJSON, just for the duration while JSON.stringify() is running, and restore it afterwards.

@edsu has fixed https://github.com/mozilla/jschannel this way, see here and here for the two steps (remove and patch), or here for the combined effect.

[csillag]

The solution mentioned above fixes the behavior of JSON.sringify(), without permanently removing the bogus Array.prototype.toJSON - which could indeed be perceived as antisocial by some misguided souls.

[csillag]

@tilgovi, @nickstenning, should we apply the solution outlined above?

[tilgovi]

After talking about it on IRC, I think @csillag is going to implement a configuration hook for serialization and then add the workaround to the bookmarklet code.

[tilgovi]

Or possibly as a plugin (which the bookmarklet would use).

[nickstenning]

In the interest of simplicity, and given the gnarliness of addressing any issue like this, I'm going to just close this for now. If a page author has sabotaged the Object or Array prototypes, there's very little we can do.