Bug 16154 - Replace CGI->param with CGI->multi_param in list context
Summary: Replace CGI->param with CGI->multi_param in list context
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
: 16150 (view as bug list)
Depends on: 15809
Blocks: 21051 16259 18632 18633
  Show dependency treegraph
 
Reported: 2016-03-29 09:16 UTC by Jonathan Druart
Modified: 2018-07-11 21:31 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 16154: CGI->multi_param - Declare a list (71.38 KB, patch)
2016-03-29 09:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16154: CGI->multi_param - Manual changes (5.74 KB, patch)
2016-03-29 09:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16154: CGI->multi_param - Force scalar context (63.04 KB, patch)
2016-03-29 09:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16154: CGI->multi_param - Assign a list (9.17 KB, patch)
2016-03-29 09:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16154: CGI->multi_param - Force scalar context (63.04 KB, patch)
2016-04-13 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16154: CGI->multi_param - Assign a list (9.17 KB, patch)
2016-04-13 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16154: CGI->multi_param - Declare a list (71.48 KB, patch)
2016-04-13 15:42 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16154: CGI->multi_param - Manual changes (5.84 KB, patch)
2016-04-13 15:42 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16154: CGI->multi_param - Force scalar context (63.14 KB, patch)
2016-04-13 15:42 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16154: CGI->multi_param - Assign a list (9.28 KB, patch)
2016-04-13 15:42 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16154: Fix some other occurrences (20.96 KB, patch)
2016-04-13 16:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16154: Fix some other occurrences (21.06 KB, patch)
2016-04-14 12:02 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16154: CGI->multi_param - Declare a list (71.38 KB, patch)
2016-04-25 15:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16154: CGI->multi_param - Manual changes (5.74 KB, patch)
2016-04-25 15:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16154: CGI->multi_param - Force scalar context (63.03 KB, patch)
2016-04-25 15:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16154: CGI->multi_param - Assign a list (9.17 KB, patch)
2016-04-25 15:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16154: Fix some other occurrences (20.96 KB, patch)
2016-04-25 15:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 16154: CGI->multi_param - Declare a list (71.45 KB, patch)
2016-04-26 20:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 16154: CGI->multi_param - Manual changes (5.81 KB, patch)
2016-04-26 20:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 16154: CGI->multi_param - Force scalar context (63.10 KB, patch)
2016-04-26 20:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 16154: CGI->multi_param - Assign a list (9.24 KB, patch)
2016-04-26 20:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 16154: Fix some other occurrences (21.02 KB, patch)
2016-04-26 20:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 16154: CGI->multi_param - Declare a list (71.50 KB, patch)
2016-04-26 21:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 16154: CGI->multi_param - Manual changes (5.86 KB, patch)
2016-04-26 21:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 16154: CGI->multi_param - Force scalar context (63.15 KB, patch)
2016-04-26 21:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 16154: CGI->multi_param - Assign a list (9.29 KB, patch)
2016-04-26 21:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 16154: Fix some other occurrences (21.07 KB, patch)
2016-04-26 21:02 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 Jonathan Druart 2016-03-29 09:16:00 UTC
Bug 15809 is now pushed, we are able to replace the occurrences of CGI->param with CGI->multi_param when needed (in list context).

  my @foo = $cgi->param('foo');
must be replaced with
  my @foo = $cgi->multi_param('foo');

and

  $template->param( foo => $cgi->param('foo') );
must be replaced with
  $template->param( foo => scalar $cgi->param('foo') );
Comment 1 Jonathan Druart 2016-03-29 09:28:02 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-03-29 09:28:06 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2016-03-29 09:28:12 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-03-29 09:28:16 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2016-03-29 09:35:13 UTC
This is the kind of patchset that does not have a clear and specific test plan.
The expected goal is to 1. get rid of the warning "CGI::param called in list context" in logs, 2. fix the vulnerability caused by the calls of CGI->param in list context (see description of bug 15809 if you need more info about that).

