We must have a test to catch intranet scripts that do not have a authentication statement, get_template_and_user, checkauth or check_cookie_auth
Created attachment 100885 [details] [review] Bug 24879: Add new test to catch missing auth statement in intranet scripts
On top of bug 24878 this test returns: # Failed test at xt/find-missing-auth_checks.t line 71. # got: '23' # expected: '0' # No auth check in the following files: # ./acqui/check_uniqueness.pl # ./catalogue/image.pl # ./cataloguing/plugin_launcher.pl # ./cataloguing/value_builder/stocknumber.pl # ./cataloguing/value_builder/barcode.pl # ./cataloguing/value_builder/marc21_orgcode.pl # ./cataloguing/value_builder/marc21_field_245h.pl # ./cataloguing/value_builder/dateaccessioned.pl # ./cataloguing/value_builder/marc21_field_005.pl # ./cataloguing/value_builder/upload.pl # ./cataloguing/value_builder/barcode_manual.pl # ./serials/lateissues-export.pl # ./installer/externalmodules.pl # ./installer/html-template-to-template-toolkit.pl # ./installer/data/mysql/fix_unclosed_nonaccruing_fines_bug17135.pl # ./installer/data/mysql/update22to30.pl # ./installer/data/mysql/labels_upgrade.pl # ./installer/data/mysql/patroncards_upgrade.pl # ./installer/data/mysql/updatedatabase.pl # ./installer/data/mysql/backfill_statistics.pl # ./labels/label-create-csv.pl # ./labels/label-create-xml.pl # ./members/default_messageprefs.pl # Looks like you failed 1 test of 1.
Created attachment 106978 [details] [review] Bug 24879: Add new test to catch missing auth statement in intranet scripts Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signing off.. this is a really important test to have. I am happy to start working through the failing scripts.. would you prefer that work added here or on it's won bug Jonathan?
Hmm.. perhaps we should add `C4::Service->init(` to your list.. it also serves to check authority.
(In reply to Martin Renvoize from comment #5) > Hmm.. perhaps we should add `C4::Service->init(` to your list.. it also > serves to check authority. The test only catch missing auth in .pl files, and C4::Service is used from the svc scripts that don't have extension.
Since Bug 16922 installer/data is not accessible anymore right ?
Comment on attachment 106978 [details] [review] Bug 24879: Add new test to catch missing auth statement Review of attachment 106978 [details] [review]: ----------------------------------------------------------------- ::: xt/find-missing-auth_checks.t @@ +50,5 @@ > + |svc > + |tags > + |tools > + |virtualshelves > + )}xms It's more robust to use a deny list rather than an allow list, as there would be fewer rules that would change less frequently. (For example, skipping directories like C4, Koha, koha-tmpl, api, bin, debian, docs, errors, etc, misc, skel, tmp, t, xt) Also, it would be easier to maintain a hash lookup rather than a large regular expression. File::Basename could be used to get the top level directory. @@ +51,5 @@ > + |tags > + |tools > + |virtualshelves > + )}xms > + && $name =~ m{\.(pl)$} It might be slower but it might be more robust to look for a Perl shebang.
Created attachment 163139 [details] [review] Bug 24879: Add new test to catch missing auth statement in intranet scripts Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 163140 [details] [review] Bug 24879: Remove installer/externalmodules.pl It is not used, if we need it back it must be moved to misc.
Created attachment 163141 [details] [review] Bug 24879: Adjust tests Installer scripts cannot be run from the UI: debian/templates/apache-shared-intranet.conf:RewriteRule ^/cgi-bin/koha/(C4|debian|etc|installer/data|install_misc|Koha|misc|selenium|t|test|tmp|xt)/|\.PL$ /notfound [PT]
Created attachment 163142 [details] [review] Bug 24879: Add check_cookie_auth when missing This can certainly be improved to adjust the permissions, but at least they are no longer opened to the world..
Created attachment 163143 [details] [review] Bug 24879: Use perl shebang to list the exec
Only 2 missings: # koha_perl_deps.pl # reports/itemtypes.plugin See bug 36323 for the first one. I am not sure what to do for the second one...
koha_perl_deps.pl is not copied from Makefile.PL so we can skip it.
reports/itemtypes.plugin is going to be removed on bug 31988
Created attachment 163183 [details] [review] Bug 24879: Exclude koha_perl_deps.pl And tidy.
Created attachment 163417 [details] [review] Bug 24879: Add new test to catch missing auth statement in intranet scripts Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 163418 [details] [review] Bug 24879: Remove installer/externalmodules.pl It is not used, if we need it back it must be moved to misc. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 163419 [details] [review] Bug 24879: Adjust tests Installer scripts cannot be run from the UI: debian/templates/apache-shared-intranet.conf:RewriteRule ^/cgi-bin/koha/(C4|debian|etc|installer/data|install_misc|Koha|misc|selenium|t|test|tmp|xt)/|\.PL$ /notfound [PT] Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 163420 [details] [review] Bug 24879: Add check_cookie_auth when missing This can certainly be improved to adjust the permissions, but at least they are no longer opened to the world.. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 163421 [details] [review] Bug 24879: Use perl shebang to list the exec Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 163422 [details] [review] Bug 24879: Exclude koha_perl_deps.pl And tidy. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Another clear fix here with clear and concise code.. there's some tidy issues highlighted by the QA script but I think they're incorrect (I couldn't tidy it to get rid of them). Passing QA straight again here.
Pushed to master-security. The tests are failing without bug 31988 - so I pushed that as well. It was already pushed to master earlier this week.
Was able to backport to 22.05.x-security for 22.05.20 Had a few conflicts like having to change $input to $data in catalogue/image.pl
2 tests are failing with: "Test ended with extra hubs on the stack!" I have tracked it down to "Bug 24879: Add check_cookie_auth when missing" and this *exact* line: cataloguing/value_builder/dateaccessioned.pl 28 my ($auth_status) = check_cookie_auth( $input->cookie('CGISESSID'), { catalogue => 1 } ); If you replaced it with my $auth_status = "ok"; The tests are passing.
(In reply to Katrin Fischer from comment #25) > Pushed to master-security. > > The tests are failing without bug 31988 - so I pushed that as well. It was > already pushed to master earlier this week. Please copy the failures in the comment next time, it will help when searching for them in bugzilla.
I try mocking in UT
Ah great we have an example in t/db_dependent/Search/History.t : https://git.koha-community.org/Koha-community/Koha/src/commit/87e53a335242f9b47183d8437885b22706bbde3c/t/db_dependent/Search/History.t#L427
Created attachment 163977 [details] [review] Bug 24879: (follow-up) Fix test suite Running cataloguing pluings (in cataloguing/value_builder) now requires authentification. This patch adds in failing unit tests a mock of C4::Auth::check_cookie_auth Test with: prove t/db_dependent/FrameworkPlugin.t t/db_dependent/Koha/UI/Form/Builder/Biblio.t t/db_dependent/Koha/UI/Form/Builder/Item.t t/db_dependent/Serials.t
I see it is pushed to master-security, great :D
Looks like this causes a bit of a regression: bug 37041
(In reply to David Cook from comment #33) > Looks like this causes a bit of a regression: bug 37041 Sure it does.
Reverting this bug for value_builder scripts would resolve the issues on 37041. Surely we should not be able to hit those value builder scripts from outside, so blocking them in Apache would be sufficient. Note that the whole design of these framework plugins dates from long ago and involves running files with perl do($file) in Koha/FrameworkPlugin. Note btw that this module did not introduce that pattern, but just moved it. Refactoring that would be nice but much more effort. The additional new CGI added in this patch would be fine for the auth_status check on a direct hit but creates new issues when logging in onto addbiblio or additem. The wrong session id is checked etc. Since simply blocking them makes the new cookie_auth checks unneeded, I would suggest to revert?
It looks like this touched quite a bit of files, if we agree, I am not against a revert, but since it's already been released I'd be a bit happier with a separate bug. Otherwise release notes will all have to be manually edited and might make things potentially more confusing. What do you think?
(In reply to Katrin Fischer from comment #36) > It looks like this touched quite a bit of files, if we agree, I am not > against a revert, but since it's already been released I'd be a bit happier > with a separate bug. Otherwise release notes will all have to be manually > edited and might make things potentially more confusing. What do you think? Yeah. Lets address the issue on 37041.
These scripts were adjusted: cataloguing/value_builder/barcode.pl | 10 ++++++++++ cataloguing/value_builder/barcode_manual.pl | 10 ++++++++++ cataloguing/value_builder/dateaccessioned.pl | 10 ++++++++++ cataloguing/value_builder/marc21_field_005.pl | 10 ++++++++++ cataloguing/value_builder/marc21_field_245h.pl | 12 +++++++++++- cataloguing/value_builder/marc21_field_260b.pl | 10 ++++++++++ cataloguing/value_builder/marc21_orgcode.pl | 9 +++++++++ cataloguing/value_builder/stocknumber.pl | 9 +++++++++ cataloguing/value_builder/upload.pl | 10 ++++++++++ But note that the remaining perl scripts here should also be adjusted actually. Just checking if get_template_and_user (a.o.) is present in the launcher sub is not sufficient ! The check $line =~ m|^[^#]*$routine| would already approve a lingering (but unused) statement like "use C4::Auth qw(get_template_and_user);".