Reject non-scalar ids with 422 instead of 500 (#10)
`group_id: [14]` — a one-element array of a REAL group id — returned a 500. The reporter's analysis was right: `exists` given an array validates that EVERY element exists, which is multi-select behaviour, so an array of one valid id passed the rule and reached the controller. findOrFail() then returned a Collection and ->nextTicketNumber() on it fatalled. What hid it is that the rule looked like it was working: `['a','b']` was correctly rejected by `exists` with a clean 422. Only arrays made of valid ids got through. Adding `integer` ahead of `exists` fixes the reported field, but the hole was a property of `exists`, not of group_id — so every id on the endpoint had it. Fixed all nine sites across both controllers: store()'s group_id and priority_id, update()'s priority_id/project_id/group_id, and storeAgent/storeProject/storeEmailConnection's group_id (plus user_id, which fed straight into an access check). Tests pin the reported repro, the equivalent on priority_id, and both shapes that must keep behaving: a nonexistent id still 422s (so the exists check cannot be dropped later while the array tests still pass) and a normal scalar create still succeeds. Verified the array test FAILS with the rules reverted — it reproduces the 500 rather than merely passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDA3fQbsa3gaHCFoV1Q8Gg
This commit is contained in:
@@ -309,8 +309,16 @@ class TicketController extends Controller
|
||||
'description' => 'required|string',
|
||||
// Archived groups are gone from the picker; they must also be
|
||||
// unreachable by posting a stale id.
|
||||
'group_id' => ['required', Rule::exists('ticketing_groups', 'id')->whereNull('archived_at')],
|
||||
'priority_id' => 'nullable|exists:ticketing_priority_levels,id',
|
||||
//
|
||||
// `integer` before `exists` is load-bearing, not tidiness: `exists`
|
||||
// given an ARRAY checks that every element exists, so `group_id: [14]`
|
||||
// sailed through with one valid id and reached the controller as an
|
||||
// array. findOrFail() then returned a Collection, ->nextTicketNumber()
|
||||
// on it fatalled, and the endpoint 500'd instead of 422ing. Only some
|
||||
// array shapes showed it — `['a','b']` was rejected by exists itself,
|
||||
// which is why this hid behind a passing validation rule.
|
||||
'group_id' => ['required', 'integer', Rule::exists('ticketing_groups', 'id')->whereNull('archived_at')],
|
||||
'priority_id' => 'nullable|integer|exists:ticketing_priority_levels,id',
|
||||
'due_date' => 'nullable|date',
|
||||
'attachments' => 'nullable|array|max:5',
|
||||
'attachments.*' => 'file|max:20480',
|
||||
@@ -452,13 +460,14 @@ class TicketController extends Controller
|
||||
'title' => 'sometimes|required|string|max:255',
|
||||
'description' => 'sometimes|required|string',
|
||||
'status' => "sometimes|in:{$validStatusSlugs}",
|
||||
'priority_id' => 'nullable|exists:ticketing_priority_levels,id',
|
||||
// `integer` guards the same array-through-`exists` hole as store().
|
||||
'priority_id' => 'nullable|integer|exists:ticketing_priority_levels,id',
|
||||
'due_date' => 'nullable|date',
|
||||
'project_id' => 'nullable|exists:ticketing_projects,id',
|
||||
'project_id' => 'nullable|integer|exists:ticketing_projects,id',
|
||||
// A ticket may not be transferred INTO an archived group, but a
|
||||
// ticket already sitting in one must still be editable — so its own
|
||||
// group id always validates.
|
||||
'group_id' => ['nullable', 'exists:ticketing_groups,id',
|
||||
'group_id' => ['nullable', 'integer', 'exists:ticketing_groups,id',
|
||||
function ($attribute, $value, $fail) use ($ticket) {
|
||||
if (! $value || (int) $value === (int) $ticket->group_id) {
|
||||
return;
|
||||
|
||||
@@ -292,8 +292,10 @@ class TicketingSettingsController extends Controller
|
||||
$this->requireAgentAccess();
|
||||
|
||||
$validated = $request->validate([
|
||||
'user_id' => 'required|exists:users,id',
|
||||
'group_id' => 'required|exists:ticketing_groups,id',
|
||||
'user_id' => 'required|integer|exists:users,id',
|
||||
// `integer` before `exists`: an array value passes `exists` element-wise,
|
||||
// so [14] would reach the code below as an array. See TicketController::store().
|
||||
'group_id' => 'required|integer|exists:ticketing_groups,id',
|
||||
'role' => 'required|in:agent,manager',
|
||||
]);
|
||||
|
||||
@@ -374,7 +376,9 @@ class TicketingSettingsController extends Controller
|
||||
$this->requireAgentAccess();
|
||||
|
||||
$validated = $request->validate([
|
||||
'group_id' => 'required|exists:ticketing_groups,id',
|
||||
// `integer` before `exists`: an array value passes `exists` element-wise,
|
||||
// so [14] would reach the code below as an array. See TicketController::store().
|
||||
'group_id' => 'required|integer|exists:ticketing_groups,id',
|
||||
'name' => 'required|string|max:100',
|
||||
'description' => 'nullable|string',
|
||||
'status' => 'required|in:active,archived',
|
||||
@@ -427,7 +431,9 @@ class TicketingSettingsController extends Controller
|
||||
$this->requireAgentAccess();
|
||||
|
||||
$validated = $request->validate([
|
||||
'group_id' => 'required|exists:ticketing_groups,id',
|
||||
// `integer` before `exists`: an array value passes `exists` element-wise,
|
||||
// so [14] would reach the code below as an array. See TicketController::store().
|
||||
'group_id' => 'required|integer|exists:ticketing_groups,id',
|
||||
'type' => 'required|in:gmail,imap',
|
||||
'active' => 'boolean',
|
||||
'config.host' => 'nullable|string|max:255',
|
||||
|
||||
Reference in New Issue
Block a user