Bug 40525 - CSV formula injection - client side (DataTables) in OPAC
Summary: CSV formula injection - client side (DataTables) in OPAC
Status: Pushed to oldoldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal
Assignee: David Cook
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 41137
  Show dependency treegraph
 
Reported: 2025-07-28 03:14 UTC by David Cook
Modified: 2025-11-10 15:36 UTC (History)
18 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Comma delimited list of Sponsors:
Crowdfunding goal: 0
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.11.00,25.05.05,24.11.10,24.05.15,22.11.32
Circulation function:


Attachments
Bug 40525: Update spreadsheet format to escape formulas (2.79 KB, patch)
2025-07-28 03:47 UTC, David Cook
Details | Diff | Splinter Review
Bug 40525: Update spreadsheet format to escape formulas (2.82 KB, patch)
2025-10-08 19:02 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 40525: Update spreadsheet format to escape formulas (2.92 KB, patch)
2025-10-17 09:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[22.11] Bug 40525: Update spreadsheet format to escape formulas (3.08 KB, patch)
2025-10-28 01:59 UTC, Wainui Witika-Park
Details | Diff | Splinter Review
Bug 40525: [22.11] Update spreadsheet format to escape formulas (7.79 KB, patch)
2025-10-28 06:03 UTC, David Cook
Details | Diff | Splinter Review
Bug 40525: [24.11] Update spreadsheet format to escape formulas (6.88 KB, patch)
2025-10-28 14:40 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 40525: [24.05] Update spreadsheet format to escape formulas (7.86 KB, patch)
2025-10-28 21:24 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 40525: [22.11] Update spreadsheet format to escape formulas (7.82 KB, patch)
2025-10-28 23:55 UTC, Wainui Witika-Park
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2025-07-28 03:14:32 UTC
Like bug 37727 it's possible for malicious formulas to be embedded in CSVs that are downloaded and run via the OPAC.
Comment 2 David Cook 2025-07-28 03:47:35 UTC
Created attachment 184698 [details] [review]
Bug 40525: Update spreadsheet format to escape formulas

This change updates the spreadsheet format to escape formulas
using the built-in function if using Buttons 3.2.0+ or using
a workaround if not.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=29
2. Click on tab 1
3. For 100$a type in the following and save:
=SUM(1+1)
4. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?findborrower=42
5. Checkout barcode 39999000001310 to the patron
6. Go to http://localhost:8080/cgi-bin/koha/opac-user.pl and login as that patron
7. Download the CSV for the summary table and open in Excel
8. Note that the "Author" is "'=SUM(1+1)" and not "=SUM(1+1)" or "2"
Comment 3 Brendan Lawlor 2025-10-08 19:02:11 UTC
Created attachment 187596 [details] [review]
Bug 40525: Update spreadsheet format to escape formulas

This change updates the spreadsheet format to escape formulas
using the built-in function if using Buttons 3.2.0+ or using
a workaround if not.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=29
2. Click on tab 1
3. For 100$a type in the following and save:
=SUM(1+1)
4. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?findborrower=42
5. Checkout barcode 39999000001310 to the patron
6. Go to http://localhost:8080/cgi-bin/koha/opac-user.pl and login as that patron
7. Download the CSV for the summary table and open in Excel
8. Note that the "Author" is "'=SUM(1+1)" and not "=SUM(1+1)" or "2"

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 4 Brendan Lawlor 2025-10-08 19:07:43 UTC
Rebased for current main. Works as described. Nice!
Comment 5 Marcel de Rooy 2025-10-17 09:20:03 UTC
Created attachment 188052 [details] [review]
Bug 40525: Update spreadsheet format to escape formulas

This change updates the spreadsheet format to escape formulas
using the built-in function if using Buttons 3.2.0+ or using
a workaround if not.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=29
2. Click on tab 1
3. For 100$a type in the following and save:
=SUM(1+1)
4. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?findborrower=42
5. Checkout barcode 39999000001310 to the patron
6. Go to http://localhost:8080/cgi-bin/koha/opac-user.pl and login as that patron
7. Download the CSV for the summary table and open in Excel
8. Note that the "Author" is "'=SUM(1+1)" and not "=SUM(1+1)" or "2"

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2025-10-17 09:21:05 UTC
Looks good to me. Seems that tabs and carriage returns are already removed by trim btw.
Comment 7 Wainui Witika-Park 2025-10-20 03:56:47 UTC
Can this please be rebased for 22.11?
Comment 8 Wainui Witika-Park 2025-10-28 01:59:08 UTC
Created attachment 188494 [details] [review]
[22.11] Bug 40525: Update spreadsheet format to escape formulas

