Bug 26102 prevented XSS via malicious authority records (which would have been fun to exploit), but it did it by creating an HTML entity-encoded string and then handing it to a function which expected to get text, not HTML. As a result, if you look up the authority record for Simon & Schuster Audio (Firm) from the advanced editor, you wind up putting Simon & Schuster Audio (Firm) in your bib record. Steps to reproduce: 1. Set EnableAdvancedCatalogingEditor to Enable 2. Edit any Topical Term authority record, and at the end of 150 subfield a add (without the newlines, just offsetting it for easy copying) & Stuff </script><script>alert('boo ❤')</script> 3. Cataloging - Advanced Editor 4. Hit return in the editor to get a new line, type 650 and press tab three times, then Ctrl+Shift+L 5. Search for the authority you edited, click Choose Expected result: The editor should show 650 _ 4 ‡aAbduction & Stuff </script><script>alert('boo ❤')</script>‡vDrama. since that's the text of the authority you selected Actual result: The editor shows 650 _ 4 ‡aAbduction & Stuff </script> <script>alert('boo ❤');</script>‡vDrama.
Created attachment 147776 [details] [review] Bug 33144: Unescape text from authority lookup for advanced editor While the basic editor is happy with an array of subfields it can stuff into separate fields, the advanced editor needs to get a JS string back from the authority lookup plugin, because it is going to just add the whole thing as text. The string has to be HTML entity encoded, both to not allow XSS and just to not break the window, but it needs to then be unencoded before being inserted into the editor. Test plan: 1. Set the system preference EnableAdvancedCatalogingEditor to Enable 2. Edit any Topical Term authority, and at the end of tag 150 subfield a, add & </script> 3. Cataloging - Advanced editor 4. Press return in the editor to get a new blank line, type 650 and press tab three times, then type Ctrl-Shift-L 5. Search for your modified authority, and click Choose 6. Verify that the tiny popup opened by the search window finished its job and closed itself 7. Verify that your 650 now shows as "‡aAbduction & </script>‡vDrama" rather than "‡aAbduction & </script>‡vDrama."
I'm no expert in assessing the XSS risks of code copy-pasted from Stack Overflow, but I've been reading Wladimir's writing on security and running his code for years, so I'd trust https://stackoverflow.com/questions/1912501/unescape-html-entities-in-javascript/34064434#34064434 and in fact we already do that same thing in https://git.koha-community.org/Koha-community/Koha/src/commit/fe872b792037ee2f8cce0d25f95c1e4f739ffe49/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt#L820
Created attachment 147817 [details] [review] Bug 33144: Unescape text from authority lookup for advanced editor While the basic editor is happy with an array of subfields it can stuff into separate fields, the advanced editor needs to get a JS string back from the authority lookup plugin, because it is going to just add the whole thing as text. The string has to be HTML entity encoded, both to not allow XSS and just to not break the window, but it needs to then be unencoded before being inserted into the editor. Test plan: 1. Set the system preference EnableAdvancedCatalogingEditor to Enable 2. Edit any Topical Term authority, and at the end of tag 150 subfield a, add & </script> 3. Cataloging - Advanced editor 4. Press return in the editor to get a new blank line, type 650 and press tab three times, then type Ctrl-Shift-L 5. Search for your modified authority, and click Choose 6. Verify that the tiny popup opened by the search window finished its job and closed itself 7. Verify that your 650 now shows as "‡aAbduction & </script>‡vDrama" rather than "‡aAbduction & </script>‡vDrama." Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 148038 [details] [review] Bug 33144: Unescape text from authority lookup for advanced editor While the basic editor is happy with an array of subfields it can stuff into separate fields, the advanced editor needs to get a JS string back from the authority lookup plugin, because it is going to just add the whole thing as text. The string has to be HTML entity encoded, both to not allow XSS and just to not break the window, but it needs to then be unencoded before being inserted into the editor. Test plan: 1. Set the system preference EnableAdvancedCatalogingEditor to Enable 2. Edit any Topical Term authority, and at the end of tag 150 subfield a, add & </script> 3. Cataloging - Advanced editor 4. Press return in the editor to get a new blank line, type 650 and press tab three times, then type Ctrl-Shift-L 5. Search for your modified authority, and click Choose 6. Verify that the tiny popup opened by the search window finished its job and closed itself 7. Verify that your 650 now shows as "‡aAbduction & </script>‡vDrama" rather than "‡aAbduction & </script>‡vDrama." Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Pushed to master for 23.05. Nice work everyone, thanks!
Nice work, thanks everyone! Pushed to 22.11.x for the next release.
Backported to 22.05.x for upcoming 22.05.12
applied to 21.11.x for 21.11.20
Not backported to 21.05.x