From 0029619eb3838eb372eca87b8eb3e6ca1a3a4629 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 (cherry picked from commit 1ca9adaa56ff809a76ff903bb231175d0195163c) Signed-off-by: Chris Cormack Conflicts: koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-facets.inc --- 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 24abcb0963..b9c73ae467 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 6a6ba0576e..7d4e56efee 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 e1b69a5abb..cd60e8221a 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 | url %] - [% 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 692d70b938..daa38f6106 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.39.5