diff --git a/src/Cms/App.php b/src/Cms/App.php index d96f8020b3..64193ef864 100644 --- a/src/Cms/App.php +++ b/src/Cms/App.php @@ -1062,7 +1062,7 @@ protected function optionsFromConfig(): array // load the main config options $root = $this->root('config'); - $options = F::load($root . '/config.php', []); + $options = F::load($root . '/config.php', [], allowOutput: false); // merge into one clean options array return $this->options = array_replace_recursive(Config::$data, $options); @@ -1080,7 +1080,7 @@ protected function optionsFromEnvironment(array $props = []): array $root = $this->root('config'); // first load `config/env.php` to access its `url` option - $envOptions = F::load($root . '/env.php', []); + $envOptions = F::load($root . '/env.php', [], allowOutput: false); // use the option from the main `config.php`, // but allow the `env.php` to override it diff --git a/src/Cms/AppPlugins.php b/src/Cms/AppPlugins.php index 9240021008..fca4088a22 100644 --- a/src/Cms/AppPlugins.php +++ b/src/Cms/AppPlugins.php @@ -717,7 +717,7 @@ protected function extensionsFromFolders() $class = str_replace(['.', '-', '_'], '', $name) . 'Page'; // load the model class - F::loadOnce($model); + F::loadOnce($model, allowOutput: false); if (class_exists($class) === true) { $models[$name] = $class; @@ -908,7 +908,7 @@ protected function pluginsLoader(): array $styles = $dir . '/index.css'; if (is_file($entry) === true) { - F::loadOnce($entry); + F::loadOnce($entry, allowOutput: false); } elseif (is_file($script) === true || is_file($styles) === true) { // if no PHP file is present but an index.js or index.css, // register as anonymous plugin (without actual extensions) diff --git a/src/Cms/Blueprint.php b/src/Cms/Blueprint.php index b781ce54ad..cde33be9c7 100644 --- a/src/Cms/Blueprint.php +++ b/src/Cms/Blueprint.php @@ -731,7 +731,7 @@ protected function preset(array $props): array $preset = static::$presets[$props['preset']]; if (is_string($preset) === true) { - $preset = require $preset; + $preset = F::load($preset, allowOutput: false); } return $preset($props); diff --git a/src/Cms/Collections.php b/src/Cms/Collections.php index 3eb071ec50..8b06e1593b 100644 --- a/src/Cms/Collections.php +++ b/src/Cms/Collections.php @@ -123,7 +123,7 @@ public function load(string $name) $file = $kirby->root('collections') . '/' . $name . '.php'; if (is_file($file) === true) { - $collection = F::load($file); + $collection = F::load($file, allowOutput: false); if ($collection instanceof Closure) { return $collection; diff --git a/src/Cms/Languages.php b/src/Cms/Languages.php index cd8667f595..5c423f5a33 100644 --- a/src/Cms/Languages.php +++ b/src/Cms/Languages.php @@ -81,7 +81,7 @@ public static function load() $files = glob(App::instance()->root('languages') . '/*.php'); foreach ($files as $file) { - $props = F::load($file); + $props = F::load($file, allowOutput: false); if (is_array($props) === true) { // inject the language code from the filename diff --git a/src/Cms/Loader.php b/src/Cms/Loader.php index 3c69b96f31..d0a5488323 100644 --- a/src/Cms/Loader.php +++ b/src/Cms/Loader.php @@ -160,12 +160,12 @@ public function resolve($item) { if (is_string($item) === true) { $item = match (F::extension($item)) { - 'php' => require $item, + 'php' => F::load($item, allowOutput: false), default => Data::read($item) }; } - if (is_callable($item)) { + if (is_callable($item) === true) { $item = $item($this->kirby); } diff --git a/src/Cms/UserActions.php b/src/Cms/UserActions.php index 2e1196de7d..e130708479 100644 --- a/src/Cms/UserActions.php +++ b/src/Cms/UserActions.php @@ -308,7 +308,7 @@ protected function readCredentials(): array $path = $this->root() . '/index.php'; if (is_file($path) === true) { - $credentials = F::load($path); + $credentials = F::load($path, allowOutput: false); return is_array($credentials) === false ? [] : $credentials; } else { diff --git a/src/Cms/Users.php b/src/Cms/Users.php index dc2315e48b..b9e51fbd3e 100644 --- a/src/Cms/Users.php +++ b/src/Cms/Users.php @@ -148,7 +148,7 @@ public static function load(string $root, array $inject = []) // get role information $path = $root . '/' . $userDirectory . '/index.php'; if (is_file($path) === true) { - $credentials = F::load($path); + $credentials = F::load($path, allowOutput: false); } // create user model based on role diff --git a/src/Data/PHP.php b/src/Data/PHP.php index e51856e923..b22d38a1d3 100644 --- a/src/Data/PHP.php +++ b/src/Data/PHP.php @@ -61,7 +61,7 @@ public static function read(string $file): array throw new Exception('The file "' . $file . '" does not exist'); } - return (array)F::load($file, []); + return (array)F::load($file, [], allowOutput: false); } /** diff --git a/src/Filesystem/F.php b/src/Filesystem/F.php index 2ea3d99138..9015a7acc8 100644 --- a/src/Filesystem/F.php +++ b/src/Filesystem/F.php @@ -5,6 +5,7 @@ use Exception; use IntlDateFormatter; use Kirby\Cms\Helpers; +use Kirby\Http\Response; use Kirby\Toolkit\I18n; use Kirby\Toolkit\Str; use Throwable; @@ -347,8 +348,12 @@ public static function link(string $source, string $link, string $method = 'link * * @param array $data Optional array of variables to extract in the variable scope */ - public static function load(string $file, $fallback = null, array $data = []) - { + public static function load( + string $file, + mixed $fallback = null, + array $data = [], + bool $allowOutput = true + ) { if (is_file($file) === false) { return $fallback; } @@ -356,9 +361,21 @@ public static function load(string $file, $fallback = null, array $data = []) // we use the loadIsolated() method here to prevent the included // file from overwriting our $fallback in this variable scope; see // https://www.php.net/manual/en/function.include.php#example-124 - $result = static::loadIsolated($file, $data); + $callback = fn () => static::loadIsolated($file, $data); + + // if the loaded file should not produce any output, + // call the loaidIsolated method from the Response class + // which checks for unintended ouput and throws an error if detected + if ($allowOutput === false) { + $result = Response::guardAgainstOutput($callback); + } else { + $result = $callback(); + } - if ($fallback !== null && gettype($result) !== gettype($fallback)) { + if ( + $fallback !== null && + gettype($result) !== gettype($fallback) + ) { return $fallback; } @@ -369,24 +386,28 @@ public static function load(string $file, $fallback = null, array $data = []) * A super simple class autoloader * @since 3.7.0 */ - public static function loadClasses(array $classmap, string|null $base = null): void - { + public static function loadClasses( + array $classmap, + string|null $base = null + ): void { // convert all classnames to lowercase $classmap = array_change_key_case($classmap); - spl_autoload_register(function ($class) use ($classmap, $base) { - $class = strtolower($class); + spl_autoload_register( + fn ($class) => Response::guardAgainstOutput(function () use ($class, $classmap, $base) { + $class = strtolower($class); - if (!isset($classmap[$class])) { - return false; - } + if (isset($classmap[$class]) === false) { + return false; + } - if ($base) { - include $base . '/' . $classmap[$class]; - } else { - include $classmap[$class]; - } - }); + if ($base) { + include $base . '/' . $classmap[$class]; + } else { + include $classmap[$class]; + } + }) + ); } /** @@ -408,13 +429,22 @@ protected static function loadIsolated(string $file, array $data = []) * Loads a file using `include_once()` and * returns whether loading was successful */ - public static function loadOnce(string $file): bool - { + public static function loadOnce( + string $file, + bool $allowOutput = true + ): bool { if (is_file($file) === false) { return false; } - include_once $file; + $callback = fn () => include_once $file; + + if ($allowOutput === false) { + Response::guardAgainstOutput($callback); + } else { + $callback(); + } + return true; } diff --git a/src/Http/Environment.php b/src/Http/Environment.php index 986ba94ec5..9a3e3c5cc9 100644 --- a/src/Http/Environment.php +++ b/src/Http/Environment.php @@ -786,12 +786,20 @@ public function options(string $root): array // load the config for the host if (empty($host) === false) { - $configHost = F::load($root . '/config.' . $host . '.php', []); + $configHost = F::load( + file: $root . '/config.' . $host . '.php', + fallback: [], + allowOutput: false + ); } // load the config for the server IP if (empty($addr) === false) { - $configAddr = F::load($root . '/config.' . $addr . '.php', []); + $configAddr = F::load( + file: $root . '/config.' . $addr . '.php', + fallback: [], + allowOutput: false + ); } return array_replace_recursive($configHost, $configAddr); diff --git a/src/Http/Response.php b/src/Http/Response.php index 3696b963f9..15bca1f0c4 100644 --- a/src/Http/Response.php +++ b/src/Http/Response.php @@ -2,7 +2,9 @@ namespace Kirby\Http; +use Closure; use Exception; +use Kirby\Exception\LogicException; use Kirby\Filesystem\F; use Throwable; @@ -182,6 +184,23 @@ public static function go(string $url = '/', int $code = 302): void die(static::redirect($url, $code)); } + /** + * Ensures that the callback does not produce the first body output + * (used to show when loading a file creates side effects) + */ + public static function guardAgainstOutput(Closure $callback, ...$args): mixed + { + $before = headers_sent(); + $result = $callback(...$args); + $after = headers_sent($file, $line); + + if ($before === false && $after === true) { + throw new LogicException("Disallowed output from file $file:$line, possible accidental whitespace?"); + } + + return $result; + } + /** * Getter for single headers * diff --git a/src/Toolkit/Component.php b/src/Toolkit/Component.php index d8e55c31a3..d9de86f265 100644 --- a/src/Toolkit/Component.php +++ b/src/Toolkit/Component.php @@ -232,7 +232,7 @@ public static function load(string $type): array throw new Exception('Component definition ' . $definition . ' does not exist'); } - static::$types[$type] = $definition = F::load($definition); + static::$types[$type] = $definition = F::load($definition, allowOutput: false); } return $definition; @@ -254,7 +254,11 @@ public static function setup(string $type): array if (isset($definition['extends']) === true) { // extend other definitions - $options = array_replace_recursive(static::defaults(), static::load($definition['extends']), $definition); + $options = array_replace_recursive( + static::defaults(), + static::load($definition['extends']), + $definition + ); } else { // inject defaults $options = array_replace_recursive(static::defaults(), $definition); @@ -266,10 +270,14 @@ public static function setup(string $type): array if (isset(static::$mixins[$mixin]) === true) { if (is_string(static::$mixins[$mixin]) === true) { // resolve a path to a mixin on demand - static::$mixins[$mixin] = include static::$mixins[$mixin]; + + static::$mixins[$mixin] = F::load(static::$mixins[$mixin], allowOutput: false); } - $options = array_replace_recursive(static::$mixins[$mixin], $options); + $options = array_replace_recursive( + static::$mixins[$mixin], + $options + ); } } } diff --git a/tests/Blueprint/NodeI18nTest.php b/tests/Blueprint/NodeI18nTest.php index 7fd3841aa3..9646d32dd7 100644 --- a/tests/Blueprint/NodeI18nTest.php +++ b/tests/Blueprint/NodeI18nTest.php @@ -3,7 +3,7 @@ namespace Kirby\Blueprint; /** - * @covers \Kirby\Blueprint\NodeI18n + * @coversDefaultClass \Kirby\Blueprint\NodeI18n */ class NodeI18nTest extends TestCase { diff --git a/tests/Filesystem/FTest.php b/tests/Filesystem/FTest.php index 01f661bd3d..0ad10fb20b 100644 --- a/tests/Filesystem/FTest.php +++ b/tests/Filesystem/FTest.php @@ -2,10 +2,14 @@ namespace Kirby\Filesystem; +use Kirby\Exception\LogicException; +use Kirby\Http\HeadersSent; use Kirby\Toolkit\I18n; use Kirby\Toolkit\Str; use PHPUnit\Framework\TestCase as TestCase; +require_once dirname(__DIR__) . '/Http/mocks.php'; + /** * @coversDefaultClass \Kirby\Filesystem\F */ @@ -35,6 +39,8 @@ public function tearDown(): void restore_error_handler(); $this->hasErrorHandler = false; } + + HeadersSent::$value = false; } /** @@ -369,6 +375,27 @@ public function testLoad() // with overwritten $file $this->assertSame('foobar', F::load($file, null, ['variable' => 'foobar', 'file' => null])); + + // protection against accidental output (without output) + F::write($file = $this->tmp . '/test.php', 'assertSame('foo', F::load($file, allowOutput: false)); + + // no protection against accidental output (with output) + F::write($file = $this->tmp . '/test.php', 'assertSame('foo', F::load($file)); + } + + /** + * @covers ::load + */ + public function testLoadAccidentalOutput() + { + $this->expectException(LogicException::class); + $this->expectExceptionMessage('Disallowed output from file file.php:123, possible accidental whitespace?'); + + F::write($file = $this->tmp . '/test.php', ' __DIR__ . '/fixtures/f/load/a/a.php', + 'ftest\\output' => __DIR__ . '/fixtures/f/load/output.php' ]); F::loadClasses([ @@ -387,6 +415,10 @@ public function testLoadClasses() $this->assertTrue(class_exists('FTest\\A')); $this->assertTrue(class_exists('FTest\\B')); $this->assertFalse(class_exists('FTest\\C')); + + $this->expectException(LogicException::class); + $this->expectExceptionMessage('Disallowed output from file file.php:123, possible accidental whitespace?'); + class_exists('FTest\\Output'); } /** @@ -395,11 +427,32 @@ public function testLoadClasses() public function testLoadOnce() { // basic behavior - F::write($file = $this->tmp . '/test.php', 'tmp . '/test1.php', 'assertTrue(F::loadOnce($file)); // non-existing file $this->assertFalse(F::loadOnce('does-not-exist.php')); + + // protection against accidental output (without output) + F::write($file = $this->tmp . '/test2.php', 'assertTrue(F::loadOnce($file, allowOutput: false)); + + // no protection against accidental output (with output) + F::write($file = $this->tmp . '/test3.php', 'assertTrue(F::loadOnce($file)); + } + + /** + * @covers ::loadOnce + */ + public function testLoadOnceAccidentalOutput() + { + $this->expectException(LogicException::class); + $this->expectExceptionMessage('Disallowed output from file file.php:123, possible accidental whitespace?'); + + F::write($file = $this->tmp . '/test4.php', 'assertSame('12-34', $result); + } + + /** + * @covers ::guardAgainstOutput + */ + public function testGuardAgainstOutputWithSubsequentOutput() + { + HeadersSent::$value = true; + + $result = Response::guardAgainstOutput(function ($arg1, $arg2) { + return $arg1 . '-' . $arg2; + }, '12', '34'); + + $this->assertSame('12-34', $result); + } + + /** + * @covers ::guardAgainstOutput + */ + public function testGuardAgainstOutputWithFirstOutput() + { + $this->expectException(LogicException::class); + $this->expectExceptionMessage('Disallowed output from file file.php:123, possible accidental whitespace?'); + + Response::guardAgainstOutput(function () { + HeadersSent::$value = true; + }); + } + public function testHeaders() { $response = new Response(); diff --git a/tests/Http/mocks.php b/tests/Http/mocks.php index 1c6222fc04..693a91985a 100644 --- a/tests/Http/mocks.php +++ b/tests/Http/mocks.php @@ -2,11 +2,34 @@ namespace Kirby\Http; +class HeadersSent +{ + public static $value = false; +} + class IniStore { public static $data = []; } +/** + * Mock for the PHP headers_sent() function (otherwise not available on CLI) + */ +function headers_sent(string &$file = null, int &$line = null): bool +{ + if (defined('KIRBY_TESTING') !== true || KIRBY_TESTING !== true) { + throw new Exception('Mock headers_sent() was loaded outside of the test environment. This should never happen.'); + } + + if (HeadersSent::$value === true) { + $file = 'file.php'; + $line = 123; + return true; + } + + return false; +} + /** * Mock for the PHP ini_get() function to ensure reliable testing *