Bug 35072 - Invalid usage of "&" in JavaScript intranet-tmpl script redirects
Summary: Invalid usage of "&" in JavaScript intranet-tmpl script redirects
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Michał
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-10-17 07:36 UTC by Michał
Modified: 2023-11-13 15:11 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.05,22.11.12


Attachments
Bug 35072: Fix invalid usage of "&" in JavaScript intranet-tmpl script redirects (6.18 KB, patch)
2023-10-23 14:02 UTC, Michał
Details | Diff | Splinter Review
Bug 35072: Fix invalid usage of "&" in JavaScript intranet-tmpl script redirects (6.24 KB, patch)
2023-10-23 16:29 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35072: Fix invalid usage of "&" in JavaScript intranet-tmpl script redirects (6.30 KB, patch)
2023-10-23 21:05 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 Michał 2023-10-17 07:36:59 UTC
There are several files under `koha-tmpl/intranet-tmpl/prog/js/**.js`, where `&` character in the URLs is incorrectly replaced with `&`.

In practice, `&amp;` 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&amp;biblionumber=` is incorrect!

The `&amp;` 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/f27ed123be8ac1f0ecd76cd54ed9c2a8e591aefe/koha-tmpl/intranet-tmpl/prog/js/catalog.js#L55

Which causes the variable `$biblionumber` to be null here: https://github.com/Koha-Community/Koha/blob/f27ed123be8ac1f0ecd76cd54ed9c2a8e591aefe/cataloguing/addbiblio.pl#L488

Which causes the `$frameworkcode` variable to be unpopulated here: https://github.com/Koha-Community/Koha/blob/f27ed123be8ac1f0ecd76cd54ed9c2a8e591aefe/cataloguing/addbiblio.pl#L510-L511

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...
Comment 1 Michał 2023-10-17 07:57:27 UTC
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 `&amp;` 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 `&amp;` 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 "&amp;" 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?
Comment 2 Katrin Fischer 2023-10-17 07:59:46 UTC
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.
Comment 3 Owen Leonard 2023-10-17 10:29:00 UTC
(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?
Comment 4 Owen Leonard 2023-10-17 10:44:36 UTC
I'm also curious what browser and operating system you're using.
Comment 5 Michał 2023-10-17 13:07:08 UTC
Firefox on Windows. But the &amp; 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...
Comment 6 Michał 2023-10-17 13:19:06 UTC
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 &amp; 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 (&amp; and afterwards is stripped). Then if you repeat the same request (Edit and repeat in Firefox devtools, and correct &amp; to &), you see it will actually work fine then...
Comment 7 Michał 2023-10-23 14:02:09 UTC
Created attachment 157654 [details] [review]
Bug 35072: Fix invalid usage of "&amp;" 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...
Comment 8 Owen Leonard 2023-10-23 16:29:48 UTC
Created attachment 157681 [details] [review]
Bug 35072: Fix invalid usage of "&amp;" 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 "&amp;" 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>
Comment 9 Katrin Fischer 2023-10-23 21:05:30 UTC
Created attachment 157693 [details] [review]
Bug 35072: Fix invalid usage of "&amp;" 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 "&amp;" 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>
Comment 10 Katrin Fischer 2023-10-23 21:05:45 UTC
Thanks for your patch!
Comment 11 Tomás Cohen Arazi 2023-10-24 13:07:00 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 12 Fridolin Somers 2023-10-26 06:57:38 UTC
Pushed to 23.05.x for 23.05.05
Comment 13 Matt Blenkinsop 2023-11-13 15:11:32 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x