Merge pull request #206 from clue-labs/parse-requests

Stricter check for invalid request-line in HTTP requests

Author: WyriHaximus

src/RequestHeaderParser.php

@@ -46,23 +46,30 @@ public function feed($data)
 
         if (false !== $endOfHeader) {
             try {
-                $this->parseAndEmitRequest();
+                $this->parseAndEmitRequest($endOfHeader);
             } catch (Exception $exception) {
                 $this->emit('error', array($exception));
             }
             $this->removeAllListeners();
         }
     }
 
-    private function parseAndEmitRequest()
+    private function parseAndEmitRequest($endOfHeader)
     {
-        list($request, $bodyBuffer) = $this->parseRequest($this->buffer);
+        $request = $this->parseRequest((string)substr($this->buffer, 0, $endOfHeader));
+        $bodyBuffer = isset($this->buffer[$endOfHeader + 4]) ? substr($this->buffer, $endOfHeader + 4) : '';
         $this->emit('headers', array($request, $bodyBuffer));
     }
 
-    private function parseRequest($data)
+    private function parseRequest($headers)
     {
-        list($headers, $bodyBuffer) = explode("\r\n\r\n", $data, 2);
+        // additional, stricter safe-guard for request line
+        // because request parser doesn't properly cope with invalid ones
+        if (!preg_match('#^[^ ]+ [^ ]+ HTTP/\d\.\d#m', $headers)) {
+            throw new \InvalidArgumentException('Unable to parse invalid request-line');
+        }
+
+        $lines = explode("\r\n", $headers);
 
         // parser does not support asterisk-form and authority-form
         // remember original target and temporarily replace and re-apply below
@@ -213,6 +220,6 @@ private function parseRequest($data)
         // always sanitize Host header because it contains critical routing information
         $request = $request->withUri($request->getUri()->withUserInfo('u')->withUserInfo(''));
 
-        return array($request, $bodyBuffer);
+        return $request;
     }
 }

tests/RequestHeaderParserTest.php

@@ -176,7 +176,7 @@ public function testHeaderOverflowShouldNotEmitErrorWhenDataExceedsMaxHeaderSize
         $this->assertSame($body, $bodyBuffer);
     }
 
-    public function testGuzzleRequestParseException()
+    public function testInvalidEmptyRequestHeadersParseException()
     {
         $error = null;
 
@@ -192,7 +192,28 @@ public function testGuzzleRequestParseException()
         $parser->feed("\r\n\r\n");
 
         $this->assertInstanceOf('InvalidArgumentException', $error);
-        $this->assertSame('Invalid message', $error->getMessage());
+        $this->assertSame('Unable to parse invalid request-line', $error->getMessage());
+        $this->assertSame(0, count($parser->listeners('headers')));
+        $this->assertSame(0, count($parser->listeners('error')));
+    }
+
+    public function testInvalidMalformedRequestLineParseException()
+    {
+        $error = null;
+
+        $parser = new RequestHeaderParser();
+        $parser->on('headers', $this->expectCallableNever());
+        $parser->on('error', function ($message) use (&$error) {
+            $error = $message;
+        });
+
+        $this->assertSame(1, count($parser->listeners('headers')));
+        $this->assertSame(1, count($parser->listeners('error')));
+
+        $parser->feed("GET /\r\n\r\n");
+
+        $this->assertInstanceOf('InvalidArgumentException', $error);
+        $this->assertSame('Unable to parse invalid request-line', $error->getMessage());
         $this->assertSame(0, count($parser->listeners('headers')));
         $this->assertSame(0, count($parser->listeners('error')));
     }