Bug 20529

Summary: Return to results link is truncated when the search contains a double quote
Product: Koha Reporter: Victor Grousset/tuxayo <victor>
Component: Staff interfaceAssignee: Victor Grousset/tuxayo <victor>
Status: CLOSED FIXED QA Contact: Nick Clemens <nick>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, gmcharlt, hagud, jonathan.druart, kyle, m.de.rooy, marjorie.barry-vila, nick, sandboxes, severine.queune
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22070
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.04,20.11.10
Attachments: Bug 20529: Fix "Return to results" link broken by a double quote
Bug 20529: Fix "Return to results" link broken by a double quote
Bug 20529: Fix "Return to results" link broken by a double quote
Bug 20529: Fix "Return to results" link broken by a double quote
Bug 20529: Fix "Return to results" link broken by a double quote
Bug 20529: Fix "Return to results" link broken by a double quote
Bug 20529: Fix "Return to results" link broken by a double quote
Bug 20529: Remove other occurrences of decodeURIComponent from browser.js
Bug 20529: Fix "Return to results" link broken by a double quote
Bug 20529: Remove other occurrences of decodeURIComponent from browser.js

Description Victor Grousset/tuxayo 2018-04-05 15:41:28 UTC
== Reproduce ==
1. Search in the staff catalogue «a "alice"»
     Replace a and alice with whatever than will return a result.
2. Click on the first result
3. Check the "Return to results"
     It should be like
     staff_url://cgi-bin/koha/catalogue/search.pl?idx=kw&q=a
     This is the bug. Because when clicking the link, your search will be
     'kw,wrdl: a'
     instead of
     'kw,wrdl: a "alice"'

== Cause ==

Something about escaping in koha-tmpl/intranet-tmpl/js/browser.js
Search for BROWSER_RETURN_TO_SEARCH
Comment 1 Victor Grousset/tuxayo 2018-04-16 17:16:50 UTC
Created attachment 74251 [details] [review]
Bug 20529: Fix "Return to results" link broken by a double quote

(in the search query)

cause: decodeURIComponent used when URI encoding was necessary. (At
least in main usecase of this feature)
It could have been a copy and paste error. From the above
browseRecords() function.

== Test plan ==
1. Search in the staff catalogue «a "alice"»
     Replace a and alice with whatever than will return a result for you
2. Click on the first result
3. Check the "Return to results" link
     It should be like
     staff_url://cgi-bin/koha/catalogue/search.pl?idx=kw&q=a
     This is the bug. Because when clicking the link, your search will be
     'kw,wrdl: a'
     instead of
     'kw,wrdl: a "alice"'
4. Apply this patch.
5. Search in the staff catalogue «a "alice"»
     Replace a and alice with whatever than will return a result for you
6. Click on the first result
7. Check the "Return to results" link
     The link should preserve the full query
Comment 2 Biblibre Sandboxes 2018-04-19 13:10:12 UTC
Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>
Comment 3 Biblibre Sandboxes 2018-04-19 13:10:33 UTC
Created attachment 74553 [details] [review]
Bug 20529: Fix "Return to results" link broken by a double quote

(in the search query)

cause: decodeURIComponent used when URI encoding was necessary. (At
least in main usecase of this feature)
It could have been a copy and paste error. From the above
browseRecords() function.

== Test plan ==
1. Search in the staff catalogue «a "alice"»
     Replace a and alice with whatever than will return a result for you
2. Click on the first result
3. Check the "Return to results" link
     It should be like
     staff_url://cgi-bin/koha/catalogue/search.pl?idx=kw&q=a
     This is the bug. Because when clicking the link, your search will be
     'kw,wrdl: a'
     instead of
     'kw,wrdl: a "alice"'
4. Apply this patch.
5. Search in the staff catalogue «a "alice"»
     Replace a and alice with whatever than will return a result for you
6. Click on the first result
7. Check the "Return to results" link
     The link should preserve the full query

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 4 Biblibre Sandboxes 2018-04-19 13:11:11 UTC
Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>
Comment 5 Biblibre Sandboxes 2018-04-19 13:11:32 UTC
Created attachment 74554 [details] [review]
Bug 20529: Fix "Return to results" link broken by a double quote

(in the search query)

cause: decodeURIComponent used when URI encoding was necessary. (At
least in main usecase of this feature)
It could have been a copy and paste error. From the above
browseRecords() function.

