From 17f7f8930a9a17d79e87a36efdfc53c983917a2f Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Tue, 30 Jan 2024 10:58:02 -0500 Subject: [PATCH 01/20] Bug 35942: OPAC user can enroll several times to the same club [23.05.x] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test Plan: 1) Create 3 clubs, 1 limited to library A, 1 limited to library B and one not limited 2) Use a patron with home library A. 3) Go to the opac-user page, "Clubs" tab show 0/2 (the one from library B is not listed) 4) Browse to /cgi-bin/koha/svc/club/enroll?id=1 5) Reload that page a couple times 6) Note the patron is now enrolled in the same club multiple times 7) Delete those enrollments 8) Apply this patch 9) Restart all the things! 10) Repeat steps 2-7, note the lack of duplicate enrollments! 11) Repeat steps 2-10 for the staff interface Signed-off-by: Jonathan Druart Signed-off-by: Martin Renvoize Signed-off-by: Lucas Gass (cherry picked from commit 9bdab108e22768b018b017ed7c0e0016270f2570) Signed-off-by: Frédéric Demians --- opac/svc/club/enroll | 51 +++++++++++++++++++++++++++----------------- svc/club/enroll | 18 ++++++++++------ 2 files changed, 43 insertions(+), 26 deletions(-) diff --git a/opac/svc/club/enroll b/opac/svc/club/enroll index aff1d51fb9..249a2bdd09 100755 --- a/opac/svc/club/enroll +++ b/opac/svc/club/enroll @@ -30,8 +30,8 @@ use Koha::Clubs; my $cgi = CGI->new; -my ( $auth_status ) = - check_cookie_auth( $cgi->cookie('CGISESSID') ); +my ($auth_status) = + check_cookie_auth( $cgi->cookie('CGISESSID') ); if ( $auth_status ne "ok" ) { exit 0; } @@ -42,31 +42,42 @@ my $id = $cgi->param('id'); my $enrollment; if ( $borrowernumber && $id ) { + my $already_enrolled = Koha::Club::Enrollments->search( + { + club_id => $id, + borrowernumber => $borrowernumber, + date_canceled => undef, + } + )->count(); + my $club = Koha::Clubs->find($id); - if ( $club->club_template()->is_enrollable_from_opac() ) { - $enrollment = Koha::Club::Enrollment->new()->set( - { - club_id => $club->id(), - borrowernumber => $borrowernumber, - date_enrolled => \'NOW()', - date_created => \'NOW()', - branchcode => C4::Context->userenv - ? C4::Context->userenv->{'branch'} - : undef, - } - )->store(); + my $wrong_branch = $club->branchcode && C4::Context->userenv && C4::Context->userenv->{branch} ne $club->branchcode; - my @enrollment_fields = $club->club_template()->club_template_enrollment_fields->as_list; + unless ( $already_enrolled || $wrong_branch ) { - foreach my $e (@enrollment_fields) { - Koha::Club::Enrollment::Field->new()->set( + if ( $club->club_template()->is_enrollable_from_opac() ) { + $enrollment = Koha::Club::Enrollment->new( { - club_enrollment_id => $enrollment->id(), - club_template_enrollment_field_id => $e->id(), - value => $cgi->param( $e->id() ), + club_id => $club->id(), + borrowernumber => $borrowernumber, + date_enrolled => \'NOW()', + date_created => \'NOW()', + branchcode => C4::Context->userenv ? C4::Context->userenv->{branch} : undef, } )->store(); + + my @enrollment_fields = $club->club_template()->club_template_enrollment_fields->as_list; + + foreach my $e (@enrollment_fields) { + Koha::Club::Enrollment::Field->new()->set( + { + club_enrollment_id => $enrollment->id(), + club_template_enrollment_field_id => $e->id(), + value => scalar $cgi->param( $e->id() ), + } + )->store(); + } } } } diff --git a/svc/club/enroll b/svc/club/enroll index 9c88c54717..87c6cddfdc 100755 --- a/svc/club/enroll +++ b/svc/club/enroll @@ -30,8 +30,8 @@ use Koha::Clubs; my $cgi = CGI->new; -my ( $auth_status ) = - check_cookie_auth( $cgi->cookie('CGISESSID'), { clubs => 'enroll' } ); +my ($auth_status) = + check_cookie_auth( $cgi->cookie('CGISESSID'), { clubs => 'enroll' } ); if ( $auth_status ne "ok" ) { exit 0; } @@ -43,15 +43,21 @@ my $club = Koha::Clubs->find($id); my $enrollment; if ($club) { + my $already_enrolled = Koha::Club::Enrollments->search( + { + club_id => $id, + borrowernumber => $borrowernumber, + date_canceled => undef, + } + )->count(); + $enrollment = Koha::Club::Enrollment->new( { club_id => $club->id(), borrowernumber => $borrowernumber, - date_enrolled => \'NOW()', - date_created => \'NOW()', - branchcode => C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef, + date_canceled => undef, } - )->store(); + )->store() unless $already_enrolled; if ($enrollment) { my @enrollment_fields = $club->club_template()->club_template_enrollment_fields->as_list; From ae48106422e333ea89c16daa57e20bba11063fa1 Mon Sep 17 00:00:00 2001 From: Andreas Jonsson Date: Thu, 7 Mar 2024 09:07:49 +0000 Subject: [PATCH 02/20] Bug 36244: Unit test for tt syntax in parameters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jonathan Druart Signed-off-by: Marcel de Rooy Signed-off-by: Kyle M Hall Signed-off-by: Katrin Fischer (cherry picked from commit 3f8b7785cd703f89de140108eb9347bf33a0c764) Signed-off-by: Fridolin Somers (cherry picked from commit 285f3093ed594d961c5618ed2b110f86f5467f35) Signed-off-by: Frédéric Demians --- t/db_dependent/Letters.t | 47 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t index 84f09a82f9..5115d4a895 100755 --- a/t/db_dependent/Letters.t +++ b/t/db_dependent/Letters.t @@ -18,7 +18,7 @@ # along with Koha; if not, see . use Modern::Perl; -use Test::More tests => 92; +use Test::More tests => 93; use Test::MockModule; use Test::Warn; use Test::Exception; @@ -1099,3 +1099,48 @@ subtest 'Test message_id parameter for SendQueuedMessages' => sub { is( $message_1->{status}, 'failed', 'Message 1 status is unchanged' ); is( $message_2->{status}, 'sent', 'Valid from_address => status sent' ); }; + +subtest 'Template toolkit syntax in parameters' => sub { + + my $borrowernumber = Koha::Patron->new( + { + firstname => 'Robert', + surname => '[% USE Categories %][% Categories.all().search_related("borrowers").count() %]', + categorycode => $patron_category, + branchcode => $library->{branchcode}, + dateofbirth => $date, + smsalertnumber => undef, + } + )->store->borrowernumber; + + my $title = q|<> - <>|; + my $content = q{Dear <> <>}; + + $dbh->do( + q|INSERT INTO letter(branchcode,module,code,name,is_html,title,content,message_transport_type) VALUES (?,'my module','tt test','my name',1,?,?,'email')|, + undef, $library->{branchcode}, $title, $content + ); + + my $tables = { + borrowers => $borrowernumber, + branches => $library->{branchcode}, + biblio => $biblionumber, + }; + my $substitute = { + status => 'overdue', + }; + my $prepared_letter = GetPreparedLetter( + module => 'my module', + branchcode => $library->{branchcode}, + letter_code => 'tt test', + tables => $tables, + substitute => $substitute, + repeat => [], + ); + + is( + $prepared_letter->{content}, + 'Dear Robert [% USE Categories %][% Categories.all().search_related("borrowers").count() %]', + 'Template toolkit syntax in parameter was not evaluated.' + ); +}; From dfcdc322e978910fa165e1d3d1be2742c6198b02 Mon Sep 17 00:00:00 2001 From: Andreas Jonsson Date: Thu, 7 Mar 2024 09:12:25 +0000 Subject: [PATCH 03/20] Bug 36244: Do template toolkit processing first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To avoid injection of template toolkit code from database fields that are controlled by untrusted sources. Test plan: * review subtest 'Template toolkit syntax in parameters' in t/db_dependent/Letters.t * Run the unit test: prove t/db_dependent/Letters.t Signed-off-by: Jonathan Druart Signed-off-by: Marcel de Rooy Signed-off-by: Kyle M Hall Signed-off-by: Katrin Fischer (cherry picked from commit 07ac3b0b9450f812bb48cfecf7bf3f47f63279b5) Signed-off-by: Fridolin Somers (cherry picked from commit 20353e094a952f506b9be7f21740e1001fbdeb69) Signed-off-by: Frédéric Demians --- C4/Letters.pm | 44 ++++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/C4/Letters.pm b/C4/Letters.pm index ab16f729c5..25300bd5ec 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -602,6 +602,28 @@ sub GetPreparedLetter { return; my $want_librarian = $params{want_librarian}; + $letter->{content} = _process_tt( + { + content => $letter->{content}, + lang => $lang, + loops => $loops, + objects => $objects, + substitute => $substitute, + tables => $tables, + } + ); + + $letter->{title} = _process_tt( + { + content => $letter->{title}, + lang => $lang, + loops => $loops, + objects => $objects, + substitute => $substitute, + tables => $tables, + } + ); + if (%$substitute) { while ( my ($token, $val) = each %$substitute ) { $val //= q{}; @@ -672,28 +694,6 @@ sub GetPreparedLetter { } } - $letter->{content} = _process_tt( - { - content => $letter->{content}, - lang => $lang, - loops => $loops, - objects => $objects, - substitute => $substitute, - tables => $tables, - } - ); - - $letter->{title} = _process_tt( - { - content => $letter->{title}, - lang => $lang, - loops => $loops, - objects => $objects, - substitute => $substitute, - tables => $tables, - } - ); - $letter->{content} =~ s/<<\S*>>//go; #remove any stragglers return $letter; From 652e3819bd35117aa7a388eeb06f8a9ee188b031 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Thu, 7 Mar 2024 11:10:35 -0500 Subject: [PATCH 04/20] Bug 36244: Add atomic update to check for affected notices Signed-off-by: Kyle M Hall Fixed some typos in bug numbers and text. Signed-off-by: Katrin Fischer (cherry picked from commit 2e18611b7d8527c7ff9253a7669aad2c13a5afb0) Signed-off-by: Fridolin Somers --- .../data/mysql/atomicupdate/bug_36244.pl | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100755 installer/data/mysql/atomicupdate/bug_36244.pl diff --git a/installer/data/mysql/atomicupdate/bug_36244.pl b/installer/data/mysql/atomicupdate/bug_36244.pl new file mode 100755 index 0000000000..5fd40ecb2d --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_36244.pl @@ -0,0 +1,27 @@ +use Modern::Perl; + +return { + bug_number => "36244", + description => "Template Toolkit syntax not escaped in letter templates", + up => sub { + my ($args) = @_; + my ( $dbh, $out ) = @$args{qw(dbh out)}; + + my $query = q{SELECT * FROM letter WHERE content LIKE "[|%%SET%<<%|%]" ESCAPE '|'}; + my $sth = $dbh->prepare($query); + $sth->execute(); + if ( $sth->rows ) { + say $out "You have one or more templates that have been affected by bug 36244."; + say $out "These templates assign template toolkit variables values"; + say $out "using the double arrows syntax. E.g. [% SET name = '<>' %]"; + say $out + "This will no longer function correctly as Template Toolkit is now rendered before the double arrow syntax."; + say $out "The following notices will need to be updated:"; + + while ( my $row = $sth->fetchrow_hashref() ) { + say $out + "ID: $row->{id} / MODULE: $row->{module} / CODE: $row->{code} / BRANCHCODE: $row->{branchcode} / NAME: $row->{name}"; + } + } + }, +}; From 193ac375aa5e9f30b4a3421cdca54856ebce3ea8 Mon Sep 17 00:00:00 2001 From: Julian Maurice Date: Thu, 1 Feb 2024 09:15:23 +0100 Subject: [PATCH 05/20] Bug 35960: Use .val() instead of string concat to prevent potential XSS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test plan: 1. Log out 2. Go to /cgi-bin/koha/mainpage.pl#somestring"withchar 3. Open the brower's inspector and find "auth_forwarded_hash" input 4. Make sure the value attribute is there and corresponds to the URL's fragment. It should be URI-encoded. Signed-off-by: Owen Leonard Signed-off-by: Victor Grousset/tuxayo Signed-off-by: Katrin Fischer (cherry picked from commit e6f8a4361e2975dfefcd9773fa61ef7d40300086) Signed-off-by: Fridolin Somers (cherry picked from commit 5409e17fb5abe0130f3cb2cd6c3d2a7707a5b251) Signed-off-by: Frédéric Demians --- koha-tmpl/intranet-tmpl/prog/en/modules/auth.tt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/auth.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/auth.tt index e87cb438c4..9ef3ebe7e0 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/auth.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/auth.tt @@ -219,7 +219,9 @@