[Bug 37573] New: Bad escaping in OPACSearchForTitleIn breaks JS in <script> tags
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 Bug ID: 37573 Summary: Bad escaping in OPACSearchForTitleIn breaks JS in <script> tags Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: minor Priority: P5 - low Component: OPAC Assignee: oleonard@myacpl.org Reporter: schodkowy.omegi-0r@icloud.com QA Contact: testopia@bugs.koha-community.org 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/8ff9e665e4d275ac601ee5165ab4233a... 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... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 Michał <schodkowy.omegi-0r@icloud.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=9504 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 --- Comment #1 from Michał <schodkowy.omegi-0r@icloud.com> --- Created attachment 170094 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=170094&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 Michał <schodkowy.omegi-0r@icloud.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch CC| |januszop@gmail.com, | |roman.dolny@jezuici.pl Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|oleonard@myacpl.org |schodkowy.omegi-0r@icloud.c | |om -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #2 from David Nind <david@davidnind.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=36805 CC| |dcook@prosentient.com.au --- Comment #3 from David Cook <dcook@prosentient.com.au> --- This does sound like a bug, but personally I wouldn't want OPACSearchForTitleIn to be able to inject <script></script> tags either... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 --- Comment #4 from David Cook <dcook@prosentient.com.au> --- 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.) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 --- Comment #5 from Michał <schodkowy.omegi-0r@icloud.com> ---
(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... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 --- Comment #6 from Michał <schodkowy.omegi-0r@icloud.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 --- Comment #7 from David Cook <dcook@prosentient.com.au> --- (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 :). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 --- Comment #8 from David Cook <dcook@prosentient.com.au> --- (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... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 Michał <schodkowy.omegi-0r@icloud.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |This fixes the inability to release notes| |use JavaScript inside of | |<script> tags in | |OPACSearchForTitleIn | |preference (the code was | |unexpectedly butchered, | |causing syntax errors) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off Patch complexity|Trivial patch |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #170094|0 |1 is obsolete| | --- Comment #9 from Magnus Enger <magnus@libriotech.no> --- Created attachment 173767 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173767&action=edit 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. 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. Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. I added the test plan from Bugzilla to the commit message above. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #10 from David Cook <dcook@prosentient.com.au> --- Marking "Failed QA". Here's the thing: If the goal here was just about fixing the removal of {TOKEN}, then I could see the purpose, but this patch would allow a lot of potential tokens to split through still. But the key problem here is that your intention here is to inject Javascript specifically, and that's a problem. I'm going to open a new bug report which will specifically prevent the injection of Javascript via this system preference. As I noted before, if you want to inject Javascript, the place to do that would be OpacUserJS. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=38321 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 --- Comment #11 from Michał <schodkowy.omegi-0r@icloud.com> --- I don’t think that’s right. If Koha was to limit where scripts are able to execute, it should do it universally and non-discriminatorily across all prefs and other places alike where HTML can be inserted. With the way things are currently, broken greedy token parsing is by all means a bug and unexpected behavior. The coincidental alignment of its practical end effect with one of long-term goals of Koha development shouldn’t stop its fixing… It shouldn’t willingly be kept broken for this reason. JavaScript is not the only thing someone could have there. Imagine someone could want to put a HTML attribute like data-json='{"something":1}' for example. It’s not JavaScript, and props like that are used sometimes, it’s another “safe” use-case that’s being broken.
but this patch would allow a lot of potential tokens to split through still
I mean having them adhere to some common and very specific format unlikely to conflict with random things is infinitely better than parsing that matches anything in between {} and thus breaks stuff.
I'm going to open a new bug report which will specifically prevent the injection of Javascript via this system preference.
I think one should be opened for all HTML system prefs and things like this alike, I don’t see a reason to single out this one specifically (I think there are more, right?). Also not sure if this blockage can really be done properly without using nonces. Naive breaking such as removing {} does not currently prevent JS code without code blocks from running at all. Matching <script> tags also has a lot of caveats. I’m not sure if any temporary solution other than the said eventual planned script execution source limiting via CSP would make much sense. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off Text to go in the|This fixes the inability to |Restrict the removal of {} release notes|use JavaScript inside of |tokens in |<script> tags in |OPACSearchForTitleIn |OPACSearchForTitleIn |preference to only |preference (the code was |underscores and |unexpectedly butchered, |alphanumeric to stop |causing syntax errors) |unintended removal of other | |data including {} | |characters. Summary|Bad escaping in |Restrict |OPACSearchForTitleIn breaks |OPACSearchForTitleIn token |JS in <script> tags |removal to underscore and | |alphanumeric --- Comment #12 from David Cook <dcook@prosentient.com.au> --- (In reply to Michał from comment #11)
I don’t think that’s right. If Koha was to limit where scripts are able to execute, it should do it universally and non-discriminatorily across all prefs and other places alike where HTML can be inserted.
It's a work in progress, but believe me when I say it's coming.
With the way things are currently, broken greedy token parsing is by all means a bug and unexpected behavior. The coincidental alignment of its practical end effect with one of long-term goals of Koha development shouldn’t stop its fixing… It shouldn’t willingly be kept broken for this reason.
I think there's some merit to what you're saying there. In fact, I think that there's an argument to be made for removing the removal of unsupported tokens completely, since it's not really necessary. (We could add the supported tokens to the syspref description and then any unsupported tokens are the library's responsibility to remove.) But your change here would also stop many potential tokens from being removed, which some could say is a regression of the current behaviour.
JavaScript is not the only thing someone could have there. Imagine someone could want to put a HTML attribute like data-json='{"something":1}' for example. It’s not JavaScript, and props like that are used sometimes, it’s another “safe” use-case that’s being broken.
I'd advise against that coding as well. You'd be better off with data-something="1" "data-somethingelse="2". Plus that's a pretty strange use-case for OPACSearchForTitleIn. Keep in mind too bug 36805 which would proably also make that impossible.
I mean having them adhere to some common and very specific format unlikely to conflict with random things is infinitely better than parsing that matches anything in between {} and thus breaks stuff.
I think that's debateable, but tell you what... I'll reset this to "Signed Off". I am going to change the title and release notes to reflect the actual change made.
I'm going to open a new bug report which will specifically prevent the injection of Javascript via this system preference.
I think one should be opened for all HTML system prefs and things like this alike, I don’t see a reason to single out this one specifically (I think there are more, right?).
There is one already for all things, but it's huge and very slow moving. There's a saying in English "the squeaky wheel gets the oil". This preference has become very noticeable (you've specifically mentioned injecting Javascript in it, for instance), and it's typically limited in scope ( as noted on bug 36805 it's typically a list of links ), so theoretically easier to restrict than other sysprefs.
Also not sure if this blockage can really be done properly without using nonces. Naive breaking such as removing {} does not currently prevent JS code without code blocks from running at all. Matching <script> tags also has a lot of caveats. I’m not sure if any temporary solution other than the said eventual planned script execution source limiting via CSP would make much sense.
We frequently use HTML::Scrubber via C4::Scrubber in a number of places to sanitize HTML outputs. It can be very effective. Brutal at times but effective. And yes CSP is coming too, so as per above... use OpacUserJS for injecting Javascript. Otherwise, you're just going to have to change your code later. May as well future-proof yourself a bit now. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 --- Comment #13 from Michał <schodkowy.omegi-0r@icloud.com> ---
It's a work in progress, but believe me when I say it's coming.
And rightly so, I fully support it.
I think there's some merit to what you're saying there. In fact, I think that there's an argument to be made for removing the removal of unsupported tokens completely, since it's not really necessary. (We could add the supported tokens to the syspref description and then any unsupported tokens are the library's responsibility to remove.)
True, I think it wasn't too thoroughly thought out to begin with. For example it could obfuscate a typo by making someone thing that for example just this particular biblio they tested with resolves to empty string for that token...
But your change here would also stop many potential tokens from being removed, which some could say is a regression of the current behaviour.
I think this is only in a very specific situation, say if some library added a new token that did not adhere to the naming convention and then forget to re-introduce the change after upgrade. I think that'd very specific conditions. On the contrary, getting rid of unrecognized token removal completely would have much more chances percentage-wise of causing broken links for non-existent tokens that someone made up or made a typo etc. In theory both scenarios are wrongful, but I think non-existent token look-alikes are more likely to emerge somewhere... But I'd personally be completely fine with either of the paths tbh.
I'd advise against that coding as well. You'd be better off with data-something="1" "data-somethingelse="2".
Fair, I'd do something like that myself probably too in practice (multiple attribs over JSON), but I know that such JSON attributes are a very common practice in some places. So I could imagine someone inserting a link with some attributes and then loading some script of some external provider that'd find an element, data in it, and turn it into a proper link or something technically.
Plus that's a pretty strange use-case for OPACSearchForTitleIn. Keep in mind too bug 36805 which would proably also make that impossible.
I don't think it's overall too strange other than the chosen method of grabbing element by using a script and then calling a func. Thing is, I believe it's the most elegant given that for OPACUserJS things like when that script is executed and whether it's gonna change between versions are less guaranteed probably. Plus if whole scripts to create the URLs were put in there, it'd make things less scattered around. But I could probably end up achieving the same stuff by just using a special id="" or class="" and it would be no trouble in practice, so there's that. But yeah I'll definitely have to chip in some concerns about bug 36805, so that such use-cases are preserved, I see I'm not the only one. The existing tokens only allow some very specific fields to be used. And even if they allowed full MARC access, that's not enough. For example what if I want to search in external catalogue by given ID if it's provided, but fall back to ISBN search otherwise, but also maybe fall back to title search or hide the link as last resort? Simple fallback wouldn't suffice, the URL for different search would be different etc. But with free HTML it's very simple stuff, few lines of code and one can get perfect links. (whether that code be here or in OPACUserJs eventually)
I think that's debateable, but tell you what... I'll reset this to "Signed Off". I am going to change the title and release notes to reflect the actual change made.
Thanks.
And yes CSP is coming too, so as per above... use OpacUserJS for injecting Javascript. Otherwise, you're just going to have to change your code later. May as well future-proof yourself a bit now.
Meh, I kinda could, but I'll wait for now. Current solution already works, and future-proof is not a guarantee if some other miniscule things change like that place of execution I mentioned above or depending how the aforementioned bug goes forward. But when inevitable comes, I'm happy to oblige, it won't be too hard to fix it up then. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |m.de.rooy@rijksmuseum.nl --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Hi, Please add some tests in: Output.t:subtest 'parametrized_url' => sub { And please fix the email of the commit author: <148193449+mkibp@users.noreply.github.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org