Created attachment 54313 [details] [review] Bug 17110: Lower CSRF expiry in Koha::Token Default expiry in WWW:CSRF is one week. This patch sets it to 8 hours by default in Koha, and allows to change the expiry period individually by passing MaxAge. Test plan: [1] Put items in your cart. [2] Apply the example patch too. [3] Send the cart from opac within the allotted 10 seconds. [4] Send again, but wait some 10 seconds before submitting. Too late!
Example patch and adjusted test still coming..
Created attachment 54323 [details] [review] Bug 17110: Add unit test for MaxAge parameter in Token.t Test plan: Run t/Token.t
Created attachment 54324 [details] [review] Bug 17110: [EXAMPLE PATCH] CSRF expiry of 10 seconds in opac-sendbasket This patch should NOT be pushed ! It is used to test the first patch only.
Created attachment 54473 [details] [review] Bug 17110: Lower CSRF expiry in Koha::Token Default expiry in WWW:CSRF is one week. This patch sets it to 8 hours by default in Koha, and allows to change the expiry period individually by passing MaxAge. Test plan: [1] Put items in your cart. [2] Apply the example patch too. [3] Send the cart from opac within the allotted 10 seconds. [4] Send again, but wait some 10 seconds before submitting. Too late! Tested 3 patches together, works as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 54474 [details] [review] Bug 17110: Add unit test for MaxAge parameter in Token.t Test plan: Run t/Token.t Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 54475 [details] [review] Bug 17110: [EXAMPLE PATCH] CSRF expiry of 10 seconds in opac-sendbasket This patch should NOT be pushed ! It is used to test the first patch only. Signed-off-by: Marc Véron <veron@veron.ch>
Could you please detail why you need this change? I will break the following use case: - Start to fill a form - *Ring belt end of the day* - you hurry up to get back at home quickly - Tomorrow morning, you finish to fill the form - Submit - You lost your changes Ok it's a bit far-fetched but I don't understand what will bring us this 8-hours limitation.
(In reply to Jonathan Druart from comment #8) > Could you please detail why you need this change? > I will break the following use case: > - Start to fill a form > - *Ring belt end of the day* > - you hurry up to get back at home quickly > - Tomorrow morning, you finish to fill the form > - Submit > - You lost your changes > > Ok it's a bit far-fetched but I don't understand what will bring us this > 8-hours limitation. You only need the token between loading the form and submitting it. I do not understand why you need 7 days for doing so? Suppose an attacker got a CSRF token somehow from one user. Now he only needs that user to click on a malicious Koha URL that also sends the token. The amount of danger is obviously directly related to the length of the expiry period. Shorter is better, but should be balanced with ease of use.
Created attachment 54806 [details] [review] Bug 17110: Lower CSRF expiry in Koha::Token Default expiry in WWW:CSRF is one week. This patch sets it to 8 hours by default in Koha, and allows to change the expiry period individually by passing MaxAge. Test plan: [1] Put items in your cart. [2] Apply the example patch too. [3] Send the cart from opac within the allotted 10 seconds. [4] Send again, but wait some 10 seconds before submitting. Too late! Tested 3 patches together, works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 54807 [details] [review] Bug 17110: Add unit test for MaxAge parameter in Token.t Test plan: Run t/Token.t Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
I do not make up my mind on what it's best. So let it go and see later if we need to adapt it. Another idea was to use max(pref(timeout), 8h or 24h) for the value of CSRF_EXPIRY_HOURS
(In reply to Jonathan Druart from comment #12) > I do not make up my mind on what it's best. So let it go and see later if we > need to adapt it. > Another idea was to use max(pref(timeout), 8h or 24h) for the value of > CSRF_EXPIRY_HOURS I will open up a new report for lowering the default timeout value. If that meets approval, we could change this accordingly. Thx for QA. PS Note that we also still have bug 15428.. And now bug 17187.
Pushed to master for 16.11, thanks Marcel!
(In reply to Kyle M Hall from comment #14) > Pushed to master for 16.11, thanks Marcel! Pushed to 16.05.x, for 16.05.12 release (required for BZ-17096)