Bugzilla – Attachment 119421 Details for
Bug 24695
Improve SQL report validation
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 24695: Improve SQL report validation
Bug-24695-Improve-SQL-report-validation.patch (text/plain), 4.81 KB, created by
Kyle M Hall (khall)
on 2021-04-09 16:44:01 UTC
(
hide
)
Description:
Bug 24695: Improve SQL report validation
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2021-04-09 16:44:01 UTC
Size:
4.81 KB
patch
obsolete
>From 1678429c9a0453e09c54deb7cf846cb4ad44525a 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. > >Signed-off-by: Owen Leonard <oleonard@myacpl.org> > >Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> >--- > C4/Reports/Guided.pm | 8 +++----- > Koha/Reports.pm | 22 ++++++++++++++++++++++ > reports/guided_reports.pl | 14 ++------------ > t/db_dependent/Koha/Reports.t | 12 ++++++++++++ > 4 files changed, 39 insertions(+), 17 deletions(-) > >diff --git a/C4/Reports/Guided.pm b/C4/Reports/Guided.pm >index a76d86ba44..06b7bd5fac 100644 >--- a/C4/Reports/Guided.pm >+++ b/C4/Reports/Guided.pm >@@ -551,11 +551,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)); > > foreach my $sql_param ( @$sql_params ){ > if ( $sql_param =~ m/\n/ ){ >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 cff8da8ffe..e882dcc8ca 100755 >--- a/reports/guided_reports.pl >+++ b/reports/guided_reports.pl >@@ -239,12 +239,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( >@@ -600,12 +595,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 ab982c84d7..b0a9923a7b 100755 >--- a/t/db_dependent/Koha/Reports.t >+++ b/t/db_dependent/Koha/Reports.t >@@ -70,3 +70,15 @@ 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' ); >+ 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.24.3 (Apple Git-128)
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 24695
:
99305
|
99625
|
99694
|
117352
|
117353
| 119421 |
119422
|
119423
|
119424