Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Return internal server error for unexpected exceptions #233

Closed
wants to merge 3 commits into from
Closed

Return internal server error for unexpected exceptions #233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ae77408
Return internal server error for unexpected exceptions
andig Oct 4, 2017
6cc61b0
Add more explicit HTTP 400 results
andig Oct 4, 2017
725d1db
Revert elvis operator
andig Oct 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/ChunkedDecoder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Member

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.

}
return;
}
Expand All @@ -117,7 +117,7 @@ public function handleData($data)

$this->chunkSize = hexdec($hexValue);
if (dechex($this->chunkSize) !== $hexValue) {
$this->handleError(new \Exception($hexValue . ' is not a valid hexadecimal number'));
$this->handleError(new \Exception($hexValue . ' is not a valid hexadecimal number', 400));
return;
}

Expand Down Expand Up @@ -152,7 +152,7 @@ public function handleData($data)

if ($positionCrlf !== 0 && $this->chunkSize === $this->transferredSize && strlen($this->buffer) > 2) {
// the first 2 characters are not CLRF, send error event
$this->handleError(new \Exception('Chunk does not end with a CLRF'));
$this->handleError(new \Exception('Chunk does not end with a CLRF', 400));
return;
}

Expand Down
8 changes: 4 additions & 4 deletions src/RequestHeaderParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ private function parseRequest($headers)
// additional, stricter safe-guard for request line
// because request parser doesn't properly cope with invalid ones
if (!preg_match('#^[^ ]+ [^ ]+ HTTP/\d\.\d#m', $headers)) {
throw new \InvalidArgumentException('Unable to parse invalid request-line');
throw new \InvalidArgumentException('Unable to parse invalid request-line', 400);
}

$lines = explode("\r\n", $headers);
Expand All @@ -87,7 +87,7 @@ private function parseRequest($headers)
$parts[1] = 'http://' . $parts[1] . '/';
$headers = implode(' ', $parts);
} else {
throw new \InvalidArgumentException('CONNECT method MUST use authority-form request target');
throw new \InvalidArgumentException('CONNECT method MUST use authority-form request target', 400);
}
}

Expand Down Expand Up @@ -159,7 +159,7 @@ private function parseRequest($headers)

// make sure value contains valid host component (IP or hostname), but no fragment
if (!isset($parts['scheme'], $parts['host']) || $parts['scheme'] !== 'http' || isset($parts['fragment'])) {
throw new \InvalidArgumentException('Invalid absolute-form request-target');
throw new \InvalidArgumentException('Invalid absolute-form request-target', 400);
}
}

Expand All @@ -175,7 +175,7 @@ private function parseRequest($headers)
// make sure value does not contain any other URI component
unset($parts['scheme'], $parts['host'], $parts['port']);
if ($parts === false || $parts) {
throw new \InvalidArgumentException('Invalid Host header value');
throw new \InvalidArgumentException('Invalid Host header value', 400);
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/Server.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ public function handleConnection(ConnectionInterface $conn)

$that->writeError(
$conn,
$e->getCode() !== 0 ? $e->getCode() : 400
$e->getCode() !== 0 ? $e->getCode() : 500
Copy link

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

);
});
}
Expand All @@ -198,7 +198,7 @@ public function handleRequest(ConnectionInterface $conn, ServerRequestInterface
$stream = new CloseProtectionStream($conn);
if ($request->hasHeader('Transfer-Encoding')) {
if (strtolower($request->getHeaderLine('Transfer-Encoding')) !== 'chunked') {
$this->emit('error', array(new \InvalidArgumentException('Only chunked-encoding is allowed for Transfer-Encoding')));
$this->emit('error', array(new \InvalidArgumentException('Only chunked-encoding is allowed for Transfer-Encoding', 400)));
return $this->writeError($conn, 501, $request);
}

Expand All @@ -214,7 +214,7 @@ public function handleRequest(ConnectionInterface $conn, ServerRequestInterface
$contentLength = (int)$string;
if ((string)$contentLength !== (string)$string) {
// Content-Length value is not an integer or not a single integer
$this->emit('error', array(new \InvalidArgumentException('The value of `Content-Length` is not valid')));
$this->emit('error', array(new \InvalidArgumentException('The value of `Content-Length` is not valid', 400)));
return $this->writeError($conn, 400, $request);
}

Expand Down