flarum / issue-archive

0 stars 0 forks source link

[Testing] Default requested parsedBody is null instead of [], 500 error on DELETE /api/discussions #91

Open clarkwinkelmann opened 3 years ago

clarkwinkelmann commented 3 years ago

Bug Report

Current Behavior When performing a non-JSON request with the integration testing package, $request->getParsedBody() is null whereas it's [] (empty array) when used via the web. 500 errors are thrown from Flarum code because Flarum itself cannot handle null values in some contexts.

Steps to Reproduce Write integration test:

<?php

namespace Kilowhat\Audit\Tests\integration;

use Carbon\Carbon;

class NullParsedBodyTest extends \Flarum\Testing\integration\TestCase
{
    public function setUp(): void
    {
        parent::setUp();

        $this->prepareDatabase([
            'discussions' => [
                ['id' => 1, 'title' => 'A', 'created_at' => Carbon::now(), 'user_id' => 1],
            ],
        ]);
    }

    public function test()
    {
        $response = $this->send($this->request('DELETE', '/api/discussions/1', [
            'authenticatedAs' => 1,
        ]));

        $this->assertEquals(204, $response->getStatusCode());
    }
}

Output: Failed asserting that 500 matches expected 204.

Error log:

[2021-06-02 16:32:47] flarum.ERROR: TypeError: Argument 3 passed to Flarum\Discussion\Command\DeleteDiscussion::__construct() must be of the type array, null given, called in /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Api/Controller/DeleteDiscussionController.php on line 43 and defined in /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Discussion/Command/DeleteDiscussion.php:44
Stack trace:
#0 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Api/Controller/DeleteDiscussionController.php(43): Flarum\Discussion\Command\DeleteDiscussion->__construct()
flarum/framework#1 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Api/Controller/AbstractDeleteController.php(24): Flarum\Api\Controller\DeleteDiscussionController->delete()
flarum/framework#2 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Http/RouteHandlerFactory.php(41): Flarum\Api\Controller\AbstractDeleteController->handle()
flarum/framework#3 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Http/Middleware/ExecuteRoute.php(27): Flarum\Http\RouteHandlerFactory->Flarum\Http\{closure}()
flarum/framework#4 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Next.php(61): Flarum\Http\Middleware\ExecuteRoute->process()
flarum/framework#5 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Api/Middleware/ThrottleApi.php(33): Laminas\Stratigility\Next->handle()
flarum/framework#6 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Next.php(61): Flarum\Api\Middleware\ThrottleApi->process()
flarum/framework#7 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Http/Middleware/CheckCsrfToken.php(40): Laminas\Stratigility\Next->handle()
flarum/framework#8 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Next.php(61): Flarum\Http\Middleware\CheckCsrfToken->process()
flarum/framework#9 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Http/Middleware/ResolveRoute.php(67): Laminas\Stratigility\Next->handle()
flarum/framework#10 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Next.php(61): Flarum\Http\Middleware\ResolveRoute->process()
flarum/framework#11 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Http/Middleware/SetLocale.php(51): Laminas\Stratigility\Next->handle()
flarum/framework#12 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Next.php(61): Flarum\Http\Middleware\SetLocale->process()
flarum/framework#13 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Http/Middleware/AuthenticateWithHeader.php(56): Laminas\Stratigility\Next->handle()
flarum/framework#14 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Next.php(61): Flarum\Http\Middleware\AuthenticateWithHeader->process()
flarum/framework#15 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Http/Middleware/AuthenticateWithSession.php(31): Laminas\Stratigility\Next->handle()
flarum/framework#16 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Next.php(61): Flarum\Http\Middleware\AuthenticateWithSession->process()
flarum/framework#17 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Http/Middleware/RememberFromCookie.php(52): Laminas\Stratigility\Next->handle()
flarum/framework#18 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Next.php(61): Flarum\Http\Middleware\RememberFromCookie->process()
flarum/framework#19 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Http/Middleware/StartSession.php(61): Laminas\Stratigility\Next->handle()
flarum/framework#20 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Next.php(61): Flarum\Http\Middleware\StartSession->process()
flarum/framework#21 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Api/Middleware/FakeHttpMethods.php(29): Laminas\Stratigility\Next->handle()
flarum/framework#22 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Next.php(61): Flarum\Api\Middleware\FakeHttpMethods->process()
flarum/framework#23 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Http/Middleware/ParseJsonBody.php(28): Laminas\Stratigility\Next->handle()
flarum/framework#24 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Next.php(61): Flarum\Http\Middleware\ParseJsonBody->process()
flarum/framework#25 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Http/Middleware/HandleErrors.php(57): Laminas\Stratigility\Next->handle()
flarum/framework#26 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Next.php(61): Flarum\Http\Middleware\HandleErrors->process()
flarum/framework#27 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Http/Middleware/InjectActorReference.php(25): Laminas\Stratigility\Next->handle()
flarum/framework#28 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Next.php(61): Flarum\Http\Middleware\InjectActorReference->process()
flarum/framework#29 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/MiddlewarePipe.php(84): Laminas\Stratigility\Next->handle()
flarum/framework#30 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/middlewares/request-handler/src/RequestHandler.php(84): Laminas\Stratigility\MiddlewarePipe->process()
flarum/framework#31 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Next.php(61): Middlewares\RequestHandler->process()
flarum/framework#32 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/middlewares/base-path-router/src/BasePathRouter.php(101): Laminas\Stratigility\Next->handle()
flarum/framework#33 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Next.php(61): Middlewares\BasePathRouter->process()
flarum/framework#34 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Middleware/OriginalMessages.php(42): Laminas\Stratigility\Next->handle()
flarum/framework#35 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Next.php(61): Laminas\Stratigility\Middleware\OriginalMessages->process()
flarum/framework#36 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/middlewares/base-path/src/BasePath.php(73): Laminas\Stratigility\Next->handle()
flarum/framework#37 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Next.php(61): Middlewares\BasePath->process()
flarum/framework#38 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/core/src/Http/Middleware/ProcessIp.php(24): Laminas\Stratigility\Next->handle()
flarum/framework#39 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/Next.php(61): Flarum\Http\Middleware\ProcessIp->process()
flarum/framework#40 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/MiddlewarePipe.php(84): Laminas\Stratigility\Next->handle()
flarum/framework#41 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/laminas/laminas-stratigility/src/MiddlewarePipe.php(73): Laminas\Stratigility\MiddlewarePipe->process()
flarum/framework#42 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/flarum/testing/src/integration/TestCase.php(238): Laminas\Stratigility\MiddlewarePipe->handle()
flarum/framework#43 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/tests/integration/NullParsedBodyTest.php(23): Flarum\Testing\integration\TestCase->send()
flarum/framework#44 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/phpunit/phpunit/src/Framework/TestCase.php(1526): Kilowhat\Audit\Tests\integration\NullParsedBodyTest->test()
flarum/framework#45 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/phpunit/phpunit/src/Framework/TestCase.php(1132): PHPUnit\Framework\TestCase->runTest()
flarum/framework#46 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/phpunit/phpunit/src/Framework/TestResult.php(722): PHPUnit\Framework\TestCase->runBare()
flarum/framework#47 /home/clark/Projects/flarum-1.0/workbench/flarum-ext-audit/vendor/phpunit/phpunit/src/Framework/TestCase.php(884): PHPUnit\Framework\TestResult->run()
flarum/framework#48 Standard input code(315): PHPUnit\Framework\TestCase->run()
flarum/framework#49 Standard input code(574): __phpunit_run_isolated_test()
flarum/framework#50 {main}  

