Bug 19560 - Unable to delete library when branchcode contains special characters
Summary: Unable to delete library when branchcode contains special characters
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Josef Moravec
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 19607
  Show dependency treegraph
 
Reported: 2017-11-01 19:04 UTC by Joy Nelson
Modified: 2017-12-26 17:31 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 19560: Unable to delete library when branchcode contains special characters (8.61 KB, patch)
2017-11-09 15:21 UTC, Owen Leonard
Details | Splinter Review
Bug 19560: Unable to delete library when branchcode contains special characters (8.67 KB, patch)
2017-11-10 22:04 UTC, David Bourgault
Details | Splinter Review
Bug 19560: Unable to delete library when branchcode contains special characters (8.72 KB, patch)
2017-11-13 16:24 UTC, Nick Clemens
Details | Splinter Review
Bug 19560: Correctly escape branchcode in admin/branches.pl (4.27 KB, patch)
2017-11-29 18:25 UTC, Jonathan Druart
Details | Splinter Review
Bug 19560: Correctly escape branchcode in admin/branches.pl (4.24 KB, patch)
2017-12-04 19:03 UTC, Jonathan Druart
Details | Splinter Review
Bug 19560: Correctly escape branchcode in admin/branches.pl (4.49 KB, patch)
2017-12-11 17:43 UTC, Owen Leonard
Details | Splinter Review
Bug 19607: Move admin templates JavaScript to the footer: Basic parameters (11.93 KB, patch)
2017-12-11 18:13 UTC, Owen Leonard
Details | Splinter Review
Bug 19560: Correctly escape branchcode in admin/branches.pl (4.84 KB, patch)
2017-12-12 14:08 UTC, Owen Leonard
Details | Splinter Review
Bug 19560: Correctly escape branchcode in admin/branches.pl (5.24 KB, patch)
2017-12-12 14:11 UTC, Owen Leonard
Details | Splinter Review
Bug 19560: Correctly escape branchcode in admin/branches.pl (5.30 KB, patch)
2017-12-13 11:48 UTC, Josef Moravec
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joy Nelson 2017-11-01 19:04:35 UTC
Special Characters in Branchcode cause the user to be unable to delete the branch.

Replicate:
1. Administration->Libraries and Groups
2. Click on New Library button
3. Create a new branch use the & or # character in the branchcode
4. Branch is created.
5. Click on Delete (under Actions) for that branch
6. Verify you wish to delete the branch.
7. Error message appears 
An error occurred when deleting this library. Check the logs. 

Can this field be validated when data is entered?  We should not allow users to create branchcodes that use special characters.
Comment 1 Owen Leonard 2017-11-09 15:21:44 UTC Comment hidden (obsolete)
Comment 2 David Bourgault 2017-11-10 22:04:56 UTC Comment hidden (obsolete)
Comment 3 David Bourgault 2017-11-10 22:06:45 UTC
QA tool passed
Works as described, but also prevents accentuated characters like é or à. For category codes I think this is acceptable/preferable.
Comment 4 Nick Clemens 2017-11-13 16:24:02 UTC Comment hidden (obsolete)
Comment 5 Nick Clemens 2017-11-13 16:26:13 UTC
I think this is a good start. We probably want these restrictions on authorised values too.

