[Koha-patches] [PATCH] Bug 1953: refactoring C4::Koha::get_itemtypeinfos_of to eliminate potential SQL injection

[Andrew Moore]

C4::Koha::get_itemtypeinfos_of was not using plceholders, opening itself up to
potential SQL injection attacks. This patch refactors it to use placeholders to
bind parameters.
I also had to extend C4::koha::get_infos_of to allow us to pass bind parameters into it.
I'm including a test module for C4::Koha::get_itemtypeinfos_of.
---
 C4/Koha.pm                                  |   22 ++++++----
 t/lib/KohaTest/Koha/get_itemtypeinfos_of.pm |   59 +++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 9 deletions(-)
 create mode 100644 t/lib/KohaTest/Koha/get_itemtypeinfos_of.pm

diff --git a/C4/Koha.pm b/C4/Koha.pm
index 65822aa..401dd9c 100644
--- a/C4/Koha.pm
+++ b/C4/Koha.pm
@@ -261,16 +261,17 @@ sub GetItemTypes {
 sub get_itemtypeinfos_of {
     my @itemtypes = @_;
 
-    my $query = '
+    my $placeholders = join( ', ', map { '?' } @itemtypes );
+    my $query = <<"END_SQL";
 SELECT itemtype,
        description,
        imageurl,
        notforloan
   FROM itemtypes
-  WHERE itemtype IN (' . join( ',', map( { "'" . $_ . "'" } @itemtypes ) ) . ')
-';
+  WHERE itemtype IN ( $placeholders )
+END_SQL
 
-    return get_infos_of( $query, 'itemtype' );
+    return get_infos_of( $query, 'itemtype', undef, \@itemtypes );
 }
 
 # this is temporary until we separate collection codes and item types
@@ -788,9 +789,12 @@ sub getFacets {
 
 =head2 get_infos_of
 
-Return a href where a key is associated to a href. You give a query, the
-name of the key among the fields returned by the query. If you also give as
-third argument the name of the value, the function returns a href of scalar.
+Return a href where a key is associated to a href. You give a query,
+the name of the key among the fields returned by the query. If you
+also give as third argument the name of the value, the function
+returns a href of scalar. The optional 4th argument is an arrayref of
+items passed to the C<execute()> call. It is designed to bind
+parameters to any placeholders in your SQL.
 
   my $query = '
 SELECT itemnumber,
@@ -810,12 +814,12 @@ SELECT itemnumber,
 =cut
 
 sub get_infos_of {
-    my ( $query, $key_name, $value_name ) = @_;
+    my ( $query, $key_name, $value_name, $bind_params ) = @_;
 
     my $dbh = C4::Context->dbh;
 
     my $sth = $dbh->prepare($query);
-    $sth->execute();
+    $sth->execute( @$bind_params );
 
     my %infos_of;
     while ( my $row = $sth->fetchrow_hashref ) {
diff --git a/t/lib/KohaTest/Koha/get_itemtypeinfos_of.pm b/t/lib/KohaTest/Koha/get_itemtypeinfos_of.pm
new file mode 100644
index 0000000..9845e90
--- /dev/null
+++ b/t/lib/KohaTest/Koha/get_itemtypeinfos_of.pm
@@ -0,0 +1,59 @@
+package KohaTest::Koha::get_itemtypeinfos_of;
+use base qw( KohaTest::Koha );
+
+use strict;
+use warnings;
+
+use Test::More;
+
+use C4::Koha;
+
+=head2 get_one
+
+calls get_itemtypeinfos_of on one item type and checks that it gets
+back something sane.
+
+=cut
+
+sub get_one : Test( 8 ) {
+    my $self = shift;
+
+    my $itemtype_info = C4::Koha::get_itemtypeinfos_of( 'BK' );
+    ok( $itemtype_info, 'we got back something from get_itemtypeinfos_of' );
+    isa_ok( $itemtype_info, 'HASH', '...and it' );
+    ok( exists $itemtype_info->{'BK'}, '...and it has a BK key' )
+      or diag( Data::Dumper->Dump( [ $itemtype_info ], [ 'itemtype_info' ] ) );
+    is( scalar keys %$itemtype_info, 1, '...and it has 1 key' );
+    foreach my $key ( qw( imageurl itemtype notforloan description ) ) {
+        ok( exists $itemtype_info->{'BK'}{$key}, "...and the BK info has a $key key" );
+    }
+    
+}
+
+=head2 get_two
+
+calls get_itemtypeinfos_of on a list of item types and verifies the
+results.
+
+=cut
+
+sub get_two : Test( 13 ) {
+    my $self = shift;
+
+    my @itemtypes = qw( BK MU );
+    my $itemtype_info = C4::Koha::get_itemtypeinfos_of( @itemtypes );
+    ok( $itemtype_info, 'we got back something from get_itemtypeinfos_of' );
+    isa_ok( $itemtype_info, 'HASH', '...and it' );
+    is( scalar keys %$itemtype_info, scalar @itemtypes, '...and it has ' . scalar @itemtypes . ' keys' );
+    foreach my $it ( @itemtypes ) {
+        ok( exists $itemtype_info->{$it}, "...and it has a $it key" )
+          or diag( Data::Dumper->Dump( [ $itemtype_info ], [ 'itemtype_info' ] ) );
+        foreach my $key ( qw( imageurl itemtype notforloan description ) ) {
+            ok( exists $itemtype_info->{$it}{$key}, "...and the $it info has a $key key" );
+        }
+    }
+    
+}
+
+  
+1;
-- 
1.5.6