Invalid Content-Length header (and/or issue with output-buffering)

[mindplay-dk]

The SapiEmitter generates a Content-Length header based on the length of the body stream length.

But it also flushes any existing output buffer, and doesn't take measures to prevent both of those those things happening during the same request.

This can leads to an invalid HTTP response body getting emitted, which e.g. Chrome will refuse to even display.

For example, if you var_dump('something') and then emit, you end up with a broken response.

I suspect this is what leads to bug-reports such as #216.

A check should probably be introduced to prevent this, as it's likely a common mistake - rather than creating an invalid response, we should probably throw an exception?

[mindplay-dk]

No comment?

I'm looking at SapiEmitter::emit() which caused unreadable pages again today.

$response = $this->injectContentLength($response);

$this->emitStatusLine($response);
$this->emitHeaders($response);
$this->flush($maxBufferLevel);
$this->emitBody($response);

There is a serious logical flaw in this code: you cannot inject the content-length until you know what the content-length is going to be - and if you both flush() and emitBody(), if both of those calls emit content, you get an invalid document.

So you have to decide, either:

1. Throw an exception if the output buffer has content.
2. Don't inject the content-length header.
3. Adjust the content-length header injection and allow only output buffer or body emit (not both)

In my opinion, you should be doing (1) - though I realize this is a BC break, it's the only correct thing to do, because no one should be emitting the content part of the response via the output buffer and the other parts (headers etc) via the PSR-7 model.

This is a serious issue, guys - dead, blank pages are difficult to debug, and when this happens in production, it leads to very unhappy clients and users.

[weierophinney]

@mindplay-dk Honestly, I had no idea how to reproduce it until your comment today, making it hard to comment.

I'll discuss with the review team how we should handle this. As you note, whatever approach we take will result in a BC break, but taking no action can leave things unusable.

[davispuh]

Just encountered this too and it took some quite debugging till found cause because all I got was white page without any errors anywhere. Basically what happened is that $response->getBody() was really small so Content-Length was set to that small size but first outputted data was from output buffering which was simply truncated (nothing were sent after Content-Length length) so nothing from actual response body were sent.

[stuartherbert]

+1 on this being a bug with serious consequences.

Had a ZFE app where a ZFE config file was emitting a blank line by mistake (a blank line had been added to the top of a ZFE config file, before the '<?php' marker. This was a mistake made in an Ansible template, therefore only showed up on deployed instances - not local dev environments). As a result, the 'Content-Length' header was out by 1 for every reply the ZFE app made.

cURL will discard any extra bytes after 'Content-Length' bytes in the response - and the PHP extension doesn't tell you that it's done so. cURL from the command-line (by default) doesn't warn you either. End result: client apps were seeing truncated JSON from the ZFE app, but the ZFE app logs showed valid JSON sent.

The serious consequences come from the time it takes to work out what's going on. Because the ZFE app looks like it's working (the logs show valid JSON sent, and the Content-Length header matches the length of the JSON that the app had in the Response object), my client has just spent a week looking everywhere else for the bug. It's the kind of problem that junior folks just aren't going to spot.

One possible fix is to simply stop sending the Content-Length header. Seems appropriate, because there's no 100% reliable way for Diactoros to know what the right value for Content-Length is. Diactoros only knows about what's in the Response object. It can't know about anything else emitted outside that context.

Unless I'm missing something, no value is better than sending the wrong value.

[weierophinney]

So you have to decide, either:

• Throw an exception if the output buffer has content.
• Don't inject the content-length header.
• Adjust the content-length header injection and allow only output buffer or body emit (not both)

In my opinion, you should be doing (1) - though I realize this is a BC break, it's the only correct thing to do, because no one should be emitting the content part of the response via the output buffer and the other parts (headers etc) via the PSR-7 model.

I meant to address this with 1.5.0, but because I'd not created a pull request, missed it. I'm looking into it again now.

