Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: prevent passing bad character to dev-server #11099

Merged
merged 2 commits into from
May 31, 2021
Merged

fix: prevent passing bad character to dev-server #11099

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a801c4
fix: prevent passing bad character to dev-server
pleku May 27, 2021
eb24d8c
chore: run formatter
pleku May 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion flow-server/src/main/java/com/vaadin/flow/internal/DevModeHandler.java
View file
Open in desktop
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
3 changes: 1 addition & 2 deletions flow-server/src/main/java/com/vaadin/flow/server/HandlerHelper.java
View file
Open in desktop
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
12 changes: 9 additions & 3 deletions vaadin-dev-server/src/main/java/com/vaadin/base/devserver/DevModeHandlerImpl.java
View file
Open in desktop
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to JavaDoc, the recommended way of dealing with encoding is to use new URI(...).toURL():

Note, the java.net.URI class does perform escaping of its
component fields in certain circumstances. The recommended way
to manage the encoding and decoding of URLs is to use java.net.URI,
and to convert between these two classes using toURI() and
toURL().

Copy link
Contributor Author

@pleku pleku May 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would make sense, yes. But the code needs more cleanup to be able to do this properly.
#11117

HttpURLConnection connection = (HttpURLConnection) uri.openConnection();
connection.setRequestMethod(method);
Expand Down
40 changes: 39 additions & 1 deletion vaadin-dev-server/src/test/java/com/vaadin/base/devserver/DevModeHandlerImplTest.java
View file
Open in desktop
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