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

[policy] [request-content-limit] Request Content Limit Policy does not support transfer-encoding #1547

Closed
briankrug opened this Issue Oct 9, 2018 · 5 comments

Comments

Projects
None yet
5 participants
@briankrug

briankrug commented Oct 9, 2018

The Request Content Limit Policy requires the Content-Length header to be present in the request. There are two scenarios where a request will fail but should not:

  1. If the request uses transfer-encoding (as in the case when content is compressed or chunked)
  2. If the request uses a method that typically does not have a body (such as GET)

Possible Solution

The Request Content Limit Policy should handle requests that use transfer-encoding. More specifically, in RequestContentLimitPolicy, the onRequest() method should be altered to not throw an Exception when the Content-Length header is present and it a method annotated with @OnRequestContent should be added which checked for the transfer-encoding header and wraps the ReadWriteStream and throws and exception when the content limit is reached.

Context

In its current implementation, not all client requests are supported

Your Environment

  • Version used: Gravitee 1.18.1
@brasseld

This comment has been minimized.

Member

brasseld commented Oct 9, 2018

Hi @briankrug

Yes, we are already aware of this issue.

I agree with the first point but not with the second. As per the RFC:

A payload within a GET request message has no defined semantics; sending a payload body on a GET request might cause some existing implementations to reject the request.

In other words, it's not forbidden, but it's undefined behavior and should be avoided.

From my point of view, is it the responsibility of the user to not attach a request-content-limit policy to GET methods (also HEAD, TRACE and OPTIONS, ...)

@jahlborn

This comment has been minimized.

jahlborn commented Oct 9, 2018

I agree that the rfc discourages a body for GET, but i feel like this is something that happens in the world today (whether or not we agree with it). by not supporting it, you exclude gravitee from being used with apis which use it. (i think DELETE falls into the same bucket).

@brasseld

This comment has been minimized.

Member

brasseld commented Oct 9, 2018

I agree. And that's the reason I said that it should be the responsibility of the API Owner to define the policy per verbs for such case.

I have in mind the case of Elasticsearch where you can search for documents by posting a query using a GET...

@briankrug

This comment has been minimized.

briankrug commented Oct 10, 2018

My concern is if a client decides to send a large payload with a GET request (regardless of whether the backend API supports it or not), I still want to be able to limit the size of that payload at the Gateway to protected the Gateway and the backend from content that is too big. If the policy can't handle a request without a Content-Length, then I can't put it on GET requests and thus protect against large payloads.

@brasseld

This comment has been minimized.

Member

brasseld commented Oct 10, 2018

And I fully understand your concern, I will take care of this issue.
Chunk transfer encoding is already supported by the policy... in my laptop... I did not take the time to push it in repository.

@brasseld brasseld changed the title from Request Content Limit Policy does not support transfer-encoding to [policy] [request-content-limit] Request Content Limit Policy does not support transfer-encoding Oct 11, 2018

@brasseld brasseld added this to the 1.20.0 milestone Oct 11, 2018

@brasseld brasseld self-assigned this Oct 11, 2018

brasseld added a commit to gravitee-io/gravitee-policy-request-content-limit that referenced this issue Oct 11, 2018

NicolasGeraud added a commit to gravitee-io/gravitee-policy-request-content-limit that referenced this issue Oct 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment