Bug 31219

Summary: Patron attribute types not cleaned/checked
Product: Koha Reporter: Mark Hofstetter <koha>
Component: PatronsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: critical    
Priority: P5 - low CC: 1joynelson, dcook, didier.gautheron, fridolin.somers, gmcharlt, jonathan.druart, kyle.m.hall, laurent.ducos, lucas, m.de.rooy, martin.renvoize, nick, tomascohen, victor
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00,21.11.14,21.05.20
Circulation function:
Attachments: Bug 31219: Prevent JS injection in patron extended attributes
Bug 31219: Prevent JS injection in patron extended attributes

Description Mark Hofstetter 2022-07-22 08:50:05 UTC
if you create a "new Patron attribute type" and make it editable via the opac interface, the user input is not checked for js injection.

Hence js code may be executed in the admin interface
Comment 1 Jonathan Druart 2022-07-25 07:26:16 UTC
Created attachment 138072 [details] [review]
Bug 31219: Prevent JS injection in patron extended attributes

We are sanitizing other attributes but "extended patron attributes".

Test plan:
Make a patron attribute editable at the OPAC
Edit an existing patron, or register a new one
Use a script tag in the new value ("<script>alert("booh!")</script>" for
instance)
With this patch the value is remove if containing an HTML tag that is
not br b i em big small strong (see C4::Scrubber)
Comment 2 Jonathan Druart 2022-07-25 07:26:52 UTC
We could certainly do better here. But not sure we could with a 2-lines patch.
Comment 3 Mark Hofstetter 2022-07-25 12:07:11 UTC
I did a quick test and it worked as expected
Comment 4 David Cook 2022-07-26 00:07:04 UTC
I'll add this to my list to check out. Should be a good one to review...
Comment 5 Katrin Fischer 2022-07-28 21:33:38 UTC
Even without this patch, the code won't be executed when moderating the change requests, but only after accepting the changes.
Comment 6 Katrin Fischer 2022-07-28 21:36:19 UTC
At the moment, if all that was added is code, the empty field will still be pushed, even if it means 'no change'. Maybe this could be improved a little, but this is still an improvement already.
Comment 7 Katrin Fischer 2022-07-28 21:38:21 UTC
Created attachment 138234 [details] [review]
Bug 31219: Prevent JS injection in patron extended attributes

We are sanitizing other attributes but "extended patron attributes".

Test plan:
Make a patron attribute editable at the OPAC
Edit an existing patron, or register a new one
Use a script tag in the new value ("<script>alert("booh!")</script>" for
instance)
With this patch the value is remove if containing an HTML tag that is
not br b i em big small strong (see C4::Scrubber)

Signed-off-by: Mark Hofstetter <koha@trust-box.at>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Victor Grousset/tuxayo 2022-10-24 16:04:30 UTC
Backported: Pushed to 21.05.x branch for the upcoming release (21.05.20)
Comment 17 Fridolin Somers 2023-03-24 19:01:42 UTC
pushed to oldoldstable right ?