Skip to content

Commit

Permalink
State parameter is not encoded
Browse files Browse the repository at this point in the history 
https://www.pivotaltracker.com/story/show/186808933
  • Loading branch information
@oharsta
oharsta committed Jan 10, 2024
1 parent 6fc5181 commit 2db0a99
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 41 deletions.
68 changes: 30 additions & 38 deletions src/main/java/oidc/endpoints/AuthorizationEndpoint.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,8 @@

import com.nimbusds.jose.JOSEException;
import com.nimbusds.jose.proc.BadJOSEException;
import com.nimbusds.oauth2.sdk.AuthorizationRequest;
import com.nimbusds.oauth2.sdk.GrantType;
import com.nimbusds.oauth2.sdk.ParseException;
import com.nimbusds.oauth2.sdk.ResponseMode;
import com.nimbusds.oauth2.sdk.ResponseType;
import com.nimbusds.oauth2.sdk.Scope;
import com.nimbusds.oauth2.sdk.*;
import com.nimbusds.oauth2.sdk.id.State;
import com.nimbusds.oauth2.sdk.pkce.CodeChallenge;
import com.nimbusds.oauth2.sdk.pkce.CodeChallengeMethod;
Expand All @@ -16,19 +12,10 @@
import com.nimbusds.openid.connect.sdk.OIDCResponseTypeValue;
import com.nimbusds.openid.connect.sdk.Prompt;
import oidc.crypto.KeyGenerator;
import oidc.exceptions.InvalidGrantException;
import oidc.exceptions.InvalidScopeException;
import oidc.exceptions.RedirectMismatchException;
import oidc.exceptions.UnknownClientException;
import oidc.exceptions.UnsupportedPromptValueException;
import oidc.exceptions.*;
import oidc.log.MDCContext;
import oidc.model.AccessToken;
import oidc.model.AuthorizationCode;
import oidc.model.EncryptedTokenValue;
import oidc.model.OpenIDClient;
import oidc.model.ProvidedRedirectURI;
import oidc.model.TokenValue;
import oidc.model.User;
import oidc.model.*;
import oidc.repository.AccessTokenRepository;
import oidc.repository.AuthorizationCodeRepository;
import oidc.repository.OpenIDClientRepository;
Expand Down Expand Up @@ -64,16 +51,7 @@
import java.net.URLDecoder;
import java.nio.charset.StandardCharsets;
import java.security.cert.CertificateException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.*;
import java.util.stream.Collectors;

import static java.util.stream.Collectors.toList;
Expand Down Expand Up @@ -222,20 +200,35 @@ private ModelAndView doAuthorization(MultiValueMap<String, String> parameters,
}
if (responseMode.equals(ResponseMode.QUERY)) {
UriComponentsBuilder builder = UriComponentsBuilder.fromUriString(redirectURI);
body.forEach(builder::queryParam);
return new ModelAndView(new RedirectView(builder.toUriString()));
body.entrySet().stream()
.filter(entry -> !entry.getKey().equalsIgnoreCase("state"))
.forEach(entry -> builder.queryParam(entry.getKey(), entry.getValue()));
return redirectViewWithState(builder, state);
}
if (responseMode.equals(ResponseMode.FRAGMENT)) {
UriComponentsBuilder builder = UriComponentsBuilder.fromUriString(redirectURI);
String fragment = body.entrySet().stream().map(entry -> String.format("%s=%s", entry.getKey(), entry.getValue())).collect(Collectors.joining("&"));
String fragment = body.entrySet().stream()
.filter(entry -> !entry.getKey().equalsIgnoreCase("state"))
.map(entry -> String.format("%s=%s", entry.getKey(), entry.getValue()))
.collect(Collectors.joining("&"));
builder.fragment(fragment);
return new ModelAndView(new RedirectView(builder.toUriString()));
return redirectViewWithState(builder, state);
}
throw new IllegalArgumentException("Response mode " + responseMode + " not supported");
}
throw new IllegalArgumentException("Not yet implemented response_type: " + responseType.toString());
}

private static ModelAndView redirectViewWithState(UriComponentsBuilder builder, State state) {
String uriString = builder.toUriString();
boolean hasState = state != null && StringUtils.hasText(state.getValue());
if (hasState) {
//We must do this after the toUriString, otherwise it will get encoded
uriString += ("&state=" + state.getValue());
}
return new ModelAndView(new RedirectView(uriString));
}

