rombert commented on code in PR #38:
URL:
https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/38#discussion_r2559460014
##########
src/test/java/org/apache/sling/auth/oauth_client/impl/RedirectHelperTest.java:
##########
@@ -85,4 +85,64 @@ void testFindLongestPathMatchingWithSibling() {
String result = RedirectHelper.findLongestPathMatching(paths, url);
assertEquals("/a/b", result);
}
+
Review Comment:
These tests look very good, thank you for the comprehensive coverage. Would
it make sense to add a test to prove that protocol-relative URLs ( e.g.
`//evil.com` ) are rejected?
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java:
##########
@@ -493,18 +493,34 @@ public boolean requestCredentials(@NotNull
HttpServletRequest request, @NotNull
} catch (IOException e) {
logger.error("Error while redirecting to default redirect: {}",
e.getMessage(), e);
throw new RuntimeException(e);
+ } catch (OAuthEntryPointException e) {
+ logger.warn("Invalid uri to redirect after login:: {}",
e.getMessage(), e);
+ throw new RuntimeException(e);
}
}
private @NotNull RedirectTarget getAuthenticationRequestUri(
- @NotNull ClientConnection connection, @NotNull HttpServletRequest
request, @NotNull URI callbackUri) {
+ @NotNull ClientConnection connection, @NotNull HttpServletRequest
request, @NotNull URI callbackUri)
+ throws OAuthEntryPointException {
ResolvedConnection conn = ResolvedOidcConnection.resolve(connection);
// The client ID provisioned by the OpenID provider when
// the client was registered is stored in the connection.
- String redirect = request.getRequestURI();
+ // Read if there is a parameter to the url where we need to redirect
the user after authentication
+ String redirect =
request.getParameter(RedirectHelper.PARAMETER_NAME_OIDC_REDIRECT);
Review Comment:
Why did you add a new parameter ? We already have
`RedirectHelper.PARAMETER_NAME_REDIRECT` for the OAuth 2.x flow.
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/RedirectHelper.java:
##########
@@ -144,4 +145,16 @@ private static boolean isDescendantOrEqual(String path,
String descendant) {
return descendant.startsWith(pattern);
}
}
+
+ public static void validateRedirect(String redirect) throws
OAuthEntryPointException {
+ // Validate that it is not a cross-site redirect
+ if (redirect == null || redirect.isEmpty()) {
+ return;
+ }
+ if (!redirect.startsWith("/")) {
+ String message = "Invalid redirect URL: " + redirect;
+ // Relative redirect within the same domain is allowed
+ throw new OAuthEntryPointException(message, new
IllegalArgumentException(message));
Review Comment:
I am not sure why we wrap this in an OauthEntryPointException, only to
unwrap it and throw it as a runtime one.
I think it's good to hide the specific messages from the end user but we
could do it in a more expressive way. The way we did this so far in the
`OAuthCallbackServlet` and `OAuthEntryPointServlet` is to catch all errors and
wrap then in a subclass of `OAuthFlowException` with a generic message.
Would that work in this case as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
