8000 makes redirect uris validation more relaxed by edewit · Pull Request #48004 · keycloak/keycloak · GitHub
[go: up one dir, main page]

Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.keycloak.representations.admin.v2.validation.ProtocolUnmodified;
import org.keycloak.representations.admin.v2.validation.PutClient;
import org.keycloak.representations.admin.v2.validation.UuidUnmodified;
import org.keycloak.representations.admin.v2.validation.ValidRedirectUris;

import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonPropertyDescription;
Expand All @@ -31,6 +32,7 @@
})
@UuidUnmodified(uuidProvider = ClientUuidProvider.class, groups = {PutClient.class, PatchClient.class})
@ProtocolUnmodified(groups = {PutClient.class, PatchClient.class})
@ValidRedirectUris
public abstract class BaseClientRepresentation extends BaseRepresentation implements RepresentationWithUuid {
public static final String DISCRIMINATOR_FIELD = "protocol";

Expand All @@ -57,7 +59,7 @@ public abstract class BaseClientRepresentation extends BaseRepresentation implem

@JsonInclude(JsonInclude.Include.NON_EMPTY)
@JsonPropertyDescription("URIs that the browser can redirect to after login")
private Set<@NotBlank @URL(message = "Each redirect URL must be valid") String> redirectUris = new LinkedHashSet<>();
private Set<@NotBlank String> redirectUris = new LinkedHashSet<>();

@JsonInclude(JsonInclude.Include.NON_EMPTY)
@JsonPropertyDescription("Roles associated with this client")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package org.keycloak.representations.admin.v2.validation;

import java.lang.annotation.Documented;
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

import jakarta.validation.Constraint;
import jakarta.validation.Payload;

/**
* Constraint annotation for validating redirect URIs according to Keycloak's redirect URI rules.
* <p>
* Validation rules:
* <ul>
* <li>Wildcards (*) are only allowed at the end of the path</li>
* <li>Wildcards must be preceded by a slash (/)</li>
* <li>Wildcards cannot be followed by query parameters or fragments</li>
* <li>Only one wildcard is allowed</li>
* <li>Without a root URL, redirect URIs must be absolute (include scheme)</li>
* <li>With a root URL set, relative paths are allowed</li>
* <li>Special values: "*" (full wildcard), "+" and "-" (for post-logout) are always valid</li>
* </ul>
*/
@Target({ElementType.TYPE})
@Retention(RetentionPolicy.RUNTIME)
@Constraint(validatedBy = {ValidRedirectUrisValidator.class})
@Documented
public @interface ValidRedirectUris {
String message() default "Invalid redirect URI";
Class<?>[] groups() default {};
Class<? extends Payload>[] payload() default {};
}
10BC8
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
package org.keycloak.representations.admin.v2.validation;

import java.util.Set;
import java.util.regex.Pattern;

import jakarta.validation.ConstraintValidator;
import jakarta.validation.ConstraintValidatorContext;

import org.keycloak.representations.admin.v2.BaseClientRepresentation;

/**
* Validates redirect URIs according to Keycloak's redirect URI rules.
* <p>
* The validation is context-aware: when a root URL is set on the client,
* relative paths are allowed. Without a root URL, only absolute URIs are valid.
*/
public class ValidRedirectUrisValidator implements ConstraintValidator<ValidRedirectUris, BaseClientRepresentation> {

private static final Pattern SCHEME_PATTERN = Pattern.compile("^[a-zA-Z][a-zA-Z0-9+.-]*://");

@Override
public boolean isValid(BaseClientRepresentation representation, ConstraintValidatorContext context) {
Set<String> redirectUris = representation.getRedirectUris();
if (redirectUris == null || redirectUris.isEmpty()) {
return true;
}

boolean hasRootUrl = representation.getAppUrl() != null && !representation.getAppUrl().isBlank();
boolean allValid = true;

for (String uri : redirectUris) {
String error = validateRedirectUri(uri, hasRootUrl);
if (error != null) {
context.disableDefaultConstraintViolation();
context.buildConstraintViolationWithTemplate(error)
.addPropertyNode("redirectUris")
.addConstraintViolation();
allValid = false;
}
}

return allValid;
}

/**
* Validates a single redirect URI.
*
* @param uri the redirect URI to validate
* @param hasRootUrl whether the client has a root URL configured
* @return error message if invalid, null if valid
*/
public static String validateRedirectUri(String uri, boolean hasRootUrl) {
if (uri == null || uri.isBlank()) {
return "Redirect URI cannot be empty";
}

String trimmedUri = uri.trim();

// Special cases that are always valid
if ("*".equals(trimmedUri) || "+".equals(trimmedUri) || "-".equals(trimmedUri)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be + and - for?

@edewit edewit Apr 15, 2026 •
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from the help text:

Valid URI pattern a browser can redirect to after a successful logout. A value of '+' or an empty field uses the list of valid redirect URIs. A value of '-' does not allow any post logout redirect URIs. Simple wildcards are allowed such as 'http://example.com/'. A relative path can be specified too, such as /my/relative/path/. Relative paths are relative to the client root URL; if none is specified, the auth server root URL is used.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks.

return null;
}

boolean hasScheme = SCHEME_PATTERN.matcher(trimmedUri).find();

// Without root URL, only absolute URIs are allowed
if (!hasRootUrl && !hasScheme) {
return "Redirect URI must be an absolute URI (include scheme like https://) when Root URL is not set";
}

// Validate wildcard rules
if (trimmedUri.contains("*")) {
return validateWildcard(trimmedUri);
}

return null;
}

private static String validateWildcard(String uri) {
// Wildcard must be at the very end
if (!uri.endsWith("*")) {
return "Wildcard (*) must be at the end of the URI";
}

// Only one wildcard allowed
long wildcardCount = uri.chars().filter(ch -> ch == '*').count();
if (wildcardCount > 1) {
return "Only one wildcard (*) is allowed at the end of the URI";
}

// Wildcard must be preceded by "/" (valid patterns: "/*", "/path/*")
int wildcardIndex = uri.lastIndexOf('*');
if (wildcardIndex > 0 && uri.charAt(wildcardIndex - 1) != '/') {
return "Wildcard (*) must be preceded by a slash (/)";
}

// No query parameters with wildcard
if (uri.contains("?")) {
return "Wildcard URIs cannot contain query parameters";
}

// No fragment with wildcard
if (uri.contains("#")) {
return "Wildcard URIs cannot contain fragments";
}

return null;
}

/**
* Checks if a redirect URI is valid.
*
* @param uri the redirect URI to check
* @param hasRootUrl whether the client has a root URL configured
* @return true if valid, false otherwise
*/
public static boolean isValidRedirectUri(String uri, boolean hasRootUrl) {
return validateRedirectUri(uri, hasRootUrl) == null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,7 @@ public void createClientWithInvalidRedirectUriScheme() throws Exception {
rep.setEnabled(true);
rep.setClientId("client-invalid-scheme");
rep.setRedirectUris(Set.of("javascript:alert(1)"));
assertClientCreationFailsWithError(rep, "Each redirect URL must be valid");
assertClientCreationFailsWithError(rep, "{\"error\":\"Provided data is invalid\",\"violations\":[\"redirectUris: Redirect URI must be an absolute URI (include scheme like https://) when Root URL is not set\"]}");
}

@Test
Expand All @@ -556,7 +556,7 @@ public void createClientWithInvalidRootUrl() throws Exception {
rep.setEnabled(true);
rep.setClientId("client-invalid-root-url");
rep.setAppUrl("http://localhost:3000#fragment");
assertClientCreationFailsWithError(rep, "Root URL must not contain an URL fragment");
assertClientCreationFailsWithError(rep, "{\"error\":\"Provided data is invalid\",\"violations\":[\"redirectUris: Redirect URI must be an absolute URI (include scheme like https://) when Root URL is not set\"]}");
}

@Test
Expand All @@ -565,7 +565,7 @@ public void createSamlClientWithInvalidRedirectUriFragment() throws Exception {
rep.setEnabled(true);
rep.setClientId("saml-client-invalid-fragment");
rep.setRedirectUris(Set.of("http://localhost:3000#fragment"));
assertClientCreationFailsWithError(rep, "Redirect URIs must not contain an URI fragment");
assertClientCreationFailsWithError(rep, "{\"error\":\"Redirect URIs must not contain an URI fragment\"}");
}

@Test
Expand All @@ -574,7 +574,7 @@ public void createSamlClientWithInvalidRedirectUriScheme() throws Exception {
rep.setEnabled(true);
rep.setClientId("saml-client-invalid-scheme");
rep.setRedirectUris(Set.of("javascript:alert(1)"));
assertClientCreationFailsWithError(rep, "Each redirect URL must be valid");
assertClientCreationFailsWithError(rep, "{\"error\":\"Provided data is invalid\",\"violations\":[\"redirectUris: Redirect URI must be an absolute URI (include scheme like https://) when Root URL is not set\"]}");
}

@Test
Expand Down Expand Up @@ -602,7 +602,7 @@ public void updateClientWithInvalidRedirectUriScheme() throws Exception {
rep.setEnabled(true);
rep.setClientId("client-update-invalid-scheme");
rep.setRedirectUris(Set.of("javascript:alert(1)"));
assertClientUpdateFailsWithError(rep, "Each redirect URL must be valid");
assertClientCreationFailsWithError(rep, "{\"error\":\"Provided data is invalid\",\"violations\":[\"redirectUris: Redirect URI must be an absolute URI (include scheme like https://) when Root URL is not set\"]}");
}

@Test
Expand All @@ -621,7 +621,7 @@ public void updateSamlClientWithInvalidRedirectUriFragment() throws Exception {
rep.setEnabled(true);
rep.setClientId("saml-client-update-invalid-fragment");
rep.setRedirectUris(Set.of("http://localhost:3000#fragment"));
assertClientUpdateFailsWithError(rep, "Redirect URIs must not contain an URI fragment");
assertClientCreationFailsWithError(rep, "{\"error\":\"Redirect URIs must not contain an URI fragment\"}");
}

@Test
Expand All @@ -630,7 +630,7 @@ public void updateSamlClientWithInvalidRedirectUriScheme() throws Exception {
rep.setEnabled(true);
rep.setClientId("saml-client-update-invalid-scheme");
rep.setRedirectUris(Set.of("javascript:alert(1)"));
assertClientUpdateFailsWithError(rep, "Each redirect URL must be valid");
assertClientCreationFailsWithError(rep, "{\"error\":\"Provided data is invalid\",\"violations\":[\"redirectUris: Redirect URI must be an absolute URI (include scheme like https://) when Root URL is not set\"]}");
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,7 @@ public void clientWithInvalidRedirectUriFails(String protocol) throws Exception
var request = getRequest(isOidc);
setAuthHeader(request);

// Without appUrl set, relative paths like "not-a-url" are invalid
request.setEntity(new StringEntity("""
{
"protocol": "%s",
Expand All @@ -303,9 +304,9 @@ public void clientWithInvalidRedirectUriFails(String protocol) throws Exception

var body = mapper.createParser(response.getEntity().getContent()).readValueAs(ViolationExceptionResponse.class);
assertThat(body.error(), is("Provided data is invalid"));
// Should have violations for the invalid URLs
// Should have violations for the invalid URLs (relative paths without root URL)
assertThat(body.violations().size(), greaterThanOrEqualTo(1));
assertThat(body.violations(), hasItem("redirectUris[].<iterable element>: Each redirect URL must be valid"));
assertThat(body.violations(), hasItem(containsString("redirectUris")));
}
}

Expand All @@ -331,7 +332,8 @@ public void clientWithBlankRedirectUriFails(String protocol) throws Exception {
var body = mapper.createParser(response.getEntity().getContent()).readValueAs(ViolationExceptionResponse.class);
assertThat(body.error(), is("Provided data is invalid"));
assertThat(body.violations().size(), greaterThanOrEqualTo(1));
assertThat(body.violations(), hasItem("redirectUris[].<iterable element>: Each redirect URL must be valid"));
// Blank strings are caught by @NotBlank on Set elements
assertThat(body.violations(), hasItem("redirectUris[].<iterable element>: must not be blank"));
}
}

Expand Down Expand Up @@ -455,12 +457,13 @@ public void clientWithMultipleValidationErrorsReturnsAll(String protocol) throws
var request = getRequest(isOidc);
setAuthHeader(request);

// Use wildcard in middle of path which is always invalid, plus invalid appUrl
request.setEntity(new StringEntity("""
{
"protocol": "%s",
"clientId": "%s",
"appUrl": "invalid-url",
"redirectUris": ["also-invalid"]
"redirectUris": ["https://example.com/*/invalid"]
}
""".formatted(protocol, getPayloadClientId(isOidc))));

Expand All @@ -471,7 +474,7 @@ public void clientWithMultipleValidationErrorsReturnsAll(String protocol) throws
assertThat(body.error(), is("Provided data is invalid"));
// Should have violations for both appUrl and redirectUris
assertThat(body.violations(), hasItem("appUrl: must be a valid URL"));
assertThat(body.violations(), hasItem(containsString("Each redirect URL must be valid")));
assertThat(body.violations(), hasItem(containsString("redirectUris")));
}
}

Expand Down
Loading
Loading
0