From 372b407d04b11f685f8135e0925f2e5b2765fd59 Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 8 Feb 2021 15:46:01 +0100 Subject: [PATCH 01/11] Fix #4275 fail URIs with ambiguous segments Fix #4275 fail URIs with ambiguous segments. --- .../eclipse/jetty/http/HttpCompliance.java | 2 +- .../jetty/http/HttpComplianceSection.java | 3 +- .../org/eclipse/jetty/http/HttpParser.java | 5 ++ .../java/org/eclipse/jetty/http/HttpURI.java | 80 ++++++++++++++++++- .../org/eclipse/jetty/http/HttpURITest.java | 39 +++++++++ .../eclipse/jetty/server/HttpConnection.java | 6 ++ .../org/eclipse/jetty/server/Request.java | 14 ++++ 7 files changed, 144 insertions(+), 5 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java index 1488c89622e5..2d840e193767 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java @@ -53,7 +53,7 @@ public enum HttpCompliance // TODO in Jetty-10 convert this enum to a class so t /** * A Legacy compliance mode to match jetty's behavior prior to RFC2616 and RFC7230. */ - LEGACY(sectionsBySpec("0,METHOD_CASE_SENSITIVE")), + LEGACY(sectionsBySpec("0,METHOD_CASE_SENSITIVE,AMBIGUOUS_PATH_SEGMENTS")), /** * The legacy RFC2616 support, which incorrectly excludes diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpComplianceSection.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpComplianceSection.java index e1dcc025a6dc..4e0f515caf16 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpComplianceSection.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpComplianceSection.java @@ -31,7 +31,8 @@ public enum HttpComplianceSection NO_FIELD_FOLDING("https://tools.ietf.org/html/rfc7230#section-3.2.4", "No line Folding"), NO_HTTP_0_9("https://tools.ietf.org/html/rfc7230#appendix-A.2", "No HTTP/0.9"), TRANSFER_ENCODING_WITH_CONTENT_LENGTH("https://tools.ietf.org/html/rfc7230#section-3.3.1", "Transfer-Encoding and Content-Length"), - MULTIPLE_CONTENT_LENGTHS("https://tools.ietf.org/html/rfc7230#section-3.3.1", "Multiple Content-Lengths"); + MULTIPLE_CONTENT_LENGTHS("https://tools.ietf.org/html/rfc7230#section-3.3.1", "Multiple Content-Lengths"), + AMBIGUOUS_PATH_SEGMENTS("", "Allows ambiguous URI path segments"); final String url; final String description; diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java index 237e9613057f..4637006c8644 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java @@ -311,6 +311,11 @@ public HttpHandler getHandler() return _handler; } + public HttpCompliance getHttpCompliance() + { + return _compliance; + } + /** * Check RFC compliance violation * diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java index 6b5cdff29952..d810dda3b9e4 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java @@ -24,7 +24,9 @@ import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; +import org.eclipse.jetty.util.ArrayTrie; import org.eclipse.jetty.util.MultiMap; +import org.eclipse.jetty.util.Trie; import org.eclipse.jetty.util.TypeUtil; import org.eclipse.jetty.util.URIUtil; import org.eclipse.jetty.util.UrlEncoded; @@ -65,6 +67,18 @@ private enum State ASTERISK } + private static final Trie __ambiguousSegments = new ArrayTrie<>(); + + static + { + __ambiguousSegments.put("%2e", Boolean.TRUE); + __ambiguousSegments.put("%2e%2e", Boolean.TRUE); + __ambiguousSegments.put(".%2e", Boolean.TRUE); + __ambiguousSegments.put("%2e.", Boolean.TRUE); + __ambiguousSegments.put("..", Boolean.FALSE); + __ambiguousSegments.put(".", Boolean.FALSE); + } + private String _scheme; private String _user; private String _host; @@ -77,6 +91,8 @@ private enum State String _uri; String _decodedPath; + boolean _ambiguousSegment; + /** * Construct a normalized URI. * Port is not set if it is the default port. @@ -208,6 +224,8 @@ private void parse(State state, final String uri, final int offset, final int en boolean encoded = false; int mark = offset; int pathMark = 0; + int segment = 0; + boolean encodedSegment = false; for (int i = offset; i < end; i++) { @@ -241,14 +259,18 @@ private void parse(State state, final String uri, final int offset, final int en _path = "*"; state = State.ASTERISK; break; - + case '%': + encoded = encodedSegment = true; + mark = pathMark = segment = i; + state = State.PATH; + break; default: mark = i; if (_scheme == null) state = State.SCHEME_OR_PATH; else { - pathMark = i; + pathMark = segment = i; state = State.PATH; } } @@ -269,6 +291,7 @@ private void parse(State state, final String uri, final int offset, final int en case '/': // must have been in a path and still are + segment = i + 1; state = State.PATH; break; @@ -288,6 +311,7 @@ private void parse(State state, final String uri, final int offset, final int en case '%': // must have be in an encoded path encoded = true; + encodedSegment = true; state = State.PATH; break; @@ -317,11 +341,13 @@ private void parse(State state, final String uri, final int offset, final int en // was a path, look again i--; pathMark = mark; + segment = mark + 1; state = State.PATH; break; default: // it is a path pathMark = mark; + segment = mark + 1; state = State.PATH; } continue; @@ -334,6 +360,7 @@ private void parse(State state, final String uri, final int offset, final int en case '/': _host = uri.substring(mark, i); pathMark = mark = i; + segment = mark + 1; state = State.PATH; break; case ':': @@ -396,6 +423,7 @@ else if (c == '/') { _port = TypeUtil.parseInt(uri, mark, i - mark, 10); pathMark = mark = i; + segment = i + 1; state = State.PATH; } continue; @@ -406,21 +434,30 @@ else if (c == '/') switch (c) { case ';': + checkSegment(encodedSegment, true, uri, segment, i); mark = i + 1; state = State.PARAM; break; case '?': + checkSegment(encodedSegment, false, uri, segment, i); _path = uri.substring(pathMark, i); mark = i + 1; state = State.QUERY; break; case '#': + checkSegment(encodedSegment, false, uri, segment, i); _path = uri.substring(pathMark, i); mark = i + 1; state = State.FRAGMENT; break; case '%': encoded = true; + encodedSegment = true; + break; + case '/': + checkSegment(encodedSegment, false, uri, segment, i); + encodedSegment = false; + segment = i + 1; break; } continue; @@ -443,8 +480,10 @@ else if (c == '/') state = State.FRAGMENT; break; case '/': - encoded = true; // ignore internal params + encoded = true; + encodedSegment = false; + segment = i + 1; state = State.PATH; break; case ';': @@ -516,6 +555,7 @@ else if (c == '/') break; case PATH: + checkSegment(encodedSegment, false, uri, segment, end); _path = uri.substring(pathMark, end); break; @@ -533,6 +573,38 @@ else if (c == '/') } } + /** + * Check for ambiguous path segments. + * + * An ambiguous path segment is one that is perhaps technically legal, but is considered undesirable to handle + * due to possible ambiguity. Examples include segments like '..;', '%2e', '%2e%2e' etc. + * @param segmentEncoded True if the segment is encoded + * @param param true if the segment has a parameter + * @param uri The URI string + * @param segment The inclusive starting index of the segment (excluding any '/') + * @param end The exclusive end index of the segment + */ + private void checkSegment(boolean segmentEncoded, boolean param, String uri, int segment, int end) + { + // if the segment is encoded or there is a segment parameter + if (!_ambiguousSegment && (segmentEncoded || param)) + { + // Look for possible bad segments + Boolean ambiguous = __ambiguousSegments.get(uri, segment, end - segment); + + // A segment is ambiguous if it is always ambiguous (TRUE) or ambiguous only with params (FALSE) and we have a param + _ambiguousSegment |= ambiguous == Boolean.TRUE || (param && ambiguous == Boolean.FALSE); + } + } + + /** + * @return True if the URI has a possibly ambiguous segment like '..;' or '%2e%2e' + */ + public boolean hasAmbiguousSegment() + { + return _ambiguousSegment; + } + public String getScheme() { return _scheme; @@ -633,6 +705,8 @@ public void clear() _fragment = null; _decodedPath = null; + + _ambiguousSegment = false; } public boolean isAbsolute() diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java index b341e57b3334..3a25920127b1 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java @@ -20,9 +20,12 @@ import java.net.URLEncoder; import java.nio.charset.StandardCharsets; +import java.util.stream.Stream; import org.eclipse.jetty.util.MultiMap; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; @@ -221,4 +224,40 @@ public void testBasicAuthCredentials() throws Exception assertEquals(uri.getAuthority(), "example.com:8888"); assertEquals(uri.getUser(), "user:password"); } + + public static Stream pathsWithAmbiguousSegments() + { + // @checkstyle-disable-check : AvoidEscapedUnicodeCharactersCheck + return Stream.of( + "/foo/%2e%2e/bar", + "/foo/%2e%2e;/bar", + "/foo/%2e%2e;param/bar", + "/foo/..;/bar", + "/foo/..;param/bar", + "foo/%2e%2e/bar", + "%2e%2e/bar", + "//host/%2e%2e/bar", + "scheme://host/%2e%2e/bar", + "scheme:/%2e%2e/bar", + "/foo/%2e%2e", + "/foo/%2e%2e?query", + "/foo/%2e%2e#fragment", + "foo/%2e.", + ".%2e", + "//host/%2e%2e", + "scheme://host/.%2e", + "scheme:/%2e%2e", + "%2e", + "%2e.", + ".%2e", + "%2e%2e" + ); + } + + @ParameterizedTest(name = "[{index}] {0}") + @MethodSource("pathsWithAmbiguousSegments") + public void testAmbiguousSegments(String uriWithBadSegment) + { + assertTrue(new HttpURI(uriWithBadSegment).hasAmbiguousSegment()); + } } 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 3ae8bbe4bc67..024d57ec2459 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 @@ -114,6 +114,12 @@ public HttpConnection(HttpConfiguration config, Connector connector, EndPoint en LOG.debug("New HTTP Connection {}", this); } + @Deprecated + public HttpCompliance getHttpCompliance() + { + return _parser.getHttpCompliance(); + } + public HttpConfiguration getHttpConfiguration() { return _config; 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 ff0f46a8e97d..b81511230014 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 @@ -65,6 +65,7 @@ import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HostPortHttpField; +import org.eclipse.jetty.http.HttpComplianceSection; import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpFields; @@ -77,6 +78,7 @@ import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MetaData; import org.eclipse.jetty.http.MimeTypes; +import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.RuntimeIOException; import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.server.handler.ContextHandler.Context; @@ -1815,6 +1817,18 @@ public void setMetaData(org.eclipse.jetty.http.MetaData.Request request) setMethod(request.getMethod()); HttpURI uri = request.getURI(); + + if (uri.hasAmbiguousSegment()) + { + // TODO replace in jetty-10 with HttpCompliance from the HttpConfiguration + Connection connection = _channel.getConnection(); + boolean allow = connection instanceof HttpConnection + ? ((HttpConnection)connection).getHttpCompliance().sections().contains(HttpComplianceSection.AMBIGUOUS_PATH_SEGMENTS) + : _channel.getServer().getAttribute(HttpComplianceSection.AMBIGUOUS_PATH_SEGMENTS.toString()) == Boolean.TRUE; + if (!allow) + throw new BadMessageException("Ambiguous segment in URI"); + } + _originalURI = uri.isAbsolute() && request.getHttpVersion() != HttpVersion.HTTP_2 ? uri.toString() : uri.getPathQuery(); String encoded = uri.getPath(); From ff645442f7b100c31af418c31988a4c5008e0678 Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 11 Feb 2021 15:39:21 +0100 Subject: [PATCH 02/11] updates from review Invert sense of ambiguous path segment compliance added test --- .../eclipse/jetty/http/HttpCompliance.java | 17 +++---------- .../jetty/http/HttpComplianceSection.java | 2 +- .../org/eclipse/jetty/server/Request.java | 8 +++--- .../org/eclipse/jetty/server/RequestTest.java | 25 +++++++++++++++++++ 4 files changed, 35 insertions(+), 17 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java index 2d840e193767..814344a81074 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java @@ -53,7 +53,7 @@ public enum HttpCompliance // TODO in Jetty-10 convert this enum to a class so t /** * A Legacy compliance mode to match jetty's behavior prior to RFC2616 and RFC7230. */ - LEGACY(sectionsBySpec("0,METHOD_CASE_SENSITIVE,AMBIGUOUS_PATH_SEGMENTS")), + LEGACY(sectionsBySpec("0,METHOD_CASE_SENSITIVE")), /** * The legacy RFC2616 support, which incorrectly excludes @@ -62,7 +62,7 @@ public enum HttpCompliance // TODO in Jetty-10 convert this enum to a class so t * {@link HttpComplianceSection#TRANSFER_ENCODING_WITH_CONTENT_LENGTH}, * {@link HttpComplianceSection#MULTIPLE_CONTENT_LENGTHS}, */ - RFC2616_LEGACY(sectionsBySpec("RFC2616,-FIELD_COLON,-METHOD_CASE_SENSITIVE,-TRANSFER_ENCODING_WITH_CONTENT_LENGTH,-MULTIPLE_CONTENT_LENGTHS")), + RFC2616_LEGACY(sectionsBySpec("RFC2616,-FIELD_COLON,-METHOD_CASE_SENSITIVE,-TRANSFER_ENCODING_WITH_CONTENT_LENGTH,-MULTIPLE_CONTENT_LENGTHS,-NO_AMBIGUOUS_PATH_SEGMENTS")), /** * The strict RFC2616 support mode @@ -72,7 +72,7 @@ public enum HttpCompliance // TODO in Jetty-10 convert this enum to a class so t /** * Jetty's current RFC7230 support, which incorrectly excludes {@link HttpComplianceSection#METHOD_CASE_SENSITIVE} */ - RFC7230_LEGACY(sectionsBySpec("RFC7230,-METHOD_CASE_SENSITIVE")), + RFC7230_LEGACY(sectionsBySpec("RFC7230,-METHOD_CASE_SENSITIVE,-NO_AMBIGUOUS_PATH_SEGMENTS")), /** * The RFC7230 support mode @@ -123,11 +123,6 @@ static EnumSet sectionsBySpec(String spec) i++; break; - case "*": - i++; - sections = EnumSet.allOf(HttpComplianceSection.class); - break; - case "RFC2616": sections = EnumSet.complementOf(EnumSet.of( HttpComplianceSection.NO_FIELD_FOLDING, @@ -135,6 +130,7 @@ static EnumSet sectionsBySpec(String spec) i++; break; + case "*": case "RFC7230": i++; sections = EnumSet.allOf(HttpComplianceSection.class); @@ -152,11 +148,6 @@ static EnumSet sectionsBySpec(String spec) if (exclude) element = element.substring(1); HttpComplianceSection section = HttpComplianceSection.valueOf(element); - if (section == null) - { - LOG.warn("Unknown section '" + element + "' in HttpCompliance spec: " + spec); - continue; - } if (exclude) sections.remove(section); else diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpComplianceSection.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpComplianceSection.java index 4e0f515caf16..ce5fa6645a3b 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpComplianceSection.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpComplianceSection.java @@ -32,7 +32,7 @@ public enum HttpComplianceSection NO_HTTP_0_9("https://tools.ietf.org/html/rfc7230#appendix-A.2", "No HTTP/0.9"), TRANSFER_ENCODING_WITH_CONTENT_LENGTH("https://tools.ietf.org/html/rfc7230#section-3.3.1", "Transfer-Encoding and Content-Length"), MULTIPLE_CONTENT_LENGTHS("https://tools.ietf.org/html/rfc7230#section-3.3.1", "Multiple Content-Lengths"), - AMBIGUOUS_PATH_SEGMENTS("", "Allows ambiguous URI path segments"); + NO_AMBIGUOUS_PATH_SEGMENTS("https://tools.ietf.org/html/rfc3986#section-3.3", "No ambiguous URI path segments"); final String url; final String description; 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 b81511230014..b532d45263a1 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 @@ -65,6 +65,7 @@ import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HostPortHttpField; +import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.http.HttpComplianceSection; import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.http.HttpField; @@ -1822,9 +1823,10 @@ public void setMetaData(org.eclipse.jetty.http.MetaData.Request request) { // TODO replace in jetty-10 with HttpCompliance from the HttpConfiguration Connection connection = _channel.getConnection(); - boolean allow = connection instanceof HttpConnection - ? ((HttpConnection)connection).getHttpCompliance().sections().contains(HttpComplianceSection.AMBIGUOUS_PATH_SEGMENTS) - : _channel.getServer().getAttribute(HttpComplianceSection.AMBIGUOUS_PATH_SEGMENTS.toString()) == Boolean.TRUE; + HttpCompliance compliance = connection instanceof HttpConnection + ? ((HttpConnection)connection).getHttpCompliance() + : _channel.getConnector().getBean(HttpCompliance.class); + boolean allow = compliance != null && !compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_PATH_SEGMENTS); if (!allow) throw new BadMessageException("Ambiguous segment in URI"); } 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 8327245054e8..ba7ab2976c40 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 @@ -1836,6 +1836,31 @@ public void testGetterSafeFromNullPointerException() assertEquals(0, request.getParameterMap().size()); } + @Test + public void testAmbiguousPaths() throws Exception + { + _handler._checker = (request, response) -> true; + + String request = "GET /ambiguous/..;/path HTTP/1.0\r\n" + + "Host: whatever\r\n" + + "\r\n"; + + _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC7230); + assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 400")); + + _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC7230_LEGACY); + assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); + + _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC2616); + assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 400")); + + _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC2616_LEGACY); + assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); + + + + } + private static long getFileCount(Path path) { try (Stream s = Files.list(path)) From fa9f6c9186217c09c8806b03885b2732014582c2 Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 11 Feb 2021 17:37:39 +0100 Subject: [PATCH 03/11] updates from review Fixed tests for `%2e%2e` --- .../java/org/eclipse/jetty/server/HttpConnectionTest.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) 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 a480503e1d18..865ccf03a494 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 @@ -507,15 +507,14 @@ public void testBadPathDotDotPath() throws Exception public void testOKPathEncodedDotDotPath() throws Exception { String response = connector.getResponse("GET /ooops/%2e%2e/path HTTP/1.0\r\nHost: localhost:80\r\n\n"); - checkContains(response, 0, "HTTP/1.1 200 OK"); - checkContains(response, 0, "pathInfo=/path"); + checkContains(response, 0, "HTTP/1.1 400 "); } @Test public void testBadPathEncodedDotDotPath() throws Exception { String response = connector.getResponse("GET /ooops/%2e%2e/%2e%2e/path HTTP/1.0\r\nHost: localhost:80\r\n\n"); - checkContains(response, 0, "HTTP/1.1 400 Bad URI"); + checkContains(response, 0, "HTTP/1.1 400 "); } @Test @@ -536,7 +535,7 @@ public void testBadSlashDotDotPath() throws Exception public void testEncodedBadDotDotPath() throws Exception { String response = connector.getResponse("GET %2e%2e/path HTTP/1.0\r\nHost: localhost:80\r\n\n"); - checkContains(response, 0, "HTTP/1.1 400 Bad URI"); + checkContains(response, 0, "HTTP/1.1 400 "); } @Test From 07f6123581081832e88df67a68d522e7389d32d3 Mon Sep 17 00:00:00 2001 From: gregw Date: Fri, 12 Feb 2021 18:19:55 +0100 Subject: [PATCH 04/11] updates from review of RFC + Correctly handle canonical trailing '.' and '..' + Perform canonical path resolution before decoding + Treat decoding of %2f as an ambiguous segment. + better tests of ambiguous segments --- .../java/org/eclipse/jetty/http/HttpURI.java | 117 ++++++++++-------- .../org/eclipse/jetty/http/HttpURITest.java | 89 ++++++++++++- .../org/eclipse/jetty/server/Request.java | 2 +- .../jetty/server/HttpConnectionTest.java | 6 +- .../java/org/eclipse/jetty/util/URIUtil.java | 81 ++++++------ .../jetty/util/URIUtilCanonicalPathTest.java | 14 ++- 6 files changed, 215 insertions(+), 94 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java index d810dda3b9e4..1f6b2d7d29e9 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java @@ -67,6 +67,18 @@ private enum State ASTERISK } + /** + * The concept of URI path parameters was originally specified in + * RFC2396, but that was + * obsoleted by + * RFC3986 which removed + * a normative definition of path parameters. Specifically it excluded them from the + * Remove Dot Segments + * algorithm. This results in some ambiguity as dot segments can result from later + * parameter removal or % encoding expansion, that are not removed from the URI + * by {@link URIUtil#canonicalPath(String)}. Thus this class flags such ambiguous + * path segments, so that they may be rejected by the server if so configured. + */ private static final Trie __ambiguousSegments = new ArrayTrie<>(); static @@ -221,11 +233,12 @@ public void parse(String uri, int offset, int length) private void parse(State state, final String uri, final int offset, final int end) { - boolean encoded = false; - int mark = offset; - int pathMark = 0; - int segment = 0; - boolean encodedSegment = false; + int mark = offset; // the start of the current section being parsed + int pathMark = 0; // the start of the path section + int segment = 0; // the start of the current segment within the path + boolean encoded = false; // set to true if the path contains % encoded characters + boolean dot = false; // set to true if the path containers . or .. segments + int escapedSlash = 0; // state of parsing a %2f for (int i = offset; i < end; i++) { @@ -260,10 +273,16 @@ private void parse(State state, final String uri, final int offset, final int en state = State.ASTERISK; break; case '%': - encoded = encodedSegment = true; + encoded = true; + escapedSlash = 1; mark = pathMark = segment = i; state = State.PATH; break; + case '.' : + dot = true; + pathMark = segment = i; + state = State.PATH; + break; default: mark = i; if (_scheme == null) @@ -277,7 +296,6 @@ private void parse(State state, final String uri, final int offset, final int en continue; } - case SCHEME_OR_PATH: { switch (c) @@ -288,33 +306,28 @@ private void parse(State state, final String uri, final int offset, final int en // Start again with scheme set state = State.START; break; - case '/': // must have been in a path and still are segment = i + 1; state = State.PATH; break; - case ';': // must have been in a path mark = i + 1; state = State.PARAM; break; - case '?': // must have been in a path _path = uri.substring(mark, i); mark = i + 1; state = State.QUERY; break; - case '%': // must have be in an encoded path encoded = true; - encodedSegment = true; + escapedSlash = 1; state = State.PATH; break; - case '#': // must have been in a path _path = uri.substring(mark, i); @@ -323,7 +336,6 @@ private void parse(State state, final String uri, final int offset, final int en } continue; } - case HOST_OR_PATH: { switch (c) @@ -334,10 +346,12 @@ private void parse(State state, final String uri, final int offset, final int en state = State.HOST; break; + case '%': case '@': case ';': case '?': case '#': + case '.': // was a path, look again i--; pathMark = mark; @@ -352,7 +366,6 @@ private void parse(State state, final String uri, final int offset, final int en } continue; } - case HOST: { switch (c) @@ -382,7 +395,6 @@ private void parse(State state, final String uri, final int offset, final int en } continue; } - case IPV6: { switch (c) @@ -407,7 +419,6 @@ private void parse(State state, final String uri, final int offset, final int en continue; } - case PORT: { if (c == '@') @@ -428,41 +439,52 @@ else if (c == '/') } continue; } - case PATH: { switch (c) { case ';': - checkSegment(encodedSegment, true, uri, segment, i); + checkSegment(uri, segment, i, true); mark = i + 1; state = State.PARAM; break; case '?': - checkSegment(encodedSegment, false, uri, segment, i); + checkSegment(uri, segment, i, false); _path = uri.substring(pathMark, i); mark = i + 1; state = State.QUERY; break; case '#': - checkSegment(encodedSegment, false, uri, segment, i); + checkSegment(uri, segment, i, false); _path = uri.substring(pathMark, i); mark = i + 1; state = State.FRAGMENT; break; + case '/': + checkSegment(uri, segment, i, false); + segment = i + 1; + break; + case '.': + dot |= segment == i; + break; case '%': encoded = true; - encodedSegment = true; + escapedSlash = 1; break; - case '/': - checkSegment(encodedSegment, false, uri, segment, i); - encodedSegment = false; - segment = i + 1; + case '2': + escapedSlash = escapedSlash == 1 ? 2 : 0; + break; + case 'f': + case 'F': + _ambiguousSegment |= (escapedSlash == 2); + escapedSlash = 0; + break; + default: + escapedSlash = 0; break; } continue; } - case PARAM: { switch (c) @@ -480,9 +502,7 @@ else if (c == '/') state = State.FRAGMENT; break; case '/': - // ignore internal params encoded = true; - encodedSegment = false; segment = i + 1; state = State.PATH; break; @@ -493,7 +513,6 @@ else if (c == '/') } continue; } - case QUERY: { if (c == '#') @@ -504,12 +523,10 @@ else if (c == '/') } continue; } - case ASTERISK: { throw new IllegalArgumentException("Bad character '*'"); } - case FRAGMENT: { _fragment = uri.substring(mark, end); @@ -525,46 +542,37 @@ else if (c == '/') case SCHEME_OR_PATH: _path = uri.substring(mark, end); break; - case HOST_OR_PATH: _path = uri.substring(mark, end); break; - case HOST: if (end > mark) _host = uri.substring(mark, end); break; - case IPV6: throw new IllegalArgumentException("No closing ']' for ipv6 in " + uri); - case PORT: _port = TypeUtil.parseInt(uri, mark, end - mark, 10); break; - case ASTERISK: break; - case FRAGMENT: _fragment = uri.substring(mark, end); break; - case PARAM: _path = uri.substring(pathMark, end); _param = uri.substring(mark, end); break; - case PATH: - checkSegment(encodedSegment, false, uri, segment, end); + checkSegment(uri, segment, end, false); _path = uri.substring(pathMark, end); break; - case QUERY: _query = uri.substring(mark, end); break; } - if (!encoded) + if (!encoded && !dot) { if (_param == null) _decodedPath = _path; @@ -578,22 +586,16 @@ else if (c == '/') * * An ambiguous path segment is one that is perhaps technically legal, but is considered undesirable to handle * due to possible ambiguity. Examples include segments like '..;', '%2e', '%2e%2e' etc. - * @param segmentEncoded True if the segment is encoded - * @param param true if the segment has a parameter * @param uri The URI string * @param segment The inclusive starting index of the segment (excluding any '/') * @param end The exclusive end index of the segment */ - private void checkSegment(boolean segmentEncoded, boolean param, String uri, int segment, int end) + private void checkSegment(String uri, int segment, int end, boolean param) { - // if the segment is encoded or there is a segment parameter - if (!_ambiguousSegment && (segmentEncoded || param)) + if (!_ambiguousSegment) { - // Look for possible bad segments Boolean ambiguous = __ambiguousSegments.get(uri, segment, end - segment); - - // A segment is ambiguous if it is always ambiguous (TRUE) or ambiguous only with params (FALSE) and we have a param - _ambiguousSegment |= ambiguous == Boolean.TRUE || (param && ambiguous == Boolean.FALSE); + _ambiguousSegment |= ambiguous == Boolean.TRUE || (param && ambiguous == Boolean.FALSE); } } @@ -633,10 +635,19 @@ public String getPath() return _path; } + /** + * @return The decoded canonical path. + * @see URIUtil#canonicalPath(String) + */ public String getDecodedPath() { if (_decodedPath == null && _path != null) - _decodedPath = URIUtil.decodePath(_path); + { + String canonical = URIUtil.canonicalPath(_path); + if (canonical == null) + throw new BadMessageException("Bad URI"); + _decodedPath = URIUtil.decodePath(canonical); + } return _decodedPath; } diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java index 3a25920127b1..8a69662faa4f 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java @@ -20,17 +20,20 @@ import java.net.URLEncoder; import java.nio.charset.StandardCharsets; +import java.util.Arrays; import java.util.stream.Stream; import org.eclipse.jetty.util.MultiMap; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -239,6 +242,8 @@ public static Stream pathsWithAmbiguousSegments() "//host/%2e%2e/bar", "scheme://host/%2e%2e/bar", "scheme:/%2e%2e/bar", + "/foo/%2E.", + "/foo/.%2E", "/foo/%2e%2e", "/foo/%2e%2e?query", "/foo/%2e%2e#fragment", @@ -251,7 +256,7 @@ public static Stream pathsWithAmbiguousSegments() "%2e.", ".%2e", "%2e%2e" - ); + ); } @ParameterizedTest(name = "[{index}] {0}") @@ -260,4 +265,86 @@ public void testAmbiguousSegments(String uriWithBadSegment) { assertTrue(new HttpURI(uriWithBadSegment).hasAmbiguousSegment()); } + + public static Stream pathsWithNonAmbiguousSegments() + { + // @checkstyle-disable-check : AvoidEscapedUnicodeCharactersCheck + return Stream.of( + "/foo/../bar", + "/foo/..", + "/foo/..#frag", + "/foo/..?query", + "./foo", + "/./foo", + "http:./foo", + "//host/./foo" + ); + } + + @ParameterizedTest(name = "[{index}] {0}") + @MethodSource("pathsWithNonAmbiguousSegments") + public void testNonAmbiguousSegments(String uriWithBadSegment) + { + assertFalse(new HttpURI(uriWithBadSegment).hasAmbiguousSegment()); + } + + public static Stream uriData() + { + return Arrays.stream(new Object[][] + { + {"http://host/path/info", "/path/info", false}, + {"http://host/../path/info", null, false}, + {"http://host/path/../info", "/info", false}, + {"http://host/path/./info", "/path/info", false}, + {"//host/path/info", "/path/info", false}, + {"//host/../path/info", null, false}, + {"//host/path/../info", "/info", false}, + {"//host/path/./info", "/path/info", false}, + {"/path/info", "/path/info", false}, + {"/../path/info", null, false}, + {"/path/../info", "/info", false}, + {"/path/./info", "/path/info", false}, + {"../path/info", null, false}, + {"path/../info", "info", false}, + {"path/./info", "path/info", false}, + + {"/path/%2e/info", "/path/./info", true}, + {"/path/%2e%2e/info", "/path/../info", true}, + {"/path/%2e%2e;/info", "/path/../info", true}, + {"/path/.;/info", "/path/./info", true}, + {"/path/.;param/info", "/path/./info", true}, + {"/path/..;/info", "/path/../info", true}, + {"/path/..;param/info", "/path/../info", true}, + + {"%2e/info", "./info", true}, + {"%2e%2e/info", "../info", true}, + {"%2e%2e;/info", "../info", true}, + {".;/info", "./info", true}, + {".;param/info", "./info", true}, + {"..;/info", "../info", true}, + {"..;param/info", "../info", true}, + + {"/path/%2f/info", "/path///info", true}, + {"%2f/info", "//info", true}, + {"%2F/info", "//info", true}, + + {"/path/%XX/info", null, false}, + }).map(Arguments::of); + } + + @ParameterizedTest + @MethodSource("uriData") + public void testDecodedPath(String input, String decodedPath, boolean ambiguous) + { + try + { + HttpURI uri = new HttpURI(input); + assertThat(uri.getDecodedPath(), is(decodedPath)); + assertThat(uri.hasAmbiguousSegment(), is(ambiguous)); + } + catch (Exception e) + { + assertThat(decodedPath, nullValue()); + } + } } 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 b532d45263a1..4bda67ae2891 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 @@ -1842,7 +1842,7 @@ public void setMetaData(org.eclipse.jetty.http.MetaData.Request request) } else if (encoded.startsWith("/")) { - path = (encoded.length() == 1) ? "/" : URIUtil.canonicalPath(uri.getDecodedPath()); + path = (encoded.length() == 1) ? "/" : uri.getDecodedPath(); } 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 865ccf03a494..746d7f9d3389 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 @@ -500,7 +500,7 @@ public void testOKPathDotDotPath() throws Exception public void testBadPathDotDotPath() throws Exception { String response = connector.getResponse("GET /ooops/../../path HTTP/1.0\r\nHost: localhost:80\r\n\n"); - checkContains(response, 0, "HTTP/1.1 400 Bad URI"); + checkContains(response, 0, "HTTP/1.1 400 "); } @Test @@ -521,14 +521,14 @@ public void testBadPathEncodedDotDotPath() throws Exception public void testBadDotDotPath() throws Exception { String response = connector.getResponse("GET ../path HTTP/1.0\r\nHost: localhost:80\r\n\n"); - checkContains(response, 0, "HTTP/1.1 400 Bad URI"); + checkContains(response, 0, "HTTP/1.1 400 "); } @Test public void testBadSlashDotDotPath() throws Exception { String response = connector.getResponse("GET /../path HTTP/1.0\r\nHost: localhost:80\r\n\n"); - checkContains(response, 0, "HTTP/1.1 400 Bad URI"); + checkContains(response, 0, "HTTP/1.1 400 "); } @Test diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java b/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java index 371e15387fac..6818e5440928 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java @@ -782,11 +782,9 @@ public static String parentPath(String p) } /** - * Convert a decoded path to a canonical form. + * Convert an encoded path to a canonical form. *

