Bug 17989: Include full path logic in _get_template_file

Similar to the full path test in sub themelanguage, this patch makes a
change in _get_template_file. This allows you to pass a template
outside the modules directory to get_template_and_user. (Note: the sub
badtemplatecheck already blocks bad paths.)

Especially, this would be helpful for plugins using templates. As can be
seen in Templates.pm, a change was made earlier to overwrite the filename
for a plugin in sub gettemplate. This exception can now be removed.

Also note the small change in Koha/Plugin/Base.pm; mbf_path is already
absolute and if we pass a full path, we do not need it. This allows use of
a regular Koha template or a shared template between plugins (as long as
badtemplatecheck allows the path).

What are the side-effects of this change?
[1] We should not pass absolute paths if we mean relative ones.
    A follow-up patch deals with one occurrence in the codebase.
    No regressions for regular use.
[2] Plugins can call get_template_and_user directly or go via get_template
    in Koha/Plugin/Base (absolute paths don't go via mbf_path).

Note: replaced two single quotes in Auth.pm to show template name in test
description.

Test plan:
[1] Open some page on OPAC or staff client to trigger a template.
[2] Run t/db_dependent/Auth.t to verify not allowing some bad templates.
[3] Run t/db_dependent/Templates.t to verify an absolute path.
[4] Run t/db_dependent/Plugins.t to verify using templates in a plugin.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

C4/Auth.pm

@@ -168,7 +168,6 @@ sub get_template_and_user {
         $in->{'template_name'},
         $in->{'type'},
         $in->{'query'},
-        $in->{'is_plugin'}
     );
 
     if ( $in->{'template_name'} !~ m/maintenance/ ) {

C4/Templates.pm

@@ -164,9 +164,10 @@ sub _get_template_file {
     my ($theme, $lang, $availablethemes) = themelanguage($htdocs, $tmplbase, $interface, $query);
     $lang //= 'en';
     $theme //= '';
-    my $filename = "$htdocs/$theme/$lang/modules/$tmplbase";
+    $tmplbase = "$htdocs/$theme/$lang/modules/$tmplbase" if $tmplbase !~ /^\//;
+        # do not prefix an absolute path
 
-    return ($htdocs, $theme, $lang, $filename);
+    return ( $htdocs, $theme, $lang, $tmplbase );
 }
 
 =head2 badtemplatecheck
@@ -201,11 +202,10 @@ sub badtemplatecheck {
 }
 
 sub gettemplate {
-    my ( $tmplbase, $interface, $query, $is_plugin ) = @_;
+    my ( $tmplbase, $interface, $query ) = @_;
     ($query) or warn "no query in gettemplate";
     my ($htdocs, $theme, $lang, $filename)
        =  _get_template_file($tmplbase, $interface, $query);
-    $filename = $tmplbase if ( $is_plugin );
     badtemplatecheck( $filename ); # single trip for bad templates
     my $template = C4::Templates->new($interface, $filename, $tmplbase, $query);
 

Koha/Plugins/Base.pm

@@ -106,12 +106,15 @@ sub get_template {
 
     require C4::Auth;
 
+    my $template_name = $args->{'file'} // '';
+    # if not absolute, call mbf_path, which dies if file does not exist
+    $template_name = $self->mbf_path( $template_name )
+        if $template_name !~ m/^\//;
     my ( $template, $loggedinuser, $cookie ) = C4::Auth::get_template_and_user(
-        {   template_name   => abs_path( $self->mbf_path( $args->{'file'} ) ),
+        {   template_name   => $template_name,
             query           => $self->{'cgi'},
             type            => "intranet",
             authnotrequired => 1,
-            is_plugin       => 1,
         }
     );
 

opac/maintenance.pl

@@ -25,7 +25,7 @@ use C4::Templates qw/gettemplate/;
 use Koha;
 
 my $query = new CGI;
-my $template = C4::Templates::gettemplate( 'maintenance.tt', 'opac', $query, 0 );
+my $template = C4::Templates::gettemplate( 'maintenance.tt', 'opac', $query );
 
 my $koha_db_version = C4::Context->preference('Version');
 my $kohaversion     = Koha::version();

t/db_dependent/Auth.t

@@ -192,7 +192,7 @@ my $hash2 = hash_password('password');
                 }
             );
         };
-        like ( $@, qr(^bad template path), 'The file $template_name should not be accessible' );
+        like ( $@, qr(^bad template path), "The file $template_name should not be accessible" );
     }
     ( $template, $loggedinuser, $cookies ) = get_template_and_user(
         {

t/db_dependent/Plugins.t

@@ -2,12 +2,15 @@
 
 use Modern::Perl;
 
-use Test::More tests => 31;
+use Test::More tests => 32;
+use CGI;
 use File::Basename;
-use File::Temp qw( tempdir );
+use File::Spec;
+use File::Temp qw( tempdir tempfile );
 use FindBin qw($Bin);
 use Archive::Extract;
 use Module::Load::Conditional qw(can_load);
+use Test::MockModule;
 
 use C4::Context;
 use t::lib::Mocks;
@@ -21,9 +24,17 @@ BEGIN {
     use_ok('Koha::Plugin::Test');
 }
 
+my $mock_plugin = Test::MockModule->new( 'Koha::Plugin::Test' );
+$mock_plugin->mock( 'test_template', sub {
+    my ( $self, $file ) = @_;
+    my $template = $self->get_template({ file => $file });
+    $template->param( filename => $file );
+    return $template->output;
+});
+
 ok( can_load( modules => { "Koha::Plugin::Test" => undef } ), 'Test can_load' );
 
-my $plugin = Koha::Plugin::Test->new({ enable_plugins => 1});
+my $plugin = Koha::Plugin::Test->new({ enable_plugins => 1, cgi => CGI->new });
 
 isa_ok( $plugin, "Koha::Plugin::Test", 'Test plugin class' );
 isa_ok( $plugin, "Koha::Plugins::Base", 'Test plugin parent class' );
@@ -46,6 +57,16 @@ is( $metadata->{'name'}, 'Test Plugin', 'Test $plugin->get_metadata()' );
 is( $plugin->get_qualified_table_name('mytable'), 'koha_plugin_test_mytable', 'Test $plugin->get_qualified_table_name()' );
 is( $plugin->get_plugin_http_path(), '/plugin/Koha/Plugin/Test', 'Test $plugin->get_plugin_http_path()' );
 
+# test absolute path change in get_template with Koha::Plugin::Test
+# using the mock set before
+# we also add tmpdir as an approved template dir
+t::lib::Mocks::mock_config( 'pluginsdir', [ File::Spec->tmpdir ] );
+my ( $fh, $fn ) = tempfile( SUFFIX => '.tt', UNLINK => 1 );
+print $fh 'I am [% filename %]';
+close $fh;
+my $classname = ref($plugin);
+like( $plugin->test_template($fn), qr/^I am $fn/, 'Template works' );
+
 # testing GetPlugins
 my @plugins = Koha::Plugins->new({ enable_plugins => 1 })->GetPlugins({
     method => 'report'

t/db_dependent/Templates.t

@@ -19,13 +19,17 @@ use Modern::Perl;
 
 use CGI;
 
-use Test::More tests => 7;
+use Test::More tests => 8;
 use Test::Deep;
 use Test::MockModule;
 use Test::Warn;
+use File::Spec;
+use File::Temp qw/tempfile/;
 
 use t::lib::Mocks;
 
+use C4::Auth qw//;
+
 BEGIN {
     use_ok( 'C4::Templates' );
     can_ok( 'C4::Templates',
@@ -102,13 +106,13 @@ subtest 'Testing gettemplate/badtemplatecheck' => sub {
 
     my $cgi = CGI->new;
     my $template;
-    warning_like { eval { $template = C4::Templates::gettemplate( '/etc/passwd', 'opac', $cgi, 1 ) }; warn $@ if $@; } qr/bad template/, 'Bad template check';
+    warning_like { eval { $template = C4::Templates::gettemplate( '/etc/passwd', 'opac', $cgi ) }; warn $@ if $@; } qr/bad template/, 'Bad template check';
     is( $template ? $template->output: '', '', 'Check output' );
 
     # Test a few more bad paths to gettemplate triggering badtemplatecheck
-    warning_like { eval { C4::Templates::gettemplate( '../topsecret.tt', 'opac', $cgi, 1 ) }; warn $@ if $@; } qr/bad template/, 'No safe chars';
-    warning_like { eval { C4::Templates::gettemplate( '/noaccess/topsecret.tt', 'opac', $cgi, 1 ) }; warn $@ if $@; } qr/bad template/, 'Directory not allowed';
-    warning_like { eval { C4::Templates::gettemplate( C4::Context->config('intrahtdocs') . '2/prog/en/modules/about.tt', 'intranet', $cgi, 1 ) }; warn $@ if $@; } qr/bad template/, 'Directory not allowed too';
+    warning_like { eval { C4::Templates::gettemplate( '../topsecret.tt', 'opac', $cgi ) }; warn $@ if $@; } qr/bad template/, 'No safe chars';
+    warning_like { eval { C4::Templates::gettemplate( '/noaccess/topsecret.tt', 'opac', $cgi ) }; warn $@ if $@; } qr/bad template/, 'Directory not allowed';
+    warning_like { eval { C4::Templates::gettemplate( C4::Context->config('intrahtdocs') . '2/prog/en/modules/about.tt', 'intranet', $cgi ) }; warn $@ if $@; } qr/bad template/, 'Directory not allowed too';
 
     # Allow templates from /tmp
     t::lib::Mocks::mock_config( 'pluginsdir', [ '/tmp' ] );
@@ -117,3 +121,23 @@ subtest 'Testing gettemplate/badtemplatecheck' => sub {
     warning_like { eval { C4::Templates::badtemplatecheck( '/tmp/about.tmpl' ) }; warn $@ if $@; } qr/bad template/, 'Warn on bad extension';
 };
 
+subtest "Absolute path change in _get_template_file" => sub {
+    plan tests => 1;
+
+    # We create a simple template in /tmp.
+    # We simulate an anonymous OPAC session; the OPACBaseURL template variable
+    # should be filled by get_template_and_user.
+    t::lib::Mocks::mock_config( 'pluginsdir', [ File::Spec->tmpdir ] );
+    t::lib::Mocks::mock_preference( 'OPACBaseURL', 'without any doubt' );
+    my ( $fh, $fn ) = tempfile( SUFFIX => '.tt', UNLINK => 1 );
+    print $fh q|I am a [% quality %] template [% OPACBaseURL %]|;
+    close $fh;
+    my ( $template, $login, $cookie ) = C4::Auth::get_template_and_user({
+        template_name => $fn,
+        query => CGI::new,
+        type => "opac",
+        authnotrequired => 1,
+    });
+    $template->param( quality => 'good-for-nothing' );
+    like( $template->output, qr/a good.+template.+doubt/, 'Testing a template with an absolute path' );
+};