From fc433cbaf5023627d239ed158fc27ef31e7a4356 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Tue, 27 Jan 2026 07:50:34 +0000 Subject: [PATCH] Bug 41651: (QA follow-up) Fix issues in plugin file copy We're focusing on reliability and security here, the improvements include: - Escape regex metacharacters in path using \Q...\E to prevent regex injection issues - Check copy() return value and collect errors, die if any copies fail to prevent silent partial installations - Make basename filter more specific to only skip cpanfile and META.json/META.yml, not files containing "META" anywhere --- Koha/Plugins.pm | 48 ++++++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/Koha/Plugins.pm b/Koha/Plugins.pm index f3a9e849f2b..4e137c4e66d 100644 --- a/Koha/Plugins.pm +++ b/Koha/Plugins.pm @@ -23,11 +23,11 @@ use Array::Utils qw( array_minus ); use Class::Inspector; use CPAN::Meta; use CPAN::Meta::Check qw( verify_dependencies ); -use File::Basename qw( fileparse ); -use File::Copy qw( copy ); -use File::Find qw( find ); -use File::Path qw( make_path ); -use List::MoreUtils qw( any none ); +use File::Basename qw( fileparse ); +use File::Copy qw( copy ); +use File::Find qw( find ); +use File::Path qw( make_path ); +use List::MoreUtils qw( any none ); use Module::CPANfile; use Module::Load::Conditional qw( can_load ); use Module::Load; @@ -242,8 +242,8 @@ sub GetPlugins { { enable_plugins => $self->{'enable_plugins'} - # loads even if plugins are disabled - # FIXME: is this for testing without bothering to mock config? + # loads even if plugins are disabled + # FIXME: is this for testing without bothering to mock config? } ); } catch { @@ -311,24 +311,32 @@ sub InstallPlugins { my $verbose = $params->{verbose} // $self->_verbose; if ( $params->{from} ) { + # We have "staged" plugin(s) passed in; check the dependencies, # copy to the regular location if we're happy - $self->_check_dependencies($params->{from}); + $self->_check_dependencies( $params->{from} ); my $plugins_dir = C4::Context->config("pluginsdir"); $plugins_dir = ref($plugins_dir) eq 'ARRAY' ? $plugins_dir->[0] : $plugins_dir; - find({ - wanted => sub { - return unless -f $_; - my ($basename) = fileparse($_); - return if $basename =~ /^cpanfile|META/; - my $destdir = $File::Find::dir =~ s/^$params->{from}//r; - $destdir = "$plugins_dir/$destdir"; - make_path($destdir); - copy($File::Find::name, $destdir); + my @copy_errors; + find( + { + wanted => sub { + return unless -f $_; + my ($basename) = fileparse($_); + return if $basename =~ /^(cpanfile|META\.(json|yml))$/; + my $destdir = $File::Find::dir =~ s/^\Q$params->{from}\E//r; + $destdir = "$plugins_dir/$destdir"; + make_path($destdir); + unless ( copy( $File::Find::name, $destdir ) ) { + push @copy_errors, "Failed to copy $File::Find::name to $destdir: $!"; + } + }, }, - }, $params->{from}); + $params->{from} + ); + die join( "\n", @copy_errors ) . "\n" if @copy_errors; } my @plugin_classes = $self->plugins(); @@ -493,7 +501,7 @@ sub get_valuebuilders_installed { } sub _check_dependencies { - my ($self, $path) = @_; + my ( $self, $path ) = @_; my $prereqs; @@ -512,7 +520,7 @@ sub _check_dependencies { return unless $prereqs; # Good for both file variants: per the docs, "$meta should be a CPAN::Meta::Prereqs or CPAN::Meta object" - die "$_\n" for verify_dependencies($prereqs, 'runtime', 'requires'); + die "$_\n" for verify_dependencies( $prereqs, 'runtime', 'requires' ); } 1; -- 2.52.0