Copilot commented on code in PR #3318:
URL: https://github.com/apache/cxf/pull/3318#discussion_r3610884712
##########
rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/DynamicRegistrationService.java:
##########
@@ -366,12 +372,44 @@ protected boolean isPasswordRequired(List<String>
grantTypes, String tokenEndpoi
||
OAuthConstants.TOKEN_ENDPOINT_AUTH_POST.equals(tokenEndpointAuthMethod));
}
+ @SuppressWarnings("PMD.CollapsibleIfStatements")
protected void validateRequestUri(String uri, String appType, List<String>
grantTypes) {
+ if (uri == null || uri.isBlank()) {
+ throw new OAuthServiceException("Empty redirect URI are not
supported");
+ }
Review Comment:
Grammar: the message should use singular "URI is" (not "URI are").
##########
rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/DynamicRegistrationService.java:
##########
@@ -366,12 +372,44 @@ protected boolean isPasswordRequired(List<String>
grantTypes, String tokenEndpoi
||
OAuthConstants.TOKEN_ENDPOINT_AUTH_POST.equals(tokenEndpointAuthMethod));
}
+ @SuppressWarnings("PMD.CollapsibleIfStatements")
protected void validateRequestUri(String uri, String appType, List<String>
grantTypes) {
+ if (uri == null || uri.isBlank()) {
+ throw new OAuthServiceException("Empty redirect URI are not
supported");
+ }
+
// Web Clients using the OAuth Implicit Grant Type MUST only register
URLs using the https scheme
// as redirect_uris; they MUST NOT use localhost as the hostname.
Native Clients MUST only register
- // redirect_uris using custom URI schemes or URLs using the http:
scheme with localhost as the hostname.
+ // redirect_uris using custom URI schemes or loopback URLs using the
http scheme; loopback URLs use
+ // localhost or the IP loopback literals 127.0.0.1 or [::1] as the
hostname.
// Authorization Servers MAY place additional constraints on Native
Clients. Authorization Servers MAY
// reject Redirection URI values using the http scheme, other than the
localhost case for Native Clients
+
+ final URI parsedUri = URI.create(uri);
+ if (parsedUri.getHost() == null || parsedUri.getScheme() == null) {
+ throw new OAuthServiceException("Unsupported redirect URI
scheme/host");
+ }
+ final String host = parsedUri.getHost().toLowerCase();
+ final String scheme = parsedUri.getScheme().toLowerCase();
+ if (!URIResolver.getAllowedSchemes().contains(scheme)) {
+ throw new OAuthServiceException("Redirect URI scheme is not
allowed: " + scheme
+ + "Allowed schemes: " + URIResolver.getAllowedSchemes());
+ }
Review Comment:
The exception message concatenation is missing a separator between the
scheme value and the "Allowed schemes" text, producing messages like `...:
httpAllowed schemes: ...`. Add punctuation/spacing.
##########
rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/DynamicRegistrationService.java:
##########
@@ -366,12 +372,44 @@ protected boolean isPasswordRequired(List<String>
grantTypes, String tokenEndpoi
||
OAuthConstants.TOKEN_ENDPOINT_AUTH_POST.equals(tokenEndpointAuthMethod));
}
+ @SuppressWarnings("PMD.CollapsibleIfStatements")
protected void validateRequestUri(String uri, String appType, List<String>
grantTypes) {
+ if (uri == null || uri.isBlank()) {
+ throw new OAuthServiceException("Empty redirect URI are not
supported");
+ }
+
// Web Clients using the OAuth Implicit Grant Type MUST only register
URLs using the https scheme
// as redirect_uris; they MUST NOT use localhost as the hostname.
Native Clients MUST only register
- // redirect_uris using custom URI schemes or URLs using the http:
scheme with localhost as the hostname.
+ // redirect_uris using custom URI schemes or loopback URLs using the
http scheme; loopback URLs use
+ // localhost or the IP loopback literals 127.0.0.1 or [::1] as the
hostname.
// Authorization Servers MAY place additional constraints on Native
Clients. Authorization Servers MAY
// reject Redirection URI values using the http scheme, other than the
localhost case for Native Clients
+
+ final URI parsedUri = URI.create(uri);
Review Comment:
`URI.create(uri)` throws `IllegalArgumentException` for malformed URIs
(e.g., spaces, bad escapes). Right now that would escape as an uncaught runtime
exception; catch it and convert to an OAuth error.
##########
rt/rs/security/oauth-parent/oauth2/src/test/java/org/apache/cxf/rs/security/oauth2/services/DynamicRegistrationServiceTest.java:
##########
@@ -80,6 +82,82 @@ public void
testAcceptsRegisteredScopesWhenAllowlistNotConfigured() {
assertEquals(Collections.singletonList("openid"),
client.getRegisteredScopes());
}
+ @Test
+ public void testAcceptsAllowedRedirectUrlsWebApp() {
+ TestDynamicRegistrationService service = new
TestDynamicRegistrationService();
+
+ ClientRegistration request = new ClientRegistration();
+ request.setScope("read write");
+ request.setRedirectUris(List.of("https://localhost";,
"http://localhost";));
+
+ Client client = createClient();
+ service.applyClientRegistration(request, client);
+
+ assertEquals(Arrays.asList("https://localhost";, "http://localhost";),
client.getRedirectUris());
Review Comment:
This test treats `http://localhost` and `https://localhost` as acceptable
redirect URIs for a (default) web application. That seems to conflict with the
PR goal of aligning with OpenID Connect Dynamic Client Registration redirect
URI rules (which generally require https for web clients and disallow
localhost). Consider using non-localhost https redirect URIs here so the test
matches the stated spec intent.
##########
rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/DynamicRegistrationService.java:
##########
@@ -366,12 +372,44 @@ protected boolean isPasswordRequired(List<String>
grantTypes, String tokenEndpoi
||
OAuthConstants.TOKEN_ENDPOINT_AUTH_POST.equals(tokenEndpointAuthMethod));
}
+ @SuppressWarnings("PMD.CollapsibleIfStatements")
protected void validateRequestUri(String uri, String appType, List<String>
grantTypes) {
+ if (uri == null || uri.isBlank()) {
+ throw new OAuthServiceException("Empty redirect URI are not
supported");
+ }
+
// Web Clients using the OAuth Implicit Grant Type MUST only register
URLs using the https scheme
// as redirect_uris; they MUST NOT use localhost as the hostname.
Native Clients MUST only register
- // redirect_uris using custom URI schemes or URLs using the http:
scheme with localhost as the hostname.
+ // redirect_uris using custom URI schemes or loopback URLs using the
http scheme; loopback URLs use
+ // localhost or the IP loopback literals 127.0.0.1 or [::1] as the
hostname.
// Authorization Servers MAY place additional constraints on Native
Clients. Authorization Servers MAY
// reject Redirection URI values using the http scheme, other than the
localhost case for Native Clients
+
+ final URI parsedUri = URI.create(uri);
+ if (parsedUri.getHost() == null || parsedUri.getScheme() == null) {
+ throw new OAuthServiceException("Unsupported redirect URI
scheme/host");
+ }
+ final String host = parsedUri.getHost().toLowerCase();
+ final String scheme = parsedUri.getScheme().toLowerCase();
+ if (!URIResolver.getAllowedSchemes().contains(scheme)) {
+ throw new OAuthServiceException("Redirect URI scheme is not
allowed: " + scheme
+ + "Allowed schemes: " + URIResolver.getAllowedSchemes());
+ }
Review Comment:
Redirect URI scheme validation is currently based on
`URIResolver.getAllowedSchemes()`, which is meant for CXF resource loading and
includes schemes like `file`, `jar`, `classpath`, etc. That can lead to
accepting redirect URIs with those schemes (for non-implicit web clients /
native clients), and it also prevents OIDC-native "custom URI schemes" unless
the global URIResolver property is modified. Redirect URI validation should use
an OIDC-specific allowlist (web: https only + disallow localhost; native:
custom scheme OR http loopback only).
##########
rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/DynamicRegistrationService.java:
##########
@@ -366,12 +372,44 @@ protected boolean isPasswordRequired(List<String>
grantTypes, String tokenEndpoi
||
OAuthConstants.TOKEN_ENDPOINT_AUTH_POST.equals(tokenEndpointAuthMethod));
}
+ @SuppressWarnings("PMD.CollapsibleIfStatements")
protected void validateRequestUri(String uri, String appType, List<String>
grantTypes) {
+ if (uri == null || uri.isBlank()) {
+ throw new OAuthServiceException("Empty redirect URI are not
supported");
+ }
+
// Web Clients using the OAuth Implicit Grant Type MUST only register
URLs using the https scheme
// as redirect_uris; they MUST NOT use localhost as the hostname.
Native Clients MUST only register
- // redirect_uris using custom URI schemes or URLs using the http:
scheme with localhost as the hostname.
+ // redirect_uris using custom URI schemes or loopback URLs using the
http scheme; loopback URLs use
+ // localhost or the IP loopback literals 127.0.0.1 or [::1] as the
hostname.
// Authorization Servers MAY place additional constraints on Native
Clients. Authorization Servers MAY
// reject Redirection URI values using the http scheme, other than the
localhost case for Native Clients
+
+ final URI parsedUri = URI.create(uri);
+ if (parsedUri.getHost() == null || parsedUri.getScheme() == null) {
+ throw new OAuthServiceException("Unsupported redirect URI
scheme/host");
+ }
+ final String host = parsedUri.getHost().toLowerCase();
+ final String scheme = parsedUri.getScheme().toLowerCase();
+ if (!URIResolver.getAllowedSchemes().contains(scheme)) {
+ throw new OAuthServiceException("Redirect URI scheme is not
allowed: " + scheme
+ + "Allowed schemes: " + URIResolver.getAllowedSchemes());
+ }
+
+ // Kind of the application. The default, if omitted, is web. The
defined values are native or web.
+ if (appType == null || appType.isBlank() ||
appType.equalsIgnoreCase(WEB_APPLICATION_TYPE)) {
+ if (grantTypes.contains(OAuthConstants.IMPLICIT_GRANT)) {
+ if (!"https".equalsIgnoreCase(scheme)) {
+ throw new OAuthServiceException("Unsupported redirect URI
scheme");
+ } else if ("localhost".equals(host)) {
+ throw new OAuthServiceException("Unsupported redirect URI
hostname");
+ }
Review Comment:
These validation failures throw `OAuthServiceException` (an unchecked
exception with no JAX-RS `ExceptionMapper` in CXF OAuth2), which will likely
surface as a 500 response instead of the RFC7591/OIDC-required 400
`invalid_client_metadata` error body (as `validateClientScopes` does via
`reportInvalidRequestError`). Consider converting redirect URI validation
failures to `reportInvalidRequestError(new OAuthError(INVALID_CLIENT_METADATA,
...))` for consistent client-visible behavior.
--
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]
