Repository: abhinavsingh/proxy.py

Issue:
[HttpParser] Handle invalid/malformed request chunks from client
**Describe the bug**
Currently, `HttpParser` works well assuming incoming client is not notorious :)  Handle cases when client can send invalid chunks in requests.

**To Reproduce**
Steps to reproduce the behavior:
1. Run `proxy.py` as `proxy.py --pac-file proxy.pac`
2. `telnet localhost 8899` and hit `Enter` key to send line feed
```
$ telnet localhost 8899
Trying ::1...
Connected to localhost.
Escape character is '^]'.

Connection closed by foreign host.
```
3. See error

```
2019-10-11 11:12:06,277 - pid:55396 [E] run:2215 - Exception while handling connection <socket.socket fd=27, family=AddressFamily.AF_INET6, type=SocketKind.SOCK_STREAM, proto=0, laddr=('::1', 8899, 0, 0), raddr=('::1', 49902, 0, 0)>
Traceback (most recent call last):
  File "./proxy.py", line 2205, in run
    teardown = self.run_once()
  File "./proxy.py", line 2173, in run_once
    teardown = self.handle_events(readables, writables)
  File "./proxy.py", line 2138, in handle_events
    teardown = self.handle_readables(readables)
  File "./proxy.py", line 2076, in handle_readables
    self.request.parse(client_data)
  File "./proxy.py", line 615, in parse
    more, raw = self.process(raw)
  File "./proxy.py", line 625, in process
    self.process_line(line)
  File "./proxy.py", line 667, in process_line
    self.url = urlparse.urlsplit(line[1])
IndexError: list index out of range
2019-10-11 11:12:06,279 - pid:55396 [I] access_log:1955 - ::1:49902 -  None
```

