Merge pull request #318 from clue-labs/smuggle

Reject requests that contain both Content-Length and Transfer-Encoding

Author: WyriHaximus

README.md

@@ -411,14 +411,14 @@ message boundaries.
 This value may be `0` if the request message does not contain a request body
 (such as a simple `GET` request).
 Note that this value may be `null` if the request body size is unknown in
-advance because the request message uses chunked transfer encoding.
+advance because the request message uses `Transfer-Encoding: chunked`.
 
 ```php 
 $server = new StreamingServer(function (ServerRequestInterface $request) {
     $size = $request->getBody()->getSize();
     if ($size === null) {
         $body = 'The request does not contain an explicit length.';
-        $body .= 'This server does not accept chunked transfer encoding.';
+        $body .= 'This example does not accept chunked transfer encoding.';
 
         return new Response(
             411,

src/StreamingServer.php

@@ -203,9 +203,14 @@ public function handleRequest(ConnectionInterface $conn, ServerRequestInterface
                 return $this->writeError($conn, 501, $request);
             }
 
-            $stream = new ChunkedDecoder($stream);
-            $request = $request->withoutHeader('Content-Length');
+            // Transfer-Encoding: chunked and Content-Length header MUST NOT be used at the same time
+            // as per https://tools.ietf.org/html/rfc7230#section-3.3.3
+            if ($request->hasHeader('Content-Length')) {
+                $this->emit('error', array(new \InvalidArgumentException('Using both `Transfer-Encoding: chunked` and `Content-Length` is not allowed')));
+                return $this->writeError($conn, 400, $request);
+            }
 
+            $stream = new ChunkedDecoder($stream);
             $contentLength = null;
         } elseif ($request->hasHeader('Content-Length')) {
             $string = $request->getHeaderLine('Content-Length');

tests/StreamingServerTest.php

@@ -1773,79 +1773,42 @@ public function testRequestZeroContentLengthWillEmitEndAndAdditionalDataWillBeIg
         $this->connection->emit('data', array($data));
     }
 
-    public function testRequestContentLengthWillBeIgnoredIfTransferEncodingIsSet()
+    public function testRequestWithBothContentLengthAndTransferEncodingWillEmitServerErrorAndSendResponse()
     {
-        $dataEvent = $this->expectCallableOnceWith('hello');
-        $endEvent = $this->expectCallableOnce();
-        $closeEvent = $this->expectCallableOnce();
-        $errorEvent = $this->expectCallableNever();
-
-        $requestValidation = null;
-        $server = new StreamingServer(function (ServerRequestInterface $request) use ($dataEvent, $endEvent, $closeEvent, $errorEvent, &$requestValidation) {
-            $request->getBody()->on('data', $dataEvent);
-            $request->getBody()->on('end', $endEvent);
-            $request->getBody()->on('close', $closeEvent);
-            $request->getBody()->on('error', $errorEvent);
-            $requestValidation = $request;
+        $error = null;
+        $server = new StreamingServer($this->expectCallableNever());
+        $server->on('error', function ($message) use (&$error) {
+            $error = $message;
         });
 
-        $server->listen($this->socket);
-        $this->socket->emit('connection', array($this->connection));
-
-        $data = "GET / HTTP/1.1\r\n";
-        $data .= "Host: example.com:80\r\n";
-        $data .= "Connection: close\r\n";
-        $data .= "Content-Length: 4\r\n";
-        $data .= "Transfer-Encoding: chunked\r\n";
-        $data .= "\r\n";
-
-        $this->connection->emit('data', array($data));
-
-        $data = "5\r\nhello\r\n";
-        $data .= "0\r\n\r\n";
-
-        $this->connection->emit('data', array($data));
-
-        $this->assertFalse($requestValidation->hasHeader('Content-Length'));
-        $this->assertEquals('chunked', $requestValidation->getHeaderLine('Transfer-Encoding'));
-    }
-
-    public function testRequestInvalidContentLengthWillBeIgnoreddIfTransferEncodingIsSet()
-    {
-        $dataEvent = $this->expectCallableOnceWith('hello');
-        $endEvent = $this->expectCallableOnce();
-        $closeEvent = $this->expectCallableOnce();
-        $errorEvent = $this->expectCallableNever();
-
-        $requestValidation = null;
-        $server = new StreamingServer(function (ServerRequestInterface $request) use ($dataEvent, $endEvent, $closeEvent, $errorEvent, &$requestValidation) {
-            $request->getBody()->on('data', $dataEvent);
-            $request->getBody()->on('end', $endEvent);
-            $request->getBody()->on('close', $closeEvent);
-            $request->getBody()->on('error', $errorEvent);
-            $requestValidation = $request;
-        });
+        $buffer = '';
+        $this->connection
+            ->expects($this->any())
+            ->method('write')
+            ->will(
+                $this->returnCallback(
+                    function ($data) use (&$buffer) {
+                        $buffer .= $data;
+                    }
+                )
+            );
 
         $server->listen($this->socket);
         $this->socket->emit('connection', array($this->connection));
 
         $data = "GET / HTTP/1.1\r\n";
         $data .= "Host: example.com:80\r\n";
         $data .= "Connection: close\r\n";
-        // this is valid behavior according to: https://www.ietf.org/rfc/rfc2616.txt chapter 4.4
-        $data .= "Content-Length: hello world\r\n";
+        $data .= "Content-Length: 5\r\n";
         $data .= "Transfer-Encoding: chunked\r\n";
         $data .= "\r\n";
+        $data .= "hello";
 
         $this->connection->emit('data', array($data));
 
-        $data = "5\r\nhello\r\n";
-        $data .= "0\r\n\r\n";
-
-        $this->connection->emit('data', array($data));
-
-        $this->assertFalse($requestValidation->hasHeader('Content-Length'));
-        $this->assertEquals('chunked', $requestValidation->getHeaderLine('Transfer-Encoding'));
+        $this->assertContains("HTTP/1.1 400 Bad Request\r\n", $buffer);
+        $this->assertContains("\r\n\r\nError 400: Bad Request", $buffer);
+        $this->assertInstanceOf('InvalidArgumentException', $error);
     }
 
     public function testRequestInvalidNonIntegerContentLengthWillEmitServerErrorAndSendResponse()