Skip to content

Answer a wrong parameter type with a bad request, not a 500 - #832

Merged
blaipr merged 1 commit into
mainfrom
fix/a-wrong-parameter-type-is-a-bad-request
Aug 20, 2026
Merged

Answer a wrong parameter type with a bad request, not a 500#832
blaipr merged 1 commit into
mainfrom
fix/a-wrong-parameter-type-is-a-bad-request

Conversation

@blaipr

@blaipr blaipr commented Aug 20, 2026

Copy link
Copy Markdown
Member

The defect

The API's transport is JSON, so the client decides each parameter's type. Three of the four readers
handed whatever arrived straight to something typed:

reader hands the value to {"x": 123} / true / 1.5 / […]
getParamInt Filter::getInt(int|string) 500 TypeError
getParamString Filter::getString(?string) 500 TypeError
getParamRaw its own ?string return 500 TypeError
getParamArray — checks first 400 Wrong parameters

So {"name": 123} on POST /api/v1/categories answered:

{"error":{"message":"SP\\Domain\\Common\\Providers\\Filter::getString(): Argument #1 ($value) must
be of type ?string, int given, called in /var/www/html/src/Application/Api/Services/Api.php on line 370"}}

Two problems in one response: a 500 for input that is merely malformed, and the class, the method and
the server's absolute path handed to the caller.

Every string and integer parameter on every endpoint could be made to do it — getParam() is the one
place they all come through. 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.

The fix

getParamArray() was already right, so this is its refusal, shared: each reader checks that the type
is the one the endpoint declares, and throws the same Wrong parameters / 400 with the help hint.
getParamArray() now defers 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. A JSON
client that sends true for a flag read with getParamInt gets a 400 rather than a 500; making
those readers accept booleans is a feature decision, not part of fixing a crash, so it is left alone.

Test

ParameterTypesTest — 13 tests through the real API dispatch, one per reader per wrong type, each
asserting both halves: the status is 400 and the body carries the API's own refusal, with no
/var/www path and no TypeError in it. A well-typed call is the control, so none of the refusals
can be satisfied by an endpoint that stopped accepting anything.

Mutation-checked: reverting Api.php fails exactly the eleven type refusals and leaves the array
reader and the control passing.

OK (3994 tests, 36797 assertions)   unit
OK (994 tests, 2979 assertions)    integration

PHPStan level 6 and PHPCS clean.

Also

CLAUDE.md gains "One of the siblings already gets it right" — the reusable part of this one is
that the correct implementation was already in the same class, and reading it settled the design.

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.
@blaipr
blaipr merged commit 074ae5c into main Aug 20, 2026
8 checks passed
@blaipr
blaipr deleted the fix/a-wrong-parameter-type-is-a-bad-request branch August 20, 2026 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant