Decode chunked transfer encoding for incoming requests #116

legionth · 2017-02-13T14:49:50Z

This ensures that only decoded body data will be emitted via the request object.

Resolves / closes #96

clue

Nice! This seems much more approachable than the previous PR, only added some minor remarks afaict 👍

clue · 2017-02-14T06:09:15Z

src/ChunkedDecoder.php

+                if (strpos($header, ';') !== false) {
+                    $array = explode(';', $header);
+                    $hexValue = $array[0];
+                    $start = strlen($header) + 2;


It looks like $start may always be $positionClrf + 2, so this var may not be needed at all?

clue · 2017-02-14T06:10:22Z

src/ChunkedDecoder.php

+                    $start = strlen($header) + 2;
+                }
+
+                if (dechex(hexdec($hexValue)) !== $hexValue || hexdec($hexValue) > 2147483647) {


Is it possible to get negative numbers here? Is the additional bound check really needed?

clue · 2017-02-14T06:10:56Z

src/ChunkedDecoder.php

+                    return;
+                }
+
+                $this->chunkSize = hexdec($hexValue);


LGTM, but can probably moved up as to avoid some function calls? 👍

clue · 2017-02-14T06:11:46Z

src/ChunkedDecoder.php

+
+                if ($positionClrf === false) {
+                    // Header shouldn't be bigger than 1024 bytes
+                    if (strlen($this->buffer) > 1024) {


LGTM, but can probably also use isset($this->buffer[$max]) to avoid some function calls? 👍

clue · 2017-02-14T06:13:33Z

tests/ChunkedDecoderTest.php

+        $this->parser->on('end', $this->expectCallableNever());
+        $this->parser->on('error', $this->expectCallableNever());
+
+        $this->input->emit('data', array("7FFFFFFE\r\n"));


Looks like a 32bit assumption here? (see also above)

clue · 2017-02-14T06:16:37Z

src/ChunkedDecoder.php

+                if ($this->transferredSize > $this->chunkSize) {
+                    $this->handleError(new \Exception('The chunk is bigger than expected'));
+                    return;
+                }


Not sure I understand this check, should this not be limited by the above substr() already? (see also below CRLF check)

clue · 2017-02-14T06:18:54Z

src/ChunkedDecoder.php

+                $this->buffer = (string)substr($this->buffer, strlen($chunk));
+            }
+
+            if (strpos($this->buffer, static::CRLF) !== false) {


This checks for any CRLF? What if the buffer is empty/incomplete here? Shouldn't this have to buffer and/or reject if there's not a CRLF at the head position instead?

Is this resolved? This still checks for any position afaict? 4\r\ntestWOOT\r\n

clue · 2017-02-14T06:19:27Z

src/ChunkedDecoder.php

+    {
+        $this->emit('error', array($e));
+        $this->input->removeListener('data', array($this, 'handleData'));
+        $this->close();


Doesn't closing already remove all listeners? (see also below)

legionth · 2017-02-14T11:31:50Z

I think, I have handled all the remarks @clue had. Have a look.

clue · 2017-02-14T11:41:33Z

src/ChunkedDecoder.php

+        $this->input->removeListener('data', array($this, 'handleData'));
+        $this->input->removeListener('end', array($this, 'handleEnd'));
+        $this->input->removeListener('error', array($this, 'handleError'));
+        $this->input->removeListener('close', array($this, 'close'));


Unneeded? Closing the input stream should remove all listeners.

clue · 2017-02-14T11:42:24Z

src/ChunkedDecoder.php

+                if ($positionClrf === false) {
+                    // Header shouldn't be bigger than 1024 bytes
+                    if (isset($this->buffer[1024])) {
+                        $this->handleError(new \Exception('Chunk size inclusive extension bigger than 1024 bytes'));


Message should refer to size of "chunk header"?

clue · 2017-02-14T11:42:37Z

src/ChunkedDecoder.php

+
+                if ($positionClrf === false) {
+                    // Header shouldn't be bigger than 1024 bytes
+                    if (isset($this->buffer[1024])) {


magic constant?

clue · 2017-02-14T11:43:20Z

src/ChunkedDecoder.php

+                }
+
+                $this->chunkSize = hexdec($hexValue);
+                if (dechex($this->chunkSize) !== $hexValue || $hexValue < 0) {


Not seeing a test for $hexValue < 0? Is this needed? Documentation suggests otherwise

clue · 2017-02-14T11:44:53Z

src/ChunkedDecoder.php

+                $this->buffer = (string)substr($this->buffer, strlen($chunk));
+            }
+
+            if (strpos($this->buffer, static::CRLF) !== false) {


Is this resolved? This still checks for any position afaict? 4\r\ntestWOOT\r\n

clue · 2017-02-14T11:45:28Z

src/ChunkedDecoder.php

+            if ($this->headerCompleted) {
+                if (strlen($this->buffer) > 2 && $this->chunkSize === $this->transferredSize) {
+                    // Send error event, the first 2 characters should be CLRF
+                    $this->handleError(new \Exception('Chunk does not end with a CLRF'));


Not sure I'm not missing something, but where do we actually check "the first 2 characters should be CLRF"? (see also above)

I can't comment the previous comment, but: Yes 4\r\ntestWOOT\r\n is handled. See tests testNoCrlfInChunk and testNoCrlfInChunkSplitted in ChunkedDecoderTests.

To this comment. This has to be seen in relation to the previous if-statements. If the previous statement couldn't find a CLRF, this is still buffered. But only if the current string length of the buffer isn't >2. That would mean that the data in the buffer isn't correct chunked encoding.

clue · 2017-02-14T11:48:25Z

src/ChunkedDecoder.php

+                    // Send error event, the first 2 characters should be CLRF
+                    $this->handleError(new \Exception('Chunk does not end with a CLRF'));
+                }
+                return;


Is this needed? Doesn't the next iteration check this? Perhaps this is just a bit unclear and could use some comments?

clue · 2017-02-14T11:54:02Z

tests/ServerTest.php

+        $data .= "2\r\nhi\r\n";
+
+        $this->connection->emit('data', array($data));
+        $this->assertEquals('hello', $buffer);


Can probably use expectCallableOnceWith('hello'); here? Also, can you add tests for the end, error and close events as well? (See also other test cases)

legionth · 2017-02-14T13:36:58Z

Is this resolved? This still checks for any position afaict? 4\r\ntestWOOT\r\n
Show outdated

The test case for this were valid because the header check have failed for this. Another test like 4\r\ntest3\r\nbla, would led to a misbehaviour.

With the newest commits. This should be fixed.

clue

Awesome! 🎉

Hate being that fussy in review, but I believe these results show it's well worth it, appreciate your endurance 👍

clue · 2017-02-14T13:55:45Z

For the reference: The changes LGTM, but we've had to move this to another milestone and we're going to release the v0.5.0 first, so we won't be able to merge this immediately 👍

jsor · 2017-02-14T14:48:00Z

src/ChunkedDecoder.php

+class ChunkedDecoder extends EventEmitter implements ReadableStreamInterface
+{
+    const CRLF = "\r\n";
+    const MAX = 1024;


Should probably be more descriptive, eg. MAX_CHUNK_HEADER_SIZE

legionth · 2017-02-15T10:31:29Z

Thank you for your input @jsor. Changed the variable name 😄

WyriHaximus · 2017-02-15T10:47:39Z

@legionth Just skimmed over the test, and something I've done on react/http-client is run a valid chunked encoded body per character through the decoder to ensure it is valid no matter how slow and in what kind of chunks it comes in: https://github.com/reactphp/http-client/blob/master/tests/DecodeChunkedStreamTest.php#L36 is it an idea to do that here as well?

legionth · 2017-02-15T13:44:22Z

@WyriHaximus good idea 👍 . Added an tests, have a look.

WyriHaximus · 2017-02-15T20:24:21Z

src/ChunkedDecoder.php

+    public function handleEnd()
+    {
+        if (!$this->closed) {
+            $this->handleError(new \Exception('Unexpected `end` event'));


Are these ` intended here?

WyriHaximus · 2017-02-15T20:25:23Z

src/ChunkedDecoder.php

+                if ($positionClrf === false) {
+                    // Header shouldn't be bigger than 1024 bytes
+                    if (isset($this->buffer[static::MAX_CHUNK_HEADER_SIZE])) {
+                        $this->handleError(new \Exception('Chunk header size inclusive extension bigger than 1024 bytes'));


Could we use static::MAX_CHUNK_HEADER_SIZE in the error message, so in case we decide to change it we only have to do it once?

WyriHaximus · 2017-02-15T20:35:57Z

src/ChunkedDecoder.php

+            } else if (strlen($this->buffer) < 2) {
+                // No CLRF found, wait for additional data which could be a CLRF
+                return;
+            }


Tbh not really a fan of else and else if. Personally I prefer a structure like this:

if () { return; } if () { return; } if () { return; }

legionth · 2017-02-15T22:01:16Z

Ping @WyriHaximus . Changed the code based on your remarks.

WyriHaximus · 2017-02-16T07:00:06Z

src/ChunkedDecoder.php

@@ -139,11 +141,15 @@ public function handleData($data)
                $this->headerCompleted = false;
                $this->transferredSize = 0;
                $this->buffer = (string)substr($this->buffer, 2);
-            } else if ($this->chunkSize === $this->transferredSize && strlen($this->buffer) > 2) {


The idea behind my comment was to put a return here as well. The reasoning behind this is a) we're done after this point of code and there is no reason to continue running code in this function. And b) is lowers cognitive load on developers reading and working on this code.

In case you're interested in these little code readability optimizations I would suggest https://www.youtube.com/watch?v=GtB5DAfOWMQ if you haven't seen it. If you have my apologies 😄

WyriHaximus · 2017-02-16T07:03:30Z

@legionth Splendid work 👍 . As far as I'm concerned my last comment is not a reason to not merge this, unless @jsor or @clue have any more comments this can be merged in once 0.5.0 has been tagged

clue · 2017-02-16T18:28:10Z

Functionally LGTM and I'd love to get this in ASAP

Can you squash this to a reasonable number of commits? 👍

legionth · 2017-02-17T00:13:47Z

Squashed the commits. I hope this is a reasonable number of commits. Tell me if not.

WyriHaximus · 2017-02-17T06:48:28Z

3 commits looks perfectly reasonable to me 👍

clue · 2017-02-17T21:02:37Z

I'd like to get these changes in, can you rebase this now that #123 is in?

Fix Endless loop Fix Add chunk size check and chunk extension handling Handle potential test cases Add ChunkedDecoder Tests Handle potential threat Rename variable Added test to add verify single characters can be emitted Fixing remarks Use Mockbuilder

Add ServerTest Fix Order

legionth · 2017-02-19T14:14:58Z

Rebased on the current master

legionth force-pushed the chunk-encoding branch 3 times, most recently from 20ab519 to c830685 Compare February 13, 2017 15:37

clue requested changes Feb 14, 2017

View reviewed changes

clue added the new feature label Feb 14, 2017

clue added this to the v0.4.6 milestone Feb 14, 2017

clue requested changes Feb 14, 2017

View reviewed changes

clue approved these changes Feb 14, 2017

View reviewed changes

clue modified the milestones: v0.5.1, v0.4.6 Feb 14, 2017

jsor requested changes Feb 14, 2017

View reviewed changes

jsor approved these changes Feb 15, 2017

View reviewed changes

WyriHaximus requested changes Feb 15, 2017

View reviewed changes

WyriHaximus reviewed Feb 16, 2017

View reviewed changes

WyriHaximus approved these changes Feb 16, 2017

View reviewed changes

jsor approved these changes Feb 16, 2017

View reviewed changes

clue modified the milestones: v0.6.0, v0.5.1 Feb 16, 2017

legionth force-pushed the chunk-encoding branch from dc57a12 to 17d5df8 Compare February 17, 2017 00:12

clue mentioned this pull request Feb 17, 2017

Closing request should stop reading from connection #123

Merged

legionth added 2 commits February 19, 2017 15:03

Add TestCase methods

070e3f0

Add Chunked Decoder class

1a1b0aa

Fix Endless loop Fix Add chunk size check and chunk extension handling Handle potential test cases Add ChunkedDecoder Tests Handle potential threat Rename variable Added test to add verify single characters can be emitted Fixing remarks Use Mockbuilder

legionth force-pushed the chunk-encoding branch 2 times, most recently from 5dea067 to 4fd5d8f Compare February 19, 2017 14:12

Add ChunkedDecoder to Server

61d7b69

Add ServerTest Fix Order

legionth force-pushed the chunk-encoding branch from 4fd5d8f to 61d7b69 Compare February 19, 2017 14:14

clue merged commit 8dfc3a3 into reactphp:master Feb 19, 2017

clue mentioned this pull request Feb 19, 2017

Answer with HTTP/1.0 until 1.1 is really implemented #12

Closed

This was referenced Mar 2, 2017

Emit end event for requests without body #136

Merged

Remove 'Transfer-Encoding' before emitting to the request event #137

Merged

clue mentioned this pull request Mar 7, 2017

Support detecting request body #104

Closed

Decode chunked transfer encoding for incoming requests #116

Decode chunked transfer encoding for incoming requests #116

Conversation

legionth commented Feb 13, 2017

clue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legionth commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legionth Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legionth commented Feb 14, 2017

clue left a comment • edited Loading

Choose a reason for hiding this comment

clue commented Feb 14, 2017

Choose a reason for hiding this comment

legionth commented Feb 15, 2017

WyriHaximus commented Feb 15, 2017

legionth commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legionth commented Feb 15, 2017

Choose a reason for hiding this comment

WyriHaximus commented Feb 16, 2017

clue commented Feb 16, 2017

legionth commented Feb 17, 2017 • edited Loading

WyriHaximus commented Feb 17, 2017

clue commented Feb 17, 2017

legionth commented Feb 19, 2017

legionth Feb 14, 2017 •

edited

Loading

clue left a comment •

edited

Loading

legionth commented Feb 17, 2017 •

edited

Loading