Skip to content

Commit e4b9478

Browse files
LennyObezLennyObez
committed
fix(security): render the CSRF 403 through the error renderer, not a hardcoded page
CsrfMiddleware emitted a hardcoded <!DOCTYPE html> 403 (and a bespoke JSON body) that bypassed the application's configured error renderer, so the CSRF rejection could not be themed or localized like every other 4xx. CsrfMiddleware now content-negotiates its 403: - JSON for API clients (Accept: application/json, or X-Requested-With: XMLHttpRequest), preserving the prior XHR-as-JSON behaviour. - HTML rendered via the application's configured ExceptionRendererInterface — themed and localized like every other error page — when one is wired, falling back to a minimal inline document otherwise. The response is returned, not thrown. Throwing would let the Kernel's outer catch build the error response after the middleware stack has unwound, stripping the SecurityHeadersMiddleware headers off the 403; returning keeps the response flowing back out through the stack so security headers still apply on rejection (covered by SecurityPipelineTest::securityHeadersAreAppliedEvenOnCsrfRejection). The renderer is wired by ExceptionHandlerWiring, which runs after SecurityWiring, so SecurityWiring injects a lazy resolver closure (mirroring its own templateEngineResolver pattern) that the middleware invokes at request time. The optional constructor parameter is additive: callers that omit it get the minimal fallback page, exactly as before.
1 parent a83fc6c commit e4b9478

4 files changed

Lines changed: 116 additions & 9 deletions

File tree

