Bug 21349 - Instructors with special characters (e.g. $, ., :) in their cardnumber cannot be removed from course reserves
Summary: Instructors with special characters (e.g. $, ., :) in their cardnumber cannot...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Course reserves (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Mark Tompsett
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-09-14 22:51 UTC by Eric Phetteplace
Modified: 2020-01-06 20:15 UTC (History)
6 users (show)

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


Attachments
Fix RemoveInstructor function (1.33 KB, patch)
2018-09-14 23:17 UTC, Eric Phetteplace
Details | Diff | Splinter Review
Bug 21349: Change cardnumber to borrowernumber (5.51 KB, patch)
2018-09-18 18:33 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21349: Instructors with special characters in cardnumber cannot be removed from course reserves (5.53 KB, patch)
2018-09-21 21:58 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21349: Instructors with special characters in cardnumber cannot be removed from course reserves (5.59 KB, patch)
2018-10-26 20:12 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 21349: Instructors with special characters in cardnumber cannot be removed from course reserves (5.66 KB, patch)
2018-10-28 00:27 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Phetteplace 2018-09-14 22:51:23 UTC
To recreate:

- create a patron with a barcode that contains a character that is meaningful for CSS/jQuery selectors: comma, $, ., :, [, ], =, or @
- create a course and add them to it as an instructor
- on /cgi-bin/koha/course_reserves/course.pl?course_id={{course_id}} click the "remove" link attempting to remove them from the course
- ...nothing happens

The source of this issue is the RemoveInstructor JavaScript function in koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course.tt - it accepts a single "cardnumber" parameter which isn't sanitized before being used in a jQuery selector. Therefore if punctuation that's meaningful in a selector appears in a patron's barcode, the function throws an error. Here's an actual example from our Koha instance:

Uncaught Error: Syntax error, unrecognized expression: #borrower_PCAC000029831$
    at Function.fa.error (jquery-2.2.3.min.js:2)
    at fa.tokenize (jquery-2.2.3.min.js:2)
    at fa.select (jquery-2.2.3.min.js:2)
    at Function.fa [as find] (jquery-2.2.3.min.js:2)
    at n.fn.init.find (jquery-2.2.3.min.js:2)
    at n.fn.init.a.fn.find (jquery-migrate-1.3.0.min.js:2)
    at a.fn.init.n.fn.init (jquery-2.2.3.min.js:2)
    at new a.fn.init (jquery-migrate-1.3.0.min.js:2)
    at n (jquery-2.2.3.min.js:2)
    at RemoveInstructor (course.pl?course_id=39:224)

The solution to this is to sanitize the cardnumber before using it, as described by this article for instance: https://learn.jquery.com/using-jquery-core/faq/how-do-i-select-an-element-by-an-id-that-has-characters-used-in-css-notation/

I'll send along a patch shortly.
Comment 1 Eric Phetteplace 2018-09-14 23:17:10 UTC
Created attachment 78835 [details] [review]
Fix RemoveInstructor function
Comment 2 Mark Tompsett 2018-09-18 18:22:51 UTC
The patch submitted by Eric Phetteplace applies, but does not solve the problem.
I created a user with the barcode of "Ka-boom:Ka-boom!KABOOM@#$!",
and was not able to remove the user.
Comment 3 Mark Tompsett 2018-09-18 18:23:18 UTC
Comment on attachment 78835 [details] [review]
Fix RemoveInstructor function

Counter patch incoming.
Comment 4 Mark Tompsett 2018-09-18 18:33:06 UTC
Created attachment 79057 [details] [review]
Bug 21349: Change cardnumber to borrowernumber

The cardnumber can contain strange non-javascript-friendly
characters. This patch converts all the internal references
to borrowernumber, but displays the cardnumber when showing
list of matching users.

TEST PLAN
---------
1) Make sure Course Reserves is being used. Check system preferences.
2) Make sure there is a Course entered.
3) Make sure there is a patron with an cardnumber which breaks
   the existing javascript (eg. Ka-boom:Ka-boom!KABOOM@#$!)
4) Go into the course list.
5) Click the course name link
6) In the "Instructor Search" box enter the patron name.
7) Select the patron which has the ugly cardnumber.
8) Click Save
   -- The course listing should have the new teacher listed.
9) Click the course name link again.
10) Click 'Remove' beside the instructor's name.
    -- nothing happens, and inspecting the error console gives
       you the ugly error described in comment #0
11) apply this patch
12) restart_all
13) sudo service apache2 restart
14) refresh the page.
15) click 'Remove' beside the instructor's name.
    -- name is removed.
16) Click Save
    -- the course listing no longer lists the new teacher.
17) Actually confirm that adding and removing others normally
    works as expected.
