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)); + } +}