[Bug 9454] New: NewBasket does not use placeholders in sql
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9454 Bug ID: 9454 Summary: NewBasket does not use placeholders in sql Classification: Unclassified Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: minor Priority: P5 - low Component: Acquisitions Assignee: koha-bugs@lists.koha-community.org Reporter: colin.campbell@ptfs-europe.com NewBasket interpolates variables into the sql string which is used to insert a new basket in the database. This is a bad practice, placeholders should be used instead allowing DBI to quote the input correctly -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9454 Colin Campbell <colin.campbell@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Assignee|koha-bugs@lists.koha-commun |colin.campbell@ptfs-europe. |ity.org |com --- Comment #1 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Created attachment 14776 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=14776&action=edit Proposed patch Patch should not change functional behaviour. To test check baskets still correctly created -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9454 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9454 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #14776|0 |1 is obsolete| | --- Comment #2 from Galen Charlton <gmcharlt@gmail.com> --- Created attachment 14924 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=14924&action=edit Bug 9454: Use placeholders when adding basket Should always use placeholders when passing variables to DBI; avoids unforeseen bugs and security issues. Also: - reformated the long lists of parameters to add CR - moved the setting of defaults out of the call to ModBasket to clarify code - Setting parameters to undef if they were not defined was unnecessary bloat and obscuration Testing: Patch should not change functional behaviour. To test check that order baskets can still be correctly created. Signed-off-by: Galen Charlton <gmc@esilibrary.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9454 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m --- Comment #3 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to comment #2)
- Setting parameters to undef if they were not defined was unnecessary bloat and obscuration
I disagree, it is not that the original code does. $t ||= undef and $t //= undef is not the same. The second one is unnecessary, not the first one. The original code says: if a variable ($basketcontractnumber, $deliveryplace or $billingplace) is equal to 0 or an empty string, we call ModBasketHeader with an undefined value. If someone calls the routine with "", we want to insert undef, not "" -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9454 --- Comment #4 from Colin Campbell <colin.campbell@ptfs-europe.com> ---
The original code says: if a variable ($basketcontractnumber, $deliveryplace or $billingplace) is equal to 0 or an empty string, we call ModBasketHeader with an undefined value.
If someone calls the routine with "", we want to insert undef, not ""
The only call of the routine in basketheader.pl already forces basketcontractnumber to undef in these conditions and this would seem the appropriate place to validate inputs. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9454 --- Comment #5 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to comment #4)
The original code says: if a variable ($basketcontractnumber, $deliveryplace or $billingplace) is equal to 0 or an empty string, we call ModBasketHeader with an undefined value.
If someone calls the routine with "", we want to insert undef, not ""
The only call of the routine in basketheader.pl already forces basketcontractnumber to undef in these conditions and this would seem the appropriate place to validate inputs.
There are 2 places: - C4::Acq::NewBasket, where the validation is done. - In the basketheader.pl script, the validation is only done for the contractnumber value. I don't know if it is a big deal, but in the doubt, without modification of this patch I would prefer that someone else of the QA team gives his opinion on it. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9454 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de QA Contact| |katrin.fischer@bsz-bw.de Severity|minor |major --- Comment #6 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Looking at this now. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9454 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9454 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #14924|0 |1 is obsolete| | --- Comment #7 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 15282 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=15282&action=edit Bug 9454: Use placeholders when adding basket Should always use placeholders when passing variables to DBI; avoids unforeseen bugs and security issues. Also: - reformated the long lists of parameters to add CR - moved the setting of defaults out of the call to ModBasket to clarify code - Setting parameters to undef if they were not defined was unnecessary bloat and obscuration Testing: Patch should not change functional behaviour. To test check that order baskets can still be correctly created. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> All tests and QA script pass. Created a new basket, added order lines and closed basket. Checked everything worked in the staff interface, also checked the entry for the new basket in aqbasket in the database. Logs are clean too. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9454 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|unspecified |master -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9454 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #8 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- This patch has been pushed to master. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9454 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |chris@bigballofwax.co.nz --- Comment #9 from Chris Cormack <chris@bigballofwax.co.nz> --- No string changes so pushed to 3.10.x will be in 3.10.3 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org