Bug 21986 - Quotation marks are wrongly escaped in several places
Summary: Quotation marks are wrongly escaped in several places
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
: 22367 22769 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-12-12 14:04 UTC by Jonathan Druart
Modified: 2019-04-25 05:28 UTC (History)
11 users (show)

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


Attachments
Bug 21986: Do not escape quotation marks when cataloguing (5.12 KB, patch)
2018-12-12 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
image of the problem (42.78 KB, image/jpeg)
2018-12-12 21:51 UTC, Elaine Bradtke
Details
Bug 21986: Do not escape quotation marks when cataloguing (5.18 KB, patch)
2018-12-13 16:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21986: Do not escape quotation marks when cataloguing (5.24 KB, patch)
2018-12-14 07:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2018-12-12 14:04:10 UTC
In several places we escape quotation marks using
  $value =~ s/"/"/g;
All the occurrences are wrong and must be removed.
Comment 1 Jonathan Druart 2018-12-12 14:14:09 UTC
Created attachment 83120 [details] [review]
Bug 21986: Do not escape quotation marks when cataloguing

In several places we escape quotation marks using
  $value =~ s/"/"/g;
All the occurrences are wrong and must be removed.
Most of them are leftover of bug 11638 (Remove HTML from
addbiblio.pl), which removes the construction of html from pl scripts.

The problem has been highlighted by bug 13618, I did not track down why
the issue did not exist before (?)

