From f20649f0357a9d464370a398d81aa2b31d79949f Mon Sep 17 00:00:00 2001 From: Rob Winch Date: Thu, 13 Jan 2011 20:29:37 -0600 Subject: [PATCH] SEC-1648: added null check for getTargetUrlParameter() in SavedRequestAwareAuthenticationSuccessHandler.onAuthenticationSuccess and updated validation for AbstractAuthenticationTargetUrlRequestHandler.setTargetUrlParameter --- ...AuthenticationTargetUrlRequestHandler.java | 4 ++- ...uestAwareAuthenticationSuccessHandler.java | 4 +-- ...wareAuthenticationSuccessHandlerTests.java | 31 +++++++++++++++++-- ...eUrlAuthenticationSuccessHandlerTests.java | 25 +++++++++++++-- 4 files changed, 56 insertions(+), 8 deletions(-) diff --git a/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationTargetUrlRequestHandler.java b/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationTargetUrlRequestHandler.java index b0fcc15e5c..000f14971c 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationTargetUrlRequestHandler.java +++ b/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationTargetUrlRequestHandler.java @@ -162,7 +162,9 @@ public abstract class AbstractAuthenticationTargetUrlRequestHandler { * to null. */ public void setTargetUrlParameter(String targetUrlParameter) { - Assert.hasText("targetUrlParameter canot be null or empty"); + if(targetUrlParameter != null) { + Assert.hasText(targetUrlParameter,"targetUrlParameter cannot be empty"); + } this.targetUrlParameter = targetUrlParameter; } diff --git a/web/src/main/java/org/springframework/security/web/authentication/SavedRequestAwareAuthenticationSuccessHandler.java b/web/src/main/java/org/springframework/security/web/authentication/SavedRequestAwareAuthenticationSuccessHandler.java index 7fc71638f5..14af5362d9 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/SavedRequestAwareAuthenticationSuccessHandler.java +++ b/web/src/main/java/org/springframework/security/web/authentication/SavedRequestAwareAuthenticationSuccessHandler.java @@ -64,8 +64,8 @@ public class SavedRequestAwareAuthenticationSuccessHandler extends SimpleUrlAuth return; } - - if (isAlwaysUseDefaultTargetUrl() || StringUtils.hasText(request.getParameter(getTargetUrlParameter()))) { + String targetUrlParameter = getTargetUrlParameter(); + if (isAlwaysUseDefaultTargetUrl() || (targetUrlParameter != null && StringUtils.hasText(request.getParameter(targetUrlParameter)))) { requestCache.removeRequest(request, response); super.onAuthenticationSuccess(request, response, authentication); diff --git a/web/src/test/java/org/springframework/security/web/authentication/SavedRequestAwareAuthenticationSuccessHandlerTests.java b/web/src/test/java/org/springframework/security/web/authentication/SavedRequestAwareAuthenticationSuccessHandlerTests.java index 684285df3e..628090eda4 100644 --- a/web/src/test/java/org/springframework/security/web/authentication/SavedRequestAwareAuthenticationSuccessHandlerTests.java +++ b/web/src/test/java/org/springframework/security/web/authentication/SavedRequestAwareAuthenticationSuccessHandlerTests.java @@ -1,9 +1,15 @@ package org.springframework.security.web.authentication; -import static org.junit.Assert.*; +import static org.junit.Assert.fail; +import static org.mockito.Mockito.*; import org.junit.Test; -import org.springframework.security.web.authentication.SavedRequestAwareAuthenticationSuccessHandler; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.security.core.Authentication; +import org.springframework.security.web.RedirectStrategy; +import org.springframework.security.web.savedrequest.RequestCache; +import org.springframework.security.web.savedrequest.SavedRequest; public class SavedRequestAwareAuthenticationSuccessHandlerTests { @@ -20,4 +26,23 @@ public class SavedRequestAwareAuthenticationSuccessHandlerTests { fail("Shouldn't accept default target without leading slash"); } catch (IllegalArgumentException expected) {} } -} + + @Test + public void onAuthenticationSuccessHasSavedRequest() throws Exception { + String redirectUrl = "http://localhost/appcontext/page"; + RedirectStrategy redirectStrategy = mock(RedirectStrategy.class); + RequestCache requestCache = mock(RequestCache.class); + SavedRequest savedRequest = mock(SavedRequest.class); + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpServletResponse response = new MockHttpServletResponse(); + when(savedRequest.getRedirectUrl()).thenReturn(redirectUrl); + when(requestCache.getRequest(request, response)).thenReturn(savedRequest); + + SavedRequestAwareAuthenticationSuccessHandler handler = new SavedRequestAwareAuthenticationSuccessHandler(); + handler.setRequestCache(requestCache); + handler.setRedirectStrategy(redirectStrategy); + handler.onAuthenticationSuccess(request, response, mock(Authentication.class)); + + verify(redirectStrategy).sendRedirect(request, response, redirectUrl); + } +} \ No newline at end of file diff --git a/web/src/test/java/org/springframework/security/web/authentication/SimpleUrlAuthenticationSuccessHandlerTests.java b/web/src/test/java/org/springframework/security/web/authentication/SimpleUrlAuthenticationSuccessHandlerTests.java index 39f1d73aa5..fc1b2ccbfd 100644 --- a/web/src/test/java/org/springframework/security/web/authentication/SimpleUrlAuthenticationSuccessHandlerTests.java +++ b/web/src/test/java/org/springframework/security/web/authentication/SimpleUrlAuthenticationSuccessHandlerTests.java @@ -1,7 +1,6 @@ package org.springframework.security.web.authentication; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; +import static org.junit.Assert.*; import static org.mockito.Mockito.*; import org.junit.Test; @@ -85,4 +84,26 @@ public class SimpleUrlAuthenticationSuccessHandlerTests { assertEquals("https://monkeymachine.co.uk/", response.getRedirectedUrl()); } + @Test + public void setTargetUrlParameterNullTargetUrlParameter() { + SimpleUrlAuthenticationSuccessHandler ash = new SimpleUrlAuthenticationSuccessHandler(); + ash.setTargetUrlParameter("targetUrl"); + ash.setTargetUrlParameter(null); + assertEquals(null,ash.getTargetUrlParameter()); + } + + @Test + public void setTargetUrlParameterEmptyTargetUrlParameter() { + SimpleUrlAuthenticationSuccessHandler ash = new SimpleUrlAuthenticationSuccessHandler(); + + try { + ash.setTargetUrlParameter(""); + fail("Expected Exception"); + }catch(IllegalArgumentException success) {} + + try { + ash.setTargetUrlParameter(" "); + fail("Expected Exception"); + }catch(IllegalArgumentException success) {} + } }