When exporting a basket with the default CSV profile: Use of uninitialized value in string eq at /home/vagrant/kohaclone/acqui/basket.pl line 173. CGI::param called in list context from package CGI::Compile::ROOT::home_vagrant_kohaclone_acqui_basket_2epl line 177, 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. When exporting a basket with some other CSV profile: CGI::param called in list context from package CGI::Compile::ROOT::home_vagrant_kohaclone_acqui_basket_2epl line 177, 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 66602 [details] [review] Bug 19200: Preventing warns when exporting a basket To test: 1) Go to Tools -> CSV profiles -> New CSV Profile 2) Create a new CSV profile with any name of SQL fields. Ensure profile type: SQL and usage: basket export in acquisition 3) Go to Acquisitions -> Find or create a vendor -> Use or create a basket 4) Click the dropdown menu next to the 'Export as CSV' button. There should be a 'Default' option and your new CSV profile (at least) 5) Click the 'Default' option. Notice warns 6) Click the 'Export as CSV' button. Notice warns 7) Click your new CSV profile option. Notice warns 8) Apply patch and refresh page 9) Repeat steps 5-7, confirm that warns do not show 10) Confirm export still works as expected Sponsored-by: Catalyst IT
Created attachment 67799 [details] [review] Bug 19200: Preventing warns when exporting a basket To test: 1) Go to Tools -> CSV profiles -> New CSV Profile 2) Create a new CSV profile with any name of SQL fields. Ensure profile type: SQL and usage: basket export in acquisition 3) Go to Acquisitions -> Find or create a vendor -> Use or create a basket 4) Click the dropdown menu next to the 'Export as CSV' button. There should be a 'Default' option and your new CSV profile (at least) 5) Click the 'Default' option. Notice warns 6) Click the 'Export as CSV' button. Notice warns 7) Click your new CSV profile option. Notice warns 8) Apply patch and refresh page 9) Repeat steps 5-7, confirm that warns do not show 10) Confirm export still works as expected Sponsored-by: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
This looks like a change in behavior. When the cgi param is default, no profile id was passed. I guess what you wanted to do was just if( $param && $param eq ... ) ?
(In reply to Marcel de Rooy from comment #3) > This looks like a change in behavior. When the cgi param is default, no > profile id was passed. > I guess what you wanted to do was just if( $param && $param eq ... ) ? Hi Marcel, there is no change in behaviour in my patch. It looks like there's originally a bug here, because the original code says "if $query->param('csv_profile') eq 'default'", but the word 'default' is never passed through that param, based on acqui/basket.tt. this means that when the Default option was selected, the $query->param('csv_profile') is not actually set to 'default' so an uninitialised value is sent to the GetBasketAsCSV method and the 'if' condition is never true. (I hope that makes sense. If you put a warn inside the if statement under where $query->param('csv_profile') eq default, you'll see that it never enters here and never prints the warn.) my patch checks if the $query->param('csv_profile') has been defined. if it has, then it uses one of the library-made csv profiles. if it hasn't been defined then it must be using the default profile and no ID is passed, as intended, with no warns. setting back to needs signoff as I believe this is still a valid patch.
Created attachment 69959 [details] [review] Bug 19200: Preventing warns when exporting a basket To test: 1) Go to Tools -> CSV profiles -> New CSV Profile 2) Create a new CSV profile with any name of SQL fields. Ensure profile type: SQL and usage: basket export in acquisition 3) Go to Acquisitions -> Find or create a vendor -> Use or create a basket 4) Click the dropdown menu next to the 'Export as CSV' button. There should be a 'Default' option and your new CSV profile (at least) 5) Click the 'Default' option. Notice warns 6) Click the 'Export as CSV' button. Notice warns 7) Click your new CSV profile option. Notice warns 8) Apply patch and refresh page 9) Repeat steps 5-7, confirm that warns do not show 10) Confirm export still works as expected Sponsored-by: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Is the if() else() statement still usefull? Why not just one call to GetBasketAsCSV?
QA: Resuming here
(In reply to Aleisha Amohia from comment #4) > (In reply to Marcel de Rooy from comment #3) > > This looks like a change in behavior. When the cgi param is default, no > > profile id was passed. > > I guess what you wanted to do was just if( $param && $param eq ... ) ? > > Hi Marcel, > > there is no change in behaviour in my patch. It looks like there's > originally a bug here, because the original code says "if > $query->param('csv_profile') eq 'default'", but the word 'default' is never > passed through that param, based on acqui/basket.tt. this means that when > the Default option was selected, the $query->param('csv_profile') is not > actually set to 'default' so an uninitialised value is sent to the > GetBasketAsCSV method and the 'if' condition is never true. (I hope that > makes sense. If you put a warn inside the if statement under where > $query->param('csv_profile') eq default, you'll see that it never enters > here and never prints the warn.) > > my patch checks if the $query->param('csv_profile') has been defined. if it > has, then it uses one of the library-made csv profiles. if it hasn't been > defined then it must be using the default profile and no ID is passed, as > intended, with no warns. > > setting back to needs signoff as I believe this is still a valid patch. You are right that the compare with 'default' makes no sense. It may have been a leftover from the past.
(In reply to Alex Arnaud from comment #6) > Is the if() else() statement still usefull? Why not just one call to > GetBasketAsCSV? Yes. Adding a follow-up for that.
Created attachment 70113 [details] [review] Bug 19200: Preventing warns when exporting a basket To test: 1) Go to Tools -> CSV profiles -> New CSV Profile 2) Create a new CSV profile with any name of SQL fields. Ensure profile type: SQL and usage: basket export in acquisition 3) Go to Acquisitions -> Find or create a vendor -> Use or create a basket 4) Click the dropdown menu next to the 'Export as CSV' button. There should be a 'Default' option and your new CSV profile (at least) 5) Click the 'Default' option. Notice warns 6) Click the 'Export as CSV' button. Notice warns 7) Click your new CSV profile option. Notice warns 8) Apply patch and refresh page 9) Repeat steps 5-7, confirm that warns do not show 10) Confirm export still works as expected Sponsored-by: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 70114 [details] [review] Bug 19200: (QA follow-up) Simplify call to GetBasketAsCSV If no profile_id is passed, GetBasketAsCSV will fallback to default itself. No need to make the distinction in basket.pl. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed to master for 18.05, thanks to everybody involved!
Awesome work all, pushed to stable for 17.11.02
Depends on Bug 8612 that is not in 17.05.x