From e376b1a951bc2a552f60a364bac69d2323689cce Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 30 Sep 2024 07:09:21 -0500 Subject: [PATCH] HttpURI.Mutable path changes should clear out any existing path violations before parsing the new path. (#12306) * 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 | 6 ++ .../org/eclipse/jetty/http/UriCompliance.java | 27 +++++++ .../rewrite/handler/CompactPathRuleTest.java | 78 ++++++++++++++++--- 3 files changed, 99 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..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 @@ -994,6 +994,9 @@ 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. + if (_violations != null) + _violations.removeIf(UriCompliance::isPathViolation); _uri = null; _path = null; _canonicalPath = null; @@ -1016,6 +1019,9 @@ 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. + 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..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 @@ -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,18 @@ 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, + Violation.SUSPICIOUS_PATH_CHARACTERS, + Violation.ILLEGAL_PATH_CHARACTERS)); + /** * Compliance mode that exactly follows RFC3986, * excluding all URI Violations. @@ -352,6 +368,17 @@ public UriCompliance without(String name, Violation... violations) return new UriCompliance(name, remainder); } + /** + * 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 PATH_VIOLATIONS.contains(violation); + } + @Override public String toString() { 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")); + } } }