Bug 32401

Summary: x-koha-query cannot contain non-ISO-8859-1 values
Product: Koha Reporter: David Cook <dcook>
Component: AcquisitionsAssignee: Tomás Cohen Arazi <tomascohen>
Status: Pushed to stable --- QA Contact: Testopia <testopia>
Severity: critical    
Priority: P5 - low CC: david, jonathan.druart+koha, lucas, m.de.rooy, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.03
Bug Depends on: 32406, 32409    
Bug Blocks: 33971    
Attachments: Bug 32401: Remove x-koha-query support
Bug 32401: Remove x-koha-query support
Bug 32401: Remove x-koha-query support

Description David Cook 2022-12-05 02:55:23 UTC
If you try to search pending orders in acquisitions using Arabic like خمسة it will silently fail. Nothing changes on the screen. No "Processing" or obvious Javascript errors.

However, if you turn on "Pause on caught exceptions", you'll see the following:

TypeError: Failed to execute 'setRequestHeader' on 'XMLHttpRequest': String contains non ISO-8859-1 code point.
Comment 1 David Cook 2022-12-05 02:57:51 UTC
Note that this issue exists in older versions of Koha too.

The error is caught in the jQuery library, but I imagine the problem will be in the DataTables library. 

If you try it out at https://datatables.net/examples/api/multi_filter.html you'll see that there's no problem on the DataTables site, so I imagine it's either... a bug in our (old) version of DataTables, or a Koha bug...

I'll take a bit more of a look to see if there's anything I can do...
Comment 2 David Cook 2022-12-05 03:07:37 UTC
Ah, the problem is "x-koha-query".

It looks like browsers don't like headers with non ISO-8859-1 values:

"{"-and":[{"basket.vendor_id":46},[[{"basket.name":{"like":"%خمسة%"}}],[{"basket.basket_group.name":{"like":"%خمسة%"}}],[{"me.order_id":{"like":"%خمسة%"}}],[{"biblio.title":{"like":"%خمسة%"}},{"biblio.author":{"like":"%خمسة%"}},{"biblio.isbn":{"like":"%خمسة%"}},{"biblio.publisher":{"like":"%خمسة%"}},{"me.internal_note":{"like":"%خمسة%"}},{"me.vendor_note":{"like":"%خمسة%"}}],[{"me.replacement_price":{"like":"%خمسة%"}}],[{"me.quantity":{"like":"%خمسة%"}}],[{"me.ecost":{"like":"%خمسة%"}}],[{"fund.name":{"like":"%خمسة%"}}]]]}"

According to https://www.rfc-editor.org/rfc/rfc7230#section-3.2.4 

Historically, HTTP has allowed field content with text in the
   ISO-8859-1 charset [ISO-8859-1], supporting other charsets only
   through use of [RFC2047] encoding.  In practice, most HTTP header
   field values use only a subset of the US-ASCII charset [USASCII].
   Newly defined header fields SHOULD limit their field values to
   US-ASCII octets.  A recipient SHOULD treat other octets in field
   content (obs-text) as opaque data.
Comment 3 Jonathan Druart 2022-12-06 05:49:33 UTC
Yes, we should remove it.