This change updates the spreadsheet format to escape formulas
using the built-in function if using Buttons 3.2.0+ or using
a workaround if not.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=29
2. Click on tab 1
3. For 100$a type in the following and save:
=SUM(1+1)
4. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?findborrower=42
5. Checkout barcode 39999000001310 to the patron
6. Go to http://localhost:8080/cgi-bin/koha/opac-user.pl and login as that patron
7. Download the CSV for the summary table and open in Excel
8. Note that the "Author" is "'=SUM(1+1)" and not "=SUM(1+1)" or "2"

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Wainui Witika-Park 2025-10-28 03:37:33 UTC
Hi I attempted a rebase for 22.11 myself.

However, I notice I am not getting the expected behaviour for this step:

8. Note that the "Author" is "'=SUM(1+1)" and not "=SUM(1+1)" or "2"

I am still getting "=SUM(1+1)" after applying my patch.

However, we don't use Microsoft in our organisation and so I am using LibreOffice Calc. I wonder if anyone with Excel could please test my 22.11 patch?
Comment 10 David Cook 2025-10-28 04:59:28 UTC
(In reply to Wainui Witika-Park from comment #9)
> Hi I attempted a rebase for 22.11 myself.
> 
> However, I notice I am not getting the expected behaviour for this step:
> 
> 8. Note that the "Author" is "'=SUM(1+1)" and not "=SUM(1+1)" or "2"
> 
> I am still getting "=SUM(1+1)" after applying my patch.
> 
> However, we don't use Microsoft in our organisation and so I am using
> LibreOffice Calc. I wonder if anyone with Excel could please test my 22.11
> patch?

I suspect that your patch is probably failing. If you look at the browser console, I'm sure it'll be full of red error messages.

I thought that I'd backported this to 23.11 but it looks like I'd actually backported bug 40522. They use the same concept so hopefully I can provide some clarity...

--

For 23.11, I had to create a custom shim. The Buttons library is too old in older versions of Koha, so we can't use "DataTable.Buttons.stripData". 

The shim I created is about 125 lines long, and is inspired by DataTable.Buttons.stripData

I'll look at putting something together. Other bugs with similar issues will need the same thing, so maybe they'll have to depend on this one...
Comment 11 David Cook 2025-10-28 06:02:13 UTC
So 22.11 is actually even worse than I expected so it took a bit of time but I think i've got a patch together...
Comment 12 David Cook 2025-10-28 06:03:25 UTC
Created attachment 188496 [details] [review]
Bug 40525: [22.11] Update spreadsheet format to escape formulas
Comment 13 Jesse Maseto 2025-10-28 08:03:51 UTC
This has merge conflicts with 24.05.x

Please resolve.
Comment 14 Baptiste Wojtkowski (bwoj) 2025-10-28 14:40:11 UTC
Created attachment 188514 [details] [review]
Bug 40525: [24.11] Update spreadsheet format to escape formulas
Comment 15 Baptiste Wojtkowski (bwoj) 2025-10-28 14:42:56 UTC
Created 24.11 from 22.11 ; patch from main were not applying.

While resolving conflicts, we weren't able to find the changes that should have been made to koha-tmpl/opac-tmpl/bootstrap/js/datatables.js. However we can follow the test plan successfully without.
Comment 16 Wainui Witika-Park 2025-10-28 21:20:57 UTC
(In reply to David Cook from comment #12)
> Created attachment 188496 [details] [review] [review]
> Bug 40525: [22.11] Update spreadsheet format to escape formulas

Thanks so much David! This patch works as expected. I am now getting "'=SUM(1+1)" and not "=SUM(1+1)" or "2"
Comment 17 Lucas Gass (lukeg) 2025-10-28 21:24:05 UTC
Created attachment 188534 [details] [review]
Bug 40525: [24.05] Update spreadsheet format to escape formulas
Comment 18 David Cook 2025-10-28 22:11:32 UTC
Hmmm let me take another look at this. I don't think the shim should be needed in 24.11, but I'll do a quick review...
Comment 19 David Cook 2025-10-28 22:35:07 UTC
Ok so the original patch will only apply to 25.05 and main because bug 38255 changed how datatables get created. However, the code itself can work in 24.11, as it uses a new enough Buttons library. Here's the breakdown of DataTables and Buttons libraries per version:

22.11: DataTables 1.10.18 Buttons 1.5.2 (./koha-tmpl/opac-tmpl/lib/jquery/plugins/jquery.dataTables.min.js)

24.05: DataTables 1.10.18 Buttons 1.5.2 (./koha-tmpl/opac-tmpl/lib/jquery/plugins/jquery.dataTables.min.js)

24.11: DataTables 2.1.8 Buttons 3.1.2 (./koha-tmpl/opac-tmpl/lib/datatables/datatables.min.js)

main: DataTables 2.1.8 Buttons 3.1.2 (./koha-tmpl/opac-tmpl/lib/datatables/datatables.min.js)

--

It's tempting to use the buttons shim in 24.11 anyways just for ease of backporting but 24.11 is also a LTS so might be better to get it right, because a short term gain could mean long term pain.

DataTables was a bit of a mess before Jonathan cleaned it up, so the 24.11 patch leaving off the datatables.js change is probably fine because I don't think the buttons in datatables.js actually get used pre-25.05.
Comment 20 David Cook 2025-10-28 22:38:07 UTC
(In reply to David Cook from comment #19)
> Ok so the original patch will only apply to 25.05 and main because bug 38255
> changed how datatables get created. However, the code itself can work in
> 24.11, as it uses a new enough Buttons library. Here's the breakdown of
> DataTables and Buttons libraries per version:
> 
> 22.11: DataTables 1.10.18 Buttons 1.5.2
> (./koha-tmpl/opac-tmpl/lib/jquery/plugins/jquery.dataTables.min.js)
> 
> 24.05: DataTables 1.10.18 Buttons 1.5.2
> (./koha-tmpl/opac-tmpl/lib/jquery/plugins/jquery.dataTables.min.js)
> 
> 24.11: DataTables 2.1.8 Buttons 3.1.2
> (./koha-tmpl/opac-tmpl/lib/datatables/datatables.min.js)
> 
> main: DataTables 2.1.8 Buttons 3.1.2
> (./koha-tmpl/opac-tmpl/lib/datatables/datatables.min.js)
> 
> --
> 
> It's tempting to use the buttons shim in 24.11 anyways just for ease of
> backporting but 24.11 is also a LTS so might be better to get it right,
> because a short term gain could mean long term pain.
> 
> DataTables was a bit of a mess before Jonathan cleaned it up, so the 24.11
> patch leaving off the datatables.js change is probably fine because I don't
> think the buttons in datatables.js actually get used pre-25.05.

Ah wait nevermind... as per my comment on bug 37727 the formula escape was added in Buttons 3.2.0, so even though stripData is available in Buttons 3.1.2 - it's not quite comprehensive enough.
Comment 21 David Cook 2025-10-28 23:37:58 UTC
(In reply to David Cook from comment #18)
> Hmmm let me take another look at this. I don't think the shim should be
> needed in 24.11, but I'll do a quick review...

In conclusion, I think this is all good. 

I'll keep in mind this experience for bugs 40524 and 40522 so that I can ease the pain of backporting.
Comment 22 Wainui Witika-Park 2025-10-28 23:55:44 UTC
Created attachment 188535 [details] [review]
Bug 40525: [22.11] Update spreadsheet format to escape formulas
Comment 23 Wainui Witika-Park 2025-10-28 23:59:25 UTC
(In reply to Wainui Witika-Park from comment #22)
> Created attachment 188535 [details] [review] [review]
> Bug 40525: [22.11] Update spreadsheet format to escape formulas

I was getting:

```
ok 132 - /kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/en/includes/date-format.inc
# excluding template /kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/en/includes/doc-head-close.inc because it cannot stand on its own
# excluding template /kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-bottom.inc because it cannot stand on its own
not ok 133 - /kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/en/includes/columns_settings.inc
#   Failed test '/kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/en/includes/columns_settings.inc'
#   at xt/author/valid-templates.t line 141.
# undef error - : filter not found
ok 134 - /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt

```
so I added the following to columns_settings.inc:
```
[% USE raw %]
```
Comment 24 David Cook 2025-10-29 04:14:13 UTC
Good catch, Wainui.

I think there might also be a problem in opac-user.pl on main but unsure. Got some more testing to do...
Comment 25 David Cook 2025-10-29 04:45:18 UTC
(In reply to David Cook from comment #24)
> Good catch, Wainui.
> 
> I think there might also be a problem in opac-user.pl on main but unsure.
> Got some more testing to do...

Nevermind - looks like an unrelated problem in main :)
Comment 26 David Cook 2025-10-30 00:11:49 UTC
I've been deep diving this more (for the first time since late July) and I think I made some mistakes on the backport. Not mistakes that will cause any negative impacts, but there's probably more changes than there need to be on the backports.
Comment 27 David Cook 2025-10-30 00:13:32 UTC
(In reply to David Cook from comment #26)
> I've been deep diving this more (for the first time since late July) and I
> think I made some mistakes on the backport. Not mistakes that will cause any
> negative impacts, but there's probably more changes than there need to be on
> the backports.

Actually, maybe I take that back. 

It's tough keeping all the different permutations of DataTables straight across so many different Koha versions with different DataTables implementations. 

I'll be glad when 24.05 and 22.11 go away. Then we can improve 24.11 as the LTS I guess...
Comment 28 David Cook 2025-10-31 05:42:57 UTC
I've merged in v24.11.10-2 with my local changes and it's working well :)
Comment 29 Lucas Gass (lukeg) 2025-11-03 16:45:36 UTC
Nice work all, pushed to main for 25.11.00