Bug 24879 - Add missing authentication checks
Summary: Add missing authentication checks
Status: Pushed to oldoldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 31988 36322
Blocks: 36511
  Show dependency treegraph
 
Reported: 2020-03-16 15:32 UTC by Jonathan Druart
Modified: 2024-04-09 17:02 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.04,23.05.10,22.11.16,22.05.20


Attachments
Bug 24879: Add new test to catch missing auth statement (2.60 KB, patch)
2020-03-17 10:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24879: Add new test to catch missing auth statement (2.66 KB, patch)
2020-07-17 07:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24879: Add new test to catch missing auth statement (2.66 KB, patch)
2024-03-14 15:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24879: Remove installer/externalmodules.pl (1.71 KB, patch)
2024-03-14 15:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24879: Adjust tests (1.33 KB, patch)
2024-03-14 15:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24879: Add check_cookie_auth when missing (10.93 KB, patch)
2024-03-14 15:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24879: Use perl shebang to list the exec (2.48 KB, patch)
2024-03-14 15:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24879: Exclude koha_perl_deps.pl (1.90 KB, patch)
2024-03-15 09:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24879: Add new test to catch missing auth statement (2.66 KB, patch)
2024-03-19 11:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24879: Remove installer/externalmodules.pl (1.77 KB, patch)
2024-03-19 11:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24879: Adjust tests (1.40 KB, patch)
2024-03-19 11:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24879: Add check_cookie_auth when missing (11.00 KB, patch)
2024-03-19 11:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24879: Use perl shebang to list the exec (2.54 KB, patch)
2024-03-19 11:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24879: Exclude koha_perl_deps.pl (1.96 KB, patch)
2024-03-19 11:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24879: (follow-up) Fix test suite (3.92 KB, patch)
2024-03-27 09:25 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2020-03-16 15:32:17 UTC
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
Comment 1 Jonathan Druart 2020-03-17 10:55:45 UTC
Created attachment 100885 [details] [review]
Bug 24879: Add new test to catch missing auth statement

in intranet scripts
Comment 2 Jonathan Druart 2020-03-17 10:56:32 UTC
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.
Comment 3 Martin Renvoize 2020-07-17 07:23:43 UTC
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>
Comment 4 Martin Renvoize 2020-07-17 07:25:18 UTC
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?
Comment 5 Martin Renvoize 2020-07-17 07:28:51 UTC
Hmm.. perhaps we should add `C4::Service->init(` to your list.. it also serves to check authority.
Comment 6 Jonathan Druart 2020-07-27 10:47:56 UTC
(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.
Comment 7 Fridolin Somers 2020-10-21 13:48:23 UTC
Since Bug 16922 installer/data is not accessible anymore right ?
Comment 8 David Cook 2020-10-21 23:07:42 UTC
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.
Comment 9 Jonathan Druart 2024-03-14 15:53:55 UTC
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>
Comment 10 Jonathan Druart 2024-03-14 15:53:57 UTC
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.
Comment 11 Jonathan Druart 2024-03-14 15:54:00 UTC
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]
Comment 12 Jonathan Druart 2024-03-14 15:54:03 UTC
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..
Comment 13 Jonathan Druart 2024-03-14 15:54:06 UTC
Created attachment 163143 [details] [review]
Bug 24879: Use perl shebang to list the exec
Comment 14 Jonathan Druart 2024-03-14 15:54:43 UTC
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...
Comment 15 Jonathan Druart 2024-03-15 07:39:44 UTC
koha_perl_deps.pl is not copied from Makefile.PL so we can skip it.
Comment 16 Jonathan Druart 2024-03-15 09:18:18 UTC
reports/itemtypes.plugin is going to be removed on bug 31988
Comment 17 Jonathan Druart 2024-03-15 09:19:41 UTC
Created attachment 163183 [details] [review]
Bug 24879: Exclude koha_perl_deps.pl

And tidy.
Comment 18 Martin Renvoize 2024-03-19 11:49:35 UTC
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>
Comment 19 Martin Renvoize 2024-03-19 11:49:38 UTC
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>
Comment 20 Martin Renvoize 2024-03-19 11:49:41 UTC
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>
Comment 21 Martin Renvoize 2024-03-19 11:49:43 UTC
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>
Comment 22 Martin Renvoize 2024-03-19 11:49:46 UTC
Created attachment 163421 [details] [review]
Bug 24879: Use perl shebang to list the exec

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2024-03-19 11:49:49 UTC
Created attachment 163422 [details] [review]
Bug 24879: Exclude koha_perl_deps.pl

And tidy.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2024-03-19 11:51:13 UTC
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.
Comment 25 Katrin Fischer 2024-03-20 16:44:55 UTC
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.
Comment 26 wainuiwitikapark 2024-03-22 01:08:08 UTC
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
Comment 27 Jonathan Druart 2024-03-27 08:24:20 UTC
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.
Comment 28 Jonathan Druart 2024-03-27 08:25:03 UTC
(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.
Comment 29 Fridolin Somers 2024-03-27 08:46:12 UTC
I try mocking in UT
Comment 30 Fridolin Somers 2024-03-27 08:53:56 UTC
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
Comment 31 Fridolin Somers 2024-03-27 09:25:22 UTC
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
Comment 32 Fridolin Somers 2024-03-27 13:11:50 UTC
I see it is pushed to master-security, great :D