[Koha-bugs] [Bug 32478] Remove Koha::Config::SysPref->find since bypasses cache

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478

--- Comment #17 from David Gustafsson <glasklas at gmail.com> ---
(In reply to Marcel de Rooy from comment #15)
> We remove these lines completely now and just trust ->yaml_preference:
> -    foreach my $line (@lines){
> -        my ($field,$array) = split /:/, $line;
> -        next if !$array;
> -        $field =~ s/^\s*|\s*$//g;
> -        $array =~ s/[ [\]\r]//g;
> -        my @array = split /,/, $array;
> -        @array = map { $_ eq '""' || $_ eq "''" ? '' : $_ } @array;
> -        @array = map { $_ eq 'NULL' ? undef : $_ } @array;
> -        $pref_as_hash->{$field} = \@array;
> -    }
> 
> But this is not the same. If I pasted correct YAML previously like below, it
> would not work.
> ccode  :
> - null
> - NULL
> - undefined
> - 2
> - '3'
> Now it does.
> 
> But perhaps more serious is the opposite, if I am entering some valid YAML
> array by 'forgetting' the first line:
> - null
> - NULL
> - undefined
> - 2
> - '3'
> I will get back an array, while the former routine returned an empty hash !
> This will trigger an error like: Not a HASH reference at
> /usr/share/koha/Koha/Item.pm line 2071. => foreach my $field (keys
> %$denyingrules) {
> 
> This is a bit long way of saying: I still think that we may need an
> intermediate routine between pure YAML (yaml_preference) and what we expect
> for specific preferences?

I see your point, but was it not already possible to make that code crash (or
even worse things) by for example using key values that does not exist on the
item object, or for example using "delete" as key :/

 sub is_denied_renewal {
        my ( $self ) = @_;                                                      

        my $denyingrules =
Koha::Config::SysPrefs->find('ItemsDeniedRenewal')->get_yaml_pref_hash();
        return 0 unless $denyingrules;                     
        foreach my $field (keys %$denyingrules) {
            my $val = $self->$field;

Validation would be nice, but feels like out of the scope for this issue. I
will update the patch to check that an actual hash before iterating over the
keys though, as this particular case was not a problem before. If I'm not
missing something $self->$field; where $field is user input is a security
vulnerability and a pretty bad one at that. Should use get_column instead! I
guess should create a new issue for this, or if we handle it in this one?

-- 
You are receiving this mail because:
You are watching all bug changes.