Bug 22715

Summary: Searching for patrons with "" in the circulation note hangs patron search
Product: Koha Reporter: Liz Rea <wizzyrea>
Component: PatronsAssignee: Bugs List <koha-bugs>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: critical    
Priority: P5 - low CC: gmcharlt, jonathan.druart, katrin.fischer, kyle.m.hall, kyle, lucas, martin.renvoize, nick, wizzyrea
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21949
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20891
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22316
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.05
Bug Depends on: 20891    
Bug Blocks: 22781    
Attachments: Bug 22715: Searching for patrons with "" in the circulation note hangs patron search
Bug 22715: Searching for patrons with "" in the circulation note hangs patron search
Bug 22715: Searching for patrons with "" in the circulation note hangs patron search
Bug 22715: Searching for patrons with "" in the circulation note hangs patron search
Bug 22715: Searching for patrons with "" in the circulation note hangs patron search
Bug 22715: Searching for patrons with "" in the circulation note hangs patron search
Bug 22715: Remove extraneous html filter
Bug 22715: Searching for patrons with "" in the circulation note hangs patron search
Bug 22715: Remove extraneous html filter

Description Liz Rea 2019-04-16 14:27:16 UTC
To replicate:
Edit a patron to have a circ note, include quoted text in it, such as "this is quoted string"

Go to the patron search, search for a set of results that would include the patron with the quotes in the note

Enjoy looking at "Processing" for forever.
Comment 1 Kyle M Hall 2019-04-16 14:36:24 UTC
Created attachment 88074 [details] [review]
Bug 22715: Searching for patrons with "" in the circulation note hangs patron search

To replicate:
Edit a patron to have a circ note, include quoted text in it, such as "this is quoted string"
Go to the patron search, search for a set of results that would include the patron with the quotes in the note
Enjoy looking at "Processing" for forever.

Test Plan:
1) Apply this patch
2) Attempt to replicate using the steps above
Comment 2 Liz Rea 2019-04-16 14:39:41 UTC
Created attachment 88076 [details] [review]
Bug 22715: Searching for patrons with "" in the circulation note hangs patron search

To replicate:
Edit a patron to have a circ note, include quoted text in it, such as "this is quoted string"
Go to the patron search, search for a set of results that would include the patron with the quotes in the note
Enjoy looking at "Processing" for forever.

Test Plan:
1) Apply this patch
2) Attempt to replicate using the steps above
Comment 3 Liz Rea 2019-04-16 14:40:07 UTC
Created attachment 88077 [details] [review]
Bug 22715: Searching for patrons with "" in the circulation note hangs patron search

To replicate:
Edit a patron to have a circ note, include quoted text in it, such as "this is quoted string"
Go to the patron search, search for a set of results that would include the patron with the quotes in the note
Enjoy looking at "Processing" for forever.

Test Plan:
1) Apply this patch
2) Attempt to replicate using the steps above

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 4 Katrin Fischer 2019-04-16 20:52:26 UTC
Created attachment 88116 [details] [review]
Bug 22715: Searching for patrons with "" in the circulation note hangs patron search

To replicate:
Edit a patron to have a circ note, include quoted text in it, such as "this is quoted string"
Go to the patron search, search for a set of results that would include the patron with the quotes in the note
Enjoy looking at "Processing" for forever.

Test Plan:
1) Apply this patch
2) Attempt to replicate using the steps above

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 5 Katrin Fischer 2019-04-16 20:53:13 UTC
This works as described, my only concern would be that we should handle these kinds of characters in a more global approach - this only fixes one field.
Comment 6 Nick Clemens 2019-04-17 10:06:49 UTC
I think we need to look a little more here.

A note like:
"Don't checkout to me!"

