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

HttpServlet.doHead sets content-length #225

Closed
gregw opened this issue Dec 11, 2018 · 8 comments · Fixed by #405
Closed

HttpServlet.doHead sets content-length #225

gregw opened this issue Dec 11, 2018 · 8 comments · Fixed by #405
Labels
Bug Software problem Candidate4NextRelease This issues should be consider for inclusion in the next release project.

Comments

@gregw
Copy link
Contributor

gregw commented Dec 11, 2018

The implementation of HttpServlet.doHead sets content-length, even if the doGet implementation does not. This results in different headers being sent, specifically a GET request that would send a chunked response will respond to a HEAD request without being chunked. This is contrary to RFC7230 which allows for content headers to be omitted, but not altered.

@markt-asf
Copy link
Contributor

How does that happen? Wouldn't whatever causes the response to chunked also commit it? That would mean the later attempt to set a content length header would be ignored.

@gregw
Copy link
Contributor Author

gregw commented Dec 12, 2018

With a GET request that writes it's response data with multiple writes, eventually the internal aggregation buffer fills up and the response is committed, typically using chunking or EOF to delimit the response.
With the corresponding HEAD request, the response data is written to the NoBodyResponse, which discards the data without writing it, but counts the bytes. The response is not committed due to a buffer overflow. Once the doGet call in doHead is complete, the content-length is set from the bytes counted by the NoBodyResponse and the response committed with the after-dispatch handling of the container.

Thus the GET request has chunked transfer-encoding header and the HEAD request has a content-length header. The spec does allows for content headers to be omitted, but in this case they are not omitted but altered... perhaps needlessly so?

Note that it is kind of useful that a HEAD request can be used to determine the content-length (when a chunked response to a head would not). But being useful does not mean it is right....

Note also that it has been this way for a long time, so perhaps it is fine to leave as is.... just wanted to make sure we are doing so on purpose !

@markt-asf
Copy link
Contributor

Thanks for the explanation. I was looking at the case of an explicit flush that forced chunking. Things work correctly in that case.
Personally, I'd lean towards correcting this as my default preference is to implement specs correctly.

@gregw
Copy link
Contributor Author

gregw commented Dec 12, 2018

Note also that I really don't like implementations in APIs. Container implementations have to deal with HEAD responses anyway because doHead may be overridden, so it adds no value.

@markt-asf
Copy link
Contributor

If a Servlet overrides doHead() then I'd argue getting the response right becomes the Servlet author's problem.
Noted and generally agreed regarding having implementation in APIs. The corner cases cause all sorts of complications. But we are where we are.

@gregw
Copy link
Contributor Author

gregw commented Dec 15, 2018

I take the view that it is the containers job to be RFC compliant on the protocol where ever possible. So if a servlet starts sending content as the response to a HEAD request, then the container has to not allow that content to be put on the wire. To do so would be a bit of a security issue as it would allow a servlet to inject a response to the following request.

So any container relying on doHead implementations is a bit on shaky ground I think.

@markt-asf
Copy link
Contributor

Sorry, should have been clearer as I think we are in broad agreement.
I agree that the container should prevent a response body from being sent for HEAD. The point I was trying to make was that if a Servlet implements doHead(), the Servlet should have the responsibility to ensure that the headers for GET and HEAD are consistent.

@gregw gregw added the Bug Software problem label Jan 18, 2020
@gregw
Copy link
Contributor Author

gregw commented Sep 8, 2020

As a related problem, the doHead implementation is based on int, so it fails if a very large content is accessed. The impl needs to be updated to support a long. It also probably needs to do something about async IO... at the very least ISE if it can't fake a callback. I'll prepare a PR.

@gregw gregw added the Candidate4NextRelease This issues should be consider for inclusion in the next release project. label Sep 8, 2020
gregw added a commit that referenced this issue Sep 8, 2020
Fix #225 by:
 + converting the contentLength field to a long
 + checking against bufferSize before setting a content length to simulate a buffer overflow
 + support flush and close
 + pass through async IO listener
gregw added a commit that referenced this issue Sep 9, 2020
 + check content-length against buffer size on every write
gregw added a commit that referenced this issue May 31, 2021
Fix #225 by:
 + converting the contentLength field to a long
 + checking against bufferSize before setting a content length to simulate a buffer overflow
 + support flush and close
 + pass through async IO listener
gregw added a commit that referenced this issue May 31, 2021
 + check content-length against buffer size on every write
@gregw gregw closed this as completed in #405 Oct 8, 2021
gregw added a commit that referenced this issue Oct 8, 2021
Issue #225 doHead contentLength

 + converting the contentLength field to a long
 + checking against bufferSize before setting a content length to simulate a buffer overflow
 + support flush and close
 + pass through async IO listener

* Issue #225 doHead contentLength

 + check content-length against buffer size on every write

* Improved clarity of the writer flush mechanism

* Added unit test

* Alternate approach
 + fixed some definite bugs in Nobody response
 + made the use of Nobody response optional.

* + use SC init parameter to enable legacy doHead handling

* Deprecated legacy mode and added text in spec

* format

Signed-off-by: Greg Wilkins <[email protected]>

* format

Signed-off-by: Greg Wilkins <[email protected]>

* merged and updated for deprecated and new methods.
gregw added a commit to gregw/servlet-api that referenced this issue Oct 8, 2021
Issue jakartaee#225 doHead fixes:
 + fixed reset handling in `NoBodyResponse` and `NoBodyOutputStream`
 + better implementation of async IO methods in `NoBodyOutputStream`
 + replaced `doHead` implementation with direct call of `doGet` and legacy nobody mode.
 + added unit tests.

Signed-off-by: Greg Wilkins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Software problem Candidate4NextRelease This issues should be consider for inclusion in the next release project.
Projects
None yet
2 participants