Bug 21526 - TT variables used to build a link should be uri filtered
Summary: TT variables used to build a link should be uri filtered
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low blocker (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
: 13812 (view as bug list)
Depends on: 13618 21576
Blocks: 22223 21706 21878 21950 21973 23132 23378 24120
  Show dependency treegraph
 
Reported: 2018-10-09 21:28 UTC by Jonathan Druart
Modified: 2020-11-30 21:44 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21526: uri escape TT variables used to build a link (708.60 KB, patch)
2018-10-10 12:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21526: uri escape TT variables when used in 'a href' (708.79 KB, patch)
2018-10-15 20:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21526: uri escape TT variables when used in 'a href' (655.95 KB, patch)
2018-10-15 22:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21526: Fix html filters (new pushes) (8.48 KB, patch)
2018-10-16 15:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21526: Remove duplicated USE raw statement (766 bytes, patch)
2018-10-17 14:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21526: uri escape TT variables when used in 'a href' (655.96 KB, patch)
2018-10-17 19:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21526: Remove duplicated USE raw statement (766 bytes, patch)
2018-10-17 19:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21526: uri escape TT variables when used in 'a href' (652.85 KB, patch)
2018-10-24 11:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21526: Remove duplicated USE raw statement (832 bytes, patch)
2018-10-24 11:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PUSHED] Bug 21526: uri escape TT variables when used in 'a href' (654.10 KB, patch)
2018-10-26 14:47 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PUSHED] Bug 21526: Remove duplicated USE raw statement (832 bytes, patch)
2018-10-26 14:47 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PUSHED] Bug 21526: (QA follow-up) Fix all the tabs (11.19 KB, patch)
2018-10-26 14:47 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PUSHED] Bug 21526: (RM follow-up) Fix merge markers and a few extra filters (6.78 KB, patch)
2018-10-26 19:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21526: Use the 'url' filter when needed (108.61 KB, patch)
2018-10-29 19:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21526: (follow-up) Use the 'url' filter when needed (3.20 KB, patch)
2018-10-31 17:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21526: Fix search result pages (url vs uri vs raw) (21.64 KB, patch)
2018-10-31 17:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21526: Use the 'url' filter when needed (108.51 KB, patch)
2018-11-06 22:59 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 21526: (follow-up) Use the 'url' filter when needed (3.25 KB, patch)
2018-11-06 22:59 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 21526: Fix search result pages (url vs uri vs raw) (21.67 KB, patch)
2018-11-06 22:59 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 21526: Use the 'url' filter when needed (108.57 KB, patch)
2018-11-09 10:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21526: (follow-up) Use the 'url' filter when needed (3.33 KB, patch)
2018-11-09 10:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21526: Fix search result pages (url vs uri vs raw) (21.77 KB, patch)
2018-11-09 10:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21526 - To fix pagination problem in OPAC (3.45 KB, patch)
2018-11-23 09:29 UTC, Devinim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2018-10-09 21:28:17 UTC
and not html filtered, see the wiki page for more details

https://wiki.koha-community.org/wiki/Coding_Guidelines#HTML9:_filter_all_the_variables
Comment 1 Jonathan Druart 2018-10-10 12:46:50 UTC
Created attachment 80337 [details] [review]
Bug 21526: uri escape TT variables used to build a link
Comment 2 Jonathan Druart 2018-10-15 20:21:08 UTC
Created attachment 80641 [details] [review]
Bug 21526: uri escape TT variables when used in 'a href'

This patch has been generated with the script provided on bug 21576.
It only affects variable used in the href attribute of a link *when*
href it the first attribute of the node (grep "a href")
Comment 3 Jonathan Druart 2018-10-15 22:52:41 UTC
Created attachment 80649 [details] [review]
Bug 21526: uri escape TT variables when used in 'a href'

This patch has been generated with the script provided on bug 21576.
It only affects variable used in the href attribute of a link *when*
href it the first attribute of the node (grep "a href")
Comment 4 Jonathan Druart 2018-10-16 15:27:35 UTC
Created attachment 80661 [details] [review]
Bug 21526: Fix html filters (new pushes)
Comment 5 Martin Renvoize 2018-10-17 11:30:53 UTC
Comment on attachment 80649 [details] [review]
Bug 21526: uri escape TT variables when used in 'a href'

Review of attachment 80649 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/includes/patron-title.inc
@@ +1,1 @@
> +[% USE raw %]

