From e26f56fc2b8a4d5a6c2eb50decfbfd9db5fc655d Mon Sep 17 00:00:00 2001 From: Abhinav Singh Date: Tue, 18 Jan 2022 00:39:46 +0530 Subject: [PATCH 1/7] Raise `HttpProtocolException` if request line scheme do not match `DEFAULT_ALLOWED_URL_SCHEMES` --- proxy/common/constants.py | 1 + proxy/http/handler.py | 6 ++++-- proxy/http/url.py | 8 +++++++- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/proxy/common/constants.py b/proxy/common/constants.py index 37ef452f54..04645af08f 100644 --- a/proxy/common/constants.py +++ b/proxy/common/constants.py @@ -94,6 +94,7 @@ def _env_threadless_compliant() -> bool: DEFAULT_EVENTS_QUEUE = None DEFAULT_ENABLE_STATIC_SERVER = False DEFAULT_ENABLE_WEB_SERVER = False +DEFAULT_ALLOWED_URL_SCHEMES = [HTTP_PROTO, HTTPS_PROTO] DEFAULT_IPV4_HOSTNAME = ipaddress.IPv4Address('127.0.0.1') DEFAULT_IPV6_HOSTNAME = ipaddress.IPv6Address('::1') DEFAULT_KEY_FILE = None diff --git a/proxy/http/handler.py b/proxy/http/handler.py index c14d3d8263..4251dad265 100644 --- a/proxy/http/handler.py +++ b/proxy/http/handler.py @@ -277,10 +277,12 @@ def _parse_first_request(self, data: memoryview) -> bool: # memoryview compliant try: self.request.parse(data.tobytes()) - except Exception: + except HttpProtocolException as e: + raise e + except Exception as e: raise HttpProtocolException( 'Error when parsing request: %r' % data.tobytes(), - ) + ) from e if not self.request.is_complete: return False # Discover which HTTP handler plugin is capable of diff --git a/proxy/http/url.py b/proxy/http/url.py index f282b2bd3a..728d254e63 100644 --- a/proxy/http/url.py +++ b/proxy/http/url.py @@ -15,9 +15,11 @@ """ from typing import Optional, Tuple -from ..common.constants import COLON, SLASH, AT +from ..common.constants import COLON, DEFAULT_ALLOWED_URL_SCHEMES, SLASH, AT from ..common.utils import text_ +from .exception import HttpProtocolException + class Url: """``urllib.urlparse`` doesn't work for proxy.py, so we wrote a simple URL. @@ -95,6 +97,10 @@ def from_bytes(cls, raw: bytes) -> 'Url': if len(parts) == 2: scheme = parts[0] rest = parts[1] + if scheme not in DEFAULT_ALLOWED_URL_SCHEMES: + raise HttpProtocolException( + 'Invalid scheme received in the request line %r' % raw, + ) else: rest = raw[len(SLASH + SLASH):] if scheme is not None or starts_with_double_slash: From 8865dc96d1f97a598cb08147739cb53d09392780 Mon Sep 17 00:00:00 2001 From: Abhinav Singh Date: Tue, 18 Jan 2022 00:44:29 +0530 Subject: [PATCH 2/7] ignore WPS329 --- proxy/http/handler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proxy/http/handler.py b/proxy/http/handler.py index 4251dad265..62c9c7ae38 100644 --- a/proxy/http/handler.py +++ b/proxy/http/handler.py @@ -277,7 +277,7 @@ def _parse_first_request(self, data: memoryview) -> bool: # memoryview compliant try: self.request.parse(data.tobytes()) - except HttpProtocolException as e: + except HttpProtocolException as e: # noqa: WPS329 raise e except Exception as e: raise HttpProtocolException( From 193097756411289dd7403c4ed3ecfcd5735ca66e Mon Sep 17 00:00:00 2001 From: Abhinav Singh Date: Wed, 19 Jan 2022 18:21:02 +0530 Subject: [PATCH 3/7] Fix tests --- proxy/http/parser/parser.py | 21 +++++++++++++++------ proxy/http/url.py | 6 +++--- tests/http/parser/test_http_parser.py | 1 + tests/http/test_url.py | 5 ++++- 4 files changed, 23 insertions(+), 10 deletions(-) diff --git a/proxy/http/parser/parser.py b/proxy/http/parser/parser.py index daeed72979..add39bdd0e 100644 --- a/proxy/http/parser/parser.py +++ b/proxy/http/parser/parser.py @@ -149,9 +149,10 @@ def del_headers(self, headers: List[bytes]) -> None: for key in headers: self.del_header(key.lower()) - def set_url(self, url: bytes) -> None: + def set_url(self, url: bytes, allowed_url_schemes: Optional[List[bytes]] = None) -> None: """Given a request line, parses it and sets line attributes a.k.a. host, port, path.""" - self._url = Url.from_bytes(url) + self._url = Url.from_bytes( + url, allowed_url_schemes=allowed_url_schemes) self._set_line_attributes() @property @@ -204,7 +205,7 @@ def body_expected(self) -> bool: """Returns true if content or chunked response is expected.""" return self._content_expected or self._is_chunked_encoded - def parse(self, raw: bytes) -> None: + def parse(self, raw: bytes, allowed_url_schemes: Optional[List[bytes]] = None) -> None: """Parses HTTP request out of raw bytes. Check for `HttpParser.state` after `parse` has successfully returned.""" @@ -217,7 +218,10 @@ def parse(self, raw: bytes) -> None: if self.state >= httpParserStates.HEADERS_COMPLETE: more, raw = self._process_body(raw) elif self.state == httpParserStates.INITIALIZED: - more, raw = self._process_line(raw) + more, raw = self._process_line( + raw, + allowed_url_schemes=allowed_url_schemes, + ) else: more, raw = self._process_headers(raw) # When server sends a response line without any header or body e.g. @@ -345,7 +349,11 @@ def _process_headers(self, raw: bytes) -> Tuple[bool, bytes]: break return len(raw) > 0, raw - def _process_line(self, raw: bytes) -> Tuple[bool, bytes]: + def _process_line( + self, + raw: bytes, + allowed_url_schemes: Optional[List[bytes]] = None, + ) -> Tuple[bool, bytes]: while True: parts = raw.split(CRLF, 1) if len(parts) == 1: @@ -363,7 +371,8 @@ def _process_line(self, raw: bytes) -> Tuple[bool, bytes]: self.method = parts[0] if self.method == httpMethods.CONNECT: self._is_https_tunnel = True - self.set_url(parts[1]) + self.set_url( + parts[1], allowed_url_schemes=allowed_url_schemes) self.version = parts[2] self.state = httpParserStates.LINE_RCVD break diff --git a/proxy/http/url.py b/proxy/http/url.py index 728d254e63..9cbdb4ae10 100644 --- a/proxy/http/url.py +++ b/proxy/http/url.py @@ -13,7 +13,7 @@ http url """ -from typing import Optional, Tuple +from typing import List, Optional, Tuple from ..common.constants import COLON, DEFAULT_ALLOWED_URL_SCHEMES, SLASH, AT from ..common.utils import text_ @@ -61,7 +61,7 @@ def __str__(self) -> str: return url @classmethod - def from_bytes(cls, raw: bytes) -> 'Url': + def from_bytes(cls, raw: bytes, allowed_url_schemes: Optional[List[bytes]] = None) -> 'Url': """A URL within proxy.py core can have several styles, because proxy.py supports both proxy and web server use cases. @@ -97,7 +97,7 @@ def from_bytes(cls, raw: bytes) -> 'Url': if len(parts) == 2: scheme = parts[0] rest = parts[1] - if scheme not in DEFAULT_ALLOWED_URL_SCHEMES: + if scheme not in (allowed_url_schemes or DEFAULT_ALLOWED_URL_SCHEMES): raise HttpProtocolException( 'Invalid scheme received in the request line %r' % raw, ) diff --git a/tests/http/parser/test_http_parser.py b/tests/http/parser/test_http_parser.py index 740c683711..4ef092e919 100644 --- a/tests/http/parser/test_http_parser.py +++ b/tests/http/parser/test_http_parser.py @@ -827,6 +827,7 @@ def test_parses_icap_protocol(self) -> None: b'I am posting this information.\r\n' + b'0\r\n' + b'\r\n', + allowed_url_schemes=[b'icap'], ) self.assertEqual(self.parser.method, b'REQMOD') assert self.parser._url is not None diff --git a/tests/http/test_url.py b/tests/http/test_url.py index 958dc098bb..c430c3e781 100644 --- a/tests/http/test_url.py +++ b/tests/http/test_url.py @@ -145,7 +145,10 @@ def test_no_scheme_suffix(self) -> None: self.assertEqual(url.password, None) def test_any_scheme_suffix(self) -> None: - url = Url.from_bytes(b'icap://example-server.net/server?arg=87') + url = Url.from_bytes( + b'icap://example-server.net/server?arg=87', + allowed_url_schemes=[b'icap'], + ) self.assertEqual(url.scheme, b'icap') self.assertEqual(url.hostname, b'example-server.net') self.assertEqual(url.port, None) From 927443ecf90b95645e358756e40bbaeaf89ce4f1 Mon Sep 17 00:00:00 2001 From: Abhinav Singh Date: Wed, 19 Jan 2022 18:22:34 +0530 Subject: [PATCH 4/7] Pin to 4.3.2 --- docs/requirements.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/requirements.in b/docs/requirements.in index 24f2e03abd..b79321c965 100644 --- a/docs/requirements.in +++ b/docs/requirements.in @@ -1,6 +1,6 @@ myst-parser[linkify] >= 0.15.2 setuptools-scm >= 6.3.2 -Sphinx >= 4.3.0 +Sphinx == 4.3.2 furo >= 2021.11.15 sphinxcontrib-apidoc >= 0.3.0 sphinxcontrib-towncrier >= 0.2.0a0 From a7ab12f5bfb1dc3e15fe79dd489591f06f59929b Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 19 Jan 2022 12:53:47 +0000 Subject: [PATCH 5/7] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- proxy/http/parser/parser.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/proxy/http/parser/parser.py b/proxy/http/parser/parser.py index add39bdd0e..6adbd558d8 100644 --- a/proxy/http/parser/parser.py +++ b/proxy/http/parser/parser.py @@ -152,7 +152,8 @@ def del_headers(self, headers: List[bytes]) -> None: def set_url(self, url: bytes, allowed_url_schemes: Optional[List[bytes]] = None) -> None: """Given a request line, parses it and sets line attributes a.k.a. host, port, path.""" self._url = Url.from_bytes( - url, allowed_url_schemes=allowed_url_schemes) + url, allowed_url_schemes=allowed_url_schemes, + ) self._set_line_attributes() @property @@ -372,7 +373,8 @@ def _process_line( if self.method == httpMethods.CONNECT: self._is_https_tunnel = True self.set_url( - parts[1], allowed_url_schemes=allowed_url_schemes) + parts[1], allowed_url_schemes=allowed_url_schemes, + ) self.version = parts[2] self.state = httpParserStates.LINE_RCVD break From 710205a8cc94bf79a88cc8c6585c56228c7392d6 Mon Sep 17 00:00:00 2001 From: Abhinav Singh Date: Wed, 19 Jan 2022 18:50:56 +0530 Subject: [PATCH 6/7] Test coverage for exception handling --- proxy/core/event/dispatcher.py | 2 +- proxy/http/handler.py | 2 ++ tests/http/test_protocol_handler.py | 30 ++++++++++++++++++++++++++++- tests/http/test_url.py | 5 +++++ 4 files changed, 37 insertions(+), 2 deletions(-) diff --git a/proxy/core/event/dispatcher.py b/proxy/core/event/dispatcher.py index d26340d2ea..49408fdca6 100644 --- a/proxy/core/event/dispatcher.py +++ b/proxy/core/event/dispatcher.py @@ -61,7 +61,7 @@ def handle_event(self, ev: Dict[str, Any]) -> None: }) elif ev['event_name'] == eventNames.UNSUBSCRIBE: # send ack - print('unsubscription request ack sent') + logger.info('unsubscription request ack sent') self.subscribers[ev['event_payload']['sub_id']].send({ 'event_name': eventNames.UNSUBSCRIBED, }) diff --git a/proxy/http/handler.py b/proxy/http/handler.py index 62c9c7ae38..3d0b042941 100644 --- a/proxy/http/handler.py +++ b/proxy/http/handler.py @@ -278,8 +278,10 @@ def _parse_first_request(self, data: memoryview) -> bool: try: self.request.parse(data.tobytes()) except HttpProtocolException as e: # noqa: WPS329 + self.work.queue(BAD_REQUEST_RESPONSE_PKT) raise e except Exception as e: + self.work.queue(BAD_REQUEST_RESPONSE_PKT) raise HttpProtocolException( 'Error when parsing request: %r' % data.tobytes(), ) from e diff --git a/tests/http/test_protocol_handler.py b/tests/http/test_protocol_handler.py index ade2101a7c..6f6af313c3 100644 --- a/tests/http/test_protocol_handler.py +++ b/tests/http/test_protocol_handler.py @@ -26,7 +26,7 @@ from proxy.common.version import __version__ from proxy.http.responses import ( BAD_GATEWAY_RESPONSE_PKT, PROXY_AUTH_FAILED_RESPONSE_PKT, - PROXY_TUNNEL_ESTABLISHED_RESPONSE_PKT, + PROXY_TUNNEL_ESTABLISHED_RESPONSE_PKT, BAD_REQUEST_RESPONSE_PKT, ) from proxy.common.constants import ( CRLF, PLUGIN_HTTP_PROXY, PLUGIN_PROXY_AUTH, PLUGIN_WEB_SERVER, @@ -114,6 +114,34 @@ async def test_proxy_authentication_failed(self) -> None: PROXY_AUTH_FAILED_RESPONSE_PKT, ) + @pytest.mark.asyncio + async def test_proxy_bails_out_for_unknown_schemes(self) -> None: + mock_selector_for_client_read(self) + self._conn.recv.return_value = CRLF.join([ + b'REQMOD icap://icap-server.net/server?arg=87 ICAP/1.0', + b'Host: icap-server.net', + CRLF, + ]) + await self.protocol_handler._run_once() + self.assertEqual( + self.protocol_handler.work.buffer[0], + BAD_REQUEST_RESPONSE_PKT, + ) + + @pytest.mark.asyncio + async def test_proxy_bails_out_for_sip_request_lines(self) -> None: + mock_selector_for_client_read(self) + self._conn.recv.return_value = CRLF.join([ + b'OPTIONS sip:nm SIP/2.0', + b'Accept: application/sdp', + CRLF, + ]) + await self.protocol_handler._run_once() + self.assertEqual( + self.protocol_handler.work.buffer[0], + BAD_REQUEST_RESPONSE_PKT, + ) + class TestHttpProtocolHandler(Assertions): diff --git a/tests/http/test_url.py b/tests/http/test_url.py index c430c3e781..d246ca77f8 100644 --- a/tests/http/test_url.py +++ b/tests/http/test_url.py @@ -11,6 +11,7 @@ import unittest from proxy.http import Url +from proxy.http.exception import HttpProtocolException class TestUrl(unittest.TestCase): @@ -155,3 +156,7 @@ def test_any_scheme_suffix(self) -> None: self.assertEqual(url.remainder, b'/server?arg=87') self.assertEqual(url.username, None) self.assertEqual(url.password, None) + + def test_assert_raises_for_unknown_schemes(self) -> None: + with self.assertRaises(HttpProtocolException): + Url.from_bytes(b'icap://example-server.net/server?arg=87') From f9a0b8ef80d17410e2dd3e0dd43155b884c638aa Mon Sep 17 00:00:00 2001 From: Abhinav Singh Date: Wed, 19 Jan 2022 18:52:44 +0530 Subject: [PATCH 7/7] type ignore --- tests/http/test_protocol_handler.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/http/test_protocol_handler.py b/tests/http/test_protocol_handler.py index 6f6af313c3..707725ab6f 100644 --- a/tests/http/test_protocol_handler.py +++ b/tests/http/test_protocol_handler.py @@ -114,7 +114,7 @@ async def test_proxy_authentication_failed(self) -> None: PROXY_AUTH_FAILED_RESPONSE_PKT, ) - @pytest.mark.asyncio + @pytest.mark.asyncio # type: ignore[misc] async def test_proxy_bails_out_for_unknown_schemes(self) -> None: mock_selector_for_client_read(self) self._conn.recv.return_value = CRLF.join([ @@ -128,7 +128,7 @@ async def test_proxy_bails_out_for_unknown_schemes(self) -> None: BAD_REQUEST_RESPONSE_PKT, ) - @pytest.mark.asyncio + @pytest.mark.asyncio # type: ignore[misc] async def test_proxy_bails_out_for_sip_request_lines(self) -> None: mock_selector_for_client_read(self) self._conn.recv.return_value = CRLF.join([