Bug 18497 - Downloading a report passes the constructed SQL as a parameter
Summary: Downloading a report passes the constructed SQL as a parameter
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 19803 (view as bug list)
Depends on:
Blocks: 19910 20945
  Show dependency treegraph
 
Reported: 2017-04-26 16:14 UTC by Nick Clemens
Modified: 2018-06-14 19:16 UTC (History)
9 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
SQL which generates error (6.61 KB, application/sql)
2017-07-31 12:49 UTC, Barton Chittenden
Details
Resultant URI (9.05 KB, text/plain)
2017-07-31 12:50 UTC, Barton Chittenden
Details
Bug 18947 - Use report id to retrieve saved SQL instead of passing param (5.97 KB, patch)
2018-01-03 13:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Sample SQL query to trigger bug. (6.70 KB, text/plain)
2018-01-17 19:53 UTC, M. Tompsett
Details
Bug 18497 - Use report id to retrieve saved SQL instead of passing param (6.02 KB, patch)
2018-01-17 20:16 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18497: Use report id to retrieve saved SQL instead of passing param (5.97 KB, patch)
2018-01-30 14:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18497: (follow-up) Chomp newlines in BLOCK (1.22 KB, patch)
2018-01-30 14:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18497: Use report id to retrieve saved SQL instead of passing param (6.04 KB, patch)
2018-01-31 08:46 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18497: (follow-up) Chomp newlines in BLOCK (1.29 KB, patch)
2018-01-31 08:46 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2017-04-26 16:14:00 UTC
This is not ideal, long queries can cause broken links and is a very messy link.

report/guided_reports.pl

line 816:
my $sql    = $input->param('sql');  # FIXME: use sql from saved report ID#, not new user-supplied SQL!

We use execute_query, so input is sanitized but links look awful

http://localhost:8081/cgi-bin/koha/reports/guided_reports.pl?reports=1&phase=Export&format=csv&sql=SELECT%20*%20FROM%20message_queue%20ORDER%20BY%20time_queued%20DESC%0D%0A&reportname=Messages
Comment 1 Barton Chittenden 2017-07-31 12:49:34 UTC
Created attachment 65338 [details]
SQL which generates error
Comment 2 Barton Chittenden 2017-07-31 12:50:27 UTC
Created attachment 65339 [details]
Resultant URI
Comment 3 Barton Chittenden 2017-07-31 12:52:23 UTC
A report with a very long query cannot be downloaded as CSV, giving the error 'Request-URI Too Long'. I'm escalating the priority to normal because this is no longer simply a cosmetic issue; it affects the operation of koha itself.

See the attached sql and uri files.

To replicate:

1) Paste the contents of the attached 'bug-18497-sql.sql' into a report.
2) Save and run the report
3) On the report page, click Download and select 'Comma separated text'
4) The resulting URI is as contained in 'bug-18497-uri.txt' (adjusting for server name), and gives the error message

    Request-URI Too Long

    The requested URL's length exceeds the capacity limit for this server.
    Apache/2.4.10 (Debian) Server at catalog.bywatersolutions.com Port 443
Comment 4 Nick Clemens 2018-01-03 13:18:01 UTC
Created attachment 70245 [details] [review]
Bug 18947 - Use report id to retrieve saved SQL instead of passing param

This patch takes some of the code when executing report and moves it to
a sub to be reused when downloading

To test:
1 - Run some very long report (see comment #1)
2 - Try to download, erk!
3 - Apply patch
4 - Run report, results hould not have changed
5 - Try to download, success!
6 - Ensure reports work as before
Comment 5 Nick Clemens 2018-01-03 13:18:53 UTC
*** Bug 19803 has been marked as a duplicate of this bug. ***
Comment 6 M. Tompsett 2018-01-17 19:53:14 UTC
Created attachment 70637 [details]
Sample SQL query to trigger bug.

Revised query that works with master and default data
Comment 7 M. Tompsett 2018-01-17 20:16:30 UTC
Created attachment 70639 [details] [review]
Bug 18497 - Use report id to retrieve saved SQL instead of passing param

This patch takes some of the code when executing report and moves it to
a sub to be reused when downloading

To test:
1 - Run some very long report (see comment #1)
2 - Try to download, erk!
3 - Apply patch
4 - Run report, results hould not have changed
5 - Try to download, success!
6 - Ensure reports work as before

https://bugs.koha-community.org/show_bug.cgi?id=18497

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 8 Julian Maurice 2018-01-30 14:11:55 UTC
This works, but it generates new HTML validation errors:

> Error: Bad value
> /cgi-bin/koha/reports/guided_reports.pl?reports=1&phase=Export&format=csv&report_id=2&reportname=Test↩                &sql_params=foo↩            
> for attribute href on element a: Tab, new line or carriage return found.

Please fix this.

Also, please fix the commit message:
- "Bug XXXXX -" should be "Bug XXXXX:" according to https://wiki.koha-community.org/wiki/Commit_messages
- Bug's URL should be removed (it's wrong)
Comment 9 Nick Clemens 2018-01-30 14:36:23 UTC
Created attachment 71060 [details] [review]
Bug 18497: Use report id to retrieve saved SQL instead of passing param

This patch takes some of the code when executing report and moves it to
a sub to be reused when downloading

To test:
1 - Run some very long report (see comment #1)
2 - Try to download, erk!
3 - Apply patch
4 - Run report, results hould not have changed
5 - Try to download, success!
6 - Ensure reports work as before

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 10 Nick Clemens 2018-01-30 14:36:27 UTC
Created attachment 71061 [details] [review]
Bug 18497: (follow-up) Chomp newlines in BLOCK

To test:
Run report
Validate results page at: https://validator.w3.org/
Comment 11 Julian Maurice 2018-01-31 08:46:35 UTC
Created attachment 71080 [details] [review]
Bug 18497: Use report id to retrieve saved SQL instead of passing param

This patch takes some of the code when executing report and moves it to
a sub to be reused when downloading

To test:
1 - Run some very long report (see comment #1)
2 - Try to download, erk!
3 - Apply patch
4 - Run report, results hould not have changed
5 - Try to download, success!
6 - Ensure reports work as before

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 12 Julian Maurice 2018-01-31 08:46:39 UTC
Created attachment 71081 [details] [review]
Bug 18497: (follow-up) Chomp newlines in BLOCK

To test:
Run report
Validate results page at: https://validator.w3.org/

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 13 Jonathan Druart 2018-02-02 15:11:04 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 14 Nick Clemens 2018-02-16 02:27:47 UTC
Awesome work all, pushed to stable for 17.11.03