Bug 28316 - Fix ES crashes related to various punctuation characters
Summary: Fix ES crashes related to various punctuation characters
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low normal with 10 votes (vote)
Assignee: Peter Vashchuk
QA Contact:
URL:
Keywords:
: 28292 (view as bug list)
Depends on:
Blocks: 26314 28484 29284 29418
  Show dependency treegraph
 
Reported: 2021-05-11 10:31 UTC by Joonas Kylmälä
Modified: 2021-11-05 17:03 UTC (History)
21 users (show)

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


Attachments
Bug 28316: screen square and curly brackets in the search query (3.93 KB, patch)
2021-05-26 14:38 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: screen square and curly brackets in the search query (3.99 KB, patch)
2021-05-27 07:48 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 28316: different QueryRegexEscapeOptions' values work as expected (4.56 KB, patch)
2021-06-03 08:20 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: different QueryRegexEscapeOptions' values work as expected (4.52 KB, patch)
2021-06-03 08:25 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: remove unquoted semicolons and screen all followup colons (1.90 KB, patch)
2021-06-10 10:51 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: escape brackets in the search query (1.69 KB, patch)
2021-06-10 10:51 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: escape exclamation signs in the query (2.04 KB, patch)
2021-06-10 10:51 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: avoid messing up regexes in the search queries (1.82 KB, patch)
2021-06-10 10:51 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: escape ES ranges if QueryAutoTruncate is enabled (2.02 KB, patch)
2021-06-10 10:51 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: add tests (4.20 KB, patch)
2021-06-10 10:51 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: screen unquoted semicolons and all followup colons (1.93 KB, patch)
2021-06-11 13:23 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: escape brackets in the search query (1.69 KB, patch)
2021-06-11 13:24 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: escape exclamation signs in the query (2.06 KB, patch)
2021-06-11 13:24 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: avoid messing up regexes in the search queries (1.86 KB, patch)
2021-06-11 13:24 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: escape ES ranges if QueryAutoTruncate is enabled (2.02 KB, patch)
2021-06-11 13:24 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: add tests (4.85 KB, patch)
2021-06-11 13:24 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: screen unquoted semicolons and all followup colons (2.31 KB, patch)
2021-06-15 07:46 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: escape brackets in the search query (1.71 KB, patch)
2021-06-15 07:47 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: escape exclamation signs in the query (2.02 KB, patch)
2021-06-15 07:47 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: avoid messing up regexes in the search queries (1.88 KB, patch)
2021-06-15 07:47 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: avoid messing up regexes in the search queries (2.27 KB, patch)
2021-06-15 07:47 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: add tests (4.87 KB, patch)
2021-06-15 07:47 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: screen unquoted semicolons and all followup colons (2.29 KB, patch)
2021-06-18 07:49 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: escape brackets in the search query (1.71 KB, patch)
2021-06-18 07:49 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: escape exclamation signs in the query (2.02 KB, patch)
2021-06-18 07:49 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: avoid messing up regexes in the search queries (1.87 KB, patch)
2021-06-18 07:49 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: avoid messing up regexes in the search queries (2.22 KB, patch)
2021-06-18 07:49 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: add tests (4.87 KB, patch)
2021-06-18 07:49 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: add tests (4.93 KB, patch)
2021-06-21 05:56 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28316: screen unquoted semicolons and all followup colons (2.35 KB, patch)
2021-06-21 05:56 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28316: escape brackets in the search query (1.76 KB, patch)
2021-06-21 05:56 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28316: escape exclamation signs in the query (2.08 KB, patch)
2021-06-21 05:56 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28316: avoid messing up regexes in the search queries (1.87 KB, patch)
2021-06-21 05:57 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28316: avoid messing up regexes in the search queries (2.22 KB, patch)
2021-06-21 05:57 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28316: add tests (4.93 KB, patch)
2021-07-13 11:24 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: screen unquoted semicolons and all followup colons (2.35 KB, patch)
2021-07-13 11:24 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: escape brackets in the search query (1.46 KB, patch)
2021-07-13 11:24 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: escape exclamation signs in the query (2.10 KB, patch)
2021-07-13 11:25 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: avoid messing up regexes in the search queries (1.87 KB, patch)
2021-07-13 11:25 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: escape ES ranges if QueryAutoTruncate is enabled (2.34 KB, patch)
2021-07-13 11:25 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: add tests (4.99 KB, patch)
2021-08-08 23:13 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 28316: screen unquoted semicolons and all followup colons (2.41 KB, patch)
2021-08-08 23:13 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 28316: escape brackets in the search query (1.52 KB, patch)
2021-08-08 23:13 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 28316: escape exclamation signs in the query (2.15 KB, patch)
2021-08-08 23:13 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 28316: avoid messing up regexes in the search queries (1.93 KB, patch)
2021-08-08 23:13 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 28316: escape ES ranges if QueryAutoTruncate is enabled (2.40 KB, patch)
2021-08-08 23:13 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 28316: add tests (5.05 KB, patch)
2021-09-01 15:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28316: screen unquoted semicolons and all followup colons (2.47 KB, patch)
2021-09-01 15:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28316: escape brackets in the search query (1.58 KB, patch)
2021-09-01 15:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28316: escape exclamation signs in the query (2.22 KB, patch)
2021-09-01 15:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28316: avoid messing up regexes in the search queries (1.99 KB, patch)
2021-09-01 15:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28316: escape ES ranges if QueryAutoTruncate is enabled (2.46 KB, patch)
2021-09-01 15:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28316: (QA follow-up) Make clean_search_term public (8.45 KB, patch)
2021-09-01 15:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28316: add tests (5.05 KB, patch)
2021-09-06 13:51 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: screen unquoted semicolons and all followup colons (2.47 KB, patch)
2021-09-06 13:51 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: escape brackets in the search query (1.58 KB, patch)
2021-09-06 13:52 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: escape exclamation signs in the query (2.22 KB, patch)
2021-09-06 13:52 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: avoid messing up regexes in the search queries (1.99 KB, patch)
2021-09-06 13:52 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: escape ES ranges if QueryAutoTruncate is enabled (2.86 KB, patch)
2021-09-06 13:52 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: (QA follow-up) Make clean_search_term public (8.45 KB, patch)
2021-09-06 13:52 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28316: add tests (5.11 KB, patch)
2021-09-09 13:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28316: screen unquoted semicolons and all followup colons (2.53 KB, patch)
2021-09-09 13:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28316: escape brackets in the search query (1.65 KB, patch)
2021-09-09 13:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28316: escape exclamation signs in the query (2.28 KB, patch)
2021-09-09 13:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28316: avoid messing up regexes in the search queries (2.05 KB, patch)
2021-09-09 13:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28316: escape ES ranges if QueryAutoTruncate is enabled (2.92 KB, patch)
2021-09-09 13:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28316: (QA follow-up) Make clean_search_term public (8.51 KB, patch)
2021-09-09 13:03 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-05-11 10:31:46 UTC
When the Elasticsearch search engine is used one cannot search for biblios with titles that contain square brakets ([ or ]). You will get an error if you try to search with the title of the biblio as it is written fully, e.g. "The dog [home edition]"
Comment 1 Nicolas Legrand 2021-05-12 08:08:40 UTC
It also make fail detail.pl and opac-detail.pl. You can't print those pages when there are brackets in the title (internal server error: reason' => "Failed to parse query [(host-item:Vietnam, un [plus] un [\x{e9}gal] un)]").

You can escape brackets in the $cleaned_title variable like this:

$cleaned_title =~ s/(?:\[|\])//g;

and then the pages are printable. But that doesn't solve the search problem.
Comment 2 Nick Clemens 2021-05-14 13:38:12 UTC
This is a decision we need to make. Do we want to automatically escape these characters, and then we cannot use range searches in Koha, or do we want to allow for more advanced searching, but ask the user to escape things?
Comment 3 Ere Maijala 2021-05-14 13:40:49 UTC
Perhaps we could try to reduce the problem by first trying without escaping and then escaping if the first attempt failed? I admit I haven't really thought this through, but it could allow the flexibility while still making the searches work.
Comment 4 Andrew Nugged 2021-05-21 11:16:52 UTC
(In reply to Nick Clemens from comment #2)
> This is a decision we need to make. Do we want to automatically escape these
> characters, and then we cannot use range searches in Koha, or do we want to
> allow for more advanced searching, but ask the user to escape things?


What if we introduce config variable (so Koha can behave in the old way for those who don't use this or not changed, because of proper default value we will set), and even we can have that variable more advanced state "extra checkbox" under the search field, so:


"useAdvancedSearchSyntax" config var with three states:
  1. always (current Koha style)
  2. never (i.e. all special characters auto escaped)
  3. show checkbox "on/off advanced search syntax" just under the search field

third option I mean: 

[___________________] [Submit]
[x] disable advanced syntax (syntax help link)


so we sill have some URL like this:
/cgi-bin/koha/catalogue/search.pl?q=test+book&disable_adv_syntax=1
and adding this +1 parameter to search form this also allows to generate externals search links if we need

?
Comment 5 Joonas Kylmälä 2021-05-25 11:40:17 UTC
The issue is a lot wider than just square brackets, please see the list of reserved characters that need to be escaped if we are not using them as operators: https://www.elastic.co/guide/en/elasticsearch/reference/2.1/query-dsl-query-string-query.html#_reserved_characters
Comment 6 Peter Vashchuk 2021-05-26 14:38:25 UTC
Created attachment 121435 [details] [review]
Bug 28316: screen square and curly brackets in the search query

This patch screens all square and curly brackets which have no special 
language meaning, while keeping ranges as they are, allowing ES to 
search for biblios with titles that contain square and curly brackets 
and at the same time allows the use of range searches ([1990 TO 2000]). 
Previously, there was old time fix for curly brackets that replaced them
with quotes which made curly brackets range functionality not working, 
that code was also removed with this patch Also this patch adds tests 
for this behavior.

To reproduce:
1) with ES enabled, search for a book with tittle like "test [test]",
which will result in error.
2) apply the patch.
3) search for that book again, which should work now.
4) run test QueryBuilder.t to make sure that it runs successfully.
Comment 7 Emmi Takkinen 2021-05-27 07:48:03 UTC
Created attachment 121443 [details] [review]
Bug 28316: screen square and curly brackets in the search query

