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 28, 2021
1 parent 7bf140b commit 2a801c4
Show file tree
Hide file tree
Showing 4 changed files with 50 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 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 @@ -99,6 +98,10 @@ public final class DevModeHandlerImpl

private static final AtomicReference<DevModeHandlerImpl> atomicHandler = new AtomicReference<>();

// webpack dev-server allows " character if passed through, need to
// explicitly check requests for it
private static final Pattern WEBPACK_ILLEGAL_CHAR_PATTERN = Pattern
.compile("\"|%22");
// It's not possible to know whether webpack is ready unless reading output
// messages. When webpack finishes, it writes either a `Compiled` or a
// `Failed` in the last line
Expand Down Expand Up @@ -316,7 +319,9 @@ public boolean serveDevModeRequest(HttpServletRequest request,
// a valid request for webpack-dev-server should start with '/VAADIN/'
String requestFilename = request.getPathInfo();

if (HandlerHelper.isPathUnsafe(requestFilename)) {
if (HandlerHelper.isPathUnsafe(requestFilename)
|| WEBPACK_ILLEGAL_CHAR_PATTERN.matcher(requestFilename)
.find()) {
getLogger().info("Blocked attempt to access file: {}",
requestFilename);
response.setStatus(HttpServletResponse.SC_FORBIDDEN);
Expand Down Expand Up @@ -393,6 +398,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 2a801c4

Please sign in to comment.