Suppose as the value of OPACSearchForTitleIn preference, you provide something like: ``` [...] <a href="https://openlibrary.org/search?author=({AUTHOR})&title=({TITLE})" target="_blank">Open Library (openlibrary.org)</a> <script> function test() { console.log("test!"); } test(); </script> ``` you will actually end up with: ``` <script> function test() test(); </script> ``` It seems that whatever parses the template arguments within {...} does so without consideration about possible other use of these brackets. What should actually happen, is that what's inside of brackets should only be replaced if it's a valid param such as {AUTHOR} etc, otherwise the argument should probably be ignored (or at the very least keep current behavior only if it passes some regex like \{[A-Z_]+\}... It actually looks like the removal was explicit: https://github.com/Koha-Community/Koha/blob/8ff9e665e4d275ac601ee5165ab4233a14189f2c/C4/Output.pm#L411 Meaning that to keep this behavior, it'd be best to adjust the regex to be more conservative. Either that, or just junk that line altogether...
Created attachment 170094 [details] [review] Bug 37573: Fix bad escaping in OPACSearchForTitleIn that broke JavaScript in <script> tags in it The helper Perl function parametrized_url that converted parameters within {} would actually remove everything between such characters, regardless of whether they were a correct parameter or any arbitrary text. The patch changes it to remove only something that would look like a parameter, so that it won't break other unrelated things, while keeping the original intended behavior.
Hi Michał. Thanks for the patch! Could you provide a test plan (see https://wiki.koha-community.org/wiki/Commit_messages#Test_plan) along the lines of: 1. How to replicate the issue (where it is a bug fix). 2. Apply the patch, update the database, and so on. 3. Demonstrate that the issue is fixed (where it is a bug fix). 4. Add a release note that librarians/library staff can understand what the change is (we generally have three types: a bug fix, enhancement, or a new feature) - This fixes..., This enhancement does XYZ..., This new feature does ABC... Thanks. David
This does sound like a bug, but personally I wouldn't want OPACSearchForTitleIn to be able to inject <script></script> tags either...
This fix also seems like a bit of a hack still, as it means your link text couldn't be "Awesome {Library}". Single braces really aren't great for template tokens, but too late to change that now. I reckon Owen's idea at bug 36805 is a good way to go. (Going back to the Javascript thing, really that should be in OpacUserJS. Also, one day in the future with Content-Security-Policy, this type of script injection won't work anymore. The script execution will be blocked by the browser.)
> (Going back to the Javascript thing, really that should be in OpacUserJS. Also, one day in the future with Content-Security-Policy, this type of script injection won't work anymore. The script execution will be blocked by the browser.) The thing is, as it stands, all the HTML fields/preferences in Koha allow using JavaScript, it goes way beyond OpacUserJS. Even the HTML templates for various parts of OPAC etc. In the big picture I kinda agree, but this will require changes all throughout Koha, with proper announcement as it's sure to break some existing setups. But for now we should fix the bug and inconsistency that happens today unintentionally. > This does sound like a bug, but personally I wouldn't want OPACSearchForTitleIn to be able to inject <script></script> tags either... I should share my use case. I have a tag like this: <a href="https://worldcat.org/search?q={TITLE}" target="_blank">Other Libraries (WorldCat)<script>var openInWorldCatCurScript = document.currentScript;</script></a> <!-- document.currentScript.parentElement will then be our <a> tag --> And at the bottom (it doesn't set up the event listener right away, as we have two searches like this, so to avoid double API calls): <script> document.addEventListener('DOMContentLoaded', () => processOPACSearchForTitleIn({BIBLIONUMBER}, openInWorldCatCurScript)); </script> The JS func that's called accesses the MARC with the public API and extracts info such as direct WorldCat id if it exists, and then swaps the generic title search URL to a direct link to the record. I realize now that is kinda weird and technically I could've just used "id" attribute in HTML, since it won't be duplicated in the DOM due to nature of Koha and XHR loading not being used in this particular place. But... > I reckon Owen's idea at bug 36805 is a good way to go. I kind of have another issue with this. Replacing this to be a very simple list of URLs would limit people to just the currently provided minimal templates, or they'd have to resort to using awkward CSS selectors in the JS. I guess it could work if one used TT templates only... To keep it being featureful, one should have access to any fields from MARC and conditional variables there. For example, with the search above in WorldCat, we want to change what kind of URL is shown based on whether custom field in MARC with the ID exists (we keep it somewhere in 9xx unfortunately, not in the standard field...). And in the second case, we want to see if a field exists in MARC and only then show the particular search URL. So the only ways to achieve it all would be either with JS, or with TT templates with full access to the MARC data of the record so that it wouldn't have to be fetched from REST API...
Test plan: 1. Go to settings and in pref OPACSearchForTitleIn enter this: <script> function test() { console.log("test!"); } test(); </script> 2. Go to OPAC and open some biblio record's page. 3. Open dev tools console (Ctrl+Shift+I). Before applying the patch you should see a syntax error, after applying it you should see "test!" logged. > This fix also seems like a bit of a hack still, as it means your link text couldn't be "Awesome {Library}". Single braces really aren't great for template tokens, but too late to change that now. I think that's just a matter of adjusting the regex. Allowing lower-case chars would be relatively reasonable if need be. Allowing a bunch of text that's a body of JS code block, constituting of other characters like semicolons and newlines: probably not.
(In reply to Michał from comment #5) > The thing is, as it stands, all the HTML fields/preferences in Koha allow > using JavaScript, it goes way beyond OpacUserJS. Even the HTML templates for > various parts of OPAC etc. > > In the big picture I kinda agree, but this will require changes all > throughout Koha, with proper announcement as it's sure to break some > existing setups. Yep, it's a work in progress :).
(In reply to Michał from comment #5) > I should share my use case. > > I realize now that is kinda weird and technically I could've just used "id" > attribute in HTML, since it won't be duplicated in the DOM due to nature of > Koha and XHR loading not being used in this particular place. It is kinda weird ;). Yeah, you could use an ID instead and just use OpacUserJS. Interesting point about the XHR loading (like in the staff interface holdings list). Ways and ways with that probably although we might need to look at adding hooks there...