Summary: | Store session_id in userenv | ||
---|---|---|---|
Product: | Koha | Reporter: | Jonathan Druart <jonathan.druart> |
Component: | Authentication | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | Signed Off --- | QA Contact: | Testopia <testopia> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | dcook, dpavlin, m.de.rooy, martin.renvoize |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37041 | ||
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: | ||
Bug Depends on: | |||
Bug Blocks: | 40993 | ||
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 |
Description
Jonathan Druart
2025-10-03 11:26:52 UTC
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. *** |