gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Bug 43092: Stop Local transport falling back to the Koha process's own cwd 

Koha::File::Transport's three backends disagree on what
change_directory(undef) means: FTP navigates to the server root,
SFTP falls back to the SFTP session's own home directory, and Local
fell back to the configured download_directory or, if that's also
unset, silently to '.' - the Koha process's own working directory,
which has no relation to any configured transport location.

test_connection() calls change_directory(undef) unconditionally for
both the download and upload directory checks on every config save
(via the auto-enqueued TestTransport background job). For a Local
transport configured with only one of download_directory/
upload_directory, the untested direction's list_files() call would
silently list the Koha process's own cwd (e.g. the Koha install root)
instead of failing cleanly.

Add a private _working_directory($configured_field) helper that
returns the tracked change_directory() override or the given
configured field, but never falls back to '.'. _upload_file,
_download_file, _list_files and _rename_file now use it and record a
clear "not configured" error instead of silently acting on an
unrelated directory when neither is set.

FTP's root and SFTP's home-directory fallbacks are left as-is: unlike
Local's '.', they navigate to a real, meaningful location for that
specific connection rather than leaking unrelated filesystem content,
so they aren't part of this fix.

Test plan:
1. prove t/db_dependent/Koha/File/Transport/Local.t
   => All tests successful, including the new subtest proving that
      list_files()/rename_file()/download_file() fail cleanly (with a
      "not configured" error) rather than falling back to '.' when
      their respective directory isn't configured, while the
      configured direction (upload, in the test) still works.
2. prove -r t/db_dependent/Koha/File/ t/db_dependent/Koha/Edifact/Transport.t t/db_dependent/api/v1/file_transports.t
   => All tests successful (43 tests total), no regressions.
3. koha-qa.pl -v 2 --more-tests
   => [PASS], no critic/pod warnings.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This commit is contained in:
Martin Renvoize 2026-07-16 16:03:45 +01:00 committed by Pedro Amorim
parent 885d1f65dc
commit a0433635fa
No known key found for this signature in database
GPG Key ID: 06B75BA7F3894D55
2 changed files with 139 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

79
Koha/File/Transport/Local.pm
View File

