Attachment 99694 Details for Bug 24695 – Bug 24695: Move to Koha::Report->is_sql_valid

[patch] Bug 24695: Move to Koha::Report->is_sql_valid

Bug-24695-Move-to-KohaReport-issqlvalid.patch (text/plain), 6.11 KB, created by Jonathan Druart on 2020-02-27 09:31:48 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2020-02-27 09:31:48 UTC

Size: 6.11 KB

patch

obsolete

>From d9c455407c57c70c4b62ff1e251141a198f650bd Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>Date: Thu, 27 Feb 2020 10:31:13 +0100
>Subject: [PATCH] Bug 24695: Move to Koha::Report->is_sql_valid
>
>---
> C4/Reports/Guided.pm          |  4 ++--
> Koha/Report.pm                | 30 +++++++++++++++++++++++++++++
> Koha/Reports.pm               | 22 ---------------------
> reports/guided_reports.pl     |  6 ++++--
> t/db_dependent/Koha/Reports.t | 45 ++++++++++++++++++++++++++++++++++---------
> 5 files changed, 72 insertions(+), 35 deletions(-)
>
>diff --git a/C4/Reports/Guided.pm b/C4/Reports/Guided.pm
>index 6f785568a1..bc76f7c124 100644
>--- a/C4/Reports/Guided.pm
>+++ b/C4/Reports/Guided.pm
>@@ -549,8 +549,8 @@ sub execute_query {
>     $limit  = 999999 unless $limit;
>     $debug and print STDERR "execute_query($sql, $offset, $limit)\n";
> 
>-    my $errors = Koha::Reports->validate_sql($sql);
>-    return (undef, @{$errors}[0]) if (scalar(@$errors));
>+    my ( $is_sql_valid, $errors ) = Koha::Report->new({ savedsql => $sql })->is_sql_valid;
>+    return (undef, @{$errors}[0]) unless $is_sql_valid;
> 
>     my ($useroffset, $userlimit);
> 
>diff --git a/Koha/Report.pm b/Koha/Report.pm
>index 01b7dff786..01d28605fd 100644
>--- a/Koha/Report.pm
>+++ b/Koha/Report.pm
>@@ -35,6 +35,36 @@ Koha::Report - Koha Report Object class
> 
> =cut
> 
>+# FIXME We could only return an error code instead of the arrayref
>+# Only 1 error is returned
>+# TODO Koha::Report->store should check this before saving
>+=head3 is_sql_valid
>+
>+my ( $is_sql_valid, $errors ) = $report->is_sql_valid;
>+
>+$errors is a arrayref of hashrefs, keys can be sqlerr or queryerr.
>+
>+Validate SQL query string so it only contains a select,
>+not any of the harmful queries.
>+
>+=cut
>+
>+sub is_sql_valid {
>+    my ($self) = @_;
>+
>+    my $sql = $self->savedsql;
>+    $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 ? 0 : 1, \@errors );
>+}
>+
> =head3 get_search_info
> 
> Return search info
>diff --git a/Koha/Reports.pm b/Koha/Reports.pm
>index 65a2a57b8f..873f295abd 100644
>--- a/Koha/Reports.pm
>+++ b/Koha/Reports.pm
>@@ -35,28 +35,6 @@ 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 986270621c..1daf0ca0bc 100755
>--- a/reports/guided_reports.pl
>+++ b/reports/guided_reports.pl
>@@ -240,7 +240,8 @@ elsif ( $phase eq 'Update SQL'){
> 
>     create_non_existing_group_and_subgroup($input, $group, $subgroup);
> 
>-    push(@errors, @{Koha::Reports->validate_sql($sql)});
>+    my ( $is_sql_valid, $validation_errors ) = Koha::Report->new({ savedsql => $sql })->is_sql_valid;
>+    push(@errors, @$validation_errors) unless $is_sql_valid;
> 
>     if (@errors) {
>         $template->param(
>@@ -596,7 +597,8 @@ 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
>-    push(@errors, @{Koha::Reports->validate_sql($sql)});
>+    my ( $is_sql_valid, $validation_errors ) = Koha::Report->new({ savedsql => $sql })->is_sql_valid;
>+    push(@errors, @$validation_errors) unless $is_sql_valid;
> 
>     if (@errors) {
>         $template->param(
>diff --git a/t/db_dependent/Koha/Reports.t b/t/db_dependent/Koha/Reports.t
>index 0e7c1a7df6..fdb2b81891 100644
>--- a/t/db_dependent/Koha/Reports.t
>+++ b/t/db_dependent/Koha/Reports.t
>@@ -50,14 +50,41 @@ is( Koha::Reports->search->count, $nb_of_reports + 1, 'Delete should have delete
> 
> $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' );
>+subtest 'is_sql_valid' => sub {
>+    plan tests => 3 + 6 * 2;
>+    my @badwords = ( 'UPDATE', 'DELETE', 'DROP', 'INSERT', 'SHOW', 'CREATE' );
>+    is_deeply(
>+        [ Koha::Report->new( { savedsql => '' } )->is_sql_valid ],
>+        [ 0, [ { queryerr => 'Missing SELECT' } ] ],
>+        'Empty sql is missing SELECT'
>+    );
>+    is_deeply(
>+        [ Koha::Report->new( { savedsql => 'FOO' } )->is_sql_valid ],
>+        [ 0, [ { queryerr => 'Missing SELECT' } ] ],
>+        'Nonsense sql is missing SELECT'
>+    );
>+    is_deeply(
>+        [ Koha::Report->new( { savedsql => 'select FOO' } )->is_sql_valid ],
>+        [ 1, [] ],
>+        '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' );
>+        is_deeply(
>+            [
>+                Koha::Report->new(
>+                    { savedsql => 'select FOO;' . $word . ' BAR' }
>+                )->is_sql_valid
>+            ],
>+            [ 0, [ { sqlerr => $word } ] ],
>+            'select FOO with ' . $word . ' BAR'
>+        );
>+        is_deeply(
>+            [
>+                Koha::Report->new( { savedsql => $word . ' qux' } )
>+                  ->is_sql_valid
>+            ],
>+            [ 0, [ { sqlerr => $word } ] ],
>+            $word . ' qux'
>+        );
>     }
>-}
>+  }
>-- 
>2.11.0

Actions: View | Diff | Splinter Review

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