Answer a wrong parameter type with a bad request, not a 500

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

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

tests/Integration/Infrastructure/Adapter/In/Api/Controllers/ParameterTypesTest.php

@@ -0,0 +1,185 @@
+<?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\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<string, array{mixed}>
+     */
+    public static function nonStringProvider(): array
+    {
+        return [
+            'int' => [123],
+            'float' => [1.5],
+            'bool' => [true],
+            'array' => [['a', 'b']],
+        ];
+    }
+
+    /**
+     * @return array<string, array{mixed}>
+     */
+    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));
+    }
+}