From e062a45f9abd617d496bd1b2390a82721e56611a Mon Sep 17 00:00:00 2001 From: Stefan Ries Date: Sat, 23 May 2026 20:01:06 +0200 Subject: [PATCH] fix: return 422 (not 500) for ValidationException on JSON API requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #6609, closes #6610 ## Bug 1 — Handler.php returns HTTP 500 for ValidationException (#6609) `ValidationException` has no `getStatusCode()` method — only a `$status` property (default 422). The existing handler checked `method_exists($exception, 'getStatusCode')`, which returns false, so the fallback of 500 was used for every JSON API validation failure. Fix: use `$exception->status` as the fallback for `ValidationException`. ## Bug 2 — timelineHome rejects empty max_id=/min_id= parameters (#6610) The Pixelfed Android app (and other Mastodon-compatible clients such as Tusky and Ivory) send `max_id=` (empty string) on the first home timeline request. The `ConvertEmptyStringsToNull` middleware converts this to null before validation. The rule `'sometimes|integer'` still validated the key (it is "present" as null) and the `integer` rule failed on null, throwing a `ValidationException`. This then triggered bug #6609, returning HTTP 500. Every other timeline method in `ApiV1Controller` already uses `'nullable|integer'` for `max_id`/`min_id`. This commit makes `timelineHome` consistent. ## Tests `tests/Feature/HomeTimelineTest.php` adds four regression tests: 1. Validates that a non-integer `max_id` returns 422 (not 500) — catches the Handler.php regression independently of the controller fix. 2. Validates that `max_id=` (empty) returns a successful response — the exact request the Pixelfed Android app sends on first load. 3. Same for `min_id=`. 4. Validates that a non-null, non-integer `max_id` is still rejected (422), ensuring the `nullable` change does not weaken input validation. --- app/Exceptions/Handler.php | 2 +- app/Http/Controllers/Api/ApiV1Controller.php | 4 +- tests/Feature/HomeTimelineTest.php | 106 +++++++++++++++++++ 3 files changed, 109 insertions(+), 3 deletions(-) create mode 100644 tests/Feature/HomeTimelineTest.php diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php index fae18efab..9c979e00a 100644 --- a/app/Exceptions/Handler.php +++ b/app/Exceptions/Handler.php @@ -71,7 +71,7 @@ class Handler extends ExceptionHandler 'message' => $exception->getMessage(), 'errors' => $exception->validator->getMessageBag(), ], - method_exists($exception, 'getStatusCode') ? $exception->getStatusCode() : 500 + method_exists($exception, 'getStatusCode') ? $exception->getStatusCode() : $exception->status ); } elseif ($request->wantsJson()) { return response()->json( diff --git a/app/Http/Controllers/Api/ApiV1Controller.php b/app/Http/Controllers/Api/ApiV1Controller.php index f9878281a..e85afec64 100644 --- a/app/Http/Controllers/Api/ApiV1Controller.php +++ b/app/Http/Controllers/Api/ApiV1Controller.php @@ -2607,8 +2607,8 @@ class ApiV1Controller extends Controller $this->validate($request, [ 'page' => 'sometimes|integer|max:40', - 'min_id' => 'sometimes|integer|min:0|max:'.PHP_INT_MAX, - 'max_id' => 'sometimes|integer|min:0|max:'.PHP_INT_MAX, + 'min_id' => 'nullable|integer|min:0|max:'.PHP_INT_MAX, + 'max_id' => 'nullable|integer|min:0|max:'.PHP_INT_MAX, 'limit' => 'sometimes|integer|min:1', 'include_reblogs' => 'sometimes', ]); diff --git a/tests/Feature/HomeTimelineTest.php b/tests/Feature/HomeTimelineTest.php new file mode 100644 index 000000000..34ac3a015 --- /dev/null +++ b/tests/Feature/HomeTimelineTest.php @@ -0,0 +1,106 @@ +create(); + Passport::actingAs($user, ['read']); + + return $user; + } + + // ------------------------------------------------------------------------- + // Bug #6609 — Exception handler returns 500 instead of 422 + // ------------------------------------------------------------------------- + + /** + * ValidationException must produce HTTP 422, not 500. + * + * Root cause: Handler.php called method_exists($exception, 'getStatusCode') + * which is false for Illuminate\Validation\ValidationException (the class + * exposes $status = 422 but has no getStatusCode() method), so the fallback + * of 500 was used for every JSON API validation failure. + */ + #[Test] + public function json_api_validation_failure_returns_422_not_500(): void + { + $this->authenticatedUser(); + + // max_id=not-a-number is non-null and not an integer. + // 'nullable|integer' fails on it → ValidationException → must be 422. + $response = $this->getJson('/api/v1/timelines/home?max_id=not-a-number'); + + $response->assertStatus(422); + $response->assertJsonStructure(['message', 'errors']); + $response->assertJsonPath('errors.max_id.0', fn ($msg) => str_contains($msg, 'integer')); + } + + // ------------------------------------------------------------------------- + // Bug #6610 — timelineHome rejects empty max_id= / min_id= + // ------------------------------------------------------------------------- + + /** + * An empty max_id= query parameter must not cause a server error. + * + * Root cause: timelineHome used 'sometimes|integer' for max_id/min_id while + * every other timeline method in ApiV1Controller used 'nullable|integer'. + * The ConvertEmptyStringsToNull middleware converts max_id= to null before + * validation. With 'sometimes', the key is still "present" (as null) and the + * integer rule runs on null → ValidationException → triggered bug #6609 → 500. + * With 'nullable', a null value skips the integer check → no exception. + * + * The Pixelfed Android app (okhttp/4.12.0) sends max_id= on every first + * home timeline load, making the app unusable without this fix. + */ + #[Test] + public function timeline_home_accepts_empty_max_id_parameter(): void + { + $this->authenticatedUser(); + + // Mirrors the exact request sent by the Pixelfed Android app on first load. + $response = $this->getJson('/api/v1/timelines/home?limit=20&max_id=&_pe=1&limit=20'); + + $response->assertSuccessful(); + } + + #[Test] + public function timeline_home_accepts_empty_min_id_parameter(): void + { + $this->authenticatedUser(); + + $response = $this->getJson('/api/v1/timelines/home?limit=20&min_id='); + + $response->assertSuccessful(); + } + + #[Test] + public function timeline_home_still_validates_non_null_non_integer_max_id(): void + { + // Ensure the nullable change does not make the endpoint accept garbage input. + // A non-empty, non-integer max_id must still fail validation (422, not 200). + $this->authenticatedUser(); + + $response = $this->getJson('/api/v1/timelines/home?max_id=abc'); + + $response->assertStatus(422); + } +}