Bugzilla – Attachment 153751 Details for
Bug 22990
Add CSRF protection to boraccount, pay, suggestions and virtualshelves on staff
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 30524: [21.11] Core CSRF checking code
Bug-30524-2111-Core-CSRF-checking-code.patch (text/plain), 9.99 KB, created by
Jonathan Druart
on 2023-07-21 06:23:19 UTC
(
hide
)
Description:
Bug 30524: [21.11] Core CSRF checking code
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2023-07-21 06:23:19 UTC
Size:
9.99 KB
patch
obsolete
>From fe1e7aac96f7f32701887554625acfd9c6b3cb26 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@ptfs-europe.com> >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 <dcook@prosentient.com.au> > >Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> > >Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> > >Bug 30524: Add xt/find-missing-csrf.t > >Signed-off-by: David Cook <dcook@prosentient.com.au> > >Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> > >Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> > >Bug 30524: (QA follow-up) Polishing xt script > >Test plan: >Run it again. Same results? > >Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> > >Bug 30524: Unit tests > >Test plan: >Run t/Output.t >Run t/db_dependent/Auth.t > >Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >--- > 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 @@ >+<input type="hidden" name="csrf_token" value="[% csrf_token | html %]" /> >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 <http://www.gnu.org/licenses>. >+ >+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|<form| } @lines; >+ my ( $open, $found ) = ( 0, 0 ); >+ my $line = 0 ; >+ for my $l (@lines) { >+ $line++; >+ $open = $line if ( $l =~ m{<form} && !( $l =~ m{method=('|")get('|")} ) ); >+ $found++ if ( $l =~ m|csrf\-token\.inc| && $open ); >+ if ( $open && $l =~ m|</form| ) { >+ push @errors, >+ "The <form> starting on line $open is missing it's corresponding csrf_token include (see bug 22990)" >+ if !$found; >+ $found = 0; >+ } >+ } >+ return @errors; >+} >-- >2.25.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 22990
:
90102
|
90192
|
90194
|
90202
|
90204
|
90209
|
90238
|
90239
|
90240
|
95381
|
95382
|
95383
|
95384
|
95385
|
95391
|
95392
|
95393
|
95394
|
95395
|
95396
|
97814
|
122010
|
122011
|
122012
|
122013
|
122014
|
122015
|
122016
|
122023
|
122024
|
122025
|
122026
|
122027
|
122028
|
122029
|
122030
|
122031
|
122046
|
124924
|
124925
|
124926
|
124927
|
124928
|
124929
|
124930
|
124931
|
124932
|
124933
|
124934
|
126091
|
126092
|
126093
|
126094
|
126095
|
126096
|
126097
|
126098
|
126099
|
126100
|
126101
|
133238
|
133239
|
133240
|
133241
|
133242
|
133243
|
133244
|
133245
|
133246
|
133247
|
133248
|
133254
|
133255
|
133256
|
133257
|
133258
|
133422
|
133423
|
133424
|
133425
|
133426
|
133427
|
134848
|
134849
|
134850
|
134851
|
134852
|
134853
|
134854
|
141294
|
141295
|
141296
|
144315
|
144316
|
144317
|
144318
|
144319
|
144320
|
144321
|
144322
|
152626
|
152627
|
152628
|
153394
|
153751
|
153753
|
154044