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.

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

feat: check custom header for dev tools connection verification #18827

Merged
merged 6 commits into from
Mar 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,15 @@ public class InitParameters implements Serializable {
*/
public static final String SERVLET_PARAMETER_DEVMODE_HOSTS_ALLOWED = "devmode.hostsAllowed";

/**
* The name of the custom HTTP header that contains the client IP address
* that is checked to allow access to the dev mode server. The HTTP header
* is supposed to contain a single address, and the HTTP request to have a
* single occurrence of the header. If not specified, remote address are
* read from the {@literal X-Forwarded-For} header.
*/
public static final String SERVLET_PARAMETER_DEVMODE_REMOTE_ADDRESS_HEADER = "devmode.remoteAddressHeader";

/**
* Configuration parameter name for enabling pnpm.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,12 @@
import java.io.UncheckedIOException;
import java.net.InetAddress;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Enumeration;
import java.util.List;
import java.util.Locale;
import java.util.Optional;
import java.util.stream.Stream;

import org.apache.commons.io.FilenameUtils;
import org.jsoup.Jsoup;
Expand All @@ -49,7 +52,6 @@
import com.vaadin.flow.server.BootstrapHandler;
import com.vaadin.flow.server.Constants;
import com.vaadin.flow.server.DevToolsToken;
import com.vaadin.flow.server.InitParameters;
import com.vaadin.flow.server.Mode;
import com.vaadin.flow.server.VaadinContext;
import com.vaadin.flow.server.VaadinRequest;
Expand All @@ -67,6 +69,8 @@
import elemental.json.JsonObject;
import elemental.json.impl.JsonUtil;

import static com.vaadin.flow.server.InitParameters.SERVLET_PARAMETER_DEVMODE_HOSTS_ALLOWED;
import static com.vaadin.flow.server.InitParameters.SERVLET_PARAMETER_DEVMODE_REMOTE_ADDRESS_HEADER;
import static com.vaadin.flow.shared.ApplicationConstants.CONTENT_TYPE_TEXT_HTML_UTF_8;
import static java.nio.charset.StandardCharsets.UTF_8;

Expand Down Expand Up @@ -396,22 +400,47 @@ private void addDevTools(Document indexDocument,
static boolean isAllowedDevToolsHost(AbstractConfiguration configuration,
VaadinRequest request) {
String remoteAddress = request.getRemoteAddr();
String hostsAllowed = configuration.getStringProperty(
InitParameters.SERVLET_PARAMETER_DEVMODE_HOSTS_ALLOWED, null);
String hostsAllowedFromCfg = configuration.getStringProperty(
SERVLET_PARAMETER_DEVMODE_HOSTS_ALLOWED, null);
String hostsAllowed = (hostsAllowedFromCfg != null
&& !hostsAllowedFromCfg.isBlank()) ? hostsAllowedFromCfg : null;

if (!isAllowedDevToolsHost(remoteAddress, hostsAllowed)) {
if (!isAllowedDevToolsHost(remoteAddress, hostsAllowed, true)) {
return false;
}
String forwardedFor = request.getHeader("X-Forwarded-For");
if (forwardedFor != null) {
String remoteHeaderIp = configuration.getStringProperty(
SERVLET_PARAMETER_DEVMODE_REMOTE_ADDRESS_HEADER, null);
Copy link
Member

Choose a reason for hiding this comment

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

Are we assuming this is a single header with a single address? Should it be in the docs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the assumption is a single header with a single address, as this is supposed to be safely handled by the proxy server.
I'll update the docs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

if (remoteHeaderIp != null) {
return isAllowedDevToolsHost(request.getHeader(remoteHeaderIp),
hostsAllowed, false);
}

Enumeration<String> allForwardedForHeaders = request
.getHeaders("X-Forwarded-For");
if (allForwardedForHeaders != null
&& allForwardedForHeaders.hasMoreElements()) {
String forwardedFor = String.join(",",
Collections.list(allForwardedForHeaders));

if (forwardedFor.contains(",")) {
// X-Forwarded-For: <client>, <proxy1>, <proxy2>
// Elements are comma-separated, with optional whitespace
// surrounding the commas.
forwardedFor = forwardedFor.split(",")[0];
}
if (!isAllowedDevToolsHost(forwardedFor.trim(), hostsAllowed)) {
return false;
// Validate all hops
String[] hops = forwardedFor.split(",");
if (hops.length > 0) {
return Stream.of(hops).map(String::trim)
.allMatch(ip -> isAllowedDevToolsHost(ip,
hostsAllowed, false));
} else {
// Potential fake header with no addresses, e.g.
// 'X-Forwarded-For: ,,,'
return false;
}

} else {
return isAllowedDevToolsHost(forwardedFor.trim(), hostsAllowed,
false);
}
}

Expand All @@ -420,16 +449,18 @@ static boolean isAllowedDevToolsHost(AbstractConfiguration configuration,
}

private static boolean isAllowedDevToolsHost(String remoteAddress,
String hostsAllowed) {
if (remoteAddress == null) {
// No remote address available so we cannot check...
String hostsAllowed, boolean allowLocal) {
if (remoteAddress == null || remoteAddress.isBlank()
|| (hostsAllowed == null && !allowLocal)) {
Copy link
Member

Choose a reason for hiding this comment

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

this interprets hostsAllowed == "" different from null while the code below interprets them as the same

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it ok to have the same behavior for null, empty and blank?
If so, I would directly trim to null the value read from the configuration

Copy link
Member

Choose a reason for hiding this comment

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

I would say it is better to have the same behavior and fewer cases

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

// No check needed if the remote address is not available
// or if local addresses must be rejected and there are no host
// rules defined
return false;
}
// Always allow localhost
try {
InetAddress inetAddress = InetAddress.getByName(remoteAddress);
if (inetAddress.isLoopbackAddress()) {
return true;
return allowLocal;
}
} catch (Exception e) {
getLogger().debug(
Expand Down
Loading
Loading