Reset CSRF cookie to minimize a risk of failures due to its expiry #37725
Conversation
This comment has been minimized.
This comment has been minimized.
|
Thanks Steph @FroMage, I wonder if there has to be a property enabling this cookie refreshment by default so that users can choose not to make it last indefinitely. I think I'll just add it before merging. |
|
I don't see why anyone would want to do that. Perhaps better to wait until someone requests this with a use-case? |
|
Hi Steph @FroMage, I was thinking today, may be the easiest is simply set the default cookie age to a few hours which will support most cases and if a whole day has to be supported then one will just increase the age. |
|
Hey @FroMage I've decided to go ahead with this PR as after I started fixing #37928 I realized I was typing exactly the same code on the GET path as in this PR, this PR just needs a minor tweak to ensure the cookie is correctly reset when the token signature is required, so instead of resetting the token immediately in various places it will be done in the response filter, in a single place |
sberyozkin
force-pushed
the
csrf_refresh_cookie
branch
3 times, most recently
from
f21dbc1
to
32cfa46
Compare
|
@FroMage I've also set a default cookie token age to 2H |
sberyozkin
force-pushed
the
csrf_refresh_cookie
branch
from
32cfa46
to
5a95be2
Compare
|
Sorry for the noise with multiple pushes, hopefully not too many people are getting them today :-) |
|
✔️ 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. |
…he token This is only a test to make sure we never regress on such a common use-case. This was already fixed in quarkusio#37725
|
Thanks! |
…he token This is only a test to make sure we never regress on such a common use-case. This was already fixed in #37725
…he token This is only a test to make sure we never regress on such a common use-case. This was already fixed in quarkusio#37725
…he token This is only a test to make sure we never regress on such a common use-case. This was already fixed in quarkusio#37725 (cherry picked from commit 511b0c7)
Fixes #36946.
Fixes #37928.
@FroMage FYI, it won't guarantee the failure won't ever happen, for example, the cookie age is 10 mins and the user just sits idle for 11 min and then returns, but with a reasonably large cookie age and with this PR recycling cookies, the chance of it happening will be very low.
If you'd like please give this PR a try with Renarde or you can try snapshots later.
Andy, @ia3andy FYI
It is not urgent to review it, thanks