From fe1e7aac96f7f32701887554625acfd9c6b3cb26 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 13 Apr 2022 13:55:04 +0100 Subject: [PATCH] Bug 30524: [21.11] 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 --- 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 + xt/find-missing-csrf.t | 82 +++++++++++++++++++ 6 files changed, 152 insertions(+), 8 deletions(-) create mode 100644 koha-tmpl/intranet-tmpl/prog/en/includes/csrf-token.inc create mode 100755 xt/find-missing-csrf.t diff --git a/C4/Auth.pm b/C4/Auth.pm index 13128407f58..26e994b66e8 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -49,6 +49,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; @@ -299,6 +300,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 42c4011ca03..94bc8e0f958 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 00000000000..703d4eb036a --- /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 02218deaddd..55ca2dc4367 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 826aef82a62..0663cdc60a6 100755 --- a/t/db_dependent/Auth.t +++ b/t/db_dependent/Auth.t @@ -388,6 +388,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"}; }; diff --git a/xt/find-missing-csrf.t b/xt/find-missing-csrf.t new file mode 100755 index 00000000000..15dd6ae6265 --- /dev/null +++ b/xt/find-missing-csrf.t @@ -0,0 +1,82 @@ +#!/usr/bin/perl + +# Copyright 2021 Koha development team +# +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; +use Test::More tests => 1; +use File::Find; +use File::Slurp; +use Data::Dumper; + +my @themes; + +# OPAC themes +my $opac_dir = 'koha-tmpl/opac-tmpl'; +opendir ( my $dh, $opac_dir ) or die "can't opendir $opac_dir: $!"; +for my $theme ( grep { not /^\.|lib|js|xslt/ } readdir($dh) ) { + push @themes, "$opac_dir/$theme/en"; +} +close $dh; + +# STAFF themes +my $staff_dir = 'koha-tmpl/intranet-tmpl'; +opendir ( $dh, $staff_dir ) or die "can't opendir $staff_dir: $!"; +for my $theme ( grep { not /^\.|lib|js/ } readdir($dh) ) { + push @themes, "$staff_dir/$theme/en"; +} +close $dh; + +my @files; +sub wanted { + my $name = $File::Find::name; + push @files, $name + if $name =~ m[\.(tt|inc)$] and -f $name; +} + +find({ wanted => \&wanted, no_chdir => 1 }, @themes ); + +my @errors; +for my $file ( @files ) { + my @e = check_csrf_in_forms($file); + push @errors, { file => $file, errors => \@e } if @e; +} + +is( @errors, 0, "Template variables should be correctly escaped" ) + or diag(Dumper @errors); + +sub check_csrf_in_forms { + my ( $file ) = @_; + + my @lines = read_file($file); + my @errors; + return @errors unless grep { $_ =~ m| starting on line $open is missing it's corresponding csrf_token include (see bug 22990)" + if !$found; + $found = 0; + } + } + return @errors; +} -- 2.25.1