Attachment 118779 Details for Bug 27916 – Bug 27916: Fee paid message can crash SIP server if paying fee that is not "renewable"

[patch] Bug 27916: Fee paid message can crash SIP server if paying fee that is not "renewable"

Bug-27916-Fee-paid-message-can-crash-SIP-server-if.patch (text/plain), 2.39 KB, created by Martin Renvoize (ashimema) on 2021-03-25 11:17:48 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2021-03-25 11:17:48 UTC

Size: 2.39 KB

patch

obsolete

>From 68c0e2d18c0076d1571c69f88bea1ba6def94365 Mon Sep 17 00:00:00 2001
>From: Kyle M Hall <kyle@bywatersolutions.com>
>Date: Wed, 10 Mar 2021 09:11:59 -0500
>Subject: [PATCH] Bug 27916: Fee paid message can crash SIP server if paying
> fee that is not "renewable"
>
>I'm not entirely sure how this is possible, but I've confirmed my fix works on two separate occasions.
>
>In the Koha::Account::pay, if a fine is "renewable" ( as determined by the method "renewable" the response to from the method "renew_item" is stored and returned by by pay().
>
>Later in C4::SIP::Sip::MsgType::handle_fee_paid we assume that each element of the list in $pay_response->{renew_result} is a hashref that had an itemnumber. This is not true. We need to check for the itemnumber and then the item. If either do not exist, the SIP server will crash without responding.
>
>My suspicion is that this is caused by Koha::Account::Line::renewable returning true while the corrosponding call to renew_item contains a simple "return;" statement if RenewAccruingItemWhenPaid is not enabled ( or there is not related item or patron ).
>
>What happens here, at least for Bibliotheca self checks is that the machine will take payment, and send the fee paid to Koha via SIP. When Koha's SIP crashes and doesn't return a response, the self check machine will refund the money to the patron, *but* the payment still exists in Koha! Considering payments may be in the tens or even hundreds of dollars, this makes for a critical SIP bug.
>
>I have been unable to recreate this bug on demand. So the test plan is:
>1) Inspect the changes in the patch
>2) Note that they make sense and prevent the SIP server from crashing
>   if the itemnumber key has no data or the item is not found
>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>---
> C4/SIP/Sip/MsgType.pm | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/C4/SIP/Sip/MsgType.pm b/C4/SIP/Sip/MsgType.pm
>index 62d3d5fcfc..92b4c32a9d 100644
>--- a/C4/SIP/Sip/MsgType.pm
>+++ b/C4/SIP/Sip/MsgType.pm
>@@ -1145,7 +1145,9 @@ sub handle_fee_paid {
>     my @success = ();
>     my @fail = ();
>     foreach my $result( @{$pay_response->{renew_result}} ) {
>+        next unless $result->{itemnumber};
>         my $item = Koha::Items->find({ itemnumber => $result->{itemnumber} });
>+        next unless $item;
>         if ($result->{success}) {
>             push @success, '"' . $item->biblio->title . '"';
>         } else {
>-- 
>2.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 27916: 118024 | 118779 | 119718