Bug 14121 - Silence warnings t/db_dependent/Auth_with_cas.t
Summary: Silence warnings t/db_dependent/Auth_with_cas.t
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Mark Tompsett
QA Contact: Galen Charlton
URL:
Keywords:
Depends on:
Blocks: 8879
  Show dependency treegraph
 
Reported: 2015-05-02 04:30 UTC by Mark Tompsett
Modified: 2016-12-05 21:22 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 14121: Silence warnings t/db_dependent/Auth_with_cas.t (1.27 KB, patch)
2015-05-02 04:33 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14121: Silence warnings t/db_dependent/Auth_with_cas.t (1.96 KB, patch)
2015-05-27 20:13 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14121: Silence warnings t/db_dependent/Auth_with_cas.t (2.03 KB, patch)
2015-06-26 22:17 UTC, Nick Clemens
Details | Diff | Splinter Review
[PASSED QA] Bug 14121: Silence warnings t/db_dependent/Auth_with_cas.t (2.21 KB, patch)
2015-07-10 15:46 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2015-05-02 04:30:52 UTC
C4::Auth_with_case has a $query->param($_) in a context which is not clearly scalar to the interpreter, so it throws a security warning.

Introduce a variable, use the variable instead of calling param() twice. Noise disappears.
Comment 1 Mark Tompsett 2015-05-02 04:33:25 UTC Comment hidden (obsolete)
Comment 2 Bernardo Gonzalez Kriegel 2015-05-15 22:35:10 UTC
No warns on current master...
Comment 3 Mark Tompsett 2015-05-15 23:57:06 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #2)
> No warns on current master...

Apache 2.4 issue.
Comment 4 Indranil Das Gupta 2015-05-16 09:35:24 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #2)
> No warns on current master...

this is what I have (gitified install):

l2c2technologies@gadfly:~/koha$ apache2 -v
Server version: Apache/2.4.7 (Ubuntu)
Server built:   Mar 10 2015 13:05:59

test result

