Bug 40336 - Self-checkouts restart every second if invalid data in timeout sysprefs
Summary: Self-checkouts restart every second if invalid data in timeout sysprefs
Status: RESOLVED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Self checkout (show other bugs)
Version: Main
Hardware: All All
: P5 - low blocker
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 36586
Blocks:
  Show dependency treegraph
 
Reported: 2025-07-10 00:19 UTC by David Cook
Modified: 2025-07-16 23:10 UTC (History)
4 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 40336: Validate timeout syspref for self-checks (3.81 KB, patch)
2025-07-10 00:47 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2025-07-10 00:19:36 UTC
As per comments on bug 36586, there are situations where self-checkouts restart every second.

The cause - or at least one identified cause - appears to be invalid data in the SelfCheckInTimeout and SelfCheckTimeout system preferences.

The Javascript isn't properly validating the incoming data and thus the bug appears.
Comment 1 David Cook 2025-07-10 00:47:26 UTC
Created attachment 183910 [details] [review]
Bug 40336: Validate timeout syspref for self-checks

This patch validates the numbers passed to the self-check timeout
code, so that it must be a valid number >= 10 seconds. If it fails
this validation, it defaults to 120 seconds.

Test plan:
0. Apply the patch
1. Enable the SelfCheckInModule and WebBasedSelfCheck syspref
2. Try out these modules using varying values in the SelfCheckInTimeout
and SelfCheckTimeout system preferences resepectives. For instance:
    - A blank space (ie " ")
    - A negative number (e.g. -7)
    - An alphabetic string (e.g. aa)
    - A positive number less than 10 (e.g. 5)
    - A positive number greater than 10 (e.g. 60)
3. Check the "idle_timeout" value set in the timer objects using the
following in the browser console:
    console.log(window.sci_login_timer);
    console.log(window.sco_login_timer);
4. Note that all cases except positive numbers greater than or equal
to 10 get reset to 120
Comment 2 Emmi Takkinen 2025-07-10 06:40:13 UTC
Tested this on SCO and all values expect positive 10 or greater indeed reset to 120. But I'm still seeing constant restarting even with these values. So patch does work as intended but doesn't solve the whole problem. I'll try to set up a SCI on my environment and test that too just to be sure.
Comment 3 Emmi Takkinen 2025-07-10 07:19:16 UTC
Tested now with SCI, same results.
Comment 4 Jonathan Druart 2025-07-10 07:30:16 UTC
It would be good to have systempreferences.type supports "integer" and have such problems caught before (during the input).
Comment 5 Lucas Gass (lukeg) 2025-07-10 15:50:03 UTC
Sorry David, we've decided to revert Bug 36586 for now.
Comment 6 David Cook 2025-07-14 01:06:51 UTC
(In reply to Emmi Takkinen from comment #2)
> Tested this on SCO and all values expect positive 10 or greater indeed reset
> to 120. But I'm still seeing constant restarting even with these values. So
> patch does work as intended but doesn't solve the whole problem. I'll try to
> set up a SCI on my environment and test that too just to be sure.

Do you have an Internet accessible Koha that can reproduce this problem? I could take a look there.

Otherwise, I think I've troubleshooted this as far as I can.
Comment 7 David Cook 2025-07-14 01:07:15 UTC
(In reply to Jonathan Druart from comment #4)
> It would be good to have systempreferences.type supports "integer" and have
> such problems caught before (during the input).

Agreed. This would be a good use case for adding syspref validation methods.
Comment 8 David Cook 2025-07-14 01:10:39 UTC
(In reply to Lucas Gass (lukeg) from comment #5)
> Sorry David, we've decided to revert Bug 36586 for now.

No worries. Did you try out the patch or was it a strategic decision? Based off Emmi's experience, it looks like there could be something else going on there in any case.

Locally, we are suspending CSRF for self-check for older versions without bug 40108. I guess we'll just keep doing that until some other fix is done. 

We recently had an experience where a library's PC catalogue was resubmitting the opac-search.pl every second which was interesting. Library staff suspect a glitch with the kiosk software (rather than Koha) but who knows.
Comment 9 David Cook 2025-07-14 01:11:13 UTC
(In reply to David Cook from comment #8)
> Locally, we are suspending CSRF for self-check for older versions without
> bug 40108. I guess we'll just keep doing that until some other fix is done. 

Sorry I meant bug 36586
Comment 10 Emmi Takkinen 2025-07-16 05:25:19 UTC
(In reply to David Cook from comment #6)
> (In reply to Emmi Takkinen from comment #2)
> > Tested this on SCO and all values expect positive 10 or greater indeed reset
> > to 120. But I'm still seeing constant restarting even with these values. So
> > patch does work as intended but doesn't solve the whole problem. I'll try to
> > set up a SCI on my environment and test that too just to be sure.
> 
> Do you have an Internet accessible Koha that can reproduce this problem? I
> could take a look there.
> 
> Otherwise, I think I've troubleshooted this as far as I can.

All our Koha instances have restricted access. But I can try to take a look at this and see if what I can find.
Comment 11 Emmi Takkinen 2025-07-16 08:04:37 UTC
David, you were on the right track with this. I ran code from bug 36586 through Copilot and it pointed out that variable const idle_timeout has to be converted as number also in function login_timeout found from sci-main.tt and sco-main.tt: const idleTimeout = Number("[% Koha.Preference('SelfCheckTimeout') | html %]"); This stopped the constant refreshing. Unfortunately with this bugs patch in place converting no longer helps no matter how many Number() methods I add :D
Comment 12 David Cook 2025-07-16 23:09:23 UTC
(In reply to Emmi Takkinen from comment #11)
> David, you were on the right track with this. I ran code from bug 36586
> through Copilot and it pointed out that variable const idle_timeout has to
> be converted as number also in function login_timeout found from sci-main.tt
> and sco-main.tt: const idleTimeout = Number("[%
> Koha.Preference('SelfCheckTimeout') | html %]"); This stopped the constant
> refreshing. Unfortunately with this bugs patch in place converting no longer
> helps no matter how many Number() methods I add :D

Sorry I'm very confused about what you're trying to say here. It sounds like you're saying that the patch works, but that it also does not work?
Comment 13 David Cook 2025-07-16 23:10:47 UTC
I suppose it doesn't really matter anyway since bug 36586 has been reverted.