Skip to content

Commit b28610c

Browse files
dsevillamartinStyleCIBotimorland
authored
[2.x] Implement content negotiation for forum error handling (#4677)
* Implement content negotiation for forum error handling * Apply fixes from StyleCI * improve content type negotiation and HTML request detection * Apply fixes from StyleCI * Pass debug mode to JSON formatter for content negotiation in forum * Harden content negotiation against malformed Accept headers + add tests getPreferredContentType() runs inside the forum error handler, so it must never throw. Two ways it could: - Mimeparse::bestMatch() returns null when the Accept header matches none of the offered types (e.g. "application/xml"), violating the ": string" return type with a TypeError. - A malformed media range (e.g. one without a subtype) makes bestMatch() emit a warning and throw UnexpectedValueException. Both now fall back to an empty string (treated as "not an API request"), so a hostile or unusual Accept header can't turn an error response into a second, fatal error. Add coverage: - RequestUtilTest: isApiRequest/isHtmlRequest/getPreferredContentType across JSON:API, JSON, */*, empty, browser, unrelated and malformed headers. - ContentNegotiationFormatterTest: API requests format as JSON, browser requests as HTML. - ErrorNegotiationTest: forum errors are negotiated through the full stack (JSON for */* — the #3850 regression — and HTML for browser requests). --------- Co-authored-by: StyleCI Bot <bot@styleci.io> Co-authored-by: IanM <ian@morland.me>
1 parent dd05fbc commit b28610c

8 files changed

Lines changed: 360 additions & 14 deletions

File tree

composer.json

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -116,13 +116,14 @@
116116
"require": {
117117
"php": "^8.3",
118118
"ext-json": "*",
119-
"fortawesome/font-awesome": "^7.0",
119+
"bitworking/mimeparse": "^2.3",
120120
"composer/composer": "^2.7",
121121
"dflydev/fig-cookies": "^3.0",
122122
"doctrine/dbal": "^3.6.2",
123123
"dragonmantank/cron-expression": "^3.3",
124124
"fakerphp/faker": "^1.9.1",
125125
"flarum/json-api-server": "^0.1.0",
126+
"fortawesome/font-awesome": "^7.0",
126127
"franzl/whoops-middleware": "2.0",
127128
"guzzlehttp/guzzle": "*",
128129
"illuminate/bus": "^13.0",
@@ -156,6 +157,7 @@
156157
"nelexa/zip": "^4.0.2",
157158
"nesbot/carbon": "^3.0",
158159
"nikic/fast-route": "^1.3",
160+
"plesk/ratchetphp": "v1.0.4",
159161
"psr/http-message": "^1.1",
160162
"psr/http-server-handler": "^1.0.2",
161163
"psr/http-server-middleware": "^1.0.2",
@@ -172,8 +174,7 @@
172174
"symfony/postmark-mailer": "^7.0",
173175
"symfony/translation": "^7.0",
174176
"symfony/yaml": "^7.0",
175-
"wikimedia/less.php": "^5.3",
176-
"plesk/ratchetphp": "v1.0.4"
177+
"wikimedia/less.php": "^5.3"
177178
},
178179
"require-dev": {
179180
"flarum/testing-tests": "*@dev",

framework/core/composer.json

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,13 @@
3737
},
3838
"require": {
3939
"php": "^8.3",
40-
"fortawesome/font-awesome": "^7.0",
40+
"bitworking/mimeparse": "^2.3",
4141
"dflydev/fig-cookies": "^3.0",
4242
"doctrine/dbal": "^3.6",
4343
"dragonmantank/cron-expression": "*",
4444
"fakerphp/faker": "^1.9.1",
45+
"flarum/json-api-server": "^0.1.0",
46+
"fortawesome/font-awesome": "^7.0",
4547
"franzl/whoops-middleware": "2.0",
4648
"guzzlehttp/guzzle": "^7.7",
4749
"illuminate/bus": "^13.0",
@@ -90,7 +92,6 @@
9092
"symfony/translation": "^7.0",
9193
"symfony/translation-contracts": "^2.5",
9294
"symfony/yaml": "^7.0",
93-
"flarum/json-api-server": "^0.1.0",
9495
"wikimedia/less.php": "^5.3"
9596
},
9697
"require-dev": {

framework/core/src/Forum/ForumServiceProvider.php

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313
use Flarum\Extension\Event\Enabled;
1414
use Flarum\Formatter\Formatter;
1515
use Flarum\Foundation\AbstractServiceProvider;
16+
use Flarum\Foundation\ErrorHandling\ContentNegotiationFormatter;
17+
use Flarum\Foundation\ErrorHandling\JsonApiFormatter;
1618
use Flarum\Foundation\ErrorHandling\Registry;
1719
use Flarum\Foundation\ErrorHandling\Reporter;
1820
use Flarum\Foundation\ErrorHandling\ViewFormatter;
@@ -80,9 +82,14 @@ public function register(): void
8082
});
8183

