fix: bootstrap blocker + 4 security bugs
- Bootstrap (critical): settings/create/index no longer 403 on fresh install. Site admins (admin/super_admin) can access settings when 0 groups exist. First group creation seeds default priorities (Low/Medium/High/Urgent). Index shows friendly first-run splash. Create shows warning + settings link. - Internal notes leak (high): submitters can no longer receive is_internal messages via ticket show, index detail panel, or any Inertia prop. filterMessagesForRole() strips internal notes for non-agents. - Arbitrary assignee (med/high): update() now validates assigned_to against actual agent-access users for the ticket's group server-side. - Cross-group priority/project forgery (medium): store() and update() now verify priority_id and project_id belong to the ticket's own group (or are global for priorities). - Foreign message_id on attachment upload (medium): message_id is now validated to belong to the current ticket, not just any message row.
This commit is contained in:
Joel Wedemire
@@ -40,12 +40,40 @@ class TicketController extends Controller
|
||||
->exists();
|
||||
}
|
||||
|
||||
/**
|
||||
* Filter messages for submitters — strip internal notes.
|
||||
*/
|
||||
private function filterMessagesForRole($messages, bool $isAgent)
|
||||
{
|
||||
if ($isAgent) {
|
||||
return $messages;
|
||||
}
|
||||
return $messages->filter(fn($m) => !$m->is_internal)->values();
|
||||
}
|
||||
|
||||
public function index(Request $request): Response
|
||||
{
|
||||
$user = Auth::user();
|
||||
$agentGroupIds = $this->agentGroupIds();
|
||||
$isAgent = count($agentGroupIds) > 0;
|
||||
|
||||
// If no groups exist at all, render a first-run / bootstrap state
|
||||
$totalGroups = TicketingGroup::count();
|
||||
if ($totalGroups === 0) {
|
||||
return Inertia::render('Ticketing/Index', [
|
||||
'tickets' => null,
|
||||
'groups' => [],
|
||||
'priorities' => [],
|
||||
'projects' => [],
|
||||
'isAgent' => $isAgent,
|
||||
'filters' => [],
|
||||
'ticketDetail' => null,
|
||||
'detailAgents' => [],
|
||||
'viewCounts' => ['all' => 0, 'mine' => 0, 'unassigned' => 0, 'pending' => 0, 'resolved' => 0],
|
||||
'isBootstrap' => true,
|
||||
]);
|
||||
}
|
||||
|
||||
$baseQuery = Ticket::query();
|
||||
|
||||
if ($isAgent) {
|
||||
@@ -169,9 +197,14 @@ class TicketController extends Controller
|
||||
|
||||
$dt->submitter = $detailUsers[$dt->submitter_id] ?? null;
|
||||
$dt->assignee = $dt->assigned_to ? ($detailUsers[$dt->assigned_to] ?? null) : null;
|
||||
$dt->messages->each(function ($msg) use ($detailUsers) {
|
||||
|
||||
// Bug #2: Filter internal notes for non-agents
|
||||
$visibleMessages = $this->filterMessagesForRole($dt->messages, $isAgent);
|
||||
$visibleMessages->each(function ($msg) use ($detailUsers) {
|
||||
$msg->author = $msg->user_id ? ($detailUsers[$msg->user_id] ?? null) : null;
|
||||
});
|
||||
$dt->setRelation('messages', $visibleMessages);
|
||||
|
||||
$ticketDetail = $dt;
|
||||
|
||||
if ($isAgent) {
|
||||
@@ -191,6 +224,7 @@ class TicketController extends Controller
|
||||
'ticketDetail' => $ticketDetail,
|
||||
'detailAgents' => $detailAgents,
|
||||
'viewCounts' => $viewCounts,
|
||||
'isBootstrap' => false,
|
||||
]);
|
||||
}
|
||||
|
||||
@@ -201,11 +235,22 @@ class TicketController extends Controller
|
||||
$isAgent = count($agentGroupIds) > 0;
|
||||
|
||||
$groups = TicketingGroup::when($isAgent, fn($q) => $q->whereIn('id', $agentGroupIds))->get();
|
||||
|
||||
// No groups at all? Show first-run state.
|
||||
if ($groups->isEmpty() && TicketingGroup::count() === 0) {
|
||||
return Inertia::render('Ticketing/Create', [
|
||||
'groups' => [],
|
||||
'priorities' => [],
|
||||
'isBootstrap' => true,
|
||||
]);
|
||||
}
|
||||
|
||||
$priorities = PriorityLevel::orderBy('sort_order')->get();
|
||||
|
||||
return Inertia::render('Ticketing/Create', [
|
||||
'groups' => $groups,
|
||||
'priorities' => $priorities,
|
||||
'isBootstrap' => false,
|
||||
]);
|
||||
}
|
||||
|
||||
@@ -219,6 +264,14 @@ class TicketController extends Controller
|
||||
'due_date' => 'nullable|date',
|
||||
]);
|
||||
|
||||
// Bug #4: Ensure priority belongs to the chosen group (or is global)
|
||||
if (!empty($validated['priority_id'])) {
|
||||
$priority = PriorityLevel::find($validated['priority_id']);
|
||||
if ($priority && $priority->group_id !== null && $priority->group_id != $validated['group_id']) {
|
||||
abort(422, 'Priority does not belong to the selected group.');
|
||||
}
|
||||
}
|
||||
|
||||
$group = TicketingGroup::findOrFail($validated['group_id']);
|
||||
$number = $group->nextTicketNumber();
|
||||
|
||||
@@ -244,13 +297,18 @@ class TicketController extends Controller
|
||||
|
||||
$ticket->load(['group', 'priority', 'project', 'messages', 'attachments']);
|
||||
|
||||
$userIds = $ticket->messages->pluck('user_id')->filter()->unique();
|
||||
// Bug #2: Filter internal notes for submitters
|
||||
$visibleMessages = $this->filterMessagesForRole($ticket->messages, $isAgent);
|
||||
|
||||
$userIds = $visibleMessages->pluck('user_id')->filter()->unique();
|
||||
$users = DB::table('users')->whereIn('id', $userIds)->get(['id', 'name', 'email'])->keyBy('id');
|
||||
|
||||
$ticket->messages->each(function ($msg) use ($users) {
|
||||
$visibleMessages->each(function ($msg) use ($users) {
|
||||
$msg->author = $msg->user_id ? ($users[$msg->user_id] ?? null) : null;
|
||||
});
|
||||
|
||||
$ticket->setRelation('messages', $visibleMessages);
|
||||
|
||||
$agents = [];
|
||||
if ($isAgent) {
|
||||
$agentIds = TicketingAgentAccess::where('group_id', $ticket->group_id)->pluck('user_id');
|
||||
@@ -296,6 +354,11 @@ class TicketController extends Controller
|
||||
abort(403);
|
||||
}
|
||||
|
||||
// Bug #3: Valid assignees are users with agent access to this group
|
||||
$validAssigneeIds = TicketingAgentAccess::where('group_id', $ticket->group_id)
|
||||
->pluck('user_id')
|
||||
->toArray();
|
||||
|
||||
$rules = [
|
||||
'title' => 'sometimes|required|string|max:255',
|
||||
'description' => 'sometimes|required|string',
|
||||
@@ -303,13 +366,28 @@ class TicketController extends Controller
|
||||
'priority_id' => 'nullable|exists:ticketing_priority_levels,id',
|
||||
'due_date' => 'nullable|date',
|
||||
'project_id' => 'nullable|exists:ticketing_projects,id',
|
||||
'assigned_to' => 'nullable|in:' . implode(',', $validAssigneeIds ?: [0]),
|
||||
];
|
||||
|
||||
if ($this->isAgent($ticket->group_id)) {
|
||||
$rules['assigned_to'] = 'nullable|exists:users,id';
|
||||
$validated = $request->validate($rules);
|
||||
|
||||
// Bug #4: priority_id must belong to this group or be global
|
||||
if (!empty($validated['priority_id'])) {
|
||||
$priority = PriorityLevel::find($validated['priority_id']);
|
||||
if ($priority && $priority->group_id !== null && $priority->group_id !== $ticket->group_id) {
|
||||
abort(422, 'Priority does not belong to this ticket\'s group.');
|
||||
}
|
||||
}
|
||||
|
||||
$ticket->update($request->validate($rules));
|
||||
// Bug #4: project_id must belong to this group
|
||||
if (!empty($validated['project_id'])) {
|
||||
$project = TicketingProject::find($validated['project_id']);
|
||||
if ($project && $project->group_id !== $ticket->group_id) {
|
||||
abort(422, 'Project does not belong to this ticket\'s group.');
|
||||
}
|
||||
}
|
||||
|
||||
$ticket->update($validated);
|
||||
|
||||
return back()->with('success', 'Ticket updated.');
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user