Hmm.. I wasn't aware of the differing syntax available here.. where's the difference between [% USE raw %] and [%- USE raw -%] if any?
Comment 6 Julian Maurice 2018-10-17 11:39:23 UTC
(In reply to Martin Renvoize from comment #5)
> Hmm.. I wasn't aware of the differing syntax available here.. where's the
> difference between [% USE raw %] and [%- USE raw -%] if any?
It changes how whitespaces are chomped. See http://template-toolkit.org/docs/manual/Syntax.html#section_Chomping_Whitespace and http://template-toolkit.org/docs/manual/Config.html#section_PRE_CHOMP_POST_CHOMP
Comment 7 Jonathan Druart 2018-10-17 14:08:17 UTC
And so you found a bug, congrats :)

+[% USE raw %]
 [%- USE raw -%]

=> This is wrong, the test to know if the USE statement exist does not the chomp chars (- ~) into account
Comment 8 Jonathan Druart 2018-10-17 14:34:48 UTC
Created attachment 80725 [details] [review]
Bug 21526: Remove duplicated USE raw statement

See patch on bug 21576 comment 14
Comment 9 Jonathan Druart 2018-10-17 19:48:12 UTC
Created attachment 80755 [details] [review]
Bug 21526: uri escape TT variables when used in 'a href'

This patch has been generated with the script provided on bug 21576.
It only affects variable used in the href attribute of a link *when*
href it the first attribute of the node (grep "a href")
Comment 10 Jonathan Druart 2018-10-17 19:48:18 UTC
Created attachment 80756 [details] [review]
Bug 21526: Remove duplicated USE raw statement

See patch on bug 21576 comment 14
Comment 11 Martin Renvoize 2018-10-24 11:29:46 UTC
Created attachment 81058 [details] [review]
Bug 21526: uri escape TT variables when used in 'a href'

This patch has been generated with the script provided on bug 21576.
It only affects variable used in the href attribute of a link *when*
href it the first attribute of the node (grep "a href")

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2018-10-24 11:29:50 UTC
Created attachment 81059 [details] [review]
Bug 21526: Remove duplicated USE raw statement

See patch on bug 21576 comment 14

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2018-10-24 11:30:22 UTC
Looking good, can't find any regressions in testing :)
Comment 14 Katrin Fischer 2018-10-26 14:47:08 UTC
Created attachment 81300 [details] [review]
[PUSHED] Bug 21526: uri escape TT variables when used in 'a href'

This patch has been generated with the script provided on bug 21576.
It only affects variable used in the href attribute of a link *when*
href it the first attribute of the node (grep "a href")

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Katrin Fischer 2018-10-26 14:47:12 UTC
Created attachment 81301 [details] [review]
[PUSHED] Bug 21526: Remove duplicated USE raw statement

See patch on bug 21576 comment 14

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Katrin Fischer 2018-10-26 14:47:16 UTC
Created attachment 81302 [details] [review]
[PUSHED] Bug 21526: (QA follow-up) Fix all the tabs
Comment 17 Nick Clemens 2018-10-26 17:16:10 UTC
Awesome work all!

