Due to me adding the sites wrongly to the caching ignore list, I got into the situation where the CSRF token failed. To my disappointment I had to realize though that no error is reported when this happens and with my site being based on the example templates, the form is just displayed again with all the entered data lost and no action performed.
Is this on purpose, i.e. should CSRF checks fail silently for security reasons?
I mean it generally shouldn’t happen, but what if someone is taking multiple times to fill out a form and then the token is expired and when they press send all the data is lost and they don’t even know about it!
I get that one could call validate() manually and return an error based on that, the question then is where to insert that check so it doesn’t get called multiple times?
Thanks for all the help! Really great plugin, @mzur!
I’m not 100 % sure what your point is but I read it as: “If a user takes a long time filling out a form and the CSRF token expires in the meantime, submission of the form should be rejected but the input data sould be retained.” Is that correct?
OWASP states: “If […] the value provided does not match the value within the session, then the request should be aborted, token should be reset and the event logged as a potential CSRF attack in progress.”
I’ve always interpreted “the request should be aborted” as “the request should be ignored” like a firewall would ignore malicious requests. This is similar to how Laravel handles a token mismatch, where I often look for how something should be done correctly. Actually Uniform used to do this exactly like Laravel (just throw an error) but I changed that a while ago. But I see your point that this might not be very user friendly in the case described above (which should not happen very often, though).
With the same origin policy for JS requests in place I don’t see any security implications for redirecting the request with a new token and the old form data. But I’m no security expert so I’m very careful with this. Can you provide any references or trustworthy examples where it’s done like this?
I have seen many examples in the past where it was done like here, i.e. where your form data simply gets thrown away, potentially leaving you with day(s) worth of work lost in the void. I mean who hasn’t written some elaborate answer on some forum only to get redirected to the login page when pressing send?
However, my problem isn’t even really with the fact that the form data gets lost, but more with the fact that I can not make a proper distinction. Currently when the token has run out, all the users sees is the form refreshing and no indication whether it was sent successful or failed. So you not only lose all your form data, but you can’t really tell whether it was an error or not.
This can be worked around by running a check on:
Was there a POST request?
Was the form NOT successful?
Was there no error generated?
My point is, when the CSRF check fails, then there should be an error that I can properly handle in my code and I shouldn’t have to depend on vague checks.
In my mind the contract/assert should be that if success() returns false, then count(errors())must be> 0.
All this wouldn’t be an issue if Uniform would handle a token mismatch as I mentioned before, so let’s focus on that. As this is a generic error it should be handled correctly by Uniform, too, and not each time anew by the developer.
So, again, the proposed solution is: In case of a token mismatch, Uniform should a) retain the entered form data and b) show an error saying that the user should submit the form again. It’s only that I’d like to do some research on this to see if there aren’t any security implications I’ve overlooked. And as I currently don’t have a lot of time at hand I welcome everybody who is interested in this change (i.e. you ) to point me to some references that convince me it’s safe to implement this change.
I’ve released kirby-form v1.1.0 with the updated handling of a CSRF token mismatch. The current Uniform master already uses this version of kirby-form.
) to point me to some references that convince me it’s safe to implement this change.