Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/Application/Api/Services/Api.php
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down
18 changes: 14 additions & 4 deletions src/Domain/Common/Providers/Filter.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, array{mixed}>
*/
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.
Expand Down
82 changes: 82 additions & 0 deletions tests/Unit/Domain/Common/Providers/FilterTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
<?php

declare(strict_types=1);
/*
* sysPass
*
* @author nuxsmin
* @link https://syspass.org
* @copyright 2012-2024, Rubén Domínguez nuxsmin@$syspass.org
*
* This file is part of sysPass.
*
* sysPass is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* sysPass is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with sysPass. If not, see <http://www.gnu.org/licenses/>.
*/

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<string, array{mixed}>
*/
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]));
}
}
Loading