I had a look at datatables.js and it seems safe to remove the related code there, we never pass query_parameters.
Comment 4 David Cook 2022-12-06 06:07:18 UTC
(In reply to Jonathan Druart from comment #3)
> Yes, we should remove it.
> 
> I had a look at datatables.js and it seems safe to remove the related code
> there, we never pass query_parameters.

I think we need to push bug 32406 and do a similar fix for koha-tmpl/intranet-tmpl/prog/en/modules/pos/register.tt as they both rely on x-koha-query at the moment
Comment 5 David Cook 2022-12-06 06:10:29 UTC
(In reply to Jonathan Druart from comment #3)
> I had a look at datatables.js and it seems safe to remove the related code
> there, we never pass query_parameters.

Although we may not pass query_parameters, it seems that we create them within datatables.js when using header_filter:

669                                     if(options.header_filter) {
670                                         options.query_parameters = query_parameters;
671                                     } else {
672                                         dataSet.q = query_parameters;
673                                         delete options.query_parameters;
674                                     }
Comment 6 Tomás Cohen Arazi 2022-12-06 14:22:42 UTC
Created attachment 144450 [details] [review]
Bug 32401: Remove x-koha-query support

HTTP headers can only contain ASCII characters, and it is thus
impractical to use a header for passing DB queries that could contain
non-ascii characters.

We don't need it, and should be removed.

This patch removes traces from the datatables.js files and also from
detail.tt, which had it set to false anyway.

See bug 32406 and bug 32409 for the cases in which it was being used,
and problematic.
Comment 7 David Nind 2022-12-07 01:09:23 UTC
Created attachment 144462 [details] [review]
Bug 32401: Remove x-koha-query support

HTTP headers can only contain ASCII characters, and it is thus
impractical to use a header for passing DB queries that could contain
non-ascii characters.

We don't need it, and should be removed.

This patch removes traces from the datatables.js files and also from
detail.tt, which had it set to false anyway.

See bug 32406 and bug 32409 for the cases in which it was being used,
and problematic.

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2022-12-07 01:20:52 UTC
This is what I did to test - I hope this is sufficient (using KTD):

1. Repeat the steps in bug 32406 to set an order and then go to receive the order.

2. Enable the web developer tools for your browser on the page.

3. For Firefox: 
   - select Network
   - change the requests that are displayed from "All" to "XHR"
   - refresh the page
   - click on the page in the results to display the headers
   - in the "Request Headers" section you should see:
     x-koha-query: {"basket.vendor_id":1}

4. After the patch is applied (and a restart_all) the x-koha-query is no longer displayed.
Comment 9 Marcel de Rooy 2022-12-09 10:01:15 UTC
(In reply to David Cook from comment #0)
> If you try to search pending orders in acquisitions using Arabic like خمسة
> it will silently fail. Nothing changes on the screen. No "Processing" or
> obvious Javascript errors.
> 
> However, if you turn on "Pause on caught exceptions", you'll see the
> following:
> 
> TypeError: Failed to execute 'setRequestHeader' on 'XMLHttpRequest': String
> contains non ISO-8859-1 code point.

Just a dumb question, but since Koha is multilingual and filtering on UTF8 chars does not really sound asking too much ?
Why dont we use encodeURIComponent on the header string and call url_decode_utf8 on the receiving side?

This could be kind of a convention for x-koha headers? Search for percent encoding, evaluate results of decoding? And be done quite early in the process on a general level when processing (PSGI) headers?
Comment 10 David Cook 2022-12-12 05:03:39 UTC
(In reply to Marcel de Rooy from comment #9)
> Why dont we use encodeURIComponent on the header string and call
> url_decode_utf8 on the receiving side?
> 
> This could be kind of a convention for x-koha headers? Search for percent
> encoding, evaluate results of decoding? And be done quite early in the
> process on a general level when processing (PSGI) headers?

It sounds hacky to me. I've never heard of any other system doing something like that. Technically we could do it, but I think we should ask ourselves why we'd want to. (You make an interesting point about that on bug 32406 of course.)
Comment 11 Marcel de Rooy 2022-12-16 08:10:48 UTC
Just for reference:

From RFC 8187 (Proposed standard) Indicating Character Encoding and Language for HTTP Header Field Parameters
Extended notation, using the Unicode character U+00A3 ("£", POUND SIGN):
     foo: bar; title*=utf-8'en'%C2%A3%20rates
Note: The Unicode pound sign character U+00A3 was encoded into the octet sequence C2 A3 using the UTF-8 character encoding, and then percent-encoded.  Also, note that the space character was encoded as %20, as it is not contained in attr-char.
Comment 12 Jonathan Druart 2023-02-01 10:10:55 UTC
Created attachment 145926 [details] [review]
Bug 32401: Remove x-koha-query support

HTTP headers can only contain ASCII characters, and it is thus
impractical to use a header for passing DB queries that could contain
non-ascii characters.

We don't need it, and should be removed.

This patch removes traces from the datatables.js files and also from
detail.tt, which had it set to false anyway.

See bug 32406 and bug 32409 for the cases in which it was being used,
and problematic.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Tomás Cohen Arazi 2023-02-02 15:01:54 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 14 Jacob O'Mara 2023-02-06 15:58:06 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 15 Lucas Gass 2023-02-14 21:40:08 UTC
Doesn't apply cleanly to 22.05.x, if needed please rebase.