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.

Sign up for GitHub

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

Fix #12481 Handle headers with 304 responses #12484

Merged
merged 3 commits into from
Nov 8, 2024
Merged

Fix #12481 Handle headers with 304 responses #12484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f285db9
Fix #12481 Handle headers with 304 responses
gregw Nov 6, 2024
5b7f434
Fix #12481 Handle headers with 304 responses
gregw Nov 6, 2024
606c30c
updates from review
gregw Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 49 additions & 41 deletions jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java
View file
Open in desktop
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();
lorban marked this conversation as resolved.
Show resolved Hide resolved

// 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);
gregw marked this conversation as resolved.
Show resolved Hide resolved
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)
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
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)
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
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
Open in desktop
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
Open in desktop
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
Open in desktop
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());
lorban marked this conversation as resolved.
Show resolved Hide resolved

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
Loading