Bug 28230 - Renewing/Checking out record with AE or OE letter in title can make Koha totally unfunctional
Summary: Renewing/Checking out record with AE or OE letter in title can make Koha tota...
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Joonas Kylmälä
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 22824
Blocks:
  Show dependency treegraph
 
Reported: 2021-04-26 14:50 UTC by Joonas Kylmälä
Modified: 2021-05-03 09:36 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00


Attachments
Bug 28230: Store C4::Message->{metadata} as Perl string always (1.74 KB, patch)
2021-04-27 07:50 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28230: Store C4::Message->{metadata} as Perl string always (2.17 KB, patch)
2021-04-27 08:29 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28230: Add tests (2.77 KB, patch)
2021-04-27 08:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28230: Add tests (2.63 KB, patch)
2021-04-27 08:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28230: Regression tests (2.69 KB, patch)
2021-04-27 11:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28230: Store C4::Message->{metadata} as Perl string always (2.22 KB, patch)
2021-04-27 11:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28230: Regression tests (2.75 KB, patch)
2021-04-27 13:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28230: Store C4::Message->{metadata} as Perl string always (2.28 KB, patch)
2021-04-27 13:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28230: Add execute bit to test (529 bytes, patch)
2021-04-27 13:41 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2021-04-26 14:50:02 UTC
Koha can lock-up/stop working totally (unrelease manual DB LOCK on messages_queue table seems to cause this) in the following example situation (others aswell):

- Have a biblio with "Ä" in title
- Enable RenewalSendNotice  
- Enable "Item checkout and renewal" message preference for patron
- Checkout an item from biblio "Ä" to patron
- Renew the patrons loan and notice the following error:

> {UNKNOWN}: YAML::XS::Load Error: The problem:
> 
>     control characters are not allowed
> 
> was found at document: 0 at /kohadevbox/koha/C4/Circulation.pm line 3150

This seems to also cause a lock staying on message_queue table and locking other Koha database transactions from happening.

I did some test code to debug this:


#use YAML;
use Encode;
use Data::Dumper;
use YAML::XS

print Dumper(Dump(YAML::XS::Load(Encode::encode_utf8("---\r\n hello: 'heÄllo'"))));

Basically here the YAML::XS version dies but if you replace that with YAML::Load it works. It might also have something to do with the Ä getting double encoded?

The code that causes this problem is the 

> return YAML::XS::Load(Encode::encode_utf8($self->{metadata}));

line in C4::Message::metadata() function.

This seems to be caused by "Bug 22824: Replace YAML::Syck with YAML::XS" (d6d01169509999e22).
Comment 1 Katrin Fischer 2021-04-26 16:10:04 UTC
I haven't verified yet, but that would be a killer for any German installation...
Comment 2 Tomás Cohen Arazi 2021-04-26 16:59:03 UTC
This works:

use utf8;

use Encode qw(encode encode_utf8);
use Data::Dumper;
use YAML::XS;

binmode STDOUT, ':encoding(UTF-8)';

my $string = YAML::XS::Load(encode('UTF-8',"---\r\n hello: 'heÄllo'"));

print STDOUT Dumper($string);

$string = YAML::XS::Load(encode_utf8("---\r\n hello: 'heÄllo'"));

print STDOUT Dumper($string);

