[Koha-patches] [PATCH] bug 1953: fixing potential SQL injection problems in C4::Branch::GetBranches

[Andrew Moore]

I moved C4::Branch::GetBranches to use bind parameters and wrote some tests to demonstrate functionality.

No functional or documentation changes here.
---
 C4/Branch.pm                         |    8 ++++--
 t/lib/KohaTest/Branch.pm             |   36 +++++++++++++++++++++++++++++
 t/lib/KohaTest/Branch/GetBranches.pm |   41 ++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 3 deletions(-)
 create mode 100644 t/lib/KohaTest/Branch.pm
 create mode 100644 t/lib/KohaTest/Branch/GetBranches.pm

diff --git a/C4/Branch.pm b/C4/Branch.pm
index 4755168..c09b129 100644
--- a/C4/Branch.pm
+++ b/C4/Branch.pm
@@ -100,12 +100,14 @@ sub GetBranches {
     my $dbh = C4::Context->dbh;
     my $sth;
     my $query="SELECT * FROM branches";
+    my @bind_parameters;
     if ($onlymine && C4::Context->userenv && C4::Context->userenv->{branch}){
-      $query .= " WHERE branchcode =".$dbh->quote(C4::Context->userenv->{branch});
+      $query .= ' WHERE branchcode = ? ';
+      push @bind_parameters, C4::Context->userenv->{branch};
     }
-    $query.=" ORDER BY branchname";
+        $query.=" ORDER BY branchname";
     $sth = $dbh->prepare($query);
-    $sth->execute;
+    $sth->execute( @bind_parameters );
     while ( my $branch = $sth->fetchrow_hashref ) {
         my $nsth =
           $dbh->prepare(
diff --git a/t/lib/KohaTest/Branch.pm b/t/lib/KohaTest/Branch.pm
new file mode 100644
index 0000000..ce7ff60
--- /dev/null
+++ b/t/lib/KohaTest/Branch.pm
@@ -0,0 +1,36 @@
+package KohaTest::Branch;
+use base qw( KohaTest );
+
+use strict;
+use warnings;
+
+use Test::More;
+
+use C4::Branch;
+sub testing_class { 'C4::Branch' };
+
+
+sub methods : Test( 1 ) {
+    my $self = shift;
+    my @methods = qw( GetBranches
+                      GetBranchName
+                      ModBranch
+                      GetBranchCategory
+                      GetBranchCategories
+                      GetCategoryTypes
+                      GetBranch
+                      GetBranchDetail
+                      get_branchinfos_of
+                      GetBranchesInCategory
+                      GetBranchInfo
+                      DelBranch
+                      ModBranchCategoryInfo
+                      DelBranchCategory
+                      CheckBranchCategorycode
+                );
+    
+    can_ok( $self->testing_class, @methods );    
+}
+
+1;
+
diff --git a/t/lib/KohaTest/Branch/GetBranches.pm b/t/lib/KohaTest/Branch/GetBranches.pm
new file mode 100644
index 0000000..1dc5d0f
--- /dev/null
+++ b/t/lib/KohaTest/Branch/GetBranches.pm
@@ -0,0 +1,41 @@
+package KohaTest::Branch::GetBranches;
+use base qw( KohaTest::Branch );
+
+use strict;
+use warnings;
+
+use Test::More;
+
+use C4::Branch;
+
+=head2 STARTUP METHODS
+
+These get run once, before the main test methods in this module
+
+=cut
+
+=head2 TEST METHODS
+
+standard test methods
+
+=head3 onlymine
+
+    When you pass in something true to GetBranches, it limits the
+    response to only your branch.
+
+=cut
+
+sub onlymine : Test( 4 ) {
+    my $self = shift;
+
+    # C4::Branch::GetBranches uses this variable, so make sure it exists.
+    ok( C4::Context->userenv->{'branch'}, 'we have a branch' );
+    my $branches = C4::Branch::GetBranches( 'onlymine' );
+    # diag( Data::Dumper->Dump( [ $branches ], [ 'branches' ] ) );
+    is( scalar( keys %$branches ), 1, 'one key for our branch only' );
+    ok( exists $branches->{ C4::Context->userenv->{'branch'} }, 'my branch was returned' );
+    is( $branches->{ C4::Context->userenv->{'branch'} }->{'branchcode'}, C4::Context->userenv->{'branch'}, 'branchcode' );
+    
+}
+
+1;
-- 
1.5.5.rc0.16.g02b00