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)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-12 14:04 UTC by Jonathan Druart
Modified: 2019-01-02 10:14 UTC (History)
6 users (show)

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


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