From ef5a1fd5f82f041531b72244e7ef226876286db1 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 30 Nov 2022 11:45:50 -0500 Subject: [PATCH 1/6] Allow customizing the status code for unrecognized requests. --- synapse/api/errors.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/synapse/api/errors.py b/synapse/api/errors.py index e2cfcea0f268..76ef12ed3a81 100644 --- a/synapse/api/errors.py +++ b/synapse/api/errors.py @@ -300,10 +300,8 @@ def __init__(self, session_id: str, result: "JsonDict"): class UnrecognizedRequestError(SynapseError): """An error indicating we don't understand the request you're trying to make""" - def __init__( - self, msg: str = "Unrecognized request", errcode: str = Codes.UNRECOGNIZED - ): - super().__init__(400, msg, errcode) + def __init__(self, msg: str = "Unrecognized request", code: int = 400): + super().__init__(code, msg, Codes.UNRECOGNIZED) class NotFoundError(SynapseError): From 88e8fc309778eb3f33b5c61201a42f19a67909e0 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 1 Dec 2022 10:52:38 -0500 Subject: [PATCH 2/6] Unknown prefixes and paths should return JSON errors. Use a custom resource which returns a JSON error (instead of an HTML error page) for unknown paths. --- synapse/util/httpresourcetree.py | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/synapse/util/httpresourcetree.py b/synapse/util/httpresourcetree.py index a0606851f7fb..f06011aac164 100644 --- a/synapse/util/httpresourcetree.py +++ b/synapse/util/httpresourcetree.py @@ -15,11 +15,34 @@ import logging from typing import Dict -from twisted.web.resource import NoResource, Resource +from twisted.python import failure +from twisted.web.resource import Resource +from twisted.web.server import NOT_DONE_YET, Request + +from synapse.api.errors import UnrecognizedRequestError +from synapse.http.server import return_json_error +from synapse.http.site import SynapseRequest logger = logging.getLogger(__name__) +class _UnrecognizedRequestResource(Resource): + """ + Similar to twisted.web.resource.NoResource, but returns a JSON 404 with an + errcode of M_UNRECOGNIZED. + """ + + def render(self, request: SynapseRequest) -> int: + f = failure.Failure(UnrecognizedRequestError(code=404)) + return_json_error(f, request, None) + # A response has already been sent but Twisted requires either NOT_DONE_YET + # or the response bytes as a return value. + return NOT_DONE_YET + + def getChild(self, name: str, request: Request) -> Resource: + return self + + def create_resource_tree( desired_tree: Dict[str, Resource], root_resource: Resource ) -> Resource: @@ -49,7 +72,7 @@ def create_resource_tree( for path_seg in full_path.split(b"/")[1:-1]: if path_seg not in last_resource.listNames(): # resource doesn't exist, so make a "dummy resource" - child_resource: Resource = NoResource() + child_resource: Resource = _UnrecognizedRequestResource() last_resource.putChild(path_seg, child_resource) res_id = _resource_id(last_resource, path_seg) resource_mappings[res_id] = child_resource From f282db19b5fb89a90252f8b0158bd58f5a8743a2 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 1 Dec 2022 12:23:31 -0500 Subject: [PATCH 3/6] Unknown endpoints for /media should return JSON responses. This is a generalization of the previous commit since /media endpoints are routed to via Twisted's standard Resource routing, not via servlet routing. --- synapse/http/server.py | 17 +++++++++++++++ synapse/rest/media/v1/media_repository.py | 4 ++-- synapse/util/httpresourcetree.py | 25 ++--------------------- 3 files changed, 21 insertions(+), 25 deletions(-) diff --git a/synapse/http/server.py b/synapse/http/server.py index 051a1899a0e1..ce95e3b9939e 100644 --- a/synapse/http/server.py +++ b/synapse/http/server.py @@ -580,6 +580,23 @@ def _unrecognised_request_handler(request: Request) -> NoReturn: raise UnrecognizedRequestError() +class UnrecognizedRequestResource(resource.Resource): + """ + Similar to twisted.web.resource.NoResource, but returns a JSON 404 with an + errcode of M_UNRECOGNIZED. + """ + + def render(self, request: SynapseRequest) -> int: + f = failure.Failure(UnrecognizedRequestError(code=404)) + return_json_error(f, request, None) + # A response has already been sent but Twisted requires either NOT_DONE_YET + # or the response bytes as a return value. + return NOT_DONE_YET + + def getChild(self, name: str, request: Request) -> resource.Resource: + return self + + class RootRedirect(resource.Resource): """Redirects the root '/' path to another path.""" diff --git a/synapse/rest/media/v1/media_repository.py b/synapse/rest/media/v1/media_repository.py index 40b0d39eb239..c70e1837afd0 100644 --- a/synapse/rest/media/v1/media_repository.py +++ b/synapse/rest/media/v1/media_repository.py @@ -24,7 +24,6 @@ import twisted.internet.error import twisted.web.http from twisted.internet.defer import Deferred -from twisted.web.resource import Resource from synapse.api.errors import ( FederationDeniedError, @@ -35,6 +34,7 @@ ) from synapse.config._base import ConfigError from synapse.config.repository import ThumbnailRequirement +from synapse.http.server import UnrecognizedRequestResource from synapse.http.site import SynapseRequest from synapse.logging.context import defer_to_thread from synapse.metrics.background_process_metrics import run_as_background_process @@ -1046,7 +1046,7 @@ async def _remove_local_media_from_disk( return removed_media, len(removed_media) -class MediaRepositoryResource(Resource): +class MediaRepositoryResource(UnrecognizedRequestResource): """File uploading and downloading. Uploads are POSTed to a resource which returns a token which is used to GET diff --git a/synapse/util/httpresourcetree.py b/synapse/util/httpresourcetree.py index f06011aac164..39fab4fe067e 100644 --- a/synapse/util/httpresourcetree.py +++ b/synapse/util/httpresourcetree.py @@ -15,34 +15,13 @@ import logging from typing import Dict -from twisted.python import failure from twisted.web.resource import Resource -from twisted.web.server import NOT_DONE_YET, Request -from synapse.api.errors import UnrecognizedRequestError -from synapse.http.server import return_json_error -from synapse.http.site import SynapseRequest +from synapse.http.server import UnrecognizedRequestResource logger = logging.getLogger(__name__) -class _UnrecognizedRequestResource(Resource): - """ - Similar to twisted.web.resource.NoResource, but returns a JSON 404 with an - errcode of M_UNRECOGNIZED. - """ - - def render(self, request: SynapseRequest) -> int: - f = failure.Failure(UnrecognizedRequestError(code=404)) - return_json_error(f, request, None) - # A response has already been sent but Twisted requires either NOT_DONE_YET - # or the response bytes as a return value. - return NOT_DONE_YET - - def getChild(self, name: str, request: Request) -> Resource: - return self - - def create_resource_tree( desired_tree: Dict[str, Resource], root_resource: Resource ) -> Resource: @@ -72,7 +51,7 @@ def create_resource_tree( for path_seg in full_path.split(b"/")[1:-1]: if path_seg not in last_resource.listNames(): # resource doesn't exist, so make a "dummy resource" - child_resource: Resource = _UnrecognizedRequestResource() + child_resource: Resource = UnrecognizedRequestResource() last_resource.putChild(path_seg, child_resource) res_id = _resource_id(last_resource, path_seg) resource_mappings[res_id] = child_resource From 2dc4b32e4f326fec8d4b85e8302dfa8ca7088d0b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 5 Dec 2022 10:47:05 -0500 Subject: [PATCH 4/6] Return a 404 instead of 400 for unrecognized requests. --- synapse/http/server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/http/server.py b/synapse/http/server.py index ce95e3b9939e..2563858f3cdf 100644 --- a/synapse/http/server.py +++ b/synapse/http/server.py @@ -577,7 +577,7 @@ def _unrecognised_request_handler(request: Request) -> NoReturn: Args: request: Unused, but passed in to match the signature of ServletCallback. """ - raise UnrecognizedRequestError() + raise UnrecognizedRequestError(code=404) class UnrecognizedRequestResource(resource.Resource): From 630ac598cda88f154fdd41f014da725ea634db58 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 5 Dec 2022 10:50:07 -0500 Subject: [PATCH 5/6] Newsfragment --- changelog.d/14621.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/14621.bugfix diff --git a/changelog.d/14621.bugfix b/changelog.d/14621.bugfix new file mode 100644 index 000000000000..cb95a87d9219 --- /dev/null +++ b/changelog.d/14621.bugfix @@ -0,0 +1 @@ +Return spec-compliant JSON errors when unknown endpoints are requested. From 6c37dabb5abb946bb50d309af78d09e8091a34f4 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 5 Dec 2022 12:09:41 -0500 Subject: [PATCH 6/6] Fix tests. --- tests/rest/admin/test_user.py | 2 +- tests/rest/client/test_login_token_request.py | 4 ++-- tests/rest/client/test_rendezvous.py | 2 +- tests/test_server.py | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index e8c945779428..5c1ced355ff1 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -3994,7 +3994,7 @@ def test_user_is_not_local(self, method: str) -> None: """ Tests that shadow-banning for a user that is not a local returns a 400 """ - url = "/_synapse/admin/v1/whois/@unknown_person:unknown_domain" + url = "/_synapse/admin/v1/users/@unknown_person:unknown_domain/shadow_ban" channel = self.make_request(method, url, access_token=self.admin_user_tok) self.assertEqual(400, channel.code, msg=channel.json_body) diff --git a/tests/rest/client/test_login_token_request.py b/tests/rest/client/test_login_token_request.py index c2e1e08811d2..6aedc1a11c9e 100644 --- a/tests/rest/client/test_login_token_request.py +++ b/tests/rest/client/test_login_token_request.py @@ -48,13 +48,13 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: def test_disabled(self) -> None: channel = self.make_request("POST", endpoint, {}, access_token=None) - self.assertEqual(channel.code, 400) + self.assertEqual(channel.code, 404) self.register_user(self.user, self.password) token = self.login(self.user, self.password) channel = self.make_request("POST", endpoint, {}, access_token=token) - self.assertEqual(channel.code, 400) + self.assertEqual(channel.code, 404) @override_config({"experimental_features": {"msc3882_enabled": True}}) def test_require_auth(self) -> None: diff --git a/tests/rest/client/test_rendezvous.py b/tests/rest/client/test_rendezvous.py index ad00a476e115..c0eb5d01a65b 100644 --- a/tests/rest/client/test_rendezvous.py +++ b/tests/rest/client/test_rendezvous.py @@ -36,7 +36,7 @@ def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: def test_disabled(self) -> None: channel = self.make_request("POST", endpoint, {}, access_token=None) - self.assertEqual(channel.code, 400) + self.assertEqual(channel.code, 404) @override_config({"experimental_features": {"msc3886_endpoint": "/asd"}}) def test_redirect(self) -> None: diff --git a/tests/test_server.py b/tests/test_server.py index 2d9a0257d410..d67d7722a485 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -174,7 +174,7 @@ def _callback(request: SynapseRequest, **kwargs: object) -> None: self.reactor, FakeSite(res, self.reactor), b"GET", b"/_matrix/foobar" ) - self.assertEqual(channel.code, 400) + self.assertEqual(channel.code, 404) self.assertEqual(channel.json_body["error"], "Unrecognized request") self.assertEqual(channel.json_body["errcode"], "M_UNRECOGNIZED")