From 65df38d371ed0205759dbb5a147a783b5c19e40f Mon Sep 17 00:00:00 2001 From: markt Date: Tue, 24 Aug 2010 09:30:11 +0000 Subject: [PATCH] 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 --- .../catalina/filters/CsrfPreventionFilter.java | 17 ++--- .../catalina/filters/TestCsrfPreventionFilter.java | 76 ++++++++++++++++++++++ webapps/docs/changelog.xml | 4 ++ 3 files changed, 89 insertions(+), 8 deletions(-) create mode 100644 test/org/apache/catalina/filters/TestCsrfPreventionFilter.java diff --git a/java/org/apache/catalina/filters/CsrfPreventionFilter.java b/java/org/apache/catalina/filters/CsrfPreventionFilter.java index 44a4d21fa..7151907ac 100644 --- 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 index 000000000..8ab654c3c --- /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 66c56fbae..4f5c38fa0 100644 --- 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) + + CSRF prevention filter did not correctly handle URLs that used anchors. + (markt) + -- 2.11.0