Fixes #5685 - AsyncProxyServlet calls onProxyResponseSuccess() when internally it throws "Response header too large" exception. #12351
Conversation
sbordet
commented
Oct 7, 2024
sbordet
commented
- Introduced "maxResponseHeadersSize" parameter to AbstractProxyServlet.
- Introduced HttpGenerator.maxResponseHeadersSize and added checks to not exceed it.
- Fixed HttpParser to generate a 400 in case HttpParser.maxHeaderBytes are exceeded for a response.
- HTTP2Flusher now resets streams in case of failures.
- Removed cases in HTTP2Session where a GOAWAY frame was generated with lastStreamId=0. GOAWAY.lastStreamId=0 means that not a single request was processed by the server, which was obviously incorrect.
- Written tests for both ProxyHandler and ProxyServlet about max response headers size exceeded.
…el clients. Fixed initialization for HTTP/2 and HTTP/3. Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
…licit. Signed-off-by: Simone Bordet <[email protected]>
…nternally it throws "Response header too large" exception. * Introduced "maxResponseHeadersSize" parameter to AbstractProxyServlet. * Introduced HttpGenerator.maxResponseHeadersSize and added checks to not exceed it. * Fixed HttpParser to generate a 400 in case HttpParser.maxHeaderBytes are exceeded for a response. * HTTP2Flusher now resets streams in case of failures. * Removed cases in HTTP2Session where a GOAWAY frame was generated with lastStreamId=0. GOAWAY.lastStreamId=0 means that not a single request was processed by the server, which was obviously incorrect. * Written tests for both ProxyHandler and ProxyServlet about max response headers size exceeded. Signed-off-by: Simone Bordet <[email protected]>
| private boolean _needCRLF = false; | ||
| private int _maxHeaderBytes; |
There was a problem hiding this comment.
I wonder if 0 is a sensible default, even if we always call the setter before using the generator.
There was a problem hiding this comment.
We did not check for response headers max size before, so 0 keeps the original behavior.
| if (_requestParser) | ||
| throw new BadMessageException(header ? HttpStatus.REQUEST_HEADER_FIELDS_TOO_LARGE_431 : HttpStatus.PAYLOAD_TOO_LARGE_413); | ||
| // There is no equivalent of 431 for response headers. | ||
| throw new BadMessageException(HttpStatus.BAD_REQUEST_400, "Response Header Fields Too Large"); |
There was a problem hiding this comment.
It's not a 400 error either as 4xx codes indicate client errors but here it really is a server error. 500 looks like a more correct response code IMHO.
There was a problem hiding this comment.
Unfortunately BadMessageException only allows 4xx error codes.
I'll check if we can throw a different exception.
There was a problem hiding this comment.
It is possible to throw a different exception, HttpException.RuntimeException, but this is a client parsing a response: should it really generate a 500 Internal Server Error (emphasis on "Server")?
The server could generate a large response; it's the client that cannot parse it, so it's not technically a server error in HTTP/1.1 where the client cannot inform the server about how much it can parse.
@gregw thoughts?
| @@ -776,7 +778,7 @@ public Action process() throws Exception | |||
| case HEADER_OVERFLOW: | |||
| { | |||
| if (_header.capacity() >= getHttpConfiguration().getResponseHeaderSize()) | |||
| throw new HttpException.RuntimeException(INTERNAL_SERVER_ERROR_500, "Response header too large"); | |||
| throw new HttpException.RuntimeException(INTERNAL_SERVER_ERROR_500, "Response Header Fields Too Large"); | |||
There was a problem hiding this comment.
Here, the status is (IMHO correctly) set to 500 when headers are too large, unlike H2.
There was a problem hiding this comment.
Not sure what you mean here? H2 does not generate an HTTP response (it just fails the connection).
There was a problem hiding this comment.
I wrote H2, I meant the parser. Silly me.
| .timeout(5, TimeUnit.SECONDS) | ||
| .send(); | ||
|
|
||
| // assertTrue(serverToProxyFailureLatch.await(5, TimeUnit.SECONDS)); |
There was a problem hiding this comment.
It should be restored!
Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
…se-headers-too-large'. Signed-off-by: Simone Bordet <[email protected]>
