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. > > > 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.
Ok, I'll redo it. Rémy --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