We will also need a warning on the about page for pre-existing values
Comment 6 Katrin Fischer 2017-11-13 18:13:01 UTC
(In reply to Nick Clemens from comment #5)
> I think this is a good start. We probably want these restrictions on
> authorised values too.
> 
> We will also need a warning on the about page for pre-existing values

We didn't do this on authorised values because of things like MAN_INV - this has the display text in the field for the code :(
Comment 7 Jonathan Druart 2017-11-29 17:48:46 UTC
This patch does 2 things:
1. Only accept few chars for branchcode (accentuated chars are rejected)
2. Fix the deletion problem if a branchcode contains special characters

It may be a problem for non latin alphabets to restrict as possiblities. Do we have known problems with branchcode containing special chars?
Comment 8 Joy Nelson 2017-11-29 18:05:57 UTC
Jonathon,
I encountered this because a library was adding a branch for "Art & Architecture" as A&A
When they decided to go with a different naming convention, they were unable to delete the branch.

joy


(In reply to Jonathan Druart from comment #7)
> This patch does 2 things:
> 1. Only accept few chars for branchcode (accentuated chars are rejected)
> 2. Fix the deletion problem if a branchcode contains special characters
> 
> It may be a problem for non latin alphabets to restrict as possiblities. Do
> we have known problems with branchcode containing special chars?
Comment 9 Jonathan Druart 2017-11-29 18:25:05 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2017-11-29 18:26:51 UTC
Joy, What I tried to say is: If we escape correctly the variable, we will not have problem to delete a library with branchcode=A&A, see the follow-up.
I am so wondering if it is really useful to restrict the code to a-Z0-9_-
Comment 11 Joy Nelson 2017-11-29 18:33:16 UTC
Ah!  I see.  Yes, escaping or restricting would work.  I can't say I have a preference in the matter.  

Thanks!
joy

(In reply to Jonathan Druart from comment #10)
> Joy, What I tried to say is: If we escape correctly the variable, we will
> not have problem to delete a library with branchcode=A&A, see the follow-up.
> I am so wondering if it is really useful to restrict the code to a-Z0-9_-
Comment 12 Owen Leonard 2017-12-04 17:22:08 UTC
As far as I can tell, JavaScript regular expressions will not recognize non-ascii "word" characters as such, and thus client-side validation can't work here unless we restrict the options quite a bit.

I appreciate that this limitation with my patch has bee pointed out because I don't think it's okay that we make it so strict if we don't have to.

Is it a limitation of branches.pl that it fails to accept a string like this?

/cgi-bin/koha/admin/branches.pl?op=add_form&branchcode=Ö×Ø&ÙÚ
Comment 13 Jonathan Druart 2017-12-04 19:03:19 UTC
Created attachment 69490 [details] [review]
Bug 19560: Correctly escape branchcode in admin/branches.pl
Comment 14 Jonathan Druart 2017-12-04 19:03:53 UTC
(In reply to Owen Leonard from comment #12)
> Is it a limitation of branches.pl that it fails to accept a string like this?
> 
> /cgi-bin/koha/admin/branches.pl?op=add_form&branchcode=Ö×Ø&ÙÚ

Fixed with the last patch.
Comment 15 Josef Moravec 2017-12-05 09:12:45 UTC
IMO the escaping is better solution - we need it anyway for prevent XSS and it is enough to solve this issue - so no reason to restrict the letters in branchcode.
Comment 16 Owen Leonard 2017-12-05 13:07:52 UTC
Should we still reject brachcodes with whitespace characters?
Comment 17 Jonathan Druart 2017-12-05 13:24:50 UTC
(In reply to Owen Leonard from comment #16)
> Should we still reject brachcodes with whitespace characters?

Yes I think so (for command line scripts)
Comment 18 Josef Moravec 2017-12-06 17:14:12 UTC
(In reply to Jonathan Druart from comment #17)
> (In reply to Owen Leonard from comment #16)
> > Should we still reject brachcodes with whitespace characters?
> 
> Yes I think so (for command line scripts)

I agree
Comment 19 Owen Leonard 2017-12-11 17:43:59 UTC Comment hidden (obsolete)
Comment 20 Owen Leonard 2017-12-11 17:45:20 UTC
Jonathan's patch is all that is needed. I've rebased it on master and signed off.
Comment 21 Owen Leonard 2017-12-11 18:13:56 UTC Comment hidden (obsolete)
Comment 22 Owen Leonard 2017-12-11 18:29:56 UTC Comment hidden (obsolete)
Comment 23 Josef Moravec 2017-12-12 07:29:03 UTC
Comment on attachment 69700 [details] [review]
Bug 19560: Correctly escape branchcode in admin/branches.pl

Review of attachment 69700 [details] [review]:
-----------------------------------------------------------------

Just a quick look at the code:

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/branches.tt
@@ +5,4 @@
>  [% ELSIF op == 'delete_confirm_category' %]
>      › Confirm deletion of group [% category.categorycode %]
>  [% ELSIF op == 'add_form' %]
> +    ›[% IF library %]Modify library[% ELSE %]New library [% library.branchcode | html %][% END %]

Shouldn't it be?

›[% IF library %]Modify library [% library.branchcode | html %][% ELSE %]New library[% END %]

looks like this is bug in current master...

@@ +67,4 @@
>  [% ELSIF op == 'delete_confirm_category' %]
>  › Confirm deletion of group [% category.categorycode %]
>  [% ELSIF op == 'add_form'  %]
> +› [% IF library %]Modify library[% ELSE %]New library [% library.branchcode | html %][% END %]

The same
Comment 24 Jonathan Druart 2017-12-12 13:39:02 UTC
(In reply to Josef Moravec from comment #23)
> looks like this is bug in current master...

Yes, definitely
Comment 25 Owen Leonard 2017-12-12 14:08:26 UTC
Created attachment 69735 [details] [review]
Bug 19560: Correctly escape branchcode in admin/branches.pl

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 26 Owen Leonard 2017-12-12 14:11:33 UTC
Created attachment 69736 [details] [review]
Bug 19560: Correctly escape branchcode in admin/branches.pl

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 27 Josef Moravec 2017-12-13 11:48:20 UTC
Created attachment 69775 [details] [review]
Bug 19560: Correctly escape branchcode in admin/branches.pl

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 28 Jonathan Druart 2017-12-14 20:17:00 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 29 Nick Clemens 2017-12-20 13:01:59 UTC
Awesome work all! Pushed to stable for 17.11.01
Comment 30 Jonathan Druart 2017-12-26 17:31:34 UTC
Owen, you modified the original patch to use a POST (form) instead of a GET (link), could you explain why?
It makes the button hard to locate from selenium tests and hide the url to the user.