This patch screens all square and curly brackets which have no special
language meaning, while keeping ranges as they are, allowing ES to
search for biblios with titles that contain square and curly brackets
and at the same time allows the use of range searches ([1990 TO 2000]).
Previously, there was old time fix for curly brackets that replaced them
with quotes which made curly brackets range functionality not working,
that code was also removed with this patch Also this patch adds tests
for this behavior.

To reproduce:
1) with ES enabled, search for a book with tittle like "test [test]",
which will result in error.
2) apply the patch.
3) search for that book again, which should work now.
4) run test QueryBuilder.t to make sure that it runs successfully.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 8 Nick Clemens 2021-05-27 13:47:35 UTC
This is a great improvement, I have been playing with this and bug 28326.

I found most things work, but QueryAutoTruncate does play some role here.

Title of record:
The ideas behind the chess openings "fox quick"~5 tag:{alpha TO omega} 

With QueryAutoTruncate = only if * is added:
The ideas behind the chess openings "fox quick"~5 tag:{alpha TO omega} => No results
Quote it:
"The ideas behind the chess openings "fox quick"~5 tag:{alpha TO omega}" => Results!

With QueryAutoTrunacte = automatically:
The ideas behind the chess openings "fox quick"~5 tag:{alpha TO omega}  => Error: Unable to perform your search

Quote it:
"The ideas behind the chess openings "fox quick"~5 tag:{alpha TO omega} " => Results


I find that with this patch, and then adding quotes around the search in bug 28326:
diff --git a/catalogue/detail.pl b/catalogue/detail.pl
index 8d06f0da51..b11346162c 100755
--- a/catalogue/detail.pl
+++ b/catalogue/detail.pl
@@ -132,7 +132,7 @@ if ( $xslfile ) {
     my $query =
       ( C4::Context->preference('UseControlNumber') and $record->field('001') )
       ? 'rcn:'. $record->field('001')->data . ' AND (bib-level:a OR bib-level:b)'
-      : "Host-item:$cleaned_title";
+      : 'Host-item:"'.$cleaned_title.'"';

Most things work.

I am pinging Victor as he has been finding ways to fail all of these, but I think we are on a good track
Comment 9 Joonas Kylmälä 2021-05-27 13:52:07 UTC
When testing this I think it would be good idea to verify if the behaviour with QueryRegexEscapeOptions set to other than "Escape" value still works as expected. Although I have come across many problems in the past already if QueryRegexEscapeOptions doesn't have the "Escape" option set so I'm not sure how it is actually even supposed to work with the other values.
Comment 10 Katrin Fischer 2021-05-29 14:22:34 UTC
If the scope here widens, please remember to update the bug description! (still in release notes mode here... )
Comment 11 Victor Grousset/tuxayo 2021-06-01 23:04:26 UTC
With the patch of comment 8 and services restarted.
Retried the case listed in the comment 8:

> With QueryAutoTruncate = only if * is added:
> The ideas behind the chess openings "fox quick"~5 tag:{alpha TO omega} => No results

I still have no result with the patch from comment 8
I still have to quote it.

> With QueryAutoTrunacte = automatically:
> The ideas behind the chess openings "fox quick"~5 tag:{alpha TO omega}  => Error: Unable to perform your search

Still error

With QueryAutoTruncate=both possible values
For a search of the following and a record named the following:
name:/joh?n(ath[oa]n)/
=> no result

With QueryAutoTruncate=only if *
For a search of the following and a record named the following:
count:[10 TO *]
=> no result

With QueryAutoTruncate=auto
For a search of the following and a record named the following:
count:[10 TO *]
=> Error: Unable to perform your search

With QueryAutoTruncate=only if *
For a search of the following and a record named the following:
age:(+>=10 +<20)
=> no result

With QueryAutoTruncate=auto
For a search of the following and a record named the following:
age:(+>=10 +<20)
=> no result

With QueryAutoTruncate=only if *
For a search of the following and a record named the following:
quick^2 fox
=> Error: Unable to perform your search

With QueryAutoTruncate=only if *
For a search of the following and a record named the following:
The reserved characters are: + - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ /
=> Error: Unable to perform your search

With QueryAutoTruncate=auto
For a search of the following and a record named the following:
The reserved characters are: + - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ /
=> Error: Unable to perform your search

With QueryAutoTruncate=auto
For a search of the following (no record)
[chess TO *]
=> Error: Unable to perform your search

With QueryAutoTruncate=only if *
For a search of the following (no record)
[chess TO *]
=> whole catalogue
Maybe it's expected

With QueryAutoTruncate=auto
For a search of the following (no record)
Nmap network scanning :  <> ><
=> Error: Unable to perform your search

Not sure what is expected in the above results.
Comment 12 Victor Grousset/tuxayo 2021-06-01 23:28:24 UTC
With QueryAutoTruncate=both possible values
nmap  onfocus=JaVaSCript:alert(9) autofocus
=> Error: Unable to perform your search

With QueryAutoTruncate=both possible values
nmap " onfocus=JaVaSCript:alert(10) autofocus
=> Error: Unable to perform your search
Comment 13 Peter Vashchuk 2021-06-03 08:20:35 UTC Comment hidden (obsolete)
Comment 14 Peter Vashchuk 2021-06-03 08:25:20 UTC Comment hidden (obsolete)
Comment 15 Peter Vashchuk 2021-06-10 10:49:47 UTC
Changed the scope of the ticket, now this ticket is about various punctuation characters, like [], {}, : and !, that cause internal server errors when you use Elastic Search.
Comment 16 Peter Vashchuk 2021-06-10 10:51:23 UTC
Created attachment 121751 [details] [review]
Bug 28316: remove unquoted semicolons and screen all followup colons

Currently searches like: "book:", ":book" and "host-item:test:n"
cause internal server errors.

This patch adds additional regexes that strips the colons from the start
and end of the query, and another regex that screens all follow-up
colons that go after the first colon to avoid errors when searching for
"host-item:test:n".

To reproduce:
1) using ES, search for the book with title that contains
semicolon at the start or at the end of the line, separated with spaces,
this should cause internal server error.
2) try doing the same with something like "host-item:test:n", it should 
result in error as well.
3) apply the patch.
4) repeat steps 1-2, ensure that it works now.
Comment 17 Peter Vashchuk 2021-06-10 10:51:28 UTC
Created attachment 121752 [details] [review]
Bug 28316: escape brackets in the search query

This patch screens square and curly brackets which have no special
language meaning, while keeping ranges as they are, allowing ES to
search for biblios with titles that contain square and curly brackets
and at the same time allows the use of range searches ([1990 TO 2000]).

To reproduce:
1) using ES, search for the book with title that contains
square and/or curly brackets, like "book [second edition]", which will
result in error.
2) apply the patch.
3) search for that book again, ensure that it works now.
4) additionally perform a search using range,
for example: "[1999 TO 2020]", ensure that it works correctly too.
Comment 18 Peter Vashchuk 2021-06-10 10:51:33 UTC
Created attachment 121753 [details] [review]
Bug 28316: escape exclamation signs in the query

Currently having exclamation sign at the end of the query makes ES
search fail, and when you try to search for a book that has exclamation
sign in the tittle (something like "Words! words") won't show results
correctly as it tries to negate everything that is after exclamation
sign, making it impossible to search for books that have in in the title

This patch escapes exclamation signs if it's at the end of the query or
has a space after it, resolving both of the issues listed above.

To reproduce:
1) with ES enabled, search for the book with title that contains
exclamation sight at the end, like "book!", this search should result
 in error.
2) do another search, but this time find/prepare beforehand book with a
title that has exclamation sign with a space after it,
e.g "exclamation! sign", it shouldn't find it as ES treats everything
after that exclamation sign as negation.
2) apply the patch.
3) perform searches from the steep one and two again.
Search from step one should no longer fail, while search from the step
two should find that book.
Comment 19 Peter Vashchuk 2021-06-10 10:51:38 UTC
Created attachment 121754 [details] [review]
Bug 28316: avoid messing up regexes in the search queries

This patch ensures that the behavior with
QueryRegexEscapeOptions set to values other than
"Escape" still will works as expected.

It does so by storing the contents of regexes
before escaping special characters and
then restores the contents of regexes back to how
it was before, ensuring that searching with regex is possible.
Comment 20 Peter Vashchuk 2021-06-10 10:51:43 UTC
Created attachment 121755 [details] [review]
Bug 28316: escape ES ranges if QueryAutoTruncate is enabled

if QueryAutoTruncate enabled we will have any special operators ruined 
for example: "test [6 TO 7]" will be converted to "test* [6* TO* 7]"
so no reason to keep ranges when QueryAutoTruncate set to "enabled"
Comment 21 Peter Vashchuk 2021-06-10 10:51:48 UTC
Created attachment 121756 [details] [review]
Bug 28316: add tests
Comment 22 Peter Vashchuk 2021-06-11 09:29:35 UTC
*** Bug 28292 has been marked as a duplicate of this bug. ***
Comment 23 Peter Vashchuk 2021-06-11 13:23:59 UTC
Created attachment 121837 [details] [review]
Bug 28316: screen unquoted semicolons and all followup colons

