Ensure the refreshed CSRF cookie retains the original value #38229
Conversation
|
✔️ The latest workflow run for the pull request has completed successfully. It should be safe to merge provided you have a look at the other checks in the summary. You can consult the Develocity build scans. |
| LOG.debugf("CSRF token found in the token header"); | ||
| // Verify the header, make sure the header value, possibly signed, is returned as the next cookie value | ||
| verifyCsrfToken(requestContext, routing, config, cookieToken, csrfTokenHeaderParam); | ||
| } else if (!config.tokenSignatureKey.isEmpty()) { |
There was a problem hiding this comment.
| } else if (!config.tokenSignatureKey.isEmpty()) { | |
| } else if (config.tokenSignatureKey.isPresent()) { |
|
Thanks @ia3andy for a quick check, sorry I missed your suggestion, I'll simplify as you propose during some other fix which will be probably needed soon enough :-) |
There was a problem hiding this comment.
OK, so it's too late for a review, too bad.
I think there's something that can potentially go bad here, if a client is stuck with a non-expired cookie or a header and the token is signed with a previous signature: in this case we always generate a bad request, even for safe methods, and so we never renew the token or fix the situation.
If that happens, the only way users can recover is by waiting for the cookie to expire, or manually clean cookies.
I think for safe methods, token issues should auto-recover, no?
| // If the signature is required, then we can not use the current cookie value | ||
| // as the HTML form token key because it represents a signed value of the previous key | ||
| // and it will lead to the double-signing issue if this value is reused as the key. | ||
| // It should be fine for simple HTML forms anyway |
There was a problem hiding this comment.
I do not understand this comment. We have two ways to get a token: cookie or header. Here we're in a situation where we have a signed cookie, and we don't have a header. Why do we have to regenerate a token? Why would the previous signature be invalid?
There was a problem hiding this comment.
I guess this would lead to double-signing given that signing is done in the ResponseFilter.
Is there a specific reason why signing cannot be done in generateNewCsrfToken(..) and use a single routing parameter?
|
@sberyozkin any opinion about the comments? |
| if (csrfTokenHeaderParam != null) { | ||
| LOG.debugf("CSRF token found in the token header"); | ||
| // Verify the header, make sure the header value, possibly signed, is returned as the next cookie value | ||
| verifyCsrfToken(requestContext, routing, config, cookieToken, csrfTokenHeaderParam); |
There was a problem hiding this comment.
Should this situation really abort the request if token is incorrect? I think it should verify and generate a new cookie if verification fails. I think it would resolve @FroMage 's concern about changed signature key (stuck cookie).
What is the reason for converting a request header to a cookie? Based on my understanding, the important part is preserving the cookie token btw requests.
Fixes #38225.
I'm feeling totally wrecked after working on another CSRF PR and it is only a middle of Jan :-)
IMHO this PR should fix all the issues.
Here is what it does when the CSRF cookie is present for safe requests:
//in the path and it has no Cookie objects, so I'm just comparing the values, the dates check in the HTML forms should be enough though@gsmet I'm not sure you'd like to wait till @FroMage and @ia3andy can confirm it resolves the reported HTMX issue with multiple forms but CC-ing just in case.
Also CC @BarDweller