Răsfoiți Sursa

fix: require int getCode() before using as HTTP status (SEC_REVIEW F73)

`JsonExceptionHandler::__invoke` lifted `$exception->getCode()`
into the response status if it was loosely "in [400, 600)":

  if ($e->getCode() >= 400 && $e->getCode() < 600) {
      $status = $e->getCode();
  }

`Throwable::getCode()` is typed `int` in the interface contract,
but PDO-derived exceptions ignore the contract and return SQLSTATE
*strings* (`'42S02'`, `'HY000'`). PHP's loose comparison then
coerces — `'42S02'` → 42 (no match, OK by accident) but `'400'`
→ 400 (matches and silently skips the prod message-suppression
the same handler applies below). A long string like
`'400 server error: foo'` also coerces to 400.

Require `is_int($code)` before treating it as a status. PDO
strings, default 0 from bare `new \Exception('msg')`, and any
other non-int value all fall back to the safe 500 default. The
`is_client_error` flag in the rendered template simplifies to
`$status < 500` since the gate above already pins `$status` to
[400, 599] — phpstan flagged the redundant `>= 400` check after
the type narrowing, so it's gone too.

Regression tests in
`ui/tests/Unit/Http/JsonExceptionHandlerTest.php`:
  - `testStringCodeFallsBackTo500` — a hand-rolled exception with
    `$this->code = '400'` (the SEC_REVIEW's literal example),
    response is 500.
  - `testIntCodeInRangeIsHonored` — real `new RuntimeException(
    'forbidden', 403)` → 403.
  - `testIntCodeOutOfRangeFallsBackTo500` — code 200 (not in
    error range) → 500.
  - `testCodeOfZeroFallsBackTo500` — default 0 → 500.
  - `testSqlstateLikeStringIsNotCoercedIntoStatus` — `'42S02'`
    → 500.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 3 zile în urmă
părinte
comite
458a224a88

+ 15 - 3
ui/src/Http/JsonExceptionHandler.php

@@ -44,16 +44,28 @@ final class JsonExceptionHandler
             'line' => $exception->getLine(),
             'line' => $exception->getLine(),
         ]);
         ]);
 
 
+        // SEC_REVIEW F73: `Throwable::getCode()` is typed `int` in the
+        // interface contract but `PDOException` (and other PDO-derived
+        // exceptions) store a SQLSTATE *string* (e.g. `'42S02'`,
+        // `'HY000'`) and return it verbatim. The previous loose
+        // comparison `>= 400 && < 600` coerced the string to int —
+        // `'42S02'` → 42 (no match, OK), but `'400'` → 400 (matches
+        // and silently skips the prod message-suppression path
+        // applied below). Require a real `int` value before treating
+        // it as an HTTP status.
         $status = 500;
         $status = 500;