Pushed to master for 18.11
Comment 18 Nick Clemens 2018-10-26 19:45:45 UTC
Created attachment 81337 [details] [review]
[PUSHED] Bug 21526: (RM follow-up) Fix merge markers and a few extra filters

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Jonathan Druart 2018-10-29 19:16:44 UTC
Created attachment 81530 [details] [review]
Bug 21526: Use the 'url' filter when needed
Comment 20 Jonathan Druart 2018-10-29 20:57:28 UTC
(In reply to Jonathan Druart from comment #19)
> Created attachment 81530 [details] [review] [review]
> Bug 21526: Use the 'url' filter when needed

This will break/not fix the facets and other links from the result page.
Comment 21 Mark Tompsett 2018-10-30 00:52:01 UTC
Comment on attachment 81530 [details] [review]
Bug 21526: Use the 'url' filter when needed

Review of attachment 81530 [details] [review]:
-----------------------------------------------------------------

Generally, if there was no comment, I don't think there was a problem, OR... I commented about a problem elsewhere, and didn't feel like repeating it over and over. Here's some feedback. There ARE some Failed QA issues.

::: koha-tmpl/intranet-tmpl/prog/en/includes/authorities-search-results.inc
@@ +10,4 @@
>  
>      [% IF marcflavour == 'UNIMARC' %]
>          [% IF authid %]
> +            [% link = BLOCK %]<a href="[% authidurl _ authid | url %]">[% heading | html %]</a>[% END %]

Perhaps this line and line 31 should use the same method to build the URL? I tend to prefer 31's split.

@@ +28,4 @@
>          <span class="heading">
>          [% IF ( linkType=='seealso' ) %]
>              [% IF ( authid ) %]
> +            <a href="[% authidurl | url %][% authid | uri %]">[% heading | html %]</a>

Nice catch on the URL vs URI. But I think URI should be used for the base part, and URL for anything we are adding to it, unless there is a bizarre & at the end or something.

::: koha-tmpl/intranet-tmpl/prog/en/includes/facets.inc
@@ +33,4 @@
>        [% IF facet.active %]
>          [% SET local_url = url _ "&amp;nolimit=" _ facet.type_link_value _ ":" _ facet.facet_link_value %]
>          <span class="facet-label">[% facet.facet_label_value | html %]</span>
> +        [<a href="[% local_url | url %]" title="Remove facet [% facet.facet_link_value | html %]">x</a>]

Yes. Because local_url is the WHOLE url, the url filter is right.

@@ +45,4 @@
>    [% END %]
>    [% IF ( facets_loo.expandable ) %]
>        <li class="showmore">
> +          <a href="[% url | url %][% IF offset %]&amp;offset=[% offset |url %][% END %]&amp;expand=[% facets_loo.expand |url %]#[% facets_loo.type_id |url %]">Show more</a>

If url is more than the base url, then url is correct, but the others are single values, so I would think it is uri.

::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/addorderiso2709.tt
@@ +413,4 @@
>                          </td>
>                          <td><span title="[% batch_lis.staged_date | html %]">[% batch_lis.staged_date | $KohaDates with_hours => 1 | html %]</span></td>
>                          <td>[% batch_lis.num_records | html %]</td>
> +                        <td><a href="[% batch_lis.scriptname | url %]?import_batch_id=[% batch_lis.import_batch_id | uri %]&amp;basketno=[% basketno | uri %]&amp;booksellerid=[% booksellerid | uri %]">Add orders</a></td>

Nice correction of the uri to url. Also, see the use of uri here for the token-y parts? That's what I would expect.

::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
@@ +2,4 @@
>  [% USE Asset %]
>  [% BLOCK csv_export %]
>      <div class="btn-group">
> +        <a id="exportbutton" class="btn btn-default btn-sm" href="[% script_name | url %]?op=export&amp;basketno=[% basketno | html %]&amp;booksellerid=[% booksellerid | html %]"><i class="fa fa-download"></i> Export as CSV</a>

Okay, so should the token-y parts be uri or html? Hmmm... html, to prevent security attacks? Perhaps all the tiny parts should be html. So far, it hasn't been consisten.

::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/supplier.tt
@@ +264,4 @@
>                  <p><span class="label">Phone: </span>[% phone | html %]</p>
>                  <p><span class="label">Fax: </span>[% fax | html %]</p>
>                  [% IF ( url ) %]
> +                    <p><span class="label">Website: </span><a href="[% url | url %]">[% url | html %]</a></p>

Right, the url filter for a full URL, and the html filter to display it without running any sort of weird scripting security side effects.

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/marctagstructure.tt
@@ +96,4 @@
>  </ol></fieldset> 
>      <fieldset class="action">
>          <input type="submit" value="Save changes" />
> +        <a class="cancel" href="[% script_name | url %]?frameworkcode=[% framework.frameworkcode | html %]">Cancel</a>

missing changing an html to uri.

::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/printslip.tt
@@ +13,4 @@
>  [% END %]
>  [% INCLUDE 'doc-head-close.inc' %]
>  <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
> +<link rel="shortcut icon" href="[% IF ( IntranetFavicon ) %][% IntranetFavicon | %][% ELSE %][% interface | html %]/[% theme | html %]/img/favicon.ico[% END %]" type="image/x-icon" />

BAD FILTER! IntranetFavicon missing a filter name.
If interface is a url, then html is the wrong filter.
theme probably should be uri.

::: koha-tmpl/intranet-tmpl/prog/en/modules/suggestion/suggestion.tt
@@ +405,4 @@
>  			<input type="text" readonly="readonly" id="total" name="total" size="10" value="[% total | html %]"/>
>                  </li></ol>
>      </fieldset><input type="hidden" id="returnsuggested" name="returnsuggested" value="[% IF ( returnsuggestedby ) %][% returnsuggestedby | html %][% ELSE %]noone[% END %]"/>
> +    <fieldset class="action"><input type="hidden" name="op" value="[% op | html %]" />[% IF ( suggestionid ) %]<input type="submit" value="Save" /> <a class="cancel" href="[% IF ( returnsuggestedby ) %]/cgi-bin/koha/members/moremember.pl?borrowernumber=[% returnsuggestedby | uri %]#suggestions[% ELSE %]suggestion.pl[% END %]">Cancel</a>[% ELSE %]<input type="submit" value="Submit your suggestion" /> <a class="cancel" href="suggestion.pl">Cancel</a>[% END %]

Nice correction of html to uri in the url build part, but the hidden value's value... perhaps uri? After all, it's like the portion of a long GET request, not a string to display.

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-basket.tt
@@ +201,4 @@
>                                                          [% IF MARCurl.part %]<p>[% MARCurl.part | html %]</p>[% END %]
>  
>                                                          [% IF OPACURLOpenInNewWindow %]
> +                                                            <a href="[% MARCurl.MARCURL | url %]" title="[% MARCurl.MARCURL | html %]" target="_blank" rel="noreferrer">[% MARCurl.linktext | html %]</a>

Should note that I agree that title should use html filter, because it's just a tooltip, not actually a link value.

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt
@@ +337,4 @@
>  
>                                          [% IF ( OPACURLOpenInNewWindow ) %]
>                                              [% IF trackclicks == 'track' || trackclicks == 'anonymous' %]
> +                                                <a href="/cgi-bin/koha/tracklinks.pl?uri=[% MARCurl.MARCURL | uri %]&amp;biblionumber=[% biblio.biblionumber | uri %]" title="[% MARCurl.MARCURL | html %]" target="_blank" rel="noreferrer">

Yes, uri filter, because it is token-y. uri=<url thing>.

@@ +341,2 @@
>                                              [% ELSE %]
> +                                                <a href="[% MARCurl.MARCURL | url %]" title="[% MARCurl.MARCURL | html %]" target="_blank" rel="noreferrer">

Whereas, this is the URL to use.

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt
@@ +71,4 @@
>              </div>
>              <div id="rssnews-container">
>                  <!-- Logged in users have a branch code or it could be explicitly set -->
> +                <a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-news-rss.pl?branchcode=[% branchcode | uri %]"><img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png"></a>

The img src url.... is interface url? is theme just a piece of the url? Something feels wrong about this, but I could be wrong.

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-opensearch.tt
@@ +20,4 @@
>     <channel>
>       <title><![CDATA[[% LibraryName | html %] Search [% IF ( query_desc ) %]for '[% query_desc | html %]'[% END %][% IF ( limit_desc ) %] with limit(s): '[% limit_desc | html %]'[% END %]]]></title>
>       <link>[% OPACBaseURL | html %]/cgi-bin/koha/opac-search.pl?[% query_cgi | html %][% limit_cgi | html %]&amp;format=rss2</link>
> +     <atom:link rel="self" type="application/rss+xml" href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi |uri %][% limit_cgi |uri %]&amp;sort_by=[% sort_by | uri %]&amp;format=rss2"/>

