A recent penetration test found hat the version of Datatables we're using is vulnerable and should be updated to the latest release. It did not however, detail an official CVE.
https://security.snyk.io/package/npm/datatables.net/1.10.18
It looks like a newer bundle of DataTables would also fix a Content-Security-Policy issue I talk about in bug 20397
Excellent, thats good news.. Also, thanks for looking into this Owen.. just wanted to show my appreciation and support.
Here's remote branch, ready for testing, which updates the staff interface: https://gitlab.com/koha-dev/koha-dev/-/commits/bug_34913 It touches 126 files... Would it be better if I split it up into smaller patches?
I have not found any regressions. The only thing I noticed is the 3 blue dots when a table is loading (so when using the REST API). We do not have much blue on the UI, could be nice to adjust or remove. Not blocker however.
(Note that it's not saying "Loading" but "Processing")
Created attachment 156480 [details] [review] Bug 34913: Fix a selenium test t/db_dependent/selenium/administration_tasks.t ....... 3/3 Cannot show all entries from table //div[@id="libraries_wrapper"] at /kohadevbox/koha/t/lib/Selenium.pm line 247.
I am wondering if we should/can add a check in the QA script to catch introduction of old Hungarian notation to prevent new occurrences. That does not seem as easy as listing them all and grep the different words. For instance var is_searchable = table_dt.settings()[0].aoColumns[i].bSearchable; bSearchable can be replaced with seachable But aoColumns[1] cannot (simply) be replaced with columns[1]. There are still a lot of occurrences of Hungarian notation after this patch, should not we replace them all?
(In reply to Jonathan Druart from comment #8) > There are still a lot of occurrences of Hungarian notation after this patch, > should not we replace them all? Eventually I think we should, but this patch specifically addresses the problem we will face with in-page configurations not playing well with our default configuration. If I have missed cases of that please let me know or provide a follow-up. Changing the use of these variables in other places is a much bigger task with more complicated testing requirements.
This is looking great to me.. also can't spot any regressions and the selenium test works for me after Jonathans patch. I'm happy for you to add my Signoff.
I also couldn't spot any regressions. I filed 3 bugs for stuff I found in the process of testing, but none of these were related to this patch set :D
Can you please add my SO lines? :) I tested with the patch here on top and rebasd on current master.
Awesome to see this in PQA :)
Created attachment 157511 [details] [review] Bug 34913: Move item groups table to kohaTable
The remote branch has been updated (https://gitlab.com/koha-dev/koha-dev/-/commits/bug_34913) but the bug fixed in Comment 14 affects other pages too: - Item search - Lists - Manage staged MARC records (batch view)
(In reply to Owen Leonard from comment #15) > The remote branch has been updated > (https://gitlab.com/koha-dev/koha-dev/-/commits/bug_34913) but the bug fixed > in Comment 14 affects other pages too: > > - Item search > - Lists > - Manage staged MARC records (batch view) Those are messy. We need to adjust the controller scripts... Not trivial.
Created attachment 157725 [details] [review] Bug 34913: Adjust C4::Utils::DataTables::VirtualShelves And remove C4::Utils::DataTables, which should no longer be reused anyway.
Created attachment 157726 [details] [review] Bug 34913: Adjust "Manage staged MARC records"
Created attachment 157729 [details] [review] Bug 34913: WIP - item search This is not working, the columnfilter plugin seems to break the sorting. If you sort twice on a colum, the request will be GET (instead of POSTed, which is the historical behaviour) and columns will contain [object Object] There are more to do here, but this problem blocks the other steps.
(In reply to Jonathan Druart from comment #19) > Created attachment 157729 [details] [review] [review] > Bug 34913: WIP - item search > > This is not working, the columnfilter plugin seems to break the sorting. > If you sort twice on a colum, the request will be GET (instead of > POSTed, which is the historical behaviour) and columns will contain > [object Object] > > There are more to do here, but this problem blocks the other steps. I am stuck on this one, asking for help.
(In reply to Jonathan Druart from comment #20) > (In reply to Jonathan Druart from comment #19) > > Created attachment 157729 [details] [review] [review] [review] > > Bug 34913: WIP - item search > > > > This is not working, the columnfilter plugin seems to break the sorting. > > If you sort twice on a colum, the request will be GET (instead of > > POSTed, which is the historical behaviour) and columns will contain > > [object Object] > > > > There are more to do here, but this problem blocks the other steps. > > I am stuck on this one, asking for help. Is that the ColVis plugin? It looks like it's been retired: https://datatables.net/extensions/colvis/ I'm wondering if this bug needs to be broken up into a few different more manageable chunks?
(In reply to David Cook from comment #21) > (In reply to Jonathan Druart from comment #20) > > (In reply to Jonathan Druart from comment #19) > > > Created attachment 157729 [details] [review] [review] [review] [review] > > > Bug 34913: WIP - item search > > > > > > This is not working, the columnfilter plugin seems to break the sorting. > > > If you sort twice on a colum, the request will be GET (instead of > > > POSTed, which is the historical behaviour) and columns will contain > > > [object Object] > > > > > > There are more to do here, but this problem blocks the other steps. > > > > I am stuck on this one, asking for help. > > Is that the ColVis plugin? It looks like it's been retired: > https://datatables.net/extensions/colvis/ No, columFilter (filters on the top of the colums). > I'm wondering if this bug needs to be broken up into a few different more > manageable chunks? It's the only missing bit apparently, better spending time on fixing it than splitting everything...
(In reply to Jonathan Druart from comment #22) > (In reply to David Cook from comment #21) > > (In reply to Jonathan Druart from comment #20) > > > I am stuck on this one, asking for help. > > > > Is that the ColVis plugin? It looks like it's been retired: > > https://datatables.net/extensions/colvis/ > > No, columFilter (filters on the top of the colums). Ahhh ok. -- So I've checked out https://gitlab.com/koha-dev/koha-dev/-/commits/bug_34913 and then applied these Bugzilla patches... koha-plack --reload kohadev perl build-resources.PL When I visit http://localhost:8081/cgi-bin/koha/mainpage.pl, I'm getting this error: DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'lang' in 'where clause' at /kohadevbox/koha/Koha/Objects.pm line 426 at /usr/share/perl5/DBIx/Class/Exception.pm line 77 But I can go to http://localhost:8081/cgi-bin/koha/catalogue/itemsearch.pl
(In reply to Jonathan Druart from comment #19) > This is not working, the columnfilter plugin seems to break the sorting. > If you sort twice on a colum, the request will be GET (instead of > POSTed, which is the historical behaviour) and columns will contain > [object Object] > > There are more to do here, but this problem blocks the other steps. This one is super interesting. The initial load uses POST and sends form data. I've got a breakpoint in the Javascript which shows me it's running our "data" handler in the "ajax" object. I click the column to sort and the breakpoint is triggered again, but this time it's a GET and sends query string data. I click the column a 2nd time to get the sort to change and the breakpoint is not activated, a GET request is sent with the columns containing [object Object], and the dataTable just hangs. -- Looking at ./koha-tmpl/intranet-tmpl/lib/jquery/plugins/jquery.dataTables.columnFilter.js it looks like it's very old code. It looks like we don't have an entry for it in http://localhost:8081/cgi-bin/koha/about.pl It looks like it's an abandoned project. This person claims to have fixed some issues with it but that was also 9 years ago: https://github.com/RobinDev/jquery.dataTables.columnFilter.js I think we might have to either remove this plugin or fork it...
(In reply to David Cook from comment #23) > (In reply to Jonathan Druart from comment #22) > > (In reply to David Cook from comment #21) > > > (In reply to Jonathan Druart from comment #20) > > > > I am stuck on this one, asking for help. > > > > > > Is that the ColVis plugin? It looks like it's been retired: > > > https://datatables.net/extensions/colvis/ > > > > No, columFilter (filters on the top of the colums). > > Ahhh ok. > > -- > > So I've checked out https://gitlab.com/koha-dev/koha-dev/-/commits/bug_34913 > and then applied these Bugzilla patches... > > koha-plack --reload kohadev > > perl build-resources.PL > > When I visit http://localhost:8081/cgi-bin/koha/mainpage.pl, I'm getting > this error: > > DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st > execute failed: Unknown column 'lang' in 'where clause' at > /kohadevbox/koha/Koha/Objects.pm line 426 > at /usr/share/perl5/DBIx/Class/Exception.pm line 77 Not related with this patch set. (In reply to David Cook from comment #24) > (In reply to Jonathan Druart from comment #19) > > This is not working, the columnfilter plugin seems to break the sorting. > > If you sort twice on a colum, the request will be GET (instead of > > POSTed, which is the historical behaviour) and columns will contain > > [object Object] > > > > There are more to do here, but this problem blocks the other steps. > > This one is super interesting. > > The initial load uses POST and sends form data. I've got a breakpoint in the > Javascript which shows me it's running our "data" handler in the "ajax" > object. > > I click the column to sort and the breakpoint is triggered again, but this > time it's a GET and sends query string data. > > I click the column a 2nd time to get the sort to change and the breakpoint > is not activated, a GET request is sent with the columns containing [object > Object], and the dataTable just hangs. > > -- > > Looking at > ./koha-tmpl/intranet-tmpl/lib/jquery/plugins/jquery.dataTables.columnFilter. > js it looks like it's very old code. > > It looks like we don't have an entry for it in > http://localhost:8081/cgi-bin/koha/about.pl > > It looks like it's an abandoned project. > > This person claims to have fixed some issues with it but that was also 9 > years ago: https://github.com/RobinDev/jquery.dataTables.columnFilter.js > > I think we might have to either remove this plugin or fork it... Good catch, this version fixed the problem we faced. I am suggesting to go with this version for now. It's not conceivable to remove the column filtering, we rely on it in several places.
No sure what is going on right now but I don't manage to attach patches to bugzilla. I have pushed a remote branch - https://gitlab.com/joubu/Koha/-/commits/bug_34913 Ready for testing!
(In reply to Jonathan Druart from comment #25) > Good catch, this version fixed the problem we faced. > > I am suggesting to go with this version for now. It's not conceivable to > remove the column filtering, we rely on it in several places. (In reply to Jonathan Druart from comment #26) > Ready for testing! Sounds like a plan to me. The problem with item search certainly seems to be fixed. I'm trying to find other pages with column filtering so I can test them too... Column filtering in lists looks fine: http://localhost:8081/cgi-bin/koha/virtualshelves/shelves.pl?op=list&public=0 Acquisitions also looks good: http://localhost:8081/cgi-bin/koha/acqui/parcel.pl?invoiceid=2 -- Found a typo causing a Javascript error: koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt : 2348 "pagingType""full",
(In reply to David Cook from comment #27) > koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt : > 2348 > "pagingType""full", Fixed (on my branch, in Owen's commit)
(In reply to Jonathan Druart from comment #28) > (In reply to David Cook from comment #27) > > koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt : > > 2348 > > "pagingType""full", > > Fixed (on my branch, in Owen's commit) Looks good to me. I think I'm happy to move this to Signed Off
Is the vulnerability particularly bad of are those XSS just a subpart of a number others currently in Koha? To know about how bad it's not have backporting because the CSS and option names changes seem way to many to be confident for backporting to be reliably done. Plus a fair number of changes in the JS and the controllers.
Back here :)
(In reply to Katrin Fischer from comment #31) > Back here :) Ok, a little confused which branch has the most recent work. I believe it's yours Joubu - could you rebase, just in case? I can have a look tomorrow or Wednesday probably.
(In reply to Katrin Fischer from comment #32) > (In reply to Katrin Fischer from comment #31) > > Back here :) > > Ok, a little confused which branch has the most recent work. I believe it's > yours Joubu - could you rebase, just in case? I can have a look tomorrow or > Wednesday probably. Yes, my branch. See my last comments, sorry if it was not meaningful enough.
The column filters on the items table in the staff detail page didn't work or look as expected to me. I tested with http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=146 Activated the column filters: formatting is off, there is no placeholder text in the columns. Typed "94" into the barcode filter, nothing was found (it works without this patch set) If you type 94 into the top search input field, the result is as expected.
(In reply to Katrin Fischer from comment #34) > The column filters on the items table in the staff detail page didn't work > or look as expected to me. > > I tested with > http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=146 > > Activated the column filters: formatting is off, there is no placeholder > text in the columns. > Typed "94" into the barcode filter, nothing was found (it works without this > patch set) > > If you type 94 into the top search input field, the result is as expected. Is it working on top of bug 33568? If so there is no need to fix it here.
(In reply to Katrin Fischer from comment #34) > The column filters on the items table in the staff detail page didn't work > or look as expected to me. > > I tested with > http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=146 > > Activated the column filters: formatting is off, there is no placeholder > text in the columns. > Typed "94" into the barcode filter, nothing was found (it works without this > patch set) > > If you type 94 into the top search input field, the result is as expected. I just tried as well and I see what you mean about the formatting/placeholder text. (That is, there is no placeholder text, and the column filters appear above the table headers rather than below them.) (Actually, if you click "Activate" and "Deactivate" a few times, sometimes the filters are above the headers and sometimes they're below the headers.) However, the "Search:" and the barcode column filter worked for me.
It looks like the same problem exists on http://localhost:8081/cgi-bin/koha/reports/itemslost.pl which uses koha-tmpl/intranet-tmpl/prog/js/table_filters.js as well Actually, it's a bit different. Initially, it shows the filters and placeholder text in the right place. But if you click "Deactivate filters" and then "Activate filters" again the placeholder text is gone and the filters are above the table headers. The "Search:" and column filters work data-wise though.
(In reply to Jonathan Druart from comment #35) > Is it working on top of bug 33568? If so there is no need to fix it here. Looks like table_filters.js is still using the legacy syntax: https://gitlab.com/joubu/Koha/-/blob/bug_33568/koha-tmpl/intranet-tmpl/prog/js/table_filters.js Something relevant is that the new "./koha-tmpl/intranet-tmpl/lib/jquery/plugins/jquery.dataTables.columnFilter.js" does not contain "bFiltersAlreadyActivated" Looks like detail.pl has some probably unnecessary code involving the header rows...
(In reply to David Cook from comment #38) > Looks like detail.pl has some probably unnecessary code involving the header > rows... Nope I'm so confused. /cgi-bin/koha/catalogue/detail.pl?biblionumber=29 won't show the placeholder text but /cgi-bin/koha/reports/itemslost.pl does even though they're running the same code... There's something funny with this line: clone = thead_row.clone().addClass('filters_row'); The action of adding that class is changing the row and that's what seems to be causing the placeholder problem...
No that's not it... the console.log() isn't showing me the data structure at the point in time. It's just going to returning the object and then I'm seeing it after it's already been transformed later...
Created attachment 158229 [details] [review] Bug 34913: Fix Activate/Deactive filters with table_filters.js This change prevents columnFilter from being called twice. If it's called twice, it compromises the formatting of the filters.
Jonathan, can you add the above patch to your branch? That problem was sure annoying but removing the second totally unnecessary call to columnFilter is what fixes the problem on the following pages: http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=29 http://localhost:8081/cgi-bin/koha/reports/itemslost.pl
Done, patch added to bug_34913 and bug_33568.
Why is this stuck?
(In reply to Jonathan Druart from comment #44) > Why is this stuck? Good question. It would be good for this one to make it into the QA Team emails. I'd hoped this would make it into 23.11 but I think it's too late now :(
Remote branch rebased against master.
Who has tested and approved the remote branch? Me, then? Owen and David?
Thanks for the rebase. I'm scheduling a bit of time to look at this first thing tomorrow.
catalogue/itemsearch.pl + use Data::Printer colored => 1; warn p $search_params;
OK.. all I've tested appears to be working.. that was a workout, we have a metric tonne of tables! Could you clean out that last Data::Printer I spotted and mentioned above then add my SO lines on the remainder of patches on the branch please? I'm happy to call this PQA, the scripts are happy and so far I can't spot any regressions now that the items table issues have been resolved. It does feel slightly odd still having that last bFiltersAlreadyActivated still in the code, but things do all appear to be working now.. it's great to see how much cleanup this resulted in. I'd advocate pushing to master and maybe waiting a week before backport in the hopes that any last strugglers we missed might be flushed out. As such.. I'm going to update the status to PQA :)
(In reply to Martin Renvoize from comment #51) > Could you clean out that last Data::Printer I spotted and mentioned above > then add my SO lines on the remainder of patches on the branch please? Done.
Can you please rebase on latest master? CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/edifactmsgs.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc error: could not apply 6bdfa73a80... Bug 34913: DataTables upgrade: Update CSS and option names
I am afk until next Tuesday.
My branch has been synced with Jonathan's and rebased against master: https://gitlab.com/koha-dev/koha-dev/-/commits/bug_34913
Unfortunately I don't have access to rmaint comms but I am the only one around at the minute to do things for the upcoming security release, but I am unsure what I need to backport
Pushed for 24.05! Well done everyone, thank you!
Please fix! koha_1 | # Failed test 'Test::Perl::Critic for "C4/Utils/DataTables/VirtualShelves.pm"' koha_1 | # at /usr/share/perl5/Test/Perl/Critic.pm line 121. koha_1 | # koha_1 | # "@order_by" is declared but not used at line 86, column 9. Unused variables clutter code and make it harder to read. (Severity: 3) koha_1 | Use of uninitialized value $_ in pattern match (m//) at /usr/share/perl5/PPIx/Regexp/Structure/Quantifier.pm line 117.
Created attachment 161575 [details] [review] Bug 34913: Fix perlcritic for VirtualShelves.pm C4/Utils/DataTables/VirtualShelves.pm: "@order_by" is declared but not used at line 86, column 9. Unused variables clutter code and make it harder to read. (Severity: 3)
(In reply to Jonathan Druart from comment #64) > Created attachment 161575 [details] [review] [review] > Bug 34913: Fix perlcritic for VirtualShelves.pm > > C4/Utils/DataTables/VirtualShelves.pm: "@order_by" is declared but not used > at line 86, column 9. Unused variables clutter code and make it harder to > read. (Severity: 3) Patch has been pushed.
Last patch (Fix perlcritic for VirtualShelves.pm) is minor, it will be in 23.11.03
Pushed to 23.11.x for 23.11.02
We need to reopen this and rebase this patchset for 23.05.x.
Backported to 23.05.x for upcoming 23.05.10
(In reply to Lucas Gass from comment #70) > Backported to 23.05.x for upcoming 23.05.10 Hey, I have found an issue which I believe comes from: "Bug 34913: Upgrade staff interface DataTables from 1.10.18 to 1.13.6" When adding orders from a marc file to a basket, the text in the form is showing up as white. Applying the following to intranetusercss temporarily fixes the issue: table.dataTable tbody tr.selected > * { color: black; } Is it possible for this issue to please be fixed? Thanks, Wainui
(In reply to wainuiwitikapark from comment #71) > When adding orders from a marc file to a basket, the text in the form is > showing up as white. I believe this refers to problem fixed by Bug 36035: Wrong text colour in addorderiso2709.pl.