Bug 17989: Extend bad template check

The check is now extended by only allowing templates from the regular
Koha template directories and additional plugin directories as defined
in koha-conf.xml.

Note: In order to prevent an uninitialized warning on $theme from sub
themelanguage for a not-existing file, I added a trivial assignment. Will
get further attention in a follow-up.

Test plan:
[1] Run t/db_dependent/Auth.t
[2] Run t/db_dependent/Templates.t

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>

diff --git a/C4/Templates.pm b/C4/Templates.pm
index 94c5baf..0f9f007 100644 (file)
--- a/C4/Templates.pm
+++ b/C4/Templates.pm
@@ -163,6 +163,7 @@ sub _get_template_file {
     my $htdocs = C4::Context->config($is_intranet ? 'intrahtdocs' : 'opachtdocs');
     my ($theme, $lang, $availablethemes) = themelanguage($htdocs, $tmplbase, $interface, $query);
     $lang //= 'en';
+    $theme //= '';
     my $filename = "$htdocs/$theme/$lang/modules/$tmplbase";
 
     return ($htdocs, $theme, $lang, $filename);
@@ -181,8 +182,22 @@ sub _get_template_file {
 
 sub badtemplatecheck {
     my ( $template ) = @_;
-    Koha::Exceptions::NoPermission->throw( 'bad template path' )
-        unless $template =~ m/^[a-zA-Z0-9_\-\/]+\.(tt|pref)$/;
+    if( !$template || $template !~ m/^[a-zA-Z0-9_\-\/]+\.(tt|pref)$/ ) {
+        # This also includes two dots
+        Koha::Exceptions::NoPermission->throw( 'bad template path' );
+    } else {
+        # Check allowed dirs
+        my $dirs = C4::Context->config("pluginsdir");
+        $dirs = [ $dirs ] if !ref($dirs);
+        unshift @$dirs, C4::Context->config('opachtdocs'), C4::Context->config('intrahtdocs');
+        my $found = 0;
+        foreach my $dir ( @$dirs ) {
+            $dir .= '/' if $dir !~ m/\/$/;
+            $found++ if $template =~ m/^$dir/;
+            last if $found;
+        }
+        Koha::Exceptions::NoPermission->throw( 'bad template path' ) if !$found;
+    }
 }
 
 sub gettemplate {

diff --git a/t/db_dependent/Templates.t b/t/db_dependent/Templates.t
index 760b9f0..1057ae5 100755 (executable)
--- a/t/db_dependent/Templates.t
+++ b/t/db_dependent/Templates.t
@@ -97,11 +97,23 @@ subtest 'Testing themelanguage' => sub {
     return;
 };
 
-subtest 'Testing gettemplate' => sub {
-    plan tests => 2;
+subtest 'Testing gettemplate/badtemplatecheck' => sub {
+    plan tests => 7;
 
+    my $cgi = CGI->new;
     my $template;
-    warning_like { eval { $template = C4::Templates::gettemplate( '/etc/passwd', 'opac', CGI->new, 1 ) }; warn $@ if $@; } qr/bad template/, 'Bad template check';
+    warning_like { eval { $template = C4::Templates::gettemplate( '/etc/passwd', 'opac', $cgi, 1 ) }; 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';
+
+    # Allow templates from /tmp
+    t::lib::Mocks::mock_config( 'pluginsdir', [ '/tmp' ] );
+    warning_like { eval { C4::Templates::badtemplatecheck( '/tmp/about.tt' ) }; warn $@ if $@; } undef, 'No warn on template from plugin dir';
+    # Refuse wrong extension
+    warning_like { eval { C4::Templates::badtemplatecheck( '/tmp/about.tmpl' ) }; warn $@ if $@; } qr/bad template/, 'Warn on bad extension';
 };