From aebc7ba8db2a1a2310d68e25b28325c6b6a75cc1 Mon Sep 17 00:00:00 2001
From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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 <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
---
 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 8c9bd3b523..2a1adab1c6 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