**Expected behavior**
`HttpParser` must either ignore invalid chunks or be strict about it and drop the connection.


Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/abhinavsingh/proxy.py/pull/740?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) Report
> Merging [#740](https://codecov.io/gh/abhinavsingh/proxy.py/pull/740?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) (8fadf49) into [develop](https://codecov.io/gh/abhinavsingh/proxy.py/commit/7f1470e3a95b7b0a0037f217b64a1cbe2cc315b8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) (7f1470e) will **decrease** coverage by `0.15%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/abhinavsingh/proxy.py/pull/740/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/740?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     #740      +/-   ##
===========================================
- Coverage    87.78%   87.63%   -0.16%     
===========================================
  Files          122      122              
  Lines         5363     5370       +7     
  Branches       431      432       +1     
===========================================
- Hits          4708     4706       -2     
- Misses         569      572       +3     
- Partials        86       92       +6     
```

| Flag | Coverage Δ | |
|---|---|---|
| pytest | `87.59% <100.00%> (-0.04%)` | :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/740?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) | Coverage Δ | |
|---|---|---|
| [proxy/http/parser/parser.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/740/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvaHR0cC9wYXJzZXIvcGFyc2VyLnB5) | `94.79% <100.00%> (+0.03%)` | :arrow_up: |
| [tests/http/test\_http\_parser.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/740/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-dGVzdHMvaHR0cC90ZXN0X2h0dHBfcGFyc2VyLnB5) | `100.00% <100.00%> (ø)` | |
| [proxy/common/types.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/740/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvY29tbW9uL3R5cGVzLnB5) | `86.66% <0.00%> (-13.34%)` | :arrow_down: |
| [tests/test\_set\_open\_file\_limit.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/740/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.29% <0.00%> (-3.71%)` | :arrow_down: |
| [proxy/common/constants.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/740/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvY29tbW9uL2NvbnN0YW50cy5weQ==) | `97.75% <0.00%> (-2.25%)` | :arrow_down: |
| [proxy/common/utils.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/740/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvY29tbW9uL3V0aWxzLnB5) | `91.86% <0.00%> (-1.63%)` | :arrow_down: |
| [proxy/common/flag.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/740/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvY29tbW9uL2ZsYWcucHk=) | `90.90% <0.00%> (-1.52%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/abhinavsingh/proxy.py/pull/740?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/740?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh). Last update [7f1470e...8fadf49](https://codecov.io/gh/abhinavsingh/proxy.py/pull/740?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).

abhinavsingh: I highly recommend you to setup https://github.com/abhinavsingh/proxy.py#setup-local-environment or at-least run those commands manually before pushing to upstream.  To avoid unnecessary to and fro.  Example now we ran into this issue:

<img width="711" alt="Screen Shot 2021-11-18 at 6 38 13 AM" src="https://user-images.githubusercontent.com/126065/142332599-1fab1e92-cff4-4c45-b655-7d20bdc95e9d.png">

at-least make sure to run

`make lib-mypy && make lib-flake8` to ensure linting is fine.  You may still run into other issues.  For total satisfaction, run `make` to run all tests locally.  :)

I know it's a PITA but is necessary to maintain code quality over time.
JerryKwan: Good advice. 
I an using a Windows development environment right now, I will change to a Linux environment and setup the hooks later

Patch:
diff --git a/proxy/http/parser/parser.py b/proxy/http/parser/parser.py
index f81a100..151e037 100644
--- a/proxy/http/parser/parser.py
+++ b/proxy/http/parser/parser.py
@@ -27,7 +27,6 @@ from .protocol import ProxyProtocol
 from .chunk import ChunkParser, chunkParserStates
 from .types import httpParserTypes, httpParserStates
 
-
 flags.add_argument(
     '--enable-proxy-protocol',
     action='store_true',
@@ -305,16 +304,27 @@ class HttpParser:
 
     def _process_line(self, raw: bytes) -> None:
         if self.type == httpParserTypes.REQUEST_PARSER:
+            # Ref:
+            # https://datatracker.ietf.org/doc/html/rfc2616#section-5.1
+            # https://greenbytes.de/tech/webdav/rfc7230.html#request.line
+            # https://greenbytes.de/tech/webdav/rfc7231.html#methods
+            # http://www.iana.org/assignments/http-methods/http-methods.xhtml
             if self.protocol is not None and self.protocol.version is None:
                 # We expect to receive entire proxy protocol v1 line
                 # in one network read and don't expect partial packets
                 self.protocol.parse(raw)
             else:
                 line = raw.split(WHITESPACE)
-                self.method = line[0].upper()
-                self.set_url(line[1])
-                self.version = line[2]
-                self.state = httpParserStates.LINE_RCVD
+                if len(line) == 3:
+                    self.method = line[0].upper()
+                    self.set_url(line[1])
+                    self.version = line[2]
+                    self.state = httpParserStates.LINE_RCVD
+                else:
+                    # raise exception
+                    # TODO, it would be better to use raise HttpProtocolException,
+                    # but we should solve circular import problem first
+                    raise NotImplementedError('Invalid request line')
         else:
             line = raw.split(WHITESPACE)
             self.version = line[0]


Test patch:
diff --git a/tests/http/test_http_parser.py b/tests/http/test_http_parser.py
index 11aa5c4..4ef4412 100644
--- a/tests/http/test_http_parser.py
+++ b/tests/http/test_http_parser.py
@@ -23,6 +23,15 @@ class TestHttpParser(unittest.TestCase):
     def setUp(self) -> None:
         self.parser = HttpParser(httpParserTypes.REQUEST_PARSER)
 
+    def test_issue_127(self) -> None:
+        with self.assertRaises(NotImplementedError):
+            self.parser.parse(CRLF)
+
+        with self.assertRaises(NotImplementedError):
+            raw = b'qwqrqw!@!#@!#ad adfad\r\n'
+            while True:
+                self.parser.parse(raw)
+
     def test_issue_398(self) -> None:
         p = HttpParser(httpParserTypes.RESPONSE_PARSER)
         p.parse(HTTP_1_0 + b' 200 OK' + CRLF)