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
Check HTTP2 headers for correctness #3603
Check HTTP2 headers for correctness #3603
Conversation
Test PASSed. |
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.
One potential problem with application/octet-stream
content types but otherwise LGTM
akka-http-core/src/main/scala/akka/http/impl/engine/http2/RequestParsing.scala
Outdated
Show resolved
Hide resolved
akka-http-core/src/main/scala/akka/http/impl/engine/http2/RequestParsing.scala
Show resolved
Hide resolved
akka-http-core/src/main/scala/akka/http/impl/engine/http2/client/ResponseParsing.scala
Outdated
Show resolved
Hide resolved
Test PASSed. |
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.
Looks good, I think 'TE' just means 'Transfer-Encoding'
case "transfer-encoding" => | ||
// https://tools.ietf.org/html/rfc7540#section-8.1.2.2 | ||
malformedRequest("Header 'Transfer-Encoding' must not be used with HTTP/2") | ||
case "te" => | ||
// https://tools.ietf.org/html/rfc7540#section-8.1.2.2 | ||
if (httpHeader.value.compareToIgnoreCase("trailers") != 0) malformedRequest(s"Header 'TE' must not contain value other than 'trailers', value was '${httpHeader.value}") |
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 think TE is used as short for Transfer-Encoding in the spec here, so:
case "transfer-encoding" => | |
// https://tools.ietf.org/html/rfc7540#section-8.1.2.2 | |
malformedRequest("Header 'Transfer-Encoding' must not be used with HTTP/2") | |
case "te" => | |
// https://tools.ietf.org/html/rfc7540#section-8.1.2.2 | |
if (httpHeader.value.compareToIgnoreCase("trailers") != 0) malformedRequest(s"Header 'TE' must not contain value other than 'trailers', value was '${httpHeader.value}") | |
case "transfer-encoding" => | |
// https://tools.ietf.org/html/rfc7540#section-8.1.2.2 | |
if (httpHeader.value.compareToIgnoreCase("trailers") != 0) malformedRequest(s"Header 'TE' must not contain value other than 'trailers', value was '${httpHeader.value}") |
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 may be wrong but I think it actually is a distinct header: https://tools.ietf.org/html/rfc7230#section-4.3
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.
huh, looks like you're completely right, sorry about he noise :(
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.
No worries, it sure is confusing with two headers meaning the same but not quite the same...
@@ -110,6 +110,29 @@ class RequestParsingSpec extends AkkaSpec() with Inside with Inspectors { | |||
} | |||
} | |||
|
|||
"not accept TE with other values than 'trailers'" in { |
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.
(also needs to be reflected here)
Test PASSed. |
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.
LGTM
@raboof good to go from your side? |
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.
Yes, looks good!
References #812
Tried to read RFC7540 for MUST rules around headers and crosscheck with the HTTP/1.1 message parsing, but not super confident I did not miss something or get something wrong.