Bug 31219 - Patron attribute types not cleaned/checked
Summary: Patron attribute types not cleaned/checked
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-07-22 08:50 UTC by Mark Hofstetter
Modified: 2024-08-15 13:16 UTC (History)
14 users (show)

See Also:
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 (1.54 KB, patch)
2022-07-25 07:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31219: Prevent JS injection in patron extended attributes (1.64 KB, patch)
2022-07-28 21:38 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 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 ?