Bug 18633 - Logs are full of CGI::param called in list context - itemsearch.pl
Summary: Logs are full of CGI::param called in list context - itemsearch.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Julian Maurice
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 11425 16154
Blocks: 18654
  Show dependency treegraph
 
Reported: 2017-05-18 20:09 UTC by Jonathan Druart
Modified: 2018-06-04 20:10 UTC (History)
6 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 18633: Remove useless use of CGI TT plugin (6.72 KB, patch)
2017-06-09 07:05 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18633: Remove remaining use of CGI.param in template (1.42 KB, patch)
2017-06-12 07:18 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18633: Remove useless use of CGI TT plugin (6.72 KB, patch)
2017-06-28 17:09 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 18633: Remove remaining use of CGI.param in template (1.40 KB, patch)
2017-06-28 17:09 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 18633: Remove useless use of CGI TT plugin (6.93 KB, patch)
2017-06-29 13:01 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18633: Remove remaining use of CGI.param in template (1.54 KB, patch)
2017-06-29 13:02 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18633: Remove useless use of CGI TT plugin (7.03 KB, patch)
2017-07-07 09:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18633: Remove remaining use of CGI.param in template (1.63 KB, patch)
2017-07-07 09:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18633: Remove useless use of CGI TT plugin [16.11.x] (7.00 KB, patch)
2017-08-04 14:33 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 18633: Remove remaining use of CGI.param in template [16.11.x] (1.69 KB, patch)
2017-08-04 14:33 UTC, Mark Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-05-18 20:09:30 UTC
The use of the CGI TT plugin in  catalogue/itemsearch.tt floods logs with:

itemsearch.pl: CGI::param called in list context from package Template::Document line 118, 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., referer: http://xxx/cgi-bin/koha/about.pl
Comment 1 Julian Maurice 2017-06-09 07:05:28 UTC
Created attachment 64123 [details] [review]
Bug 18633: Remove useless use of CGI TT plugin

The form is sent using AJAX so there is never a query string in
the URL (no POST parameters either)
Comment 2 Julian Maurice 2017-06-09 07:08:01 UTC
Test plan:
1. Verify that item search works exactly the same with or without the patch
Comment 3 Julian Maurice 2017-06-09 07:08:33 UTC
And 2. Verify that "CGI::param" warns are gone
Comment 4 Owen Leonard 2017-06-09 15:28:11 UTC
I still get a warning in the log:

itemsearch.pl: CGI::param called in list context from package Template::Document line 3, 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 5 Julian Maurice 2017-06-12 07:18:46 UTC
Created attachment 64193 [details] [review]
Bug 18633: Remove remaining use of CGI.param in template
Comment 6 Julian Maurice 2017-06-12 07:20:33 UTC
Thanks for testing Owen.
With this new patch the last warning should disappear
Comment 7 Alex Buckley 2017-06-25 18:03:22 UTC
Merge conflict on clean up to date branch

Applying: Bug 18633: Remove useless use of CGI TT plugin
Using index info to reconstruct a base tree...
M	koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/itemsearch.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/itemsearch.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/itemsearch.tt
Failed to merge in the changes.
Patch failed at 0001 Bug 18633: Remove useless use of CGI TT plugin
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-18633-Remove-useless-use-of-CGI-TT-plugin-W31_uJ.patch
Comment 8 Mark Tompsett 2017-06-28 17:09:01 UTC
Created attachment 64709 [details] [review]
Bug 18633: Remove useless use of CGI TT plugin

The form is sent using AJAX so there is never a query string in
the URL (no POST parameters either)
Comment 9 Mark Tompsett 2017-06-28 17:09:07 UTC
Created attachment 64710 [details] [review]
Bug 18633: Remove remaining use of CGI.param in template
Comment 10 Mark Tompsett 2017-06-28 17:14:05 UTC
Resurrected. Check for side-effects related to previous values being filled in. I think this might break something like that, but I have not tested. I merely made the patches apply cleanly.
Comment 11 Marc Véron 2017-06-29 13:01:33 UTC
Created attachment 64721 [details] [review]
Bug 18633: Remove useless use of CGI TT plugin

The form is sent using AJAX so there is never a query string in
the URL (no POST parameters either)

Tested both patches together, item search behaves as before, log
entries are gone
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 12 Marc Véron 2017-06-29 13:02:28 UTC
Created attachment 64722 [details] [review]
Bug 18633: Remove remaining use of CGI.param in template

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 13 Marcel de Rooy 2017-07-07 09:33:27 UTC
Created attachment 64900 [details] [review]
Bug 18633: Remove useless use of CGI TT plugin

The form is sent using AJAX so there is never a query string in
the URL (no POST parameters either)

Tested both patches together, item search behaves as before, log
entries are gone
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2017-07-07 09:33:31 UTC
Created attachment 64901 [details] [review]
Bug 18633: Remove remaining use of CGI.param in template

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Jonathan Druart 2017-07-10 21:12:43 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 16 Fridolin Somers 2017-07-18 12:04:28 UTC
Pushed to 17.05.x, will be in 17.05.02
Comment 17 Katrin Fischer 2017-07-19 21:12:53 UTC
Doesn't apply cleanly on 16.11.x - please rebase!
Comment 18 Mark Tompsett 2017-08-04 14:33:46 UTC
Created attachment 65502 [details] [review]
Bug 18633: Remove useless use of CGI TT plugin [16.11.x]

The form is sent using AJAX so there is never a query string in
the URL (no POST parameters either)

Tested both patches together, item search behaves as before, log
entries are gone
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Mark Tompsett 2017-08-04 14:33:53 UTC
Created attachment 65503 [details] [review]
Bug 18633: Remove remaining use of CGI.param in template [16.11.x]

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Mark Tompsett 2017-08-04 14:36:47 UTC
I believe I have rebased this for 16.11.x correctly. However, I did not test. I probably should have removed the sign offs for the 16.11.x
Comment 21 Katrin Fischer 2017-08-16 21:24:44 UTC
Hi Mark, thx for the patches. I tested and it seems they break itemsearch on 16.11.x. Please take another look, if you can.
Comment 22 Katrin Fischer 2017-11-15 20:26:39 UTC
Closing.