* All instances of "." and ".." are factored out. - *

- *

* Null is returned if the path tries to .. above its root. *

* @@ -795,31 +793,35 @@ public static String parentPath(String p) */ public static String canonicalPath(String path) { + // See https://tools.ietf.org/html/rfc3986#section-5.2.4 + if (path == null || path.isEmpty()) return path; - boolean slash = true; int end = path.length(); int i = 0; + int dots = 0; - loop: - while (i < end) + loop: while (i < end) { char c = path.charAt(i); switch (c) { case '/': - slash = true; + dots = 0; break; case '.': - if (slash) + if (dots == 0) + { + dots = 1; break loop; - slash = false; + } + dots = -1; break; default: - slash = false; + dots = -1; } i++; @@ -831,7 +833,6 @@ public static String canonicalPath(String path) StringBuilder canonical = new StringBuilder(path.length()); canonical.append(path, 0, i); - int dots = 1; i++; while (i <= end) { @@ -839,14 +840,18 @@ public static String canonicalPath(String path) switch (c) { case '\0': + if (dots == 2) + { + if (canonical.length() < 2) + return null; + canonical.setLength(canonical.length() - 1); + canonical.setLength(canonical.lastIndexOf("/") + 1); + } + break; + case '/': switch (dots) { - case 0: - if (c != '\0') - canonical.append(c); - break; - case 1: break; @@ -858,36 +863,42 @@ public static String canonicalPath(String path) break; default: - while (dots-- > 0) - { - canonical.append('.'); - } - if (c != '\0') - canonical.append(c); + canonical.append(c); } - - slash = true; dots = 0; break; case '.': - if (dots > 0) - dots++; - else if (slash) - dots = 1; - else - canonical.append('.'); - slash = false; + switch (dots) + { + case 0: + dots = 1; + break; + case 1: + dots = 2; + break; + case 2: + canonical.append("..."); + dots = -1; + break; + default: + canonical.append('.'); + } break; default: - while (dots-- > 0) + switch (dots) { - canonical.append('.'); + case 1: + canonical.append('.'); + break; + case 2: + canonical.append(".."); + break; + default: } canonical.append(c); - dots = 0; - slash = false; + dots = -1; } i++; diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java index 396004ff7057..82f2771a42cb 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java @@ -34,6 +34,10 @@ public static Stream data() { String[][] canonical = { + // Examples from RFC + {"/a/b/c/./../../g", "/a/g"}, + {"mid/content=5/../6", "mid/6"}, + // Basic examples (no changes expected) {"/hello.html", "/hello.html"}, {"/css/main.css", "/css/main.css"}, @@ -56,8 +60,12 @@ public static Stream data() {"/aaa/./bbb/", "/aaa/bbb/"}, {"/aaa/./bbb", "/aaa/bbb"}, {"./bbb/", "bbb/"}, + {"./aaa", "aaa"}, + {"./aaa/", "aaa/"}, + {"/./aaa/", "/aaa/"}, {"./aaa/../bbb/", "bbb/"}, {"/foo/.", "/foo/"}, + {"/foo/./", "/foo/"}, {"./", ""}, {".", ""}, {".//", "/"}, @@ -121,6 +129,10 @@ public static Stream data() {"/foo/.;/bar", "/foo/.;/bar"}, {"/foo/..;/bar", "/foo/..;/bar"}, {"/foo/..;/..;/bar", "/foo/..;/..;/bar"}, + + // Trailing / is preserved + {"/foo/bar/..", "/foo/"}, + {"/foo/bar/../", "/foo/"}, }; ArrayList ret = new ArrayList<>(); @@ -135,6 +147,6 @@ public static Stream data() @MethodSource("data") public void testCanonicalPath(String input, String expectedResult) { - assertThat("Canonical", URIUtil.canonicalPath(input), is(expectedResult)); + assertThat(URIUtil.canonicalPath(input), is(expectedResult)); } } From b65a73927018076de9e9a5cc3c51aeac5ae60133 Mon Sep 17 00:00:00 2001 From: gregw Date: Sun, 14 Feb 2021 16:37:38 +0100 Subject: [PATCH 05/11] updates from review of RFC + Fixed test failures that were using %2f --- .../org/eclipse/jetty/servlet/AsyncContextTest.java | 12 ++++++------ .../org/eclipse/jetty/servlet/RequestURITest.java | 3 +++ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncContextTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncContextTest.java index 87c08dc3545d..d6e9b838c2a2 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncContextTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncContextTest.java @@ -229,7 +229,7 @@ public void testDispatchAsyncContext() throws Exception @Test public void testDispatchAsyncContextEncodedUrl() throws Exception { - String request = "GET /ctx/test/hello%2fthere?dispatch=true HTTP/1.1\r\n" + + String request = "GET /ctx/test/hello%20there?dispatch=true HTTP/1.1\r\n" + "Host: localhost\r\n" + "Content-Type: application/x-www-form-urlencoded\r\n" + "Connection: close\r\n" + @@ -253,16 +253,16 @@ public void testDispatchAsyncContextEncodedUrl() throws Exception // async run attributes assertThat("async run attr servlet path is original", responseBody, containsString("async:run:attr:servletPath:/test")); - assertThat("async run attr path info has correct encoding", responseBody, containsString("async:run:attr:pathInfo:/hello/there")); + assertThat("async run attr path info has correct encoding", responseBody, containsString("async:run:attr:pathInfo:/hello there")); assertThat("async run attr query string", responseBody, containsString("async:run:attr:queryString:dispatch=true")); assertThat("async run context path", responseBody, containsString("async:run:attr:contextPath:/ctx")); - assertThat("async run request uri has correct encoding", responseBody, containsString("async:run:attr:requestURI:/ctx/test/hello%2fthere")); + assertThat("async run request uri has correct encoding", responseBody, containsString("async:run:attr:requestURI:/ctx/test/hello%20there")); } @Test public void testDispatchAsyncContextSelfEncodedUrl() throws Exception { - String request = "GET /ctx/self/hello%2fthere?dispatch=true HTTP/1.1\r\n" + + String request = "GET /ctx/self/hello%20there?dispatch=true HTTP/1.1\r\n" + "Host: localhost\r\n" + "Content-Type: application/x-www-form-urlencoded\r\n" + "Connection: close\r\n" + @@ -272,8 +272,8 @@ public void testDispatchAsyncContextSelfEncodedUrl() throws Exception String responseBody = response.getContent(); - assertThat("servlet request uri initial", responseBody, containsString("doGet.REQUEST.requestURI:/ctx/self/hello%2fthere")); - assertThat("servlet request uri async", responseBody, containsString("doGet.ASYNC.requestURI:/ctx/self/hello%2fthere")); + assertThat("servlet request uri initial", responseBody, containsString("doGet.REQUEST.requestURI:/ctx/self/hello%20there")); + assertThat("servlet request uri async", responseBody, containsString("doGet.ASYNC.requestURI:/ctx/self/hello%20there")); } @Test diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java index de3814accf20..7de53075e785 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java @@ -34,6 +34,8 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.eclipse.jetty.http.HttpCompliance; +import org.eclipse.jetty.server.HttpConnectionFactory; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.hamcrest.Matchers; @@ -112,6 +114,7 @@ public static void startServer() throws Exception ServerConnector connector = new ServerConnector(server); connector.setPort(0); server.addConnector(connector); + connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC7230_LEGACY); // Allow ambiguous segments ServletContextHandler context = new ServletContextHandler(); context.setContextPath("/"); From 7a649f34ecfe8f988af46a1a02c6090dae42f7d2 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Sun, 14 Feb 2021 20:07:08 +0100 Subject: [PATCH 06/11] Fix test Signed-off-by: Greg Wilkins --- .../java/org/eclipse/jetty/servlet/DefaultServletTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java index b2c063503f84..1661109d9a5c 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java @@ -47,12 +47,14 @@ import javax.servlet.http.HttpServletResponse; import org.eclipse.jetty.http.DateGenerator; +import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.http.HttpContent; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.server.HttpConfiguration; +import org.eclipse.jetty.server.HttpConnectionFactory; import org.eclipse.jetty.server.LocalConnector; import org.eclipse.jetty.server.ResourceContentFactory; import org.eclipse.jetty.server.ResourceService; @@ -116,6 +118,7 @@ public void init() throws Exception connector = new LocalConnector(server); connector.getConnectionFactory(HttpConfiguration.ConnectionFactory.class).getHttpConfiguration().setSendServerVersion(false); + connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC7230_LEGACY); // allow ambiguous path segments File extraJarResources = MavenTestingUtils.getTestResourceFile(ODD_JAR); URL[] urls = new URL[]{extraJarResources.toURI().toURL()}; From 89869fad2b428d870cc682892dd7f4abdf870e0d Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 15 Feb 2021 09:34:07 +0100 Subject: [PATCH 07/11] updates from review of RFC + simplified tests --- .../org/eclipse/jetty/http/HttpURITest.java | 104 +++++------------- 1 file changed, 25 insertions(+), 79 deletions(-) diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java index 8a69662faa4f..6da7dbfafd28 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java @@ -33,7 +33,6 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -219,103 +218,46 @@ public void testSchemeAndOrAuthority() throws Exception assertEquals("http:/path/info", uri.toString()); } - @Test - public void testBasicAuthCredentials() throws Exception - { - HttpURI uri = new HttpURI("http://user:password@example.com:8888/blah"); - assertEquals("http://user:password@example.com:8888/blah", uri.toString()); - assertEquals(uri.getAuthority(), "example.com:8888"); - assertEquals(uri.getUser(), "user:password"); - } - - public static Stream pathsWithAmbiguousSegments() - { - // @checkstyle-disable-check : AvoidEscapedUnicodeCharactersCheck - return Stream.of( - "/foo/%2e%2e/bar", - "/foo/%2e%2e;/bar", - "/foo/%2e%2e;param/bar", - "/foo/..;/bar", - "/foo/..;param/bar", - "foo/%2e%2e/bar", - "%2e%2e/bar", - "//host/%2e%2e/bar", - "scheme://host/%2e%2e/bar", - "scheme:/%2e%2e/bar", - "/foo/%2E.", - "/foo/.%2E", - "/foo/%2e%2e", - "/foo/%2e%2e?query", - "/foo/%2e%2e#fragment", - "foo/%2e.", - ".%2e", - "//host/%2e%2e", - "scheme://host/.%2e", - "scheme:/%2e%2e", - "%2e", - "%2e.", - ".%2e", - "%2e%2e" - ); - } - - @ParameterizedTest(name = "[{index}] {0}") - @MethodSource("pathsWithAmbiguousSegments") - public void testAmbiguousSegments(String uriWithBadSegment) - { - assertTrue(new HttpURI(uriWithBadSegment).hasAmbiguousSegment()); - } - - public static Stream pathsWithNonAmbiguousSegments() - { - // @checkstyle-disable-check : AvoidEscapedUnicodeCharactersCheck - return Stream.of( - "/foo/../bar", - "/foo/..", - "/foo/..#frag", - "/foo/..?query", - "./foo", - "/./foo", - "http:./foo", - "//host/./foo" - ); - } - - @ParameterizedTest(name = "[{index}] {0}") - @MethodSource("pathsWithNonAmbiguousSegments") - public void testNonAmbiguousSegments(String uriWithBadSegment) - { - assertFalse(new HttpURI(uriWithBadSegment).hasAmbiguousSegment()); - } - - public static Stream uriData() + public static Stream decodePathTests() { return Arrays.stream(new Object[][] { + // Simple path example {"http://host/path/info", "/path/info", false}, + {"//host/path/info", "/path/info", false}, + {"/path/info", "/path/info", false}, + + // legal non ambiguous relative paths {"http://host/../path/info", null, false}, {"http://host/path/../info", "/info", false}, {"http://host/path/./info", "/path/info", false}, - {"//host/path/info", "/path/info", false}, - {"//host/../path/info", null, false}, {"//host/path/../info", "/info", false}, {"//host/path/./info", "/path/info", false}, - {"/path/info", "/path/info", false}, - {"/../path/info", null, false}, {"/path/../info", "/info", false}, {"/path/./info", "/path/info", false}, - {"../path/info", null, false}, {"path/../info", "info", false}, {"path/./info", "path/info", false}, + // illegal paths + {"//host/../path/info", null, false}, + {"/../path/info", null, false}, + {"../path/info", null, false}, + {"/path/%XX/info", null, false}, + {"/path/%2/F/info", null, false}, + + // ambiguous dot encodings or parameter inclusions + {"scheme://host/path/%2e/info", "/path/./info", true}, + {"scheme:/path/%2e/info", "/path/./info", true}, {"/path/%2e/info", "/path/./info", true}, + {"path/%2e/info/", "path/./info/", true}, {"/path/%2e%2e/info", "/path/../info", true}, {"/path/%2e%2e;/info", "/path/../info", true}, + {"/path/%2e%2e;param/info", "/path/../info", true}, + {"/path/%2e%2e;param;other/info;other", "/path/../info", true}, {"/path/.;/info", "/path/./info", true}, {"/path/.;param/info", "/path/./info", true}, {"/path/..;/info", "/path/../info", true}, {"/path/..;param/info", "/path/../info", true}, - {"%2e/info", "./info", true}, {"%2e%2e/info", "../info", true}, {"%2e%2e;/info", "../info", true}, @@ -323,17 +265,21 @@ public static Stream uriData() {".;param/info", "./info", true}, {"..;/info", "../info", true}, {"..;param/info", "../info", true}, + {"%2e", ".", true}, + {"%2e.", "..", true}, + {".%2e", "..", true}, + {"%2e%2e", "..", true}, + // ambiguous segment separators {"/path/%2f/info", "/path///info", true}, {"%2f/info", "//info", true}, {"%2F/info", "//info", true}, - {"/path/%XX/info", null, false}, }).map(Arguments::of); } @ParameterizedTest - @MethodSource("uriData") + @MethodSource("decodePathTests") public void testDecodedPath(String input, String decodedPath, boolean ambiguous) { try From db2ee4105672c28d42cc22ca7f2f5f7018620a7e Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 16 Feb 2021 00:19:33 +0100 Subject: [PATCH 08/11] Updates from review Signed-off-by: Greg Wilkins --- .../eclipse/jetty/http/HttpCompliance.java | 9 +- .../java/org/eclipse/jetty/http/HttpURI.java | 99 ++++++++++--------- .../jetty/server/HttpConnectionTest.java | 14 --- .../org/eclipse/jetty/server/RequestTest.java | 3 - 4 files changed, 60 insertions(+), 65 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java index 814344a81074..8d6334cc307d 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java @@ -56,11 +56,12 @@ public enum HttpCompliance // TODO in Jetty-10 convert this enum to a class so t LEGACY(sectionsBySpec("0,METHOD_CASE_SENSITIVE")), /** - * The legacy RFC2616 support, which incorrectly excludes + * The legacy RFC2616 support, which excludes * {@link HttpComplianceSection#METHOD_CASE_SENSITIVE}, * {@link HttpComplianceSection#FIELD_COLON}, * {@link HttpComplianceSection#TRANSFER_ENCODING_WITH_CONTENT_LENGTH}, - * {@link HttpComplianceSection#MULTIPLE_CONTENT_LENGTHS}, + * {@link HttpComplianceSection#MULTIPLE_CONTENT_LENGTHS} and + * {@link HttpComplianceSection#NO_AMBIGUOUS_PATH_SEGMENTS}. */ RFC2616_LEGACY(sectionsBySpec("RFC2616,-FIELD_COLON,-METHOD_CASE_SENSITIVE,-TRANSFER_ENCODING_WITH_CONTENT_LENGTH,-MULTIPLE_CONTENT_LENGTHS,-NO_AMBIGUOUS_PATH_SEGMENTS")), @@ -70,7 +71,9 @@ public enum HttpCompliance // TODO in Jetty-10 convert this enum to a class so t RFC2616(sectionsBySpec("RFC2616")), /** - * Jetty's current RFC7230 support, which incorrectly excludes {@link HttpComplianceSection#METHOD_CASE_SENSITIVE} + * Jetty's current RFC7230 support, which excludes + * {@link HttpComplianceSection#METHOD_CASE_SENSITIVE} and + * {@link HttpComplianceSection#NO_AMBIGUOUS_PATH_SEGMENTS}. */ RFC7230_LEGACY(sectionsBySpec("RFC7230,-METHOD_CASE_SENSITIVE,-NO_AMBIGUOUS_PATH_SEGMENTS")), diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java index 1f6b2d7d29e9..37fffea1b5d5 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java @@ -99,11 +99,9 @@ private enum State private String _param; private String _query; private String _fragment; - - String _uri; - String _decodedPath; - - boolean _ambiguousSegment; + private String _uri; + private String _decodedPath; + private boolean _ambiguousSegment; /** * Construct a normalized URI. @@ -136,6 +134,7 @@ public HttpURI(String scheme, String host, int port, String path, String param, _scheme = scheme; _host = host; _port = port; + parse(State.PATH, path, 0, path.length()); _path = path; _param = param; _query = query; @@ -146,6 +145,7 @@ public HttpURI(HttpURI uri) { this(uri._scheme, uri._host, uri._port, uri._path, uri._param, uri._query, uri._fragment); _uri = uri._uri; + _ambiguousSegment = uri._ambiguousSegment; } public HttpURI(String uri) @@ -157,40 +157,44 @@ public HttpURI(String uri) public HttpURI(URI uri) { _uri = null; - _scheme = uri.getScheme(); _host = uri.getHost(); if (_host == null && uri.getRawSchemeSpecificPart().startsWith("//")) _host = ""; _port = uri.getPort(); _user = uri.getUserInfo(); - _path = uri.getRawPath(); - - _decodedPath = uri.getPath(); - if (_decodedPath != null) - { - int p = _decodedPath.lastIndexOf(';'); - if (p >= 0) - _param = _decodedPath.substring(p + 1); - } + String path = uri.getRawPath(); + if (path != null) + parse(State.PATH, path, 0, path.length()); _query = uri.getRawQuery(); _fragment = uri.getFragment(); - - _decodedPath = null; } public HttpURI(String scheme, String host, int port, String pathQuery) { _uri = null; - _scheme = scheme; _host = host; _port = port; - if (pathQuery != null) parse(State.PATH, pathQuery, 0, pathQuery.length()); } + public void clear() + { + _uri = null; + _scheme = null; + _user = null; + _host = null; + _port = -1; + _path = null; + _param = null; + _query = null; + _fragment = null; + _decodedPath = null; + _ambiguousSegment = false; + } + public void parse(String uri) { clear(); @@ -333,6 +337,8 @@ private void parse(State state, final String uri, final int offset, final int en _path = uri.substring(mark, i); state = State.FRAGMENT; break; + default: + break; } continue; } @@ -388,10 +394,11 @@ private void parse(State state, final String uri, final int offset, final int en _user = uri.substring(mark, i); mark = i + 1; break; - case '[': state = State.IPV6; break; + default: + break; } continue; } @@ -415,8 +422,9 @@ private void parse(State state, final String uri, final int offset, final int en state = State.PATH; } break; + default: + break; } - continue; } case PORT: @@ -510,6 +518,8 @@ else if (c == '/') // multiple parameters mark = i + 1; break; + default: + break; } continue; } @@ -532,6 +542,8 @@ else if (c == '/') _fragment = uri.substring(mark, end); i = end; } + default: + break; } } @@ -570,6 +582,8 @@ else if (c == '/') case QUERY: _query = uri.substring(mark, end); break; + default: + break; } if (!encoded && !dot) @@ -579,8 +593,21 @@ else if (c == '/') else _decodedPath = _path.substring(0, _path.length() - _param.length() - 1); } + else if (_path != null) + { + String canonical = URIUtil.canonicalPath(_path); + if (canonical == null) + throw new BadMessageException("Bad URI"); + _decodedPath = URIUtil.decodePath(canonical); + } } + private void decodePath() + { + + } + + /** * Check for ambiguous path segments. * @@ -641,13 +668,6 @@ public String getPath() */ public String getDecodedPath() { - if (_decodedPath == null && _path != null) - { - String canonical = URIUtil.canonicalPath(_path); - if (canonical == null) - throw new BadMessageException("Bad URI"); - _decodedPath = URIUtil.decodePath(canonical); - } return _decodedPath; } @@ -703,23 +723,6 @@ public void decodeQueryTo(MultiMap parameters, Charset encoding) throws UrlEncoded.decodeTo(_query, parameters, encoding); } - public void clear() - { - _uri = null; - - _scheme = null; - _host = null; - _port = -1; - _path = null; - _param = null; - _query = null; - _fragment = null; - - _decodedPath = null; - - _ambiguousSegment = false; - } - public boolean isAbsolute() { return _scheme != null && !_scheme.isEmpty(); @@ -773,6 +776,12 @@ public boolean equals(Object o) return toString().equals(o.toString()); } + @Override + public int hashCode() + { + return toString().hashCode(); + } + public void setScheme(String scheme) { _scheme = scheme; 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 746d7f9d3389..72197e6867a3 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 @@ -503,20 +503,6 @@ public void testBadPathDotDotPath() throws Exception checkContains(response, 0, "HTTP/1.1 400 "); } - @Test - public void testOKPathEncodedDotDotPath() throws Exception - { - String response = connector.getResponse("GET /ooops/%2e%2e/path HTTP/1.0\r\nHost: localhost:80\r\n\n"); - checkContains(response, 0, "HTTP/1.1 400 "); - } - - @Test - public void testBadPathEncodedDotDotPath() throws Exception - { - String response = connector.getResponse("GET /ooops/%2e%2e/%2e%2e/path HTTP/1.0\r\nHost: localhost:80\r\n\n"); - checkContains(response, 0, "HTTP/1.1 400 "); - } - @Test public void testBadDotDotPath() throws Exception { 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 ba7ab2976c40..8ac0e709e2af 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 @@ -1856,9 +1856,6 @@ public void testAmbiguousPaths() throws Exception _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC2616_LEGACY); assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); - - - } private static long getFileCount(Path path) From 58377721b19433c2007dbd4351da5e99f91b02fc Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 16 Feb 2021 00:30:59 +0100 Subject: [PATCH 09/11] Updates from review Signed-off-by: Greg Wilkins --- .../java/org/eclipse/jetty/http/HttpURI.java | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java index 37fffea1b5d5..4f7c4d26f619 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java @@ -135,7 +135,6 @@ public HttpURI(String scheme, String host, int port, String path, String param, _host = host; _port = port; parse(State.PATH, path, 0, path.length()); - _path = path; _param = param; _query = query; _fragment = fragment; @@ -143,8 +142,16 @@ public HttpURI(String scheme, String host, int port, String path, String param, public HttpURI(HttpURI uri) { - this(uri._scheme, uri._host, uri._port, uri._path, uri._param, uri._query, uri._fragment); + _scheme = uri._scheme; + _user = uri._user; + _host = uri._host; + _port = uri._port; + _path = uri._path; + _param = uri._param; + _query = uri._query; + _fragment = uri._fragment; _uri = uri._uri; + _decodedPath = uri._decodedPath; _ambiguousSegment = uri._ambiguousSegment; } @@ -805,8 +812,9 @@ public void setAuthority(String host, int port) public void setPath(String path) { _uri = null; - _path = path; - _decodedPath = null; + _path = null; + if (path != null) + parse(State.PATH, path, 0, path.length()); } public void setPathQuery(String path) From 1b2414b4829a8bbd073b7281a5eb343d01aa6f63 Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 16 Feb 2021 14:35:24 +0100 Subject: [PATCH 10/11] updates from review updates from review --- .../java/org/eclipse/jetty/http/HttpURI.java | 31 ++++++---- .../org/eclipse/jetty/http/HttpURITest.java | 59 +++++++++++++++++++ 2 files changed, 77 insertions(+), 13 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java index 4f7c4d26f619..25b2e2c082cd 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java @@ -23,6 +23,7 @@ import java.net.URISyntaxException; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; +import java.util.Objects; import org.eclipse.jetty.util.ArrayTrie; import org.eclipse.jetty.util.MultiMap; @@ -134,10 +135,14 @@ public HttpURI(String scheme, String host, int port, String path, String param, _scheme = scheme; _host = host; _port = port; - parse(State.PATH, path, 0, path.length()); - _param = param; - _query = query; - _fragment = fragment; + if (path != null) + parse(State.PATH, path, 0, path.length()); + if (param != null) + _param = param; + if (query != null) + _query = query; + if (fragment != null) + _fragment = fragment; } public HttpURI(HttpURI uri) @@ -548,6 +553,7 @@ else if (c == '/') { _fragment = uri.substring(mark, end); i = end; + break; } default: break; @@ -609,12 +615,6 @@ else if (_path != null) } } - private void decodePath() - { - - } - - /** * Check for ambiguous path segments. * @@ -685,10 +685,14 @@ public String getParam() public void setParam(String param) { - _param = param; - if (_path != null && !_path.contains(_param)) + if (!Objects.equals(_param,param)) { - _path += ";" + _param; + if (_param != null && _path.endsWith(";" + _param)) + _path = _path.substring(0, _path.length() - 1 - _param.length()); + _param = param; + if (_param != null) + _path = (_path == null ? "" : _path) + ";" + _param; + _uri = null; } } @@ -824,6 +828,7 @@ public void setPathQuery(String path) _decodedPath = null; _param = null; _fragment = null; + _query = null; if (path != null) parse(State.PATH, path, 0, path.length()); } diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java index 6da7dbfafd28..24b2a37e48a1 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java @@ -218,6 +218,65 @@ public void testSchemeAndOrAuthority() throws Exception assertEquals("http:/path/info", uri.toString()); } + @Test + public void testSetters() throws Exception + { + HttpURI uri = new HttpURI(); + assertEquals("", uri.toString()); + + uri = new HttpURI(null, null, 0, null, null, null, null); + assertEquals("", uri.toString()); + + uri.setPath("/path/info"); + assertEquals("/path/info", uri.toString()); + + uri.setAuthority("host", 8080); + assertEquals("//host:8080/path/info", uri.toString()); + + uri.setParam("param"); + assertEquals("//host:8080/path/info;param", uri.toString()); + + uri.setQuery("a=b"); + assertEquals("//host:8080/path/info;param?a=b", uri.toString()); + + uri.setScheme("http"); + assertEquals("http://host:8080/path/info;param?a=b", uri.toString()); + + uri.setPathQuery("/other;xxx/path;ppp?query"); + assertEquals("http://host:8080/other;xxx/path;ppp?query", uri.toString()); + + assertThat(uri.getScheme(), is("http")); + assertThat(uri.getAuthority(), is("host:8080")); + assertThat(uri.getHost(), is("host")); + assertThat(uri.getPort(), is(8080)); + assertThat(uri.getPath(), is("/other;xxx/path;ppp")); + assertThat(uri.getDecodedPath(), is("/other/path")); + assertThat(uri.getParam(), is("ppp")); + assertThat(uri.getQuery(), is("query")); + assertThat(uri.getPathQuery(), is("/other;xxx/path;ppp?query")); + + uri.setPathQuery(null); + assertEquals("http://host:8080", uri.toString()); + + uri.setPathQuery("/other;xxx/path;ppp?query"); + assertEquals("http://host:8080/other;xxx/path;ppp?query", uri.toString()); + + uri.setScheme(null); + assertEquals("//host:8080/other;xxx/path;ppp?query", uri.toString()); + + uri.setAuthority(null,-1); + assertEquals("/other;xxx/path;ppp?query", uri.toString()); + + uri.setParam(null); + assertEquals("/other;xxx/path?query", uri.toString()); + + uri.setQuery(null); + assertEquals("/other;xxx/path", uri.toString()); + + uri.setPath(null); + assertEquals("", uri.toString()); + } + public static Stream decodePathTests() { return Arrays.stream(new Object[][] From 4a670fc935b4fbf0b40d0161cd584c91a9344d75 Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 16 Feb 2021 14:39:12 +0100 Subject: [PATCH 11/11] updates from review updates from review --- jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java index 25b2e2c082cd..74d5f2471d13 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java @@ -685,7 +685,7 @@ public String getParam() public void setParam(String param) { - if (!Objects.equals(_param,param)) + if (!Objects.equals(_param, param)) { if (_param != null && _path.endsWith(";" + _param)) _path = _path.substring(0, _path.length() - 1 - _param.length());