Bug 20804 - Sanitize input of timeout syspref
Summary: Sanitize input of timeout syspref
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-05-23 05:54 UTC by Pablo AB
Modified: 2020-11-17 09:52 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.06, 19.11.12


Attachments
Bug 20804: Add tests (1.48 KB, patch)
2020-07-30 13:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20804: Add support for "days" to the timeout syspref (1.79 KB, patch)
2020-07-30 13:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20804: Add support for "days" to the timeout syspref (1.80 KB, patch)
2020-09-07 07:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20804: Add tests (1.54 KB, patch)
2020-10-02 14:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20804: Add support for "days" to the timeout syspref (1.87 KB, patch)
2020-10-02 14:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20804: Add tests (1.60 KB, patch)
2020-10-13 12:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20804: Add support for "days" to the timeout syspref (1.92 KB, patch)
2020-10-13 12:39 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pablo AB 2018-05-23 05:54:55 UTC
I naively change timeout syspref from default "1d" to "2h". I get logout immediatly, and after login again, at the first clic I was logged out again, with any user, even with koha_instance.

plack-error.log
Use of uninitialized value $lasttime in numeric lt (<) at /usr/share/koha/lib/C4/Auth.pm line 1425.
Argument "2h" isn't numeric in subtraction (-) at /usr/share/koha/lib/C4/Auth.pm line 1425.
Argument "2h" isn't numeric in subtraction (-) at /usr/share/koha/lib/C4/Auth.pm line 862.

I was back again with via sql update (value='1d') and flushing memcached.

I suggest:
- Don't been possible to input an invalid input on timeout (well, on any, but for the moment on this one). m/^[0-9]*[dD]?$/
- This syspref (maybe others also) shouldn't apply to koha_instance user (God shouldn't be able to microwave a burrito so hot he himself couldn't eat it).

Would be great 30m and 2h for minutes and hours, but that should be other bug.
Comment 1 Jonathan Druart 2020-07-30 13:20:15 UTC
Created attachment 107587 [details] [review]
Bug 20804: Add tests
Comment 2 Jonathan Druart 2020-07-30 13:20:20 UTC
Created attachment 107588 [details] [review]
Bug 20804: Add support for "days" to the timeout syspref

If the timeout syspref did not contain an integer, or was not matching
integer.'d|D', then it "fallback" to 0

We can easily add support for hours and fallback to 600 if the value is
not correct.
It will prevent the session to timeout immediately

Test plan:
0. Do not apply the patches
1. Fill the timeout syspref with "5m"
2. Login
3. Click somewhere
=> Notice that the session timed out
4. Apply the patches, restart_all
5. Login
6. Click somewhere
=> You have 5 minutes to enjoy Koha
7. Fill the pref with an incorrect value ("5x" for instance)
8. Logout, login
9. There is a warning in the log, and you have 10 minutes to enjoy Koha
Comment 3 Jonathan Druart 2020-08-20 07:39:41 UTC
Ivan, you modified the status but there is no signed off patches attached.
Comment 4 Fridolin Somers 2020-09-03 12:15:56 UTC
> 1. Fill the timeout syspref with "5m"
But minutes are not supported, only days and hours.
Comment 5 Fridolin Somers 2020-09-03 12:29:04 UTC
Also if invalid value, warn will be logged at each page.
I'd say we better tell default behavior in system preference description.
Also add in the description you can use h/H for hours.
Comment 6 Jonathan Druart 2020-09-07 07:29:23 UTC
Created attachment 109688 [details] [review]
Bug 20804: Add support for "days" to the timeout syspref

If the timeout syspref did not contain an integer, or was not matching
integer.'d|D', then it "fallback" to 0

We can easily add support for hours and fallback to 600 if the value is
not correct.
It will prevent the session to timeout immediately

Test plan:
0. Do not apply the patches
1. Fill the timeout syspref with "5h"
2. Login
3. Click somewhere
=> Notice that the session timed out
4. Apply the patches, restart_all
5. Login
6. Click somewhere
=> You have 5 hours to enjoy Koha
7. Fill the pref with an incorrect value ("5x" for instance)
8. Logout, login
9. There is a warning in the log, and you have 10 minutes (600 secondes) to enjoy Koha
Comment 7 Jonathan Druart 2020-09-07 07:30:07 UTC
(In reply to Fridolin SOMERS from comment #5)
> Also if invalid value, warn will be logged at each page.
> I'd say we better tell default behavior in system preference description.
> Also add in the description you can use h/H for hours.

I think we want to be verbose if the value is not correct.
Comment 8 Martin Renvoize 2020-10-02 14:06:28 UTC
Created attachment 111160 [details] [review]
Bug 20804: Add tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2020-10-02 14:06:39 UTC
Created attachment 111161 [details] [review]
Bug 20804: Add support for "days" to the timeout syspref

If the timeout syspref did not contain an integer, or was not matching
integer.'d|D', then it "fallback" to 0

We can easily add support for hours and fallback to 600 if the value is
not correct.
It will prevent the session to timeout immediately

Test plan:
0. Do not apply the patches
1. Fill the timeout syspref with "5h"
2. Login
3. Click somewhere
=> Notice that the session timed out
4. Apply the patches, restart_all
5. Login
6. Click somewhere
=> You have 5 hours to enjoy Koha
7. Fill the pref with an incorrect value ("5x" for instance)
8. Logout, login
9. There is a warning in the log, and you have 10 minutes (600 secondes) to enjoy Koha

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2020-10-02 14:07:19 UTC
This works well and I agree with the verbosity of the errors.

Signing off
Comment 11 Katrin Fischer 2020-10-13 12:39:24 UTC
Created attachment 111547 [details] [review]
Bug 20804: Add tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2020-10-13 12:39:29 UTC
Created attachment 111548 [details] [review]
Bug 20804: Add support for "days" to the timeout syspref

If the timeout syspref did not contain an integer, or was not matching
integer.'d|D', then it "fallback" to 0

We can easily add support for hours and fallback to 600 if the value is
not correct.
It will prevent the session to timeout immediately

Test plan:
0. Do not apply the patches
1. Fill the timeout syspref with "5h"
2. Login
3. Click somewhere
=> Notice that the session timed out
4. Apply the patches, restart_all
5. Login
6. Click somewhere
=> You have 5 hours to enjoy Koha
7. Fill the pref with an incorrect value ("5x" for instance)
8. Logout, login
9. There is a warning in the log, and you have 10 minutes (600 secondes) to enjoy Koha

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Jonathan Druart 2020-10-14 12:53:57 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 14 Lucas Gass 2020-11-13 14:38:17 UTC
backported to 20.05.x for 20.05.06
Comment 15 Aleisha Amohia 2020-11-17 04:06:06 UTC
backported to 19.11.x for 19.11.12
Comment 16 Victor Grousset/tuxayo 2020-11-17 09:52:55 UTC
Not backported to oldoldstable (19.05.x). Feel free to ask if it's needed.