From 48ac552699996ec5cb8632f2098174ac2dbeb61f Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 27 Feb 2020 10:31:13 +0100 Subject: [PATCH] Bug 24695: Move to Koha::Report->is_sql_valid Signed-off-by: Owen Leonard Signed-off-by: Kyle M Hall Signed-off-by: Jonathan Druart --- 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 06b7bd5fac..21627331dd 100644 --- a/C4/Reports/Guided.pm +++ b/C4/Reports/Guided.pm @@ -552,8 +552,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; foreach my $sql_param ( @$sql_params ){ if ( $sql_param =~ m/\n/ ){ diff --git a/Koha/Report.pm b/Koha/Report.pm index e11e75d4a0..c341ed60ab 100644 --- a/Koha/Report.pm +++ b/Koha/Report.pm @@ -36,6 +36,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 e882dcc8ca..49dfc95a3e 100755 --- a/reports/guided_reports.pl +++ b/reports/guided_reports.pl @@ -239,7 +239,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( @@ -595,7 +596,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 b6812599bb..e580c12d5d 100755 --- a/t/db_dependent/Koha/Reports.t +++ b/t/db_dependent/Koha/Reports.t @@ -71,14 +71,41 @@ subtest 'prep_report' => sub { $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.39.5