From 5dab6708e0f56f6f93302d32782d8d389b7b107a Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Thu, 26 Jul 2018 14:47:51 +0200 Subject: [PATCH] SONAR-11072 Allow customization of request parameter used to check CSRF state --- .../authentication/OAuth2CallbackFilter.java | 5 +++++ .../authentication/OAuth2ContextFactory.java | 5 +++++ .../server/authentication/OAuthCsrfVerifier.java | 7 ++++++- .../authentication/OAuthCsrfVerifierTest.java | 16 ++++++++++++++++ .../authentication/OAuth2IdentityProvider.java | 10 ++++++++++ 5 files changed, 42 insertions(+), 1 deletion(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/OAuth2CallbackFilter.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/OAuth2CallbackFilter.java index 0712cc0b52d..1b2944cc9f0 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/authentication/OAuth2CallbackFilter.java +++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/OAuth2CallbackFilter.java @@ -152,6 +152,11 @@ public class OAuth2CallbackFilter extends AuthenticationFilter { delegate.verifyCsrfState(); } + @Override + public void verifyCsrfState(String parameterName) { + delegate.verifyCsrfState(parameterName); + } + @Override public void redirectToRequestedPage() { delegate.redirectToRequestedPage(); diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/OAuth2ContextFactory.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/OAuth2ContextFactory.java index 674bc6a5629..26549396c6d 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/authentication/OAuth2ContextFactory.java +++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/OAuth2ContextFactory.java @@ -113,6 +113,11 @@ public class OAuth2ContextFactory { csrfVerifier.verifyState(request, response, identityProvider); } + @Override + public void verifyCsrfState(String parameterName) { + csrfVerifier.verifyState(request, response, identityProvider, parameterName); + } + @Override public void redirectToRequestedPage() { try { diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/OAuthCsrfVerifier.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/OAuthCsrfVerifier.java index 297f52dea50..953a2ff89f6 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/authentication/OAuthCsrfVerifier.java +++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/OAuthCsrfVerifier.java @@ -37,6 +37,7 @@ import static org.sonar.server.authentication.event.AuthenticationEvent.Source; public class OAuthCsrfVerifier { private static final String CSRF_STATE_COOKIE = "OAUTHSTATE"; + private static final String DEFAULT_STATE_PARAMETER_NAME = "state"; public String generateState(HttpServletRequest request, HttpServletResponse response) { // Create a state token to prevent request forgery. @@ -47,6 +48,10 @@ public class OAuthCsrfVerifier { } public void verifyState(HttpServletRequest request, HttpServletResponse response, OAuth2IdentityProvider provider) { + verifyState(request, response, provider, DEFAULT_STATE_PARAMETER_NAME); + } + + public void verifyState(HttpServletRequest request, HttpServletResponse response, OAuth2IdentityProvider provider, String parameterName) { Cookie cookie = findCookie(CSRF_STATE_COOKIE, request) .orElseThrow(AuthenticationException.newBuilder() .setSource(Source.oauth2(provider)) @@ -56,7 +61,7 @@ public class OAuthCsrfVerifier { // remove cookie response.addCookie(newCookieBuilder(request).setName(CSRF_STATE_COOKIE).setValue(null).setHttpOnly(true).setExpiry(0).build()); - String stateInRequest = request.getParameter("state"); + String stateInRequest = request.getParameter(parameterName); if (isBlank(stateInRequest) || !sha256Hex(stateInRequest).equals(hashInCookie)) { throw AuthenticationException.newBuilder() .setSource(Source.oauth2(provider)) diff --git a/server/sonar-server/src/test/java/org/sonar/server/authentication/OAuthCsrfVerifierTest.java b/server/sonar-server/src/test/java/org/sonar/server/authentication/OAuthCsrfVerifierTest.java index 0ae64700060..7dc94f061c2 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/authentication/OAuthCsrfVerifierTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/authentication/OAuthCsrfVerifierTest.java @@ -72,6 +72,22 @@ public class OAuthCsrfVerifierTest { @Test public void verify_state() { + String state = "state"; + when(request.getCookies()).thenReturn(new Cookie[] {new Cookie("OAUTHSTATE", sha256Hex(state))}); + when(request.getParameter("aStateParameter")).thenReturn(state); + + underTest.verifyState(request, response, identityProvider, "aStateParameter"); + + verify(response).addCookie(cookieArgumentCaptor.capture()); + Cookie updatedCookie = cookieArgumentCaptor.getValue(); + assertThat(updatedCookie.getName()).isEqualTo("OAUTHSTATE"); + assertThat(updatedCookie.getValue()).isNull(); + assertThat(updatedCookie.getPath()).isEqualTo("/"); + assertThat(updatedCookie.getMaxAge()).isEqualTo(0); + } + + @Test + public void verify_state_using_default_state_parameter() { String state = "state"; when(request.getCookies()).thenReturn(new Cookie[] {new Cookie("OAUTHSTATE", sha256Hex(state))}); when(request.getParameter("state")).thenReturn(state); diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/server/authentication/OAuth2IdentityProvider.java b/sonar-plugin-api/src/main/java/org/sonar/api/server/authentication/OAuth2IdentityProvider.java index ac9395efa24..6ef11a059fd 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/server/authentication/OAuth2IdentityProvider.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/server/authentication/OAuth2IdentityProvider.java @@ -76,10 +76,20 @@ public interface OAuth2IdentityProvider extends IdentityProvider { /** * Check that the state is valid. + * The state will be read from the 'state' parameter of the HTTP request + * * It should only be called If {@link InitContext#generateCsrfState()} was used in the init */ void verifyCsrfState(); + /** + * Check that the state is valid + * The state will be read from the given parameter name of the HTTP request + * + * It should only be called If {@link InitContext#generateCsrfState()} was used in the init + */ + void verifyCsrfState(String parameterName); + /** * Redirect the request to the requested page. * Must be called at the end of {@link OAuth2IdentityProvider#callback(CallbackContext)} -- 2.39.5