https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 --- Comment #1 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 47901 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=47901&action=edit Bug 15747 Auth.pm flooding error log with "CGI::param called in list context" TEST PLAN --------- 1) git checkout -b bug_15747 origin/master 2) echo > ~/koha-dev/var/log/koha-error_log 3) open up staff interface and log in. 4) cat ~/koha-dev/var/log/koha-error_log --- BEGIN SNIP --- [Thu Feb 11 09:49:43.627711 2016] [cgi:error] [pid 2132] [client 192.168.71.222: 51107] AH01215: [Thu Feb 11 09:49:43 2016] mainpage.pl: CGI::param called in lis t context from package C4::Auth line 401, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /us r/share/perl5/CGI.pm line 436., referer: http://library-admin.debiankoha.ca/ --- END SNIP --- 5) logout 6) echo > ~/koha-dev/var/log/koha-error_log 7) log back in. 8) cat ~/koha-dev/var/log/koha-error_log -- this time should not be there. 9) koha qa test tools -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #47901|0 |1 is obsolete| | --- Comment #2 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 47903 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=47903&action=edit Bug 15747 Auth.pm flooding error log with "CGI::param called in list context" TEST PLAN --------- 1) git checkout -b bug_15747 origin/master 2) echo > ~/koha-dev/var/log/koha-error_log 3) open up staff interface and log in. 4) cat ~/koha-dev/var/log/koha-error_log *** BEGIN SNIP *** [Thu Feb 11 09:49:43.627711 2016] [cgi:error] [pid 2132] [client 192.168.71.222:51107] AH01215: [Thu Feb 11 09:49:43 2016] mainpage.pl: CGI::param called in list context from package C4::Auth line 401, 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://library-admin.debiankoha.ca/ *** END SNIP *** 5) logout 6) apply patch 7) echo > ~/koha-dev/var/log/koha-error_log 8) log back in. 9) cat ~/koha-dev/var/log/koha-error_log -- this time should not be there. 10) koha qa test tools -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #47903|0 |1 is obsolete| | --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 47960 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=47960&action=edit Bug 15747 Auth.pm flooding error log with "CGI::param called in list context" TEST PLAN --------- 1) git checkout -b bug_15747 origin/master 2) echo > ~/koha-dev/var/log/koha-error_log 3) open up staff interface and log in. 4) cat ~/koha-dev/var/log/koha-error_log *** BEGIN SNIP *** [Thu Feb 11 09:49:43.627711 2016] [cgi:error] [pid 2132] [client 192.168.71.222:51107] AH01215: [Thu Feb 11 09:49:43 2016] mainpage.pl: CGI::param called in list context from package C4::Auth line 401, 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://library-admin.debiankoha.ca/ *** END SNIP *** 5) logout 6) apply patch 7) echo > ~/koha-dev/var/log/koha-error_log 8) log back in. 9) cat ~/koha-dev/var/log/koha-error_log -- this time should not be there. 10) koha qa test tools Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Warning probably depends on the version of perl/CGI. But this clearly makes it a scalar. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |15809 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15809 [Bug 15809] versions of CGI < 4.08 do not have multi_param -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #47960|0 |1 is obsolete| | --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 47966 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=47966&action=edit Bug 15747: Do not use CGI->param in list context - Auth.pm Test plan: No idea! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #7 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Jonathan: What happened here? Previous patch, status etc ?? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I was working on bug 14075 and bug 15809 at the same time (same issue). I am trying to find a global solution to fix them. The previous patch uses an unneeded var. To remove the warnings all around the code, it would be good to have the same pattern everywhere. That's why I have submitted another patch. To be sure everything is fine (even if I am sure it is), I was trying to understand how I could test there is no regression. I have asked Jesse to explain the goal of the auth_forwarded_hash and how it can be tested. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 --- Comment #11 from M. Tompsett <mtompset@hotmail.com> --- (In reply to Jonathan Druart from comment #9)

I was working on bug 14075 and bug 15809 at the same time (same issue). I am trying to find a global solution to fix them.

Oooo... bug 15809 looks promising. I'm okay with this simplified patch. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 --- Comment #13 from M. Tompsett <mtompset@hotmail.com> --- (In reply to Marcel de Rooy from comment #12)

That is fine. Well, Jonathan, go for it. Can we close this one?

