Bug 25774 - REST API searches don't handle correctly utf8 characters
Summary: REST API searches don't handle correctly utf8 characters
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-06-16 14:21 UTC by Jonathan Druart
Modified: 2020-11-30 21:51 UTC (History)
3 users (show)

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


Attachments
Bug 25774: Handle utf8 chars in REST API queries (3.53 KB, patch)
2020-06-16 14:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25774: Handle utf8 chars in REST API queries (3.60 KB, patch)
2020-06-16 19:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25774: replace decode_json with from_json (1.72 KB, patch)
2020-06-17 04:18 UTC, David Cook
Details | Diff | Splinter Review
Bug 25774: Handle utf8 chars in REST API queries (3.56 KB, patch)
2020-06-17 07:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25774: Handle utf8 chars in REST API queries (3.63 KB, patch)
2020-06-19 19:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25774: Handle utf8 chars in REST API queries (3.67 KB, patch)
2020-06-19 19:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2020-06-16 14:21:28 UTC
In the search box, search "❤":

[2020/06/16 14:11:37] [ERROR] GET /api/v1/libraries: unhandled exception (Mojo::Exception)<<Wide character in subroutine entry at /kohadevbox/koha/Koha/REST/Plugin/Objects.pm line 107.>> Koha::REST::Plugin::Exceptions::__ANON__ /kohadevbox/koha/Koha/REST/Plugin/Exceptions.pm (73)


172.18.0.1 - - [16/Jun/2020:14:11:37 +0000] "GET /api/v1/app.pl/api/v1/libraries?_page=1&_per_page=20&q=%5B%7B%22me.name%22%3A%7B%22like%22%3A%22%25%E2%9D%A4%25%22%7D%7D%2C%7B%22me.library_id%22%3A%7B%22like%22%3A%22%25%E2%9D%A4%25%22%7D%7D%2C%7B%22me.marc_org_code%22%3A%7B%22like%22%3A%22%25%E2%9D%A4%25%22%7D%7D%2C%7B%22me.ip%22%3A%7B%22like%22%3A%22%25%E2%9D%A4%25%22%7D%7D%2C%7B%22me.pickup_location%22%3A%7B%22like%22%3A%22%25%E2%9D%A4%25%22%7D%7D%2C%7B%22me.address1%22%3A%7B%22like%22%3A%22%25%E2%9D%A4%25%22%7D%7D%2C%7B%22me.address2%22%3A%7B%22like%22%3A%22%25%E2%9D%A4%25%22%7D%7D%2C%7B%22me.address3%22%3A%7B%22like%22%3A%22%25%E2%9D%A4%25%22%7D%7D%2C%7B%22me.city%22%3A%7B%22like%22%3A%22%25%E2%9D%A4%25%22%7D%7D%2C%7B%22me.state%22%3A%7B%22like%22%3A%22%25%E2%9D%A4%25%22%7D%7D%2C%7B%22me.postal_code%22%3A%7B%22like%22%3A%22%25%E2%9D%A4%25%22%7D%7D%2C%7B%22me.country%22%3A%7B%22like%22%3A%22%25%E2%9D%A4%25%22%7D%7D%2C%7B%22me.opac_info%22%3A%7B%22like%22%3A%22%25%E2%9D%A4%25%22%7D%7D%2C%7B%22me.notes%22%3A%7B%22like%22%3A%22%25%E2%9D%A4%25%22%7D%7D%5D&_match=contains&_order_by=%2Bme.name HTTP/1.1" 500 - "http://kohadev-intra.mydnsname.org:8081/cgi-bin/koha/admin/branches.pl" "Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0"
Comment 1 Jonathan Druart 2020-06-16 14:35:05 UTC
Created attachment 105920 [details] [review]
Bug 25774: Handle utf8 chars in REST API queries

We must utf8 encode the string before json decoding it.

Test plan:
Use "❤" and play with the library search (bug 25288) and Postman to
generate queries using it.

