From 0255b5a9082fd6a581a9c9ab6ecfc28a0683b47c Mon Sep 17 00:00:00 2001 From: Abdul Majid <43958759+abdulmajidcse@users.noreply.github.com> Date: Thu, 5 Nov 2026 18:54:51 +0600 Subject: [PATCH 1/3] fix(security): harden redirects, 2FA, error views and debug output - Keep the trailing-slash redirect on the current site - Require the 2FA code when signing in with a remember-me cookie - Escape the message in the bundled error views - Hide exception internals in JSON errors unless debugging - Treat APP_DEBUG="false" as off in the web error handler --- src/Phaseolies/Auth/Security/Authenticate.php | 6 +- .../Error/Handlers/WebErrorHandler.php | 13 +- src/Phaseolies/Error/JsonErrorRenderer.php | 24 +++- src/Phaseolies/Launchers/RouteLauncher.php | 16 ++- .../Support/View/errors/401.odo.php | 2 +- .../Support/View/errors/403.odo.php | 2 +- .../Support/View/errors/404.odo.php | 2 +- .../Support/View/errors/429.odo.php | 2 +- .../Support/View/errors/500.odo.php | 2 +- tests/Auth/AuthenticateTest.php | 111 +++++++++++++++++- tests/Error/ErrorViewEscapingTest.php | 27 +++++ tests/Error/JsonErrorRendererTest.php | 30 +++++ tests/Error/WebErrorHandlerTest.php | 41 +++++++ tests/Launchers/RouteLauncherTest.php | 37 ++++++ 14 files changed, 303 insertions(+), 12 deletions(-) create mode 100644 tests/Error/ErrorViewEscapingTest.php create mode 100644 tests/Error/WebErrorHandlerTest.php create mode 100644 tests/Launchers/RouteLauncherTest.php diff --git a/src/Phaseolies/Auth/Security/Authenticate.php b/src/Phaseolies/Auth/Security/Authenticate.php index 4e413fd6..b2881898 100644 --- a/src/Phaseolies/Auth/Security/Authenticate.php +++ b/src/Phaseolies/Auth/Security/Authenticate.php @@ -314,9 +314,13 @@ public function user(): ?Authable } if (Hash::check($token, $user->getRememberToken())) { - if ($this->hasTwoFactorEnabled($user)) { + // The cookie replaces the password, not the second factor: like + // login(), leave the user signed out until the code is verified. + if ($this->hasTwoFactorEnabled($user) && !$this->isApiRequest()) { session()->put($this->getTwoFactorUserKey(), $user->getAuthIdentifier()); session()->put($this->getTwoFactorRememberKey(), true); + + return null; } $this->setUser($user); diff --git a/src/Phaseolies/Error/Handlers/WebErrorHandler.php b/src/Phaseolies/Error/Handlers/WebErrorHandler.php index ab9841e9..70fd4c66 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 ($this->isDebugging()) { $response = $renderer->renderDebug($exception); } else { $response = $renderer->renderProduction($exception); @@ -34,6 +34,17 @@ public function handle(Throwable $exception): void $response->prepare(request())->send(); } + /** + * Whether debugging is on. A server environment variable arrives as a + * string, and "false" must not count as true. + * + * @return bool + */ + protected function isDebugging(): bool + { + return filter_var(env('APP_DEBUG', false), FILTER_VALIDATE_BOOLEAN); + } + /** * Checks if this handler should handle the current request. * diff --git a/src/Phaseolies/Error/JsonErrorRenderer.php b/src/Phaseolies/Error/JsonErrorRenderer.php index 8ce2f100..79ee96a5 100644 --- a/src/Phaseolies/Error/JsonErrorRenderer.php +++ b/src/Phaseolies/Error/JsonErrorRenderer.php @@ -32,7 +32,23 @@ public function render(Throwable $exception, int $statusCode, mixed $errorDetail 'message' => $message, 'errors' => $errorDetails ?? $exception->getMessage(), ] - : [ + : $this->exceptionPayload($exception, $statusCode, $errorDetails); + + return response()->json($response, $statusCode); + } + + /** + * Describe an exception, leaving out its internals unless debugging. + * + * @param Throwable $exception + * @param int $statusCode + * @param mixed|null $errorDetails + * @return array + */ + protected function exceptionPayload(Throwable $exception, int $statusCode, mixed $errorDetails): array + { + if (filter_var(env('APP_DEBUG', false), FILTER_VALIDATE_BOOLEAN)) { + return [ 'message' => $errorDetails ?? $exception->getMessage(), 'errors' => [ 'file' => $exception->getFile(), @@ -40,7 +56,11 @@ public function render(Throwable $exception, int $statusCode, mixed $errorDetail 'trace' => $exception->getTraceAsString(), ], ]; + } - return response()->json($response, $statusCode); + return [ + 'message' => $errorDetails ?? ($statusCode >= 500 ? 'Something went wrong' : $exception->getMessage()), + 'errors' => [], + ]; } } diff --git a/src/Phaseolies/Launchers/RouteLauncher.php b/src/Phaseolies/Launchers/RouteLauncher.php index d054ddc5..64c12314 100644 --- a/src/Phaseolies/Launchers/RouteLauncher.php +++ b/src/Phaseolies/Launchers/RouteLauncher.php @@ -18,11 +18,25 @@ public function register() ); if ($path !== '/' && str_ends_with(request()->server->get('REQUEST_URI'), '/')) { - header('Location: ' . rtrim(request()->server->get('REQUEST_URI'), '/'), true, 301); + header('Location: ' . $this->withoutTrailingSlash(request()->server->get('REQUEST_URI')), true, 301); exit; } } + /** + * Remove the trailing slash from a request URI, keeping the result on this site. + * + * 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. + * + * @param string $uri + * @return string + */ + protected function withoutTrailingSlash(string $uri): string + { + return '/' . ltrim(rtrim($uri, '/'), '/\\'); + } + /** * 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/Auth/AuthenticateTest.php b/tests/Auth/AuthenticateTest.php index 828594cd..b5b90753 100644 --- a/tests/Auth/AuthenticateTest.php +++ b/tests/Auth/AuthenticateTest.php @@ -63,7 +63,16 @@ function cookie(): object return new class { public function has(string $key): bool { - return false; + global $authenticateRememberCookies; + + return isset($authenticateRememberCookies[$key]); + } + + public function get(string $key): mixed + { + global $authenticateRememberCookies; + + return $authenticateRememberCookies[$key] ?? null; } }; } @@ -83,7 +92,9 @@ public function isApiRequest(): bool use Phaseolies\Auth\Authable; use Phaseolies\Auth\Security\Authenticate; + use Phaseolies\DI\Container; use PHPUnit\Framework\TestCase; + use Tests\Support\MockContainer; class FakeAuthableModel extends Authable { @@ -129,16 +140,112 @@ public function user(): ?Authable } } + class RememberedFakeUser extends FakeAuthableModel + { + public ?string $fakeRememberToken = null; + public ?string $fakeTwoFactorSecret = null; + + public function getRememberToken(): ?string + { + return $this->fakeRememberToken; + } + + public function getTwoFactorSecret(): ?string + { + return $this->fakeTwoFactorSecret; + } + } + + class RememberCookieAuthenticate extends Authenticate + { + public function __construct() + { + parent::__construct('web', [ + 'model' => RememberedFakeUser::class, + 'session_key' => 'web_session', + ]); + } + + protected function getModel(): Authable + { + return new RememberedFakeUser(); + } + + protected function getRememberCookieName(): string + { + return 'remember_test'; + } + } + class AuthenticateTest extends TestCase { protected function setUp(): void { - global $authenticateSessionStore; + global $authenticateSessionStore, $authenticateRememberCookies; $authenticateSessionStore = new \Phaseolies\Auth\Security\TestSessionStore(); + $authenticateRememberCookies = []; FakeAuthableModel::$resolvedUser = null; } + /** + * Sign a browser in with a valid remember cookie for a user. + */ + private function rememberUser(?string $twoFactorSecret): RememberedFakeUser + { + global $authenticateRememberCookies; + + $container = new MockContainer(); + Container::setInstance($container); + $container->bind('hash', fn() => new class { + public function check(?string $value, ?string $hash): bool + { + return password_verify((string) $value, (string) $hash); + } + }); + $container->bind('crypt', fn() => new class { + public function decrypt(string $value): string + { + return $value; + } + }); + + $token = 'remember-token'; + + $user = new RememberedFakeUser(); + $user->id = 7; + $user->fakeRememberToken = password_hash($token, PASSWORD_BCRYPT, ['cost' => 4]); + $user->fakeTwoFactorSecret = $twoFactorSecret; + FakeAuthableModel::$resolvedUser = $user; + + $authenticateRememberCookies['remember_test'] = + '7|' . $token . '|' . password_hash('7' . $token, PASSWORD_BCRYPT, ['cost' => 4]); + + return $user; + } + + public function testRememberCookieLogsInUserWithoutTwoFactor() + { + global $authenticateSessionStore; + + $user = $this->rememberUser(null); + + $this->assertSame($user, (new RememberCookieAuthenticate())->user()); + $this->assertSame(7, $authenticateSessionStore->get('web_session')); + } + + public function testRememberCookieDoesNotBypassTwoFactor() + { + global $authenticateSessionStore; + + $this->rememberUser('encrypted-secret'); + + $this->assertNull((new RememberCookieAuthenticate())->user()); + $this->assertFalse($authenticateSessionStore->has('web_session')); + $this->assertSame(7, $authenticateSessionStore->get('2fa_web_user_id')); + $this->assertTrue($authenticateSessionStore->get('2fa_web_remember')); + } + public function testAuthableGetAuthKeyNameDefaultsToEmail() { $user = new FakeAuthableModel(); 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..349eccc0 100644 --- a/tests/Error/JsonErrorRendererTest.php +++ b/tests/Error/JsonErrorRendererTest.php @@ -41,6 +41,7 @@ protected function setUp(): void protected function tearDown(): void { $this->deleteDir($this->translationPath); + unset($_ENV['APP_DEBUG']); parent::tearDown(); } @@ -63,8 +64,37 @@ 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 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/Error/WebErrorHandlerTest.php b/tests/Error/WebErrorHandlerTest.php new file mode 100644 index 00000000..fe51f3e6 --- /dev/null +++ b/tests/Error/WebErrorHandlerTest.php @@ -0,0 +1,41 @@ + [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; + + $method = new ReflectionMethod(WebErrorHandler::class, 'isDebugging'); + + $this->assertSame($expected, $method->invoke(new WebErrorHandler())); + } +} diff --git a/tests/Launchers/RouteLauncherTest.php b/tests/Launchers/RouteLauncherTest.php new file mode 100644 index 00000000..45efda21 --- /dev/null +++ b/tests/Launchers/RouteLauncherTest.php @@ -0,0 +1,37 @@ + ['/users/', '/users'], + 'nested path' => ['/users/7/posts/', '/users/7/posts'], + 'several trailing slashes' => ['/users///', '/users'], + 'query string is kept' => ['/users?next=/', '/users?next='], + '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'], + 'only slashes' => ['//', '/'], + ]; + } + + #[DataProvider('uriProvider')] + public function testRedirectTargetStaysOnThisSite(string $uri, string $expected): void + { + $launcher = (new ReflectionClass(RouteLauncher::class))->newInstanceWithoutConstructor(); + $method = new ReflectionMethod(RouteLauncher::class, 'withoutTrailingSlash'); + + $this->assertSame($expected, $method->invoke($launcher, $uri)); + } +} From c82bc12aa0ba03a2ceabe4283f24c137d82234e7 Mon Sep 17 00:00:00 2001 From: Abdul Majid <43958759+abdulmajidcse@users.noreply.github.com> Date: Thu, 5 Nov 2026 19:59:23 +0600 Subject: [PATCH 2/3] fix(security): address review feedback on error output and redirects - Keep remember-me signing in users with 2FA enabled - Decide whether a JSON error message is public by exception type - Check the trailing slash on the path and keep the query unchanged - Move the APP_DEBUG check into a shared app_debug() helper --- src/Phaseolies/Auth/Security/Authenticate.php | 6 +- .../Error/Handlers/WebErrorHandler.php | 13 +- src/Phaseolies/Error/JsonErrorRenderer.php | 51 ++++---- src/Phaseolies/Helpers/helpers.php | 13 ++ src/Phaseolies/Launchers/RouteLauncher.php | 24 ++-- tests/Auth/AuthenticateTest.php | 111 +----------------- tests/Error/JsonErrorRendererTest.php | 43 +++++++ .../AppDebugTest.php} | 10 +- tests/Launchers/RouteLauncherTest.php | 15 ++- 9 files changed, 117 insertions(+), 169 deletions(-) rename tests/{Error/WebErrorHandlerTest.php => Helpers/AppDebugTest.php} (72%) diff --git a/src/Phaseolies/Auth/Security/Authenticate.php b/src/Phaseolies/Auth/Security/Authenticate.php index b2881898..4e413fd6 100644 --- a/src/Phaseolies/Auth/Security/Authenticate.php +++ b/src/Phaseolies/Auth/Security/Authenticate.php @@ -314,13 +314,9 @@ public function user(): ?Authable } if (Hash::check($token, $user->getRememberToken())) { - // The cookie replaces the password, not the second factor: like - // login(), leave the user signed out until the code is verified. - if ($this->hasTwoFactorEnabled($user) && !$this->isApiRequest()) { + if ($this->hasTwoFactorEnabled($user)) { session()->put($this->getTwoFactorUserKey(), $user->getAuthIdentifier()); session()->put($this->getTwoFactorRememberKey(), true); - - return null; } $this->setUser($user); diff --git a/src/Phaseolies/Error/Handlers/WebErrorHandler.php b/src/Phaseolies/Error/Handlers/WebErrorHandler.php index 70fd4c66..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 ($this->isDebugging()) { + if (app_debug()) { $response = $renderer->renderDebug($exception); } else { $response = $renderer->renderProduction($exception); @@ -34,17 +34,6 @@ public function handle(Throwable $exception): void $response->prepare(request())->send(); } - /** - * Whether debugging is on. A server environment variable arrives as a - * string, and "false" must not count as true. - * - * @return bool - */ - protected function isDebugging(): bool - { - return filter_var(env('APP_DEBUG', false), FILTER_VALIDATE_BOOLEAN); - } - /** * Checks if this handler should handle the current request. * diff --git a/src/Phaseolies/Error/JsonErrorRenderer.php b/src/Phaseolies/Error/JsonErrorRenderer.php index 79ee96a5..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,42 +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), ] - : $this->exceptionPayload($exception, $statusCode, $errorDetails); + : [ + 'message' => $errorDetails ?? $this->publicMessage($exception), + 'errors' => app_debug() + ? [ + 'file' => $exception->getFile(), + 'line' => $exception->getLine(), + 'trace' => $exception->getTraceAsString(), + ] + : [], + ]; return response()->json($response, $statusCode); } /** - * Describe an exception, leaving out its internals unless debugging. + * 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 - * @param int $statusCode - * @param mixed|null $errorDetails - * @return array + * @return string */ - protected function exceptionPayload(Throwable $exception, int $statusCode, mixed $errorDetails): array + protected function publicMessage(Throwable $exception): string { - if (filter_var(env('APP_DEBUG', false), FILTER_VALIDATE_BOOLEAN)) { - return [ - 'message' => $errorDetails ?? $exception->getMessage(), - 'errors' => [ - 'file' => $exception->getFile(), - 'line' => $exception->getLine(), - 'trace' => $exception->getTraceAsString(), - ], - ]; - } + $isClientError = $exception instanceof TokenMismatchException || ( + $exception instanceof HttpException && + $exception->getStatusCode() >= 400 && + $exception->getStatusCode() < 500 + ); - return [ - 'message' => $errorDetails ?? ($statusCode >= 500 ? 'Something went wrong' : $exception->getMessage()), - 'errors' => [], - ]; + 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 64c12314..deb85b50 100644 --- a/src/Phaseolies/Launchers/RouteLauncher.php +++ b/src/Phaseolies/Launchers/RouteLauncher.php @@ -13,28 +13,34 @@ 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: ' . $this->withoutTrailingSlash(request()->server->get('REQUEST_URI')), true, 301); + if ($target !== null) { + header('Location: ' . $target, true, 301); exit; } } /** - * Remove the trailing slash from a request URI, keeping the result on this site. + * 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. * * @param string $uri - * @return string + * @return string|null */ - protected function withoutTrailingSlash(string $uri): string + protected function trailingSlashRedirect(string $uri): ?string { - return '/' . ltrim(rtrim($uri, '/'), '/\\'); + [$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; } /** diff --git a/tests/Auth/AuthenticateTest.php b/tests/Auth/AuthenticateTest.php index b5b90753..828594cd 100644 --- a/tests/Auth/AuthenticateTest.php +++ b/tests/Auth/AuthenticateTest.php @@ -63,16 +63,7 @@ function cookie(): object return new class { public function has(string $key): bool { - global $authenticateRememberCookies; - - return isset($authenticateRememberCookies[$key]); - } - - public function get(string $key): mixed - { - global $authenticateRememberCookies; - - return $authenticateRememberCookies[$key] ?? null; + return false; } }; } @@ -92,9 +83,7 @@ public function isApiRequest(): bool use Phaseolies\Auth\Authable; use Phaseolies\Auth\Security\Authenticate; - use Phaseolies\DI\Container; use PHPUnit\Framework\TestCase; - use Tests\Support\MockContainer; class FakeAuthableModel extends Authable { @@ -140,112 +129,16 @@ public function user(): ?Authable } } - class RememberedFakeUser extends FakeAuthableModel - { - public ?string $fakeRememberToken = null; - public ?string $fakeTwoFactorSecret = null; - - public function getRememberToken(): ?string - { - return $this->fakeRememberToken; - } - - public function getTwoFactorSecret(): ?string - { - return $this->fakeTwoFactorSecret; - } - } - - class RememberCookieAuthenticate extends Authenticate - { - public function __construct() - { - parent::__construct('web', [ - 'model' => RememberedFakeUser::class, - 'session_key' => 'web_session', - ]); - } - - protected function getModel(): Authable - { - return new RememberedFakeUser(); - } - - protected function getRememberCookieName(): string - { - return 'remember_test'; - } - } - class AuthenticateTest extends TestCase { protected function setUp(): void { - global $authenticateSessionStore, $authenticateRememberCookies; + global $authenticateSessionStore; $authenticateSessionStore = new \Phaseolies\Auth\Security\TestSessionStore(); - $authenticateRememberCookies = []; FakeAuthableModel::$resolvedUser = null; } - /** - * Sign a browser in with a valid remember cookie for a user. - */ - private function rememberUser(?string $twoFactorSecret): RememberedFakeUser - { - global $authenticateRememberCookies; - - $container = new MockContainer(); - Container::setInstance($container); - $container->bind('hash', fn() => new class { - public function check(?string $value, ?string $hash): bool - { - return password_verify((string) $value, (string) $hash); - } - }); - $container->bind('crypt', fn() => new class { - public function decrypt(string $value): string - { - return $value; - } - }); - - $token = 'remember-token'; - - $user = new RememberedFakeUser(); - $user->id = 7; - $user->fakeRememberToken = password_hash($token, PASSWORD_BCRYPT, ['cost' => 4]); - $user->fakeTwoFactorSecret = $twoFactorSecret; - FakeAuthableModel::$resolvedUser = $user; - - $authenticateRememberCookies['remember_test'] = - '7|' . $token . '|' . password_hash('7' . $token, PASSWORD_BCRYPT, ['cost' => 4]); - - return $user; - } - - public function testRememberCookieLogsInUserWithoutTwoFactor() - { - global $authenticateSessionStore; - - $user = $this->rememberUser(null); - - $this->assertSame($user, (new RememberCookieAuthenticate())->user()); - $this->assertSame(7, $authenticateSessionStore->get('web_session')); - } - - public function testRememberCookieDoesNotBypassTwoFactor() - { - global $authenticateSessionStore; - - $this->rememberUser('encrypted-secret'); - - $this->assertNull((new RememberCookieAuthenticate())->user()); - $this->assertFalse($authenticateSessionStore->has('web_session')); - $this->assertSame(7, $authenticateSessionStore->get('2fa_web_user_id')); - $this->assertTrue($authenticateSessionStore->get('2fa_web_remember')); - } - public function testAuthableGetAuthKeyNameDefaultsToEmail() { $user = new FakeAuthableModel(); diff --git a/tests/Error/JsonErrorRendererTest.php b/tests/Error/JsonErrorRendererTest.php index 349eccc0..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; @@ -78,6 +79,48 @@ public function testRenderHidesExceptionInternalsWhenNotDebugging(): void } } + 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'; diff --git a/tests/Error/WebErrorHandlerTest.php b/tests/Helpers/AppDebugTest.php similarity index 72% rename from tests/Error/WebErrorHandlerTest.php rename to tests/Helpers/AppDebugTest.php index fe51f3e6..ffee88c9 100644 --- a/tests/Error/WebErrorHandlerTest.php +++ b/tests/Helpers/AppDebugTest.php @@ -1,13 +1,11 @@ assertSame($expected, $method->invoke(new WebErrorHandler())); + $this->assertSame($expected, app_debug()); } } diff --git a/tests/Launchers/RouteLauncherTest.php b/tests/Launchers/RouteLauncherTest.php index 45efda21..972b7488 100644 --- a/tests/Launchers/RouteLauncherTest.php +++ b/tests/Launchers/RouteLauncherTest.php @@ -16,21 +16,28 @@ public static function uriProvider(): array '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='], + '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'], - 'only slashes' => ['//', '/'], + '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], ]; } #[DataProvider('uriProvider')] - public function testRedirectTargetStaysOnThisSite(string $uri, string $expected): void + public function testRedirectTargetStaysOnThisSite(string $uri, ?string $expected): void { $launcher = (new ReflectionClass(RouteLauncher::class))->newInstanceWithoutConstructor(); - $method = new ReflectionMethod(RouteLauncher::class, 'withoutTrailingSlash'); + $method = new ReflectionMethod(RouteLauncher::class, 'trailingSlashRedirect'); $this->assertSame($expected, $method->invoke($launcher, $uri)); } From 2da65d90bfccdb255b4e57d386305987b5370652 Mon Sep 17 00:00:00 2001 From: Abdul Majid <43958759+abdulmajidcse@users.noreply.github.com> Date: Thu, 5 Nov 2026 21:43:33 +0600 Subject: [PATCH 3/3] fix(security): never redirect a URI with control characters Browsers drop tabs and newlines from a Location, so a target such as "//evil.com" would still lead to another site. --- src/Phaseolies/Launchers/RouteLauncher.php | 6 ++++++ tests/Launchers/RouteLauncherTest.php | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/src/Phaseolies/Launchers/RouteLauncher.php b/src/Phaseolies/Launchers/RouteLauncher.php index deb85b50..e9bf9f66 100644 --- a/src/Phaseolies/Launchers/RouteLauncher.php +++ b/src/Phaseolies/Launchers/RouteLauncher.php @@ -26,12 +26,18 @@ public function register() * * 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, '/'), '/\\'); diff --git a/tests/Launchers/RouteLauncherTest.php b/tests/Launchers/RouteLauncherTest.php index 972b7488..18d886d2 100644 --- a/tests/Launchers/RouteLauncherTest.php +++ b/tests/Launchers/RouteLauncherTest.php @@ -30,6 +30,13 @@ public static function uriProvider(): array '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'], ]; }