Bug 40818

Summary: marc_lib is mostly used raw in templates
Product: Koha Reporter: Fridolin Somers <fridolin.somers>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
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, wainuiwitikapark
Version: unspecified   
Hardware: All   
OS: All   
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 40818: Remove HTML from marc_lib
Bug 40818: Replace raw with html filter for marc_lib
Bug 40818: Remove HTML from marc_lib
Bug 40818: Replace raw with html filter for marc_lib
Bug 40818: Add missing html filter on html_helpers.inc
Bug 40818: Remove HTML from marc_lib
Bug 40818: Replace raw with html filter for marc_lib
Bug 40818: Add missing html filter on html_helpers.inc
Bug 40818: [22.11] Remove HTML from marc_lib
Bug 40818: [22.11] Replace raw with html filter for marc_lib
Bug 40818: [24.05.x] Remove HTML from marc_lib
Bug 40818: [24.05.x] Replace raw with html filter for marc_lib

Description Fridolin Somers 2025-09-16 15:09:00 UTC
Field or subfield label "marc_lib" is mostly used raw in templates.

In OPAC templates :
 > git grep marc_lib -- koha-tmpl/opac-tmpl/
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt:                                                <td>[% subfiel.marc_lib | $raw %]</td>
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt:                                                <td>[% subfiel.marc_lib | $raw %]</td>
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt:                                                <td>[% subfiel.marc_lib | $raw %]</td>
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt:                                                <td>[% subfiel.marc_lib | $raw %]</td>
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt:                                                <td>[% subfiel.marc_lib | $raw %]</td>
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt:                                                <td>[% subfiel.marc_lib | $raw %]</td>
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt:                                                <td>[% subfiel.marc_lib | $raw %]</td>
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt:                                                <td>[% subfiel.marc_lib | $raw %]</td>
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt:                                                <td>[% subfiel.marc_lib | $raw %]</td>
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt:                                                <td>[% subfiel.marc_lib | $raw %]</td>
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth-MARCdetail.tt:                                    ><strong>[% subfiel.marc_lib | $raw %]:</strong> [% IF subfiel.isurl %]

In staff interface :
 > git grep marc_lib -- koha-tmpl/intranet-tmpl/
koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc:                        <label class="required">[% subfield.subfield | html %] - [% subfield.marc_lib | $raw %]</label>
koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc:                        <label>[% subfield.subfield | html %] - [% subfield.marc_lib | $raw %]</label>
koha-tmpl/intranet-tmpl/prog/en/modules/acqui/addorderiso2709.tt:                                                                            <label class="required">[% iteminformatio.subfield | html %] - [% iteminformatio.marc_lib | $raw %]</label>
koha-tmpl/intranet-tmpl/prog/en/modules/acqui/addorderiso2709.tt:                                                                            <label>[% iteminformatio.subfield | html %] - [% iteminformatio.marc_lib | $raw %]</label>
koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt:                                <label>[% field.marc_lib | html %] ([% field.tag | html %][% field.subfield | html %])</label>
koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt:    <label>[% field.marc_lib | html %] ([% field.tag | html %][% field.subfield | html %])</label>
koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tt:                        label.push("[% subfield_loo.marc_lib | $raw %]");
koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tt:                                                                        title="[% subfield_loo.marc_lib | $raw %]"
koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tt:                                                                        [% subfield_loo.marc_lib | $raw %]
koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt:                                label.push("[% subfield_loo.marc_lib | $raw %]");
koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt:                                label.push("[% subfield_loo.marc_lib | $raw %]");
koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt:                                                                title="[% subfield_loo.marc_lib | $raw %]"
koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt:                                                            [% subfield_loo.marc_lib | $raw %]
koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt:                                                                                    <label class="required">[% iteminformatio.subfield | html %] - [% iteminformatio.marc_lib | $raw %]</label>
koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt:                                                                                    <label>[% iteminformatio.subfield | html %] - [% iteminformatio.marc_lib | $raw %]</label>
koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt:                                                                                <label class="required">[% iteminformatio.subfield | html %] - [% iteminformatio.marc_lib | $raw %]</label>
koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt:                                                                                <label>[% iteminformatio.subfield | html %] - [% iteminformatio.marc_lib | $raw %]</label>
koha-tmpl/intranet-tmpl/prog/en/modules/services/itemrecorddisplay.tt:                    <label class="required">[% iteminfo.subfield | html %] - [% iteminfo.marc_lib | $raw %]</label>
koha-tmpl/intranet-tmpl/prog/en/modules/services/itemrecorddisplay.tt:                    <label>[% iteminfo.subfield | html %] - [% iteminfo.marc_lib | $raw %]</label>

Isn't that a security issue ?

