Correctly handle anchors in URLs with the CSRF prevention filter.

git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@988448 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/java/org/apache/catalina/filters/CsrfPreventionFilter.java b/java/org/apache/catalina/filters/CsrfPreventionFilter.java
index 44a4d21..7151907 100644 (file)
--- a/java/org/apache/catalina/filters/CsrfPreventionFilter.java
+++ b/java/org/apache/catalina/filters/CsrfPreventionFilter.java
@@ -128,6 +128,7 @@ public class CsrfPreventionFilter extends FilterBase {
         }
     }
 
+    @Override
     public void doFilter(ServletRequest request, ServletResponse response,
             FilterChain chain) throws IOException, ServletException {
 
@@ -215,7 +216,7 @@ public class CsrfPreventionFilter extends FilterBase {
         return buffer.toString();
     }
 
-    private static class CsrfResponseWrapper
+    protected static class CsrfResponseWrapper
             extends HttpServletResponseWrapper {
 
         private String nonce;
@@ -248,7 +249,7 @@ public class CsrfPreventionFilter extends FilterBase {
         }
         
         /**
-         * Return the specified URL with the nonce added to the query string
+         * Return the specified URL with the nonce added to the query string. 
          *
          * @param url URL to be modified
          * @param nonce The nonce to add
@@ -261,18 +262,17 @@ public class CsrfPreventionFilter extends FilterBase {
             String path = url;
             String query = "";
             String anchor = "";
-            int question = url.indexOf('?');
-            if (question >= 0) {
-                path = url.substring(0, question);
-                query = url.substring(question);
-            }
             int pound = path.indexOf('#');
             if (pound >= 0) {
                 anchor = path.substring(pound);
                 path = path.substring(0, pound);
             }
+            int question = path.indexOf('?');
+            if (question >= 0) {
+                query = path.substring(question);
+                path = path.substring(0, question);
+            }
             StringBuilder sb = new StringBuilder(path);
-            sb.append(anchor);
             if (query.length() >0) {
                 sb.append(query);
                 sb.append('&');
@@ -282,6 +282,7 @@ public class CsrfPreventionFilter extends FilterBase {
             sb.append(Constants.CSRF_NONCE_REQUEST_PARAM);
             sb.append('=');
             sb.append(nonce);
+            sb.append(anchor);
             return (sb.toString());
         }
     }

diff --git a/test/org/apache/catalina/filters/TestCsrfPreventionFilter.java b/test/org/apache/catalina/filters/TestCsrfPreventionFilter.java
new file mode 100644 (file)
index 0000000..8ab654c
--- /dev/null
+++ b/test/org/apache/catalina/filters/TestCsrfPreventionFilter.java
@@ -0,0 +1,76 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one or more
+ *  contributor license agreements.  See the NOTICE file distributed with
+ *  this work for additional information regarding copyright ownership.
+ *  The ASF licenses this file to You under the Apache License, Version 2.0
+ *  (the "License"); you may not use this file except in compliance with
+ *  the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ */
+
+package org.apache.catalina.filters;
+
+import javax.servlet.http.HttpServletResponse;
+
+import org.apache.catalina.core.DummyResponse;
+import org.apache.catalina.startup.TomcatBaseTest;
+
+public class TestCsrfPreventionFilter extends TomcatBaseTest {
+
+    private static final String RESULT_NONCE =
+        Constants.CSRF_NONCE_SESSION_ATTR_NAME + "=TESTNONCE";
+
+    private final HttpServletResponse wrapper =
+        new CsrfPreventionFilter.CsrfResponseWrapper(
+                new NonEncodingResponse(), "TESTNONCE");
+
+    public void testAddNonceNoQueryNoAnchor() throws Exception {
+        assertEquals("/test?" + RESULT_NONCE ,
+                wrapper.encodeRedirectURL("/test"));
+    }
+    
+    public void testAddNonceQueryNoAnchor() throws Exception {
+        assertEquals("/test?a=b&" + RESULT_NONCE ,
+                wrapper.encodeRedirectURL("/test?a=b"));
+    }
+    
+    public void testAddNonceNoQueryAnchor() throws Exception {
+        assertEquals("/test?" + RESULT_NONCE + "#c",
+                wrapper.encodeRedirectURL("/test#c"));
+    }
+    
+    public void testAddNonceQueryAnchor() throws Exception {
+        assertEquals("/test?a=b&" + RESULT_NONCE + "#c",
+                wrapper.encodeRedirectURL("/test?a=b#c"));
+    }
+    
+    private static class NonEncodingResponse extends DummyResponse {
+
+        @Override
+        public String encodeRedirectURL(String url) {
+            return url;
+        }
+
+        @Override
+        public String encodeRedirectUrl(String url) {
+            return url;
+        }
+
+        @Override
+        public String encodeURL(String url) {
+            return url;
+        }
+
+        @Override
+        public String encodeUrl(String url) {
+            return url;
+        }
+    }
+}

diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 66c56fb..4f5c38f 100644 (file)
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -75,6 +75,10 @@
         response by repeating the POST request including a request body. Any
         request body provided at this point is now swallowed. (markt)  
       </fix>
+      <fix>
+        CSRF prevention filter did not correctly handle URLs that used anchors.
+        (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Coyote">