Bug 40524

Summary: Stored XSS run by DataTables Print button in staff interface
Product: Koha Reporter: David Cook <dcook>
Component: Architecture, internals, and plumbingAssignee: David Cook <dcook>
Status: Pushed to oldoldoldstable --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: dcook, didier.gautheron, fridolin.somers, jesse, jonathan.druart, katrin.fischer, laurent.ducos, lisette, lucas, m.de.rooy, martin.renvoize, nick, phil, philippe.blouin, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40522
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40525
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:
Adds a custom function to ensure cleaner outputs when using DateTable's print button.
Version(s) released in:
25.11.00,25.05.06,24.11.11,24.05.16,22.11.33
Circulation function:
Bug Depends on:    
Bug Blocks: 41137    
Attachments: Bug 40524: Update print format to use built-in formatter
Bug 40524: Update print format to use built-in formatter
Bug 40524: Update print format to use built-in formatter
Bug 40524: [24.11.x] Update print format to use built-in formatter
Bug 40524: [24.05.x] Update print format to use built-in formatter
Bug 40524: [22.11.x] Update print format to use built-in formatter
Bug 40524: [24.05.x] Update print format to use built-in formatter

Description David Cook 2025-07-28 03:08:42 UTC
Like bug 40522, it's possible to execute stored XSS in a DataTable when you press the "Print" button in a datatable.
Comment 3 David Cook 2025-07-28 03:30:56 UTC
Created attachment 184697 [details] [review]
Bug 40524: Update print format to use built-in formatter

This change updates the print format to use the built-in formatter
for operations like trim, strip HTML, strip newlines, and not
decoding entities.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/acqui/basket.pl?basketno=1
2. Click "add to basket"
3. Type "test" next to "From an existing record" and click "Submit"
4. On the "Gairm" row, click "Add order"
5. Fill in the required fields
6. In the "Internal note" put the following:
abc123
7. Click "Save"
8. In the Orders table, click "Export" and then "Print"
9. Note that the print page loads correctly, you can see the internal note,
and you cannot see lines for "Add internal note",
"Edit internal note", or "Add vendor note"
Comment 4 Brendan Lawlor 2025-10-08 19:29:51 UTC
Created attachment 187597 [details] [review]
Bug 40524: Update print format to use built-in formatter

