RecurringTicketModel::claimForRun() deliberately advances next_run_at
before TicketModel::createTicket() runs, to prevent duplicate-ticket
floods if creation fails partway and the cron retries. The tradeoff:
if createTicket() then fails, that specific occurrence is gone forever
with no record anywhere an admin would normally look — the catch
block only wrote a line to stdout/the cron log.
Added recordMissedOccurrence(), called from both the "createTicket()
returned success:false" branch and the exception catch, which writes
an audit_log entry (entity_type='recurring_ticket', action_type='error')
and fires a new NotificationHelper::sendSystemAlert() — a generic
operational alert (unlike the ticket-specific notification methods,
it has no associated ticket) sent to the shared MATRIX_NOTIFY_USERS
list regardless of any per-event toggle, so a silently-skipped
recurring ticket surfaces immediately instead of requiring someone to
grep cron logs.
Verified against real MariaDB and a real webhook-capturing server:
calling the recorder writes the audit_log row with the failure reason
and schedule details, and fires the Matrix alert with the same
information.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lhz7pGMaoTfL5sdYS5XiKv
NotificationHelper::fire() set CURLOPT_TIMEOUT (10s total) but no
CURLOPT_CONNECTTIMEOUT, so a slow-but-not-hung hookshot endpoint could
add up to the full 10s per fire() call — and a single request can call
fire() more than once sequentially (e.g. add_comment.php firing
mention + comment + watcher notifications back to back), stacking into
tens of seconds of added latency on the user-facing response.
Added a 3s CURLOPT_CONNECTTIMEOUT so a slow-to-connect endpoint fails
fast without needing the full request to time out. Verified the
webhook still fires correctly end-to-end against a real local HTTP
server after the change.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lhz7pGMaoTfL5sdYS5XiKv
README documented ErrorHandler.php as a "global error/exception
handler", but ErrorHandler::init() had exactly one caller app-wide
(api/get_template.php). 13 endpoints never called
ini_set('display_errors', 0) at all, relying on the server's global
php.ini default, and index.php never registered any handler — a
genuine PHP fatal during a page render fell through to PHP's raw
default handling with no app-level 500 response, styled or otherwise.
Investigating the "13 endpoints" claim turned up that 9 of them
(assign_ticket.php, audit_log.php, check_duplicates.php,
get_comments.php, get_users.php, notifications.php, saved_filters.php,
user_preferences.php, watch_ticket.php) already require
api/bootstrap.php as their first statement, which itself calls
ini_set('display_errors', 0) — so they were never actually exposed;
the static grep just couldn't see through the require. The 3 that
were genuinely unprotected (bulk_operation.php, download_attachment.php,
health.php) are fixed here. ticket_dependencies.php already has its
own complete hand-rolled equivalent (shutdown handler, error handler,
exception handler, output-buffer aware) and was deliberately left
alone rather than risk double-registering handlers.
Rather than duplicate the fix 30+ times, wired ErrorHandler::init()
directly into api/bootstrap.php (covering all 9 files above at once)
and into each of the other endpoints' own ini_set/error_reporting
pair, replacing it in place — additive only: existing try/catch blocks
in every endpoint still handle what they already handled identically,
this only adds a safety net for genuinely uncaught fatals that fell
through everything else. Before doing this app-wide, removed
ErrorHandler::init()'s override of PHP's 'error_log' ini setting: it
redirected every error_log() call in the request to a fixed /tmp file,
which would have silently diverted logs away from wherever the server
is actually configured to send them the moment this got wired into
more than one endpoint. That override only existed to support
getRecentErrors(), which has zero callers app-wide.
For index.php (page views, not JSON), added an 'html' response mode to
ErrorHandler that renders a new views/error_500.php instead of a JSON
body. That view is deliberately self-contained (no layout_header.php,
no $GLOBALS/session/DB dependency) since a genuine fatal can happen
before config.php finishes loading or mid-session-start.
Verified: a real uncaught error with no prior output correctly
produces a clean JSON 500 (API mode) or the styled HTML page (page
mode) to the client while the full stack trace goes to error_log, not
the response; normal (non-fatal) requests through both a
bootstrap.php-based endpoint and index.php are byte-for-byte
unaffected. Full project phpcs pass is clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lhz7pGMaoTfL5sdYS5XiKv
sendAssignmentNotification() had the same missing-visibility gap as
#46 in a separate function: assigning a user to a confidential/internal
ticket broadcast the ticket title to the shared Matrix notify list
unconditionally when MATRIX_NOTIFY_ASSIGNMENTS is enabled.
Threaded visibility through using the same redactedTitle() helper
added for #46, wired up from the already-fetched ticket row in
assign_ticket.php. The assignee is still DMed directly regardless,
since being assigned gives them standing access to the ticket — but
because notify_users is one shared payload, they see the same redacted
title as everyone else on it rather than a personalized one. Verified
end-to-end with a local HTTP server capturing the webhook payload for
both public and confidential tickets.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015nCxwFFsy8ouMWzn56rPVP
notifyWatchers() only redacted the comment/activity preview for
non-public tickets — the shared MATRIX_NOTIFY_USERS list was still
merged into notify_users unconditionally, and the ticket title was
never redacted at all. A status-change/comment notification for a
confidential ticket with watchers still broadcast that ticket's title
to the shared list, even though the function's own docblock intended
to protect non-public tickets from it.
For non-public tickets, the shared list is now excluded entirely
(only actual watchers are notified) and the title is redacted via the
same redactedTitle() helper added for #46. Verified against real
MariaDB with a real watcher row: for a confidential ticket, the
captured webhook payload has only the watcher's Matrix ID (no shared
list) and a redacted title; for the same ticket made public, the
shared list is included and the title passes through unchanged.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015nCxwFFsy8ouMWzn56rPVP
sendTicketNotification() and sendStatusChangeNotification() always sent
the ticket title to the shared MATRIX_NOTIFY_USERS list regardless of
visibility, unlike sendCommentNotification()/notifyWatchers() which
already redact the comment/activity preview for non-public tickets.
Creating or changing the status of a confidential ticket broadcast its
title to a shared Matrix room, defeating the point of the Confidential
visibility level.
Added a shared redactedTitle() helper and threaded visibility through
both functions (sendTicketNotification reads it from the existing
$ticketData['visibility'] key; sendStatusChangeNotification takes a new
optional parameter, wired up in both callers from the already-fetched
ticket row). Verified end-to-end with a local HTTP server capturing the
actual webhook payloads: public tickets pass the title through
unchanged, confidential/internal tickets get the redacted placeholder.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015nCxwFFsy8ouMWzn56rPVP
ticket_watchers.ticket_id was int(11) while every other satellite
table (ticket_comments, ticket_attachments, ticket_dependencies,
custom_field_values) uses varchar(9)/varchar(10) matching
tickets.ticket_id, and it had no FK constraint at all — unlike every
other satellite table — so orphaned watcher rows could never be
caught by referential integrity.
Changed the column to varchar(9) with an ON DELETE CASCADE FK to
tickets, in both 000_baseline.sql and a new idempotent
004_fix_ticket_watchers_type.sql (which also deletes any pre-existing
orphaned watcher rows before adding the constraint, since orphans
would otherwise make the ADD CONSTRAINT fail). Updated
watch_ticket.php, NotificationHelper::notifyWatchers(), and
notifications.php's audit-log JOIN to bind/compare ticket_id as a
string instead of casting to int, including replacing a fragile
CAST(entity_id AS UNSIGNED) with a direct string comparison.
Verified against real MariaDB: applied 004 against a simulated
pre-fix deployment with one valid and one orphaned watcher row —
the orphan is removed, the column converts losslessly, the FK is
added, and the migration is idempotent on re-run. Confirmed
ON DELETE CASCADE actually removes watchers when their ticket is
deleted, that inserting a watcher for a nonexistent ticket now fails
with a real FK violation, and exercised the updated watch/unwatch and
status-change-notification query paths end-to-end against the fixed
schema.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015nCxwFFsy8ouMWzn56rPVP
remember() had no protection against a slow cache-miss recomputation
overwriting a fresher write. If Request A started computing stats just
before a ticket mutation + invalidateCache(), and Request B started
just after (correctly computing fresh, post-mutation data), A could
finish (using stale pre-mutation data) after B and overwrite B's fresh
cache entry — extending staleness by up to another full TTL.
Added a per-prefix invalidation epoch: delete() bumps it, and
remember() snapshots it before running the callback and only writes
if the epoch hasn't changed since — otherwise a newer invalidation
happened mid-computation and the result being written is already
stale, so it's dropped (the caller still gets its own result; only the
cache write is skipped).
Verified with two real concurrent PHP processes racing against the
same cache key (a slow "Request A" callback vs. a fast "Request B"
that invalidates then recomputes): the cache ends up holding B's fresh
value, not A's late stale overwrite.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X
Zero callers anywhere in the app — confirmed via grep for
"OutputHelper::" across the whole codebase. Every view actually calls
htmlspecialchars() directly instead, which a prior audit confirmed is
done consistently, so escaping was never actually at risk. This was
just a misleading, unused class that README.md's file reference
implied was part of the app's active XSS-prevention story. Removed the
file and its README Project Structure entry.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X
The hosts were upgraded to PHP 8.4, where mysqli::ping() is deprecated
(auto-reconnect was removed in 8.2). Database::getConnection() called it on
every reused connection, and api/ticket_dependencies.php's custom error
handler treated the deprecation as a fatal 500 ('A server error occurred'),
breaking the ticket Dependencies tab.
- Database.php: remove the redundant ping()/reconnect check (connection is
request-scoped; no liveness check needed on PHP 8.2+).
- ticket_dependencies.php: only abort on genuine errors; log notices/
warnings/deprecations and continue, so a future deprecation can't 500 it.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Database.php: pin MySQL session time_zone to the configured named zone
(mysql.time_zone tables now loaded on the DB) with a fixed-offset
fallback, so NOW()/TIMESTAMP and PHP agree regardless of the DB server's
SYSTEM tz. Best-effort, never fatals the connection.
- NotificationHelper: redact comment-body previews for internal/
confidential tickets in sendCommentNotification and notifyWatchers so
they are not leaked to the shared Matrix notify list (new $visibility
param; callers wired in the API batch).
- config.php: die with a clear error if parse_ini_file fails instead of
silently falling back to insecure defaults (empty DB pass / proxies).
- CacheHelper: create cache dir 0700 and cache files 0600 so other local
users cannot read or poison security-relevant cached data.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- SynapseHelper: memoize username->Matrix-ID lookups per-request (incl. negative
results) and add an overall time budget to resolveUsernames() plus a 2s connect
timeout, so notifying N watchers with a slow/unreachable Synapse can't stall the
request for N x 5s. (Chosen over async/queue per maintainer.)
- DependencyModel: fix cycle detection treating 'blocks' and 'blocked_by' as the
same edge direction. They are inverse relationships (single row each, no mirror
row), so the traversal now walks a unified precedence graph (blocks: ticket->
depends_on; blocked_by: depends_on->ticket) and wouldCreateCycle normalizes the
new edge's direction. Prevents both false-positive and missed cycles.
- CacheHelper: anchor prefix-delete to exact key boundaries (bare prefix or
prefix + '_' + md5) so delete('workflow') can't wipe a 'workflow_rules' cache.
- RateLimitMiddleware: hold an exclusive flock across the per-IP counter's
read-modify-write so concurrent requests can't both read N and write N+1
(undercounting past the limit). Fails open if the file can't be locked.
- dashboard.js: kanban status update now uses lt.api.post (per no-raw-fetch
convention) and reverts the card AND the optimistic column counts on failure
(the old raw-fetch catch left the card moved without reverting).
- BulkOperationsModel: document that bulk_status/bulk_close intentionally bypass
workflow transition validation (admin override, by design).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- create_ticket_api.php: validate status (against TICKET_STATUSES) and
priority (numeric 1-5). A non-numeric priority previously cast to 0 and
escalated the ticket below P1 on the dedup/update path.
- manage_workflows.php: reject empty/invalid from_status/to_status on POST
and PUT (must be valid ticket statuses) so the workflow table can't be
populated with bogus transitions.
- TicketModel::getAllTickets: COUNT(*) OVER() rides on returned rows, so a
page past the last row returned total/pages = 0. Fall back to a direct
COUNT when an over-range page yields no rows, keeping pager math correct.
- DashboardView: stop double-escaping category/type/assigned active-filter
labels (they were htmlspecialchars'd into the label and again at output,
rendering R&D as R&D); output escaping is retained.
- check_duplicates.php / NotificationHelper::notifyWatchers: wrap the DB
lookups in try/catch so a failed prepare/query degrades gracefully
(advisory dup-check returns none; best-effort watcher notify is skipped)
instead of fataling the request. Works whether mysqli throws or returns
false. (manage_* endpoints already have a top-level try/catch.)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
XSS / security:
- markdown.js: sanitize footnote labels to a safe slug before using them in
id/href attributes. Labels are captured before the HTML-escape pass, so a
label like x"><img onerror=...> broke out → stored XSS (the earlier quote-
escape fix didn't cover this path). Verified neutralized.
- RateLimitMiddleware: only trust X-Forwarded-For / X-Real-IP when REMOTE_ADDR
is a configured trusted proxy, and use the rightmost (proxy-appended) entry.
Previously any client could rotate XFF to escape the per-IP rate limit.
- .env.example: document TRUSTED_PROXIES so fresh deploys aren't fail-open on
the Authelia forward-auth spoofing protection.
Correctness:
- notifications.php: my previous assigned-to LIKE fix anchored only on '}', so
BULK assignments (logged {"assigned_to":N,"bulk_operation_id":..}) produced
no "assigned to you" notification — now matches both '}' and ',' delimiters.
- notifications.php: implement the documented @mention notifications (query
action_type='mention' rows for the current user); they were never delivered.
- NotificationHelper::notifyWatchers: guard unchecked prepare() so a missing
ticket_watchers table can't fatal the request after its DB write committed.
- AuditLogModel::getTicketTimeline: JSON_UNQUOTE the extracted ticket_id so
comment events actually match (string vs JSON-number comparison never did).
- AuditLogModel/audit_log.php: CSV export no longer silently truncates to the
1000-row UI cap; uses a dedicated higher export limit.
- DashboardView: quick-preview drawer read .ticket-link from the title cell
(which has none), so the title was always blank — use the cell text.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- NotificationHelper::notifyWatchers: excludeUserId parameter was
accepted but never used; actors were notified of their own actions.
Fix: add AND tw.user_id != ? clause to watcher query when exclusion
is requested.
- TicketView.php: formatAction() default case returned raw
$event['action_type'] unescaped into HTML context. Fix: wrap with
htmlspecialchars().
- Admin views: field_id, recurring_id, template_id, transition_id
in data-id attributes were uncast; field_type was unescaped in
CustomFieldsView; from/to_status slugs derived from DB values were
used directly in class attributes in WorkflowDesignerView.
Fix: (int) cast for IDs, htmlspecialchars for field_type,
preg_replace to sanitize DB-derived CSS class slugs.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rawurlencode($username) was called on line 38 (encoding the username),
then rawurlencode($matrixId) was called on line 39 encoding the already-
encoded string — causing %20 to become %2520 for usernames with special
characters. Fixed by building $matrixId with the plain username and only
encoding the full Matrix ID once in the URL path.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Ticket watchers:
- api/watch_ticket.php: GET (watch state) + POST (watch/unwatch toggle)
- index.php: route for /api/watch_ticket.php
- TicketView: WATCH/UNWATCH button with live state fetch and toggle
- NotificationHelper::notifyWatchers(): fetches watchers from DB, resolves
Matrix IDs via Synapse, fires notification to watchers + global list
- add_comment.php, update_ticket.php: call notifyWatchers on comment and
status-change events respectively
Fulltext search:
- TicketModel::hasFulltextIndex(): detects FULLTEXT index via information_schema
- getAllTickets(): uses MATCH...AGAINST when fulltext index exists, LIKE fallback
when not yet applied — zero-downtime rollout
Single-query pagination:
- getAllTickets() replaces separate COUNT + SELECT with COUNT(*) OVER() window
function — one round trip to DB per page load instead of two
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment pagination:
- CommentModel: add getCommentCount(), paginated getCommentsByTicketId()
with getThreadedCommentsPaged() for threading + LIMIT/OFFSET
- TicketController: load first 50 root comments + total count on page load
- api/get_comments.php: new AJAX endpoint for Load More (index.php routed)
- TicketView: Load More button + buildCommentEl() JS renderer for AJAX comments;
passes totalComments/commentOffset/isAdmin to window.ticketData
Matrix integration:
- NotificationHelper: add sendStatusChangeNotification(), sendCommentNotification(),
sendMentionNotification(), sendAssignmentNotification() alongside existing
sendTicketNotification(); internal fire() helper replaces duplicated cURL logic
- SynapseHelper: new helper that resolves SSO usernames → Matrix IDs by querying
Synapse Admin REST API directly (no caching, no stale data)
- config.php: add SYNAPSE_ADMIN_URL, SYNAPSE_ADMIN_TOKEN, MATRIX_NOTIFY_COMMENTS,
MATRIX_NOTIFY_ASSIGNMENTS config keys (all from .env)
- api/update_ticket.php: fire status-change notification after successful save
- api/add_comment.php: resolve @mentioned usernames via SynapseHelper and fire
mention notification; fire general comment notification when MATRIX_NOTIFY_COMMENTS=1
- api/assign_ticket.php: fire assignment notification (resolves assignee via Synapse)
when MATRIX_NOTIFY_ASSIGNMENTS=1
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add helpers/NotificationHelper.php: shared Matrix webhook sender
that reads MATRIX_WEBHOOK_URL and MATRIX_NOTIFY_USERS from config
- Remove sendDiscordWebhook() from TicketController; call
NotificationHelper::sendTicketNotification() instead
- Replace 60-line Discord embed block in create_ticket_api.php
with a single NotificationHelper call
- config/config.php: DISCORD_WEBHOOK_URL → MATRIX_WEBHOOK_URL +
new MATRIX_NOTIFY_USERS key (comma-separated Matrix user IDs)
- .env.example: updated env var names and comments
Payload sent to hookshot includes notify_users array so the
JS transform can build proper @mention links for each user.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add ErrorHandler class for consistent error handling and logging
- Provides methods for common error responses (401, 403, 404, 422, 500)
- Includes error logging to temp directory
- Update get_template.php to use ErrorHandler (example migration)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add CacheHelper for file-based caching with TTL support
- Add Database helper for centralized connection management
- Update WorkflowModel to cache status transitions (10 min TTL)
- Update UserPreferencesModel to cache user prefs (5 min TTL)
- Update manage_workflows.php to clear cache on changes
- Update get_users.php to use Database helper (example migration)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
jaredandClaude Sonnet 5