From 652b98f53c1eee2f7bd6f66d23da8425ea7fab53 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Tue, 24 May 2016 21:12:09 +0200 Subject: [PATCH] #793 added configuration parameter to enable/disable xsrf protection. The protection is disabled by default until it is battle tested. --- .../sonia/scm/config/ScmConfiguration.java | 37 +++++++++++++++++++ .../scm/security/XsrfProtectionFilter.java | 26 ++++++++++++- .../security/XsrfProtectionFilterTest.java | 32 +++++++++++++++- 3 files changed, 93 insertions(+), 2 deletions(-) diff --git a/scm-core/src/main/java/sonia/scm/config/ScmConfiguration.java b/scm-core/src/main/java/sonia/scm/config/ScmConfiguration.java index eebefe4e1c..9fea54289e 100644 --- a/scm-core/src/main/java/sonia/scm/config/ScmConfiguration.java +++ b/scm-core/src/main/java/sonia/scm/config/ScmConfiguration.java @@ -177,6 +177,7 @@ public class ScmConfiguration this.skipFailedAuthenticators = other.skipFailedAuthenticators; this.loginAttemptLimit = other.loginAttemptLimit; this.loginAttemptLimitTimeout = other.loginAttemptLimitTimeout; + this.enabledXsrfProtection = other.enabledXsrfProtection; // deprecated fields this.servername = other.servername; @@ -424,6 +425,19 @@ public class ScmConfiguration return disableGroupingGrid; } + /** + * Returns {@code true} if the cookie xsrf protection is enabled. + * + * @see Issue 793 + * @return {@code true} if the cookie xsrf protection is enabled + * + * @since 1.47 + */ + public boolean isEnabledXsrfProtection() + { + return enabledXsrfProtection; + } + /** * Returns true if port forwarding is enabled. * @@ -800,6 +814,21 @@ public class ScmConfiguration this.sslPort = sslPort; } + /** + * Set {@code true} to enable xsrf cookie protection. + * + * @param enabledXsrfProtection {@code true} to enable xsrf protection + * @see Issue 793 + * + * @since 1.47 + */ + public void setEnabledXsrfProtection(boolean enabledXsrfProtection) + { + this.enabledXsrfProtection = enabledXsrfProtection; + } + + + //~--- fields --------------------------------------------------------------- /** Field description */ @@ -913,4 +942,12 @@ public class ScmConfiguration /** Field description */ private boolean anonymousAccessEnabled = false; + + /** + * Enables xsrf cookie protection. + * + * @since 1.47 + */ + @XmlElement(name = "xsrf-protection") + private boolean enabledXsrfProtection = false; } diff --git a/scm-webapp/src/main/java/sonia/scm/security/XsrfProtectionFilter.java b/scm-webapp/src/main/java/sonia/scm/security/XsrfProtectionFilter.java index 7e7391190c..333c71066f 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/XsrfProtectionFilter.java +++ b/scm-webapp/src/main/java/sonia/scm/security/XsrfProtectionFilter.java @@ -32,6 +32,7 @@ package sonia.scm.security; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; +import com.google.inject.Inject; import java.io.IOException; import java.util.UUID; import javax.inject.Singleton; @@ -43,6 +44,7 @@ import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpSession; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import sonia.scm.config.ScmConfiguration; import sonia.scm.util.HttpUtil; import sonia.scm.web.filter.HttpFilter; @@ -52,7 +54,8 @@ import sonia.scm.web.filter.HttpFilter; * session, the web interface has to send the token from the cookie as http header on every request. If the filter * receives an request to a protected session, without proper xsrf header the filter will abort the request and send an * http error code back to the client. If the filter receives an request to a non protected session, from a non web - * interface client the filter will call the chain. + * interface client the filter will call the chain. The {@link XsrfProtectionFilter} is disabled by default and can be + * enabled with {@link ScmConfiguration#setEnabledXsrfProtection(boolean)}. * * TODO for scm-manager 2 we have to store the csrf token as part of the jwt token instead of session. * @@ -73,10 +76,30 @@ public final class XsrfProtectionFilter extends HttpFilter * Key used for session, header and cookie. */ static final String KEY = "X-XSRF-Token"; + + @Inject + public XsrfProtectionFilter(ScmConfiguration configuration) + { + this.configuration = configuration; + } @Override protected void doFilter(HttpServletRequest request, HttpServletResponse response, FilterChain chain) throws IOException, ServletException + { + if (configuration.isEnabledXsrfProtection()) + { + doXsrfProtection(request, response, chain); + } + else + { + logger.trace("xsrf protection is disabled, skipping check"); + chain.doFilter(request, response); + } + } + + private void doXsrfProtection(HttpServletRequest request, HttpServletResponse response, FilterChain chain) throws + IOException, ServletException { HttpSession session = request.getSession(true); String storedToken = (String) session.getAttribute(KEY); @@ -109,4 +132,5 @@ public final class XsrfProtectionFilter extends HttpFilter return UUID.randomUUID().toString(); } + private ScmConfiguration configuration; } diff --git a/scm-webapp/src/test/java/sonia/scm/security/XsrfProtectionFilterTest.java b/scm-webapp/src/test/java/sonia/scm/security/XsrfProtectionFilterTest.java index 2cb187c24d..0a2145de04 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/XsrfProtectionFilterTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/XsrfProtectionFilterTest.java @@ -44,8 +44,10 @@ import org.junit.Before; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.Mock; +import org.mockito.Mockito; import static org.mockito.Mockito.*; import org.mockito.runners.MockitoJUnitRunner; +import sonia.scm.config.ScmConfiguration; import sonia.scm.util.HttpUtil; /** @@ -68,7 +70,9 @@ public class XsrfProtectionFilterTest { @Mock private FilterChain chain; - private final XsrfProtectionFilter filter = new XsrfProtectionFilter(); + private final ScmConfiguration configuration = new ScmConfiguration(); + + private final XsrfProtectionFilter filter = new XsrfProtectionFilter(configuration); /** * Prepare mocks for testing. @@ -77,6 +81,7 @@ public class XsrfProtectionFilterTest { public void setUp(){ when(request.getSession(true)).thenReturn(session); when(request.getContextPath()).thenReturn("/scm"); + configuration.setEnabledXsrfProtection(true); } /** @@ -91,6 +96,31 @@ public class XsrfProtectionFilterTest { filter.doFilter(request, response, chain); verify(chain).doFilter(request, response); } + + /** + * Test filter method with disabled xsrf protection. + * + * @throws IOException + * @throws ServletException + */ + @Test + public void testDoFilterWithDisabledXsrfProtection() throws IOException, ServletException + { + // disable xsrf protection + configuration.setEnabledXsrfProtection(false); + + // set webui user-agent + when(request.getHeader(HttpUtil.HEADER_SCM_CLIENT)).thenReturn(HttpUtil.SCM_CLIENT_WUI); + + // call the filter + filter.doFilter(request, response, chain); + + // verify that no xsrf other any other cookie was set + verify(response, never()).addCookie(Mockito.any(Cookie.class)); + + // ensure filter chain is called + verify(chain).doFilter(request, response); + } /** * Test filter method for first web interface request.