Fix the exception that is raised when invalid JSON is encountered. (#8291)

6 files changed, 167 insertions, 2 deletions

diff --git a/changelog.d/8291.bugfix b/changelog.d/8291.bugfix
new file mode 100644
index 0000000000..bc01d26f53
--- /dev/null
+++ b/changelog.d/8291.bugfix
@@ -0,0 +1 @@
+Fix a bug introduced in v1.20.0rc1 that the wrong exception was raised when invalid JSON data is encountered.
diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py
index 775fad3be4..5eaf3151ce 100644
--- a/synapse/http/matrixfederationclient.py
+++ b/synapse/http/matrixfederationclient.py
@@ -54,6 +54,7 @@ from synapse.logging.opentracing import (
     start_active_span,
     tags,
 )
+from synapse.util import json_decoder
 from synapse.util.async_helpers import timeout_deferred
 from synapse.util.metrics import Measure
 
@@ -164,7 +165,9 @@ async def _handle_json_response(
     try:
         check_content_type_is_json(response.headers)
 
-        d = treq.json_content(response)
+        # Use the custom JSON decoder (partially re-implements treq.json_content).
+        d = treq.text_content(response, encoding="utf-8")
+        d.addCallback(json_decoder.decode)
         d = timeout_deferred(d, timeout=timeout_sec, reactor=reactor)
 
         body = await make_deferred_yieldable(d)
diff --git a/synapse/util/__init__.py b/synapse/util/__init__.py
index 3ad4b28fc7..a13f11f8d8 100644
--- a/synapse/util/__init__.py
+++ b/synapse/util/__init__.py
@@ -28,7 +28,7 @@ logger = logging.getLogger(__name__)
 
 def _reject_invalid_json(val):
     """Do not allow Infinity, -Infinity, or NaN values in JSON."""
-    raise json.JSONDecodeError("Invalid JSON value: '%s'" % val)
+    raise ValueError("Invalid JSON value: '%s'" % val)
 
 
 # Create a custom encoder to reduce the whitespace produced by JSON encoding and
diff --git a/tests/federation/test_federation_server.py b/tests/federation/test_federation_server.py
index 296dc887be..da933ecd75 100644
--- a/tests/federation/test_federation_server.py
+++ b/tests/federation/test_federation_server.py
@@ -15,6 +15,8 @@
 # limitations under the License.
 import logging
 
+from parameterized import parameterized
+
 from synapse.events import make_event_from_dict
 from synapse.federation.federation_server import server_matches_acl_event
 from synapse.rest import admin
@@ -23,6 +25,37 @@ from synapse.rest.client.v1 import login, room
 from tests import unittest
 
 
+class FederationServerTests(unittest.FederatingHomeserverTestCase):
+
+    servlets = [
+        admin.register_servlets,
+        room.register_servlets,
+        login.register_servlets,
+    ]
+
+    @parameterized.expand([(b"",), (b"foo",), (b'{"limit": Infinity}',)])
+    def test_bad_request(self, query_content):
+        """
+        Querying with bad data returns a reasonable error code.
+        """
+        u1 = self.register_user("u1", "pass")
+        u1_token = self.login("u1", "pass")
+
+        room_1 = self.helper.create_room_as(u1, tok=u1_token)
+        self.inject_room_member(room_1, "@user:other.example.com", "join")
+
+        "/get_missing_events/(?P<room_id>[^/]*)/?"
+
+        request, channel = self.make_request(
+            "POST",
+            "/_matrix/federation/v1/get_missing_events/%s" % (room_1,),
+            query_content,
+        )
+        self.render(request)
+        self.assertEquals(400, channel.code, channel.result)
+        self.assertEqual(channel.json_body["errcode"], "M_NOT_JSON")
+
+
 class ServerACLsTestCase(unittest.TestCase):
     def test_blacklisted_server(self):
         e = _create_acl_event({"allow": ["*"], "deny": ["evil.com"]})
diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py
index 7f1dfb2128..5604af3795 100644
--- a/tests/http/test_fedclient.py
+++ b/tests/http/test_fedclient.py
@@ -16,6 +16,7 @@
 from mock import Mock
 
 from netaddr import IPSet
+from parameterized import parameterized
 
 from twisted.internet import defer
 from twisted.internet.defer import TimeoutError
@@ -511,3 +512,50 @@ class FederationClientTests(HomeserverTestCase):
         self.reactor.advance(120)
 
         self.assertTrue(conn.disconnecting)
+
+    @parameterized.expand([(b"",), (b"foo",), (b'{"a": Infinity}',)])
+    def test_json_error(self, return_value):
+        """
+        Test what happens if invalid JSON is returned from the remote endpoint.
+        """
+
+        test_d = defer.ensureDeferred(self.cl.get_json("testserv:8008", "foo/bar"))
+
+        self.pump()
+
+        # Nothing happened yet
+        self.assertNoResult(test_d)
+
+        # Make sure treq is trying to connect
+        clients = self.reactor.tcpClients
+        self.assertEqual(len(clients), 1)
+        (host, port, factory, _timeout, _bindAddress) = clients[0]
+        self.assertEqual(host, "1.2.3.4")
+        self.assertEqual(port, 8008)
+
+        # complete the connection and wire it up to a fake transport
+        protocol = factory.buildProtocol(None)
+        transport = StringTransport()
+        protocol.makeConnection(transport)
+
+        # that should have made it send the request to the transport
+        self.assertRegex(transport.value(), b"^GET /foo/bar")
+        self.assertRegex(transport.value(), b"Host: testserv:8008")
+
+        # Deferred is still without a result
+        self.assertNoResult(test_d)
+
+        # Send it the HTTP response
+        protocol.dataReceived(
+            b"HTTP/1.1 200 OK\r\n"
+            b"Server: Fake\r\n"
+            b"Content-Type: application/json\r\n"
+            b"Content-Length: %i\r\n"
+            b"\r\n"
+            b"%s" % (len(return_value), return_value)
+        )
+
+        self.pump()
+
+        f = self.failureResultOf(test_d)
+        self.assertIsInstance(f.value, ValueError)
diff --git a/tests/http/test_servlet.py b/tests/http/test_servlet.py
new file mode 100644
index 0000000000..45089158ce
--- /dev/null
+++ b/tests/http/test_servlet.py
@@ -0,0 +1,80 @@
+# -*- coding: utf-8 -*-
+# Copyright 2020 The Matrix.org Foundation C.I.C.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+import json
+from io import BytesIO
+
+from mock import Mock
+
+from synapse.api.errors import SynapseError
+from synapse.http.servlet import (
+    parse_json_object_from_request,
+    parse_json_value_from_request,
+)
+
+from tests import unittest
+
+
+def make_request(content):
+    """Make an object that acts enough like a request."""
+    request = Mock(spec=["content"])
+
+    if isinstance(content, dict):
+        content = json.dumps(content).encode("utf8")
+
+    request.content = BytesIO(content)
+    return request
+
+
+class TestServletUtils(unittest.TestCase):
+    def test_parse_json_value(self):
+        """Basic tests for parse_json_value_from_request."""
+        # Test round-tripping.
+        obj = {"foo": 1}
+        result = parse_json_value_from_request(make_request(obj))
+        self.assertEqual(result, obj)
+
+        # Results don't have to be objects.
+        result = parse_json_value_from_request(make_request(b'["foo"]'))
+        self.assertEqual(result, ["foo"])
+
+        # Test empty.
+        with self.assertRaises(SynapseError):
+            parse_json_value_from_request(make_request(b""))
+
+        result = parse_json_value_from_request(make_request(b""), allow_empty_body=True)
+        self.assertIsNone(result)
+
+        # Invalid UTF-8.
+        with self.assertRaises(SynapseError):
+            parse_json_value_from_request(make_request(b"\xFF\x00"))
+
+        # Invalid JSON.
+        with self.assertRaises(SynapseError):
+            parse_json_value_from_request(make_request(b"foo"))
+
+        with self.assertRaises(SynapseError):
+            parse_json_value_from_request(make_request(b'{"foo": Infinity}'))
+
+    def test_parse_json_object(self):
+        """Basic tests for parse_json_object_from_request."""
+        # Test empty.
+        result = parse_json_object_from_request(
+            make_request(b""), allow_empty_body=True
+        )
+        self.assertEqual(result, {})
+
+        # Test not an object
+        with self.assertRaises(SynapseError):
+            parse_json_object_from_request(make_request(b'["foo"]'))