The second option is essentially a non-starter, though technically potentially do-able. It would require that we flush() before calling emitStatusLine() and emitHeaders(); however, we'd have to capture the contents of the output buffer(s) to do so, which could lead to other potential errors (out of memory issues being one scenario). Additionally, if we did have content from the output buffer, we now have another issue: where does it go in the response? Before, or after the existing body content? (Technically, we currently throw it before existing content, as we flush before emitting the response body, but, as noted with this issue, that's broken!)

The third already works if there is only buffered content or only PSR-7 response content. In the latter case, the Content-Length header is emitted correctly; in the former, no Content-Length header is emitted.

My current thought is this: we know the content-length by the time we currently call flush(), and whether or not we have a Content-Length header injected in the response. If we were to pass this information to flush() (or just pass the current response to it), we could have it raise an exception on the first non-empty buffer if the response is non-empty. While I agree it is a BC break in behavior, as @mindplay-dk and others have noted, not having the exception leads to inconsistent and unexpected results otherwise.

The problem I've run up against, however, in trying to understand when exactly the buffered content is emitted is that when the $maxBufferLevel is less than or equal to the current buffering level, PHP emits the buffered content first even though we do not call ob_end_flush() ourselves. This means that while I can reproduce the issue (e.g., calling var_dump() in middleware leads to a truncated var_dump, and no actual response content), I'm not 100% sure how to detect the condition, nor where to fix it.

The easiest way to detect the condition would be to loop through every buffer (not just those above the $maxBufferLevel), check for non-empty content, and raise an exception if detected (but only if the response itself is non-empty). The problem with this is that if a user was potentially emitting multiple responses, this can and WILL lead to an exception when emitting the second and any later responses. However, considering this is a SAPI emitter, I think we can argue that's an unsupported use case anyways.

I'll try a few more experiments, and hopefully get a patch up for review in the next day or two.

[weierophinney]

New discovery: if we throw an exception during flush(), but that happens after emitHeaders(), the Content-Length header is still emitted, which means you now get a truncated exception message (assuming you have display_errors enabled) or an empty page (when you do not).

This lends credence to what @stuartherbert suggests: that the emitter simply not auto-inject the header.

Middleware can easily be registered to calculate and inject the Content-Length header:

$app->pipe(function ($request, DelegateInterface $delegate) {
    if ($response->hasHeader('Content-Length')) {
        return $response;
    }
    $length = $response->getBody()->getSize();
    return null === $length
        ? $response
        : $response->withHeader('Content-Length', $length);
});

This still poses a problem, however, as the contents will be truncated and/or inconsistent if var_dump() or any other buffer-altering command has been issued; the Content-Length will be invalid in such a case.

As such, I suggest that the only possible step forward is as follows:

• If the response is empty and does not have a Content-Length header, we can invoke flush() following emitHeaders().
• If the response is non-empty and/or has a Content-Length header, we need to test for non-empty buffers prior to emitting anything, and raise an exception if any are detected.

[weierophinney]

I've spent around 8 hours on this now, and I'm stumped.

I can easily test for an empty response, and, if detected, simply flush the output buffer:

if ($this->isEmptyResponse($response)) {
    $this->flush($maxBufferLevel);
    return;
}

// where:
private function isEmptyResponse(ResponseInterface $response)
{
    if (200 !== $response->getStatusCode()) {
        return false;
    }

    if ([] !== $response->getHeaders()) {
        return false;
    }

    return 0 === $response->getBody()->getSize();
}

That works fine.

The bigger issue is determining what to do after that. I tried writing a method to check for an empty output buffer, and, if not empty, raise an exception, and slip-streaming that call immediately after the conditional checking for an empty response.

I tried three different approaches.

The first, and current, approach, looks like this:

    private function checkForEmptyOutputBuffer($maxBufferLevel = null)
    {
        $maxBufferLevel = $maxBufferLevel ?: ob_get_level();
        $maxBufferLevel = $maxBufferLevel > -1 ? $maxBufferLevel : ob_get_level();

        $bufferLevel = ob_get_level();
        while ($maxBufferLevel <= $bufferLevel && 0 < $bufferLevel) {
            $content = ob_get_contents();
            if ('' !== $content) {
                throw new RuntimeException(
                    'Unable to emit response; content emitted outside request/response lifecycle: '
                    . $content
                );
            }
            ob_end_flush();
            $bufferLevel = ob_get_level();
        }
    }

