Bug 33144 - Authority lookup in advanced editor overencodes HTML
Summary: Authority lookup in advanced editor overencodes HTML
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Phil Ringnalda
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 26102
Blocks:
  Show dependency treegraph
 
Reported: 2023-03-06 03:11 UTC by Phil Ringnalda
Modified: 2023-11-01 00:04 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.04,22.05.12,21.11.20


Attachments
Bug 33144: Unescape text from authority lookup for advanced editor (2.44 KB, patch)
2023-03-06 05:47 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 33144: Unescape text from authority lookup for advanced editor (2.51 KB, patch)
2023-03-07 09:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33144: Unescape text from authority lookup for advanced editor (2.57 KB, patch)
2023-03-09 22:16 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 Phil Ringnalda 2023-03-06 03:11:15 UTC
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 &amp; Stuff &lt;/script&gt; &lt;script&gt;alert('boo ❤');&lt;/script&gt;‡vDrama.
Comment 1 Phil Ringnalda 2023-03-06 05:47:47 UTC
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  &amp;
   &lt;/script&gt;‡vDrama."
Comment 2 Phil Ringnalda 2023-03-06 05:48:34 UTC
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
Comment 3 Jonathan Druart 2023-03-07 09:31:39 UTC
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  &amp;
   &lt;/script&gt;‡vDrama."

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 4 Katrin Fischer 2023-03-09 22:16:46 UTC
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  &amp;
   &lt;/script&gt;‡vDrama."

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 5 Tomás Cohen Arazi 2023-03-14 12:10:41 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 6 Jacob O'Mara 2023-03-16 19:49:17 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 7 Lucas Gass 2023-04-14 18:12:36 UTC
Backported to 22.05.x for upcoming 22.05.12
Comment 8 Arthur Suzuki 2023-04-20 09:16:13 UTC
applied to 21.11.x for 21.11.20
Comment 9 wainuiwitikapark 2023-04-27 05:58:55 UTC
Not backported to 21.05.x