Repository navigation
Add OAuth 2.0 compliant redirect URI validation #47320
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -43,6 +43,7 @@ | |||||||||||||||||||
| import static org.keycloak.services.clientpolicy.executor.SecureRedirectUrisEnforcerExecutorFactory.ALLOW_PERMITTED_DOMAINS; | ||||||||||||||||||||
| import static org.keycloak.services.clientpolicy.executor.SecureRedirectUrisEnforcerExecutorFactory.ALLOW_PRIVATE_USE_URI_SCHEME; | ||||||||||||||||||||
| import static org.keycloak.services.clientpolicy.executor.SecureRedirectUrisEnforcerExecutorFactory.ALLOW_WILDCARD_CONTEXT_PATH; | ||||||||||||||||||||
| import static org.keycloak.services.clientpolicy.executor.SecureRedirectUrisEnforcerExecutorFactory.OAUTH_2_0_COMPLIANT; | ||||||||||||||||||||
| import static org.keycloak.services.clientpolicy.executor.SecureRedirectUrisEnforcerExecutorFactory.OAUTH_2_1_COMPLIANT; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import static org.junit.Assert.assertEquals; | ||||||||||||||||||||
|
|
@@ -76,6 +77,7 @@ public void defaultConfiguration() { | |||||||||||||||||||
| assertFalse(configuration.isAllowPrivateUseUriScheme()); | ||||||||||||||||||||
| assertFalse(configuration.isAllowHttpScheme()); | ||||||||||||||||||||
| assertFalse(configuration.isAllowWildcardContextPath()); | ||||||||||||||||||||
| assertFalse(configuration.isOAuth2_0Compliant()); | ||||||||||||||||||||
| assertFalse(configuration.isOAuth2_1Compliant()); | ||||||||||||||||||||
| assertFalse(configuration.isAllowOpenRedirect()); | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -339,6 +341,72 @@ public void successAllConfigurationDisabled() { | |||||||||||||||||||
| ).forEach(i->checkSuccess(i, false)); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @Test | ||||||||||||||||||||
| public void failNormalUri_OAuth2_0Compliant() { | ||||||||||||||||||||
| enable(OAUTH_2_0_COMPLIANT); | ||||||||||||||||||||
| Stream.of( | ||||||||||||||||||||
| "https://oauth.redirect:8443/auth/callback#fragment", | ||||||||||||||||||||
| "https://example.org/path#frag", | ||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new OAuth 2.0 compliance tests include a wildcard case ("https://example.org/*"), but ALLOW_WILDCARD_CONTEXT_PATH is not enabled here, so the wildcard would already be rejected even without the new OAuth 2.0 compliance logic. To actually cover the new behavior (OAuth 2.0 compliance overriding the wildcard-allowed setting), add a case that enables ALLOW_WILDCARD_CONTEXT_PATH and asserts that a wildcard redirect URI still fails when OAUTH_2_0_COMPLIANT is enabled.
Suggested change
|
||||||||||||||||||||
| "https://example.org/*" | ||||||||||||||||||||
| ).forEach(i->checkFail(i, false, SecureRedirectUrisEnforcerExecutor.ERR_NORMALURI)); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @Test | ||||||||||||||||||||
| public void successNormalUri_OAuth2_0Compliant() { | ||||||||||||||||||||
| enable(OAUTH_2_0_COMPLIANT); | ||||||||||||||||||||
| // HTTP is still allowed (unlike OAuth 2.1) when allow-http-scheme is on | ||||||||||||||||||||
| enable(ALLOW_HTTP_SCHEME); | ||||||||||||||||||||
| Stream.of( | ||||||||||||||||||||
| "https://example.org/redirect", | ||||||||||||||||||||
| "http://example.org/redirect", | ||||||||||||||||||||
| "https://example.org:8443/auth/callback" | ||||||||||||||||||||
| ).forEach(i->checkSuccess(i, false)); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @Test | ||||||||||||||||||||
| public void failLoopbackAddress_OAuth2_0Compliant() { | ||||||||||||||||||||
| enable(ALLOW_IPV4_LOOPBACK_ADDRESS, ALLOW_IPV6_LOOPBACK_ADDRESS, ALLOW_HTTP_SCHEME, OAUTH_2_0_COMPLIANT); | ||||||||||||||||||||
| Stream.of( | ||||||||||||||||||||
| "http://127.0.0.1/auth/admin#fragment", | ||||||||||||||||||||
| "http://[::1]/auth/admin#fragment", | ||||||||||||||||||||
| "http://127.0.0.1/*", | ||||||||||||||||||||
| "http://[::1]/*" | ||||||||||||||||||||
| ).forEach(i->checkFail(i, false, SecureRedirectUrisEnforcerExecutor.ERR_LOOPBACK)); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @Test | ||||||||||||||||||||
| public void successLoopbackAddress_OAuth2_0Compliant() { | ||||||||||||||||||||
| enable(ALLOW_IPV4_LOOPBACK_ADDRESS, ALLOW_IPV6_LOOPBACK_ADDRESS, ALLOW_HTTP_SCHEME, OAUTH_2_0_COMPLIANT); | ||||||||||||||||||||
| // localhost is still allowed (unlike OAuth 2.1) | ||||||||||||||||||||
| Stream.of( | ||||||||||||||||||||
| "http://localhost/auth/admin", | ||||||||||||||||||||
| "http://localhost:8080/auth/admin", | ||||||||||||||||||||
| "http://127.0.0.1:8080/auth/admin", | ||||||||||||||||||||
| "http://127.0.0.1/auth/admin", | ||||||||||||||||||||
| "http://[::1]:8080/auth/admin", | ||||||||||||||||||||
| "http://[::1]/auth/admin" | ||||||||||||||||||||
| ).forEach(i->checkSuccess(i, false)); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @Test | ||||||||||||||||||||
| public void failPrivateUseUriScheme_OAuth2_0Compliant() { | ||||||||||||||||||||
| enable(ALLOW_PRIVATE_USE_URI_SCHEME, OAUTH_2_0_COMPLIANT); | ||||||||||||||||||||
| Stream.of( | ||||||||||||||||||||
| "myapp:/oauth.redirect#pinpoint", | ||||||||||||||||||||
| "com.example.app:/oauth.redirect/*" | ||||||||||||||||||||
| ).forEach(i->checkFail(i, false, SecureRedirectUrisEnforcerExecutor.ERR_PRIVATESCHEME)); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @Test | ||||||||||||||||||||
| public void successPrivateUseUriScheme_OAuth2_0Compliant() { | ||||||||||||||||||||
| enable(ALLOW_PRIVATE_USE_URI_SCHEME, OAUTH_2_0_COMPLIANT); | ||||||||||||||||||||
| // single-word scheme is still allowed (unlike OAuth 2.1) | ||||||||||||||||||||
| Stream.of( | ||||||||||||||||||||
| "com.example.app:/oauth2redirect/example-provider", | ||||||||||||||||||||
| "myapp:/oauth.redirect" | ||||||||||||||||||||
| ).forEach(i->checkSuccess(i, false)); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private void permittedDomains(String... domains) { | ||||||||||||||||||||
| ArrayNode arrayNode = JsonSerialization.mapper.createArrayNode(); | ||||||||||||||||||||
| Arrays.stream(domains).forEach(arrayNode::add); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -474,7 +474,7 @@ public void testSecureRedirectUrisEnforcerExecutor_AllowPermittedDomains() throw | |
| } | ||
|
|
||
| @Test | ||
| public void testSecureRedirectUrisEnforcerExecutor_OAuth2_1Complient() throws Exception { | ||
| public void testSecureRedirectUrisEnforcerExecutor_OAuth2_1Compliant() throws Exception { | ||
| // register profiles | ||
| String json = (new ClientPoliciesUtil.ClientProfilesBuilder()).addProfile( | ||
| (new ClientPoliciesUtil.ClientProfileBuilder()).createProfile(PROFILE_NAME, "Le Premier Profil") | ||
|
|
@@ -522,6 +522,62 @@ public void testSecureRedirectUrisEnforcerExecutor_OAuth2_1Complient() throws Ex | |
| testSecureRedirectUrisEnforcerExecutor_failAuthorizationRequest(alphaClientId, "com.example.app:/oauth3redirect/example-provider"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSecureRedirectUrisEnforcerExecutor_OAuth2_0Compliant() throws Exception { | ||
| // register profiles - OAuth 2.0 compliant rejects fragments and wildcards | ||
| // but is less strict than OAuth 2.1 (allows localhost, HTTP, single-word schemes) | ||
| String json = (new ClientPoliciesUtil.ClientProfilesBuilder()).addProfile( | ||
| (new ClientPoliciesUtil.ClientProfileBuilder()).createProfile(PROFILE_NAME, "Le Premier Profil") | ||
| .addExecutor(SecureRedirectUrisEnforcerExecutorFactory.PROVIDER_ID, | ||
| createSecureRedirectUrisEnforcerExecutorConfig(it->{ | ||
| it.setAllowIPv4LoopbackAddress(true); | ||
| it.setAllowIPv6LoopbackAddress(true); | ||
| it.setAllowHttpScheme(true); | ||
| it.setOAuth2_0Compliant(true);})) | ||
| .toRepresentation() | ||
| ).toString(); | ||
| updateProfiles(json); | ||
|
|
||
| // register policies | ||
| json = (new ClientPoliciesUtil.ClientPoliciesBuilder()).addPolicy( | ||
| (new ClientPoliciesUtil.ClientPolicyBuilder()).createPolicy(POLICY_NAME, "La Premiere Politique", Boolean.TRUE) | ||
| .addCondition(AnyClientConditionFactory.PROVIDER_ID, | ||
| createAnyClientConditionConfig()) | ||
| .addProfile(PROFILE_NAME) | ||
| .toRepresentation() | ||
| ).toString(); | ||
| updatePolicies(json); | ||
|
|
||
| // register - fail | ||
| // fragment in redirect URI not allowed per RFC 6749 | ||
| testSecureRedirectUrisEnforcerExecutor_failRegisterByAdmin(List.of("https://example.com/callback#fragment")); | ||
|
|
||
| // register - fail | ||
| // wildcard in redirect URI not allowed | ||
| testSecureRedirectUrisEnforcerExecutor_failRegisterByAdmin(List.of("https://example.com/*")); | ||
|
Comment on lines
+525
to
+557
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This OAuth 2.0 compliance integration test checks that a wildcard redirect URI ("https://example.com/*") fails, but the profile config here does not enable allow-wildcard-context-path, so this failure would happen even without the new OAuth 2.0 compliant flag. To validate the new behavior, add a step that enables allow-wildcard-context-path and still asserts wildcard redirect URIs are rejected when OAuth 2.0 Compliant is ON. |
||
|
|
||
| // register - success | ||
| // localhost is still allowed (unlike OAuth 2.1) | ||
| List<String> registerResultList = testSecureRedirectUrisEnforcerExecutor_successRegisterByAdmin( | ||
| List.of("http://localhost/auth/admin")); | ||
| String alphaClientId = registerResultList.get(0); | ||
| String alphaCid = registerResultList.get(1); | ||
|
|
||
| // update - fail | ||
| // fragment not allowed | ||
| testSecureRedirectUrisEnforcerExecutor_failUpdateByAdmin(alphaCid, | ||
| List.of("http://localhost/auth/admin#frag")); | ||
|
|
||
| // update - success | ||
| // HTTP scheme and localhost are allowed under OAuth 2.0 (unlike OAuth 2.1) | ||
| testSecureRedirectUrisEnforcerExecutor_successUpdateByAdmin(alphaCid, | ||
| List.of("http://localhost/auth/redirect", "http://127.0.0.1:8080/callback")); | ||
|
|
||
| // authorization request - fail | ||
| // redirect_uri not match with registered redirect uris | ||
| testSecureRedirectUrisEnforcerExecutor_failAuthorizationRequest(alphaClientId, "http://localhost/auth/other"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSecureRedirectUrisEnforcerExecutor_AllowOpenRedirect() throws Exception { | ||
| // Allow open redirect | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this new config property's help text, "If On" is inconsistent with the capitalization used in the other executor properties in this same factory (they use "If ON"). Consider standardizing the wording/capitalization for consistency in the admin UI text.