From e80f08ff393aab5daf10c33e4b3ddbe57993b7ed 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 + xt/find-missing-csrf.t | 82 +++++++++++++++++++ 4 files changed, 112 insertions(+), 6 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 13128407f5..26e994b66e 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 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/xt/find-missing-csrf.t b/xt/find-missing-csrf.t new file mode 100755 index 0000000000..15dd6ae626 --- /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.39.5