Test plan:
0/ Use strings with quotation marks, like:
'Fiddle tune history : "bad" tunes'
You can also use other html characters to make the tests more complete,
like 'Fiddle tune history : <"bad" tunes>'
1/ authorities/authorities.pl
a. Edit an authority filling different fields with quotation marks
b. Edit it again
=> The display (inputs' values) is wrong, if you save the escaped quotes
will be inserted
2/ cataloguing/addbiblio.pl
Same editing a bibliographic record
3/ cataloguing/additem.pl
Same editing items
4/ members/memberentry.pl
Edit a patron's record and fill some fields with quotation marks
+ fields borrowernotes and opacnotes
=> The quotes are inserted directly in DB (escape is done before the
insert!)
5/ opac/opac-review.pl
For QA only: $js_ok_review is never used
6/ tools/batchMod.pl
For QA only: $value is always undefined at that point
Comment 2 Elaine Bradtke 2018-12-12 21:51:42 UTC
Created attachment 83143 [details]
image of the problem
Comment 3 Jonathan Druart 2018-12-12 22:38:15 UTC
(In reply to Elaine Bradtke from comment #2)
> Created attachment 83143 [details]
> image of the problem

Hi Elaine, do you still have this problem with the patch applied?
Comment 4 Martin Renvoize 2018-12-13 16:12:25 UTC
Created attachment 83179 [details] [review]
Bug 21986: Do not escape quotation marks when cataloguing

In several places we escape quotation marks using
  $value =~ s/"/&quot;/g;
All the occurrences are wrong and must be removed.
Most of them are leftover of bug 11638 (Remove HTML from
addbiblio.pl), which removes the construction of html from pl scripts.

The problem has been highlighted by bug 13618, I did not track down why
the issue did not exist before (?)

Test plan:
0/ Use strings with quotation marks, like:
'Fiddle tune history : "bad" tunes'
You can also use other html characters to make the tests more complete,
like 'Fiddle tune history : <"bad" tunes>'
1/ authorities/authorities.pl
a. Edit an authority filling different fields with quotation marks
b. Edit it again
=> The display (inputs' values) is wrong, if you save the escaped quotes
will be inserted
2/ cataloguing/addbiblio.pl
Same editing a bibliographic record
3/ cataloguing/additem.pl
Same editing items
4/ members/memberentry.pl
Edit a patron's record and fill some fields with quotation marks
+ fields borrowernotes and opacnotes
=> The quotes are inserted directly in DB (escape is done before the
insert!)
5/ opac/opac-review.pl
For QA only: $js_ok_review is never used
6/ tools/batchMod.pl
For QA only: $value is always undefined at that point

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2018-12-13 16:13:00 UTC
I cannot replicate the issue after applying the patches.. Signing Off
Comment 6 Marcel de Rooy 2018-12-14 06:41:03 UTC
QA: Looking here
Comment 7 Marcel de Rooy 2018-12-14 07:10:51 UTC
> The problem has been highlighted by bug 13618, I did not track down why
> the issue did not exist before (?)

If you pass a&amp;b in a template var thru the html filter, it will run the ampersand conversion once again. Resulting in: a&amp;amp;b

So we need to choose where to escape.
Comment 8 Marcel de Rooy 2018-12-14 07:15:17 UTC
(In reply to Martin Renvoize from comment #4)
> For QA only: $js_ok_review is never used
> 6/ tools/batchMod.pl
> For QA only: $value is always undefined at that point

Thx. Just seeing these remarks by accident. They were quite hidden..
Comment 9 Marcel de Rooy 2018-12-14 07:18:41 UTC
(In reply to Marcel de Rooy from comment #7)
> > The problem has been highlighted by bug 13618, I did not track down why
> > the issue did not exist before (?)
> 
> If you pass a&amp;b in a template var thru the html filter, it will run the
> ampersand conversion once again. Resulting in: a&amp;amp;b
> 
> So we need to choose where to escape.

Should be quotation. But same thing applies.
Comment 10 Marcel de Rooy 2018-12-14 07:19:09 UTC
Created attachment 83195 [details] [review]
Bug 21986: Do not escape quotation marks when cataloguing

In several places we escape quotation marks using
  $value =~ s/"/&quot;/g;
All the occurrences are wrong and must be removed.
Most of them are leftover of bug 11638 (Remove HTML from
addbiblio.pl), which removes the construction of html from pl scripts.

The problem has been highlighted by bug 13618, I did not track down why
the issue did not exist before (?)

Test plan:
0/ Use strings with quotation marks, like:
'Fiddle tune history : "bad" tunes'
You can also use other html characters to make the tests more complete,
like 'Fiddle tune history : <"bad" tunes>'
1/ authorities/authorities.pl
a. Edit an authority filling different fields with quotation marks
b. Edit it again
=> The display (inputs' values) is wrong, if you save the escaped quotes
will be inserted
2/ cataloguing/addbiblio.pl
Same editing a bibliographic record
3/ cataloguing/additem.pl
Same editing items
4/ members/memberentry.pl
Edit a patron's record and fill some fields with quotation marks
+ fields borrowernotes and opacnotes
=> The quotes are inserted directly in DB (escape is done before the
insert!)
5/ opac/opac-review.pl
For QA only: $js_ok_review is never used
6/ tools/batchMod.pl
For QA only: $value is always undefined at that point

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Nick Clemens 2018-12-14 15:33:18 UTC
Awesome work all!

Pushed to master for 19.05
Comment 12 Martin Renvoize 2018-12-17 12:06:57 UTC
Pushed to 18.11.x for 18.11.01
Comment 13 Lucas Gass 2018-12-26 17:59:21 UTC
backported to 18.05 for 18.05.08
Comment 14 Fridolin SOMERS 2019-01-02 10:14:44 UTC
Pushed to 17.11.x for 17.11.14
Comment 15 Magnus Enger 2019-02-12 07:29:54 UTC
Bug 22316 is similar to this, but only manifests when you try to sort the patron search result table. Tested with 18.11.02.
Comment 16 Kelly McElligott 2019-02-22 14:10:54 UTC
On version Koha 18.05.08, still experiencing issues with this same thing.  When editing a record, or Edit as New - a title with quotation marks will be removed.  Filing a new bug: 22395
Comment 17 Jonathan Druart 2019-02-26 17:08:12 UTC
Lucas and Fridolin, take a look at bug 22395, it seems that this patch should not have been backported into your versions.
Comment 18 Fridolin SOMERS 2019-02-27 08:44:23 UTC
(In reply to Jonathan Druart from comment #17)
> Lucas and Fridolin, take a look at bug 22395, it seems that this patch
> should not have been backported into your versions.

Oh indeed I see the problem.
Reverted from 17.11.x, will be removed in 17.11.16.
Comment 19 Lucas Gass 2019-02-28 17:04:42 UTC
reverted in 18.05.x and will be removed in 18.05.10
Comment 20 Katrin Fischer 2019-04-25 05:26:46 UTC
*** Bug 22367 has been marked as a duplicate of this bug. ***
Comment 21 Katrin Fischer 2019-04-25 05:28:55 UTC
*** Bug 22769 has been marked as a duplicate of this bug. ***