Redirect using client data when the session has expired #39330
Conversation
| //set redirect uri from client data parameter | ||
| try { | ||
| ClientData clientData = ClientData.decodeClientDataFromParameter(clientDataString); | ||
| authSession.setRedirectUri(clientData.getRedirectUri()); |
There was a problem hiding this comment.
It seems to me that this approach can have other issues as redirectUri can belong to completely different client. For example in tab1, there could be client1 and in tab2 there could be client2, which has completely different redirect urls.
There is one point that if client_data parameter is present, there should be also client_id parameter present. We can check if client_id from the parameter is the same as the client_id from the KC_RESTART cookie. If it is same:
- We can replace
redirect_uribut also other things, which are available from clientData (likestate,responseMode,responseType) - If clientId parameter is not the same like the one from cookie, we either should not replace anything OR we should maybe skip restart from the cookie at all as it is obvious that
KC_RESTARTcookie was created for different browser tab than the current tab.
There was a problem hiding this comment.
The check that compares the client_id in the request param with the client_id in the KC_RESTART cookie is already done here:
https://github.com/keycloak/keycloak/blob/main/services/src/main/java/org/keycloak/protocol/RestartLoginCookie.java#L155
So the behavior should not change, if the client_ids are different the restart from cookie will be skipped. Working on replacing the other claims from clientData.
| @@ -727,6 +727,54 @@ public void testLoginPageRefresh() { | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
| public void testRedirectToCorrectUrlAfterAuthSessionExpiration() { | |||
There was a problem hiding this comment.
Is it possible to add automated tests also for some other scenarios? Like for example the scenarios like:
- Fake
client_datais injected with the incorrect redirect_uri. In this case, we should check that redirect did not happened - Maybe test with multiple different clients in two browser tabs (in this case, the redirectUri either should not be rewritten or the restart from cookie should not happen at all if we do the same what I did in other comment).
There was a problem hiding this comment.
I'm working on adding tests, I'm having some trouble testing the replacement of the client_data, because it would require replacing the parameter in the authenticate browser form inside the login page, so far, I haven't found a way to do that
There was a problem hiding this comment.
Ah, I see. That might be tricky...
Not 100% sure, but maybe this trick can be useful to do something like this https://github.com/keycloak/keycloak/blob/main/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/AppInitiatedActionResetPasswordTest.java#L275-L276 ?
Closes keycloak#36150 Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
Closes keycloak#36150 Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
Closes #36150
When the authentication session doesn't exist and it's restarted using the
KC_RESTARTcookie, with this PR theredirect_uriis taken from theclient_dataparameter of the restart endpoint (which belongs to the tab) instead of from the value inside theKC_RESTARTcookie, which is shared across tabs.I'm not sure if there are any security implications with this approach, since
KC_RESTARTis encrypted and not accessible from javascript, insteadclient_datais only Base64-encoded and accessible via javascript so theredirect_uricould potentially be injected but it still needs to be one of the allowed values.