Avoid adding multiple CSRF tokens to a URL (#923)

* Avoid adding multiple CSRF tokens to a URL

* Add javadoc

* Handle special case where query-string contains a ?

Author: ChristopherSchultz

java/org/apache/catalina/filters/CsrfPreventionFilter.java

@@ -552,6 +552,8 @@ public String encodeRedirectUrl(String url) {
 
         @Override
         public String encodeRedirectURL(String url) {
+            url = removeQueryParameters(url, nonceRequestParameterName);
+
             if (shouldAddNonce(url)) {
                 return addNonce(super.encodeRedirectURL(url));
             } else {
@@ -567,6 +569,8 @@ public String encodeUrl(String url) {
 
         @Override
         public String encodeURL(String url) {
+            url = removeQueryParameters(url, nonceRequestParameterName);
+
             if (shouldAddNonce(url)) {
                 return addNonce(super.encodeURL(url));
             } else {
@@ -588,6 +592,85 @@ private boolean shouldAddNonce(String url) {
             return true;
         }
 
+        /**
+         * Removes zero or more query parameters from a URL. All instances of the query parameter and any associated
+         * values will be removed.
+         *
+         * @param url           The URL whose query parameters should be removed.
+         * @param parameterName The name of the parameter to remove.
+         *
+         * @return The URL without any instances of the query parameter <code>parameterName</code> present.
+         */
+        public static String removeQueryParameters(String url, String parameterName) {
+            if (null != parameterName) {
+                // Check for query string
+                int q = url.indexOf('?');
+                if (q > -1) {
+
+                    // Look for parameter end
+                    int start = q + 1;
+                    int pos = url.indexOf('&', start);
+
+                    int iterations = 0;
+
+                    while (-1 < pos) {
+                        // Process all parameters
+                        if (++iterations > 100000) {
+                            // Just in case things get out of control
+                            throw new IllegalStateException("Way too many loop iterations");
+                        }
+
+                        int eq = url.indexOf('=', start);
+                        int paramNameEnd;
+                        if (-1 == eq || eq > pos) {
+                            paramNameEnd = pos;
+                            // Found no equal sign at all or past next & ; Parameter is all name.
+                        } else {
+                            // Found this param's equal sign
+                            paramNameEnd = eq;
+                        }
+                        if (parameterName.equals(url.substring(start, paramNameEnd))) {
+                            // Remove the parameter
+                            url = url.substring(0, start) + url.substring(pos + 1); // +1 to consume the &
+                        } else {
+                            start = pos + 1; // Go to next parameter
+                        }
+                        pos = url.indexOf('&', start);
+                    }
+
+                    // Check final parameter
+                    String paramName;
+                    pos = url.indexOf('=', start);
+
+                    if (-1 < pos) {
+                        paramName = url.substring(start, pos);
+                    } else {
+                        paramName = url.substring(start);
+                        // No value
+                    }
+                    if (paramName.equals(parameterName)) {
+                        // Remove this parameter
+
+                        // Remove any trailing ? or & as well
+                        char c = url.charAt(start - 1);
+                        if ('?' == c || '&' == c) {
+                            start--;
+                        }
+
+                        url = url.substring(0, start);
+                    } else {
+                        // Remove trailing ? if it's there. Is this worth it?
+                        int length = url.length();
+                        if (length == q + 1) {
+                            url = url.substring(0, length - 1);
+                        }
+                    }
+                }
+            }
+
+            return url;
+        }
+
         /*
          * Return the specified URL with the nonce added to the query string.
          *

test/org/apache/catalina/filters/TestCsrfPreventionFilter.java

@@ -25,6 +25,8 @@
 import java.util.Collections;
 import java.util.Iterator;
 import java.util.function.Predicate;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 import javax.servlet.http.HttpServletResponse;
 
@@ -196,6 +198,106 @@ public void testNoNonceMimeMatcher() {
         Assert.assertEquals("/foo/images/home.jpg", response.encodeURL("/foo/images/home.jpg"));
     }
 
+    @Test
+    public void testMultipleTokens() {
+        String nonce = "TESTNONCE";
+        String testURL = "/foo/bar?" + Constants.CSRF_NONCE_SESSION_ATTR_NAME + "=sample";
+        CsrfPreventionFilter.CsrfResponseWrapper response = new CsrfPreventionFilter.CsrfResponseWrapper(new NonEncodingResponse(),
+                Constants.CSRF_NONCE_SESSION_ATTR_NAME, nonce, null);
+
+        Assert.assertTrue("Original URL does not contain CSRF token",
+                testURL.contains(Constants.CSRF_NONCE_SESSION_ATTR_NAME));
+
+        String result = response.encodeURL(testURL);
+
+        int pos = result.indexOf(Constants.CSRF_NONCE_SESSION_ATTR_NAME);
+        Assert.assertTrue("Result URL does not contain CSRF token",
+                pos >= 0);
+        pos = result.indexOf(Constants.CSRF_NONCE_SESSION_ATTR_NAME, pos + 1);
+        Assert.assertFalse("Result URL contains multiple CSRF tokens: " + result,
+                pos >= 0);
+    }
+
+    @Test
+    public void testURLCleansing() {
+        String[] urls = new String[] {
+                "/foo/bar",
+                "/foo/bar?",
+                "/foo/bar?csrf",
+                "/foo/bar?csrf&",
+                "/foo/bar?csrf=",
+                "/foo/bar?csrf=&",
+                "/foo/bar?csrf=abc",
+                "/foo/bar?csrf=abc&bar=foo",
+                "/foo/bar?bar=foo&csrf=abc",
+                "/foo/bar?bar=foo&csrf=abc&foo=bar",
+                "/foo/bar?csrfx=foil&bar=foo&csrf=abc&foo=bar",
+                "/foo/bar?csrfx=foil&bar=foo&csrf=abc&foo=bar&csrf=def",
+                "/foo/bar?csrf=&csrf&csrf&csrf&csrf=abc&csrf=",
+                "/foo/bar?xcsrf=&xcsrf&xcsrf&xcsrf&xcsrf=abc&xcsrf=",
+                "/foo/bar?xcsrf=&xcsrf&xcsrf&csrf=foo&xcsrf&xcsrf=abc&csrf=bar&xcsrf=&",
+                "/foo/bar?bar=fo?&csrf=abc",
+                "/foo/bar?bar=fo?&csrf=abc&baz=boh",
+        };
+
+        String csrfParameterName = "csrf";
+
+        for(String url : urls) {
+            String result = CsrfPreventionFilter.CsrfResponseWrapper.removeQueryParameters(url, csrfParameterName);
+
+            Assert.assertEquals("Failed to cleanse URL '" + url + "' properly", dumbButAccurateCleanse(url, csrfParameterName), result);
+        }
+
+    }
+
+    private static String dumbButAccurateCleanse(String url, String csrfParameterName) {
+        // Get rid of [&csrf=value] with optional =value
+        Pattern p = Pattern.compile(Pattern.quote("&") + Pattern.quote(csrfParameterName) + "(=[^&]*)?(&|$)");
+
+        // Do this iteratively to get everything
+        Matcher m = p.matcher(url);
+
+        while (m.find()) {
+            url = m.replaceFirst("$2");
+            m = p.matcher(url);
+        }
+
+        // Get rid of [?csrf=value] with optional =value
+        url = url.replaceAll(Pattern.quote("?") + csrfParameterName + "(=[^&]*)?(&|$)", "?");
+
+        if (url.endsWith("?")) {
+            // Special case: it's possible to have multiple ? in a URL:
+            // the query-string is technically allowed to contain ? characters.
+            // Only trim the trailing ? if it is actually the quest-string
+            // separator.
+            if(url.lastIndexOf('?', url.length() - 2) < 0) {
+                url = url.substring(0, url.length() - 1);
+            }
+        }
+
+        return url;
+    }
+
+    @Test
+    public void testDuplicateTokens() {
+        String nonce = "TESTNONCE";
+        String testURL = "/foo/bar?" + Constants.CSRF_NONCE_SESSION_ATTR_NAME + "=" + nonce;
+        CsrfPreventionFilter.CsrfResponseWrapper response = new CsrfPreventionFilter.CsrfResponseWrapper(new NonEncodingResponse(),
+                Constants.CSRF_NONCE_SESSION_ATTR_NAME, nonce, null);
+
+        Assert.assertTrue("Original URL does not contain CSRF token",
+                testURL.contains(Constants.CSRF_NONCE_SESSION_ATTR_NAME));
+
+        String result = response.encodeURL(testURL);
+
+        int pos = result.indexOf(Constants.CSRF_NONCE_SESSION_ATTR_NAME);
+        Assert.assertTrue("Result URL does not contain CSRF token",
+                pos >= 0);
+        pos = result.indexOf(Constants.CSRF_NONCE_SESSION_ATTR_NAME, pos + 1);
+        Assert.assertFalse("Result URL contains multiple CSRF tokens: " + result,
+                pos >= 0);
+    }
+
     private static class MimeTypeServletContext extends TesterServletContext {
         private String mimeType;
 

webapps/docs/changelog.xml

@@ -113,6 +113,10 @@
         failed when made from within a web application with resource caching
         enabled if the target resource was packaged in a JAR file. (markt)
       </fix>
+      <fix>
+        Pull request <pr>923</pr>: Avoid adding multiple CSRF tokens to a URL in
+        the <code>CsrfPreventionFilter</code>. (schultz)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Coyote">