diff --git a/src/Phaseolies/Error/Handlers/WebErrorHandler.php b/src/Phaseolies/Error/Handlers/WebErrorHandler.php index ab9841e9..0a7cced1 100644 --- a/src/Phaseolies/Error/Handlers/WebErrorHandler.php +++ b/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); diff --git a/src/Phaseolies/Error/JsonErrorRenderer.php b/src/Phaseolies/Error/JsonErrorRenderer.php index 8ce2f100..ce2b67e3 100644 --- a/src/Phaseolies/Error/JsonErrorRenderer.php +++ b/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'; + } } diff --git a/src/Phaseolies/Helpers/helpers.php b/src/Phaseolies/Helpers/helpers.php index 50234bcb..14ef9507 100644 --- a/src/Phaseolies/Helpers/helpers.php +++ b/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. diff --git a/src/Phaseolies/Launchers/RouteLauncher.php b/src/Phaseolies/Launchers/RouteLauncher.php index d054ddc5..e9bf9f66 100644 --- a/src/Phaseolies/Launchers/RouteLauncher.php +++ b/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 "//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. * diff --git a/src/Phaseolies/Support/View/errors/401.odo.php b/src/Phaseolies/Support/View/errors/401.odo.php index 3d13ab6b..c447bfe1 100644 --- a/src/Phaseolies/Support/View/errors/401.odo.php +++ b/src/Phaseolies/Support/View/errors/401.odo.php @@ -15,7 +15,7 @@
- +
diff --git a/src/Phaseolies/Support/View/errors/403.odo.php b/src/Phaseolies/Support/View/errors/403.odo.php index 45664168..ecc41953 100644 --- a/src/Phaseolies/Support/View/errors/403.odo.php +++ b/src/Phaseolies/Support/View/errors/403.odo.php @@ -15,7 +15,7 @@
- +
diff --git a/src/Phaseolies/Support/View/errors/404.odo.php b/src/Phaseolies/Support/View/errors/404.odo.php index f288c860..7ca272c5 100644 --- a/src/Phaseolies/Support/View/errors/404.odo.php +++ b/src/Phaseolies/Support/View/errors/404.odo.php @@ -15,7 +15,7 @@
- +
diff --git a/src/Phaseolies/Support/View/errors/429.odo.php b/src/Phaseolies/Support/View/errors/429.odo.php index ac9e6e3e..b1205300 100644 --- a/src/Phaseolies/Support/View/errors/429.odo.php +++ b/src/Phaseolies/Support/View/errors/429.odo.php @@ -15,7 +15,7 @@
- +
diff --git a/src/Phaseolies/Support/View/errors/500.odo.php b/src/Phaseolies/Support/View/errors/500.odo.php index 3b40ffcc..01746423 100644 --- a/src/Phaseolies/Support/View/errors/500.odo.php +++ b/src/Phaseolies/Support/View/errors/500.odo.php @@ -15,7 +15,7 @@
- +
diff --git a/tests/Error/ErrorViewEscapingTest.php b/tests/Error/ErrorViewEscapingTest.php new file mode 100644 index 00000000..5108e353 --- /dev/null +++ b/tests/Error/ErrorViewEscapingTest.php @@ -0,0 +1,27 @@ +alert(1)] not found'; + + ob_start(); + include dirname(__DIR__, 2) . "/src/Phaseolies/Support/View/errors/{$status}.odo.php"; + $html = ob_get_clean(); + + $this->assertStringNotContainsString('', $html); + $this->assertStringContainsString('<script>alert(1)</script>', $html); + } +} diff --git a/tests/Error/JsonErrorRendererTest.php b/tests/Error/JsonErrorRendererTest.php index 71df0721..6a34c74a 100644 --- a/tests/Error/JsonErrorRendererTest.php +++ b/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( diff --git a/tests/Helpers/AppDebugTest.php b/tests/Helpers/AppDebugTest.php new file mode 100644 index 00000000..ffee88c9 --- /dev/null +++ b/tests/Helpers/AppDebugTest.php @@ -0,0 +1,37 @@ + [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()); + } +} diff --git a/tests/Launchers/RouteLauncherTest.php b/tests/Launchers/RouteLauncherTest.php new file mode 100644 index 00000000..18d886d2 --- /dev/null +++ b/tests/Launchers/RouteLauncherTest.php @@ -0,0 +1,51 @@ + ['/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)); + } +}