The second is similar, but does a > check, allowing for removal of the $bufferLevel variable:

private function checkForEmptyOutputBuffer($maxBufferLevel = null)
{
    $maxBufferLevel = $maxBufferLevel ?: ob_get_level();

    while (ob_get_level() > $maxBufferLevel) {
        $content = ob_get_contents();
        if ('' !== $content) {
            throw new RuntimeException(
                'Unable to emit response; content emitted outside request/response lifecycle: '
                . $content
            );
        }
        ob_end_flush();
    }
}

The third approach is brute-force:

private function checkForEmptyOutputBuffer()
{
    while (ob_get_level()) {
        $content = ob_get_contents();
        if ('' !== $content) {
            throw new RuntimeException(
                'Unable to emit response; content emitted outside request/response lifecycle: '
                . $content
            );
        }
        ob_end_flush();
    }
}

The third approach raises problems (risky test detection) in PHPUnit:

Test code or tested code did not (only) close its own output buffers

Essentially, we also end up closing the output buffer PHPUnit initializes; PHPUnit detects that, and flags the test. It's not a failure, but the verbose output gives me pause; it feels like swatting a fly with a hammer.

The second approach raises an issue in that most of the time, the current output buffer level is the SAME AS the $maxBufferLevel, so the loop never gets executed. It took me a while to figure out why the expected exception was not being raised.

The first approach addresses the problems of the second approach, but, believe it or not, still has the problems of the third approach in that the output buffers are not correctly closed. Because PHPUnit's error message is vague, I do not know if I did not close enough buffers, or too many; this, then, also makes me wonder if the approach is viable.

Of more interest is the fact that NONE of these are compatible with the SapiStreamEmitter tests, specifically the testEmitMemoryUsage() test case. This one ends up in an infinite loop if I do any measures to clear the output buffer OTHER THAN the second approach above; as noted, this is still not ideal, because other buffers may still exist, which will be flushed once the PHP process closes, and the approach does not take into account that the current buffer may have content. The only approach I took that works is to create the following method:

    private function clearOutputBuffer($maxBufferLevel)
    {
        $maxBufferLevel = $maxBufferLevel ?: ob_get_level();
        while (ob_get_level() > $maxBufferLevel) {
            ob_end_clean();
        }
    }

and then to call it within emit() immediately prior to the emitStatusLine() call. This demonstrates one of the same problems as above: PHPUnit complains about output buffers. Additionally:

• If there was content in the output buffer, that content is now lost.
• If there are other output buffers wrapping the current one, that content will still be emitted.

What it comes down to is:

1. We can leave the code as-is, and document the problems, so that others can identify causes when they see truncated content.
2. We can do nothing with output buffers, and, again, document this so developers are aware of the ramifications. In this case, Server::listen() would no longer call ob_start(), which means that emit() will typically raise an exception due to headers already being sent. This may be the simplest and most predictable approach, though it might also lead to the most WTF moments when output is present followed by a strack trace.
3. We can use a brute-force approach and clear ALL output buffers before emitting the response. This, too, would need documentation, as it can and will lead to unexpected outcomes if users do var_dump(), var_export(), echo, or other output-generating operations.
4. We can do a brute-force approach (look in all buffers) to *detecting if buffer content exists, and raise an exception in such a case. If we go this route, we need to likely rewrite the SapiStreamEmitterTest::testEmitMemoryUsage() test method (though I have zero ideas on how to do so).

In either of the latter two cases, we likely need to use something like phpt instead of phpunit to test the emitters due to the output buffering issues I've encountered when trying to test these approaches under PHPUnit.

I'm leading towards the second option at this point, but really, really need feedback from those of you who have experienced the issue before proceeding.

[weierophinney]

The second approach mostly works, but requires a check for content in the current output buffer to be robust, as PHP may not yet have flushed that content. You can see a patch here:

• weierophinney@8009285

The primary issue with this approach is that if the developer has called ob_start() manually at any point outside of the call to Server::listen(), you could still end up with a potential for the issues reported.