private void logout(HttpServletRequest request) {
SecurityContextHolder.getContext().setAuthentication(null);
SecurityContextHolder.clearContext();
Expand Down Expand Up @@ -301,7 +294,7 @@ private Map<String, Object> authorizationEndpointResponse(User user, OpenIDClien
result.put("id_token", tokenValue.getValue());
}
result.put("expires_in", client.getAccessTokenValidity());
if (state != null) {
if (state != null && StringUtils.hasText(state.getValue())) {
result.put("state", state.getValue());
}
return result;
Expand Down Expand Up @@ -366,17 +359,16 @@ private String authorizationRedirect(String redirectionURI, State state, String
boolean hasState = state != null && StringUtils.hasText(state.getValue());
if (isFragment) {
String fragment = "code=" + code;
if (hasState) {
fragment = fragment.concat("&state=" + state.getValue());
}
builder.fragment(fragment);
} else {
builder.queryParam("code", code);
if (hasState) {
builder.queryParam("state", state.getValue());
}
}
return builder.toUriString();
String uriString = builder.toUriString();
if (hasState) {
//We must do this after the toUriString, otherwise it will get encoded
uriString += ("&state=" + state.getValue());
}
return uriString;
}

private static final String unsupportedPromptMessage = "Unsupported prompt=%s is requested, redirecting the user back to the RP";
Expand Down
25 changes: 22 additions & 3 deletions src/test/java/oidc/endpoints/AuthorizationEndpointTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,14 @@ public void authorizeCodeFlowWithNonce() throws IOException, BadJOSEException, P

@Test
public void oauth2NonOidcImplicitFlow() throws IOException {
String state = "https%3A%2F%2Fexample.com";
Response response = doAuthorizeWithClaimsAndScopes("mock-sp", "token",
null, null, null, null, "groups", "example");
null, null, null, null, "groups", state);
String url = response.getHeader("Location");
String fragment = url.substring(url.indexOf("#") + 1);
Map<String, String> fragmentParameters = fragmentToMap(fragment);
assertFalse(fragmentParameters.containsKey("id_token"));
assertEquals(state, fragmentParameters.get("state"));
}

@Test
Expand All @@ -125,6 +127,17 @@ public void noScopeNoState() throws IOException {
assertFalse(tokenResponse.containsKey("id_token"));
}

@Test
public void queryParamState() throws IOException {
String state = "https%3A%2F%2Fexample.com";
Response response = doAuthorizeWithClaimsAndScopes("mock-sp", "code",
null, null, null, null, null, state);
String location = response.getHeader("Location");
UriComponentsBuilder builder = UriComponentsBuilder.fromUriString(location);
String returnedState = builder.build().getQueryParams().getFirst("state");
assertEquals(state, returnedState);
}

@Test
public void validationMissingParameter() {
Map<String, String> queryParams = new HashMap<>();
Expand Down Expand Up @@ -233,11 +246,14 @@ public void implicitFlowFragment() throws IOException, BadJOSEException, ParseEx

@Test
public void hybridFlowFragment() throws IOException, BadJOSEException, ParseException, JOSEException {
Response response = doAuthorize("mock-sp", "code id_token token", null, "nonce", null);
String state = "https%3A%2F%2Fexample.com";
Response response = doAuthorizeWithClaimsAndScopes("mock-sp", "code id_token token", null, "nonce", null,
Collections.emptyList(),"openid", state);
String url = response.getHeader("Location");
String fragment = url.substring(url.indexOf("#") + 1);
Map<String, String> fragmentParameters = fragmentToMap(fragment);
String code = fragmentParameters.get("code");
assertEquals(state, fragmentParameters.get("state"));

AuthorizationCode authorizationCode = mongoTemplate.findOne(Query.query(Criteria.where("code").is(code)), AuthorizationCode.class);
User user = mongoTemplate.findOne(Query.query(Criteria.where("sub").is(authorizationCode.getSub())), User.class);
Expand Down Expand Up @@ -271,9 +287,12 @@ public void hybridFlowFragment() throws IOException, BadJOSEException, ParseExce

@Test
public void implicitFlowQuery() throws IOException, BadJOSEException, ParseException, JOSEException {
Response response = doAuthorize("mock-sp", "id_token token", ResponseMode.QUERY.getValue(), "nonce", null);
String state = "https%3A%2F%2Fexample.com";
Response response = doAuthorizeWithClaimsAndScopes("mock-sp", "id_token token", ResponseMode.QUERY.getValue(), "nonce", null,
Collections.emptyList(), "openid", state);
String url = response.getHeader("Location");
Map<String, String> queryParameters = UriComponentsBuilder.fromUriString(url).build().getQueryParams().toSingleValueMap();
assertEquals(state, queryParameters.get("state"));
assertImplicitFlowResponse(queryParameters);
}

Expand Down

0 comments on commit 2db0a99

Please sign in to comment.