I don't think so, because this doesn't use the 15809 patch. If it did, then it would be multi_param. :) Either way, I'm cool with this (without 15809) or a multi-param version dependent on 15809. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 --- Comment #15 from Jesse Weaver <jweaver@bywatersolutions.com> --- (In reply to Jesse Weaver from comment #14)

(In reply to Jonathan Druart from comment #4)

...

Jesse, could you explain what you are doing with document.location.hash?

I do not understand the following line in includes/cateditor-ui.inc:

1094 if ( "[% auth_forwarded_hash %]" ) { 1095 document.location.hash = "[% auth_forwarded_hash %]"; 1096 }

Should not be:

1094 [% IF auth_forwarded_hash %] 1095 document.location.hash = "[% auth_forwarded_hash %]"; 1096 [% END %]

instead?

I don't quite remember why I did it that way. Your proposed change should be fine.

And to clarify; this is to make sure that the hash in .../editor.pl#catalog/647 is passed through when you have to log in again. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #47966|0 |1 is obsolete| | --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 48067 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=48067&action=edit Bug 15747: Do not use CGI->param in list context - Auth.pm Test plan: Log you in! Without this patch applied, you will get a warning "Fetching the value or values of a single named parameter" With this patch applied, the warning from C4::Auth line 401 should not appear anymore in the log file. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #18 from David Cook <dcook@prosentient.com.au> --- Jonathan's patch looks good - provided that $in->{'query'}->param('auth_forwarded_hash') is a single value and not a multi value. (I assume that it is a single value.) I'd like to clarify one thing though. The warning "Do not use CGI->param in list context" is not strictly necessary. The problem isn't with CGI->param() in list context per se but rather with CGI->param("foo") in list context. I'm using "my @param_names = CGI->param()" elsewhere and it doesn't generate warnings. It only generates warnings if you're passing the method an argument, as it introduces some ambiguity and the possibility of buggy behaviour. Here's the example from CGI: my %user_info = ( id => 1, name => $query->param('name'), ); In theory, someone could submit multiple "name" arguments, so the above could actually be interpolated like this: my %user_info = ( id => 1, name => "bruce", "wayne", "clark", "kent" ); If you run the code with that list as the value for "name", you'll get the following hash after interpolation: 'id' => 1, 'name' => 'bruce', 'kent' => undef, 'wayne' => 'clark' That's definitely a vulnerability. So if you know that you only have one "name", you can use "scalar $query->param('name')". However, if you know that you have multiple names, but don't want to wind up with a mangled hash, I think you should be able to do the following: my %user_info = ( id => 1, name => [$query->param('name')], ); That should give you the following: 'id' => 1, 'name' => [ 'bruce', 'wayne', 'clark', 'kent' ] Of course, I think you'd still get the warning even if you did $query->param('name'), which is why CGI->multi_param probably makes more sense as per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15809. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to David Cook from comment #18)

I'm using "my @param_names = CGI->param()" elsewhere and it doesn't generate warnings.

Yes it does. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 --- Comment #22 from David Cook <dcook@prosentient.com.au> --- P.S. That is CGI.pm version 4.13. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 --- Comment #24 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #23)

(In reply to David Cook from comment #21)

...

As you can observe in the warning itself, the warning is for "Fetching the value or values of a single named parameter". In the case of '$query->param("name")', we're fetching the value of a single named parameter. In the case of '$query->param()', we're fetching the names of all the parameters. It's completely different.

I don't understand, we both agree, it generates a warning.

Misunderstanding? He talks about calling it without arguments (and without warning). You mean calls with an argument (and warnings dependent on context). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|15809 | See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=15809 --- Comment #26 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Yes sorry, didn't read carefully enough. What's missing to get a signoff? Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15809 [Bug 15809] versions of CGI < 4.08 do not have multi_param -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | --- Comment #29 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QA Comment: As you can see, this already received (too) much attention :) Taking the liberty to promote this trivial patch to PQA therefore. Note: I checked the specific variable auth_forwarded_hash. It is used to store the document.location.hash. Single value! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com Status|Pushed to Master |Pushed to Stable --- Comment #31 from Julian Maurice <julian.maurice@biblibre.com> --- Patch pushed to 3.22.x, will be in 3.22.4 -- You are receiving this mail because: You are watching all bug changes.