From 1f8b6d5daad407ed289a5b1f097e3ae57d6f452d Mon Sep 17 00:00:00 2001
From: Phil Ringnalda <phil@chetcolibrary.org>
Date: Fri, 6 Sep 2024 14:33:17 -0700
Subject: [PATCH] Bug 37786: members/cancel-charge.pl needs CSRF protection

members/cancel-charge.pl will take either a POST or a GET, and as long as the
accountline_id it is passed can be cancelled, will cancel it. That means any
link you click anywhere while logged in to Koha might cancel a charge. It also
takes a borrowernumber which isn't used for the cancelling, only to determine
what account to show after a charge is cancelled, letting a malicious link
show an account other than the one whose charge was just cancelled.

Test plan:
 1. Without the patch, Circulation - Checkout - search for the 'koha' patron
    you log in as
 2. Accounting - Create manual invoice - Make it a Manual fee of 100.00 and
    Save
 3. Pretending it's a well-disguised link in a spear-phishing email, load
    http://localhost:8081/cgi-bin/koha/members/cancel-charge.pl?borrowernumber=5&accountlines_id=1
 4. You are now looking at charges for the patron Acosta, Edna rather than for
    the patron koha, but if you look at the patron koha, its 100.00 charge
    has been cancelled.
 5. Apply patch and reset_all (or if you don't, you'll have to manually adjust
    the link to reflect the charge being accountlines_id 3 rather than 1)
 6. Circulation - Checkout - search for the 'koha' patron you log in as
 7. Accounting - Create manual invoice - Make it a Manual fee of 100.00 and
    Save
 8. Click the link http://localhost:8081/cgi-bin/koha/members/cancel-charge.pl?borrowernumber=5&accountlines_id=1
 9. You got a 403 because you didn't pass the op cud-cancel, but if you did
    pass that op, you would also get a 403 for having a cud- op in a GET (and
    if you POST, you won't have a csrf_token)
10. Checkout - search for koha - Accounting - Cancel charge
11. Having done it the right way, you're now on koha's list of transactions,
    where you can see you just cancelled it

Sponsored-by: Chetco Community Public Library

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
---
 .../prog/en/modules/members/boraccount.tt     |  2 +-
 members/cancel-charge.pl                      | 30 +++++++++++--------
 2 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt
index f8e5a90f1c..cb2bc93ccd 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt
@@ -190,7 +190,7 @@
         [% IF account.is_debit && account.amount == account.amountoutstanding && account.status != 'CANCELLED' && !(account.debit_type_code == 'PAYOUT') %]
           <form method="post" action="/cgi-bin/koha/members/cancel-charge.pl">
             [% INCLUDE 'csrf-token.inc' %]
-            <input type="hidden" name="borrowernumber" value="[% patron.borrowernumber | html %]">
+            <input type="hidden" name="op" value="cud-cancel">
             <input type="hidden" name="accountlines_id" value="[% account.accountlines_id | html %]">
             <button type="submit" class="btn btn-default btn-xs cancel-action">
                 <i class="fa fa-ban"></i>
diff --git a/members/cancel-charge.pl b/members/cancel-charge.pl
index 5748fa54c4..0b6d756cd0 100755
--- a/members/cancel-charge.pl
+++ b/members/cancel-charge.pl
@@ -32,15 +32,21 @@ my $flags = {
 my $type = 'intranet';
 my ($user, $cookie) = C4::Auth::checkauth($cgi, $authnotrequired, $flags, $type);
 
-my $borrowernumber = $cgi->param('borrowernumber');
-my $accountlines_id = $cgi->param('accountlines_id');
-
-my $charge = Koha::Account::Lines->find($accountlines_id);
-$charge->cancel(
-    {
-        branch   => C4::Context->userenv->{'branch'},
-        staff_id => C4::Context->userenv->{'number'}
-    }
-);
-
-print $cgi->redirect('/cgi-bin/koha/members/boraccount.pl?borrowernumber=' . $borrowernumber);
+my $op = $cgi->param('op') // q{};
+
+if ( $op eq "cud-cancel" ) {
+    my $accountlines_id = $cgi->param('accountlines_id');
+
+    my $charge         = Koha::Account::Lines->find($accountlines_id);
+    my $borrowernumber = $charge->patron->borrowernumber;
+    $charge->cancel(
+        {
+            branch   => C4::Context->userenv->{'branch'},
+            staff_id => C4::Context->userenv->{'number'}
+        }
+    );
+    print $cgi->redirect( '/cgi-bin/koha/members/boraccount.pl?borrowernumber=' . $borrowernumber );
+    exit;
+}
+
+print $cgi->redirect('/cgi-bin/koha/errors/403.pl');
-- 
2.39.5