From c9c5f17e2b07f5aac2430eb26cee56f9cb6f841e Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 30 Jul 2020 12:06:36 -0400 Subject: [PATCH 1/8] Servlets should respond to HEAD requests. --- synapse/http/server.py | 2 +- synapse/http/servlet.py | 10 +++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/synapse/http/server.py b/synapse/http/server.py index d4f9ad6e6732..0d3e36a97fc3 100644 --- a/synapse/http/server.py +++ b/synapse/http/server.py @@ -579,7 +579,7 @@ def set_cors_headers(request: Request): """ request.setHeader(b"Access-Control-Allow-Origin", b"*") request.setHeader( - b"Access-Control-Allow-Methods", b"GET, POST, PUT, DELETE, OPTIONS" + b"Access-Control-Allow-Methods", b"GET, HEAD, POST, PUT, DELETE, OPTIONS" ) request.setHeader( b"Access-Control-Allow-Headers", diff --git a/synapse/http/servlet.py b/synapse/http/servlet.py index a34e5ead8863..dcc1aba92e49 100644 --- a/synapse/http/servlet.py +++ b/synapse/http/servlet.py @@ -270,6 +270,7 @@ class attribute containing a pre-compiled regular expression. The automatic instance methods associated with the corresponding HTTP method: on_GET + on_HEAD on_PUT on_POST on_DELETE @@ -284,7 +285,7 @@ def register(self, http_server): if hasattr(self, "PATTERNS"): patterns = self.PATTERNS - for method in ("GET", "PUT", "POST", "OPTIONS", "DELETE"): + for method in ("GET", "HEAD", "PUT", "POST", "OPTIONS", "DELETE"): if hasattr(self, "on_%s" % (method,)): servlet_classname = self.__class__.__name__ method_handler = getattr(self, "on_%s" % (method,)) @@ -292,5 +293,12 @@ def register(self, http_server): method, patterns, method_handler, servlet_classname ) + # If there's not HEAD handler, but there is a GET handler, + # register it again. + if method == "GET" and not hasattr(self, "on_HEAD"): + http_server.register_paths( + "HEAD", patterns, method_handler, servlet_classname + ) + else: raise NotImplementedError("RestServlet must register something.") From c4e88397bc477d41d4920b29fde96ea33f39d6f5 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 30 Jul 2020 12:35:07 -0400 Subject: [PATCH 2/8] Handle HEAD requests for direct serve resources. --- synapse/http/server.py | 8 ++++++++ tests/test_server.py | 15 +++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/synapse/http/server.py b/synapse/http/server.py index 0d3e36a97fc3..1cd85422f66b 100644 --- a/synapse/http/server.py +++ b/synapse/http/server.py @@ -216,6 +216,14 @@ def render(self, request): defer.ensureDeferred(self._async_render_wrapper(request)) return NOT_DONE_YET + def _async_render_HEAD(self, request): + """ + The default handling of HEAD method is to treat it as a GET request. + + This is an async version of twisted.web.resource.Resource.render_HEAD. + """ + return self._async_render_GET(request) + @wrap_async_request_handler async def _async_render_wrapper(self, request: SynapseRequest): """This is a wrapper that delegates to `_async_render` and handles diff --git a/tests/test_server.py b/tests/test_server.py index 073b2362cc87..cd85adcabc0e 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -312,3 +312,18 @@ def callback(request, **kwargs): self.assertEqual(location_headers, [b"/no/over/there"]) cookies_headers = [v for k, v in headers if k == b"Set-Cookie"] self.assertEqual(cookies_headers, [b"session=yespls"]) + + def test_head_request(self): + """A head request should work by being turned into a GET request.""" + def callback(request): + request.write(b"response") + request.finish() + + res = WrapHtmlRequestHandlerTests.TestResource() + res.callback = callback + + request, channel = make_request(self.reactor, b"HEAD", b"/path") + render(request, res, self.reactor) + + self.assertEqual(channel.result["code"], b"200") + self.assertNotIn("body", channel.result) From 445ae8d691b54ff9dce3b3962a6dde12ce59dc9a Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 30 Jul 2020 12:51:27 -0400 Subject: [PATCH 3/8] Add changelog. --- changelog.d/7999.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/7999.bugfix diff --git a/changelog.d/7999.bugfix b/changelog.d/7999.bugfix new file mode 100644 index 000000000000..6f335fb98099 --- /dev/null +++ b/changelog.d/7999.bugfix @@ -0,0 +1 @@ +Fix a long standing bug where HEAD requests were not properly rendered. From 2f62a4c6a1653db2fb854db6c9a79cce7ee65250 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 30 Jul 2020 12:54:49 -0400 Subject: [PATCH 4/8] Lint. --- tests/test_server.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_server.py b/tests/test_server.py index cd85adcabc0e..1c379b48d80c 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -315,6 +315,7 @@ def callback(request, **kwargs): def test_head_request(self): """A head request should work by being turned into a GET request.""" + def callback(request): request.write(b"response") request.finish() From 343801e9a5c9dc5db0bf025488e8b0c0f173af9f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 30 Jul 2020 13:28:49 -0400 Subject: [PATCH 5/8] Fix typo. Co-authored-by: David Vo --- synapse/http/servlet.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/http/servlet.py b/synapse/http/servlet.py index dcc1aba92e49..5563feace7eb 100644 --- a/synapse/http/servlet.py +++ b/synapse/http/servlet.py @@ -293,7 +293,7 @@ def register(self, http_server): method, patterns, method_handler, servlet_classname ) - # If there's not HEAD handler, but there is a GET handler, + # If there's no HEAD handler, but there is a GET handler, # register it again. if method == "GET" and not hasattr(self, "on_HEAD"): http_server.register_paths( From 2d751ccd09d0b8b44d48964ec8773d26a567151b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 31 Jul 2020 07:44:11 -0400 Subject: [PATCH 6/8] Convert HEAD -> GET in async_render. --- synapse/http/server.py | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/synapse/http/server.py b/synapse/http/server.py index 1cd85422f66b..8c5b4f46c7bd 100644 --- a/synapse/http/server.py +++ b/synapse/http/server.py @@ -216,14 +216,6 @@ def render(self, request): defer.ensureDeferred(self._async_render_wrapper(request)) return NOT_DONE_YET - def _async_render_HEAD(self, request): - """ - The default handling of HEAD method is to treat it as a GET request. - - This is an async version of twisted.web.resource.Resource.render_HEAD. - """ - return self._async_render_GET(request) - @wrap_async_request_handler async def _async_render_wrapper(self, request: SynapseRequest): """This is a wrapper that delegates to `_async_render` and handles @@ -250,10 +242,12 @@ async def _async_render(self, request: Request): no appropriate method exists. Can be overriden in sub classes for different routing. """ + method = request.method.decode("ascii") + # Treat HEAD requests as GET requests. + if method == "HEAD": + method = "GET" - method_handler = getattr( - self, "_async_render_%s" % (request.method.decode("ascii"),), None - ) + method_handler = getattr(self, "_async_render_%s" % (method,), None) if method_handler: raw_callback_return = method_handler(request) From f1c709b70b6af616db6ffd29a539a34c6023084b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 31 Jul 2020 07:48:11 -0400 Subject: [PATCH 7/8] Simplify the servlet handling. --- synapse/http/server.py | 14 +++++++++----- synapse/http/servlet.py | 10 +--------- tests/test_server.py | 31 +++++++++++++++++++++++++++---- 3 files changed, 37 insertions(+), 18 deletions(-) diff --git a/synapse/http/server.py b/synapse/http/server.py index 8c5b4f46c7bd..94ab29974aa0 100644 --- a/synapse/http/server.py +++ b/synapse/http/server.py @@ -242,12 +242,12 @@ async def _async_render(self, request: Request): no appropriate method exists. Can be overriden in sub classes for different routing. """ - method = request.method.decode("ascii") # Treat HEAD requests as GET requests. - if method == "HEAD": - method = "GET" + request_method = request.method.decode("ascii") + if request_method == "HEAD": + request_method = "GET" - method_handler = getattr(self, "_async_render_%s" % (method,), None) + method_handler = getattr(self, "_async_render_%s" % (request_method,), None) if method_handler: raw_callback_return = method_handler(request) @@ -364,11 +364,15 @@ def _get_handler_for_request( A tuple of the callback to use, the name of the servlet, and the key word arguments to pass to the callback """ + # Treat HEAD requests as GET requests. request_path = request.path.decode("ascii") + request_method = request.method + if request_method == b"HEAD": + request_method = b"GET" # Loop through all the registered callbacks to check if the method # and path regex match - for path_entry in self.path_regexs.get(request.method, []): + for path_entry in self.path_regexs.get(request_method, []): m = path_entry.pattern.match(request_path) if m: # We found a match! diff --git a/synapse/http/servlet.py b/synapse/http/servlet.py index 5563feace7eb..a34e5ead8863 100644 --- a/synapse/http/servlet.py +++ b/synapse/http/servlet.py @@ -270,7 +270,6 @@ class attribute containing a pre-compiled regular expression. The automatic instance methods associated with the corresponding HTTP method: on_GET - on_HEAD on_PUT on_POST on_DELETE @@ -285,7 +284,7 @@ def register(self, http_server): if hasattr(self, "PATTERNS"): patterns = self.PATTERNS - for method in ("GET", "HEAD", "PUT", "POST", "OPTIONS", "DELETE"): + for method in ("GET", "PUT", "POST", "OPTIONS", "DELETE"): if hasattr(self, "on_%s" % (method,)): servlet_classname = self.__class__.__name__ method_handler = getattr(self, "on_%s" % (method,)) @@ -293,12 +292,5 @@ def register(self, http_server): method, patterns, method_handler, servlet_classname ) - # If there's no HEAD handler, but there is a GET handler, - # register it again. - if method == "GET" and not hasattr(self, "on_HEAD"): - http_server.register_paths( - "HEAD", patterns, method_handler, servlet_classname - ) - else: raise NotImplementedError("RestServlet must register something.") diff --git a/tests/test_server.py b/tests/test_server.py index 1c379b48d80c..d628070e48d6 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -157,6 +157,29 @@ def _callback(request, **kwargs): self.assertEqual(channel.json_body["error"], "Unrecognized request") self.assertEqual(channel.json_body["errcode"], "M_UNRECOGNIZED") + def test_head_request(self): + """ + JsonResource.handler_for_request gives correctly decoded URL args to + the callback, while Twisted will give the raw bytes of URL query + arguments. + """ + + def _callback(request, **kwargs): + return 200, {"result": True} + + res = JsonResource(self.homeserver) + res.register_paths( + "GET", [re.compile("^/_matrix/foo$")], _callback, "test_servlet", + ) + + # The path was registered as GET, but this is a HEAD request. + request, channel = make_request(self.reactor, b"HEAD", b"/_matrix/foo") + render(request, res, self.reactor) + + self.assertEqual(channel.result["code"], b"200") + self.assertNotIn("body", channel.result) + self.assertEqual(channel.headers.getRawHeaders(b"Content-Length"), [b"15"]) + class OptionsResourceTests(unittest.TestCase): def setUp(self): @@ -255,7 +278,7 @@ def setUp(self): self.reactor = ThreadedMemoryReactorClock() def test_good_response(self): - def callback(request): + async def callback(request): request.write(b"response") request.finish() @@ -275,7 +298,7 @@ def test_redirect_exception(self): with the right location. """ - def callback(request, **kwargs): + async def callback(request, **kwargs): raise RedirectException(b"/look/an/eagle", 301) res = WrapHtmlRequestHandlerTests.TestResource() @@ -295,7 +318,7 @@ def test_redirect_exception_with_cookie(self): returned too """ - def callback(request, **kwargs): + async def callback(request, **kwargs): e = RedirectException(b"/no/over/there", 304) e.cookies.append(b"session=yespls") raise e @@ -316,7 +339,7 @@ def callback(request, **kwargs): def test_head_request(self): """A head request should work by being turned into a GET request.""" - def callback(request): + async def callback(request): request.write(b"response") request.finish() From 09588c2bd1fefe24556a5858bda441acc2e04cf0 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 3 Aug 2020 08:12:53 -0400 Subject: [PATCH 8/8] Clarify changelog. Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- changelog.d/7999.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/7999.bugfix b/changelog.d/7999.bugfix index 6f335fb98099..e0b8c4922f86 100644 --- a/changelog.d/7999.bugfix +++ b/changelog.d/7999.bugfix @@ -1 +1 @@ -Fix a long standing bug where HEAD requests were not properly rendered. +Fix a long standing bug where HTTP HEAD requests resulted in a 400 error.