Bug 40943

Summary: Store session_id in userenv
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: AuthenticationAssignee: Jonathan Druart <jonathan.druart>
Status: Pushed to main --- QA Contact: David Cook <dcook>
Severity: enhancement    
Priority: P5 - low CC: dcook, dpavlin, lucas, m.de.rooy, martin.renvoize, mik, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37041
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40993
GIT URL: Initiative type: ---
Sponsorship status: --- Comma delimited list of Sponsors:
Crowdfunding goal: 0 Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.11.00
Circulation function:
Attachments: Bug 40943: Store session_id in userenv
Bug 40943: Use session_id from userenv instead of cookie for value_builder
Bug 40943: Store session_id in userenv
Bug 40943: Use session_id from userenv instead of cookie for value_builder
Bug 40943: (QA follow-up) Add a few POD lines for new sub
Bug 40943: Store session_id in userenv
Bug 40943: Use session_id from userenv instead of cookie for value_builder
Bug 40943: (QA follow-up) Add a few POD lines for new sub
Bug 40943: Fix Serials.t
Bug 40943: Fix Script_cron.t
Bug 40943: Fix Koha/BackgroundJob.t
Bug 40943: Fix t/Koha/Script.t

Description Jonathan Druart 2025-10-03 11:26:52 UTC
Split from bug 37041
Comment 1 Jonathan Druart 2025-10-03 11:27:24 UTC
Created attachment 187379 [details] [review]
Bug 40943: Store session_id in userenv
Comment 2 Jonathan Druart 2025-10-03 11:27:26 UTC
Created attachment 187380 [details] [review]
Bug 40943: Use session_id from userenv instead of cookie for value_builder
Comment 3 Jonathan Druart 2025-10-03 11:30:08 UTC
As requested on bug 37041 the patches have been moved to a separate bug report. However they do fix the issue raised there.

Test plan:
Logout - http://localhost:8081/cgi-bin/koha/mainpage.pl?logout.x=1
Go to add biblio - http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=4
Login koha/koha
=> Without this patch you get a 403
=> With this patch applied you get the edit biblio form.
Comment 4 David Cook 2025-10-09 06:22:36 UTC
Ok, so I've taken a little look at this and I do think that this will solve the immediate problem.

I do think that Marcel has a good point about the usage of check_cookie_auth and CGI->new in those scripts, but I think they're separate things.

I'd be tempted to put in this solution, and then work on fixing the other issue separately.

(I suppose one thing we haven't considered is custom value_builder plugins that other people might've put together, but I think they'll need to pattern those on what we have in Koha going forward anyway...)
Comment 5 Marcel de Rooy 2025-10-10 07:30:15 UTC
We only had the fallback to empty string in check_cookie_auth:

-                    $session->param('number'),       $session->param('id') // '',

Just noting for the record.
Comment 6 Marcel de Rooy 2025-10-10 07:54:57 UTC
These three parameters always confuse:
$usernum,      $userid,     $usercnum,

usernum is actually the real id, i.e. borrowernumber
userid is a bad column name for username, but we recognize it normally
usercnum is cryptic, cardnumber would be clearer

They are converted to userenv keys: number, id and cardnumber. Where id is probably the most confusing one. Since we mean username.

Out of scope, but please keep thinking about it :)
Comment 7 Marcel de Rooy 2025-10-10 08:51:31 UTC
One potential/theoretical concern could be leaking userenv to a template, logfile, cache, etc where it somehow could be exploited to get the session id or leak it into the http body.

While scrolling thru userenv occurrences in the codebase, I dont see any reason for an actual concern about that.

Obviously, we should (somehow) not allow that in the future too ;)
Comment 8 Marcel de Rooy 2025-10-10 09:34:01 UTC
Created attachment 187720 [details] [review]
Bug 40943: Store session_id in userenv

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Inline tidy.
Comment 9 Marcel de Rooy 2025-10-10 09:34:03 UTC
Created attachment 187721 [details] [review]
Bug 40943: Use session_id from userenv instead of cookie for value_builder

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Inline tidy. Moved set_userenv_from_session before POD of next sub.
Comment 10 Marcel de Rooy 2025-10-10 09:34:05 UTC
Created attachment 187722 [details] [review]
Bug 40943: (QA follow-up) Add a few POD lines for new sub

