Skip to content
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

Do not validate parameters in ServerBearerTokenAuthenticationConverter and DefaultBearerTokenResolver if not enabled #16039

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Do not validate parameters in ServerBearerTokenAuthenticationConverter and DefaultBearerTokenResolver if not enabled #16039

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f0a8173
Evaluate URI query parameter only if enabled in reactive stack
jonah1und1 Nov 5, 2024
80fd041
Evaluate parameter access token only if enabled in servlet stack
jonah1und1 Nov 5, 2024
0aa03fc
Merge branch 'main' into gh-16038
jonah1und1 Nov 5, 2024
dd7d303
comply with formatting rules
jonah1und1 Nov 5, 2024
cbd6613
these lines can be omitted since they are the default
jonah1und1 Nov 6, 2024
b6c49b5
add test case with 2 query parameters and support disabled
jonah1und1 Nov 6, 2024
ac39cf7
remove redundant test case [same as resolveWhenQueryParameterIsPresen…
jonah1und1 Nov 6, 2024
b9ae6ef
reformat
jonah1und1 Nov 6, 2024
94c0e34
fix failing tests
jonah1und1 Nov 6, 2024
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
26 changes: 11 additions & 15 deletions ...a/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ public final class DefaultBearerTokenResolver implements BearerTokenResolver {
@Override
public String resolve(final HttpServletRequest request) {
final String authorizationHeaderToken = resolveFromAuthorizationHeader(request);
final String parameterToken = isParameterTokenSupportedForRequest(request)
? resolveFromRequestParameters(request) : null;
final String parameterToken = resolveFromRequestParameters(request);

if (authorizationHeaderToken != null) {
if (parameterToken != null) {
BearerTokenError error = BearerTokenErrors
Expand All @@ -63,15 +63,12 @@ public String resolve(final HttpServletRequest request) {
}
return authorizationHeaderToken;
}
if (parameterToken != null && isParameterTokenEnabledForRequest(request)) {
if (!StringUtils.hasText(parameterToken)) {
BearerTokenError error = BearerTokenErrors
.invalidRequest("The requested token parameter is an empty string");
throw new OAuth2AuthenticationException(error);
}
return parameterToken;
if (parameterToken != null && !StringUtils.hasText(parameterToken)) {
BearerTokenError error = BearerTokenErrors
.invalidRequest("The requested token parameter is an empty string");
throw new OAuth2AuthenticationException(error);
}
return null;
return parameterToken;
}

/**
Expand Down Expand Up @@ -122,7 +119,10 @@ private String resolveFromAuthorizationHeader(HttpServletRequest request) {
return matcher.group("token");
}

private static String resolveFromRequestParameters(HttpServletRequest request) {
private String resolveFromRequestParameters(HttpServletRequest request) {
if (!isParameterTokenEnabledForRequest(request)) {
return null;
}
String[] values = request.getParameterValues(ACCESS_TOKEN_PARAMETER_NAME);
if (values == null || values.length == 0) {
return null;
Expand All @@ -134,10 +134,6 @@ private static String resolveFromRequestParameters(HttpServletRequest request) {
throw new OAuth2AuthenticationException(error);
}

private boolean isParameterTokenSupportedForRequest(final HttpServletRequest request) {
return isFormEncodedRequest(request) || isGetRequest(request);
}

private static boolean isGetRequest(HttpServletRequest request) {
return HttpMethod.GET.name().equals(request.getMethod());
}
Expand Down
18 changes: 9 additions & 9 deletions ...2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,18 +77,18 @@ private String token(ServerHttpRequest request) {
}
return authorizationHeaderToken;
}
if (parameterToken != null && isParameterTokenSupportedForRequest(request)) {
if (!StringUtils.hasText(parameterToken)) {
BearerTokenError error = BearerTokenErrors
.invalidRequest("The requested token parameter is an empty string");
throw new OAuth2AuthenticationException(error);
}
return parameterToken;
if (parameterToken != null && !StringUtils.hasText(parameterToken)) {
BearerTokenError error = BearerTokenErrors
.invalidRequest("The requested token parameter is an empty string");
throw new OAuth2AuthenticationException(error);
}
return null;
return parameterToken;
}

private static String resolveAccessTokenFromRequest(ServerHttpRequest request) {
private String resolveAccessTokenFromRequest(ServerHttpRequest request) {
if (!isParameterTokenSupportedForRequest(request)) {
return null;
}
List<String> parameterTokens = request.getQueryParams().get("access_token");
if (CollectionUtils.isEmpty(parameterTokens)) {
return null;
Expand Down
27 changes: 27 additions & 0 deletions .../springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolverTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ public void resolveWhenHeaderWithInvalidCharactersIsPresentThenAuthenticationExc

@Test
public void resolveWhenValidHeaderIsPresentTogetherWithFormParameterThenAuthenticationExceptionIsThrown() {
this.resolver.setAllowFormEncodedBodyParameter(true);
MockHttpServletRequest request = new MockHttpServletRequest();
request.addHeader("Authorization", "Bearer " + TEST_TOKEN);
request.setMethod("POST");
Expand All @@ -121,6 +122,7 @@ public void resolveWhenValidHeaderIsPresentTogetherWithFormParameterThenAuthenti

@Test
public void resolveWhenValidHeaderIsPresentTogetherWithQueryParameterThenAuthenticationExceptionIsThrown() {
this.resolver.setAllowUriQueryParameter(true);
MockHttpServletRequest request = new MockHttpServletRequest();
request.addHeader("Authorization", "Bearer " + TEST_TOKEN);
request.setMethod("GET");
Expand All @@ -133,6 +135,7 @@ public void resolveWhenValidHeaderIsPresentTogetherWithQueryParameterThenAuthent
// gh-10326
@Test
public void resolveWhenRequestContainsTwoAccessTokenQueryParametersThenAuthenticationExceptionIsThrown() {
this.resolver.setAllowUriQueryParameter(true);
MockHttpServletRequest request = new MockHttpServletRequest();
request.setMethod("GET");
request.addParameter("access_token", "token1", "token2");
Expand All @@ -143,6 +146,7 @@ public void resolveWhenRequestContainsTwoAccessTokenQueryParametersThenAuthentic
// gh-10326
@Test
public void resolveWhenRequestContainsTwoAccessTokenFormParametersThenAuthenticationExceptionIsThrown() {
this.resolver.setAllowFormEncodedBodyParameter(true);
MockHttpServletRequest request = new MockHttpServletRequest();
request.setMethod("POST");
request.setContentType("application/x-www-form-urlencoded");
Expand Down Expand Up @@ -235,6 +239,7 @@ public void resolveWhenPostAndFormParameterIsSupportedAndQueryParameterIsPresent

@Test
public void resolveWhenFormParameterIsPresentAndNotSupportedThenTokenIsNotResolved() {
this.resolver.setAllowFormEncodedBodyParameter(false);
jonah1und1 marked this conversation as resolved.
Show resolved Hide resolved
MockHttpServletRequest request = new MockHttpServletRequest();
request.setMethod("POST");
request.setContentType("application/x-www-form-urlencoded");
Expand All @@ -261,6 +266,28 @@ public void resolveWhenQueryParameterIsPresentAndNotSupportedThenTokenIsNotResol
assertThat(this.resolver.resolve(request)).isNull();
}

// gh-16038
@Test
void resolveWhenRequestContainsTwoAccessTokenFormParametersAndSupportIsDisabledThenTokenIsNotResolved() {
this.resolver.setAllowFormEncodedBodyParameter(false);
jonah1und1 marked this conversation as resolved.
Show resolved Hide resolved
MockHttpServletRequest request = new MockHttpServletRequest();
request.setMethod("POST");
request.setContentType("application/x-www-form-urlencoded");
request.addParameter("access_token", "token1", "token2");
assertThat(this.resolver.resolve(request)).isNull();
}

// gh-16038
@Test
void resolveWhenRequestContainsTwoAccessTokenQueryParameterAndSupportIsDisabledThenTokenIsNotResolved() {
jonah1und1 marked this conversation as resolved.
Show resolved Hide resolved
this.resolver.setAllowUriQueryParameter(false);
jonah1und1 marked this conversation as resolved.
Show resolved Hide resolved
MockHttpServletRequest request = new MockHttpServletRequest();
request.setMethod("GET");
request.setQueryString("access_token=" + TEST_TOKEN);
request.addParameter("access_token", "token1", "token2");
assertThat(this.resolver.resolve(request)).isNull();
}

@Test
public void resolveWhenQueryParameterIsPresentAndEmptyStringThenTokenIsNotResolved() {
this.resolver.setAllowUriQueryParameter(true);
Expand Down
11 changes: 11 additions & 0 deletions ...ver/resource/web/server/authentication/ServerBearerTokenAuthenticationConverterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ public void resolveWhenHeaderWithInvalidCharactersIsPresentAndNotSubscribedThenN
@Test
public void resolveWhenValidHeaderIsPresentTogetherWithQueryParameterThenAuthenticationExceptionIsThrown() {
// @formatter:off
this.converter.setAllowUriQueryParameter(true);
MockServerHttpRequest.BaseBuilder<?> request = MockServerHttpRequest.get("/")
.queryParam("access_token", TEST_TOKEN)
.header(HttpHeaders.AUTHORIZATION, "Bearer " + TEST_TOKEN);
Expand Down Expand Up @@ -205,6 +206,7 @@ public void resolveWhenQueryParameterIsPresentAndNotSupportedThenTokenIsNotResol

@Test
void resolveWhenQueryParameterHasMultipleAccessTokensThenOAuth2AuthenticationException() {
this.converter.setAllowUriQueryParameter(true);
MockServerHttpRequest.BaseBuilder<?> request = MockServerHttpRequest.get("/")
.queryParam("access_token", TEST_TOKEN, TEST_TOKEN);
assertThatExceptionOfType(OAuth2AuthenticationException.class).isThrownBy(() -> convertToToken(request))
Expand All @@ -217,6 +219,15 @@ void resolveWhenQueryParameterHasMultipleAccessTokensThenOAuth2AuthenticationExc

}

// gh-16038
@Test
void resoleWhenAllowUriQueryParameterIsFalseThenQueryParameterIsIgnored() {
jonah1und1 marked this conversation as resolved.
Show resolved Hide resolved
this.converter.setAllowUriQueryParameter(false);
MockServerHttpRequest.BaseBuilder<?> request = MockServerHttpRequest.get("/")
.queryParam("access_token", TEST_TOKEN);
assertThat(convertToToken(request)).isNull();
}

private BearerTokenAuthenticationToken convertToToken(MockServerHttpRequest.BaseBuilder<?> request) {
return convertToToken(request.build());
}
Expand Down
Loading