Bug 40079

Summary: C4::Scrubber "note" profile should allow for list (ul, ol, li, dl, dt, and dd) HTML tags
Product: Koha Reporter: Martin Renvoize (ashimema) <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize (ashimema) <martin.renvoize>
Status: Pushed to oldstable --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: david, dcook, fridolin.somers, lucas, m.de.rooy, mirjam.vantieghem
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22501
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23978
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38499
GIT URL: Change sponsored?: ---
Patch complexity: Small patch Documentation contact:
Documentation submission: Text to go in the release notes:
This adds unordered, ordered, and description list tags (<ul>, <ol>, <li>, <dl>, <dt>, and <dd>) to the HTML that is allowed in notes fields (for example, course reserves staff and public notes).
Version(s) released in:
25.11.00,25.05.02,24.11.08
Circulation function:
Bug Depends on: 36328    
Bug Blocks:    
Attachments: Bug 40079: Add ul/ol and li to allowed tags in notes profile
Bug 40079: Add ul/ol and li to allowed tags in notes profile
Bug 40079: Add ul/ol and li to allowed tags in notes profile
Bug 40079: (follow-up) Add dl, dt, dd tags to notes profile scrubber
Bug 40079: (follow-up) Update unit test
Bug 40079: (follow-up) Improve test coverage and fix undef handling
Bug 40079: Add ul/ol and li to allowed tags in notes profile
Bug 40079: (follow-up) Add dl, dt, dd tags to notes profile scrubber
Bug 40079: (follow-up) Update unit test
Bug 40079: (follow-up) Improve test coverage and fix undef handling

Description Martin Renvoize (ashimema) 2025-06-05 09:13:48 UTC
We added support for some additional tags support in the 'note' type scrubbed fields, but we didn't expose lists.

I see no security risk with allowing those tags, please correct me if I'm incorrect there, so I think we should allow these too.
Comment 1 Martin Renvoize (ashimema) 2025-06-05 10:07:22 UTC
Created attachment 182985 [details] [review]
Bug 40079: Add ul/ol and li to allowed tags in notes profile

This patch add the unordered list, ordered list and list item html tags
to the list of allowed tags in C4::Scrubber.

Test plan:
1) Add a course reserve and include the following in the public note
   field:
   <ul>
     <li>First item</li>
     <li>Second item</li>
   </ul>
2) View the course details page and note "First item Second item" all
   appears on the same line (the ul and li tags have been stripped)
3) View the course reserves page and note the same issue in the public
   notes field of the table display
4) Apply this patch
5) Repeat 2 and 3 and note you now have a bullet pointed list displayed
   in both places
6) The display of the note on the details page may look mis-aligned..
   that will be dealt with on it's own bug
Comment 2 Martin Renvoize (ashimema) 2025-06-05 10:27:41 UTC
*** Bug 38499 has been marked as a duplicate of this bug. ***
Comment 3 Phil Ringnalda 2025-06-05 19:24:22 UTC
*** Bug 40077 has been marked as a duplicate of this bug. ***
Comment 4 Phil Ringnalda 2025-06-05 19:24:45 UTC
*** Bug 40078 has been marked as a duplicate of this bug. ***
Comment 5 David Nind 2025-06-05 22:04:19 UTC
Created attachment 183014 [details] [review]
Bug 40079: Add ul/ol and li to allowed tags in notes profile

This patch add the unordered list, ordered list and list item html tags
to the list of allowed tags in C4::Scrubber.

Test plan:
1) Add a course reserve and include the following in the public note
   field:
   <ul>
     <li>First item</li>
     <li>Second item</li>
   </ul>
2) View the course details page and note "First item Second item" all
   appears on the same line (the ul and li tags have been stripped)
3) View the course reserves page and note the same issue in the public
   notes field of the table display
4) Apply this patch
5) Repeat 2 and 3 and note you now have a bullet pointed list displayed
   in both places
6) The display of the note on the details page may look mis-aligned..
   that will be dealt with on it's own bug

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2025-06-05 22:14:04 UTC
I've signed off, as things work as per the test plan.

However, I noticed that the description for the bug says it also allows ordered lists (<ol>) and description list (<dl>, <dt>, and <dd>).

Only unordered lists work for me - looking at the patch, only ul and li were added.
Comment 7 David Cook 2025-06-06 01:03:11 UTC
Sounds like a reasonable change to me, but it does seem like we need some clarification.

David: I don't the original description mentioned description lists per se? But I do see the patch says it adds "unordered list, ordered list and list item html tags", but it only adds "ul" and "li" - not "ol".

Martin: Did you intend to add "ol" as well?
Comment 8 David Cook 2025-06-06 01:06:01 UTC
I also like how this patch removes the useless "note" parameter from the constructor/new method (bug 38499). I've wanted that done for a while.

I'm going to update the title of this bug to specify the note profile.
Comment 9 David Nind 2025-06-06 01:28:50 UTC
(In reply to David Cook from comment #7)
> David: I don't the original description mentioned description lists per se?
> But I do see the patch says it adds "unordered list, ordered list and list
> item html tags", but it only adds "ul" and "li" - not "ol".

I meant the description in the commit message 8-).
Comment 10 David Cook 2025-06-06 01:43:04 UTC
Actually, could we get the t/Scrubber.t updated too? It looks like there's an existing test for the 'note' profile, although it's not a brilliant test. 

I'm also happy to do that if you don't want to. (Obviously the C4::Scrubber has turned into a bit of a passion for me...)
Comment 11 Martin Renvoize (ashimema) 2025-06-26 14:37:50 UTC
Created attachment 183534 [details] [review]
Bug 40079: Add ul/ol and li to allowed tags in notes profile

