From 1d4f2f23aba2b61411f70d0e7edd22f6a038b957 Mon Sep 17 00:00:00 2001 From: Pasi Kallinen 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 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