On Mon, Mar 3, 2025 at 10:30 AM Mark Thomas <ma...@apache.org> wrote:
>
> On 28/02/2025 22:41, 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 7f0df68d29 69602: Allow weak etags in If-Range header
> > 7f0df68d29 is described below
> >
> > commit 7f0df68d292bcb576a48b975f169010b911d728c
> > Author: remm <r...@apache.org>
> > AuthorDate: Fri Feb 28 23:40:42 2025 +0100
> >
> > 69602: Allow weak etags in If-Range header
> >
> > Adjust with the specification language, '"' in the first three chars
> > should detect if this should be a date or an etag.
> > Trim first.
> > ---
> > .../apache/catalina/servlets/DefaultServlet.java | 38
> > +++++++++++++---------
> > .../servlets/TestDefaultServletRangeRequests.java | 36
> > ++++++++++++++++++--
> > webapps/docs/changelog.xml | 5 +++
> > 3 files changed, 60 insertions(+), 19 deletions(-)
> >
> > diff --git a/java/org/apache/catalina/servlets/DefaultServlet.java
> > b/java/org/apache/catalina/servlets/DefaultServlet.java
> > index 0e4a33e50c..9fb75069f1 100644
> > --- a/java/org/apache/catalina/servlets/DefaultServlet.java
> > +++ b/java/org/apache/catalina/servlets/DefaultServlet.java
> > @@ -2376,40 +2376,46 @@ public class DefaultServlet extends HttpServlet {
> > // If-Range is not present
> > return true;
> > }
> > - String headerValue = headerEnum.nextElement();
> > + String headerValue = headerEnum.nextElement().trim();
> > if (headerEnum.hasMoreElements()) {
> > // Multiple If-Range headers
> > response.sendError(HttpServletResponse.SC_BAD_REQUEST);
> > return false;
> > }
> >
> > - long headerValueTime = -1L;
> > - try {
> > - headerValueTime = request.getDateHeader("If-Range");
> > - } catch (IllegalArgumentException e) {
> > - // Ignore
> > - }
> > -
> > - if (headerValueTime == -1L) {
> > - // Not HTTP-date so this should be a single strong etag
> > - if (headerValue.length() < 2 || headerValue.charAt(0) != '"' ||
> > + if (headerValue.length() > 2 && (headerValue.charAt(0) == '"' ||
> > headerValue.charAt(2) == '"')) {
> > + boolean weakETag = headerValue.startsWith("W/\"");
> > + if ((!weakETag && headerValue.charAt(0) != '"') ||
> > headerValue.charAt(headerValue.length() - 1) != '"' ||
> > - headerValue.indexOf('"', 1) != headerValue.length() -
> > 1) {
> > - // Not a single, strong entity tag
> > + headerValue.indexOf('"', weakETag ? 3 : 1) !=
> > headerValue.length() - 1) {
> > + // Not a single entity tag
> > response.sendError(HttpServletResponse.SC_BAD_REQUEST);
> > return false;
> > }
> > // If the ETag the client gave does not match the entity
> > // etag, then the entire entity is returned.
> > - if (resourceETag != null && resourceETag.startsWith("\"") &&
> > resourceETag.equals(headerValue.trim())) {
> > + if (resourceETag != null && resourceETag.equals(headerValue)) {
>
> This doesn't look right. Weak entity tags should never match for a
> request using If-Range. I think you need "... && !weakETag" in the above
> condition as well.
Given the report in BZ and my reading of the specification, I missed
it. Should I revert it ?
Rémy
> Mark
>
>
>
> > return true;
> > } else {
> > return false;
> > }
> > } else {
> > - // unit of HTTP date is second, ignore millisecond part.
> > - return resourceLastModified >= headerValueTime &&
> > resourceLastModified < headerValueTime + 1000;
> > + long headerValueTime = -1L;
> > + try {
> > + headerValueTime = request.getDateHeader("If-Range");
> > + } catch (IllegalArgumentException e) {
> > + // Ignore
> > + }
> > + if (headerValueTime >= 0) {
> > + // unit of HTTP date is second, ignore millisecond part.
> > + return resourceLastModified >= headerValueTime &&
> > resourceLastModified < headerValueTime + 1000;
> > + } else {
> > + // Not a single entity tag and not a valid date either
> > + response.sendError(HttpServletResponse.SC_BAD_REQUEST);
> > + return false;
> > + }
> > }
> > +
> > }
> >
> > /**
> > diff --git
> > a/test/org/apache/catalina/servlets/TestDefaultServletRangeRequests.java
> > b/test/org/apache/catalina/servlets/TestDefaultServletRangeRequests.java
> > index af6031a159..4fb3952531 100644
> > --- a/test/org/apache/catalina/servlets/TestDefaultServletRangeRequests.java
> > +++ b/test/org/apache/catalina/servlets/TestDefaultServletRangeRequests.java
> > @@ -16,7 +16,10 @@
> > */
> > package org.apache.catalina.servlets;
> >
> > +import java.io.ByteArrayOutputStream;
> > import java.io.File;
> > +import java.io.FileInputStream;
> > +import java.security.MessageDigest;
> > import java.util.ArrayList;
> > import java.util.Collection;
> > import java.util.HashMap;
> > @@ -30,9 +33,12 @@ import org.junit.runners.Parameterized;
> > import org.junit.runners.Parameterized.Parameter;
> >
> > import org.apache.catalina.Context;
> > +import org.apache.catalina.Wrapper;
> > import org.apache.catalina.startup.Tomcat;
> > import org.apache.catalina.startup.TomcatBaseTest;
> > +import org.apache.catalina.util.IOTools;
> > import org.apache.tomcat.util.buf.ByteChunk;
> > +import org.apache.tomcat.util.buf.HexUtils;
> > import org.apache.tomcat.util.http.FastHttpDateFormat;
> >
> > @RunWith(Parameterized.class)
> > @@ -47,6 +53,14 @@ public class TestDefaultServletRangeRequests extends
> > TomcatBaseTest {
> > long len = index.length();
> > String strLen = Long.toString(len);
> > String lastModified =
> > FastHttpDateFormat.formatDate(index.lastModified());
> > + String weakETag = "W/\"" + strLen + "-" +
> > Long.toString(index.lastModified()) + "\"";
> > + String strongETag = "\"\"";
> > + try (FileInputStream is = new FileInputStream(index)) {
> > + ByteArrayOutputStream os = new ByteArrayOutputStream();
> > + IOTools.flow(is, os);
> > + strongETag = "\"" +
> > HexUtils.toHexString(MessageDigest.getInstance("SHA-1").digest(os.toByteArray()))
> > + "\"";
> > + } catch (Exception e) {
> > + }
> >
> > List<Object[]> parameterSets = new ArrayList<>();
> >
> > @@ -98,11 +112,24 @@ public class TestDefaultServletRangeRequests extends
> > TomcatBaseTest {
> > parameterSets.add(new Object[] {
> > "bytes=0-9", lastModified, Integer.valueOf(206), "10",
> > "0-9/" + len });
> > // Nonsense data (request rejected)
> > + parameterSets.add(new Object[] { "bytes=0-9", "a-b-c",
> > Integer.valueOf(400), null, "" });
> > + parameterSets.add(new Object[] { "bytes=0-9", "W\"123\"",
> > Integer.valueOf(400), null, "" });
> > + parameterSets.add(new Object[] { "bytes=0-9", "\"123\"W",
> > Integer.valueOf(400), null, "" });
> > + // More than one (request rejected)
> > parameterSets.add(new Object[] {
> > - "bytes=0-9", "a-b-c", Integer.valueOf(400), null, ""});
> > + "bytes=0-9", "\"46273648\", " + weakETag,
> > Integer.valueOf(400), null, "" });
> > // Different date (return whole entity)
> > parameterSets.add(new Object[] {
> > - "bytes=0-9", FastHttpDateFormat.formatDate(1000),
> > Integer.valueOf(200), strLen, ""});
> > + "bytes=0-9", FastHttpDateFormat.formatDate(1000),
> > Integer.valueOf(200), strLen, "" });
> > + // Valid weak etag
> > + parameterSets.add(new Object[] {
> > + "bytes=0-9", weakETag, Integer.valueOf(206), "10", "0-9/"
> > + len });
> > + // Invalid strong etag
> > + parameterSets.add(new Object[] {
> > + "bytes=0-9", "\"46273648\"", Integer.valueOf(200), strLen,
> > "" });
> > + // Valid strong etag
> > + parameterSets.add(new Object[] {
> > + "bytes=0-9", strongETag, Integer.valueOf(206), "10",
> > "0-9/" + len });
> >
> > return parameterSets;
> > }
> > @@ -126,7 +153,10 @@ public class TestDefaultServletRangeRequests extends
> > TomcatBaseTest {
> > File appDir = new File("test/webapp");
> > Context ctxt = tomcat.addContext("", appDir.getAbsolutePath());
> >
> > - Tomcat.addServlet(ctxt, "default", DefaultServlet.class.getName());
> > + Wrapper wrapper = Tomcat.addServlet(ctxt, "default",
> > DefaultServlet.class.getName());
> > + if (ifRangeHeader != null && ifRangeHeader.startsWith("\"")) {
> > + wrapper.addInitParameter("useStrongETags", "true");
> > + }
> > ctxt.addServletMappingDecoded("/", "default");
> >
> > tomcat.start();
> > diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
> > index 7c3c7baf41..8b39743e1d 100644
> > --- a/webapps/docs/changelog.xml
> > +++ b/webapps/docs/changelog.xml
> > @@ -170,6 +170,11 @@
> > Avoid scenarios where temporary files used for partial PUT would
> > not
> > be deleted. (remm)
> > </fix>
> > + <fix>
> > + <bug>69602</bug>: Fix regression in releases from 12-2024 that
> > were too
> > + strict and rejected weak etags in the <code>If-Range</code> header.
> > + (remm)
> > + </fix>
> > </changelog>
> > </subsection>
> > <subsection name="Coyote">
> >
> >
> > ---------------------------------------------------------------------
> > 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
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
