fix(security): harden redirects, error views and debug output

src/Phaseolies/Error/Handlers/WebErrorHandler.php

@@ -25,7 +25,7 @@ public function handle(Throwable $exception): void
 
         $renderer = new WebErrorRenderer();
 
-        if (env('APP_DEBUG', false)) {
+        if (app_debug()) {
             $response = $renderer->renderDebug($exception);
         } else {
             $response = $renderer->renderProduction($exception);

src/Phaseolies/Error/JsonErrorRenderer.php

@@ -2,6 +2,8 @@
 
 namespace Phaseolies\Error;
 
+use Phaseolies\Http\Exceptions\HttpException;
+use Phaseolies\Http\Exceptions\TokenMismatchException;
 use Phaseolies\Http\Response;
 use Phaseolies\Http\Response\JsonResponse;
 use Throwable;
@@ -25,22 +27,45 @@ public function render(Throwable $exception, int $statusCode, mixed $errorDetail
             Response::HTTP_UNAUTHORIZED         => trans('validation.unauthorized.message'),
         ];
 
-        $message = $messages[$statusCode] ?? $exception->getMessage();
+        // Some callers pass the exception's own message, which is not extra detail.
+        $errorDetails = $errorDetails === $exception->getMessage() ? null : $errorDetails;
 
         $response = isset($messages[$statusCode])
             ? [
-                'message' => $message,
-                'errors'  => $errorDetails ?? $exception->getMessage(),
+                'message' => $messages[$statusCode],
+                'errors'  => $errorDetails ?? $this->publicMessage($exception),
             ]
             : [
-                'message' => $errorDetails ?? $exception->getMessage(),
-                'errors'  => [
-                    'file'  => $exception->getFile(),
-                    'line'  => $exception->getLine(),
-                    'trace' => $exception->getTraceAsString(),
-                ],
+                'message' => $errorDetails ?? $this->publicMessage($exception),
+                'errors'  => app_debug()
+                    ? [
+                        'file'  => $exception->getFile(),
+                        'line'  => $exception->getLine(),
+                        'trace' => $exception->getTraceAsString(),
+                    ]
+                    : [],
             ];
 
         return response()->json($response, $statusCode);
     }
+
+    /**
+     * The exception message, when it is safe to show to the client.
+     *
+     * The status of a non-HTTP exception comes from its code, so the decision
+     * is made by exception type and not by status number.
+     *
+     * @param Throwable $exception
+     * @return string
+     */
+    protected function publicMessage(Throwable $exception): string
+    {
+        $isClientError = $exception instanceof TokenMismatchException || (
+            $exception instanceof HttpException &&
+            $exception->getStatusCode() >= 400 &&
+            $exception->getStatusCode() < 500
+        );
+
+        return app_debug() || $isClientError ? $exception->getMessage() : 'Something went wrong';
+    }
 }

src/Phaseolies/Helpers/helpers.php

@@ -36,6 +36,19 @@ function env(string $key, string|float|int|bool|null $default = null): string|fl
     }
 }
 