It's weird that the query_cgi and limit_cgi lack an &amp; here. Something feels weird, but no sense busting it.

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt
@@ +18,4 @@
>      [% END %]
>  [% END %]
>  
> +<link rel="alternate" type="application/rss+xml" title="[% LibraryName | html %] Search RSS feed" href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi | url %][% limit_cgi |url %]&amp;count=[% countrss |uri %]&amp;sort_by=acqdate_dsc&amp;format=rss2" />

opac-opensearch.tt had uri and uri on the query and limit. But like I said, this lack of &amp; feels weird.

@@ +43,4 @@
>                          <strong>No results found!</strong>
>                          <p>
>                              [% IF ( searchdesc ) %]
> +                                No results found for that in [% LibraryName | html %] catalog. <a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi | url %][% limit_cgi | url %]&amp;format=rss2" class="rsssearchlink noprint"><img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png" alt="Subscribe to this search" title="Subscribe to this search" border="0" class="rsssearchicon"/></a>

But like I said before, I'm not sure url is the correct filter. There's some lack of consistency on how to treat them.

@@ -43,4 @@
>                          <strong>No results found!</strong>
>                          <p>
>                              [% IF ( searchdesc ) %]
> -                                No results found for that in [% LibraryName | html %] catalog. <a href="[% OPACBaseURL | uri %]/cgi-bin/koha/opac-search.pl?[% query_cgi | url %][% limit_cgi | html | url %]&amp;format=rss2" class="rsssearchlink noprint"><img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png" alt="Subscribe to this search" title="Subscribe to this search" border="0" class="rsssearchicon"/></a>