1;
Comment 3 Tomás Cohen Arazi 2021-04-26 18:26:14 UTC
(In reply to Tomás Cohen Arazi from comment #2)
> This works:

I wanted to add that I totally reproduced this following the test plan, with the first record that comes with KTD (Russian).
Comment 4 Joonas Kylmälä 2021-04-27 06:17:53 UTC
(In reply to Tomás Cohen Arazi from comment #2)
> This works:
> 
> use utf8;

also adding use "utf8;" to the debug statement makes things work but I actually get different results with them:

YAML::Load:

> ---
> hello: heÃ
>           llo


YAML::XS

> ---
> hello: heÄllo

Anyway, if I'm not mistaken the use utf8 is just for the source code encoding, here we load the string from database. But good note this down. Maybe our input / biblio title is not properly utf8 encoded or something.
Comment 5 Joonas Kylmälä 2021-04-27 06:18:43 UTC
(In reply to Joonas Kylmälä from comment #4)
> also adding use "utf8;" to the debug statement makes things work but I
> actually get different results with them:

Meant to say the debug statement I posted in the first comment here.
Comment 6 Joonas Kylmälä 2021-04-27 07:50:26 UTC
Created attachment 120196 [details] [review]
Bug 28230: Store C4::Message->{metadata} as Perl string always

When loading C4::Message->{metadata} we load it as perl string in all
other cases, see e.g. the subroutine find_last_message(). In this one
case we incorrectly populate the C4::Message->{metadata} as an UTF-8
octet instead of a Perl string. This causes a problem later on because
encode_utf8 later on in the line:

YAML::XS::Load(Encode::encode_utf8($self->{metadata}));

excepts a perl string and not a UTF-8 octet (please refer to the
functions perldoc). This breaks the encoding and causes an internal
server error:

To test the error is gone:
1. Create biblio with "AE" (bugzilla doesn't let me write the real
letter here, see it in the bugzilla comments) in title
2. Enable RenewalSendNotice
3. Enable "Item checkout and renewal" message preference for patron
4. Checkout an item from biblio "AE" to patron
5. Try to renew the patron's loan and notice the renewal fails
6. Apply patch and restart plack
7. Notice renewal works now and message h's the AE letter displayed correctly
Comment 7 Jonathan Druart 2021-04-27 07:56:25 UTC
I was going to attach this patch:

diff --git a/C4/Message.pm b/C4/Message.pm
index 659ff96aeb2..d9bb945abb6 100644
--- a/C4/Message.pm
+++ b/C4/Message.pm
@@ -283,7 +283,7 @@ sub metadata {
         $self->content($self->render_metadata);
         return $data;
     } else {
-        return YAML::XS::Load(Encode::encode_utf8($self->{metadata}));
+        return YAML::XS::Load($self->{metadata});
     }
 }
Comment 8 Andrew Nugged 2021-04-27 08:02:04 UTC
So Joonas proposes to store octets in metadata by added missed "decoding to octets",

and Jonathan proposes to store strings so to remove excessive in this case "encoding",

I would agree to have more "just normal Perl strings around" and walk away as much as possible from "encode/decode", ... if this works (this might be uh :) ).
Comment 9 Joonas Kylmälä 2021-04-27 08:07:06 UTC
(In reply to Jonathan Druart from comment #7)
> I was going to attach this patch:
> 
> diff --git a/C4/Message.pm b/C4/Message.pm
> index 659ff96aeb2..d9bb945abb6 100644
> --- a/C4/Message.pm
> +++ b/C4/Message.pm
> @@ -283,7 +283,7 @@ sub metadata {
>          $self->content($self->render_metadata);
>          return $data;
>      } else {
> -        return YAML::XS::Load(Encode::encode_utf8($self->{metadata}));
> +        return YAML::XS::Load($self->{metadata});
>      }
>  }

Why I didn't do this is because to me it looks to be incorrect in this scenario:

1. Load the object with find_last_message() -> metadata is now perl string
2. Call $message->append() -> it then calls my $metadata = $self->metadata; and we would be calling 

> +        return YAML::XS::Load($self->{metadata});

which means we are calling Load() with a Perl string and not UTF-8 octets which is wrong.
Comment 10 Andrew Nugged 2021-04-27 08:16:27 UTC
right. Quote:

    USING YAML::XS WITH UNICODE
    Handling unicode properly in Perl can be a pain.
    YAML::XS only deals with streams of utf8 octets

so seems this is that case when I want to have less encode/decode, but is impossible to avoid. 

Jonathan, then: Joonas did right
Comment 11 Joonas Kylmälä 2021-04-27 08:19:51 UTC
The 

> sub enqueue {

looks buggy as well.

>     $letter->{metadata} = Dump($metadata);

We should decode it to be a Perl string as well because in     C4::Letters::EnqueueLetter which is called next and this is passed there it calls $sth->execute() with it. At least the other execute params I see are perl strings and not utf-8 octets so if it is not buggy it is unintuitive at least. 

but this enqueue bug is not related to this bug just wanted to mention because it is similar and might cause badly encoded emails.
Comment 12 Joonas Kylmälä 2021-04-27 08:29:55 UTC
Created attachment 120202 [details] [review]
Bug 28230: Store C4::Message->{metadata} as Perl string always

When loading C4::Message->{metadata} we load it as perl string in all
other cases, see e.g. the subroutine find_last_message(). In this one
case we incorrectly populate the C4::Message->{metadata} as an UTF-8
octet instead of a Perl string. This causes a problem later on because
encode_utf8 later on in the line:

YAML::XS::Load(Encode::encode_utf8($self->{metadata}));

excepts a perl string and not a UTF-8 octet (please refer to the
functions perldoc). This breaks the encoding and causes an internal
server error:

To test the error is gone:
1. Create biblio with "AE" (bugzilla doesn't let me write the real
letter here, see it in the bugzilla comments) in title
2. Enable RenewalSendNotice
3. Enable "Item checkout and renewal" message preference for patron
4. Checkout an item from biblio "AE" to patron
5. Try to renew the patron's loan and notice the renewal fails
6. Apply patch and restart plack
7. Notice renewal works now and message h's the AE letter displayed correctly
Comment 13 Joonas Kylmälä 2021-04-27 08:30:59 UTC
I re-made the patch now so that the enqueue() function is free of this bug as well.
Comment 14 Jonathan Druart 2021-04-27 08:37:21 UTC
Created attachment 120203 [details] [review]
Bug 28230: Add tests
Comment 15 Jonathan Druart 2021-04-27 08:38:57 UTC
Created attachment 120204 [details] [review]
Bug 28230: Add tests
Comment 16 Jonathan Druart 2021-04-27 08:41:24 UTC
And those tests prove you are right Joonas :)

There is a warning displayed when the tests are run but I am suspecting an issue in t::lib::TestBuilder::builder_sample_biblio.
Comment 17 Jonathan Druart 2021-04-27 08:41:44 UTC
(In reply to Jonathan Druart from comment #16)
> And those tests prove you are right Joonas :)
> 
> There is a warning displayed when the tests are run but I am suspecting an
> issue in t::lib::TestBuilder::builder_sample_biblio.

build_sample_biblio
Comment 18 Jonathan Druart 2021-04-27 08:47:43 UTC
See bug 28234.
Comment 19 Tomás Cohen Arazi 2021-04-27 11:17:32 UTC
Created attachment 120210 [details] [review]
Bug 28230: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Tomás Cohen Arazi 2021-04-27 11:17:47 UTC
Created attachment 120211 [details] [review]
Bug 28230: Store C4::Message->{metadata} as Perl string always

When loading C4::Message->{metadata} we load it as perl string in all
other cases, see e.g. the subroutine find_last_message(). In this one
case we incorrectly populate the C4::Message->{metadata} as an UTF-8
octet instead of a Perl string. This causes a problem later on because
encode_utf8 later on in the line:

YAML::XS::Load(Encode::encode_utf8($self->{metadata}));

excepts a perl string and not a UTF-8 octet (please refer to the
functions perldoc). This breaks the encoding and causes an internal
server error:

To test the error is gone:
1. Create biblio with "AE" (bugzilla doesn't let me write the real
letter here, see it in the bugzilla comments) in title
2. Enable RenewalSendNotice
3. Enable "Item checkout and renewal" message preference for patron
4. Checkout an item from biblio "AE" to patron
5. Try to renew the patron's loan and notice the renewal fails
6. Apply patch and restart plack
7. Notice renewal works now and message h's the AE letter displayed correctly

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi 2021-04-27 12:58:53 UTC
(In reply to Joonas Kylmälä from comment #13)
> I re-made the patch now so that the enqueue() function is free of this bug
> as well.

+1
Comment 22 Martin Renvoize 2021-04-27 13:38:19 UTC
Created attachment 120227 [details] [review]
Bug 28230: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2021-04-27 13:38:23 UTC
Created attachment 120228 [details] [review]
Bug 28230: Store C4::Message->{metadata} as Perl string always

When loading C4::Message->{metadata} we load it as perl string in all
other cases, see e.g. the subroutine find_last_message(). In this one
case we incorrectly populate the C4::Message->{metadata} as an UTF-8
octet instead of a Perl string. This causes a problem later on because
encode_utf8 later on in the line:

YAML::XS::Load(Encode::encode_utf8($self->{metadata}));

excepts a perl string and not a UTF-8 octet (please refer to the
functions perldoc). This breaks the encoding and causes an internal
server error:

To test the error is gone:
1. Create biblio with "AE" (bugzilla doesn't let me write the real
letter here, see it in the bugzilla comments) in title
2. Enable RenewalSendNotice
3. Enable "Item checkout and renewal" message preference for patron
4. Checkout an item from biblio "AE" to patron
5. Try to renew the patron's loan and notice the renewal fails
6. Apply patch and restart plack
7. Notice renewal works now and message h's the AE letter displayed correctly

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2021-04-27 13:39:27 UTC
Good catch guys, that's a nasty bug.

Man I hate encoding issues.. 

All works as expected for me now.. passing QA
Comment 25 Martin Renvoize 2021-04-27 13:41:26 UTC
Created attachment 120230 [details] [review]
Bug 28230: Add execute bit to test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Jonathan Druart 2021-04-28 09:17:25 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 27 Fridolin Somers 2021-04-30 09:04:57 UTC
Depends on Bug 22824 not in 20.11.x