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

Issue #5214 - Servlet HEAD doesn't support content-length over Integer.MAX_VALUE #5215

Merged
merged 7 commits into from
Sep 2, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ protected boolean passConditionalHeaders(HttpServletRequest request, HttpServlet
{
//Get jetty's Response impl
String mdlm = content.getLastModifiedValue();
if (mdlm != null && ifms.equals(mdlm))
if (ifms.equals(mdlm))
{
sendStatus(response, HttpServletResponse.SC_NOT_MODIFIED, content::getETagValue);
return false;
Expand Down Expand Up @@ -674,10 +674,10 @@ protected boolean sendData(HttpServletRequest request,
content.getResource().writeTo(out, 0, content_length);
}
// else if we can't do a bypass write because of wrapping
else if (written || !(out instanceof HttpOutput))
else if (written)
{
// write normally
putHeaders(response, content, written ? -1 : 0);
putHeaders(response, content, Response.NO_CONTENT_LENGTH);
ByteBuffer buffer = content.getIndirectBuffer();
if (buffer != null)
BufferUtil.writeTo(buffer, out);
Expand All @@ -688,7 +688,7 @@ else if (written || !(out instanceof HttpOutput))
else
{
// write the headers
putHeaders(response, content, 0);
putHeaders(response, content, Response.USE_KNOWN_CONTENT_LENGTH);

// write the content asynchronously if supported
if (request.isAsyncSupported() && content.getContentLengthValue() > response.getBufferSize())
Expand Down Expand Up @@ -736,7 +736,7 @@ public String toString()
// if there are no satisfiable ranges, send 416 response
if (ranges == null || ranges.size() == 0)
{
putHeaders(response, content, 0);
putHeaders(response, content, Response.USE_KNOWN_CONTENT_LENGTH);
response.setHeader(HttpHeader.CONTENT_RANGE.asString(),
InclusiveByteRange.to416HeaderRangeString(content_length));
sendStatus(response, HttpServletResponse.SC_REQUESTED_RANGE_NOT_SATISFIABLE, null);
Expand All @@ -763,8 +763,8 @@ public String toString()
// 216 response which does not require an overall
// content-length header
//
putHeaders(response, content, -1);
String mimetype = (content == null ? null : content.getContentTypeValue());
putHeaders(response, content, Response.NO_CONTENT_LENGTH);
String mimetype = content.getContentTypeValue();
if (mimetype == null)
LOG.warn("Unknown mimetype for " + request.getRequestURI());
MultiPartOutputStream multi = new MultiPartOutputStream(out);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ public class Response implements HttpServletResponse
private static final Logger LOG = Log.getLogger(Response.class);
private static final int __MIN_BUFFER_SIZE = 1;
private static final HttpField __EXPIRES_01JAN1970 = new PreEncodedHttpField(HttpHeader.EXPIRES, DateGenerator.__01Jan1970);
public static final int NO_CONTENT_LENGTH = -1;
public static final int USE_KNOWN_CONTENT_LENGTH = -2;

public enum OutputType
{
Expand Down Expand Up @@ -1289,12 +1291,12 @@ public void putHeaders(HttpContent content, long contentLength, boolean etag)
if (lm != null)
_fields.put(lm);

if (contentLength == 0)
if (contentLength == USE_KNOWN_CONTENT_LENGTH)
{
_fields.put(content.getContentLength());
_contentLength = content.getContentLengthValue();
}
else if (contentLength > 0)
else if (contentLength > NO_CONTENT_LENGTH)
{
_fields.putLongField(HttpHeader.CONTENT_LENGTH, contentLength);
_contentLength = contentLength;
Expand Down Expand Up @@ -1337,9 +1339,9 @@ public static void putHeaders(HttpServletResponse response, HttpContent content,
if (lml >= 0)
response.setDateHeader(HttpHeader.LAST_MODIFIED.asString(), lml);

if (contentLength == 0)
if (contentLength == USE_KNOWN_CONTENT_LENGTH)
contentLength = content.getContentLengthValue();
if (contentLength >= 0)
if (contentLength > NO_CONTENT_LENGTH)
{
if (contentLength < Integer.MAX_VALUE)
response.setContentLength((int)contentLength);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -462,20 +462,28 @@ protected void doPost(HttpServletRequest request, HttpServletResponse response)
doGet(request, response);
}

@Override
protected void doHead(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException
{
doGet(request, response);
}

/* (non-Javadoc)
* @see javax.servlet.http.HttpServlet#doTrace(javax.servlet.http.HttpServletRequest, javax.servlet.http.HttpServletResponse)
*/
@Override
protected void doTrace(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
protected void doTrace(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException
{
resp.sendError(HttpServletResponse.SC_METHOD_NOT_ALLOWED);
response.sendError(HttpServletResponse.SC_METHOD_NOT_ALLOWED);
}

@Override
protected void doOptions(HttpServletRequest req, HttpServletResponse resp)
protected void doOptions(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException
{
resp.setHeader("Allow", "GET,HEAD,POST,OPTIONS");
response.setHeader("Allow", "GET,HEAD,POST,OPTIONS");
}

/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,31 @@ public void testDownload(String filename, long expectedSize) throws Exception
}
}

@ParameterizedTest
@MethodSource("staticFiles")
public void testHead(String filename, long expectedSize) throws Exception
{
URI destUri = server.getURI().resolve("/" + filename);
InputStreamResponseListener responseListener = new InputStreamResponseListener();

Request request = client.newRequest(destUri)
.method(HttpMethod.HEAD);
request.send(responseListener);
Response response = responseListener.get(5, TimeUnit.SECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

responseListener.get() only waits for the headers, so to be sure here I would also do (maybe in a try-with-resources):

assertEquals(-1, response.getInputStream().read());

which would guarantee that the HEAD response really came without body, as it should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.
But if that fails, does that mean that both jetty server and jetty client have bugs then? ;-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not matter - we will have a failed test and we'll investigate :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joakime I'll fix this to get this merged ASAP.


try (InputStream in = responseListener.getInputStream())
{
assertThat(in.read(), is(-1));
}

assertThat("HTTP Response Code", response.getStatus(), is(200));
// dumpResponse(response);

String contentLength = response.getHeaders().get(HttpHeader.CONTENT_LENGTH);
long contentLengthLong = Long.parseLong(contentLength);
assertThat("Http Response Header: \"Content-Length: " + contentLength + "\"", contentLengthLong, is(expectedSize));
}

@ParameterizedTest
@MethodSource("staticFiles")
public void testUpload(String filename, long expectedSize) throws Exception
Expand Down