I think that this approach, however, is the simplest, most robust solution, and we can document the remaining issues to aid developers.

Thoughts?

[davispuh]

It's really hard to say here what would be best solution but it sounds fine.

In our codebase we worked around this issue by simply discarding all output buffers since we assume that they shouldn't contain anything and everything should be written inside Response.

while (ob_get_level() > 0)
{
    ob_end_clean();
}
$emitter->emit($response);

[weierophinney]

@davispuh That was one approach I tried with the SapiStreamEmitter. My main issue with it is that if you have emitted output, there are two possible problems:

• You don't see expected output, and do not know why.
• You have errant echo, var_dump, etc. statements in your code that you forget to remove, because that code masks the fact that they exist.

Both of these are really hard to discover and/or debug, which is what led to the solution I've proposed. It means the library does less to protect the developer from their self, but also ensures that problems like these are exposed clearly and early.

It also means that you can write your own emitter or Server implementation to do like what you suggest, without concern about conflicts with the library.

[stuartherbert]

A few thoughts:

• We can leave out 'Content-Length' (it's an optional header), and accept that the response may get downgraded to HTTP/1.0 (ie the connection to the client will be closed after the PHP SAPI has finished writing the content). A little bit of testing against Nginx, Apache/mod_php and the built-in PHP webserver will show us whether or not they inject the header for us or not.

  If they do inject the header for us, that's awesome, and hoops do not need to be jumped through :)

• If we have to emit 'Content-Length', then it needs to be 100% accurate or it can't be emitted. cURL doesn't care about what anyone documents; it only cares about the 'Content-Length' header we send.

  In my case, this bug wasn't seen by a developer, but by a sysadmin team, and they were utterly baffled by it. They weren't PHP developers, had no idea what the cause was, and even if it was documented in the ZF docs, almost certain they would never have found the docs, and 100% certain that the docs wouldn't have meant anything to them. It's not the world they work in. There just isn't enough of a common frame-of-reference for the docs to make sense to that audience.

  In my experience, it'd be better to trigger a fatal error (eg. ob_flush() before emitting any headers) than have silent, incorrect behaviour. A fatal error gives developers something they can see and track down. It looks like an application crash to sysadmins, and they know that they need to escalate those to a suitable developer. It taps into established workflows that exist up and down the land.

  Do that, and then in the ZF docs tell people to use a PSR3 logger if they need to debug an app in-situ, instead of using echo, var_dump et al. That has the bonus effect of encouraging better developer behaviour too.

Either way, I wouldn't be going to the lengths you are to make 'Content-Length' accurate 90-odd% of the time. That kind of effort and complexity has a nasty habit of breaking somewhere down the road - today's clever code is tomorrow's bug and all that. Maybe I'm just scarred by bitter experience with what people do on top of ZF :)

[weierophinney]