8284
$this->container->bind('flarum.forum.error_handler', function (Container $container) {
85+
$inDebugMode = $container['flarum.config']->inDebugMode();
86+
8387
return new HttpMiddleware\HandleErrors(
8488
$container->make(Registry::class),
85-
$container['flarum.config']->inDebugMode() ? $container->make(WhoopsFormatter::class) : $container->make(ViewFormatter::class),
89+
new ContentNegotiationFormatter(
90+
new JsonApiFormatter($inDebugMode),
91+
$inDebugMode ? $container->make(WhoopsFormatter::class) : $container->make(ViewFormatter::class),
92+
),
8693
$container->tagged(Reporter::class)
8794
);
8895
});
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
<?php
2+
3+
/*
4+
* This file is part of Flarum.
5+
*
6+
* For detailed copyright and license information, please view the
7+
* LICENSE file that was distributed with this source code.
8+
*/
9+
10+
namespace Flarum\Foundation\ErrorHandling;
11+
12+
use Flarum\Http\RequestUtil;
13+
use Psr\Http\Message\ResponseInterface as Response;
14+
use Psr\Http\Message\ServerRequestInterface as Request;
15+
16+
readonly class ContentNegotiationFormatter implements HttpFormatter
17+
{
18+
public function __construct(
19+
private HttpFormatter $jsonFormatter,
20+
private HttpFormatter $htmlFormatter,
21+
) {
22+
}
23+
24+
public function format(HandledError $error, Request $request): Response
25+
{
26+
/**
27+
* If this request is an API request, and we got an error,
28+
* return a JSON response instead of HTML.
29+
*/
30+
if (RequestUtil::isApiRequest($request)) {
31+
return $this->jsonFormatter->format($error, $request);
32+
}
33+
34+
return $this->htmlFormatter->format($error, $request);
35+
}
36+
}

framework/core/src/Http/RequestUtil.php

Lines changed: 41 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,27 +9,60 @@
99

1010
namespace Flarum\Http;
1111

12+
use Bitworking\Mimeparse;
1213
use Flarum\User\User;
13-
use Illuminate\Support\Str;
1414
use Psr\Http\Message\ServerRequestInterface as Request;
1515
use Tobyz\JsonApiServer\Exception\BadRequestException;
1616