This patch add the unordered list, ordered list and list item html tags
to the list of allowed tags in C4::Scrubber.

Test plan:
1) Add a course reserve and include the following in the public note
   field:
   <ul>
     <li>First item</li>
     <li>Second item</li>
   </ul>
2) View the course details page and note "First item Second item" all
   appears on the same line (the ul and li tags have been stripped)
3) View the course reserves page and note the same issue in the public
   notes field of the table display
4) Apply this patch
5) Repeat 2 and 3 and note you now have a bullet pointed list displayed
   in both places
6) The display of the note on the details page may look mis-aligned..
   that will be dealt with on it's own bug

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 Martin Renvoize (ashimema) 2025-06-26 14:37:52 UTC
Created attachment 183535 [details] [review]
Bug 40079: (follow-up) Add dl, dt, dd tags to notes profile scrubber

Adds support for HTML description list elements (dl, dt, dd) to the
'note' profile in the HTML scrubber configuration. This allows
librarians to use description lists in note fields for better
structured content presentation.
Comment 13 Martin Renvoize (ashimema) 2025-06-26 14:37:54 UTC
Created attachment 183536 [details] [review]
Bug 40079: (follow-up) Update unit test
Comment 14 Martin Renvoize (ashimema) 2025-06-26 14:37:56 UTC
Created attachment 183537 [details] [review]
Bug 40079: (follow-up) Improve test coverage and fix undef handling

This patch modernizes the Scrubber.t unit test with comprehensive
coverage following Koha best practices:

- Uses subtest structure for logical grouping
- Tests all scrubber types (default, comment, note)
- Includes security testing for XSS prevention
- Covers edge cases and error handling
- Tests new list elements added in previous commits

Also fixes C4::Scrubber to handle undef parameters cleanly by
treating them as 'default' type instead of generating warnings.

Test plan:
1. Run prove t/Scrubber.t
2. Verify all tests pass without warnings
3. Confirm comprehensive coverage of module functionality
Comment 15 Martin Renvoize (ashimema) 2025-06-26 14:39:52 UTC
Ooops.. seems I submitted the patch without ol.. my mistake, could have swarn I'd added it.

Anyway, thanks or the testing and spotting that. I've corrected that inline and I've also added a follow-up to add the definitions list options.. Finally I've fixed the existing tests in one follow-up and then completely reworked them in a second follow-up which drastically increases our coverage and spots an edge case we hadn't found before which I've also fixed inline.
Comment 16 David Nind 2025-06-26 19:06:02 UTC
Thanks Martin!

I can confirm that ordered list and description list tags now work as well!

David
Comment 17 Marcel de Rooy 2025-06-27 07:39:32 UTC
Created attachment 183583 [details] [review]
Bug 40079: Add ul/ol and li to allowed tags in notes profile

This patch add the unordered list, ordered list and list item html tags
to the list of allowed tags in C4::Scrubber.

Test plan:
1) Add a course reserve and include the following in the public note
   field:
   <ul>
     <li>First item</li>
     <li>Second item</li>
   </ul>
2) View the course details page and note "First item Second item" all
   appears on the same line (the ul and li tags have been stripped)
3) View the course reserves page and note the same issue in the public
   notes field of the table display
4) Apply this patch
5) Repeat 2 and 3 and note you now have a bullet pointed list displayed
   in both places
6) The display of the note on the details page may look mis-aligned..
   that will be dealt with on it's own bug

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2025-06-27 07:39:34 UTC
Created attachment 183584 [details] [review]
Bug 40079: (follow-up) Add dl, dt, dd tags to notes profile scrubber

Adds support for HTML description list elements (dl, dt, dd) to the
'note' profile in the HTML scrubber configuration. This allows
librarians to use description lists in note fields for better
structured content presentation.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2025-06-27 07:39:36 UTC
Created attachment 183585 [details] [review]
Bug 40079: (follow-up) Update unit test

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2025-06-27 07:39:38 UTC
Created attachment 183586 [details] [review]
Bug 40079: (follow-up) Improve test coverage and fix undef handling

This patch modernizes the Scrubber.t unit test with comprehensive
coverage following Koha best practices:

- Uses subtest structure for logical grouping
- Tests all scrubber types (default, comment, note)
- Includes security testing for XSS prevention
- Covers edge cases and error handling
- Tests new list elements added in previous commits

Also fixes C4::Scrubber to handle undef parameters cleanly by
treating them as 'default' type instead of generating warnings.

Test plan:
1. Run prove t/Scrubber.t
2. Verify all tests pass without warnings
3. Confirm comprehensive coverage of module functionality

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 David Cook 2025-06-30 00:08:48 UTC
Comment on attachment 183586 [details] [review]
Bug 40079: (follow-up) Improve test coverage and fix undef handling

Review of attachment 183586 [details] [review]:
-----------------------------------------------------------------

::: C4/Scrubber.pm
@@ +35,4 @@
>  sub new {
>      shift;            # ignore our class we are wrapper
>      my $type = (@_) ? shift : 'default';
> +    $type = 'default' if !defined $type;

Nice one. I have been thinking this would be a good addition, so glad to see it.
Comment 22 Lucas Gass (lukeg) 2025-06-30 14:32:31 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 23 Paul Derscheid 2025-07-10 20:50:34 UTC
Nice work everyone!

Pushed to 25.05.x
Comment 24 Fridolin Somers 2025-07-25 14:50:27 UTC
Pushed to 24.11.x for 24.11.08