will still break the results page
Comment 7 Kyle M Hall 2019-04-17 10:53:20 UTC
(In reply to Katrin Fischer from comment #5)
> This works as described, my only concern would be that we should handle
> these kinds of characters in a more global approach - this only fixes one
> field.

This entire script needs to be re-engineered to output json using the JSON module at the end of the script instead of using a template to generate json. This will solve all these encoding problems.
Comment 8 Katrin Fischer 2019-04-17 11:29:28 UTC
(In reply to Kyle M Hall from comment #7)
> (In reply to Katrin Fischer from comment #5)
> > This works as described, my only concern would be that we should handle
> > these kinds of characters in a more global approach - this only fixes one
> > field.
> 
> This entire script needs to be re-engineered to output json using the JSON
> module at the end of the script instead of using a template to generate
> json. This will solve all these encoding problems.

Makes sense. My thinking here was that as we have another patch right now making HTML available for this field it might be worth catching some of the possible problems that would break search immediately. 

We also have similar problems in item search - we have a publisher with a backslash that breaks the item search regularly. We might want to take a closer look at the places where we work with JSON.
Comment 9 Liz Rea 2019-04-17 14:31:03 UTC
I think we need to get this in, because this case will happen quite often and a partial fix now is better than months later complete fix. I've been seeing this crop up ever since the patron search results were converted to JSON, which means this bug has technically been active for at least 5 releases now.
Comment 10 Jonathan Druart 2019-04-24 03:00:11 UTC
See bug 20891 for a similar issue, I think it is the correct fix.
Comment 11 Kyle M Hall 2019-04-25 11:22:27 UTC
Created attachment 88749 [details] [review]
Bug 22715: Searching for patrons with "" in the circulation note hangs patron search

To replicate:
Edit a patron to have a circ note, include quoted text in it, such as "this is quoted string"
Go to the patron search, search for a set of results that would include the patron with the quotes in the note
Enjoy looking at "Processing" for forever.

Test Plan:
1) Apply this patch
2) Attempt to replicate using the steps above
Comment 12 Liz Rea 2019-04-25 14:48:34 UTC
Created attachment 88786 [details] [review]
Bug 22715: Searching for patrons with "" in the circulation note hangs patron search

To replicate:
Edit a patron to have a circ note, include quoted text in it, such as "this is quoted string"
Go to the patron search, search for a set of results that would include the patron with the quotes in the note
Enjoy looking at "Processing" for forever.

Test Plan:
1) Apply this patch
2) Attempt to replicate using the steps above

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
I took this a step further and used <script>"something in
quotes"</script> for my test note and this worked just fine. YAY.
Comment 13 Katrin Fischer 2019-04-25 17:09:27 UTC
I am in doubt about the filters used here:

"[% data.borrowernotes | html_line_break | collapse | html | $To %]"

html_line_break = turns line breaks into <br> OK
collapse = removes additional whitespace - OK
html = escapes... so it shoudl escape the <br> tags
$To = ?

We had other cases where we needed to switch to html html_line_break to make things work. Can someone help me out here an explain?

That said.... it DOES work.
Comment 14 Katrin Fischer 2019-04-25 17:13:56 UTC
Ok... actually it's me: I was testing without the patch applied. It did work. But now with the patch, I have:

<b>TEst</b><br /> das <br /> Hier\

Instead of the correct display.

Without the patch (just master), I get:

TEst (in bold)
das
Hier\

