Bug 19200 - Warns when exporting a basket
Summary: Warns when exporting a basket
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Aleisha Amohia
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 8612
Blocks:
  Show dependency treegraph
 
Reported: 2017-08-29 21:07 UTC by Aleisha Amohia
Modified: 2018-01-19 10:14 UTC (History)
5 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19200: Preventing warns when exporting a basket (1.89 KB, patch)
2017-08-29 21:11 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 19200: Preventing warns when exporting a basket (1.95 KB, patch)
2017-10-07 13:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19200: Preventing warns when exporting a basket (2.00 KB, patch)
2017-12-20 17:18 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 19200: Preventing warns when exporting a basket (2.09 KB, patch)
2017-12-22 10:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19200: (QA follow-up) Simplify call to GetBasketAsCSV (1.29 KB, patch)
2017-12-22 10:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2017-08-29 21:07:05 UTC
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.
Comment 1 Aleisha Amohia 2017-08-29 21:11:58 UTC
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
Comment 2 Katrin Fischer 2017-10-07 13:07:24 UTC
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>
Comment 3 Marcel de Rooy 2017-12-15 11:18:08 UTC
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 ... ) ?
Comment 4 Aleisha Amohia 2017-12-18 02:10:37 UTC
(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.
Comment 5 Jon Knight 2017-12-20 17:18:26 UTC
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>
Comment 6 Alex Arnaud 2017-12-21 10:21:33 UTC
Is the if() else() statement still usefull? Why not just one call to GetBasketAsCSV?
Comment 7 Marcel de Rooy 2017-12-22 10:05:00 UTC
QA: Resuming here
Comment 8 Marcel de Rooy 2017-12-22 10:25:26 UTC
(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.
Comment 9 Marcel de Rooy 2017-12-22 10:25:43 UTC
(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.
Comment 10 Marcel de Rooy 2017-12-22 10:26:08 UTC
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>
Comment 11 Marcel de Rooy 2017-12-22 10:26:12 UTC
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>
Comment 12 Jonathan Druart 2017-12-22 16:16:44 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 13 Nick Clemens 2018-01-15 16:44:09 UTC
Awesome work all, pushed to stable for 17.11.02
Comment 14 Fridolin SOMERS 2018-01-19 10:14:08 UTC
Depends on Bug 8612 that is not in 17.05.x