‎src/Core/Wiring/SecurityWiring.php‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
use Pulsar\DataProtection\InMemoryConsentManager;
2929
use Pulsar\DataProtection\RetentionPolicyInterface;
3030
use Pulsar\DataProtection\SessionPurge;
31+
use Pulsar\ErrorHandling\ExceptionRendererInterface;
3132
use Pulsar\Http\Middleware\MiddlewarePipeline;
3233
use Pulsar\Http\Middleware\MiddlewareRegistry;
3334
use Pulsar\Http\TrustedProxy;
@@ -335,7 +336,22 @@ public function wire(
335336
$container->instance(CsrfTokenManager::class, $csrfTokenManager);
336337
$container->instance(CsrfTokenManagerInterface::class, $csrfTokenManager);
337338

338-
$csrfMiddleware = new CsrfMiddleware($csrfTokenManager, $securityConfig->csrf);
339+
// Lazy resolver: the error-page renderer is wired by ExceptionHandlerWiring,
340+
// which runs after this wiring, so the CSRF middleware resolves it at
341+
// request time to theme its 403 page (mirrors ExceptionHandlerWiring's
342+
// own templateEngineResolver pattern).
343+
$errorRendererResolver = static function () use ($container): ?ExceptionRendererInterface {
344+
if (!$container->has(ExceptionRendererInterface::class)) {
345+
return null;
346+
}
347+
348+
/** @var ExceptionRendererInterface $renderer */
349+
$renderer = $container->get(ExceptionRendererInterface::class);
350+
351+
return $renderer;
352+
};
353+
354+
$csrfMiddleware = new CsrfMiddleware($csrfTokenManager, $securityConfig->csrf, $errorRendererResolver);
339355
$container->instance(CsrfMiddleware::class, $csrfMiddleware);
340356

341357
// Security Headers: gate X-Forwarded-Proto on trusted proxy IPs (CFR-71).

‎src/Security/Csrf/CsrfMiddleware.php‎

Lines changed: 33 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,16 @@
44

55
namespace Pulsar\Security\Csrf;
66

7+
use Closure;
78
use JsonException;
89
use Override;
910
use Psr\Http\Message\ResponseInterface;
1011
use Psr\Http\Message\ServerRequestInterface;
1112
use Psr\Http\Server\RequestHandlerInterface;
1213
use Pulsar\Api\Api;
1314
use Pulsar\Config\CsrfConfig;
15+
use Pulsar\ErrorHandling\ExceptionRendererInterface;
16+
use Pulsar\ErrorHandling\HttpException;
1417
use Pulsar\Http\Message\Response;
1518
use Pulsar\Http\Middleware\MiddlewareInterface;
1619
use Pulsar\Http\ResponseStatus;
@@ -42,15 +45,27 @@
4245
* - The configured header (default: `X-CSRF-Token`)
4346
* - The configured POST field (default: `_csrf_token`)
4447
*
45-
* Returns a 403 Forbidden JSON response when validation fails.
48+
* Returns a 403 Forbidden response when validation fails, content-negotiated:
49+
* JSON for API clients, and — for browsers — HTML rendered by the application's
50+
* configured {@see ExceptionRendererInterface} (themed and localized like every
51+
* other 4xx) when one is wired, falling back to a minimal page otherwise. The
52+
* response is returned (not thrown) so it still flows through the outer
53+
* middleware (e.g. security headers) before reaching the client.
4654
* @api
4755
*/
4856
#[Api(since: '1.0.0')]
4957
final readonly class CsrfMiddleware implements MiddlewareInterface
5058
{
59+
/**
60+
* @param (Closure(): ?ExceptionRendererInterface)|null $errorRendererResolver
61+
* Lazy resolver for the error-page renderer, supplied by the composition
62+
* root because the renderer is wired after this middleware. Invoked at
63+
* request time to theme the 403 HTML page; null yields a minimal page.
64+
*/
5165
public function __construct(
5266
private CsrfTokenManagerInterface $tokenManager,
5367
private CsrfConfig $config,
68+
private ?Closure $errorRendererResolver = null,
5469
) {}
5570

5671
#[Override]
@@ -243,13 +258,17 @@ private function extractOriginFromUrl(string $url): ?string
243258
}
244259

245260
/**
246-
* Create a 403 Forbidden response, content-negotiated for the client.
261+
* Build a 403 Forbidden response, content-negotiated for the client.
262+
*
263+
* JSON clients (Accept: application/json or X-Requested-With: XMLHttpRequest)
264+
* get a JSON body. Browsers get HTML rendered by the configured error-page
265+
* renderer — themed and localized like every other 4xx — when one is wired;
266+
* otherwise a minimal inline page. The response is returned (not thrown) so
267+
* the outer middleware (e.g. security headers) still applies to it.
247268
*/
248269
private function forbiddenResponse(ServerRequestInterface $request, string $message): ResponseInterface
249270
{
250-
$accept = $request->getHeaderLine('Accept');
251-
252-
if (str_contains($accept, 'application/json')
271+
if (str_contains($request->getHeaderLine('Accept'), 'application/json')
253272
|| $request->getHeaderLine('X-Requested-With') === 'XMLHttpRequest'
254273
) {
255274
return Response::json(
@@ -258,13 +277,20 @@ private function forbiddenResponse(ServerRequestInterface $request, string $mess
258277
);
259278
}
260279

261-
$escapedMessage = htmlspecialchars($message);
280+
$renderer = $this->errorRendererResolver !== null ? ($this->errorRendererResolver)() : null;
281+
282+
if ($renderer instanceof ExceptionRendererInterface) {
283+
return Response::html(
284+
$renderer->render(HttpException::forbidden($message), $request, ResponseStatus::Forbidden),
285+
ResponseStatus::Forbidden->value,
286+
);
287+
}
262288

263289
return Response::html(
264290
sprintf(
265291
'<!DOCTYPE html><html><head><title>403 Forbidden</title></head>'
266292
. '<body><h1>403 Forbidden</h1><p>%s</p></body></html>',
267-
$escapedMessage,
293+
htmlspecialchars($message),
268294
),
269295
ResponseStatus::Forbidden->value,
270296
);

‎tests/Unit/Security/Csrf/CsrfMiddlewareTest.php‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,16 @@
88
use PHPUnit\Framework\Attributes\Test;
99
use PHPUnit\Framework\MockObject\Stub;
1010
use PHPUnit\Framework\TestCase;
11+
use Psr\Http\Message\ServerRequestInterface;
1112
use Psr\Http\Server\RequestHandlerInterface;
1213
use Pulsar\Config\CsrfConfig;
14+
use Pulsar\ErrorHandling\ExceptionRendererInterface;
1315
use Pulsar\Http\Message\Response;
1416
use Pulsar\Http\Message\ServerRequest;
1517
use Pulsar\Http\ResponseStatus;
1618
use Pulsar\Security\Csrf\CsrfMiddleware;
1719
use Pulsar\Security\Csrf\CsrfTokenManagerInterface;
20+
use Throwable;
1821

1922
#[CoversClass(CsrfMiddleware::class)]
2023
final class CsrfMiddlewareTest extends TestCase
@@ -512,4 +515,38 @@ public function postWithOversizedJsonBodyDoesNotParse(): void
512515

513516
self::assertSame(ResponseStatus::Forbidden->value, $response->getStatusCode());
514517
}
518+
519+
#[Test]
520+
public function rendersThemedHtmlViaConfiguredRendererForBrowserRequest(): void
521+
{
522+
// A configured error-page renderer themes/localizes the 403 page just
523+
// like every other 4xx; the middleware must delegate to it (not emit a
524+
// hardcoded document) when one is wired through the composition root.
525+
$renderer = new class implements ExceptionRendererInterface {
526+
public function render(Throwable $exception, ServerRequestInterface $request, ResponseStatus $status): string
527+
{
528+
return '<main data-themed="1">' . $status->value . ': ' . $exception->getMessage() . '</main>';
529+
}
530+
};
531+
532+
$middleware = new CsrfMiddleware(
533+
$this->tokenManager,
534+
$this->config,
535+
static fn(): ExceptionRendererInterface => $renderer,
536+
);
537+
538+
$request = $this->createRequest(
539+
'POST',
540+
'/submit',
541+
headers: ['Accept' => 'text/html'],
542+
);
543+
$response = $middleware->process($request, $this->successHandler());
544+
545+
self::assertSame(ResponseStatus::Forbidden->value, $response->getStatusCode());
546+
self::assertStringContainsString('text/html', $response->getHeaderLine('Content-Type'));
547+
self::assertSame(
548+
'<main data-themed="1">403: CSRF token is missing</main>',
549+
(string) $response->getBody(),
550+
);
551+
}
515552
}

‎tools/api/public-api.snapshot.json‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9133,6 +9133,18 @@
91339133
"since": "1.0.0",
91349134
"methods": [],
91359135
"signatures": {
9136+
"activate": {
9137+
"params": [
9138+
"Pulsar\\Config\\Environment $environment"
9139+
],
9140+
"return": "void",
9141+
"static": true
9142+
},
9143+
"active": {
9144+
"params": [],
9145+
"return": "?Pulsar\\Config\\Environment",
9146+
"static": true
9147+
},
91369148
"all": {
91379149
"params": [],
91389150
"return": "array",
@@ -9169,13 +9181,26 @@
91699181
"return": "Pulsar\\Config\\Environment",
91709182
"static": true
91719183
},
9184+
"read": {
9185+
"params": [
9186+
"string $key",
9187+
"string $default"
9188+
],
9189+
"return": "string",
9190+
"static": true
9191+
},
91729192
"require": {
91739193
"params": [
91749194
"string $key"
91759195
],
91769196
"return": "string",
91779197
"static": false
91789198
},
9199+
"resetActive": {
9200+
"params": [],
9201+
"return": "void",
9202+
"static": true
9203+
},
91799204
"resolveMode": {
91809205
"params": [],
91819206
"return": "Pulsar\\Config\\EnvironmentMode",
@@ -43574,7 +43599,8 @@
4357443599
"__construct": {
4357543600
"params": [
4357643601
"Pulsar\\Security\\Csrf\\CsrfTokenManagerInterface $tokenManager",
43577-
"Pulsar\\Config\\CsrfConfig $config"
43602+
"Pulsar\\Config\\CsrfConfig $config",
43603+
"?Closure $errorRendererResolver"
4357843604
],
4357943605
"return": null,
4358043606
"static": false
@@ -56172,11 +56198,13 @@
5617256198
"Pulsar\\Mail\\Transport\\Config\\SendgridTransportConfig",
5617356199
"Pulsar\\Mail\\Transport\\Config\\SesTransportConfig",
5617456200
"Pulsar\\Mail\\Transport\\Config\\SmtpTransportConfig",
56201+
"Pulsar\\Mail\\Transport\\CurlMailHttpClient",
5617556202
"Pulsar\\Mail\\Transport\\LogTransport",
5617656203
"Pulsar\\Mail\\Transport\\MailHttpClientInterface",
5617756204
"Pulsar\\Mail\\Transport\\MailHttpResponse",
5617856205
"Pulsar\\Mail\\Transport\\MailgunTransport",
5617956206
"Pulsar\\Mail\\Transport\\PostmarkTransport",
56207+
"Pulsar\\Mail\\Transport\\Psr18MailHttpClient",
5618056208
"Pulsar\\Mail\\Transport\\SendgridTransport",
5618156209
"Pulsar\\Mail\\Transport\\SesTransport",
5618256210
"Pulsar\\Mail\\Transport\\SmtpTransport",

0 commit comments

Comments
 (0)