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.
Created attachment 41644 [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.
Comment on attachment 41644 [details] [review] Bug 14691 - Can't delete patron with ' character in cardnumber Oops, typo.
Created attachment 41645 [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.
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?
Possibly you are right, I'd need to do a bit of reading. Thanks for pointing that out.
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.
(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...
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.
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.
(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
I'll take a look at doing an alternate patch in a sec...
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)
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.
Created attachment 41803 [details] [review] Bug 14691 - [Alternate] Can't delete patron with ' character in cardnumber This patch adds a new Template Toolkit plugin. To use it, simple add [% USE EscapeURI %] at the top of the template, and then use "escape_uri" just as you would with the normal "uri" filter (e.g. "[% variable | escape_uri %]". It is based on the code from version 2.26 of Template::Filter::uri_filter(), but with a fix for the RFC3986 reserved character definition. In versions prior to 2.26, single quotes weren't escapes, which causes problems when trying to use the uri filter for text containing single quotes within a single quoted string, as observed in some Javascript for deleting patrons. 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; note that the patron gets deleted if you click OK.
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.
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
(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.
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.
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.
Awesome, getting rid of the cardnumber altogether here is probably best. :)
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.
*** This bug has been marked as a duplicate of bug 14910 ***
Sounds good to me. I'm pretty much in favour of whatever solution at this point :)