@stuartherbert Would you be able to do some research for me, please, and check to see what each of Apache/mod_php, Apache/fastcgi + phpfpm, nginx + phpfpm, and the built-in PHP webserver do? My guess is one or more of these is not injecting the Content-Length, as this feature was provided by a contributor, which I assume was for a reason... If these are all injecting the header for us (with the exception of the built-in server, which I don't really care about), then I have zero issue dropping the functionality entirely.

However, if any of these are NOT injecting the header, we need some way of injecting it, whether that's via middleware or an emitter. The problems I'm worried about are:

• if we inject it in the emitter, what should be the behavior when there's content in the output buffer?
• if the header is present already in the response, what should we do if we discover content in the output buffer?

In each of those two cases, we need an approach, as they lead to the same issues we're seeing currently - we don't get the entire content back to the user. This could be as simple as removing the header if it was set and we discover content in the current buffer, or it could be doing something like raising an exception.

I just need some guidance on what approach to take — as in, what would be most valuable or helpful to users. 😄

[stuartherbert]

I'll get it done this weekend, and post the results. Did the original contributor provide any details on what problem their contribution fixed? Can we reach out to them and ask for their experience? As mentioned earlier, I wouldn't go looking in the output buffers. I'd flush them, try to emit the headers, and let PHP's existing SAPI code detect that we're writing headers after having sent content. That'll (correctly) show up as an application bug that people can see and do something about. I haven't come up with a credible reason yet why a PSR7 app should accommodate content being emitted from elsewhere in the app code.

…

On 24 Aug 2017, at 22:52, weierophinney ***@***.***> wrote: @stuartherbert Would you be able to do some research for me, please, and check to see what each of Apache/mod_php, Apache/fastcgi + phpfpm, nginx + phpfpm, and the built-in PHP webserver do? My guess is one or more of these is not injecting the Content-Length, as this feature was provided by a contributor, which I assume was for a reason... If these are all injecting the header for us (with the exception of the built-in server, which I don't really care about), then I have zero issue dropping the functionality entirely. However, if any of these are NOT injecting the header, we need some way of injecting it, whether that's via middleware or an emitter. The problems I'm worried about are: if we inject it in the emitter, what should be the behavior when there's content in the output buffer? if the header is present already in the response, what should we do if we discover content in the output buffer? In each of those two cases, we need an approach, as they lead to the same issues we're seeing currently - we don't get the entire content back to the user. This could be as simple as removing the header if it was set and we discover content in the current buffer, or it could be doing something like raising an exception. I just need some guidance on what approach to take — as in, what would be most valuable or helpful to users. 😄 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[weierophinney]

Pinging @mnapoli — you contributed #88, which adds the Content-Length injection in the SapiEmitter; could you help us understand what problem you were solving when you made the contribution? If you read through this issue, you'll see the new problems it leads to that we need to try and solve. Thanks!

[weierophinney]

@stuartherbert —

As mentioned earlier, I wouldn't go looking in the output buffers. I'd flush them, try to emit the headers, and let PHP's existing SAPI code detect that we're writing headers after having sent content. That'll (correctly) show up as an application bug that people can see and do something about.

That would be semantically similar to what I'm attempting now, and my previous attempts as well. The main issue I run into with that is testing, as it causes quite a number of warnings from PHPUnit (not test failures, mind you, but warnings of risky tests and/or tested code, due to flushing the output buffer PHPUnit uses). We may have to find another way to test these classes if we go that route.

[stuartherbert]

Pragmatically, does it need a unit test? It's something that clearly falls outside what unit testing was invented to cover. In systems I've built in the past, I've wrapped this kind of code in @codeCoverageIgnore tags, and tested it at a different layer of testing. (Always a huge advocate of a layered approach :) ). I appreciate that different folks have different approaches :)

…

On 24 Aug 2017, at 23:19, weierophinney ***@***.***> wrote: @stuartherbert — As mentioned earlier, I wouldn't go looking in the output buffers. I'd flush them, try to emit the headers, and let PHP's existing SAPI code detect that we're writing headers after having sent content. That'll (correctly) show up as an application bug that people can see and do something about. That would be semantically similar to what I'm attempting now, and my previous attempts as well. The main issue I run into with that is testing, as it causes quite a number of warnings from PHPUnit (not test failures, mind you, but warnings of risky tests and/or tested code, due to flushing the output buffer PHPUnit uses). We may have to find another way to test these classes if we go that route. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[mnapoli]

Hi, I don't recall exactly why I looked into that but I think it may have been because some downloads did not work with Diactoros (because of the missing header). Whatever the reason I had a look at Slim which did add the header at the time (2015): https://github.com/slimphp/Slim/blob/ca438e786d24d9d96bb3dcb993c33ab8be8af5e9/Slim/App.php#L473 I then opened #84 to suggest adding the header in Diactoros as well.

Slim seems to have changed on this point as well:

• the same problem was discussed in Content-Length header causing truncated content with New Relic slimphp/Slim#1690
• they made the behavior optional in Add option to remove content-length slimphp/Slim#1856
• in Replace addContentLengthHeader with ContentLength Middleware slimphp/Slim#2254 they moved it to a middleware that you add explicitly if you want this behavior (they mention an issue with New Relic which makes sense, I had the same issue IIRC)

So I would say "go for it" and remove that behavior? But there may be some problems with some kind of HTTP responses that need a Content-Length, I don't know HTTP well enough to know which one though…

[stuartherbert]

Some test results ...

Server	HTTP Response	Behaviour
Nginx + PHP-FPM	HTTP/1.1	No 'Content-Length' header, sets 'Transfer-Encoding: chunked' instead
Apache2 + mod_php	HTTP/1.1	Sets 'Content-Length' header
Apache2 + PHP-FPM	HTTP/1.1	No 'Content-Length' header, sets 'Transfer-Encoding: chunked' instead

Test environment:

• All tests done on Ubuntu 16.04 LTS, with currently uses PHP 7.0.22.
• The docker images are available in https://github.com/ganbarodigital/docker-images (I'll tag and push the docker images for Apache2 up to Docker Hub later on).
• Test code was a skeleton ZE2 app, with SapiEmitterTrait::injectContentLength() forced to return $response on line 24.

What do these results mean? I think they're really good news.

• No connection was downgraded to HTTP/1.0.
• Every response will work with a compliant HTTP client implementation.
• No evidence that the SapiEmitterTrait needs to inject a Content-Length header by default.

There will be problems around non-compliant / incomplete HTTP client implementations - ones that do not correctly / fully support HTTP/1.1 streaming (chucked transport encoding). They are going to need a 'Content-Length' header from somewhere.

My advice is:

1. move emitContentLength() into an optional middleware
2. add a question about adding this new middleware into the skeleton app installer (default answer 'N')

I've changed my mind on ob_flush() before emitting headers. I wouldn't call it from Diactoros. That way, folks have the option of configuring output_buffering in their php.ini file if they want to mix out-of-band output with the response object.

[mindplay-dk]

I'm mostly with @stuartherbert on this subject, the last remark of his last post: stay away from output buffering entirely. It's global state. You shouldn't be mixing, at all - from my point of view, it defeats the entire purpose of PSR-7 and Diactoros, which was to avoid the aches and pains caused by using anything that manipulates or depends upon global state.

We're struggling to take control of something that is almost by definition out of control.

From my point of view, you can simplify the whole thing and boil it down to:

if (headers_sent() || (ob_get_level() > 0 && ob_get_length() > 0)) {
    throw new GlobalStateException();
}

In other words, don't allow global state in the mix.

In my opinion, you either choose PSR-7 and work with it, or you don't.

As demonstrates by all the edge cases and marginal issues with every conceivable work-around discussed above, there isn't really any peaceful way to reconcile global state mixed with something designed to avoid global state. I personally wouldn't struggle to provide a solution for those who are unwilling to move away from global state.

Note that this doesn't block anyone from integrating with legacy scripts that do use output buffering - they'll simply need to capture the output and take control of it, by injecting it into a Response object; or in other words, integrate it properly with PSR-7 and Diactoros which they've elected to use.

I don't think Diactoros needs to enable code that relies on global state - I think it's actually better to help developers learn how to avoid it.

[weierophinney]

Thanks for the research and feedback, @stuartherbert, and thanks to @mindplay-dk for the follow-up.

I'm currently recovering from pneumonia, but plan to tackle this soon when I've got a bit more energy. I'll ping each of you with the pull request I create.

[weierophinney]

I've created Zend\Expressive\Helper\ContentLengthMiddleware in the zend-expressive-helpers 4.1.0 release: https://github.com/zendframework/zend-expressive-helpers/releases/tag/4.1.0.

TODO:

• Add the Content-Length middleware to zend-expressive-helpers
• Document the middleware in the Expressive documentation
• Create a PR removing support for the Content-Length header injection in this library; that PR should also remove any flush() functionality.

I have decided against prompting to add the ContentLengthMiddleware during initial Expressive installation. Knowing that omitting the Content-Length does not cause fallback to HTTP/1.0, and that we have only one specific use case where the header is required (New Relic), I think having documentation covering its registration and usage will be sufficient; users can then add it if and only if they need it, and delay that decision until they know for certain.

[franzliedke]

Whew, nice, thanks for fixing this! I think we (Flarum) have been bitten by this before as well. :)