From 8c510e1a92a4ed76de69748bbda91a33b5edd3cf Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Wed, 25 Jan 2017 10:22:13 +0100 Subject: [PATCH] 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 Signed-off-by: Nick Clemens Signed-off-by: Jonathan Druart --- C4/Auth.pm | 1 - C4/Templates.pm | 8 ++++---- Koha/Plugins/Base.pm | 7 +++++-- opac/maintenance.pl | 2 +- t/db_dependent/Auth.t | 2 +- t/db_dependent/Plugins.t | 27 ++++++++++++++++++++++++--- t/db_dependent/Templates.t | 34 +++++++++++++++++++++++++++++----- 7 files changed, 64 insertions(+), 17 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index f2b5f6cf30..bf1457e1ed 100644 --- a/C4/Auth.pm +++ b/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/ ) { diff --git a/C4/Templates.pm b/C4/Templates.pm index 0f9f007d1a..434e7bd7fb 100644 --- a/C4/Templates.pm +++ b/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); diff --git a/Koha/Plugins/Base.pm b/Koha/Plugins/Base.pm index ba20a58fca..bf401b11ce 100644 --- a/Koha/Plugins/Base.pm +++ b/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, } ); diff --git a/opac/maintenance.pl b/opac/maintenance.pl index 1839e2e848..380cf20db7 100755 --- a/opac/maintenance.pl +++ b/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(); diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t index b451f154a6..f1d1ddd702 100644 --- a/t/db_dependent/Auth.t +++ b/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( { diff --git a/t/db_dependent/Plugins.t b/t/db_dependent/Plugins.t index dffcea7d2f..4fa327f47d 100755 --- a/t/db_dependent/Plugins.t +++ b/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' diff --git a/t/db_dependent/Templates.t b/t/db_dependent/Templates.t index 1057ae5b78..73a42a6494 100755 --- a/t/db_dependent/Templates.t +++ b/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' ); +}; -- 2.39.5