Bug 36349 - Login for SCO/SCI broken by CSRF
Summary: Login for SCO/SCI broken by CSRF
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Self checkout (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Jonathan Druart
QA Contact: David Cook
URL:
Keywords:
Depends on: 34478 36102 36195
Blocks: 32256
  Show dependency treegraph
 
Reported: 2024-03-18 16:27 UTC by Nick Clemens (kidclamp)
Modified: 2024-12-23 08:06 UTC (History)
6 users (show)

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


Attachments
Bug 36349: Make sure CSRF token is included for all login scenarios (1.75 KB, patch)
2024-03-18 16:30 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36349: Fix AutoSelfCheckAllowed (1.19 KB, patch)
2024-03-19 10:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36349: Fix AutoSelfCheckAllowed (2.66 KB, patch)
2024-03-20 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36349: Make sure CSRF token is included for all login scenarios (1.81 KB, patch)
2024-03-20 15:06 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 36349: Fix AutoSelfCheckAllowed (2.71 KB, patch)
2024-03-20 15:06 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 36349: Remove passing CGI params from sco/printslip.pl (1.08 KB, patch)
2024-04-04 10:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36349: Add tests (4.27 KB, patch)
2024-04-04 10:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36349: Make sure CSRF token is included for all login scenarios (1.87 KB, patch)
2024-04-05 05:55 UTC, David Cook
Details | Diff | Splinter Review
Bug 36349: Fix AutoSelfCheckAllowed (2.77 KB, patch)
2024-04-05 05:55 UTC, David Cook
Details | Diff | Splinter Review
Bug 36349: Remove passing CGI params from sco/printslip.pl (1.14 KB, patch)
2024-04-05 05:55 UTC, David Cook
Details | Diff | Splinter Review
Bug 36349: Add tests (4.33 KB, patch)
2024-04-05 05:55 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 Nick Clemens (kidclamp) 2024-03-18 16:27:48 UTC
bug 36102 / bug 34478 added the csrf token to opac-auth for patron login, however, it isn't used for SCO/SCI logins
Comment 1 Nick Clemens (kidclamp) 2024-03-18 16:30:51 UTC
Created attachment 163356 [details] [review]
Bug 36349: Make sure CSRF token is included for all login scenarios

To test:
1 - In KTD visit:
    http://localhost:8080/cgi-bin/koha/sci/sci-main.pl
2 - Everything should be set for auto self check user etc, just login as a patron
    If not (or not using KTD) setup a self check user, enable SCO and SCI, set self check patron system preferences, then login with patron
3 - 403 Error
4 - Repeat with sco:
    http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
5 - Apply patch, restart all
6 - Try again, both should be successful
Comment 2 David Nind 2024-03-19 01:30:19 UTC
For the SCO log in (step 3), I have to log in three times - I'm assuming this is not what is expected:
1. Normal log in
2. SCO log in
3. Normal log on
--> Then get to check out items

In addition, when using KTD I needed to enable the SelfCheckInModule system preference for step 1.

Tested using both Firefox and Chromium.
Comment 3 Jonathan Druart 2024-03-19 10:33:30 UTC
We have a problem here.

We are removing the credential (login_userid and login_password from CGI) in get_template_and_user if not a POST (and not op ne 'cud-login').

I am failing at finding a correct fix. Should we actually fix the AutoSelfCheckAllowed behavior in sco-main.pl and deal with the credentials retrieved from the prefs in the controller?
Comment 4 Jonathan Druart 2024-03-19 10:39:53 UTC
Created attachment 163386 [details] [review]
Bug 36349: Fix AutoSelfCheckAllowed

This feels terribly wrong...
Comment 5 Jonathan Druart 2024-03-19 10:40:20 UTC
And logout is broken as well, you cannot finish the user session.
Comment 6 Jonathan Druart 2024-03-20 09:10:10 UTC
Created attachment 163514 [details] [review]
Bug 36349: Fix AutoSelfCheckAllowed

Move the check to C4::Auth.

Yes, it's not nice, I didn't expect to add exception to this code, and
it's adding more ugly code to get_template_and_user, but... Suggestions
welcome!
Comment 7 Jonathan Druart 2024-03-20 09:11:05 UTC
Still not happy with this new patch, but it seems to restore the feature.
Comment 8 Owen Leonard 2024-03-20 15:06:32 UTC
Created attachment 163552 [details] [review]
Bug 36349: Make sure CSRF token is included for all login scenarios

To test:
1 - In KTD visit:
    http://localhost:8080/cgi-bin/koha/sci/sci-main.pl
2 - Everything should be set for auto self check user etc, just login
    as a patron
    If not (or not using KTD) setup a self check user, enable SCO and
    SCI, set self check patron system preferences, then login with
    patron
3 - 403 Error
4 - Repeat with sco:
    http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
5 - Apply patch, restart all
6 - Try again, both should be successful

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 9 Owen Leonard 2024-03-20 15:06:34 UTC
Created attachment 163553 [details] [review]
Bug 36349: Fix AutoSelfCheckAllowed

Move the check to C4::Auth.

Yes, it's not nice, I didn't expect to add exception to this code, and
it's adding more ugly code to get_template_and_user, but... Suggestions
welcome!

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 10 Nick Clemens (kidclamp) 2024-03-21 13:47:00 UTC
This works, but it doesn't cover the SCI too - separate bug or want to update the patch?
Comment 11 Marcel de Rooy 2024-03-22 07:46:16 UTC
Changes to Auth need tests.
Comment 12 Jonathan Druart 2024-03-22 15:41:10 UTC
(In reply to Nick Clemens from comment #10)
> This works, but it doesn't cover the SCI too - separate bug or want to
> update the patch?

What's broken with SCI?
Comment 13 Jonathan Druart 2024-03-22 15:42:11 UTC
(In reply to Marcel de Rooy from comment #11)
> Changes to Auth need tests.

Yes, but first I would like to make sure there is not a better solution, because it smells!
Comment 14 Victor Grousset/tuxayo 2024-03-23 01:52:24 UTC
(In reply to Jonathan Druart from comment #12)
> (In reply to Nick Clemens from comment #10)
> > This works, but it doesn't cover the SCI too - separate bug or want to
> > update the patch?
> 
> What's broken with SCI?

Login fails too with
No CSRF token passed for POST http://localhost:8080/opac/sci/sci-main.pl
Comment 15 Victor Grousset/tuxayo 2024-03-23 02:07:16 UTC
I see what is confusing: When logged in the OPAC already (or in staff if it's the same domain), SCI seems to have no issue because it uses the current session.
Comment 16 Jonathan Druart 2024-03-23 12:04:57 UTC
(In reply to Victor Grousset/tuxayo from comment #14)
> (In reply to Jonathan Druart from comment #12)
> > (In reply to Nick Clemens from comment #10)
> > > This works, but it doesn't cover the SCI too - separate bug or want to
> > > update the patch?
> > 
> > What's broken with SCI?
> 
> Login fails too with
> No CSRF token passed for POST http://localhost:8080/opac/sci/sci-main.pl

With the patches?
Comment 17 Victor Grousset/tuxayo 2024-03-24 00:29:16 UTC
oops, I though the confusion was about whether or not both SCI and SCO were affected by the bug. Turns out patches indeed addresses both even if touching opac/sci/sci-main.pl wasn't needed.

---

Anyway, found this while testing:
1. Open both
  http://localhost:8080/cgi-bin/koha/sci/sci-main.pl
  and http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
2. log in and log out from SCI
3. go back to the SCO tab and try to log in
4. "The form submission failed (Wrong CSRF token). Try to come back, refresh the page, then try again."

That would likely only affect testing scenarios depending on detailed testing habits about when opening stuff. So not much impact likely.
I don't know if the cause of that could still have a wider relevance.
Comment 18 Jonathan Druart 2024-03-25 12:20:06 UTC
What you describe is "expected". At least other login forms are affected by this and should be reported on its own bug (ie. not only sci/sco).
Comment 19 Victor Grousset/tuxayo 2024-03-26 03:04:10 UTC
(In reply to Jonathan Druart from comment #18)
> What you describe is "expected". At least other login forms are affected by
> this and should be reported on its own bug (ie. not only sci/sco).

Ok, no deeper issue, great.

What is the next step? Calling for help on koha-devel to find a cleaner approach?
Or going with the current one? In that case I can sign-off. Found nothing else weird after messing around with the forms.
Comment 20 David Cook 2024-04-04 02:37:54 UTC
Sorry for neglecting this one so long. I'm taking a deeper look at the issue today...
Comment 21 David Cook 2024-04-04 05:06:11 UTC
With the 1st patch, we've got manual log in fixed for /cgi-bin/koha/sco/sco-main.pl

If it's the first session, I can manually log into /cgi-bin/koha/sci/sci-main.pl

(However, if I log into /cgi-bin/koha/sco/sco-main.pl and then log into /cgi-bin/koha/sci/sci-main.pl it doesn't work, as I'm presented with the message "Access denied. Sorry, the system doesn't think you have permission to access this page." It looks like it's because it's doing the "kick_out" since we have $is_sco_user. That's a current system thing so nothing to worry about.)

--

The 2nd patch fixes AutoSelfCheckAllowed, but I can see what Jonathan means about it being ugly. 

Something to keep in mind is that we're re-authenticating on every SCO page load, but my bug 34478 commit 8b69d0b4d3e72171d9f2d51234f345405a433c4f actually makes the AutoSelfCheck session persistent, so once we have that authenticated session, we don't really need to re-authenticate. 

One option could be to move the AutoSelfCheck auth process into "checkauth".

Another option could be to allow sco-main.pl to do a sort of "preauth" and create its own authenticated session before get_template_and_user() if necessary. (At the moment, this is sort of what we're trying to do by force stuffing the AutoSelfCheck credentials on every load of sco-main.pl.) However, this option would need some tweaks to things like get_template_and_user() and checkauth() to be able to accept a sessionID passed in (instead of just looking at the incoming session cookie).

So I think we could do something a bit smarter when handling our sessions.

--

But... both of those options would take more work. Jonathan has provided something that already appears to work, and we're getting closer to our May release deadline, so I think perhaps we shouldn't make the perfect the enemy of the good.
Comment 22 David Cook 2024-04-04 05:08:09 UTC
For completeness, we should remove the AutoSelfCheck stuff from opac/sco/printslip.pl as well, since it's covered by the second patch.
Comment 23 David Cook 2024-04-04 05:08:56 UTC
I need to switch tasks, but let me know what you think.

Otherwise, I can QA stamp this tomorrow...
Comment 24 Jonathan Druart 2024-04-04 10:31:11 UTC
Created attachment 164411 [details] [review]
Bug 36349: Remove passing CGI params from sco/printslip.pl
Comment 25 Jonathan Druart 2024-04-04 10:31:14 UTC
Created attachment 164412 [details] [review]
Bug 36349: Add tests
Comment 26 Jonathan Druart 2024-04-04 10:32:27 UTC
Good to go now if nothing better is found.
Comment 27 David Cook 2024-04-05 05:55:38 UTC
Created attachment 164444 [details] [review]
Bug 36349: Make sure CSRF token is included for all login scenarios

To test:
1 - In KTD visit:
    http://localhost:8080/cgi-bin/koha/sci/sci-main.pl
2 - Everything should be set for auto self check user etc, just login
    as a patron
    If not (or not using KTD) setup a self check user, enable SCO and
    SCI, set self check patron system preferences, then login with
    patron
3 - 403 Error
4 - Repeat with sco:
    http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
5 - Apply patch, restart all
6 - Try again, both should be successful

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 28 David Cook 2024-04-05 05:55:41 UTC
Created attachment 164445 [details] [review]
Bug 36349: Fix AutoSelfCheckAllowed

Move the check to C4::Auth.

Yes, it's not nice, I didn't expect to add exception to this code, and
it's adding more ugly code to get_template_and_user, but... Suggestions
welcome!

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 29 David Cook 2024-04-05 05:55:44 UTC
Created attachment 164446 [details] [review]
Bug 36349: Remove passing CGI params from sco/printslip.pl

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 30 David Cook 2024-04-05 05:55:47 UTC
Created attachment 164447 [details] [review]
Bug 36349: Add tests

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 31 David Cook 2024-04-05 05:57:59 UTC
I think this was only in Failed QA because missing test, so with it added I think we can skip back to Signed Off, and I'll add Passed QA.

--

It might not be the most elegant solution, but it's the most workable one, and I think workable is what we need right now.
Comment 32 Katrin Fischer 2024-04-05 09:52:52 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 33 Fridolin Somers 2024-04-17 15:21:06 UTC
Depends on Bug 34478 not in 23.11.x