From fbfe27eb1dded880020c9da6118abc233e60b1e0 Mon Sep 17 00:00:00 2001 From: Jim Anderson Date: Mon, 18 Dec 2023 10:01:25 -0600 Subject: [PATCH] [SDK-4670] Improved state handling errors (#140) --- src/main/java/com/auth0/RequestProcessor.java | 12 +++ .../java/com/auth0/RandomStorageTest.java | 7 ++ .../java/com/auth0/RequestProcessorTest.java | 78 ++++++++++++++++++- 3 files changed, 95 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/auth0/RequestProcessor.java b/src/main/java/com/auth0/RequestProcessor.java index 6163ca3..6796982 100644 --- a/src/main/java/com/auth0/RequestProcessor.java +++ b/src/main/java/com/auth0/RequestProcessor.java @@ -292,8 +292,17 @@ private void assertNoError(HttpServletRequest request) throws InvalidRequestExce * @throws InvalidRequestException if the request contains a different state from the expected one */ private void assertValidState(HttpServletRequest request, HttpServletResponse response) throws InvalidRequestException { + // TODO in v2: + // - only store state/nonce in cookies, remove session storage + // - create specific exception classes for various state validation failures (missing from auth response, missing + // state cookie, mismatch) + String stateFromRequest = request.getParameter(KEY_STATE); + if (stateFromRequest == null) { + throw new InvalidRequestException(INVALID_STATE_ERROR, "The received state doesn't match the expected one. No state parameter was found on the authorization response."); + } + // If response is null, check the Session. // This can happen when the deprecated handle method that only takes the request parameter is called if (response == null) { @@ -306,6 +315,9 @@ private void assertValidState(HttpServletRequest request, HttpServletResponse re // Just in case state was stored in Session by building auth URL with deprecated method, but then called the // supported handle method with the request and response if (cookieState == null) { + if (SessionUtils.get(request, StorageUtils.STATE_KEY) == null) { + throw new InvalidRequestException(INVALID_STATE_ERROR, "The received state doesn't match the expected one. No state cookie or state session attribute found. Check that you are using non-deprecated methods and that cookies are not being removed on the server."); + } checkSessionState(request, stateFromRequest); return; } diff --git a/src/test/java/com/auth0/RandomStorageTest.java b/src/test/java/com/auth0/RandomStorageTest.java index 7ffd766..49a4af7 100644 --- a/src/test/java/com/auth0/RandomStorageTest.java +++ b/src/test/java/com/auth0/RandomStorageTest.java @@ -24,6 +24,13 @@ public void shouldAcceptBothNullStates() { assertThat(validState, is(true)); } + @Test + public void shouldFailIfSessionStateIsNullButCurrentStateNotNull() { + MockHttpServletRequest req = new MockHttpServletRequest(); + boolean validState = RandomStorage.checkSessionState(req, "12345"); + assertThat(validState, is(false)); + } + @Test public void shouldCheckAndRemoveInvalidState() { MockHttpServletRequest req = new MockHttpServletRequest(); diff --git a/src/test/java/com/auth0/RequestProcessorTest.java b/src/test/java/com/auth0/RequestProcessorTest.java index aa6641c..7ffcf60 100644 --- a/src/test/java/com/auth0/RequestProcessorTest.java +++ b/src/test/java/com/auth0/RequestProcessorTest.java @@ -107,7 +107,7 @@ public void shouldThrowOnProcessIfRequestHasMissingStateParameter() throws Excep .build(); InvalidRequestException e = assertThrows(InvalidRequestException.class, () -> handler.process(request, response)); assertThat(e, InvalidRequestExceptionMatcher.hasCode("a0.invalid_state")); - assertEquals("The received state doesn't match the expected one.", e.getMessage()); + assertEquals("The received state doesn't match the expected one. No state parameter was found on the authorization response.", e.getMessage()); } @Test @@ -120,7 +120,7 @@ public void shouldThrowOnProcessIfRequestHasMissingStateCookie() throws Exceptio .build(); InvalidRequestException e = assertThrows(InvalidRequestException.class, () -> handler.process(request, response)); assertThat(e, InvalidRequestExceptionMatcher.hasCode("a0.invalid_state")); - assertEquals("The received state doesn't match the expected one.", e.getMessage()); + assertEquals("The received state doesn't match the expected one. No state cookie or state session attribute found. Check that you are using non-deprecated methods and that cookies are not being removed on the server.", e.getMessage()); } @Test @@ -288,6 +288,80 @@ public void shouldReturnTokensOnProcessIfIdTokenCodeRequestPassesIdTokenVerifica assertThat(tokens.getExpiresIn(), is(8400L)); } + @Test + public void shouldReturnTokensOnProcessIfIdTokenCodeRequestPassesIdTokenVerificationWhenUsingSessionStorage() throws Exception { + doNothing().when(tokenVerifier).verify(eq("frontIdToken"), eq(verifyOptions)); + + Map params = new HashMap<>(); + params.put("code", "abc123"); + params.put("state", "1234"); + params.put("id_token", "frontIdToken"); + params.put("expires_in", "8400"); + params.put("token_type", "frontTokenType"); + MockHttpServletRequest request = getRequest(params); + request.getSession().setAttribute("com.auth0.state", "1234"); + + TokenRequest codeExchangeRequest = mock(TokenRequest.class); + TokenHolder tokenHolder = mock(TokenHolder.class); + when(tokenHolder.getIdToken()).thenReturn("backIdToken"); + when(tokenHolder.getExpiresIn()).thenReturn(4800L); + when(tokenHolder.getTokenType()).thenReturn("backTokenType"); + when(codeExchangeRequest.execute()).thenReturn(tokenHolder); + when(client.exchangeCode("abc123", "https://me.auth0.com:80/callback")).thenReturn(codeExchangeRequest); + + RequestProcessor handler = new RequestProcessor.Builder(client, "id_token code", verifyOptions) + .withIdTokenVerifier(tokenVerifier) + .build(); + Tokens tokens = handler.process(request, response); + + //Should not verify the ID Token twice + verify(tokenVerifier).verify("frontIdToken", verifyOptions); + verify(tokenVerifier, never()).verify("backIdToken", verifyOptions); + verifyNoMoreInteractions(tokenVerifier); + + assertThat(tokens, is(notNullValue())); + assertThat(tokens.getIdToken(), is("frontIdToken")); + assertThat(tokens.getType(), is("frontTokenType")); + assertThat(tokens.getExpiresIn(), is(8400L)); + } + + @Test + public void shouldReturnTokensOnProcessIfIdTokenCodeRequestPassesIdTokenVerificationWhenUsingSessionStorageWithNullSession() throws Exception { + doNothing().when(tokenVerifier).verify(eq("frontIdToken"), eq(verifyOptions)); + + Map params = new HashMap<>(); + params.put("code", "abc123"); + params.put("state", "1234"); + params.put("id_token", "frontIdToken"); + params.put("expires_in", "8400"); + params.put("token_type", "frontTokenType"); + MockHttpServletRequest request = getRequest(params); + request.getSession().setAttribute("com.auth0.state", "1234"); + + TokenRequest codeExchangeRequest = mock(TokenRequest.class); + TokenHolder tokenHolder = mock(TokenHolder.class); + when(tokenHolder.getIdToken()).thenReturn("backIdToken"); + when(tokenHolder.getExpiresIn()).thenReturn(4800L); + when(tokenHolder.getTokenType()).thenReturn("backTokenType"); + when(codeExchangeRequest.execute()).thenReturn(tokenHolder); + when(client.exchangeCode("abc123", "https://me.auth0.com:80/callback")).thenReturn(codeExchangeRequest); + + RequestProcessor handler = new RequestProcessor.Builder(client, "id_token code", verifyOptions) + .withIdTokenVerifier(tokenVerifier) + .build(); + Tokens tokens = handler.process(request, null); + + //Should not verify the ID Token twice + verify(tokenVerifier).verify("frontIdToken", verifyOptions); + verify(tokenVerifier, never()).verify("backIdToken", verifyOptions); + verifyNoMoreInteractions(tokenVerifier); + + assertThat(tokens, is(notNullValue())); + assertThat(tokens.getIdToken(), is("frontIdToken")); + assertThat(tokens.getType(), is("frontTokenType")); + assertThat(tokens.getExpiresIn(), is(8400L)); + } + @Test public void shouldReturnTokensOnProcessIfTokenIdTokenCodeRequestPassesIdTokenVerification() throws Exception { doNothing().when(tokenVerifier).verify(eq("frontIdToken"), eq(verifyOptions));