Koha-community
/
Koha
13
5
Fork 0
Code Issues Releases Wiki Activity
Browse Source

kohabug 2458 Disallowing non-SELECT SQL in reports module 

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: Joshua Ferraro <jmf@liblime.com>
3.0.x
Chris Nighswonger 16 years ago
committed by Joshua Ferraro
parent
482a87eaf9
commit
860f1f70e5
3 changed files with 224 additions and 115 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 200
      C4/Reports.pm
  2. 49
      koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tmpl
  3. 90
      reports/guided_reports.pl

200
C4/Reports.pm
View File

@ -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",@data);
$string .="\n" . $row;
if ($format eq 'csv') {
$string = join(",",@$colnames);
}
if ($format eq 'csv' ){
$row = join(",",@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",@data);
$string .="\n" . $row;
}
if ($format eq 'csv' ){
$row = join(",",@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;
}
}

49
koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tmpl
View File

@ -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>

90
reports/guided_reports.pl
View File

@ -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 );
}

Write Preview
Loading…
Cancel
Save