From a0433635fa0f7791292129037fdebc8f6df61bf0 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Thu, 16 Jul 2026 16:03:45 +0100 Subject: [PATCH] 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 Signed-off-by: Andrew Fuerste Henry Signed-off-by: Pedro Amorim --- Koha/File/Transport/Local.pm | 79 ++++++++++++++++++++-- t/db_dependent/Koha/File/Transport/Local.t | 68 ++++++++++++++++++- 2 files changed, 139 insertions(+), 8 deletions(-) diff --git a/Koha/File/Transport/Local.pm b/Koha/File/Transport/Local.pm index c2076ec1cd..985380c77f 100644 --- a/Koha/File/Transport/Local.pm +++ b/Koha/File/Transport/Local.pm @@ -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 or C). + +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( diff --git a/t/db_dependent/Koha/File/Transport/Local.t b/t/db_dependent/Koha/File/Transport/Local.t index 1b6691cc17..717c3e609d 100755 --- a/t/db_dependent/Koha/File/Transport/Local.t +++ b/t/db_dependent/Koha/File/Transport/Local.t @@ -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;