From 6fe61773dc3bbbe9570922fd72cf8d615180946b Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Thu, 23 Oct 2025 09:47:45 +0100 Subject: [PATCH] Bug 40811: (follow-up) Fix list context bug and add comprehensive tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch fixes a critical bug where plain_text_password() was called in list context during hash construction. When a transport has no password, the method's bare 'return' statement returns an empty list () in list context rather than undef, causing all subsequent key-value pairs to shift positions. This resulted in malformed connection parameters being passed to Net::SFTP::Foreign and Net::FTP. Example of the bug: my %hash = ( user => 'john', password => $self->plain_text_password, # Returns () when no password timeout => 10 ); # Results in: (user => 'john', timeout => 10) instead of # (user => 'john', password => undef, timeout => 10) Changes to code: - Wrap plain_text_password calls with scalar() to force scalar context in both FTP.pm and SFTP.pm - Make SFTP key and password authentication mutually exclusive using ternary operator - Prefer key-based auth over password when both are available (SFTP) Test plan: 1. Run t/db_dependent/Koha/File/Transport/FTP.t - should pass 2. Run t/db_dependent/Koha/File/Transport/SFTP.t - should pass 3. Verify tests cover: - Scalar context behavior with no password - FTP authentication with proper parameter passing - SFTP password-only authentication - SFTP key-only authentication - SFTP key preference when both key and password exist Signed-off-by: Tomás Cohen Arazi --- Koha/File/Transport/FTP.pm | 2 +- Koha/File/Transport/SFTP.pm | 13 ++- t/db_dependent/Koha/File/Transport/FTP.t | 58 +++++++++++- t/db_dependent/Koha/File/Transport/SFTP.t | 109 +++++++++++++++++++++- 4 files changed, 174 insertions(+), 8 deletions(-) diff --git a/Koha/File/Transport/FTP.pm b/Koha/File/Transport/FTP.pm index 47bc264054..d2ad1dafeb 100644 --- a/Koha/File/Transport/FTP.pm +++ b/Koha/File/Transport/FTP.pm @@ -46,7 +46,7 @@ sub _connect { Passive => $self->passive ? 1 : 0, ) or return $self->_abort_operation($operation); - $self->{connection}->login( $self->user_name, $self->plain_text_password ) + $self->{connection}->login( $self->user_name, scalar $self->plain_text_password ) or return $self->_abort_operation($operation); $self->add_message( diff --git a/Koha/File/Transport/SFTP.pm b/Koha/File/Transport/SFTP.pm index 0df9c30b75..51f609a7b8 100644 --- a/Koha/File/Transport/SFTP.pm +++ b/Koha/File/Transport/SFTP.pm @@ -23,6 +23,7 @@ use File::Spec; use IO::File; use Net::SFTP::Foreign; use Try::Tiny; +use JSON qw( decode_json encode_json ); use base qw(Koha::File::Transport); @@ -47,12 +48,14 @@ sub _connect { # String to capture STDERR output $self->{stderr_capture} = ''; open my $stderr_fh, '>', \$self->{stderr_capture} or die "Can't open scalar as filehandle: $!"; + + # Use key-based auth if available, otherwise password auth + my $key_file = $self->_locate_key_file; $self->{connection} = Net::SFTP::Foreign->new( - host => $self->host, - port => $self->port, - user => $self->user_name, - password => $self->plain_text_password, - $self->_locate_key_file ? ( key_path => $self->_locate_key_file ) : (), + host => $self->host, + port => $self->port, + user => $self->user_name, + $key_file ? ( key_path => $key_file ) : ( password => scalar $self->plain_text_password ), timeout => $self->DEFAULT_TIMEOUT, stderr_fh => $stderr_fh, more => [qw( -v -o StrictHostKeyChecking=no)], diff --git a/t/db_dependent/Koha/File/Transport/FTP.t b/t/db_dependent/Koha/File/Transport/FTP.t index 86aa400e45..214314c7f4 100755 --- a/t/db_dependent/Koha/File/Transport/FTP.t +++ b/t/db_dependent/Koha/File/Transport/FTP.t @@ -17,10 +17,11 @@ use Modern::Perl; -use Test::More tests => 6; +use Test::More tests => 7; use Test::Exception; use Test::NoWarnings; use Test::Warn; +use Test::MockModule; use Koha::File::Transports; @@ -30,6 +31,61 @@ use t::lib::Mocks; my $schema = Koha::Database->new->schema; my $builder = t::lib::TestBuilder->new; +subtest 'scalar context in authentication tests' => sub { + plan tests => 4; + + $schema->storage->txn_begin; + + # Test 1: Transport with no password - plain_text_password should return undef in scalar context + my $transport_no_pass = $builder->build_object( + { + class => 'Koha::File::Transports', + value => { transport => 'ftp', password => undef } + } + ); + + # Mock Net::FTP to verify parameters passed to login() + my $login_called = 0; + my @login_params; + my $mock_ftp = Test::MockModule->new('Net::FTP'); + $mock_ftp->mock( + 'new', + sub { + my $class = shift; + return bless {}, $class; + } + ); + $mock_ftp->mock( + 'login', + sub { + $login_called = 1; + @login_params = @_; + return 1; + } + ); + $mock_ftp->mock( 'quit', sub { return 1; } ); + $mock_ftp->mock( 'abort', sub { return 1; } ); + $mock_ftp->mock( 'status', sub { return 0; } ); + $mock_ftp->mock( 'message', sub { return ''; } ); + + # Test that plain_text_password returns undef when no password set + is( + scalar $transport_no_pass->plain_text_password, undef, + 'plain_text_password returns undef in scalar context when no password' + ); + + # Test 2: Attempt connection with no password - should pass undef not empty list + $transport_no_pass->connect; + is( $login_called, 1, 'login was called' ); + is( scalar @login_params, 3, 'login called with correct number of parameters (self, user, password)' ); + is( $login_params[2], undef, 'password parameter is undef, not empty list' ); + + # Clean up connection to avoid warnings in DESTROY + $transport_no_pass->{connection} = undef; + + $schema->storage->txn_rollback; +}; + subtest 'connect() tests' => sub { plan tests => 1; diff --git a/t/db_dependent/Koha/File/Transport/SFTP.t b/t/db_dependent/Koha/File/Transport/SFTP.t index 8e5febfcfa..33ec965ba9 100755 --- a/t/db_dependent/Koha/File/Transport/SFTP.t +++ b/t/db_dependent/Koha/File/Transport/SFTP.t @@ -17,11 +17,13 @@ use Modern::Perl; -use Test::More tests => 8; +use Test::More tests => 9; use Test::Exception; use Test::NoWarnings; use Test::Warn; +use Test::MockModule; +use Koha::Encryption; use Koha::File::Transports; use t::lib::TestBuilder; @@ -30,6 +32,111 @@ use t::lib::Mocks; my $schema = Koha::Database->new->schema; my $builder = t::lib::TestBuilder->new; +subtest 'scalar context and mutually exclusive authentication tests' => sub { + plan tests => 10; + + $schema->storage->txn_begin; + + # Setup mock upload path for key file operations + my $path = '/tmp/kohadev_test'; + t::lib::Mocks::mock_config( 'upload_path', $path ); + mkdir $path if !-d $path; + + # Mock Net::SFTP::Foreign to capture constructor parameters + my %sftp_new_params; + my $mock_sftp = Test::MockModule->new('Net::SFTP::Foreign'); + $mock_sftp->mock( + 'new', + sub { + my $class = shift; + %sftp_new_params = @_; + my $obj = bless { status => 0, error => undef }, $class; + return $obj; + } + ); + my $undef; + $mock_sftp->mock( 'error', sub { return $undef; } ); + $mock_sftp->mock( 'status', sub { return 0; } ); + $mock_sftp->mock( 'cwd', sub { return '/'; } ); + + # Test 1: Transport with no password and no key - plain_text_password returns undef + my $transport_no_auth = $builder->build_object( + { + class => 'Koha::File::Transports', + value => { transport => 'sftp', password => undef, key_file => undef } + } + ); + + is( + scalar $transport_no_auth->plain_text_password, undef, + 'plain_text_password returns undef in scalar context when no password' + ); + + # Test 2: Connection with no auth should pass password => undef (not empty list) + $transport_no_auth->connect; + ok( exists $sftp_new_params{password}, 'password key exists in connection params' ); + is( $sftp_new_params{password}, undef, 'password parameter is undef when no password set' ); + ok( !exists $sftp_new_params{key_path}, 'key_path is not set when no key file exists' ); + + # Test 3: Transport with key only - should use key_path, not password + # Must encrypt key_file since plain_text_key() will decrypt it + my $encrypted_key = Koha::Encryption->new->encrypt_hex('test_key_content'); + my $transport_key_only = $builder->build_object( + { + class => 'Koha::File::Transports', + value => { transport => 'sftp', password => undef, key_file => $encrypted_key } + } + ); + $transport_key_only->store; # Trigger key file write + $transport_key_only->discard_changes; + + %sftp_new_params = (); # Clear params + $transport_key_only->connect; + ok( exists $sftp_new_params{key_path}, 'key_path exists in connection params when key is set' ); + ok( !exists $sftp_new_params{password}, 'password is not set when using key-based auth' ); + + # Test 4: Transport with password only - should use password, not key_path + # Must encrypt password since plain_text_password() will decrypt it + my $encrypted_pass = Koha::Encryption->new->encrypt_hex('testpass'); + my $transport_pass_only = $builder->build_object( + { + class => 'Koha::File::Transports', + value => { transport => 'sftp', password => $encrypted_pass, key_file => undef } + } + ); + + %sftp_new_params = (); # Clear params + $transport_pass_only->connect; + ok( exists $sftp_new_params{password}, 'password exists in connection params when password is set' ); + is( $sftp_new_params{password}, 'testpass', 'password is correctly decrypted' ); + ok( !exists $sftp_new_params{key_path}, 'key_path is not set when using password auth' ); + + # Test 5: Transport with both key and password - should prefer key + my $encrypted_both_pass = Koha::Encryption->new->encrypt_hex('bothpass'); + my $encrypted_both_key = Koha::Encryption->new->encrypt_hex('both_test_key'); + my $transport_both = $builder->build_object( + { + class => 'Koha::File::Transports', + value => { transport => 'sftp', password => $encrypted_both_pass, key_file => $encrypted_both_key } + } + ); + $transport_both->store; # Trigger key file write + $transport_both->discard_changes; + + %sftp_new_params = (); # Clear params + $transport_both->connect; + ok( + exists $sftp_new_params{key_path} && !exists $sftp_new_params{password}, + 'When both key and password exist, key_path is used and password is excluded' + ); + + # Cleanup + unlink $transport_key_only->_locate_key_file if $transport_key_only->_locate_key_file; + unlink $transport_both->_locate_key_file if $transport_both->_locate_key_file; + + $schema->storage->txn_rollback; +}; + subtest 'store() and _post_store_trigger() tests' => sub { plan tests => 2; -- 2.50.1 (Apple Git-155)