And for example, adding a double quote in a mandatory subfield breaks the record edition page.
Comment 1 David Cook 2025-09-17 00:42:15 UTC
(In reply to Fridolin Somers from comment #0)
> Field or subfield label "marc_lib" is mostly used raw in templates.
 
> Isn't that a security issue ?

It certainly sounds like it based on what you've provided here.

It took a bit of time to work out where "lib" comes from in the following line:

marc_lib      => $tagslib->{$tag}->{$subfield}->{lib},

But it's from C4::Biblio::GetMarcStructure. We'd need to sanitize values there I think, and double-check other places...

> And for example, adding a double quote in a mandatory subfield breaks the
> record edition page.

Do you mean a double quote in the liblibrarian/libopac db fields?
Comment 2 David Cook 2025-09-17 00:51:59 UTC
Because of the way Koha/UI/Form/Builder/* modules work, we probably can't use a filter like "html", but we could use a scrubber filter.

That would work for the HTML, but it does look like there's Javascript in there as well like koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt, which is more painful.

I'm struggling to come up with the solution here. (One reason why I want us to have Secure Coding Guidelines so we don't have to make it up from scratch every time.)

Because I saw in Koha/UI/Form/Builder/* there were times where marc_lib contained HTML which is maybe why they have raw filters here. I don't know. Lots of checking needs to be done there.
Comment 3 Jonathan Druart 2025-10-03 10:36:06 UTC
(In reply to David Cook from comment #2)
> Because of the way Koha/UI/Form/Builder/* modules work, we probably can't
> use a filter like "html", but we could use a scrubber filter.

I am not sure I understand what you mean.

It's using raw since bug 13618 comment 85. I don't really see a good reason to not html escape marc_lib everywhere actually.
Comment 4 David Cook 2025-10-07 03:29:22 UTC
(In reply to Jonathan Druart from comment #3)
> (In reply to David Cook from comment #2)
> > Because of the way Koha/UI/Form/Builder/* modules work, we probably can't
> > use a filter like "html", but we could use a scrubber filter.
> 
> I am not sure I understand what you mean.
> 
> It's using raw since bug 13618 comment 85. I don't really see a good reason
> to not html escape marc_lib everywhere actually.

Basically marc_lib isn't "lib" or "lib_opac" straight from the DB in all cases. It's composed of generated HTML.

Koha/UI/Form/Builder/Biblio.pm:        marc_lib      => $tagslib->{$tag}->{$subfield}->{lib},

Koha/UI/Form/Builder/Item.pm:    $subfield_data{marc_lib}      = "<span title=\"" . $subfield->{lib} . "\">" . $subfield->{lib} . "</span>";

For Biblio.pm I think it's taken straight from the authorized values, but as you can see for Item.pm, the marc_lib is actually a span wrapping that data, so you can't use the "html" filter in the template toolkit without breaking the display output of Koha/UI/Form/Builder/Item.pm
Comment 5 David Cook 2025-10-07 03:29:54 UTC
(In reply to David Cook from comment #4)
> Koha/UI/Form/Builder/Item.pm:    $subfield_data{marc_lib}      = "<span
> title=\"" . $subfield->{lib} . "\">" . $subfield->{lib} . "</span>";
> 
> For Biblio.pm I think it's taken straight from the authorized values, but as
> you can see for Item.pm, the marc_lib is actually a span wrapping that data,
> so you can't use the "html" filter in the template toolkit without breaking
> the display output of Koha/UI/Form/Builder/Item.pm

I'm guessing there's a reason we have the span here instead of somewhere else, but you might have greater insight into this one.
Comment 6 Jonathan Druart 2025-10-07 07:18:20 UTC
(In reply to David Cook from comment #5)
> (In reply to David Cook from comment #4)
> > Koha/UI/Form/Builder/Item.pm:    $subfield_data{marc_lib}      = "<span
> > title=\"" . $subfield->{lib} . "\">" . $subfield->{lib} . "</span>";
> > 
> > For Biblio.pm I think it's taken straight from the authorized values, but as
> > you can see for Item.pm, the marc_lib is actually a span wrapping that data,
> > so you can't use the "html" filter in the template toolkit without breaking
> > the display output of Koha/UI/Form/Builder/Item.pm
> 
> I'm guessing there's a reason we have the span here instead of somewhere
> else, but you might have greater insight into this one.

Hard to tell really...

commit 3b6f7a6d06e71662f4d5736383cbf584230694e7
Date:   Tue Jan 14 16:45:33 2003 +0000

    little improvments (mandatory subfields in item appear red if not filled)

-       $subfield_data{marc_lib}=$tagslib->{$tag}->{$subfield}->{lib};
-       $subfield_data{marc_mandatory}=$tagslib->{$tag}->{$subfield}->{mandatory};
-       $subfield_data{marc_repeatable}=$tagslib->{$tag}->{$subfield}->{repeatable};
+#      $subfield_data{marc_lib}=$tagslib->{$tag}->{$subfield}->{lib};
+       $subfield_data{marc_lib}="<DIV id=\"error$i\">".$tagslib->{$tag}->{$subfield}->{lib}."</div>";
+       $subfield_data{mandatory}=$tagslib->{$tag}->{$subfield}->{mandatory};
+       $subfield_data{repeatable}=$tagslib->{$tag}->{$subfield}->{repeatable};

There is never a good reason to generate html from the controller anyway.
Comment 7 Jonathan Druart 2025-10-07 07:33:26 UTC
Created attachment 187503 [details] [review]
Bug 40818: Remove HTML from marc_lib
Comment 8 Jonathan Druart 2025-10-07 07:33:29 UTC
Created attachment 187504 [details] [review]
Bug 40818: Replace raw with html filter for marc_lib
Comment 9 Jonathan Druart 2025-10-07 07:34:30 UTC
The first patch is moving the span to the template. The second one is using the html filter for marc_lib. I think that's all what is needed here.
Comment 10 Fridolin Somers 2025-10-10 14:58:15 UTC
Maybe this is not a security level issue then ?
Comment 11 David Cook 2025-10-12 22:43:52 UTC
(In reply to Fridolin Somers from comment #10)
> Maybe this is not a security level issue then ?

I would say it definitely is a security issue. I've been away/busy a bit lately, but I'll try to put together a POC exploit for this to show the problem and prove the solution.
Comment 14 David Cook 2025-10-13 02:42:32 UTC
It looks like a filter was missed on that first patch
Comment 15 David Cook 2025-10-13 02:43:55 UTC
Created attachment 187782 [details] [review]
Bug 40818: Remove HTML from marc_lib

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 16 David Cook 2025-10-13 02:43:59 UTC
Created attachment 187783 [details] [review]
Bug 40818: Replace raw with html filter for marc_lib

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 17 David Cook 2025-10-13 02:44:02 UTC
Created attachment 187784 [details] [review]
Bug 40818: Add missing html filter on html_helpers.inc

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 18 David Cook 2025-10-13 02:45:14 UTC
(In reply to Jonathan Druart from comment #9)
> The first patch is moving the span to the template. The second one is using
> the html filter for marc_lib. I think that's all what is needed here.

Thanks for that, Jonathan. I've added a patch as a html filter was needed for html_helpers.inc, but with those 3 I think it all looks good now.
Comment 19 Marcel de Rooy 2025-10-17 09:34:45 UTC
+ <label [% IF subfield.mandatory %]class="required"[% END %]>

Wasnt that a pattern that we forbid? Using an IF within the tag. Does it interfere with translation?
Comment 20 Marcel de Rooy 2025-10-17 09:41:35 UTC
Created attachment 188053 [details] [review]
Bug 40818: Remove HTML from marc_lib

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2025-10-17 09:41:37 UTC
Created attachment 188054 [details] [review]
Bug 40818: Replace raw with html filter for marc_lib

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2025-10-17 09:41:40 UTC
Created attachment 188055 [details] [review]
Bug 40818: Add missing html filter on html_helpers.inc

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Jonathan Druart 2025-10-17 10:12:57 UTC
(In reply to Marcel de Rooy from comment #19)
> + <label [% IF subfield.mandatory %]class="required"[% END %]>
> 
> Wasnt that a pattern that we forbid? Using an IF within the tag. Does it
> interfere with translation?

It was, yes. No longer valid, see bug 38720 and bug 38714.
Comment 24 Wainui Witika-Park 2025-10-20 04:11:53 UTC
Can the patches please be rebased for 22.11
Comment 25 Jonathan Druart 2025-10-20 07:49:15 UTC
Created attachment 188140 [details] [review]
Bug 40818: [22.11] Remove HTML from marc_lib

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Jonathan Druart 2025-10-20 07:49:18 UTC
Created attachment 188141 [details] [review]
Bug 40818: [22.11] Replace raw with html filter for marc_lib
Comment 27 Jonathan Druart 2025-10-20 07:50:21 UTC
For the other branches:
Apply "[22.11] Remove HTML from marc_lib". If it does not apply then fix the conflict, it's a one line change.
Then run `perl -p -i -e "s/marc_lib \| .raw /marc_lib \| html /g" **/*.tt **/*.inc` to generate the second patch.
Comment 28 Wainui Witika-Park 2025-10-21 01:31:54 UTC
Thanks so much Jonathan!
Comment 29 Wainui Witika-Park 2025-10-21 01:32:14 UTC
Pushed to 22.11.x-security for 22.11.32
Comment 30 Jesse Maseto 2025-10-28 08:05:09 UTC
This has merge conflicts with 24.05.x

Please resolve.
Comment 31 Lucas Gass (lukeg) 2025-10-28 21:13:32 UTC
Created attachment 188531 [details] [review]
Bug 40818: [24.05.x] Remove HTML from marc_lib

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Lucas Gass (lukeg) 2025-10-28 21:13:35 UTC
Created attachment 188532 [details] [review]
Bug 40818: [24.05.x] Replace raw with html filter for marc_lib
Comment 33 Lucas Gass (lukeg) 2025-11-03 16:47:46 UTC
Nice work all, pushed to main for 25.11.00