Skip to content

Commit

Permalink
fix: prevent passing bad character to dev-server
Browse files Browse the repository at this point in the history
The webpack dev-server does not escape " character, as it is not valid
URL. This limitation was not checked when passing request to it via
DevModeHandlerImpl.
  • Loading branch information
pleku committed May 27, 2021
1 parent 2c07bd3 commit 3952101
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public interface DevModeHandler extends RequestHandler {
* Prepare a HTTP connection against webpack-dev-server.
*
* @param path
* the file to request
* the file to request, needs to be safe
* @param method
* the http method to use
* @return the connection
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package com.vaadin.flow.server;

import javax.servlet.http.HttpServletRequest;
import java.io.Serializable;
import java.io.UnsupportedEncodingException;
import java.net.URLDecoder;
Expand All @@ -28,8 +29,6 @@
import java.util.regex.Pattern;
import java.util.stream.Collectors;

import javax.servlet.http.HttpServletRequest;

import com.vaadin.flow.component.UI;
import com.vaadin.flow.server.communication.PwaHandler;
import com.vaadin.flow.server.frontend.FrontendUtils;
Expand All @@ -56,6 +55,10 @@ public class HandlerHelper implements Serializable {

private static final Pattern PARENT_DIRECTORY_REGEX = Pattern
.compile("(/|\\\\)\\.\\.(/|\\\\)?", Pattern.CASE_INSENSITIVE);
// webpack dev-server allows " character if passed through, need to
// explicitly check requests for it
private static final Pattern ILLEGAL_CHARACTER_REGEX = Pattern
.compile("(?:%22)|(?:\")");

/**
* Framework internal enum for tracking the type of a request.
Expand Down Expand Up @@ -409,7 +412,8 @@ public static boolean isPathUnsafe(String path) {
throw new RuntimeException("An error occurred during decoding URL.",
e);
}
return PARENT_DIRECTORY_REGEX.matcher(path).find();
return PARENT_DIRECTORY_REGEX.matcher(path).find()
|| ILLEGAL_CHARACTER_REGEX.matcher(path).find();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import javax.servlet.ServletOutputStream;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import java.io.File;
import java.io.IOException;
import java.io.InputStream;
Expand Down Expand Up @@ -48,8 +47,8 @@
import com.vaadin.flow.di.Lookup;
import com.vaadin.flow.internal.BrowserLiveReload;
import com.vaadin.flow.internal.BrowserLiveReloadAccessor;
import com.vaadin.flow.internal.Pair;
import com.vaadin.flow.internal.DevModeHandler;
import com.vaadin.flow.internal.Pair;
import com.vaadin.flow.server.ExecutionFailedException;
import com.vaadin.flow.server.HandlerHelper;
import com.vaadin.flow.server.InitParameters;
Expand Down Expand Up @@ -393,6 +392,7 @@ private boolean checkWebpackConnection() {
@Override
public HttpURLConnection prepareConnection(String path, String method)
throws IOException {
// path should have been checked at this point for any outside requests
URL uri = new URL(WEBPACK_HOST + ":" + getPort() + path);
HttpURLConnection connection = (HttpURLConnection) uri.openConnection();
connection.setRequestMethod(method);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@
import javax.servlet.ServletOutputStream;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.IOException;
import java.lang.reflect.Field;
import java.net.ConnectException;
import java.net.HttpURLConnection;
import java.net.InetSocketAddress;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
Expand Down Expand Up @@ -640,6 +640,44 @@ public void devModeNotReady_handleRequest_returnsHtml() throws Exception {
Mockito.verify(response).setContentType("text/html;charset=utf-8");
}

@Test
public void serveDevModeRequest_uriForDevmodeGizmo_goesToWebpack()
throws Exception {
HttpServletRequest request = prepareRequest("/VAADIN/build/vaadin-devmodeGizmo-f679dbf313191ec3d018.cache.js");
HttpServletResponse response = prepareResponse();

final String manifestJsonResponse = "{ \"sw.js\": "
+ "\"sw.js\", \"index.html\": \"index.html\" }";
int port = prepareHttpServer(0, HTTP_OK, manifestJsonResponse);

DevModeHandlerImpl devModeHandler = DevModeHandlerImpl.start(port,
createDevModeLookup(), npmFolder,
CompletableFuture.completedFuture(null));
devModeHandler.join();

assertTrue(devModeHandler.serveDevModeRequest(request, response));
assertEquals(HTTP_OK, responseStatus);
}

@Test
public void serveDevModeRequest_uriWithScriptInjected_returnsImmediatelyAndSetsForbiddenStatus()
throws Exception {
HttpServletRequest request = prepareRequest("/VAADIN/build/vaadin-devmodeGizmo-f679dbf313191ec3d018.cache%3f%22onload=%22alert(1)");
HttpServletResponse response = prepareResponse();

final String manifestJsonResponse = "{ \"sw.js\": "
+ "\"sw.js\", \"index.html\": \"index.html\" }";
int port = prepareHttpServer(0, HTTP_OK, manifestJsonResponse);

DevModeHandlerImpl devModeHandler = DevModeHandlerImpl.start(port,
createDevModeLookup(), npmFolder,
CompletableFuture.completedFuture(null));
devModeHandler.join();

assertTrue(devModeHandler.serveDevModeRequest(request, response));
assertEquals(HTTP_FORBIDDEN, responseStatus);
}

@Test
public void serveDevModeRequest_uriWithDirectoryChangeWithSlash_returnsImmediatelyAndSetsForbiddenStatus()
throws IOException {
Expand Down

0 comments on commit 3952101

Please sign in to comment.