This change updates the print format to use the built-in formatter
for operations like trim, strip HTML, strip newlines, and not
decoding entities.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/acqui/basket.pl?basketno=1
2. Click "add to basket"
3. Type "test" next to "From an existing record" and click "Submit"
4. On the "Gairm" row, click "Add order"
5. Fill in the required fields
6. In the "Internal note" put the following:
abc123
7. Click "Save"
8. In the Orders table, click "Export" and then "Print"
9. Note that the print page loads correctly, you can see the internal note,
and you cannot see lines for "Add internal note",
"Edit internal note", or "Add vendor note"

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 5 Marcel de Rooy 2025-10-17 09:26:37 UTC
What is the impact on the print results here?
Since you remove html..
Comment 6 Marcel de Rooy 2025-10-24 07:57:37 UTC
Needs feedback. Changing status
Comment 7 David Cook 2025-10-27 00:33:07 UTC
(In reply to Marcel de Rooy from comment #5)
> What is the impact on the print results here?
> Since you remove html..

(In reply to Marcel de Rooy from comment #6)
> Needs feedback. Changing status

Yeah, good question. I should've written the test plan better. Instead of adding "abc123" to the "Internal note" try something like this:
<p>This is HTML text.</p>This is plain text.

When you go to print, you'll see the following:

Internal note:
This is HTML text.

This is plain text.

So rather than scrubbing the whole element, you are still getting your inner text content. 

Hopefully that answers the question. I'll reset the status for this one and bug 40522. Let me know if you need any more clarification.
Comment 8 Marcel de Rooy 2025-10-28 13:36:39 UTC
(In reply to David Cook from comment #7)
> So rather than scrubbing the whole element, you are still getting your inner
> text content. 
> 
> Hopefully that answers the question. I'll reset the status for this one and
> bug 40522. Let me know if you need any more clarification.

Couldnt we just scrub the bad/untrusted elements/content instead of removing all html and leaving the associated styles? This feels like a completely different user experience with just plain text.
Comment 9 David Cook 2025-10-28 23:12:32 UTC
After having some issues around backporting for bug 40525, I want to refactor this to avoid headaches for RMaints (and me).
Comment 10 David Cook 2025-10-28 23:21:16 UTC
(In reply to Marcel de Rooy from comment #8)
> Couldnt we just scrub the bad/untrusted elements/content instead of removing
> all html and leaving the associated styles? This feels like a completely
> different user experience with just plain text.

I'm not sure I 100% understand what you're saying, but I think I understand.

When it comes to scrubbing, it's significantly more difficult to have a deny list than an allow list. It's not really feasible to just scrub bad/untrusted elements/content. But I take your point about the UX because it might be that there are some HTML/style that Koha admins/users expect to come through.

Typically in Koha we use HTML::Scrubber via C4::Scrubber which is Perl-based and it's great. But then we run into problems with DataTables when content isn't Perl-generated.
Comment 11 David Cook 2025-10-28 23:26:24 UTC
Regarding UX, I just took a look on master, and we use a "html" filter on "Internal note", so on /cgi-bin/koha/acqui/basket.pl?basketno=1 you'll see this:

Internal note: <b>Internal note2</b>

When you go to print, then you see the rendered HTML so it's the following where Internal note2 is bolded:

Internal note: Internal note2

--

So really the UX would be able the same. The only difference being that the HTML markup is stripped rather than escaped.

I think that DataTable.Buttons.stripData() is used by default in DataTables. We have to invoke it manually in this patch only because we provide our own formatter (to remove elements with .no_export class for instance). So I'm not really doing anything too crazy here. I'm just re-adding some built-in functionality.
Comment 12 David Cook 2025-10-28 23:27:01 UTC
Note also that this is the staff side equivalent of bug 40522 that already has a CVE taken out on it.
Comment 13 Marcel de Rooy 2025-10-29 10:17:16 UTC
(In reply to David Cook from comment #10)
> When it comes to scrubbing, it's significantly more difficult to have a deny
> list than an allow list. It's not really feasible to just scrub
> bad/untrusted elements/content. But I take your point about the UX because
> it might be that there are some HTML/style that Koha admins/users expect to
> come through.
> 
> Typically in Koha we use HTML::Scrubber via C4::Scrubber which is Perl-based
> and it's great. But then we run into problems with DataTables when content
> isn't Perl-generated.

Something like https://www.npmjs.com/package/sanitize-html ?
Maybe the standard allowedTags are sufficient.
Or https://github.com/cure53/DOMPurify
Comment 14 David Cook 2025-10-29 23:36:25 UTC
(In reply to Marcel de Rooy from comment #13)
> (In reply to David Cook from comment #10)
> > When it comes to scrubbing, it's significantly more difficult to have a deny
> > list than an allow list. It's not really feasible to just scrub
> > bad/untrusted elements/content. But I take your point about the UX because
> > it might be that there are some HTML/style that Koha admins/users expect to
> > come through.
> > 
> > Typically in Koha we use HTML::Scrubber via C4::Scrubber which is Perl-based
> > and it's great. But then we run into problems with DataTables when content
> > isn't Perl-generated.
> 
> Something like https://www.npmjs.com/package/sanitize-html ?
> Maybe the standard allowedTags are sufficient.
> Or https://github.com/cure53/DOMPurify

I can't speak to https://www.npmjs.com/package/sanitize-html, but OWASP does recommend DOMPurify, and it is something I have suggested in the past in the Koha community, but no one else seemed interested.

Working on security in Koha is often like shouting into the void.
Comment 15 David Cook 2025-10-30 00:21:58 UTC
(In reply to Marcel de Rooy from comment #8)
> Couldnt we just scrub the bad/untrusted elements/content instead of removing
> all html and leaving the associated styles? This feels like a completely
> different user experience with just plain text.

Regarding user experience, could you perhaps provide some screenshots or some concrete examples of how it feels like a different user experience? Because so far I think it's looked identical to me before/after the patch?
Comment 16 David Cook 2025-10-30 00:27:10 UTC
(In reply to Marcel de Rooy from comment #8)
> (In reply to David Cook from comment #7)
> > So rather than scrubbing the whole element, you are still getting your inner
> > text content. 
> > 
> > Hopefully that answers the question. I'll reset the status for this one and
> > bug 40522. Let me know if you need any more clarification.
> 
> Couldnt we just scrub the bad/untrusted elements/content instead of removing
> all html and leaving the associated styles? This feels like a completely
> different user experience with just plain text.

Thinking back on the amount of time and energy I've invested into this code... I'm happy to review other people's alternatives, but this is the only code I am prepared to write for this issue. I'd rather work on other issues than re-write this one.
Comment 17 Marcel de Rooy 2025-10-30 07:03:50 UTC
(In reply to David Cook from comment #15)
> (In reply to Marcel de Rooy from comment #8)
> > Couldnt we just scrub the bad/untrusted elements/content instead of removing
> > all html and leaving the associated styles? This feels like a completely
> > different user experience with just plain text.
> 
> Regarding user experience, could you perhaps provide some screenshots or
> some concrete examples of how it feels like a different user experience?
> Because so far I think it's looked identical to me before/after the patch?

I dont have a concrete example. Just thinking about possible reactions after pushing this. We are going back to plain text. So it is obvious that we lose styles/formatting.
Maybe we should now balance security versus presentation. And security always wins.

(In reply to David Cook from comment #16)

> Thinking back on the amount of time and energy I've invested into this
> code... I'm happy to review other people's alternatives, but this is the
> only code I am prepared to write for this issue. I'd rather work on other
> issues than re-write this one.

This is by no means meant to stop this development btw.
Comment 18 Marcel de Rooy 2025-10-30 07:05:15 UTC
(In reply to David Cook from comment #14)
> I can't speak to https://www.npmjs.com/package/sanitize-html, but OWASP does
> recommend DOMPurify, and it is something I have suggested in the past in the
> Koha community, but no one else seemed interested.
> 
> Working on security in Koha is often like shouting into the void.

Unfortunately these seem more general issues than just security.
Comment 19 David Cook 2025-10-31 00:13:07 UTC
(In reply to Marcel de Rooy from comment #17)
> I dont have a concrete example. Just thinking about possible reactions after
> pushing this. We are going back to plain text. So it is obvious that we lose
> styles/formatting.
> Maybe we should now balance security versus presentation. And security
> always wins.

I appreciate that you're thinking of all the different angles. I think that's a key factor in being good at QA :). Perhaps I have tunnel vision on this one.

That said, in practice, I don't think that we are losing styles/formatting,  but I admit that I haven't done a comprehensive review. In the basket print output, I can't see any loss of style/formatting before/after the patch. But... maybe there is a different DataTable that I haven't seen yet where that might happen.  

/cgi-bin/koha/admin/branches.pl seems OK
/cgi-bin/koha/admin/categories.pl seems OK
/cgi-bin/koha/admin/itemtypes.pl seems OK

These are fairly basic tables though. Trying to think of a table with a lot of content that might be stylized... but really I can't think of when that would happen, because all the datatables should have fairly atomic data in their cells. All the DataTables are already using plain text in the print output (except for the case where I've managed to sneak in HTML to perform the XSS exploit).

I guess I'm struggling with the idea that we're losing styles/formatting when there's no practical evidence to suggest that is true?
 
> (In reply to David Cook from comment #16)
> This is by no means meant to stop this development btw.

Thanks< Marcel. I appreciate that. Apologies for my grumpiness.
Comment 20 Marcel de Rooy 2025-10-31 08:17:06 UTC
Created attachment 188757 [details] [review]
Bug 40524: Update print format to use built-in formatter

This change updates the print format to use the built-in formatter
for operations like trim, strip HTML, strip newlines, and not
decoding entities.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/acqui/basket.pl?basketno=1
2. Click "add to basket"
3. Type "test" next to "From an existing record" and click "Submit"
4. On the "Gairm" row, click "Add order"
5. Fill in the required fields
6. In the "Internal note" put the following:
abc123
7. Click "Save"
8. In the Orders table, click "Export" and then "Print"
9. Note that the print page loads correctly, you can see the internal note,
and you cannot see lines for "Add internal note",
"Edit internal note", or "Add vendor note"

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2025-10-31 08:18:57 UTC
Pragmatic choice. In view of provided arguments no practical basis to hold this back for presentational reasons where we should favor security. Unclear enough :)
Comment 22 Marcel de Rooy 2025-10-31 08:21:17 UTC
(In reply to Marcel de Rooy from comment #20)
> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>

@Brendan: Would you care to test/signoff the 40522 counterpart ?
Comment 23 Lucas Gass (lukeg) 2025-12-01 15:51:44 UTC
Created attachment 190054 [details] [review]
Bug 40524: [24.11.x] Update print format to use built-in formatter

This change updates the print format to use the built-in formatter
for operations like trim, strip HTML, strip newlines, and not
decoding entities.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/acqui/basket.pl?basketno=1
2. Click "add to basket"
3. Type "test" next to "From an existing record" and click "Submit"
4. On the "Gairm" row, click "Add order"
5. Fill in the required fields
6. In the "Internal note" put the following:
abc123
7. Click "Save"
8. In the Orders table, click "Export" and then "Print"
9. Note that the print page loads correctly, you can see the internal note,
and you cannot see lines for "Add internal note",
"Edit internal note", or "Add vendor note"

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Lucas Gass (lukeg) 2025-12-01 15:54:14 UTC
Created attachment 190055 [details] [review]
Bug 40524: [24.05.x] Update print format to use built-in formatter

This change updates the print format to use the built-in formatter
for operations like trim, strip HTML, strip newlines, and not
decoding entities.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/acqui/basket.pl?basketno=1
2. Click "add to basket"
3. Type "test" next to "From an existing record" and click "Submit"
4. On the "Gairm" row, click "Add order"
5. Fill in the required fields
6. In the "Internal note" put the following:
abc123
7. Click "Save"
8. In the Orders table, click "Export" and then "Print"
9. Note that the print page loads correctly, you can see the internal note,
and you cannot see lines for "Add internal note",
"Edit internal note", or "Add vendor note"

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 Lucas Gass (lukeg) 2025-12-01 15:56:54 UTC
Created attachment 190056 [details] [review]
Bug 40524: [22.11.x] Update print format to use built-in formatter

This change updates the print format to use the built-in formatter
for operations like trim, strip HTML, strip newlines, and not
decoding entities.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/acqui/basket.pl?basketno=1
2. Click "add to basket"
3. Type "test" next to "From an existing record" and click "Submit"
4. On the "Gairm" row, click "Add order"
5. Fill in the required fields
6. In the "Internal note" put the following:
abc123
7. Click "Save"
8. In the Orders table, click "Export" and then "Print"
9. Note that the print page loads correctly, you can see the internal note,
and you cannot see lines for "Add internal note",
"Edit internal note", or "Add vendor note"

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Lucas Gass (lukeg) 2025-12-01 17:29:10 UTC
Created attachment 190061 [details] [review]
Bug 40524: [24.05.x] Update print format to use built-in formatter

This change updates the print format to use the built-in formatter
for operations like trim, strip HTML, strip newlines, and not
decoding entities.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/acqui/basket.pl?basketno=1
2. Click "add to basket"
3. Type "test" next to "From an existing record" and click "Submit"
4. On the "Gairm" row, click "Add order"
5. Fill in the required fields
6. In the "Internal note" put the following:
abc123
7. Click "Save"
8. In the Orders table, click "Export" and then "Print"
9. Note that the print page loads correctly, you can see the internal note,
and you cannot see lines for "Add internal note",
"Edit internal note", or "Add vendor note"

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Jesse Maseto 2025-12-01 21:02:50 UTC
Thank you lucas. This will be in 24.05.16
Comment 28 Lucas Gass (lukeg) 2025-12-04 22:57:29 UTC
Pushed to main for 25.11.00