This patch prevents 500:
[2020/06/16 14:11:37] [ERROR] GET /api/v1/libraries: unhandled exception (Mojo::Exception)<<Wide character in subroutine entry at /kohadevbox/koha/Koha/REST/Plugin/Objects.pm line 107.>> Koha::REST::Plugin::Exceptions::__ANON__ /kohadevbox/koha/Koha/REST/Plugin/Exceptions.pm (73)
Comment 2 Kyle M Hall 2020-06-16 19:18:05 UTC
Created attachment 105930 [details] [review]
Bug 25774: Handle utf8 chars in REST API queries

We must utf8 encode the string before json decoding it.

Test plan:
Use "❤" and play with the library search (bug 25288) and Postman to
generate queries using it.

This patch prevents 500:
[2020/06/16 14:11:37] [ERROR] GET /api/v1/libraries: unhandled exception (Mojo::Exception)<<Wide character in subroutine entry at /kohadevbox/koha/Koha/REST/Plugin/Objects.pm line 107.>> Koha::REST::Plugin::Exceptions::__ANON__ /kohadevbox/koha/Koha/REST/Plugin/Exceptions.pm (73)

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 3 David Cook 2020-06-17 03:43:43 UTC
I don't think that this is the most elegant/maintainable solution.

If you change "decode_json" to "from_json", you shouldn't need to use encode_utf8, since decode_json expects bytes and from_json expects a string.

By using encode_utf8, you're taking a string and turning it into bytes, when you could just keep it as a string. 

The documentation for the JSON module is pretty terrible, but I've proved this this with the following three sample files:

{"test":"❤"}

#!/usr/bin/perl
use strict;
use warnings;
use JSON;
open(my $fh, '<', 'test.txt');
my $line = <$fh>;
my $perl = decode_json($line);
warn $perl->{test};


#!/usr/bin/perl
use strict;
use warnings;
use JSON;
open(my $fh, '<', 'test.txt');
my $line = <$fh>;
my $perl = from_json($line);
warn $perl->{test};
Comment 4 David Cook 2020-06-17 04:18:45 UTC
Created attachment 105932 [details] [review]
Bug 25774: replace decode_json with from_json

decode_json expects octets/bytes but from_json expects a string.

We don't need to encode the query parameters as bytes to give to
decode_json. Instead, we can just pass the string to from_json,
and it will work as expected.
Comment 5 David Cook 2020-06-17 04:20:35 UTC
Found this one difficult to test, since the added unit test doesn't test the change.

Applied Bug 25288 and its dependencies...

Changed the "Name" of the first library to ❤.

Tried the AJAX search at /cgi-bin/koha/admin/branches.pl without Bug 25288 and got a 500 error.

Tried the AJAX search at /cgi-bin/koha/admin/branches.pl with Bug 25288 (including my follow-up) and it successfully/correctly filtered.
Comment 7 Jonathan Druart 2020-06-17 07:45:07 UTC
Created attachment 105935 [details] [review]
Bug 25774: Handle utf8 chars in REST API queries

We must utf8 encode the string before json decoding it.

Test plan:
Use "❤" and play with the library search (bug 25288) and Postman to
generate queries using it.

This patch prevents 500:
[2020/06/16 14:11:37] [ERROR] GET /api/v1/libraries: unhandled exception (Mojo::Exception)<<Wide character in subroutine entry at /kohadevbox/koha/Koha/REST/Plugin/Objects.pm line 107.>> Koha::REST::Plugin::Exceptions::__ANON__ /kohadevbox/koha/Koha/REST/Plugin/Exceptions.pm (73)
Comment 8 Jonathan Druart 2020-06-17 07:46:44 UTC
Hum weird, no idea what I did with the test. It should test correctly the changes now.

David, from the JSON pod:
"""
This module also exports to_json and from_json for backward compatibility. These are slower, and may expect/generate different stuff from what encode_json and decode_json do, depending on their options. It's better just to use Object-Oriented interfaces than using these two functions.
"""

