Bug 20891 - Lists in staff don't load when \ was used in the description
Summary: Lists in staff don't load when \ was used in the description
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Lists (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 22715
  Show dependency treegraph
 
Reported: 2018-06-07 10:47 UTC by Katrin Fischer
Modified: 2019-08-09 12:46 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.05


Attachments
Bug 20891: Escape list's names in JSON (2.33 KB, patch)
2018-06-07 15:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20891: Escape list's names in JSON (2.42 KB, patch)
2018-06-08 05:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20891: Escape html then JSON (3.12 KB, patch)
2018-06-08 15:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20891: Escape list's names in JSON (2.54 KB, patch)
2019-04-24 15:29 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 20891: Escape html then JSON (3.26 KB, patch)
2019-04-24 15:29 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 20891: Escape list's names in JSON (2.59 KB, patch)
2019-04-25 05:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20891: Escape html then JSON (3.31 KB, patch)
2019-04-25 05:44 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2018-06-07 10:47:31 UTC
When someone uses \ in the description of a list, the datatable in staff won't load and keeps processing.

Tested in 16.11
Comment 1 Jonathan Druart 2018-06-07 15:20:19 UTC
Created attachment 75887 [details] [review]
Bug 20891: Escape list's names in JSON

When someone uses \ in the description of a list, the datatable in staff
won't load and keeps processing.

Test plan:
- Create a list named "<script>alert('hola');</script>"
- Create another list named "k\o\h\a"
- Hit /cgi-bin/koha/virtualshelves/shelves.pl

=> Without this patch the lists will not be displayed, JSON is
malformated
=> With this patch everything is ok
Comment 2 Marcel de Rooy 2018-06-08 05:51:54 UTC
Created attachment 75890 [details] [review]
Bug 20891: Escape list's names in JSON

When someone uses \ in the description of a list, the datatable in staff
won't load and keeps processing.

Test plan:
- Create a list named "<script>alert('hola');</script>"
- Create another list named "k\o\h\a"
- Hit /cgi-bin/koha/virtualshelves/shelves.pl

=> Without this patch the lists will not be displayed, JSON is
malformated
=> With this patch everything is ok

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2018-06-08 05:52:39 UTC
Combining SO and QA for trivial patch
Comment 4 Nick Clemens 2018-06-08 13:05:19 UTC
I took your test plant too literally, if your list name is surrounded by quotes:
"<script>alert('hola');</script>"

it works before this patch, but not after. I think the JSON conversion doesn't like the quotes
Comment 5 Jonathan Druart 2018-06-08 15:23:35 UTC
Created attachment 75922 [details] [review]
Bug 20891: Escape html then JSON

To my understanding we need to escape first html chars then to JSON.

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

IIRC we should keep the 2 different ways to use it:
 * [% To.json( string ) %]
 * [% string | $To %]
otherwise it will be hard to use it when called in argument of
patron-title.inc (`git grep To.json`)
Comment 6 Jonathan Druart 2018-06-08 15:25:32 UTC
Good catch Nick. From trivial it is becoming a complex one now.

Last patch for discussion and review before a full rewrite of the TT plugin.
Comment 7 Katrin Fischer 2018-06-18 22:06:32 UTC
I think we had similar problems on patron search - anything we could pick from there?
Comment 8 Jonathan Druart 2018-06-19 12:53:14 UTC
(In reply to Katrin Fischer from comment #7)
> I think we had similar problems on patron search - anything we could pick
> from there?

We are using the same code for lists and patrons, so I supposed patron's result will explode if '\' is contained in the surname/firstname, which certainly does not exist in a production DB :)
Comment 9 Katrin Fischer 2018-06-19 13:02:05 UTC
(In reply to Jonathan Druart from comment #8)
> (In reply to Katrin Fischer from comment #7)
> > I think we had similar problems on patron search - anything we could pick
> > from there?
> 
> We are using the same code for lists and patrons, so I supposed patron's
> result will explode if '\' is contained in the surname/firstname, which
> certainly does not exist in a production DB :)

Actually, we had a lot of issues with patron search on migrating a lot of libraries recently. You never know what is in the data you get and it's hard to debug the JSON errors in some cases.
Comment 10 Jonathan Druart 2018-06-19 13:32:44 UTC
(In reply to Katrin Fischer from comment #9)
> (In reply to Jonathan Druart from comment #8)
> > (In reply to Katrin Fischer from comment #7)
> > > I think we had similar problems on patron search - anything we could pick
> > > from there?
> > 
> > We are using the same code for lists and patrons, so I supposed patron's
> > result will explode if '\' is contained in the surname/firstname, which
> > certainly does not exist in a production DB :)
> 
> Actually, we had a lot of issues with patron search on migrating a lot of
> libraries recently. You never know what is in the data you get and it's hard
> to debug the JSON errors in some cases.

open the web dev console, get the answer from the server, copy/paste it on a JSON validator website, and you will know exactly where the problem is.
Comment 11 Katrin Fischer 2018-06-19 15:13:30 UTC
The number of patrons in this libraries is so big that the answer is cut off in the developer tools in the browser... so you have to page until you can limit it to the right 'area'. I am open for ideas ;)
Comment 12 Katrin Fischer 2018-06-21 07:42:43 UTC
We have checked recent migration scripts that we adapted for several problems with patron search. Problematic are afawk: backslash, double quotes, non-printable and for some reason we also have \x11 to ae in there, but not sure if that was just a presentation problem.
Comment 13 Katrin Fischer 2019-01-03 09:59:12 UTC
We've had a similar issue now with the item search where a publisher is spelled with a \ in the name. I think we can't rule \ appearing in real data and for patrons. Can we have another go here?
Comment 14 Jonathan Druart 2019-02-14 19:32:15 UTC
Small steps, can we make sure the proposed patches fix the original issue reported?
Comment 15 Fridolin SOMERS 2019-02-26 06:32:15 UTC
I reported in Bug 22369 that combination of To.JSON and |html is wrong for double quotes. JSON converts to \" and then html converts to \&quot; which is a non valid JSON.
You may test it also in list description.
Comment 16 Jonathan Druart 2019-02-26 17:03:04 UTC
(In reply to Fridolin SOMERS from comment #15)
> I reported in Bug 22369 that combination of To.JSON and |html is wrong for
> double quotes. JSON converts to \" and then html converts to \&quot; which
> is a non valid JSON.
> You may test it also in list description.

Read and tried the patch?
Comment 17 Fridolin SOMERS 2019-02-27 08:51:50 UTC
(In reply to Jonathan Druart from comment #16)
> (In reply to Fridolin SOMERS from comment #15)
> > I reported in Bug 22369 that combination of To.JSON and |html is wrong for
> > double quotes. JSON converts to \" and then html converts to \&quot; which
> > is a non valid JSON.
> > You may test it also in list description.
> 
> Read and tried the patch?

Ha sorry I missed the second patch which looks great.
I see this issue as already been spotted in comment 4.
The problem is in good hands ;)
Comment 18 Liz Rea 2019-04-24 15:29:36 UTC
Created attachment 88637 [details] [review]
Bug 20891: Escape list's names in JSON

When someone uses \ in the description of a list, the datatable in staff
won't load and keeps processing.

Test plan:
- Create a list named "<script>alert('hola');</script>"
- Create another list named "k\o\h\a"
- Hit /cgi-bin/koha/virtualshelves/shelves.pl

=> Without this patch the lists will not be displayed, JSON is
malformated
=> With this patch everything is ok

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Resolved minor merge issues, QA plz check they are ok - but it works.
Comment 19 Liz Rea 2019-04-24 15:29:53 UTC
Created attachment 88638 [details] [review]
Bug 20891: Escape html then JSON

To my understanding we need to escape first html chars then to JSON.

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

IIRC we should keep the 2 different ways to use it:
 * [% To.json( string ) %]
 * [% string | $To %]
otherwise it will be hard to use it when called in argument of
patron-title.inc (`git grep To.json`)

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 20 Katrin Fischer 2019-04-25 05:43:46 UTC
Tested with a little meaner version: <script>alert('hola\');</script>\
Seems to work like a charm!
Comment 21 Katrin Fischer 2019-04-25 05:44:34 UTC
Created attachment 88675 [details] [review]
Bug 20891: Escape list's names in JSON

When someone uses \ in the description of a list, the datatable in staff
won't load and keeps processing.

Test plan:
- Create a list named "<script>alert('hola');</script>"
- Create another list named "k\o\h\a"
- Hit /cgi-bin/koha/virtualshelves/shelves.pl

=> Without this patch the lists will not be displayed, JSON is
malformated
=> With this patch everything is ok

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Katrin Fischer 2019-04-25 05:44:38 UTC
Created attachment 88676 [details] [review]
Bug 20891: Escape html then JSON

To my understanding we need to escape first html chars then to JSON.

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

IIRC we should keep the 2 different ways to use it:
 * [% To.json( string ) %]
 * [% string | $To %]
otherwise it will be hard to use it when called in argument of
patron-title.inc (`git grep To.json`)

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2019-04-25 05:45:12 UTC
Updating severity a little bit - broken searches can be quite an issue for the library and are hard to fix without SQL access.
Comment 24 Nick Clemens 2019-04-25 11:52:39 UTC
Awesome work all!

Pushed to master for 19.05
Comment 25 Martin Renvoize 2019-04-26 15:20:10 UTC
Pushed to 18.11.x for 18.11.05
Comment 26 Lucas Gass 2019-05-24 15:32:08 UTC
backported to 18.05.x for 18.05.13