8000 [OAuth] Prefer authorization error on redirect_uri by tdiesler · Pull Request #48553 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

[OAuth] Prefer authorization error on redirect_uri #48553

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.

Sign up for GitHub

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

Closed
tdiesler wants to merge 1 commit into from
Closed

[OAuth] Prefer authorization error on redirect_uri #48553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import org.keycloak.models.UserSessionModel;
import org.keycloak.protocol.ClientData;
import org.keycloak.protocol.LoginProtocol;
import org.keycloak.protocol.oidc.endpoints.AuthorizationCheckException;
import org.keycloak.protocol.oidc.endpoints.AuthorizationEndpointChecker;
import org.keycloak.protocol.oidc.endpoints.request.AuthorizationEndpointRequest;
import org.keycloak.protocol.oidc.utils.LogoutUtil;
Expand All @@ -57,6 +58,7 @@
import org.keycloak.representations.AccessTokenResponse;
import org.keycloak.representations.adapters.action.PushNotBeforeAction;
import org.keycloak.representations.idm.OAuth2ErrorRepresentation;
import org.keycloak.services.ErrorPageException;
import org.keycloak.services.ServicesLogger;
import org.keycloak.services.Urls;
import org.keycloak.services.clientpolicy.ClientPolicyException;
Expand Down Expand Up @@ -436,8 +438,8 @@ public Response sendError(ClientModel client, ClientData clientData, Error error
try {
checker.checkResponseType();
checker.checkRedirectUri();
} catch (AuthorizationEndpointChecker.AuthorizationCheckException ex) {
ex.throwAsErrorPageException(null);
} catch (AuthorizationCheckException ex) {
throw new ErrorPageException(session, ex.getError(), ex.getStatus(), ex.getErrorMessage(), ex.getParameters());
}

setupResponseTypeAndMode(clientData.getResponseType(), clientData.getResponseMode());
Expand Down
49 changes: 49 additions & 0 deletions services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationCheckException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package org.keycloak.protocol.oidc.endpoints;

import java.text.MessageFormat;

import jakarta.ws.rs.core.Response;

// Exception propagated to the caller, which will allow caller to send proper error response based on the context (Browser OIDC Authorization Endpoint, PAR etc)
public class AuthorizationCheckException extends Exception {

private final Response.Status status;
private final String error;
private final String errorMessage;
private final Object[] parameters;

public AuthorizationCheckException(Response.Status status, String error, String errorDescription) {
this(error, status, errorDescription);
}

public AuthorizationCheckException(String error, Response.Status status, String errorMessage, Object... params) {
this.status = status;
this.error = error;
this.errorMessage = errorMessage;
this.parameters = params;
}

public Response.Status getStatus() {
return status;
}

public String getError() {
return error;
}

public String getErrorDescription() {
String errorDescription = errorMessage;
if (parameters.length > 0) {
errorDescription = MessageFormat.format(errorMessage, parameters);
}
return errorDescription;
}

public String getErrorMessage() {
return errorMessage;
}

public Object[] getParameters() {
return parameters;
}
}
177 changes: 133 additions & 44 deletions services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,12 @@

package org.keycloak.protocol.oidc.endpoints;

import java.io.IOException;
import java.text.MessageFormat;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Properties;
import java.util.function.BiConsumer;

import jakarta.ws.rs.Consumes;
Expand Down Expand Up @@ -48,12 +52,14 @@
import org.keycloak.protocol.oidc.OIDCLoginProtocol;
import org.keycloak.protocol.oidc.endpoints.request.AuthorizationEndpointRequest;
import org.keycloak.protocol.oidc.endpoints.request.AuthorizationEndpointRequestParserProcessor;
import org.keycloak.protocol.oidc.endpoints.request.AuthorizationEndpointRequestParserProcessor.EndpointType;
import org.keycloak.protocol.oidc.endpoints.request.RequestUriType;
import org.keycloak.protocol.oidc.grants.device.endpoints.DeviceEndpoint;
import org.keycloak.protocol.oidc.utils.AcrUtils;
import org.keycloak.protocol.oidc.utils.OIDCRedirectUriBuilder;
import org.keycloak.protocol.oidc.utils.OIDCResponseMode;
import org.keycloak.protocol.oidc.utils.OIDCResponseType;
import org.keycloak.representations.idm.OAuth2ErrorRepresentation;
import org.keycloak.services.ErrorPageException;
import org.keycloak.services.Urls;
import org.keycloak.services.clientpolicy.ClientPolicyException;
8000 Expand All @@ -65,6 +71,7 @@
import org.keycloak.services.util.CacheControlUtil;
import org.keycloak.services.util.LocaleUtil;
import org.keycloak.sessions.AuthenticationSessionModel;
import org.keycloak.theme.Theme;
import org.keycloak.util.TokenUtil;

import org.jboss.logging.Logger;
Expand All @@ -80,6 +87,9 @@ public class AuthorizationEndpoint extends AuthorizationEndpointBase {

public static final String CODE_AUTH_TYPE = "code";

// Prefer error delivery on `redirect_uri` - rather than as HTML error page
public static final String AUTHORIZATION_PREFER_ERROR_ON_REDIRECT = "authorization.preferErrorOnRedirect";

/**
* Prefix used to store additional HTTP GET params from original client request into {@link AuthenticationSessionModel} note to be available later in Authenticators, RequiredActions etc. Prefix is used to
* prevent collisions with internally used notes.
Expand All @@ -93,14 +103,15 @@ private enum Action {
}

private ClientModel client;
private AuthorizationEndpointChecker checker;
private AuthenticationSessionModel authenticationSession;

private Action action;
private OIDCResponseType parsedResponseType;
private OIDCResponseMode parsedResponseMode;

private AuthorizationEndpointRequest request;
private String redirectUri;
private String parsedRedirectUri;

public AuthorizationEndpoint(KeycloakSession session, EventBuilder event) {
super(session, event);
Expand Down Expand Up @@ -132,52 +143,58 @@ public Object authorizeDevice() {
}

private Response process(final MultivaluedMap<String, String> params) {
String clientId = AuthorizationEndpointRequestParserProcessor.getClientId(event, session, params);

checkSsl();
checkRealm();

try {
session.clientPolicy().triggerOnEvent(new PreAuthorizationRequestContext(clientId, params));
} catch (ClientPolicyException cpe) {
event.detail(Details.REASON, Details.CLIENT_POLICY_ERROR);
event.detail(Details.CLIENT_POLICY_ERROR, cpe.getError());
event.detail(Details.CLIENT_POLICY_ERROR_DETAIL, cpe.getErrorDetail());
event.error(cpe.getError());
throw new ErrorPageException(session, authenticationSession, cpe.getErrorStatus(), cpe.getErrorDetail());
}
checkClient(clientId);
String clientId = AuthorizationEndpointRequestParserProcessor.getClientId(event, session, params);

request = AuthorizationEndpointRequestParserProcessor.parseRequest(event, session, client, params, AuthorizationEndpointRequestParserProcessor.EndpointType.OIDC_AUTH_ENDPOINT);
checkSsl();
checkRealm();

AuthorizationEndpointChecker checker = new AuthorizationEndpointChecker()
.event(event)
.client(client)
.realm(realm)
.request(request)
.session(session)
.params(params);
try {
session.clientPolicy().triggerOnEvent(new PreAuthorizationRequestContext(clientId, params));
} catch (ClientPolicyException cpe) {
event.detail(Details.REASON, Details.CLIENT_POLICY_ERROR);
event.detail(Details.CLIENT_POLICY_ERROR, cpe.getError());
event.detail(Details.CLIENT_POLICY_ERROR_DETAIL, cpe.getErrorDetail());
event.error(cpe.getError());
throw new ErrorPageException(session, cpe.getError(), cpe.getErrorStatus(), cpe.getErrorDetail());
}
checkClient(clientId);

request = AuthorizationEndpointRequestParserProcessor.parseRequest(event, session, client, params, EndpointType.OIDC_AUTH_ENDPOINT);

} catch (ErrorPageException ex) {
buildAuthorizationEndpointChecker(params);
return handleErrorPageException(ex);
}

buildAuthorizationEndpointChecker(params);

try {
checker.checkRedirectUri();
this.redirectUri = checker.getRedirectUri();
} catch (AuthorizationEndpointChecker.AuthorizationCheckException ex) {
ex.throwAsErrorPageException(authenticationSession);
parsedRedirectUri = checker.getRedirectUri();
} catch (AuthorizationCheckException ex) {
ErrorPageException epex = new ErrorPageException(session, ex.getError(), ex.getStatus(), ex.getErrorMessage(), ex.getParameters());
return handleErrorPageException(epex);
}

try {
checker.checkResponseType();
this.parsedResponseType = checker.getParsedResponseType();
this.parsedResponseMode = checker.getParsedResponseMode();
} catch (AuthorizationEndpointChecker.AuthorizationCheckException ex) {
OIDCResponseMode responseMode;
parsedResponseType = checker.getParsedResponseType();
parsedResponseMode = checker.getParsedResponseMode();
} catch (AuthorizationCheckException ex) {
if (checker.isInvalidResponseType(ex)) {
responseMode = OIDCResponseMode.parseWhenInvalidResponseType(request.getResponseMode());
} else {
responseMode = checker.getParsedResponseMode() != null ? checker.getParsedResponseMode() : OIDCResponseMode.QUERY;
parsedResponseMode = OIDCResponseMode.parseWhenInvalidResponseType(request.getResponseMode());
}
if (parsedResponseMode == null) {
parsedResponseMode = checker.getParsedResponseMode();
}
return redirectErrorToClient(responseMode, ex.getError(), ex.getErrorDescription());
if (parsedResponseMode == null) {
parsedResponseMode = OIDCResponseMode.QUERY;
}
return handleRedirectErrorToClient(ex.getError(), ex.getErrorDescription());
}

if (action == null) {
action = AuthorizationEndpoint.Action.CODE;
}
Expand All @@ -191,8 +208,8 @@ private Response process(final MultivaluedMap<String, String> params) {
checker.checkValidResource();
checker.checkOIDCParams();
checker.checkPKCEParams();
} catch (AuthorizationEndpointChecker.AuthorizationCheckException ex) {
return redirectErrorToClient(parsedResponseMode, ex.getError(), ex.getErrorDescription());
} catch (AuthorizationCheckException ex) {
return handleRedirectErrorToClient(ex.getError(), ex.getErrorDescription());
}

// If DPoP Proof existed with PAR request, its public key needs to be matched with the one with Token Request afterward
Expand All @@ -205,14 +222,14 @@ private Response process(final MultivaluedMap<String, String> params) {
authenticationSession = createAuthenticationSession(client, request.getState());

try {
session.clientPolicy().triggerOnEvent(new AuthorizationRequestContext(parsedResponseType, request, redirectUri, params, authenticationSession));
session.clientPolicy().triggerOnEvent(new AuthorizationRequestContext(parsedResponseType, request, parsedRedirectUri, params, authenticationSession));
} catch (ClientPolicyException cpe) {
event.detail(Details.REASON, Details.CLIENT_POLICY_ERROR);
event.detail(Details.CLIENT_POLICY_ERROR, cpe.getError());
event.detail(Details.CLIENT_POLICY_ERROR_DETAIL, cpe.getErrorDetail());
event.error(cpe.getError());
new AuthenticationSessionManager(session).removeAuthenticationSession(realm, authenticationSession, false);
return redirectErrorToClient(parsedResponseMode, cpe.getError(), cpe.getErrorDetail());
return handleRedirectErrorToClient(cpe.getError(), cpe.getErrorDetail());
}

updateAuthenticationSession();
Expand Down Expand Up @@ -241,6 +258,19 @@ private Response process(final MultivaluedMap<String, String> params) {
throw new RuntimeException("Unknown action " + action);
}

private AuthorizationEndpointChecker buildAuthorizationEndpointChecker(MultivaluedMap<String, String> params) {
if (checker == null) {
checker = new AuthorizationEndpointChecker()
.event(event)
.client(client)
.realm(realm)
.request(request)
.session(session)
.params(params);
}
return checker;
}

public AuthorizationEndpoint register(String tokenString) {
event.event(EventType.REGISTER);
action = Action.REGISTER;
Expand Down Expand Up @@ -271,7 +301,7 @@ public AuthorizationEndpoint forgotCredentials() {
return this;
}

private void checkClient(String clientId) {
private ClientModel checkClient(String clientId) {
if (clientId == null) {
event.detail(Details.REASON, "Missing parameter: " + OIDCLoginProtocol.CLIENT_ID_PARAM);
event.error(Errors.INVALID_REQUEST);
Expand Down Expand Up @@ -309,24 +339,83 @@ private void checkClient(String clientId) {
}

session.getContext().setClient(client);
return client;
}

private Response handleErrorPageException(ErrorPageException epex) {

// Prefer authorization error on redirect_uri
// https://github.com/keycloak/keycloak/issues/48542
boolean preferErrorOnRedirect = realm.getAttribute(AUTHORIZATION_PREFER_ERROR_ON_REDIRECT, false);
Comment thread
tdiesler marked this conversation as resolved.
Resolved
if (!preferErrorOnRedirect)
throw epex;

Response.Status status = epex.getStatus();

// Get the localized error message (english)
//
String errorMessage = epex.getErrorMessage();
try {
Theme theme = session.theme().getTheme(Theme.Type.LOGIN);
Properties localizedMessages = theme.getEnhancedMessages(realm, Locale.ENGLISH);
if (localizedMessages.containsKey(errorMessage)) {
errorMessage = localizedMessages.getProperty(errorMessage);
Object[] params = epex.getParameters();
if (params.length > 0) {
errorMessage = MessageFormat.format(errorMessage, params);
}
}
} catch (IOException ioe) {
// ignore
}

// Do a second pass trying to obtain the required parameters for a valid redirect
//
try {
MultivaluedMap<String, String> params = checker.getRequestParams();
AuthorizationEndpointRequest request = checker.getAuthorizationEndpointRequest();
if (client == null) {
String clientId = request != null ? request.getClientId() : params.getFirst(OIDCLoginProtocol.CLIENT_ID_PARAM);
client = checkClient(clientId);
checker.client(client);
}
if (parsedRedirectUri == null) {
checker.checkRedirectUri();
parsedRedirectUri = checker.getRedirectUri();
}
if (parsedResponseType == null || parsedResponseMode == null) {
checker.checkResponseType();
parsedResponseType = checker.getParsedResponseType();
parsedResponseMode = checker.getParsedResponseMode();
}
} catch (Exception aux) {
// ignore
}

if (client != null && parsedRedirectUri != null && parsedResponseType != null && parsedResponseMode != null) {
return handleRedirectErrorToClient(epex.getError(), errorMessage);
}

// [TODO >>>] Should we throw an ErrorResponseException instead?
@VinodAnandan VinodAnandan May 7, 2026
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.

@tdiesler Would you happen to know if there is already an existing issue tracking the use of ErrorResponseException instead? If not, do you think it would be worthwhile creating one for future tracking?

Also, I noticed there are 14 failing checks. Do you think these are related to test flakiness, or does this PR require some changes in order for the tests to pass?

@tdiesler tdiesler May 8, 2026 •
edited
Loading
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.

This needs to be discussed. The spec does not say that there should be a json error response at all. It also does not prohibit it AFAIK.

var errRep = new OAuth2ErrorRepresentation(epex.getError(), errorMessage);
return Response.status(status).entity(errRep).type(MediaType.APPLICATION_JSON_TYPE).build();
Comment thread
tdiesler marked this conversation as resolved.
Resolved
}

private Response redirectErrorToClient(OIDCResponseMode responseMode, String error, String errorDescription) {
private Response handleRedirectErrorToClient(String error, String errorDescription) {
CacheControlUtil.noBackButtonCacheControlHeader(session);

OIDCRedirectUriBuilder errorResponseBuilder = OIDCRedirectUriBuilder.fromUri(redirectUri, responseMode, session, null)
OIDCRedirectUriBuilder errorResponseBuilder = OIDCRedirectUriBuilder.fromUri(parsedRedirectUri, parsedResponseMode, session, null)
.addParam(OAuth2Constants.ERROR, error);

if (errorDescription != null) {
errorResponseBuilder.addParam(OAuth2Constants.ERROR_DESCRIPTION, errorDescription);
}

if (request.getState() != null) {
if (request != null && request.getState() != null) {
errorResponseBuilder.addParam(OAuth2Constants.STATE, request.getState());
}

OIDCAdvancedConfigWrapper clientConfig = OIDCAdvancedConfigWrapper.fromClientModel(client);
if (!clientConfig.isExcludeIssuerFromAuthResponse()) {
if (client != null && !OIDCAdvancedConfigWrapper.fromClientModel(client).isExcludeIssuerFromAuthResponse()) {
errorResponseBuilder.addParam(OAuth2Constants.ISSUER, Urls.realmIssuer(session.getContext().getUri().getBaseUri(), realm.getName()));
}

Expand All @@ -335,7 +424,7 @@ private Response redirectErrorToClient(OIDCResponseMode responseMode, String err

private void updateAuthenticationSession() {
authenticationSession.setProtocol(OIDCLoginProtocol.LOGIN_PROTOCOL);
authenticationSession.setRedirectUri(redirectUri);
authenticationSession.setRedirectUri(parsedRedirectUri);
authenticationSession.setAction(AuthenticationSessionModel.Action.AUTHENTICATE.name());
authenticationSession.setClientNote(OIDCLoginProtocol.RESPONSE_TYPE_PARAM, request.getResponseType());
authenticationSession.setClientNote(OIDCLoginProtocol.REDIRECT_URI_PARAM, request.getRedirectUriParam());
Expand Down
Loading
Loading
0