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.

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

ErrorController no longer called for response.sendError #29105

Closed
robtimus opened this issue Dec 17, 2021 · 6 comments
Closed

ErrorController no longer called for response.sendError #29105

robtimus opened this issue Dec 17, 2021 · 6 comments
Labels
status: duplicate A duplicate of another issue

Comments

@robtimus
Copy link

In Spring Boot 2.5.7 and before, a not found error, AuthenticationException or AccessDeniedException (basically anything that uses response.sendError) goes through the ErrorController - either BasicErrorController or a custom bean implementing ErrorController. In Spring Boot 2.6.0 and 2.6.1, this no longer occurs. That makes any custom ErrorController bean useless for a lot of cases, and requires other ways of getting the same functionality.

This is very easy to reproduce using non-custom code (apart from maybe a small main method). Put a breakpoint in class BasicErrorController, method error(HttpServletRequest request), and call any non-existing endpoint. In Spring Boot 2.5.7 the breakpoint is triggered, in 2.6.0 and 2.6.1 it is not.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 17, 2021
@robtimus
Copy link
Author

A small POC to showcase this: https://github.com/robtimus/spring-boot-test

Tests succeed:

  • mvn package -Dversion.spring-boot=2.5.7

Tests fail:

  • mvn package -Dversion.spring-boot=2.6.0
  • mvn package -Dversion.spring-boot=2.6.1

Note: Java 8 may be needed, although I tested it with Java 11 and Java 17 as well.

@quaff
Copy link
Contributor

quaff commented Dec 20, 2021

Maybe fixed by d9d161c, please try latest snapshot.

@robtimus
Copy link
Author

@quaff the 404 test no longer fails for Spring Boot 2.6.2-SNAPSHOT. However, the 401 error (triggered by an AuthenticationException) still does not go through the error controller. If that's intentional I would have expected something in the release notes.

@robtimus
Copy link
Author

I've done some deep debugging, and ErrorPageSecurityFilter.isAllowed calls getPrivilegeEvaluator().isAllowed, where getPrivilegeEvaluator is a DefaultWebInvocationPrivilegeEvaluator. This isAllowed returns false because there is no authentication. As a result, ErrorPageSecurityFilter.doFilter calls response.sendError instead of proceeding with the filter chain.

It appears that the ErrorPageSecurityFilter, which is new since Spring Boot 2.6.0, is too strict when it comes to the error controller itself.

@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Dec 20, 2021
@bclozel bclozel removed the for: team-attention An issue we'd like other members of the team to review label Dec 20, 2021
@mbhave
Copy link
Contributor

mbhave commented Dec 20, 2021

@robtimus Denying access to the error controller on a 401 when the error page is secure is intended behavior. This was the inconsistency that was fixed in #26356 where an authorization error would deny access but an authentication error would not. The reason this is not in the release notes is because it was a bug fix.

If you need to open access to the error controller, you can do so by adding permitAll() to /error in the security configuration.

@mbhave mbhave closed this as completed Dec 20, 2021
@mbhave mbhave added status: duplicate A duplicate of another issue and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 20, 2021
@robtimus
Copy link
Author

Thanks, I have solved the issue in my test suite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue
Projects
None yet
Development

No branches or pull requests

6 participants