Skip to content

Refactor: Create Login State Values Once #3713

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

Open
strehle wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Refactor: Create Login State Values Once #3713

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
8 changes: 3 additions & 5 deletions .../java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthProviderConfigurator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,7 @@ public String getIdpAuthenticationUrl(
var responseType = URLEncoder.encode(definition.getResponseType(), StandardCharsets.UTF_8);
var relyingPartyId = definition.getRelyingPartyId();

var state = generateStateParam();
SessionUtils.setStateParam(request.getSession(), SessionUtils.stateParameterAttributeKeyForIdp(idpOriginKey), state);
var state = SessionUtils.setStateParam(request.getSession(), SessionUtils.stateParameterAttributeKeyForIdp(idpOriginKey), generateStateParam());

UriComponentsBuilder uriBuilder = UriComponentsBuilder
.fromUriString(idpUrlBase)
Expand All @@ -96,9 +95,8 @@ public String getIdpAuthenticationUrl(
// https://docs.spring.io/spring-security/site/docs/5.3.1.RELEASE/reference/html5/#initiating-the-authorization-request
if (isPkceNeeded(definition)) {
var pkceVerifier = new S256PkceVerifier();
var codeVerifier = generateCodeVerifier();
var codeChallenge = pkceVerifier.compute(codeVerifier);
SessionUtils.setStateParam(request.getSession(), SessionUtils.codeVerifierParameterAttributeKeyForIdp(idpOriginKey), codeVerifier);
var codeVerifier = SessionUtils.setStateParam(request.getSession(), SessionUtils.codeVerifierParameterAttributeKeyForIdp(idpOriginKey), generateCodeVerifier());
var codeChallenge = SessionUtils.setStateParam(request.getSession(), SessionUtils.codeChallengeParameterAttributeKeyForIdp(idpOriginKey), pkceVerifier.compute(codeVerifier));
Comment on lines +98 to +99
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

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

The code challenge is being computed from a potentially cached code verifier, but the code challenge itself is also being cached. This creates an inconsistency problem: if the code verifier already exists in the session, the code will compute a new challenge from that existing verifier but then attempt to cache this newly computed challenge. However, if a code challenge already exists in the session, it will be returned instead of the newly computed one, leading to a mismatch between the cached verifier and cached challenge.

The issue arises because pkceVerifier.compute() always computes a fresh challenge, but setStateParam may return a previously cached challenge that was computed from a different execution path or even a different verifier value.

To fix this, the code challenge should be computed once when the code verifier is first generated, and both should be cached together atomically. If the code verifier already exists in the session, the cached code challenge should be retrieved directly without recomputation.

Suggested change
var codeVerifier = SessionUtils.setStateParam(request.getSession(), SessionUtils.codeVerifierParameterAttributeKeyForIdp(idpOriginKey), generateCodeVerifier());
var codeChallenge = SessionUtils.setStateParam(request.getSession(), SessionUtils.codeChallengeParameterAttributeKeyForIdp(idpOriginKey), pkceVerifier.compute(codeVerifier));
var session = request.getSession();
var codeVerifierKey = SessionUtils.codeVerifierParameterAttributeKeyForIdp(idpOriginKey);
var codeChallengeKey = SessionUtils.codeChallengeParameterAttributeKeyForIdp(idpOriginKey);
String codeVerifier = (String) session.getAttribute(codeVerifierKey);
String codeChallenge = (String) session.getAttribute(codeChallengeKey);
if (codeVerifier == null) {
// First-time generation: create verifier and challenge and cache both atomically
codeVerifier = generateCodeVerifier();
codeChallenge = pkceVerifier.compute(codeVerifier);
session.setAttribute(codeVerifierKey, codeVerifier);
session.setAttribute(codeChallengeKey, codeChallenge);
} else if (codeChallenge == null) {
// Verifier exists but challenge does not: compute and cache from existing verifier
codeChallenge = pkceVerifier.compute(codeVerifier);
session.setAttribute(codeChallengeKey, codeChallenge);
}

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@adrianhoelzl-sap adrianhoelzl-sap Jan 9, 2026

Choose a reason for hiding this comment

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

Can we replace var with the concrete types here?

uriBuilder.queryParam("code_challenge", codeChallenge);
uriBuilder.queryParam("code_challenge_method", pkceVerifier.getCodeChallengeMethod());
}
Expand Down
14 changes: 12 additions & 2 deletions server/src/main/java/org/cloudfoundry/identity/uaa/util/SessionUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ public final class SessionUtils {

private static final String EXTERNAL_OAUTH_STATE_ATTRIBUTE_PREFIX = "external-oauth-state-";
private static final String EXTERNAL_OAUTH_CODE_VERIFIER_ATTRIBUTE_PREFIX = "external-oauth-verifier-";
private static final String EXTERNAL_OAUTH_CODE_CHALLENGE_ATTRIBUTE_PREFIX = "external-oauth-challenge-";
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

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

The naming convention for the new constant EXTERNAL_OAUTH_CODE_CHALLENGE_ATTRIBUTE_PREFIX follows the established pattern of similar constants (EXTERNAL_OAUTH_STATE_ATTRIBUTE_PREFIX and EXTERNAL_OAUTH_CODE_VERIFIER_ATTRIBUTE_PREFIX). However, consider whether storing the code challenge in the session is necessary.

In PKCE, the code challenge is derived from the code verifier and sent to the authorization endpoint, while only the code verifier needs to be retrieved from the session later to send to the token endpoint. The code challenge is not used after the authorization request, so caching it provides no functional benefit and only increases session size.

If the code challenge cache is kept for consistency with the refactoring approach, this is acceptable but should be documented as to why it's cached.

Copilot uses AI. Check for mistakes.

private SessionUtils() {
}
Expand Down Expand Up @@ -56,8 +57,13 @@ public static UaaAuthentication getForcePasswordExpiredUser(HttpSession session)
return (UaaAuthentication) session.getAttribute(FORCE_PASSWORD_EXPIRED_USER);
}

public static void setStateParam(HttpSession session, String stateParamKey, String state) {
session.setAttribute(stateParamKey, state);
public static String setStateParam(HttpSession session, String stateParamKey, String state) {
if (session.isNew() || session.getAttribute(stateParamKey) == null) {
session.setAttribute(stateParamKey, state);
return state;
} else {
return session.getAttribute(stateParamKey) instanceof String existingState ? existingState : null;
Comment on lines +61 to +65
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

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

The setStateParam method returns null when the session attribute exists but is not a String instance. This could lead to a null state value being used in the OAuth flow, which would cause authentication failures. The calling code in ExternalOAuthProviderConfigurator does not check for null return values.

While type safety should prevent non-String values from being stored, defensive error handling or logging would be more robust than silently returning null. Consider throwing an IllegalStateException or logging a warning if the cached value is of an unexpected type.

Suggested change
if (session.isNew() || session.getAttribute(stateParamKey) == null) {
session.setAttribute(stateParamKey, state);
return state;
} else {
return session.getAttribute(stateParamKey) instanceof String existingState ? existingState : null;
Object existingAttribute = session.getAttribute(stateParamKey);
if (session.isNew() || existingAttribute == null) {
session.setAttribute(stateParamKey, state);
return state;
} else if (existingAttribute instanceof String) {
return (String) existingAttribute;
} else {
throw new IllegalStateException(
"Expected session attribute '" + stateParamKey + "' to be a String but found: " +
existingAttribute.getClass().getName()
);

Copilot uses AI. Check for mistakes.
Comment on lines +61 to +65
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

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

The condition 'session.isNew()' may not correctly identify all cases where state parameters should be regenerated. According to the Servlet specification, isNew() returns true only for newly created sessions where the client hasn't yet acknowledged the session (i.e., hasn't sent back the session ID). This means:

  1. If a session exists but the state attribute is null (e.g., after session timeout and recreation), isNew() could be false, and a new state would be generated correctly via the second condition.
  2. However, if isNew() is true but the attribute already exists (which shouldn't normally happen), the existing value would be overwritten.

The logic appears functional but could be clearer. Consider simplifying to just check if the attribute is null, which is the actual condition that matters for determining whether to generate or reuse state values.

Suggested change
if (session.isNew() || session.getAttribute(stateParamKey) == null) {
session.setAttribute(stateParamKey, state);
return state;
} else {
return session.getAttribute(stateParamKey) instanceof String existingState ? existingState : null;
Object existing = session.getAttribute(stateParamKey);
if (existing == null) {
session.setAttribute(stateParamKey, state);
return state;
} else {
return existing instanceof String existingState ? existingState : null;

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@adrianhoelzl-sap adrianhoelzl-sap Jan 9, 2026

Choose a reason for hiding this comment

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

Can we add a comment or change the method's name to represent this behavior? If a value already exists, we do not update it.

}
}
Comment on lines +60 to 67
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

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

The method signature has been changed from void to String without updating the method's documentation or JavaDoc. This is a breaking API change that affects how the method is used. Callers now need to use the return value instead of relying on side effects alone.

Consider adding JavaDoc to document:

  1. The return value represents the state parameter value (either newly generated or retrieved from session)
  2. The behavior of reusing cached values when they exist in the session
  3. That null may be returned if the cached value is not a String (though this should be addressed separately)

Copilot uses AI. Check for mistakes.

public static Object getStateParam(HttpSession session, String stateParamKey) {
Expand Down Expand Up @@ -87,4 +93,8 @@ public static String stateParameterAttributeKeyForIdp(String idpOriginKey) {
public static String codeVerifierParameterAttributeKeyForIdp(String idpOriginKey) {
return EXTERNAL_OAUTH_CODE_VERIFIER_ATTRIBUTE_PREFIX + idpOriginKey;
}

public static String codeChallengeParameterAttributeKeyForIdp(String idpOriginKey) {
return EXTERNAL_OAUTH_CODE_CHALLENGE_ATTRIBUTE_PREFIX + idpOriginKey;
}
}
Loading