search

spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.77k stars 5.89k forks source link

DefaultBearerTokenResolver Regarding throwing an IllegalArgumentException when creating a BearerTokenAuthenticationToken instance when the allowFormEncodedBodyParameter member field is true or the allowUriQueryParameter member field is true. #15885

Open jacknie84 opened 1 week ago

jacknie84 commented 1 week ago

Describe the bug If allowFormEncodedBodyParameter or allowUriQueryParameter of DefaultBearerTokenResolver is set to true, a token will be retrieved from the request parameter. If the token is an empty string rather than null, an IllegalArgumentException will be thrown when creating a BearerTokenAuthenticationToken instance in the doFilterInternal method code of BearerTokenAuthenticationFilter. In that case, the HTTP response code will be 500 (Internal Server Error). I don't think this is an accurate response, and I think it should be a 401(Unauthorized) response.

To Reproduce build.gradle

plugins {
  id 'java'
  id 'idea'
  id 'org.springframework.boot' version '3.3.0'
  id 'io.spring.dependency-management' version '1.1.5'
}

...

dependencies {
  annotationProcessor 'org.projectlombok:lombok'

  compileOnly 'org.projectlombok:lombok'

  implementation 'org.springframework.boot:spring-boot-starter-web'
  implementation 'org.springframework.boot:spring-boot-starter-actuator'
  implementation 'org.springframework.boot:spring-boot-starter-validation'
  implementation 'org.springframework.boot:spring-boot-starter-security'
  implementation 'org.springframework.boot:spring-boot-starter-cache'
  implementation 'org.springframework.security:spring-security-oauth2-resource-server'
  implementation 'org.springframework.security:spring-security-oauth2-jose'
  implementation 'org.springframework.security:spring-security-acl'
}

Configuration

@Bean
public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception {
  String jwkSetUri = UriComponentsBuilder.fromUriString(AUTH0_JWKS_URI).host(auth0Domain).toUriString();
  DefaultBearerTokenResolver bearerTokenResolver = new DefaultBearerTokenResolver();
  bearerTokenResolver.setAllowUriQueryParameter(true);
  bearerTokenResolver.setAllowFormEncodedBodyParameter(true);

  return http
    .csrf(AbstractHttpConfigurer::disable)
    .cors(it -> it.configurationSource(corsConfigurationSource()))
    .formLogin(AbstractHttpConfigurer::disable)
    .logout(AbstractHttpConfigurer::disable)
    .sessionManagement(it -> it.sessionCreationPolicy(SessionCreationPolicy.STATELESS))
    .oauth2ResourceServer(it -> it.bearerTokenResolver(bearerTokenResolver).jwt(jwt -> jwt.jwkSetUri(jwkSetUri)))
    .securityMatchers(it -> it.requestMatchers("/partners/**", "/pa/**"))
    .authorizeHttpRequests(it -> it.anyRequest().authenticated())
    .build();
}

curl

$ curl -v http://localhost:8080/path/to/path?access_token=
...
* using HTTP/1.x
> GET /path/to/path?access_token= HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/8.7.1
> Accept: */*
>
* Request completely sent off
< HTTP/1.1 500
< Date: Tue, 08 Oct 2024 06:59:21 GMT
< Content-Type: application/json
< Transfer-Encoding: chunked
...

server log exception trace

java.lang.IllegalArgumentException: token cannot be empty
    at org.springframework.util.Assert.hasText(Assert.java:240) ~[spring-core-6.1.8.jar:6.1.8]
    at org.springframework.security.oauth2.server.resource.authentication.BearerTokenAuthenticationToken.<init>(BearerTokenAuthenticationToken.java:50) ~[spring-security-oauth2-resource-server-6.3.0.jar:6.3.0]
    at org.springframework.security.oauth2.server.resource.web.authentication.BearerTokenAuthenticationFilter.doFilterInternal(BearerTokenAuthenticationFilter.java:132) ~[spring-security-oauth2-resource-server-6.3.0.jar:6.3.0]
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116) ~[spring-web-6.1.8.jar:6.1.8]
    at org.springframework.security.web.ObservationFilterChainDecorator$ObservationFilter.wrapFilter(ObservationFilterChainDecorator.java:240) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.ObservationFilterChainDecorator$ObservationFilter.doFilter(ObservationFilterChainDecorator.java:227) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.ObservationFilterChainDecorator$VirtualFilterChain.doFilter(ObservationFilterChainDecorator.java:137) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.web.filter.CorsFilter.doFilterInternal(CorsFilter.java:91) ~[spring-web-6.1.8.jar:6.1.8]
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116) ~[spring-web-6.1.8.jar:6.1.8]
    at org.springframework.security.web.ObservationFilterChainDecorator$ObservationFilter.wrapFilter(ObservationFilterChainDecorator.java:240) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.ObservationFilterChainDecorator$ObservationFilter.doFilter(ObservationFilterChainDecorator.java:227) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.ObservationFilterChainDecorator$VirtualFilterChain.doFilter(ObservationFilterChainDecorator.java:137) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.header.HeaderWriterFilter.doHeadersAfter(HeaderWriterFilter.java:90) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.header.HeaderWriterFilter.doFilterInternal(HeaderWriterFilter.java:75) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116) ~[spring-web-6.1.8.jar:6.1.8]
    at org.springframework.security.web.ObservationFilterChainDecorator$ObservationFilter.wrapFilter(ObservationFilterChainDecorator.java:240) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.ObservationFilterChainDecorator$ObservationFilter.doFilter(ObservationFilterChainDecorator.java:227) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.ObservationFilterChainDecorator$VirtualFilterChain.doFilter(ObservationFilterChainDecorator.java:137) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.context.SecurityContextHolderFilter.doFilter(SecurityContextHolderFilter.java:82) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.context.SecurityContextHolderFilter.doFilter(SecurityContextHolderFilter.java:69) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.ObservationFilterChainDecorator$ObservationFilter.wrapFilter(ObservationFilterChainDecorator.java:240) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.ObservationFilterChainDecorator$ObservationFilter.doFilter(ObservationFilterChainDecorator.java:227) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.ObservationFilterChainDecorator$VirtualFilterChain.doFilter(ObservationFilterChainDecorator.java:137) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.context.request.async.WebAsyncManagerIntegrationFilter.doFilterInternal(WebAsyncManagerIntegrationFilter.java:62) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116) ~[spring-web-6.1.8.jar:6.1.8]
    at org.springframework.security.web.ObservationFilterChainDecorator$ObservationFilter.wrapFilter(ObservationFilterChainDecorator.java:240) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.ObservationFilterChainDecorator$ObservationFilter.doFilter(ObservationFilterChainDecorator.java:227) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.ObservationFilterChainDecorator$VirtualFilterChain.doFilter(ObservationFilterChainDecorator.java:137) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.session.DisableEncodeUrlFilter.doFilterInternal(DisableEncodeUrlFilter.java:42) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116) ~[spring-web-6.1.8.jar:6.1.8]
    at org.springframework.security.web.ObservationFilterChainDecorator$ObservationFilter.wrapFilter(ObservationFilterChainDecorator.java:240) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.ObservationFilterChainDecorator$AroundFilterObservation$SimpleAroundFilterObservation.lambda$wrap$0(ObservationFilterChainDecorator.java:323) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.ObservationFilterChainDecorator$ObservationFilter.doFilter(ObservationFilterChainDecorator.java:224) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.ObservationFilterChainDecorator$VirtualFilterChain.doFilter(ObservationFilterChainDecorator.java:137) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.FilterChainProxy.doFilterInternal(FilterChainProxy.java:233) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.security.web.FilterChainProxy.doFilter(FilterChainProxy.java:191) ~[spring-security-web-6.3.0.jar:6.3.0]
    at org.springframework.web.filter.CompositeFilter$VirtualFilterChain.doFilter(CompositeFilter.java:113) ~[spring-web-6.1.8.jar:6.1.8]
    at org.springframework.web.servlet.handler.HandlerMappingIntrospector.lambda$createCacheFilter$3(HandlerMappingIntrospector.java:195) ~[spring-webmvc-6.1.8.jar:6.1.8]
    at org.springframework.web.filter.CompositeFilter$VirtualFilterChain.doFilter(CompositeFilter.java:113) ~[spring-web-6.1.8.jar:6.1.8]
    at org.springframework.web.filter.CompositeFilter.doFilter(CompositeFilter.java:74) ~[spring-web-6.1.8.jar:6.1.8]
    at org.springframework.security.config.annotation.web.configuration.WebMvcSecurityConfiguration$CompositeFilterChainProxy.doFilter(WebMvcSecurityConfiguration.java:230) ~[spring-security-config-6.3.0.jar:6.3.0]
    at org.springframework.web.filter.DelegatingFilterProxy.invokeDelegate(DelegatingFilterProxy.java:352) ~[spring-web-6.1.8.jar:6.1.8]
    at org.springframework.web.filter.DelegatingFilterProxy.doFilter(DelegatingFilterProxy.java:268) ~[spring-web-6.1.8.jar:6.1.8]
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:164) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:140) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.springframework.web.filter.RequestContextFilter.doFilterInternal(RequestContextFilter.java:100) ~[spring-web-6.1.8.jar:6.1.8]
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116) ~[spring-web-6.1.8.jar:6.1.8]
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:164) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:140) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.springframework.web.filter.FormContentFilter.doFilterInternal(FormContentFilter.java:93) ~[spring-web-6.1.8.jar:6.1.8]
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116) ~[spring-web-6.1.8.jar:6.1.8]
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:164) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:140) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.springframework.web.filter.ServerHttpObservationFilter.doFilterInternal(ServerHttpObservationFilter.java:109) ~[spring-web-6.1.8.jar:6.1.8]
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116) ~[spring-web-6.1.8.jar:6.1.8]
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:164) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:140) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal(CharacterEncodingFilter.java:201) ~[spring-web-6.1.8.jar:6.1.8]
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116) ~[spring-web-6.1.8.jar:6.1.8]
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:164) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:140) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.springframework.web.filter.AbstractRequestLoggingFilter.doFilterInternal(AbstractRequestLoggingFilter.java:289) ~[spring-web-6.1.8.jar:6.1.8]
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116) ~[spring-web-6.1.8.jar:6.1.8]
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:164) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:140) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:167) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:90) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:482) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:115) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:93) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:74) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.catalina.valves.RemoteIpValve.invoke(RemoteIpValve.java:731) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:344) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:389) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:63) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:896) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1741) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:52) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.tomcat.util.threads.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1190) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.tomcat.util.threads.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:659) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:63) ~[tomcat-embed-core-10.1.24.jar:10.1.24]
    at java.base/java.lang.Thread.run(Thread.java:1583) ~[na:na]

Expected behavior

I think it should be a 401 (Unnauthoized) response.

sjohnr commented 6 days ago

Thanks for reaching out @jacknie84!

If the token is an empty string rather than null, an IllegalArgumentException will be thrown when creating a BearerTokenAuthenticationToken instance

This is standard practice throughout the Spring Security codebase and Spring at large. The contract for BearerTokenAuthenticationToken specifies through that assertion that empty string is invalid. You can (for example) customize Spring Boot error handling in various ways to return a particular response for IllegalArgumentException, with @ControllerAdvice being one of the easier methods for this customization.

In that case, the HTTP response code will be 500 (Internal Server Error). I don't think this is an accurate response, and I think it should be a 401(Unauthorized) response.

I'm sorry you're seeing an error you don't want. Typically, an IllegalArgumentException would be mapped at the application level to a 400 Bad Request error.

Regardless, assuming the request is unauthenticated, Spring Security protects the ERROR dispatch by default just as it does for any other request, and requires authentication. Forwarded errors (such as this case with Spring Boot's /error page) would by default return a 401 Unauthorized for that reason. If you have found a case that returns a 500 Internal Server Error for an unauthenticated request (without permitting the ERROR dispatch), please provide a minimal sample and I will take a look. Otherwise, I plan to close this issue with the above explanation.

jacknie84 commented 5 days ago

@sjohnr Thanks for your response. I will create a sample right away and share it with you.

sjohnr commented 5 days ago

Thanks @jacknie84. Please note that I just spotted this line in your configuration:

http
    ....
    .securityMatchers(it -> it.requestMatchers("/partners/**", "/pa/**"))

This leaves part of your application unprotected, including /error. You should remove this line to have a 401 response returned for unauthenticated users. Alternatively, you can set up another filter chain to protect the rest of the application. In that case, make sure to order the filter chains accordingly with @Order.

In any case, a 500 would still be returned for uathenticated users, and you need to set up error handling for that exception if you want to customize the response.

If I've missed something feel free to continue providing a sample, but if that line is the reason you're seeing 500 for unauthenticated users (I believe it is) then please don't feel the need to provide a sample.

jacknie84 commented 4 days ago

Thanks @sjohnr I understand that an IllegalArgumentException is thrown and forwarded to the/error path. But my application does not protect the /error path.

If the configuration line you mentioned is removed, when an IllegalArgumentException is thrown, a series of logic will be executed, forwarding the /error path. When executing the /error request, the security filter chain recognizes the user as an unauthenticated user and sends a 401 response, but this is not what I want.

I think it should be processed so that a 401 response is possible within the security filter chain that handles the existing requested path rather than forwarding to the /error path. If so, WWW-Authenticate: Bearer error="invalid_token", error_description="An error occurred while attempting to decode the Jwt: Malformed token", error_uri="https://tools.ietf.org/html/rfc6750#section-3.1 " I expect it to respond with a header.

I have prepared a simple sample. github repository link

When you start the sample application server I prepared, you can request the/sample path and receive a 500 response.

Please check my PR for information on how to request the /sample route and receive a 401 response.

sjohnr commented 2 days ago

@jacknie84 thanks for your reply and sample.

But my application does not protect the /error path.

I would recommend that the entire application be protected unless it is not possible for some other reason.

If the configuration line you mentioned is removed, when an IllegalArgumentException is thrown, a series of logic will be executed, forwarding the /error path. When executing the /error request, the security filter chain recognizes the user as an unauthenticated user and sends a 401 response, but this is not what I want.

Spring Security uses a secure-by-default approach, so this is by design. If necessary, you can permit the /error page or the ERROR dispatch if desired. For example:

@Configuration
@EnableWebSecurity
public class SecurityConfiguration {

    @Bean
    public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception {
        http
                ...
                .authorizeHttpRequests(authorize -> authorize
                        // Either of the following lines will display errors
                        .dispatcherTypeMatchers(DispatcherType.ERROR).permitAll()
                        .requestMatchers("/error").permitAll()
                        ...
                        .anyRequest().authenticated()
                );

        return http.build();
    }

}

I think it should be processed so that a 401 response is possible within the security filter chain that handles the existing requested path rather than forwarding to the /error path. If so, WWW-Authenticate: Bearer error="invalid_token", error_description="An error occurred while attempting to decode the Jwt: Malformed token", error_uri="https://tools.ietf.org/html/rfc6750#section-3.1 " I expect it to respond with a header.

An empty token value is not a malformed token, but is invalid input. The IllegalArgumentException is the correct exception. The assertion in the constructor is part of the contract of BearerTokenAuthenticationToken so it would be incorrect to change the constructor of this class to achieve the goal of getting a particular HTTP response.

In RFC 6750, it defines an error for invalid_request with status 400 Bad Request when the request is invalid.

invalid_request

    The request is missing a required parameter, includes an
    unsupported parameter or parameter value, repeats the same
    parameter, uses more than one method for including an access
    token, or is otherwise malformed.  The resource server SHOULD
    respond with the HTTP 400 (Bad Request) status code.

This error could be appropriate to return in this case. This change could be made in both DefaultBearerTokenResolver and ServerBearerTokenAuthenticationConverter. This would be a better change than what was provided in the PR, so please consider it instead. Let me know your thoughts on this.