Bug 19655 - To.json doesn't escape newlines which can create invalid JSON
Summary: To.json doesn't escape newlines which can create invalid JSON
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-11-20 16:04 UTC by Kyle M Hall
Modified: 2017-12-04 19:26 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19655 - To.json doesn't escape newlines which can create invalid JSON (1.37 KB, patch)
2017-11-20 16:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19655 - To.json doesn't escape newlines which can create invalid JSON (1.43 KB, patch)
2017-11-21 13:18 UTC, Simon Pouchol
Details | Diff | Splinter Review
Bug 19655 - To.json doesn't escape newlines which can create invalid JSON (1.52 KB, patch)
2017-11-24 06:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19655: Add tests (1.91 KB, patch)
2017-11-26 15:55 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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!