Introduce logs in RestCsrfPreventionFilter to improve troubleshooting. #452
Conversation
| return true; | ||
| } | ||
|
|
||
| storeNonceToResponse(response, Constants.CSRF_REST_NONCE_HEADER_NAME, | ||
| Constants.CSRF_REST_NONCE_HEADER_REQUIRED_VALUE); | ||
| response.sendError(getDenyStatus(), | ||
| sm.getString("restCsrfPreventionFilter.invalidNonce")); | ||
| if (log.isErrorEnabled()) { |
There was a problem hiding this comment.
I don't think this should be logged at ERROR level, because an attacker could fill-up your logs just by repeatedly sending-in garbage. I would recommend using DEBUG log-level for this.
There was a problem hiding this comment.
I agree with @ChristopherSchultz !
| return true; | ||
| } | ||
|
|
||
| storeNonceToResponse(response, Constants.CSRF_REST_NONCE_HEADER_NAME, | ||
| Constants.CSRF_REST_NONCE_HEADER_REQUIRED_VALUE); | ||
| response.sendError(getDenyStatus(), | ||
| sm.getString("restCsrfPreventionFilter.invalidNonce")); | ||
| if (log.isErrorEnabled()) { | ||
| log.error("CSRF validation for REST failed! Request with method [" + request.getMethod() + "] and URI [" | ||
| + RequestUtil.filter(request.getRequestURI()) |
There was a problem hiding this comment.
I don't think there is a reason to use RequestUtil.filter() for this value; it's not going to be embedded in HTML so it's okay to use the value as-is.
| @@ -217,6 +238,10 @@ public boolean apply(HttpServletRequest request, HttpServletResponse response) { | |||
| } | |||
| storeNonceToResponse(response, Constants.CSRF_REST_NONCE_HEADER_NAME, | |||
| nonceFromSessionStr); | |||
| if (log.isDebugEnabled()) { | |||
There was a problem hiding this comment.
I might use TRACE, here.
| @@ -192,6 +209,10 @@ private String extractNonceFromRequestParams(HttpServletRequest request) { | |||
| String nonce = params[0]; | |||
| for (String param : params) { | |||
| if (!Objects.equals(param, nonce)) { | |||
| if (log.isErrorEnabled()) { | |||
There was a problem hiding this comment.
Same: use DEBUG not ERROR.
| @@ -217,6 +238,10 @@ public boolean apply(HttpServletRequest request, HttpServletResponse response) { | |||
| } | |||
| storeNonceToResponse(response, Constants.CSRF_REST_NONCE_HEADER_NAME, | |||
| nonceFromSessionStr); | |||
| if (log.isDebugEnabled()) { | |||
| log.debug("CSRF Fetch request is succesfully handled - nonce is added to the response." | |||
| return true; | ||
| } | ||
|
|
||
| storeNonceToResponse(response, Constants.CSRF_REST_NONCE_HEADER_NAME, | ||
| Constants.CSRF_REST_NONCE_HEADER_REQUIRED_VALUE); | ||
| response.sendError(getDenyStatus(), | ||
| sm.getString("restCsrfPreventionFilter.invalidNonce")); | ||
| if (log.isErrorEnabled()) { |
There was a problem hiding this comment.
I agree with @ChristopherSchultz !
|
It has been 2 months and the requested changes have not been made. I made the changes and a number of other fixes and applied this manually. |
Add some logs in RestCsrfPreventionFilter that would improve troubleshooting in case of failed CSRF validation.
Note that the RequestUtil.filter method is used only to apply some basic sensitive characters filtering of the requested path.
But as this method is deprecated in v.8.5.x and removed in v.9.0.x, would you recommend an alternative method I could use in this case?