Currently searches like: "book:", ":book" and "host-item:test:n"
cause internal server errors.

This patch adds additional regexes that screens the colons at the start
and end of the query, and another regex that screens all follow-up
colons that go after the first colon to avoid errors when searching for
"host-item:test:n".

To reproduce:
1) using ES, search for the book with title that contains
semicolon at the start or at the end of the line, separated with spaces,
this should cause internal server error.
2) try doing the same with something like "host-item:test:n", it should
result in error as well.
3) apply the patch.
4) repeat steps 1-2, ensure that it works now.
Comment 24 Peter Vashchuk 2021-06-11 13:24:06 UTC
Created attachment 121838 [details] [review]
Bug 28316: escape brackets in the search query

This patch screens square and curly brackets which have no special
language meaning, while keeping ranges as they are, allowing ES to
search for biblios with titles that contain square and curly brackets
and at the same time allows the use of range searches ([1990 TO 2000]).

To reproduce:
1) using ES, search for the book with title that contains
square and/or curly brackets, like "book [second edition]", which will
result in error.
2) apply the patch.
3) search for that book again, ensure that it works now.
4) additionally perform a search using range,
for example: "[1999 TO 2020]", ensure that it works correctly too.
Comment 25 Peter Vashchuk 2021-06-11 13:24:12 UTC
Created attachment 121839 [details] [review]
Bug 28316: escape exclamation signs in the query

Currently having exclamation sign at the end of the query makes ES
search fail, and when you try to search for a book that has exclamation
sign in the tittle (something like "Words! words") won't show results
correctly as it tries to negate everything that is after exclamation
sign, making it impossible to search for books that have in in the title

This patch escapes exclamation signs if it's at the end of the query or
has a space after it, resolving both of the issues listed above.

To reproduce:
1) with ES enabled, search for the book with title that contains
exclamation sight at the end, like "book!", this search should result
 in error.
2) do another search, but this time find/prepare beforehand book with a
title that has exclamation sign with a space after it,
e.g "exclamation! sign", it shouldn't find it as ES treats everything
after that exclamation sign as negation.
2) apply the patch.
3) perform searches from the steep one and two again.
Search from step one should no longer fail, while search from the step
two should find that book.
Comment 26 Peter Vashchuk 2021-06-11 13:24:17 UTC
Created attachment 121840 [details] [review]
Bug 28316: avoid messing up regexes in the search queries

This patch ensures that the behavior with
QueryRegexEscapeOptions set to values other than
"Escape" still will works as expected.

It does so by storing the contents of regexes
before escaping special characters and
then restores the contents of regexes back to how
it was before, ensuring that searching with regex is possible.
Comment 27 Peter Vashchuk 2021-06-11 13:24:23 UTC
Created attachment 121841 [details] [review]
Bug 28316: escape ES ranges if QueryAutoTruncate is enabled

if QueryAutoTruncate enabled we will have any special operators ruined 
for example: "test [6 TO 7]" will be converted to "test* [6* TO* 7]"
so no reason to keep ranges when QueryAutoTruncate set to "enabled"
Comment 28 Peter Vashchuk 2021-06-11 13:24:28 UTC
Created attachment 121842 [details] [review]
Bug 28316: add tests
Comment 29 Peter Vashchuk 2021-06-15 07:46:54 UTC
Created attachment 121975 [details] [review]
Bug 28316: screen unquoted semicolons and all followup colons

Currently searches like: "book:", ":book" and "host-item:test:n"
cause internal server errors.

This patch adds additional regexes that remove the colons at the start
and end of the query, and another regex that screens all follow-up
colons that go after the first colon to avoid errors when searching for
"host-item:test:n".

To reproduce:
1) using ES, search for the book with title that contains
semicolon at the start or at the end of the line, separated with spaces,
this should cause internal server error.
2) try doing the same with something like "host-item:test:n", it should
result in error as well.
3) apply the patch.
4) repeat steps 1-2, ensure that it works now.
Comment 30 Peter Vashchuk 2021-06-15 07:47:00 UTC
Created attachment 121976 [details] [review]
Bug 28316: escape brackets in the search query

This patch screens square and curly brackets which have no special
language meaning, while keeping ranges as they are, allowing ES to
search for biblios with titles that contain square and curly brackets
and at the same time allows the use of range searches ([1990 TO 2000]).

To reproduce:
1) using ES, search for the book with title that contains
square and/or curly brackets, like "book [second edition]", which will
result in error.
2) apply the patch.
3) search for that book again, ensure that it works now.
4) additionally perform a search using range,
for example: "[1999 TO 2020]", ensure that it works correctly too.
Comment 31 Peter Vashchuk 2021-06-15 07:47:05 UTC
Created attachment 121977 [details] [review]
Bug 28316: escape exclamation signs in the query

Currently having exclamation sign at the end of the query makes ES
search fail, and when you try to search for a book that has exclamation
sign in the tittle (something like "Words! words") won't show results
correctly as it tries to negate everything that is after exclamation
sign, making it impossible to search for books that have in in the title

This patch escapes exclamation signs if it's at the end of the query or
has a space after it, resolving both of the issues listed above.

To reproduce:
1) with ES enabled, search for the book with title that contains
exclamation sight at the end, like "book!", this search should result
 in error.
2) do another search, but this time find/prepare beforehand book with a
title that has exclamation sign with a space after it,
e.g "exclamation! sign", it shouldn't find it as ES treats everything
after that exclamation sign as negation.
2) apply the patch.
3) perform searches from the steep one and two again.
Search from step one should no longer fail, while search from the step
two should find that book.
Comment 32 Peter Vashchuk 2021-06-15 07:47:10 UTC
Created attachment 121978 [details] [review]
Bug 28316: avoid messing up regexes in the search queries

This patch ensures that the behavior with
QueryRegexEscapeOptions set to values other than
"Escape" still will works as expected.

It does so by storing the contents of regexes
before escaping special characters and
then restores the contents of regexes back to how
it was before, ensuring that searching with regex is possible.
Comment 33 Peter Vashchuk 2021-06-15 07:47:15 UTC
Created attachment 121979 [details] [review]
Bug 28316: avoid messing up regexes in the search queries

This patch ensures that the behavior with
QueryRegexEscapeOptions set to values other than
"Escape" still will works as expected.

It does so by storing the contents of regexes
before escaping special characters and
then restores the contents of regexes back to how
it was before, ensuring that searching with regex is possible.
Comment 34 Peter Vashchuk 2021-06-15 07:47:20 UTC
Created attachment 121980 [details] [review]
Bug 28316: add tests
Comment 35 Peter Vashchuk 2021-06-18 07:49:03 UTC
Created attachment 122103 [details] [review]
Bug 28316: screen unquoted semicolons and all followup colons

Currently searches like: "book:", ":book" and "host-item:test:n"
cause internal server errors.

This patch adds additional regexes that remove the colons at the start
and end of the query, and another regex that screens all follow-up
colons that go after the first colon to avoid errors when searching for
"host-item:test:n".

To reproduce:
1) using ES, search for the book with title that contains
semicolon at the start or at the end of the line, separated with spaces,
this should cause internal server error.
2) try doing the same with something like "host-item:test:n", it should
result in error as well.
3) apply the patch.
4) repeat steps 1-2, ensure that it works now.
Comment 36 Peter Vashchuk 2021-06-18 07:49:09 UTC
Created attachment 122104 [details] [review]
Bug 28316: escape brackets in the search query

This patch screens square and curly brackets which have no special
language meaning, while keeping ranges as they are, allowing ES to
search for biblios with titles that contain square and curly brackets
and at the same time allows the use of range searches ([1990 TO 2000]).

To reproduce:
1) using ES, search for the book with title that contains
square and/or curly brackets, like "book [second edition]", which will
result in error.
2) apply the patch.
3) search for that book again, ensure that it works now.
4) additionally perform a search using range,
for example: "[1999 TO 2020]", ensure that it works correctly too.
Comment 37 Peter Vashchuk 2021-06-18 07:49:15 UTC
Created attachment 122105 [details] [review]
Bug 28316: escape exclamation signs in the query

Currently having exclamation sign at the end of the query makes ES
search fail, and when you try to search for a book that has exclamation
sign in the tittle (something like "Words! words") won't show results
correctly as it tries to negate everything that is after exclamation
sign, making it impossible to search for books that have in in the title

This patch escapes exclamation signs if it's at the end of the query or
has a space after it, resolving both of the issues listed above.

To reproduce:
1) with ES enabled, search for the book with title that contains
exclamation sight at the end, like "book!", this search should result
 in error.
2) do another search, but this time find/prepare beforehand book with a
title that has exclamation sign with a space after it,
e.g "exclamation! sign", it shouldn't find it as ES treats everything
after that exclamation sign as negation.
2) apply the patch.
3) perform searches from the steep one and two again.
Search from step one should no longer fail, while search from the step
two should find that book.
Comment 38 Peter Vashchuk 2021-06-18 07:49:20 UTC
Created attachment 122106 [details] [review]
Bug 28316: avoid messing up regexes in the search queries

This patch ensures that the behavior with
QueryRegexEscapeOptions set to values other than
"Escape" still will works as expected.

