Summary: | New CSRF token generated everytime we need one | ||
---|---|---|---|
Product: | Koha | Reporter: | Jonathan Druart <jonathan.druart> |
Component: | Architecture, internals, and plumbing | Assignee: | David Cook <dcook> |
Status: | Pushed to oldstable --- | QA Contact: | Nick Clemens (kidclamp) <nick> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | dcook, didier.gautheron, fridolin.somers, jonathan.druart, laurent.ducos, lucas, m.de.rooy, nick, phil |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
24.05.00,23.11.04
|
|
Circulation function: | |||
Bug Depends on: | |||
Bug Blocks: | 34478 | ||
Attachments: |
Bug 35955: Cache CSRF token in template plugin
Bug 35955: Add tests Bug 35955: Cache CSRF token in template plugin Bug 35955: Add tests |
Description
Jonathan Druart
2024-01-31 11:31:28 UTC
(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 |