To recreate: 1 - Set borrower surname to: <script>alert("here comes trouble");</script> 2 - Save, nothing happens 3 - Enable showLastPatron 4 - Reload patron 5 - Here comes trouble indeed
Created attachment 163615 [details] [review] Bug 36382: XSS in showLastPatron dropdown 1) Set borrower surname to: <script>alert("here comes trouble");</script> 2) Save, nothing happens 3) Enable showLastPatron 4) Reload patron 5) Note the alert popup 6) Apply this patch 7) Reload patron 8) No alert!
Created attachment 163727 [details] [review] Bug 36382: XSS in showLastPatron dropdown 1) Set borrower surname to: <script>alert("here comes trouble");</script> 2) Save, nothing happens 3) Enable showLastPatron 4) Reload patron 5) Note the alert popup 6) Apply this patch 7) Reload patron 8) No alert! Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 164119 [details] [review] Bug 36382: XSS in showLastPatron dropdown 1) Set borrower surname to: <script>alert("here comes trouble");</script> 2) Save, nothing happens 3) Enable showLastPatron 4) Reload patron 5) Note the alert popup 6) Apply this patch 7) Reload patron 8) No alert! Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
+ "'": ''', + '"': '"' Do you really estimate the possible regression from this? maybe we are already escaping them in some places and they will get double escaped.
(In reply to Jonathan Druart from comment #4) > + "'": ''', > + '"': '"' > > Do you really estimate the possible regression from this? maybe we are > already escaping them in some places and they will get double escaped. Good point
Created attachment 164123 [details] [review] Bug 36382 (QA follow-up) Don't escape quotes in escapeHtml
(In reply to Jonathan Druart from comment #4) > + "'": ''', > + '"': '"' > > Do you really estimate the possible regression from this? maybe we are > already escaping them in some places and they will get double escaped. Where are we escaping quotes? A grep grep found function encodeHTMLEntities in supplier.tt. HTML escaping really needs to be unified, making bug 36385 all the more important.
(In reply to Kyle M Hall from comment #7) > Where are we escaping quotes? A grep grep found function encodeHTMLEntities > in supplier.tt. HTML escaping really needs to be unified, making bug 36385 > all the more important. +1
Also, part of the reason we have XSS issues like these is because we work with the DOM using strings rather than objects. Take this particular XSS: const el = `<li><a href="/cgi-bin/koha/circ/circulation.pl?borrowernumber=${p["borrowernumber"]}">${p["name"]} (${p["card"]})</a></li>`; Here we could be using a "Safe Sink"[1] for the name and card to eliminate the XSS. const el = document.createElement('li'); const a_el = document.createElement('a'); a_el.setAttribute('href',`/cgi-bin/koha/circ/circulation.pl?borrowernumber=${p["borrowernumber"]}`); const link_text = document.createTextNode(`${p["name"]} (${p["card"]})`); a_el.appendChild(link_text); el.appendChild(a_el); [1] https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html#safe-sinks -- Yes, it is more verbose, but it's way safer. It also doesn't need an escape_str. (Not that I think escape_str is bad. The more layers of protection the better I always say.)
(In reply to David Cook from comment #9) > Here we could be using a "Safe Sink"[1] for the name and card to eliminate > the XSS. Bug 35960 has a clever way of using jquery and strings to create objects without any user input, and then using a safe sink for the user input. So there could be a balance in terms of verbosity as well.
Applying: Bug 36482: Allow embedding desks and cash_registers on libraries endpoints error: sha1 information is lacking or useless (api/v1/swagger/paths/libraries.yaml).
I can't recreate the bug on 22.05 - I also can't apply the patch cleanly
(In reply to wainuiwitikapark from comment #12) > I can't recreate the bug on 22.05 - I also can't apply the patch cleanly Same on 22.11: previous_patrons doesn't exist on this branch
(In reply to wainuiwitikapark from comment #12) > I can't recreate the bug on 22.05 - I also can't apply the patch cleanly I believe the issue was introduced in 23.11.00, so that is ok!
(In reply to Kyle M Hall from comment #14) > (In reply to wainuiwitikapark from comment #12) > > I can't recreate the bug on 22.05 - I also can't apply the patch cleanly > > I believe the issue was introduced in 23.11.00, so that is ok! Ok cool thanks Kyle So 22.05, 22.11 and 23.05 can ignore backporting this?
(In reply to wainuiwitikapark from comment #15) > (In reply to Kyle M Hall from comment #14) > > (In reply to wainuiwitikapark from comment #12) > > > I can't recreate the bug on 22.05 - I also can't apply the patch cleanly > > > > I believe the issue was introduced in 23.11.00, so that is ok! > > Ok cool thanks Kyle > > So 22.05, 22.11 and 23.05 can ignore backporting this? That is correct!
Created attachment 166717 [details] [review] Bug 36382: XSS in showLastPatron dropdown 1) Set borrower surname to: <script>alert("here comes trouble");</script> 2) Save, nothing happens 3) Enable showLastPatron 4) Reload patron 5) Note the alert popup 6) Apply this patch 7) Reload patron 8) No alert! Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 166718 [details] [review] Bug 36382: (QA follow-up) Don't escape quotes in escapeHtml Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Rebased after a few hundred commits pushed to main.
Pushed for 24.05! Well done everyone, thank you!
Bug 21246 since 23.11 so no backport to older branches