== Test plan ==
1. Search in the staff catalogue «a "alice"»
     Replace a and alice with whatever than will return a result for you
2. Click on the first result
3. Check the "Return to results" link
     It should be like
     staff_url://cgi-bin/koha/catalogue/search.pl?idx=kw&q=a
     This is the bug. Because when clicking the link, your search will be
     'kw,wrdl: a'
     instead of
     'kw,wrdl: a "alice"'
4. Apply this patch.
5. Search in the staff catalogue «a "alice"»
     Replace a and alice with whatever than will return a result for you
6. Click on the first result
7. Check the "Return to results" link
     The link should preserve the full query

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 6 Séverine Queune 2018-04-19 13:14:02 UTC
Works with both Zebra (Biblibre's sandbox #10) and ElasticSearch (Biblibre's sandbox #12), with the original navigation tool and the new " BrowseResultSelection" option.
Thanks Victor !
Comment 7 Victor Grousset/tuxayo 2018-04-19 13:19:02 UTC
Thanks Séverine for testing with ES!
Comment 8 Nick Clemens 2018-04-20 10:38:55 UTC
Created attachment 74610 [details] [review]
Bug 20529: Fix "Return to results" link broken by a double quote

(in the search query)

cause: decodeURIComponent used when URI encoding was necessary. (At
least in main usecase of this feature)
It could have been a copy and paste error. From the above
browseRecords() function.

== Test plan ==
1. Search in the staff catalogue «a "alice"»
     Replace a and alice with whatever than will return a result for you
2. Click on the first result
3. Check the "Return to results" link
     It should be like
     staff_url://cgi-bin/koha/catalogue/search.pl?idx=kw&q=a
     This is the bug. Because when clicking the link, your search will be
     'kw,wrdl: a'
     instead of
     'kw,wrdl: a "alice"'
4. Apply this patch.
5. Search in the staff catalogue «a "alice"»
     Replace a and alice with whatever than will return a result for you
6. Click on the first result
7. Check the "Return to results" link
     The link should preserve the full query

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Jonathan Druart 2018-04-20 15:40:30 UTC
Why this occurrence and not the others?
Comment 10 Victor Grousset/tuxayo 2018-04-23 08:12:08 UTC
Because I wasn't able to trigger the code of the other occurrences ^^"

https://gitlab.com/koha-community/Koha/blob/95031b035b5de87211d0de2040eb685ae005606c/koha-tmpl/intranet-tmpl/js/browser.js#L33
Comment 11 Jonathan Druart 2018-04-24 14:30:15 UTC
I guess it's:
go to first, next, previous, go to last, etc.
Comment 12 Victor Grousset/tuxayo 2018-05-02 15:33:17 UTC
> go to first, next, previous, go to last, etc.

It doesn't work, placed few console.log() and those next to the two occurrences of the usage of `decodeURIComponent(current_search.query)` weren't triggered.
Comment 13 Jonathan Druart 2018-05-07 14:24:56 UTC
Reading the code I'd say these 2 switches prevent wrong calls to browseRecords (like you call it with movement != 1 and -1)
Comment 14 Victor Grousset/tuxayo 2019-04-11 12:13:33 UTC
Unfortunately I'm out of time to expand much more [1] the scope of this issue.
1. Is the fix simple enough to be applied without having a test plan?
2. Is it okay to keep the scope as it is?
3. Should the patch stay pending waiting until someone to complete it?

[1] implementation (not much work) + test plan that is still a mystery for me about how to expand it
Comment 15 Jonathan Druart 2019-04-27 20:33:27 UTC
I'd say we could go without test plan, if the changes make sense and work in that case it must work for the "special" cases.
Comment 16 Séverine Queune 2019-11-19 14:17:07 UTC
Sorry Victor, it failed when I tried.
Comment 17 Fridolin Somers 2020-06-29 14:03:26 UTC
Why keeping decodeURIComponent() for current_search.limit ?
Comment 18 Victor Grousset/tuxayo 2020-06-29 15:28:31 UTC
(In reply to Jonathan Druart from comment #15)
> I'd say we could go without test plan, if the changes make sense and work in
> that case it must work for the "special" cases.

Ok, thanks for the additional analysis.


(In reply to Séverine Queune from comment #16)
> Sorry Victor, it failed when I tried.

Thanks for testing.


(In reply to Fridolin SOMERS from comment #17)
> Why keeping decodeURIComponent() for current_search.limit ?

I mostly lost the context. I guess that was because it didn't relate to the bug. Should decodeURIComponent be also removed for current_search.limit?

The good news is that this part is known how to be tested. As opposed to the other occurrences.
Comment 19 Fridolin Somers 2021-01-25 09:18:36 UTC
What does this bug need ?
Comment 20 Katrin Fischer 2021-04-17 13:27:00 UTC
The patch needs a major rebase, since we since moved some things around like changing the way we do the translations on this script.

But the problem remains. I tested with: a "test"

The link back to the results turns it into a search for: a

++<<<<<<< HEAD
 +                    var searchURL = '/cgi-bin/koha/catalogue/search.pl?' + decodeURIComponent(current_search.query) + '&limit=' + decodeURIComponent(current_search.limit) + '&sort_by=' + current_search.sort + '&searchid=' + me.searchid + '&offset=' + me.offset;
++=======
+                     var searchURL = '/cgi-bin/koha/catalogue/search.pl?' + current_search.query + '&limit=' + decodeURIComponent(current_search.limit) + '&sort=' + current_search.sort + '&searchid=' + me.searchid + '&offset=' + me.offset;
++>>>>>>> Bug 20529: Fix "Return to results" link broken by a double quote
Comment 21 Victor Grousset/tuxayo 2021-05-10 22:35:16 UTC
Created attachment 120816 [details] [review]
Bug 20529: Fix "Return to results" link broken by a double quote

(in the search query)

cause: decodeURIComponent used when URI encoding was necessary. (At
least in main usecase of this feature)
It could have been a copy and paste error. From the above
browseRecords() function.

== Test plan ==
1. Search in the staff catalogue «a "alice"»
     Replace a and alice with whatever than will return a result for you
2. Click on the first result
3. Check the "Return to results" link
     It should be like
     staff_url://cgi-bin/koha/catalogue/search.pl?idx=kw&q=a
     This is the bug. Because when clicking the link, your search will be
     'kw,wrdl: a'
     instead of
     'kw,wrdl: a "alice"'
4. Apply this patch.
5. Search in the staff catalogue «a "alice"»
     Replace a and alice with whatever than will return a result for you
6. Click on the first result
7. Check the "Return to results" link
     The link should preserve the full query

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 22 Victor Grousset/tuxayo 2021-05-10 22:38:56 UTC
Rebased and solved conflict.

====

> The patch needs a major rebase, since we since moved some things around like changing the way we do the translations on this script.

Does it? Test plan work on my instance.

====

> But the problem remains. I tested with: a "test"

With sample data, the test plan succeeds with «a "test"»
Comment 23 Fridolin Somers 2021-07-07 00:56:14 UTC
Created attachment 122626 [details] [review]
Bug 20529: Fix "Return to results" link broken by a double quote

(in the search query)

cause: decodeURIComponent used when URI encoding was necessary. (At
least in main usecase of this feature)
It could have been a copy and paste error. From the above
browseRecords() function.

== Test plan ==
1. Search in the staff catalogue «a "alice"»
     Replace a and alice with whatever than will return a result for you
2. Click on the first result
3. Check the "Return to results" link
     It should be like
     staff_url://cgi-bin/koha/catalogue/search.pl?idx=kw&q=a
     This is the bug. Because when clicking the link, your search will be
     'kw,wrdl: a'
     instead of
     'kw,wrdl: a "alice"'
4. Apply this patch.
5. Search in the staff catalogue «a "alice"»
     Replace a and alice with whatever than will return a result for you
6. Click on the first result
7. Check the "Return to results" link
     The link should preserve the full query

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 24 Fridolin Somers 2021-07-07 00:57:36 UTC
I totally reproduce bug and patch fixes it.

We already see :
  decodeURIComponent(current_search.limit)
It is logical to use :
  decodeURIComponent(current_search.query)
Comment 25 Nick Clemens 2021-07-09 12:29:57 UTC
Created attachment 122736 [details] [review]
Bug 20529: Fix "Return to results" link broken by a double quote

(in the search query)

cause: decodeURIComponent used when URI encoding was necessary. (At
least in main usecase of this feature)
It could have been a copy and paste error. From the above
browseRecords() function.

== Test plan ==
1. Search in the staff catalogue «a "alice"»
     Replace a and alice with whatever than will return a result for you
2. Click on the first result
3. Check the "Return to results" link
     It should be like
     staff_url://cgi-bin/koha/catalogue/search.pl?idx=kw&q=a
     This is the bug. Because when clicking the link, your search will be
     'kw,wrdl: a'
     instead of
     'kw,wrdl: a "alice"'
4. Apply this patch.
5. Search in the staff catalogue «a "alice"»
     Replace a and alice with whatever than will return a result for you
6. Click on the first result
7. Check the "Return to results" link
     The link should preserve the full query

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 26 Jonathan Druart 2021-07-09 14:00:12 UTC
(In reply to Fridolin Somers from comment #24)
> I totally reproduce bug and patch fixes it.
> 
> We already see :
>   decodeURIComponent(current_search.limit)
> It is logical to use :
>   decodeURIComponent(current_search.query)

Hum? What do you mean?

If it makes sense to remove this decode call then we should remove other occurrences from this file.

I don't see the point of decodeURIComponent to generate an URI, unless we are doing weird stuffs somewhere else (or it's Friday afternoon and I am tired).
Comment 27 Jonathan Druart 2021-07-09 14:02:02 UTC
Created attachment 122751 [details] [review]
Bug 20529: Remove other occurrences of decodeURIComponent from browser.js
Comment 28 Jonathan Druart 2021-07-09 14:03:29 UTC
Can someone stress the feature with this patch? IIRC weird bugs usually happens "on the edge" of the pagination.

Like search with your favourite weird terms and click the last records from the page, then next, previous.
Comment 29 Marcel de Rooy 2021-07-16 06:21:42 UTC
(In reply to Jonathan Druart from comment #28)
> Can someone stress the feature with this patch? IIRC weird bugs usually
> happens "on the edge" of the pagination.
> 
> Like search with your favourite weird terms and click the last records from
> the page, then next, previous.

This needs feedback from the author
Comment 30 Victor Grousset/tuxayo 2021-08-09 22:05:57 UTC
> Can someone stress the feature with this patch?

What do you mean? Is the patch name too vague?
The first 3 steps of the test plan show the issue.
Comment 31 Jonathan Druart 2021-08-10 08:54:26 UTC
(In reply to Victor Grousset/tuxayo from comment #30)
> > Can someone stress the feature with this patch?
> 
> What do you mean? Is the patch name too vague?
> The first 3 steps of the test plan show the issue.

I've explained it:
"Like search with your favourite weird terms and click the last records from the page, then next, previous."
Comment 32 Jonathan Druart 2021-08-19 09:13:34 UTC
*** Bug 28866 has been marked as a duplicate of this bug. ***
Comment 33 Nick Clemens 2021-09-01 13:01:48 UTC
Created attachment 124329 [details] [review]
Bug 20529: Fix "Return to results" link broken by a double quote

(in the search query)

cause: decodeURIComponent used when URI encoding was necessary. (At
least in main usecase of this feature)
It could have been a copy and paste error. From the above
browseRecords() function.

== Test plan ==
1. Search in the staff catalogue «a "alice"»
     Replace a and alice with whatever than will return a result for you
2. Click on the first result
3. Check the "Return to results" link
     It should be like
     staff_url://cgi-bin/koha/catalogue/search.pl?idx=kw&q=a
     This is the bug. Because when clicking the link, your search will be
     'kw,wrdl: a'
     instead of
     'kw,wrdl: a "alice"'
4. Apply this patch.
5. Search in the staff catalogue «a "alice"»
     Replace a and alice with whatever than will return a result for you
6. Click on the first result
7. Check the "Return to results" link
     The link should preserve the full query

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 34 Nick Clemens 2021-09-01 13:01:52 UTC
Created attachment 124330 [details] [review]
Bug 20529: Remove other occurrences of decodeURIComponent from browser.js

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 35 Nick Clemens 2021-09-01 13:04:04 UTC
Moving to PQA - I tested with as many odd searches as I could, easier to break search and get no results than to break this.

Any search with ccode will break without these patches as we double quote their values in the search code (in case they contain strange values, but we don't care about other auth_values?) so I think we should get this in
Comment 36 Jonathan Druart 2021-09-02 10:10:54 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 37 Kyle M Hall 2021-09-03 15:20:39 UTC
Pushed to 21.05.x for 21.05.04
Comment 38 Fridolin Somers 2021-09-08 20:49:23 UTC
Pushed to 20.11.x for 20.11.10
Comment 39 Victor Grousset/tuxayo 2021-09-20 11:56:33 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.