-
Notifications
You must be signed in to change notification settings - Fork 896
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 "431 Request Header Fields Too Large" for long headers on HTTP/1 #4655
Conversation
Motivation: Netty HTTP/1 codec raises `TooLongHttpHeaderException` if headers exceed the max length limit. However, `Http1RequestDecoder` does not take account into `TooLongHttpLineException` and returns "400 Bad Request" instead. Modifications: - Make `Http1RequestDecoder` return `431 Request Header Fields Too Large` if a Netty HttpRequest fails with `TooLongHttpHeaderException` Result: - "431 Request Header Fields Too Large" is now returned instead of `400 Bad Request` if the size of header exceeds `ServerBuilder.http1MaxHeaderSize()`. - Fixes line#4609
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.
Left a super minor comment! Thanks @ikhoon ! 🙇 👍 🙇
@@ -151,6 +152,9 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception | |||
final Throwable cause = nettyReq.decoderResult().cause(); | |||
if (cause instanceof TooLongHttpLineException) { | |||
fail(id, null, HttpStatus.REQUEST_URI_TOO_LONG, "Too Long URI", cause); | |||
} else if (cause instanceof TooLongHttpHeaderException) { |
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.
double checked that TooLongHttpHeaderException
is not related to TooLongHttpLineException
, so it doesn't matter which if statement is checked first 👍
@@ -151,6 +152,9 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception | |||
final Throwable cause = nettyReq.decoderResult().cause(); | |||
if (cause instanceof TooLongHttpLineException) { | |||
fail(id, null, HttpStatus.REQUEST_URI_TOO_LONG, "Too Long URI", cause); | |||
} else if (cause instanceof TooLongHttpHeaderException) { | |||
fail(id, null, HttpStatus.REQUEST_HEADER_FIELDS_TOO_LARGE, "Too Long Header Fields", |
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.
How about just using the default message for the http status?
fail(id, null, HttpStatus.REQUEST_HEADER_FIELDS_TOO_LARGE, "Too Long Header Fields", | |
fail(id, null, HttpStatus.REQUEST_HEADER_FIELDS_TOO_LARGE, "Request Header Fields Too Large", |
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.
👍 👍 👍
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.
Still looks good 👍
Thanks @ikhoon ! 🙇 👍 🙇 |
Motivation:
Netty HTTP/1 codec raises
TooLongHttpHeaderException
if headers exceed the max length limit. However,Http1RequestDecoder
does not take account intoTooLongHttpLineException
and returns "400 Bad Request" instead.Modifications:
Http1RequestDecoder
return431 Request Header Fields Too Large
if a Netty HttpRequest fails withTooLongHttpHeaderException
Result:
431 Request Header Fields Too Large
is now returned instead of400 Bad Request
if the size of headers exceedsServerBuilder.http1MaxHeaderSize()
.