Bug 40943 - Store session_id in userenv
Summary: Store session_id in userenv
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
: 37041 (view as bug list)
Depends on:
Blocks: 40993
  Show dependency treegraph
 
Reported: 2025-10-03 11:26 UTC by Jonathan Druart
Modified: 2025-10-10 09:43 UTC (History)
4 users (show)

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


Attachments
Bug 40943: Store session_id in userenv (3.75 KB, patch)
2025-10-03 11:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 40943: Use session_id from userenv instead of cookie for value_builder (8.59 KB, patch)
2025-10-03 11:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 40943: Store session_id in userenv (3.60 KB, patch)
2025-10-10 09:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40943: Use session_id from userenv instead of cookie for value_builder (9.06 KB, patch)
2025-10-10 09:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40943: (QA follow-up) Add a few POD lines for new sub (982 bytes, patch)
2025-10-10 09:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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. ***