-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Provide a way to handle RequestRejectedException #5007
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
Cannot use Spring Security greater than 5.0.0. Unless fixed, all releases past 5.0.0 are moot |
We will look into this for 5.1. In the meantime, you can handle this by handling the exception (you should be doing this anyways). Handle the ExceptionSpring BootFor example, with Spring Boot add ErrorPageRegistrar: @Bean
public static ErrorPageRegistrar securityErrorPageRegistrar() {
return registry -> registry.addErrorPages(new ErrorPage(RequestRejectedException.class, "/errors/400"));
} @Controller
public class SecurityErrorController {
@ResponseStatus(HttpStatus.BAD_REQUEST)
@RequestMapping("/errors/400")
String error(RequestRejectedException e) {
return "errors/400";
}
}
NOTE: Ensure the error page is public (i.e. permitAll) so that unauthenticated users are able to view the error page. Servlet Container
Ensure you add a URL mapping for NOTE: Ensure the error page is public (i.e. permitAll) so that unauthenticated users are able to view the error page. Customizing HttpFirewallYou can also revert to the previous |
I would recommend including #5044 with this issue |
@rwinch I tried those but they still result in an unwanted stacktrace in the log, because the exception is happening outside the normal exception handling flow. Looking at the stacktrace, it is not caught anywhere and so can't be processed by exception or error page handlers |
For anyone trying to get rid of the stacktrace and is running undertow:
|
Thanks to this new feature I discovered that Jetty was returning HTML errors for exception thrown outside of spring MVC. To handle this RequestRejectedException I did: contextHandler.setErrorHandler( new JsonErrorHandler() ); and in JsonErrorHandler public class JsonErrorHandler extends ErrorHandler
{
private final ObjectMapper mapper = new ObjectMapper();
@Override
protected void generateAcceptableResponse( Request baseRequest, HttpServletRequest request,
HttpServletResponse response, int code, String message, String mimeType ) throws IOException
{
response.setStatus( 400 );
baseRequest.setHandled( true );
Writer writer = getAcceptableWriter( baseRequest, request, response );
if ( writer != null )
{
response.setContentType( MimeTypes.Type.APPLICATION_JSON.asString() );
Throwable th = (Throwable) request.getAttribute( RequestDispatcher.ERROR_EXCEPTION );
writer.write( getPayload( th ) );
}
} getPayload should return a JSON string describing the error. |
@rwinch
but it is not working for me. do you have any pointer? spring is not forwarding the request to this endpoint. Checked and saw that this bean is initialised/picked up during startup. after some debugging and searching I came accross |
Hope this one does not get pushed to another milestone. |
Another solution to skip the logging is using janino togheter with logback and filtering out the exception. For example:
And for the appropriate appender add a EvaluatorFilter using JaninoEventEvaluator , <appender name="FILE" class="ch.qos.logback.core.rolling.RollingFileAppender">
<filter class="ch.qos.logback.core.filter.EvaluatorFilter">
<evaluator class="ch.qos.logback.classic.boolex.JaninoEventEvaluator">
<expression>return throwable instanceof org.springframework.security.web.firewall.RequestRejectedException;</expression>
</evaluator>
<OnMismatch>NEUTRAL</OnMismatch>
<OnMatch>DENY</OnMatch>
</filter>
<encoder>
<Pattern>
${FILE_LOG_PATTERN}
</Pattern>
</encoder>
<file>${LOG_FILE}</file>
</appender> Now all RequestRejectedException logging should be filtered out by logback. Some references: |
I implemented a workaround for this bug that is fairly flexible (see How to intercept a RequestRejectedException in Spring?). I also posted a second solution for those who simply want to suppress or control the logging. Essentially, I extended |
Is this going to be resolved or its gone in oblivion? |
@hth Thanks for the nudge. With a reasonable workaround and lots to do we haven't had time to do it. If someone is interested in submitting a PR I'd be glad to help coach them through it. |
the same to me
|
"reasonable workaround" ... does not work. It does respond with the configured 400 JSP but still logs the exception with stacktrace. The exception is logged here (StandardWrapperValve)...
This exception should be logged but there is no need for the stacktrace. During a security scan this exception overwhelms the logs. There are other Spring Security exceptions that are logged as WARN without the stacktrace. |
This is likely something you want to be drawn to your attention since it may be someone trying to bypass the firewall. If you are not interested in that log, you can also create a Filter that catches the exception and handles it. |
One way to handle by implement GenericFilterBean and more important defined the order if you have multiple filters in your application "//" in url @component
} |
Check my answer in below thread: |
A much easier and simpler way to handle it could be by using Spring AOP. We can wrap the default FilterChainProxy execution with a try-catch block and translate the exception to a 400 response.
|
@rwinch if you'd like a contribution, then it would be quite helpful if you provide some hints as to how you'd like it to be solved, so that it fits well into the architecture. |
Thanks for volunteering @leonard84 The issue is yours. I think we do the following
Let me know if you have any questions or otherwise need any help contributing. |
@rwinch the |
I've started with the implementation leonard84@44bf916 (still WIP), and I'd like some feedback. However, there are some issues:
|
Thanks @leonard84! Could you submit a draft pr so we can discuss more easily. |
Done #7052 |
i found this code in the if (path.indexOf("//") > -1) {
return false;
} Why add this code? i think browsers support this . Thx |
Any idea when is this version available for consumption? |
The linked pull request #7052 indicates it was made available in 5.4.0-M1. You can look at the milestones to find out when it will be in a GA release. |
Summary
With Spring Security 4.2.4 used in a Spring Boot application, accessing a "non-normalized" URI, e.g. one containing a double //, will cause an HTTP 500.
Actual Behavior
Access to a URI containing a double // causes an HTTP 500 response.
Expected Behavior
Access to such a URI causes some HTTP 4xx response, e.g. 400, 403 or 404. I'd expect that, because it's the client who has to change the request.
Version
Affected version is Spring Security 4.2.4.
Comment
I did some research in the code, and it looks as if the exception thrown in
spring-security/web/src/main/java/org/springframework/security/web/firewall/StrictHttpFirewall.java
Line 123 in 1517e9b
The text was updated successfully, but these errors were encountered: