Support buffering requests with unknown length (Transfer-Encoding: chunked) in RequestBodyBufferMiddleware #235

andig · 2017-10-09T06:16:46Z

Implement chunked response handling, following discussion in #232

andig · 2017-10-13T17:39:38Z

@WyriHaximus I need your help with rebasing this for reactphp/promise-stream#3. In the tests there's currently this for the RequestBodyBufferMiddlewareTest:

public function testExcessiveSizeReturnsError413()
{
    $stream = new BufferStream(2);
    $stream->write('aa');

    $serverRequest = new ServerRequest(
        'GET',
        'https://example.com/',
        array(),
        $stream
    );

    $buffer = new RequestBodyBufferMiddleware(1);
    $response = $buffer(
        $serverRequest,
        function () {}
    );

    $this->assertSame(413, $response->getStatusCode());
}

Now what goes in is NOT a ReadableStreamInterface and therefore returned early before any buffering in RequestBodyBufferMiddleware.php.

Is that really desired, i.e. should it be possible to construct ServerRequest with a body that doesn't implement Psr7 semantics + ReadableStreamInterface (and therefore actually is a HttpBodyStream? Hidden bug or feature that I fail to grasp?

clue · 2017-10-13T18:13:08Z

@andig Have you seen the documentation for the RequestBodyBufferMiddleware? I think that this middleware should support all valid requests. In a normal stack this will be the incoming streaming request, but another prior middleware handler may also expose a buffered request to this middleware handler.

The following (completely contrived) example should allow request up to 8 MiB:

$middleware = new MiddlewareRunner([
    new RequestBodyBufferMiddleware(16 * 1024 * 1024), // 16 MiB
    new RequestBodyBufferMiddleware(8 * 1024 * 1024), // 8 MiB
});

clue · 2017-10-13T18:15:33Z

src/Middleware/RequestBodyBufferMiddleware.php

-
-        if ($size > $this->sizeLimit) {
-            return new Response(413, array('Content-Type' => 'text/plain'), 'Request body exceeds allowed limit');
-        }


We should probably keep this check: it makes sense to fail early in case the client is trying to upload a gigabyte file over a slow uplink :-)

Fixed. Please forgive me for force-pushing. I didn't realize you were already reviewing.

No worries, happy to see you're faster with coding than I am with reviewing

andig · 2017-10-13T18:34:05Z

Small side note: I've also changed mentions of g7 and Psr7Implemention to Psr7 for sake of coding consistency.

andig · 2017-10-13T18:40:33Z

@clue

Have you seen the documentation for the RequestBodyBufferMiddleware? I think that this middleware should support all valid requests.

Reading this:

This can be useful if full PSR-7 compatibility is needed for the request handler and the default streaming request body handling is not needed.

I understand the behaviour. It's still totally confusing though that- if you use the RequestBodyBufferMiddleware and send in a react stream- it can be of any size (as long as it doesn't tell you which size) and will not be buffered or rejected simply because the RequestBodyBufferMiddleware will not do its job in this case.

Shouldn't at least the RequestBodyBufferMiddleware reject non-Psr7 bodies as they are used in the tests? Along the lines of "you want buffered request bodies, so you should send in valid Psr requests"?

The following (completely contrived) example should allow request up to 8 MiB ... In a normal stack this will be the incoming streaming request, but another prior middleware handler may also expose a buffered request to this middleware handler.

The buffer outputs BufferStream which is of StreamInterface and can be fed to the next buffer. Imho you should not have a middleware in between that returns ReadableStreamInterfaces or similar simply because after the buffer you should be able to expect 100% compilant Psr7 request bodies?

Update maybe my explanation is too complicated. The Server wraps every kind of body inside a HttpBodyStream which implements StreamInterface. No middleware on the stack should remove or accept other interfaces than StreamInterface - or we should have a big disclaimer in the docs that functionality may depend on type of input stream?

clue · 2017-10-13T21:33:33Z

I'm not sure I follow tbh. Our middleware implementation works around the concept of PSR-7 requests coming in and PSR-7 responses coming out (plus wrapped in promises). This means that there's no such thing as a "non-Psr7 body".

What's special about this implementation is that bodies can be in a streaming state when a ReadableStreamInterface is attached. This allows us to transport bodies of arbitrary sizes without having to store huge bodies in memory.

Does this help?

andig · 2017-10-14T12:29:14Z

I'm not sure I follow tbh.

Never mind. I've just noticed that I had to rewrite https://github.com/reactphp/http/pull/235/files#diff-5ff94d7c5e50a34ed39b11c33717c0b9R68 to use a different stream type. I'm probably making this too complicated ;)

WyriHaximus · 2017-10-14T22:48:17Z

src/Server.php

@@ -3,13 +3,12 @@
 namespace React\Http;


Could you revert all changes in this file?

Will open separate PR for Psr7 but imho at least this change https://github.com/reactphp/http/pull/235/files#diff-c611f5024b5b13f69f779505d1c6735eR170 makes sense.

Do you mind the reordering of the use clauses by library?

WyriHaximus · 2017-10-14T22:48:55Z

src/Response.php

@@ -21,7 +21,7 @@ public function __construct(
        $version = '1.1',
        $reason = null
    ) {
-        if ($body instanceof ReadableStreamInterface) {
+        if ($body instanceof ReadableStreamInterface &! $body instanceof HttpBodyStream) {


Could you revert all changes in this file?

Artifact removed.

WyriHaximus · 2017-10-14T22:49:25Z

src/RequestHeaderParser.php

@@ -92,7 +92,7 @@ private function parseRequest($headers)
        }

        // parse request headers into obj implementing RequestInterface
-        $request = g7\parse_request($headers);
+        $request = Psr7\parse_request($headers);


Any reason why you changed this? Or is this left over from earlier changes in this PR?

I wanted to have Psr7 consistently named in the codebase (not Psr7, g7, and PsrImplementation). Could move to separate PR?

WyriHaximus · 2017-10-14T22:50:06Z

src/HttpBodyStream.php

@@ -25,7 +25,7 @@ class HttpBodyStream extends EventEmitter implements StreamInterface, ReadableSt
     * @param ReadableStreamInterface $input - Stream data from $stream as a body of a PSR-7 object4
     * @param int|null $size - size of the data body
     */
-    public function __construct(ReadableStreamInterface $input, $size)
+    public function __construct(ReadableStreamInterface $input, $size = null)


Could you revert all changes in this file?

Imho this change makes sense. It's aligned with the comment now, the constructor handles null and it prevents explicitly passing null for bodies of unknown length?

I feel that this adds noise and adds no value and as part of this PR, maybe suggest this in a separate PR? 👍

Despite this, I don't see what value this would provide, as this is an internal class only and this change basically means you're allowed th instantiate the class without considering what its "size" would be. If you feel this is relevant, I encourage you to file a PR so we can discuss this further 👍

WyriHaximus · 2017-10-14T23:08:26Z

@andig Seems you're getting there, keep up the good work 👍

WyriHaximus · 2017-10-18T15:27:55Z

@andig 🎉 https://github.com/reactphp/promise-stream/releases/tag/v0.1.2 🎉

WyriHaximus · 2017-10-19T15:08:39Z

@andig updated this PR's title to something that reflects what the current code is doing

andig · 2017-10-19T15:25:18Z

I've added one more commit to test for 413 in case on known but excessive content length. Still need to have another look at the README.

WyriHaximus

LGTM 🎉

WyriHaximus · 2017-10-24T16:50:56Z

@andig could you update to ^1.0 || ^0.1.2 since 1.0 has been release earlier today? 🎉

clue

Thank you for keeping this updated! I feel this could use another higher level test to ensure consistent behavior (functional test?) and a README update, otherwise LGTM 👍

clue · 2017-10-24T16:57:15Z

README.md

+unknown size (i.e. with `Transfer-Encoding: chunked` header). To simplify request 
+handling for downstream middlewares or applications buffered `chunked` requests will 
+have the `Transfer-Encoding` header removed and a `Content-Length` header with the 
+actual request body size received added instead.


README updates are much appreciated 👍 However, I feel this is a bit misleading here, as this makes it sounds like this is what this middleware handler does, whereas this is actually handled by the Server itself. Also, the word "middlewares" doesn't exist (think "softwares"), I usually use "middleware handlers" instead :-) Can you rephrase this? 👍

I think it has actually become a duplicate since the response section already describes the behaviour. Removed.

clue · 2017-10-24T16:58:46Z

README.md

+actual request body size received added instead.
+
+Note that this only affects incoming requests, the chunked transfer encoding for 
+outgoing responses is not affected.


This sentence made much more sense for the old behavior where these requests were rejected. Does it make sense to you to remove this sentence?

Yep, removed.

clue · 2017-10-24T19:09:42Z

src/Middleware/RequestBodyBufferMiddleware.php

+        }, function($error) {
+            if ($error instanceof OverflowException) {
+                return new Response(413, array('Content-Type' => 'text/plain'), 'Request body exceeds allowed limit');
+            }


It looks like this error handles the same condition as above while it actually does not. I suppose this could use some comments and and explicit test. Also, it looks like this should also close the incoming request to stop wasting incoming bandwidth if the limit is exceeded. Can you also add a test for this?

this could use some comments and and explicit test

I'll add two comments:

// request body of known size exceeding limit // request body of unknown size exceeding limit during buffering

It is imho also tested: https://github.com/reactphp/http/pull/235/files/635a9659159e195c8dc3614439858a7535ade0c5#diff-5ff94d7c5e50a34ed39b11c33717c0b9R68 and https://github.com/reactphp/http/pull/235/files/635a9659159e195c8dc3614439858a7535ade0c5#diff-5ff94d7c5e50a34ed39b11c33717c0b9R68. Or I'm misunderstanding you.

Also, it looks like this should also close the incoming request to stop wasting incoming bandwidth if the limit is exceeded

This is as per original code which didn't close the body either. Is this responsibility of the middleware or the server? I.e. who should close the body if open after the middleware stack has finished or a middleware doesn't consume the body?

I think this is a relevant point, let's postpone this discussion for now and address this in a follow-up ticket 👍

See for example https://stackoverflow.com/questions/14250991/is-it-acceptable-for-a-server-to-send-a-http-response-before-the-entire-request

Understood. Looking forward to your insights once this is in.

andig · 2017-10-24T21:01:24Z

src/Middleware/RequestBodyBufferMiddleware.php

            return new Response(413, array('Content-Type' => 'text/plain'), 'Request body exceeds allowed limit');
        }

-        $body = $request->getBody();
        if (!$body instanceof ReadableStreamInterface) {


Shouldn't this check be moved up as this type of stream won't be buffered anyway? Why complain about size in this case (shouldn't happen when using the server at all, might as well just throw)?

IMO it makes sense to keep this like this in (the rare) case this middleware is used behind another buffering middleware handler, see also #235 (comment) 👍

@clue What I mean is that- if not a ReadableStreamInterface, which means we won't buffer it, we're still returning 413 although the middleware is not responsible for handling this request as it is right now.

My suggestion would be to move the ReadableStreamInterface to the Beginning of the method?

ping @clue could you check my question above?
What's missing to make make progress with this PR (and 0.8 in general)?

Well, technically buffering and limiting sizes are in fact independent responsibilities. For practical reasons however, I think that it makes sense to bring them close together because buffering without applying a limit is actually harmful (potential for trivial DOS otherwise). If we accept the fact that checking size limits here, then I think it makes sense to also support buffered requests here. This means that I think it makes sense to also reject buffered requests that exceed the given size limits.

Accordingly, I think this code LGTM 👍 What do you think about this?

What do you think about this?

@clue scratch head. I'm not getting my point across. We're only buffering and limiting PSR streams. We are however still limiting react-only streams.

This is imho wrong- as the buffering middleware is not responsible for these requests. I'm proposing- to be explicit- to change order of things like this:

public function __invoke(ServerRequestInterface $request, $stack) { $body = $request->getBody(); if (!$body instanceof ReadableStreamInterface) { return $stack($request); } // request body of known size exceeding limit if ($body->getSize() > $this->sizeLimit) { return new Response(413, array('Content-Type' => 'text/plain'), 'Request body exceeds allowed limit'); }

Yes, it looks like we're talking past one another 🤷‍♂️ :-)

The getBody() method is guaranteed to always return an instance of Psr\Message\StreamInterface. This very instance may also implement the React\Stream\ReadableStreamInterface ("streaming state"), but this is not guaranteed (already in "buffered state").

When this middleware handler is used as documented, then the body is always in a "streaming state". If one were to use this middleware handler twice in the same stack (conceived example), then the first will see a body in "streaming state" while the second will see it in "buffered state".

If this conceived(!) second middleware handler uses a smaller size limit (which I would expect to be more of a programming error than intentional), then IMO the second middleware handler should still reject the message in my opinion.

If I understand your above suggestion correctly, then it looks like this would actually allow the body to pass through without applying the second limit at all.

I think it makes sense to add a test for this behavior to ensure that this is properly covered. Can you add a test for this or would you rather want me to add this to this PR? 👍

clue · 2017-11-13T17:09:59Z

src/Middleware/RequestBodyBufferMiddleware.php

+                return new Response(413, array('Content-Type' => 'text/plain'), 'Request body exceeds allowed limit');
+            }
+
+            return $error;


This probably shouldn't return an Exception here :-) Can you fix this and add a test?

@clue good catch! Undecided what to return though- right now it's an HTTP 400 with the Exception message as text. Do you have a better suggestion?

"it depends" is probably the best answer here, so I think that this would be best addressed in a separate PR 👍 As such, I would suggest restoring the original behavior of simply returning a rejected promise (throwing an exception).

Done. Do you think we need to guard for error actually being an exception and not something else?

WyriHaximus · 2017-11-17T13:58:00Z

What is the status here @clue and @andig?

andig · 2017-11-17T15:00:23Z

Needs review of the last 2 commits

clue

@andig Thank you so much for your patience and for keeping up with, the changes LGTM! 👍 Now let's get this in, cheers! 🍻 🎉

I've rebased your changes due to merge conflicts with master and squashed this down to 2 commits.

WyriHaximus

👍 🎉

andig mentioned this pull request Oct 9, 2017

RequestBodyBufferMiddleware: 411 No Content Length for GET requests? #232

Closed

andig force-pushed the chunked branch 2 times, most recently from 1a593ee to 830f9f4 Compare October 10, 2017 14:23

andig mentioned this pull request Oct 12, 2017

Roadmap to stable v1.0.0 #120

Closed

andig force-pushed the chunked branch 3 times, most recently from 2551059 to 400d0da Compare October 13, 2017 18:02

clue reviewed Oct 13, 2017

View reviewed changes

andig force-pushed the chunked branch from 400d0da to b724a55 Compare October 13, 2017 18:26

andig changed the title ~~[WIP] Draft receiving chunked requests~~ Handle chunked requests Oct 13, 2017

WyriHaximus requested changes Oct 14, 2017

View reviewed changes

andig force-pushed the chunked branch from 2e60c34 to 80f52e6 Compare October 15, 2017 09:32

andig mentioned this pull request Oct 15, 2017

Use Psr7 consistently #239

Closed

WyriHaximus added this to the v0.8.0 milestone Oct 19, 2017

WyriHaximus requested review from jsor, WyriHaximus and clue October 19, 2017 15:03

WyriHaximus changed the title ~~Handle chunked requests~~ Add chunked encoding support to RequestBodyBufferMiddleware Oct 19, 2017

WyriHaximus changed the title ~~Add chunked encoding support to RequestBodyBufferMiddleware~~ Support requests without length in RequestBodyBufferMiddleware Oct 19, 2017

andig force-pushed the chunked branch from d6b95a9 to f044fd8 Compare October 19, 2017 15:24

WyriHaximus approved these changes Oct 19, 2017

View reviewed changes

jsor approved these changes Oct 21, 2017

View reviewed changes

clue reviewed Oct 24, 2017

View reviewed changes

andig commented Oct 24, 2017

View reviewed changes

clue reviewed Nov 13, 2017

View reviewed changes

andig force-pushed the chunked branch from a7d9174 to 15a0ca1 Compare November 14, 2017 17:32

clue mentioned this pull request Nov 17, 2017

Respect ini settings for file uploads #250

Closed

andig added 2 commits November 20, 2017 19:22

Implement buffering for requests with chunked transfer encoding

cdf9deb

Handle errors during buffering

cbf14eb

clue added HTTP/1.1 new feature labels Nov 20, 2017

clue force-pushed the chunked branch from 15a0ca1 to cbf14eb Compare November 20, 2017 18:28

clue approved these changes Nov 20, 2017

View reviewed changes

WyriHaximus requested review from jsor and WyriHaximus November 20, 2017 18:42

jsor approved these changes Nov 20, 2017

View reviewed changes

clue changed the title ~~Support requests without length in RequestBodyBufferMiddleware~~ Support buffering requests with unknown length (Transfer-Encoding: chunked) in RequestBodyBufferMiddleware Nov 20, 2017

WyriHaximus approved these changes Nov 20, 2017

View reviewed changes

WyriHaximus merged commit 1d5bd0b into reactphp:master Nov 20, 2017

andig deleted the chunked branch November 21, 2017 09:35

Support buffering requests with unknown length (Transfer-Encoding: chunked) in RequestBodyBufferMiddleware #235

Support buffering requests with unknown length (Transfer-Encoding: chunked) in RequestBodyBufferMiddleware #235

Conversation

andig commented Oct 9, 2017 • edited Loading

andig commented Oct 13, 2017 • edited Loading

clue commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andig commented Oct 13, 2017

andig commented Oct 13, 2017 • edited Loading

clue commented Oct 13, 2017

andig commented Oct 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WyriHaximus commented Oct 14, 2017

WyriHaximus commented Oct 18, 2017

WyriHaximus commented Oct 19, 2017

andig commented Oct 19, 2017

WyriHaximus left a comment

Choose a reason for hiding this comment

WyriHaximus commented Oct 24, 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

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

andig Nov 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WyriHaximus commented Nov 17, 2017

andig commented Nov 17, 2017

clue left a comment

Choose a reason for hiding this comment

WyriHaximus left a comment

Choose a reason for hiding this comment

andig commented Oct 9, 2017 •

edited

Loading

andig commented Oct 13, 2017 •

edited

Loading

andig commented Oct 13, 2017 •

edited

Loading

andig Nov 14, 2017 •

edited

Loading