Bug 21115 - Add multi_param call and add divider in cache key in svc/report and opac counterpart
Summary: Add multi_param call and add divider in cache key in svc/report and opac coun...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-07-25 12:37 UTC by Marcel de Rooy
Modified: 2019-10-14 19:55 UTC (History)
3 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 21115: Add multi_param call and add divider in cache key in svc/report and opac counterpart (3.15 KB, patch)
2018-07-25 14:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21115: Add multi_param call and add divider in cache key in svc/report and opac counterpart (3.17 KB, patch)
2018-10-10 16:03 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21115: Add multi_param call and add divider in cache key in svc/report and opac counterpart (3.22 KB, patch)
2018-10-12 00:35 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2018-07-25 12:37:43 UTC
CGI::param called in list context from package CGI::Compile::ROOT::usr_share_koha_prodclone_opac_svc_report line 42, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
Comment 1 Marcel de Rooy 2018-07-25 14:06:42 UTC
Created attachment 77256 [details] [review]
Bug 21115: Add multi_param call and add divider in cache key in svc/report and opac counterpart

Resolve things like:
CGI::param called in list context from package CGI::Compile::ROOT::usr_share_koha_prodclone_opac_svc_report line 42, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.

The cache key in both script looks like:
    opac:report:id:602018
but should for consistency be:
    opac:report:id:60:2018
Note: The 2018 here is part of the sql_params and should not be
concatenated to the report id.

Test plan:
Do not yet apply this patch.
Make a report public, set cache to 300 secs.
Check its output with opac/svc/report.
Check for the warn in your log.
Apply the patch, restart Plack and flush cache.
Check opac/svc/report.
Modify your report; e.g. add a simple string to the SELECT.
Check opac/svc/report. You should still see cached output.
Flush the cache.
Check opac/svc/report. You should now see the added text.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested also by clearing individual keys with $cache->clear_from_cache.
Comment 2 Marcel de Rooy 2018-07-25 14:15:50 UTC
Note for QA:
Looks like we should remove the opac and intranet key from the cache when modifying a report. But that is not the topic of this report. So I use this phenomenon in my test plan.
Comment 3 Mark Tompsett 2018-10-10 16:03:27 UTC
Created attachment 80365 [details] [review]
Bug 21115: Add multi_param call and add divider in cache key in svc/report and opac counterpart

Resolve things like:
CGI::param called in list context from package CGI::Compile::ROOT::usr_share_koha_prodclone_opac_svc_report line 42, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.

The cache key in both script looks like:
    opac:report:id:602018
but should for consistency be:
    opac:report:id:60:2018
Note: The 2018 here is part of the sql_params and should not be
concatenated to the report id.

Test plan:
Do not yet apply this patch.
Make a report public, set cache to 300 secs.
Check its output with opac/svc/report.
Check for the warn in your log.
Apply the patch, restart Plack and flush cache.
Check opac/svc/report.
Modify your report; e.g. add a simple string to the SELECT.
Check opac/svc/report. You should still see cached output.
Flush the cache.
Check opac/svc/report. You should now see the added text.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested also by clearing individual keys with $cache->clear_from_cache.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 4 Marcel de Rooy 2018-10-11 06:21:39 UTC
(In reply to M. Tompsett from comment #3)
> Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Thx
Comment 5 Chris Cormack 2018-10-12 00:35:22 UTC
Created attachment 80441 [details] [review]
Bug 21115: Add multi_param call and add divider in cache key in svc/report and opac counterpart

Resolve things like:
CGI::param called in list context from package CGI::Compile::ROOT::usr_share_koha_prodclone_opac_svc_report line 42, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.

The cache key in both script looks like:
    opac:report:id:602018
but should for consistency be:
    opac:report:id:60:2018
Note: The 2018 here is part of the sql_params and should not be
concatenated to the report id.

Test plan:
Do not yet apply this patch.
Make a report public, set cache to 300 secs.
Check its output with opac/svc/report.
Check for the warn in your log.
Apply the patch, restart Plack and flush cache.
Check opac/svc/report.
Modify your report; e.g. add a simple string to the SELECT.
Check opac/svc/report. You should still see cached output.
Flush the cache.
Check opac/svc/report. You should now see the added text.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested also by clearing individual keys with $cache->clear_from_cache.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 6 Nick Clemens 2018-10-15 17:02:09 UTC
Awesome work all!

Pushed to master for 18.11
Comment 7 Martin Renvoize 2018-10-17 10:56:21 UTC
Pushed to 18.05.x for 18.05.05
Comment 8 Fridolin Somers 2018-11-09 09:30:44 UTC
Pushed to 17.11.x for 17.11.12