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.

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

Different behaviour with maxFormContentSize=0 if Content-Length header is present/missing #3856

Closed
gbu-censhare opened this issue Jul 8, 2019 · 13 comments
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@gbu-censhare
Copy link

Hi,

with jetty 9.4.19 some visitors cannot post on our website anymore whe HTTP 2 is in use.
from our tests it works fine as long as a "content-length" header is supplied. if this header is missing it is still working with HTTP 1.1, but not with HTTP 2

it seems the the inputstream of UrlEncoded.decodeUtf8To reads some bytes if HTTP 2 is used but not in HTTP 1.1.

this breaks as soon as HttpServletRequest.getParameters() is called.

if i interpreted the RFCs right the content-length header is not required.

curl 'https://localhost/' -H 'Content-Length:' -H 'Connection: keep-alive' -H 'Cache-Control: max-age=0' -H 'Content-Type: application/x-www-form-urlencoded' --data 'login=user&password=pass' --compressed -D - -q -o /dev/null --http1.1

curl 'https://localhost/' -H 'content-length:' -H 'upgrade-insecure-requests: 1' -H 'content-type: application/x-www-form-urlencoded' --data 'login=login&password=pass'  -D- -q -o /dev/null --http2```

Thanks,
Georg
@gregw
Copy link
Contributor

gregw commented Jul 8, 2019

See point 6 in RFC7230 3.3.3, I think this indicates that if you want to have content in a request, then you need a content-length header (or a transfer encoding). Are you sure that without a content-length that Curl is not chunk encoding the message body? Can you show us the full headers of a request that fails?

@gbu-censhare
Copy link
Author

Hi Greg,

  1. If this is a request message and none of the above are true, then
    the message body length is zero (no message body is present).

okay, i can get that, so the parameters or not parsed, then, but why is the Body-InputStream checked against this then, and throw a "Form too large"?

HTTP2:

* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Using Stream ID: 1 (easy handle 0x7fd681807a00)
> POST /en HTTP/2
> Host: localhost
> User-Agent: curl/7.54.0
> Accept: */*
> upgrade-insecure-requests: 1
> content-type: application/x-www-form-urlencoded
>
} [25 bytes data]
* We are completely uploaded and fine
* Connection state changed (MAX_CONCURRENT_STREAMS updated)!
100    25    0     0  100    25      0      7  0:00:03  0:00:03 --:--:--     7< HTTP/2 400
< x-cs-request-id: a40f2f91-7610-4053-a632-1a33d25b9ef3
< cache-control: must-revalidate,no-cache,no-store
< content-type: text/html;charset=iso-8859-1

HTTP1.1:

> POST /en HTTP/1.1
> Host: localhost
> User-Agent: curl/7.54.0
> Accept: */*
> Content-Type: application/x-www-form-urlencoded
>
} [25 bytes data]
* upload completely sent off: 25 out of 25 bytes
100    25    0     0  100    25      0     11  0:00:02  0:00:02 --:--:--    11< HTTP/1.1 200 OK
< Date: Mon, 08 Jul 2019 09:17:53 GMT
< X-Cs-Request-Id: b7e1e912-1d78-4dcc-9ded-b34d8e1e43f6
< X-Cs-Location: /en
< X-Cs-RequestUrl: https://localhost/
< Cache-Control: no-cache, no-store, must-revalidate
< Pragma: no-cache
< Expires: Thu, 01 Jan 1970 00:00:00 GMT
< Accept-Ranges: bytes
< Vary: Accept-Encoding
< Content-Type: text/html;charset=utf-8
< Content-Length: 114316

Thanks, Georg

@gregw
Copy link
Contributor

gregw commented Jul 8, 2019

Georg,

So without a content-length, HTTP/1.1 should conclude that there is no body to the request. This risk here is that any data sent as the body of the request will then be interpreted as the next request on the connection.
For HTTP/2, it has a better framing mechanism, so it actually knows there is a body... just not what length it is.... so perhaps it is trying to read the data... but then no idea why it would fail to read it properly.

In short, send content-length. But we will investigate what we are doing if there is no content-length. @sbordet thoughts?

@gbu-censhare
Copy link
Author

@gregw thanks, so far!

@sbordet
Copy link
Contributor

sbordet commented Jul 8, 2019

@gregw yes this behavior looks strange, so needs investigation.

@sbordet
Copy link
Contributor

sbordet commented Jul 8, 2019

I cannot reproduce, as it always works for me.

I am using curl 7.64.0.

At this point we need an exact way to reproduce, including the server side. The error 400 that you get may be due to something else.

@gbu-censhare
Copy link
Author

i'll try to do wrap it in a test case, is there a good point / example where to put in the jetty code?

@sbordet
Copy link
Contributor

sbordet commented Jul 9, 2019

Please zip it and attach it here.

@gbu-censhare
Copy link
Author

http2-max-form-size-test-master.zip

it seems to be connected to how we initialize the jetty ServletContextHandler, in our default the _maxFormContentSize/_maxFormKeys is set to 0

and it behaves the same in http1.1/http2 i'm bit puzzeled, why i had this different behavior ...

but i would expect, if maxFormContentSize = 0, the form is still to large if the content-length is set ....

@sbordet
Copy link
Contributor

sbordet commented Jul 23, 2019

@gbu-censhare with your reproducer project, I can see the same behavior for HTTP/1.1 and HTTP/2, but a different behavior depending on whether the Content-Length is present or not.

So we'll make sure the behavior is consistent with regards to Content-Length, but it's not about HTTP/1.1 vs HTTP/2.
Are we on the same page?

@gbu-censhare
Copy link
Author

gbu-censhare commented Jul 23, 2019

Yes, please, i don't know why i misinterpreted in the first place

and thanks :)

@sbordet sbordet changed the title different behaviour in http2 vs http1.1 if content-length header on post is missing Different behaviour with maxFormContentSize=0 if Content-Length header is present/missing Jul 23, 2019
@sbordet
Copy link
Contributor

sbordet commented Jul 23, 2019

@gbu-censhare can you please build and try branch jetty-9.4.x-3856-maxForm_contentLength_behavior and see if it works for you?

sbordet added a commit that referenced this issue Jul 23, 2019
…t-Length header is present/missing.

Changes after review.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@gbu-censhare
Copy link
Author

@sbordet thanks, looks good to me!

@joakime joakime added this to In Progress in Jetty 9.4.20 Jul 31, 2019
@joakime joakime added the Bug For general bugs on Jetty side label Jul 31, 2019
sbordet added a commit that referenced this issue Aug 2, 2019
…t-Length header is present/missing.

Updated code to reflect reviews.
Now lookup of system properties and server attributes is done in
ContextHandler.doStart(), so that the getter always return the
actual value (and this is good for JMX too).

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet added a commit that referenced this issue Aug 2, 2019
…t-Length header is present/missing.

Changed the logic to lookup server attributes if there is no context.
This fixes a failing test that was explicitly setting the server
attributes after start.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet added a commit that referenced this issue Aug 7, 2019
…t-Length header is present/missing.

Removed duplicated, unused, code.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet closed this as completed in d939c94 Aug 7, 2019
sbordet added a commit that referenced this issue Aug 7, 2019
…tLength_behavior

Fixes #3856 - Different behaviour with maxFormContentSize=0 if Content-Length header is present/missing.
Jetty 9.4.20 automation moved this from In Progress to Done Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
No open projects
Jetty 9.4.20
  
Done
Development

No branches or pull requests

4 participants