Bug 36244 - Template toolkit syntax not escaped in letter templates
Summary: Template toolkit syntax not escaped in letter templates
Status: Pushed to oldoldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P1 - high blocker (vote)
Assignee: Andreas Jonsson
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 36267
  Show dependency treegraph
 
Reported: 2024-03-06 13:09 UTC by Andreas Jonsson
Modified: 2024-04-03 14:00 UTC (History)
21 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.04,23.05.10,22.11.16,22.05.20


Attachments
Bug 36244: Unit test for tt syntax in parameters (2.53 KB, patch)
2024-03-07 09:33 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 36244: Do template toolkit processing first (2.25 KB, patch)
2024-03-07 09:33 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 36244: Prevent TT to evaluate data from DB (1.49 KB, patch)
2024-03-07 09:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36244: Unit test for tt syntax in parameters (2.60 KB, patch)
2024-03-07 09:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36244: Do template toolkit processing first (2.33 KB, patch)
2024-03-07 09:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36244: Unit test for tt syntax in parameters (2.70 KB, patch)
2024-03-07 10:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36244: Do template toolkit processing first (2.42 KB, patch)
2024-03-07 10:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36244: Unit test for tt syntax in parameters (2.71 KB, patch)
2024-03-07 16:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36244: Do template toolkit processing first (2.44 KB, patch)
2024-03-07 16:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36244: Add atomic update to check for affected notices (1.87 KB, patch)
2024-03-07 16:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36244: Unit test for tt syntax in parameters (2.71 KB, patch)
2024-03-15 15:53 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 36244: Do template toolkit processing first (2.44 KB, patch)
2024-03-15 15:53 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 36244: Add atomic update to check for affected notices (1.92 KB, patch)
2024-03-15 15:53 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 36244: [22.05] Unit test for tt syntax in parameters (2.71 KB, patch)
2024-03-19 08:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36244: [22.05] Do template toolkit processing first (2.17 KB, patch)
2024-03-19 08:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36244: [22.05] Add atomic update to check for affected notices (1.92 KB, patch)
2024-03-19 08:48 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Jonsson 2024-03-06 13:09:35 UTC
Template toolkit syntax is not escaped after <<parameter>> substitution in messages.

Typically, letter templates contains:

<<borrowers.firstname>> <<borrowers.surname>>

But this is a problem when we for instance want to send a letter to Mr Robert [% USE Categories %][% Categories.all().search_related("borrowers").delete_all() %].

Please, use count() instead of delete_all() to test.
Comment 1 Andreas Jonsson 2024-03-06 13:16:01 UTC
Test script for devbox:

use Koha::Patrons;

my $borrower = Koha::Patrons->find(1);

$borrower->surname('[% USE Categories %][% Categories.all().search_related("borrowers").count() %]')->store();

my $letter = C4::Letters::GetPreparedLetter (
        module => 'circulation',
        letter_code => 'PICKUP_RECALLED_ITEM',
        branchcode => 'CPL',
        lang => '',
        tables => { 'borrowers' => $borrower->borrowernumber, branches => 'CPL' },
        loops => {
        },
        substitute => {},
        repeat => { },
        message_transport_type => 'email',
    );


print $letter->{content}, "\n";
Comment 2 Jonathan Druart 2024-03-07 08:41:38 UTC
The delete_all is not a problem. We have the notice generation that is run inside a transaction (that is rollbacked), see bug 28739.

However we are leaking info, all the DB I guess... This is terrible.


$borrower->surname('[% USE Dumper( Indent=1, SortKeys=1 ) %][% USE Categories %][% Dumper.dump(Categories.all().search_related("borrowers").unblessed()) %]')->store();
Comment 3 Jonathan Druart 2024-03-07 08:42:17 UTC
There is NOTHING more important than fixing, testing and applying this everywhere.
Comment 4 Jonathan Druart 2024-03-07 08:57:18 UTC
Please all start by:
* turning off PatronSelfRegistration
* turn on AutoApprovePatronProfileSettings
* tell staff to be careful when approving OPAC members's self modifications
Comment 5 Jonathan Druart 2024-03-07 09:19:30 UTC
Comment when sending a list is not affected.
Comment 6 Andreas Jonsson 2024-03-07 09:33:01 UTC
Created attachment 162866 [details] [review]
Bug 36244: Unit test for tt syntax in parameters
Comment 7 Andreas Jonsson 2024-03-07 09:33:04 UTC
Created attachment 162867 [details] [review]
Bug 36244: Do template toolkit processing first