We should use JSON->decode, see the last patch.
Comment 9 Tomás Cohen Arazi 2020-06-17 12:50:40 UTC
(In reply to Jonathan Druart from comment #8)
> Hum weird, no idea what I did with the test. It should test correctly the
> changes now.
> 
> David, from the JSON pod:
> """
> This module also exports to_json and from_json for backward compatibility.
> These are slower, and may expect/generate different stuff from what
> encode_json and decode_json do, depending on their options. It's better just
> to use Object-Oriented interfaces than using these two functions.
> """
> 
> We should use JSON->decode, see the last patch.

What about 

  my $json = JSON->new->utf8;

?
Comment 10 Tomás Cohen Arazi 2020-06-17 13:00:43 UTC
(In reply to Tomás Cohen Arazi from comment #9)
> (In reply to Jonathan Druart from comment #8)
> > Hum weird, no idea what I did with the test. It should test correctly the
> > changes now.
> > 
> > David, from the JSON pod:
> > """
> > This module also exports to_json and from_json for backward compatibility.
> > These are slower, and may expect/generate different stuff from what
> > encode_json and decode_json do, depending on their options. It's better just
> > to use Object-Oriented interfaces than using these two functions.
> > """
> > 
> > We should use JSON->decode, see the last patch.
> 
> What about 
> 
>   my $json = JSON->new->utf8;
> 
> ?

Forget about that last comment. Setting ->utf8 would mean we would need to explicitly call encode_utf8 on the string as the original patch did [1]

[1] utf8 flag disabled section in https://metacpan.org/pod/JSON#ENCODING/CODESET-FLAG-NOTES
Comment 11 David Cook 2020-06-18 02:30:09 UTC
(In reply to Jonathan Druart from comment #8)
> Hum weird, no idea what I did with the test. It should test correctly the
> changes now.
> 
> David, from the JSON pod:
> """
> This module also exports to_json and from_json for backward compatibility.
> These are slower, and may expect/generate different stuff from what
> encode_json and decode_json do, depending on their options. It's better just
> to use Object-Oriented interfaces than using these two functions.
> """
> 
> We should use JSON->decode, see the last patch.

Sounds good to me. Looking at the code for "from_json", I see what they mean, although they don't articulate it very well. 

I have some more urgent things to attend to, but I'll keep this in my inbox.
Comment 12 Kyle M Hall 2020-06-19 19:43:47 UTC
Created attachment 106109 [details] [review]
Bug 25774: Handle utf8 chars in REST API queries

We must utf8 encode the string before json decoding it.

Test plan:
Use "❤" and play with the library search (bug 25288) and Postman to
generate queries using it.

This patch prevents 500:
[2020/06/16 14:11:37] [ERROR] GET /api/v1/libraries: unhandled exception (Mojo::Exception)<<Wide character in subroutine entry at /kohadevbox/koha/Koha/REST/Plugin/Objects.pm line 107.>> Koha::REST::Plugin::Exceptions::__ANON__ /kohadevbox/koha/Koha/REST/Plugin/Exceptions.pm (73)

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Tomás Cohen Arazi 2020-06-19 19:47:34 UTC
Created attachment 106110 [details] [review]
Bug 25774: Handle utf8 chars in REST API queries

We must utf8 encode the string before json decoding it.

Test plan:
Use "❤" and play with the library search (bug 25288) and Postman to
generate queries using it.

This patch prevents 500:
[2020/06/16 14:11:37] [ERROR] GET /api/v1/libraries: unhandled exception (Mojo::Exception)<<Wide character in subroutine entry at /kohadevbox/koha/Koha/REST/Plugin/Objects.pm line 107.>> Koha::REST::Plugin::Exceptions::__ANON__ /kohadevbox/koha/Koha/REST/Plugin/Exceptions.pm (73)

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Jonathan Druart 2020-06-23 08:29:59 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 15 Jonathan Druart 2020-06-23 08:41:18 UTC
backported to 20.05.x for 20.05.01
Comment 16 Aleisha Amohia 2020-06-24 00:27:08 UTC
Does not apply cleanly to 19.11.x. Please rebase if required on 19.11.x