From 84450df2912bdd6ff4305bcb58db7136fad3b6e0 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 13 Apr 2022 13:55:04 +0100 Subject: [PATCH] Bug 30524: Core CSRF checking code Split out from bug 22990 as requested. Signed-off-by: David Cook Signed-off-by: Kyle M Hall Signed-off-by: Marcel de Rooy Bug 30524: Add xt/find-missing-csrf.t Signed-off-by: David Cook Signed-off-by: Kyle M Hall Signed-off-by: Marcel de Rooy Bug 30524: (QA follow-up) Polishing xt script Test plan: Run it again. Same results? Signed-off-by: Marcel de Rooy Bug 30524: Unit tests Test plan: Run t/Output.t Run t/db_dependent/Auth.t Signed-off-by: Marcel de Rooy Signed-off-by: Tomas Cohen Arazi --- C4/Auth.pm | 2 + C4/Output.pm | 33 ++++++++++++--- .../prog/en/includes/csrf-token.inc | 1 + t/Output.t | 41 ++++++++++++++++++- t/db_dependent/Auth.t | 1 + 5 files changed, 70 insertions(+), 8 deletions(-) create mode 100644 koha-tmpl/intranet-tmpl/prog/en/includes/csrf-token.inc diff --git a/C4/Auth.pm b/C4/Auth.pm index 43aef8b178..c00c610526 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -50,6 +50,7 @@ use C4::Auth_with_shibboleth qw( shib_ok get_login_shib login_shib_url logout_sh use Net::CIDR; use C4::Log qw( logaction ); use Koha::CookieManager; +use Koha::Token; # use utf8; @@ -301,6 +302,7 @@ sub get_template_and_user { $template->param( loggedinusernumber => $borrowernumber ); # FIXME Should be replaced with logged_in_user.borrowernumber $template->param( logged_in_user => $patron ); $template->param( sessionID => $sessionID ); + $template->param( csrf_token => Koha::Token->new->generate_csrf({ session_id => scalar $sessionID })); if ( $in->{'type'} eq 'opac' ) { require Koha::Virtualshelves; diff --git a/C4/Output.pm b/C4/Output.pm index 42c4011ca0..94bc8e0f95 100644 --- a/C4/Output.pm +++ b/C4/Output.pm @@ -34,6 +34,7 @@ use URI::Escape; use C4::Auth qw( get_template_and_user ); use C4::Context; use C4::Templates; +use Koha::Token; our (@ISA, @EXPORT_OK); @@ -314,9 +315,17 @@ sub is_ajax { To executed at the beginning of scripts to stop the script at this point if some errors are found. -Tests for module 'members': -* patron is not defined (we are looking for a patron that does no longer exist/never existed) -* The logged in user cannot see patron's infos (feature 'cannot_see_patron_infos') +A series of tests can be run for a given module, or a specific check. +Params "module" and "check" are mutually exclusive. + +Tests for modules: +* members: + - Patron is not defined (we are looking for a patron that does no longer exist/never existed) + - The logged in user cannot see patron's infos (feature 'cannot_see_patron_infos') + +Tests for specific check: +* csrf_token + will test if the csrf_token CGI param is valid Others will be added here depending on the needs (for instance biblio does not exist will be useful). @@ -332,16 +341,28 @@ sub output_and_exit_if_error { if ( not $current_patron ) { $error = 'unknown_patron'; } - elsif( not $logged_in_user->can_see_patron_infos( $current_patron ) ) { + elsif ( not $logged_in_user->can_see_patron_infos($current_patron) ) + { $error = 'cannot_see_patron_infos'; } - } elsif ( $params->{module} eq 'cataloguing' ) { + } + elsif ( $params->{module} eq 'cataloguing' ) { # We are testing the record to avoid additem to fetch the Koha::Biblio # But in the long term we will want to get a biblio in parameter $error = 'unknown_biblio' unless $params->{record}; } } - + elsif ( $params and exists $params->{check} ) { + if ( $params->{check} eq 'csrf_token' ) { + $error = 'wrong_csrf_token' + unless Koha::Token->new->check_csrf( + { + session_id => scalar $query->cookie('CGISESSID'), + token => scalar $query->param('csrf_token'), + } + ); + } + } output_and_exit( $query, $cookie, $template, $error ) if $error; return; } diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/csrf-token.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/csrf-token.inc new file mode 100644 index 0000000000..703d4eb036 --- /dev/null +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/csrf-token.inc @@ -0,0 +1 @@ + diff --git a/t/Output.t b/t/Output.t index 02218deadd..55ca2dc436 100755 --- a/t/Output.t +++ b/t/Output.t @@ -17,16 +17,23 @@ use Modern::Perl; -use Test::More tests => 7; +use Test::More tests => 8; use Test::Warn; +use Test::MockModule; + +use File::Temp qw/tempfile/; use CGI qw ( -utf8 ); +use C4::Auth qw( get_template_and_user ); + use t::lib::Mocks; BEGIN { - use_ok('C4::Output', qw( output_html_with_http_headers parametrized_url )); + use_ok('C4::Output', qw( output_html_with_http_headers output_and_exit_if_error parametrized_url )); } +our $output_module = Test::MockModule->new('C4::Output'); + my $query = CGI->new(); my $cookie; my $output = 'foobarbaz'; @@ -93,3 +100,33 @@ subtest 'output_with_http_headers() tests' => sub { like($stdout, qr/Access-control-allow-origin: https:\/\/koha-community\.org/, 'Header set to https://koha-community.org'); close STDOUT; }; + +subtest 'output_and_exit_if_error() tests' => sub { + plan tests => 1; + + $output_module->mock( + 'output_and_exit', + sub { + my ( $query, $cookie, $template, $error ) = @_; + is( $error, 'wrong_csrf_token', 'Got right error message' ); + } + ); + + t::lib::Mocks::mock_config( 'pluginsdir', [ C4::Context::temporary_directory ] ); + my ( $fh, $fn ) = tempfile( SUFFIX => '.tt', UNLINK => 1, DIR => C4::Context::temporary_directory ); + print $fh qq|[% blocking_error %]|; + close $fh; + + my $query = CGI->new(); + $query->param('csrf_token',''); + my ( $template, $loggedinuser, $cookies ) = get_template_and_user( + { + template_name => $fn, + query => $query, + type => "intranet", + authnotrequired => 1, + } + ); + # Next call triggers test in the mocked sub + output_and_exit_if_error($query, $cookie, $template, { check => 'csrf_token' }); +}; diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t index d0112642c5..52c9b5f5e3 100755 --- a/t/db_dependent/Auth.t +++ b/t/db_dependent/Auth.t @@ -545,6 +545,7 @@ subtest 'get_template_and_user' => sub { # Tests for the language URL paramete ); is($template->{VARS}->{'opac_name'}, "multibranch-19", "Opac name was set correctly"); is($template->{VARS}->{'opac_search_limit'}, "branch:multibranch-19", "Search limit was set correctly"); + ok(defined($template->{VARS}->{'csrf_token'}), "CSRF token returned"); delete $ENV{"HTTP_COOKIE"}; }; -- 2.39.5