On 11/12/2024 09:56, r...@apache.org wrote:
This is an automated email from the ASF dual-hosted git repository.
remm pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 990f7e65ff Improve HTTP If headers processing according to RFC 9110
990f7e65ff is described below
commit 990f7e65fff23755bdda505225b77843a189107f
Author: remm <r...@apache.org>
AuthorDate: Wed Dec 11 10:56:06 2024 +0100
Improve HTTP If headers processing according to RFC 9110
PR#796 by Chenjp.
Also includes better test cases.
I think this needs some more work. (I'm looking at that now). Some
detailed comments in-line.
---
.../apache/catalina/servlets/DefaultServlet.java | 376 +++++++++---
.../TestDefaultServletRfc9110Section13.java | 672 +++++++++++++++++++++
...efaultServletRfc9110Section13Parameterized.java | 433 +++++++++++++
webapps/docs/changelog.xml | 4 +
4 files changed, 1387 insertions(+), 98 deletions(-)
diff --git a/java/org/apache/catalina/servlets/DefaultServlet.java
b/java/org/apache/catalina/servlets/DefaultServlet.java
index e5dff13949..8eb1ba8524 100644
--- a/java/org/apache/catalina/servlets/DefaultServlet.java
+++ b/java/org/apache/catalina/servlets/DefaultServlet.java
@@ -726,10 +726,72 @@ public class DefaultServlet extends HttpServlet {
*/
protected boolean checkIfHeaders(HttpServletRequest request,
HttpServletResponse response, WebResource resource)
throws IOException {
+ String ifNoneMatchHeader = request.getHeader("If-None-Match");
- return checkIfMatch(request, response, resource) && checkIfModifiedSince(request, response, resource) &&
- checkIfNoneMatch(request, response, resource) &&
checkIfUnmodifiedSince(request, response, resource);
-
+ // RFC9110 #13.3.2 defines preconditions evaluation order
+ int next = 1;
+ while (true) {
+ switch (next) {
+ case 1:
This seems overly complicated. A series of if statements should be
sufficient.
+ if (request.getHeader("If-Match") != null) {
+ if (checkIfMatch(request, response, resource)) {
+ next = 3;
+ } else {
+ return false;
+ }
+ } else {
+ next = 2;
+ }
+ break;
+ case 2:
+ if (request.getHeader("If-Unmodified-Since") != null) {
+ if (checkIfUnmodifiedSince(request, response,
resource)) {
+ next = 3;
+ } else {
+ return false;
+ }
+ } else {
+ next = 3;
+ }
+ break;
+ case 3:
+ if (ifNoneMatchHeader != null) {
+ if (checkIfNoneMatch(request, response, resource)) {
+ next = 5;
+ } else {
+ return false;
+ }
+ } else {
+ next = 4;
+ }
+ break;
+ case 4:
+ if (("GET".equals(request.getMethod()) ||
"HEAD".equals(request.getMethod())) &&
+ ifNoneMatchHeader == null &&
request.getHeader("If-Modified-Since") != null) {
+ if (checkIfModifiedSince(request, response, resource))
{
+ next = 5;
+ } else {
+ return false;
+ }
+ } else {
+ next = 5;
+ }
+ break;
+ case 5:
Every return value from this case is true. It isn't clear to me yet how
the code dfferentiates between a 206 and a 200 response. The above block
appears to be essentially a NO-OP.
+ if ("GET".equals(request.getMethod()) &&
request.getHeader("If-Range") != null
+ && request.getHeader("Range") != null) {
+ if (checkIfRange(request, response, resource) &&
determineRangeRequestsApplicable(resource)) {
+ // Partial content, precondition passed
+ return true;
+ } else {
+ // ignore the Range header field
+ return true;
+ }
+ } else {
+ return true;
> + }> + }
+ }
}
A number of tests appear to be duplicated in the case blocks and in the
specific checkIfXxxxx() methods.
@@ -825,15 +887,6 @@ public class DefaultServlet extends HttpServlet {
}
boolean included = false;
- // Check if the conditions specified in the optional If headers are
- // satisfied.
- if (resource.isFile()) {
- // Checking If headers
- included =
(request.getAttribute(RequestDispatcher.INCLUDE_CONTEXT_PATH) != null);
- if (!included && !isError && !checkIfHeaders(request, response,
resource)) {
- return;
- }
- }
// Find content type.
String contentType = resource.getMimeType();
@@ -847,11 +900,21 @@ public class DefaultServlet extends HttpServlet {
// be needed later
String eTag = null;
String lastModifiedHttp = null;
+
if (resource.isFile() && !isError) {
eTag = generateETag(resource);
lastModifiedHttp = resource.getLastModifiedHttp();
}
+ // Check if the conditions specified in the optional If headers are
+ // satisfied.
+ if (resource.isFile()) {
+ // Checking If headers
+ included =
(request.getAttribute(RequestDispatcher.INCLUDE_CONTEXT_PATH) != null);
+ if (!included && !isError && !checkIfHeaders(request, response,
resource)) {
+ return;
+ }
+ }
// Serve a precompressed version of the file if present
boolean usingPrecompressedVersion = false;
@@ -1445,41 +1508,24 @@ public class DefaultServlet extends HttpServlet {
protected Ranges parseRange(HttpServletRequest request,
HttpServletResponse response, WebResource resource)
throws IOException {
- if (!"GET".equals(request.getMethod())) {
+ // Retrieving the range header (if any is specified)
+ String rangeHeader = request.getHeader("Range");
+
+ if (rangeHeader == null) {
+ // No Range header is the same as ignoring any Range header
+ return FULL;
+ }
+
+ if (!"GET".equals(request.getMethod()) ||
!determineRangeRequestsApplicable(resource)) {
// RFC 9110 - Section 14.2: GET is the only method for which
range handling is defined.
// Otherwise MUST ignore a Range header field
return FULL;
}
- // Checking If-Range
- String headerValue = request.getHeader("If-Range");
-
- if (headerValue != null) {
-
- long headerValueTime = (-1L);
- try {
- headerValueTime = request.getDateHeader("If-Range");
- } catch (IllegalArgumentException e) {
- // Ignore
- }
-
- String eTag = generateETag(resource);
- long lastModified = resource.getLastModified();
-
- if (headerValueTime == (-1L)) {
- // If the ETag the client gave does not match the entity
- // etag, then the entire entity is returned.
- if (!eTag.equals(headerValue.trim())) {
- return FULL;
- }
- } else {
- // If the timestamp of the entity the client got differs from
- // the last modification date of the entity, the entire entity
- // is returned.
- if (Math.abs(lastModified - headerValueTime) > 1000) {
- return FULL;
- }
- }
+ // Although If-Range evaluation was performed previously, the result
were not propagated.
+ // Hence we have to evaluate If-Range again.
+ if (!checkIfRange(request, response, resource)) {
+ return FULL;
}
If IfRange is handled here, it doesn't need to be handled above.
long fileLength = resource.getContentLength();
@@ -1490,13 +1536,6 @@ public class DefaultServlet extends HttpServlet {
return FULL;
}
- // Retrieving the range header (if any is specified)
- String rangeHeader = request.getHeader("Range");
-
- if (rangeHeader == null) {
- // No Range header is the same as ignoring any Range header
- return FULL;
- }
Ranges ranges = Ranges.parse(new StringReader(rangeHeader));
@@ -2092,36 +2131,49 @@ public class DefaultServlet extends HttpServlet {
protected boolean checkIfMatch(HttpServletRequest request,
HttpServletResponse response, WebResource resource)
throws IOException {
- String headerValue = request.getHeader("If-Match");
- if (headerValue != null) {
-
- boolean conditionSatisfied;
+ String resourceETag = generateETag(resource);
+ if (resourceETag == null) {
+ // if a current representation for the target resource is not
present
+ response.sendError(HttpServletResponse.SC_PRECONDITION_FAILED);
+ return false;
+ }
Ths is depending on an implementation detail (that
EmptyResource.getEtag() returns null). That seems fragile.
This is as far as I have progressed with this review so far. I still
need to look at the rest.
I am going to start looking in more detail at the things I've found so
far and then I'll move on to review the remainder of this change. If any
changes are required, I'm expecting to make a series of small commits -
one for each change.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
