Bug 36328

Summary: C4::Scrubber should allow more HTML tags
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: Needs documenting --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: david, dcook, fridolin.somers, martin.renvoize, wainuiwitikapark
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38488
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38498
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.05,23.05.11,22.05.21
Circulation function:
Bug Depends on:    
Bug Blocks: 40079, 19613, 38499    
Attachments: Bug 36328: Add p span div to Scrubber
Bug 36328: Add test
Bug 36328: Add a separate 'note' profile
Bug 36328: Add p span div to Scrubber
Bug 36328: Add test
Bug 36328: Add a separate 'note' profile
Bug 36328: Add p span div to Scrubber
Bug 36328: Add test
Bug 36328: Add a separate 'note' profile
Bug 36328: (QA follow-up) Expand tests and reorder elements to clarify differences

Description Jonathan Druart 2024-03-15 10:38:59 UTC
Should not we allow span, div, etc. as well as existing "br b i em big small strong"?
Comment 1 Jonathan Druart 2024-03-15 10:41:33 UTC
Created attachment 163192 [details] [review]
Bug 36328: Add p span div to Scrubber
Comment 2 Jonathan Druart 2024-03-15 10:41:36 UTC
Created attachment 163193 [details] [review]
Bug 36328: Add test
Comment 3 David Cook 2024-03-17 23:35:57 UTC
I think it probably makes sense to have a few different profiles. 

For the "comment" scrubber, we probably want to keep it fairly minimal I think.

Whereas "borrowernotes" and "opacnote" could have more elaborate structures. 

Bug 31934 has a profile with a fairly comprehensive list (although I wouldn't use it for "borrowernotes" or "opacnote"). 

I think an argument could be made for also including headings, table elements, fieldset/legend, ul/ol/li.
Comment 4 Martin Renvoize (ashimema) 2024-03-19 12:58:00 UTC
We could really do with getting bug 31934 moving again I think..
Comment 5 Jonathan Druart 2024-03-20 07:34:23 UTC
Created attachment 163497 [details] [review]
Bug 36328: Add a separate 'note' profile
Comment 6 Jonathan Druart 2024-03-20 07:34:50 UTC
Would this patch work for you? Here I am trying to tackle down bug 19613.
Comment 7 David Cook 2024-03-21 00:10:03 UTC
(In reply to Jonathan Druart from comment #6)
> Would this patch work for you? Here I am trying to tackle down bug 19613.

Looks good. I'll look at signing this off pronto...
Comment 8 David Cook 2024-03-21 00:42:35 UTC
Created attachment 163582 [details] [review]
Bug 36328: Add p span div to Scrubber

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 9 David Cook 2024-03-21 00:42:37 UTC
Created attachment 163583 [details] [review]
Bug 36328: Add test

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 10 David Cook 2024-03-21 00:42:40 UTC
Created attachment 163584 [details] [review]
Bug 36328: Add a separate 'note' profile

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 11 Nick Clemens (kidclamp) 2024-04-01 16:11:39 UTC
Created attachment 164206 [details] [review]
Bug 36328: Add p span div to Scrubber

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Nick Clemens (kidclamp) 2024-04-01 16:11:41 UTC
Created attachment 164207 [details] [review]
Bug 36328: Add test

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Nick Clemens (kidclamp) 2024-04-01 16:11:43 UTC
Created attachment 164208 [details] [review]
Bug 36328: Add a separate 'note' profile

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Nick Clemens (kidclamp) 2024-04-01 16:11:45 UTC
Created attachment 164209 [details] [review]
Bug 36328: (QA follow-up) Expand tests and reorder elements to clarify differences

Also tidy

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Katrin Fischer 2024-04-22 10:40:19 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 16 Wainui Witika-Park 2024-04-24 01:50:44 UTC
Applied to 22.05.x
Comment 17 David Cook 2024-11-20 00:35:47 UTC
In hindsight, perhaps we should've allowed "id" and "class" attributes through for the "note" profile to allow people to easily add styling via CSS...
Comment 18 Katrin Fischer 2024-11-20 10:39:55 UTC
(In reply to David Cook from comment #17)
> In hindsight, perhaps we should've allowed "id" and "class" attributes
> through for the "note" profile to allow people to easily add styling via
> CSS...

Yes - new bug?
Comment 19 David Cook 2024-11-20 22:59:52 UTC
Comment on attachment 164208 [details] [review]
Bug 36328: Add a separate 'note' profile

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

::: C4/Scrubber.pm
@@ +50,4 @@
>          rules   => exists $settings->{rules} ? $settings->{rules} : [],
>          default => exists $settings->{default} ? $settings->{default} : [ 0 => { '*' => 0 } ],
>          comment => exists $settings->{comment} ? $settings->{comment} : 0,
> +        note    => exists $settings->{note} ? $settings->{note} : 0,

This line actually looks like it doesn't do anything. I'll open a bug to remove it as well.
Comment 20 David Cook 2024-11-20 23:02:05 UTC
(In reply to Katrin Fischer from comment #18)
> (In reply to David Cook from comment #17)
> > In hindsight, perhaps we should've allowed "id" and "class" attributes
> > through for the "note" profile to allow people to easily add styling via
> > CSS...
> 
> Yes - new bug?

Bug 38498
Comment 21 Martin Renvoize (ashimema) 2025-06-05 09:04:46 UTC
I'm also going to add a bug asking for ul/ol and li. We see those used quite often and I can't see a security issue with those?
Comment 22 Martin Renvoize (ashimema) 2025-06-05 10:16:54 UTC
Added bug 40079 for ol/ul and li tags
Comment 23 David Nind 2025-07-01 10:08:30 UTC
Enhancement - the manual needs updating.