@ -118,6 +118,28 @@ sub _connect {
return 1;
}
=head3 _working_directory
my $directory = $self->_working_directory('download_directory');
Returns the directory a Local operation should act in: the directory most
recently set via change_directory(), falling back to the given configured
directory field (C<download_directory> or C<upload_directory>).
Returns undef if neither is set. Callers must not fall back to '.' -
that's the Koha process's own working directory, which has no relation to
any configured transport location and would silently list/act on whatever
directory the process happens to be running in (e.g. the Koha install
root) instead of failing cleanly.
=cut
sub _working_directory {
my ( $self, $configured_field ) = @_;
return $self->{current_directory} || $self->$configured_field;
}
=head3 _upload_file
Internal method that performs the local file system upload operation.
@ -130,7 +152,18 @@ sub _upload_file {
my ( $self, $local_file, $remote_file ) = @_;
my $operation = "upload";
my $upload_dir = $self->{current_directory} || $self->upload_directory || '.';
my $upload_dir = $self->_working_directory('upload_directory');
unless ( defined $upload_dir ) {
$self->add_message(
{
message => $operation,
type => 'error',
payload => { error => 'No upload directory configured or set via change_directory()' }
}
);
return;
}
my $destination = File::Spec->catfile( $upload_dir, $remote_file );
unless ( copy( $local_file, $destination ) ) {
@ -170,8 +203,19 @@ sub _download_file {
my ( $self, $remote_file, $local_file ) = @_;
my $operation = 'download';
my $download_dir = $self->{current_directory} || $self->download_directory || '.';
my $source = File::Spec->catfile( $download_dir, $remote_file );
my $download_dir = $self->_working_directory('download_directory');
unless ( defined $download_dir ) {
$self->add_message(
{
message => $operation,
type => 'error',
payload => { error => 'No download directory configured or set via change_directory()' }
}
);
return;
}
my $source = File::Spec->catfile( $download_dir, $remote_file );
unless ( -f $source ) {
$self->add_message(
@ -282,7 +326,17 @@ sub _list_files {
my ($self) = @_;
my $operation = "list";
my $directory = $self->{current_directory} || $self->download_directory || '.';
my $directory = $self->_working_directory('download_directory');
unless ( defined $directory ) {
$self->add_message(
{
message => $operation,
type => 'error',
payload => { error => 'No download directory configured or set via change_directory()' }
}
);
return;
}
unless ( -d $directory ) {
$self->add_message(
@ -371,9 +425,20 @@ sub _rename_file {
my ( $self, $old_name, $new_name ) = @_;
my $operation = "rename";
my $directory = $self->{current_directory} || $self->download_directory || '.';
my $old_path = File::Spec->catfile( $directory, $old_name );
my $new_path = File::Spec->catfile( $directory, $new_name );
my $directory = $self->_working_directory('download_directory');
unless ( defined $directory ) {
$self->add_message(
{
message => $operation,
type => 'error',
payload => { error => 'No download directory configured or set via change_directory()' }
}
);
return;
}
my $old_path = File::Spec->catfile( $directory, $old_name );
my $new_path = File::Spec->catfile( $directory, $new_name );
unless ( -f $old_path ) {
$self->add_message(

68
t/db_dependent/Koha/File/Transport/Local.t
View File

@ -17,7 +17,7 @@
use Modern::Perl;
use Test::More tests => 3;
use Test::More tests => 4;
use Test::NoWarnings;
use File::Temp qw( tempdir );
use File::Spec;
@ -117,4 +117,70 @@ subtest 'current_directory() tests' => sub {
$schema->storage->txn_rollback;
};
subtest 'an unconfigured directory fails cleanly instead of falling back to "." (the Koha process cwd)' => sub {
plan tests => 8;
$schema->storage->txn_begin;
my $tempdir = tempdir( CLEANUP => 1 );
my $upload_dir = File::Spec->catdir( $tempdir, 'upload' );
mkdir $upload_dir or die "Cannot create upload_dir: $!";
# Only upload_directory is configured - download_directory is explicitly
# unset (TestBuilder would otherwise auto-fill it with a random string),
# as is any change_directory() override.
my $transport = $builder->build_object(
{
class => 'Koha::File::Transports',
value => {
transport => 'local',
upload_directory => $upload_dir,
download_directory => undef,
}
}
);
my $files = $transport->list_files();
is( $files, undef, 'list_files() fails when no download_directory is configured and no override is set' );
my ($list_error) = grep { $_->type eq 'error' } @{ $transport->object_messages };
ok( $list_error, 'list_files() records an error message' );
like(
$list_error->payload->{error}, qr/No download directory configured/,
'error explains the directory is unconfigured, rather than silently listing an unrelated directory'
);
my $renamed = $transport->rename_file( 'a.txt', 'b.txt' );
is( $renamed, undef, 'rename_file() fails the same way when no download_directory is configured' );
my $tmp_upload_src = File::Spec->catfile( $tempdir, 'to_upload.txt' );
open my $fh, '>', $tmp_upload_src or die "Cannot create test file: $!";
print $fh 'content';
close $fh;
# upload_directory IS configured, so this direction should still work.
my $uploaded = $transport->upload_file( $tmp_upload_src, 'to_upload.txt' );
is( $uploaded, 1, 'upload_file() still succeeds when upload_directory is configured' );
ok(
-f File::Spec->catfile( $upload_dir, 'to_upload.txt' ),
'the file was uploaded to the configured upload_directory'
);
# download_file() has no download_directory to fall back to either. Reset
# {current_directory} first: the auto-managed upload_file() call above
# left it pointing at upload_dir as a side effect (unrelated to this fix)
# of _auto_change_directory() calling _change_directory() internally.
$transport->{current_directory} = undef;
my $downloaded = $transport->download_file( 'to_upload.txt', File::Spec->catfile( $tempdir, 'downloaded.txt' ) );
is( $downloaded, undef, 'download_file() fails when no download_directory is configured' );
my ($download_error) = grep { $_->type eq 'error' } reverse @{ $transport->object_messages };
like(
$download_error->payload->{error}, qr/No (?:download|upload) directory configured/,
'download_file() also records a clear "not configured" error rather than falling back to "."'
);
$schema->storage->txn_rollback;
};
1;