18) run koha qa test tools.
Comment 5 Pierre-Marc Thibault 2018-09-21 19:33:55 UTC
An error occured when I tried to apply the patch. Can you check the patch to fix it please? Thank you.  

Applying: Bug 21349: Change cardnumber to borrowernumber
fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course.tt).
error: could not build fake ancestor
Patch failed at 0001 Bug 21349: Change cardnumber to borrowernumber
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-21349-Change-cardnumber-to-borrowernumber-6lnMN1.patch
Comment 6 Mark Tompsett 2018-09-21 21:58:26 UTC
Created attachment 79263 [details] [review]
Bug 21349: Instructors with special characters in cardnumber cannot be removed from course reserves

The cardnumber can contain strange non-javascript-friendly
characters. This patch converts all the internal references
to borrowernumber, but displays the cardnumber when showing
list of matching users.

TEST PLAN
---------
1) Make sure Course Reserves is being used. Check system preferences.
2) Make sure there is a Course entered.
3) Make sure there is a patron with an cardnumber which breaks
   the existing javascript (eg. Ka-boom:Ka-boom!KABOOM@#$!)
4) Go into the course list.
5) Click the course name link
6) In the "Instructor Search" box enter the patron name.
7) Select the patron which has the ugly cardnumber.
8) Click Save
   -- The course listing should have the new teacher listed.
9) Click the course name link again.
10) Click 'Remove' beside the instructor's name.
    -- nothing happens, and inspecting the error console gives
       you the ugly error described in comment #0
11) apply this patch
12) restart_all
13) sudo service apache2 restart
14) refresh the page.
15) click 'Remove' beside the instructor's name.
    -- name is removed.
16) Click Save
    -- the course listing no longer lists the new teacher.
17) Actually confirm that adding and removing others normally
    works as expected.
18) run koha qa test tools.
Comment 7 Charles Farmer 2018-10-26 20:12:42 UTC
Created attachment 81338 [details] [review]
Bug 21349: Instructors with special characters in cardnumber cannot be removed from course reserves

The cardnumber can contain strange non-javascript-friendly
characters. This patch converts all the internal references
to borrowernumber, but displays the cardnumber when showing
list of matching users.

TEST PLAN
---------
1) Make sure Course Reserves is being used. Check system preferences.
2) Make sure there is a Course entered.
3) Make sure there is a patron with an cardnumber which breaks
   the existing javascript (eg. Ka-boom:Ka-boom!KABOOM@#$!)
4) Go into the course list.
5) Click the course name link
6) In the "Instructor Search" box enter the patron name.
7) Select the patron which has the ugly cardnumber.
8) Click Save
   -- The course listing should have the new teacher listed.
9) Click the course name link again.
10) Click 'Remove' beside the instructor's name.
    -- nothing happens, and inspecting the error console gives
       you the ugly error described in comment #0
11) apply this patch
12) restart_all
13) sudo service apache2 restart
14) refresh the page.
15) click 'Remove' beside the instructor's name.
    -- name is removed.
16) Click Save
    -- the course listing no longer lists the new teacher.
17) Actually confirm that adding and removing others normally
    works as expected.
18) run koha qa test tools.

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Comment 8 Katrin Fischer 2018-10-28 00:27:18 UTC
Created attachment 81383 [details] [review]
Bug 21349: Instructors with special characters in cardnumber cannot be removed from course reserves

The cardnumber can contain strange non-javascript-friendly
characters. This patch converts all the internal references
to borrowernumber, but displays the cardnumber when showing
list of matching users.

TEST PLAN
---------
1) Make sure Course Reserves is being used. Check system preferences.
2) Make sure there is a Course entered.
3) Make sure there is a patron with an cardnumber which breaks
   the existing javascript (eg. Ka-boom:Ka-boom!KABOOM@#$!)
4) Go into the course list.
5) Click the course name link
6) In the "Instructor Search" box enter the patron name.
7) Select the patron which has the ugly cardnumber.
8) Click Save
   -- The course listing should have the new teacher listed.
9) Click the course name link again.
10) Click 'Remove' beside the instructor's name.
    -- nothing happens, and inspecting the error console gives
       you the ugly error described in comment #0
11) apply this patch
12) restart_all
13) sudo service apache2 restart
14) refresh the page.
15) click 'Remove' beside the instructor's name.
    -- name is removed.
16) Click Save
    -- the course listing no longer lists the new teacher.
17) Actually confirm that adding and removing others normally
    works as expected.
18) run koha qa test tools.

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Nick Clemens 2018-10-29 01:19:15 UTC
Awesome work all!

Pushed to master for 18.11
Comment 10 Martin Renvoize 2018-11-08 15:46:03 UTC
Pushed to 18.05.x for 18.05.06
Comment 11 Fridolin Somers 2018-11-28 11:06:27 UTC
Pushed to 17.11.x for 17.11.13