fix: never leak exception messages from JsonErrorHandler (SEC_REVIEW F26)

Production responses must not echo `Throwable::getMessage()`. The handler
now maps every status code to a fixed snake_case token (`bad_request`,
`forbidden`, …); raw messages — possibly attacker-influenced or carrying
internal details (DBAL/PDO strings, file paths) — are only added under a
separate `detail` field when expose mode is on (dev or Slim's
`displayErrorDetails`).

Out-of-range or default `getCode()=0` HttpExceptions clamp to 500 so we
never serialise an invalid HTTP status. Non-HttpException Throwables
collapse to 500 regardless of their numeric code, closing the previous
4xx-bypass path. Unit tests cover the canonical statuses, the leak guards,
the clamp, and the dev-mode detail shape.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

api/src/Infrastructure/Http/JsonErrorHandler.php

@@ -16,17 +16,42 @@ use Throwable;
 
 /**
  * Slim-compatible error handler that produces uniform
- * `{"error":"...","details":...}` JSON responses.
+ * `{"error":"<token>"}` JSON responses.
  *
- * Logs every uncaught exception at error level. In production we do not
- * leak exception messages from non-HTTP exceptions to clients (returns a
- * generic "internal error"); in development the logger has the full
- * detail and the response body carries the message for debuggability.
+ * Production responses never echo `Throwable::getMessage()`. Status codes
+ * are mapped to a fixed set of snake_case tokens; any unmapped or
+ * out-of-range code collapses to 500 / `internal_error`. In development
+ * (`exposeDetails=true` or Slim's `$displayErrorDetails` flag), the
+ * exception class and message are added under `detail` for debuggability.
  *
  * Wired in public/index.php as the default invokable.
  */
 final class JsonErrorHandler
 {
+    /** @var array<int, string> */
+    private const STATUS_TOKENS = [
+        400 => 'bad_request',
+        401 => 'unauthorized',
+        403 => 'forbidden',
+        404 => 'not_found',
+        405 => 'method_not_allowed',
+        406 => 'not_acceptable',
+        409 => 'conflict',
+        410 => 'gone',
+        411 => 'length_required',
+        412 => 'precondition_failed',
+        413 => 'payload_too_large',
+        414 => 'uri_too_long',
+        415 => 'unsupported_media_type',
+        422 => 'unprocessable_entity',
+        429 => 'too_many_requests',
+        500 => 'internal_error',
+        501 => 'not_implemented',
+        502 => 'bad_gateway',
+        503 => 'service_unavailable',
+        504 => 'gateway_timeout',
+    ];
+
     public function __construct(
         private readonly ResponseFactoryInterface $responseFactory,
         private readonly LoggerInterface $logger,
@@ -50,12 +75,8 @@ final class JsonErrorHandler
             ]);
         }
 
-        [$status, $payload] = $this->shape($exception);
         $expose = $displayErrorDetails || $this->exposeDetails;
-
-        if (!$expose && $status >= 500) {
-            $payload = ['error' => 'internal error'];
-        }
+        [$status, $payload] = $this->shape($exception, $expose);
 
         $response = $this->responseFactory->createResponse($status)
             ->withHeader('Content-Type', 'application/json');
@@ -67,18 +88,46 @@ final class JsonErrorHandler
     /**
      * @return array{int, array<string, mixed>}
      */
-    private function shape(Throwable $e): array
+    private function shape(Throwable $e, bool $expose): array
     {
         if ($e instanceof HttpNotFoundException) {
-            return [404, ['error' => 'not_found']];
+            return [404, $this->payload(404, $e, $expose)];
         }
         if ($e instanceof HttpMethodNotAllowedException) {
-            return [405, ['error' => 'method_not_allowed']];
+            return [405, $this->payload(405, $e, $expose)];
         }
         if ($e instanceof HttpException) {
-            return [$e->getCode(), ['error' => $e->getMessage()]];
+            $status = $this->safeStatus($e->getCode());
+
+            return [$status, $this->payload($status, $e, $expose)];
+        }
+
+        return [500, $this->payload(500, $e, $expose)];
+    }
+
+    /**
+     * @return array<string, mixed>
+     */
+    private function payload(int $status, Throwable $e, bool $expose): array
+    {
+        $payload = ['error' => self::STATUS_TOKENS[$status] ?? 'internal_error'];
+        if ($expose) {
+            $message = $e->getMessage();
+            $payload['detail'] = [
+                'exception' => $e::class,
+                'message' => $message !== '' ? $message : null,
+            ];
+        }
+
+        return $payload;
+    }
+
+    private function safeStatus(int $code): int
+    {
+        if ($code < 400 || $code > 599) {
+            return 500;
         }
 
-        return [500, ['error' => $e->getMessage() !== '' ? $e->getMessage() : 'internal error']];
+        return $code;
     }
 }