It does so by storing the contents of regexes
before escaping special characters and
then restores the contents of regexes back to how
it was before, ensuring that searching with regex is possible.
Comment 39 Peter Vashchuk 2021-06-18 07:49:26 UTC
Created attachment 122107 [details] [review]
Bug 28316: avoid messing up regexes in the search queries

This patch ensures that the behavior with
QueryRegexEscapeOptions set to values other than
"Escape" still will works as expected.

It does so by storing the contents of regexes
before escaping special characters and
then restores the contents of regexes back to how
it was before, ensuring that searching with regex is possible.
Comment 40 Peter Vashchuk 2021-06-18 07:49:32 UTC
Created attachment 122108 [details] [review]
Bug 28316: add tests
Comment 41 Victor Grousset/tuxayo 2021-06-21 03:32:35 UTC
> 4) additionally perform a search using range,
for example: "[1999 TO 2020]", ensure that it works correctly too.

What values should be in the record for the search to yield it?
It doesn't say anymore "You did not specify any search criteria." So it's better.
But it says that there are not matches. I'm not sure ranges actually work. (because i don't know how to use them ^^")

I tried «date-of-publication:[1950:2020]»
Is that a correct query for a record with [1984] or 1984 in 260$c
Comment 42 Victor Grousset/tuxayo 2021-06-21 03:33:48 UTC
errata: «date-of-publication:[1950 TO 2020]»
Comment 43 Victor Grousset/tuxayo 2021-06-21 05:56:39 UTC
Created attachment 122201 [details] [review]
Bug 28316: add tests

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 44 Victor Grousset/tuxayo 2021-06-21 05:56:45 UTC
Created attachment 122202 [details] [review]
Bug 28316: screen unquoted semicolons and all followup colons

Currently searches like: "book:", ":book" and "host-item:test:n"
cause internal server errors.

This patch adds additional regexes that remove the colons at the start
and end of the query, and another regex that screens all follow-up
colons that go after the first colon to avoid errors when searching for
"host-item:test:n".

To reproduce:
1) using ES, search for the book with title that contains
semicolon at the start or at the end of the line, separated with spaces,
this should cause internal server error.
2) try doing the same with something like "host-item:test:n", it should
result in error as well.
3) apply the patch.
4) repeat steps 1-2, ensure that it works now.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 45 Victor Grousset/tuxayo 2021-06-21 05:56:50 UTC
Created attachment 122203 [details] [review]
Bug 28316: escape brackets in the search query

This patch screens square and curly brackets which have no special
language meaning, while keeping ranges as they are, allowing ES to
search for biblios with titles that contain square and curly brackets
and at the same time allows the use of range searches ([1990 TO 2000]).

To reproduce:
1) using ES, search for the book with title that contains
square and/or curly brackets, like "book [second edition]", which will
result in error.
2) apply the patch.
3) search for that book again, ensure that it works now.
4) additionally perform a search using range,
for example: "[1999 TO 2020]", ensure that it works correctly too.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 46 Victor Grousset/tuxayo 2021-06-21 05:56:55 UTC
Created attachment 122204 [details] [review]
Bug 28316: escape exclamation signs in the query

Currently having exclamation sign at the end of the query makes ES
search fail, and when you try to search for a book that has exclamation
sign in the tittle (something like "Words! words") won't show results
correctly as it tries to negate everything that is after exclamation
sign, making it impossible to search for books that have in in the title

This patch escapes exclamation signs if it's at the end of the query or
has a space after it, resolving both of the issues listed above.

To reproduce:
1) with ES enabled, search for the book with title that contains
exclamation sight at the end, like "book!", this search should result
 in error.
2) do another search, but this time find/prepare beforehand book with a
title that has exclamation sign with a space after it,
e.g "exclamation! sign", it shouldn't find it as ES treats everything
after that exclamation sign as negation.
2) apply the patch.
3) perform searches from the steep one and two again.
Search from step one should no longer fail, while search from the step
two should find that book.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 47 Victor Grousset/tuxayo 2021-06-21 05:57:00 UTC
Created attachment 122205 [details] [review]
Bug 28316: avoid messing up regexes in the search queries

This patch ensures that the behavior with
QueryRegexEscapeOptions set to values other than
"Escape" still will works as expected.

It does so by storing the contents of regexes
before escaping special characters and
then restores the contents of regexes back to how
it was before, ensuring that searching with regex is possible.
Comment 48 Victor Grousset/tuxayo 2021-06-21 05:57:06 UTC
Created attachment 122206 [details] [review]
Bug 28316: avoid messing up regexes in the search queries

This patch ensures that the behavior with
QueryRegexEscapeOptions set to values other than
"Escape" still will works as expected.

It does so by storing the contents of regexes
before escaping special characters and
then restores the contents of regexes back to how
it was before, ensuring that searching with regex is possible.
Comment 49 Victor Grousset/tuxayo 2021-06-21 06:00:40 UTC
The test plan yielded good results :)

Patches were reordered to make it easier to check the tests (they indeed fail when the implementation is missing)

Do these commits need a test plan?
Bug 28316: avoid messing up regexes in the search queries
Bug 28316: avoid messing up regexes in the search queries
(they have the same name and description, suspicious ^^)
Comment 50 Andrew Nugged 2021-06-21 06:15:59 UTC
You right, Victor, I see also that description messed up and diverged after lot of reworks, let's Petro fix this,

Petro,

I assume patch 122205 explanation should be without "QueryRegexEscapeOptions" paragraph,

and patch 122206 related to:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28316#c27
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28316#c45
so it should be about "range", not regex, about hiding ranges from escaping
actually, I see most description from 122206 appears in 122203,
and 122203 is just about "replacing brackets which not have special meaning" only.
Comment 51 Victor Grousset/tuxayo 2021-06-22 07:33:22 UTC
I forgot I tested this in the past.

What should be expected to be different from comment 11 results?
- Should I still have to quote stuff (the cases of comment 8)
- Should some stuff return results instead of none?
- No crash I guess? Speaking of that:

With QueryAutoTruncate=only if *
For a search of the following and a record named the following:
The reserved characters are: + - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ /
=> Error: Unable to perform your search

With QueryAutoTruncate=auto
For a search of the following and a record named the following:
The reserved characters are: + - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ /
=> Error: Unable to perform your search
Comment 52 Peter Vashchuk 2021-07-13 11:24:48 UTC
Created attachment 122801 [details] [review]
Bug 28316: add tests

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 53 Peter Vashchuk 2021-07-13 11:24:53 UTC
Created attachment 122802 [details] [review]
Bug 28316: screen unquoted semicolons and all followup colons

Currently searches like: "book:", ":book" and "host-item:test:n"
cause internal server errors.

This patch adds additional regexes that remove the colons at the start
and end of the query, and another regex that screens all follow-up
colons that go after the first colon to avoid errors when searching for
"host-item:test:n".

To reproduce:
1) using ES, search for the book with title that contains
semicolon at the start or at the end of the line, separated with spaces,
this should cause internal server error.
2) try doing the same with something like "host-item:test:n", it should
result in error as well.
3) apply the patch.
4) repeat steps 1-2, ensure that it works now.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 54 Peter Vashchuk 2021-07-13 11:24:58 UTC
Created attachment 122803 [details] [review]
Bug 28316: escape brackets in the search query

This patch screens square and curly brackets which have no special
language meaning.

To reproduce:
1) using ES, search for the book with title that contains
square and/or curly brackets, like "book [second edition]", which will
result in error.
2) apply the patch.
3) search for that book again, ensure that it works now.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 55 Peter Vashchuk 2021-07-13 11:25:04 UTC
Created attachment 122804 [details] [review]
Bug 28316: escape exclamation signs in the query

Currently having exclamation sign at the end of the query makes ES
search fail, and when you try to search for a book that has exclamation
sign in the tittle (something like "Words! words") won't show results
correctly as it tries to negate everything that is after exclamation
sign, making it impossible to search for books that have in in the title

This patch escapes exclamation signs if it's at the end of the query or
has a space after it, resolving both of the issues listed above.

To reproduce:
1) with ES enabled, search for the book with title that contains
exclamation sight at the end, like "book!", this search should result
 in error.
2) do another search, but this time find/prepare beforehand book with a
title that has exclamation sign with a space after it,
e.g "exclamation! sign", it shouldn't find it as ES treats everything
after that exclamation sign as negation.
2) apply the patch.
3) perform searches from the steep one and two again.
Search from step one should no longer fail, while search from the step
two should find that book.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 56 Peter Vashchuk 2021-07-13 11:25:09 UTC
Created attachment 122805 [details] [review]
Bug 28316: avoid messing up regexes in the search queries

This patch ensures that the behavior with
QueryRegexEscapeOptions set to values other than
"Escape" still will works as expected.

It does so by storing the contents of regexes
before escaping special characters and
then restores the contents of regexes back to how
it was before, ensuring that searching with regex is possible.
Comment 57 Peter Vashchuk 2021-07-13 11:25:14 UTC
Created attachment 122806 [details] [review]
Bug 28316: escape ES ranges if QueryAutoTruncate is enabled

if QueryAutoTruncate enabled we will have any special operators ruined 
for example: "test [6 TO 7]" will be converted to "test* [6* TO* 7]"
so no reason to keep ranges when QueryAutoTruncate set to "enabled"

