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
Pedro, this is the one I mentioned via DM.
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
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>
Created attachment 184792 [details] [review] Bug 40538: Simplify code
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);
(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.
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>
Created attachment 184970 [details] [review] Bug 40538: Simplify code Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
@Jonathan, I just backported this one to my local Koha, and the Object.assign() is growing on me!
Patch doesn't apply cleanly on 22.11.x - please can I get a rebase?
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>
Created attachment 185740 [details] [review] Bug 40538: Simplify code Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Rebased on current upstream/main.
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>
Added a squashed version for 22.11.x.
(In reply to Paul Derscheid from comment #17) > Added a squashed version for 22.11.x. You're a legend, Paul.
(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
This doesn't apply to 24.05.x. Merge conflicts. Please resolve.
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>
Added another one for 24.05.x. It's probably identical to squash 22.11.x one, but just to be certain.
Applied to 25.05.x-security for 25.05.03.