-
Notifications
You must be signed in to change notification settings - Fork 596
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
core: disallow parsing of Transfer-Encoding other than chunked #3754
core: disallow parsing of Transfer-Encoding other than chunked #3754
Conversation
Test FAILed. Pull request validation reportFailed Test SuitesTest result for
|
Missing test coverage for responses. |
Test PASSed. |
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.
I think we can be even more strict than that. Basically, akka-http is implementing the "Transfer", so whatever we cannot decode, cannot be decoded.
The part that ends up in the entity should be the "Content", i.e. the representation that is described by the Content-...
headers.
Especially in a proxy case, preserving any additional Transfer-Encodings seems dangerous, as we might be missing something to allow a fully functional round-trip in that case, or the user misses something trying to inspect a half-decoded entity.
So, I would suggest, for now we only allow a single chunked
encoding here and fail everything else.
Btw. the current HTTP spec is RFC 7230.
The question is whether we need an escape hatch? I'm somewhat inclined to be lazy about it and wait until someone complains and explains why we should support their custom use-case (or contributes that feature). WDYT?
Test PASSed. |
akka-http-core/src/main/scala/akka/http/impl/engine/parsing/HttpMessageParser.scala
Outdated
Show resolved
Hide resolved
akka-http-core/src/main/scala/akka/http/impl/engine/parsing/HttpMessageParser.scala
Outdated
Show resolved
Hide resolved
akka-http-core/src/test/scala/akka/http/impl/engine/parsing/RequestParserSpec.scala
Show resolved
Hide resolved
akka-http-core/src/test/scala/akka/http/impl/engine/parsing/ResponseParserSpec.scala
Show resolved
Hide resolved
* test coverage for Transfer-Encoding + Content-Length * Inconsistent period removed * better error for multiple transfer encodings
akka-http-core/src/main/scala/akka/http/impl/engine/parsing/HttpResponseParser.scala
Show resolved
Hide resolved
Test FAILed. Pull request validation reportMima Failures
|
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, only some cosmetics below
...in/mima-filters/10.2.3.backwards.excludes/stricter-transfer-encoding-header-parsing.excludes
Outdated
Show resolved
Hide resolved
akka-http-core/src/main/scala/akka/http/impl/engine/parsing/HttpMessageParser.scala
Outdated
Show resolved
Hide resolved
akka-http-core/src/main/scala/akka/http/impl/engine/parsing/HttpResponseParser.scala
Show resolved
Hide resolved
akka-http-core/src/test/scala/akka/http/impl/engine/parsing/RequestParserSpec.scala
Show resolved
Hide resolved
Test PASSed. |
Thanks for fixing, @johanandren. |
…3754) Backport from 10.2
Will this be released soon? |
Is there any chance to get this change into 10.1.x too? We also need to close CVE-2021-23339. |
See #3765 for the backport. |
No description provided.