tractorcow / silverstripe-dynamiccache

Simple on the fly caching of dynamic content for Silverstripe
39 stars 27 forks source link

Correcting an issue where a new user's security token would be regene… #49

Closed nglasl closed 7 years ago

nglasl commented 7 years ago

…rated incorrectly, preventing secure form submission.

This is an interesting one that has taken me almost two days of debugging to track down. When using your module out of the box with a fresh install (using 3.2), the security ID for a user is being incorrectly regenerated for a new user. You can confirm this by caching a page that has a form present (similar to https://docs.silverstripe.org/en/3.4/tutorials/forms/), opening a new user session, and then by attempting to submit this form. You should be presented with a "your session has expired, please re-submit this form".

From what I've managed to track down, this is because https://github.com/tractorcow/silverstripe-dynamiccache/blob/master/code/DynamicCache.php#L254 updates the security ID, however subsequent page loads bring in a new security ID, since this isn't written back to the session as expected. The solution here may not be the most elegant, but it prevents this from happening.

In regards to the YAML change, I also noticed a bug where a Set-Cookie header was being cached, because it had an expires= that was being matched. This would result in a new user pulling in the previous user's security ID, resulting in similar issues to above.

It would be good to hear your thoughts on these, as they've been causing havoc with some forms on a site that I have at the moment. These changes resolve all the issues, and cause the security ID to be generated as expected.

nglasl commented 7 years ago

I was just wondering if you had managed to take a peek at this one? There's no rush, merely keen on helping improve the module :)

tractorcow commented 7 years ago

ok, only one change requested there. :)

nglasl commented 7 years ago

That's fair enough, I only updated the one because I wasn't convinced they were all meant to be "starts with" :)

Have updated and squashed the commits.

tractorcow commented 7 years ago

None of the tests failed so we're good.