This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 98187ffef4 Avoid adding multiple CSRF tokens to a URL (#923)
98187ffef4 is described below
commit 98187ffef4cec6043a602b500a2e3dfd5ef71c20
Author: Christopher Schultz <[email protected]>
AuthorDate: Wed Jan 7 12:18:19 2026 -0500
Avoid adding multiple CSRF tokens to a URL (#923)
* Avoid adding multiple CSRF tokens to a URL
* Add javadoc
* Whitespace police
* Process parameters in a more straightforward way
This fixes incorrect matching of parameters whose names *end with* the CSRF
token name
The code is arguably easier to read
* Code style whitespace ; no functional change
* Handle special case where query-string contains a ?
* Remove debugging System.outs
---
.../catalina/filters/CsrfPreventionFilter.java | 87 ++++++++++++++++++
.../catalina/filters/TestCsrfPreventionFilter.java | 102 +++++++++++++++++++++
2 files changed, 189 insertions(+)
diff --git a/java/org/apache/catalina/filters/CsrfPreventionFilter.java
b/java/org/apache/catalina/filters/CsrfPreventionFilter.java
index 8f131439ee..41bd83a8f2 100644
--- a/java/org/apache/catalina/filters/CsrfPreventionFilter.java
+++ b/java/org/apache/catalina/filters/CsrfPreventionFilter.java
@@ -544,6 +544,8 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
@Override
public String encodeRedirectURL(String url) {
+ url = removeQueryParameters(url, nonceRequestParameterName);
+
if (shouldAddNonce(url)) {
return addNonce(super.encodeRedirectURL(url));
} else {
@@ -553,6 +555,8 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
@Override
public String encodeURL(String url) {
+ url = removeQueryParameters(url, nonceRequestParameterName);
+
if (shouldAddNonce(url)) {
return addNonce(super.encodeURL(url));
} else {
@@ -574,6 +578,89 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
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.
*
diff --git a/test/org/apache/catalina/filters/TestCsrfPreventionFilter.java
b/test/org/apache/catalina/filters/TestCsrfPreventionFilter.java
index 8cf872dda8..d053552a2e 100644
--- a/test/org/apache/catalina/filters/TestCsrfPreventionFilter.java
+++ b/test/org/apache/catalina/filters/TestCsrfPreventionFilter.java
@@ -25,6 +25,8 @@ import java.util.Collection;
import java.util.Collections;
import java.util.Iterator;
import java.util.function.Predicate;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
import jakarta.servlet.http.HttpServletResponse;
@@ -196,6 +198,106 @@ public class TestCsrfPreventionFilter extends
TomcatBaseTest {
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;
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
