From c4ddaeb6aaaaae68c97a8424c704f8d0854b7a54 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Fri, 9 Aug 2024 09:50:44 +0000 Subject: [PATCH] Bug 37508: (QA follow-up) Move check to Koha::Report, extend Do not allow password but allow password_expiry_days etc. Do not allow token, secret and uuid too. Test plan: Run t/db_dependent/Koha/Reports.t Signed-off-by: Marcel de Rooy Signed-off-by: Tomas Cohen Arazi --- Koha/Report.pm | 42 +++++++++++++++++++++++++++++++++++ t/db_dependent/Koha/Reports.t | 29 ++++++++++++++++++++---- 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/Koha/Report.pm b/Koha/Report.pm index 55daece836..78fcf4bb26 100644 --- a/Koha/Report.pm +++ b/Koha/Report.pm @@ -28,6 +28,10 @@ use base qw(Koha::Object); # Only 1 error is returned # TODO Koha::Report->store should check this before saving +use constant FORBIDDEN_COLUMN_MATCHES => qw(password token uuid secret); +use constant WHITELISTED_COLUMN_NAMES => + qw(password_expiration_date password_expiry_days reset_password change_password min_password_length require_strong_password password_expiration_date); + =head1 NAME Koha::Report - Koha Report Object class @@ -58,11 +62,49 @@ sub is_sql_valid { push @errors, { sqlerr => $1 }; } elsif ($sql !~ /^\s*SELECT\b\s*/i) { push @errors, { queryerr => 'Missing SELECT' }; + } else { + push @errors, { passworderr => "Illegal column in results" } + if $self->check_columns($sql); } return ( @errors ? 0 : 1, \@errors ); } +=head3 check_columns + + $self->check_columns('SELECT password from borrowers'); + $self->check_columns( undef, [qw(password token)] ); + + Check if passed sql statement or column_names arrayref contains + forbidden column names (credentials etc.). + Returns all bad column names or empty list. + +=cut + +sub check_columns { + my ( $self, $sql, $column_names ) = @_; + + # TODO When passing sql, we actually go thru the whole statement, not just columns + + my $regex; + push my @columns, @{ $column_names // [] }; + if ($sql) { + + # Now find all matches for e.g. password but also modulepassword or password_hash + # Note that \w includes underscore, not hyphen + $regex = '(\w*(?:' . ( join '|', FORBIDDEN_COLUMN_MATCHES ) . ')\w*)'; + @columns = ( $sql =~ /$regex/ig ); + } else { + $regex = '(' . ( join '|', FORBIDDEN_COLUMN_MATCHES ) . ')'; + @columns = grep { /$regex/i } @columns; + } + + # Check for whitelisted variants + $regex = '(^' . ( join '|', WHITELISTED_COLUMN_NAMES ) . ')$'; # should be full match + @columns = grep { !/$regex/i } @columns; + return @columns; +} + =head3 get_search_info Return search info diff --git a/t/db_dependent/Koha/Reports.t b/t/db_dependent/Koha/Reports.t index 5db4d3bae9..3f7eaacd6c 100755 --- a/t/db_dependent/Koha/Reports.t +++ b/t/db_dependent/Koha/Reports.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 6; +use Test::More tests => 7; use Koha::Report; use Koha::Reports; @@ -76,8 +76,6 @@ subtest 'prep_report' => sub { is_deeply( $headers, { 'itemnumber for batch' => 'itemnumber' } ); }; -$schema->storage->txn_rollback; - subtest 'is_sql_valid' => sub { plan tests => 3 + 6 * 2; my @badwords = ( 'UPDATE', 'DELETE', 'DROP', 'INSERT', 'SHOW', 'CREATE' ); @@ -115,4 +113,27 @@ subtest 'is_sql_valid' => sub { $word . ' qux' ); } - } +}; + +subtest 'check_columns' => sub { + plan tests => 3; + + my $report = Koha::Report->new; + is_deeply( [ $report->check_columns('SELECT passWorD from borrowers') ], ['passWorD'], 'Bad column found in SQL' ); + is( scalar $report->check_columns('SELECT reset_passWorD from borrowers'), 0, 'No bad column found in SQL' ); + + is_deeply( + [ + $report->check_columns( + undef, + [ + qw(change_password hash secret test place mytoken hersecret password_expiry_days password_expiry_days2) + ] + ) + ], + [qw(secret mytoken hersecret password_expiry_days2)], + 'Check column_names parameter' + ); +}; + +$schema->storage->txn_rollback; -- 2.39.5