From 814024fbd5ed842e286449f32218ee4e8aef8dc6 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 24 Sep 2024 11:45:26 -0500 Subject: [PATCH 1/4] HttpURI.Mutable path changes should clear out any existing path violations before parsing the new path. Fixes: #11298 --- .../java/org/eclipse/jetty/http/HttpURI.java | 19 +++++ .../rewrite/handler/CompactPathRuleTest.java | 78 ++++++++++++++++--- 2 files changed, 85 insertions(+), 12 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java index 6db99245a561..72fe78778a48 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java @@ -954,6 +954,21 @@ public Collection getViolations() return _violations == null ? Collections.emptySet() : Collections.unmodifiableCollection(_violations); } + /** + * Clear, from the violations list, any path related violations. + */ + private void clearPathViolations() + { + if (_violations == null) + return; + _violations.removeIf((violation) -> + (violation == Violation.AMBIGUOUS_PATH_PARAMETER) || + (violation == Violation.AMBIGUOUS_PATH_SEGMENT) || + (violation == Violation.AMBIGUOUS_PATH_SEPARATOR) || + (violation == Violation.AMBIGUOUS_PATH_ENCODING) || + (violation == Violation.AMBIGUOUS_EMPTY_SEGMENT)); + } + public Mutable normalize() { HttpScheme scheme = _scheme == null ? null : HttpScheme.CACHE.get(_scheme); @@ -994,6 +1009,8 @@ public Mutable path(String path) throw new IllegalArgumentException("Relative path with authority"); if (!URIUtil.isPathValid(path)) throw new IllegalArgumentException("Path not correctly encoded: " + path); + // since we are resetting the path, lets clear out the path specific violations. + clearPathViolations(); _uri = null; _path = null; _canonicalPath = null; @@ -1016,6 +1033,8 @@ public Mutable pathQuery(String pathQuery) { if (hasAuthority() && !isPathValidForAuthority(pathQuery)) throw new IllegalArgumentException("Relative path with authority"); + // since we are resetting the path, lets clear out the path specific violations. + clearPathViolations(); _uri = null; _path = null; _canonicalPath = null; diff --git a/jetty-core/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/CompactPathRuleTest.java b/jetty-core/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/CompactPathRuleTest.java index 9c707c753c54..ec19f3e139a5 100644 --- a/jetty-core/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/CompactPathRuleTest.java +++ b/jetty-core/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/CompactPathRuleTest.java @@ -13,13 +13,18 @@ package org.eclipse.jetty.rewrite.handler; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.nio.ByteBuffer; +import java.util.Objects; +import java.util.Properties; import java.util.stream.Stream; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.http.UriCompliance; -import org.eclipse.jetty.io.Content; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.Response; @@ -36,14 +41,20 @@ public static Stream scenarios() { return Stream.of( // shouldn't change anything - Arguments.of("/foo", null, "/foo", null, "/foo"), - Arguments.of("/", null, "/", null, "/"), + Arguments.of("/foo", null, "/foo", "", "/foo"), + Arguments.of("/", null, "/", "", "/"), // simple compact path - Arguments.of("////foo", null, "/foo", null, "/foo"), + Arguments.of("////foo", null, "/foo", "", "/foo"), // with simple query Arguments.of("//foo//bar", "a=b", "/foo/bar", "a=b", "/foo/bar?a=b"), // with query that has double slashes (should preserve slashes in query) - Arguments.of("//foo//bar", "a=b//c", "/foo/bar", "a=b//c", "/foo/bar?a=b//c") + Arguments.of("//foo//bar", "a=b//c", "/foo/bar", "a=b//c", "/foo/bar?a=b//c"), + // with ambiguous path parameter + Arguments.of("//foo/..;/bar", "a=b//c", "/bar", "a=b//c", "/bar?a=b//c"), + // with ambiguous path separator (not changed) + Arguments.of("//foo/b%2far", "a=b//c", "/foo/b%2Far", "a=b//c", "/foo/b%2Far?a=b//c"), + // with ambiguous path encoding (not changed) + Arguments.of("//foo/%2562ar", "a=b//c", "/foo/%2562ar", "a=b//c", "/foo/%2562ar?a=b//c") ); } @@ -59,24 +70,67 @@ public void testCompactPathRule(String inputPath, String inputQuery, String expe @Override public boolean handle(Request request, Response response, Callback callback) { - Content.Sink.write(response, true, request.getHttpURI().getPathQuery(), callback); + Properties props = new Properties(); + HttpURI httpURI = request.getHttpURI(); + props.setProperty("uri.path", of(httpURI.getPath())); + props.setProperty("uri.query", of(httpURI.getQuery())); + props.setProperty("uri.pathQuery", of(httpURI.getPathQuery())); + props.setProperty("uri.hasViolations", of(httpURI.hasViolations())); + props.setProperty("uri.isAmbiguous", of(httpURI.isAmbiguous())); + props.setProperty("uri.hasAmbiguousEmptySegment", of(httpURI.hasAmbiguousEmptySegment())); + props.setProperty("uri.hasAmbiguousEncoding", of(httpURI.hasAmbiguousEncoding())); + props.setProperty("uri.hasAmbiguousParameter", of(httpURI.hasAmbiguousParameter())); + props.setProperty("uri.hasAmbiguousSeparator", of(httpURI.hasAmbiguousSeparator())); + props.setProperty("uri.hasAmbiguousSegment", of(httpURI.hasAmbiguousSegment())); + try (ByteArrayOutputStream out = new ByteArrayOutputStream()) + { + props.store(out, "HttpURI State"); + response.write(true, ByteBuffer.wrap(out.toByteArray()), callback); + } + catch (IOException e) + { + callback.failed(e); + } return true; } + + private String of(Object obj) + { + if (obj == null) + return ""; + if (obj instanceof Boolean) + return Boolean.toString((Boolean)obj); + return Objects.toString(obj); + } }); String request = """ GET %s HTTP/1.1 Host: localhost - + """.formatted(HttpURI.build().path(inputPath).query(inputQuery)); HttpTester.Response response = HttpTester.parseResponse(_connector.getResponse(request)); - System.err.println(response.getReason()); assertEquals(HttpStatus.OK_200, response.getStatus()); - HttpURI.Mutable result = HttpURI.build(response.getContent()); - assertEquals(expectedPath, result.getPath()); - assertEquals(expectedQuery, result.getQuery()); - assertEquals(expectedPathQuery, result.getPathQuery()); + Properties props = new Properties(); + try (ByteArrayInputStream in = new ByteArrayInputStream(response.getContentBytes())) + { + props.load(in); + assertEquals(expectedPath, props.getProperty("uri.path")); + assertEquals(expectedQuery, props.getProperty("uri.query")); + assertEquals(expectedPathQuery, props.getProperty("uri.pathQuery")); + + boolean ambiguousPathSep = inputPath.contains("%2f"); + boolean ambiguousPathEncoding = inputPath.contains("%25"); + + assertEquals(Boolean.toString(ambiguousPathSep || ambiguousPathEncoding), props.getProperty("uri.isAmbiguous")); + assertEquals(Boolean.toString(ambiguousPathSep || ambiguousPathEncoding), props.getProperty("uri.hasViolations")); + assertEquals("false", props.getProperty("uri.hasAmbiguousEmptySegment")); + assertEquals(Boolean.toString(ambiguousPathEncoding), props.getProperty("uri.hasAmbiguousEncoding")); + assertEquals("false", props.getProperty("uri.hasAmbiguousParameter")); + assertEquals(Boolean.toString(ambiguousPathSep), props.getProperty("uri.hasAmbiguousSeparator")); + assertEquals("false", props.getProperty("uri.hasAmbiguousSegment")); + } } } From 1681812fe22445aab298f3822baed76162a86e9b Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Thu, 26 Sep 2024 06:48:44 -0500 Subject: [PATCH 2/4] Changes from review --- .../java/org/eclipse/jetty/http/HttpURI.java | 21 ++++--------------- .../org/eclipse/jetty/http/UriCompliance.java | 15 +++++++++++++ 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java index 72fe78778a48..3b4a6b573c54 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java @@ -954,21 +954,6 @@ public Collection getViolations() return _violations == null ? Collections.emptySet() : Collections.unmodifiableCollection(_violations); } - /** - * Clear, from the violations list, any path related violations. - */ - private void clearPathViolations() - { - if (_violations == null) - return; - _violations.removeIf((violation) -> - (violation == Violation.AMBIGUOUS_PATH_PARAMETER) || - (violation == Violation.AMBIGUOUS_PATH_SEGMENT) || - (violation == Violation.AMBIGUOUS_PATH_SEPARATOR) || - (violation == Violation.AMBIGUOUS_PATH_ENCODING) || - (violation == Violation.AMBIGUOUS_EMPTY_SEGMENT)); - } - public Mutable normalize() { HttpScheme scheme = _scheme == null ? null : HttpScheme.CACHE.get(_scheme); @@ -1010,7 +995,8 @@ public Mutable path(String path) if (!URIUtil.isPathValid(path)) throw new IllegalArgumentException("Path not correctly encoded: " + path); // since we are resetting the path, lets clear out the path specific violations. - clearPathViolations(); + if (_violations != null) + _violations.removeIf(UriCompliance::isPathViolation); _uri = null; _path = null; _canonicalPath = null; @@ -1034,7 +1020,8 @@ public Mutable pathQuery(String pathQuery) if (hasAuthority() && !isPathValidForAuthority(pathQuery)) throw new IllegalArgumentException("Relative path with authority"); // since we are resetting the path, lets clear out the path specific violations. - clearPathViolations(); + if (_violations != null) + _violations.removeIf(UriCompliance::isPathViolation); _uri = null; _path = null; _canonicalPath = null; diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java index 183a283c9e0c..405cb0e356d0 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java @@ -352,6 +352,21 @@ public UriCompliance without(String name, Violation... violations) return new UriCompliance(name, remainder); } + /** + * Test if violation is referencing a URI path violation. + * + * @param violation the violation to test. + * @return true if violation is a path violation. + */ + public static boolean isPathViolation(UriCompliance.Violation violation) + { + return (violation == Violation.AMBIGUOUS_PATH_PARAMETER) || + (violation == Violation.AMBIGUOUS_PATH_SEGMENT) || + (violation == Violation.AMBIGUOUS_PATH_SEPARATOR) || + (violation == Violation.AMBIGUOUS_PATH_ENCODING) || + (violation == Violation.AMBIGUOUS_EMPTY_SEGMENT); + } + @Override public String toString() { From 5836808eaad50d59e6e831878ed9d237cd23634c Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Thu, 26 Sep 2024 16:57:51 -0500 Subject: [PATCH 3/4] Using EnumSet --- .../org/eclipse/jetty/http/UriCompliance.java | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java index 405cb0e356d0..3e2b2d1fc00c 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java @@ -145,6 +145,10 @@ public String getDescription() } public static final Set NO_VIOLATION = Collections.unmodifiableSet(EnumSet.noneOf(Violation.class)); + + /** + * Set of violations that can trigger a HttpURI.isAmbiguous violation. + */ public static final Set AMBIGUOUS_VIOLATIONS = Collections.unmodifiableSet(EnumSet.of( Violation.AMBIGUOUS_EMPTY_SEGMENT, Violation.AMBIGUOUS_PATH_ENCODING, @@ -152,6 +156,16 @@ public String getDescription() Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_SEPARATOR)); + /** + * List of Violations that apply only to the HttpURI.path section. + */ + private static final Set PATH_VIOLATIONS = Collections.unmodifiableSet(EnumSet.of( + Violation.AMBIGUOUS_EMPTY_SEGMENT, + Violation.AMBIGUOUS_PATH_ENCODING, + Violation.AMBIGUOUS_PATH_PARAMETER, + Violation.AMBIGUOUS_PATH_SEGMENT, + Violation.AMBIGUOUS_PATH_SEPARATOR)); + /** * Compliance mode that exactly follows RFC3986, * excluding all URI Violations. @@ -353,18 +367,14 @@ public UriCompliance without(String name, Violation... violations) } /** - * Test if violation is referencing a URI path violation. + * Test if violation is referencing a HttpURI.path violation. * * @param violation the violation to test. * @return true if violation is a path violation. */ public static boolean isPathViolation(UriCompliance.Violation violation) { - return (violation == Violation.AMBIGUOUS_PATH_PARAMETER) || - (violation == Violation.AMBIGUOUS_PATH_SEGMENT) || - (violation == Violation.AMBIGUOUS_PATH_SEPARATOR) || - (violation == Violation.AMBIGUOUS_PATH_ENCODING) || - (violation == Violation.AMBIGUOUS_EMPTY_SEGMENT); + return PATH_VIOLATIONS.contains(violation); } @Override From 610ef367f033947c3f8ca1848e7b4ed7025ee2cd Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Fri, 27 Sep 2024 14:16:47 -0500 Subject: [PATCH 4/4] Add missing PATH violations to enumset --- .../src/main/java/org/eclipse/jetty/http/UriCompliance.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java index 3e2b2d1fc00c..fb8dad6cd127 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java @@ -164,7 +164,9 @@ public String getDescription() Violation.AMBIGUOUS_PATH_ENCODING, Violation.AMBIGUOUS_PATH_PARAMETER, Violation.AMBIGUOUS_PATH_SEGMENT, - Violation.AMBIGUOUS_PATH_SEPARATOR)); + Violation.AMBIGUOUS_PATH_SEPARATOR, + Violation.SUSPICIOUS_PATH_CHARACTERS, + Violation.ILLEGAL_PATH_CHARACTERS)); /** * Compliance mode that exactly follows RFC3986,