Bug 26606 - Correctly URI-encode query string in URL loaded after deleting an authority record
Summary: Correctly URI-encode query string in URL loaded after deleting an authority r...
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Phil Ringnalda
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-10-02 22:30 UTC by Phil Ringnalda
Modified: 2020-11-17 13:00 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
20.11.00, 19.11.12, 19.05.17


Attachments
Bug 26606: Correctly URI-encode query string in URL loaded after deleting an authority record (1.91 KB, patch)
2020-10-02 22:43 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 26606: Correctly URI-encode query string in URL loaded after deleting an authority record (1.97 KB, patch)
2020-10-05 08:05 UTC, Didier Gautheron
Details | Diff | Splinter Review
Bug 26606: Correctly URI-encode query string in URL loaded after deleting an authority record (2.03 KB, patch)
2020-10-15 14:36 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 Phil Ringnalda 2020-10-02 22:30:32 UTC
If you search for "this & that" and delete one of the results, after it is deleted the search in the reloaded page is for "this " rather than for "this & that". The template uses the url filter, which is for filtering an entire URI where :, &, #, etc. should be left alone, rather than the uri filter which is for filtering a URI component where they should be encoded.
Comment 1 Phil Ringnalda 2020-10-02 22:43:16 UTC
Created attachment 111184 [details] [review]
Bug 26606: Correctly URI-encode query string in URL loaded after deleting an authority record

If you search for "this & that" and delete one of the results, after
it is deleted the search in the reloaded page is for "this " rather
than for "this & that". The template uses the url filter, which is
for filtering an entire URI where :, &, #, etc. should be left alone,
rather than the uri filter which is for filtering a URI component
where they should be encoded.

To test:
1) Go to Authorities > New authority > New from Z39.50/SRU
2) Search for the Author (corporate) this & that collective
3) Actions > Import, Save
4) From the detail for that authority, Edit > Edit as new (duplicate)
5) Edit the 110 field, add 2 at the end and Save
6) Search authorities for this & that
7) For one of the ones you created, Actions > Delete, confirm
8) The page that reloads should have a search for this & that, not
just for this
Comment 2 Didier Gautheron 2020-10-05 08:05:47 UTC
Created attachment 111208 [details] [review]
Bug 26606: Correctly URI-encode query string in URL loaded after deleting an authority record

If you search for "this & that" and delete one of the results, after
it is deleted the search in the reloaded page is for "this " rather
than for "this & that". The template uses the url filter, which is
for filtering an entire URI where :, &, #, etc. should be left alone,
rather than the uri filter which is for filtering a URI component
where they should be encoded.

To test:
1) Go to Authorities > New authority > New from Z39.50/SRU
2) Search for the Author (corporate) this & that collective
3) Actions > Import, Save
4) From the detail for that authority, Edit > Edit as new (duplicate)
5) Edit the 110 field, add 2 at the end and Save
6) Search authorities for this & that
7) For one of the ones you created, Actions > Delete, confirm
8) The page that reloads should have a search for this & that, not
just for this

Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>
Comment 3 David Cook 2020-10-06 00:08:00 UTC
Looks like all the other filters shown in the context should be changed to "uri" from "html" as well, but seems out of scope for this change.
Comment 4 Phil Ringnalda 2020-10-06 20:22:41 UTC
A bug (not this bug) about properly escaping authtypecode everywhere it is used really ought to exist, but as for e.g. orderby which has four possible values, HeadingDsc or HeadingAsc or null or an XSS attack in a spearphishing link, I'm unable to come up with any scenario where it would be valuable to URI-escape the quote that starts the XSS attack as %22 so it would be carefully passed through to the search that reloads after a deletion rather than HTML-escaping it as &quot; and letting UA error handling deal with a bogus &quot; URI param.
Comment 5 David Cook 2020-10-06 22:37:09 UTC
(In reply to Phil Ringnalda from comment #4)
> A bug (not this bug) about properly escaping authtypecode everywhere it is
> used really ought to exist

In this case, I think a lot of those "html" filters were added by an automated script to add XSS protection. Not sure why the value would've had a "url" filter instead of a "uri" filter. Probably just human error.

> I'm unable to come up with any scenario where it would be valuable to
> URI-escape the quote that starts the XSS attack as %22 so it would be
> carefully passed through to the search that reloads after a deletion rather
> than HTML-escaping it as &quot; and letting UA error handling deal with a
> bogus &quot; URI param.

I do not understand what you're trying to say here. 

As for my earlier comment, I was saying that we should be using uri filters instead of html filters when URI building. But yes not in this bug.
Comment 6 Phil Ringnalda 2020-10-07 02:05:49 UTC
Happy to discuss things which are not about this bug in a place which is not in this bug.
Comment 7 David Cook 2020-10-07 03:30:56 UTC
(In reply to Phil Ringnalda from comment #6)
> Happy to discuss things which are not about this bug in a place which is not
> in this bug.

I think that I'm finished participating in this discussion, but feel free to open up more bug reports for URI encoding the rest of the query string arguments and authtypecode as you said.
Comment 8 Katrin Fischer 2020-10-15 14:36:22 UTC
Created attachment 111749 [details] [review]
Bug 26606: Correctly URI-encode query string in URL loaded after deleting an authority record

If you search for "this & that" and delete one of the results, after
it is deleted the search in the reloaded page is for "this " rather
than for "this & that". The template uses the url filter, which is
for filtering an entire URI where :, &, #, etc. should be left alone,
rather than the uri filter which is for filtering a URI component
where they should be encoded.

To test:
1) Go to Authorities > New authority > New from Z39.50/SRU
2) Search for the Author (corporate) this & that collective
3) Actions > Import, Save
4) From the detail for that authority, Edit > Edit as new (duplicate)
5) Edit the 110 field, add 2 at the end and Save
6) Search authorities for this & that
7) For one of the ones you created, Actions > Delete, confirm
8) The page that reloads should have a search for this & that, not
just for this

Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Jonathan Druart 2020-10-25 23:07:22 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 10 Lucas Gass 2020-11-13 21:52:02 UTC
backported to 20.05.x for 20.05.06
Comment 11 Aleisha Amohia 2020-11-17 05:05:17 UTC
backported to 19.11.x for 19.11.12
Comment 12 Victor Grousset/tuxayo 2020-11-17 13:00:19 UTC
Backported to 19.05.x branch for 19.05.17