Merge pull request #1117 from rmartinc/UNDERTOW-1886-master

[UNDERTOW-1886] [master] Request dispatcher is returned when the path points to outside the servlet context
undertow-io · May 25, 2021 · 0559617 · 0559617
2 parents 4a7a8c8 + 66c6252
commit 0559617
Show file tree

Hide file tree

Showing 7 changed files with 57 additions and 10 deletions.
diff --git a/core/src/main/java/io/undertow/util/CanonicalPathUtils.java b/core/src/main/java/io/undertow/util/CanonicalPathUtils.java
@@ -170,6 +170,10 @@ private static String realCanonicalize(final String path, final int lastDot, fin
                 }
             }
         }
+        if (eatCount > 0) {
+            // the relative path is outside the context
+            return null;
+        }
         final StringBuilder result = new StringBuilder();
         if (tokenEnd != 0) {
             result.append(path.substring(0, tokenEnd));

diff --git a/core/src/test/java/io/undertow/util/CanonicalPathUtilsTestCase.java b/core/src/test/java/io/undertow/util/CanonicalPathUtilsTestCase.java
@@ -61,8 +61,11 @@ public void testCanonicalization() {
         Assert.assertEquals("/b", CanonicalPathUtils.canonicalize("/a/../b"));
         Assert.assertEquals("/b", CanonicalPathUtils.canonicalize("/a/../c/../e/../b"));
         Assert.assertEquals("/b", CanonicalPathUtils.canonicalize("/a/c/../../b"));
-        Assert.assertEquals("/", CanonicalPathUtils.canonicalize("/a/../.."));
-        Assert.assertEquals("/foo", CanonicalPathUtils.canonicalize("/a/../../foo"));
+
+        // out of servlet context
+        Assert.assertNull(CanonicalPathUtils.canonicalize("/a/../.."));
+        Assert.assertNull(CanonicalPathUtils.canonicalize("/a/../../foo"));
+        Assert.assertNull(CanonicalPathUtils.canonicalize("/../../a/b/bar"));
 
         //preserve (single) trailing /
         Assert.assertEquals("/a/", CanonicalPathUtils.canonicalize("/a/"));
@@ -101,8 +104,11 @@ public void testCanonicalizationBackslash() {
         Assert.assertEquals("\\b", CanonicalPathUtils.canonicalize("\\a\\..\\b"));
         Assert.assertEquals("\\b", CanonicalPathUtils.canonicalize("\\a\\..\\c\\..\\e\\..\\b"));
         Assert.assertEquals("\\b", CanonicalPathUtils.canonicalize("\\a\\c\\..\\..\\b"));
-        Assert.assertEquals("/", CanonicalPathUtils.canonicalize("\\a\\..\\.."));
-        Assert.assertEquals("\\foo", CanonicalPathUtils.canonicalize("\\a\\..\\..\\foo"));
+
+         // out of servlet context
+        Assert.assertNull(CanonicalPathUtils.canonicalize("\\a\\..\\.."));
+        Assert.assertNull(CanonicalPathUtils.canonicalize("\\a\\..\\..\\foo"));
+        Assert.assertNull(CanonicalPathUtils.canonicalize("\\..\\..\\a\\b\\bar"));
 
         //preserve (single) trailing \
         Assert.assertEquals("\\a\\", CanonicalPathUtils.canonicalize("\\a\\"));

diff --git a/servlet/src/main/java/io/undertow/servlet/UndertowServletMessages.java b/servlet/src/main/java/io/undertow/servlet/UndertowServletMessages.java
@@ -232,4 +232,7 @@ public interface UndertowServletMessages {
 
     @Message(id = 10062, value = "No SecurityContext available")
     ServletException noSecurityContextAvailable();
+
+    @Message(id = 10063, value = "Path %s must start with a / to get the request dispatcher")
+    IllegalArgumentException pathMustStartWithSlashForRequestDispatcher(String path);
 }
diff --git a/servlet/src/main/java/io/undertow/servlet/spec/HttpServletRequestImpl.java b/servlet/src/main/java/io/undertow/servlet/spec/HttpServletRequestImpl.java
@@ -41,7 +41,6 @@
 import io.undertow.servlet.util.EmptyEnumeration;
 import io.undertow.servlet.util.IteratorEnumeration;
 import io.undertow.util.AttachmentKey;
-import io.undertow.util.CanonicalPathUtils;
 import io.undertow.util.DateUtils;
 import io.undertow.util.HeaderMap;
 import io.undertow.util.HeaderValues;
@@ -985,18 +984,21 @@ public boolean isSecure() {
 
     @Override
     public RequestDispatcher getRequestDispatcher(final String path) {
+        if (path == null) {
+            return null;
+        }
         String realPath;
         if (path.startsWith("/")) {
-            realPath = CanonicalPathUtils.canonicalize(path);
+            realPath = path;
         } else {
             String current = exchange.getRelativePath();
             int lastSlash = current.lastIndexOf("/");
             if (lastSlash != -1) {
                 current = current.substring(0, lastSlash + 1);
             }
-            realPath = CanonicalPathUtils.canonicalize(current + path);
+            realPath = current + path;
         }
-        return new RequestDispatcherImpl(realPath, servletContext);
+        return servletContext.getRequestDispatcher(realPath);
     }
 
     @Override

diff --git a/servlet/src/main/java/io/undertow/servlet/spec/ServletContextImpl.java b/servlet/src/main/java/io/undertow/servlet/spec/ServletContextImpl.java
@@ -338,7 +338,18 @@ public InputStream getResourceAsStream(final String path) {
 
     @Override
     public RequestDispatcher getRequestDispatcher(final String path) {
-        return new RequestDispatcherImpl(path, this);
+        if (path == null) {
+            return null;
+        }
+        if (!path.startsWith("/")) {
+            throw UndertowServletMessages.MESSAGES.pathMustStartWithSlashForRequestDispatcher(path);
+        }
+        final String realPath = CanonicalPathUtils.canonicalize(path);
+        if (realPath == null) {
+            // path is outside the servlet context, return null per spec
+            return null;
+        }
+        return new RequestDispatcherImpl(realPath, this);
     }
 
     @Override

diff --git a/servlet/src/test/java/io/undertow/servlet/test/dispatcher/DispatcherForwardTestCase.java b/servlet/src/test/java/io/undertow/servlet/test/dispatcher/DispatcherForwardTestCase.java
@@ -41,6 +41,8 @@
 import org.apache.http.HttpResponse;
 import org.apache.http.client.methods.HttpGet;
 import org.apache.http.client.methods.HttpPost;
+import org.hamcrest.CoreMatchers;
+import org.hamcrest.MatcherAssert;
 import org.junit.Assert;
 import org.junit.Assume;
 import org.junit.BeforeClass;
@@ -159,6 +161,21 @@ public void testNameBasedInclude() throws IOException {
         }
     }
 
+    @Test
+    public void testNameBasedForwardOutServletContext() throws IOException {
+        TestHttpClient client = new TestHttpClient();
+        try {
+            HttpGet get = new HttpGet(DefaultServer.getDefaultServerURL() + "/servletContext/dispatch");
+            get.setHeader("forward", "../forward");
+            HttpResponse result = client.execute(get);
+            Assert.assertEquals(StatusCodes.INTERNAL_SERVER_ERROR, result.getStatusLine().getStatusCode());
+            final String response = HttpClientUtils.readResponse(result);
+            MatcherAssert.assertThat(response, CoreMatchers.containsString("dispatcher was null!"));
+        } finally {
+            client.getConnectionManager().shutdown();
+        }
+    }
+
     @Test
     public void testPathBasedStaticInclude() throws IOException {
         TestHttpClient client = new TestHttpClient();

diff --git a/servlet/src/test/java/io/undertow/servlet/test/dispatcher/ForwardServlet.java b/servlet/src/test/java/io/undertow/servlet/test/dispatcher/ForwardServlet.java
@@ -41,7 +41,11 @@ protected void doGet(final HttpServletRequest req, final HttpServletResponse res
         } else {
             dispatcher = req.getRequestDispatcher(req.getHeader("forward"));
         }
-        dispatcher.forward(req, resp);
+        if (dispatcher != null) {
+            dispatcher.forward(req, resp);
+        } else {
+            resp.sendError(500, "dispatcher was null!");
+        }
     }
 
     @Override