As Michal Kula noted on Mattermost, trying to log into http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=9908 in main will yield a bizarre HTTP response that includes headers for a 403 response and a 200 response... This is partially due to bug 37040, but it's mostly due to a bug in addbiblio.pl where the form builder is generating a 403 that gets printed before the 200 response somehow...
If I capture STDOUT around a subsection of build_tabs(), I get 10 of these messages in STDOUT: [2024/06/06 01:12:47] [WARN] Status: 403 Forbidden Content-Type: text/plain; charset=ISO-8859-1 Status: 403 Forbidden Content-Type: text/plain; charset=ISO-8859-1 -- I suspect is has something to do with these warnings: [2024/06/06 00:08:33] [WARN] ERROR: Plugin marc21_orgcode.pl: ARRAY(0x561ffb8bfa98) at /kohadevbox/koha/Koha/UI/Form/Builder/Biblio.pm line 222. [2024/06/06 00:08:33] [WARN] ERROR: Plugin marc21_field_005.pl: ARRAY(0x561ffb8cdb60) at /kohadevbox/koha/Koha/UI/Form/Builder/Biblio.pm line 222.
Looks like a regression caused by bug 24879 Note that I'm not planning on working on this one.
(In reply to David Cook from comment #1) > I suspect is has something to do with these warnings: > > [2024/06/06 00:08:33] [WARN] ERROR: Plugin marc21_orgcode.pl: > ARRAY(0x561ffb8bfa98) at /kohadevbox/koha/Koha/UI/Form/Builder/Biblio.pm > line 222. > [2024/06/06 00:08:33] [WARN] ERROR: Plugin marc21_field_005.pl: > ARRAY(0x561ffb8cdb60) at /kohadevbox/koha/Koha/UI/Form/Builder/Biblio.pm > line 222. Seeing those too. Will have a look.
The arrays mentioned just contain: $VAR2 = [ 'EXIT ', 0 ]; This is just caused by the new CGI on bug 24879. Easier to replace that by blocking direct access to valuebuilder scripts. See my comment there.
(In reply to Marcel de Rooy from comment #4) > The arrays mentioned just contain: > $VAR2 = [ > 'EXIT > ', > 0 > ]; > > This is just caused by the new CGI on bug 24879. Easier to replace that by > blocking direct access to valuebuilder scripts. See my comment there. To clarify: The new CGI makes that the wrong session ID is checked and that the builder scripts bail out with 403.
For my understanding of the urgency of this: if you log in from another page or the normal "main" login page this problem doesn't occur?
(In reply to Katrin Fischer from comment #6) > For my understanding of the urgency of this: if you log in from another page > or the normal "main" login page this problem doesn't occur? Yeah thats true. So the impact is small. But our catalogers are constantly on those pages and when their session times out, they come across it.
I think in this case we might keep this one open for the next regular maintenance release - still high priority to fix, but I don't want to interrupt release process.
There is something fundamentally wrong in the way we handle the session id. Which leads to this kind of problems. Should we consider setting the session id in the L1 (actually in the userenv) so that we could have access to the current session id. So if a new one has been generated and is different than the one in the cookie of the request we can access it easily? We would have then: check_cookie_auth( C4::context->{userenv}->{session_id}, $required_flags ); and not relying on CGI. Another way would be to trick CGI and replace HTTP_COOKIE env var. But this seems very hacky and certainly not something recommended.
(In reply to Jonathan Druart from comment #9) > There is something fundamentally wrong in the way we handle the session id. > Which leads to this kind of problems. > > Should we consider setting the session id in the L1 (actually in the > userenv) so that we could have access to the current session id. So if a new > one has been generated and is different than the one in the cookie of the > request we can access it easily? > > We would have then: > check_cookie_auth( C4::context->{userenv}->{session_id}, $required_flags ); > and not relying on CGI. > > Another way would be to trick CGI and replace HTTP_COOKIE env var. But this > seems very hacky and certainly not something recommended. Looks easier to remove the newly added CGI in the value_builder scripts and block access in another way (RewriteRule or just move the files?). The original design should not have used scripts here .. The tric with passing another session id feels hacky too?
(In reply to Marcel de Rooy from comment #10) > (In reply to Jonathan Druart from comment #9) > > There is something fundamentally wrong in the way we handle the session id. > > Which leads to this kind of problems. > > > > Should we consider setting the session id in the L1 (actually in the > > userenv) so that we could have access to the current session id. So if a new > > one has been generated and is different than the one in the cookie of the > > request we can access it easily? > > > > We would have then: > > check_cookie_auth( C4::context->{userenv}->{session_id}, $required_flags ); > > and not relying on CGI. > > > > Another way would be to trick CGI and replace HTTP_COOKIE env var. But this > > seems very hacky and certainly not something recommended. > > Looks easier to remove the newly added CGI in the value_builder scripts and > block access in another way (RewriteRule or just move the files?). Easier maybe but then we have something specific in the apache config we could avoid. I am asking if there is something "better", not "easier" ;) > The original design should not have used scripts here .. Yes, but too late. > The tric with passing another session id feels hacky too? It is not another session id, it is actually the *current* session id!
(In reply to Jonathan Druart from comment #9) > Should we consider setting the session id in the L1 (actually in the > userenv) so that we could have access to the current session id. So if a new > one has been generated and is different than the one in the cookie of the > request we can access it easily? I'm not aware of the exact code problem, but this sounds all right to me. If it were a MVC like Catalyst/Mojo, we'd have something like $c->user or $c->session, so C4::Context->userenv is probably our next best thing (for now)? One day we'll move away from CGI scripts (even ones wrapped with Plack::App::CGIBin)...
Created attachment 171931 [details] [review] Bug 37041: Store session_id in userenv
Created attachment 171932 [details] [review] Bug 37041: Use session_id from userenv instead of cookie for value_builder
Only to share what I had in mind. It fixes the problem it seems, but can eventually introduce new ones... Now the session's id is stored in userenv, so we don't want to leak it! It seems safe however.
(In reply to Jonathan Druart from comment #15) > Only to share what I had in mind. It fixes the problem it seems, but can > eventually introduce new ones... > > Now the session's id is stored in userenv, so we don't want to leak it! It > seems safe however. Yeah, interesting. But in terms of security perhaps not the way we want to go..
Will try to submit another proposal.
Created attachment 172685 [details] [review] Bug 37041: Add C4::Auth::check_value_builder_caller Test plan: Run t/db_dependent/FrameworkPlugin.t Run xt/find-missing-auth_checks.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 172686 [details] [review] Bug 37041: Use check_value_builder_caller in value builder plugins Test plan: Verify if item editor and tools/batchmod work as expected. Directly hit a value builder plugin like: https://YOUR_STAFF_SERVER/cgi-bin/koha/cataloguing/value_builder/barcode.pl Verify that you get a 403 page regardless of being logged in or not. Run t/db_dependent/FrameworkPlugin.t Run xt/find-missing-auth_checks.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Counter proposal. Not finished yet, but to give an idea. Check caller stack. Optional exit via new Auth subroutine. Remove check_cookie_auth from value builder plugins. Adjust missing checks script.
Created attachment 172694 [details] [review] Bug 37041: Add C4::Auth::check_value_builder_caller Test plan: Run t/db_dependent/FrameworkPlugin.t Run xt/find-missing-auth_checks.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 172695 [details] [review] Bug 37041: Use check_value_builder_caller in two value builder plugins Test plan: Verify if item editor and tools/batchmod work as expected. Directly hit a value builder plugin like: https://YOUR_STAFF_SERVER/cgi-bin/koha/cataloguing/value_builder/barcode.pl Verify that you get a 403 page regardless of being logged in or not. Run t/db_dependent/FrameworkPlugin.t Run xt/find-missing-auth_checks.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 172696 [details] [review] Bug 37041: Adjust remaining MARC21 value builder plugins Test plan: Edit a biblio record and verify active plugins work as expected. (See first test plan) Test hitting some plugins directly. Run t/db_dependent/FrameworkPlugin.t Run xt/find-missing-auth_checks.t
Still needs attention at UNIMARC side
(In reply to Marcel de Rooy from comment #16) > (In reply to Jonathan Druart from comment #15) > > Only to share what I had in mind. It fixes the problem it seems, but can > > eventually introduce new ones... > > > > Now the session's id is stored in userenv, so we don't want to leak it! It > > seems safe however. > > Yeah, interesting. But in terms of security perhaps not the way we want to > go.. In term of cleaning our session handling code however we need that, and it could be a good excuse to introduce it. If we want it, then do it now. Otherwise we can go with your alternative approach.
(In reply to Jonathan Druart from comment #25) > (In reply to Marcel de Rooy from comment #16) > > (In reply to Jonathan Druart from comment #15) > > > Only to share what I had in mind. It fixes the problem it seems, but can > > > eventually introduce new ones... > > > > > > Now the session's id is stored in userenv, so we don't want to leak it! It > > > seems safe however. > > > > Yeah, interesting. But in terms of security perhaps not the way we want to > > go.. > > In term of cleaning our session handling code however we need that, and it > could be a good excuse to introduce it. > > If we want it, then do it now. Otherwise we can go with your alternative > approach. Lets separate both developments. I think that the cookie_auth_check should not be in the value builder plugins regardless of the session id being in the context or not. Suppose that the cookie check would pass via context [user logged in with perms], than we still have an unwanted script call resulting in a 500 error. (The original design should have prevented calling those scripts..) So the session_id patches do not resolve the problem of this report. As said before, we could also rely on Apache/nginx etc to restrict access to that folder. But the caller logic is a trivial fix in one place, not affecting the installation itself (apache file etc). So easier to backport? Could we move your patches to a new specific report? What do you think?
(In reply to Marcel de Rooy from comment #26) > (In reply to Jonathan Druart from comment #25) > > (In reply to Marcel de Rooy from comment #16) > > > (In reply to Jonathan Druart from comment #15) > > > > Only to share what I had in mind. It fixes the problem it seems, but can > > > > eventually introduce new ones... > > > > > > > > Now the session's id is stored in userenv, so we don't want to leak it! It > > > > seems safe however. > > > > > > Yeah, interesting. But in terms of security perhaps not the way we want to > > > go.. > > > > In term of cleaning our session handling code however we need that, and it > > could be a good excuse to introduce it. > > > > If we want it, then do it now. Otherwise we can go with your alternative > > approach. > > Lets separate both developments. I think that the cookie_auth_check should > not be in the value builder plugins regardless of the session id being in > the context or not. Suppose that the cookie check would pass via context > [user logged in with perms], than we still have an unwanted script call > resulting in a 500 error. (The original design should have prevented calling > those scripts..) So the session_id patches do not resolve the problem of > this report. How that? It worked when I've written it. > As said before, we could also rely on Apache/nginx etc to restrict access to > that folder. But the caller logic is a trivial fix in one place, not > affecting the installation itself (apache file etc). So easier to backport? Easier but I don't think it is really nice. > Could we move your patches to a new specific report? What do you think? That will be yet another thing that will get lost. I am willing to abandon it. Obsoleting the patches now.
(In reply to Jonathan Druart from comment #27) > How that? It worked when I've written it. Please explain. Those scripts return a hash with builder and launcher. No HTTP for a browser. What probably worked, was calling without being logged in. > Easier but I don't think it is really nice. Agreed > That will be yet another thing that will get lost. I am willing to abandon > it. > Obsoleting the patches now. Not my intention. But lacking the time now to follow-up on them too. There was a reason to implement this approach?
With the session's id available in userenv you don't have to rely on CGI. And with CGI you get the session's id from the cookie which may be obsolete (example here if you are in the middle/after of a login operation). I am not sure what you want me to explain as I am sure you understood all that already. I just thought it was a good opportunity to clean up this area, as the way we handle the session's id lead us to problem like this one. We should certainly try to reach this workflow: - Auth from credential or CGISESSID cookie - Set userenv - Rely on userenv to know if the user is currently authenticated - Add CGISESSID to the response And the value_builder scripts here would return 403 unless C4::Auth::is_authenticated({catalogue => 1}) is_auth would just need to get the patron's id from userenv, if exists it means that the user is authenticated already.
(In reply to Jonathan Druart from comment #29) > is_auth would just need to get the patron's id from userenv, if exists it > means that the user is authenticated already. Wait, isn't that true already actually? Cannot we simply replace the check_cookie_auth call with C4::Auth::haspermission(C4::Context->userenv->{id}, {catalogue => 1}) here?
(In reply to Jonathan Druart from comment #30) > (In reply to Jonathan Druart from comment #29) > > is_auth would just need to get the patron's id from userenv, if exists it > > means that the user is authenticated already. > > Wait, isn't that true already actually? > > Cannot we simply replace the check_cookie_auth call with > C4::Auth::haspermission(C4::Context->userenv->{id}, {catalogue => 1}) > here? No, if this is a direct call to a value builder, the answer should be 403 (or 400) in any case. Not depending on login or permissions. This is what check_value_builder_caller implements too.