From 8b679f8d814974f5ddd4a410474267af2fa30fe5 Mon Sep 17 00:00:00 2001 From: Andrew Moore Date: Fri, 25 Jul 2008 15:31:11 -0500 Subject: [PATCH] Bug 1953: refactoring C4::Koha::get_itemtypeinfos_of to eliminate potential SQL injection 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. Signed-off-by: Joshua Ferraro --- 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 65822aa8e8..401dd9c985 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 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 0000000000..9845e90576 --- /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; -- 2.39.5