From 0299f8971839f7c1aa5513f3d08563ca3cc6a666 Mon Sep 17 00:00:00 2001
From: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Date: Thu, 20 Feb 2020 11:31:08 +0200
Subject: [PATCH] Bug 24695: Improve SQL report validation

The saved SQL report code validates the SQL in multiple places:
when saving, when updating, and when executing the query.
Move the validation code into Koha::Reports, and write tests for it.

Test plan:
1) Apply patch
2) Create a new valid SQL report, save it (success)
3) Create a new illegal SQL report, try to save (fails)
4) Update already saved SQL report by adding one of
   the forbidden words, eg. delete or drop (saving will fail)
5) Edit a save_sql in the database, changing it to eg.
   "drop borrowers", and try to execute it (fails)
6) Prove t/db_dependent/Koha/Reports.t

Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
---
 C4/Reports/Guided.pm          |  8 +++-----
 Koha/Reports.pm               | 22 ++++++++++++++++++++++
 reports/guided_reports.pl     | 14 ++------------
 t/db_dependent/Koha/Reports.t | 14 +++++++++++++-
 4 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/C4/Reports/Guided.pm b/C4/Reports/Guided.pm
index 61aa804628..6f785568a1 100644
--- a/C4/Reports/Guided.pm
+++ b/C4/Reports/Guided.pm
@@ -548,11 +548,9 @@ sub execute_query {
     $offset = 0    unless $offset;
     $limit  = 999999 unless $limit;
     $debug and print STDERR "execute_query($sql, $offset, $limit)\n";
-    if ($sql =~ /;?\W?(UPDATE|DELETE|DROP|INSERT|SHOW|CREATE)\W/i) {
-        return (undef, {  sqlerr => $1} );
-    } elsif ($sql !~ /^\s*SELECT\b\s*/i) {
-        return (undef, { queryerr => 'Missing SELECT'} );
-    }
+
+    my $errors = Koha::Reports->validate_sql($sql);
+    return (undef, @{$errors}[0]) if (scalar(@$errors));
 
     my ($useroffset, $userlimit);
 
diff --git a/Koha/Reports.pm b/Koha/Reports.pm
index fc2be72d70..8957ea9d9d 100644
--- a/Koha/Reports.pm
+++ b/Koha/Reports.pm
@@ -35,6 +35,28 @@ Koha::Reports - Koha Report Object set class
 
 =cut
 
+=head3 validate_sql
+
+Validate SQL query string so it only contains a select,
+not any of the harmful queries.
+
+=cut
+
+sub validate_sql {
+    my ($self, $sql) = @_;
+
+    $sql //= '';
+    my @errors = ();
+
+    if ($sql =~ /;?\W?(UPDATE|DELETE|DROP|INSERT|SHOW|CREATE)\W/i) {
+        push @errors, { sqlerr => $1 };
+    } elsif ($sql !~ /^\s*SELECT\b\s*/i) {
+        push @errors, { queryerr => 'Missing SELECT' };
+    }
+
+    return \@errors;
+}
+
 =head3 _type
 
 Returns name of corresponding DBIC resultset
diff --git a/reports/guided_reports.pl b/reports/guided_reports.pl
index fa2817f1c7..986270621c 100755
--- a/reports/guided_reports.pl
+++ b/reports/guided_reports.pl
@@ -240,12 +240,7 @@ elsif ( $phase eq 'Update SQL'){
 
     create_non_existing_group_and_subgroup($input, $group, $subgroup);
 
-    if ($sql =~ /;?\W?(UPDATE|DELETE|DROP|INSERT|SHOW|CREATE)\W/i) {
-        push @errors, {sqlerr => $1};
-    }
-    elsif ($sql !~ /^(SELECT)/i) {
-        push @errors, {queryerr => "No SELECT"};
-    }
+    push(@errors, @{Koha::Reports->validate_sql($sql)});
 
     if (@errors) {
         $template->param(
@@ -601,12 +596,7 @@ elsif ( $phase eq 'Save Report' ) {
 
     create_non_existing_group_and_subgroup($input, $group, $subgroup);
     ## FIXME this is AFTER entering a name to save the report under
-    if ($sql =~ /;?\W?(UPDATE|DELETE|DROP|INSERT|SHOW|CREATE)\W/i) {
-        push @errors, {sqlerr => $1};
-    }
-    elsif ($sql !~ /^(SELECT)/i) {
-        push @errors, {queryerr => "No SELECT"};
-    }
+    push(@errors, @{Koha::Reports->validate_sql($sql)});
 
     if (@errors) {
         $template->param(
diff --git a/t/db_dependent/Koha/Reports.t b/t/db_dependent/Koha/Reports.t
index 45c5662247..0e7c1a7df6 100644
--- 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 => 4;
+use Test::More tests => 5;
 
 use Koha::Report;
 use Koha::Reports;
@@ -49,3 +49,15 @@ $retrieved_report_1->delete;
 is( Koha::Reports->search->count, $nb_of_reports + 1, 'Delete should have deleted the report' );
 
 $schema->storage->txn_rollback;
+
+subtest 'validate_sql' => sub {
+    plan tests => 3 + 6*2;
+    my @badwords = ('UPDATE', 'DELETE', 'DROP', 'INSERT', 'SHOW', 'CREATE');
+    is_deeply( Koha::Reports->validate_sql(), [{ queryerr => 'Missing SELECT'}], 'Empty sql is missing SELECT' );
+    is_deeply( Koha::Reports->validate_sql('FOO'), [{ queryerr => 'Missing SELECT'}], 'Nonsense sql is missing SELECT' );
+    is_deeply( Koha::Reports->validate_sql('select FOO'), [], 'select FOO is good' );
+    foreach my $word (@badwords) {
+        is_deeply( Koha::Reports->validate_sql('select FOO;'.$word.' BAR'), [{ sqlerr => $word}], 'select FOO with '.$word.' BAR' );
+        is_deeply( Koha::Reports->validate_sql($word.' qux'), [{ sqlerr => $word}], $word.' qux' );
+    }
+}
-- 
2.11.0