Be stricter about JSON that is accepted by Sygnal

[richvdh]

This disables the JSON extensions which Python supports by default (parsing of
Infinity / -Infinity and NaN). These shouldn't be accepted since they're
not technically valid JSON and other languages won't be able to interpret it
properly.

[callahad]

I rewrote the json decoder to use functools.partial to bind json.loads instead of instantiating a new JSONDecoder. Tests pass now, and it behaves as expected:

>>> safe_json_loads('{"hello": "world"}')
{'hello': 'world'}

>>> safe_json_loads('{"hello": Infinity}')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.7/json/__init__.py", line 361, in loads
    return cls(**kw).decode(s)
  File "/usr/local/lib/python3.7/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/local/lib/python3.7/json/decoder.py", line 353, in raw_decode
    obj, end = self.scan_once(s, idx)
  File "<stdin>", line 3, in _reject_invalid_json
ValueError: Invalid JSON value: 'Infinity'

Trying in vain to clear my review and replace it with a request for review by synapse-core... but apparently GitHub doesn't make that easy if you're a member of synapse-core

[richvdh]

given that json.loads instantiates a JSONDecoder, why this is an improvement?

(Also: we do the local json_decoder thing in Sydent and Synapse, and there is value in being consistent)

[callahad]

why this is an improvement?

Because it works 🙂 cf failures in CI with the previous solution.

Keeping aligned with loads fits my brain better, but I don't have strong feelings at all so long as the tests pass.

[callahad]

Upon further reflection, I think I have a very slight preference for the partial solution, since it is literally a drop-in replacement for json.loads, while JSONDecoder.decode, empirically, is not.

The actual difference is that json.loads accepts str | bytes and handles decoding while JSONDecoder.decode only accepts str.

[richvdh]

ok, I'd rather avoid constructing a new JSONDecoder every call if possible. I've tweaked this to use a static json_decoder whilst still (hopefully) passing CI.

[callahad]

I'd rather avoid constructing a new JSONDecoder every call if possible

Seems like a premature optimization, but tests pass, so if you're happy I'm happy :)

[richvdh]

Sorry, our comments have been crossing.

Personally, I would rather we were type-strict and were certain about whether we had bytes or strs, rather than using interfaces which handle both. (Incidentally: I now realise the reason this didn't bite us on Synapse or Sydent is that both of those support python 3.5, where json.loads does not accept bytes, so we had already done the explicit .decode() calls).

So, to summarise, I see the following in favour of this approach:

• consistency with Synapse and Sydent
• type-strictness
• avoids constructing a JSONDecoder on each call (yes, a tiny optimisation, but stuff like this adds up, especially on Synapse)

So, thank you for humouring me :)