To reproduce: 1) Ensure UseCourseReserves is enabled 2) Go to Course Reserves, create a course 3) Delete course 4) Notice warn CGI::param called in list context from package CGI::Compile::ROOT::home_vagrant_kohaclone_course_reserves_mod_course_2epl line 43, 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.
Created attachment 66678 [details] [review] Bug 19230: Preventing warn when deleting course To test: 1) Ensure UseCourseReserves is enabled 2) Go to Course Reserves, create a course 3) Delete course 4) Notice warn 5) Apply patch 6) Create a course again and delete it 7) Notice no warn Sponsored-by: Catalyst IT
Created attachment 67797 [details] [review] Bug 19230: Preventing warn when deleting course To test: 1) Ensure UseCourseReserves is enabled 2) Go to Course Reserves, create a course 3) Delete course 4) Notice warn 5) Apply patch 6) Create a course again and delete it 7) Notice no warn Sponsored-by: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
I think it would be better to assign a $course_id variable and reuse it. Something like: modified: course_reserves/mod_course.pl @ mod_course.pl:41 @ my ( $template, $loggedinuser, $cookie ) = get_template_and_user( ); my $action = $cgi->param('action') || ''; +my $course_id = $cgi->param('course_id'); if ( $action eq 'del' ) { - DelCourse( $cgi->param('course_id') ); + DelCourse( $course_id ); print $cgi->redirect("/cgi-bin/koha/course_reserves/course-reserves.pl"); } else { my %params; - $params{'course_id'} = $cgi->param('course_id') - if ( $cgi->param('course_id') ); + $params{'course_id'} = $course_id; $params{'department'} = $cgi->param('department'); $params{'course_number'} = $cgi->param('course_number'); $params{'section'} = $cgi->param('section'); @ mod_course.pl:60 @ if ( $action eq 'del' ) { $params{'students_count'} = $cgi->param('students_count'); $params{'enabled'} = ( $cgi->param('enabled') eq 'on' ) ? 'yes' : 'no'; - my $course_id = ModCourse(%params); + my $new_course_id = ModCourse(%params); my @instructors = $cgi->multi_param('instructors'); ModCourseInstructors( mode => 'replace', cardnumbers => \@instructors, - course_id => $course_id + course_id => $new_course_id ); - print $cgi->redirect("/cgi-bin/koha/course_reserves/course-details.pl?course_id=$course_id"); + print $cgi->redirect("/cgi-bin/koha/course_reserves/course-details.pl?course_id=$new_course_id"); } Would it make more sense?
Created attachment 69946 [details] [review] Bug 19230: Preventing warn when deleting course To test: 1) Ensure UseCourseReserves is enabled 2) Go to Course Reserves, create a course 3) Delete course 4) Notice warn 5) Apply patch 6) Create a course again and delete it 7) Notice no warn Sponsored-by: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
(In reply to Jonathan Druart from comment #3) > Would it make more sense? hi Jonathan, Yes I believe your fix makes more sense so I have implemented in the next patch. When I was testing, I also found a new warn when looking at a course with NO course reserves in it (course_reserves/course-details.pl) Argument "" isn't numeric in numeric gt (>) at /home/vagrant/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course-details.tt line 219. Also, when deleting a course with no course reserves, the confirm message is not triggered. I've added a fix for this also in my next patch.
Created attachment 69985 [details] [review] Bug 19230: Preventing warn when deleting course To test: 1) Ensure UseCourseReserves is enabled 2) Go to Course Reserves, create a course 3) When you are taken to the course page, you should notice the 'Argument "" isn't numeric in numeric gt' warn 4) Delete course 5) Notice warn, and that no confirm message popped up 6) Apply patch 7) Create a course again. Confirm the 'Argument "" isn't numeric in numeric gt' warn did not show 8) Delete the course 9) Notice no warn and the confirm message pops up Sponsored-by: Catalyst IT
Created attachment 70780 [details] [review] Bug 19230: Preventing warn when deleting course To test: 1) Ensure UseCourseReserves is enabled 2) Go to Course Reserves, create a course 3) When you are taken to the course page, you should notice the 'Argument "" isn't numeric in numeric gt' warn 4) Delete course 5) Notice warn, and that no confirm message popped up 6) Apply patch 7) Create a course again. Confirm the 'Argument "" isn't numeric in numeric gt' warn did not show 8) Delete the course 9) Notice no warn and the confirm message pops up Sponsored-by: Catalyst IT Signed-off-by: Roch D'Amour <roch.damour@inlibro.com>
Created attachment 70933 [details] [review] Bug 19230: Preventing warn when deleting course To test: 1) Ensure UseCourseReserves is enabled 2) Go to Course Reserves, create a course 3) When you are taken to the course page, you should notice the 'Argument "" isn't numeric in numeric gt' warn 4) Delete course 5) Notice warn, and that no confirm message popped up 6) Apply patch 7) Create a course again. Confirm the 'Argument "" isn't numeric in numeric gt' warn did not show 8) Delete the course 9) Notice no warn and the confirm message pops up Sponsored-by: Catalyst IT Signed-off-by: Roch D'Amour <roch.damour@inlibro.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 71067 [details] [review] Bug 19230: (follow-up) Preventing warn when deleting course Simplify 'unless' statement
Pushed to master for 18.05, thanks to everybody involved!
Awesome work all! This patch has been pushed to 17.11.x and will be in 17.11.03