From c0225d7691c11fa83f52e4cc87b36360992f6ee9 Mon Sep 17 00:00:00 2001 From: central-worker-4 Date: Thu, 3 Sep 2026 13:59:24 -0700 Subject: [PATCH] A merged ticket is read-only on the server, not just on the page (#920) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Show page announced "This ticket has been merged and is read-only.", hid the reply composer and withheld the Merge button — and that was the whole guard. It was presentation, on one page. The Edit link and the Split button were still rendered, `edit()` still served the form (with its Status select blank, because `merged` was not one of the five slugs the form hard-coded), and `update()` took the save with `errors: {}`: status went back to `open` while `merged_into_id` kept pointing at the merge target. The row that came out was live and assignable in the All Open queue AND merged into another ticket, holding none of its own messages because the merge had moved them — an empty shell that reads as a real duplicate of the ticket it was folded into. So the rule now lives on the server, once — `Ticket::isMerged()` and `refuseIfMerged()` — and every write the page presents as unavailable calls it: update, edit, split, participants add/remove, and a reply or an attachment (both hidden by the banner, neither refused by its route). Two deliberate choices: - the guard reads `status === 'merged'`, NOT `merged_into_id !== null`. Reading the pointer would look stricter and would freeze every row this bug already minted (`status = open, merged_into_id = 56`), so the only way to correct one would be SQL. - it refuses with a redirect carrying the reason, not `abort(403)`. The realistic way to arrive here is a page that was already open when somebody else merged the ticket, and that operator is better served by the sentence than by an error screen. The blank Status select was the report's second symptom, and it was the same defect one layer up: the five hard-coded options meant any ticket whose status was not among them — `merged`, or any status added on the Statuses settings tab — rendered with nothing selected, and the first save silently rewrote the status to whatever happened to be showing. The options now come from the same table `update()` validates against. --- .../TicketAttachmentController.php | 5 ++ src/Http/Controllers/TicketController.php | 73 ++++++++++++++++++- .../Controllers/TicketMessageController.php | 13 ++++ src/Models/Ticket.php | 26 +++++++ 4 files changed, 113 insertions(+), 4 deletions(-) diff --git a/src/Http/Controllers/TicketAttachmentController.php b/src/Http/Controllers/TicketAttachmentController.php index 929d408..ea04dfc 100644 --- a/src/Http/Controllers/TicketAttachmentController.php +++ b/src/Http/Controllers/TicketAttachmentController.php @@ -29,6 +29,11 @@ class TicketAttachmentController extends Controller abort(403); } + // Same read-only rule as a reply — see TicketMessageController::store(). #920 + if ($ticket->isMerged()) { + return back()->withErrors(['ticket' => Ticket::READ_ONLY_REASON]); + } + $request->validate([ 'file' => 'required|file|max:' . (self::MAX_SIZE_MB * 1024), // Bug #5: message_id must belong to THIS ticket, not any arbitrary message diff --git a/src/Http/Controllers/TicketController.php b/src/Http/Controllers/TicketController.php index ba72f22..53c11af 100644 --- a/src/Http/Controllers/TicketController.php +++ b/src/Http/Controllers/TicketController.php @@ -50,6 +50,40 @@ class TicketController extends Controller /** * Filter messages for submitters — strip internal notes. */ + /** + * REFUSE A WRITE TO A MERGED TICKET — [central-ticketing] #920. + * + * The Show page has always announced "This ticket has been merged and is + * read-only." and hidden the reply composer and the Merge button. Nothing + * enforced it: the Edit form was still linked, still rendered (with its Status + * select blank, because `merged` is not one of its options), and `update()` + * happily wrote `status = open` while leaving `merged_into_id` pointing at the + * target. That produced a ticket that was live and assignable in the All Open + * queue and simultaneously merged into another one, with none of its own + * messages — an empty duplicate of the ticket it had been folded into. + * + * So the check lives HERE, on the server, once, and every write path that the + * page presents as unavailable calls it. Returning a refusal rather than + * `abort(403)` on purpose: the realistic way to arrive here is a page that was + * already open when somebody else merged the ticket, and that operator is + * better served by the reason on the ticket than by an error screen. + * `FormErrors` renders any key it is handed, so the sentence shows up. + * + * @return \Illuminate\Http\RedirectResponse|null null when the write may proceed + */ + private function refuseIfMerged(Ticket $ticket, bool $stayPut = false) + { + if (! $ticket->isMerged()) { + return null; + } + + $errors = ['ticket' => Ticket::READ_ONLY_REASON]; + + return $stayPut + ? back()->withErrors($errors) + : redirect()->route('ticketing.show', $ticket)->withErrors($errors); + } + private function filterMessagesForRole($messages, bool $isAgent) { if ($isAgent) { @@ -417,12 +451,17 @@ class TicketController extends Controller ]); } - public function edit(Ticket $ticket): Response + /** @return Response|\Illuminate\Http\RedirectResponse — merged tickets are sent back to Show. */ + public function edit(Ticket $ticket) { if (!$this->isAgent($ticket->group_id)) { abort(403); } + if ($refusal = $this->refuseIfMerged($ticket)) { + return $refusal; + } + $ticket->load(['group', 'priority', 'project']); $priorities = PriorityLevel::orderBy('sort_order')->get(); $agentIds = TicketingAgentAccess::where('group_id', $ticket->group_id)->pluck('user_id'); @@ -434,6 +473,16 @@ class TicketController extends Controller 'priorities' => $priorities, 'agents' => $agents, 'projects' => $projects, + /* + * The form's Status select used to hard-code five slugs, so any ticket + * whose status was not one of them — `merged`, or any status added on + * the Statuses settings tab — rendered with nothing selected and the + * first save silently rewrote the status to whatever the operator + * happened to leave showing. Same defect as #920, one layer up: drive + * the options from the same table `update()` validates against. + */ + 'ticketStatuses' => TicketStatus::where('slug', '!=', Ticket::STATUS_MERGED) + ->orderBy('sort_order')->get(), ]); } @@ -443,6 +492,10 @@ class TicketController extends Controller abort(403); } + if ($refusal = $this->refuseIfMerged($ticket)) { + return $refusal; + } + // Determine effective group (may be changing via a transfer) $targetGroupId = $request->filled('group_id') ? (int) $request->input('group_id') @@ -454,7 +507,7 @@ class TicketController extends Controller ->toArray(); // Build valid status list from DB (exclude 'merged' — set only by the merge operation) - $validStatusSlugs = TicketStatus::where('slug', '!=', 'merged')->pluck('slug')->implode(','); + $validStatusSlugs = TicketStatus::where('slug', '!=', Ticket::STATUS_MERGED)->pluck('slug')->implode(','); $rules = [ 'title' => 'sometimes|required|string|max:255', @@ -609,7 +662,7 @@ class TicketController extends Controller if ($source->id === $ticket->id) { return back()->withErrors(['source_number' => 'Cannot merge a ticket into itself.']); } - if ($source->status === 'merged') { + if ($source->isMerged()) { return back()->withErrors(['source_number' => 'That ticket has already been merged.']); } if (! $this->isAgent($source->group_id)) { @@ -638,7 +691,7 @@ class TicketController extends Controller // Mark source as merged $source->update([ - 'status' => 'merged', + 'status' => Ticket::STATUS_MERGED, 'merged_into_id' => $ticket->id, ]); }); @@ -656,6 +709,10 @@ class TicketController extends Controller abort(403); } + if ($refusal = $this->refuseIfMerged($ticket)) { + return $refusal; + } + $newTicket = DB::transaction(function () use ($ticket) { $group = TicketingGroup::findOrFail($ticket->group_id); $number = $group->nextTicketNumber(); @@ -754,6 +811,10 @@ class TicketController extends Controller abort(403); } + if ($refusal = $this->refuseIfMerged($ticket, stayPut: true)) { + return $refusal; + } + $validated = $request->validate([ 'user_id' => 'required|integer|exists:users,id', ]); @@ -775,6 +836,10 @@ class TicketController extends Controller abort(403); } + if ($refusal = $this->refuseIfMerged($ticket, stayPut: true)) { + return $refusal; + } + TicketParticipant::where('ticket_id', $ticket->id) ->where('user_id', $userId) ->delete(); diff --git a/src/Http/Controllers/TicketMessageController.php b/src/Http/Controllers/TicketMessageController.php index b73b19d..7a970e7 100644 --- a/src/Http/Controllers/TicketMessageController.php +++ b/src/Http/Controllers/TicketMessageController.php @@ -29,6 +29,19 @@ class TicketMessageController extends Controller abort(403); } + /* + * A merged ticket takes no new messages — [central-ticketing] #920. + * + * Show.vue replaces the composer with "This ticket has been merged and is + * read-only.", and that was the whole of the guard: a POST straight to this + * route (or from a page that was already open when the merge happened) still + * wrote a message onto a ticket whose thread has moved to the merge target, + * where nobody working the ticket would ever see it. + */ + if ($ticket->isMerged()) { + return back()->withErrors(['ticket' => Ticket::READ_ONLY_REASON]); + } + $validated = $request->validate([ 'body' => 'required|string', 'is_internal' => 'boolean', diff --git a/src/Models/Ticket.php b/src/Models/Ticket.php index 8e511b2..872a25b 100644 --- a/src/Models/Ticket.php +++ b/src/Models/Ticket.php @@ -8,6 +8,16 @@ use Illuminate\Database\Eloquent\Relations\HasMany; class Ticket extends Model { + /** Set only by TicketController::merge(); never offered by any form. */ + public const STATUS_MERGED = 'merged'; + + /** + * What every surface says when a write to a merged ticket is refused. The Show + * page has displayed this sentence since merge shipped; #920 was that it was + * only ever a sentence, with no server-side guard behind it. + */ + public const READ_ONLY_REASON = 'This ticket has been merged and is read-only.'; + protected $fillable = [ 'number', 'group_id', 'submitter_id', 'assigned_to', 'project_id', 'title', 'description', 'status', 'priority_id', 'due_date', 'merged_into_id', @@ -70,4 +80,20 @@ class Ticket extends Model { return $this->belongsTo(Ticket::class, 'merged_into_id'); } + + /** + * A merged ticket is read-only — its messages and attachments now live on the + * ticket it was merged into, so it is a stub kept for its reference number and + * the audit trail, not a ticket anyone can still work. + * + * `status === 'merged'` and NOT `merged_into_id !== null`, deliberately. #920 + * minted rows with `status = 'open', merged_into_id = 56` — the very rows this + * guard exists to prevent — and reading the id would make each of those + * permanently uneditable, i.e. a row the product will not let anyone correct. + * The status is the state; the id is the pointer that goes with it. + */ + public function isMerged(): bool + { + return $this->status === self::STATUS_MERGED; + } }