-        if (method_exists($exception, 'getCode') && $exception->getCode() >= 400 && $exception->getCode() < 600) {
-            $status = $exception->getCode();
+        $code = $exception->getCode();
+        if (is_int($code) && $code >= 400 && $code < 600) {
+            $status = $code;
         }
         }
 
 
         $response = $this->responseFactory->createResponse($status);
         $response = $this->responseFactory->createResponse($status);
         try {
         try {
             return $this->twig->render($response, 'pages/error.twig', [
             return $this->twig->render($response, 'pages/error.twig', [
                 'status' => $status,
                 'status' => $status,
-                'is_client_error' => $status >= 400 && $status < 500,
+                // `$status` is constrained to [400, 599] by the gate
+                // above, so client-error == "below 500".
+                'is_client_error' => $status < 500,
                 'message' => $this->isDev ? $exception->getMessage() : null,
                 'message' => $this->isDev ? $exception->getMessage() : null,
             ]);
             ]);
         } catch (Throwable) {
         } catch (Throwable) {

+ 112 - 0
ui/tests/Unit/Http/JsonExceptionHandlerTest.php

@@ -0,0 +1,112 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Unit\Http;
+
+use App\Http\JsonExceptionHandler;
+use Monolog\Handler\NullHandler;
+use Monolog\Logger;
+use PHPUnit\Framework\TestCase;
+use Slim\Psr7\Factory\ResponseFactory;
+use Slim\Psr7\Factory\ServerRequestFactory;
+use Slim\Views\Twig;
+
+/**
+ * SEC_REVIEW F73 — `Throwable::getCode()` is typed `int` in the
+ * interface contract, but PDO-derived exceptions return SQLSTATE
+ * strings. The previous loose `>= 400 && < 600` comparison
+ * coerced strings to int and could land on a real HTTP status
+ * for the wrong reason. The handler now requires `is_int($code)`
+ * before treating it as an HTTP status.
+ */
+final class JsonExceptionHandlerTest extends TestCase
+{
+    public function testStringCodeFallsBackTo500(): void
+    {
+        // PDOException uses a string SQLSTATE for getCode(). A
+        // hand-rolled subclass below mimics the real-world shape:
+        // string `'400'` would loose-compare as 400 and previously
+        // set the response status to 400, skipping the prod
+        // message-suppression. Now it falls back to 500.
+        $exception = new class () extends \RuntimeException {
+            public function __construct()
+            {
+                parent::__construct('SQLSTATE-style error');
+                $this->code = '400'; // PDOException stores a string here.
+            }
+        };
+
+        $handler = $this->makeHandler(isDev: false);
+        $request = (new ServerRequestFactory())->createServerRequest('GET', '/x');
+        $response = $handler($request, $exception, false, false, false);
+
+        self::assertSame(500, $response->getStatusCode());
+    }
+
+    public function testIntCodeInRangeIsHonored(): void
+    {
+        $exception = new \RuntimeException('forbidden', 403);
+
+        $handler = $this->makeHandler(isDev: false);
+        $request = (new ServerRequestFactory())->createServerRequest('GET', '/x');
+        $response = $handler($request, $exception, false, false, false);
+
+        self::assertSame(403, $response->getStatusCode());
+    }
+
+    public function testIntCodeOutOfRangeFallsBackTo500(): void
+    {
+        $exception = new \RuntimeException('not http', 200);
+
+        $handler = $this->makeHandler(isDev: false);
+        $request = (new ServerRequestFactory())->createServerRequest('GET', '/x');
+        $response = $handler($request, $exception, false, false, false);
+
+        self::assertSame(500, $response->getStatusCode());
+    }
+
+    public function testCodeOfZeroFallsBackTo500(): void
+    {
+        // Code 0 is the default for `new \Exception('msg')` — we
+        // must not let it accidentally land on a 400-class status.
+        $exception = new \RuntimeException('plain');
+
+        $handler = $this->makeHandler(isDev: false);
+        $request = (new ServerRequestFactory())->createServerRequest('GET', '/x');
+        $response = $handler($request, $exception, false, false, false);
+
+        self::assertSame(500, $response->getStatusCode());
+    }
+
+    public function testSqlstateLikeStringIsNotCoercedIntoStatus(): void
+    {
+        // PDOException for a missing table is `'42S02'`. Loose
+        // coerce to int gives 42 (out of HTTP range, falls back to
+        // 500 even before the F73 fix). But the SEC_REVIEW called
+        // out a `'400'` that would coerce neatly. Both must be
+        // rejected by the int-only gate.
+        $exception = new class () extends \RuntimeException {
+            public function __construct()
+            {
+                parent::__construct('SQL error');
+                $this->code = '42S02';
+            }
+        };
+
+        $handler = $this->makeHandler(isDev: false);
+        $request = (new ServerRequestFactory())->createServerRequest('GET', '/x');
+        $response = $handler($request, $exception, false, false, false);
+
+        self::assertSame(500, $response->getStatusCode());
+    }
+
+    private function makeHandler(bool $isDev): JsonExceptionHandler
+    {
+        $twig = Twig::create(__DIR__ . '/../../../resources/views', ['cache' => false]);
+        $logger = new Logger('test');
+        $logger->pushHandler(new NullHandler());
+
+        return new JsonExceptionHandler($twig, new ResponseFactory(), $logger, $isDev);
+    }
+}