diff --git a/src/Application/Api/Services/Api.php b/src/Application/Api/Services/Api.php index 615f619e0..111ae84ef 100644 --- a/src/Application/Api/Services/Api.php +++ b/src/Application/Api/Services/Api.php @@ -415,7 +415,14 @@ public function getParamArray(string $param, bool $required = false, $default = $value = $this->getParam($param, $required, $default); if (null !== $value) { - if (!is_array($value)) { + // The elements as well as the container. Checking only `is_array()` let `[true]` or + // `[[1]]` through to `Filter::getArray()`, which threw a `TypeError` — the 500 with a + // class name and a server path in the body that the three scalar readers above were + // already fixed to refuse with a 400. An element is an id or a value, so the same + // scalars those readers accept are the ones accepted here. + if (!is_array($value) + || array_filter($value, static fn($item) => !is_int($item) && !is_string($item) && $item !== null) + ) { throw $this->wrongParameterType(); } diff --git a/src/Domain/Common/Providers/Filter.php b/src/Domain/Common/Providers/Filter.php index 3499c92f1..c4c20b7e4 100644 --- a/src/Domain/Common/Providers/Filter.php +++ b/src/Domain/Common/Providers/Filter.php @@ -54,10 +54,20 @@ public static function getArray(array $array): array { return array_map( static function ($value) { - if ($value !== null) { - return is_numeric($value) - ? Filter::getInt($value) - : Filter::getString($value); + // Decided by type rather than by `is_numeric()` alone. `getInt()` takes + // `int|string` and `getString()` takes `?string`, under strict types, so anything + // else — a bool, a float, a nested array, an object — used to reach one of them and + // throw a `TypeError`. That is not a hypothetical shape: a JSON body can carry + // `[true]` or `[1.5]`, and a form field named `x[a][]` makes one element an array. + // Nothing in between caught it, so it surfaced as a 500 whose body named the class, + // the method and the server's absolute path. An element this cannot represent is + // answered as null, the same as a missing one. + if (is_int($value) || (is_string($value) && is_numeric($value))) { + return Filter::getInt($value); + } + + if (is_string($value)) { + return Filter::getString($value); } return null; diff --git a/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/ParameterTypesTest.php b/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/ParameterTypesTest.php index 904b955d6..e3e976d24 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/ParameterTypesTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/ParameterTypesTest.php @@ -148,6 +148,46 @@ public function anArrayParameterOfTheWrongTypeIsRefused(): void $this->assertBadRequest($r); } + /** + * An array whose *elements* are the wrong type is refused the same way. + * + * `getParamArray()` checked only `is_array()`, so `[true]`, `[1.5]` or `[[1]]` reached + * `Filter::getArray()`, which handed each element to `getInt(int|string)` or + * `getString(?string)` under strict types. That threw a `TypeError`, and the catch-all answered + * it as a 500 with the class, the method and the server's absolute path in the body — the exact + * leak the scalar readers above were fixed for, on the one reader this test called "always + * right". It was right about the container and never looked inside it. + * + * @return array + */ + public static function arrayWithABadElementProvider(): array + { + return [ + 'a bool' => [[true]], + 'a float' => [[1.5]], + 'a nested array' => [[[1]]], + 'a good id beside a bad one' => [[1, false]], + ]; + } + + #[Test] + #[DataProvider('arrayWithABadElementProvider')] + public function anArrayParameterWithABadElementIsRefused(array $tagsId): void + { + $r = $this->callApi( + AclActionsInterface::ACCOUNT_CREATE, + [ + 'name' => 'an account', + 'categoryId' => 1, + 'clientId' => 1, + 'pass' => 'a-password', + 'tagsId' => $tagsId, + ] + ); + + $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. diff --git a/tests/Unit/Domain/Common/Providers/FilterTest.php b/tests/Unit/Domain/Common/Providers/FilterTest.php new file mode 100644 index 000000000..791dc09ea --- /dev/null +++ b/tests/Unit/Domain/Common/Providers/FilterTest.php @@ -0,0 +1,82 @@ +. + */ + +namespace SP\Tests\Unit\Domain\Common\Providers; + +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\TestCase; +use SP\Domain\Common\Providers\Filter; + +/** + * `Filter::getArray()` is where both doors meet for an array parameter: the API's + * `getParamArray()` and the web's `Request::analyzeArray()` both hand their value to it. + * + * It chose a filter per element by `is_numeric()` alone and passed the element straight on, and + * under strict types `getInt()` accepts only `int|string` and `getString()` only `?string`. So a + * bool, a float, a nested array or an object threw a `TypeError` that nothing caught, and the + * request ended as a 500 naming the class, the method and the server's absolute path. From the web + * that needs no more than a form field named `x[a][]`, which makes one element an array; the web's + * `analyzeArray()` reads through `InputBag::all()`, which skips Symfony's scalar check. + */ +#[Group('unitary')] +class FilterTest extends TestCase +{ + /** + * @return array + */ + public static function unrepresentableElementProvider(): array + { + return [ + 'a bool' => [true], + 'a float' => [1.5], + 'a nested array' => [[1, 2]], + 'an object' => [new \stdClass()], + ]; + } + + /** + * An element the filters cannot represent is answered as null, like a missing one, rather than + * throwing. + */ + #[Test] + #[DataProvider('unrepresentableElementProvider')] + public function anElementItCannotRepresentIsNull(mixed $element): void + { + self::assertSame([null], Filter::getArray([$element])); + } + + /** + * The control: the elements it was written for still come through, as the type each filter + * gives them. + */ + #[Test] + public function idsAndTextStillComeThrough(): void + { + self::assertSame([5, 7, 'a tag', null], Filter::getArray([5, '7', 'a tag', null])); + } +}