RequestRejectedException should be 400 by default

[brijesh15893]

Summary

We are getting 500 from spring security jar if we use // in URL, ideally it should give 400 bad request.

Ex. - https://com.sap/Spring//Security - as it has // in URL is should give 400 bad request but we are getting 500

Actual Behavior

https://com.sap/Spring//Security - as it had // in URL is should give 400 bad request

Please describe step by step the behavior you are observing

Use any valid URL and add // in it ex. /ThingConfiguration/v1/Packages// and use spring security version - 5.1.5.RELEASE.

Expected Behavior

it should give 400 bad request

Configuration

Version

spring security version - 5.1.5.RELEASE.

Sample

[ankurpathak]

@fhanik I would like to take this one.

[dadikovi]

Additional information:

It seems for me that in these cases the following exception is thrown:

org.springframework.security.web.firewall.RequestRejectedException: The request was rejected because the URL was not normalized. at org.springframework.security.web.firewall.StrictHttpFirewall.getFirewalledRequest(StrictHttpFirewall.java:248) at org.springframework.security.web.FilterChainProxy.doFilterInternal(FilterChainProxy.java:194)

If I understand properly the task is here that this exception should be handled with 400 http status (not 500). I could find an analogous solution in the BearerTokenAuthenticationFilter, where the exception is passed to authenticationFailureHandler which will return the necessary status code.

So I think this issue should be solved with a similiar solution: maybe with authenticationFailureHandler, or with a new type of failure handler, which is called in org.springframework.security.web.FilterChainProxy#doFilterInternal if an exception is thrown.

@fhanik What do you think about this solution?
@ankurpathak Are you still working on this issue, or can I take it?

[rwinch]

We cannot change the default, but you can now change the behavior with gh-7052

[dadikovi]

Well, I have a few questions, but for illustration I sent a draft commit too.

• When I tested this exception was thrown from line 192 where the rejected exception is not cought. Is there any reason for this, or is it okay to move the exception catch to the doFilterInternal method itself (see my draft commit)
• Is my understanding right that filterChainProxy should use HttpStatusRequestRejectedHandler instead of the default implementation?

[rwinch]

When I tested this exception was thrown from line 192 where the rejected exception is not cought. Is there any reason for this, or is it okay to move the exception catch to the doFilterInternal method itself (see my draft commit)

This was the past behavior and it should remain this way for passivity. User's were encouraged to add their own error handling using a Filter or mapping the Exception to the servlet containers error handling

Is my understanding right that filterChainProxy should use HttpStatusRequestRejectedHandler instead of the default implementation?

This will not change until Spring Security 6.x

[walec51]

the default 500 code is just bad
change it to 400 in the next major release

[rwinch]

@walec51 Yes The plan is to change the behavior in the next major release. We need to remain passive for those that may be catching the RequestRejectedException and handling it in the container error handling or within a Filter that is before Spring Security

[dratler]

Hi @rwinch ,
it this still for the greb ?

[rwinch]

Yes, but it won't happen until 6.0 which we don't have branch for yet

[ST-DDT]

I found a way to handle it according to my needs.

First the log level and status code:

@Override
public void handle(HttpServletRequest request, HttpServletResponse response, RequestRejectedException requestRejectedException) throws IOException {

	LOGGER.warn("Application firewall: {}", requestRejectedException.getMessage(),
			LOGGER.isDebugEnabled() ? requestRejectedException : null);

	request.setAttribute(RequestDispatcher.ERROR_EXCEPTION, requestRejectedException);

	response.sendError(403, "firewall rejected");
}

And additionally the response body via the ErrorController I already had:
(This made me really happy since I don't have to care about the response Content-Type/serialization)

@RestController
public class ErrorPageController implements ErrorController {

    @RequestMapping("/error")
    public ResponseEntity<ErrorResponse> renderErrorPage(final HttpServletRequest request) {
        return new ResponseEntity(request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE), new ErrorResponse(request.getAttribute(RequestDispatcher.ERROR_EXCEPTION)));
    }

}

I would still prefer a simple @ExceptionHandler(RequestRejectedException.class)...

[wwang107]

I used Kotlin and the following is my workaround

@Component
class RequestRejectedExceptionHandler : RequestRejectedHandler {

    override fun handle(
        request: HttpServletRequest,
        response: HttpServletResponse,
        requestRejectedException: RequestRejectedException
    ) {
        logger.warn(requestRejectedException.toString())
        response.sendError(HttpServletResponse.SC_BAD_REQUEST)
    }
}

[roeniss]

I coudn't find the history, but found the HttpStatusRequestRejectedHandler which returns 400 instead of 500.

my case:

• org.springframework.boot:spring-boot-starter-security:2.4.4
  • org.springframework.security:spring-security-config:5.4.5

There were DefaultRequestRejectedHandler and HttpStatusRequestRejectedHandler. And the former was primary so I changed it with custom configuration:

// kotlin 
@Configuration
class RequestRejectedHandlerConfig {
    @Bean
    fun requestRejectedHandler(): RequestRejectedHandler {
        return HttpStatusRequestRejectedHandler()
    }
}

edit) I've noticed that DefaultRequestRejectedHandler actually redirects request to /error, so I just made another GetMapping instead of config bean:

@Controller
class ErrorController {
    @GetMapping("/error")
    fun error(): ResponseEntity<Any> {
        return ResponseEntity.status(400).build()
    }
}