From 947865f83b11ddf32155db7acf89b6b389b99842 Mon Sep 17 00:00:00 2001
From: Aleisha Amohia <aleishaamohia@hotmail.com>
Date: Mon, 29 Jul 2024 03:53:06 +0000
Subject: [PATCH] Bug 37508: Throw error if password column is detected in SQL
 report

This enhancement prevents SQL queries from being run if they would return a password field from the database table.

To test:

1. Run tests and notice they fail t/db_dependent/Reports/Guided.t

2. Apply patch and restart services

3. Create a public report with an SQL report which would access a password column in a database table
4. Try to run the report. Notice you are met with an error and the results are not shown.
5. Access the JSON URL, you should not get the results and should be shown an error
6. Confirm tests pass t/db_dependent/Reports/Guided.t

Sponsored-by: Reserve Bank of New Zealand
Signed-off-by: David Cook <dcook@prosentient.com.au>

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>
---
 C4/Reports/Guided.pm                          |  7 +++++
 .../modules/reports/guided_reports_start.tt   |  4 +++
 svc/report                                    |  7 ++---
 t/db_dependent/Reports/Guided.t               | 30 ++++++++++++++++++-
 4 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/C4/Reports/Guided.pm b/C4/Reports/Guided.pm
index f69804c283..8558666041 100644
--- a/C4/Reports/Guided.pm
+++ b/C4/Reports/Guided.pm
@@ -623,6 +623,13 @@ sub execute_query {
         ->info("Report finished: $report_id") if $report_id;
 
     return ( $sth, { queryerr => $sth->errstr } ) if ( $sth->err );
+
+    foreach my $column ( @{ $sth->{NAME_lc} } ) {
+        if ( $column eq 'password' ) {
+            return ( $sth, { passworderr => $column } );
+        }
+    }
+
     return ($sth);
 }
 
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt
index 9dd38ba53b..ea57844345 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt
@@ -1443,6 +1443,10 @@
                                     [% error.queryerr | html %]<br />Please check the log for further details.
                                 [% ELSIF ( error.cache_expiry ) %]
                                     Please select a cache expiry less than 30 days.
+                                [% ELSIF ( error.passworderr ) %]
+                                    The column selection in this report includes a password field.<br>
+                                    The report cannot be executed due to security risks.<br>
+                                    Please edit this report and ensure no password columns have been selected.
                                 [% ELSE %]
                                 [% END %]
                                 <div id="onerror_actions">
diff --git a/svc/report b/svc/report
index e36e913280..b5aa5ddb2a 100755
--- a/svc/report
+++ b/svc/report
@@ -74,7 +74,9 @@ unless ($json_text) {
             report_id  => $report_id,
         }
     );
-    if ($sth) {
+    if ($errors) {
+        $json_text = encode_json($errors);
+    } else {
         my $lines;
         if ($report_annotation) {
             $lines = $sth->fetchall_arrayref({});
@@ -88,9 +90,6 @@ unless ($json_text) {
             $cache->set_in_cache( $cache_key, $json_text, { expiry => $report_rec->cache_expiry } );
         }
     }
-    else {
-        $json_text = encode_json($errors);
-    }
 }
 
 print $query->header(
diff --git a/t/db_dependent/Reports/Guided.t b/t/db_dependent/Reports/Guided.t
index 6fda469ba0..0db1a6295b 100755
--- a/t/db_dependent/Reports/Guided.t
+++ b/t/db_dependent/Reports/Guided.t
@@ -18,7 +18,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 11;
+use Test::More tests => 12;
 use Test::Warn;
 
 use t::lib::TestBuilder;
@@ -499,6 +499,34 @@ subtest 'nb_rows() tests' => sub {
     $schema->storage->txn_rollback;
 };
 
+subtest 'Returning passwords tests' => sub {
+
+    plan tests => 3;
+
+    my $dbh = C4::Context->dbh;
+    $schema->storage->txn_begin;
+
+    my $query = q{ SELECT * FROM borrowers };
+
+    my ( $sth, $errors ) = execute_query( { sql => $query } );
+
+    is( defined($errors), 1, 'Query returns password field' );
+
+    $query = q{ SELECT * FROM z3950servers };
+
+    ( $sth, $errors ) = execute_query( { sql => $query } );
+
+    is( defined($errors), 1, 'Query returns password field' );
+
+    $query = q{ SELECT password FROM deletedborrowers };
+
+    ( $sth, $errors ) = execute_query( { sql => $query } );
+
+    is( defined($errors), 1, 'Query returns password field' );
+
+    $schema->storage->txn_rollback;
+};
+
 sub trim {
     my ($s) = @_;
     $s =~ s/^\s*(.*?)\s*$/$1/s;