Fix #6870 Encode control characters (#6874)

Fix #6870 URIUtil.encodePath encodes control characters

* Better test for wider range of characters
* Encode all control characters

Signed-off-by: Greg Wilkins <gregw@webtide.com>

jetty-proxy/pom.xml

@@ -47,6 +47,12 @@
       <classifier>tests</classifier>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.eclipse.jetty</groupId>
+      <artifactId>jetty-rewrite</artifactId>
+      <version>${project.version}</version>
+      <scope>test</scope>
+    </dependency>
     <dependency>
       <groupId>org.eclipse.jetty.toolchain</groupId>
       <artifactId>jetty-test-helper</artifactId>

jetty-proxy/src/test/java/org/eclipse/jetty/proxy/BalancerServletTest.java

@@ -31,6 +31,8 @@ import javax.servlet.http.HttpServletResponse;
 
 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.AfterEach;
 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,12 +116,17 @@ public class BalancerServletTest
         return server.getURI().getPort();
     }
 
-    protected byte[] sendRequestToBalancer(String path) throws Exception
+    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();
+    }
+
+    protected byte[] sendRequestToBalancer(String path) throws Exception
+    {
+        ContentResponse response = getBalancedResponse(path);
         return response.getContent();
     }
 
@@ -160,12 +170,44 @@ public class BalancerServletTest
         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();

jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java

@@ -120,7 +120,7 @@ public class URIUtil
                         buf = new StringBuilder(path.length() * 2);
                         break loop;
                     default:
-                        if (c > 127)
+                        if (c < 0x20 || c >= 0x7f)
                         {
                             bytes = path.getBytes(URIUtil.__CHARSET);
                             buf = new StringBuilder(path.length() * 2);
@@ -193,7 +193,7 @@ public class URIUtil
                     continue;
 
                 default:
-                    if (c > 127)
+                    if (c < 0x20 || c >= 0x7f)
                     {
                         bytes = path.getBytes(URIUtil.__CHARSET);
                         break loop;
@@ -261,7 +261,7 @@ public class URIUtil
                         buf.append("%7D");
                         continue;
                     default:
-                        if (c < 0)
+                        if (c < 0x20 || c >= 0x7f)
                         {
                             buf.append('%');
                             TypeUtil.toHex(c, buf);

jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilTest.java

@@ -69,6 +69,7 @@ public class URIUtilTest
     {
         // @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\"/;<script>window.alert('xss');</script>",
                 "/context/%27list%27/%22me%22/%3B%3Cscript%3Ewindow.alert(%27xss%27)%3B%3C/script%3E"),
@@ -732,4 +733,25 @@ public class URIUtilTest
     {
         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);
+    }
 }