From 1749a90d954c6c85b51b7719835b2540b6cf77e8 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 20 Sep 2021 10:55:29 +1000 Subject: [PATCH 1/4] Reproduce #6870 Rewritten Balancer Reproduce #6870 Rewritten Balancer Signed-off-by: Greg Wilkins --- jetty-proxy/pom.xml | 6 +++ .../jetty/proxy/BalancerServletTest.java | 46 +++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/jetty-proxy/pom.xml b/jetty-proxy/pom.xml index e21421bb70e0..4cff18ace882 100644 --- a/jetty-proxy/pom.xml +++ b/jetty-proxy/pom.xml @@ -47,6 +47,12 @@ tests test + + org.eclipse.jetty + jetty-rewrite + ${project.version} + test + org.eclipse.jetty.toolchain jetty-test-helper diff --git a/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/BalancerServletTest.java b/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/BalancerServletTest.java index 06a89e7c7cb1..79794c58f4d8 100644 --- a/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/BalancerServletTest.java +++ b/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/BalancerServletTest.java @@ -31,6 +31,8 @@ import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.client.api.ContentResponse; +import org.eclipse.jetty.rewrite.handler.RewriteHandler; +import org.eclipse.jetty.rewrite.handler.VirtualHostRuleContainer; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.server.session.DefaultSessionIdManager; @@ -40,6 +42,9 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertEquals; public class BalancerServletTest @@ -111,6 +116,15 @@ private int getServerPort(Server server) return server.getURI().getPort(); } + protected ContentResponse getBalancedResponse(String path) throws Exception + { + ContentResponse response = client.newRequest("localhost", getServerPort(balancer)) + .path(CONTEXT_PATH + SERVLET_PATH + path) + .timeout(5, TimeUnit.SECONDS) + .send(); + return response; + } + protected byte[] sendRequestToBalancer(String path) throws Exception { ContentResponse response = client.newRequest("localhost", getServerPort(balancer)) @@ -160,12 +174,44 @@ public void testProxyPassReverse() throws Exception assertEquals("success", msg); } + @Test + public void testRewrittenBalancerWithEncodedURI() throws Exception + { + startBalancer(DumpServlet.class); + balancer.stop(); + RewriteHandler rewrite = new RewriteHandler(); + rewrite.setHandler(balancer.getHandler()); + balancer.setHandler(rewrite); + rewrite.setRewriteRequestURI(true); + rewrite.addRule(new VirtualHostRuleContainer()); + balancer.start(); + + ContentResponse response = getBalancedResponse("/test/%0A"); + assertThat(response.getStatus(), is(200)); + assertThat(response.getContentAsString(), containsString("requestURI='/context/mapping/test/%0A'")); + assertThat(response.getContentAsString(), containsString("servletPath='/mapping'")); + assertThat(response.getContentAsString(), containsString("pathInfo='/test/\n'")); + } + private String readFirstLine(byte[] responseBytes) throws IOException { BufferedReader reader = new BufferedReader(new InputStreamReader(new ByteArrayInputStream(responseBytes))); return reader.readLine(); } + public static final class DumpServlet extends HttpServlet + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + resp.setContentType("text/plain"); + resp.getWriter().printf("requestURI='%s'%n", req.getRequestURI()); + resp.getWriter().printf("servletPath='%s'%n", req.getServletPath()); + resp.getWriter().printf("pathInfo='%s'%n", req.getPathInfo()); + resp.getWriter().flush(); + } + } + public static final class CounterServlet extends HttpServlet { private final AtomicInteger counter = new AtomicInteger(); From 365bed18aa72857c1f8da762e26a569ca0c79c05 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 20 Sep 2021 11:05:53 +1000 Subject: [PATCH 2/4] Fix #6870 URIUtil.encodePath encodes control characters Fix #6870 URIUtil.encodePath encodes control characters Signed-off-by: Greg Wilkins --- .../src/main/java/org/eclipse/jetty/util/URIUtil.java | 6 +++--- .../src/test/java/org/eclipse/jetty/util/URIUtilTest.java | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) 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 c078fc135061..7822f572cc12 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 @@ -120,7 +120,7 @@ private static StringBuilder encodePath(StringBuilder buf, String path, int offs buf = new StringBuilder(path.length() * 2); break loop; default: - if (c > 127) + if (c < 20 || c > 127) { bytes = path.getBytes(URIUtil.__CHARSET); buf = new StringBuilder(path.length() * 2); @@ -193,7 +193,7 @@ private static StringBuilder encodePath(StringBuilder buf, String path, int offs continue; default: - if (c > 127) + if (c < 20 || c > 127) { bytes = path.getBytes(URIUtil.__CHARSET); break loop; @@ -261,7 +261,7 @@ private static StringBuilder encodePath(StringBuilder buf, String path, int offs buf.append("%7D"); continue; default: - if (c < 0) + if (c < 20) { buf.append('%'); TypeUtil.toHex(c, buf); diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilTest.java index 13eb24dc6c61..3ddbdb6037ae 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilTest.java @@ -69,6 +69,7 @@ public static Stream encodePathSource() { // @checkstyle-disable-check : AvoidEscapedUnicodeCharactersCheck return Stream.of( + Arguments.of("/foo/\n/bar", "/foo/%0A/bar"), Arguments.of("/foo%23+;,:=/b a r/?info ", "/foo%2523+%3B,:=/b%20a%20r/%3Finfo%20"), Arguments.of("/context/'list'/\"me\"/;", "/context/%27list%27/%22me%22/%3B%3Cscript%3Ewindow.alert(%27xss%27)%3B%3C/script%3E"), From 1489e9cde854196a95c40c83c408065e71071f65 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 21 Sep 2021 08:56:31 +1000 Subject: [PATCH 3/4] Better test for wider range of characters Encode all control characters Signed-off-by: Greg Wilkins --- .../java/org/eclipse/jetty/util/URIUtil.java | 6 +++--- .../org/eclipse/jetty/util/URIUtilTest.java | 21 +++++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) 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 7822f572cc12..227db9815b5d 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 @@ -120,7 +120,7 @@ private static StringBuilder encodePath(StringBuilder buf, String path, int offs buf = new StringBuilder(path.length() * 2); break loop; default: - if (c < 20 || c > 127) + if (c < 0x20 || c >= 0x7f) { bytes = path.getBytes(URIUtil.__CHARSET); buf = new StringBuilder(path.length() * 2); @@ -193,7 +193,7 @@ private static StringBuilder encodePath(StringBuilder buf, String path, int offs continue; default: - if (c < 20 || c > 127) + if (c < 0x20 || c >= 0x7f) { bytes = path.getBytes(URIUtil.__CHARSET); break loop; @@ -261,7 +261,7 @@ private static StringBuilder encodePath(StringBuilder buf, String path, int offs buf.append("%7D"); continue; default: - if (c < 20) + if (c < 0x20 || c >= 0x7f) { buf.append('%'); TypeUtil.toHex(c, buf); diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilTest.java index 3ddbdb6037ae..fea195ab472b 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilTest.java @@ -733,4 +733,25 @@ public void testAddQueryParam(String param1, String param2, Matcher matc { assertThat(URIUtil.addQueries(param1, param2), matcher); } + + @Test + public void testEncodeDecodeVisibleOnly() + { + StringBuilder builder = new StringBuilder(); + builder.append('/'); + for (char i = 0; i < 0x7FFF; i++) + builder.append(i); + String path = builder.toString(); + String encoded = URIUtil.encodePath(path); + // Check endoded is visible + for (char c : encoded.toCharArray()) + { + assertTrue(c > 0x20 && c < 0x80); + assertFalse(Character.isWhitespace(c)); + assertFalse(Character.isISOControl(c)); + } + // check decode to original + String decoded = URIUtil.decodePath(encoded); + assertEquals(path, decoded); + } } From 5ee8aa91a80eacccb51a274af15cb13e73ae9395 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 21 Sep 2021 08:59:14 +1000 Subject: [PATCH 4/4] updates from review of test Signed-off-by: Greg Wilkins --- .../java/org/eclipse/jetty/proxy/BalancerServletTest.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/BalancerServletTest.java b/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/BalancerServletTest.java index 79794c58f4d8..6941298ff09b 100644 --- a/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/BalancerServletTest.java +++ b/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/BalancerServletTest.java @@ -118,19 +118,15 @@ private int getServerPort(Server server) protected ContentResponse getBalancedResponse(String path) throws Exception { - ContentResponse response = client.newRequest("localhost", getServerPort(balancer)) + return client.newRequest("localhost", getServerPort(balancer)) .path(CONTEXT_PATH + SERVLET_PATH + path) .timeout(5, TimeUnit.SECONDS) .send(); - return response; } protected byte[] sendRequestToBalancer(String path) throws Exception { - ContentResponse response = client.newRequest("localhost", getServerPort(balancer)) - .path(CONTEXT_PATH + SERVLET_PATH + path) - .timeout(5, TimeUnit.SECONDS) - .send(); + ContentResponse response = getBalancedResponse(path); return response.getContent(); }