Bug 42746: (QA follow-up) Sanitize agefield and substitution field at the sink too

The original fix only whitelisted agefield, condition_field, and
substitution_field at the point they're saved in
tools/automatic_item_modification_by_age.pl, leaving the actual
consumer of the stored syspref, C4::Items::ToggleNewStatus, trusting
agefield and substitution field unchecked. agefield is spliced
directly into raw SQL (TO_DAYS($agefield)) and so can't be
parameterized like an ordinary value; substitution field reaches a
dynamic method call ($item->$field($value)). Any future or
alternate writer of the automatic_item_modification_by_age_configuration
syspref would have reopened both holes without touching the form at
all.

This adds the same allow-list validation directly in ToggleNewStatus
(falling back to the safe default agefield, or skipping the
substitution/condition, exactly as before for missing values), shares
the age-field allow-list between the two files via a new
GetAutomaticItemModificationAgeFields() in C4::Items so it can't drift
between them, and logs rejected values via Koha::Logger instead of
silently discarding them so tampering leaves an audit trail.

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>

C4/Items.pm

@@ -38,6 +38,7 @@ BEGIN {
         SearchItems
         PrepareItemrecordDisplay
         ToggleNewStatus
+        GetAutomaticItemModificationAgeFields
     );
 }
 
@@ -58,6 +59,7 @@ use Try::Tiny qw( catch try );
 use Koha::AuthorisedValues;
 use Koha::DateUtils qw( dt_from_string );
 use Koha::Database;
+use Koha::Logger;
 
 use Koha::Biblios;
 use Koha::Biblioitems;
@@ -1655,6 +1657,25 @@ sub PrepareItemrecordDisplay {
     };
 }
 
+=head2 GetAutomaticItemModificationAgeFields
+
+    my @fields = GetAutomaticItemModificationAgeFields();
+
+Returns the fixed, C<items.>-prefixed list of date columns that are valid
+"age field" values for automatic item modification rules (see
+ToggleNewStatus below). This is the single source of truth for that
+allow-list; it is shared with F<tools/automatic_item_modification_by_age.pl>
+so the two cannot drift apart.
+
+=cut
+
+sub GetAutomaticItemModificationAgeFields {
+    return (
+        'items.dateaccessioned', 'items.replacementpricedate', 'items.datelastborrowed', 'items.datelastseen',
+        'items.damaged_on',      'items.itemlost_on',          'items.withdrawn_on'
+    );
+}
+
 =head2 ToggleNewStatus
 
 Missing POD for ToggleNewStatus.
@@ -1671,17 +1692,32 @@ sub ToggleNewStatus {
     my @item_columns       = map { "items.$_" } Koha::Items->columns;
     my @biblioitem_columns = map { "biblioitems.$_" } Koha::Biblioitems->columns;
     my @biblio_columns     = map { "biblio.$_" } Koha::Biblios->columns;
+    my @age_fields         = GetAutomaticItemModificationAgeFields();
     my $report;
     for my $rule (@rules) {
         my $age = $rule->{age};
 
         # Default to using items.dateaccessioned if there's an old item modification rule
-        # missing an agefield value
+        # missing an agefield value, or if the stored value is not (or is no longer) a
-        my $agefield      = $rule->{agefield} ? $rule->{agefield} : 'items.dateaccessioned';
+        # recognised age field - values are only trusted from a fixed allow-list since
+        # this is spliced directly into the SQL below and cannot be bound as a placeholder.
+        my $agefield = $rule->{agefield} // '';
+        unless ( grep { $_ eq $agefield } @age_fields ) {
+            Koha::Logger->get->warn(qq{Rejecting invalid agefield "$agefield" in automatic item modification rule})
+                if length $agefield;
+            $agefield = 'items.dateaccessioned';
+        }
         my $conditions    = $rule->{conditions};
         my $substitutions = $rule->{substitutions};
-        foreach (@$substitutions) {
+        foreach my $substitution (@$substitutions) {
-            ( $_->{item_field} ) = ( $_->{field} =~ /items\.(.*)/ );
+            my $field = $substitution->{field} // '';
+            if ( grep { $_ eq $field } @item_columns ) {
+                ( $substitution->{item_field} ) = ( $field =~ /^items\.(.*)/ );
+            } else {
+                Koha::Logger->get->warn(
+                    qq{Rejecting invalid substitution field "$field" in automatic item modification rule})
+                    if length $field;
+            }
         }
         my @params;
 
@@ -1706,6 +1742,9 @@ sub ToggleNewStatus {
                     $query .= qq| AND $condition->{field} = ?|;
                     push @params, $condition->{value};
                 }
+            } else {
+                Koha::Logger->get->warn(
+                    qq{Rejecting invalid condition field "$condition->{field}" in automatic item modification rule});
             }
         }
         if ( defined $age ) {
@@ -1722,6 +1761,7 @@ sub ToggleNewStatus {
                 my $field = $substitution->{item_field};
                 my $value = $substitution->{value};
                 next unless $substitution->{field};
+                next unless defined $field;
                 next
                     if ( defined $values->{ $substitution->{item_field} }
                     and $values->{ $substitution->{item_field} } eq $substitution->{value} );

tools/automatic_item_modification_by_age.pl

@@ -39,6 +39,7 @@ use C4::Auth qw( get_template_and_user );
 use C4::Context;
 use C4::Output qw( output_html_with_http_headers );
 use C4::Koha;
+use C4::Items qw( GetAutomaticItemModificationAgeFields );
 
 use Koha::Items;
 use Koha::Biblioitems;
@@ -62,10 +63,7 @@ my @biblioitem_fields           = map { "biblioitems.$_" } Koha::Biblioitems->co
 my @biblio_fields               = map { "biblio.$_" } Koha::Biblios->columns;
 my @allowed_condition_fields    = ( @item_fields, @biblioitem_fields, @biblio_fields );
 my @allowed_substitution_fields = @item_fields;
-my @allowed_age_fields          = (
+my @allowed_age_fields          = GetAutomaticItemModificationAgeFields();
-    'items.dateaccessioned', 'items.replacementpricedate', 'items.datelastborrowed', 'items.datelastseen',
-    'items.damaged_on',      'items.itemlost_on',          'items.withdrawn_on'
-);
 
 my $syspref_name = q|automatic_item_modification_by_age_configuration|;
 if ( $op eq 'cud-update' ) {