An important thing to check is that the subroutines modified by this patch should not be called from scripts which do not use C4::Context (if multi_param is called).
Comment 6 Jonathan Druart 2016-03-29 09:39:55 UTC
*** Bug 16150 has been marked as a duplicate of this bug. ***
Comment 7 Owen Leonard 2016-04-12 13:26:38 UTC
Applying: Bug 16154: CGI->multi_param - Declare a list
Applying: Bug 16154: CGI->multi_param - Manual changes
Applying: Bug 16154: CGI->multi_param - Force scalar context
fatal: sha1 information is lacking or useless (admin/patron-attr-types.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 16154: CGI->multi_param - Force scalar context
Comment 8 Jonathan Druart 2016-04-13 08:24:34 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2016-04-13 08:24:38 UTC Comment hidden (obsolete)
Comment 10 Owen Leonard 2016-04-13 15:42:16 UTC Comment hidden (obsolete)
Comment 11 Owen Leonard 2016-04-13 15:42:24 UTC Comment hidden (obsolete)
Comment 12 Owen Leonard 2016-04-13 15:42:31 UTC Comment hidden (obsolete)
Comment 13 Owen Leonard 2016-04-13 15:42:38 UTC Comment hidden (obsolete)
Comment 14 Owen Leonard 2016-04-13 15:48:33 UTC
I tried to go through all the affected pages looking for problems and everything looked good, nothing broken.

I did find errors in my log for some files, so I think a follow-up is required:

acqui/histsearch.pl
admin/aqbudgetperiods.pl
admin/aqcontract.pl
admin/authorised_values.pl
admin/branches.pl
admin/categories.pl
admin/patron-attr-types.pl
admin/preferences.pl
catalogue/itemsearch.pl
catalogue/search.pl
circ/circulation.pl
patroncards/add_user_search.pl
reserve/request.pl
serials/add_user_search.pl
serials/checkexpiration.pl
tools/koha-news.pl
tools/letter.pl
virtualshelves/shelves.pl

Each of these left an error in my log about "CGI::param called in list context"
Comment 15 Jonathan Druart 2016-04-13 16:52:33 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2016-04-13 16:53:56 UTC
(In reply to Owen Leonard from comment #14)
> I tried to go through all the affected pages looking for problems and
> everything looked good, nothing broken.
> 
> I did find errors in my log for some files, so I think a follow-up is
> required

Thanks Owen for testing.
This follow-up won't fix all the files you listed, but almost all of them.

I'd suggest to open another bug report to catching the others.
Comment 17 Owen Leonard 2016-04-14 12:02:17 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2016-04-25 10:54:28 UTC
Sorry, patches no longer apply cleanly, can you please rebase?

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 16154: CGI->multi_param - Declare a list
Applying: Bug 16154: CGI->multi_param - Manual changes
Applying: Bug 16154: CGI->multi_param - Force scalar context
fatal: sha1 information is lacking or useless (admin/patron-attr-types.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 16154: CGI->multi_param - Force scalar context
The copy of the patch that failed is found in:
   /home/katrin/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/SIGNED-OFF-Bug-16154-CGI-multiparam---Force-scalar-CQshm9.patch
Comment 19 Jonathan Druart 2016-04-25 15:27:49 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2016-04-25 15:27:53 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2016-04-25 15:27:58 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2016-04-25 15:28:02 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2016-04-25 15:28:08 UTC Comment hidden (obsolete)
Comment 24 Katrin Fischer 2016-04-26 20:56:55 UTC Comment hidden (obsolete)
Comment 25 Katrin Fischer 2016-04-26 20:56:59 UTC Comment hidden (obsolete)
Comment 26 Katrin Fischer 2016-04-26 20:57:03 UTC Comment hidden (obsolete)
Comment 27 Katrin Fischer 2016-04-26 20:57:06 UTC Comment hidden (obsolete)
Comment 28 Katrin Fischer 2016-04-26 20:57:09 UTC Comment hidden (obsolete)
Comment 29 Katrin Fischer 2016-04-26 20:59:48 UTC
It looks like we lost Owen's sign-off line at some point. I am adding it back.
Comment 30 Katrin Fischer 2016-04-26 21:01:53 UTC
Created attachment 50757 [details] [review]
[PASSED QA] Bug 16154: CGI->multi_param - Declare a list

This patch replaces the occurrences of
  my @foo = $cgi->param('foo');
with
  my @foo = $cgi->multi_param('foo');

perl -p -i -e
's/^(\s*my\s*@\w+\s*=\s*)\$(cgi|input|query)\->param\(/$1\$$2\->multi_param\(/xms'
**/*.pl

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 31 Katrin Fischer 2016-04-26 21:01:56 UTC
Created attachment 50758 [details] [review]
[PASSED QA] Bug 16154: CGI->multi_param - Manual changes

This patch does the same as the previous one, but affects lines which
have not been caught by the regex.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 32 Katrin Fischer 2016-04-26 21:02:00 UTC
Created attachment 50759 [details] [review]
[PASSED QA] Bug 16154: CGI->multi_param - Force scalar context

This patch replaces the occurrences of
  $template->param( foo => $cgi->param('foo') );
with
  $template->param( foo => scalar $cgi->param('foo') );

perl -p -i -e 's/(\s*=>\s*)\$(cgi|input|query)\->param\(/$1scalar
\$$2\->param\(/xms' **/*.pl

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 33 Katrin Fischer 2016-04-26 21:02:03 UTC
Created attachment 50760 [details] [review]
[PASSED QA] Bug 16154: CGI->multi_param - Assign a list

This patch replaces the occurrences of
  @foo = $cgi->param('foo');
with
  @foo = $cgi->multi_param('foo');

perl -p -i -e
's/^(\s*@\w+\s*=\s*)\$(cgi|input|query)\->param\(/$1\$$2\->multi_param\(/xms'
**/*.pl

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 34 Katrin Fischer 2016-04-26 21:02:07 UTC
Created attachment 50761 [details] [review]
[PASSED QA] Bug 16154: Fix some other occurrences

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 35 Brendan Gallagher 2016-04-26 23:18:00 UTC
Pushed to Master - Should be in the May 2016 Release.  Thanks!
Comment 36 Julian Maurice 2016-05-16 12:45:20 UTC
Patches pushed to 3.22.x, will be in 3.22.7