Bug 40538

Summary: XSS in hold suspend modal in staff interface
Product: Koha Reporter: David Cook <dcook>
Component: CirculationAssignee: David Cook <dcook>
Status: Passed QA --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: minor    
Priority: P5 - low CC: dcook, didier.gautheron, fridolin.somers, gmcharlt, jesse, jonathan.druart, katrin.fischer, kyle, laurent.ducos, lisette, lucas, m.de.rooy, martin.renvoize, nick, paul.derscheid, pedro.amorim, phil, philippe.blouin, tomascohen, wainuiwitikapark
Version: MainKeywords: release-notes-needed
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: Small patch Documentation contact:
Documentation submission: Text to go in the release notes:
Fixes XSS vulnerability in suspend hold modal and suspend hold button by refactoring the Javascript that creates the HTML.
Version(s) released in:
25.05.03,24.11.08,24.05.13,22.11.30
Circulation function:
Attachments: Bug 40538: Fix XSS in suspend hold modal
Bug 40538: Fix XSS in suspend hold modal
Bug 40538: Simplify code
Bug 40538: Fix XSS in suspend hold modal
Bug 40538: Simplify code
Bug 40538: Fix XSS in suspend hold modal
Bug 40538: Simplify code
[22.11.x] Bug 40538: Fix XSS in suspend hold modal
[24.05.x] Bug 40538: Fix XSS in suspend hold modal

Description David Cook 2025-07-29 03:21:25 UTC
There is a XSS vulnerability in the staff interface in the hold suspend modal found on /cgi-bin/koha/members/moremember.pl and /cgi-bin/koha/circ/circulation.pl
Comment 1 David Cook 2025-07-29 03:22:21 UTC
Pedro, this is the one I mentioned via DM.
Comment 3 David Cook 2025-07-29 04:09:32 UTC
Created attachment 184775 [details] [review]
Bug 40538: Fix XSS in suspend hold modal

This patch fixes a XSS vulnerability in the
suspend hold modal in the staff interface.

By building our Javascript using Web APIs rather
than with strings and jQuery's html() method,
we're able to securely handle malicious payloads.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/reserve/request.pl?biblionumber=29
2. Add a hold for "koha" patron
3. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51
4. Click "Holds" tab and click "Suspend"
5. Note the modal loads properly
6. Go to http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51
7. Click "Holds" tab and click "Suspend"
8. Note the modal loads properly
Comment 5 Pedro Amorim 2025-07-29 10:10:53 UTC
Created attachment 184790 [details] [review]
Bug 40538: Fix XSS in suspend hold modal

This patch fixes a XSS vulnerability in the
suspend hold modal in the staff interface.

By building our Javascript using Web APIs rather
than with strings and jQuery's html() method,
we're able to securely handle malicious payloads.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/reserve/request.pl?biblionumber=29
2. Add a hold for "koha" patron
3. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51
4. Click "Holds" tab and click "Suspend"
5. Note the modal loads properly
6. Go to http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51
7. Click "Holds" tab and click "Suspend"
8. Note the modal loads properly

Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Comment 6 Jonathan Druart 2025-07-29 10:40:12 UTC
Created attachment 184792 [details] [review]
Bug 40538: Simplify code
Comment 7 Jonathan Druart 2025-07-29 10:40:34 UTC
I guess this is personal preference but I always think it's hard to read JS code that generates HTML elements. I've tried to simplify a bit the code here, feel free to drop the patch if you disagree.

Also confirm we can replace

link.insertAdjacentElement(
    "afterbegin",
    icon
)

with 

