Attachment 68669 Details for Bug 17989 – Bug 17989: Include full path logic in _get_template_file

[patch] Bug 17989: Include full path logic in _get_template_file

Bug-17989-Include-full-path-logic-in-gettemplatefi.patch (text/plain), 10.71 KB, created by Nick Clemens (kidclamp) on 2017-10-27 09:41:35 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nick Clemens (kidclamp)

Created: 2017-10-27 09:41:35 UTC

Size: 10.71 KB

patch

obsolete

>From ffb7eb1cf07c7140fa30c59613a9ac67173b97cf Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>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 <m.de.rooy@rijksmuseum.nl>
>
>Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
>---
> 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 f2b5f6c..bf1457e 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 0f9f007..434e7bd 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 ba20a58..bf401b1 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 1839e2e..380cf20 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 b451f15..f1d1ddd 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 dffcea7..4fa327f 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 1057ae5..73a42a6 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.1.4

Actions: View | Diff | Splinter Review

Attachments on bug 17989: 65277 | 65278 | 65279 | 65280 | 65281 | 66272 | 66273 | 66274 | 66275 | 66276 | 66317 | 66318 | 66319 | 66320 | 66321 | 67514 | 67515 | 67516 | 67517 | 67518 | 68654 | 68667 | 68668 | 68669 | 68670 | 68671 | 68672 | 68842 | 68843