Bug 19655

Summary: To.json doesn't escape newlines which can create invalid JSON
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Architecture, internals, and plumbingAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: critical    
Priority: P5 - low CC: chris, fridolin.somers, jonathan.druart, m.de.rooy, simon.pouchol
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19721
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 15734    
Attachments: Bug 19655 - To.json doesn't escape newlines which can create invalid JSON
Bug 19655 - To.json doesn't escape newlines which can create invalid JSON
Bug 19655 - To.json doesn't escape newlines which can create invalid JSON
Bug 19655: Add tests
Bug 19655: Use ->build instead of build_object in 16.11.x
Bug 19655: Use ->build instead of build_object in 16.11.x

Description Kyle M Hall 2017-11-20 16:04:42 UTC
JSON does not allow real line-breaks. If a field contains them and they are not escaped, a JSON parser will be unable to convert the stringified JSON back into an object.

This is clearly exemplified by the guarantor search, where a multi-line note on the guarantor's record will break the ability to select that guarantor.
Comment 1 Kyle M Hall 2017-11-20 16:08:05 UTC
Created attachment 69235 [details] [review]
Bug 19655 - To.json doesn't escape newlines which can create invalid JSON

JSON does not allow real line-breaks. If a field contains them and they are not escaped, a JSON parser will be unable to convert the stringified JSON back into an object.

This is clearly exemplified by the guarantor search, where a multi-line note on the guarantor's record will break the ability to select that guarantor.

Test Plan:
1) Create Patron A with a "Circulation note" that has multiple lines in it
2) Create Patron B
3) Attempt to set Patron A to be the guarantor for Patron B
4) Note selecting the patron does nothing
5) Apply this patch
6) Repeat step 3
7) Selecting the guarantor now works!
Comment 2 Simon Pouchol 2017-11-21 13:18:39 UTC
Created attachment 69244 [details] [review]
Bug 19655 - To.json doesn't escape newlines which can create invalid JSON

JSON does not allow real line-breaks. If a field contains them and they are not escaped, a JSON parser will be unable to convert the stringified JSON back into an object.

This is clearly exemplified by the guarantor search, where a multi-line note on the guarantor's record will break the ability to select that guarantor.

Test Plan:
1) Create Patron A with a "Circulation note" that has multiple lines in it
2) Create Patron B
3) Attempt to set Patron A to be the guarantor for Patron B
4) Note selecting the patron does nothing
5) Apply this patch
6) Repeat step 3
7) Selecting the guarantor now works!

Signed-off-by: Simon Pouchol <simon.pouchol@biblibre.com>
Comment 3 Marcel de Rooy 2017-11-24 06:59:39 UTC
Created attachment 69326 [details] [review]
Bug 19655 - To.json doesn't escape newlines which can create invalid JSON

JSON does not allow real line-breaks. If a field contains them and they are not escaped, a JSON parser will be unable to convert the stringified JSON back into an object.

This is clearly exemplified by the guarantor search, where a multi-line note on the guarantor's record will break the ability to select that guarantor.

Test Plan:
1) Create Patron A with a "Circulation note" that has multiple lines in it
2) Create Patron B
3) Attempt to set Patron A to be the guarantor for Patron B
4) Note selecting the patron does nothing
5) Apply this patch
6) Repeat step 3
7) Selecting the guarantor now works!

Signed-off-by: Simon Pouchol <simon.pouchol@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Jonathan Druart 2017-11-26 15:55:47 UTC
Created attachment 69346 [details] [review]
Bug 19655: Add tests
Comment 5 Jonathan Druart 2017-11-26 16:01:32 UTC
I am not sure it is the best way to fix it.
1. We may encounter issues later using To.json on other values that use escaped strings (ok we will update the code to make it works)
2. Maybe we should replace all \\ with \\\\

I will push as it and adapt later if needed.
Comment 6 Jonathan Druart 2017-11-26 16:26:59 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 7 Fridolin Somers 2017-12-11 08:24:54 UTC
Poushed to 17.05.x, will be in v17.05.07
Comment 8 Chris Cormack 2017-12-12 08:24:11 UTC
Pushed to 16.11.x will be in 16.11.15
Comment 9 Jonathan Druart 2017-12-12 13:27:03 UTC
(In reply to Chris Cormack from comment #8)
> Pushed to 16.11.x will be in 16.11.15

This patch breaks tests:
https://jenkins.koha-community.org/job/Koha_16.11_D8/89/

#   Failed test 'No tests run for subtest "json"'
#   at t/db_dependent/Template/Plugin/To.t line 44.
Can't locate object method "build_object" via package "t::lib::TestBuilder" at t/db_dependent/Template/Plugin/To.t line 32.
# Looks like your test exited with 255 just after 1.
[09:41:40] t/db_dependent/Template/Plugin/To.t

The build_object method of TestBuilder has been added by bug 18182, which is not in 16.11.x
You should use ->build instead.
Comment 10 Jonathan Druart 2017-12-12 13:29:55 UTC
Created attachment 69732 [details] [review]
Bug 19655: Use ->build instead of build_object in 16.11.x

The build_object method of TestBuilder has been added by bug 18182,
which is not in 16.11.x

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Chris Cormack 2017-12-14 21:27:05 UTC
Created attachment 69806 [details] [review]
Bug 19655: Use ->build instead of build_object in 16.11.x

The build_object method of TestBuilder has been added by bug 18182,
which is not in 16.11.x

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>