Fix non-standard HTTP method for CsrfWebFilter #8452
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
There was a problem hiding this comment.
Thanks for the PR @parikshitdutta! I've added some feedback inline
| @@ -187,7 +187,7 @@ public static void skipExchange(ServerWebExchange exchange) { | |||
| @Override | |||
| public Mono<MatchResult> matches(ServerWebExchange exchange) { | |||
| return Mono.just(exchange.getRequest()) | |||
| .map(r -> r.getMethod()) | |||
| .map(r -> (r.getMethod() != null ? r.getMethod() : Mono.empty())) | |||
There was a problem hiding this comment.
This doesn't look right to me. You shouldn't map to a publisher. It should be a .flatMap and both code paths need to be a Publisher
| .map(r -> (r.getMethod() != null ? r.getMethod() : Mono.empty())) | |
| .flatMap(r -> Mono.justOrEmpty(r.getMethod())) |
There was a problem hiding this comment.
Just to understand the required change, my understanding was the return from "getMethod" in question is time bounded, not a value for future, therefore it would be okay to use condition in map.
Please enlighten me if I am missing something.
There was a problem hiding this comment.
The issue is that if r.getMethod() is null then, it is a .map(Mono.empty()) means that it is now a Mono<Mono<Empty>>. Instead you want just Mono.empty().
There was a problem hiding this comment.
totally make sense! thanks for clarifying.
| @@ -284,4 +305,12 @@ String ok() { | |||
| return "ok"; | |||
| } | |||
| } | |||
|
|
|||
| static class MockNonStandardHttpRequest { | |||
There was a problem hiding this comment.
Is there a need to externalize this or could you just directly use MockServerHttpRequest.method directly?
There was a problem hiding this comment.
The idea was to build MockServerWebExchange from NonStandard HTTP Request (i.e: "PURGE", "REVOKE" etc.), by using user defined string which is not a valid HTTP Method.
Now mocking without externalizing meaning I have to use get, post or other defined builders in MockServerHttpRequest (MockServerHttpRequest itself not extensible) or even if I try to do in different way, most likely i would end up writing redundant code.
Please share your thought.
There was a problem hiding this comment.
It looks like it is only being used once, so I think it can safely be consolidated without having redundant code.
| public void filterWhenNonStandardHTTPMethodUsedThenMatchesRequireCSRFProtection() { | ||
| MockServerWebExchange nonStandardHttpRequest = from(MockNonStandardHttpRequest.nonStandardHTTPMethod("/")); | ||
|
|
||
| ServerWebExchangeMatcher serverWebExchangeMatcher = spy(CsrfWebFilter.DEFAULT_CSRF_MATCHER); |
There was a problem hiding this comment.
I don't think we need to test the CsrfWebFilter and the matcher here. I think we can just check CsrfWebFilter.DEFAULT_CSRF_MATCHER directly. If we need to test both, we should probably create separate tests.
…thod in CsrfWebFilter
|
Hi @rwinch, Please review the changes as you find time, I will look forward to your feedback. |
rwinch
changed the title
rwinch
added
in: web
spring-projects-issues
added
the
status: backported
|
Thanks for the Pull Request! This is now merged into master 😄 |
Fixed NPE for HTTP requests with non-standard HTTP method in CsrfWebFilter
Expected Result:
Non-standard HTTP methods should match for CsrfProtection Requirement now, instead of throwing NPE and resulting HTTP 5xx
Fixes gh-8149