Answer a wrong parameter type with a bad request, not a 500 - #832
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
{"x": 123}/true/1.5/[…]getParamIntFilter::getInt(int|string)TypeErrorgetParamStringFilter::getString(?string)TypeErrorgetParamRaw?stringreturnTypeErrorgetParamArrayWrong parameters✓So
{"name": 123}onPOST /api/v1/categoriesanswered:{"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 oneplace 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 typeis 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.5becomes the id15—FILTER_SANITIZE_NUMBER_INTdrops the point — and how a boolean becomes somebody's name. A JSONclient that sends
truefor a flag read withgetParamIntgets a 400 rather than a 500; makingthose 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, eachasserting both halves: the status is 400 and the body carries the API's own refusal, with no
/var/wwwpath and noTypeErrorin it. A well-typed call is the control, so none of the refusalscan be satisfied by an endpoint that stopped accepting anything.
Mutation-checked: reverting
Api.phpfails exactly the eleven type refusals and leaves the arrayreader and the control passing.
PHPStan level 6 and PHPCS clean.
Also
CLAUDE.mdgains "One of the siblings already gets it right" — the reusable part of this one isthat the correct implementation was already in the same class, and reading it settled the design.