From b46ae6ac62dfbbe94e7f0d241d88af8486d9ddd9 Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Sun, 28 Feb 2010 14:00:06 +0000 Subject: [PATCH] SEC-1425: Add check for empty cookie in AbstractRememberMeServices. Prevents ArrayOutOfBoundsException later when processing the tokeniszed cookie. --- .../AbstractRememberMeServices.java | 13 +++- .../AbstractRememberMeServicesTests.java | 76 ++++++++++++++++++- 2 files changed, 81 insertions(+), 8 deletions(-) diff --git a/web/src/main/java/org/springframework/security/web/authentication/rememberme/AbstractRememberMeServices.java b/web/src/main/java/org/springframework/security/web/authentication/rememberme/AbstractRememberMeServices.java index aa94680683..1b8cd7bbe7 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/rememberme/AbstractRememberMeServices.java +++ b/web/src/main/java/org/springframework/security/web/authentication/rememberme/AbstractRememberMeServices.java @@ -58,9 +58,7 @@ public abstract class AbstractRememberMeServices implements RememberMeServices, public void afterPropertiesSet() throws Exception { Assert.hasLength(key); - Assert.hasLength(parameter); - Assert.hasLength(cookieName); - Assert.notNull(userDetailsService); + Assert.notNull(userDetailsService, "A UserDetailsService is required"); } /** @@ -80,6 +78,12 @@ public abstract class AbstractRememberMeServices implements RememberMeServices, logger.debug("Remember-me cookie detected"); + if (rememberMeCookie.isEmpty()) { + logger.debug("Cookie was empty"); + cancelCookie(request, response); + return null; + } + UserDetails user = null; try { @@ -335,6 +339,7 @@ public abstract class AbstractRememberMeServices implements RememberMeServices, } public void setCookieName(String cookieName) { + Assert.hasLength(cookieName, "Cookie name cannot be empty or null"); this.cookieName = cookieName; } @@ -353,7 +358,7 @@ public abstract class AbstractRememberMeServices implements RememberMeServices, * @param parameter the HTTP request parameter */ public void setParameter(String parameter) { - Assert.hasText(parameter, "Parameter name cannot be null"); + Assert.hasText(parameter, "Parameter name cannot be empty or null"); this.parameter = parameter; } diff --git a/web/src/test/java/org/springframework/security/web/authentication/rememberme/AbstractRememberMeServicesTests.java b/web/src/test/java/org/springframework/security/web/authentication/rememberme/AbstractRememberMeServicesTests.java index 73036db01f..79cbba3afb 100644 --- a/web/src/test/java/org/springframework/security/web/authentication/rememberme/AbstractRememberMeServicesTests.java +++ b/web/src/test/java/org/springframework/security/web/authentication/rememberme/AbstractRememberMeServicesTests.java @@ -1,6 +1,7 @@ package org.springframework.security.web.authentication.rememberme; import static org.junit.Assert.*; +import static org.mockito.Mockito.*; import javax.servlet.http.Cookie; import javax.servlet.http.HttpServletRequest; @@ -9,6 +10,7 @@ import javax.servlet.http.HttpServletResponse; import org.junit.Test; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.security.authentication.AuthenticationDetailsSource; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.Authentication; import org.springframework.security.core.authority.AuthorityUtils; @@ -34,7 +36,29 @@ public class AbstractRememberMeServicesTests { } @Test - public void cookieShouldBeCorrectlyEncodedAndDecoded() { + public void setAndGetAreConsistent() throws Exception { + MockRememberMeServices services = new MockRememberMeServices(); + assertNotNull(services.getCookieName()); + assertNotNull(services.getParameter()); + services.setKey("xxxx"); + assertEquals("xxxx", services.getKey()); + services.setParameter("rm"); + assertEquals("rm", services.getParameter()); + services.setCookieName("kookie"); + assertEquals("kookie", services.getCookieName()); + services.setTokenValiditySeconds(600); + assertEquals(600, services.getTokenValiditySeconds()); + UserDetailsService uds = mock(UserDetailsService.class); + services.setUserDetailsService(uds); + assertSame(uds, services.getUserDetailsService()); + AuthenticationDetailsSource ads = mock(AuthenticationDetailsSource.class); + services.setAuthenticationDetailsSource(ads); + assertSame(ads, services.getAuthenticationDetailsSource()); + services.afterPropertiesSet(); + } + + @Test + public void cookieShouldBeCorrectlyEncodedAndDecoded() throws Exception { String[] cookie = new String[] {"name", "cookie", "tokens", "blah"}; MockRememberMeServices services = new MockRememberMeServices(); @@ -86,9 +110,10 @@ public class AbstractRememberMeServicesTests { } @Test - public void successfulAutoLoginReturnsExpectedAuthentication() { + public void successfulAutoLoginReturnsExpectedAuthentication() throws Exception { MockRememberMeServices services = new MockRememberMeServices(); services.setUserDetailsService(new MockUserDetailsService(joe, false)); + services.afterPropertiesSet(); assertNotNull(services.getUserDetailsService()); MockHttpServletRequest request = new MockHttpServletRequest(); @@ -101,13 +126,39 @@ public class AbstractRememberMeServicesTests { assertNotNull(result); } + @Test + public void autoLoginShouldFailIfCookieIsNotBase64() throws Exception { + MockRememberMeServices services = new MockRememberMeServices(); + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpServletResponse response = new MockHttpServletResponse(); + + request.setCookies(new Cookie[] { + new Cookie(AbstractRememberMeServices.SPRING_SECURITY_REMEMBER_ME_COOKIE_KEY, "ZZZ")}); + Authentication result = services.autoLogin(request, response); + assertNull(result); + assertCookieCancelled(response); + } + + @Test + public void autoLoginShouldFailIfCookieIsEmpty() throws Exception { + MockRememberMeServices services = new MockRememberMeServices(); + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpServletResponse response = new MockHttpServletResponse(); + + request.setCookies(new Cookie[] { + new Cookie(AbstractRememberMeServices.SPRING_SECURITY_REMEMBER_ME_COOKIE_KEY, "")}); + Authentication result = services.autoLogin(request, response); + assertNull(result); + assertCookieCancelled(response); + } + @Test public void autoLoginShouldFailIfInvalidCookieExceptionIsRaised() { MockRememberMeServices services = new MockRememberMeServices(); - services.setUserDetailsService(new MockUserDetailsService(joe, true)); +// services.setUserDetailsService(new MockUserDetailsService(joe, true)); MockHttpServletRequest request = new MockHttpServletRequest(); - // Wrong number of tokes + // Wrong number of tokens request.setCookies(createLoginCookie("cookie:1")); MockHttpServletResponse response = new MockHttpServletResponse(); @@ -166,6 +217,23 @@ public class AbstractRememberMeServicesTests { assertCookieCancelled(response); } + @Test + public void logoutShouldCancelCookie() throws Exception { + MockRememberMeServices services = new MockRememberMeServices(); + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setContextPath("contextpath"); + request.setCookies(createLoginCookie("cookie:1:2")); + MockHttpServletResponse response = new MockHttpServletResponse(); + + services.logout(request, response, mock(Authentication.class)); + // Try again with null Authentication + response = new MockHttpServletResponse(); + + services.logout(request, response, null); + + assertCookieCancelled(response); + } + @Test(expected = CookieTheftException.class) public void cookieTheftExceptionShouldBeRethrown() { MockRememberMeServices services = new MockRememberMeServices() {