RequestRejectedHandler does not reliable prevent Internal Server Error

[osiegmar]

Describe the bug
The RequestRejectedHandler was added to provide a configurable way of handling RequestRejectedException thrown by HttpFirewall. When using a HttpStatusRequestRejectedHandler the exception is handled by returning a HTTP status code (400 by default) to the client. Unfortunately this is not reliably the case and I consider this as a bug.

To Reproduce

• Use StrictHttpFirewall with HttpStatusRequestRejectedHandler
• Send a request with a HTTP header X-Test with a value containing \u0099

Then two things can happen:

1. Internal Server Error

• Implement an endpoint that reads the header X-Test
• A HTTP status code 500 is returned (org.springframework.security.web.firewall.RequestRejectedException: The request was rejected because the header value "Test �" is not allowed.)

or

2. Success

• Implement an endpoint that does not read headers
• A HTTP status code 200 is returned

Expected behavior

• Consistent behavior in both cases
• The configured status code of HttpStatusRequestRejectedHandler (400 by default).

Sample

https://github.com/osiegmar/spring-firewall-bug

Additional notes

• The character value \u0099 is considered invalid by StrictHttpFirewall (via regex pattern [\p{IsAssigned}&&[^\p{IsControl}]]*) but it seems to be valid according section 3.2 of RFC 7230 (field-vchar = VCHAR / obs-text ; VCHAR = %x21-7E ; obs-text = %x80-FF)
• I assume that there are more cases when this can happen (e.g. invalid parameter names)

[osiegmar]

Related PR: #8644

[rwinch]

@osiegmar Thank you for the report. Could you please create another ticket in regards to your concerns around the additional notes you provide. This will allow us to split up the work into discrete units and provide fixes faster

[osiegmar]

Thanks a lot!

I can confirm that PR #11670 solves this issue by ensuring HttpStatusRequestRejectedHandler can do its work even if the RequestRejectedException is wrapped (e.g. by a ServletException). A status code of 400 is returned instead of 500.

I understand that my other expectation (consistent behaviour for read and unread HTTP headers) is not the currently intended behaviour. I’l probably create a new enhancement issue for that.

[rwinch]

I understand that my other expectation (consistent behaviour for read and unread HTTP headers) is not the currently intended behaviour. I’l probably create a new enhancement issue for that.

If you still have concerns around what is being rejected, I'd create a ticket for that as well