Bug 14691 - Can't delete patron with ' character in cardnumber
Summary: Can't delete patron with ' character in cardnumber
Status: RESOLVED DUPLICATE of bug 14910
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Liz Rea
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-18 20:40 UTC by HB-NEKLS
Modified: 2015-09-29 04:33 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 14691 - Can't delete patron with ' character in cardnumber (1.92 KB, patch)
2015-08-19 01:31 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 14691 - Can't delete patron with ' character in cardnumber (1.52 KB, patch)
2015-08-19 01:36 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 14691 - [Alternate] Can't delete patron with ' character in cardnumber (4.71 KB, patch)
2015-08-24 01:10 UTC, David Cook
Details | Diff | Splinter Review
Bug 14691 - Can't delete patron with ' character in cardnumber (1.59 KB, patch)
2015-09-27 23:31 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description HB-NEKLS 2015-08-18 20:40:57 UTC
If a patron cardnumber has a ' character in it, and you try to delete the account, nothing happens -- the delete pop-up message doesn't appear. 

This appears to not affect cards with any other special character, and I've tested " & - so far]. It's only cardnumbers with the ' character in them. 

Test plan: 

1. Create patron account with cardnumber sss'ssssssss and save the account.
2. Try to delete the account. Nothing happens.
3. Edit the account, and remove the ' character. Save the acccount. 
4. Try to delete the account; note the Delete pop-up message that now appears.
Comment 1 Liz Rea 2015-08-19 01:31:07 UTC Comment hidden (obsolete)
Comment 2 Liz Rea 2015-08-19 01:32:44 UTC
Comment on attachment 41644 [details] [review]
Bug 14691 - Can't delete patron with ' character in cardnumber

Oops, typo.
Comment 3 Liz Rea 2015-08-19 01:36:42 UTC Comment hidden (obsolete)
Comment 4 David Cook 2015-08-19 05:18:11 UTC
I encountered this a while ago and thought about fixing it as well, but then Chris Cormack pointed me to http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13618 which should fix this problem everywhere I think?
Comment 5 Liz Rea 2015-08-20 04:43:07 UTC
Possibly you are right, I'd need to do a bit of reading.

