Fix BZ 66471 - JSessionId secure attribute missing with RemoteIpFilte…

…r and X-Forwarded-Proto set to https https://bz.apache.org/bugzilla/show_bug.cgi?id=66471
apache · Feb 9, 2023 · c64d496 · c64d496
1 parent 2c2a1bd
commit c64d496
Show file tree

Hide file tree

Showing 4 changed files with 94 additions and 29 deletions.
diff --git a/java/org/apache/catalina/Globals.java b/java/org/apache/catalina/Globals.java
@@ -95,6 +95,13 @@ public final class Globals {
     public static final String SENDFILE_SUPPORTED_ATTR = org.apache.coyote.Constants.SENDFILE_SUPPORTED_ATTR;
 
 
+    /**
+     * The request attribute that is set to the value of {@code Boolean.TRUE}
+     * if {@code org.apache.catalina.filters.RemoteIpFilter } determines
+     * that this request was submitted via a secure channel.
+     */
+    public static final String REMOTE_IP_FILTER_SECURE = "org.apache.catalina.filters.RemoteIpFilter.secure";
+
     /**
      * The request attribute that can be used by a servlet to pass
      * to the connector the name of the file that is to be served

diff --git a/java/org/apache/catalina/connector/Request.java b/java/org/apache/catalina/connector/Request.java
@@ -3489,5 +3489,19 @@ public void set(Request request, String name, Object value) {
                         // NO-OP
                     }
                 });
+        specialAttributes.put(Globals.REMOTE_IP_FILTER_SECURE,
+            new SpecialAttributeAdapter() {
+                @Override
+                public Object get(Request request, String name) {
+                    return Boolean.valueOf(request.isSecure());
+                }
+
+                @Override
+                public void set(Request request, String name, Object value) {
+                    if (value instanceof Boolean) {
+                        request.setSecure(((Boolean) value).booleanValue());
+                    }
+                }
+            });
     }
 }
diff --git a/java/org/apache/catalina/filters/RemoteIpFilter.java b/java/org/apache/catalina/filters/RemoteIpFilter.java
@@ -584,11 +584,6 @@ public int getServerPort() {
             return serverPort;
         }
 
-        @Override
-        public boolean isSecure() {
-            return secure;
-        }
-
         public void removeHeader(String name) {
             Map.Entry<String, List<String>> header = getHeaderEntry(name);
             if (header != null) {
@@ -628,7 +623,7 @@ public void setScheme(String scheme) {
         }
 
         public void setSecure(boolean secure) {
-            this.secure = secure;
+            super.getRequest().setAttribute(Globals.REMOTE_IP_FILTER_SECURE, Boolean.valueOf(secure));
         }
 
         public void setServerName(String serverName) {

diff --git a/test/org/apache/catalina/filters/TestRemoteIpFilter.java b/test/org/apache/catalina/filters/TestRemoteIpFilter.java
@@ -82,15 +82,21 @@ public static class MockHttpServlet extends HttpServlet {
 
         private static final long serialVersionUID = 1L;
 
-        private transient HttpServletRequest request;
-
-        public HttpServletRequest getRequest() {
-            return request;
-        }
+        public String remoteAddr;
+        public String remoteHost;
+        public String scheme;
+        public String serverName;
+        public int serverPort;
+        public boolean isSecure;
 
         @Override
         public void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
-            this.request = request;
+            this.isSecure = request.isSecure();
+            this.remoteAddr = request.getRemoteAddr();
+            this.remoteHost = request.getRemoteHost();
+            this.scheme = request.getScheme();
+            this.serverName = request.getServerName();
+            this.serverPort = request.getServerPort();
             PrintWriter writer = response.getWriter();
 
             writer.println("request.remoteAddr=" + request.getRemoteAddr());
@@ -129,16 +135,6 @@ public void setScheme(String scheme) {
             getCoyoteRequest().scheme().setString(scheme);
         }
 
-        @Override
-        public void setAttribute(String name, Object value) {
-            getCoyoteRequest().getAttributes().put(name, value);
-        }
-
-        @Override
-        public Object getAttribute(String name) {
-            return getCoyoteRequest().getAttributes().get(name);
-        }
-
         @Override
         public String getServerName() {
             return "localhost";
@@ -770,16 +766,69 @@ public void testWithTomcatServer() throws Exception {
 
         // VALIDATE
         Assert.assertEquals(HttpURLConnection.HTTP_OK, httpURLConnection.getResponseCode());
-        HttpServletRequest request = mockServlet.getRequest();
-        Assert.assertNotNull(request);
 
         // VALIDATE X-FORWARDED-FOR
-        Assert.assertEquals(expectedRemoteAddr, request.getRemoteAddr());
-        Assert.assertEquals(expectedRemoteAddr, request.getRemoteHost());
+        Assert.assertEquals(expectedRemoteAddr, mockServlet.remoteAddr);
+        Assert.assertEquals(expectedRemoteAddr, mockServlet.remoteHost);
 
         // VALIDATE X-FORWARDED-PROTO
-        Assert.assertTrue(request.isSecure());
-        Assert.assertEquals("https", request.getScheme());
-        Assert.assertEquals(443, request.getServerPort());
+        Assert.assertTrue(mockServlet.isSecure);
+        Assert.assertEquals("https", mockServlet.scheme);
+        Assert.assertEquals(443, mockServlet.serverPort);
+    }
+
+    @Test
+    public void testJSessionIdSecureAttributeMissing() throws Exception {
+
+        // mostly default configuration : enable "x-forwarded-proto"
+        Map<String, String> remoteIpFilterParameter = new HashMap<>();
+        remoteIpFilterParameter.put("protocolHeader", "x-forwarded-proto");
+
+        // SETUP
+        Tomcat tomcat = getTomcatInstance();
+        Context root = tomcat.addContext("", TEMP_DIR);
+
+        FilterDef filterDef = new FilterDef();
+        filterDef.getParameterMap().putAll(remoteIpFilterParameter);
+        filterDef.setFilterClass(RemoteIpFilter.class.getName());
+        filterDef.setFilterName(RemoteIpFilter.class.getName());
+
+        root.addFilterDef(filterDef);
+
+        FilterMap filterMap = new FilterMap();
+        filterMap.setFilterName(RemoteIpFilter.class.getName());
+        filterMap.addURLPatternDecoded("*");
+        root.addFilterMap(filterMap);
+
+        Bug66471Servlet bug66471Servlet = new Bug66471Servlet();
+
+        Tomcat.addServlet(root, bug66471Servlet.getClass().getName(), bug66471Servlet);
+        root.addServletMappingDecoded("/test", bug66471Servlet.getClass().getName());
+
+        getTomcatInstance().start();
+
+        Map<String, List<String>> resHeaders = new HashMap<>();
+        Map<String, List<String>> reqHeaders = new HashMap<>();
+        String expectedRemoteAddr = "my-remote-addr";
+        List<String> forwardedFor = new ArrayList<>(1);
+        forwardedFor.add(expectedRemoteAddr);
+        List<String> forwardedProto = new ArrayList<>(1);
+        forwardedProto.add("https");
+        reqHeaders.put("x-forwarded-for", forwardedFor);
+        reqHeaders.put("x-forwarded-proto", forwardedProto);
+
+        getUrl("http://localhost:" + tomcat.getConnector().getLocalPort() +
+            "/test", null, reqHeaders, resHeaders);
+        String setCookie = resHeaders.get("Set-Cookie").get(0);
+        Assert.assertTrue(setCookie.contains("Secure"));
+        Assert.assertTrue(bug66471Servlet.isSecure.booleanValue());
+    }
+    public static class Bug66471Servlet extends HttpServlet {
+        public Boolean isSecure;
+        @Override
+        protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
+            req.getSession();
+            isSecure = (Boolean) req.getAttribute(Globals.REMOTE_IP_FILTER_SECURE);
+        }
     }
 }