On Wed, Dec 11, 2024 at 4:28 PM Mark Thomas <ma...@apache.org> wrote: > > 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.
Ok, that's the only item left. The original checkIfHeaders was correct (with the added check for GET and HEAD in one of the other methods) so I restored it. Rémy > > 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 > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
