Hacker bots causing 500 error "Unrecognized protocol version"

[rodmcnew]

We have some hacker bots hitting our site with invalid http protocol headers. This causes an exception to throw on the following line and a 500 error. This is annoying because it shows in our error logs as a server error.

I believe the app should 400 in this case and not throw an exception since the client is at fault, not the server. What do you think?

https://github.com/zendframework/zend-diactoros/blob/master/src/ServerRequestFactory.php#L475

[rodmcnew]

Here is the work around I am using for now incase it helps anyone. Another odd thing is that Diactoros throws a generic exception in this case so we must catch all exceptions and check the message with substr().

try {
    $server = Server::createServer(
        $app,
        $_SERVER,
        $_GET,
        $_POST,
        $_COOKIE,
        $_FILES
    );
} catch (UnexpectedValueException $e) {
    if (substr($e->getMessage(), 29) == 'Unrecognized protocol version') {
        /**
         * Diactoros should return "400 Bad Request" in this case but it
         * throws an exception instead. So we must return "400 Bad Request"
         * ourselves to prevent 500 errors when hacker bots send us requests
         * with invalid HTTP versions.
         */
        header('HTTP/1.0 400 Bad Request');
        echo '400 Bad Request - Unrecognized protocol version';
        exit;
    } else {
        //If the exception is not what we are looking for, let it fly and show in error logs
        throw $e;
    }
}
$server->listen();

[rodmcnew]

Note: The work-around I posted above prevents some errors but not all.

[weierophinney]

I honestly think this should be resolved at the application level. Since usage of the factory typically happens during bootstrap of an application, that would be the appropriate location to handle it, and having the factory raise an exception is the best way to notify the consumer that the issue occurred.

I'll likely add something like this to Expressive's Application::run() method; I suggest handling it just the way you've outlined above in your own application.

[rodmcnew]

@weierophinney I guess that solves it for expressive users but we are using diactoros and stagility without expressive. Great job on these packages by the way, we really enjoy how much the middleware pipeline has simplified our app.

If diactoros is providing an "http server", it doesn't seem right for every app that uses diactoros to have to wrap it in a try-catch in order to get it to follow the rules of http. (The rule being to return 400 bad request if the request is bad)

I'm looking at nginx or apache as an example. Neither of those dies and returns a 500 just because a bad request was sent to them. They simply return 400 Bad Request and keep going.

[weierophinney]

@rodmcnew — Two remarks.

First, Server is not like Apache or nginx; it still requires a web server sitting in front of it (which may be the PHP built-in web server, Apache, nginx, ReactPHP, etc.); it handles a single request at a time as provided by one of these. As such, its a bit of a misnomer; it's more like a front controller than anything. If I had to do it again, I'd likely omit the class entirely, because of the confusion it introduces.

Second, the difference between Expressive and Zend\Diactoros\Server is that the latter doesn't create the request/response when dispatching the application, but, rather, is passed them during instantiation — even if that's when calling Server::createServer().

What this means is that you need to do the exception handling within your bootstrap script. As an example:

try {
    $server = Server::createServer(
        $app,
        $_SERVER,
        $_GET,
        $_POST,
        $_COOKIE,
        $_FILES
    );
} catch (InvalidArgumentException $e) {
    $response = $finalHandler(new ServerRequest, (new Response())->withStatus(400), 400);
    (new Response\SapiEmitter())->emit($response);
    exit(0);
} catch (UnexpectedValueException $e) {
    $response = $finalHandler(new ServerRequest, (new Response())->withStatus(400), 400);
    (new Response\SapiEmitter())->emit($response);
    exit(0);
}

$server->listen($finalHandler);

Yes, agreed, it's kind of a pain. But that's on purpose.

Diactoros and Stratigility are providing low-level tools for HTTP messages and middleware; think of it like the relationship between C and PHP. The idea is to keep the scope narrow, to allow others to build more robust frameworks on top of them. Sure, you can write code using just Diactoros and Stratigility, but it means you have to do more plumbing for yourself; they provide just the basic value objects and infrastructure. If you don't want to manage that plumbing yourself, you use a framework built on top of them.

[rodmcnew]

@weierophinney Thanks for the improved try-catch and explanation. I have some slight doubts about having the code assume that every kind of InvalidArgumentException and UnexpectedValueException comes from bad requests, but we can try this out.

[weierophinney]

Could you open an issue asking for more specific exception types, please? I noticed this when doing the solution for Expressive as well, and it would be a welcome improvement.

On Sep 1, 2016 2:39 PM, "Rod McNew" notifications@github.com wrote:

@weierophinney https://github.com/weierophinney I guess that solves it for expressive users but we are using diactoros and stagility without expressive. Great job on these packages by the way, we really enjoy how much the middleware pipeline has simplified our app.

If diactoros is providing an "http server", it doesn't seem right for every app that uses diactoros to have to wrap it in a try-catch in order to get it to follow the rules of http. (The rule being to return 400 bad request if the request is bad)

I'm looking at nginx or apache as an example. Neither of those dies and returns a 500 just because a bad request was sent to them. They simply return 400 Bad Request and keep going.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/zendframework/zend-diactoros/issues/171#issuecomment-244141113, or mute the thread https://github.com/notifications/unsubscribe-auth/AABlV8kKVcQbrKA8s4Mm53APNbYOW0CRks5qlwJ8gaJpZM4IdQRn .