| html | url?! What was that?! That was clearly wrong. :)

@@ +87,4 @@
>                                  [% END %]
>                              ).
>                          [% END %]
> +                        <a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi | url %][% limit_cgi | url %]&amp;count=[% countrss | uri %]&amp;sort_by=acqdate_dsc&amp;format=rss2" class="rsssearchlink noprint"><img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png" alt="Subscribe to this search" title="Subscribe to this search" class="rsssearchicon"/></a>

On a positive note, it should be OPACBaseURL | url. :)

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-search-history.tt
@@ +71,4 @@
>                                                      <tr>
>                                                          <td><input type="checkbox" name="id" value="[% s.id | html %]" /></td>
>                                                          <td><span title="[% s.time | html %]">[% s.time |$KohaDates with_hours => 1 | html %]</span></td>
> +                                                        <td><a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi |url %][% limit_cgi | url %]&amp;[% s.query_cgi | uri %]&amp;count=[% countrss | uri %]&amp;sort_by=acqdate_dsc&amp;format=rss2" class="rsssearchlink"><img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png" alt="Subscribe to this search" title="Subscribe to this search" class="rsssearchicon"/></a> <a href="/cgi-bin/koha/opac-search.pl?[% s.query_cgi | html %]">[% s.query_desc | html %]</a></td>

So why is s.queru_cgi merit a uri, but query_cgi a url? What is the weirdness with query_cgi? Does it have an &? -- Yuck. The s.query_cgi near the end is still through | html even though it is part of an href.

@@ +111,4 @@
>                                                  <tr>
>                                                      <td><input type="checkbox" name="id" value="[% s.id | html %]" /></td>
>                                                      <td><span title="[% s.time | html %]">[% s.time |$KohaDates with_hours => 1 | html %]</span></td>
> +                                                    <td><a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi |url %][% limit_cgi | url %]&amp;[% s.query_cgi | uri %]&amp;count=[% countrss | uri %]&amp;sort_by=acqdate_dsc&amp;format=rss2" class="rsssearchlink"><img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png" alt="Subscribe to this search" title="Subscribe to this search" class="rsssearchicon"/></a> <a href="/cgi-bin/koha/opac-search.pl?[% s.query_cgi | html %]">[% s.query_desc | html %]</a></td>

Same yuckiness here.

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-showreviews.tt
@@ +29,4 @@
>                      <div class="span12">
>                  [% END %]
>                     <div id="showreviews" class="searchresults maincontent">
> +                        <h3>Recent comments <a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-showreviews.pl?format=rss" class="rsssearchlink"><img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png" alt="Subscribe to recent comments" title="Subscribe to recent comments" border="0" class="rsssearchicon"/></a></h3>

I know I've talked about interface before, but can someone clarify what values I might expect, so I can think about whether that filter is right. Same with theme.

::: misc/cronjobs/rss/lastAcquired-1.0.tt
@@ +32,4 @@
>  <br>[% place | html %] [% i.publishercode | html %] [% i.publicationyear | html %]
>  <br>[% pages | html %] [% i.illus | html %] [% i.size | html %]
>  [% IF i.notes %]<br><br>[% i.notes | html %][% END %]<br>
> +<a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-detail.pl?biblionumber=[% i.biblionumber | uri %]">View Details</a> | <a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-reserve.pl?biblionumber=[% i.biblionumber | uri %]">Reserve this Item</a>[% IF i.author %] | <a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?marclist=biblio.author&amp;and_or=and&amp;excluding=&amp;operator=contains&amp;value=[% i.author | uri %]&amp;resultsperpage=20&amp;orderby=biblio.title&amp;op=do_search">More by this Author</a>[% END %]

