Skip to content

Commit

Permalink
Fix #12481 Handle headers with 304 responses (#12484)
Browse files Browse the repository at this point in the history 
Allow Content-Length header to be set with a 304 response
Follow RFC9110 recommendations for headers to be sent with 304 response.
  • Loading branch information
@gregw
gregw authored Nov 8, 2024
1 parent c064dd5 commit f78cbc1
Show file tree
Hide file tree
Showing 4 changed files with 95 additions and 43 deletions.
90 changes: 49 additions & 41 deletions jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.EnumSet;
import java.util.Enumeration;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -58,6 +59,12 @@ public class ResourceService
private static final Logger LOG = LoggerFactory.getLogger(ResourceService.class);
private static final int NO_CONTENT_LENGTH = -1;
private static final int USE_KNOWN_CONTENT_LENGTH = -2;
private static final EnumSet<HttpHeader> CONTENT_HEADERS = EnumSet.of(
HttpHeader.LAST_MODIFIED,
HttpHeader.CONTENT_LENGTH,
HttpHeader.CONTENT_TYPE
);
private static final PreEncodedHttpField ACCEPT_RANGES_BYTES = new PreEncodedHttpField(HttpHeader.ACCEPT_RANGES, "bytes");

private final List<CompressedContentFormat> _precompressedFormats = new ArrayList<>();
private final Map<String, List<String>> _preferredEncodingOrderCache = new ConcurrentHashMap<>();
Expand Down Expand Up @@ -351,6 +358,7 @@ protected boolean passConditionalHeaders(Request request, Response response, Htt
if (matched != null)
{
response.getHeaders().put(HttpHeader.ETAG, matched);
putNotModifiedHeaders(response, content);
writeHttpError(request, response, callback, HttpStatus.NOT_MODIFIED_304);
return true;
}
Expand All @@ -368,17 +376,18 @@ protected boolean passConditionalHeaders(Request request, Response response, Htt
String mdlm = content.getLastModifiedValue();
if (ifms.equals(mdlm))
{
putNotModifiedHeaders(response, content);
writeHttpError(request, response, callback, HttpStatus.NOT_MODIFIED_304);
return true;
}

// TODO: what should we do when we get a crappy date?
long ifmsl = HttpDateTime.parseToEpoch(ifms);
if (ifmsl != -1)
{
long lm = content.getResource().lastModified().toEpochMilli();
if (lm != -1 && lm / 1000 <= ifmsl / 1000)
{
putNotModifiedHeaders(response, content);
writeHttpError(request, response, callback, HttpStatus.NOT_MODIFIED_304);
return true;
}
Expand All @@ -388,7 +397,6 @@ protected boolean passConditionalHeaders(Request request, Response response, Htt
// Parse the if[un]modified dates and compare to resource
if (ifums != null && ifm == null)
{
// TODO: what should we do when we get a crappy date?
long ifumsl = HttpDateTime.parseToEpoch(ifums);
if (ifumsl != -1)
{
Expand Down Expand Up @@ -728,55 +736,55 @@ protected void writeHttpContent(Request request, Response response, Callback cal

protected void putHeaders(Response response, HttpContent content, long contentLength)
{
// TODO it is very inefficient to do many put's to a HttpFields, as each put is a full iteration.
// it might be better remove headers en masse and then just add the extras:
// NOTE: If these headers come from a Servlet Filter we shouldn't override them here.
// headers.remove(EnumSet.of(
// HttpHeader.LAST_MODIFIED,
// HttpHeader.CONTENT_LENGTH,
// HttpHeader.CONTENT_TYPE,
// HttpHeader.CONTENT_ENCODING,
// HttpHeader.ETAG,
// HttpHeader.ACCEPT_RANGES,
// HttpHeader.CACHE_CONTROL
// ));
// HttpField lm = content.getLastModified();
// if (lm != null)
// headers.add(lm);
// etc.
HttpFields.Mutable headers = response.getHeaders();

// Existing etags have priority over content etags (often set by compression handler)
if (_etags && !headers.contains(HttpHeader.ETAG))
{
HttpField et = content.getETag();
if (et != null)
headers.add(et);
}

// Existing content encoding is kept only if not set for content.
HttpField ce = content.getContentEncoding();
if (ce != null)
headers.put(ce);

// Remove content headers and re-add if we have them
headers.remove(CONTENT_HEADERS);
HttpField lm = content.getLastModified();
if (lm != null)
response.getHeaders().put(lm);

headers.add(lm);
if (contentLength == USE_KNOWN_CONTENT_LENGTH)
{
response.getHeaders().put(content.getContentLength());
}
headers.add(content.getContentLength());
else if (contentLength > NO_CONTENT_LENGTH)
{
response.getHeaders().put(HttpHeader.CONTENT_LENGTH, contentLength);
}

headers.add(HttpHeader.CONTENT_LENGTH, contentLength);
HttpField ct = content.getContentType();
if (ct != null)
response.getHeaders().put(ct);
headers.add(ct);

HttpField ce = content.getContentEncoding();
if (ce != null)
response.getHeaders().put(ce);
putHeaders(response);
}

if (_etags)
{
HttpField et = content.getETag();
if (et != null)
response.getHeaders().put(et);
}
protected void putNotModifiedHeaders(Response response, HttpContent content)
{
HttpFields.Mutable headers = response.getHeaders();
// send only lastModified, as it is too difficult to determine the etag with compression
HttpField lm = content.getLastModified();
if (lm != null)
headers.put(lm);

if (_acceptRanges && !response.getHeaders().contains(HttpHeader.ACCEPT_RANGES))
response.getHeaders().put(new PreEncodedHttpField(HttpHeader.ACCEPT_RANGES, "bytes"));
if (_cacheControl != null && !response.getHeaders().contains(HttpHeader.CACHE_CONTROL))
response.getHeaders().put(_cacheControl);
putHeaders(response);
}

protected void putHeaders(Response response)
{
HttpFields.Mutable headers = response.getHeaders();
if (_acceptRanges && !headers.contains(HttpHeader.ACCEPT_RANGES))
headers.add(ACCEPT_RANGES_BYTES);
if (_cacheControl != null && !headers.contains(HttpHeader.CACHE_CONTROL))
headers.add(_cacheControl);
}

/**
Expand Down
7 changes: 5 additions & 2 deletions ...y-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1250,7 +1250,8 @@ public void write(boolean last, ByteBuffer content, Callback callback)
long committedContentLength = httpChannelState._committedContentLength;
long contentLength = committedContentLength >= 0 ? committedContentLength : getHeaders().getLongField(HttpHeader.CONTENT_LENGTH);

if (contentLength >= 0 && totalWritten != contentLength)
if (contentLength >= 0 && totalWritten != contentLength &&
!(totalWritten == 0 && (HttpMethod.HEAD.is(_request.getMethod()) || getStatus() == HttpStatus.NOT_MODIFIED_304)))
{
// If the content length were not compatible with what was written, then we need to abort.
String lengthError = null;
Expand Down Expand Up @@ -1525,7 +1526,9 @@ public void succeeded()
long totalWritten = response._contentBytesWritten;
long committedContentLength = httpChannelState._committedContentLength;

if (committedContentLength >= 0 && committedContentLength != totalWritten && !(totalWritten == 0 && HttpMethod.HEAD.is(_request.getMethod())))
if (committedContentLength >= 0 &&
committedContentLength != totalWritten &&
!(totalWritten == 0 && (HttpMethod.HEAD.is(_request.getMethod()) || response.getStatus() == HttpStatus.NOT_MODIFIED_304)))
failure = ExceptionUtil.combine(failure, new IOException("content-length %d != %d written".formatted(committedContentLength, totalWritten)));

// Is the request fully consumed?
Expand Down
33 changes: 33 additions & 0 deletions jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1456,6 +1456,39 @@ public boolean handle(Request request, Response response, Callback callback) thr
}
}

@Test
public void test304WithContentLength() throws Exception
{
startServer(new Handler.Abstract()
{
@Override
public boolean handle(Request request, Response response, Callback callback) throws Exception
{
response.setStatus(304);
response.getHeaders().add(HttpHeader.CONTENT_LENGTH, 10);
callback.succeeded();
return true;
}
});

try (Socket client = newSocket(_serverURI.getHost(), _serverURI.getPort()))
{
OutputStream os = client.getOutputStream();
InputStream is = client.getInputStream();

os.write(("""
GET /R1 HTTP/1.1\r
Host: localhost\r
Connection: close\r
""").getBytes(StandardCharsets.ISO_8859_1));

String in = IO.toString(is);
assertThat(in, containsString("304 Not Modified"));
assertThat(in, containsString("Content-Length: 10"));
}
}

@Test
public void testBlockedClient() throws Exception
{
Expand Down
8 changes: 8 additions & 0 deletions ...core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1480,6 +1480,7 @@ public void testCachingPrecompressedFilesCachedEtagged() throws Exception
\r
""".formatted(eTag1)));
assertThat(response3.getStatus(), is(HttpStatus.NOT_MODIFIED_304));
assertThat(response3.getField(ETAG), notNullValue());

HttpTester.Response response4 = HttpTester.parseResponse(
_local.getResponse("""
Expand All @@ -1491,6 +1492,7 @@ public void testCachingPrecompressedFilesCachedEtagged() throws Exception
\r
""".formatted(eTag2)));
assertThat(response4.getStatus(), is(HttpStatus.NOT_MODIFIED_304));
assertThat(response3.getField(ETAG), notNullValue());
}

assertThat(contentFactory.getCachedFiles(), is(2));
Expand Down Expand Up @@ -1989,6 +1991,7 @@ public void testEtagIfNoneMatchNotModifiedFile() throws Exception
\r
""".formatted(etag)));
assertThat(response.getStatus(), is(HttpStatus.NOT_MODIFIED_304));
assertThat(response.getField(ETAG), notNullValue());
assertThat(response.getContent(), is(""));
}

Expand Down Expand Up @@ -2346,6 +2349,7 @@ public void testIfETag(String content) throws Exception
""".replace("@ETAG@", etag));
response = HttpTester.parseResponse(rawResponse);
assertThat(response.toString(), response.getStatus(), is(HttpStatus.NOT_MODIFIED_304));
assertThat(response.getField(ETAG), notNullValue());

rawResponse = _local.getResponse("""
GET /context/file.txt HTTP/1.1\r
Expand All @@ -2356,6 +2360,7 @@ public void testIfETag(String content) throws Exception
""".replace("@ETAG@", etag));
response = HttpTester.parseResponse(rawResponse);
assertThat(response.toString(), response.getStatus(), is(HttpStatus.NOT_MODIFIED_304));
assertThat(response.getField(ETAG), notNullValue());

rawResponse = _local.getResponse("""
GET /context/file.txt HTTP/1.1\r
Expand Down Expand Up @@ -2468,6 +2473,7 @@ public void testIfModified(String content) throws Exception
""".replace("@LASTMODIFIED@", lastModified));
response = HttpTester.parseResponse(rawResponse);
assertThat(response.toString(), response.getStatus(), is(HttpStatus.NOT_MODIFIED_304));
assertThat(response.getField(LAST_MODIFIED), notNullValue());

rawResponse = _local.getResponse("""
GET /context/file.txt HTTP/1.1\r
Expand All @@ -2488,6 +2494,7 @@ public void testIfModified(String content) throws Exception
""".replace("@DATE@", DateGenerator.formatDate(System.currentTimeMillis() + 10000)));
response = HttpTester.parseResponse(rawResponse);
assertThat(response.toString(), response.getStatus(), is(HttpStatus.NOT_MODIFIED_304));
assertThat(response.getField(LAST_MODIFIED), notNullValue());

rawResponse = _local.getResponse("""
GET /context/file.txt HTTP/1.1\r
Expand Down Expand Up @@ -2537,6 +2544,7 @@ public void testIfModifiedSince() throws Exception
""".formatted(lastModified)));

assertThat(response.getStatus(), equalTo(304));
assertThat(response.getField(LAST_MODIFIED), notNullValue());
assertThat(response.getContent(), is(""));

response = HttpTester.parseResponse(
Expand Down

0 comments on commit f78cbc1

Please sign in to comment.