From 2703faa84660da20a243cd7adfa0ebd5cc995b92 Mon Sep 17 00:00:00 2001 From: James Clarke Date: Wed, 4 Sep 2024 22:50:30 +0100 Subject: [PATCH] Fix binary over http protocol to use the actually selected protocol version in the response `content-type` header (#7701) Currently the `content-type` header of the response for the binary over http protocol, always contains the latest protocol version supported by the server, even if the client requested an older protocol version and the response data actually being encoded using that requested version. The current edgedb-js client which still uses protocol v1, just happens to work despite this bug, as it also has a bug that results in it ignoring the incorrect protocol version header from the server and assuming it's the version requested. (Will be fixed in https://github.com/edgedb/edgedb-js/pull/710) --- edb/server/protocol/protocol.pyx | 15 +++- tests/test_http_auth.py | 119 +++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 1 deletion(-) diff --git a/edb/server/protocol/protocol.pyx b/edb/server/protocol/protocol.pyx index 3eb0f98ab66..a76ba5c853a 100644 --- a/edb/server/protocol/protocol.pyx +++ b/edb/server/protocol/protocol.pyx @@ -586,6 +586,16 @@ cdef class HttpProtocol: int(ver_m.group(2).decode()), ) + if proto_ver < edbdef.MIN_PROTOCOL: + return self._bad_request( + request, + response, + message="requested protocol version is too old and " + "no longer supported", + ) + if proto_ver > edbdef.CURRENT_PROTOCOL: + proto_ver = edbdef.CURRENT_PROTOCOL + params = request.params if params is None: conn_params = {} @@ -609,7 +619,10 @@ cdef class HttpProtocol: tcp_transport=self.transport, ) response.status = http.HTTPStatus.OK - response.content_type = PROTO_MIME + response.content_type = ( + f'application/x.edgedb.v_' + f'{proto_ver[0]}_{proto_ver[1]}.binary' + ).encode() response.close_connection = True else: diff --git a/tests/test_http_auth.py b/tests/test_http_auth.py index 735a5afe81a..18159ea76f7 100644 --- a/tests/test_http_auth.py +++ b/tests/test_http_auth.py @@ -243,6 +243,125 @@ async def test_http_auth_scram_cors(self): headers['access-control-expose-headers'] ) + def test_http_binary_proto_too_old(self): + args = self.get_connect_args() + (token, _, status, _, _) = self._scram_auth( + args["user"], args["password"] + ) + + proto_ver = (0, 1) + proto_ver_str = f"v_{proto_ver[0]}_{proto_ver[1]}" + mime_type = f"application/x.edgedb.{proto_ver_str}.binary" + + with self.http_con() as con: + content, _, status = self.http_con_request( + con, + method="POST", + path=f"db/{args["database"]}", + prefix="", + body=protocol.Execute( + annotations=[], + allowed_capabilities=protocol.Capability.ALL, + compilation_flags=protocol.CompilationFlag(0), + implicit_limit=0, + command_text="SELECT 42", + output_format=protocol.OutputFormat.JSON, + expected_cardinality=protocol.Cardinality.AT_MOST_ONE, + input_typedesc_id=b"\0" * 16, + output_typedesc_id=b"\0" * 16, + state_typedesc_id=b"\0" * 16, + arguments=b"", + state_data=b"", + ).dump() + protocol.Sync().dump(), + headers={ + "Content-Type": mime_type, + "X-EdgeDB-User": args["user"], + "Authorization": f"Bearer {token.decode("ascii")}" + }, + ) + + self.assertEqual(status, 400) + self.assertEqual( + content, + b"requested protocol version is too old and no longer supported" + ) + + def test_http_binary_proto_old_supported(self): + args = self.get_connect_args() + (token, _, status, _, _) = self._scram_auth( + args["user"], args["password"] + ) + + proto_ver = (edbdef.CURRENT_PROTOCOL[0] - 1, edbdef.CURRENT_PROTOCOL[1]) + proto_ver_str = f"v_{proto_ver[0]}_{proto_ver[1]}" + mime_type = f"application/x.edgedb.{proto_ver_str}.binary" + + with self.http_con() as con: + _, headers, status = self.http_con_request( + con, + method="POST", + path=f"db/{args["database"]}", + prefix="", + body=protocol.Execute( + annotations=[], + allowed_capabilities=protocol.Capability.ALL, + compilation_flags=protocol.CompilationFlag(0), + implicit_limit=0, + command_text="SELECT 42", + output_format=protocol.OutputFormat.JSON, + expected_cardinality=protocol.Cardinality.AT_MOST_ONE, + input_typedesc_id=b"\0" * 16, + output_typedesc_id=b"\0" * 16, + state_typedesc_id=b"\0" * 16, + arguments=b"", + state_data=b"", + ).dump() + protocol.Sync().dump(), + headers={ + "Content-Type": mime_type, + "X-EdgeDB-User": args["user"], + "Authorization": f"Bearer {token.decode("ascii")}" + }, + ) + + self.assertEqual(status, 200) + self.assertEqual(headers, headers | {"content-type": mime_type}) + + def test_http_binary_proto_too_new(self): + args = self.get_connect_args() + (token, _, status, _, _) = self._scram_auth( + args["user"], args["password"] + ) + self.assertEqual(status, 200) + + proto_ver = (edbdef.CURRENT_PROTOCOL[0] + 1, edbdef.CURRENT_PROTOCOL[1]) + + expect_proto_ver = edbdef.CURRENT_PROTOCOL + proto_ver_str = f"v_{expect_proto_ver[0]}_{expect_proto_ver[1]}" + + with self.http_con() as con: + msgs, headers, status = self.http_con_binary_request( + con, + "SELECT 42", + proto_ver=proto_ver, + bearer_token=token.decode("ascii"), + user=args["user"], + database=args["database"], + ) + self.assertEqual(status, 200) + self.assertEqual(headers, headers | { + "content-type": f"application/x.edgedb.{proto_ver_str}.binary" + }) + self.assertIsInstance(msgs[0], protocol.CommandDataDescription) + self.assertIsInstance(msgs[1], protocol.Data) + self.assertEqual(bytes(msgs[1].data[0].data), b"42") + self.assertIsInstance(msgs[2], protocol.CommandComplete) + self.assertEqual(msgs[2].status, "SELECT") + self.assertIsInstance(msgs[3], protocol.ReadyForCommand) + self.assertEqual( + msgs[3].transaction_state, + protocol.TransactionState.NOT_IN_TRANSACTION, + ) + class TestHttpAuthSystem(BaseTestHttpAuth):