Single line monstrosities are really hard to eyeball, perhaps a follow up patch to better indent the modified sections across multiple lines?
Comment 22 Jonathan Druart 2018-10-31 17:37:02 UTC
Created attachment 81763 [details] [review]
Bug 21526: (follow-up) Use the 'url' filter when needed
Comment 23 Jonathan Druart 2018-10-31 17:37:11 UTC
Created attachment 81764 [details] [review]
Bug 21526: Fix search result pages (url vs uri vs raw)

query_cgi is uri_escaped from the pl, so we should displayed as raw

Test plan:
Use wide characters ❤
Search, filter, facets, search history, rss (both interfaces)

Please test deeply!
Comment 24 Jonathan Druart 2018-10-31 17:39:13 UTC
(In reply to M. Tompsett from comment #21)
> Comment on attachment 81530 [details] [review] [review]
> Bug 21526: Use the 'url' filter when needed
> 
> Review of attachment 81530 [details] [review] [review]:

Thanks Mark!


> > +            [% link = BLOCK %]<a href="[% authidurl _ authid | url %]">[% heading | html %]</a>[% END %]
> 
> Perhaps this line and line 31 should use the same method to build the URL? I
> tend to prefer 31's split.

I do not understand what you mean.

> > +    <fieldset class="action"><input type="hidden" name="op" value="[% op | html %]" />[% IF ( suggestionid ) %]<input type="submit" value="Save" /> <a class="cancel" href="[% IF ( returnsuggestedby ) %]/cgi-bin/koha/members/moremember.pl?borrowernumber=[% returnsuggestedby | uri %]#suggestions[% ELSE %]suggestion.pl[% END %]">Cancel</a>[% ELSE %]<input type="submit" value="Submit your suggestion" /> <a class="cancel" href="suggestion.pl">Cancel</a>[% END %]
> 
> Nice correction of html to uri in the url build part, but the hidden value's
> value... perhaps uri? After all, it's like the portion of a long GET
> request, not a string to display.

What are you suggestion here?


> > +                <a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-news-rss.pl?branchcode=[% branchcode | uri %]"><img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png"></a>
> 
> The img src url.... is interface url? is theme just a piece of the url?
> Something feels wrong about this, but I could be wrong.

"interface" must be "intranet-tmpl" or "opac-tmpl"
"theme" must be "bootstrap" or "prog"

=> I did not take care of them, not important for now.

I have fixed the search/facets on a separate bug reports.

Other comments have been fixed or ignored when I disagree.

Please provide tests to recreate an error if you think you find some.

Reopening the bug, as I think it must be fixed here. The last 3 patches need to be tested.
Comment 25 Jonathan Druart 2018-11-01 15:46:01 UTC
Upping severity to get the attention it needs.
Comment 26 Chris Cormack 2018-11-06 22:59:09 UTC
Created attachment 82011 [details] [review]
Bug 21526: Use the 'url' filter when needed

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 27 Chris Cormack 2018-11-06 22:59:31 UTC
Created attachment 82012 [details] [review]
Bug 21526: (follow-up) Use the 'url' filter when needed

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 28 Chris Cormack 2018-11-06 22:59:55 UTC
Created attachment 82013 [details] [review]
Bug 21526: Fix search result pages (url vs uri vs raw)

query_cgi is uri_escaped from the pl, so we should displayed as raw

Test plan:
Use wide characters ❤
Search, filter, facets, search history, rss (both interfaces)

Please test deeply!

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 29 Marcel de Rooy 2018-11-09 07:35:21 UTC
QA: Looking here
Comment 30 Marcel de Rooy 2018-11-09 08:03:38 UTC
(In reply to M. Tompsett from comment #21)
> Nice catch on the URL vs URI. But I think URI should be used for the base 
> part, and URL for anything we are adding to it, unless there is a bizarre & 
> at the end or something.

AFAIK uri should be used to encode parameters. And does more than url which should be used for the URL :) So especially for the path component. Not sure what you exactly mean by "base part" here.. But somehow the above statement does not feel completely right..
Comment 31 Marcel de Rooy 2018-11-09 08:15:40 UTC
koha-tmpl/intranet-tmpl/prog/en/includes/facets.inc
[% url = BLOCK %][% url | $raw %][% "&sort_by=" _ sort_by | url %][% END %]
So sort_by should be filtered by uri.

[% local_url = BLOCK %][% url | $raw %][% "&nolimit=" _  facet.type_link_value _ ":" _ facet.facet_link_value | url %][% END %]
Same here facet_link_value should be filtered by uri.

[% local_url = BLOCK %][% url | $raw %][% "&limit=" _  facet.type_link_value _ ":" _ facet.facet_link_value | url %][% END %]
Same line again

Since you normally filter params with uri, you have a reason here. Could you document such exceptions (or correct them if no reason) ?
Comment 32 Marcel de Rooy 2018-11-09 08:22:00 UTC
IF ( subfiel.is_url )
Nothing for you but how ugly. Existing choice of variables..
Comment 33 Marcel de Rooy 2018-11-09 08:26:35 UTC
koha-tmpl/intranet-tmpl/prog/en/modules/circ/printslip.tt
+<link rel="shortcut icon" href="[% IF ( IntranetFavicon ) %][% IntranetFavicon | url %][% ELSE %][% interface | html %]/[% theme | html %]/img/favicon.ico[% END %]" type="image/x-icon" />
interface and theme are parts here of the url path and not part of a html fragment so they should go here through url (not html or uri)
Comment 34 Marcel de Rooy 2018-11-09 08:27:46 UTC
<a href="[% cr.item.uri | url %]">Item URI</a>
Nice but perfect :)
Comment 35 Marcel de Rooy 2018-11-09 08:30:58 UTC
koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-facets.inc
[% url = BLOCK %][% url | $raw %][% "&amp;sort_by=" _ sort_by | url %][% END %]
Same as for facets above
Comment 36 Marcel de Rooy 2018-11-09 08:35:45 UTC
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt
<img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png">
interface and theme here are path parts so should go thru url
Comment 37 Marcel de Rooy 2018-11-09 08:37:17 UTC
+     <link>[% OPACBaseURL | html %]/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi | url %]&amp;format=rss2</link>
+     <atom:link rel="self" type="application/rss+xml" href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi | url %]&amp;sort_by=[% sort_by | uri %]&amp;format=rss2"/>