1) enable QueryAutoTruncate at your sysprefs.
2) perform a search using range, for example: "[1999 TO 2020]", 
it shouldn't work the way it's supposed to.
3) apply the patch.
4) perform the same search with range, ensure that it works correctly.
Comment 58 Peter Vashchuk 2021-07-13 13:52:39 UTC
(In reply to Victor Grousset/tuxayo from comment #51)
> With QueryAutoTruncate=only if *
> For a search of the following and a record named the following:
> The reserved characters are: + - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ /
> => Error: Unable to perform your search
> 
> With QueryAutoTruncate=auto
> For a search of the following and a record named the following:
> The reserved characters are: + - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ /
> => Error: Unable to perform your search

Hey Victor, thanks for the response,

I think search query like that should never work as it will always cause ES syntax error, ElasticSearch is a mini-language after all and has it's own syntax and it will die on something like that.
Though we do need to notify the user, in cases like that, that the search query is incorrect and that they need to correct it and search with it again. I will make a ticket for it a bit later if.
Comment 59 Peter Vashchuk 2021-07-14 07:54:33 UTC
Pushed the patches with fixed descriptions and removed some unnecessary newlines that were left there.
Comment 60 Peter Vashchuk 2021-08-03 12:55:35 UTC
Victor, please check this ticket:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28736

it will make sense why search queries like this one "+ - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ /" fail :)
Comment 61 Andrew Nugged 2021-08-05 10:03:52 UTC
(In reply to Victor Grousset/tuxayo from comment #51)

Victor, please can you again give-this-a-go?
i.e. Re-Sign-off this ticket again according to that so we now might have proper "error messages" for the wrong syntax in ES requests (bug 28736), if that comforts you?
Comment 62 Alex Buckley 2021-08-08 23:13:18 UTC
Created attachment 123601 [details] [review]
Bug 28316: add tests

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 63 Alex Buckley 2021-08-08 23:13:25 UTC
Created attachment 123602 [details] [review]
Bug 28316: screen unquoted semicolons and all followup colons

Currently searches like: "book:", ":book" and "host-item:test:n"
cause internal server errors.

This patch adds additional regexes that remove the colons at the start
and end of the query, and another regex that screens all follow-up
colons that go after the first colon to avoid errors when searching for
"host-item:test:n".

To reproduce:
1) using ES, search for the book with title that contains
semicolon at the start or at the end of the line, separated with spaces,
this should cause internal server error.
2) try doing the same with something like "host-item:test:n", it should
result in error as well.
3) apply the patch.
4) repeat steps 1-2, ensure that it works now.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 64 Alex Buckley 2021-08-08 23:13:30 UTC
Created attachment 123603 [details] [review]
Bug 28316: escape brackets in the search query

This patch screens square and curly brackets which have no special
language meaning.

To reproduce:
1) using ES, search for the book with title that contains
square and/or curly brackets, like "book [second edition]", which will
result in error.
2) apply the patch.
3) search for that book again, ensure that it works now.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 65 Alex Buckley 2021-08-08 23:13:35 UTC
Created attachment 123604 [details] [review]
Bug 28316: escape exclamation signs in the query

Currently having exclamation sign at the end of the query makes ES
search fail, and when you try to search for a book that has exclamation
sign in the tittle (something like "Words! words") won't show results
correctly as it tries to negate everything that is after exclamation
sign, making it impossible to search for books that have in in the title

This patch escapes exclamation signs if it's at the end of the query or
has a space after it, resolving both of the issues listed above.

To reproduce:
1) with ES enabled, search for the book with title that contains
exclamation sight at the end, like "book!", this search should result
 in error.
2) do another search, but this time find/prepare beforehand book with a
title that has exclamation sign with a space after it,
e.g "exclamation! sign", it shouldn't find it as ES treats everything
after that exclamation sign as negation.
2) apply the patch.
3) perform searches from the steep one and two again.
Search from step one should no longer fail, while search from the step
two should find that book.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 66 Alex Buckley 2021-08-08 23:13:40 UTC
Created attachment 123605 [details] [review]
Bug 28316: avoid messing up regexes in the search queries

This patch ensures that the behavior with
QueryRegexEscapeOptions set to values other than
"Escape" still will works as expected.

It does so by storing the contents of regexes
before escaping special characters and
then restores the contents of regexes back to how
it was before, ensuring that searching with regex is possible.

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 67 Alex Buckley 2021-08-08 23:13:46 UTC
Created attachment 123606 [details] [review]
Bug 28316: escape ES ranges if QueryAutoTruncate is enabled

if QueryAutoTruncate enabled we will have any special operators ruined
for example: "test [6 TO 7]" will be converted to "test* [6* TO* 7]"
so no reason to keep ranges when QueryAutoTruncate set to "enabled"

1) enable QueryAutoTruncate at your sysprefs.
2) perform a search using range, for example: "[1999 TO 2020]",
it shouldn't work the way it's supposed to.
3) apply the patch.
4) perform the same search with range, ensure that it works correctly.

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 68 Alex Buckley 2021-08-08 23:14:53 UTC
Followed patch test plans (and run unit test t/Koha/SearchEngine/Elasticsearch/QueryBuilder.t) which work as described in devbox running Elasticsearch. So have signed off.
Comment 69 Victor Grousset/tuxayo 2021-08-20 01:31:17 UTC
(In reply to Peter Vashchuk from comment #58)
> I think search query like that should never work as it will always cause ES
> syntax error, ElasticSearch is a mini-language after all and has it's own
> syntax and it will die on something like that.

Yes it makes sense.

> Though we do need to notify the user, in cases like that, that the search
> query is incorrect and that they need to correct it and search with it
> again. I will make a ticket for it a bit later if.

Thanks for bug 28736 :)
Comment 70 Victor Grousset/tuxayo 2021-08-20 04:13:07 UTC
(In reply to Andrew Nugged from comment #61)
> Victor, please can you again give-this-a-go?
> i.e. Re-Sign-off this ticket again according to that so we now might have
> proper "error messages" for the wrong syntax in ES requests (bug 28736), if
> that comforts you?

test result: my signoffs still hold on top of bug 28736 :)

Last two patches still not tested.
- I don't know how QueryRegexEscapeOptions works
- comment 41 about the last patch

But thanks to Alex we have these covered.
Comment 71 Andrew Nugged 2021-08-21 18:31:05 UTC
Generally about these complications and some mess with ES search:

we don't have at all ES query language parser and transmitting requests directly to ES and getting its "parse_exception" reactions, so in Perl can't explain in which part of the syntax the issue,

if Operator wants to have a direct search result, then he/she must escape special symbols for not to be "commands" for ES or use quoted texts to search exact terms.

this is a "fuzzy" search though so sometimes wrong queries might lead to not having results even if there are such characters in the book name, a tradeoff for full-text search.

We have only basic analysis which by the way was lost and not worked and fixed in Bug 28736,

at least now the user will know that it's not "nothing found", but there are problems in special ES query syntax. How to use that syntax the user should read from ES language docs (or it we have some intro in Koha Doc for that?),

but if the operator knows that there is ES syntax used and there are special characters and should use those properly: escape or quote.

In other cases, it becomes a "weird" mix of words and special characters which leads to "unpredictable behavior" if you don't know how to use ES syntax.

And also error message was mangled before, but now in Bug 28736 user knows that it's not "book not found", but the syntax and special language for ES used wrongly, so "please escape or quote".

But this my answer actually to forward that 28736 brought some light, as first part covering "most things", which we not covering in this current ticket.
Comment 72 Andrew Nugged 2021-08-21 18:57:43 UTC
Now regarding the current ticket - why it's appeared:


There already was some "prevention from dies (exceptions) in ES language" to make requests "just regular characters" instead of special commands,

We already had some FUZY pre-processing before this current bugfix code in Koha/SearchEngine/Elasticsearch/QueryBuilder.pm, "sub _clean_search_term":
some extra "precleaning", 

for example, curly braces "{}" any of them were replaced by '"' doublequote,
and "=" replaced with ":",
and colons with space before or space after (": " or " :") were just removed so only the space left,