Thanks for pointing that out.
Comment 6 Liz Rea 2015-08-20 22:02:44 UTC
Right, ok. So, bug 13618 is nowhere near ready - I think if it isn't ready by shortly before string freeze we should go with this one in the meantime (and same for bug 14505) because even though we have a workaround (delete the '), it still shouldn't break like this.
Comment 7 David Cook 2015-08-20 23:42:38 UTC
(In reply to Liz Rea from comment #6)
> Right, ok. So, bug 13618 is nowhere near ready - I think if it isn't ready
> by shortly before string freeze we should go with this one in the meantime
> (and same for bug 14505) because even though we have a workaround (delete
> the '), it still shouldn't break like this.

When I originally mentioned bug 13618, I didn't realize that it wasn't ready yet, so I think you're totally right about moving ahead with something else in the meantime.

Another idea would be to upgrade Template Toolkit from version 2.24 to 2.26 and use the "uri" or "url" filter:

"As of version 2.26 of the Template Toolkit, the uri and url filters use the unsafe character set defined by RFC3986. This means that certain characters ("(", ")", "~", "*", "!" and the single quote "'") are now deemed unsafe and will be escaped as hex character sequences. The double quote character ('"') is now deemed safe and will not be escaped."*
(http://template-toolkit.org/docs/manual/Filters.html#section_uri)

But it seems like Debian is still using 2.24 and only moved to 2.24 in the Squeeze-Wheezy move... (https://packages.debian.org/search?keywords=libtemplate-perl)

*I suppose double quote characters could cause problems if the Javascript was using double quotes instead of single quotes... another idea is to roll our own TT::Filter...
Comment 8 Liz Rea 2015-08-20 23:45:14 UTC
Yeah I was looking at that - I wasn't sure (didn't test, actually) whether |html or |uri or |url would work here - the manual for TT doesn't indicate that ' is an escaped character in uri/url, and it will convert it to an entity in html which I didn't test, but certainly could.
Comment 9 Liz Rea 2015-08-21 00:13:27 UTC
Yeah ok, I've tested those filters and none of them actually work, where this one does.

If someone has a better solution, I'm happy to know it because then I'll learn something. Otherwise... yeah lets test this one.
Comment 10 David Cook 2015-08-23 23:48:03 UTC
(In reply to Liz Rea from comment #8)
> Yeah I was looking at that - I wasn't sure (didn't test, actually) whether
> |html or |uri or |url would work here - the manual for TT doesn't indicate
> that ' is an escaped character in uri/url, and it will convert it to an
> entity in html which I didn't test, but certainly could.

(I know we talked about this already on IRC but I'll mention it here for everyone else)

The TT manual mentions that it doesn't escape the single quote prior to version 2.26, but it will with version 2.26 and later (unless the line "Template::Filters->use_rfc2732;" is added). 

But the version in Debian Wheezy onwards is 2.24, so the url/uri filters won't work until Debian moves on to 2.26+, which I don't imagine will be for a while, since they just moved to 2.24 from 2.22 between Squeeze and Wheezy.

--

Personally, I think I'm more in favour of adding our own Template Toolkit Filter Plugin based on the 2.26 uri filter. It would be more comprehensive than the replace vmethod and if one day we have TT 2.26 as the required version, we can always do a simple replacement in the code of "| escape_uri" with "| uri".



--
http://template-toolkit.org/docs/modules/Template/Filters.html#section_FILTERS

http://template-toolkit.org/docs/manual/Directives.html#section_FILTER

http://www.template-toolkit.org/docs/modules/Template/Plugin/Filter.html
Comment 11 David Cook 2015-08-23 23:53:47 UTC
I'll take a look at doing an alternate patch in a sec...
Comment 12 David Cook 2015-08-24 00:06:35 UTC
Actually, at a glance, it seems like the default in 2.26 is still to use RFC2732  despite what the manual says... 

(see "our $UNSAFE_CHARS = $UNSAFE_SPEC->{ RFC2732 };" in  http://cpansearch.perl.org/src/ABW/Template-Toolkit-2.26/lib/Template/Filters.pm)
Comment 13 David Cook 2015-08-24 00:21:34 UTC
Yikes and maybe this is why:

https://github.com/abw/Template2/issues/13
https://github.com/abw/Template2/issues/35

I was wondering why they were no longer escaping double quotes... I was going to do a custom version. 

That must be why 2.26 still uses the code for RFC2732... the manual must be out of date.
Comment 14 David Cook 2015-08-24 01:10:00 UTC Comment hidden (obsolete)
Comment 15 David Cook 2015-08-24 01:21:30 UTC
Looks like the RFC3986 regression was fixed in master...
https://github.com/abw/Template2/commit/af3444f719490d814c15c2fea3ff3c4e34d1f9a9

--

Another alternative option to my alternate patch is instead of using a Template::Plugin::Filter, we could add a "&escape_uri" function to the FILTERS configuration option in C4::Templates.
Comment 16 Liz Rea 2015-09-27 20:37:54 UTC
A patch almost identical to my original patch was just pushed to bug 14505 to fix a similar issue. Just wondering now why my original patch is insufficient to solve this problem in the interim to bug 13618.

Liz
Comment 17 David Cook 2015-09-27 23:28:38 UTC
(In reply to Liz Rea from comment #16)
> A patch almost identical to my original patch was just pushed to bug 14505
> to fix a similar issue. Just wondering now why my original patch is
> insufficient to solve this problem in the interim to bug 13618.
> 
> Liz

Personally, I think we're adding technical debt by escaping the single quotes in this way, but since these patches are being pushed for other bugs, I don't see any reason not to sign off this one.
Comment 18 David Cook 2015-09-27 23:31:21 UTC
Created attachment 42897 [details] [review]
Bug 14691 - Can't delete patron with ' character in cardnumber

Test plan:

1. Create patron account with cardnumber sss'ssssssss and save the account.
2. Try to delete the account. Nothing happens.
3. Apply the patch, refresh the page.
4. Try to delete the account; note the Delete pop-up message that now appears.

Signed-off-by: David Cook <dcook@prosentient.com.au>

Works as described.
Comment 19 Jonathan Druart 2015-09-28 10:16:29 UTC
Liz and David,
Sorry about that but... I have submitted kind of another counter-patch on bug 14910.
I think it fixes the issue bypassing the escape problematic :)
It also fixes another issue, that's why I have opened a new bug report.
Mark this one as duplicate if you agree with it.
Comment 20 Liz Rea 2015-09-28 20:39:27 UTC
Awesome, getting rid of the cardnumber altogether here is probably best. :)
Comment 21 HB-NEKLS 2015-09-28 20:45:48 UTC
If the patch for bug 14910 fixes this issue, too, then great -- I'll mark this as a duplicate. Thanks Jonathan, Liz, and David for looking into and working on this.
Comment 22 HB-NEKLS 2015-09-28 20:46:30 UTC

*** This bug has been marked as a duplicate of bug 14910 ***
Comment 23 David Cook 2015-09-29 04:33:30 UTC
Sounds good to me. I'm pretty much in favour of whatever solution at this point :)