1717
class RequestUtil
1818
{
19+
/**
20+
* Determine if the request is an API request. We do not manually set the `Accepts` header
21+
* for API requests, so we need to check the priority list to determine whether it is or not.
22+
* Normal browsing requests will have `text/html` as their preferred content type, while API will have `* / *`.
23+
*/
1924
public static function isApiRequest(Request $request): bool
2025
{
21-
return Str::contains(
22-
$request->getHeaderLine('Accept'),
23-
'application/vnd.api+json'
26+
$preferred = self::getPreferredContentType(
27+
$request,
28+
['application/vnd.api+json', 'application/json', 'text/html']
2429
);
30+
31+
return in_array($preferred, ['application/vnd.api+json', 'application/json'], true);
2532
}
2633

2734
public static function isHtmlRequest(Request $request): bool
2835
{
29-
return Str::contains(
30-
$request->getHeaderLine('Accept'),
31-
'text/html'
32-
);
36+
return ! self::isApiRequest($request);
37+
}
38+
39+
/**
40+
* Determine the client's preferred content type out of the given list.
41+
*
42+
* @param Request $request
43+
* @param array $types The content types to check against, in order of priority. The best match will be returned.
44+
* @return string The best matching type, or an empty string if none could be determined.
45+
*/
46+
public static function getPreferredContentType(Request $request, array $types): string
47+
{
48+
$accept = $request->getHeaderLine('Accept');
49+
50+
// We get some errors if $accept is empty, so we need to check for that and set it to */* if it is.
51+
if (! filled($accept)) {
52+
$accept = '*/*';
53+
}
54+
55+
// This is called from the error handler, so it must never throw or emit noise.
56+
// `bestMatch()` returns null when nothing matches, and on a malformed Accept
57+
// header (e.g. a media range without a subtype) it emits a warning and then
58+
// throws. The `@` swallows that warning and the catch swallows the exception;
59+
// in either case we fall back to an empty string, which callers treat as
60+
// "no preference" (i.e. not an API request).
61+
try {
62+
return @Mimeparse::bestMatch($types, $accept) ?? '';
63+
} catch (\UnexpectedValueException) {
64+
return '';
65+
}
3366
}
3467

3568
public static function getActor(Request $request): User
Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
<?php
2+
3+
/*
4+
* This file is part of Flarum.
5+
*
6+
* For detailed copyright and license information, please view the
7+
* LICENSE file that was distributed with this source code.
8+
*/
9+
10+
namespace Flarum\Tests\integration\forum;
11+
12+
use Flarum\Extend;
13+
use Flarum\Testing\integration\TestCase;
14+
use Illuminate\Database\Eloquent\ModelNotFoundException;
15+
use PHPUnit\Framework\Attributes\Test;
16+
use Psr\Http\Message\ResponseInterface;
17+
use Psr\Http\Message\ServerRequestInterface;
18+
use Psr\Http\Server\RequestHandlerInterface;
19+
20+
class ErrorNegotiationTest extends TestCase
21+
{
22+
protected function setUp(): void
23+
{
24+
parent::setUp();
25+
26+
// Register a forum route that always errors, so we can assert how the
27+
// forum error handler formats the response based on the Accept header
28+
// (the regression from #3850, where forum errors were always HTML).
29+
$this->extend(
30+
(new Extend\Routes('forum'))
31+
->get('/test-error-negotiation', 'test.error-negotiation', ErrorThrowingRoute::class)
32+
);
33+
}
34+
35+
private function error(?string $accept): ResponseInterface
36+
{
37+
$request = $this->request('GET', '/test-error-negotiation');
38+
39+
if ($accept !== null) {
40+
$request = $request->withHeader('Accept', $accept);
41+
}
42+
43+
return $this->send($request);
44+
}
45+
46+
#[Test]
47+
public function forum_errors_are_returned_as_json_for_api_requests(): void
48+
{
49+
$response = $this->error('application/vnd.api+json');
50+
51+
$this->assertEquals(404, $response->getStatusCode());
52+
$this->assertStringContainsString('json', $response->getHeaderLine('Content-Type'));
53+
54+
$body = json_decode((string) $response->getBody(), true);
55+
$this->assertIsArray($body);
56+
$this->assertArrayHasKey('errors', $body);
57+
}
58+
59+
#[Test]
60+
public function forum_errors_are_returned_as_json_for_xhr_requests_without_an_explicit_accept(): void
61+
{
62+
// Flarum's own XHR requests send `Accept: */*`. This is the regression from #3850:
63+
// such requests previously received an HTML error page instead of JSON.
64+
$response = $this->error('*/*');
65+
66+
$this->assertEquals(404, $response->getStatusCode());
67+
$this->assertStringContainsString('json', $response->getHeaderLine('Content-Type'));
68+
69+
$body = json_decode((string) $response->getBody(), true);
70+
$this->assertIsArray($body);
71+
$this->assertArrayHasKey('errors', $body);
72+
}
73+
74+
#[Test]
75+
public function forum_errors_are_returned_as_html_for_browser_requests(): void
76+
{
77+
$response = $this->error('text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8');
78+
79+
$this->assertEquals(404, $response->getStatusCode());
80+
$this->assertStringContainsString('html', $response->getHeaderLine('Content-Type'));
81+
$this->assertNull(json_decode((string) $response->getBody(), true));
82+
}
83+
}
84+
85+
class ErrorThrowingRoute implements RequestHandlerInterface
86+
{
87+
public function handle(ServerRequestInterface $request): ResponseInterface
88+
{
89+
throw (new ModelNotFoundException())->setModel('Test');
90+
}
91+
}
Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
<?php
2+
3+
/*
4+
* This file is part of Flarum.
5+
*
6+
* For detailed copyright and license information, please view the
7+
* LICENSE file that was distributed with this source code.
8+
*/
9+
10+
namespace Flarum\Tests\unit\Foundation\ErrorHandling;
11+
12+
use Exception;
13+
use Flarum\Foundation\ErrorHandling\ContentNegotiationFormatter;
14+
use Flarum\Foundation\ErrorHandling\HandledError;
15+
use Flarum\Foundation\ErrorHandling\HttpFormatter;
16+
use Flarum\Testing\unit\TestCase;
17+
use Laminas\Diactoros\ServerRequest;
18+
use Mockery as m;
19+
use PHPUnit\Framework\Attributes\Test;
20+
use Psr\Http\Message\ResponseInterface;
21+
use Psr\Http\Message\ServerRequestInterface;
22+
23+
class ContentNegotiationFormatterTest extends TestCase
24+
{
25+
private HandledError $error;
26+
private ResponseInterface $jsonResponse;
27+
private ResponseInterface $htmlResponse;
28+
private HttpFormatter $json;
29+
private HttpFormatter $html;
30+
31+
protected function setUp(): void
32+
{
33+
parent::setUp();
34+
35+
$this->error = new HandledError(new Exception(), 'unknown', 500);
36+
$this->jsonResponse = m::mock(ResponseInterface::class);
37+
$this->htmlResponse = m::mock(ResponseInterface::class);
38+
39+
$this->json = m::mock(HttpFormatter::class);
40+
$this->html = m::mock(HttpFormatter::class);
41+
}
42+
43+
private function formatter(): ContentNegotiationFormatter
44+
{
45+
return new ContentNegotiationFormatter($this->json, $this->html);
46+
}
47+
48+
private function requestWithAccept(string $accept): ServerRequestInterface
49+
{
50+
return (new ServerRequest([], [], '/', 'GET'))->withHeader('Accept', $accept);
51+
}
52+
53+
#[Test]
54+
public function api_requests_are_formatted_as_json(): void
55+
{
56+
$request = $this->requestWithAccept('application/vnd.api+json');
57+
58+
$this->json->shouldReceive('format')->once()->with($this->error, $request)->andReturn($this->jsonResponse);
59+
$this->html->shouldNotReceive('format');
60+
61+
$this->assertSame($this->jsonResponse, $this->formatter()->format($this->error, $request));
62+
}
63+
64+
#[Test]
65+
public function browser_requests_are_formatted_as_html(): void
66+
{
67+
$request = $this->requestWithAccept('text/html,application/xhtml+xml,*/*;q=0.8');
68+
69+
$this->html->shouldReceive('format')->once()->with($this->error, $request)->andReturn($this->htmlResponse);
70+
$this->json->shouldNotReceive('format');
71+
72+
$this->assertSame($this->htmlResponse, $this->formatter()->format($this->error, $request));
73+
}
74+
}

0 commit comments

Comments
 (0)