+if (!function_exists('app_debug')) {
+    /**
+     * Whether debugging is on. A server environment variable arrives as a
+     * string, and "false" must not count as true.
+     *
+     * @return bool
+     */
+    function app_debug(): bool
+    {
+        return filter_var(env('APP_DEBUG', false), FILTER_VALIDATE_BOOLEAN);
+    }
+}
+
 if (!function_exists('dopparEnv')) {
     /**
      * Retrieves an environment variable or returns a default value if the variable is not set.

src/Phaseolies/Launchers/RouteLauncher.php

@@ -13,16 +13,42 @@ class RouteLauncher extends ServiceLauncher
      */
     public function register()
     {
-        $path = urldecode(
-            parse_url(request()->server->get("REQUEST_URI", "/"), PHP_URL_PATH)
-        );
+        $target = $this->trailingSlashRedirect(request()->server->get('REQUEST_URI', '/'));
 
-        if ($path !== '/' && str_ends_with(request()->server->get('REQUEST_URI'), '/')) {
-            header('Location: ' . rtrim(request()->server->get('REQUEST_URI'), '/'), true, 301);
+        if ($target !== null) {
+            header('Location: ' . $target, true, 301);
             exit;
         }
     }
 
+    /**
+     * Where a request URI with a trailing slash on its path should redirect, if anywhere.
+     *
+     * The URI comes from the client. Browsers treat a Location that starts with
+     * "//" or "/\" as another host, so it must start with exactly one slash.
+     * They also drop tabs and newlines, so "/<tab>/host" would become "//host":
+     * a URI with control characters is never redirected.
+     *
+     * @param string $uri
+     * @return string|null
+     */
+    protected function trailingSlashRedirect(string $uri): ?string
+    {
+        if (preg_match('/[\x00-\x1F\x7F]/', $uri) === 1) {
+            return null;
+        }
+
+        [$path, $query] = array_pad(explode('?', $uri, 2), 2, null);
+
+        $target = '/' . ltrim(rtrim($path, '/'), '/\\');
+
+        if (!str_ends_with($path, '/') || $target === '/') {
+            return null;
+        }
+
+        return $query === null ? $target : $target . '?' . $query;
+    }
+
     /**
      * Bootstrap any application services.
      *

src/Phaseolies/Support/View/errors/401.odo.php

@@ -15,7 +15,7 @@
                 </div>
                 <div
                     style="margin-left:1rem; font-size:1.125rem; color:#718096; text-transform:uppercase; letter-spacing:0.05em;">
-                    <?php echo $message === 'An error occurred.' ? 'Unauthorized': $message; ?>
+                    <?php echo htmlspecialchars($message === 'An error occurred.' ? 'Unauthorized' : $message, ENT_QUOTES, 'UTF-8'); ?>
                 </div>
             </div>
         </div>

src/Phaseolies/Support/View/errors/403.odo.php

@@ -15,7 +15,7 @@
                 </div>
                 <div
                     style="margin-left:1rem; font-size:1.125rem; color:#718096; text-transform:uppercase; letter-spacing:0.05em;">
-                    <?php echo $message === 'An error occurred.' ? 'Access to the resource is forbidden': $message; ?>
+                    <?php echo htmlspecialchars($message === 'An error occurred.' ? 'Access to the resource is forbidden' : $message, ENT_QUOTES, 'UTF-8'); ?>
                 </div>
             </div>
         </div>

src/Phaseolies/Support/View/errors/404.odo.php

@@ -15,7 +15,7 @@
                 </div>
                 <div
                     style="margin-left:1rem; font-size:1.125rem; color:#718096; text-transform:uppercase; letter-spacing:0.05em;">
-                    <?php echo $message === 'An error occurred.' ? 'Not Found': $message; ?>
+                    <?php echo htmlspecialchars($message === 'An error occurred.' ? 'Not Found' : $message, ENT_QUOTES, 'UTF-8'); ?>
                 </div>
             </div>
         </div>

src/Phaseolies/Support/View/errors/429.odo.php

@@ -15,7 +15,7 @@
                 </div>
                 <div
                     style="margin-left:1rem; font-size:1.125rem; color:#718096; text-transform:uppercase; letter-spacing:0.05em;">
-                    <?php echo $message ?>
+                    <?php echo htmlspecialchars($message, ENT_QUOTES, 'UTF-8'); ?>
                 </div>
             </div>
         </div>

src/Phaseolies/Support/View/errors/500.odo.php

@@ -15,7 +15,7 @@
                 </div>
                 <div
                     style="margin-left:1rem; font-size:1.125rem; color:#718096; text-transform:uppercase; letter-spacing:0.05em;">
-                    <?php echo $message === 'An error occurred.' ? 'Server Error' : $message; ?>
+                    <?php echo htmlspecialchars($message === 'An error occurred.' ? 'Server Error' : $message, ENT_QUOTES, 'UTF-8'); ?>
                 </div>
             </div>
         </div>

tests/Error/ErrorViewEscapingTest.php

@@ -0,0 +1,27 @@
+<?php
+
+namespace Tests\Unit\Error;
+
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+
+class ErrorViewEscapingTest extends TestCase
+{
+    public static function statusProvider(): array
+    {
+        return [[400], [401], [403], [404], [429], [500]];
+    }
+
+    #[DataProvider('statusProvider')]
+    public function testBundledErrorViewEscapesTheMessage(int $status): void
+    {
+        $message = 'Route [/<script>alert(1)</script>] not found';
+
+        ob_start();
+        include dirname(__DIR__, 2) . "/src/Phaseolies/Support/View/errors/{$status}.odo.php";
+        $html = ob_get_clean();
+
+        $this->assertStringNotContainsString('<script>alert(1)</script>', $html);
+        $this->assertStringContainsString('&lt;script&gt;alert(1)&lt;/script&gt;', $html);
+    }
+}

tests/Error/JsonErrorRendererTest.php

@@ -6,6 +6,7 @@
 
 use Phaseolies\DI\Container;
 use Phaseolies\Error\JsonErrorRenderer;
+use Phaseolies\Http\Exceptions\HttpException;
 use Phaseolies\Http\Response;
 use Phaseolies\Http\Response\JsonResponse;
 use Phaseolies\Translation\FileLoader;
@@ -41,6 +42,7 @@ protected function setUp(): void
     protected function tearDown(): void
     {
         $this->deleteDir($this->translationPath);
+        unset($_ENV['APP_DEBUG']);
 
         parent::tearDown();
     }
@@ -63,8 +65,79 @@ protected function deleteDir(string $dir): void
         @rmdir($dir);
     }
 
+    public function testRenderHidesExceptionInternalsWhenNotDebugging(): void
+    {
+        foreach (['false', '0', ''] as $debug) {
+            $_ENV['APP_DEBUG'] = $debug;
+
+            $payload = json_decode((new JsonErrorRenderer())->render(
+                new RuntimeException('SQLSTATE secret', Response::HTTP_INTERNAL_SERVER_ERROR),
+                Response::HTTP_INTERNAL_SERVER_ERROR
+            )->getBody(), true);
+
+            $this->assertSame(['message' => 'Something went wrong', 'errors' => []], $payload);
+        }
+    }
+
+    public function testRenderHidesPlainExceptionMessageWhateverItsCode(): void
+    {
+        $_ENV['APP_DEBUG'] = 'false';
+
+        foreach ([2, 404, Response::HTTP_UNPROCESSABLE_ENTITY] as $code) {
+            $body = (new JsonErrorRenderer())->render(
+                new RuntimeException('Cannot connect to 10.0.3.7 as root', $code),
+                $code
+            )->getBody();
+
+            $this->assertStringNotContainsString('10.0.3.7', $body);
+            $this->assertStringContainsString('Something went wrong', $body);
+        }
+    }
+
+    public function testRenderHidesServerHttpExceptionMessage(): void
+    {
+        $_ENV['APP_DEBUG'] = 'false';
+
+        $exception = new HttpException(Response::HTTP_INTERNAL_SERVER_ERROR, 'SQLSTATE secret');
+
+        $body = (new JsonErrorRenderer())->render(
+            $exception,
+            Response::HTTP_INTERNAL_SERVER_ERROR,
+            $exception->getMessage()
+        )->getBody();
+
+        $this->assertSame(['message' => 'Something went wrong', 'errors' => []], json_decode($body, true));
+    }
+
+    public function testRenderShowsClientHttpExceptionMessage(): void
+    {
+        $_ENV['APP_DEBUG'] = 'false';
+
+        $payload = json_decode((new JsonErrorRenderer())->render(
+            new HttpException(Response::HTTP_BAD_REQUEST, 'Invalid JSON data in request body.'),
+            Response::HTTP_BAD_REQUEST
+        )->getBody(), true);
+
+        $this->assertSame(['message' => 'Invalid JSON data in request body.', 'errors' => []], $payload);
+    }
+
+    public function testRenderKeepsClientErrorMessageWhenNotDebugging(): void
+    {
+        $_ENV['APP_DEBUG'] = 'false';
+
+        $payload = json_decode((new JsonErrorRenderer())->render(
+            new RuntimeException('Boom'),
+            Response::HTTP_BAD_REQUEST,
+            'Invalid JSON data in request body.'
+        )->getBody(), true);
+
+        $this->assertSame(['message' => 'Invalid JSON data in request body.', 'errors' => []], $payload);
+    }
+
     public function testRenderReturnsJsonResponseInstance(): void
     {
+        $_ENV['APP_DEBUG'] = 'true';
+
         $renderer = new JsonErrorRenderer();
 
         $response = $renderer->render(

tests/Helpers/AppDebugTest.php

@@ -0,0 +1,37 @@
+<?php
+
+namespace Tests\Unit\Helpers;
+
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+
+class AppDebugTest extends TestCase
+{
+    protected function tearDown(): void
+    {
+        unset($_ENV['APP_DEBUG']);
+
+        parent::tearDown();
+    }
+
+    public static function debugValueProvider(): array
+    {
+        return [
+            'boolean false' => [false, false],
+            'string false' => ['false', false],
+            'string zero' => ['0', false],
+            'empty string' => ['', false],
+            'boolean true' => [true, true],
+            'string true' => ['true', true],
+            'string one' => ['1', true],
+        ];
+    }
+
+    #[DataProvider('debugValueProvider')]
+    public function testDebugModeIsReadFromAppDebug(bool|string $value, bool $expected): void
+    {
+        $_ENV['APP_DEBUG'] = $value;
+
+        $this->assertSame($expected, app_debug());
+    }
+}

tests/Launchers/RouteLauncherTest.php

@@ -0,0 +1,51 @@
+<?php
+
+namespace Tests\Unit\Launchers;
+
+use Phaseolies\Launchers\RouteLauncher;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+use ReflectionClass;
+use ReflectionMethod;
+
+class RouteLauncherTest extends TestCase
+{
+    public static function uriProvider(): array
+    {
+        return [
+            'plain path' => ['/users/', '/users'],
+            'nested path' => ['/users/7/posts/', '/users/7/posts'],
+            'several trailing slashes' => ['/users///', '/users'],
+            'query string is kept' => ['/users/?next=/', '/users?next=/'],
+            'encoded query is not decoded' => ['/users/?next=%2Fhome', '/users?next=%2Fhome'],
+            'encoded path is not decoded' => ['/caf%C3%A9/', '/caf%C3%A9'],
+            'protocol-relative host' => ['//evil.com/foo/', '/evil.com/foo'],
+            'three leading slashes' => ['///evil.com/foo/', '/evil.com/foo'],
+            'leading backslash' => ['/\\evil.com/foo/', '/evil.com/foo'],
+            'mixed leading slashes' => ['/\\/\\evil.com/foo/', '/evil.com/foo'],
+            'absolute URI' => ['http://evil.com/foo/', '/http://evil.com/foo'],
+            'no trailing slash' => ['/users', null],
+            'slash only in query' => ['/users?next=/', null],
+            'encoded slash at the end' => ['/users%2F', null],
+            'root' => ['/', null],
+            'root with query' => ['/?next=/', null],
+            'only slashes' => ['//', null],
+            'tab before a host' => ["/\t/evil.com/", null],
+            'newline before a host' => ["/\n/evil.com/", null],
+            'carriage return before a host' => ["/\r/evil.com/", null],
+            'tab between leading slashes' => ["/\t\t//evil.com/foo/", null],
+            'control character in the query' => ["/users/?next=\t/", null],
+            'delete character' => ["/\x7F/evil.com/", null],
+            'encoded tab is not a control character' => ['/%09/evil.com/', '/%09/evil.com'],
+        ];
+    }
+
+    #[DataProvider('uriProvider')]
+    public function testRedirectTargetStaysOnThisSite(string $uri, ?string $expected): void
+    {
+        $launcher = (new ReflectionClass(RouteLauncher::class))->newInstanceWithoutConstructor();
+        $method = new ReflectionMethod(RouteLauncher::class, 'trailingSlashRedirect');
+
+        $this->assertSame($expected, $method->invoke($launcher, $uri));
+    }
+}