When someone uses \ in the description of a list, the datatable in staff won't load and keeps processing. Tested in 16.11
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
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>
Combining SO and QA for trivial patch
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
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`)
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.
I think we had similar problems on patron search - anything we could pick from there?
(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 :)
(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.
(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.
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 ;)
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.
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?
Small steps, can we make sure the proposed patches fix the original issue reported?
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 \" which is a non valid JSON. You may test it also in list description.
(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 \" which > is a non valid JSON. > You may test it also in list description. Read and tried the patch?
(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 \" 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 ;)
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.
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>
Tested with a little meaner version: <script>alert('hola\');</script>\ Seems to work like a charm!
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>
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>
Updating severity a little bit - broken searches can be quite an issue for the library and are hard to fix without SQL access.
Awesome work all! Pushed to master for 19.05
Pushed to 18.11.x for 18.11.05
backported to 18.05.x for 18.05.13