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