Bug 20945 - Report params not escaped when downloading
Summary: Report params not escaped when downloading
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Nick Clemens (kidclamp)
QA Contact: Testopia
URL:
Keywords:
Depends on: 18497
Blocks:
  Show dependency treegraph
 
Reported: 2018-06-14 14:20 UTC by Nick Clemens (kidclamp)
Modified: 2019-10-14 19:57 UTC (History)
4 users (show)

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


Attachments
Bug 20945: Escape SQL parameters when constructing download links (1.24 KB, patch)
2018-06-14 14:23 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 20945: Escape SQL parameters when constructing download links (1.30 KB, patch)
2018-06-25 21:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20945: Escape SQL parameters when constructing download links (1.37 KB, patch)
2018-06-29 09:42 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 20945: (RM follow-up) Escape param_name too (1.04 KB, patch)
2018-06-29 20:10 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2018-06-14 14:20:29 UTC
Caused by bug 18497
Comment 1 Nick Clemens (kidclamp) 2018-06-14 14:23:01 UTC
Created attachment 76056 [details] [review]
Bug 20945: Escape SQL parameters when constructing download links

TO test:
1 - Create a report that takes a parameter
2 - Enter a parameter containing '%'
3 - Attempt to download report, note link is misconstructed
4 - Apply patch
5 - Reload
6 - Note URL is now correct
Comment 2 Katrin Fischer 2018-06-18 21:37:06 UTC
I've been testing with the following SQL:

select * from borrowers where firstname like <<TEST|Asort1>> and surname like <<surname>>

Asort1 has a % in my test. But I can download fine in all formats and the URL looks like this:

cgi-bin/koha/reports/guided_reports.pl?reports=2&phase=Run+this+report&param_name=TEST|Asort1&sql_params=%25&param_name=surname&sql_params=%25

Can you explain more about the problem? What am I missing?
Comment 3 Nick Clemens (kidclamp) 2018-06-20 00:19:43 UTC
Ah, I don't see it in Chrome, but I do in Firefox, what browser are you using?
Comment 4 Katrin Fischer 2018-06-20 06:33:27 UTC
Firefox - maybe a different version? But it trying to be smart could explain.
Comment 5 Nick Clemens (kidclamp) 2018-06-20 11:51:54 UTC
(In reply to Katrin Fischer from comment #4)
> Firefox - maybe a different version? But it trying to be smart could explain.

My version: 61.0b13

Just to check, that URL you pasted is the report url, I meant the URL of the download links.

To download as tab I get:
http://localhost:8081/cgi-bin/koha/reports/guided_reports.pl?reports=1&phase=Export&format=tab&report_id=13&reportname=Test&sql_params=%25&param_name=Enter|Asort1

Versus the report URL:
http://localhost:8081/cgi-bin/koha/reports/guided_reports.pl?reports=13&phase=Run+this+report&param_name=Enter%7CAsort1&sql_params=%2525#

Notice the param changes from:
%2525 in the report URL
to
%25 in the download URL

This has the effect of running the report with param as '%' when downloading (because it translates %25 as an html code)

The results will download, however, they will include all the barcodes, instead of the limited set it should be
Comment 6 Katrin Fischer 2018-06-25 21:15:42 UTC
Created attachment 76423 [details] [review]
Bug 20945: Escape SQL parameters when constructing download links

TO test:
1 - Create a report that takes a parameter
2 - Enter a parameter containing '%'
3 - Attempt to download report, note link is misconstructed
4 - Apply patch
5 - Reload
6 - Note URL is now correct

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 Martin Renvoize (ashimema) 2018-06-29 09:42:08 UTC
Created attachment 76558 [details] [review]
Bug 20945: Escape SQL parameters when constructing download links

TO test:
1 - Create a report that takes a parameter
2 - Enter a parameter containing '%'
3 - Attempt to download report, note link is misconstructed
4 - Apply patch
5 - Reload
6 - Note URL is now correct

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize (ashimema) 2018-06-29 09:42:50 UTC
Trivial fix that works as expected, Passed QA :)
Comment 9 Jonathan Druart 2018-06-29 11:28:33 UTC
Any good reasons to not escape param_name as well?
Comment 10 Nick Clemens (kidclamp) 2018-06-29 20:10:46 UTC
Created attachment 76587 [details] [review]
Bug 20945: (RM follow-up) Escape param_name too
Comment 11 Nick Clemens (kidclamp) 2018-06-29 21:02:47 UTC
Awesome work all!

Pushed to master for 18.11
Comment 12 Martin Renvoize (ashimema) 2018-07-02 08:19:55 UTC
Pushed to 18.05.x for 18.05.02
Comment 13 Fridolin Somers 2018-07-04 08:09:43 UTC
Pushed to 17.11.x for 17.11.08.

Follow-up not needed, it depends on Bug 9634 not in 17.11.x
Comment 14 Fridolin Somers 2018-07-04 08:10:14 UTC
Depends on Bug 18497 not in 17.05.x