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

StackOverflowError on invalid media range in accept header. #1072

Closed
mrumkovskis opened this issue Apr 30, 2017 · 5 comments · Fixed by #1076
Closed

StackOverflowError on invalid media range in accept header. #1072

mrumkovskis opened this issue Apr 30, 2017 · 5 comments · Fixed by #1076
Assignees
@ktoso
Labels
bug
Milestone
10.0.6

Comments

@mrumkovskis
Copy link

Route produces java.lang.StackOverflowError (shuts the server down) if http request contains Accept header with invalid media range like: '*/xml'

For example: Accept: */xml

Can be reproduced on almost any running akka-http server using routing DSL including sample one provided in the documentation: http://doc.akka.io/docs/akka-http/current/scala/http/introduction.html#using-akka-http
Was not able to reproduce error using ScalatestRouteTest

This seems due to the error in header parsing i.e.
akka.http.scaladsl.model.headers.Accept.parseFromValueString("*/xml").toOption.get.acceptsAll == true

akka-http version: 10.0.5
@ktoso
Copy link
Member

ktoso commented May 1, 2017

Hi and thanks for the reproducer!
You're right, it should not fail so not-nicely, I'll have a look into it right away.

@ktoso ktoso self-assigned this May 1, 2017
@ktoso ktoso added 3 - in progress Someone is working on this ticket bug labels May 1, 2017
@ktoso ktoso added this to the 10.0.6 milestone May 1, 2017
ktoso added a commit to ktoso/akka-http that referenced this issue May 2, 2017
@ktoso
=htc akka#1072 avoid infinite loops with withAcceptAll retries
87aac1e
ktoso added a commit to ktoso/akka-http that referenced this issue May 2, 2017
@ktoso
=htc akka#1072 reject unsupported weird */subType media types
79df288
@ktoso ktoso mentioned this issue May 2, 2017
Merged
ktoso added a commit to ktoso/akka-http that referenced this issue May 2, 2017
@ktoso
=htc akka#1072 reject unsupported weird */subType media types
89f88d9
@jrudolph
Copy link
Member

jrudolph commented May 2, 2017