Help?
Comment 15 Nick Clemens 2019-04-25 17:47:38 UTC
Created attachment 88804 [details] [review]
Bug 22715: Remove extraneous html filter
Comment 16 Nick Clemens 2019-04-25 17:52:43 UTC
(In reply to Katrin Fischer from comment #13)
> I am in doubt about the filters used here:
> 
> "[% data.borrowernotes | html_line_break | collapse | html | $To %]"

I think we just need one html filter - the link break should take care of filtering:
[% data.borrowernotes | html_line_break | collapse | $To %]

Alternately:
If we don't want to process any of the html here but add line breaks:
[% data.borrowernotes | html | html_line_break | collapse | $To %]
 
I suspect we do expect 'bold' etc to be processed though, I imagine some libraries could have links here
Comment 17 Katrin Fischer 2019-04-25 18:04:30 UTC
(In reply to Nick Clemens from comment #16)
> (In reply to Katrin Fischer from comment #13)
> > I am in doubt about the filters used here:
> > 
> > "[% data.borrowernotes | html_line_break | collapse | html | $To %]"
> 
> I think we just need one html filter - the link break should take care of
> filtering:
> [% data.borrowernotes | html_line_break | collapse | $To %]
> 
> Alternately:
> If we don't want to process any of the html here but add line breaks:
> [% data.borrowernotes | html | html_line_break | collapse | $To %]
>  
> I suspect we do expect 'bold' etc to be processed though, I imagine some
> libraries could have links here

I don't think that html_line_break does any escapign beyond changing the new lines. I think actually there should be a $raw as we just allowed to add HTML to that field.
Comment 18 Nick Clemens 2019-04-25 18:19:01 UTC
(In reply to Katrin Fischer from comment #17)
> I don't think that html_line_break does any escapign beyond changing the new
> lines. I think actually there should be a $raw as we just allowed to add
> HTML to that field.


I don't think we need a raw since we are filtering in other ways, I think we do want the link breaks, so this works as expected, no?
Comment 19 Katrin Fischer 2019-04-25 18:39:36 UTC
I think the $raw would have no other effect than showing we want HTML there. We are not really filtering.. we are adding and removing whitespace. Not sure what $To all does. Need another opinion, maybe Jonathan?
Comment 20 Katrin Fischer 2019-04-25 18:47:55 UTC
Created attachment 88805 [details] [review]
Bug 22715: Searching for patrons with "" in the circulation note hangs patron search

To replicate:
Edit a patron to have a circ note, include quoted text in it, such as "this is quoted string"
Go to the patron search, search for a set of results that would include the patron with the quotes in the note
Enjoy looking at "Processing" for forever.

Test Plan:
1) Apply this patch
2) Attempt to replicate using the steps above

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
I took this a step further and used <script>"something in
quotes"</script> for my test note and this worked just fine. YAY.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2019-04-25 18:47:59 UTC
Created attachment 88806 [details] [review]
Bug 22715: Remove extraneous html filter

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Jonathan Druart 2019-04-25 21:53:30 UTC
(In reply to Katrin Fischer from comment #19)
> I think the $raw would have no other effect than showing we want HTML there.
> We are not really filtering.. we are adding and removing whitespace. Not
> sure what $To all does. Need another opinion, maybe Jonathan?

Hi Katrin, see bug 20891 comment 5:
"""
If this patch works we will need to rethink the 'To' TT plugin.
It was originally designed to have several escape methods, but with
these changes it will not make sense to name it 'To' if used only to
escape JSON
"""
Comment 23 Katrin Fischer 2019-04-25 22:59:20 UTC
(In reply to Jonathan Druart from comment #22)
> (In reply to Katrin Fischer from comment #19)
> > I think the $raw would have no other effect than showing we want HTML there.
> > We are not really filtering.. we are adding and removing whitespace. Not
> > sure what $To all does. Need another opinion, maybe Jonathan?
> 
> Hi Katrin, see bug 20891 comment 5:
> """
> If this patch works we will need to rethink the 'To' TT plugin.
> It was originally designed to have several escape methods, but with
> these changes it will not make sense to name it 'To' if used only to
> escape JSON
> """

Sorry Jonathan, but that doesn't help me :) 

What filters should be used here?
Comment 24 Jonathan Druart 2019-04-25 23:45:56 UTC
We need "To" (which must be renamed or adjusted) to correctly json escaped.
We do not want to html filter the circulation notes, so it makes sense to remove this filter.

The patches look ok to me.
Comment 25 Jonathan Druart 2019-04-25 23:47:30 UTC
Additionally we should json filter all the other TT variables in this template.
Comment 26 Katrin Fischer 2019-04-25 23:47:42 UTC
Thank you!
Comment 27 Nick Clemens 2019-04-26 12:42:05 UTC
Awesome work all!

Pushed to master for 19.05
Comment 28 Martin Renvoize 2019-04-26 15:54:19 UTC
Pushed to 18.11.x for 18.11.05
Comment 29 Lucas Gass 2019-05-24 16:20:55 UTC
backported to 18.05.x for 18.05.13