Bug 39860 - Add a way to allow for additional/custom MARC fields in the record display
Summary: Add a way to allow for additional/custom MARC fields in the record display
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic data support (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Lucas Gass (lukeg)
QA Contact: Testopia
URL:
Keywords:
Depends on: 39184 40089
Blocks:
  Show dependency treegraph
 
Reported: 2025-05-07 21:35 UTC by Lucas Gass (lukeg)
Modified: 2025-07-04 01:41 UTC (History)
12 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 39860: Add ExtraContentForXSLTDisplay system preference (3.58 KB, patch)
2025-05-07 21:43 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39860: Allow for the defining of custom MARC fields on the staff interface record display pages (4.30 KB, patch)
2025-05-07 21:43 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39860: Add ExtraContentForXSLTDisplay system preference (3.58 KB, patch)
2025-05-07 21:47 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39860: Allow for the defining of custom MARC fields on the staff interface record display pages (3.95 KB, patch)
2025-05-07 21:47 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39860: Use HTML scrubber to sanitize HTML (1.32 KB, patch)
2025-05-07 22:25 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39860: Fix scrubbing and handling of HTML/TT in MARC21 xsl (6.11 KB, patch)
2025-05-08 00:16 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39860: Do not XSLT transform the custom content (2.23 KB, patch)
2025-05-09 21:25 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39860: Do not XSLT transform the custom content (6.83 KB, patch)
2025-05-09 21:34 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39860: Add ExtraContentForXSLTDisplay system preference (3.58 KB, patch)
2025-05-09 21:42 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39860: Allow for the defining of custom MARC fields on the staff interface record display pages (3.08 KB, patch)
2025-05-09 21:42 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39860: Move display control from system preference to additional contents (8.11 KB, patch)
2025-06-05 20:19 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39860: Allow for the defining of custom MARC fields on the staff interface record display pages (3.84 KB, patch)
2025-06-05 20:33 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39860: Move display control from system preference to additional contents (8.11 KB, patch)
2025-06-05 20:33 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39860: Fix OPAC display location (1.05 KB, patch)
2025-06-05 20:39 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39860: Add options for opac/staff lists (2.14 KB, patch)
2025-06-05 20:55 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Detail page after adding 538a (40.57 KB, image/png)
2025-06-06 05:54 UTC, Anneli Österman
Details
Bug 39860: Add record_display srubber profile (3.91 KB, patch)
2025-06-06 17:32 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
StaffDetailPage (40.67 KB, image/png)
2025-06-09 06:40 UTC, Anneli Österman
Details
Bug 39860: Add record_display srubber profile (4.21 KB, patch)
2025-07-03 17:49 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass (lukeg) 2025-05-07 21:35:48 UTC
It would be nice if we could define additional MARC fields in a system preference via Template Toolkit and then have them display on the results/detail pages.
Comment 1 Lucas Gass (lukeg) 2025-05-07 21:43:39 UTC
Created attachment 182054 [details] [review]
Bug 39860: Add ExtraContentForXSLTDisplay system preference
Comment 2 Lucas Gass (lukeg) 2025-05-07 21:43:41 UTC
Created attachment 182055 [details] [review]
Bug 39860: Allow for the defining of custom MARC fields on the staff interface record display pages

To test:
1. APPLY PATCH, updatedatabase, restart_all
2. Search for the new 'ExtraContentForXSLTDisplay' system preference
3. Find or create a record that has some MARC fields that don't show up by default in the record display. ( I tested using a 538$a )
4. Add the following to the ExtraContentForXSLTDisplay sys pref:

[% IF record.subfield('538' , 'a') %]
  <span class="results_summary test1">
      <span class="label">The 538a: </span>
      [% record.subfield('538' , 'a') %]
  </span>
[% END %]

5. Go to the record from step 3, you should see the 538$a labeled as 'The 538a:' with the appropriate data in it.
Comment 3 Lucas Gass (lukeg) 2025-05-07 21:47:03 UTC
Created attachment 182056 [details] [review]
Bug 39860: Add ExtraContentForXSLTDisplay system preference
Comment 4 Lucas Gass (lukeg) 2025-05-07 21:47:05 UTC
Created attachment 182057 [details] [review]
Bug 39860: Allow for the defining of custom MARC fields on the staff interface record display pages

To test:
1. APPLY PATCH, updatedatabase, restart_all
2. Search for the new 'ExtraContentForXSLTDisplay' system preference
3. Find or create a record that has some MARC fields that don't show up by default in the record display. ( I tested using a 538$a )
4. Add the following to the ExtraContentForXSLTDisplay sys pref:

[% IF record.subfield('538' , 'a') %]
  <span class="results_summary test1">
      <span class="label">The 538a: </span>
      [% record.subfield('538' , 'a') %]
  </span>
[% END %]

5. Go to the record from step 3, you should see the 538$a labeled as 'The 538a:' with the appropriate data in it.
Comment 5 Lucas Gass (lukeg) 2025-05-07 22:25:12 UTC
Created attachment 182058 [details] [review]
Bug 39860: Use HTML scrubber to sanitize HTML
Comment 6 Lucas Gass (lukeg) 2025-05-07 22:34:19 UTC
Still some problems:
-Needs unit tests 
-I tried using HTML scrubber to scrub script tags but it scrubs too much. I want to be able to use most HTML tags, maybe just not JS?
Comment 7 Lucas Gass (lukeg) 2025-05-08 00:16:10 UTC
Created attachment 182059 [details] [review]
Bug 39860: Fix scrubbing and handling of HTML/TT in MARC21 xsl
Comment 9 Lucas Gass (lukeg) 2025-05-08 00:20:28 UTC
For repeating fields:

[% IF record.subfield('538' , 'a') %]
  <span class="results_summary test1">
    <span class="label">The 538a: </span>
    [% FOREACH f IN record.field('538') %]
      [% f.subfield('a') %]
    [% END %]
  </span>
[% END %]
Comment 10 David Cook 2025-05-08 04:14:06 UTC
I've got a few comments:

1.
It looks like you figured out the profile/settings argument for C4::Scrubber in the end, but I'd argue that "staff" is too permissive. 

One could argue since it's a system preference an admin user could already inject malicious JS via *UserJS, but I suppose the danger here is the XSS coming from the MARC record without the admin user's knowledge. I think we'd need a more specific profile/setting for scrubbing this content.

I had a bug report somewhere that allowed for scrubber settings that could be deployed locally for maximum flexibility, but I can't find it right now...

Failing QA for this one.

2.
Why would we need to run this through the XSLTs? 

Wouldn't it be more efficient add it to the return data of C4::XSLT::XSLTParse4Display()? 

(That is, right now we return the return value of $engine->transform(), but we could store that, append to it, and then return that concatenated value. You might have a good reason for not doing that, but I don't see what it is at a glance on my own.)

3.
Technically, we don't use XHTML anymore - we use HTML5 which isn't fully XML-compatible, so in theory someone could enter valid HTML which might cause XSLT errors. I'd be curious how it handles HTML5's "<br>" instead of XHTML's "<br/>" for instance. I wonder if it would croak on there not being a closing tag.

4.
You shouldn't use Koha::TemplateUtils::process_tt() here. I'll DM you with the reason why. 

Also Failed QA for this one.
Comment 11 David Cook 2025-05-08 04:21:12 UTC
(In reply to Lucas Gass (lukeg) from comment #6)
> -I tried using HTML scrubber to scrub script tags but it scrubs too much. I
> want to be able to use most HTML tags, maybe just not JS?

In practice, this is actually pretty challenging to do. 

The obvious one is to restrict <script> tags, but there's lots of other ways of injecting Javascript via other tags and attributes. (I should compile a list one of these days, as it's difficult to keep track of them all, but that's also part of the problem with a list... maintenance of the list.)

Anyway, not going to give away all my security secrets here, but just... yeah it's challenging balancing security and convenience/flexibility.
Comment 12 David Cook 2025-05-08 04:22:11 UTC
Note that once we apply Content-Security-Policy, we'll have a great broad layer that prevents XSS, although there's still other malicious things people could do beyond XSS.
Comment 13 David Cook 2025-05-08 04:28:58 UTC
That all said, I do think this is an interesting idea! In fact, generally speaking, it's a direction that I'm quite keen to explore! I like the ideas of libraries being able to have more direct control over the display of their data.

I just want to make sure we do it in a secure way.
Comment 14 Katrin Fischer 2025-05-08 05:29:00 UTC
Disclaimer: I didn't read David's longer comment, but stumbled on the pref name:
ExtraContentForXSLTDisplay. It made me think I would have to add XSLT, maybe we could use ExtraContentForDetailPage or similar instead?
Comment 15 David Cook 2025-05-08 05:42:38 UTC
(In reply to Katrin Fischer from comment #14)
> Disclaimer: I didn't read David's longer comment

The short version is mostly "there are security problems" and a couple other potential gotchas.
Comment 16 Lucas Gass (lukeg) 2025-05-09 21:25:58 UTC
Created attachment 182230 [details] [review]
Bug 39860: Do not XSLT transform the custom content
Comment 17 Lucas Gass (lukeg) 2025-05-09 21:28:42 UTC
> 2.
> Why would we need to run this through the XSLTs? 
> 
> Wouldn't it be more efficient add it to the return data of
> C4::XSLT::XSLTParse4Display()? 
> 
> (That is, right now we return the return value of $engine->transform(), but
> we could store that, append to it, and then return that concatenated value.
> You might have a good reason for not doing that, but I don't see what it is
> at a glance on my own.)

You're right, no need to do that. I wrote a follow up to append the content you like suggest, after the XSLT transformation.

> 3.
> Technically, we don't use XHTML anymore - we use HTML5 which isn't fully
> XML-compatible, so in theory someone could enter valid HTML which might
> cause XSLT errors. I'd be curious how it handles HTML5's "<br>" instead of
> XHTML's "<br/>" for instance. I wonder if it would croak on there not being
> a closing tag.

This shouldn't be a problem now with the follow up patch not running through the XSLT transformation.
Comment 18 Lucas Gass (lukeg) 2025-05-09 21:34:26 UTC
Created attachment 182231 [details] [review]
Bug 39860: Do not XSLT transform the custom content
Comment 19 Lucas Gass (lukeg) 2025-05-09 21:42:31 UTC
Created attachment 182232 [details] [review]
Bug 39860: Add ExtraContentForXSLTDisplay system preference
Comment 20 Lucas Gass (lukeg) 2025-05-09 21:42:33 UTC
Created attachment 182233 [details] [review]
Bug 39860: Allow for the defining of custom MARC fields on the staff interface record display pages

To test:
1. APPLY PATCH, updatedatabase, restart_all
2. Search for the new 'ExtraContentForXSLTDisplay' system preference
3. Find or create a record that has some MARC fields that don't show up by default in the record display. ( I tested using a 538$a )
4. Add the following to the ExtraContentForXSLTDisplay sys pref:

[% IF record.subfield('538' , 'a') %]
  <span class="results_summary test1">
      <span class="label">The 538a: </span>
      [% record.subfield('538' , 'a') %]
  </span>
[% END %]

5. Go to the record from step 3, you should see the 538$a labeled as 'The 538a:' with the appropriate data in it.
Comment 21 paxed 2025-05-14 04:12:13 UTC
This is interesting, I'll have to look how the xslt changes our libraries want would work with this ...
Comment 22 Lari Strand 2025-05-16 09:16:33 UTC
Could it be possible to pass the user's selected UI/intranet language so we could build translations based on it like:

[% IF record.subfield('020' , 'q') %]
  [% IF lang == 'fi-FI' %]
  <span class="results_summary test1">
      <span class="label">Sidosasu: </span>
      [% record.subfield('020' , 'q') %]
  </span>
  [% ELSIF lang == 'en' %]
  ...

I tried to pass it as a variable in the XSLT.pm module but couldn't get it to show and the lang TT parameter used in other parts of Koha templates was not available. Could be that I'm not using the TT correctly. It feels like we can't use:

[% USE AuthorisedValues %]
[% USE Languages %]

etc. either but my skills with template toolkit end here :).
Comment 23 Lari Strand 2025-05-16 09:25:43 UTC
Our specific need would be to translate codes you get from 942c ( Koha [default] item type ) to a readable form with translations for them.
Comment 24 Lucas Gass (lukeg) 2025-05-16 20:34:49 UTC
(In reply to Lari Strand from comment #23)
> Our specific need would be to translate codes you get from 942c ( Koha
> [default] item type ) to a readable form with translations for them.

(In reply to Lari Strand from comment #22)
> Could it be possible to pass the user's selected UI/intranet language so we
> could build translations based on it like:
> 
> [% IF record.subfield('020' , 'q') %]
>   [% IF lang == 'fi-FI' %]
>   <span class="results_summary test1">
>       <span class="label">Sidosasu: </span>
>       [% record.subfield('020' , 'q') %]
>   </span>
>   [% ELSIF lang == 'en' %]
>   ...
> 
> I tried to pass it as a variable in the XSLT.pm module but couldn't get it
> to show and the lang TT parameter used in other parts of Koha templates was
> not available. Could be that I'm not using the TT correctly. It feels like
> we can't use:
> 
> [% USE AuthorisedValues %]
> [% USE Languages %]
> 
> etc. either but my skills with template toolkit end here :).

Translations are a good point, I think the way to deal with that is probably to move this from a system preference to something like an HTML customization/additional contents.
Comment 25 David Cook 2025-05-18 23:24:43 UTC
(In reply to Lucas Gass (lukeg) from comment #24)
> Translations are a good point, I think the way to deal with that is probably
> to move this from a system preference to something like an HTML
> customization/additional contents.

Yeah I reckon that's probably the way to do it.
Comment 26 Lucas Gass (lukeg) 2025-06-05 20:19:02 UTC
Created attachment 183007 [details] [review]
Bug 39860: Move display control from system preference to additional contents
Comment 27 Lucas Gass (lukeg) 2025-06-05 20:33:16 UTC
Created attachment 183008 [details] [review]
Bug 39860: Allow for the defining of custom MARC fields on the staff interface record display pages

To test:
0. APPLY PATCH, restart_all
1. Find or create a record that has some MARC fields that don't show up by default in the record display. ( I tested using a 538$a )
2. Go to Tools and you should see a new link for 'Record display customizations'.
3. From Record display customizations click 'New Entry'
4. Choose a display location, the choices should be StaffResultsPage, StaffDetailPage, OPACResultsPage, and OPACDetailPage
5. Add the following to the entry:

[% IF record.subfield('538' , 'a') %]
  <span class="results_summary test1">
      <span class="label">The 538a: </span>
      [% record.subfield('538' , 'a') %]
  </span>
[% END %]

6. Go to the record from step 1, you should see the 538$a labeled as 'The 538a:' with the appropriate data in it.
7. This should display in the appropriate location depending on if you chose StaffResultsPage, StaffDetailPage, OPACResultsPage, or OPACDetailPage. Check each of the locations.
8. Make a new, or edit existing, Record display customization and mark the 'Library:' field for a specific branch. Make sure that the Record display customization only shows up when you are logged in at that branch.
9. Install a new language pack, I tested with es-ES.
10. Make a new, or edit existing, Record display customization and fill out the language specific tab. Make sure the Record display customization shows correctly with the language you chose.
Comment 28 Lucas Gass (lukeg) 2025-06-05 20:33:27 UTC
Created attachment 183009 [details] [review]
Bug 39860: Move display control from system preference to additional contents
Comment 29 Lucas Gass (lukeg) 2025-06-05 20:35:53 UTC
Moved this over to additional contents and resetting the status to 'Needs sign-off'.
Comment 30 Lucas Gass (lukeg) 2025-06-05 20:39:44 UTC
Created attachment 183010 [details] [review]
Bug 39860: Fix OPAC display location
Comment 31 Lucas Gass (lukeg) 2025-06-05 20:55:52 UTC
Created attachment 183011 [details] [review]
Bug 39860: Add options for opac/staff lists
Comment 32 David Cook 2025-06-06 01:19:55 UTC
Sorry but I'm going to fail this one again.

I think 3/4 of my original issues from Comment 10 have been resolved, which is awesome.

But I notice we're still using the "staff" profile for the HTML scrubber, and that's not going to prevent XSS, because it allows everything. 

In fact... I don't know why that profile was ever created. It looks like it goes back to the original creating of C4::Scrubber at f8fecb78634

Looking at existing use of the C4::Scrubber... we're using the profiles "note", "comment", and "default". I think we should actually remove the "staff" profile. I'll add a new bug for that...
Comment 33 David Cook 2025-06-06 01:20:25 UTC
Comment on attachment 183008 [details] [review]
Bug 39860: Allow for the defining of custom MARC fields on the staff interface record display pages

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

::: koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2intranetDetail.xsl
@@ +14,3 @@
>      <xsl:template match="/">
>              <xsl:apply-templates/>
>      </xsl:template>

Also this looks like an unnecessary whitespace change to this file.
Comment 34 David Cook 2025-06-06 01:37:34 UTC
(In reply to David Cook from comment #32)
> But I notice we're still using the "staff" profile for the HTML scrubber,
> and that's not going to prevent XSS, because it allows everything. 
> 
> In fact... I don't know why that profile was ever created. It looks like it
> goes back to the original creating of C4::Scrubber at f8fecb78634
> 
> Looking at existing use of the C4::Scrubber... we're using the profiles
> "note", "comment", and "default". I think we should actually remove the
> "staff" profile. I'll add a new bug for that...

I've opened bug 40087 to remove the "staff" profile.

I think this bug report should add its own profile to C4::Scrubber. I know it's hard to think up a list of allowed elements. I do think we need better ways of allowing for per-instance configuration of C4::Scrubber profiles (while retaining sensible secure defaults), but for now... especially since this is a new feature... I think we start with a small list and build up.

I can't find it anymore, but once upon a time I swear I wrote a patch to make C4::Scrubber per-instance configurable, and I had a comprehensive list of safe elements. I'm sorry I can't find it now. 

I'd also encourage you to look at bug 38498. I don't have a patch there, but it's important to keep in mind. That is, we need to block many attributes (for instance, anything that starts with "on"), but it would be nice to be able to use some attributes like "id" and "class" at the very least, so we can use CSS.

Happy to work with you on this one...
Comment 35 Anneli Österman 2025-06-06 05:54:00 UTC
I tested this in a sandbox and it did not work as expected. I added the 538a and the results were like in attached picture detailpage.png.
Comment 36 Anneli Österman 2025-06-06 05:54:55 UTC
Created attachment 183035 [details]
Detail page after adding 538a
Comment 37 Lucas Gass (lukeg) 2025-06-06 13:48:24 UTC
(In reply to Anneli Österman from comment #35)
> I tested this in a sandbox and it did not work as expected. I added the 538a
> and the results were like in attached picture detailpage.png.

What exactly did you add? It looks like it was for the OPAC detail page? I am unable to recreate this.
Comment 38 Lucas Gass (lukeg) 2025-06-06 13:50:18 UTC
(In reply to Anneli Österman from comment #35)
> I tested this in a sandbox and it did not work as expected. I added the 538a
> and the results were like in attached picture detailpage.png.

Oh, I think maybe you copied my example into the WYSIWYG?
Comment 39 Lucas Gass (lukeg) 2025-06-06 16:20:04 UTC
(In reply to David Cook from comment #32)
> Sorry but I'm going to fail this one again.
> 
> I think 3/4 of my original issues from Comment 10 have been resolved, which
> is awesome.
> 
> But I notice we're still using the "staff" profile for the HTML scrubber,
> and that's not going to prevent XSS, because it allows everything. 
> 
> In fact... I don't know why that profile was ever created. It looks like it
> goes back to the original creating of C4::Scrubber at f8fecb78634
> 
> Looking at existing use of the C4::Scrubber... we're using the profiles
> "note", "comment", and "default". I think we should actually remove the
> "staff" profile. I'll add a new bug for that...

I'm just curious why we scrub nothing in HTML customization, News, or Pages ( the other additional contents ) but need to scrub here?
Comment 40 Lucas Gass (lukeg) 2025-06-06 16:33:43 UTC
Maybe what we need is a profile for all additional contents that was based on things to deny, instead of things to allow?
Comment 41 Lucas Gass (lukeg) 2025-06-06 17:32:49 UTC
Created attachment 183077 [details] [review]
Bug 39860: Add record_display srubber profile
Comment 42 Lucas Gass (lukeg) 2025-06-06 17:34:59 UTC
David, 

I added a specific 'record_display' scrubber profile. Let me know if you think that is going to restrictive enough.
Comment 43 Anneli Österman 2025-06-09 04:55:14 UTC
I copied this to the StaffDetailPage HTML customization

[% IF record.subfield('538' , 'a') %]
  <span class="results_summary test1">
      <span class="label">The 538a: </span>
      [% record.subfield('538' , 'a') %]
  </span>
[% END %]


I changed the lable text but otherwise it was like this one. I did not change any system preferences in the sandbox so it propably was using WYSIWYG because by default system preference AdditionalContentsEditor is "a WYSIWYG editor (TinyMCE)"
Comment 44 Anneli Österman 2025-06-09 06:39:36 UTC
I tested this again in StaffDetailPage without the WYSIWYG with this

[% IF record.subfield('538' , 'a') %] The 538a: [% record.subfield('538' , 'a') %] [% END %] [% IF record.subfield('504' , 'a') %] The 504a: [% record.subfield('504' , 'a') %] [% END %]

and it worked as expected (see image staffdetailpage1.png).

If the WYSIWYG mode cannot be used, it should be said clearly somewhere in the documentation when/if this goes to the main.

I did not test things like adding links or showing auhtorised value's description instead of the ID.
Comment 45 Anneli Österman 2025-06-09 06:40:06 UTC
Created attachment 183095 [details]
StaffDetailPage
Comment 46 Anneli Österman 2025-06-09 10:43:17 UTC
(In reply to Anneli Österman from comment #44)
> I tested this again in StaffDetailPage without the WYSIWYG with this
> 
> [% IF record.subfield('538' , 'a') %] The 538a: [% record.subfield('538' ,
> 'a') %] [% END %] [% IF record.subfield('504' , 'a') %] The 504a: [%
> record.subfield('504' , 'a') %] [% END %]
> 
> and it worked as expected (see image staffdetailpage1.png).

Oh, I guess I copied the code when it was in WYSIWYG mode again after the saving and it changed the code to this. *sigh* x)
Comment 47 Anneli Österman 2025-06-09 11:04:17 UTC
Now I had the time to test how this works with authorised values and it does work when done like this:

- Add a new authorised value class/values or use existing one
- Go to Administration -> MARC bibliographic frameworks and make sure the authorised value is used in the MARC field you want to use it
- Find a bibliographic record and add the authorised value to the chosen MARC field
- Add the chosen field to the StaffDetailPage
- Notice that the MARC field is shown with the description of the authorised value

I used this

[% IF record.subfield('049' , 'c') %]
  <span class="results_summary test1">
      <span class="label">Age restriction: </span>
      [% record.subfield('049' , 'c') %]
  </span>
[% END %]

And added new authorised value called "AGE" with values K18, K13 and S and made 049c use it in BK framwork. Then added K13 to the chosen record and the detail page showed it with the description "K13-raja".
Comment 48 David Cook 2025-06-10 00:23:40 UTC
(In reply to Lucas Gass (lukeg) from comment #42)
> David, 
> 
> I added a specific 'record_display' scrubber profile. Let me know if you
> think that is going to restrictive enough.

Hi Lucas,

Thanks for doing that, but unfortunately I don't think that it's going to be restrictive enough. 

The difficulty in trying to do a deny list rather than an allow list is that it's very easy to miss things initially, or for new things to be added in browsers after the initial list is created, which then creates new holes.

For instance, there's many more "on*" event handler attributes that aren't included in the attached deny list (see https://html.spec.whatwg.org/multipage/webappapis.html#event-handlers-on-elements,-document-objects,-and-window-objects). I just did a simple test in my own browser using "oninput" and "oncopy" for instance. They're not as easily abused as the ones on the denylist, but they can still be used to run malicious scripts. There's also ones like "onmouseleave" which are trivial to exploit. 

With the current patch, a number of different HTML elements could also be abused. 

It might be interesting to read Wordpress's code for filtering elements and attributes: https://github.com/WordPress/WordPress/blob/master/wp-includes/kses.php . You can see there that they take an allow list approach. 

Overall, I'd say the creativity of an attacker is going to be greater than our ability as defenders to imagine and defend all the possibilities, so it's best to err on the side of minimalism and restriction.
Comment 49 David Cook 2025-06-10 00:27:20 UTC
(In reply to Anneli Österman from comment #46)
> (In reply to Anneli Österman from comment #44)
> > I tested this again in StaffDetailPage without the WYSIWYG with this
> > 
> > [% IF record.subfield('538' , 'a') %] The 538a: [% record.subfield('538' ,
> > 'a') %] [% END %] [% IF record.subfield('504' , 'a') %] The 504a: [%
> > record.subfield('504' , 'a') %] [% END %]
> > 
> > and it worked as expected (see image staffdetailpage1.png).
> 
> Oh, I guess I copied the code when it was in WYSIWYG mode again after the
> saving and it changed the code to this. *sigh* x)

That's a really interesting observation. I don't think the WYSIWYG can work with Template::Toolkit. 

On bug 11583, I've worked on adding support for Template::Toolkit to the WYSIWYG/TinyMCE, but even that work... the handling of whitespace is tricky.
Comment 50 David Cook 2025-06-10 00:33:45 UTC
(In reply to Lucas Gass (lukeg) from comment #39)
> I'm just curious why we scrub nothing in HTML customization, News, or Pages
> ( the other additional contents ) but need to scrub here?

That's a really good question, and the answer is that there are open reports for adding scrubbing to those areas. 

We want to scrub here now, so we're not adding fuel to the fire. We have an opportunity here to set things up securely from the start, which will mean fewer headaches for people going forward.

(In reply to Lucas Gass (lukeg) from comment #40)
> Maybe what we need is a profile for all additional contents that was based
> on things to deny, instead of things to allow?

Adding a profile for all additional contents isn't a bad idea. I think that most of them probably will share a similar profile. (I think where we'll struggle with existing additional contents is the use of "form" and existing inline Javascript.)

But see my above comment about why deny lists don't work as well as allow lists for security (and the reference to Wordpress since I think they're a good example of a system that offers content editing for users but also balances security against it).
Comment 51 David Cook 2025-06-10 00:35:40 UTC
So the WYSIWYG thing is an issue for sure. 

We might want to make a change to the HTML customizations feature (or rather DB schema) that allows us to specify that some can only be edited using the text editor and not the WYSIWYG I reckon...
Comment 52 Lisette Scheer 2025-06-11 16:30:01 UTC
(In reply to David Cook from comment #51)
> So the WYSIWYG thing is an issue for sure. 
> 
> We might want to make a change to the HTML customizations feature (or rather
> DB schema) that allows us to specify that some can only be edited using the
> text editor and not the WYSIWYG I reckon...

In my in progress work on css and js snippets I'm doing this to make them always load the text:

[% IF ( wysiwyg ) && category != 'js_snippets' && category != 'css_snippets' %]
    [% SET editmode = "wysiwyg" %]

I worry about being too restrictive when the library is already going to be vetting employees. 
Could we compromise by doing a more restrictive scrub unless you have specific permissions and not allow editing of ones that would be scrubbed if you didn't have the right permission?
Comment 53 David Cook 2025-06-12 07:09:36 UTC
(In reply to Lisette Scheer from comment #52)
> (In reply to David Cook from comment #51)
> > So the WYSIWYG thing is an issue for sure. 
> > 
> > We might want to make a change to the HTML customizations feature (or rather
> > DB schema) that allows us to specify that some can only be edited using the
> > text editor and not the WYSIWYG I reckon...
> 
> In my in progress work on css and js snippets I'm doing this to make them
> always load the text:
> 
> [% IF ( wysiwyg ) && category != 'js_snippets' && category != 'css_snippets'
> %]
>     [% SET editmode = "wysiwyg" %]

Hard-coding might work for a POC but I think we'll probably want to put that into the database.
 
> I worry about being too restrictive when the library is already going to be
> vetting employees. 

I don't think most libraries worldwide are vetting their employees very carefully or at all. 

> Could we compromise by doing a more restrictive scrub unless you have
> specific permissions and not allow editing of ones that would be scrubbed if
> you didn't have the right permission?

Honestly, I'd say both-and rather than either-or. The person editing the UI should have higher permissions than someone adding a news item. But it should still be scrubbed either way. Content management systems do this, and if we're going to offer content management, we need to do it too.

--

Does Bywater already have this feature locally? If so, then it should be easy to come up with a list of elements and attributes to allow list. If not, then I think we use the Wordpress list as a first pass. If there is concern that it won't be comprehensive enough, then we need to make a way to re-configure it on the backend.
Comment 54 David Cook 2025-06-12 07:12:53 UTC
(Apologies if it takes me some time to respond. My mail server is blocking Bugzilla so I'm not getting email notifications at this time.)
Comment 55 Lucas Gass (lukeg) 2025-07-03 17:49:51 UTC
Created attachment 183774 [details] [review]
Bug 39860: Add record_display srubber profile
Comment 56 Lucas Gass (lukeg) 2025-07-03 17:51:18 UTC
David,

I have written a more comprehensive allow list for 'record display'. It is based on the elements I find in the details and results pages. The idea is to be able to use the elements I find there to create similar custom displays.
Comment 57 David Cook 2025-07-04 01:41:20 UTC
(In reply to Lucas Gass (lukeg) from comment #56)
> David,
> 
> I have written a more comprehensive allow list for 'record display'. It is
> based on the elements I find in the details and results pages. The idea is
> to be able to use the elements I find there to create similar custom
> displays.

I've sent you some feedback on Mattermost.

Overall, I think it's looking OK although there are some rules missing for elements like "img" and the table related elements.

Are you sure that you can use regex for the key names in the rules? A glance at HTML::Scrubber source code suggests that won't work. 

I would suggest adding a unit test for the C4/Scrubber.pm to test your profile to make sure it's working the way you expect it to work. 

Overall, I think it's looking good though.