fix: check if Url contains directory change in Dev Mode (#9392)

Checks whether the Url contains a directory change and a double encoding in Dev Mode. Returns 403 Forbidden immediately and skip the request handling, if does.

(cherry picked from commit 19fe386)

flow-server/src/main/java/com/vaadin/flow/server/DevModeHandler.java

@@ -300,6 +300,13 @@ public boolean serveDevModeRequest(HttpServletRequest request,
         // a valid request for webpack-dev-server should start with '/VAADIN/'
         String requestFilename = request.getPathInfo();
 
+        if (HandlerHelper.isPathUnsafe(requestFilename)) {
+            getLogger().info(HandlerHelper.UNSAFE_PATH_ERROR_MESSAGE_PATTERN,
+                    requestFilename);
+            response.setStatus(HttpServletResponse.SC_FORBIDDEN);
+            return true;
+        }
+
         HttpURLConnection connection = prepareConnection(requestFilename,
                 request.getMethod());
 

flow-server/src/main/java/com/vaadin/flow/server/HandlerHelper.java

@@ -16,8 +16,12 @@
 package com.vaadin.flow.server;
 
 import java.io.Serializable;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.nio.charset.StandardCharsets;
 import java.util.Locale;
 import java.util.function.BiConsumer;
+import java.util.regex.Pattern;
 
 import com.vaadin.flow.component.UI;
 import com.vaadin.flow.shared.ApplicationConstants;
@@ -35,6 +39,15 @@ public class HandlerHelper implements Serializable {
      */
     static final SystemMessages DEFAULT_SYSTEM_MESSAGES = new SystemMessages();
 
+    /**
+     * The pattern of error message shown when the URL path contains unsafe
+     * double encoding.
+     */
+    static final String UNSAFE_PATH_ERROR_MESSAGE_PATTERN = "Blocked attempt to access file: {}";
+
+    private static final Pattern PARENT_DIRECTORY_REGEX = Pattern
+            .compile("(/|\\\\)\\.\\.(/|\\\\)?", Pattern.CASE_INSENSITIVE);
+
     /**
      * Framework internal enum for tracking the type of a request.
      */
@@ -171,4 +184,26 @@ public static String getCancelingRelativePath(String pathToCancel) {
         return sb.toString();
     }
 
+    /**
+     * Checks if the given URL path contains the directory change instruction
+     * (dot-dot), taking into account possible double encoding in hexadecimal
+     * format, which can be injected maliciously.
+     *
+     * @param path
+     *            the URL path to be verified.
+     * @return {@code true}, if the given path has a directory change
+     *         instruction, {@code false} otherwise.
+     */
+    public static boolean isPathUnsafe(String path) {
+        // Check that the path does not have '/../', '\..\', %5C..%5C,
+        // %2F..%2F, nor '/..', '\..', %5C.., %2F..
+        try {
+            path = URLDecoder.decode(path, StandardCharsets.UTF_8.name());
+        } catch (UnsupportedEncodingException e) {
+            throw new RuntimeException("An error occurred during decoding URL.",
+                    e);
+        }
+        return PARENT_DIRECTORY_REGEX.matcher(path).find();
+    }
+
 }

flow-server/src/main/java/com/vaadin/flow/server/StaticFileServer.java

@@ -19,11 +19,8 @@
 import javax.servlet.http.HttpServletResponse;
 import java.io.IOException;
 import java.io.InputStream;
-import java.io.UnsupportedEncodingException;
 import java.net.URL;
 import java.net.URLConnection;
-import java.net.URLDecoder;
-import java.nio.charset.StandardCharsets;
 import java.util.regex.Pattern;
 
 import org.slf4j.Logger;
@@ -54,8 +51,6 @@ public class StaticFileServer implements StaticFileHandler {
             + "fixIncorrectWebjarPaths";
     private static final Pattern INCORRECT_WEBJAR_PATH_REGEX = Pattern
             .compile("^/frontend[-\\w/]*/webjars/");
-    private static final Pattern PARENT_DIRECTORY_REGEX = Pattern
-            .compile("(/|\\\\)\\.\\.(/|\\\\)", Pattern.CASE_INSENSITIVE);
 
     private final ResponseWriter responseWriter;
     private final VaadinServletService servletService;
@@ -107,10 +102,10 @@ public boolean serveStaticResource(HttpServletRequest request,
             HttpServletResponse response) throws IOException {
 
         String filenameWithPath = getRequestFilename(request);
-        if (!isPathSafe(filenameWithPath)) {
-            getLogger().info("Blocked attempt to access file: {}",
+        if (HandlerHelper.isPathUnsafe(filenameWithPath)) {
+            getLogger().info(HandlerHelper.UNSAFE_PATH_ERROR_MESSAGE_PATTERN,
                     filenameWithPath);
-            response.setStatus(HttpServletResponse.SC_NOT_MODIFIED);
+            response.setStatus(HttpServletResponse.SC_FORBIDDEN);
             return true;
         }
 
@@ -189,18 +184,6 @@ private String fixIncorrectWebjarPath(String requestFilename) {
                 .replaceAll("/webjars/");
     }
 
-    private boolean isPathSafe(String path) {
-        // Check that the path does not have '/../', '\..\', %5C..%5C, or
-        // %2F..%2F
-        try {
-            path = URLDecoder.decode(path, StandardCharsets.UTF_8.name());
-        } catch (UnsupportedEncodingException e) {
-            throw new RuntimeException("An error occurred during decoding URL.",
-                    e);
-        }
-        return !PARENT_DIRECTORY_REGEX.matcher(path).find();
-    }
-
     /**
      * Check if it is ok to serve the requested file from the classpath.
      * <p>

flow-server/src/test/java/com/vaadin/flow/server/DevModeHandlerTest.java

@@ -19,6 +19,7 @@
 import static com.vaadin.flow.server.InitParameters.SERVLET_PARAMETER_DEVMODE_WEBPACK_TIMEOUT;
 import static com.vaadin.flow.server.frontend.NodeUpdateTestUtil.WEBPACK_TEST_OUT_FILE;
 import static com.vaadin.flow.server.frontend.NodeUpdateTestUtil.createStubWebpackServer;
+import static java.net.HttpURLConnection.HTTP_FORBIDDEN;
 import static java.net.HttpURLConnection.HTTP_NOT_FOUND;
 import static java.net.HttpURLConnection.HTTP_NOT_MODIFIED;
 import static java.net.HttpURLConnection.HTTP_OK;
@@ -437,6 +438,12 @@ public void startDevModeHandler_prepareTasksThrows_handleThrows()
         throwFuture.completeExceptionally(new CustomRuntimeException());
         DevModeHandler handler = DevModeHandler.start(0, configuration,
                 npmFolder, throwFuture);
+        try {
+            handler.join();
+        } catch (CompletionException ignore) {
+            // this is an expected exception thrown on join for the handler
+
+        }
         handler.handleRequest(Mockito.mock(VaadinSession.class),
                 Mockito.mock(VaadinRequest.class),
                 Mockito.mock(VaadinResponse.class));
@@ -539,6 +546,74 @@ public void start_serverPortDoesNotWork_throws() throws Exception {
         handler.join();
     }
 
+    @Test
+    public void serveDevModeRequest_uriWithDirectoryChangeWithSlash_returnsImmediatelyAndSetsForbiddenStatus()
+            throws IOException {
+        verifyServeDevModeRequestReturnsTrueAndSetsProperStatusCode(
+                "/VAADIN/build/../vaadin-bundle-1234.cache.js");
+    }
+
+    @Test
+    public void serveDevModeRequest_uriWithDirectoryChangeWithBackslash_returnsImmediatelyAndSetsForbiddenStatus()
+            throws IOException {
+        verifyServeDevModeRequestReturnsTrueAndSetsProperStatusCode(
+                "/VAADIN/build/something\\..\\vaadin-bundle-1234.cache.js");
+    }
+
+    @Test
+    public void serveDevModeRequest_uriWithDirectoryChangeWithEncodedBackslashUpperCase_returnsImmediatelyAndSetsForbiddenStatus()
+            throws IOException {
+        verifyServeDevModeRequestReturnsTrueAndSetsProperStatusCode(
+                "/VAADIN/build/something%5C..%5Cvaadin-bundle-1234.cache.js");
+    }
+
+    @Test
+    public void serveDevModeRequest_uriWithDirectoryChangeWithEncodedBackslashLowerCase_returnsImmediatelyAndSetsForbiddenStatus()
+            throws IOException {
+        verifyServeDevModeRequestReturnsTrueAndSetsProperStatusCode(
+                "/VAADIN/build/something%5c..%5cvaadin-bundle-1234.cache.js");
+    }
+
+    @Test
+    public void serveDevModeRequest_uriWithDirectoryChangeInTheEndWithSlash_returnsImmediatelyAndSetsForbiddenStatus()
+            throws IOException {
+        verifyServeDevModeRequestReturnsTrueAndSetsProperStatusCode(
+                "/VAADIN/build/..");
+    }
+
+    @Test
+    public void serveDevModeRequest_uriWithDirectoryChangeInTheEndWithBackslash_returnsImmediatelyAndSetsForbiddenStatus()
+            throws IOException {
+        verifyServeDevModeRequestReturnsTrueAndSetsProperStatusCode(
+                "/VAADIN/build/something\\..");
+    }
+
+    @Test
+    public void serveDevModeRequest_uriWithDirectoryChangeInTheEndWithEncodedBackslashUpperCase_returnsImmediatelyAndSetsForbiddenStatus()
+            throws IOException {
+        verifyServeDevModeRequestReturnsTrueAndSetsProperStatusCode(
+                "/VAADIN/build/something%5C..");
+    }
+
+    @Test
+    public void serveDevModeRequest_uriWithDirectoryChangeInTheEndWithEncodedBackslashLowerCase_returnsImmediatelyAndSetsForbiddenStatus()
+            throws IOException {
+        verifyServeDevModeRequestReturnsTrueAndSetsProperStatusCode(
+                "/VAADIN/build/something%5c..");
+    }
+
+    private void verifyServeDevModeRequestReturnsTrueAndSetsProperStatusCode(
+            String uri) throws IOException {
+        HttpServletRequest request = prepareRequest(uri);
+        HttpServletResponse response = prepareResponse();
+        DevModeHandler handler = DevModeHandler.start(configuration, npmFolder,
+                CompletableFuture.completedFuture(null));
+        handler.join();
+        assertTrue(handler.serveDevModeRequest(request, response));
+
+        Assert.assertEquals(HTTP_FORBIDDEN, responseStatus);
+    }
+
     private VaadinServlet prepareServlet(int port)
             throws ServletException, IOException {
         DevModeHandler.start(port, configuration, npmFolder,

flow-server/src/test/java/com/vaadin/flow/server/StaticFileServerTest.java

@@ -558,36 +558,66 @@ private void staticBuildResourceWithDirectoryChange_nothingServed(
 
         Assert.assertTrue(fileServer.serveStaticResource(request, response));
         Assert.assertEquals(0, out.getOutput().length);
+        Assert.assertEquals(HttpServletResponse.SC_FORBIDDEN,
+                responseCode.get());
     }
 
     @Test
-    public void staticBuildResourceWithDirectoryChangeWithSlash_nothingServed()
+    public void serveStaticResource_uriWithDirectoryChangeWithSlash_returnsImmediatelyAndSetsForbiddenStatus()
             throws IOException {
         staticBuildResourceWithDirectoryChange_nothingServed(
                 "/VAADIN/build/../vaadin-bundle-1234.cache.js");
     }
 
     @Test
-    public void staticBuildResourceWithDirectoryChangeWithBackslash_nothingServed()
+    public void serveStaticResource_uriWithDirectoryChangeWithBackslash_returnsImmediatelyAndSetsForbiddenStatus()
             throws IOException {
         staticBuildResourceWithDirectoryChange_nothingServed(
                 "/VAADIN/build/something\\..\\vaadin-bundle-1234.cache.js");
     }
 
     @Test
-    public void staticBuildResourceWithDirectoryChangeWithEncodedBackslashUpperCase_nothingServed()
+    public void serveStaticResource_uriWithDirectoryChangeWithEncodedBackslashUpperCase_returnsImmediatelyAndSetsForbiddenStatus()
             throws IOException {
         staticBuildResourceWithDirectoryChange_nothingServed(
                 "/VAADIN/build/something%5C..%5Cvaadin-bundle-1234.cache.js");
     }
 
     @Test
-    public void staticBuildResourceWithDirectoryChangeWithEncodedBackslashLowerCase_nothingServed()
+    public void serveStaticResource_uriWithDirectoryChangeWithEncodedBackslashLowerCase_returnsImmediatelyAndSetsForbiddenStatus()
             throws IOException {
         staticBuildResourceWithDirectoryChange_nothingServed(
                 "/VAADIN/build/something%5c..%5cvaadin-bundle-1234.cache.js");
     }
 
+    @Test
+    public void serveStaticResource_uriWithDirectoryChangeInTheEndWithSlash_returnsImmediatelyAndSetsForbiddenStatus()
+            throws IOException {
+        staticBuildResourceWithDirectoryChange_nothingServed(
+                "/VAADIN/build/..");
+    }
+
+    @Test
+    public void serveStaticResource_uriWithDirectoryChangeInTheEndWithBackslash_returnsImmediatelyAndSetsForbiddenStatus()
+            throws IOException {
+        staticBuildResourceWithDirectoryChange_nothingServed(
+                "/VAADIN/build/something\\..");
+    }
+
+    @Test
+    public void serveStaticResource_uriWithDirectoryChangeInTheEndWithEncodedBackslashUpperCase_returnsImmediatelyAndSetsForbiddenStatus()
+            throws IOException {
+        staticBuildResourceWithDirectoryChange_nothingServed(
+                "/VAADIN/build/something%5C..");
+    }
+
+    @Test
+    public void serveStaticResource_uriWithDirectoryChangeInTheEndWithEncodedBackslashLowerCase_returnsImmediatelyAndSetsForbiddenStatus()
+            throws IOException {
+        staticBuildResourceWithDirectoryChange_nothingServed(
+                "/VAADIN/build/something%5c..");
+    }
+
     @Test
     public void customStaticBuildResource_isServed() throws IOException {
         String pathInfo = "/VAADIN/build/my-text.txt";

flow-tests/test-dev-mode/src/test/java/com/vaadin/flow/uitest/ui/UrlValidationIT.java

@@ -0,0 +1,50 @@
+/*
+ * Copyright 2000-2020 Vaadin Ltd.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package com.vaadin.flow.uitest.ui;
+
+import java.io.IOException;
+import java.net.HttpURLConnection;
+import java.net.URL;
+
+import com.vaadin.flow.testutil.ChromeBrowserTest;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class UrlValidationIT extends ChromeBrowserTest {
+
+    @Test
+    public void devModeUriValidation_uriWithDirectoryChange_statusForbidden() throws IOException {
+        sendRequestAndValidateResponseStatusForbidden(
+                "/VAADIN/build/%252E%252E/");
+    }
+
+    @Test
+    public void staticResourceUriValidation_uriWithDirectoryChange_statusForbidden() throws IOException {
+        sendRequestAndValidateResponseStatusForbidden(
+                "/VAADIN/build/%252E%252E/some-resource.css");
+    }
+
+    private void sendRequestAndValidateResponseStatusForbidden(String pathToResource) throws IOException {
+        final String urlString = getRootURL() + "/view" + pathToResource;
+        URL url = new URL(urlString);
+        HttpURLConnection connection = (HttpURLConnection) url.openConnection();
+        connection.setRequestMethod("GET");
+        int responseCode = connection.getResponseCode();
+        Assert.assertEquals("HTTP 403 Forbidden expected for urls with " +
+                "directory change", HttpURLConnection.HTTP_FORBIDDEN, responseCode);
+    }
+}