but trailing colon, trailing exclamation mark, and square range braces which actually not range (like in "[OHO]" phrase: ES dies because it's not [X TO Y] format) misled ES and anyway made ES die sometimes,

So, that was incomplete and this ticket adds some more of such fuzziness but in the proper way (for example, ranges like "{... TO ...]" (i.e. full mix of ],},[,{ now works), 

But please don't demand from it to be "fully covering ES language" thing, 

in my opinion, this ticket decreases the number of "crashes" and "syntax errors" but at the same time, fixes range searches and started to recognize even "ES regular expressions" if they are enabled (yes, "QueryRegexEscapeOptions" case!), and fixes behavior with "QueryAutoTruncate" enabled, let me provide some examples below.
Comment 73 Andrew Nugged 2021-08-21 20:20:17 UTC
First, if you create" crazy request like (exactly this to the search field):

    The reserved characters are: + - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ /

it's impossible to do such fuzziness to guess what's wrong in such message
in ES query, so we will just tell the user this:

  Error: There might be results, but there is ElasticSearch query language
  syntax error in your search request: remove or escape with backslash
  ES-language reserved characters (those are: + - = && || > < ! ( ) { }
  [ ] ^ " ~ * ? : / ) or change it to be the proper ES-language
  request and try again."

(as from Bug 28736)

But in the current ticket, we added some more clean fuzziness (let me describe
difference between "current master" and what this current bugfix brings
further
Comment 74 Andrew Nugged 2021-08-21 21:08:41 UTC
QueryAutoTruncate case:
=========================

This is a feature that BLINDLY adds to alphanumeric-ended words asterisk
in the end before tossing that phrase to ES as a search query.

So, if QueryAutoTruncate is enabled (default), the Perl part just adds
to the end of any alphanumeric-ended word asterisk, disregards ES syntax
(ranges too), so if we will put such request phrase, the current master branch
(Perl part) will transfer the below examples to ES as a query string (here I
provide "what we entered" in the search field as 'IN:' and what transferred
to ES as 'OUT:' pairs):


(before the patch, phrase 1)

    IN:    The [Book]
    OUT:   The* [Book]

(will make ES fail with a yellow error text box)
in fact, ES doesn't understand your "special language usage"
[Book] confuses ES because not in [... TO ...] form,


and with ranges:

(before the patch, phrase 2)

    IN:    The "Book" {2000 TO 2002} [1900 TO 1990]
    OUT:   The* "Book" "2000 TO 2002" [1900* TO* 1990]

(will make ES fail with a yellow error text box)
[1900* TO* 1990] confuses ES because not in [... TO ...] form,

Here we see that it converted '{' and '}' to '"', and added '*' because of
"QueryAutoTruncate enabled" to all alphanumeric-ended words.
Second even more weird example:


(before the patch, phrase 3)

    IN:    The "Book" [2000 TO 2002}
    OUT:   The* Book* [2000* TO* 2002*

(will make ES fail with a yellow error text box)
[2000* confuses ES because [ has no pair and no range there,
And here quotes get lost because one "}" brace was converted to doublequote,
but because it was only one, so was "unpaired", then the logic of the current
code is to remove quotes at all if they are unbalanced (I agree, but it
shouldn't convert braces to quotes).

All requests above lead to a "yellow" Error text box, which means that ES
fired an exception because of the error in query language syntax (and in Koha server error log there will be the error message ES 'Failed to parse query').


Anyway, as I see, this "QueryAutoTruncate" mode is designed for very simple
usage behavior, i.e. user expected to do the requests without using any special
language at the most, and give as much as possible results to the user assuming that there was "just beginnings of the words" requested if one enters:
    so bi
this will be translated by Perl code to
    so* bi*
and passed to ES so ES will match with for examples books named "Something Big"
and "Solved Binary Book"

This QueryAutoTruncate as well ruins the syntax of ES range requests
(that '[1900 TO 1990]' becomes '[1900* TO* 1990]' in ES-language)
so no reason to keep the ranges, and that accounted in the patch:




Now with the patch:

This patch is not intended to make the "full fix", it's not so easy for this,
but at least the patch makes braces in QueryAutoTruncate mode just regular
symbols by pre-slashing them. Same as above but WITH this patch will make such
internal transformations, let's see by example:

(after patch, phrase 1)

    IN:    The "Book" {2000 TO 2002} [1900 TO 1990]
    OUT:   The* \[Book\]

ES understands that here we search 'the* book' and actually even will find
6 results in the current dev-test database, instead of "failing to the error"
like in the example above without the patch,


(after patch, phrase 2)

    IN:    The "Book" {2000 TO 2002} [1900 TO 1990]
    OUT:   The* "Book" \{2000* TO* 2002\} \[1900* TO* 1990\]

ES understands that here we search 'The* "Book" 2000* TO* 2002 1900* TO* 1990'
but it's a pretty rare phrase to find something, so - zero results,
but no "ES exception",

(after patch, phrase 3)

    IN:    The "Book" [2000 TO 2002}
    OUT:   The* "Book" \[2000* TO* 2002\}

Same here as above, zero results but no "exceptions".


I.e. those braces now become not-special-language characters and just passed by
to ES to decide. In real life, those braces ignored by ES as non-alphanumeric,
so querying:
    some [word]
with QueryAutoTruncate enabled after the patch will be the same as search:
    some* word

and BEFORE the patch it was just the "yellow" "error happened" text box result.
Comment 75 Andrew Nugged 2021-08-21 22:20:40 UTC
QueryRegexEscapeOptions
=========================

Now let's turn "QueryAutoTruncate" off and check with QueryRegexEscapeOptions:

It's all about RegEx in ES queries,
https://www.elastic.co/guide/en/elasticsearch/reference/6.8/query-dsl-regexp-query.html

Koha admin settings QueryRegexEscapeOptions has three options:



- escape,

  i.e. if the request will have any regular expressions between slashes:
      /book/
  this will be passed in "disabled, not RegEx anymore" to ES:
      \/book\/
  so ES will get this just as regular text to search, and in this example
  will just search for the word 'book', ignoring other non-alphanum chars.

  (*)NOTE: but this means that passing RegEx to ES is impossible, at all.
           and this means any book name like in the search field will work:
               We are / They are
           so if we copy-paste this to the search field, ES gets:
               We are \/ They are
           and Koha just searches it as it is.

  I assume this was added as a feature to disable "/" meaning as a special RegEx
  limiter and allow librarians to search "We are / They are" text directly.


- don't escape, 

  so:
      /go+d/
  will be passed as it is to ES, so this will be special "Regular Expressions"
  in this example, this means "'g' then 'o' one or more times then 'd'"
  so search in ES for /go+d/ example is similar to search:
      god OR good
  ( yet not the same because RegExp 'go+d' is not limited by a few 'o's,
    so might match for 'gooooood' too)

  BTW it founds 7 books for me in both ways (/go+d/ and god OR good) in
  the current master dev-test DB.

  (*)NOTE: but this means that now any '/' which appears in the text will
           cause ES to throw an exception if the syntax is not understood,
           i.e. search for exactly this:
               We are / They are
           now won't work so the users need to do escaping themself and ask:
               We are \/ They are
           (by backslashing '/' in the request field) to make ES not "die"
           and search to work.

   That's why I assume to make 1+2 both working, 3rd one was introduced:



- unescape escaped,

  yeah, that's "The Trick" to make 1st way "We are / They are" requests to work
  and ES not to fail, but at the same time allowing to bring Regular Expressions 
  by users, just in "pre-slashed form", i.e. what pre-slashed WILL BE RegExs, 
  what isn't -- won't be but won't "throw a syntax error" too, so:
        We are / They are
  will be tossed to ES as "We are \/ They are" and will search
  for "We are They are", bit this (NOW THE TRICK! BEHOooooLD!):
      We are / They are \/go+\/
  will be transferred to ES in such form:
      We are \/ They are /go+/
  and ES will search for 'we', 'they', 'are' words, all to be present, plus
  one of the any: go, goo, gooo, goooo.. words
  ( this founds, btw, 
      "Seesaw [sound recording] / Beth Hart, Joe Bonamassa"
    in current test DB :-), hey! )




This is how it is expected to work. But the reality in current master a little
worse,

Let's have "unescape escaped", let's invent such request:
    \/go{1,2}d\/

which EXPECTED by us to be transferred to ES as RegEx:
    /go{1,2}d/

what means we want ES to find for us: 'g', then one or max two 'o', and 'd'
i.e. this is exactly ONLY for finding "good" or "god".
But current master branch code breaks these expectations: replaces "{" and "}"
with double quotes), so for real this transferred to ES as the request:
    /go"1,2"d/
and obviously, nothing was found. 

But with this patch, the request:
    \/go{1,2}d\/

will be passed to ES as:
    /go{1,2}d/

and voila, 
  "7 result(s) found for '\/go{1,2}d\/'." in our test DB.
Huh!



More, because this patch respects {,} in regex but escapes those outside,
it will pass those escaped, so if you will search:
    \/go{1,2}d\/ [go]

this will pass to ES request WITHOUT patch:
    /go"1,2"d/ [go]

and bring yellow "Error" text box,
and this will be passed to ES request WITH patch:
    /go{1,2}d/ \[go\]

and because "\[" and "\]" ignored by ES, it searches just for "good" or "god"
plus the word "go", so the final searched text should contain "good to go"
or "go god" combinations (or others like those).

And yes, on test DB it founds 3 results! With patch.
And gives "error/nothing" without the patch.
Comment 76 Andrew Nugged 2021-08-21 22:40:39 UTC
Ranges in ES language
=========================

let's disable QueryAutoTruncate for not adding those asterisks,
and QueryRegexEscapeOptions doesn't matter here unless we will add
regular expressions. So:

ES allows us to have ranges requests, like:

    novel {1947 TO 1950]

this expects to find word "novel" and one of the words in the range between
"1948 to 1950", and 1948 said here because '{' means "not included in range" value, but starting from it (i.e. not '>=', but '>'),

but code in current master replaces '{' with '"', and then even more, unbalanced 
(unpaired) '"' removed as well, so the final request to ES will be:

    novel  1947 TO 1950]

which has 'hanging ]' that ES doesn't understands and throws an exception
(welcome to the 'yellow Error box' instead of 'found' result here again).



Now, with the patch:

    novel {1947 TO 1950]

transferred correctly as it is, and ES on test DB finds single:
    The vixens. A novel.
    Publication details: London, 1948.
for obvious reasons.


Now to play another example "without and with patch", here:

    [novel] [1900 TO 1950]

without the patch, it transferred "as is" to ES, and gives a yellow error,
because ES doesn't understands range as in '[novel]'.

With patch it becomes transformed to such request to ES:

    \[novel\] [1900 TO 1950]

and because backslashed non-alphanumeric characters are ignored, it
works and brings the result - one book, like in the previous example.
Comment 77 Andrew Nugged 2021-08-21 23:57:20 UTC
Tricks with colons:
======================

colons in Koha expected to separate search field and search value,
so "title:good" should search word "good" only in "titles",

but if you will use "some : like this" form, when copypasting from usual combined
book names we often have, current code removed colons at all from the text, if
colon has the space at any side, but there was forgotten trailing ":" removal,
i.e. ES failed without patch if to search:
    Books:

also, some fuzziness added, for example, if you rename any book to have "23:59"
in the name, then will search with an old code for:

    title:23:59

it will die ("yellow error box"),
but with the patch, it will find, as this transformed by the patch to:

    title:23\:59

i.e. there every next semicolon after first in the term preslashed.
Comment 78 Andrew Nugged 2021-08-22 00:01:38 UTC
And the exclamation mark:
======================

this used to be the "negate" command for words in queries, ONLY when '!' sticking
to the beginning of the word, so patch fuzziness added: non-sticky and trailing
exclamations marks removed, so in the case of the search:

    Dalek!

without the patch, this brought a yellow error box, but with the patch dangling
"!" was removed and it just passes "Dalek" to ES.
It finds two books in our test DB, btw.

And this one:

    Dalek !War

works normally with both, with and without the patch. It searches for "Dalek"
but negates "war" word presence. In our test dev-DB by the way, it gives just one book as a result.
Comment 79 Andrew Nugged 2021-08-22 00:15:58 UTC
(In reply to Victor Grousset/tuxayo from comment #41)
> > 4) additionally perform a search using range,

Vicor, this seems working now,

By the way, it should find normally this with and without patches for you:

    date-of-publication:[1983 TO 1985]

But before the patch, if you will use "not including border values" ranges:

    date-of-publication:{1983 TO 1985}

and then because not-patched code converts it to this:

    date-of-publication:"1983 TO 1985"

it even did not fail to "yellow error box", just said "no results", but
there is: with patch now it finds 4 books for me on test DB for {1983 TO 1985}.


Besides you for sure can find when ES crashes, :hugs:
but without the patch, you will have much more possibilities to get that ES
syntax error now.

And I covered come in/outs and experiments above
Comment 80 Andrew Nugged 2021-08-22 00:16:19 UTC
So, the patch adds more order with "QueryRegexEscapeOptions" than before,
same with "QueryAutoTruncate", and keeps ranges working as well.

Yet it loooo+ng read above :), for a strong coffee and murky cat nearby ^_^
Comment 81 Andrew Nugged 2021-08-22 00:27:03 UTC
Why this fuzziness and the extra job was done by us? We wanted to limit syntax errors from ES and make it easier for librarians to copy-paste just book names and have results, 

but NOT TO FORCE THEM backslash or re-edit text "for as long as possible",

so for example earlier, they pasted "normal human phrases",
like each of these:

    The Book!

    The Book [directed by robert b weide]

    The Book:

and that lead earlier for ES to crash and bring "yellow Error box", 
even worse: before Bug 28736 people were confused that "oh, nothing found!" but for real there not even searched because of syntax error.

Now, these above examples working and finding results
(except the middle one because no such book in test DB yet :) ).
Comment 82 Martin Renvoize 2021-09-01 15:00:52 UTC
Created attachment 124372 [details] [review]
Bug 28316: add tests

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 83 Martin Renvoize 2021-09-01 15:00:57 UTC
Created attachment 124373 [details] [review]
Bug 28316: screen unquoted semicolons and all followup colons

Currently searches like: "book:", ":book" and "host-item:test:n"
cause internal server errors.

This patch adds additional regexes that remove the colons at the start
and end of the query, and another regex that screens all follow-up
colons that go after the first colon to avoid errors when searching for
"host-item:test:n".

To reproduce:
1) using ES, search for the book with title that contains
semicolon at the start or at the end of the line, separated with spaces,
this should cause internal server error.
2) try doing the same with something like "host-item:test:n", it should
result in error as well.
3) apply the patch.
4) repeat steps 1-2, ensure that it works now.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 84 Martin Renvoize 2021-09-01 15:01:03 UTC
Created attachment 124374 [details] [review]
Bug 28316: escape brackets in the search query

