Bug 9239 QA follow-up: escape CGI input

Koha was not previously escaping CGI input, which caused problems for
highlighting and is a security issue.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Thx for fixing this.
Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>

diff --git a/C4/Search.pm b/C4/Search.pm
index 6772ce2..1a1e2cf 100644 (file)
--- a/C4/Search.pm
+++ b/C4/Search.pm
@@ -1194,11 +1194,16 @@ sub parseQuery {
         }
         foreach my $limit (@limits) {
         }
-        foreach my $modifier (@sort_by) {
-            $query .= " #$modifier";
+        if (scalar (@sort_by) > 0) {
+            my $modifier_re = '#(' . join( '|', @{$QParser->modifiers}) . ')';
+            $query =~ s/$modifier_re//g;
+            foreach my $modifier (@sort_by) {
+                $query .= " #$modifier";
+            }
         }
 
         $query_desc = $query;
+        $query_desc =~ s/\s+/ /g;
         if ( C4::Context->preference("QueryWeightFields") ) {
         }
         $QParser->add_bib1_filter_map( 'biblioserver', 'su-br', { 'callback' => \&_handle_exploding_index });
@@ -1206,9 +1211,9 @@ sub parseQuery {
         $QParser->add_bib1_filter_map( 'biblioserver', 'su-rl', { 'callback' => \&_handle_exploding_index });
         $QParser->parse( $query );
         $operands[0] = "pqf=" . $QParser->target_syntax('biblioserver');
-# TODO: once we are using QueryParser, all this special case code for
-#       exploded search indexes will be replaced by a callback to
-#       _handle_exploding_index
+    } else {
+        my $modifier_re = '#(' . join( '|', @{Koha::QueryParser::Driver::PQF->modifiers}) . ')';
+        s/$modifier_re//g for @operands;
     }
 
     return ( $operators, \@operands, $indexes, $limits, $sort_by, $scan, $lang, $query_desc);
@@ -1292,17 +1297,17 @@ sub buildQuery {
         if ( @limits ) {
             $q .= ' and '.join(' and ', @limits);
         }
-        return ( undef, $q, $q, "q=ccl=$q", $q, '', '', '', '', 'ccl' );
+        return ( undef, $q, $q, "q=ccl=".uri_escape($q), $q, '', '', '', '', 'ccl' );
     }
     if ( $query =~ /^cql=/ ) {
-        return ( undef, $', $', "q=cql=$'", $', '', '', '', '', 'cql' );
+        return ( undef, $', $', "q=cql=".uri_escape($'), $', '', '', '', '', 'cql' );
     }
     if ( $query =~ /^pqf=/ ) {
         if ($query_desc) {
-            $query_cgi = "q=$query_desc";
+            $query_cgi = "q=".uri_escape($query_desc);
         } else {
             $query_desc = $';
-            $query_cgi = "q=pqf=$'";
+            $query_cgi = "q=pqf=".uri_escape($');
         }
         return ( undef, $', $', $query_cgi, $query_desc, '', '', '', '', 'pqf' );
     }
@@ -1474,9 +1479,9 @@ sub buildQuery {
                         $query     .= " $operators[$i-1] ";
                         $query     .= " $index_plus " unless $indexes_set;
                         $query     .= " $operand";
-                        $query_cgi .= "&op=$operators[$i-1]";
-                        $query_cgi .= "&idx=$index" if $index;
-                        $query_cgi .= "&q=$operands[$i]" if $operands[$i];
+                        $query_cgi .= "&op=".uri_escape($operators[$i-1]);
+                        $query_cgi .= "&idx=".uri_escape($index) if $index;
+                        $query_cgi .= "&q=".uri_escape($operands[$i]) if $operands[$i];
                         $query_desc .=
                           " $operators[$i-1] $index_plus $operands[$i]";
                     }
@@ -1486,8 +1491,8 @@ sub buildQuery {
                         $query      .= " and ";
                         $query      .= "$index_plus " unless $indexes_set;
                         $query      .= "$operand";
-                        $query_cgi  .= "&op=and&idx=$index" if $index;
-                        $query_cgi  .= "&q=$operands[$i]" if $operands[$i];
+                        $query_cgi  .= "&op=and&idx=".uri_escape($index) if $index;
+                        $query_cgi  .= "&q=".uri_escape($operands[$i]) if $operands[$i];
                         $query_desc .= " and $index_plus $operands[$i]";
                     }
                 }
@@ -1499,8 +1504,8 @@ sub buildQuery {
                     $query .= " $index_plus " unless $indexes_set;
                     $query .= $operand;
                     $query_desc .= " $index_plus $operands[$i]";
-                    $query_cgi  .= "&idx=$index" if $index;
-                    $query_cgi  .= "&q=$operands[$i]" if $operands[$i];
+                    $query_cgi  .= "&idx=".uri_escape($index) if $index;
+                    $query_cgi  .= "&q=".uri_escape($operands[$i]) if $operands[$i];
                     $previous_operand = 1;
                 }
             }    #/if $operands

diff --git a/opac/opac-search.pl b/opac/opac-search.pl
index 6be3333..d2ba9fc 100755 (executable)
--- a/opac/opac-search.pl
+++ b/opac/opac-search.pl
@@ -358,10 +358,12 @@ unless (@servers) {
 # operators include boolean and proximity operators and are used
 # to evaluate multiple operands
 my @operators = $cgi->param('op');
+@operators = map { uri_unescape($_) } @operators;
 
 # indexes are query qualifiers, like 'title', 'author', etc. They
 # can be single or multiple parameters separated by comma: kw,right-Truncation 
 my @indexes = $cgi->param('idx');
+@indexes = map { uri_unescape($_) } @indexes;
 
 # if a simple index (only one)  display the index used in the top search box
 if ($indexes[0] && !$indexes[1]) {
@@ -369,16 +371,20 @@ if ($indexes[0] && !$indexes[1]) {
 }
 # an operand can be a single term, a phrase, or a complete ccl query
 my @operands = $cgi->param('q');
+@operands = map { uri_unescape($_) } @operands;
 
 $template->{VARS}->{querystring} = join(' ', @operands);
 
 # if a simple search, display the value in the search box
 if ($operands[0] && !$operands[1]) {
-    $template->param(ms_value => $operands[0]);
+    my $ms_query = $operands[0];
+    $ms_query =~ s/ #\S+//;
+    $template->param(ms_value => $ms_query);
 }
 
 # limits are use to limit to results to a pre-defined category such as branch or language
 my @limits = $cgi->param('limit');
+@limits = map { uri_unescape($_) } @limits;
 
 if($params->{'multibranchlimit'}) {
     push @limits, '('.join( " or ", map { "branch: $_ " } @{ GetBranchesInCategory( $params->{'multibranchlimit'} ) } ).')';