From da94fbe4315556e8541d194df8a815817698fe9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonah=20Kl=C3=B6ckner?= Date: Tue, 5 Nov 2024 02:30:03 -0600 Subject: [PATCH 1/2] Evaluate URI query parameter only if enabled Issue gh-16038 --- .../OAuth2ResourceServerConfigurerTests.java | 3 +++ ...verBeanDefinitionParserTests-JwkSetUri.xml | 7 ++++- .../web/DefaultBearerTokenResolver.java | 26 ++++++++----------- ...verBearerTokenAuthenticationConverter.java | 18 ++++++------- .../web/DefaultBearerTokenResolverTests.java | 23 ++++++++++++++++ ...arerTokenAuthenticationConverterTests.java | 10 +++++++ 6 files changed, 62 insertions(+), 25 deletions(-) diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java index cb2ba0e137..493a7f0eaf 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java @@ -1560,12 +1560,15 @@ public class OAuth2ResourceServerConfigurerTests { @Bean SecurityFilterChain filterChain(HttpSecurity http) throws Exception { // @formatter:off + DefaultBearerTokenResolver defaultBearerTokenResolver = new DefaultBearerTokenResolver(); + defaultBearerTokenResolver.setAllowUriQueryParameter(true); http .authorizeRequests() .requestMatchers("/requires-read-scope").access("hasAuthority('SCOPE_message:read')") .anyRequest().authenticated() .and() .oauth2ResourceServer() + .bearerTokenResolver(defaultBearerTokenResolver) .jwt() .jwkSetUri(this.jwkSetUri); return http.build(); diff --git a/config/src/test/resources/org/springframework/security/config/http/OAuth2ResourceServerBeanDefinitionParserTests-JwkSetUri.xml b/config/src/test/resources/org/springframework/security/config/http/OAuth2ResourceServerBeanDefinitionParserTests-JwkSetUri.xml index 3f81363d27..aac12989e9 100644 --- a/config/src/test/resources/org/springframework/security/config/http/OAuth2ResourceServerBeanDefinitionParserTests-JwkSetUri.xml +++ b/config/src/test/resources/org/springframework/security/config/http/OAuth2ResourceServerBeanDefinitionParserTests-JwkSetUri.xml @@ -25,10 +25,15 @@ + + + + - + diff --git a/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolver.java b/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolver.java index d238e87017..96aa1be570 100644 --- a/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolver.java +++ b/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolver.java @@ -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 @@ -63,15 +63,12 @@ public final class DefaultBearerTokenResolver implements BearerTokenResolver { } 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 && parameterToken.isBlank()) { + BearerTokenError error = BearerTokenErrors + .invalidRequest("The requested token parameter is an empty string"); + throw new OAuth2AuthenticationException(error); } - return null; + return parameterToken; } /** @@ -122,7 +119,10 @@ public final class DefaultBearerTokenResolver implements BearerTokenResolver { 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; @@ -134,10 +134,6 @@ public final class DefaultBearerTokenResolver implements BearerTokenResolver { 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()); } diff --git a/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverter.java b/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverter.java index bd07c59b74..ec7675acd0 100644 --- a/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverter.java +++ b/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverter.java @@ -77,18 +77,18 @@ public class ServerBearerTokenAuthenticationConverter implements ServerAuthentic } 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 parameterTokens = request.getQueryParams().get("access_token"); if (CollectionUtils.isEmpty(parameterTokens)) { return null; diff --git a/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolverTests.java b/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolverTests.java index f04fb69a3d..6c62d7c27f 100644 --- a/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolverTests.java +++ b/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolverTests.java @@ -110,6 +110,7 @@ public class DefaultBearerTokenResolverTests { @Test public void resolveWhenValidHeaderIsPresentTogetherWithFormParameterThenAuthenticationExceptionIsThrown() { + this.resolver.setAllowFormEncodedBodyParameter(true); MockHttpServletRequest request = new MockHttpServletRequest(); request.addHeader("Authorization", "Bearer " + TEST_TOKEN); request.setMethod("POST"); @@ -121,6 +122,7 @@ public class DefaultBearerTokenResolverTests { @Test public void resolveWhenValidHeaderIsPresentTogetherWithQueryParameterThenAuthenticationExceptionIsThrown() { + this.resolver.setAllowUriQueryParameter(true); MockHttpServletRequest request = new MockHttpServletRequest(); request.addHeader("Authorization", "Bearer " + TEST_TOKEN); request.setMethod("GET"); @@ -133,6 +135,7 @@ public class DefaultBearerTokenResolverTests { // gh-10326 @Test public void resolveWhenRequestContainsTwoAccessTokenQueryParametersThenAuthenticationExceptionIsThrown() { + this.resolver.setAllowUriQueryParameter(true); MockHttpServletRequest request = new MockHttpServletRequest(); request.setMethod("GET"); request.addParameter("access_token", "token1", "token2"); @@ -143,6 +146,7 @@ public class DefaultBearerTokenResolverTests { // gh-10326 @Test public void resolveWhenRequestContainsTwoAccessTokenFormParametersThenAuthenticationExceptionIsThrown() { + this.resolver.setAllowFormEncodedBodyParameter(true); MockHttpServletRequest request = new MockHttpServletRequest(); request.setMethod("POST"); request.setContentType("application/x-www-form-urlencoded"); @@ -261,6 +265,25 @@ public class DefaultBearerTokenResolverTests { assertThat(this.resolver.resolve(request)).isNull(); } + // gh-16038 + @Test + void resolveWhenRequestContainsTwoAccessTokenFormParametersAndSupportIsDisabledThenTokenIsNotResolved() { + 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 resolveWhenRequestContainsTwoAccessTokenQueryParametersAndSupportIsDisabledThenTokenIsNotResolved() { + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setMethod("GET"); + request.addParameter("access_token", "token1", "token2"); + assertThat(this.resolver.resolve(request)).isNull(); + } + @Test public void resolveWhenQueryParameterIsPresentAndEmptyStringThenTokenIsNotResolved() { this.resolver.setAllowUriQueryParameter(true); diff --git a/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverterTests.java b/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverterTests.java index 1f4b17697f..3e47401824 100644 --- a/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverterTests.java +++ b/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverterTests.java @@ -157,6 +157,7 @@ public class ServerBearerTokenAuthenticationConverterTests { @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); @@ -205,6 +206,7 @@ public class ServerBearerTokenAuthenticationConverterTests { @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)) @@ -217,6 +219,14 @@ public class ServerBearerTokenAuthenticationConverterTests { } + // gh-16038 + @Test + void resolveWhenRequestContainsTwoAccessTokenQueryParametersAndSupportIsDisabledThenTokenIsNotResolved() { + MockServerHttpRequest.BaseBuilder request = MockServerHttpRequest.get("/") + .queryParam("access_token", TEST_TOKEN, TEST_TOKEN); + assertThat(convertToToken(request)).isNull(); + } + private BearerTokenAuthenticationToken convertToToken(MockServerHttpRequest.BaseBuilder request) { return convertToToken(request.build()); } From 3c0fef59b547359ac3e5824c6b56694c3cb7b8dd Mon Sep 17 00:00:00 2001 From: Steve Riesenberg <5248162+sjohnr@users.noreply.github.com> Date: Fri, 6 Dec 2024 17:12:44 -0600 Subject: [PATCH 2/2] Polish gh-16039 Closes gh-16038 --- .../OAuth2ResourceServerConfigurerTests.java | 2 +- .../web/DefaultBearerTokenResolver.java | 113 ++++++++++-------- ...verBearerTokenAuthenticationConverter.java | 2 +- .../web/DefaultBearerTokenResolverTests.java | 19 ++- ...arerTokenAuthenticationConverterTests.java | 2 +- 5 files changed, 80 insertions(+), 58 deletions(-) diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java index 493a7f0eaf..903df193d9 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. diff --git a/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolver.java b/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolver.java index 96aa1be570..f00305843a 100644 --- a/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolver.java +++ b/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolver.java @@ -52,23 +52,77 @@ public final class DefaultBearerTokenResolver implements BearerTokenResolver { @Override public String resolve(final HttpServletRequest request) { - final String authorizationHeaderToken = resolveFromAuthorizationHeader(request); - final String parameterToken = resolveFromRequestParameters(request); + // @formatter:off + return resolveToken( + resolveFromAuthorizationHeader(request), + resolveAccessTokenFromQueryString(request), + resolveAccessTokenFromBody(request) + ); + // @formatter:on + } - if (authorizationHeaderToken != null) { - if (parameterToken != null) { + private static String resolveToken(String... accessTokens) { + if (accessTokens == null || accessTokens.length == 0) { + return null; + } + + String accessToken = null; + for (String token : accessTokens) { + if (accessToken == null) { + accessToken = token; + } + else if (token != null) { BearerTokenError error = BearerTokenErrors .invalidRequest("Found multiple bearer tokens in the request"); throw new OAuth2AuthenticationException(error); } - return authorizationHeaderToken; } - if (parameterToken != null && parameterToken.isBlank()) { + + if (accessToken != null && accessToken.isBlank()) { BearerTokenError error = BearerTokenErrors .invalidRequest("The requested token parameter is an empty string"); throw new OAuth2AuthenticationException(error); } - return parameterToken; + + return accessToken; + } + + private String resolveFromAuthorizationHeader(HttpServletRequest request) { + String authorization = request.getHeader(this.bearerTokenHeaderName); + if (!StringUtils.startsWithIgnoreCase(authorization, "bearer")) { + return null; + } + + Matcher matcher = authorizationPattern.matcher(authorization); + if (!matcher.matches()) { + BearerTokenError error = BearerTokenErrors.invalidToken("Bearer token is malformed"); + throw new OAuth2AuthenticationException(error); + } + + return matcher.group("token"); + } + + private String resolveAccessTokenFromQueryString(HttpServletRequest request) { + if (!this.allowUriQueryParameter || !HttpMethod.GET.name().equals(request.getMethod())) { + return null; + } + + return resolveToken(request.getParameterValues(ACCESS_TOKEN_PARAMETER_NAME)); + } + + private String resolveAccessTokenFromBody(HttpServletRequest request) { + if (!this.allowFormEncodedBodyParameter + || !MediaType.APPLICATION_FORM_URLENCODED_VALUE.equals(request.getContentType()) + || HttpMethod.GET.name().equals(request.getMethod())) { + return null; + } + + String queryString = request.getQueryString(); + if (queryString != null && queryString.contains(ACCESS_TOKEN_PARAMETER_NAME)) { + return null; + } + + return resolveToken(request.getParameterValues(ACCESS_TOKEN_PARAMETER_NAME)); } /** @@ -106,49 +160,4 @@ public final class DefaultBearerTokenResolver implements BearerTokenResolver { this.bearerTokenHeaderName = bearerTokenHeaderName; } - private String resolveFromAuthorizationHeader(HttpServletRequest request) { - String authorization = request.getHeader(this.bearerTokenHeaderName); - if (!StringUtils.startsWithIgnoreCase(authorization, "bearer")) { - return null; - } - Matcher matcher = authorizationPattern.matcher(authorization); - if (!matcher.matches()) { - BearerTokenError error = BearerTokenErrors.invalidToken("Bearer token is malformed"); - throw new OAuth2AuthenticationException(error); - } - return matcher.group("token"); - } - - 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; - } - if (values.length == 1) { - return values[0]; - } - BearerTokenError error = BearerTokenErrors.invalidRequest("Found multiple bearer tokens in the request"); - throw new OAuth2AuthenticationException(error); - } - - private static boolean isGetRequest(HttpServletRequest request) { - return HttpMethod.GET.name().equals(request.getMethod()); - } - - private static boolean isFormEncodedRequest(HttpServletRequest request) { - return MediaType.APPLICATION_FORM_URLENCODED_VALUE.equals(request.getContentType()); - } - - private static boolean hasAccessTokenInQueryString(HttpServletRequest request) { - return (request.getQueryString() != null) && request.getQueryString().contains(ACCESS_TOKEN_PARAMETER_NAME); - } - - private boolean isParameterTokenEnabledForRequest(HttpServletRequest request) { - return ((this.allowFormEncodedBodyParameter && isFormEncodedRequest(request) && !isGetRequest(request) - && !hasAccessTokenInQueryString(request)) || (this.allowUriQueryParameter && isGetRequest(request))); - } - } diff --git a/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverter.java b/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverter.java index ec7675acd0..c3c7699f8e 100644 --- a/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverter.java +++ b/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverter.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. diff --git a/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolverTests.java b/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolverTests.java index 6c62d7c27f..db8c6292bd 100644 --- a/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolverTests.java +++ b/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolverTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -237,6 +237,19 @@ public class DefaultBearerTokenResolverTests { assertThat(this.resolver.resolve(request)).isNull(); } + @Test + public void resolveWhenPostAndQueryParameterIsSupportedAndFormParameterIsPresentThenTokenIsNotResolved() { + this.resolver.setAllowUriQueryParameter(true); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setMethod("POST"); + request.setContentType("application/x-www-form-urlencoded"); + request.setQueryString("access_token=" + TEST_TOKEN); + request.addParameter("access_token", TEST_TOKEN); + + assertThat(this.resolver.resolve(request)).isNull(); + } + @Test public void resolveWhenFormParameterIsPresentAndNotSupportedThenTokenIsNotResolved() { MockHttpServletRequest request = new MockHttpServletRequest(); @@ -267,7 +280,7 @@ public class DefaultBearerTokenResolverTests { // gh-16038 @Test - void resolveWhenRequestContainsTwoAccessTokenFormParametersAndSupportIsDisabledThenTokenIsNotResolved() { + public void resolveWhenRequestContainsTwoAccessTokenFormParametersAndSupportIsDisabledThenTokenIsNotResolved() { MockHttpServletRequest request = new MockHttpServletRequest(); request.setMethod("POST"); request.setContentType("application/x-www-form-urlencoded"); @@ -277,7 +290,7 @@ public class DefaultBearerTokenResolverTests { // gh-16038 @Test - void resolveWhenRequestContainsTwoAccessTokenQueryParametersAndSupportIsDisabledThenTokenIsNotResolved() { + public void resolveWhenRequestContainsTwoAccessTokenQueryParametersAndSupportIsDisabledThenTokenIsNotResolved() { MockHttpServletRequest request = new MockHttpServletRequest(); request.setMethod("GET"); request.addParameter("access_token", "token1", "token2"); diff --git a/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverterTests.java b/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverterTests.java index 3e47401824..32349f121d 100644 --- a/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverterTests.java +++ b/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverterTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License.