Bug 13789 - facets with accented utf-8 characters generate double encoded links
Summary: facets with accented utf-8 characters generate double encoded links
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Dobrica Pavlinusic
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 14955
  Show dependency treegraph
 
Reported: 2015-03-04 08:55 UTC by Dobrica Pavlinusic
Modified: 2016-06-21 21:40 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13789 - facets with accented utf-8 characters generate double encoded links (3.20 KB, patch)
2015-03-04 10:42 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 13789 - facets with accented utf-8 characters generate double encoded links (3.81 KB, patch)
2015-03-04 13:32 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 13789 - facets with accented utf-8 characters generate double encoded links (3.74 KB, patch)
2015-03-04 15:52 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 13789 - facets with accented utf-8 characters generate double encoded links (8.28 KB, patch)
2015-03-05 09:07 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 13789 - facets with accented utf-8 characters generate double encoded links (8.28 KB, patch)
2015-03-05 14:55 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 13789 - facets with accented utf-8 characters generate double encoded links (8.36 KB, patch)
2015-03-05 15:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 13789 - facets with accented utf-8 characters generate double encoded links (8.43 KB, patch)
2015-03-06 07:11 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Dobrica Pavlinusic 2015-03-04 08:55:25 UTC

    
Comment 1 Dobrica Pavlinusic 2015-03-04 10:42:03 UTC Comment hidden (obsolete)
Comment 2 Dobrica Pavlinusic 2015-03-04 13:32:01 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-03-04 14:57:16 UTC
Sorry Dobrica, I found an issue :-/
The encoding problem is fixed for the facet lib, but try to click on one facet, it should be selected (i.e. not a link and with the [x] to remove it), and it's not.
It looks like there is another encoding issue somewhere else...
Comment 4 Dobrica Pavlinusic 2015-03-04 15:52:55 UTC Comment hidden (obsolete)
Comment 5 Dobrica Pavlinusic 2015-03-05 09:07:35 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2015-03-05 14:30:27 UTC
Sorry, failing qa again.
The title of the [X] link (to delete the facet) is badly encoded.
Comment 7 Dobrica Pavlinusic 2015-03-05 14:55:14 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2015-03-05 15:16:15 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-03-05 15:28:56 UTC
Bumping the severity since it's a regression.
Comment 10 Katrin Fischer 2015-03-06 07:11:58 UTC
Created attachment 36638 [details] [review]
[PASSED QA] 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='"><script>prompt('Happy_Holidays')</script>&limit=123
   and verifying that you DON'T see prompt window in your browser

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 11 Tomás Cohen Arazi 2015-03-07 21:11:02 UTC
Patch pushed to master.

Thanks Dobrica!
Comment 12 Chris Cormack 2015-03-12 07:26:18 UTC
Pushed to 3.18.x will be in 3.18.5
Comment 13 Jonathan Druart 2015-03-12 07:57:48 UTC
(In reply to Chris Cormack from comment #12)
> Pushed to 3.18.x will be in 3.18.5

It introduces regression in 3.18.x, the facet is not selected anymore.
To reproduce:
- At the opac, search for something
- click on the author facet '<a>Goupy, Xavier</a>'
- the facet is correctly selected (Goupy, Xavier [<a>x</a>])
- search for something again
- click on the author facet '<a>Leblanc, Stéphane</a>'
- the facet is not correctly selected (<a>Leblanc, Stéphane</a>)

Note: Without this patch, the only issue on facet is the title of the [x] link (<a title="Remove facet Leblanc%2C%20St%C3%A9phane">x</a>)
Comment 14 Chris Cormack 2015-03-12 08:01:33 UTC
Good catch, reverted