-
-
Notifications
You must be signed in to change notification settings - Fork 143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Return internal server error for unexpected exceptions #233
Conversation
@@ -186,7 +186,7 @@ public function handleConnection(ConnectionInterface $conn) | |||
|
|||
$that->writeError( | |||
$conn, | |||
$e->getCode() !== 0 ? $e->getCode() : 400 | |||
$e->getCode() !== 0 ? $e->getCode() : 500 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think any exception code should just be forwarded to the client here as a status code. Most exception codes aren't HTTP status codes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most exception codes aren't HTTP status codes.
I've not been able to figure out if/what exception codes PHP uses. But inside react/http all codes are specifically generated with the purpose of being used as HTTP return codes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular code only catches Exception
instances from the RequestHeaderParser
which is specifically designed to throw with HTTP status codes.
The test failure
is interesting. It does not actually happen inside the but inside ringcentral: https://github.com/reactphp/http/blob/master/src/RequestHeaderParser.php#L95 Now the question is if this should in the sense of this PR be a 400 or a 500? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for filing this! While I'm not sure what kind of "unexpected exceptions" you're referring to, I'm not opposed to getting this in.
May I ask you to look into the failing test and add some tests to trigger "unexpected exceptions" and show how these are now handled? 👍
@@ -186,7 +186,7 @@ public function handleConnection(ConnectionInterface $conn) | |||
|
|||
$that->writeError( | |||
$conn, | |||
$e->getCode() !== 0 ? $e->getCode() : 400 | |||
$e->getCode() !== 0 ? $e->getCode() : 500 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular code only catches Exception
instances from the RequestHeaderParser
which is specifically designed to throw with HTTP status codes.
@@ -95,7 +95,7 @@ public function handleData($data) | |||
if ($positionCrlf === 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' . static::MAX_CHUNK_HEADER_SIZE. ' bytes')); | |||
$this->handleError(new \Exception('Chunk header size inclusive extension bigger than' . static::MAX_CHUNK_HEADER_SIZE. ' bytes', 400)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I agree that it may make sense for invalid request headers, I'm not sure about the changes in this class. This class only deals with decoding the request body which will be streamed to the request handler. As such, the status codes do not necessarily end up in the response code.
See also other occurrences in this class.
This is clearly triggered due to invalid client input and should thus reject with a |
Fully agree for this test case. This would require catching the exception and re-throwing with status 400. @clue Since we cannot really know what triggers a problem (e.g. the UDS issue)- do you think that this PR still makes sense or should I rather abandon it? |
Afaict we do know what can cause an |
Closing this for the time being. |
Debugging improvement for #231
Alle exceptions are treated as HTTP 500 unless they explicitly provide an error code.