From 1ca9adaa56ff809a76ff903bb231175d0195163c Mon Sep 17 00:00:00 2001 From: Dobrica Pavlinusic Date: Wed, 4 Mar 2015 11:38:42 +0100 Subject: [PATCH] Bug 13789 - facets with accented utf-8 characters generate double encoded links Bug 13425 tried to fix XSS in OPAC, by using url filter in template toolkit on whole generated url. This doesn't work and create double encoded strings in facets because we are creating url variable by concatenating query_cgi (which did pass through uri_escape_utf8 on perl side) and other parameters which have to be escaped in template. Also, code like [% SET limit_cgi_f = limit_cgi | url %] doesn't do anything (at least doesn't apply url filter) so it's not needed. This patch also fixes encoding of hidden fields used in sort by form. And lastly, it tries to make facet changes for opac and intranet as same as possible to simplify future maintencence of this code. Test scenario: 1. find results in your opac which contain accented characters 2. click on them and verify that results are missing 3. apply this patch 4. re-run search and click on facets link verifying that there are now results 5. test sort by form and verify that results are ok 6. verify that facets are still safe from injection by constructing url like /cgi-bin/koha/opac-search.pl?q=123&sort_by='">&limit=123 and verifying that you DON'T see prompt window in your browser Signed-off-by: Jonathan Druart Signed-off-by: Katrin Fischer Signed-off-by: Tomas Cohen Arazi --- catalogue/search.pl | 5 ++--- .../intranet-tmpl/prog/en/includes/facets.inc | 14 +++++++------- .../bootstrap/en/includes/opac-facets.inc | 11 ++++------- opac/opac-search.pl | 3 +-- 4 files changed, 14 insertions(+), 19 deletions(-) diff --git a/catalogue/search.pl b/catalogue/search.pl index d25e19e1c1..1309563369 100755 --- a/catalogue/search.pl +++ b/catalogue/search.pl @@ -519,7 +519,7 @@ for my $this_cgi ( split('&',$limit_cgi) ) { my $input_name = $1; my $input_value = $2; $input_name =~ s/=$//; - push @limit_inputs, { input_name => $input_name, input_value => uri_unescape($input_value) }; + push @limit_inputs, { input_name => $input_name, input_value => Encode::decode_utf8( uri_unescape($input_value) ) }; } $template->param ( LIMIT_INPUTS => \@limit_inputs ); @@ -723,12 +723,11 @@ my $gotopage = $cgi->param('gotoPage'); $template->{'VARS'}->{'gotoPage'} = $gotopage if $gotopage =~ m/^(ISBD|labeledMARC|MARC|more)?detail.pl$/; -my @input_values = map { Encode::decode_utf8($_->{input_value}) } @limit_inputs; for my $facet ( @$facets ) { for my $entry ( @{ $facet->{facets} } ) { my $index = $entry->{type_link_value}; my $value = $entry->{facet_link_value}; - $entry->{active} = grep { $_ eq qq{$index:$value} } @input_values; + $entry->{active} = grep { $_->{input_value} eq qq{$index:$value} } @limit_inputs; } } diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/facets.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/facets.inc index 96691fe03b..157366e032 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/facets.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/facets.inc @@ -23,17 +23,14 @@
    [% FOREACH facet IN facets_loo.facets %]
  • - [% SET query_cgi_f = query_cgi | html %] - [% SET limit_cgi_f = limit_cgi | html %] - [% SET url = "/cgi-bin/koha/catalogue/search.pl?" _ query_cgi_f _ limit_cgi_f %] + [% SET url = "/cgi-bin/koha/catalogue/search.pl?" _ query_cgi _ limit_cgi %] [% IF ( sort_by ) %] - [% SET url = url _ "&sort_by=" _ sort_by %] + [% url = BLOCK %][% url %][% "&sort_by=" _ sort_by |url %][% END %] [% END %] - [% facet.facet_link_value = BLOCK %][% facet.facet_link_value | uri %][% END %] [% IF facet.active %] [% SET url = url _ "&nolimit=" _ facet.type_link_value _ ":" _ facet.facet_link_value %] [% facet.facet_label_value %] - [x] + [x] [% ELSE %] [% SET url = url _ "&limit=" _ facet.type_link_value _ ":" _ facet.facet_link_value %] [% facet.facet_label_value %] @@ -44,7 +41,10 @@
    • [% END %] [% IF ( facets_loo.expandable ) %] -
  • Show more
    • +
  • + Show more +
    • [% END %]
[% END %] diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-facets.inc b/koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-facets.inc index 6e846f0185..958d5716ca 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-facets.inc +++ b/koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-facets.inc @@ -32,20 +32,17 @@
    [% FOREACH facet IN facets_loo.facets %]
  • - [% SET query_cgi_f = query_cgi %] - [% SET limit_cgi_f = limit_cgi | url %] - [% SET url = "/cgi-bin/koha/opac-search.pl?" _ query_cgi_f _ limit_cgi_f %] + [% SET url = "/cgi-bin/koha/opac-search.pl?" _ query_cgi _ limit_cgi %] [% IF ( sort_by ) %] - [% SET url = url _ "&sort_by=" _ sort_by |url %] + [% url = BLOCK %][% url %][% "&sort_by=" _ sort_by |url %][% END %] [% END %] - [% facet.facet_link_value = BLOCK %][% facet.facet_link_value | uri %][% END %] [% IF facet.active %] [% SET url = url _ "&nolimit=" _ facet.type_link_value _ ":" _ facet.facet_link_value %] [% facet.facet_label_value %] - [x] + [x] [% ELSE %] [% SET url = url _ "&limit=" _ facet.type_link_value _ ":" _ facet.facet_link_value %] - [% facet.facet_label_value %] + [% facet.facet_label_value %] [% IF ( displayFacetCount ) %] ([% facet.facet_count %]) [% END %] diff --git a/opac/opac-search.pl b/opac/opac-search.pl index b0feaeb8cd..02038cc5af 100755 --- a/opac/opac-search.pl +++ b/opac/opac-search.pl @@ -872,12 +872,11 @@ for (my $i=0;$i<@servers;$i++) { } #/end of the for loop #$template->param(FEDERATED_RESULTS => \@results_array); -my @input_values = map { Encode::decode_utf8($_->{input_value}) } @limit_inputs; for my $facet ( @$facets ) { for my $entry ( @{ $facet->{facets} } ) { my $index = $entry->{type_link_value}; my $value = $entry->{facet_link_value}; - $entry->{active} = grep { $_ eq qq{$index:$value} } @input_values; + $entry->{active} = grep { $_->{input_value} eq qq{$index:$value} } @limit_inputs; } } -- 2.20.1