Why do we generate a new token everytime? I thought it was one per session.
(In reply to Jonathan Druart from comment #0) > Why do we generate a new token everytime? I thought it was one per session. I don't think we've ever done per-session tokens for CSRF. I think it's always been per-request, as they're regarded as more secure than per-session tokens (based on the OWASP CSRF prevention guidelines at least).
(In reply to David Cook from comment #1) > (In reply to Jonathan Druart from comment #0) > > Why do we generate a new token everytime? I thought it was one per session. > > I don't think we've ever done per-session tokens for CSRF. I think it's > always been per-request, as they're regarded as more secure than per-session > tokens (based on the OWASP CSRF prevention guidelines at least). Synchronizer Token Pattern CSRF tokens should be generated on the server-side and they should be generated only once per user session or each request. Because the time range for an attacker to exploit the stolen tokens is minimal for per-request tokens, they are more secure than per-session tokens. However, using per-request tokens may result in usability concerns. [Back button etc] And since we chose for request years ago, why change it now? Good reasons?
We are not doing "per-request", we are generating one for each form. If there are several forms on the same page we will generate several ones. I am not asking to change how we generate it, but asking if it is relevant and if this is what we really want.
(In reply to Jonathan Druart from comment #3) > We are not doing "per-request", we are generating one for each form. If > there are several forms on the same page we will generate several ones. > > I am not asking to change how we generate it, but asking if it is relevant > and if this is what we really want. Thx for clarifying :)
(In reply to Jonathan Druart from comment #3) > We are not doing "per-request", we are generating one for each form. If > there are several forms on the same page we will generate several ones. > > I am not asking to change how we generate it, but asking if it is relevant > and if this is what we really want. I also appreciate the clarification. I just took a look at Koha/Template/Plugin/Koha.pm and I see what you mean. No, I don't think that we really want this. I think we should generate it once, store it in the L1 cache, and re-use the same token string for the duration of the HTTP request.
(In reply to David Cook from comment #5) > No, I don't think that we really want this. I think we should generate it > once, store it in the L1 cache, and re-use the same token string for the > duration of the HTTP request. Actually we should be able to get away with putting it in the Koha/Templates/Plugin/Koha object instead of using L1 cache, since I think the lifecycle of that object is tied to the HTTP request anyway...
Created attachment 162485 [details] [review] Bug 35955: Cache CSRF token in template plugin This change uses the Koha::Cache::Memory::Lite cache to cache the CSRF token, so that it is only generated once, and is re-used by the Koha::Template::Plugin::Koha object throughout the entire template processing for the HTTP request. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 162486 [details] [review] Bug 35955: Add tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 162511 [details] [review] Bug 35955: Cache CSRF token in template plugin This change uses the Koha::Cache::Memory::Lite cache to cache the CSRF token, so that it is only generated once, and is re-used by the Koha::Template::Plugin::Koha object throughout the entire template processing for the HTTP request. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 162512 [details] [review] Bug 35955: Add tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Pushed to master for 24.05.00.
May we backport this for better performance ?
(In reply to Fridolin Somers from comment #12) > May we backport this for better performance ? I can't imagine that it would hurt.
OK thanks, will do
Pushed to 23.11.x for 23.11.04