[Koha-patches] Fwd: [PATCH] kohabug 2458 Disallowing non-SELECT SQL in reports module
Chris Nighswonger
chris.nighswonger at liblime.com
Thu Aug 7 03:44:08 CEST 2008
From: Chris Nighswonger <chris.nighswonger at liblime.com>
This patch enforces SELECT-only SQL in the reports module.
It introduces code to check SQL in two places. The first is
when a save is attempted on a user constructed SQL statement.
If a non-SELECT SQL statement is entered, the user will be
presented with an error message and a button giving the
option of editing the SQL. The second is when any SQL is
executed. If execution of a non-SELECT SQL statement is
attempted, the user is presented with an error message and
instructed to delete that report as the SQL is invalid.
The second check is intended as a safety net as no non-SELECT
SQL should ever be saved.
It may be well to document the proper usage of the direct SQL
entry type report.
Signed-off-by: Andrew Moore <andrew.moore at liblime.com>
---
C4/Reports.pm | 200 +++++++++++---------
.../en/modules/reports/guided_reports_start.tmpl | 49 ++++-
reports/guided_reports.pl | 90 +++++++--
3 files changed, 224 insertions(+), 115 deletions(-)
diff --git a/C4/Reports.pm b/C4/Reports.pm
index 811a919..9a6de77 100644
--- a/C4/Reports.pm
+++ b/C4/Reports.pm
@@ -335,10 +335,17 @@ sub get_criteria {
When passed C<$sql>, this function returns an array ref containing
a result set
suitably formatted for display in html or for output as a flat
file when passed in
- C<$format> and C<$id>. Valid values for C<$format> are 'text,'
'tab,' 'csv,' or 'url.
- C<$sql>, C<$type>, C<$offset>, and C<$limit> are required
parameters. If a valid
- C<$format> is passed in, C<$offset> and C<$limit> are ignored for
obvious reasons.
- A LIMIT specified by the user in a user-supplied SQL query WILL
apply in any case.
+ C<$format> and C<$id>. It also returns the C<$total> records
available for the
+ supplied query. If passed any query other than a SELECT, or if
there is a db error,
+ C<$errors> an array ref is returned containing the error after this manner:
+
+ C<$error->{'sqlerr'}> contains the offending SQL keyword.
+ C<$error->{'queryerr'}> contains the native db engine error
returned for the query.
+
+ Valid values for C<$format> are 'text,' 'tab,' 'csv,' or 'url.
C<$sql>, C<$type>,
+ C<$offset>, and C<$limit> are required parameters. If a valid
C<$format> is passed
+ in, C<$offset> and C<$limit> are ignored for obvious reasons. A
LIMIT specified by
+ the user in a user-supplied SQL query WILL apply in any case.
=cut
@@ -347,96 +354,117 @@ sub execute_query ($$$$;$$) {
my @params;
my $total = 0;
my ($useroffset, $userlimit);
- my $dbh = C4::Context->dbh();
- unless ($format eq 'text' || $format eq 'tab' || $format eq 'csv'
|| $format eq 'url'){
- # Grab offset/limit from user supplied LIMIT and drop the
LIMIT so we can control pagination
- if ($sql =~ /LIMIT/i) {
- $sql =~ s/LIMIT\W?(\d+)?\,?\W+?(\d+)//ig;
- $debug and warn "User has supplied LIMIT\n";
- $useroffset = $1;
- $userlimit = $2;
- $debug and warn "User supplied offset = $useroffset,
limit = $userlimit\n";
- $offset += $useroffset if $useroffset;
- # keep track of where we are if there is a user supplied LIMIT
- if ( $offset + $limit > $userlimit ) {
- $limit = $userlimit - $offset;
- }
- }
- my $countsql = $sql;
- $sql .= " LIMIT ?, ?";
- $debug and warn "Passing query with params offset = $offset,
limit = $limit\n";
- @params = ($offset, $limit);
- # Modify the query passed in to create a count query... (I
think this covers all cases -crn)
- $countsql =~
s/\bSELECT\W+(?:\w+\W+){1,}?FROM\b|\bSELECT\W\*\WFROM\b/SELECT
count(*) FROM /ig;
- $debug and warn "original query: $sql\n";
- $debug and warn "count query: $countsql\n";
- my $sth1 = $dbh->prepare($countsql);
- $sth1->execute();
- $total = $sth1->fetchrow();
- $debug and warn "total records for this query: $total\n";
- $total = $userlimit if defined($userlimit) and $userlimit <
$total; # we will never exceed a user defined LIMIT and...
- $userlimit = $total if defined($userlimit) and $userlimit >
$total; # we will never exceed the total number of records
available to satisfy the query
+ my @errors = ();
+ my $error = {};
+ my $sqlerr = 0;
+ if ($sql =~ /;?\W?(UPDATE|DELETE|DROP|INSERT|SHOW|CREATE)\W/i) {
+ $sqlerr = 1;
+ $error->{'sqlerr'} = $1;
+ push @errors, $error;
+ } elsif ($sql !~ /^(SELECT)/i) {
+ $sqlerr = 1;
+ $error->{'queryerr'} = 'Missing SELECT';
+ push @errors, $error;
}
- my $sth = $dbh->prepare($sql);
- $sth->execute(@params);
- my $colnames=$sth->{'NAME'};
- my @results;
- my $row;
- my %temphash;
- $row = join ('</th><th>',@$colnames);
- $row = "<tr><th>$row</th></tr>";
- $temphash{'row'} = $row;
- push @results, \%temphash;
- my $string;
- if ($format eq 'tab') {
- $string = join("\t",@$colnames);
- }
- if ($format eq 'csv') {
- $string = join(",",@$colnames);
- }
- my @xmlarray;
- while ( my @data = $sth->fetchrow_array() ) {
- # if the field is a date field, it needs formatting
- foreach my $data (@data) {
- next unless $data =~ C4::Dates->regexp("iso");
- my $date = C4::Dates->new($data, "iso");
- $data = $date->output();
+ if ($sqlerr == 0) {
+ my $dbh = C4::Context->dbh();
+ unless ($format eq 'text' || $format eq 'tab' || $format eq
'csv' || $format eq 'url'){
+ # Grab offset/limit from user supplied LIMIT and drop the
LIMIT so we can control pagination
+ if ($sql =~ /LIMIT/i) {
+ $sql =~ s/LIMIT\W?(\d+)?\,?\W+?(\d+)//ig;
+ $debug and warn "User has supplied LIMIT\n";
+ $useroffset = $1;
+ $userlimit = $2;
+ $debug and warn "User supplied offset = $useroffset,
limit = $userlimit\n";
+ $offset += $useroffset if $useroffset;
+ # keep track of where we are if there is a user supplied LIMIT
+ if ( $offset + $limit > $userlimit ) {
+ $limit = $userlimit - $offset;
+ }
+ }
+ my $countsql = $sql;
+ $sql .= " LIMIT ?, ?";
+ $debug and warn "Passing query with params offset =
$offset, limit = $limit\n";
+ @params = ($offset, $limit);
+ # Modify the query passed in to create a count query...
(I think this covers all cases -crn)
+ $countsql =~
s/\bSELECT\W+(?:\w+\W+){1,}?FROM\b|\bSELECT\W\*\WFROM\b/SELECT
count(*) FROM /ig;
+ $debug and warn "original query: $sql\n";
+ $debug and warn "count query: $countsql\n";
+ my $sth1 = $dbh->prepare($countsql);
+ $sth1->execute();
+ $total = $sth1->fetchrow();
+ $debug and warn "total records for this query: $total\n";
+ $total = $userlimit if defined($userlimit) and $userlimit
< $total; # we will never exceed a user defined LIMIT and...
+ $userlimit = $total if defined($userlimit) and $userlimit
> $total; # we will never exceed the total number of records
available to satisfy the query
}
- # tabular
+ my $sth = $dbh->prepare($sql);
+ $sth->execute(@params);
+ my $colnames=$sth->{'NAME'};
+ my @results;
+ my $row;
my %temphash;
- my $row = join( '</td><td>', @data );
- $row = "<tr><td>$row</td></tr>";
+ $row = join ('</th><th>',@$colnames);
+ $row = "<tr><th>$row</th></tr>";
$temphash{'row'} = $row;
- if ( $format eq 'text' ) {
- $string .= "\n" . $row;
+ push @results, \%temphash;
+ my $string;
+ if ($format eq 'tab') {
+ $string = join("\t",@$colnames);
}
- if ($format eq 'tab' ){
- $row = join("\t", at data);
- $string .="\n" . $row;
+ if ($format eq 'csv') {
+ $string = join(",",@$colnames);
}
- if ($format eq 'csv' ){
- $row = join(",", at data);
- $string .="\n" . $row;
+ my @xmlarray;
+ while ( my @data = $sth->fetchrow_array() ) {
+ # if the field is a date field, it needs formatting
+ foreach my $data (@data) {
+ next unless $data =~ C4::Dates->regexp("iso");
+ my $date = C4::Dates->new($data, "iso");
+ $data = $date->output();
+ }
+ # tabular
+ my %temphash;
+ my $row = join( '</td><td>', @data );
+ $row = "<tr><td>$row</td></tr>";
+ $temphash{'row'} = $row;
+ if ( $format eq 'text' ) {
+ $string .= "\n" . $row;
+ }
+ if ($format eq 'tab' ){
+ $row = join("\t", at data);
+ $string .="\n" . $row;
+ }
+ if ($format eq 'csv' ){
+ $row = join(",", at data);
+ $string .="\n" . $row;
+ }
+ if ($format eq 'url'){
+ my $temphash;
+ @$temphash{@$colnames}=@data;
+ push @xmlarray,$temphash;
+ }
+ push @results, \%temphash;
}
- if ($format eq 'url'){
- my $temphash;
- @$temphash{@$colnames}=@data;
- push @xmlarray,$temphash;
+ if (defined($sth->errstr)) {
+ $error->{'queryerr'} = $sth->errstr;
+ push @errors, $error;
+ warn "Database returned: $sth->errstr";
}
- push @results, \%temphash;
- }
- if ( $format eq 'text' || $format eq 'tab' || $format eq 'csv' ) {
- return $string;
- }
- elsif ($format eq 'url') {
- my $url =
"/cgi-bin/koha/reports/guided_reports.pl?phase=retrieve%20results&id=$id";
- my $dump = new XML::Dumper;
- my $xml = $dump->pl2xml( \@xmlarray );
- store_results($id,$xml);
- return $url;
- }
- else {
- return ( \@results, $total );
+ if ( $format eq 'text' || $format eq 'tab' || $format eq 'csv' ) {
+ return $string, $total, \@errors;
+ }
+ elsif ($format eq 'url') {
+ my $url =
"/cgi-bin/koha/reports/guided_reports.pl?phase=retrieve%20results&id=$id";
+ my $dump = new XML::Dumper;
+ my $xml = $dump->pl2xml( \@xmlarray );
+ store_results($id,$xml);
+ return $url, $total, \@errors;
+ }
+ else {
+ return \@results, $total, \@errors;
+ }
+ } else {
+ return undef, undef, \@errors;
}
}
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tmpl
b/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tmpl
index 8bf572a..38fa9af 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tmpl
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tmpl
@@ -359,7 +359,8 @@ NAME="name" -->"><!-- TMPL_VAR
NAME="name"--></label></td><td>
<!-- TMPL_IF NAME="execute" -->
<h1><!-- TMPL_VAR NAME="name" --></h1>
<p><!-- TMPL_VAR NAME="notes" --></p>
-<!-- TMPL_VAR name='pagination_bar'-->
+<!-- TMPL_IF name="pagination_bar" --><!-- TMPL_VAR
name='pagination_bar'--><!-- /TMPL_IF -->
+<!-- TMPL_UNLESS name="errors" -->
<table>
<!-- TMPL_LOOP NAME="results" -->
<!-- TMPL_VAR NAME="row" -->
@@ -376,6 +377,21 @@ NAME="name" -->"><!-- TMPL_VAR
NAME="name"--></label></td><td>
<input type="hidden" name="phase" value="Export" />
<input type="submit" name="submit" value="Download" /></fieldset>
</form>
+<!-- /TMPL_UNLESS -->
+<!-- TMPL_IF NAME="errors" -->
+<form action="/cgi-bin/koha/reports/guided_reports.pl" method="post">
+<div class="dialog alert">
+<b>The following error was encountered:</b><br />
+<!-- TMPL_LOOP name="errors" -->
+<!-- TMPL_IF NAME="sqlerr" -->This report contains the SQL keyword
<!-- TMPL_VAR name="sqlerr" -->.<br />Use of this keyword is not
allowed in Koha reports due to security and data integrity risks.<br
/>Please return to the "Saved Reports" screen and delete this report.
+<!-- TMPL_ELSIF NAME="queryerr" -->The database returned the
following error: <br /><!-- TMPL_VAR NAME="queryerr" --><br />Please
check the log for further details.
+<!-- /TMPL_IF -->
+<!-- /TMPL_LOOP -->
+</div>
+<fieldset class="action"><input type="hidden" name="phase"
value="Used saved" />
+<input type="submit" name="submit" value="Saved Reports" /></fieldset>
+</form>
+<!-- /TMPL_IF -->
<!-- /TMPL_IF -->
<!-- TMPL_IF NAME="create" -->
@@ -383,10 +399,10 @@ NAME="name" -->"><!-- TMPL_VAR
NAME="name"--></label></td><td>
<fieldset class="rows">
<legend>Create Report From SQL</legend>
<ol>
- <li><label for="reportname">Report Name:</label> <input
type="text" id="reportname" name="reportname" /> </li>
- <li><label for="notes">Notes:</label> <textarea id="notes"
name="notes" cols="50" rows="2"></textarea></li>
- <li><label for="types">Type:</label><select id="types"
name="types"><!-- TMPL_LOOP NAME="types" --><option value="<!--
TMPL_VAR NAME="id" -->"><!-- TMPL_VAR NAME="name"--></option><!--
/TMPL_LOOP --></select></li>
- <li><label for="sql">SQL: </label><textarea id="sql"
name="sql" cols="50" rows="10"></textarea></li>
+ <li><label for="reportname">Report Name:</label> <input
type="text" id="reportname" name="reportname"<!--TMPL_IF
NAME="reportname" --> value="<!-- TMPL_VAR NAME="reportname"-->"<!--
/TMPL_IF --> /> </li>
+ <li><label for="notes">Notes:</label> <textarea id="notes"
name="notes" cols="50" rows="2"><!--TMPL_IF NAME="notes" --><!--
TMPL_VAR NAME="notes"--><!-- /TMPL_IF --></textarea></li>
+ <li><label for="types">Type:</label><select id="types"
name="types"><!-- TMPL_LOOP NAME="types" --><option value="<!--
TMPL_VAR NAME="id" -->"<!-- TMPL_IF NAME="selected" -->
selected="selected"<!-- /TMPL_IF -->><!-- TMPL_VAR
NAME="name"--></option><!-- /TMPL_LOOP --></select></li>
+ <li><label for="sql">SQL: </label><textarea id="sql"
name="sql" cols="50" rows="10"><!--TMPL_IF NAME="sql" --><!-- TMPL_VAR
NAME="sql"--><!-- /TMPL_IF --></textarea></li>
</ol>
</fieldset>
@@ -437,6 +453,7 @@ Sub report:<select name="subreport">
<!-- /TMPL_IF -->
<!-- TMPL_IF NAME="save_successful" -->
+<!-- TMPL_UNLESS NAME="errors" -->
<h2>Your report has been saved</h2>
<p>The report you have created has now been saved. You can now</p>
<ul>
@@ -444,9 +461,25 @@ Sub report:<select name="subreport">
<li>Schedule this report to run using the: <a
href="/cgi-bin/koha/tools/scheduler.pl">Scheduler Tool</a></li>
<li>Return to: <a
href="/cgi-bin/koha/reports/guided_reports.pl?phase=Used+saved">Guided
Reports</a></li>
</ul>
-
-
-
+<!-- /TMPL_UNLESS -->
+<!-- TMPL_IF NAME="errors" -->
+<form action="/cgi-bin/koha/reports/guided_reports.pl" method="post">
+<div class="dialog alert">
+<b>The following error was encountered:</b><br />
+<!-- TMPL_LOOP name="errors" -->
+<!-- TMPL_IF NAME="sqlerr" -->The SQL statement contains the keyword
<!-- TMPL_VAR name="sqlerr" -->. Use of this keyword is not allowed in
Koha reports due to security and data integrity risks.
+<!-- TMPL_ELSIF NAME="queryerr" -->The SQL statment is missing the
SELECT keyword.<br />Please check the SQL statement syntax.
+<!-- /TMPL_IF -->
+<!-- /TMPL_LOOP -->
+</div>
+<input type="hidden" name="sql" value="<!-- TMPL_VAR NAME="sql" -->" />
+<input type="hidden" name="reportname" value="<!-- TMPL_VAR
NAME="reportname" -->" />
+<input type="hidden" name="type" value="<!-- TMPL_VAR NAME="type" -->" />
+<input type="hidden" name="notes" value="<!-- TMPL_VAR NAME="notes" -->" />
+<fieldset class="action"><input type="hidden" name="phase"
value="Create report from SQL" />
+<input type="submit" name="submit" value="Edit SQL" /></fieldset>
+</form>
+<!-- /TMPL_IF -->
<!-- /TMPL_IF -->
</div>
diff --git a/reports/guided_reports.pl b/reports/guided_reports.pl
index b7cb46d..9d80314 100755
--- a/reports/guided_reports.pl
+++ b/reports/guided_reports.pl
@@ -323,12 +323,34 @@ elsif ( $phase eq 'Save Report' ) {
# save the sql pasted in by a user
my $sql = $input->param('sql');
my $name = $input->param('reportname');
- my $type = $input->param('type');
- my $notes = $input->param('notes');
- save_report( $sql, $name, $type, $notes );
- $template->param(
- 'save_successful' => 1,
- );
+ my $type = $input->param('types');
+ my $notes = $input->param('notes');
+ my @errors = ();
+ my $error = {};
+ if ($sql =~ /;?\W?(UPDATE|DELETE|DROP|INSERT|SHOW|CREATE)\W/i) {
+ $error->{'sqlerr'} = $1;
+ push @errors, $error;
+ }
+ elsif ($sql !~ /^(SELECT)/i) {
+ $error->{'queryerr'} = 1;
+ push @errors, $error;
+ }
+ if (@errors) {
+ $template->param(
+ 'save_successful' => 1,
+ 'errors' => \@errors,
+ 'sql' => $sql,
+ 'reportname'=> $name,
+ 'type' => $type,
+ 'notes' => $notes,
+ );
+ }
+ else {
+ save_report( $sql, $name, $type, $notes );
+ $template->param(
+ 'save_successful' => 1,
+ );
+ }
}
# This condition is not used currently
@@ -336,7 +358,7 @@ elsif ( $phase eq 'Save Report' ) {
# # run the sql, and output results in a template
# my $sql = $input->param('sql');
# my $type = $input->param('type');
-# my $results = execute_query($sql, $type);
+# my ($results, $total, $errors) = execute_query($sql, $type);
# $template->param(
# 'results' => $results,
# 'sql' => $sql,
@@ -346,18 +368,19 @@ elsif ( $phase eq 'Save Report' ) {
elsif ($phase eq 'Run this report'){
# execute a saved report
- # FIXME: The default limit should not be hardcoded...
+ # FIXME The default limit should not be hardcoded...
my $limit = 20;
my $offset;
my $report = $input->param('reports');
# offset algorithm
if ($input->param('page')) {
$offset = ($input->param('page') - 1) * 20;
- } else {
+ }
+ else {
$offset = 0;
}
my ($sql,$type,$name,$notes) = get_saved_report($report);
- my ($results, $total) = execute_query($sql, $type, $offset, $limit);
+ my ($results, $total, $errors) = execute_query($sql, $type,
$offset, $limit);
my $totpages = int($total/$limit) + (($total % $limit) > 0 ? 1 : 0);
my $url = "/cgi-bin/koha/reports/guided_reports.pl?reports=$report&phase=Run%20this%20report";
$template->param(
@@ -367,25 +390,50 @@ elsif ($phase eq 'Run this report'){
'name' => $name,
'notes' => $notes,
'pagination_bar' => pagination_bar($url, $totpages,
$input->param('page'), "page"),
+ 'errors' => $errors,
);
}
elsif ($phase eq 'Export'){
# export results to tab separated text
- my $sql = $input->param('sql');
- $no_html=1;
- print $input->header( -type => 'application/octet-stream',
- -attachment=>'reportresults.csv');
- my $format=$input->param('format');
- my $results = execute_query($sql,1,0,0,$format);
- print $results;
-
+ my $sql = $input->param('sql');
+ my $format = $input->param('format');
+ my ($results, $total, $errors) = execute_query($sql,1,0,0,$format);
+ if (!defined($errors)) {
+ $no_html=1;
+ print $input->header( -type => 'application/octet-stream',
+ -attachment=>'reportresults.csv'
+ );
+ print $results;
+ } else {
+ $template->param(
+ 'results' => $results,
+ 'sql' => $sql,
+ 'execute' => 1,
+ 'name' => 'Error exporting report!',
+ 'notes' => '',
+ 'pagination_bar' => '',
+ 'errors' => $errors,
+ );
+ }
}
-elsif ($phase eq 'Create report from SQL'){
- # alllow the user to paste in sql
+elsif ($phase eq 'Create report from SQL') {
+ # allow the user to paste in sql
+ if ($input->param('sql')) {
+ $template->param(
+ 'sql' => $input->param('sql'),
+ 'reportname' => $input->param('reportname'),
+ 'notes' => $input->param('notes'),
+ );
+ }
$template->param('create' => 1);
- my $types = C4::Reports::get_report_types();
+ my $types = C4::Reports::get_report_types();
+ if (my $type = $input->param('type')) {
+ for my $i ( 0 .. $#{@$types}) {
+ @$types[$i]->{'selected'} = 1 if @$types[$i]->{'id'} eq $type;
+ }
+ }
$template->param( 'types' => $types );
}
--
1.5.6
More information about the Koha-patches
mailing list