diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java b/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java index 656c25e9036..0eb0d557ddd 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java @@ -25,7 +25,9 @@ import org.slf4j.LoggerFactory; import static java.util.Arrays.asList; import static java.util.Collections.unmodifiableSet; import static java.util.EnumSet.allOf; +import static java.util.EnumSet.complementOf; import static java.util.EnumSet.noneOf; +import static java.util.EnumSet.of; /** * URI compliance modes for Jetty request handling. @@ -37,23 +39,29 @@ public final class UriCompliance implements ComplianceViolation.Mode protected static final Logger LOG = LoggerFactory.getLogger(UriCompliance.class); /** - * These are URI compliance violations, which may be allowed by the compliance mode. Currently all these - * violations are for additional criteria in excess of the strict requirements of rfc3986. + * These are URI compliance "violations", which may be allowed by the compliance mode. These are actual + * violations of the RFC, as they represent additional requirements in excess of the strict compliance of rfc3986. + * A compliance mode that contains one or more of these Violations, allows request to violate the corresponding + * additional requirement. */ public enum Violation implements ComplianceViolation { /** - * Ambiguous path segments e.g. /foo/%2e%2e/bar + * Allow ambiguous path segments e.g. /foo/%2e%2e/bar */ AMBIGUOUS_PATH_SEGMENT("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path segment"), /** - * Ambiguous path separator within a URI segment e.g. /foo/b%2fr + * Allow ambiguous path separator within a URI segment e.g. /foo/b%2fr */ AMBIGUOUS_PATH_SEPARATOR("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path separator"), /** - * Ambiguous path parameters within a URI segment e.g. /foo/..;/bar + * Allow ambiguous path parameters within a URI segment e.g. /foo/..;/bar */ - AMBIGUOUS_PATH_PARAMETER("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path parameter"); + AMBIGUOUS_PATH_PARAMETER("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path parameter"), + /** + * Allow Non canonical ambiguous paths. eg /foo/x%2f%2e%2e%/bar provided to applications as /foo/x/../bar + */ + NON_CANONICAL_AMBIGUOUS_PATHS("https://tools.ietf.org/html/rfc3986#section-3.3", "Non canonical ambiguous paths"); private final String _url; private final String _description; @@ -84,19 +92,28 @@ public final class UriCompliance implements ComplianceViolation.Mode } /** - * The default compliance mode that extends RFC3986 compliance with additional violations to avoid ambiguous URIs + * The default compliance mode that extends RFC3986 compliance with additional violations to avoid most ambiguous URIs. + * This mode does allow {@link Violation#AMBIGUOUS_PATH_SEPARATOR}, but disallows + * {@link Violation#AMBIGUOUS_PATH_PARAMETER} and {@link Violation#AMBIGUOUS_PATH_SEGMENT}. + * Ambiguous paths are not allowed by {@link Violation#NON_CANONICAL_AMBIGUOUS_PATHS}. */ - public static final UriCompliance DEFAULT = new UriCompliance("DEFAULT", noneOf(Violation.class)); + public static final UriCompliance DEFAULT = new UriCompliance("DEFAULT", of(Violation.AMBIGUOUS_PATH_SEPARATOR)); /** - * LEGACY compliance mode that disallows only ambiguous path parameters as per Jetty-9.4 + * LEGACY compliance mode that models Jetty-9.4 behavior by allowing {@link Violation#AMBIGUOUS_PATH_SEGMENT} and {@link Violation#AMBIGUOUS_PATH_SEPARATOR} */ - public static final UriCompliance LEGACY = new UriCompliance("LEGACY", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_SEPARATOR)); + public static final UriCompliance LEGACY = new UriCompliance("LEGACY", of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_SEPARATOR)); /** - * Compliance mode that exactly follows RFC3986, including allowing all additional ambiguous URI Violations + * Compliance mode that exactly follows RFC3986, including allowing all additional ambiguous URI Violations, + * except {@link Violation#NON_CANONICAL_AMBIGUOUS_PATHS}, thus ambiguous paths are canonicalized for safety. */ - public static final UriCompliance RFC3986 = new UriCompliance("RFC3986", allOf(Violation.class)); + public static final UriCompliance RFC3986 = new UriCompliance("RFC3986", complementOf(of(Violation.NON_CANONICAL_AMBIGUOUS_PATHS))); + + /** + * Compliance mode that allows all URI Violations, including allowing ambiguous paths in non canonicalized form. + */ + public static final UriCompliance UNSAFE = new UriCompliance("UNSAFE", allOf(Violation.class)); /** * @deprecated equivalent to DEFAULT @@ -125,6 +142,17 @@ public final class UriCompliance implements ComplianceViolation.Mode return null; } + /** + * Create compliance set from a set of allowed Violations. + * + * @param violations A string of violations to allow: + * @return the compliance from the string spec + */ + public static UriCompliance from(Set violations) + { + return new UriCompliance("CUSTOM" + __custom.getAndIncrement(), violations); + } + /** * Create compliance set from string. *

@@ -151,22 +179,23 @@ public final class UriCompliance implements ComplianceViolation.Mode */ public static UriCompliance from(String spec) { - Set sections; + Set violations; String[] elements = spec.split("\\s*,\\s*"); switch (elements[0]) { case "0": - sections = noneOf(Violation.class); + violations = noneOf(Violation.class); break; case "*": - sections = allOf(Violation.class); + violations = allOf(Violation.class); break; default: { UriCompliance mode = UriCompliance.valueOf(elements[0]); - sections = (mode == null) ? noneOf(Violation.class) : copyOf(mode.getAllowed()); + violations = (mode == null) ? noneOf(Violation.class) : copyOf(mode.getAllowed()); + break; } } @@ -178,12 +207,12 @@ public final class UriCompliance implements ComplianceViolation.Mode element = element.substring(1); Violation section = Violation.valueOf(element); if (exclude) - sections.remove(section); + violations.remove(section); else - sections.add(section); + violations.add(section); } - UriCompliance compliance = new UriCompliance("CUSTOM" + __custom.getAndIncrement(), sections); + UriCompliance compliance = new UriCompliance("CUSTOM" + __custom.getAndIncrement(), violations); if (LOG.isDebugEnabled()) LOG.debug("UriCompliance from {}->{}", spec, compliance); return compliance; @@ -192,7 +221,7 @@ public final class UriCompliance implements ComplianceViolation.Mode private final String _name; private final Set _allowed; - private UriCompliance(String name, Set violations) + public UriCompliance(String name, Set violations) { Objects.requireNonNull(violations); _name = name; diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/SyntaxTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/SyntaxTest.java index a31995cbd24..7e944e38f55 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/SyntaxTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/SyntaxTest.java @@ -18,6 +18,7 @@ import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.containsString; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.fail; public class SyntaxTest @@ -61,17 +62,11 @@ public class SyntaxTest for (String token : tokens) { - try - { - Syntax.requireValidRFC2616Token(token, "Test Based"); - fail("RFC2616 Token [" + token + "] Should have thrown " + IllegalArgumentException.class.getName()); - } - catch (IllegalArgumentException e) - { - assertThat("Testing Bad RFC2616 Token [" + token + "]", e.getMessage(), + Throwable e = assertThrows(IllegalArgumentException.class, + () -> Syntax.requireValidRFC2616Token(token, "Test Based")); + assertThat("Testing Bad RFC2616 Token [" + token + "]", e.getMessage(), allOf(containsString("Test Based"), - containsString("RFC2616"))); - } + containsString("RFC2616"))); } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java index 824b1b21676..3b57dae3545 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java @@ -263,9 +263,7 @@ public class HttpConnection extends AbstractConnection implements Runnable, Http { // Fill the request buffer (if needed). int filled = fillRequestBuffer(); - if (filled > 0) - bytesIn.add(filled); - else if (filled == -1 && getEndPoint().isOutputShutdown()) + if (filled < 0 && getEndPoint().isOutputShutdown()) close(); // Parse the request buffer. @@ -352,8 +350,9 @@ public class HttpConnection extends AbstractConnection implements Runnable, Http if (filled == 0) // Do a retry on fill 0 (optimization for SSL connections) filled = getEndPoint().fill(_requestBuffer); - // tell parser - if (filled < 0) + if (filled > 0) + bytesIn.add(filled); + else if (filled < 0) _parser.atEOF(); if (LOG.isDebugEnabled()) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 9e1628927e6..0cc9ff52fc5 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -67,7 +67,6 @@ import jakarta.servlet.http.WebConnection; import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.HostPortHttpField; -import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.http.HttpCookie.SetCookieHttpField; import org.eclipse.jetty.http.HttpField; @@ -1692,10 +1691,11 @@ public class Request implements HttpServletRequest _httpFields = request.getFields(); final HttpURI uri = request.getURI(); + UriCompliance compliance = null; boolean ambiguous = uri.isAmbiguous(); if (ambiguous) { - UriCompliance compliance = _channel == null || _channel.getHttpConfiguration() == null ? null : _channel.getHttpConfiguration().getUriCompliance(); + compliance = _channel == null || _channel.getHttpConfiguration() == null ? null : _channel.getHttpConfiguration().getUriCompliance(); if (uri.hasAmbiguousSegment() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_PATH_SEGMENT))) throw new BadMessageException("Ambiguous segment in URI"); if (uri.hasAmbiguousSeparator() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_PATH_SEPARATOR))) @@ -1746,9 +1746,9 @@ public class Request implements HttpServletRequest path = (encoded.length() == 1) ? "/" : _uri.getDecodedPath(); // Strictly speaking if a URI is legal and encodes ambiguous segments, then they should be // reflected in the decoded string version. However, it can be ambiguous to provide a decoded path as - // a string, so we normalize again. If an application wishes to see ambiguous URIs, then they can look - // at the encoded form of the URI - if (ambiguous) + // a string, so we normalize again. If an application wishes to see ambiguous URIs, then they must + // set the {@link UriCompliance.Violation#NON_CANONICAL_AMBIGUOUS_PATHS} compliance. + if (ambiguous && (compliance == null || !compliance.allows(UriCompliance.Violation.NON_CANONICAL_AMBIGUOUS_PATHS))) path = URIUtil.canonicalPath(path); } else if ("*".equals(encoded) || HttpMethod.CONNECT.is(getMethod())) diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java index 8e35702eda4..bfb7e84e246 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java @@ -32,6 +32,8 @@ import java.util.List; import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; +import java.util.stream.IntStream; import java.util.stream.Stream; import jakarta.servlet.ServletException; @@ -40,6 +42,7 @@ import jakarta.servlet.http.HttpServletResponse; import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpParser; +import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.logging.StacklessLogging; @@ -47,6 +50,7 @@ import org.eclipse.jetty.server.LocalConnector.LocalEndPoint; import org.eclipse.jetty.server.handler.AbstractHandler; import org.eclipse.jetty.server.handler.ErrorHandler; import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.util.IO; import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -59,9 +63,11 @@ import org.slf4j.LoggerFactory; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.lessThanOrEqualTo; import static org.hamcrest.Matchers.not; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; public class HttpConnectionTest @@ -1353,6 +1359,68 @@ public class HttpConnectionTest } } + @Test + public void testBytesIn() throws Exception + { + String chunk1 = "0123456789ABCDEF"; + String chunk2 = IntStream.range(0, 64).mapToObj(i -> chunk1).collect(Collectors.joining()); + long dataLength = chunk1.length() + chunk2.length(); + server.stop(); + server.setHandler(new AbstractHandler() + { + @Override + public void handle(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response) throws IOException + { + jettyRequest.setHandled(true); + IO.copy(request.getInputStream(), IO.getNullStream()); + + HttpConnection connection = HttpConnection.getCurrentConnection(); + long bytesIn = connection.getBytesIn(); + assertThat(bytesIn, greaterThan(dataLength)); + } + }); + server.start(); + + LocalEndPoint localEndPoint = connector.executeRequest("" + + "POST / HTTP/1.1\r\n" + + "Host: localhost\r\n" + + "Content-Length: " + dataLength + "\r\n" + + "\r\n" + + chunk1); + + // Wait for the server to block on the read(). + Thread.sleep(500); + + // Send more content. + localEndPoint.addInput(chunk2); + + HttpTester.Response response = HttpTester.parseResponse(localEndPoint.getResponse()); + assertEquals(response.getStatus(), HttpStatus.OK_200); + localEndPoint.close(); + + localEndPoint = connector.executeRequest("" + + "POST / HTTP/1.1\r\n" + + "Host: localhost\r\n" + + "Transfer-Encoding: chunked\r\n" + + "\r\n" + + Integer.toHexString(chunk1.length()) + "\r\n" + + chunk1 + "\r\n"); + + // Wait for the server to block on the read(). + Thread.sleep(500); + + // Send more content. + localEndPoint.addInput("" + + Integer.toHexString(chunk2.length()) + "\r\n" + + chunk2 + "\r\n" + + "0\r\n" + + "\r\n"); + + response = HttpTester.parseResponse(localEndPoint.getResponse()); + assertEquals(response.getStatus(), HttpStatus.OK_200); + localEndPoint.close(); + } + private int checkContains(String s, int offset, String c) { assertThat(s.substring(offset), Matchers.containsString(c)); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java index c1bdaa51226..ca7541f3a86 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java @@ -28,6 +28,7 @@ import java.security.Principal; import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; +import java.util.EnumSet; import java.util.Enumeration; import java.util.List; import java.util.Locale; @@ -1733,12 +1734,45 @@ public class RequestTest "Host: whatever\r\n" + "\r\n"; _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.DEFAULT); + assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); + _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(new UriCompliance("Test", EnumSet.noneOf(UriCompliance.Violation.class))); assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 400")); _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.LEGACY); assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.RFC3986); assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); } + + @Test + public void testAmbiguousPaths() throws Exception + { + _handler._checker = (request, response) -> + { + response.getOutputStream().println("servletPath=" + request.getServletPath()); + response.getOutputStream().println("pathInfo=" + request.getPathInfo()); + return true; + }; + String request = "GET /unnormal/.././path/ambiguous%2f%2e%2e/%2e;/info HTTP/1.0\r\n" + + "Host: whatever\r\n" + + "\r\n"; + + _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.from(EnumSet.of( + UriCompliance.Violation.AMBIGUOUS_PATH_SEPARATOR, + UriCompliance.Violation.AMBIGUOUS_PATH_SEGMENT, + UriCompliance.Violation.AMBIGUOUS_PATH_PARAMETER))); + assertThat(_connector.getResponse(request), Matchers.allOf( + startsWith("HTTP/1.1 200"), + containsString("pathInfo=/path/info"))); + + _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.from(EnumSet.of( + UriCompliance.Violation.AMBIGUOUS_PATH_SEPARATOR, + UriCompliance.Violation.AMBIGUOUS_PATH_SEGMENT, + UriCompliance.Violation.AMBIGUOUS_PATH_PARAMETER, + UriCompliance.Violation.NON_CANONICAL_AMBIGUOUS_PATHS))); + assertThat(_connector.getResponse(request), Matchers.allOf( + startsWith("HTTP/1.1 200"), + containsString("pathInfo=/path/ambiguous/.././info"))); + } @Test public void testPushBuilder() diff --git a/tests/test-integration/src/test/java/org/eclipse/jetty/test/GzipWithSendErrorTest.java b/tests/test-integration/src/test/java/org/eclipse/jetty/test/GzipWithSendErrorTest.java index 96f7cb17e22..eb56b12e533 100644 --- a/tests/test-integration/src/test/java/org/eclipse/jetty/test/GzipWithSendErrorTest.java +++ b/tests/test-integration/src/test/java/org/eclipse/jetty/test/GzipWithSendErrorTest.java @@ -251,7 +251,8 @@ public class GzipWithSendErrorTest assertThat("Request Input Content Received", inputContentReceived.get(), is(0L)); assertThat("Request Input Content Received less then initial buffer", inputContentReceived.get(), lessThanOrEqualTo((long)sizeActuallySent)); assertThat("Request Connection BytesIn should have some minimal data", inputBytesIn.get(), greaterThanOrEqualTo(1024L)); - assertThat("Request Connection BytesIn read should not have read all of the data", inputBytesIn.get(), lessThanOrEqualTo((long)sizeActuallySent)); + long requestBytesSent = sizeActuallySent + 512; // Take into account headers and chunked metadata. + assertThat("Request Connection BytesIn read should not have read all of the data", inputBytesIn.get(), lessThanOrEqualTo(requestBytesSent)); // Now provide rest content.offer(ByteBuffer.wrap(compressedRequest, sizeActuallySent, compressedRequest.length - sizeActuallySent)); @@ -351,7 +352,8 @@ public class GzipWithSendErrorTest assertThat("Request Input Content Received", inputContentReceived.get(), read ? greaterThan(0L) : is(0L)); assertThat("Request Input Content Received less then initial buffer", inputContentReceived.get(), lessThanOrEqualTo((long)sizeActuallySent)); assertThat("Request Connection BytesIn should have some minimal data", inputBytesIn.get(), greaterThanOrEqualTo(1024L)); - assertThat("Request Connection BytesIn read should not have read all of the data", inputBytesIn.get(), lessThanOrEqualTo((long)sizeActuallySent)); + long requestBytesSent = sizeActuallySent + 512; // Take into account headers and chunked metadata. + assertThat("Request Connection BytesIn read should not have read all of the data", inputBytesIn.get(), lessThanOrEqualTo(requestBytesSent)); // Now provide rest content.offer(ByteBuffer.wrap(compressedRequest, sizeActuallySent, compressedRequest.length - sizeActuallySent));