This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new 569316517b Improve encoding of URLs with CSRF nonce.
569316517b is described below
commit 569316517b25ffd915a135b15e0e42b0062d3cc8
Author: Mark Thomas <[email protected]>
AuthorDate: Mon Jun 1 19:38:14 2026 +0100
Improve encoding of URLs with CSRF nonce.
This isn't perfect but is better than it was. The issue with context
path matching has always existed (and for session tokens too).
---
java/org/apache/catalina/connector/Response.java | 42 ++------------
.../catalina/filters/CsrfPreventionFilter.java | 47 ++++++++++++----
java/org/apache/catalina/util/RequestUtil.java | 51 +++++++++++++++++
.../catalina/filters/TestCsrfPreventionFilter.java | 64 +++++++++-------------
test/org/apache/tomcat/unittest/TesterContext.java | 22 ++++----
webapps/docs/changelog.xml | 4 ++
6 files changed, 133 insertions(+), 97 deletions(-)
diff --git a/java/org/apache/catalina/connector/Response.java
b/java/org/apache/catalina/connector/Response.java
index bfe8f9e36a..9c1d85c8d8 100644
--- a/java/org/apache/catalina/connector/Response.java
+++ b/java/org/apache/catalina/connector/Response.java
@@ -52,6 +52,7 @@ import javax.servlet.http.HttpServletResponseWrapper;
import org.apache.catalina.Context;
import org.apache.catalina.Session;
import org.apache.catalina.security.SecurityUtil;
+import org.apache.catalina.util.RequestUtil;
import org.apache.catalina.util.SessionConfig;
import org.apache.coyote.ActionCode;
import org.apache.coyote.ContinueResponseTiming;
@@ -1502,46 +1503,13 @@ public class Response implements HttpServletResponse {
return false;
}
- // Does this URL match down to (and including) the context path?
- if (!hreq.getScheme().equalsIgnoreCase(url.getProtocol())) {
+ if (!RequestUtil.isSameWebApplication(hreq, url)) {
return false;
}
- if (!hreq.getServerName().equalsIgnoreCase(url.getHost())) {
- return false;
- }
- int serverPort = hreq.getServerPort();
- if (serverPort == -1) {
- if ("https".equals(hreq.getScheme())) {
- serverPort = 443;
- } else {
- serverPort = 80;
- }
- }
- int urlPort = url.getPort();
- if (urlPort == -1) {
- if ("https".equals(url.getProtocol())) {
- urlPort = 443;
- } else {
- urlPort = 80;
- }
- }
- if (serverPort != urlPort) {
- return false;
- }
-
- String contextPath = context.getPath();
- if (contextPath != null) {
- String file = url.getFile();
- if (!file.startsWith(contextPath)) {
- return false;
- }
- String tok = ";" + SessionConfig.getSessionUriParamName(context) +
"=" + session.getIdInternal();
- return file.indexOf(tok, contextPath.length()) < 0;
- }
-
- // This URL belongs to our web application, so it is encodeable
- return true;
+ // Don't encode if the correct session ID is already present
+ String tok = ";" + SessionConfig.getSessionUriParamName(context) + "="
+ session.getIdInternal();
+ return location.indexOf(tok) < 0;
}
diff --git a/java/org/apache/catalina/filters/CsrfPreventionFilter.java
b/java/org/apache/catalina/filters/CsrfPreventionFilter.java
index d43712fcdb..e95ef40ffb 100644
--- a/java/org/apache/catalina/filters/CsrfPreventionFilter.java
+++ b/java/org/apache/catalina/filters/CsrfPreventionFilter.java
@@ -18,6 +18,10 @@ package org.apache.catalina.filters;
import java.io.IOException;
import java.io.Serializable;
+import java.net.MalformedURLException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.net.URL;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
@@ -39,6 +43,7 @@ import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpServletResponseWrapper;
import javax.servlet.http.HttpSession;
+import org.apache.catalina.util.RequestUtil;
import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
import org.apache.tomcat.util.http.Method;
@@ -456,7 +461,7 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
// requiring the use of response.encodeURL.
request.setAttribute(Constants.CSRF_NONCE_REQUEST_ATTR_NAME,
newNonce);
- wResponse = new CsrfResponseWrapper(res,
nonceRequestParameterName, newNonce, noNoncePredicates);
+ wResponse = new CsrfResponseWrapper(req, res,
nonceRequestParameterName, newNonce, noNoncePredicates);
}
}
@@ -574,6 +579,7 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
*/
protected static class CsrfResponseWrapper extends
HttpServletResponseWrapper {
+ private final HttpServletRequest request;
private final String nonceRequestParameterName;
private final String nonce;
private final Collection<Predicate<String>> noNoncePatterns;
@@ -581,14 +587,16 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
/**
* Construct a new CsrfResponseWrapper.
*
+ * @param request The associated request
* @param response The wrapped response
* @param nonceRequestParameterName The name of the nonce request
parameter
* @param nonce The current nonce value
* @param noNoncePatterns The patterns for URLs that should
not have nonces added
*/
- public CsrfResponseWrapper(HttpServletResponse response, String
nonceRequestParameterName, String nonce,
- Collection<Predicate<String>> noNoncePatterns) {
+ public CsrfResponseWrapper(HttpServletRequest request,
HttpServletResponse response,
+ String nonceRequestParameterName, String nonce,
Collection<Predicate<String>> noNoncePatterns) {
super(response);
+ this.request = request;
this.nonceRequestParameterName = nonceRequestParameterName;
this.nonce = nonce;
this.noNoncePatterns = noNoncePatterns;
@@ -602,6 +610,10 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
@Override
public String encodeRedirectURL(String url) {
+ if (url == null) {
+ return url;
+ }
+
url = removeQueryParameters(url, nonceRequestParameterName);
if (shouldAddNonce(url)) {
@@ -619,6 +631,10 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
@Override
public String encodeURL(String url) {
+ if (url == null) {
+ return url;
+ }
+
url = removeQueryParameters(url, nonceRequestParameterName);
if (shouldAddNonce(url)) {
@@ -628,18 +644,27 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
}
}
- private boolean shouldAddNonce(String url) {
- if (null == noNoncePatterns || noNoncePatterns.isEmpty()) {
- return true;
+ private boolean shouldAddNonce(String location) {
+ if (null != noNoncePatterns && !noNoncePatterns.isEmpty()) {
+ for (Predicate<String> p : noNoncePatterns) {
+ if (p.test(location)) {
+ return false;
+ }
+ }
}
- for (Predicate<String> p : noNoncePatterns) {
- if (p.test(url)) {
- return false;
- }
+ URL urlLocation;
+ try {
+ URI locationUri = new URI(location);
+ URI requestUri = new URI(request.getRequestURL().toString());
+ locationUri = requestUri.resolve(locationUri);
+ urlLocation = locationUri.toURL();
+ } catch (MalformedURLException | URISyntaxException |
IllegalArgumentException e) {
+ // Invalid location - don't try and add nonce
+ return false;
}
- return true;
+ return RequestUtil.isSameWebApplication(request, urlLocation);
}
/**
diff --git a/java/org/apache/catalina/util/RequestUtil.java
b/java/org/apache/catalina/util/RequestUtil.java
index 9f70a4b790..f5047715ca 100644
--- a/java/org/apache/catalina/util/RequestUtil.java
+++ b/java/org/apache/catalina/util/RequestUtil.java
@@ -16,6 +16,8 @@
*/
package org.apache.catalina.util;
+import java.net.URL;
+
import javax.servlet.http.HttpServletRequest;
import org.apache.catalina.connector.Request;
@@ -105,4 +107,53 @@ public final class RequestUtil {
return sb.toString();
}
+
+
+ /**
+ * Tests whether the provided URL is for a resource contained within the
same web application as the request.
+ *
+ * @param request The request to test
+ * @param url The URL to test
+ *
+ * @return {@code true} if the provided URL is for a resource contained
within the same web application as the
+ * request, otherwise {@code false}
+ */
+ public static boolean isSameWebApplication(HttpServletRequest request, URL
url) {
+ // Does this URL match down to (and including) the context path?
+ if (!request.getScheme().equalsIgnoreCase(url.getProtocol())) {
+ return false;
+ }
+ if (!request.getServerName().equalsIgnoreCase(url.getHost())) {
+ return false;
+ }
+ int serverPort = request.getServerPort();
+ if (serverPort == -1) {
+ if ("https".equals(request.getScheme())) {
+ serverPort = 443;
+ } else {
+ serverPort = 80;
+ }
+ }
+ int urlPort = url.getPort();
+ if (urlPort == -1) {
+ if ("https".equals(url.getProtocol())) {
+ urlPort = 443;
+ } else {
+ urlPort = 80;
+ }
+ }
+ if (serverPort != urlPort) {
+ return false;
+ }
+
+ /*
+ * This isn't perfect but is the best that can be done without running
the full mapping logic on the url to
+ * determine which web application that url will map to.
+ */
+ if
(!url.getPath().startsWith(request.getServletContext().getContextPath())) {
+ return false;
+ }
+
+ return true;
+ }
}
diff --git a/test/org/apache/catalina/filters/TestCsrfPreventionFilter.java
b/test/org/apache/catalina/filters/TestCsrfPreventionFilter.java
index 925cdb42b5..ec4e519e07 100644
--- a/test/org/apache/catalina/filters/TestCsrfPreventionFilter.java
+++ b/test/org/apache/catalina/filters/TestCsrfPreventionFilter.java
@@ -35,14 +35,15 @@ import org.junit.Test;
import org.apache.catalina.filters.CsrfPreventionFilter.LruCache;
import org.apache.catalina.startup.TomcatBaseTest;
+import org.apache.tomcat.unittest.TesterRequest;
import org.apache.tomcat.unittest.TesterServletContext;
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(),
- Constants.CSRF_NONCE_SESSION_ATTR_NAME, "TESTNONCE", null);
+ private final HttpServletResponse wrapper = new
CsrfPreventionFilter.CsrfResponseWrapper(new TesterRequest(),
+ new NonEncodingResponse(), Constants.CSRF_NONCE_SESSION_ATTR_NAME,
"TESTNONCE", null);
@Test
public void testAddNonceNoQueryNoAnchor() throws Exception {
@@ -149,8 +150,8 @@ public class TestCsrfPreventionFilter extends
TomcatBaseTest {
chain.add(suffix);
chain.add(regex);
- HttpServletResponse response = new
CsrfPreventionFilter.CsrfResponseWrapper(new NonEncodingResponse(),
- Constants.CSRF_NONCE_SESSION_ATTR_NAME, "TESTNONCE", chain);
+ HttpServletResponse response = new
CsrfPreventionFilter.CsrfResponseWrapper(new TesterRequest(),
+ new NonEncodingResponse(),
Constants.CSRF_NONCE_SESSION_ATTR_NAME, "TESTNONCE", chain);
// These URLs should include nonces
Assert.assertEquals("/foo?" + RESULT_NONCE,
response.encodeURL("/foo"));
@@ -177,8 +178,8 @@ public class TestCsrfPreventionFilter extends
TomcatBaseTest {
Assert.assertFalse("MIME match succeeds where it should fail",
mime.test("/test.txt"));
Collection<Predicate<String>> chain = Collections.singleton(mime);
- HttpServletResponse response = new
CsrfPreventionFilter.CsrfResponseWrapper(new NonEncodingResponse(),
- Constants.CSRF_NONCE_SESSION_ATTR_NAME, "TESTNONCE", chain);
+ HttpServletResponse response = new
CsrfPreventionFilter.CsrfResponseWrapper(new TesterRequest(),
+ new NonEncodingResponse(),
Constants.CSRF_NONCE_SESSION_ATTR_NAME, "TESTNONCE", chain);
// These URLs should include nonces
Assert.assertEquals("/foo?" + RESULT_NONCE,
response.encodeURL("/foo"));
@@ -202,8 +203,8 @@ public class TestCsrfPreventionFilter extends
TomcatBaseTest {
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);
+ CsrfPreventionFilter.CsrfResponseWrapper response = new
CsrfPreventionFilter.CsrfResponseWrapper(
+ new TesterRequest(), 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));
@@ -211,41 +212,28 @@ public class TestCsrfPreventionFilter extends
TomcatBaseTest {
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);
+ 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);
+ 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=",
+ 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",
- };
+
"/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) {
+ for (String url : urls) {
String result =
CsrfPreventionFilter.CsrfResponseWrapper.removeQueryParameters(url,
csrfParameterName);
- Assert.assertEquals("Failed to cleanse URL '" + url + "'
properly", dumbButAccurateCleanse(url, csrfParameterName), result);
+ Assert.assertEquals("Failed to cleanse URL '" + url + "' properly",
+ dumbButAccurateCleanse(url, csrfParameterName), result);
}
}
@@ -270,7 +258,7 @@ public class TestCsrfPreventionFilter extends
TomcatBaseTest {
// 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) {
+ if (url.lastIndexOf('?', url.length() - 2) < 0) {
url = url.substring(0, url.length() - 1);
}
}
@@ -282,8 +270,8 @@ public class TestCsrfPreventionFilter extends
TomcatBaseTest {
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);
+ CsrfPreventionFilter.CsrfResponseWrapper response = new
CsrfPreventionFilter.CsrfResponseWrapper(
+ new TesterRequest(), 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));
@@ -291,11 +279,9 @@ public class TestCsrfPreventionFilter extends
TomcatBaseTest {
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);
+ 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);
+ Assert.assertFalse("Result URL contains multiple CSRF tokens: " +
result, pos >= 0);
}
private static class MimeTypeServletContext extends TesterServletContext {
diff --git a/test/org/apache/tomcat/unittest/TesterContext.java
b/test/org/apache/tomcat/unittest/TesterContext.java
index a62da150f7..d24ebf5184 100644
--- a/test/org/apache/tomcat/unittest/TesterContext.java
+++ b/test/org/apache/tomcat/unittest/TesterContext.java
@@ -110,6 +110,18 @@ public class TesterContext implements Context {
}
+ private String path = "";
+ @Override
+ public String getPath() {
+ return path;
+ }
+
+ @Override
+ public void setPath(String path) {
+ this.path = path;
+ }
+
+
@Override
public Log getLogger() {
return log;
@@ -540,16 +552,6 @@ public class TesterContext implements Context {
// NO-OP
}
- @Override
- public String getPath() {
- return null;
- }
-
- @Override
- public void setPath(String path) {
- // NO-OP
- }
-
@Override
public String getPublicId() {
return null;
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 1fe830e7ae..6868b6372f 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -219,6 +219,10 @@
Reset the encoding used for query string parameters between requests in
case an application changed the encoding in a previous request. (markt)
</fix>
+ <fix>
+ When encoding URLs with the <code>CsrfPreventionFilter</code>, don't
add
+ the nonce to URLs that are known not to require it. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Coyote">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
