Repository: abhinavsingh/proxy.py
Pull request #1002 (2022-01-17 19:10:27)

Restrict request handling to `DEFAULT_ALLOWED_URL_SCHEMES`
Production environment instances may receive variety of non-http schemes in the request line.  E.g. `sip`, `icap` etc.  We want to bail out as early as possible upon identification of such requests.

Another scenario `\x03\x00\x00*%\xe0\x00\x00\x00\x00\x00Cookie: mstshash=Test\r\n\x01\x00\x08\x00\x03\x00\x00\x00'` will still run into the default `HttpProtocolException: Error when parsing request` exception

codecov[bot]: # [Codecov](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) Report
> Merging [#1002](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) (8197279) into [develop](https://codecov.io/gh/abhinavsingh/proxy.py/commit/f7b945800551e6bc981c365045cbe9007877f44c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) (f7b9458) will **decrease** coverage by `0.43%`.
> The diff coverage is `70.58%`.

[![Impacted file tree graph](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002/graphs/tree.svg?width=650&height=150&src=pr&token=Zh9J7b4la2&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh)](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh)

```diff
@@             Coverage Diff             @@
##           develop    #1002      +/-   ##
===========================================
- Coverage    87.26%   86.83%   -0.44%     
===========================================
  Files          146      146              
  Lines         6314     6320       +6     
  Branches       632      584      -48     
===========================================
- Hits          5510     5488      -22     
- Misses         699      716      +17     
- Partials       105      116      +11     
```

| Flag | Coverage Δ | |
|---|---|---|
| pytest | `86.83% <70.58%> (-0.34%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) | Coverage Δ | |
|---|---|---|
| [proxy/http/handler.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvaHR0cC9oYW5kbGVyLnB5) | `73.33% <0.00%> (-2.90%)` | :arrow_down: |
| [tests/http/parser/test\_http\_parser.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-dGVzdHMvaHR0cC9wYXJzZXIvdGVzdF9odHRwX3BhcnNlci5weQ==) | `100.00% <ø> (ø)` | |
| [proxy/http/url.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvaHR0cC91cmwucHk=) | `95.89% <66.66%> (-2.69%)` | :arrow_down: |
| [proxy/common/constants.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvY29tbW9uL2NvbnN0YW50cy5weQ==) | `100.00% <100.00%> (ø)` | |
| [proxy/http/parser/parser.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvaHR0cC9wYXJzZXIvcGFyc2VyLnB5) | `95.85% <100.00%> (ø)` | |
| [tests/http/test\_url.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-dGVzdHMvaHR0cC90ZXN0X3VybC5weQ==) | `100.00% <100.00%> (ø)` | |
| [tests/http/parser/test\_proxy\_protocol.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-dGVzdHMvaHR0cC9wYXJzZXIvdGVzdF9wcm94eV9wcm90b2NvbC5weQ==) | `96.07% <0.00%> (-3.93%)` | :arrow_down: |
| [tests/test\_set\_open\_file\_limit.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-dGVzdHMvdGVzdF9zZXRfb3Blbl9maWxlX2xpbWl0LnB5) | `96.42% <0.00%> (-3.58%)` | :arrow_down: |
| [tests/test\_main.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-dGVzdHMvdGVzdF9tYWluLnB5) | `96.51% <0.00%> (-3.49%)` | :arrow_down: |
| [tests/testing/test\_embed.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-dGVzdHMvdGVzdGluZy90ZXN0X2VtYmVkLnB5) | `90.90% <0.00%> (-3.04%)` | :arrow_down: |
| ... and [6 more](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh). Last update [f7b9458...8197279](https://codecov.io/gh/abhinavsingh/proxy.py/pull/1002?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh).


Patch:
diff --git a/docs/requirements.in b/docs/requirements.in
index 24f2e03..b79321c 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
diff --git a/proxy/common/constants.py b/proxy/common/constants.py
index 37ef452..04645af 100644
--- a/proxy/common/constants.py
+++ b/proxy/common/constants.py
@@ -94,6 +94,7 @@ DEFAULT_ENABLE_EVENTS = False
 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/core/event/dispatcher.py b/proxy/core/event/dispatcher.py
index d26340d..49408fd 100644
--- a/proxy/core/event/dispatcher.py
+++ b/proxy/core/event/dispatcher.py
@@ -61,7 +61,7 @@ class EventDispatcher:
             })
         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 c14d3d8..3d0b042 100644
--- a/proxy/http/handler.py
+++ b/proxy/http/handler.py
@@ -277,10 +277,14 @@ class HttpProtocolHandler(BaseTcpServerHandler[HttpClientConnection]):
         # memoryview compliant
         try:
             self.request.parse(data.tobytes())
-        except Exception:
+        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
         if not self.request.is_complete:
             return False
         # Discover which HTTP handler plugin is capable of
diff --git a/proxy/http/parser/parser.py b/proxy/http/parser/parser.py
index daeed72..6adbd55 100644
--- a/proxy/http/parser/parser.py
+++ b/proxy/http/parser/parser.py
@@ -149,9 +149,11 @@ class HttpParser:
         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 +206,7 @@ class HttpParser:
         """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 +219,10 @@ class HttpParser:
             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 +350,11 @@ class HttpParser:
                 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 +372,9 @@ class HttpParser:
                     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 f282b2b..9cbdb4a 100644
--- a/proxy/http/url.py
+++ b/proxy/http/url.py
@@ -13,11 +13,13 @@
        http
        url
 """
-from typing import Optional, Tuple
+from typing import List, 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.
@@ -59,7 +61,7 @@ class Url:
         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.
 
@@ -95,6 +97,10 @@ class Url:
             if len(parts) == 2:
                 scheme = parts[0]
                 rest = parts[1]
+                if scheme not in (allowed_url_schemes or 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:


Test patch:
diff --git a/tests/http/parser/test_http_parser.py b/tests/http/parser/test_http_parser.py
index 740c683..4ef092e 100644
--- a/tests/http/parser/test_http_parser.py
+++ b/tests/http/parser/test_http_parser.py
@@ -827,6 +827,7 @@ class TestHttpParser(unittest.TestCase):
             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_protocol_handler.py b/tests/http/test_protocol_handler.py
index ade2101..707725a 100644
--- a/tests/http/test_protocol_handler.py
+++ b/tests/http/test_protocol_handler.py
@@ -26,7 +26,7 @@ from proxy.common.plugins import Plugins
 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 @@ class TestHttpProtocolHandlerWithoutServerMock(Assertions):
             PROXY_AUTH_FAILED_RESPONSE_PKT,
         )
 
+    @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([
+            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    # 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([
+            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 958dc09..d246ca7 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):
@@ -145,10 +146,17 @@ class TestUrl(unittest.TestCase):
         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)
         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')