We seem to have multiple problems here leading to the explained outcome:

  • */xml is allowed as a valid media range. It seems to be allowed by the grammar in RFC 7231 but it isn't clearly defined what the meaning would be. We also didn't decide and didn't foresee that there will be other media ranges with mainType == "" other than "/*" and returned acceptsAll == true.
  • RequestContextImpl.complete allows unbounded recursion through attemptRecoveryFromUnacceptableResponseContentTypeException (in this particular case, what happens is that */xml qualifies as acceptsAll == true but still doesn't accept anything).

History:

In akka/akka#16494, support for withAcceptAll was added but it didn't foresee the */xyz media-range which seems to be valid by grammar (but what should it mean?). In the model it returns true for acceptsAll but the actual range checking doesn't accept all media types.

In akka/akka#19397 and akka/akka#19842 (fixed in akka/akka#19849), it was attempted to make non-200 responses ignore the accept header. The solution was to rerun the marshalling with withAcceptAll in the case of non-2xx responses. This handling was added in a prominent place in RequestContextImpl. This is problematic for various reasons:

  • The fix was added at a code path that basically every request runs through, risking unintended side-effects (like here).
  • Marshallers might be run twice for one complete call. That might be unexpected.
  • A marshaller can already decide whether to take part in content negotiation or not. If it returns an opaque marshalling, it will work regardless of the Accept header.
  • So, in fact, the problem described in request.completeWithStatus(NOT_FOUND) results in 406 Not Acceptable akka#19397 is only one of the fromStatusCodeXXX marshallers. That's the only place where non-2xx status codes can be injected. So, that's the place where it might be possible to fix it in a less-intrusive way.

ktoso added a commit to ktoso/akka-http that referenced this issue May 2, 2017
@ktoso
=htc akka#1072 reject unsupported weird */subType media types
7958eb1
jrudolph added a commit to jrudolph/akka-http that referenced this issue May 2, 2017
@jrudolph
=htp akka#1072 move special non-2xx handling from RequestContextImpl …
4320263 
…to fromStatusCodeAndHeadersAndValue marshaller

This is an alternative solution for akka/akka#19397 that replaces the one
introduced in akka/akka#19849.

The former solution placed the special handling inside the main processing
path of requests in Akka HTTP. This solution is simpler and fixes the issue
in a narrower way.

The code comment explains the solution:

For non-2xx status, add an opaque fallback marshalling
using the first returned marshalling that will be used if the
result wouldn't otherwise be accepted. The reasoning is that users
often use routes like  `complete(400 -> "Illegal request in this context")`
to fail a route (instead of using rejection handling). If the client uses
narrow Accept headers like `Accept: application/json` the user
supplied error message will not be rendered because it will only
accept `text/plain` but not `application/json` responses and fail
the whole request with a "406 Not Acceptable" response.

Adding the opaque fallback rendering will give an escape hatch for
those situations.
ktoso added a commit to ktoso/akka-http that referenced this issue May 2, 2017
@ktoso
=htc akka#1072 ignore unsupported weird */subType media types
15c0ae7
jrudolph added a commit to jrudolph/akka-http that referenced this issue May 2, 2017
@jrudolph
=htp akka#1072 move special non-2xx handling from RequestContextImpl …
a20a8ee 
…to fromStatusCodeAndHeadersAndValue marshaller

This is an alternative solution for akka/akka#19397 that replaces the one
introduced in akka/akka#19849.

The former solution placed the special handling inside the main processing
path of requests in Akka HTTP. This solution is simpler and fixes the issue
in a narrower way.

The code comment explains the solution:

For non-2xx status, add an opaque fallback marshalling
using the first returned marshalling that will be used if the
result wouldn't otherwise be accepted. The reasoning is that users
often use routes like  `complete(400 -> "Illegal request in this context")`
to fail a route (instead of using rejection handling). If the client uses
narrow Accept headers like `Accept: application/json` the user
supplied error message will not be rendered because it will only
accept `text/plain` but not `application/json` responses and fail
the whole request with a "406 Not Acceptable" response.

Adding the opaque fallback rendering will give an escape hatch for
those situations.
jrudolph added a commit to jrudolph/akka-http that referenced this issue May 2, 2017
@jrudolph
=htp akka#1072 move special non-2xx handling from RequestContextImpl …
ba1471b 
…to fromStatusCodeAndHeadersAndValue marshaller

This is an alternative solution for akka/akka#19397 that replaces the one
introduced in akka/akka#19849.

The former solution placed the special handling inside the main processing
path of requests in Akka HTTP. This solution is simpler and fixes the issue
in a narrower way.

The code comment explains the solution:

For non-2xx status, add an opaque fallback marshalling
using the first returned marshalling that will be used if the
result wouldn't otherwise be accepted. The reasoning is that users
often use routes like  `complete(400 -> "Illegal request in this context")`
to fail a route (instead of using rejection handling). If the client uses
narrow Accept headers like `Accept: application/json` the user
supplied error message will not be rendered because it will only
accept `text/plain` but not `application/json` responses and fail
the whole request with a "406 Not Acceptable" response.

Adding the opaque fallback rendering will give an escape hatch for
those situations.
jrudolph added a commit to jrudolph/akka-http that referenced this issue May 2, 2017
@jrudolph
=htp akka#1072 move special non-2xx handling from RequestContextImpl …
81ce167 
…to fromStatusCodeAndHeadersAndValue marshaller

This is an alternative solution for akka/akka#19397 that replaces the one
introduced in akka/akka#19849.

The former solution placed the special handling inside the main processing
path of requests in Akka HTTP. This solution is simpler and fixes the issue
in a narrower way.

The code comment explains the solution:

For non-2xx status, add an opaque fallback marshalling
using the first returned marshalling that will be used if the
result wouldn't otherwise be accepted. The reasoning is that users
often use routes like  `complete(400 -> "Illegal request in this context")`
to fail a route (instead of using rejection handling). If the client uses
narrow Accept headers like `Accept: application/json` the user
supplied error message will not be rendered because it will only
accept `text/plain` but not `application/json` responses and fail
the whole request with a "406 Not Acceptable" response.

Adding the opaque fallback rendering will give an escape hatch for
those situations.

Added more tests to `FromStatusCodeAndXYZMarshallerSpec` (formerly known
as `ContentNegotiationGivenResponseCodeSpec`) to cover the new behavior.
This was referenced May 2, 2017
Merged
Open
jrudolph added a commit to jrudolph/akka-http that referenced this issue May 3, 2017
@jrudolph
=htp akka#1072 move special non-2xx handling from RequestContextImpl …
d058c2c 
…to fromStatusCodeAndHeadersAndValue marshaller

This is an alternative solution for akka/akka#19397 that replaces the one
introduced in akka/akka#19849.

The former solution placed the special handling inside the main processing
path of requests in Akka HTTP. This solution is simpler and fixes the issue
in a narrower way.

The code comment explains the solution:

For non-2xx status, add an opaque fallback marshalling
using the first returned marshalling that will be used if the
result wouldn't otherwise be accepted. The reasoning is that users
often use routes like  `complete(400 -> "Illegal request in this context")`
to fail a route (instead of using rejection handling). If the client uses
narrow Accept headers like `Accept: application/json` the user
supplied error message will not be rendered because it will only
accept `text/plain` but not `application/json` responses and fail
the whole request with a "406 Not Acceptable" response.

Adding the opaque fallback rendering will give an escape hatch for
those situations.

Added more tests to `FromStatusCodeAndXYZMarshallerSpec` (formerly known
as `ContentNegotiationGivenResponseCodeSpec`) to cover the new behavior.
ktoso added a commit to ktoso/akka-http that referenced this issue May 3, 2017
@ktoso
=htc akka#1072 ignore unsupported weird */subType media types
18a67a4
jrudolph added a commit that referenced this issue May 3, 2017
@jrudolph
Merge pull request #1076 from jrudolph/jr/w/1072-simplify-RequestCont…
8ebad2a 
…extImpl

=htp #1072 move special non-2xx handling from RequestContextImpl to fromStatusCodeAndHeadersAndValue marshaller
@jrudolph
Copy link
Member

jrudolph commented May 3, 2017

Fixed in #1075 and #1076. Followed up in #1082.

@jrudolph
Copy link
Member

jrudolph commented May 3, 2017

Thanks a lot, @mrumkovskis to make us aware of this issue.

@ktoso ktoso removed the 3 - in progress Someone is working on this ticket label May 3, 2017
ktoso added a commit that referenced this issue May 3, 2017
@ktoso
=htc,doc #1072 add security note on stack overflow issue
67e88eb
@mrumkovskis
Copy link
Author

Thanks @jrudolph @ktoso for quick reaction! Waiting for release 10.0.6 to upgrade my projects.

tomrf1 pushed a commit to tomrf1/akka-http that referenced this issue Aug 13, 2017
@jrudolph @tomrf1
=htp akka#1072 move special non-2xx handling from RequestContextImpl …
7e1bf97 
…to fromStatusCodeAndHeadersAndValue marshaller

This is an alternative solution for akka/akka#19397 that replaces the one
introduced in akka/akka#19849.

The former solution placed the special handling inside the main processing
path of requests in Akka HTTP. This solution is simpler and fixes the issue
in a narrower way.

The code comment explains the solution:

For non-2xx status, add an opaque fallback marshalling
using the first returned marshalling that will be used if the
result wouldn't otherwise be accepted. The reasoning is that users
often use routes like  `complete(400 -> "Illegal request in this context")`
to fail a route (instead of using rejection handling). If the client uses
narrow Accept headers like `Accept: application/json` the user
supplied error message will not be rendered because it will only
accept `text/plain` but not `application/json` responses and fail
the whole request with a "406 Not Acceptable" response.

Adding the opaque fallback rendering will give an escape hatch for
those situations.

Added more tests to `FromStatusCodeAndXYZMarshallerSpec` (formerly known
as `ContentNegotiationGivenResponseCodeSpec`) to cover the new behavior.
tomrf1 pushed a commit to tomrf1/akka-http that referenced this issue Aug 13, 2017
@ktoso @tomrf1
=htc akka#1072 ignore unsupported weird */subType media types
f606610
tomrf1 pushed a commit to tomrf1/akka-http that referenced this issue Aug 13, 2017
@ktoso @tomrf1
=htc,doc akka#1072 add security note on stack overflow issue
ea40ed4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ktoso ktoso

Labels
bug
Projects
None yet
Milestone
10.0.6
3 participants
@jrudolph @ktoso @mrumkovskis