https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 --- Comment #2 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 187380 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=187380&action=edit Bug 40943: Use session_id from userenv instead of cookie for value_builder -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au, | |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- We only had the fallback to empty string in check_cookie_auth: - $session->param('number'), $session->param('id') // '', Just noting for the record. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 --- Comment #7 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 ;) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #187379|0 |1 is obsolete| | --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 187720 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=187720&action=edit Bug 40943: Store session_id in userenv Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Inline tidy. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 187722 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=187722&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@openfifth.c | |o.uk --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Care to add a QA stamp to this patch, David or Martin? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- *** Bug 37041 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 --- Comment #14 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |dcook@prosentient.com.au |y.org | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #187720|0 |1 is obsolete| | --- Comment #15 from David Cook <dcook@prosentient.com.au> --- Created attachment 188188 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188188&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #187722|0 |1 is obsolete| | --- Comment #17 from David Cook <dcook@prosentient.com.au> --- Created attachment 188190 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188190&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 --- Comment #18 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to main for 25.11 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 --- Comment #20 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 --- Comment #22 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 189002 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189002&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 --- Comment #24 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 189004 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189004&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #26 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (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). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed --- Comment #29 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | --- Comment #31 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- follow-up pushed to main -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 Andrii Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nugged@gmail.com --- Comment #32 from Andrii Nugged <nugged@gmail.com> --- Koha::BackgroundJob::enqueue mutates live C4::Context->userenv when sanitizing job context - - - Houston, we have a problem. In Koha::BackgroundJob::enqueue we currently do: my $job_context = $params->{job_context} // C4::Context->userenv; This does not create a copy. It keeps an alias to the live request C4::Context->userenv hashref when no explicit job_context is passed. A few lines later we sanitize the job context with: # session_id must not be logged delete $job_context->{session_id}; Because $job_context may alias C4::Context->userenv, this also removes session_id from the live request context for the rest of the same Plack request. I traced this while debugging a 403 during item save from: POST .../cgi-bin/koha/cataloguing/additem.pl?biblionumber=... The visible failure happened in a value_builder plugin that does: my ($auth_status) = check_cookie_auth( C4::Context->userenv->{session_id}, { catalogue => 1 } ); After BackgroundJob::enqueue has deleted session_id from the live userenv, that auth call fails. In my case dateaccessioned.pl then printed a 403 header, and the request ended up as: PAGE GENERATED 403 So the plugin is not the root cause here. The root cause is that enqueue mutates the live request userenv while preparing background job context. A safer fix would be to decouple the job context from the live request context first, for example: my $job_context = { %{ $params->{job_context} // C4::Context->userenv // {} } }; Then: delete $job_context->{session_id}; $job_context->{interface} = C4::Context->interface; For a temporary local workaround I used: delete local $job_context->{session_id}; local $job_context->{interface} = C4::Context->interface; but I think copying the hashref is the better upstream fix because it avoids mutating the live request context at all. I only found one related production occurrence: Koha/BackgroundJob.pm delete $job_context->{session_id}; and one test occurrence: t/db_dependent/Koha/BackgroundJob.t delete $expected_context->{session_id}; It would be good to add a regression test to ensure enqueue does not mutate the live userenv, something along the lines of: my $original_session_id = C4::Context->userenv->{session_id}; ... is( C4::Context->userenv->{session_id}, $original_session_id, 'enqueue must not mutate live userenv session_id' ); -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 --- Comment #34 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #33)

Nice one, Andrii.

Yeah, I think dereferencing the hashref is probably the way to go.

Might not hurt to double-check that there's no references within the hashref that would need to be cloned/dereferenced too... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42394 --- Comment #36 from Jonathan Druart <jonathan.druart@gmail.com> --- Opened bug 42394. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42394 [Bug 42394] Session_id lost when a job is enqueued -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40943 Laura Escamilla <Laura.escamilla@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Needs documenting CC| |Laura.escamilla@bywatersolu | |tions.com --- Comment #37 from Laura Escamilla <Laura.escamilla@bywatersolutions.com> --- Not backported to 25.05.x as this is an enhancement but I’m happy to revisit if needed. -- You are receiving this mail because: You are watching all bug changes.