From 6a442930032c5b089b50d2bbf2d5c2592516882e Mon Sep 17 00:00:00 2001 From: Nightrider0098 Date: Fri, 26 Jan 2024 01:29:38 +0530 Subject: [PATCH 1/9] Disable tunnel for http2 --- src/urllib3/http2.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/urllib3/http2.py b/src/urllib3/http2.py index 05de106f73..21dc41c356 100644 --- a/src/urllib3/http2.py +++ b/src/urllib3/http2.py @@ -33,6 +33,9 @@ def __init__( if "proxy" in kwargs or "proxy_config" in kwargs: # Defensive: raise NotImplementedError("Proxies aren't supported with HTTP/2") + if self._tunnel_host is not None: + raise NotImplementedError("Tunneling isn't supported with HTTP/2") + super().__init__(host, port, **kwargs) @contextlib.contextmanager From 1949484574e91cbdebd9824fecbdaf49ff577452 Mon Sep 17 00:00:00 2001 From: Nightrider0098 Date: Fri, 26 Jan 2024 01:30:59 +0530 Subject: [PATCH 2/9] Fix lints --- src/urllib3/http2.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/urllib3/http2.py b/src/urllib3/http2.py index 21dc41c356..68f0e19db7 100644 --- a/src/urllib3/http2.py +++ b/src/urllib3/http2.py @@ -34,7 +34,7 @@ def __init__( raise NotImplementedError("Proxies aren't supported with HTTP/2") if self._tunnel_host is not None: - raise NotImplementedError("Tunneling isn't supported with HTTP/2") + raise NotImplementedError("Tunneling isn't supported with HTTP/2") super().__init__(host, port, **kwargs) From c889806924a6c546313e832befecbefb77cc6ee3 Mon Sep 17 00:00:00 2001 From: Nightrider0098 Date: Fri, 26 Jan 2024 18:56:44 +0530 Subject: [PATCH 3/9] Move checks below --- .gitignore | 1 + src/urllib3/http2.py | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index 0ff7ddb2a1..7100901911 100644 --- a/.gitignore +++ b/.gitignore @@ -8,3 +8,4 @@ /build /docs/_build coverage.xml +venv/ \ No newline at end of file diff --git a/src/urllib3/http2.py b/src/urllib3/http2.py index 68f0e19db7..11ed9800cf 100644 --- a/src/urllib3/http2.py +++ b/src/urllib3/http2.py @@ -33,11 +33,11 @@ def __init__( if "proxy" in kwargs or "proxy_config" in kwargs: # Defensive: raise NotImplementedError("Proxies aren't supported with HTTP/2") + super().__init__(host, port, **kwargs) + if self._tunnel_host is not None: raise NotImplementedError("Tunneling isn't supported with HTTP/2") - super().__init__(host, port, **kwargs) - @contextlib.contextmanager def _lock_h2_conn(self) -> typing.Generator[h2.connection.H2Connection, None, None]: with self._h2_lock: From e0bef9d5ca85968de15e70cbdece9f30513d1075 Mon Sep 17 00:00:00 2001 From: Nightrider0098 Date: Thu, 8 Feb 2024 08:03:41 +0530 Subject: [PATCH 4/9] Add checks to disable tunnel in h2 --- src/urllib3/http2.py | 9 ++++ test/with_dummyserver/test_connectionpool.py | 47 ++++++++++++-------- test/with_dummyserver/test_https.py | 24 ++++++---- 3 files changed, 54 insertions(+), 26 deletions(-) diff --git a/src/urllib3/http2.py b/src/urllib3/http2.py index 11ed9800cf..e068778f07 100644 --- a/src/urllib3/http2.py +++ b/src/urllib3/http2.py @@ -96,6 +96,15 @@ def send(self, data: bytes) -> None: # type: ignore[override] # Defensive: return raise NotImplementedError("Sending data isn't supported yet") + def set_tunnel( + self, + host: str, + port: int | None = None, + headers: typing.Mapping[str, str] | None = None, + scheme: str = "http", + ) -> None: + raise ValueError("HTTP/2 does not support setting up a tunnel through a proxy") + def getresponse( # type: ignore[override] self, ) -> HTTP2Response: diff --git a/test/with_dummyserver/test_connectionpool.py b/test/with_dummyserver/test_connectionpool.py index 4fbe6a4f74..5cccfcc163 100644 --- a/test/with_dummyserver/test_connectionpool.py +++ b/test/with_dummyserver/test_connectionpool.py @@ -437,33 +437,44 @@ def test_socket_timeout_updated_on_reuse_parameter( [mock.call(x) for x in expect_settimeout_calls] ) - def test_tunnel(self) -> None: + def test_tunnel(self, http_version: str) -> None: # note the actual httplib.py has no tests for this functionality timeout = Timeout(total=None) with HTTPConnectionPool(self.host, self.port, timeout=timeout) as pool: conn = pool._get_conn() - try: - conn.set_tunnel(self.host, self.port) - with mock.patch.object( - conn, "_tunnel", create=True, return_value=None - ) as conn_tunnel: - pool._make_request(conn, "GET", "/") - conn_tunnel.assert_called_once_with() - finally: - conn.close() + if http_version == "h2": + with pytest.raises(ValueError) as e: + conn.set_tunnel(self.host, self.port) + assert ( + str(e.value) + == "HTTP/2 does not support setting up a tunnel through a proxy" + ) + else: + try: + conn.set_tunnel(self.host, self.port) + with mock.patch.object( + conn, "_tunnel", create=True, return_value=None + ) as conn_tunnel: + pool._make_request(conn, "GET", "/") + conn_tunnel.assert_called_once_with() + finally: + conn.close() # test that it's not called when tunnel is not set timeout = Timeout(total=None) with HTTPConnectionPool(self.host, self.port, timeout=timeout) as pool: conn = pool._get_conn() - try: - with mock.patch.object( - conn, "_tunnel", create=True, return_value=None - ) as conn_tunnel: - pool._make_request(conn, "GET", "/") - assert not conn_tunnel.called - finally: - conn.close() + if http_version == "h2": + pass + else: + try: + with mock.patch.object( + conn, "_tunnel", create=True, return_value=None + ) as conn_tunnel: + pool._make_request(conn, "GET", "/") + assert not conn_tunnel.called + finally: + conn.close() def test_redirect_relative_url_no_deprecation(self) -> None: with HTTPConnectionPool(self.host, self.port) as pool: diff --git a/test/with_dummyserver/test_https.py b/test/with_dummyserver/test_https.py index aa22f11879..e744a3d086 100644 --- a/test/with_dummyserver/test_https.py +++ b/test/with_dummyserver/test_https.py @@ -603,7 +603,7 @@ def test_https_timeout(self) -> None: with pytest.warns(InsecureRequestWarning): https_pool.request("GET", "/") - def test_tunnel(self) -> None: + def test_tunnel(self, http_version: str) -> None: """test the _tunnel behavior""" timeout = Timeout(total=None) with HTTPSConnectionPool( @@ -614,13 +614,21 @@ def test_tunnel(self) -> None: ssl_minimum_version=self.tls_version(), ) as https_pool: with contextlib.closing(https_pool._new_conn()) as conn: - conn.set_tunnel(self.host, self.port) - with mock.patch.object( - conn, "_tunnel", create=True, return_value=None - ) as conn_tunnel: - with pytest.warns(InsecureRequestWarning): - https_pool._make_request(conn, "GET", "/") - conn_tunnel.assert_called_once_with() + if http_version == "h2": + with pytest.raises(ValueError) as e: + conn.set_tunnel(self.host, self.port) + assert ( + str(e.value) + == "HTTP/2 does not support setting up a tunnel through a proxy" + ) + else: + conn.set_tunnel(self.host, self.port) + with mock.patch.object( + conn, "_tunnel", create=True, return_value=None + ) as conn_tunnel: + with pytest.warns(InsecureRequestWarning): + https_pool._make_request(conn, "GET", "/") + conn_tunnel.assert_called_once_with() @requires_network() def test_enhanced_timeout(self) -> None: From be61dfabf94d1b1dfa99af6753ba5e654c6584a4 Mon Sep 17 00:00:00 2001 From: Nightrider0098 Date: Thu, 8 Feb 2024 08:12:41 +0530 Subject: [PATCH 5/9] Resolve comments --- .gitignore | 3 +-- src/urllib3/http2.py | 2 +- test/with_dummyserver/test_connectionpool.py | 2 +- test/with_dummyserver/test_https.py | 2 +- 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/.gitignore b/.gitignore index 7100901911..bc5f5cd8f1 100644 --- a/.gitignore +++ b/.gitignore @@ -7,5 +7,4 @@ /dist /build /docs/_build -coverage.xml -venv/ \ No newline at end of file +coverage.xml \ No newline at end of file diff --git a/src/urllib3/http2.py b/src/urllib3/http2.py index e068778f07..0c16557ecd 100644 --- a/src/urllib3/http2.py +++ b/src/urllib3/http2.py @@ -103,7 +103,7 @@ def set_tunnel( headers: typing.Mapping[str, str] | None = None, scheme: str = "http", ) -> None: - raise ValueError("HTTP/2 does not support setting up a tunnel through a proxy") + raise NotImplementedError("HTTP/2 does not support setting up a tunnel through a proxy") def getresponse( # type: ignore[override] self, diff --git a/test/with_dummyserver/test_connectionpool.py b/test/with_dummyserver/test_connectionpool.py index 5cccfcc163..04c4b5307f 100644 --- a/test/with_dummyserver/test_connectionpool.py +++ b/test/with_dummyserver/test_connectionpool.py @@ -443,7 +443,7 @@ def test_tunnel(self, http_version: str) -> None: with HTTPConnectionPool(self.host, self.port, timeout=timeout) as pool: conn = pool._get_conn() if http_version == "h2": - with pytest.raises(ValueError) as e: + with pytest.raises(NotImplementedError) as e: conn.set_tunnel(self.host, self.port) assert ( str(e.value) diff --git a/test/with_dummyserver/test_https.py b/test/with_dummyserver/test_https.py index e744a3d086..b80345ba91 100644 --- a/test/with_dummyserver/test_https.py +++ b/test/with_dummyserver/test_https.py @@ -615,7 +615,7 @@ def test_tunnel(self, http_version: str) -> None: ) as https_pool: with contextlib.closing(https_pool._new_conn()) as conn: if http_version == "h2": - with pytest.raises(ValueError) as e: + with pytest.raises(NotImplementedError) as e: conn.set_tunnel(self.host, self.port) assert ( str(e.value) From c8406a486ad007c011cc36dd087fe2dc8d8b9285 Mon Sep 17 00:00:00 2001 From: Nightrider0098 Date: Thu, 8 Feb 2024 08:19:41 +0530 Subject: [PATCH 6/9] Fix master merge --- src/urllib3/http2.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/urllib3/http2.py b/src/urllib3/http2.py index a311f97e1d..9c9c7edc92 100644 --- a/src/urllib3/http2.py +++ b/src/urllib3/http2.py @@ -64,11 +64,6 @@ def __init__( if self._tunnel_host is not None: raise NotImplementedError("Tunneling isn't supported with HTTP/2") - @contextlib.contextmanager - def _lock_h2_conn(self) -> typing.Generator[h2.connection.H2Connection, None, None]: - with self._h2_lock: - yield self._h2_conn - def _new_h2_conn(self) -> _LockedObject[h2.connection.H2Connection]: config = h2.config.H2Configuration(client_side=True) return _LockedObject(h2.connection.H2Connection(config=config)) @@ -133,7 +128,9 @@ def set_tunnel( headers: typing.Mapping[str, str] | None = None, scheme: str = "http", ) -> None: - raise NotImplementedError("HTTP/2 does not support setting up a tunnel through a proxy") + raise NotImplementedError( + "HTTP/2 does not support setting up a tunnel through a proxy" + ) def getresponse( # type: ignore[override] self, From 60d6fd53aaa6e79ea9a0bdc858921acc33f530f3 Mon Sep 17 00:00:00 2001 From: Nightrider0098 Date: Thu, 8 Feb 2024 08:20:18 +0530 Subject: [PATCH 7/9] add end line --- .gitignore | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index bc5f5cd8f1..0ff7ddb2a1 100644 --- a/.gitignore +++ b/.gitignore @@ -7,4 +7,4 @@ /dist /build /docs/_build -coverage.xml \ No newline at end of file +coverage.xml From 47d695fe7dbe533d6fdc9c86ee658f1c3fc5d2ac Mon Sep 17 00:00:00 2001 From: Nightrider0098 Date: Thu, 8 Feb 2024 14:22:25 +0530 Subject: [PATCH 8/9] Resolve comments --- test/with_dummyserver/test_socketlevel.py | 28 +++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/with_dummyserver/test_socketlevel.py b/test/with_dummyserver/test_socketlevel.py index 69d8070b8b..10a10f1c37 100644 --- a/test/with_dummyserver/test_socketlevel.py +++ b/test/with_dummyserver/test_socketlevel.py @@ -1133,6 +1133,34 @@ def echo_socket_handler(listener: socket.socket) -> None: retries=False, ) + + def test_alpn_protocol_in_first_request_packet(self) -> None: + self.buf = b'' + def echo_socket_handler(listener: socket.socket) -> None: + sock = listener.accept()[0] + self.buf = b'' + while not self.buf.endswith(b"\r\n\r\n"): + self.buf += sock.recv(65536) + + sock.send( + ( + "HTTP/1.1 200 OK\r\n" + "Content-Type: text/plain\r\n" + "Content-Length: %d\r\n" + "\r\n" + "%s" % (len(self.buf), self.buf.decode("utf-8")) + ).encode("utf-8") + ) + sock.close() + + self._start_server(echo_socket_handler) + base_url = f"http://{self.host}:{self.port}" + with proxy_from_url(base_url) as proxy: + r = proxy.request("GET", "http://google.com/") + assert r.status == 200 + assert b'HTTP/1.1' in self.buf + assert b'h2' not in self.buf + def test_connect_reconn(self) -> None: def proxy_ssl_one(listener: socket.socket) -> None: sock = listener.accept()[0] From a061b9103affe27fe3ed2676fe88cf625c5441a9 Mon Sep 17 00:00:00 2001 From: Nightrider0098 Date: Thu, 8 Feb 2024 14:22:48 +0530 Subject: [PATCH 9/9] Resolve comments --- test/with_dummyserver/test_socketlevel.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/with_dummyserver/test_socketlevel.py b/test/with_dummyserver/test_socketlevel.py index 10a10f1c37..634df64171 100644 --- a/test/with_dummyserver/test_socketlevel.py +++ b/test/with_dummyserver/test_socketlevel.py @@ -1133,12 +1133,12 @@ def echo_socket_handler(listener: socket.socket) -> None: retries=False, ) - def test_alpn_protocol_in_first_request_packet(self) -> None: - self.buf = b'' + self.buf = b"" + def echo_socket_handler(listener: socket.socket) -> None: sock = listener.accept()[0] - self.buf = b'' + self.buf = b"" while not self.buf.endswith(b"\r\n\r\n"): self.buf += sock.recv(65536) @@ -1158,8 +1158,8 @@ def echo_socket_handler(listener: socket.socket) -> None: with proxy_from_url(base_url) as proxy: r = proxy.request("GET", "http://google.com/") assert r.status == 200 - assert b'HTTP/1.1' in self.buf - assert b'h2' not in self.buf + assert b"HTTP/1.1" in self.buf + assert b"h2" not in self.buf def test_connect_reconn(self) -> None: def proxy_ssl_one(listener: socket.socket) -> None: