[Bug 35072] New: Invalid usage of "& " in JavaScript intranet-tmpl script redirects
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 Bug ID: 35072 Summary: Invalid usage of "&" in JavaScript intranet-tmpl script redirects Change sponsored?: --- Product: Koha Version: 23.05 Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Templates Assignee: oleonard@myacpl.org Reporter: schodkowy.omegi-0r@icloud.com QA Contact: testopia@bugs.koha-community.org There are several files under `koha-tmpl/intranet-tmpl/prog/js/**.js`, where `&` character in the URLs is incorrectly replaced with `&`. In practice, `&` should only ever be used a part of HTML elements, for example inside of <a href=""> link. Using it in JS redirects such as `window.location="addbiblio.pl?op=delete&biblionumber=` is incorrect! The `&` part is preserved like this by both the web browser and the web server. This creates several bugs, such as the one I've encountered and managed to trace back to this very issue. For example this makes you unable to delete a biblio record in admin interface AT ALL. To reproduce just install a fresh Koha installation, import a record and try to delete it. It would redirect you to the search.pl page, but nothing would happen. I traced the above to this: https://github.com/Koha-Community/Koha/blob/f27ed123be8ac1f0ecd76cd54ed9c2a8... Which causes the variable `$biblionumber` to be null here: https://github.com/Koha-Community/Koha/blob/f27ed123be8ac1f0ecd76cd54ed9c2a8... Which causes the `$frameworkcode` variable to be unpopulated here: https://github.com/Koha-Community/Koha/blob/f27ed123be8ac1f0ecd76cd54ed9c2a8... Which causes the following warnings: ``` [2023/10/17 09:04:43] [WARN] Use of uninitialized value $frameworkcode in string eq at /usr/share/koha/intranet/cgi-bin/cataloguing/addbiblio.pl line 513. [2023/10/17 09:04:43] [WARN] Use of uninitialized value $frameworkcode in string eq at /usr/share/koha/intranet/cgi-bin/cataloguing/addbiblio.pl line 517. [2023/10/17 09:04:43] [WARN] Use of uninitialized value $frameworkcode in string eq at /usr/share/koha/intranet/cgi-bin/cataloguing/addbiblio.pl line 536. ``` I did a quick search, and found out that these files need to be corrected: * koha-tmpl/intranet-tmpl/prog/js/catalog.js * koha-tmpl/intranet-tmpl/prog/js/members-menu.js * koha-tmpl/intranet-tmpl/prog/js/holds.js * koha-tmpl/intranet-tmpl/prog/js/cart.js * koha-tmpl/intranet-tmpl/prog/js/ill-list-table.js * koha-tmpl/intranet-tmpl/prog/js/pages/results.js I ended up submitting it as bug, as it will probably be much faster to get submitted and approved by someone who already has a dev workflow set up to contribute the patch... Not sure if the severity I used is okay, but this prevents many features in the UI from working properly at all, so I think it is pretty important to fix this quickly. The random bugs/partial breakage this can cause can make hard-to-notice and hard-to-trace problems appear... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 M <schodkowy.omegi-0r@icloud.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Priority|P5 - low |P2 Patch complexity|--- |Trivial patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 --- Comment #1 from M <schodkowy.omegi-0r@icloud.com> --- I see that this issue is actually present in a lot of .pl Perl files on the server... All of this needs to be fixed, the escape to `&` can only ever be done within HTML elements, but it's not even obligatory there either, so it's easier to just remove it outright. There's no reason to ever enter `&` manually in URLs! I dug a tad deeper, and it seems a reverse proxy that we're currently forced to use does sanitize URLs and so it strips out invalid parameter entirely, meaning that anything after "&" is inclusively removed... I imagine the way it was never noticed and all fixed is because Perl must handle this issue somewhere internally in case someone makes a malformed request? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 --- Comment #2 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi, thx for the research and explanation. The bug is filed perfectly - I only moved it up to "master", because I am sure these are also present in our development version. A fix will then be backported to all older versions affected. If you want, we can help you with submitting a patch? The koha-testing-docker (https://gitlab.com/koha-community/koha-testing-docker) dev environment gives you all the tools you'd need to do it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 --- Comment #3 from Owen Leonard <oleonard@myacpl.org> --- (In reply to M from comment #0)
For example this makes you unable to delete a biblio record in admin interface AT ALL.
I'm not able to reproduce this problem. Can you describe in more detail where you're trying to delete the record? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|23.05 |master Priority|P2 |P5 - low -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 --- Comment #4 from Owen Leonard <oleonard@myacpl.org> --- I'm also curious what browser and operating system you're using. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 --- Comment #5 from M <schodkowy.omegi-0r@icloud.com> --- Firefox on Windows. But the & part gets removed by reverse proxy actually, and currently we're forced to use this proxy. I remove the record from staff interface (/cgi-bin/koha/catalogue/detail.pl?biblionumber=1715&found1=1), I press Modify->Delete Record. It does seem that using Apache2 directly there will not make it complain, but there are intermediate servers that reparse URLs, and that's where there's trouble. This just coincidentally works in Perl, but is undefined behavior due to being invalid request. For example in PHP you'd end up with $_REQUEST variable having key "amp;biblionumber" instead of "biblionumber" in such request... Btw the addbiblio.pl script could have some validation if delete operation supplied the number parameter instead of just silently redirecting to search.pl without doing anything actually... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 --- Comment #6 from M <schodkowy.omegi-0r@icloud.com> --- In default setup: - When making a request to delete biblio record, open up devtools (enable persistent request saving in Network tab), and look at what request browser is making (it should not have & in the url, just &) - Observe the same in Apache2 logs in /var/log/koha Our actual setup currently uses serveo.net to create a simple tunnel to access Koha over the internet, as we haven't set up proper redirection *yet*, and this is setup so that staff can work on the database until it's ready to move to the proper server. Generally this setup works great, but the URLs are sanitized and so invalid parameter is removed. You can try for yourself as the site doesn't require an account and you just use appropriate SSH command to set up port redirect and get an address in return. Use that over the dev setup and try removing a biblio record, and then see what gets logged in Apache2 now (& and afterwards is stripped). Then if you repeat the same request (Edit and repeat in Firefox devtools, and correct & to &), you see it will actually work fine then... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtj@kohaaloha.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 M <schodkowy.omegi-0r@icloud.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|oleonard@myacpl.org |schodkowy.omegi-0r@icloud.c | |om Status|NEW |Needs Signoff --- Comment #7 from M <schodkowy.omegi-0r@icloud.com> --- Created attachment 157654 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157654&action=edit Bug 35072: Fix invalid usage of "&" in JavaScript intranet-tmpl script redirects This patch fixes it in just JS templates, which is where it causes actual breakage (as invalid HTTP requests are being sent). It does not fix invalid escape occurences that might happen inside of Perl templates (second case I've mentioned later), as those are silently handled by this piece of code in case of CGI redirects: https://metacpan.org/release/LDS/CGI.pm-3.48/source/lib/CGI.pm#L1630-1647 As such, this patch itself fixes only JS templates to keep things simple, and is ready to be merged and fix the prime bug. As for the second case with Perl templates, I'll wait for feedback on whether I should look into that and make a patch as well, as that technically happens to not break things, just isn't right pedantically... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157654|0 |1 is obsolete| | --- Comment #8 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 157681 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157681&action=edit Bug 35072: Fix invalid usage of "&" in JavaScript intranet-tmpl script redirects These escapes were invalid in these places, as HTML entity escapes are meant to be used only inside of HTML elements/attributes, not inside of JavaScript code. These URLs would be sent out by the browser as-is, and that'd usually work on the default install only coincidentally. Unfortunately, on some setups (such as when using reverse proxies), this would break, and the URL after "&" would have been truncated. This small patch adjusts the URLs in templates to not use wrong escapes, and makes them consistent with how URLs are formatted for JavaScript redirects in most of the templates already. Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |katrin.fischer@bsz-bw.de |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157681|0 |1 is obsolete| | --- Comment #9 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 157693 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157693&action=edit Bug 35072: Fix invalid usage of "&" in JavaScript intranet-tmpl script redirects These escapes were invalid in these places, as HTML entity escapes are meant to be used only inside of HTML elements/attributes, not inside of JavaScript code. These URLs would be sent out by the browser as-is, and that'd usually work on the default install only coincidentally. Unfortunately, on some setups (such as when using reverse proxies), this would break, and the URL after "&" would have been truncated. This small patch adjusts the URLs in templates to not use wrong escapes, and makes them consistent with how URLs are formatted for JavaScript redirects in most of the templates already. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 --- Comment #10 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Thanks for your patch! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |23.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 --- Comment #11 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable Version(s)|23.11.00 |23.11.00,23.05.05 released in| | CC| |fridolin.somers@biblibre.co | |m --- Comment #12 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 23.05.x for 23.05.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable Version(s)|23.11.00,23.05.05 |23.11.00,23.05.05,22.11.12 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35072 --- Comment #13 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- Nice work everyone! Pushed to oldstable for 22.11.x -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org