Attachment 99625 Details for Bug 24695 – Bug 24695: Improve SQL report validation

[patch] Bug 24695: Improve SQL report validation

Bug-24695-Improve-SQL-report-validation.patch (text/plain), 4.87 KB, created by Bernardo Gonzalez Kriegel on 2020-02-25 23:32:21 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Bernardo Gonzalez Kriegel

Created: 2020-02-25 23:32:21 UTC

Size: 4.87 KB

patch

obsolete

>From 1d4f2f23aba2b61411f70d0e7edd22f6a038b957 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: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
>Work as described, no qa errors.
>---
> 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 873f295abd..65a2a57b8f 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.17.1

Actions: View | Diff | Splinter Review

Attachments on bug 24695: 99305 | 99625 | 99694 | 117352 | 117353 | 119421 | 119422 | 119423 | 119424