From 30a654729b08eac79126e26343e2e6af7e00cb17 Mon Sep 17 00:00:00 2001 From: blaipr Date: Thu, 20 Aug 2026 20:58:51 +0200 Subject: [PATCH] Answer a wrong parameter type with a bad request, not a 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The API's transport is JSON, so the caller decides each parameter's type, and three of the four readers handed whatever arrived straight to something typed: Filter::getInt(int|string), Filter::getString(?string), and getParamRaw()'s own ?string return. A body carrying {"name": 123}, {"userGroupId": true} or an array where a scalar belongs was therefore an uncaught TypeError, which escaped as a 500 with the class, the method and the server's absolute path in the response. Every string and integer parameter on every endpoint could be made to do it, and the same value sent in a query string was fine, because everything arrives as a string there — so the transport decided whether a request crashed. getParamArray() was already right: it checks the type and answers Wrong parameters with a 400 and the help hint. This is that refusal, shared by all four, with getParamArray() deferring to the shared copy rather than keeping its own. Nothing is coerced, deliberately. Converting silently is how 1.5 becomes the id 15 — FILTER_SANITIZE_NUMBER_INT drops the point — and how a boolean becomes somebody's name. Making the integer reader accept JSON booleans for flag parameters is a feature decision rather than part of fixing a crash, so it is left alone. ParameterTypesTest covers each reader against each wrong type through the real dispatch, asserting both that the status is a bad request and that the body carries the API's own refusal rather than a PHP error with a server path in it. Reverting the change fails exactly the eleven type refusals and leaves the array reader and the well-typed control passing. --- CLAUDE.md | 12 ++ src/Application/Api/Services/Api.php | 44 ++++- .../In/Api/Controllers/ParameterTypesTest.php | 185 ++++++++++++++++++ 3 files changed, 235 insertions(+), 6 deletions(-) create mode 100644 tests/Integration/Infrastructure/Adapter/In/Api/Controllers/ParameterTypesTest.php diff --git a/CLAUDE.md b/CLAUDE.md index 2189fc606..df9b45902 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -422,6 +422,18 @@ put the check somewhere both doors reach — a shared base method, or the servic than a second copy that will drift; and move the constant it compares against out of whichever one owned it privately. +**One of the siblings already gets it right.** Where a small family of near-identical methods does +the same job for different types, the correct one is usually already there, and reading it settles +the design before you invent one. The API's four parameter readers are the case: `getParamArray()` +checked the type it had been sent and answered `Wrong parameters` with a 400, while `getParamInt()`, +`getParamString()` and `getParamRaw()` each handed the value straight to something typed — so +`{"name": 123}` was a `TypeError` escaping as a 500 with the class, the method and the server's +absolute path in the body, on every string and integer parameter of every endpoint. The same value +in a query string was fine, since everything arrives as a string there. + +**Find the sibling that is right before deciding what right means** — and once the rule is shared, +have the one that already had it defer to the shared copy, or there are two definitions again. + **The wiring, not the code.** php-di skips a constructor parameter that has a default *even when the container has a binding for its type*, silently. `Init::$sessionKeyService` was null that way, so `reKey()` — and the `session_regenerate_id()` inside it — never ran, and session identifiers were diff --git a/src/Application/Api/Services/Api.php b/src/Application/Api/Services/Api.php index d88ba356d..615f619e0 100644 --- a/src/Application/Api/Services/Api.php +++ b/src/Application/Api/Services/Api.php @@ -345,6 +345,31 @@ private function requireInitialized(): void } } + /** + * A parameter the caller sent with the wrong JSON type is a bad request, not a crash. + * + * The readers below hand their value straight to a typed function — `Filter::getInt(int|string)`, + * `Filter::getString(?string)`, or `getParamRaw()`'s own `?string` return — so a JSON body + * carrying `{"name": 123}`, `{"userGroupId": true}` or an array where a scalar belongs raised a + * `TypeError` that escaped as a 500 carrying the class, the method and the server's absolute + * path. Every string and integer parameter on every endpoint could be made to do it, and the + * same value sent through a query string works, because everything arrives as a string there. + * + * `getParamArray()` already answered this correctly, and this is its refusal, shared: the type + * has to be the one the endpoint declares. Nothing is coerced — converting silently is how + * `1.5` becomes the id `15` (`FILTER_SANITIZE_NUMBER_INT` drops the point) and how a boolean + * becomes somebody's name. + */ + private function wrongParameterType(): ServiceException + { + return new ServiceException( + __u('Wrong parameters'), + SPException::ERROR, + $this->getHelpHint($this->apiRequest->getMethod()), + Code::BAD_REQUEST->value + ); + } + /** * @throws ServiceException */ @@ -353,6 +378,10 @@ public function getParamInt(string $param, bool $required = false, $default = nu $value = $this->getParam($param, $required, $default); if (null !== $value) { + if (!is_int($value) && !is_string($value)) { + throw $this->wrongParameterType(); + } + return Filter::getInt($value); } @@ -367,6 +396,10 @@ public function getParamString(string $param, bool $required = false, $default = $value = $this->getParam($param, $required, $default); if (null !== $value) { + if (!is_string($value)) { + throw $this->wrongParameterType(); + } + return Filter::getString($value); } @@ -383,12 +416,7 @@ public function getParamArray(string $param, bool $required = false, $default = if (null !== $value) { if (!is_array($value)) { - throw new ServiceException( - __u('Wrong parameters'), - SPException::ERROR, - $this->getHelpHint($this->apiRequest->getMethod()), - Code::BAD_REQUEST->value - ); + throw $this->wrongParameterType(); } return Filter::getArray($value); @@ -405,6 +433,10 @@ public function getParamRaw(string $param, bool $required = false, $default = nu $value = $this->getParam($param, $required, $default); if (null !== $value) { + if (!is_string($value)) { + throw $this->wrongParameterType(); + } + return $value; } diff --git a/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/ParameterTypesTest.php b/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/ParameterTypesTest.php new file mode 100644 index 000000000..904b955d6 --- /dev/null +++ b/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/ParameterTypesTest.php @@ -0,0 +1,185 @@ +. + */ + +namespace SP\Tests\Integration\Infrastructure\Adapter\In\Api\Controllers; + +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\Attributes\Test; +use SP\Domain\Core\Acl\AclActionsInterface; +use SP\Tests\Integration\Infrastructure\Adapter\In\Api\ApiTestCase; +use stdClass; + +/** + * A parameter sent with the wrong JSON type is a bad request, on every reader. + * + * The API's transport is JSON, so a client decides each parameter's type, and three of the four + * readers handed whatever arrived straight to a typed function: `Filter::getInt(int|string)`, + * `Filter::getString(?string)`, and `getParamRaw()`'s own `?string` return. `{"name": 123}` was + * therefore an uncaught `TypeError` — HTTP 500, with the class, the method and the server's + * absolute path in the body — and every string and integer parameter on every endpoint could be + * made to do it. The same values sent through a query string were fine, since everything arrives + * as a string there, so the transport decided whether a request crashed. + * + * `getParamArray()` always answered this correctly; these assert the other three now do too. + */ +#[Group('integration')] +class ParameterTypesTest extends ApiTestCase +{ + /** + * @return array + */ + public static function nonStringProvider(): array + { + return [ + 'int' => [123], + 'float' => [1.5], + 'bool' => [true], + 'array' => [['a', 'b']], + ]; + } + + /** + * @return array + */ + public static function nonIntProvider(): array + { + return [ + 'bool' => [true], + 'float' => [1.5], + 'array' => [[1]], + ]; + } + + /** + * `name` is read with getParamString(). + */ + #[Test] + #[DataProvider('nonStringProvider')] + public function aStringParameterOfTheWrongTypeIsRefused(mixed $value): void + { + $r = $this->callApi(AclActionsInterface::CATEGORY_CREATE, ['name' => $value]); + + $this->assertBadRequest($r); + } + + /** + * `userGroupId` is read with getParamInt(). Every other required parameter is supplied, so the + * type is the only thing under test — without that the refusal would come from the missing + * `pass` and this would pass with the bug still in place. + */ + #[Test] + #[DataProvider('nonIntProvider')] + public function anIntegerParameterOfTheWrongTypeIsRefused(mixed $value): void + { + $r = $this->callApi( + AclActionsInterface::USER_CREATE, + [ + 'name' => 'a user', + 'login' => 'a_user', + 'pass' => 'a-provisioned-password', + 'userGroupId' => $value, + 'userProfileId' => 1, + ] + ); + + $this->assertBadRequest($r); + } + + /** + * `password` is read with getParamRaw(), which returned the value unconverted from a `?string` + * method — so this one failed on the way out rather than on the way in. + */ + #[Test] + #[DataProvider('nonStringProvider')] + public function aRawParameterOfTheWrongTypeIsRefused(mixed $value): void + { + $r = $this->callApi( + AclActionsInterface::AUTHTOKEN_CREATE, + [ + 'userId' => 1, + 'actionId' => AclActionsInterface::CATEGORY_SEARCH, + 'password' => $value, + ] + ); + + $this->assertBadRequest($r); + } + + /** + * The reader that was always right, kept here so the rule is asserted as one rule. + */ + #[Test] + public function anArrayParameterOfTheWrongTypeIsRefused(): void + { + $r = $this->callApi( + AclActionsInterface::ACCOUNT_CREATE, + [ + 'name' => 'an account', + 'categoryId' => 1, + 'clientId' => 1, + 'pass' => 'a-password', + 'tagsId' => 'not-an-array', + ] + ); + + $this->assertBadRequest($r); + } + + /** + * The control. Every refusal above would be satisfied by an endpoint that had simply stopped + * accepting anything, so the same call with the right types has to still work. + */ + #[Test] + public function wellTypedParametersAreStillAccepted(): void + { + $r = $this->callApi( + AclActionsInterface::ACCOUNT_CREATE, + [ + 'name' => 'an account', + 'categoryId' => 1, + 'clientId' => 1, + 'pass' => 'a-password', + 'tagsId' => [], + ] + ); + + $this->assertSame(201, $r->status); + $this->assertSame('Account created', $r->body->message); + } + + /** + * Both halves: the status is a bad request, and the body carries the API's own refusal rather + * than a PHP error. Asserting only the status would pass on a 400 that still leaked the path. + */ + private function assertBadRequest(stdClass $response): void + { + $this->assertSame(400, $response->status); + $this->assertInstanceOf(stdClass::class, $response->body->error ?? null); + $this->assertSame('Wrong parameters', $response->body->error->message); + $this->assertStringNotContainsString('/var/www', json_encode($response->body)); + $this->assertStringNotContainsString('TypeError', json_encode($response->body)); + } +}