Correctly handle the case when the request passes through one or more trustedProxies but no internalProxies.
Based on a patch by zhanhb
git-svn-id: https://svn.apache.org/repos/asf/tomcat/tc8.0.x/trunk@1832884 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/java/org/apache/catalina/filters/RemoteIpFilter.java b/java/org/apache/catalina/filters/RemoteIpFilter.java
index df8580d..2322faa 100644
--- a/java/org/apache/catalina/filters/RemoteIpFilter.java
+++ b/java/org/apache/catalina/filters/RemoteIpFilter.java
@@ -65,7 +65,8 @@
* This servlet filter proceeds as follows:
* </p>
* <p>
- * If the incoming <code>request.getRemoteAddr()</code> matches the servlet filter's list of internal proxies :
+ * If the incoming <code>request.getRemoteAddr()</code> matches the servlet
+ * filter's list of internal or trusted proxies:
* </p>
* <ul>
* <li>Loop on the comma delimited list of IPs and hostnames passed by the preceding load balancer or proxy in the given request's Http
@@ -761,8 +762,11 @@
public void doFilter(HttpServletRequest request, HttpServletResponse response, FilterChain chain) throws IOException, ServletException {
- if (internalProxies != null &&
- internalProxies.matcher(request.getRemoteAddr()).matches()) {
+ boolean isInternal = internalProxies != null &&
+ internalProxies.matcher(request.getRemoteAddr()).matches();
+
+ if (isInternal || (trustedProxies != null &&
+ trustedProxies.matcher(request.getRemoteAddr()).matches())) {
String remoteIp = null;
// In java 6, proxiesHeaderValue should be declared as a java.util.Deque
LinkedList<String> proxiesHeaderValue = new LinkedList<>();
@@ -778,11 +782,14 @@
String[] remoteIpHeaderValue = commaDelimitedListToStringArray(concatRemoteIpHeaderValue.toString());
int idx;
+ if (!isInternal) {
+ proxiesHeaderValue.addFirst(request.getRemoteAddr());
+ }
// loop on remoteIpHeaderValue to find the first trusted remote ip and to build the proxies chain
for (idx = remoteIpHeaderValue.length - 1; idx >= 0; idx--) {
String currentRemoteIp = remoteIpHeaderValue[idx];
remoteIp = currentRemoteIp;
- if (internalProxies.matcher(currentRemoteIp).matches()) {
+ if (internalProxies !=null && internalProxies.matcher(currentRemoteIp).matches()) {
// do nothing, internalProxies IPs are not appended to the
} else if (trustedProxies != null &&
trustedProxies.matcher(currentRemoteIp).matches()) {
diff --git a/java/org/apache/catalina/valves/RemoteIpValve.java b/java/org/apache/catalina/valves/RemoteIpValve.java
index 15904c2..7df4b89 100644
--- a/java/org/apache/catalina/valves/RemoteIpValve.java
+++ b/java/org/apache/catalina/valves/RemoteIpValve.java
@@ -47,7 +47,8 @@
* This valve proceeds as follows:
* </p>
* <p>
- * If the incoming <code>request.getRemoteAddr()</code> matches the valve's list of internal proxies :
+ * If the incoming <code>request.getRemoteAddr()</code> matches the valve's list
+ * of internal or trusted proxies:
* </p>
* <ul>
* <li>Loop on the comma delimited list of IPs and hostnames passed by the preceding load balancer or proxy in the given request's Http
@@ -571,9 +572,11 @@
final int originalServerPort = request.getServerPort();
final String originalProxiesHeader = request.getHeader(proxiesHeader);
final String originalRemoteIpHeader = request.getHeader(remoteIpHeader);
+ boolean isInternal = internalProxies != null &&
+ internalProxies.matcher(originalRemoteAddr).matches();
- if (internalProxies !=null &&
- internalProxies.matcher(originalRemoteAddr).matches()) {
+ if (isInternal || (trustedProxies != null &&
+ trustedProxies.matcher(originalRemoteAddr).matches())) {
String remoteIp = null;
// In java 6, proxiesHeaderValue should be declared as a java.util.Deque
LinkedList<String> proxiesHeaderValue = new LinkedList<>();
@@ -589,11 +592,14 @@
String[] remoteIpHeaderValue = commaDelimitedListToStringArray(concatRemoteIpHeaderValue.toString());
int idx;
+ if (!isInternal) {
+ proxiesHeaderValue.addFirst(originalRemoteAddr);
+ }
// loop on remoteIpHeaderValue to find the first trusted remote ip and to build the proxies chain
for (idx = remoteIpHeaderValue.length - 1; idx >= 0; idx--) {
String currentRemoteIp = remoteIpHeaderValue[idx];
remoteIp = currentRemoteIp;
- if (internalProxies.matcher(currentRemoteIp).matches()) {
+ if (internalProxies !=null && internalProxies.matcher(currentRemoteIp).matches()) {
// do nothing, internalProxies IPs are not appended to the
} else if (trustedProxies != null &&
trustedProxies.matcher(currentRemoteIp).matches()) {
diff --git a/test/org/apache/catalina/filters/TestRemoteIpFilter.java b/test/org/apache/catalina/filters/TestRemoteIpFilter.java
index d2bf2b8..acfc881 100644
--- a/test/org/apache/catalina/filters/TestRemoteIpFilter.java
+++ b/test/org/apache/catalina/filters/TestRemoteIpFilter.java
@@ -349,6 +349,75 @@
}
@Test
+ public void testInvokeAllProxiesAreTrustedEmptyInternal() throws Exception {
+
+ // PREPARE
+ RemoteIpFilter remoteIpFilter = new RemoteIpFilter();
+ FilterDef filterDef = new FilterDef();
+ filterDef.addInitParameter("internalProxies", "");
+ filterDef.addInitParameter("trustedProxies", "proxy1|proxy2|proxy3");
+ filterDef.addInitParameter("remoteIpHeader", "x-forwarded-for");
+ filterDef.addInitParameter("proxiesHeader", "x-forwarded-by");
+
+ filterDef.setFilter(remoteIpFilter);
+ MockHttpServletRequest request = new MockHttpServletRequest();
+
+ request.setRemoteAddr("proxy3");
+ request.setRemoteHost("remote-host-original-value");
+ request.setHeader("x-forwarded-for", "140.211.11.130, proxy1, proxy2");
+
+ // TEST
+ HttpServletRequest actualRequest = testRemoteIpFilter(filterDef, request).getRequest();
+
+ // VERIFY
+ String actualXForwardedFor = actualRequest.getHeader("x-forwarded-for");
+ Assert.assertNull("all proxies are trusted, x-forwarded-for must be null", actualXForwardedFor);
+
+ String actualXForwardedBy = actualRequest.getHeader("x-forwarded-by");
+ Assert.assertEquals("all proxies are trusted, they must appear in x-forwarded-by", "proxy1, proxy2, proxy3", actualXForwardedBy);
+
+ String actualRemoteAddr = actualRequest.getRemoteAddr();
+ Assert.assertEquals("remoteAddr", "140.211.11.130", actualRemoteAddr);
+
+ String actualRemoteHost = actualRequest.getRemoteHost();
+ Assert.assertEquals("remoteHost", "140.211.11.130", actualRemoteHost);
+ }
+
+ @Test
+ public void testInvokeAllProxiesAreTrustedUnusedInternal() throws Exception {
+
+ // PREPARE
+ RemoteIpFilter remoteIpFilter = new RemoteIpFilter();
+ FilterDef filterDef = new FilterDef();
+ filterDef.addInitParameter("trustedProxies", "proxy1|proxy2|proxy3");
+ filterDef.addInitParameter("remoteIpHeader", "x-forwarded-for");
+ filterDef.addInitParameter("proxiesHeader", "x-forwarded-by");
+
+ filterDef.setFilter(remoteIpFilter);
+ MockHttpServletRequest request = new MockHttpServletRequest();
+
+ request.setRemoteAddr("proxy3");
+ request.setRemoteHost("remote-host-original-value");
+ request.setHeader("x-forwarded-for", "140.211.11.130, proxy1, proxy2");
+
+ // TEST
+ HttpServletRequest actualRequest = testRemoteIpFilter(filterDef, request).getRequest();
+
+ // VERIFY
+ String actualXForwardedFor = actualRequest.getHeader("x-forwarded-for");
+ Assert.assertNull("all proxies are trusted, x-forwarded-for must be null", actualXForwardedFor);
+
+ String actualXForwardedBy = actualRequest.getHeader("x-forwarded-by");
+ Assert.assertEquals("all proxies are trusted, they must appear in x-forwarded-by", "proxy1, proxy2, proxy3", actualXForwardedBy);
+
+ String actualRemoteAddr = actualRequest.getRemoteAddr();
+ Assert.assertEquals("remoteAddr", "140.211.11.130", actualRemoteAddr);
+
+ String actualRemoteHost = actualRequest.getRemoteHost();
+ Assert.assertEquals("remoteHost", "140.211.11.130", actualRemoteHost);
+ }
+
+ @Test
public void testInvokeAllProxiesAreTrustedAndRemoteAddrMatchRegexp() throws Exception {
// PREPARE
diff --git a/test/org/apache/catalina/valves/TestRemoteIpValve.java b/test/org/apache/catalina/valves/TestRemoteIpValve.java
index 69dc6f8..92c8c80 100644
--- a/test/org/apache/catalina/valves/TestRemoteIpValve.java
+++ b/test/org/apache/catalina/valves/TestRemoteIpValve.java
@@ -199,6 +199,87 @@
}
@Test
+ public void testInvokeAllProxiesAreTrustedEmptyInternal() throws Exception {
+
+ // PREPARE
+ RemoteIpValve remoteIpValve = new RemoteIpValve();
+ remoteIpValve.setInternalProxies("");
+ remoteIpValve.setTrustedProxies("proxy1|proxy2|proxy3");
+ remoteIpValve.setRemoteIpHeader("x-forwarded-for");
+ remoteIpValve.setProxiesHeader("x-forwarded-by");
+ RemoteAddrAndHostTrackerValve remoteAddrAndHostTrackerValve = new RemoteAddrAndHostTrackerValve();
+ remoteIpValve.setNext(remoteAddrAndHostTrackerValve);
+
+ Request request = new MockRequest();
+ request.setCoyoteRequest(new org.apache.coyote.Request());
+ request.setRemoteAddr("proxy3");
+ request.setRemoteHost("remote-host-original-value");
+ request.getCoyoteRequest().getMimeHeaders().addValue("x-forwarded-for").setString("140.211.11.130, proxy1, proxy2");
+
+ // TEST
+ remoteIpValve.invoke(request, null);
+
+ // VERIFY
+ String actualXForwardedFor = remoteAddrAndHostTrackerValve.getForwardedFor();
+ Assert.assertNull("all proxies are trusted, x-forwarded-for must be null", actualXForwardedFor);
+
+ String actualXForwardedBy = remoteAddrAndHostTrackerValve.getForwardedBy();
+ Assert.assertEquals("all proxies are trusted, they must appear in x-forwarded-by", "proxy1, proxy2, proxy3", actualXForwardedBy);
+
+ String actualRemoteAddr = remoteAddrAndHostTrackerValve.getRemoteAddr();
+ Assert.assertEquals("remoteAddr", "140.211.11.130", actualRemoteAddr);
+
+ String actualRemoteHost = remoteAddrAndHostTrackerValve.getRemoteHost();
+ Assert.assertEquals("remoteHost", "140.211.11.130", actualRemoteHost);
+
+ String actualPostInvokeRemoteAddr = request.getRemoteAddr();
+ Assert.assertEquals("postInvoke remoteAddr", "proxy3", actualPostInvokeRemoteAddr);
+
+ String actualPostInvokeRemoteHost = request.getRemoteHost();
+ Assert.assertEquals("postInvoke remoteAddr", "remote-host-original-value", actualPostInvokeRemoteHost);
+ }
+
+ @Test
+ public void testInvokeAllProxiesAreTrustedUnusedInternal() throws Exception {
+
+ // PREPARE
+ RemoteIpValve remoteIpValve = new RemoteIpValve();
+ remoteIpValve.setTrustedProxies("proxy1|proxy2|proxy3");
+ remoteIpValve.setRemoteIpHeader("x-forwarded-for");
+ remoteIpValve.setProxiesHeader("x-forwarded-by");
+ RemoteAddrAndHostTrackerValve remoteAddrAndHostTrackerValve = new RemoteAddrAndHostTrackerValve();
+ remoteIpValve.setNext(remoteAddrAndHostTrackerValve);
+
+ Request request = new MockRequest();
+ request.setCoyoteRequest(new org.apache.coyote.Request());
+ request.setRemoteAddr("proxy3");
+ request.setRemoteHost("remote-host-original-value");
+ request.getCoyoteRequest().getMimeHeaders().addValue("x-forwarded-for").setString("140.211.11.130, proxy1, proxy2");
+
+ // TEST
+ remoteIpValve.invoke(request, null);
+
+ // VERIFY
+ String actualXForwardedFor = remoteAddrAndHostTrackerValve.getForwardedFor();
+ Assert.assertNull("all proxies are trusted, x-forwarded-for must be null", actualXForwardedFor);
+
+ String actualXForwardedBy = remoteAddrAndHostTrackerValve.getForwardedBy();
+ Assert.assertEquals("all proxies are trusted, they must appear in x-forwarded-by", "proxy1, proxy2, proxy3", actualXForwardedBy);
+
+ String actualRemoteAddr = remoteAddrAndHostTrackerValve.getRemoteAddr();
+ Assert.assertEquals("remoteAddr", "140.211.11.130", actualRemoteAddr);
+
+ String actualRemoteHost = remoteAddrAndHostTrackerValve.getRemoteHost();
+ Assert.assertEquals("remoteHost", "140.211.11.130", actualRemoteHost);
+
+ String actualPostInvokeRemoteAddr = request.getRemoteAddr();
+ Assert.assertEquals("postInvoke remoteAddr", "proxy3", actualPostInvokeRemoteAddr);
+
+ String actualPostInvokeRemoteHost = request.getRemoteHost();
+ Assert.assertEquals("postInvoke remoteAddr", "remote-host-original-value", actualPostInvokeRemoteHost);
+ }
+
+ @Test
public void testInvokeAllProxiesAreTrustedOrInternal() throws Exception {
// PREPARE
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index a152e4a..accf7a8 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -117,6 +117,12 @@
<code>internalProxies</code> regular expression. Patch by Craig Andrews.
(markt)
</add>
+ <fix>
+ In the <code>RemoteIpValve</code> and <code>RemoteIpFilter</code>,
+ correctly handle the case when the request passes through one or more
+ <code>trustedProxies</code> but no <code>internalProxies</code>. Based
+ on a patch by zhanhb. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Coyote">