l2c2technologies@gadfly:~/koha$ prove t/db_dependent/Auth_with_cas.t
t/db_dependent/Auth_with_cas.t .. ok   
All tests successful.
Files=1, Tests=4,  1 wallclock secs ( 0.01 usr  0.01 sys +  0.18 cusr  0.00 csys =  0.20 CPU)
Result: PASS
Comment 5 Bernardo Gonzalez Kriegel 2015-05-16 11:43:15 UTC
(In reply to M. Tompsett from comment #3)
> (In reply to Bernardo Gonzalez Kriegel from comment #2)
> > No warns on current master...
> 
> Apache 2.4 issue.

Server version: Apache/2.4.7 (Ubuntu)

HEAD: 607b224 Bug 13591: Pass OPACBaseURL to rss creation process

prove t/db_dependent/Auth_with_cas.t
t/db_dependent/Auth_with_cas.t .. ok   
All tests successful.
Files=1, Tests=4,  1 wallclock secs ( 0.04 usr  0.00 sys +  0.29 cusr  0.01 csys =  0.34 CPU)
Result: PASS

No noise for me!
Comment 6 Mark Tompsett 2015-05-16 22:00:01 UTC
mtompset@debian:~/kohaclone$ git reset --hard origin/master
HEAD is now at 607b224 Bug 13591: Pass OPACBaseURL to rss creation process
mtompset@debian:~/kohaclone$ prove t/db_dependent/Auth_with_cas.t
t/db_dependent/Auth_with_cas.t .. 1/4 CGI::param called in list context from package C4::Auth_with_cas line 217, 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.
CGI::param called in list context from package C4::Auth_with_cas line 217, 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.
t/db_dependent/Auth_with_cas.t .. ok
All tests successful.
Files=1, Tests=4,  1 wallclock secs ( 0.02 usr  0.00 sys +  0.16 cusr  0.01 csys =  0.19 CPU)
Result: PASS
mtompset@debian:~/kohaclone$ git bz apply 14121
Bug 14121 - Silence warnings t/db_dependent/Auth_with_cas.t

38763 - Bug 14121: Silence warnings t/db_dependent/Auth_with_cas.t

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 14121: Silence warnings t/db_dependent/Auth_with_cas.t
mtompset@debian:~/kohaclone$ prove t/db_dependent/Auth_with_cas.t
t/db_dependent/Auth_with_cas.t .. ok
All tests successful.
Files=1, Tests=4,  0 wallclock secs ( 0.01 usr  0.00 sys +  0.16 cusr  0.00 csys =  0.17 CPU)
Result: PASS


mtompset@debian:~/dhmake$ cpan2deb CGI
== dh-make-perl 0.84 ==
CGI is a standard module. Will not build without --core-ok.
mtompset@debian:~/dhmake$ perl -v

This is perl 5, version 20, subversion 2 (v5.20.2) built for x86_64-linux-gnu-thread-multi
(with 40 registered patches, see perl -V for more detail)

Copyright 1987-2015, Larry Wall

Perl may be copied only under the terms of either the Artistic License or the
GNU General Public License, which may be found in the Perl 5 source kit.

Complete documentation for Perl, including FAQ lists, should be found on
this system using "man perl" or "perldoc perl".  If you have access to the
Internet, point your browser at http://www.perl.org/, the Perl Home Page.

I'm testing Debian Jessie with perl 5.20.2
Both of you were testing Ubuntu.
Comment 7 Mark Tompsett 2015-05-16 22:07:26 UTC
I attempted to test with Ubuntu 14.04 LTS which has Perl 5.18.2, and I did not get noise either. This is clearly a newer perl (hence newer CGI) issue.
Comment 8 Aleisha Amohia 2015-05-26 22:30:10 UTC
t/db_dependent/Auth_with_cas.t .. 1/4 
#   Failed test 'Intranet URL should be returned when using intranet login (Bug 13507)'
#   at t/db_dependent/Auth_with_cas.t line 60.
#          got: 'localhost/cgi-bin/koha/opac-user.pl?bar=baz'
#     expected: 'localhost:8080?bar=baz'
# Looks like you failed 1 test of 4.
t/db_dependent/Auth_with_cas.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/4 subtests 

Test Summary Report
-------------------
t/db_dependent/Auth_with_cas.t (Wstat: 256 Tests: 4 Failed: 1)
  Failed test:  4
  Non-zero exit status: 1
Files=1, Tests=4,  1 wallclock secs ( 0.02 usr  0.00 sys +  0.23 cusr  0.02 csys =  0.27 CPU)
Result: FAIL

Setting to Failed QA.
Comment 9 Mark Tompsett 2015-05-27 04:10:59 UTC
(In reply to Aleisha Amohia from comment #8)
> t/db_dependent/Auth_with_cas.t .. 1/4 
> #   Failed test 'Intranet URL should be returned when using intranet login
> (Bug 13507)'
> #   at t/db_dependent/Auth_with_cas.t line 60.
> #          got: 'localhost/cgi-bin/koha/opac-user.pl?bar=baz'
> #     expected: 'localhost:8080?bar=baz'
> # Looks like you failed 1 test of 4.
> t/db_dependent/Auth_with_cas.t .. Dubious, test returned 1 (wstat 256, 0x100)
> Failed 1/4 subtests 

I can't replicate this failure. If I can't replicate, I can't fix. Could you retest on a fresh branch after a git pull? Thanks.
Comment 10 Mark Tompsett 2015-05-27 20:13:28 UTC Comment hidden (obsolete)
Comment 11 Mark Tompsett 2015-05-27 20:40:27 UTC
(In reply to M. Tompsett from comment #7)
> I attempted to test with Ubuntu 14.04 LTS which has Perl 5.18.2, and I did
> not get noise either. This is clearly a newer perl (hence newer CGI) issue.

I tested this on Debian Wheezy which has Perl 5.14.2 with no noise either, so this should be tested on a machine with Debian Jessie, so the issue will be triggered. Test plan in comment #10.
Comment 12 Nick Clemens 2015-06-26 22:17:59 UTC Comment hidden (obsolete)
Comment 13 Nick Clemens 2015-06-26 22:19:07 UTC
Cleared a small conflict
Comment 14 Mark Tompsett 2015-06-26 22:25:11 UTC
(In reply to Nick Clemens from comment #13)
> Cleared a small conflict

Thanks. Looks like a correct fix. :)
Comment 15 Jonathan Druart 2015-07-09 13:50:28 UTC
% more /etc/debian_version
jessie/sid

% perl --version | grep version
This is perl 5, version 20, subversion 2 (v5.20.2) built for i586-linux-gnu-thread-multi-64int

% git reset --hard origin/master

% prove t/db_dependent/Auth_with_cas.t
t/db_dependent/Auth_with_cas.t .. ok   
All tests successful.
Files=1, Tests=4,  1 wallclock secs ( 0.02 usr  0.00 sys +  0.96 cusr  0.02 csys =  1.00 CPU)
Result: PASS

No warn here.
Comment 16 Jonathan Druart 2015-07-09 13:51:29 UTC
% pmvers CGI
3.65
Comment 17 Jonathan Druart 2015-07-09 13:56:55 UTC
I suppose it comes from this line:
  217             $uri_params_part .= URI::Escape::uri_escape( $query->param($_) );

Could you confirm?
Comment 18 Mark Tompsett 2015-07-09 17:09:23 UTC
(In reply to Jonathan Druart from comment #17)
> I suppose it comes from this line:
>   217             $uri_params_part .= URI::Escape::uri_escape(
> $query->param($_) );
> 
> Could you confirm?

Yes, the other change is an optimization. It's faster to access a variable than function call twice.
Comment 19 Mark Tompsett 2015-07-09 17:20:06 UTC
mtompset@debian:~/kohaclone$ cat /etc/debian_version
8.1
mtompset@debian:~/kohaclone$ perl --version | grep version
This is perl 5, version 20, subversion 2 (v5.20.2) built for x86_64-linux-gnu-thread-multi
mtompset@debian:~/kohaclone$ git reset --hard origin/master
HEAD is now at 6e9086f Bug 3206: (QA followup) missing comma on sysprefs.sql
mtompset@debian:~/kohaclone$ prove t/db_dependent/Auth_with_cas.t
t/db_dependent/Auth_with_cas.t .. 1/4 CGI::param called in list context from package C4::Auth_with_cas line 217, 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.
CGI::param called in list context from package C4::Auth_with_cas line 217, 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.
t/db_dependent/Auth_with_cas.t .. ok
All tests successful.
Files=1, Tests=4,  0 wallclock secs ( 0.02 usr  0.00 sys +  0.67 cusr  0.01 csys =  0.70 CPU)
Result: PASS
mtompset@debian:~/kohaclone$ pmvers CGI
4.09
Comment 20 Jonathan Druart 2015-07-10 07:05:06 UTC
(In reply to M. Tompsett from comment #19)
> mtompset@debian:~/kohaclone$ pmvers CGI
> 4.09

Did you install it using from CPAN?
Comment 21 Mark Tompsett 2015-07-10 13:28:15 UTC
(In reply to Jonathan Druart from comment #20)
> Did you install [CGI] using from CPAN?

https://packages.debian.org/jessie/libcgi-pm-perl
Not that I can tell. I avoid CPAN like the plague. And the latest is 4.21 as far as I can tell.

JD wrote:
> % more /etc/debian_version
> jessie/sid

I wrote:
> mtompset@debian:~/kohaclone$ cat /etc/debian_version
> 8.1

Your debian version is an experimental pre-release of Jessie? Try a fresher install?
Comment 22 Kyle M Hall 2015-07-10 15:46:44 UTC
Created attachment 40936 [details] [review]
[PASSED QA] Bug 14121: Silence warnings t/db_dependent/Auth_with_cas.t

TEST PLAN
---------
1) $ prove t/db_dependent/Auth_with_cas.t
   -- CGI security warning
2) apply patch
3) $ prove t/db_dependent/Auth_with_cas.t
   -- no noise.
4) koha qa test tools

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Not able to reproduce the error on my setup, but the code
is a clear improvement over the previous version.
Comment 23 Tomás Cohen Arazi 2015-07-20 14:07:30 UTC
Patch pushed to master.

Thanks Mark!
Comment 24 Chris Cormack 2015-07-21 22:05:06 UTC
Pushed to 3.20.x will be in 3.20.2