This patch screens square and curly brackets which have no special
language meaning.

To reproduce:
1) using ES, search for the book with title that contains
square and/or curly brackets, like "book [second edition]", which will
result in error.
2) apply the patch.
3) search for that book again, ensure that it works now.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 85 Martin Renvoize 2021-09-01 15:01:09 UTC
Created attachment 124375 [details] [review]
Bug 28316: escape exclamation signs in the query

Currently having exclamation sign at the end of the query makes ES
search fail, and when you try to search for a book that has exclamation
sign in the tittle (something like "Words! words") won't show results
correctly as it tries to negate everything that is after exclamation
sign, making it impossible to search for books that have in in the title

This patch escapes exclamation signs if it's at the end of the query or
has a space after it, resolving both of the issues listed above.

To reproduce:
1) with ES enabled, search for the book with title that contains
exclamation sight at the end, like "book!", this search should result
 in error.
2) do another search, but this time find/prepare beforehand book with a
title that has exclamation sign with a space after it,
e.g "exclamation! sign", it shouldn't find it as ES treats everything
after that exclamation sign as negation.
2) apply the patch.
3) perform searches from the steep one and two again.
Search from step one should no longer fail, while search from the step
two should find that book.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 86 Martin Renvoize 2021-09-01 15:01:14 UTC
Created attachment 124376 [details] [review]
Bug 28316: avoid messing up regexes in the search queries

This patch ensures that the behavior with
QueryRegexEscapeOptions set to values other than
"Escape" still will works as expected.

It does so by storing the contents of regexes
before escaping special characters and
then restores the contents of regexes back to how
it was before, ensuring that searching with regex is possible.

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 87 Martin Renvoize 2021-09-01 15:01:20 UTC
Created attachment 124377 [details] [review]
Bug 28316: escape ES ranges if QueryAutoTruncate is enabled

if QueryAutoTruncate enabled we will have any special operators ruined
for example: "test [6 TO 7]" will be converted to "test* [6* TO* 7]"
so no reason to keep ranges when QueryAutoTruncate set to "enabled"

1) enable QueryAutoTruncate at your sysprefs.
2) perform a search using range, for example: "[1999 TO 2020]",
it shouldn't work the way it's supposed to.
3) apply the patch.
4) perform the same search with range, ensure that it works correctly.

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 88 Martin Renvoize 2021-09-01 15:01:25 UTC
Created attachment 124378 [details] [review]
Bug 28316: (QA follow-up) Make clean_search_term public

With all the work that's gone into improving the internal
_clean_search_term method I feel we should expose it publically as it's
going to be more widely helpful

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 89 Martin Renvoize 2021-09-01 15:02:51 UTC
Wow, this is a pretty rigorous treatment.. I'm impressed.

I've tested to the extent of my knowledge and am really happy with the results. No regressions found, but I'd love to see it pushed and tested with various configurations in the wild with quick fixes to follow.

Passing QA.
Comment 90 Martin Renvoize 2021-09-01 15:07:09 UTC
*** Bug 28326 has been marked as a duplicate of this bug. ***
Comment 91 Jonathan Druart 2021-09-02 07:14:06 UTC
+        $term =~ s/(?<!\\)((?:[\\]{2})*)(\{|\[)([^\s\[\]\{\}]+ TO [^\s\[\]\{\}]+(?<!\\)(?:[\\]{2})*)(\}|\])/$1.'~~L'.($2 eq '[' ? 'S':'C').'~~'.$3.'~~R'.($4 eq ']' ? 'S':'C').'~~'/ge;



+            $term =~ s@(
+                (?<!\\)(?:[\\]{2})*/
+                (?:[^/]+|(?<=\\)(?:[\\]{2})*/)+
+                (?<!\\)(?:[\\]{2})*/
+            )$lookahead@"~~XI$rgx_i~~"@ex


I can push this kind of stuffs if everybody agrees, but I must warn that we are opening a sensible door. If we find regressions that will be a mess to maintain, or even to enhance.

Questions and suggestions:
1. Could we use named capture in the regexs?
2. What means XI?
3. Why do we need the 'e' modifier in the second one?
4. Can we split on several lines the regexs to make them more readable? (/xms)
Comment 92 Andrew Nugged 2021-09-02 07:57:06 UTC
1: yes, naming can be added

2: this is a "placeholder" which:
    a) first: part of text removed and replaced by placeholders (~~XI1~~ / ~~LC1~~ for example and others, life "LC for LeftCurly" and so on). This is for temporary hiding out current brackets from "next RegExs"
    b) then text processed expecting that all others curly/square brackets and regex and what must be kept – removed and hidden, so we can clean up and be brave with all other not-replaced-by-placeholders brackets,
    c) so we cleaning up text, screening, removing or whatever those special characters,
    d) and now we replacing BACK those placeholders with their saved in step a) values back to keep unscreened and working special characters we wanted to be kept

3: because it has " .. ? .. : .." depending of what bracket was found, but otherwise if you speaking security-wise and don't want to have "e"'s at all this RE probably can be split to few "just with different brackets" each,
should Petro split this regex to two without "e"? We discussed this too, I also pointed that this is less secure "if something changes", but in this case, I don't see how this can be hacked (how?)

4: for sure, this is good point. And some more comments in code. Let's do this.
Comment 93 Andrew Nugged 2021-09-02 08:03:11 UTC
> This is for temporary hiding out current brackets from "next RegExs"

I agree that this is not the best solution: with 0.00..1% probability that some books can have in the name "~~LC1~~" and this then will make code work unexpectedly for that book ... but then we need to have ES to allow to index "~~..~~" things, ... :).

I also pointed this to Petro but we thought together that then the best solution is to scan text for placeholders and change placeholder (i.e. "generate placeholder" to be unique) but oh – that then will make code so complicated... we will create our own MIME then :)... 

If think from perfectionism POV this not solved yet, but other way – ... )... eh?
Comment 94 Jonathan Druart 2021-09-02 08:21:28 UTC
(In reply to Andrew Nugged from comment #92)
> 1: yes, naming can be added
> 
> 2: this is a "placeholder" which:

Yes, I got that, and L for Left and R for Right. But then XI for... what? :)