To avoid injection of template toolkit code
from database fields that are controlled by
untrusted sources.

Test plan:

* review subtest 'Template toolkit syntax in
  parameters' in t/db_dependent/Letters.t
* Run the unit test:
  prove t/db_dependent/Letters.t
Comment 8 Andreas Jonsson 2024-03-07 09:34:45 UTC
A simple mitigation for the worst consequences of this is to perform the template toolkit processing before the parameter substitution.

But there is still a problem that we are relying on regexp-soup style parsing to perform the parameter substitutions.  Later passes will substitute parameters that are inserted by earlier passes.  This will make it possible to leak confidential information.

Maybe it is possible to use mustache to substitute the parameters. (http://mustache.github.io/)

For now I'm going to suggest doing the template toolkit substitution first and open a separate issue to fix the regexp-soup.
Comment 9 Jonathan Druart 2024-03-07 09:36:39 UTC
Created attachment 162868 [details] [review]
Bug 36244: Prevent TT to evaluate data from DB

Reversing the order could be a nice fix here (Thanks Martin for the
idea).
Comment 10 Jonathan Druart 2024-03-07 09:37:15 UTC
Comment on attachment 162868 [details] [review]
Bug 36244: Prevent TT to evaluate data from DB

Heh :)
Comment 11 Jonathan Druart 2024-03-07 09:44:35 UTC
Created attachment 162871 [details] [review]
Bug 36244: Unit test for tt syntax in parameters

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Jonathan Druart 2024-03-07 09:44:39 UTC
Created attachment 162872 [details] [review]
Bug 36244: Do template toolkit processing first

To avoid injection of template toolkit code
from database fields that are controlled by
untrusted sources.

Test plan:

* review subtest 'Template toolkit syntax in
  parameters' in t/db_dependent/Letters.t
* Run the unit test:
  prove t/db_dependent/Letters.t

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Jonathan Druart 2024-03-07 09:45:19 UTC
Keeping NSO status, we need more testers. Possible edge cases with this approach?
Comment 14 Andreas Jonsson 2024-03-07 09:54:13 UTC
I would be surprised if there aren't any libraries that is doing silly things such as:

[% IF <<borrowers.categorycode>> == 'SPECIAL' %]

But we cannot reasonably do anything about that.
Comment 15 Jonathan Druart 2024-03-07 09:57:33 UTC
(In reply to Andreas Jonsson from comment #8)
> For now I'm going to suggest doing the template toolkit substitution first
> and open a separate issue to fix the regexp-soup.

We should remove the historical <<>> syntax.
Comment 16 Marcel de Rooy 2024-03-07 10:01:31 UTC
Created attachment 162876 [details] [review]
Bug 36244: Unit test for tt syntax in parameters

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2024-03-07 10:01:34 UTC
Created attachment 162877 [details] [review]
Bug 36244: Do template toolkit processing first

To avoid injection of template toolkit code
from database fields that are controlled by
untrusted sources.

Test plan:

* review subtest 'Template toolkit syntax in
  parameters' in t/db_dependent/Letters.t
* Run the unit test:
  prove t/db_dependent/Letters.t

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Martin Renvoize 2024-03-07 10:02:06 UTC
This is the solution I was thinking about, swapping the parsing order as you have.

I think it's the quickest route to goal, but as you highlight, I'm confident it'll break some peoples setups as I'm sure I've seen <<>> inside [% %] blocks in a few places.. people will do interesting things whenever it's possible.  We'll just need to advertise the breaking change at release time.

As for replacing with mustache, I'm not keen on that at all.. introducing yet another template syntax.. What we should really be focusing on and should have achieved years ago frankly is removing the old <<>> syntax entirely and leaning on just TT as our chosen templating engine.. There's a lot of work still to go there given our UI still basically encourages the use of the old syntax instead of us ever haveing followed through with improved notice builders that actually advocate the use of proper TT.
Comment 19 Martin Renvoize 2024-03-07 10:03:03 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to Andreas Jonsson from comment #8)
> > For now I'm going to suggest doing the template toolkit substitution first
> > and open a separate issue to fix the regexp-soup.
> 
> We should remove the historical <<>> syntax.

I've been advocating for this for years
Comment 20 Marcel de Rooy 2024-03-07 10:03:42 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to Andreas Jonsson from comment #8)
> > For now I'm going to suggest doing the template toolkit substitution first
> > and open a separate issue to fix the regexp-soup.
> 
> We should remove the historical <<>> syntax.

To get there, we might now forbid using both syntaxes in the same letter.
Comment 21 Andreas Jonsson 2024-03-07 11:47:19 UTC
(In reply to Martin Renvoize from comment #18)
> 
> As for replacing with mustache, I'm not keen on that at all.. introducing
> yet another template syntax.. What we should really be focusing on and
> should have achieved years ago frankly is removing the old <<>> syntax
> entirely and leaning on just TT as our chosen templating engine.. There's a
> lot of work still to go there given our UI still basically encourages the
> use of the old syntax instead of us ever haveing followed through with
> improved notice builders that actually advocate the use of proper TT.

Having two different templating engines is bad. But it is also not great that editing message texts is such a powerful mechanism. The privilege 'edit_notices' must be considered essentially as powerful as 'plugins/manage' or 'superlibrarian'.

In theory it might be possible to sandbox template toolkit.  (For instance, the template plugins should not return dbic-objects.)  But I would have preferred having a simpler template engine for message texts.
Comment 22 Kyle M Hall 2024-03-07 16:24:08 UTC
Created attachment 162908 [details] [review]
Bug 36244: Unit test for tt syntax in parameters

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2024-03-07 16:24:16 UTC
Created attachment 162909 [details] [review]
Bug 36244: Do template toolkit processing first

To avoid injection of template toolkit code
from database fields that are controlled by
untrusted sources.

Test plan:

* review subtest 'Template toolkit syntax in
  parameters' in t/db_dependent/Letters.t
* Run the unit test:
  prove t/db_dependent/Letters.t

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Kyle M Hall 2024-03-07 16:24:19 UTC
Created attachment 162910 [details] [review]
Bug 36244: Add atomic update to check for affected notices

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 David Cook 2024-03-07 22:28:49 UTC
(In reply to Andreas Jonsson from comment #21)
> In theory it might be possible to sandbox template toolkit.  (For instance,
> the template plugins should not return dbic-objects.)  But I would have
> preferred having a simpler template engine for message texts.

You don't need a different template engine. Template::Toolkit can be simple. We just use it in a problematic way.

Firstly, we have it insecurely configured with EVAL_PERL and ABSOLUTE. I've complained about those for years, and I disable them locally. 

Secondly, as you note, we pass it DBIC objects. We could easily just pass it hashes instead and not have the security problem. That said, we'd lose convenience in other ways. It's a fine balance for sure.
Comment 26 Martin Renvoize 2024-03-08 08:21:51 UTC
Being able to navigate relationships and having access to our includes and local tt filters are both very important for our end users.. I'd be very much against losing that functionality.

Also, given the multi-year challenge to try and get us from <<>> to TT and the failure we've had there.. introducing yet another migration path before the first has even been completed is the road to madness in my opinion.
Comment 27 David Cook 2024-03-10 23:37:58 UTC
(In reply to Martin Renvoize from comment #26)
> Being able to navigate relationships and having access to our includes and
> local tt filters are both very important for our end users.. I'd be very
> much against losing that functionality.

You should be able to do those without EVAL_PERL and ABSOLUTE. 

> Also, given the multi-year challenge to try and get us from <<>> to TT and
> the failure we've had there.. introducing yet another migration path before
> the first has even been completed is the road to madness in my opinion.

Do you mean Mustache or something else?
Comment 28 Martin Renvoize 2024-03-11 03:47:27 UTC
Yes, I mean Mustache for that comment.  Passing just hashes, for me takes us backwards from the direction we've been moving. Giving end users less ability to get the data they want in notices and adding maintenance burden to developers as any and all additional data a librarian might want would need specific development to add to the hashes per notice.

Our editor is terrible and doesn't expose at all what's actually available per notice, it's misleading at best right now.
Comment 29 David Cook 2024-03-11 04:22:05 UTC
(In reply to Martin Renvoize from comment #28)
> Yes, I mean Mustache for that comment.  Passing just hashes, for me takes us
> backwards from the direction we've been moving. Giving end users less
> ability to get the data they want in notices and adding maintenance burden
> to developers as any and all additional data a librarian might want would
> need specific development to add to the hashes per notice.

It is a tough one. I've certainly enjoyed being able to work with objects in the notices. But it makes me think about the security vs convenience balance. I could be wrong, but it seems like it could be used to circumvent a lot of our security at the moment (although the attackers would be staff rather than patrons at least).

> Our editor is terrible and doesn't expose at all what's actually available
> per notice, it's misleading at best right now.

Yeah, I figure we get rid of <<>> and then we continue working on improving things with the Template::Toolkit. 

Anyway, seems like this particular issue raised by OP is taken care of at least, so that's the main thing.
Comment 30 Andreas Jonsson 2024-03-11 07:08:38 UTC
One concern about this fix is the fact that a Library who already have replaced all <<parameters>> with template toolkit syntax would not have been vulnerable to this issue in the first place.  But by changing evaluation order we open up the possiblility to inject <<parameters>> and thus making them vulnerable to bug 36267.
Comment 31 wainuiwitikapark 2024-03-14 03:05:48 UTC
Please let me know if I need to backport this as it is to 22.05.x-security
Comment 32 Katrin Fischer 2024-03-15 15:32:42 UTC
(In reply to wainuiwitikapark from comment #31)
> Please let me know if I need to backport this as it is to 22.05.x-security

Yes, this is for all stable releases.
Comment 33 Katrin Fischer 2024-03-15 15:53:50 UTC
Created attachment 163255 [details] [review]
Bug 36244: Unit test for tt syntax in parameters

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Katrin Fischer 2024-03-15 15:53:54 UTC
Created attachment 163256 [details] [review]
Bug 36244: Do template toolkit processing first

To avoid injection of template toolkit code
from database fields that are controlled by
untrusted sources.

Test plan:

* review subtest 'Template toolkit syntax in
  parameters' in t/db_dependent/Letters.t
* Run the unit test:
  prove t/db_dependent/Letters.t

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 35 Katrin Fischer 2024-03-15 15:53:57 UTC
Created attachment 163257 [details] [review]
Bug 36244: Add atomic update to check for affected notices

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Fixed some typos in bug numbers and text.
Comment 36 Katrin Fischer 2024-03-15 15:54:30 UTC
Fixed some typos in the database update (bug number especially), so re-attaching just in case.
Comment 37 Katrin Fischer 2024-03-15 16:24:55 UTC
Pushed to security repository.
Comment 38 Katrin Fischer 2024-03-18 15:56:04 UTC
Actually ... could we make the database update even smarter? It doesn't catch a quite common case in our databases that I know was also on the mailing list in the past:

[% <<aqorders.listprice>> | $Price %]

It was the only way to format prices int he ACQORDER email previous to bug 31858.
Comment 39 Katrin Fischer 2024-03-18 16:02:12 UTC
Maybe check for this additionally?

SELECT * FROM letter WHERE content LIKE "[%%% <<%";
Comment 40 wainuiwitikapark 2024-03-18 23:42:41 UTC
This doesn't apply 100% cleanly to 22.05.x-security and I am getting the following:

kohadev-koha@kohadevbox:koha((1ee9fe6a1f0...))$ prove t/db_dependent/Letters.t
t/db_dependent/Letters.t .. Global symbol "$objects" requires explicit package name (did you forget to declare "my $objects"?) at /kohadevbox/koha/C4/Letters.pm line 605.
Global symbol "$objects" requires explicit package name (did you forget to declare "my $objects"?) at /kohadevbox/koha/C4/Letters.pm line 616.
Type of arg 1 to Try::Tiny::catch must be block or sub {} (not reference constructor) at /kohadevbox/koha/C4/Letters.pm line 1410, near "};"
Type of arg 1 to Try::Tiny::try must be block or sub {} (not reference constructor) at /kohadevbox/koha/C4/Letters.pm line 1410, near "};"
Type of arg 1 to Try::Tiny::catch must be block or sub {} (not reference constructor) at /kohadevbox/koha/C4/Letters.pm line 1459, near "};"
Type of arg 1 to Try::Tiny::try must be block or sub {} (not reference constructor) at /kohadevbox/koha/C4/Letters.pm line 1459, near "};"
Compilation failed in require at /kohadevbox/koha/C4/Reserves.pm line 31.
BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Reserves.pm line 31.
Compilation failed in require at /kohadevbox/koha/C4/Circulation.pm line 30.
BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Circulation.pm line 30.
Compilation failed in require at /kohadevbox/koha/Koha/Items.pm line 27.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Items.pm line 27.
Compilation failed in require at /kohadevbox/koha/Koha/Statistic.pm line 22.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Statistic.pm line 22.
Compilation failed in require at /kohadevbox/koha/Koha/Statistics.pm line 23.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Statistics.pm line 23.
Compilation failed in require at /kohadevbox/koha/C4/Stats.pm line 26.
BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Stats.pm line 26.
Compilation failed in require at /kohadevbox/koha/C4/Accounts.pm line 23.
BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Accounts.pm line 23.
Compilation failed in require at /kohadevbox/koha/C4/Overdues.pm line 31.
BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Overdues.pm line 31.
Compilation failed in require at /kohadevbox/koha/Koha/Account/Line.pm line 23.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Account/Line.pm line 23.
Compilation failed in require at /kohadevbox/koha/Koha/Account/Lines.pm line 22.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Account/Lines.pm line 22.
Compilation failed in require at /kohadevbox/koha/Koha/ArticleRequest.pm line 22.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/ArticleRequest.pm line 22.
Compilation failed in require at /kohadevbox/koha/Koha/ArticleRequests.pm line 26.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/ArticleRequests.pm line 26.
Compilation failed in require at /kohadevbox/koha/Koha/Patrons.pm line 27.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Patrons.pm line 27.
Compilation failed in require at /kohadevbox/koha/Koha/Acquisition/Basket.pm line 27.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Acquisition/Basket.pm line 27.
Compilation failed in require at /kohadevbox/koha/Koha/Acquisition/Baskets.pm line 23.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Acquisition/Baskets.pm line 23.
Compilation failed in require at /kohadevbox/koha/Koha/Acquisition/Order.pm line 24.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Acquisition/Order.pm line 24.
Compilation failed in require at /kohadevbox/koha/Koha/Acquisition/Orders.pm line 24.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Acquisition/Orders.pm line 24.
Compilation failed in require at /kohadevbox/koha/Koha/Biblio.pm line 33.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Biblio.pm line 33.
Compilation failed in require at /kohadevbox/koha/Koha/Biblios.pm line 25.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Biblios.pm line 25.
Compilation failed in require at /kohadevbox/koha/Koha/Libraries.pm line 25.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Libraries.pm line 25.
Compilation failed in require at /kohadevbox/koha/Koha/Object/Limit/Library.pm line 22.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Object/Limit/Library.pm line 22.
Compilation failed in require at /usr/lib/x86_64-linux-gnu/perl-base/base.pm line 135.
	...propagated at /usr/lib/x86_64-linux-gnu/perl-base/base.pm line 157.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/AuthorisedValue.pm line 25.
Compilation failed in require at /kohadevbox/koha/Koha/AuthorisedValues.pm line 25.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/AuthorisedValues.pm line 25.
Compilation failed in require at /kohadevbox/koha/C4/Koha.pm line 27.
BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Koha.pm line 27.
Compilation failed in require at /kohadevbox/koha/C4/Biblio.pm line 91.
BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Biblio.pm line 91.
Compilation failed in require at /kohadevbox/koha/t/lib/TestBuilder.pm line 6.
BEGIN failed--compilation aborted at /kohadevbox/koha/t/lib/TestBuilder.pm line 6.
Compilation failed in require at t/db_dependent/Letters.t line 51.
BEGIN failed--compilation aborted at t/db_dependent/Letters.t line 51.
# Looks like your test exited with 255 before it could output anything.
t/db_dependent/Letters.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 86/86 subtests 

Test Summary Report
-------------------
t/db_dependent/Letters.t (Wstat: 65280 Tests: 0 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 86 tests but ran 0.
Files=1, Tests=0,  1 wallclock secs ( 0.01 usr  0.00 sys +  0.48 cusr  0.05 csys =  0.54 CPU)
Result: FAIL
Comment 41 Jonathan Druart 2024-03-19 08:48:49 UTC
Created attachment 163383 [details] [review]
Bug 36244: [22.05] Unit test for tt syntax in parameters

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 42 Jonathan Druart 2024-03-19 08:48:54 UTC
Created attachment 163384 [details] [review]
Bug 36244: [22.05] Do template toolkit processing first

To avoid injection of template toolkit code
from database fields that are controlled by
untrusted sources.

Test plan:

* review subtest 'Template toolkit syntax in
  parameters' in t/db_dependent/Letters.t
* Run the unit test:
  prove t/db_dependent/Letters.t

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 43 Jonathan Druart 2024-03-19 08:48:58 UTC
Created attachment 163385 [details] [review]
Bug 36244: [22.05] Add atomic update to check for affected notices

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Fixed some typos in bug numbers and text.
Comment 44 Jonathan Druart 2024-03-19 08:50:28 UTC
(In reply to wainuiwitikapark from comment #40)
> This doesn't apply 100% cleanly to 22.05.x-security

Hello Wainui, I have attached a version for 22.05.
Comment 45 wainuiwitikapark 2024-03-19 22:22:00 UTC
(In reply to Jonathan Druart from comment #44)
> (In reply to wainuiwitikapark from comment #40)
> > This doesn't apply 100% cleanly to 22.05.x-security
> 
> Hello Wainui, I have attached a version for 22.05.

Thanks so much!

I have now backported the 22.05 patches to 22.05.x-security for 22.05.20
Comment 46 Fridolin Somers 2024-03-25 09:45:10 UTC
RMaints :
Don't forget to create a DBRev by moving the atomic update :
installer/data/mysql/atomicupdate/bug_36244.pl
Comment 47 wainuiwitikapark 2024-03-25 23:44:31 UTC
(In reply to Fridolin Somers from comment #46)
> RMaints :
> Don't forget to create a DBRev by moving the atomic update :
> installer/data/mysql/atomicupdate/bug_36244.pl

oops I just set up my release but I didn't do this
Comment 48 wainuiwitikapark 2024-03-26 01:32:28 UTC
(In reply to wainuiwitikapark from comment #47)
> (In reply to Fridolin Somers from comment #46)
> > RMaints :
> > Don't forget to create a DBRev by moving the atomic update :
> > installer/data/mysql/atomicupdate/bug_36244.pl
> 
> oops I just set up my release but I didn't do this

Added the db rev commit to 22.05.x-security branch https://git.koha-community.org/Koha-community/security/commit/975b3886de245ed4444138842683a8af4dbdde5b
Comment 49 Blou 2024-04-03 13:16:28 UTC
Am I right to understand this has been backported but is not yet "Pushed to master" ?

First Bug I see handled in that order, so I just want to validate this is eventually making it to master.

Thanks
Comment 50 Jonathan Druart 2024-04-03 13:23:28 UTC
(In reply to Blou from comment #49)
> Am I right to understand this has been backported but is not yet "Pushed to
> master" ?
> 
> First Bug I see handled in that order, so I just want to validate this is
> eventually making it to master.
> 
> Thanks

It's the usual workflow for security bugs. It will be pushed to master very soon.
Comment 51 Blou 2024-04-03 13:46:29 UTC
AH!  Big thanks for the clarification.  I usually am not CC'd on those.
Comment 52 Jonathan Druart 2024-04-03 14:00:32 UTC
(In reply to Blou from comment #51)
> AH!  Big thanks for the clarification.  I usually am not CC'd on those.

I've just added you to the security group and default CC for security bugs.