Split from bug 37041
Created attachment 187379 [details] [review] Bug 40943: Store session_id in userenv
Created attachment 187380 [details] [review] Bug 40943: Use session_id from userenv instead of cookie for value_builder
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.
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...)
We only had the fallback to empty string in check_cookie_auth: - $session->param('number'), $session->param('id') // '', Just noting for the record.
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 :)
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 ;)
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.
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.
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>
Care to add a QA stamp to this patch, David or Martin?
Have a simple idea to still improve the specific check in value_builder on top of this dev. Continuing there.
*** Bug 37041 has been marked as a duplicate of this bug. ***
(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.
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>
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>
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>
Nice work everyone! Pushed to main for 25.11
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.
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
/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
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.
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
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.
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!
(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).
(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.
follow-ups pushed to main
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
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
follow-up pushed to main