limit_cgi is a parameter, uri needed ?
Comment 38 Marcel de Rooy 2018-11-09 08:39:51 UTC
(In reply to Marcel de Rooy from comment #37)
> +     <link>[% OPACBaseURL | html %]/cgi-bin/koha/opac-search.pl?[%
> query_cgi | $raw %][% limit_cgi | url %]&amp;format=rss2</link>
> +     <atom:link rel="self" type="application/rss+xml" href="[% OPACBaseURL
> | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi |
> url %]&amp;sort_by=[% sort_by | uri %]&amp;format=rss2"/>
> 
> limit_cgi is a parameter, uri needed ?

koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-opensearch.tt
And more occurrences. Seems deliberate, not documented.
Comment 39 Marcel de Rooy 2018-11-09 08:43:30 UTC
<img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png" alt="Subscribe to this search" title="Subscribe to this search" class="rsssearchicon"/
As above. url instead of html in src attribute
Comment 40 Marcel de Rooy 2018-11-09 08:45:39 UTC
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-search-history.tt
<a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi |url %][% limit_cgi | url %]&amp;[% s.query_cgi | uri %]&

Funny mixup. Your script picks url for query_cgi again (for some reason) but does not fir s.query_cgi.
Comment 41 Marcel de Rooy 2018-11-09 08:47:14 UTC
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-showreviews.tt
<img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png"
Same as above
Comment 42 Marcel de Rooy 2018-11-09 08:49:49 UTC
koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/help.tt
[% interface | html %]/[% theme | html %]
Same again
koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/printslip.tt
[% interface | html %]/[% theme | html %]
Same again
koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/sco-main.tt
[% interface | html %]/[% theme | html %]
Comment 43 Marcel de Rooy 2018-11-09 10:01:53 UTC
Tested little bit with the facets. Included & in author name. Searching for Foy & Foy (from learning perl :)

/cgi-bin/koha/opac-search.pl?idx=kw&q=a&sort_by=relevance_dsc&limit=au:Foy%20&%20Foy
This is the result of url and it works better than the uri version. But actually both are not working so good.

/cgi-bin/koha/opac-search.pl?idx=kw&q=a&sort_by=relevance_dsc%26limit%3Dau%3AFoy%20%26amp%3B%20Foy
This comes from uri.
Does not work.