Expected Behavior It should be possible to make a DELETE request to /api/discussions/<id> without specifying a content type in the integration test, just like it's possible to do from the web.

Environment

Possible Solution More parameters should be passed to the ServerRequest constructor in https://github.com/flarum/testing/blob/main/src/integration/TestCase.php#L267 to provide a default $parsedBody value of [] instead of null.

I think it would also make sense to either allow null in https://github.com/flarum/core/blob/master/src/Discussion/Command/DeleteDiscussion.php#L44 or add ?? [] in https://github.com/flarum/core/blob/master/src/Api/Controller/DeleteDiscussionController.php#L43 to make sure a null parsed body doesn't crash the request with a 500 error, since an extension could also alter the parsedBody attribute. The same probably also applies to other DELETE endpoints where we pass the parsedBody value directly to an array type-hint.

Same issue affects https://github.com/flarum/flags/blob/master/src/Api/Controller/DeleteFlagsController.php#L40

Additional Context

We default to an empty array in https://github.com/flarum/core/blob/master/src/Http/Middleware/ParseJsonBody.php but the reason an empty array is also present for non-JSON requests is because inside of Lamina's ServerRequestFactory class the default value for parsedBody is $body ?: $_POST (line 74).

In the testing package we build the request from scratch so we never default to $_POST for parsedBody.

A workaround for the test package is to provide 'json' => [], in the request options, this forces Flarum to parse the body as JSON and default to an empty array.