> 3: because it has " .. ? .. : .." depending of what bracket was found

No, the second regex does not have it.

> , but
> otherwise if you speaking security-wise and don't want to have "e"'s at all
> this RE probably can be split to few "just with different brackets" each,
> should Petro split this regex to two without "e"? We discussed this too, I
> also pointed that this is less secure "if something changes", but in this
> case, I don't see how this can be hacked (how?)

Looks ok for now. But if it can be avoided I should do it.

> 4: for sure, this is good point. And some more comments in code. Let's do
> this.

Thx!
Comment 95 Peter Vashchuk 2021-09-06 13:51:50 UTC
Created attachment 124560 [details] [review]
Bug 28316: add tests

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 96 Peter Vashchuk 2021-09-06 13:51:57 UTC
Created attachment 124561 [details] [review]
Bug 28316: screen unquoted semicolons and all followup colons

Currently searches like: "book:", ":book" and "host-item:test:n"
cause internal server errors.

This patch adds additional regexes that remove the colons at the start
and end of the query, and another regex that screens all follow-up
colons that go after the first colon to avoid errors when searching for
"host-item:test:n".

To reproduce:
1) using ES, search for the book with title that contains
semicolon at the start or at the end of the line, separated with spaces,
this should cause internal server error.
2) try doing the same with something like "host-item:test:n", it should
result in error as well.
3) apply the patch.
4) repeat steps 1-2, ensure that it works now.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 97 Peter Vashchuk 2021-09-06 13:52:04 UTC
Created attachment 124562 [details] [review]
Bug 28316: escape brackets in the search query

This patch screens square and curly brackets which have no special
language meaning.

To reproduce:
1) using ES, search for the book with title that contains
square and/or curly brackets, like "book [second edition]", which will
result in error.
2) apply the patch.
3) search for that book again, ensure that it works now.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 98 Peter Vashchuk 2021-09-06 13:52:10 UTC
Created attachment 124563 [details] [review]
Bug 28316: escape exclamation signs in the query

Currently having exclamation sign at the end of the query makes ES
search fail, and when you try to search for a book that has exclamation
sign in the tittle (something like "Words! words") won't show results
correctly as it tries to negate everything that is after exclamation
sign, making it impossible to search for books that have in in the title

This patch escapes exclamation signs if it's at the end of the query or
has a space after it, resolving both of the issues listed above.

To reproduce:
1) with ES enabled, search for the book with title that contains
exclamation sight at the end, like "book!", this search should result
 in error.
2) do another search, but this time find/prepare beforehand book with a
title that has exclamation sign with a space after it,
e.g "exclamation! sign", it shouldn't find it as ES treats everything
after that exclamation sign as negation.
2) apply the patch.
3) perform searches from the steep one and two again.
Search from step one should no longer fail, while search from the step
two should find that book.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 99 Peter Vashchuk 2021-09-06 13:52:16 UTC
Created attachment 124564 [details] [review]
Bug 28316: avoid messing up regexes in the search queries

This patch ensures that the behavior with
QueryRegexEscapeOptions set to values other than
"Escape" still will works as expected.

It does so by storing the contents of regexes
before escaping special characters and
then restores the contents of regexes back to how
it was before, ensuring that searching with regex is possible.

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 100 Peter Vashchuk 2021-09-06 13:52:22 UTC
Created attachment 124565 [details] [review]
Bug 28316: escape ES ranges if QueryAutoTruncate is enabled

if QueryAutoTruncate enabled we will have any special operators ruined
for example: "test [6 TO 7]" will be converted to "test* [6* TO* 7]"
so no reason to keep ranges when QueryAutoTruncate set to "enabled"

1) enable QueryAutoTruncate at your sysprefs.
2) perform a search using range, for example: "[1999 TO 2020]",
it shouldn't work the way it's supposed to.
3) apply the patch.
4) perform the same search with range, ensure that it works correctly.

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 101 Peter Vashchuk 2021-09-06 13:52:29 UTC
Created attachment 124566 [details] [review]
Bug 28316: (QA follow-up) Make clean_search_term public

With all the work that's gone into improving the internal
_clean_search_term method I feel we should expose it publically as it's
going to be more widely helpful

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 102 Peter Vashchuk 2021-09-06 14:04:39 UTC
(In reply to Jonathan Druart from comment #91)
> Questions and suggestions:
> 1. Could we use named capture in the regexs?
> 2. What means XI?
> 3. Why do we need the 'e' modifier in the second one?
> 4. Can we split on several lines the regexs to make them more readable?
> (/xms)

I implemented those changes and pushed the changed version of the patches, please take a look :)
Comment 103 Martin Renvoize 2021-09-09 12:58:19 UTC
For future reference, once a bug has gone through SO and QA please respond to change requests as follow-ups... that way it's easy for a reviewer to see that the requested change has been made and nothing else beyond it.. with the changed squashed in it's much more difficult.. especially if one has cleaned up their previous branches.
Comment 104 Martin Renvoize 2021-09-09 13:02:53 UTC
Created attachment 124709 [details] [review]
Bug 28316: add tests

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 105 Martin Renvoize 2021-09-09 13:02:59 UTC
Created attachment 124710 [details] [review]
Bug 28316: screen unquoted semicolons and all followup colons

Currently searches like: "book:", ":book" and "host-item:test:n"
cause internal server errors.

This patch adds additional regexes that remove the colons at the start
and end of the query, and another regex that screens all follow-up
colons that go after the first colon to avoid errors when searching for
"host-item:test:n".

To reproduce:
1) using ES, search for the book with title that contains
semicolon at the start or at the end of the line, separated with spaces,
this should cause internal server error.
2) try doing the same with something like "host-item:test:n", it should
result in error as well.
3) apply the patch.
4) repeat steps 1-2, ensure that it works now.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 106 Martin Renvoize 2021-09-09 13:03:05 UTC
Created attachment 124711 [details] [review]
Bug 28316: escape brackets in the search query

This patch screens square and curly brackets which have no special
language meaning.

To reproduce:
1) using ES, search for the book with title that contains
square and/or curly brackets, like "book [second edition]", which will
result in error.
2) apply the patch.
3) search for that book again, ensure that it works now.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 107 Martin Renvoize 2021-09-09 13:03:11 UTC
Created attachment 124712 [details] [review]
Bug 28316: escape exclamation signs in the query

Currently having exclamation sign at the end of the query makes ES
search fail, and when you try to search for a book that has exclamation
sign in the tittle (something like "Words! words") won't show results
correctly as it tries to negate everything that is after exclamation
sign, making it impossible to search for books that have in in the title

This patch escapes exclamation signs if it's at the end of the query or
has a space after it, resolving both of the issues listed above.

To reproduce:
1) with ES enabled, search for the book with title that contains
exclamation sight at the end, like "book!", this search should result
 in error.
2) do another search, but this time find/prepare beforehand book with a
title that has exclamation sign with a space after it,
e.g "exclamation! sign", it shouldn't find it as ES treats everything
after that exclamation sign as negation.
2) apply the patch.
3) perform searches from the steep one and two again.
Search from step one should no longer fail, while search from the step
two should find that book.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 108 Martin Renvoize 2021-09-09 13:03:17 UTC
Created attachment 124713 [details] [review]
Bug 28316: avoid messing up regexes in the search queries

This patch ensures that the behavior with
QueryRegexEscapeOptions set to values other than
"Escape" still will works as expected.

It does so by storing the contents of regexes
before escaping special characters and
then restores the contents of regexes back to how
it was before, ensuring that searching with regex is possible.

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 109 Martin Renvoize 2021-09-09 13:03:22 UTC
Created attachment 124714 [details] [review]
Bug 28316: escape ES ranges if QueryAutoTruncate is enabled

if QueryAutoTruncate enabled we will have any special operators ruined
for example: "test [6 TO 7]" will be converted to "test* [6* TO* 7]"
so no reason to keep ranges when QueryAutoTruncate set to "enabled"

1) enable QueryAutoTruncate at your sysprefs.
2) perform a search using range, for example: "[1999 TO 2020]",
it shouldn't work the way it's supposed to.
3) apply the patch.
4) perform the same search with range, ensure that it works correctly.

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 110 Martin Renvoize 2021-09-09 13:03:29 UTC
Created attachment 124715 [details] [review]
Bug 28316: (QA follow-up) Make clean_search_term public

With all the work that's gone into improving the internal
_clean_search_term method I feel we should expose it publically as it's
going to be more widely helpful

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 111 Martin Renvoize 2021-09-09 13:04:32 UTC
Phew.. I had an old branch hanging around I could diff with.

Passing QA again, looks like everything has been taken care of.
Comment 112 Andrew Nugged 2021-09-09 14:18:12 UTC
:huggs:

yeah, right, Petro, look, we should next time add follow-up (thanks Martin!)
Comment 113 Martin Renvoize 2021-09-09 15:22:44 UTC
(In reply to Andrew Nugged from comment #112)
> :huggs:
> 
> yeah, right, Petro, look, we should next time add follow-up (thanks Martin!)

No worries; if there's a mistake to be made like this I've probably made it at least a few times already myself ;)
Comment 114 Jonathan Druart 2021-09-20 12:47:22 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 115 Kyle M Hall 2021-10-01 18:16:09 UTC
Pushed to 21.05.x for 21.05.05
Comment 116 Martin Renvoize 2021-10-26 09:31:13 UTC
Can we get a backport for this for 20.11 pretty please?
Comment 117 Fridolin Somers 2021-10-27 07:09:46 UTC
Yep, I was waiting for Bug 29284.
Will be in 20.11.12 release.