Conclusion: This problem is greater than just a filter. Keep url as best of both. Needs attention somewhere else. Probably the unparalleled C4::Search.
Comment 44 Marcel de Rooy 2018-11-09 10:03:38 UTC
Created attachment 82140 [details] [review]
Bug 21526: Use the 'url' filter when needed

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Marcel de Rooy 2018-11-09 10:03:44 UTC
Created attachment 82141 [details] [review]
Bug 21526: (follow-up) Use the 'url' filter when needed

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 46 Marcel de Rooy 2018-11-09 10:03:49 UTC
Created attachment 82142 [details] [review]
Bug 21526: Fix search result pages (url vs uri vs raw)

query_cgi is uri_escaped from the pl, so we should displayed as raw

Test plan:
Use wide characters ❤
Search, filter, facets, search history, rss (both interfaces)

Please test deeply!

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 47 Marcel de Rooy 2018-11-09 10:04:45 UTC
I recommend to push these patches now. Some refinements can follow (see above comments). And Search needs attention, which we knew already..
Comment 48 Jonathan Druart 2018-11-09 12:20:43 UTC
(In reply to Marcel de Rooy from comment #31)
> koha-tmpl/intranet-tmpl/prog/en/includes/facets.inc
> [% url = BLOCK %][% url | $raw %][% "&sort_by=" _ sort_by | url %][% END %]
> So sort_by should be filtered by uri.

Yes.

> [% local_url = BLOCK %][% url | $raw %][% "&nolimit=" _ 
> facet.type_link_value _ ":" _ facet.facet_link_value | url %][% END %]
> Same here facet_link_value should be filtered by uri.

Yes but more complex then, we should not only replace the filter.

> [% local_url = BLOCK %][% url | $raw %][% "&limit=" _  facet.type_link_value
> _ ":" _ facet.facet_link_value | url %][% END %]
> Same line again
> 
> Since you normally filter params with uri, you have a reason here. Could you
> document such exceptions (or correct them if no reason) ?

uri will replace the &, we do not want that (see bellow)

(In reply to Marcel de Rooy from comment #33)
> koha-tmpl/intranet-tmpl/prog/en/modules/circ/printslip.tt
> +<link rel="shortcut icon" href="[% IF ( IntranetFavicon ) %][%
> IntranetFavicon | url %][% ELSE %][% interface | html %]/[% theme | html
> %]/img/favicon.ico[% END %]" type="image/x-icon" />
> interface and theme are parts here of the url path and not part of a html
> fragment so they should go here through url (not html or uri)

Yes, see comment 24, I ignored them.

(In reply to Marcel de Rooy from comment #35)
> koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-facets.inc
> [% url = BLOCK %][% url | $raw %][% "&amp;sort_by=" _ sort_by | url %][% END
> %]
> Same as for facets above
(In reply to Marcel de Rooy from comment #37)
> +     <link>[% OPACBaseURL | html %]/cgi-bin/koha/opac-search.pl?[%
> query_cgi | $raw %][% limit_cgi | url %]&amp;format=rss2</link>
> +     <atom:link rel="self" type="application/rss+xml" href="[% OPACBaseURL
> | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi |
> url %]&amp;sort_by=[% sort_by | uri %]&amp;format=rss2"/>
> 
> limit_cgi is a parameter, uri needed ?
(In reply to Marcel de Rooy from comment #47)
> I recommend to push these patches now. Some refinements can follow (see
> above comments). And Search needs attention, which we knew already..

The search part is tricky, and we should rewrite it. Did you find regressions since 18.05? If you did we really need to take care of them (it's critical).
Comment 49 Tomás Cohen Arazi 2018-11-12 19:48:05 UTC
Pushed for 18.11. Awesome work all!
Comment 50 Martin Renvoize 2018-11-16 14:21:58 UTC
Bug 13618 not in 18.05.x series, no requirement to backport.
Comment 51 Devinim 2018-11-23 09:29:40 UTC Comment hidden (obsolete)
Comment 52 Marcel de Rooy 2018-11-26 08:54:08 UTC Comment hidden (obsolete)
Comment 53 Marcel de Rooy 2018-11-26 09:14:33 UTC Comment hidden (obsolete)
Comment 54 Marcel de Rooy 2018-11-26 09:15:51 UTC
Please submit further fixes to NEW reports.
Comment 55 Lucas Gass 2018-12-26 17:28:24 UTC
requires 21526 wont backport to 18.05
Comment 56 Nick Clemens 2019-06-06 17:16:31 UTC
*** Bug 13812 has been marked as a duplicate of this bug. ***