From 4cf235c40de35aa31ee2f70527884b92badb4d3b Mon Sep 17 00:00:00 2001 From: Jim Anderson Date: Tue, 12 Dec 2023 14:39:34 -0600 Subject: [PATCH 1/7] tests for state validation --- .../java/com/auth0/RequestProcessorTest.java | 78 ++++++++++++++++++- 1 file changed, 76 insertions(+), 2 deletions(-) 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)); From 603f619b4e56085a02f60cceee204b46db7b1eba Mon Sep 17 00:00:00 2001 From: Jim Anderson Date: Tue, 12 Dec 2023 14:40:11 -0600 Subject: [PATCH 2/7] added messaging for state validation exceptions --- src/main/java/com/auth0/RequestProcessor.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) 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; } From 24fdb5ebc3ba4219dd791d805a37448c9c50d6aa Mon Sep 17 00:00:00 2001 From: Jim Anderson Date: Fri, 15 Dec 2023 10:09:51 -0600 Subject: [PATCH 3/7] empty commit From f598974fa279d0e4e5cfc5a71a767c29f09d7d6c Mon Sep 17 00:00:00 2001 From: Jim Anderson Date: Fri, 15 Dec 2023 10:21:34 -0600 Subject: [PATCH 4/7] small refactor to checkSessionState to get more codecov info --- src/main/java/com/auth0/RandomStorage.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/auth0/RandomStorage.java b/src/main/java/com/auth0/RandomStorage.java index 66659a0..1d5017d 100644 --- a/src/main/java/com/auth0/RandomStorage.java +++ b/src/main/java/com/auth0/RandomStorage.java @@ -15,7 +15,11 @@ class RandomStorage extends SessionUtils { */ static boolean checkSessionState(HttpServletRequest req, String state) { String currentState = (String) remove(req, StorageUtils.STATE_KEY); - return (currentState == null && state == null) || currentState != null && currentState.equals(state); + if (currentState == null && state == null) { + return true; + } else { + return (currentState != null && currentState.equals(state)); + } } /** From 717b1473b39523a2f824bebb65f07b1598ee28c5 Mon Sep 17 00:00:00 2001 From: Jim Anderson Date: Fri, 15 Dec 2023 10:32:35 -0600 Subject: [PATCH 5/7] temporary change to investigate codecov report --- src/main/java/com/auth0/RandomStorage.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/auth0/RandomStorage.java b/src/main/java/com/auth0/RandomStorage.java index 1d5017d..f9510bb 100644 --- a/src/main/java/com/auth0/RandomStorage.java +++ b/src/main/java/com/auth0/RandomStorage.java @@ -15,10 +15,10 @@ class RandomStorage extends SessionUtils { */ static boolean checkSessionState(HttpServletRequest req, String state) { String currentState = (String) remove(req, StorageUtils.STATE_KEY); - if (currentState == null && state == null) { - return true; + if (currentState == null) { + return state == null; } else { - return (currentState != null && currentState.equals(state)); + return currentState.equals(state); } } From 33b913ac90bda6b3c161fe8de652e4f7327c4cc0 Mon Sep 17 00:00:00 2001 From: Jim Anderson Date: Fri, 15 Dec 2023 10:44:10 -0600 Subject: [PATCH 6/7] add test for RandomStorage for missing session state --- src/test/java/com/auth0/RandomStorageTest.java | 7 +++++++ 1 file changed, 7 insertions(+) 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(); From 4c896110a2df1e68dc9d0c56793ee5d0929ec310 Mon Sep 17 00:00:00 2001 From: Jim Anderson Date: Fri, 15 Dec 2023 10:48:03 -0600 Subject: [PATCH 7/7] revert RandomStorage checkSessionState changes to original (changes made to inspect coverage warnings) --- src/main/java/com/auth0/RandomStorage.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/main/java/com/auth0/RandomStorage.java b/src/main/java/com/auth0/RandomStorage.java index f9510bb..66659a0 100644 --- a/src/main/java/com/auth0/RandomStorage.java +++ b/src/main/java/com/auth0/RandomStorage.java @@ -15,11 +15,7 @@ class RandomStorage extends SessionUtils { */ static boolean checkSessionState(HttpServletRequest req, String state) { String currentState = (String) remove(req, StorageUtils.STATE_KEY); - if (currentState == null) { - return state == null; - } else { - return currentState.equals(state); - } + return (currentState == null && state == null) || currentState != null && currentState.equals(state); } /**