Twig config improvements (#5923)

* deprecate direct kimai_config access, which was only used internally
* prevent access to system configs
* prevent access to most configs in sandbox environments

phpstan.neon

@@ -3435,11 +3435,6 @@ parameters:
             count: 1
             path: src/Twig/Configuration.php
 
-        -
-            message: "#^Parameter \\#1 \\$callback of function call_user_func expects callable\\(\\)\\: mixed, array\\{App\\\\Configuration\\\\SystemConfiguration, mixed\\} given\\.$#"
-            count: 1
-            path: src/Twig/Configuration.php
-
         -
             message: "#^Parameter \\#1 \\$string of function strtolower expects string, string\\|null given\\.$#"
             count: 2

src/Twig/Configuration.php

@@ -11,23 +11,26 @@ namespace App\Twig;
 
 use App\Configuration\SystemConfiguration;
 use App\Constants;
+use Twig\Environment;
 use Twig\Extension\AbstractExtension;
+use Twig\Extension\SandboxExtension;
+use Twig\Sandbox\SecurityError;
 use Twig\TwigFunction;
 
 final class Configuration extends AbstractExtension
 {
-    public function __construct(private SystemConfiguration $configuration)
+    public function __construct(private readonly SystemConfiguration $configuration)
     {
     }
 
     public function getFunctions(): array
     {
         return [
-            new TwigFunction('config', [$this, 'get']),
+            new TwigFunction('config', $this->get(...), ['needs_environment' => true]),
         ];
     }
 
-    public function get(string $name)
+    public function get(Environment $environment, string $name)
     {
         switch ($name) {
             case 'chart-class':
@@ -42,6 +45,31 @@ final class Configuration extends AbstractExtension
                 return '300';
             case 'theme.calendar.background_color':
                 return Constants::DEFAULT_COLOR;
+            case 'themeAllowAvatarUrls':
+                return $this->configuration->isThemeAllowAvatarUrls();
+                // whitelisted configs that can be read even in invoice environments
+            case 'theme.branding.logo':
+            case 'theme.branding.company':
+                return $this->configuration->find($name);
+        }
+
+        if (str_starts_with($name, 'saml.') || str_starts_with($name, 'ldap.')) {
+            throw new SecurityError(\sprintf('Templates cannot access security configuration %s.', $name));
+        }
+
+        if ($environment->hasExtension(SandboxExtension::class)) {
+            $sandbox = $environment->getExtension(SandboxExtension::class);
+            if ($sandbox->isSandboxed()) {
+                throw new SecurityError('Sandboxed template tried to access configuration key: ' . $name);
+            }
+        }
+
+        $checks = ['is' . $name, 'get' . $name, 'has' . $name, $name];
+
+        foreach ($checks as $methodName) {
+            if (method_exists($this->configuration, $methodName)) {
+                return \call_user_func($this->configuration->$methodName(...));
+            }
         }
 
         return $this->configuration->find($name);
@@ -49,14 +77,8 @@ final class Configuration extends AbstractExtension
 
     public function __call($name, $arguments)
     {
-        $checks = ['is' . $name, 'get' . $name, 'has' . $name, $name];
+        @trigger_error('Accessing "kimai_config" is deprecated and always return null, use config() instead', E_USER_DEPRECATED);
 
-        foreach ($checks as $methodName) {
+        return null;
-            if (method_exists($this->configuration, $methodName)) {
-                return \call_user_func([$this->configuration, $methodName], $arguments);
-            }
-        }
-
-        return $this->configuration->find($name);
     }
 }

src/Twig/Context.php

@@ -9,12 +9,11 @@
 
 namespace App\Twig;
 
-use App\Configuration\SystemConfiguration;
 use Symfony\Component\HttpFoundation\RequestStack;
 
 final class Context
 {
-    public function __construct(private SystemConfiguration $systemConfiguration, private RequestStack $requestStack)
+    public function __construct(private readonly RequestStack $requestStack)
     {
     }
 
@@ -58,8 +57,8 @@ final class Context
 
     public function getBranding(string $config): mixed
     {
-        @trigger_error('Use "kimai_config" instead of "kimai_context" to access system configurations', E_USER_DEPRECATED);
+        @trigger_error('Use config() instead of "kimai_context" to access system configurations', E_USER_DEPRECATED);
 
-        return $this->systemConfiguration->find('theme.branding.' . $config);
+        return null;
     }
 }

templates/activity/details.html.twig

@@ -93,7 +93,7 @@
     {% endif %}
 
     {% if stats is not null %}
-        {% set currency = kimai_config.customerDefaultCurrency %}
+        {% set currency = config('customerDefaultCurrency') %}
         {% if activity.project is not null %}
             {% set currency = activity.project.customer.currency %}
         {% endif %}

templates/bundles/TablerBundle/components/avatar_image.html.twig

@@ -1,5 +1,5 @@
 {% macro avatar_image(user) %}
-    {% if user.avatar is not empty and kimai_config.themeAllowAvatarUrls %}
+    {% if user.avatar is not empty and config('themeAllowAvatarUrls') %}
         {% set avatar = asset(user.avatar, 'avatars') %}
         <span class="avatar" style="background-image:url({{ avatar }})" title="{{ user.displayName }}">&nbsp;</span>
     {% else %}

templates/macros/widgets.html.twig

@@ -122,7 +122,7 @@
 
 {% macro user_avatar(user, tooltip, class, badge) %}
     {% set avatar = null %}
-    {% if user.avatar is not empty and kimai_config.themeAllowAvatarUrls %}
+    {% if user.avatar is not empty and config('themeAllowAvatarUrls') %}
         {% set avatar = asset(user.avatar, 'avatars') %}
     {% endif %}
     {% if not user.enabled %}

templates/security/login.html.twig

@@ -48,12 +48,12 @@
 
 {% block login_social_auth %}
     {% if saml_config.isActivated() %}
-        {% if kimai_config.loginFormActive %}
+        {% if config('loginFormActive') %}
             <div class="hr-text">{{ 'or'|trans({}, 'TablerBundle') }}</div>
         {% endif %}
         <div class="card-body">
             <div class="row">
-                {% if not kimai_config.loginFormActive %}
+                {% if not config('loginFormActive') %}
                     <h2 class="card-title text-center mb-4">{{ block('login_box_msg') }}</h2>
                 {% endif %}
                 <div class="col text-center">
@@ -75,25 +75,25 @@
 {% endblock %}
 
 {% block login_box %}
-    {% if kimai_config.loginFormActive %}
+    {% if config('loginFormActive') %}
         {{ parent() }}
     {% endif %}
 {% endblock %}
 
 {% block login_form %}
-    {% if kimai_config.loginFormActive %}
+    {% if config('loginFormActive') %}
         {{ parent() }}
     {% endif %}
 {% endblock %}
 
 {% block password_forgotten %}
-    {% if kimai_config.passwordResetActive %}
+    {% if config('passwordResetActive') %}
         {{ parent() }}
     {% endif %}
 {% endblock %}
 
 {% block registration %}
-    {% if kimai_config.selfRegistrationActive %}
+    {% if config('selfRegistrationActive') %}
         {{ parent() }}
     {% endif %}
 {% endblock %}

templates/security/unlock.html.twig

@@ -19,7 +19,7 @@
             {% endif %}
         </div>
         {% block unlock_form %}
-            {% if kimai_config.loginFormActive %}
+            {% if config('loginFormActive') %}
             <form action="{{ path('tabler_login_check'|tabler_route) }}" method="post" autocomplete="off" class="login-box-body security-login">
                 <input type="hidden" name="_remember_me" value="on">
                 <input type="hidden" name="_username" value="{{ app.user.userIdentifier }}">

tests/Twig/ConfigurationTest.php

@@ -0,0 +1,193 @@
+<?php
+
+/*
+ * This file is part of the Kimai time-tracking app.
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace App\Tests\Twig;
+
+use App\Constants;
+use App\Tests\Mocks\SystemConfigurationFactory;
+use App\Twig\Configuration;
+use App\Twig\SecurityPolicy\StrictPolicy;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+use Twig\Environment;
+use Twig\Extension\SandboxExtension;
+use Twig\Loader\ArrayLoader;
+use Twig\Sandbox\SecurityError;
+use Twig\TwigFunction;
+
+#[CoversClass(Configuration::class)]
+class ConfigurationTest extends TestCase
+{
+    private function createEnvironment(array $templates = ['template' => ''], bool $sandboxed = false): Environment
+    {
+        $environment = new Environment(new ArrayLoader($templates));
+
+        if ($sandboxed) {
+            $sandbox = new SandboxExtension(new StrictPolicy());
+            $environment->addExtension($sandbox);
+            $sandbox->enableSandbox();
+        }
+
+        return $environment;
+    }
+
+    private function createExtension(array $settings = []): Configuration
+    {
+        return new Configuration(SystemConfigurationFactory::createStub($settings));
+    }
+
+    private function getDefaultSettings(): array
+    {
+        return [
+            'theme' => [
+                'show_about' => true,
+                'avatar_url' => true,
+                'branding' => [
+                    'logo' => 'logo.png',
+                    'company' => 'Acme Inc.',
+                ],
+            ],
+            'user' => [
+                'login' => false,
+                'password_reset' => true,
+                'password_reset_token_ttl' => 3600,
+            ],
+            'ldap' => [
+                'activate' => false,
+            ],
+            'saml' => [
+                'activate' => false,
+                'title' => 'SAML Login',
+            ],
+        ];
+    }
+
+    public function testGetFunctions(): void
+    {
+        $sut = $this->createExtension($this->getDefaultSettings());
+
+        $functions = $sut->getFunctions();
+
+        self::assertCount(1, $functions);
+        self::assertInstanceOf(TwigFunction::class, $functions[0]);
+        self::assertSame('config', $functions[0]->getName());
+
+        $environment = $this->createEnvironment(['template' => '{{ config(\'theme.branding.company\') }}']);
+        $environment->addExtension($sut);
+
+        self::assertSame('Acme Inc.', $environment->render('template'));
+    }
+
+    public static function provideFixedConfigValues(): iterable
+    {
+        yield 'chart class' => ['chart-class', ''];
+        yield 'chart background' => ['theme.chart.background_color', '#3c8dbc'];
+        yield 'chart border' => ['theme.chart.border_color', '#3b8bba'];
+        yield 'chart grid' => ['theme.chart.grid_color', 'rgba(0,0,0,.05)'];
+        yield 'chart height' => ['theme.chart.height', '300'];
+        yield 'calendar background' => ['theme.calendar.background_color', Constants::DEFAULT_COLOR];
+    }
+
+    #[DataProvider('provideFixedConfigValues')]
+    public function testReturnsFixedConfigValues(string $name, string $expected): void
+    {
+        $sut = $this->createExtension();
+
+        self::assertSame($expected, $sut->get($this->createEnvironment(), $name));
+    }
+
+    public static function provideSandboxAllowedTemplates(): iterable
+    {
+        yield 'avatar urls' => ['{{ config(\'themeAllowAvatarUrls\') ? \'1\' : \'0\' }}', '1'];
+        yield 'branding logo' => ['{{ config(\'theme.branding.logo\') }}', 'logo.png'];
+        yield 'branding company' => ['{{ config(\'theme.branding.company\') }}', 'Acme Inc.'];
+    }
+
+    #[DataProvider('provideSandboxAllowedTemplates')]
+    public function testSandboxAllowsWhitelistedConfigAccess(string $template, string $expected): void
+    {
+        $environment = $this->createEnvironment(['template' => $template], true);
+        $environment->addExtension($this->createExtension($this->getDefaultSettings()));
+
+        self::assertSame($expected, $environment->render('template'));
+    }
+
+    public static function provideSensitiveConfigKeys(): iterable
+    {
+        yield 'saml active' => ['saml.activate'];
+        yield 'saml title' => ['saml.title'];
+        yield 'ldap active' => ['ldap.activate'];
+        yield 'ldap connection' => ['ldap.connection.host'];
+    }
+
+    #[DataProvider('provideSensitiveConfigKeys')]
+    public function testRejectsSensitiveConfigKeys(string $name): void
+    {
+        $sut = $this->createExtension($this->getDefaultSettings());
+
+        $this->expectException(SecurityError::class);
+        $this->expectExceptionMessage(\sprintf('Templates cannot access security configuration %s.', $name));
+
+        $sut->get($this->createEnvironment([], true), $name);
+    }
+
+    public function testSandboxRejectsNonWhitelistedConfigAccess(): void
+    {
+        $environment = $this->createEnvironment(['template' => '{{ config(\'showAbout\') ? \'1\' : \'0\' }}'], true);
+        $environment->addExtension($this->createExtension($this->getDefaultSettings()));
+
+        $this->expectException(SecurityError::class);
+        $this->expectExceptionMessage('Sandboxed template tried to access configuration key: showAbout');
+
+        $environment->render('template');
+    }
+
+    public function testUsesIsAccessorOutsideSandbox(): void
+    {
+        $sut = $this->createExtension($this->getDefaultSettings());
+
+        self::assertTrue($sut->get($this->createEnvironment(), 'showAbout'));
+        self::assertTrue($sut->get($this->createEnvironment(), 'loginFormActive'));
+    }
+
+    public function testUsesGetAccessorOutsideSandbox(): void
+    {
+        $sut = $this->createExtension($this->getDefaultSettings());
+
+        self::assertSame(3600, $sut->get($this->createEnvironment(), 'passwordResetTokenLifetime'));
+    }
+
+    public function testFallsBackToFindOutsideSandbox(): void
+    {
+        $sut = $this->createExtension($this->getDefaultSettings());
+        $environment = $this->createEnvironment();
+
+        self::assertTrue($sut->get($environment, 'theme.show_about'));
+        self::assertNull($sut->get($environment, 'does.not.exist'));
+    }
+
+    public function testDeprecatedKimaiConfigAccessAlwaysReturnsNull(): void
+    {
+        $sut = $this->createExtension($this->getDefaultSettings());
+
+        $previousHandler = set_error_handler(static function (int $type, string $message): true {
+            self::assertSame(E_USER_DEPRECATED, $type);
+            self::assertSame('Accessing "kimai_config" is deprecated and always return null, use config() instead', $message);
+
+            return true;
+        });
+
+        try {
+            self::assertNull($sut->__call('legacyAccess', []));
+        } finally {
+            restore_error_handler();
+        }
+    }
+}

tests/Twig/ContextTest.php

@@ -10,8 +10,6 @@
 namespace App\Tests\Twig;
 
 use App\Configuration\SystemConfiguration;
-use App\Tests\Configuration\TestConfigLoader;
-use App\Tests\Mocks\SystemConfigurationFactory;
 use App\Twig\Context;
 use PHPUnit\Framework\Attributes\CoversClass;
 use PHPUnit\Framework\TestCase;
@@ -24,9 +22,6 @@ class ContextTest extends TestCase
 {
     protected function getSut(array $settings, array $headers = []): Context
     {
-        $loader = new TestConfigLoader([]);
-        $config = SystemConfigurationFactory::create($loader, ['theme' => $settings]);
-
         $stack = new RequestStack();
         $request = new Request();
         foreach ($headers as $name => $value) {
@@ -34,13 +29,13 @@ class ContextTest extends TestCase
         }
         $stack->push($request);
 
-        return new Context($config, $stack);
+        return new Context($stack);
     }
 
     /**
-     * @return array
+     * @return array<string, mixed>
      */
-    protected function getDefaultSettings()
+    protected function getDefaultSettings(): array
     {
         return [
             'show_about' => true,

tests/phpstan.neon

@@ -1996,11 +1996,6 @@ parameters:
             count: 4
             path: TranslationsTest.php
 
-        -
-            message: "#^Method App\\\\Tests\\\\Twig\\\\ContextTest\\:\\:getDefaultSettings\\(\\) return type has no value type specified in iterable type array\\.$#"
-            count: 1
-            path: Twig/ContextTest.php
-
         -
             message: "#^Method App\\\\Tests\\\\Twig\\\\ContextTest\\:\\:getSut\\(\\) has parameter \\$headers with no value type specified in iterable type array\\.$#"
             count: 1