From 8933e69863a581a799767af4d0a0e1e7b908e238 Mon Sep 17 00:00:00 2001
From: Mark Tompsett <mtompset@hotmail.com>
Date: Fri, 25 Aug 2017 15:09:38 -0400
Subject: [PATCH] Bug 18956: Prevent leaking during password recovery

TEST PLAN
---------

It is assumed you have set the OpacResetPassword to 'allowed',
and likely in combination with OpacPasswordChange to 'Allowed'.

You will have two patrons: one with and another without
any email address entered. You will want to test this test plan
with both patrons.

$ git checkout -b bug_18956 origin/master

Prepend the following as understood between step sections:
opac -> forgot password and then enter...

correct login/cardnumber, it will email
delete from borrower_password_recovery;

correct email, it will email
delete from borrower_password_recovery;

correct login/cardnumber && correct email, it will email
delete from borrower_password_recovery;

wrong login/cardnumber && correct email, error page as expected
delete from borrower_password_recovery;

correct login/cardnumber && wrong email, error page as expected
delete from borrower_password_recovery;

wrong login/cardnumber && wrong email, error page as expected
delete from borrower_password_recovery;

submit empty -- INTERNAL SERVER ERROR?!
delete from borrower_password_recovery;

-- None of the above step sections displayed email.

correct login/cardnumber, it will email

correct login/cardnumber again, but it leaks email address!
delete from borrower_password_recovery;

correct email, it will email

correct email again, but it leaks login/cardnumber!
delete from borrower_password_recovery;

$ git bz apply 18956
-- choose interactive, and choose this counter patch.

repeat the same test set again
-- no leaks will occur, error message pages returned should
   be reasonable, code should read reasonably.

run koha qa test tools.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
---
 .../bootstrap/en/modules/opac-password-recovery.tt     |  7 ++++++-
 opac/opac-password-recovery.pl                         | 10 ++++------
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-password-recovery.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-password-recovery.tt
index 4f3f8571d5..b16a33bc0a 100644
--- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-password-recovery.tt
+++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-password-recovery.tt
@@ -58,7 +58,12 @@
                         Account identification with this email address only is ambiguous.
                         <br />Please use the field 'Login' as well.
                     [% ELSIF (errAlreadyStartRecovery) %]
-                        The process of password recovery has already been started for this account ("<strong>[% username %]</strong>")
+                        The process of password recovery has already been started for this account
+                        [% IF username %]
+                            ("<strong>[% username %]</strong>")
+                        [% ELSIF email %]
+                            ("<strong>[% email %]</strong>")
+                        [% END %]
                         <br/>You should have received an email with a link to reset your password.
                         <br/>If you did not receive this email, you can request a new one: <a href="/cgi-bin/koha/opac-password-recovery.pl?resendEmail=true&email=[% email %]&username=[% username %]">Get new password recovery link</a>
                     [% ELSIF (errPassNotMatch) %]
diff --git a/opac/opac-password-recovery.pl b/opac/opac-password-recovery.pl
index f15a93e2f3..9a7a0ca3ee 100755
--- a/opac/opac-password-recovery.pl
+++ b/opac/opac-password-recovery.pl
@@ -31,7 +31,7 @@ my $repeatPassword = $query->param('repeatPassword');
 my $minPassLength  = C4::Context->preference('minPasswordLength');
 my $id             = $query->param('id');
 my $uniqueKey      = $query->param('uniqueKey');
-my $username       = $query->param('username');
+my $username       = $query->param('username') // q{};
 my $borrower_number;
 
 #errors
@@ -53,7 +53,6 @@ my $errPassTooShort;
 if ( $query->param('sendEmail') || $query->param('resendEmail') ) {
 
     #try with the main email
-    $email ||= '';    # avoid undef
     my $borrower;
     my $search_results;
 
@@ -65,7 +64,7 @@ if ( $query->param('sendEmail') || $query->param('resendEmail') ) {
         $search_results = Koha::Patrons->search( { -or => { email => $email, emailpro => $email, B_email  => $email } } );
     }
 
-    if ( not $search_results || $search_results->count < 1) {
+    if ( !defined $search_results || $search_results->count < 1) {
         $hasError           = 1;
         $errNoBorrowerFound = 1;
     }
@@ -78,7 +77,6 @@ if ( $query->param('sendEmail') || $query->param('resendEmail') ) {
         $errMultipleAccountsForEmail = 1;
     }
     elsif ( $borrower = $search_results->next() ) {    # One matching borrower
-        $username ||= $borrower->userid;
         my @emails = ( $borrower->email, $borrower->emailpro, $borrower->B_email );
 
         my $firstNonEmptyEmail = '';
@@ -93,8 +91,8 @@ if ( $query->param('sendEmail') || $query->param('resendEmail') ) {
             $errNoBorrowerFound = 1;
         }
 
-# If we dont have an email yet. Get one of the borrower's email or raise an error.
-        elsif ( !$email && !( $email = $firstNonEmptyEmail ) ) {
+        # If there is no given email, and there is no email on record
+        elsif ( !$email && !$firstNonEmptyEmail ) {
             $hasError           = 1;
             $errNoBorrowerEmail = 1;
         }
-- 
2.39.5