Fix issues found in multi-agent code review
Verified, high-confidence fixes from a project-wide review: - markdown.js: escape " and ' in the HTML-escape step. User-controlled image/link URLs and alt text were interpolated into "..." attributes without quote escaping, allowing attribute breakout and injected event handlers (stored XSS, only mitigated by CSP). Flagged independently by two reviewers. - cron/create_recurring_tickets.php & cron/cleanup_ratelimit.php: a mangled crontab example inside the docblock contained */ which closed the comment early, causing a fatal parse error — both cron jobs never ran. Rewrote the docblocks without a literal */. - update_ticket.php: validate visibility BEFORE the core DB write so an invalid payload can't leave the ticket updated while the request reports failure (which also skipped the audit delta and stats cache invalidation). - watch_ticket.php: GET watcher_count was capped at 6 (count of a LIMIT 6 list); use an unbounded COUNT(*) so it matches the POST path. - notifications.php: "assigned to me" LIKE pattern lacked a trailing delimiter, so user 12 also matched 120/123/etc.; anchor with }. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -59,7 +59,9 @@ $assignSql = "SELECT
|
||||
ORDER BY al.created_at DESC
|
||||
LIMIT 15";
|
||||
|
||||
$assignLike = '%"assigned_to":' . $userId . '%';
|
||||
// Match the exact JSON value with a trailing delimiter so user 12 doesn't also
|
||||
// match 120/123/etc. The assign detail is logged as {"assigned_to":<int>}.
|
||||
$assignLike = '%"assigned_to":' . (int)$userId . '}%';
|
||||
$stmt = $conn->prepare($assignSql);
|
||||
$stmt->bind_param('is', $userId, $assignLike);
|
||||
$stmt->execute();
|
||||
|
||||
+20
-15
@@ -127,6 +127,25 @@ try {
|
||||
];
|
||||
}
|
||||
|
||||
// Validate visibility BEFORE any DB write so a bad payload can't leave the
|
||||
// ticket half-updated (core fields committed but request reported as failed).
|
||||
$visibilityGroups = null;
|
||||
if (isset($data['visibility'])) {
|
||||
$visibilityGroups = $data['visibility_groups'] ?? null;
|
||||
// Convert array to comma-separated string if needed
|
||||
if (is_array($visibilityGroups)) {
|
||||
$visibilityGroups = implode(',', array_map('trim', $visibilityGroups));
|
||||
}
|
||||
|
||||
// Internal visibility requires at least one group
|
||||
if ($data['visibility'] === 'internal' && (empty($visibilityGroups) || trim($visibilityGroups) === '')) {
|
||||
return [
|
||||
'success' => false,
|
||||
'error' => 'Internal visibility requires at least one group to be specified'
|
||||
];
|
||||
}
|
||||
}
|
||||
|
||||
// Validate status transition using workflow model
|
||||
if ($currentTicket['status'] !== $updateData['status']) {
|
||||
$allowed = $this->workflowModel->isTransitionAllowed(
|
||||
@@ -160,22 +179,8 @@ try {
|
||||
return $response;
|
||||
}
|
||||
|
||||
// Handle visibility update if provided
|
||||
// Handle visibility update if provided (already validated above)
|
||||
if (isset($data['visibility'])) {
|
||||
$visibilityGroups = $data['visibility_groups'] ?? null;
|
||||
// Convert array to comma-separated string if needed
|
||||
if (is_array($visibilityGroups)) {
|
||||
$visibilityGroups = implode(',', array_map('trim', $visibilityGroups));
|
||||
}
|
||||
|
||||
// Validate internal visibility requires groups
|
||||
if ($data['visibility'] === 'internal' && (empty($visibilityGroups) || trim($visibilityGroups) === '')) {
|
||||
return [
|
||||
'success' => false,
|
||||
'error' => 'Internal visibility requires at least one group to be specified'
|
||||
];
|
||||
}
|
||||
|
||||
$visResult = $this->ticketModel->updateVisibility($id, $data['visibility'], $visibilityGroups, $this->userId);
|
||||
if ($visResult && $this->userId) {
|
||||
$this->auditLog->log(
|
||||
|
||||
@@ -103,7 +103,13 @@ while ($row = $watchersResult->fetch_assoc()) {
|
||||
$watchers[] = ['user_id' => (int)$row['user_id'], 'display_name' => $row['display_name']];
|
||||
}
|
||||
$watchersStmt->close();
|
||||
$count = count($watchers);
|
||||
|
||||
// True watcher count (the list above is capped at 6 for the avatar group)
|
||||
$countStmt = $conn->prepare("SELECT COUNT(*) AS cnt FROM ticket_watchers WHERE ticket_id = ?");
|
||||
$countStmt->bind_param("i", $ticketId);
|
||||
$countStmt->execute();
|
||||
$count = (int)$countStmt->get_result()->fetch_assoc()['cnt'];
|
||||
$countStmt->close();
|
||||
|
||||
echo json_encode([
|
||||
'success' => true,
|
||||
|
||||
@@ -25,10 +25,14 @@ function parseMarkdown(markdown) {
|
||||
|
||||
let html = markdown;
|
||||
|
||||
// Escape HTML first to prevent XSS
|
||||
// Escape HTML first to prevent XSS. Quotes MUST be escaped too: user-controlled
|
||||
// text (e.g. image/link URLs and alt text) is later interpolated into "..."
|
||||
// attributes, so an unescaped " would break out and inject event handlers.
|
||||
html = html.replace(/&/g, '&')
|
||||
.replace(/</g, '<')
|
||||
.replace(/>/g, '>');
|
||||
.replace(/>/g, '>')
|
||||
.replace(/"/g, '"')
|
||||
.replace(/'/g, ''');
|
||||
|
||||
// Ticket references (#123456789) - convert to clickable links
|
||||
html = html.replace(/#(\d{9})\b/g, '<a href="/ticket/$1" class="ticket-link-ref">#$1</a>');
|
||||
|
||||
@@ -6,12 +6,10 @@
|
||||
*
|
||||
* Cleans up expired rate limit files from the temp directory.
|
||||
* Should be run via cron every 5-10 minutes:
|
||||
* */
|
||||
|
||||
5 * * * * / usr / bin / php / path / to / cron / cleanup_ratelimit . php
|
||||
* 5 * * * * /usr/bin/php /path/to/cron/cleanup_ratelimit.php
|
||||
*
|
||||
* This script can also be run manually for immediate cleanup .
|
||||
* /
|
||||
* This script can also be run manually for immediate cleanup.
|
||||
*/
|
||||
|
||||
// Prevent web access
|
||||
if (php_sapi_name() !== 'cli') {
|
||||
|
||||
@@ -5,13 +5,11 @@
|
||||
* Recurring Tickets Cron Job
|
||||
*
|
||||
* Run this script via cron to automatically create tickets from recurring schedules.
|
||||
* Recommended: Run every 5-15 minutes
|
||||
* Recommended: run every 5-15 minutes.
|
||||
*
|
||||
* Example crontab entry:
|
||||
* */
|
||||
|
||||
10 * * * * / usr / bin / php / path / to / cron / create_recurring_tickets . php >> / var / log / recurring_tickets . log 2 > & 1
|
||||
* /
|
||||
* Example crontab entry (minute 10 of every hour):
|
||||
* 10 * * * * /usr/bin/php /path/to/cron/create_recurring_tickets.php >> /var/log/recurring_tickets.log 2>&1
|
||||
*/
|
||||
|
||||
// Change to project root directory
|
||||
chdir(dirname(__DIR__));
|
||||
|
||||
Reference in New Issue
Block a user