api/tests/Unit/Http/JsonErrorHandlerTest.php

@@ -0,0 +1,241 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Unit\Http;
+
+use App\Infrastructure\Http\JsonErrorHandler;
+use PHPUnit\Framework\TestCase;
+use Psr\Log\NullLogger;
+use RuntimeException;
+use Slim\Exception\HttpBadRequestException;
+use Slim\Exception\HttpException;
+use Slim\Exception\HttpForbiddenException;
+use Slim\Exception\HttpInternalServerErrorException;
+use Slim\Exception\HttpMethodNotAllowedException;
+use Slim\Exception\HttpNotFoundException;
+use Slim\Psr7\Factory\ResponseFactory;
+use Slim\Psr7\Factory\ServerRequestFactory;
+
+/**
+ * Production responses must never leak attacker-influenced exception
+ * messages (SEC_REVIEW F26). The handler maps statuses to a fixed token
+ * set, clamps invalid codes, and only adds `detail` when expose is on.
+ */
+final class JsonErrorHandlerTest extends TestCase
+{
+    public function testNotFoundReturns404Token(): void
+    {
+        $request = $this->request();
+        $handler = $this->handler(expose: false);
+
+        $response = $handler($request, new HttpNotFoundException($request, 'attacker /etc/passwd'), false, false, false);
+
+        self::assertSame(404, $response->getStatusCode());
+        self::assertSame(['error' => 'not_found'], $this->json($response));
+    }
+
+    public function testMethodNotAllowedReturns405Token(): void
+    {
+        $request = $this->request();
+        $handler = $this->handler(expose: false);
+
+        $response = $handler($request, new HttpMethodNotAllowedException($request), false, false, false);
+
+        self::assertSame(405, $response->getStatusCode());
+        self::assertSame(['error' => 'method_not_allowed'], $this->json($response));
+    }
+
+    public function testHttpExceptionMessageNeverLeaksInProduction(): void
+    {
+        $request = $this->request();
+        $handler = $this->handler(expose: false);
+
+        // A controller that wrapped a DB error into an HttpBadRequestException
+        // would otherwise echo the raw driver message verbatim.
+        $exception = new HttpBadRequestException(
+            $request,
+            "SQLSTATE[23000]: duplicate key value violates unique constraint 'users_email_idx'",
+        );
+
+        $response = $handler($request, $exception, false, false, false);
+
+        self::assertSame(400, $response->getStatusCode());
+        self::assertSame(['error' => 'bad_request'], $this->json($response));
+    }
+
+    public function testHttpForbiddenMapsToToken(): void
+    {
+        $request = $this->request();
+        $handler = $this->handler(expose: false);
+
+        $response = $handler($request, new HttpForbiddenException($request, 'role missing: foo'), false, false, false);
+
+        self::assertSame(403, $response->getStatusCode());
+        self::assertSame(['error' => 'forbidden'], $this->json($response));
+    }
+
+    public function testHttp500ExceptionDoesNotLeakMessage(): void
+    {
+        $request = $this->request();
+        $handler = $this->handler(expose: false);
+
+        $response = $handler(
+            $request,
+            new HttpInternalServerErrorException($request, 'connection to db failed at 10.0.0.5:5432'),
+            false,
+            false,
+            false,
+        );
+
+        self::assertSame(500, $response->getStatusCode());
+        self::assertSame(['error' => 'internal_error'], $this->json($response));
+    }
+
+    public function testGenericThrowableIsAlways500AndNeverLeaksInProd(): void
+    {
+        $request = $this->request();
+        $handler = $this->handler(expose: false);
+
+        $response = $handler(
+            $request,
+            new RuntimeException('database connection refused: pgsql://app@10.0.0.5:5432'),
+            false,
+            false,
+            false,
+        );
+
+        self::assertSame(500, $response->getStatusCode());
+        self::assertSame(['error' => 'internal_error'], $this->json($response));
+    }
+
+    public function testGenericThrowableWithCodeInClientRangeStillReturns500(): void
+    {
+        $request = $this->request();
+        $handler = $this->handler(expose: false);
+
+        // Some PDO/DBAL/parse exceptions carry a numeric code that *happens*
+        // to fall in 400-499. The handler must not honour that as an HTTP
+        // status — non-HttpException Throwables collapse to 500.
+        $exception = new RuntimeException('PDO: 42 invalid SQL syntax', 422);
+
+        $response = $handler($request, $exception, false, false, false);
+
+        self::assertSame(500, $response->getStatusCode());
+        self::assertSame(['error' => 'internal_error'], $this->json($response));
+    }
+
+    public function testHttpExceptionWithOutOfRangeCodeClampsTo500(): void
+    {
+        $request = $this->request();
+        $handler = $this->handler(expose: false);
+
+        // Bare `new HttpException(...)` with the default code=0 must not
+        // emit an invalid HTTP status to the wire.
+        $exception = new HttpException($request, 'oops', 0);
+
+        $response = $handler($request, $exception, false, false, false);
+
+        self::assertSame(500, $response->getStatusCode());
+        self::assertSame(['error' => 'internal_error'], $this->json($response));
+    }
+
+    public function testHttpExceptionWithUnmappedKnownStatusFallsBackToInternalError(): void
+    {
+        $request = $this->request();
+        $handler = $this->handler(expose: false);
+
+        // 418 is in the valid range but not in the canonical token map.
+        $exception = new HttpException($request, 'I am a teapot', 418);
+
+        $response = $handler($request, $exception, false, false, false);
+
+        self::assertSame(418, $response->getStatusCode());
+        self::assertSame(['error' => 'internal_error'], $this->json($response));
+    }
+
+    public function testExposeDetailsAddsDetailFieldInDevelopment(): void
+    {
+        $request = $this->request();
+        $handler = $this->handler(expose: true);
+
+        $response = $handler(
+            $request,
+            new RuntimeException('database connection refused'),
+            false,
+            false,
+            false,
+        );
+
+        self::assertSame(500, $response->getStatusCode());
+        $body = $this->json($response);
+        self::assertSame('internal_error', $body['error']);
+        self::assertIsArray($body['detail']);
+        self::assertSame(RuntimeException::class, $body['detail']['exception']);
+        self::assertSame('database connection refused', $body['detail']['message']);
+    }
+
+    public function testDisplayErrorDetailsFlagAlsoExposesDetail(): void
+    {
+        $request = $this->request();
+        // Constructor flag is false (production), but Slim's per-request
+        // $displayErrorDetails should still gate exposure on for debug paths.
+        $handler = $this->handler(expose: false);
+
+        $response = $handler(
+            $request,
+            new HttpBadRequestException($request, 'bespoke validation failure'),
+            true,
+            false,
+            false,
+        );
+
+        self::assertSame(400, $response->getStatusCode());
+        $body = $this->json($response);
+        self::assertSame('bad_request', $body['error']);
+        self::assertSame('bespoke validation failure', $body['detail']['message']);
+    }
+
+    public function testEmptyMessageInDetailIsNullNotEmptyString(): void
+    {
+        $request = $this->request();
+        $handler = $this->handler(expose: true);
+
+        $response = $handler($request, new RuntimeException(''), false, false, false);
+
+        $body = $this->json($response);
+        self::assertNull($body['detail']['message']);
+    }
+
+    public function testResponseAlwaysHasJsonContentType(): void
+    {
+        $request = $this->request();
+        $handler = $this->handler(expose: false);
+
+        $response = $handler($request, new RuntimeException('x'), false, false, false);
+
+        self::assertSame('application/json', $response->getHeaderLine('Content-Type'));
+    }
+
+    private function handler(bool $expose): JsonErrorHandler
+    {
+        return new JsonErrorHandler(new ResponseFactory(), new NullLogger(), $expose);
+    }
+
+    private function request(): \Psr\Http\Message\ServerRequestInterface
+    {
+        return (new ServerRequestFactory())->createServerRequest('GET', 'https://api.test/x');
+    }
+
+    /**
+     * @return array<string, mixed>
+     */
+    private function json(\Psr\Http\Message\ResponseInterface $response): array
+    {
+        $body = (string) $response->getBody();
+        /** @var array<string, mixed> $decoded */
+        $decoded = json_decode($body, true, flags: JSON_THROW_ON_ERROR);
+
+        return $decoded;
+    }
+}