link.prepend(icon);
Comment 8 David Cook 2025-07-30 00:48:12 UTC
(In reply to Jonathan Druart from comment #7)
> I guess this is personal preference but I always think it's hard to read JS
> code that generates HTML elements. I've tried to simplify a bit the code
> here, feel free to drop the patch if you disagree.

I think that you're right about it being personal preference, as I like the line by line approach. But I'm OK with the approach you've taken too. I can see why you like it better.

You spend a lot of time on Koha, so I'm happy to move forward with that syntax.

> Also confirm we can replace
> 
> link.insertAdjacentElement(
>     "afterbegin",
>     icon
> )
> 
> with 
> 
> link.prepend(icon);

Yep, that looks good. insertAdjacentElement is certainly verbose haha. In general, I like it for its flexibility, but here prepend() makes more sense.
Comment 9 Marcel de Rooy 2025-08-01 07:31:02 UTC
Created attachment 184969 [details] [review]
Bug 40538: Fix XSS in suspend hold modal

This patch fixes a XSS vulnerability in the
suspend hold modal in the staff interface.

By building our Javascript using Web APIs rather
than with strings and jQuery's html() method,
we're able to securely handle malicious payloads.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/reserve/request.pl?biblionumber=29
2. Add a hold for "koha" patron
3. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51
4. Click "Holds" tab and click "Suspend"
5. Note the modal loads properly
6. Go to http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51
7. Click "Holds" tab and click "Suspend"
8. Note the modal loads properly

Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2025-08-01 07:31:06 UTC
Created attachment 184970 [details] [review]
Bug 40538: Simplify code

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 David Cook 2025-08-04 02:04:12 UTC
@Jonathan, I just backported this one to my local Koha, and the Object.assign() is growing on me!
Comment 12 Wainui Witika-Park 2025-08-25 02:56:33 UTC
Patch doesn't apply cleanly on 22.11.x - please can I get a rebase?
Comment 13 Paul Derscheid 2025-08-25 11:54:38 UTC
Created attachment 185739 [details] [review]
Bug 40538: Fix XSS in suspend hold modal

This patch fixes a XSS vulnerability in the
suspend hold modal in the staff interface.

By building our Javascript using Web APIs rather
than with strings and jQuery's html() method,
we're able to securely handle malicious payloads.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/reserve/request.pl?biblionumber=29
2. Add a hold for "koha" patron
3. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51
4. Click "Holds" tab and click "Suspend"
5. Note the modal loads properly
6. Go to http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51
7. Click "Holds" tab and click "Suspend"
8. Note the modal loads properly

Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Paul Derscheid 2025-08-25 11:54:41 UTC
Created attachment 185740 [details] [review]
Bug 40538: Simplify code

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Paul Derscheid 2025-08-25 11:55:24 UTC
Rebased on current upstream/main.
Comment 16 Paul Derscheid 2025-08-25 12:02:17 UTC
Created attachment 185741 [details] [review]
[22.11.x] Bug 40538: Fix XSS in suspend hold modal

This patch fixes a XSS vulnerability in the
suspend hold modal in the staff interface.

By building our Javascript using Web APIs rather
than with strings and jQuery's html() method,
we're able to securely handle malicious payloads.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/reserve/request.pl?biblionumber=29
2. Add a hold for "koha" patron
3. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51
4. Click "Holds" tab and click "Suspend"
5. Note the modal loads properly
6. Go to http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51
7. Click "Holds" tab and click "Suspend"
8. Note the modal loads properly

Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 40538: Simplify code

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Paul Derscheid 2025-08-25 12:02:43 UTC
Added a squashed version for 22.11.x.
Comment 18 David Cook 2025-08-26 01:51:00 UTC
(In reply to Paul Derscheid from comment #17)
> Added a squashed version for 22.11.x.

You're a legend, Paul.
Comment 19 Wainui Witika-Park 2025-08-26 02:12:58 UTC
(In reply to Paul Derscheid from comment #17)
> Added a squashed version for 22.11.x.

Thanks so much!!

Applied to 22.11.x-security for 22.11.30
Comment 20 Jesse Maseto 2025-08-26 14:00:05 UTC
This doesn't apply to 24.05.x. Merge conflicts. Please resolve.
Comment 21 Paul Derscheid 2025-08-26 15:53:52 UTC
Created attachment 185795 [details] [review]
[24.05.x] Bug 40538: Fix XSS in suspend hold modal

This patch fixes a XSS vulnerability in the
suspend hold modal in the staff interface.

By building our Javascript using Web APIs rather
than with strings and jQuery's html() method,
we're able to securely handle malicious payloads.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/reserve/request.pl?biblionumber=29
2. Add a hold for "koha" patron
3. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51
4. Click "Holds" tab and click "Suspend"
5. Note the modal loads properly
6. Go to http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51
7. Click "Holds" tab and click "Suspend"
8. Note the modal loads properly

Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 40538: Simplify code

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Paul Derscheid 2025-08-26 15:54:33 UTC
Added another one for 24.05.x. It's probably identical to squash 22.11.x one, but just to be certain.
Comment 23 Paul Derscheid 2025-08-27 08:53:41 UTC
Applied to 25.05.x-security for 25.05.03.