From e91f4547b679edee41fa011a21c391749e0de43e Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Sat, 12 Sep 2026 01:17:13 -0400 Subject: [PATCH 1/3] Log watch/unwatch actions to the audit trail (#93) api/watch_ticket.php performed the ticket_watchers INSERT IGNORE/DELETE directly with no AuditLogModel call, unlike every other ticket-adjacent mutation (comments, attachments, dependencies, status/field changes), so watching/unwatching never showed up in a ticket's timeline. Added AuditLogModel::log() calls to both the watch and unwatch paths, gated on the DB statement's affected_rows so a no-op (already watching, already not watching) doesn't produce a duplicate timeline entry. Added 'watch'/'unwatch' to AuditLogModel's VALID_ACTION_TYPES, and timeline rendering in views/TicketView.php ("started watching this ticket" / "stopped watching this ticket"). Verified against real MariaDB: watch -> unwatch -> watch again produces exactly 2 timeline entries (not 4) since the two no-op repeats correctly produced zero rows changed and were not logged; confirmed formatAction()/ getEventIcon() render both action types correctly. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_0117oBw2jN4kALYeS8HPq4zV --- api/watch_ticket.php | 10 ++++++++++ models/AuditLogModel.php | 3 ++- views/TicketView.php | 6 ++++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/api/watch_ticket.php b/api/watch_ticket.php index 40e9867..a9448f5 100644 --- a/api/watch_ticket.php +++ b/api/watch_ticket.php @@ -9,6 +9,7 @@ require_once __DIR__ . '/bootstrap.php'; require_once dirname(__DIR__) . '/models/TicketModel.php'; +require_once dirname(__DIR__) . '/models/AuditLogModel.php'; $data = json_decode(file_get_contents('php://input'), true) ?? []; @@ -43,6 +44,7 @@ if ($_SERVER['REQUEST_METHOD'] === 'POST') { ); $stmt->bind_param("si", $ticketId, $userId); $stmt->execute(); + $rowsChanged = $stmt->affected_rows; $stmt->close(); } else { $stmt = $conn->prepare( @@ -50,9 +52,17 @@ if ($_SERVER['REQUEST_METHOD'] === 'POST') { ); $stmt->bind_param("si", $ticketId, $userId); $stmt->execute(); + $rowsChanged = $stmt->affected_rows; $stmt->close(); } + // Only log an actual state change — INSERT IGNORE/DELETE are no-ops when + // the user was already watching/not watching, and that shouldn't show up + // in the ticket's timeline as a new event. + if ($rowsChanged > 0) { + (new AuditLogModel($conn))->log($userId, $action, 'ticket', $ticketId); + } + // Return updated state $countStmt = $conn->prepare( "SELECT COUNT(*) as cnt FROM ticket_watchers WHERE ticket_id = ?" diff --git a/models/AuditLogModel.php b/models/AuditLogModel.php index dd691cc..001abcb 100644 --- a/models/AuditLogModel.php +++ b/models/AuditLogModel.php @@ -20,7 +20,8 @@ class AuditLogModel private const VALID_ACTION_TYPES = [ 'create', 'update', 'delete', 'view', 'security_event', 'login', 'logout', 'assign', 'unassign', 'comment', 'mention', - 'revoke', 'attachment_upload', 'attachment_delete', 'bulk_update' + 'revoke', 'attachment_upload', 'attachment_delete', 'bulk_update', + 'watch', 'unwatch' ]; /** @var array Allowed entity types for filtering */ diff --git a/views/TicketView.php b/views/TicketView.php index f84c4fd..3c65875 100644 --- a/views/TicketView.php +++ b/views/TicketView.php @@ -32,6 +32,8 @@ function getEventIcon(string $actionType): string 'status_change' => '[!]', 'attachment' => '[^]', 'delete' => '[x]', + 'watch' => '[o]', + 'unwatch' => '[o]', default => '[*]', }; } @@ -53,6 +55,10 @@ function formatAction(array $event): string return 'uploaded a file'; case 'delete': return 'deleted a comment'; + case 'watch': + return 'started watching this ticket'; + case 'unwatch': + return 'stopped watching this ticket'; case 'assign': if (is_array($det) && isset($det['assigned_to']['to'])) { $to = $det['assigned_to']['to'] ?: 'Unassigned'; From 6609320c836f8a76c3683ddff6823edc0e26e897 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Sat, 12 Sep 2026 01:17:53 -0400 Subject: [PATCH 2/3] Restrict API keys to public-visibility tickets by default (#70) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit api/tickets_api.php (both the single-ticket read and the list/triage path) bypassed ticket visibility entirely for any 'read'-scope key, regardless of who it was issued to or what it was for — any key got blanket read access to Confidential and Internal ticket titles, descriptions, and comments, with no way to scope a key more narrowly. Added see_all_visibility to api_keys (migration 006), defaulting to false for both new and existing keys — the prior blanket-access behavior is what's being restricted here, so unlike scope's own un-migrated-database fallback (which defaults toward preserving old behavior), a missing/null value here defaults to the new, restrictive one. An admin can opt a specific key in via a new checkbox in the API Key Management UI when it genuinely needs the full queue. tickets_api.php now builds a synthetic "no special access" user and runs it through TicketModel's existing per-user visibility plumbing (getVisibilityFilter/canUserAccessTicket) instead of a separate SQL path, so this stays in lockstep with however visibility rules evolve for real users. That synthetic user_id is -1, not 0: testing surfaced that canUserAccessTicket()'s confidential-ticket check does a PHP-level (int) cast, and (int)null === 0, so an unassigned confidential ticket's NULL assigned_to would otherwise false-positive-match a user_id of 0. Verified against real MariaDB with public/confidential/internal test tickets: a public-only-scoped key's list only returns the public ticket, and canUserAccessTicket() correctly returns false for both the confidential ticket (unassigned, then reassigned to a real user — both cases) and the internal one; a see_all_visibility key sees all three, unchanged from the prior behavior. Also verified createKey()/ validateKey()'s default-false and explicit-true paths round-trip correctly through the real DB. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_0117oBw2jN4kALYeS8HPq4zV --- README.md | 3 ++ api/generate_api_key.php | 6 ++-- api/tickets_api.php | 35 ++++++++++++++++++--- middleware/ApiKeyAuth.php | 3 +- migrations/006_api_key_visibility_scope.sql | 13 ++++++++ models/ApiKeyModel.php | 19 ++++++++--- views/admin/ApiKeysView.php | 32 ++++++++++++++++--- 7 files changed, 94 insertions(+), 17 deletions(-) create mode 100644 migrations/006_api_key_visibility_scope.sql diff --git a/README.md b/README.md index 0177c49..286ba3b 100644 --- a/README.md +++ b/README.md @@ -97,12 +97,15 @@ The following features are intentionally **not planned** for this system: - **Admin UI**: Generate and manage API keys at `/admin/api-keys` (paginated) - **Bearer Token Auth**: Use API keys with `Authorization: Bearer YOUR_KEY` header - **Key Scopes**: `read` (GET only) or `read_write` (create/comment/close). A `read` key cannot mutate anything, including creating tickets. Existing keys default to `read_write`. +- **Visibility Scope**: keys default to **public-visibility tickets only** — Confidential and Internal tickets are excluded from `/api/tickets_api.php`, same as they'd be for a regular user with no special access. Check **See all visibility** when generating a key only if that specific integration genuinely needs the full queue; this is a deliberate opt-in, not something a key gets by having `read_write` scope or any other setting. - **Expiration**: Optional expiration dates for keys - **Revocation**: Revoke compromised keys instantly ### Bearer API (automation / triage) All Bearer-authenticated, rate-limited, and (like `create_ticket_api.php`) exempt from Authelia at the reverse proxy — the API key is the only credential. Comments/closes made via the API are attributed to the **key's name** (linked to the key's owner). +`/api/tickets_api.php` filters by ticket visibility according to the key's **Visibility Scope** setting above (public-only by default); every other Bearer endpoint below operates on a single ticket_id supplied by the caller and does not filter a list, so this setting doesn't apply to them. + | Endpoint | Method | Scope | Purpose | |----------|--------|-------|---------| | `/create_ticket_api.php` | POST | read_write | Create a ticket (hwmonDaemon, external tools) | diff --git a/api/generate_api_key.php b/api/generate_api_key.php index 394c203..936d6a2 100644 --- a/api/generate_api_key.php +++ b/api/generate_api_key.php @@ -67,6 +67,7 @@ try { $keyName = trim($input['key_name'] ?? ''); $expiresInDays = $input['expires_in_days'] ?? null; $scope = $input['scope'] ?? 'read_write'; + $seeAllVisibility = !empty($input['see_all_visibility']); if (empty($keyName)) { http_response_code(400); @@ -100,7 +101,7 @@ try { // Generate API key $apiKeyModel = new ApiKeyModel($conn); - $result = $apiKeyModel->createKey($keyName, $_SESSION['user']['user_id'], $expiresInDays, $scope); + $result = $apiKeyModel->createKey($keyName, $_SESSION['user']['user_id'], $expiresInDays, $scope, $seeAllVisibility); if (!$result['success']) { throw new Exception($result['error'] ?? "Failed to generate API key"); @@ -113,7 +114,7 @@ try { 'create', 'api_key', $result['key_id'], - ['key_name' => $keyName, 'expires_in_days' => $expiresInDays, 'scope' => $scope] + ['key_name' => $keyName, 'expires_in_days' => $expiresInDays, 'scope' => $scope, 'see_all_visibility' => $seeAllVisibility] ); // Clear output buffer @@ -127,6 +128,7 @@ try { 'key_prefix' => $result['key_prefix'], 'key_id' => $result['key_id'], 'scope' => $result['scope'], + 'see_all_visibility' => $result['see_all_visibility'], 'expires_at' => $result['expires_at'] ]); } catch (Exception $e) { diff --git a/api/tickets_api.php b/api/tickets_api.php index 95e220c..7bb85e0 100644 --- a/api/tickets_api.php +++ b/api/tickets_api.php @@ -4,8 +4,10 @@ * tickets_api.php — Bearer-key read endpoint (list/triage + read-one). * * GET only. Requires 'read' scope (a 'read_write' key also satisfies it). - * Acts as a trusted automation/server credential: reads return the full queue - * (no per-user visibility filtering). + * By default, a key only sees public-visibility tickets — Confidential and + * Internal tickets are excluded, the same as they would be for a regular user + * with no special access. An admin can mark a specific key see_all_visibility + * (API Key Management) when it genuinely needs the full queue. * * GET ?ticket_id=NNN -> {success, ticket, comments} * GET ?status=&priority=&host= -> {success, tickets, page, total, pages} @@ -48,6 +50,20 @@ try { // Reads only need the 'read' scope. $apiKeyAuth->requireScope('read'); +// Keys are public-ticket-only by default (#70) — a key must be explicitly +// marked see_all_visibility to bypass Confidential/Internal restrictions. +// Reuse TicketModel's existing per-user visibility plumbing with a synthetic +// "no special access" user rather than a separate SQL path, so this stays in +// lockstep with however visibility rules evolve for real users. user_id is +// -1, not 0: canUserAccessTicket() does a PHP-level (int) cast for the +// confidential-ticket check, and (int)null === 0, so an unassigned +// confidential ticket's assigned_to would otherwise false-positive-match a +// synthetic user_id of 0. No real user_id is ever <= 0, so -1 can't collide. +$keyContext = $apiKeyAuth->getKeyContext(); +$visibilityUser = !empty($keyContext['see_all_visibility']) + ? null + : ['user_id' => -1, 'is_admin' => false, 'groups' => '']; + if ($_SERVER['REQUEST_METHOD'] !== 'GET') { http_response_code(405); echo json_encode(['success' => false, 'error' => 'Method not allowed. Use GET.']); @@ -67,6 +83,15 @@ if (isset($_GET['ticket_id']) && trim((string)$_GET['ticket_id']) !== '') { exit; } + // A public-only key gets a plain 404 for a non-public ticket — same as a + // regular user hitting a ticket they can't see — rather than a 403 that + // would confirm the ticket exists. + if ($visibilityUser !== null && !$ticketModel->canUserAccessTicket($ticket, $visibilityUser)) { + http_response_code(404); + echo json_encode(['success' => false, 'error' => 'Ticket not found']); + exit; + } + // Flat list of comments (newest first) — same fetch the ticket view uses. $commentModel = new CommentModel($conn); $comments = $commentModel->getCommentsByTicketId($ticketId, false); @@ -114,8 +139,8 @@ if (isset($_GET['host']) && trim((string)$_GET['host']) !== '') { $search = trim((string)$_GET['host']); } -// user = null => getAllTickets skips visibility filtering and returns the full -// queue (this is a trusted server credential, not an end user). +// $visibilityUser is null (skip filtering, full queue) only for a key marked +// see_all_visibility; otherwise it restricts to public tickets (see above). $result = $ticketModel->getAllTickets( $page, $limit, @@ -126,7 +151,7 @@ $result = $ticketModel->getAllTickets( null, $search, $filters, - null + $visibilityUser ); echo json_encode([ diff --git a/middleware/ApiKeyAuth.php b/middleware/ApiKeyAuth.php index cf37670..cbf900e 100644 --- a/middleware/ApiKeyAuth.php +++ b/middleware/ApiKeyAuth.php @@ -37,6 +37,7 @@ class ApiKeyAuth { $this->keyContext = [ 'scope' => $keyData['scope'] ?? 'read_write', + 'see_all_visibility' => !empty($keyData['see_all_visibility']), 'key_name' => $keyData['key_name'] ?? null, 'created_by' => $keyData['created_by'] ?? null, 'api_key_id' => $keyData['api_key_id'] ?? null, @@ -46,7 +47,7 @@ class ApiKeyAuth /** * Get the context of the authenticated API key. * - * @return array|null ['scope', 'key_name', 'created_by', 'api_key_id'] or null + * @return array|null ['scope', 'see_all_visibility', 'key_name', 'created_by', 'api_key_id'] or null */ public function getKeyContext(): ?array { diff --git a/migrations/006_api_key_visibility_scope.sql b/migrations/006_api_key_visibility_scope.sql new file mode 100644 index 0000000..feb1797 --- /dev/null +++ b/migrations/006_api_key_visibility_scope.sql @@ -0,0 +1,13 @@ +-- Restrict Bearer API keys to public-visibility tickets by default (#70). +-- +-- Previously any 'read'-scope key bypassed ticket visibility entirely — +-- Confidential and Internal tickets were readable by any key, regardless +-- of who it was issued to. see_all_visibility is an explicit opt-in an +-- admin sets per-key when a key genuinely needs to see non-public tickets; +-- it defaults to 0 (public-only) for both new and existing keys, since the +-- prior blanket-access behavior is the thing being restricted. +-- +-- Safe to re-run. + +ALTER TABLE `api_keys` + ADD COLUMN IF NOT EXISTS `see_all_visibility` tinyint(1) NOT NULL DEFAULT 0 AFTER `scope`; diff --git a/models/ApiKeyModel.php b/models/ApiKeyModel.php index 9213030..1af924a 100644 --- a/models/ApiKeyModel.php +++ b/models/ApiKeyModel.php @@ -19,9 +19,11 @@ class ApiKeyModel * @param int $createdBy User ID who created the key * @param int|null $expiresInDays Number of days until expiration (null for no expiration) * @param string $scope Access scope: 'read' or 'read_write' (default 'read_write') + * @param bool $seeAllVisibility If true, the key bypasses ticket visibility (Confidential/ + * Internal included); defaults to false (public tickets only) * @return array Array with 'success', 'api_key' (plaintext), 'key_prefix', 'scope', 'error' */ - public function createKey($keyName, $createdBy, $expiresInDays = null, $scope = 'read_write') + public function createKey($keyName, $createdBy, $expiresInDays = null, $scope = 'read_write', $seeAllVisibility = false) { // Validate the requested scope — only the two known values are allowed if (!in_array($scope, ['read', 'read_write'], true)) { @@ -47,11 +49,12 @@ class ApiKeyModel } // Insert API key into database + $seeAllVisibilityInt = $seeAllVisibility ? 1 : 0; $stmt = $this->conn->prepare( - "INSERT INTO api_keys (key_name, key_hash, key_prefix, scope, created_by, expires_at) " - . "VALUES (?, ?, ?, ?, ?, ?)" + "INSERT INTO api_keys (key_name, key_hash, key_prefix, scope, see_all_visibility, created_by, expires_at) " + . "VALUES (?, ?, ?, ?, ?, ?, ?)" ); - $stmt->bind_param("ssssis", $keyName, $keyHash, $keyPrefix, $scope, $createdBy, $expiresAt); + $stmt->bind_param("ssssiis", $keyName, $keyHash, $keyPrefix, $scope, $seeAllVisibilityInt, $createdBy, $expiresAt); if ($stmt->execute()) { $keyId = $this->conn->insert_id; @@ -63,6 +66,7 @@ class ApiKeyModel 'key_prefix' => $keyPrefix, 'key_id' => $keyId, 'scope' => $scope, + 'see_all_visibility' => $seeAllVisibility, 'expires_at' => $expiresAt ]; } else { @@ -114,6 +118,13 @@ class ApiKeyModel $keyData['scope'] = 'read_write'; } + // Unlike scope's backward-compatible fallback above, an un-migrated or + // null see_all_visibility defaults to the RESTRICTIVE value (public + // tickets only) — this column exists specifically to lock down a + // previously-unrestricted default, so a missing value must not fall + // back to the permissive behavior it's replacing. + $keyData['see_all_visibility'] = !empty($keyData['see_all_visibility']); + // Check expiration if ($keyData['expires_at'] !== null) { $expiresAt = strtotime($keyData['expires_at']); diff --git a/views/admin/ApiKeysView.php b/views/admin/ApiKeysView.php index 24b8e6e..9502d90 100644 --- a/views/admin/ApiKeysView.php +++ b/views/admin/ApiKeysView.php @@ -45,10 +45,18 @@ include __DIR__ . '/../../views/layout_header.php'; +
+ +

Scope: read = GET only; read_write = create/comment/close. + By default a key only sees public-visibility tickets — check + See all visibility only if this key genuinely needs Confidential/Internal tickets too.

@@ -74,6 +82,7 @@ include __DIR__ . '/../../views/layout_header.php'; Name Key Prefix Scope + Visibility Created By Created Expires @@ -86,7 +95,7 @@ include __DIR__ . '/../../views/layout_header.php'; - No API keys found. Generate one above. + No API keys found. Generate one above. + + + all + + public only + + @@ -239,11 +255,17 @@ document.addEventListener('click', function (e) { document.getElementById('generateKeyForm').addEventListener('submit', function (e) { e.preventDefault(); - var keyName = document.getElementById('keyName').value.trim(); - var expiresIn = document.getElementById('expiresIn').value; - var keyScope = document.getElementById('keyScope').value; + var keyName = document.getElementById('keyName').value.trim(); + var expiresIn = document.getElementById('expiresIn').value; + var keyScope = document.getElementById('keyScope').value; + var seeAllVisibility = document.getElementById('keySeeAllVisibility').checked; if (!keyName) { lt.toast.error('Please enter a key name'); return; } - lt.api.post('/api/generate_api_key.php', { key_name: keyName, expires_in_days: expiresIn || null, scope: keyScope }) + lt.api.post('/api/generate_api_key.php', { + key_name: keyName, + expires_in_days: expiresIn || null, + scope: keyScope, + see_all_visibility: seeAllVisibility + }) .then(function (data) { if (data.success) { document.getElementById('newKeyValue').value = data.api_key; From 35192aaadc25ced2e3acafba57379491bd106e97 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Sat, 12 Sep 2026 01:18:13 -0400 Subject: [PATCH 3/3] Retry failed Matrix webhook notifications with backoff (#78) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NotificationHelper::fire() logged a failed webhook post via error_log() only, with no retry and no persistent record — once a notification failed, it was gone with no trace beyond the log line, even though the underlying DB write (audit_log entry, status change, etc.) it was reporting on had already committed. Extracted the curl POST into attemptDelivery(), shared between fire() (unchanged best-effort caller-facing behavior) and the new cron/retry_failed_notifications.php. On failure, fire() now also queues the payload to notification_retry_queue (migration 007) via Database::getConnection() — most fire() call sites don't have a $conn handy, and threading one through every caller would be a much larger, more invasive change than reusing the existing connection singleton. The cron script processes due rows with exponential backoff (2, 4, 8... capped at 60 minutes) up to each row's max_attempts (default 6), then leaves an exhausted row in place — not deleted — so it stays visible for manual investigation instead of disappearing a second time. Verified against real MariaDB and a local HTTP server standing in for the Matrix webhook, toggled between failing and succeeding: confirmed a real failure via fire() is correctly queued; the retry script reschedules a still-failing row with the expected backoff delay; flipping the fake webhook to succeed lets the same row's next retry delete it; a row that exhausts all attempts is left in place and correctly excluded from the next run's due-row query; and a success via fire() queues nothing (no regression on the common case). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_0117oBw2jN4kALYeS8HPq4zV --- README.md | 3 + cron/retry_failed_notifications.php | 133 ++++++++++++++++++++ helpers/NotificationHelper.php | 74 ++++++++--- migrations/007_notification_retry_queue.sql | 20 +++ 4 files changed, 216 insertions(+), 14 deletions(-) create mode 100644 cron/retry_failed_notifications.php create mode 100644 migrations/007_notification_retry_queue.sql diff --git a/README.md b/README.md index 286ba3b..f9c7d4d 100644 --- a/README.md +++ b/README.md @@ -525,6 +525,9 @@ Add to crontab for recurring tickets and maintenance cleanup: # Delete orphaned upload files with no attachment row, past a 24h grace period (daily). # Add --dry-run to preview without deleting. 0 4 * * * php /path/to/tinkertickets/scripts/cleanup_orphan_uploads.php + +# Retry failed Matrix webhook notifications (exponential backoff, every 5 minutes) +*/5 * * * * php /path/to/tinkertickets/cron/retry_failed_notifications.php ``` ### 3. File Uploads diff --git a/cron/retry_failed_notifications.php b/cron/retry_failed_notifications.php new file mode 100644 index 0000000..36e3b19 --- /dev/null +++ b/cron/retry_failed_notifications.php @@ -0,0 +1,133 @@ +#!/usr/bin/env php +getMessage()); + exit(1); +} + +// Process a bounded batch per run so one cron tick can't run indefinitely if +// the queue has backed up. +$batchLimit = 50; + +$stmt = $conn->prepare( + "SELECT retry_id, payload, attempts, max_attempts + FROM notification_retry_queue + WHERE next_attempt_at <= NOW() AND attempts < max_attempts + ORDER BY retry_id ASC + LIMIT ?" +); +$stmt->bind_param('i', $batchLimit); +$stmt->execute(); +$dueRows = $stmt->get_result()->fetch_all(MYSQLI_ASSOC); +$stmt->close(); + +if (empty($dueRows)) { + logMessage('No notifications due for retry.'); + exit(0); +} + +$succeeded = 0; +$failed = 0; +$exhausted = 0; + +foreach ($dueRows as $row) { + $payload = json_decode($row['payload'], true); + if (!is_array($payload)) { + // Corrupt row — can't retry something unparseable. Remove it rather + // than retrying forever against a row that will never succeed. + $del = $conn->prepare("DELETE FROM notification_retry_queue WHERE retry_id = ?"); + $del->bind_param('i', $row['retry_id']); + $del->execute(); + $del->close(); + logMessage("Discarded retry #{$row['retry_id']}: payload is not valid JSON"); + continue; + } + + $result = NotificationHelper::attemptDelivery($webhookUrl, $payload); + + if ($result['success']) { + $del = $conn->prepare("DELETE FROM notification_retry_queue WHERE retry_id = ?"); + $del->bind_param('i', $row['retry_id']); + $del->execute(); + $del->close(); + $succeeded++; + logMessage("Retry #{$row['retry_id']} succeeded (attempt " . ((int)$row['attempts'] + 1) . ')'); + continue; + } + + $newAttempts = (int)$row['attempts'] + 1; + if ($newAttempts >= (int)$row['max_attempts']) { + // Exhausted: leave the row (attempts is now == max_attempts, so the + // WHERE clause above naturally excludes it from future runs) rather + // than deleting it, so it stays visible for manual investigation. + $upd = $conn->prepare( + "UPDATE notification_retry_queue SET attempts = ?, last_error = ? WHERE retry_id = ?" + ); + $upd->bind_param('isi', $newAttempts, $result['error'], $row['retry_id']); + $upd->execute(); + $upd->close(); + $exhausted++; + logMessage("Retry #{$row['retry_id']} exhausted after {$newAttempts} attempts: {$result['error']}"); + continue; + } + + $delayMinutes = nextAttemptDelayMinutes($newAttempts); + $upd = $conn->prepare( + "UPDATE notification_retry_queue + SET attempts = ?, last_error = ?, next_attempt_at = DATE_ADD(NOW(), INTERVAL ? MINUTE) + WHERE retry_id = ?" + ); + $upd->bind_param('isii', $newAttempts, $result['error'], $delayMinutes, $row['retry_id']); + $upd->execute(); + $upd->close(); + $failed++; + logMessage("Retry #{$row['retry_id']} failed (attempt {$newAttempts}), next attempt in {$delayMinutes}m: {$result['error']}"); +} + +logMessage("Done: {$succeeded} succeeded, {$failed} rescheduled, {$exhausted} exhausted (of " . count($dueRows) . ' processed)'); diff --git a/helpers/NotificationHelper.php b/helpers/NotificationHelper.php index f4590e5..6535f2c 100644 --- a/helpers/NotificationHelper.php +++ b/helpers/NotificationHelper.php @@ -7,13 +7,16 @@ class NotificationHelper { // ─── Internal: fire a webhook ───────────────────────────────────────────── - private static function fire(array $payload): void + /** + * POST a payload to the configured Matrix webhook and report the raw + * result. Shared by fire() (best-effort, queues on failure) and + * cron/retry_failed_notifications.php (retries a previously-queued + * payload) so both use identical request handling. + * + * @return array{success: bool, http_code: ?int, error: ?string} + */ + public static function attemptDelivery(string $webhookUrl, array $payload): array { - $webhookUrl = $GLOBALS['config']['MATRIX_WEBHOOK_URL'] ?? null; - if (empty($webhookUrl)) { - return; - } - $ch = curl_init($webhookUrl); curl_setopt($ch, CURLOPT_HTTPHEADER, ['Content-Type: application/json']); curl_setopt($ch, CURLOPT_POST, 1); @@ -21,10 +24,11 @@ class NotificationHelper curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); curl_setopt($ch, CURLOPT_TIMEOUT, 10); // A slow-but-not-fully-hung hookshot endpoint could otherwise add up - // to the full CURLOPT_TIMEOUT per fire() call, and a single request - // can call fire() (via notifyWatchers/sendCommentNotification/etc.) - // more than once sequentially — capping just the connect phase keeps - // that from stacking into tens of seconds of added latency. + // to the full CURLOPT_TIMEOUT per call, and a single request can + // trigger more than one notification sequentially (via + // notifyWatchers/sendCommentNotification/etc.) — capping just the + // connect phase keeps that from stacking into tens of seconds of + // added latency. curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 3); $response = curl_exec($ch); @@ -32,12 +36,54 @@ class NotificationHelper $curlError = curl_error($ch); curl_close($ch); - $id = $payload['ticket_id'] ?? '?'; if ($curlError) { - error_log("Matrix webhook cURL error for ticket #{$id}: {$curlError}"); - } elseif ($httpCode < 200 || $httpCode >= 300) { - error_log("Matrix webhook failed for ticket #{$id}. HTTP {$httpCode}: {$response}"); + return ['success' => false, 'http_code' => null, 'error' => $curlError]; } + if ($httpCode < 200 || $httpCode >= 300) { + return ['success' => false, 'http_code' => $httpCode, 'error' => "HTTP {$httpCode}: {$response}"]; + } + return ['success' => true, 'http_code' => $httpCode, 'error' => null]; + } + + /** + * Persist a failed payload for later retry by + * cron/retry_failed_notifications.php. Best-effort: a DB failure here + * must not throw back into the original (already-failed) notification + * attempt — it just means this particular failure isn't retried, no + * worse than the pre-existing behavior. + */ + private static function queueForRetry(array $payload, string $error): void + { + try { + require_once dirname(__DIR__) . '/helpers/Database.php'; + $conn = Database::getConnection(); + $stmt = $conn->prepare( + "INSERT INTO notification_retry_queue (payload, last_error) VALUES (?, ?)" + ); + $payloadJson = json_encode($payload); + $stmt->bind_param("ss", $payloadJson, $error); + $stmt->execute(); + $stmt->close(); + } catch (Throwable $e) { + error_log('NotificationHelper: failed to queue notification for retry: ' . $e->getMessage()); + } + } + + private static function fire(array $payload): void + { + $webhookUrl = $GLOBALS['config']['MATRIX_WEBHOOK_URL'] ?? null; + if (empty($webhookUrl)) { + return; + } + + $result = self::attemptDelivery($webhookUrl, $payload); + if ($result['success']) { + return; + } + + $id = $payload['ticket_id'] ?? '?'; + error_log("Matrix webhook failed for ticket #{$id}: {$result['error']}"); + self::queueForRetry($payload, $result['error']); } private static function notifyUsers(): array diff --git a/migrations/007_notification_retry_queue.sql b/migrations/007_notification_retry_queue.sql new file mode 100644 index 0000000..fa995b8 --- /dev/null +++ b/migrations/007_notification_retry_queue.sql @@ -0,0 +1,20 @@ +-- Queue for Matrix webhook notifications that failed to send (#78). +-- +-- NotificationHelper::fire() previously logged a failed webhook post via +-- error_log() only, with no retry and no persistent record — once a +-- notification failed, it was gone. Failed payloads are now queued here and +-- retried by cron/retry_failed_notifications.php with exponential backoff. +-- +-- Safe to re-run. + +CREATE TABLE IF NOT EXISTS `notification_retry_queue` ( + `retry_id` int(11) NOT NULL AUTO_INCREMENT, + `payload` longtext CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL CHECK (json_valid(`payload`)), + `attempts` int(11) NOT NULL DEFAULT 0, + `max_attempts` int(11) NOT NULL DEFAULT 6, + `next_attempt_at` timestamp NULL DEFAULT current_timestamp(), + `last_error` varchar(500) DEFAULT NULL, + `created_at` timestamp NULL DEFAULT current_timestamp(), + PRIMARY KEY (`retry_id`), + KEY `idx_next_attempt` (`next_attempt_at`) +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci;