-
Notifications
You must be signed in to change notification settings - Fork 41.1k
ResponseStatusException no longer returning response body in 2.6.1 using spring security #28953
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.
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
Comments
Thanks for the sample. The behaviour you have observed is intended with Spring Boot 2.6. Your security configuration requires authentication to access everything but If you want any user, authenticated or not, to receive Spring Boot's default error response, you should permit access to diff --git a/src/main/kotlin/uk/gov/justice/digital/hmpps/token/config/ResourceServerConfiguration.kt b/src/main/kotlin/uk/gov/justice/digital/hmpps/token/config/ResourceServerConfiguration.kt
index aa77ab2..6a4d0a6 100644
--- a/src/main/kotlin/uk/gov/justice/digital/hmpps/token/config/ResourceServerConfiguration.kt
+++ b/src/main/kotlin/uk/gov/justice/digital/hmpps/token/config/ResourceServerConfiguration.kt
@@ -25,7 +25,7 @@ open class ResourceServerConfiguration : WebSecurityConfigurerAdapter() {
.authorizeRequests { auth ->
auth.antMatchers(
"/favicon.ico", "/csrf", "/health/**", "/info",
- "/webjars/**", "/v2/api-docs"
+ "/webjars/**", "/v2/api-docs", "/error"
)
.permitAll().anyRequest().authenticated()
} |
I think the important behavioural change here in 2.6 is that authenticated users no longer receive a full error response if the application relies on the default error handler. It would be good to document this change and provide the recommended solution. It does feel a bit strange though that due to Error page is accessible when no credentials are provided we have now ended up with Error page is not accessible when credentials are provided and the recommended solution to that problem is to allow the error page to be accessible to everyone! It does sound therefore that the default error handler should not be used. Instead I guess the application could catch the exception and return a |
That shouldn't be the case.
That is not the recommendation. If a user is authenticated, they should received a body in an error response without permitting all to access the error page. If that's not happening, then we need to investigate further. It could be that there's a faulty filter involved (as I believe was the case in the second issue to which you linked above), you're hitting a limitation of Spring Security, or something else. |
The user in the sample project above is fully authenticated but they are not receiving a body in the error response. It looks like the spring boot forwards onto the /error page but the BearerTokenAuthenticationFIlter which extends the OncePerRequestFilter doesn't add the necessary authentication to the spring security context when in error state. Should I therefore raise a bug against the BearerTokenAuthenticationFIlter? (I presume other filters will be affected as well) |
Apologies, @SimonMitchellMOJ, I had misunderstood the problem that the sample was demonstrating. The problem is due to Spring Security's
By contrast, when using a bearer token, the forward to
The newly introduced |
I'm seeing the same problem in our applications. We wrote our own custom |
@cmdjulian Can you provide a minimal sample that we can use to replicate the issue, please? You're most likely running into the same issue but I want to be sure since you mentioned custom |
We use public class JwtAuthorizationFilter extends OncePerRequestFilter {
public static final String BEARER_TOKEN_PREFIX = "Bearer ";
/**
* Put token into Spring Security Context, so that {@link JwtAuthenticationProvider}
* can verify the tokens validity.
*/
@Override
protected void doFilterInternal(@NonNull HttpServletRequest req, @NonNull HttpServletResponse res, FilterChain chain)
throws IOException, ServletException {
getJwtFromRequest(req).ifPresent(jwtCredentials -> {
JwtAuthenticationToken jwtAuthenticationToken = JwtAuthenticationToken.builder()
.token(jwtCredentials)
.details(new WebAuthenticationDetailsSource().buildDetails(req))
.authenticated(false)
.build();
var context = SecurityContextHolder.createEmptyContext();
context.setAuthentication(jwtAuthenticationToken);
SecurityContextHolder.setContext(context);
});
chain.doFilter(req, res);
}
/**
* Extract JWT value from Request header.
*/
private Optional<String> getJwtFromRequest(HttpServletRequest request) {
String token = request.getHeader(HttpHeaders.AUTHORIZATION);
return StringUtils.startsWith(token, BEARER_TOKEN_PREFIX)
? Optional.of(token.substring(BEARER_TOKEN_PREFIX.length()))
: Optional.empty();
}
} @RequiredArgsConstructor
public class JwtAuthenticationProvider implements AuthenticationProvider {
private final JwtManager manager;
private final UserDetailsService userDetailsService;
@Override
public Authentication authenticate(Authentication authentication) {
final VerifiedUserToken verifiedToken = manager.verifyToken(authentication.getCredentials().toString());
return authenticateUser(verifiedToken, authentication.getDetails());
}
private Authentication authenticateUser(VerifiedUserToken token, Object details) {
UserDetails user = userDetailsService.loadUserByUsername(token.getUsername());
return constructAuthentication(token.getRawToken(), user, details);
}
private Authentication constructAuthentication(String token, UserDetails user, Object details) {
return JwtAuthenticationToken.builder()
.token(token)
.user(user)
.details(details)
.authorities(user.getAuthorities())
.authenticated(true)
.build();
}
@Override
public boolean supports(Class<?> authentication) {
return authentication.equals(JwtAuthenticationToken.class);
}
} The |
Is there a possible (temporary) workaround within an app being upgraded from 2.5.x to 2.6.1, or are we pretty much stuck waiting for 2.6.2? |
@mjustin A workaround would be to remove the |
Thanks. Won't that have the side effect of also enabling error pages when the user is not logged authenticated, though? |
@mbhave I just updated to private boolean isAllowed(HttpServletRequest request, Integer errorCode) {
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
if (isUnauthenticated(authentication) && isNotAuthenticationError(errorCode)) {
return true;
}
return getPrivilegeEvaluator().isAllowed(request.getRequestURI(), authentication);
} The |
I will say this fixed the specific issue I was having, so it looks to be at least a partial fix. |
@mbhave @datagitlies We have the same scenario: Show a (custom) spring boot error page if a user has no access (403) to a resource. As far i can tell the current The implementation call During debugging i observed the following:
This seems to be the reason that a matcher like
Are these known limitations, or is the described case not meant to be implemented by using spring error pages? |
@lgraf I believe that the |
we found that after upgrading to spring boot 2.6.1 that the response body from the ResponseStatusException is no longer being populated even for authenticated users
It looks like the spring boot forwards onto the /error page but the BearerTokenAuthenticationFIlter which extends the OncePerRequestFilter doesn't add the necessary authentication to the spring security context when in error state.
This means that we then hit #26356 and the body is empty.
an example project is https://github.com/ministryofjustice/hmpps-spring-boot-2.6-bug
if you run ./gradlew test then it will fail
the branch https://github.com/ministryofjustice/hmpps-spring-boot-2.6-bug/tree/previous-working-version shows it working in the previous version of spring boot 2.5.6 alternately allowlisting /error fixes it too ( but we don't want to allowlist /error )
The text was updated successfully, but these errors were encountered: