Bug 9454 - NewBasket does not use placeholders in sql
Summary: NewBasket does not use placeholders in sql
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Colin Campbell
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-23 11:17 UTC by Colin Campbell
Modified: 2014-05-26 21:04 UTC (History)
3 users (show)

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


Attachments
Proposed patch (2.31 KB, patch)
2013-01-23 11:43 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 9454: Use placeholders when adding basket (2.49 KB, patch)
2013-01-29 18:31 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9454: Use placeholders when adding basket (2.77 KB, patch)
2013-02-12 20:59 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2013-01-23 11:17:51 UTC
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
Comment 1 Colin Campbell 2013-01-23 11:43:33 UTC Comment hidden (obsolete)
Comment 2 Galen Charlton 2013-01-29 18:31:11 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2013-01-31 13:36:05 UTC
(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 ""
Comment 4 Colin Campbell 2013-02-01 15:49:17 UTC
> 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.
Comment 5 Jonathan Druart 2013-02-04 09:17:05 UTC
(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.
Comment 6 Katrin Fischer 2013-02-12 20:48:45 UTC
Looking at this now.
Comment 7 Katrin Fischer 2013-02-12 20:59:26 UTC
Created attachment 15282 [details] [review]
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.
Comment 8 Jared Camins-Esakov 2013-02-15 12:44:34 UTC
This patch has been pushed to master.
Comment 9 Chris Cormack 2013-02-16 05:29:18 UTC
No string changes so pushed to 3.10.x will be in 3.10.3