This clears qa warnings.

Test plan:
Read the POD.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2025-10-10 09:35:22 UTC
Care to add a QA stamp to this patch, David or Martin?
Comment 12 Marcel de Rooy 2025-10-10 09:36:15 UTC
Have a simple idea to still improve the specific check in value_builder on top of this dev. Continuing there.
Comment 13 Marcel de Rooy 2025-10-10 09:40:45 UTC
*** Bug 37041 has been marked as a duplicate of this bug. ***
Comment 14 David Cook 2025-10-12 22:48:44 UTC
(In reply to Marcel de Rooy from comment #11)
> Care to add a QA stamp to this patch, David or Martin?

Adding to the todo list as well. I'll be back here shortly.
Comment 15 David Cook 2025-10-21 05:03:12 UTC
Created attachment 188188 [details] [review]
Bug 40943: Store session_id in userenv

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Inline tidy.
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 16 David Cook 2025-10-21 05:03:17 UTC
Created attachment 188189 [details] [review]
Bug 40943: Use session_id from userenv instead of cookie for value_builder

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Inline tidy. Moved set_userenv_from_session before POD of next sub.
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 17 David Cook 2025-10-21 05:03:24 UTC
Created attachment 188190 [details] [review]
Bug 40943: (QA follow-up) Add a few POD lines for new sub

This clears qa warnings.

Test plan:
Read the POD.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 18 Lucas Gass (lukeg) 2025-11-03 21:16:39 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 19 Lucas Gass (lukeg) 2025-11-04 00:04:38 UTC
t/db_dependent/Serials.t fails now because dateaccessioned is producing warnings:

#   Failed test 'no warnings'
#   at /usr/share/perl/5.36/Test/Builder.pm line 193.
# There were 4 warning(s)
#     Previous test 25 'Subscription has at least one serial'
#     ERROR: Plugin dateaccessioned.pl: Can't use an undefined value as a HASH reference at /kohadevbox/koha/cataloguing/value_builder/dateaccessioned.pl line 30.
#  at /kohadevbox/koha/C4/Items.pm line 1620.
Comment 20 Lucas Gass (lukeg) 2025-11-04 00:06:29 UTC
also I think for this bug:

/kohadevbox/koha/t/Koha/Script_cron.t .. 
1..4
ok 1 - use Koha::Script;
not ok 2 - Context userenv set correctly with -cron

#   Failed test 'Context userenv set correctly with -cron'
#   at /kohadevbox/koha/t/Koha/Script_cron.t line 28.
#     Structures begin differing at:
#          $got->{session_id} = undef
#     $expected->{session_id} = Does not exist
Comment 21 Lucas Gass (lukeg) 2025-11-04 00:08:01 UTC
/kohadevbox/koha/t/db_dependent/Koha/BackgroundJob.t ..

    not ok 10 - Context set from userenv + interface

    #   Failed test 'Context set from userenv + interface'
    #   at /kohadevbox/koha/t/db_dependent/Koha/BackgroundJob.t line 132.
    #     Structures begin differing at:
    #          $got->{session_id} = undef
    #     $expected->{session_id} = Does not exist
    # Looks like you failed 1 test of 10.
not ok 2 - enqueue() tests




    not ok 3 - Userenv set from job context on process

    #   Failed test 'Userenv set from job context on process'
    #   at /kohadevbox/koha/t/db_dependent/Koha/BackgroundJob.t line 240.
    #     Structures begin differing at:
    #          $got->{session_id} = undef
    #     $expected->{session_id} = Does not exist
    ok 4 - Interface set from job context on process
    ok 5
    # Looks like you failed 1 test of 5.
not ok 4 - process tests
Comment 22 Jonathan Druart 2025-11-04 08:26:39 UTC
Created attachment 189002 [details] [review]
Bug 40943: Fix Serials.t

We need to mock userenv
ERROR: Plugin dateaccessioned.pl: Can't use an undefined value as a HASH reference at /kohadevbox/koha/cataloguing/value_builder/dateaccessioned.pl line 30.
Comment 23 Jonathan Druart 2025-11-04 08:26:41 UTC
Created attachment 189003 [details] [review]
Bug 40943: Fix Script_cron.t

Failed with:
   at t/Koha/Script_cron.t line 28.
     Structures begin differing at:
          $got->{session_id} = undef
     $expected->{session_id} = Does not exist

session_id is now part of userenv
Comment 24 Jonathan Druart 2025-11-04 08:26:44 UTC
Created attachment 189004 [details] [review]
Bug 40943: Fix Koha/BackgroundJob.t

We should not log/leak session_id!

   Failed test 'Context set from userenv + interface'
   at t/db_dependent/Koha/BackgroundJob.t line 132.
     Structures begin differing at:
          $got->{session_id} = undef
     $expected->{session_id} = Does not exist
 Looks like you failed 1 test of 10.

   Failed test 'enqueue() tests'
   at t/db_dependent/Koha/BackgroundJob.t line 135.

   Failed test 'Userenv set from job context on process'
   at t/db_dependent/Koha/BackgroundJob.t line 240.
     Structures begin differing at:
          $got->{session_id} = undef
     $expected->{session_id} = Does not exist
 Looks like you failed 1 test of 5.
Comment 25 Jonathan Druart 2025-11-04 08:28:12 UTC
We were logging the full userenv for background jobs, I've deleted specifically 'session_id' from the userenv structure before we enqueue the job.

QA review welcome!
Comment 26 Tomás Cohen Arazi (tcohen) 2025-11-04 13:52:08 UTC
(In reply to Jonathan Druart from comment #25)
> We were logging the full userenv for background jobs, I've deleted
> specifically 'session_id' from the userenv structure before we enqueue the
> job.
> 
> QA review welcome!

Are you removing it because someone with background jobs rights could pick the session id and impersonate others through some cookie mangling?

If that's the case, the follow-up looks correct to me. Adding a test on `$job_context->{session_id}` not existing in the data structure might be worth. I guess it can be inferid from the context (the 'delete' line above).
Comment 27 Jonathan Druart 2025-11-04 14:10:33 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #26)
> (In reply to Jonathan Druart from comment #25)
> > We were logging the full userenv for background jobs, I've deleted
> > specifically 'session_id' from the userenv structure before we enqueue the
> > job.
> > 
> > QA review welcome!
> 
> Are you removing it because someone with background jobs rights could pick
> the session id and impersonate others through some cookie mangling?

Yes, exactly.

> If that's the case, the follow-up looks correct to me. Adding a test on
> `$job_context->{session_id}` not existing in the data structure might be
> worth. I guess it can be inferid from the context (the 'delete' line above).

It's the reason I have added the comment "# session_id must not be logged!" to make it explicit it should not be present.
Comment 28 Lucas Gass (lukeg) 2025-11-04 14:16:03 UTC
follow-ups pushed to main
Comment 29 Lucas Gass (lukeg) 2025-11-05 16:17:17 UTC
Still one more failure here:

kohadev-koha@kohadevbox:koha(main)$ prove -v /kohadevbox/koha/t/Koha/Script.t
/kohadevbox/koha/t/Koha/Script.t .. 
1..5
ok 1 - use Koha::Script;
not ok 2 - Context userenv set correctly with no flags

#   Failed test 'Context userenv set correctly with no flags'
#   at /kohadevbox/koha/t/Koha/Script.t line 31.
#     Structures begin differing at:
#          $got->{session_id} = undef
#     $expected->{session_id} = Does not exist
ok 3 - Context interface set correctly with no flags
# Subtest: lock_exec() tests
    1..3
    ok 1 - Exception found
Exception 'Koha::Exception' thrown 'Unable to acquire the lock /var/lock/koha/kohadev/sleep.pl: Resource temporarily unavailable'
    ok 2 - wait.pl successfully waits for the lock
    ok 3 - Not passing the "script" parameter makes it raise an exception
ok 4 - lock_exec() tests
ok 5 - no warnings
# Looks like you failed 1 test of 5.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/5 subtests
Comment 30 Jonathan Druart 2025-11-06 07:58:34 UTC
Created attachment 189145 [details] [review]
Bug 40943: Fix t/Koha/Script.t

Failed with:
   Failed test 'Context userenv set correctly with no flags'
   at t/Koha/Script.t line 31.
     Structures begin differing at:
          $got->{session_id} = undef
     $expected->{session_id} = Does not exist

session_id is now part of userenv
Comment 31 Lucas Gass (lukeg) 2025-11-07 14:44:43 UTC
follow-up pushed to main