04/06 remediate some issues

app/__init__.py

@@ -166,13 +166,38 @@ def _seed_settings():
 
 
 def _seed_admin(app):
-    """Create the default admin account if none exists."""
+    """Create the default admin account if none exists.
+
+    Username collision guard
+    -----------------------
+    The seed username is hardcoded to 'admin'. If a user registered with
+    that username before the first admin seed runs (possible when
+    registration_enabled=True on a fresh install), the INSERT would raise
+    an IntegrityError and break application startup.
+
+    We guard against this by checking for username conflicts independently
+    of the role check, and falling back to a derived username when 'admin'
+    is already taken.
+    """
     from app.models import User, UserRole
     if User.query.filter_by(role=UserRole.ADMIN).first():
         return
+
+    # Resolve a safe username — 'admin' is preferred but may already be taken.
+    seed_username = 'admin'
+    if User.query.filter_by(username=seed_username).first():
+        # Derive a unique fallback so startup never fails on a collision.
+        import uuid
+        seed_username = f'admin_{uuid.uuid4().hex[:6]}'
+        app.logger.warning(
+            f'[SEED] Username "admin" is already taken — '
+            f'seeding admin account with username "{seed_username}". '
+            f'Rename via Admin → Users after first login.'
+        )
+
     admin = User(
         email     = app.config['ADMIN_EMAIL'],
-        username  = 'admin',
+        username  = seed_username,
         full_name = 'System Administrator',
         role      = UserRole.ADMIN,
         department= 'IT',
@@ -181,4 +206,4 @@ def _seed_admin(app):
     admin.set_password(app.config['ADMIN_PASSWORD'])
     db.session.add(admin)
     db.session.commit()
-    app.logger.info(f'[SEED] Default admin account created: {admin.email}')
+    app.logger.info(f'[SEED] Default admin account created: {admin.email} (username={seed_username})')

app/models.py

@@ -255,7 +255,7 @@ class ActivityLog(db.Model):
     __tablename__ = 'activity_logs'
 
     id          = db.Column(db.Integer, primary_key=True)
-    user_id     = db.Column(db.Integer, db.ForeignKey('users.id'))
+    user_id     = db.Column(db.Integer, db.ForeignKey('users.id', ondelete='SET NULL'))
     action      = db.Column(db.String(100), nullable=False)
     entity_type = db.Column(db.String(50))
     entity_id   = db.Column(db.Integer)

app/routes/admin.py

@@ -9,7 +9,7 @@ from flask import Blueprint, render_template, redirect, url_for, flash, request,
 from flask_login import login_required, current_user
 from werkzeug.utils import secure_filename
 import bleach
-from app import db
+from app import db, limiter
 from app.models import (User, Ticket, Comment, ActivityLog, KnowledgeBase,
                         KBAttachment, UserRole, TicketStatus)
 from app.services.log_service import log_action
@@ -404,8 +404,10 @@ def all_tickets():
 
     if search:
         from app.models import Comment
+        from sqlalchemy import func
         submitter_alias = db.aliased(User)
         assignee_alias  = db.aliased(User)
+        stripped_body   = func.regexp_replace(Comment.body, r'<[^>]+>', '', 'g')
         q = (
             q
             .outerjoin(submitter_alias, submitter_alias.id == Ticket.created_by_id)
@@ -417,7 +419,7 @@ def all_tickets():
                 Ticket.description.ilike(f'%{search}%') |
                 submitter_alias.full_name.ilike(f'%{search}%') |
                 assignee_alias.full_name.ilike(f'%{search}%') |
-                Comment.body.ilike(f'%{search}%')
+                stripped_body.ilike(f'%{search}%')
             )
             .distinct()
         )
@@ -540,6 +542,7 @@ def kb_upload_image():
 # ── File-serve route (images embedded in articles + attachment downloads) ─────
 
 @admin_bp.route('/kb/files/<string:stored_name>')
+@login_required
 def kb_serve_file(stored_name):
     """Serve a KB attachment file. Login required — no public access.
 
@@ -551,6 +554,12 @@ def kb_serve_file(stored_name):
     traverse outside the upload directory. <string:> disallows slashes,
     restricting the value to a flat filename — matching the UUID-based
     stored_name format (e.g. 'a1b2c3d4e5f6....png') used by all upload helpers.
+
+    @login_required is applied here because the upload directory is shared
+    across KB files, ticket attachments, comment images, and user avatars.
+    Without authentication, an unauthenticated caller who knows or guesses
+    any stored_name (UUID-based) could retrieve arbitrary files from the
+    shared uploads folder — including confidential ticket attachments.
     """
     upload_dir = current_app.config['UPLOAD_FOLDER']
     return send_from_directory(upload_dir, stored_name)
@@ -572,7 +581,6 @@ def kb_delete_attachment(article_id, att_id):
     logger.info(f'[KB ATTACHMENT DELETE] att_id={att.id} article_id={article_id} by user_id={current_user.id}')
     db.session.delete(att)
     db.session.commit()
-    logger.info(f'[KB ATTACHMENT DELETE] att_id={att.id} article_id={article_id} completed')
     # Return JSON so the edit page can remove the row without a full reload
     return jsonify({'ok': True, 'att_id': att.id})
 
@@ -766,6 +774,7 @@ def activity_logs():
 @admin_bp.route('/tickets/export')
 @login_required
 @admin_required
+@limiter.limit('10 per hour')
 def export_tickets():
     """Stream a CSV of tickets matching the current filter params."""
     status   = request.args.get('status', '')
@@ -780,8 +789,10 @@ def export_tickets():
     elif assigned == 'unassigned': q = q.filter_by(assigned_to_id=None)
 
     if search:
+        from sqlalchemy import func
         submitter_alias = db.aliased(User)
         assignee_alias  = db.aliased(User)
+        stripped_body   = func.regexp_replace(Comment.body, r'<[^>]+>', '', 'g')
         q = (
             q
             .outerjoin(submitter_alias, submitter_alias.id == Ticket.created_by_id)
@@ -793,7 +804,7 @@ def export_tickets():
                 Ticket.description.ilike(f'%{search}%') |
                 submitter_alias.full_name.ilike(f'%{search}%') |
                 assignee_alias.full_name.ilike(f'%{search}%') |
-                Comment.body.ilike(f'%{search}%')
+                stripped_body.ilike(f'%{search}%')
             )
             .distinct()
         )
@@ -845,6 +856,7 @@ def _roles():
 
 
 @admin_bp.route('/settings', methods=['GET', 'POST'])
+@login_required
 @admin_required
 def settings():
     from app.models import SystemSetting

app/routes/chatbot.py

@@ -42,22 +42,46 @@ def _call_groq(api_key, history, user_msg):
     The system prompt is prepended as a system message. Prior history and the
     new user message are appended in order.
 
+    History is capped at the most recent _MAX_HISTORY_TURNS turns and each
+    message content is truncated to _MAX_MSG_CHARS characters before being
+    forwarded. This prevents a malicious or runaway client from exhausting
+    the model's context window or inflating token costs.
+
     Raises requests.HTTPError or requests.exceptions.RequestException on failure.
     """
+    # ── History sanitisation ──────────────────────────────────────────────────
+    # 1. Strip create_ticket action blocks — re-sending them causes the model
+    #    to re-trigger ticket creation on every subsequent turn.
+    # 2. Cap to the most recent N turns so the client cannot inflate context.
+    # 3. Truncate each message's content to avoid per-message token blowout.
+    _MAX_HISTORY_TURNS = 20
+    _MAX_MSG_CHARS     = 2000
+
     model = current_app.config.get('GROQ_MODEL', _GROQ_MODEL)
 
-    # Strip any assistant messages containing a create_ticket action block.
-    # These should never reach the model — if they do, the model re-triggers
-    # ticket creation on every subsequent turn.
     clean_history = [
         msg for msg in history
         if not (msg.get('role') == 'assistant' and '"action": "create_ticket"' in msg.get('content', ''))
     ]
 
+    # Keep only the most recent turns after filtering
+    if len(clean_history) > _MAX_HISTORY_TURNS:
+        logger.warning(
+            f'[CHATBOT] history truncated from {len(clean_history)} to '
+            f'{_MAX_HISTORY_TURNS} turns for user_id={current_user.id}'
+        )
+        clean_history = clean_history[-_MAX_HISTORY_TURNS:]
+
+    # Truncate individual message content lengths
+    clean_history = [
+        {**msg, 'content': msg.get('content', '')[:_MAX_MSG_CHARS]}
+        for msg in clean_history
+    ]
+
     messages = (
         [{'role': 'system', 'content': _SYSTEM_PROMPT}]
         + clean_history
-        + [{'role': 'user', 'content': user_msg}]
+        + [{'role': 'user', 'content': user_msg[:_MAX_MSG_CHARS]}]
     )
     resp = requests.post(
         _GROQ_API_URL,

app/routes/tickets.py

@@ -25,6 +25,23 @@ logger     = logging.getLogger(__name__)
 ALLOWED_EXT = {'png', 'jpg', 'jpeg', 'gif', 'pdf', 'doc', 'docx', 'txt', 'zip', 'log'}
 
 
+def _strip_html(text: str) -> str:
+    """Remove HTML tags from *text* for plain-text search matching.
+
+    Comment bodies are stored as sanitized HTML (rendered at write time via
+    render_comment_body).  Searching with ilike('%term%') against raw HTML
+    produces two problems:
+      1. A search for 'bold' misses '<strong>bold</strong>' in the stored body.
+      2. HTML tag names ('strong', 'pre') can accidentally match search terms.
+    Stripping tags before comparison gives consistent, tag-agnostic results.
+
+    Uses a simple regex rather than a full HTML parser — sufficient for the
+    sanitized subset of HTML that bleach allows in comment bodies.
+    """
+    import re
+    return re.sub(r'<[^>]+>', '', text)
+
+
 def _resolve_mime_type(att):
     """Return a reliable MIME type for an attachment.
 
@@ -195,8 +212,13 @@ def ticket_list():
     if search:
         # Extend search to cover comments and assignee name via outer joins.
         # distinct() prevents duplicate ticket rows when multiple comments match.
+        # Comment bodies are stored as sanitized HTML — use REGEXP_REPLACE to
+        # strip tags at the SQL level before matching so 'bold' finds
+        # '<strong>bold</strong>' and HTML tag names don't pollute results.
         from app.models import Comment
+        from sqlalchemy import func
         assignee_alias = db.aliased(User)
+        stripped_body  = func.regexp_replace(Comment.body, r'<[^>]+>', '', 'g')
         query = (
             query
             .outerjoin(Comment, Comment.ticket_id == Ticket.id)
@@ -205,7 +227,7 @@ def ticket_list():
                 Ticket.title.ilike(f'%{search}%') |
                 Ticket.ticket_number.ilike(f'%{search}%') |
                 Ticket.description.ilike(f'%{search}%') |
-                Comment.body.ilike(f'%{search}%') |
+                stripped_body.ilike(f'%{search}%') |
                 assignee_alias.full_name.ilike(f'%{search}%')
             )
             .distinct()
@@ -390,11 +412,31 @@ def update_ticket(ticket_id):
 @login_required
 def delete_comment(comment_id):
     comment   = db.session.get(Comment, comment_id) or abort(404)
-    if not current_user.is_it_staff and comment.author_id != current_user.id:
-        abort(403)
     ticket_id = comment.ticket_id
+
+    # Authorization: IT staff may always delete any comment.
+    # Employees may only delete their own comments, and only while the
+    # ticket is still open or in-progress. Allowing deletion on resolved
+    # or closed tickets would silently alter the historical record of a
+    # completed support interaction.
+    if not current_user.is_it_staff:
+        if comment.author_id != current_user.id:
+            abort(403)
+        ticket = db.session.get(Ticket, ticket_id) or abort(404)
+        if ticket.status in (TicketStatus.RESOLVED, TicketStatus.CLOSED):
+            logger.warning(
+                f'[COMMENT DELETE BLOCKED] comment_id={comment.id} '
+                f'ticket_id={ticket_id} status={ticket.status} '
+                f'user_id={current_user.id} — ticket is {ticket.status}'
+            )
+            flash('Comments cannot be deleted on resolved or closed tickets.', 'warning')
+            return redirect(url_for('tickets.ticket_detail', ticket_id=ticket_id))
+
+    # Include a truncated snapshot of the body in the audit log so the
+    # content is recoverable from logs even after the DB row is gone.
+    body_snapshot = comment.body[:200].replace('\n', ' ')
     log_action(current_user.id, 'comment_delete', 'comment', comment.id,
-               f'ticket_id={ticket_id}')
+               f'ticket_id={ticket_id} body_snapshot="{body_snapshot}"')
     logger.info(f'[COMMENT DELETE] comment_id={comment.id} ticket_id={ticket_id} by user_id={current_user.id}')
     db.session.delete(comment)
     db.session.commit()

app/services/notification_service.py

@@ -1,4 +1,5 @@
 import logging
+import re
 from flask import current_app, render_template_string
 from flask_mail import Message
 from app import db, mail, socketio
@@ -7,6 +8,17 @@ from app.models import Notification, NotificationType, User, UserRole
 logger = logging.getLogger(__name__)
 
 
+def _strip_tags(html: str) -> str:
+    """Strip HTML tags from a stored comment body for use in notification text.
+
+    Comment bodies are persisted as sanitized HTML. When a snippet is included
+    in an in-app notification message or email, raw tags like <strong>, <p>
+    render as literal text in notification dropdowns and look unsightly.
+    This helper produces a clean plain-text preview for those contexts.
+    """
+    return re.sub(r'<[^>]+>', '', html or '')
+
+
 # ─── Email Templates ──────────────────────────────────────────────────────────
 
 _NEW_TICKET_EMAIL = """
@@ -63,7 +75,22 @@ def _priority_badge_color(priority):
 # ─── In-App Notification ──────────────────────────────────────────────────────
 
 def create_notification(user_id, notif_type, title, message, ticket_id=None, link=None):
-    """Persist an in-app notification and push via WebSocket."""
+    """Persist an in-app notification and push via WebSocket.
+
+    Transaction note
+    ----------------
+    This function does NOT call db.session.commit(). It flushes the new
+    Notification row to surface constraint violations early, then defers
+    the final commit to the caller. This keeps all notifications for a
+    given event (e.g. notifying every IT staff member on a new ticket)
+    in a single atomic transaction rather than N separate commits, and
+    prevents partial notification state if an error occurs mid-loop.
+
+    Callers that use create_notification in a loop (notify_new_ticket,
+    notify_comment_added) must commit after the loop completes.
+    Callers that use it standalone (notify_status_change, notify_assignment)
+    must also commit after calling this function.
+    """
     try:
         notif = Notification(
             user_id  = user_id,
@@ -74,7 +101,9 @@ def create_notification(user_id, notif_type, title, message, ticket_id=None, lin
             link     = link,
         )
         db.session.add(notif)
-        db.session.commit()
+        # Flush to obtain notif.id for the WebSocket payload without committing.
+        # The caller is responsible for the final db.session.commit().
+        db.session.flush()
         logger.info(f'[NOTIFICATION CREATE] user_id={user_id} type={notif_type} ticket_id={ticket_id}')
 
         # Real-time push — schedule via socketio.start_background_task so the
@@ -99,7 +128,13 @@ def create_notification(user_id, notif_type, title, message, ticket_id=None, lin
         socketio.start_background_task(_emit)
 
     except Exception as exc:
-        db.session.rollback()
+        # Expunge only the failed notification entry — do NOT roll back the
+        # full session, as that would undo the parent operation (e.g. a ticket
+        # update) that triggered this notification call.
+        try:
+            db.session.expunge(notif)
+        except Exception:
+            pass
         logger.error(f'[NOTIFICATION ERROR] Failed to create notification: {exc}')
 
 
@@ -177,6 +212,10 @@ def notify_new_ticket(ticket):
         ticket_id = ticket.id,
         link      = f'/tickets/{ticket.id}',
     )
+    # Commit all notifications in a single transaction.
+    # create_notification() flushes but does not commit — the loop above
+    # accumulates all Notification rows and this single commit persists them all.
+    db.session.commit()
 
 
 def notify_status_change(ticket, old_status, changed_by):
@@ -205,6 +244,7 @@ def notify_status_change(ticket, old_status, changed_by):
         ticket_id = ticket.id,
         link      = f'/tickets/{ticket.id}',
     )
+    db.session.commit()
     logger.info(f'[TICKET STATUS] ticket_id={ticket.id} {old_status} -> {ticket.status} by user_id={changed_by.id}')
 
 
@@ -254,11 +294,15 @@ def notify_comment_added(comment):
         if user_id in notified:
             return
         notified.add(user_id)
+        # Strip HTML tags from the stored comment body before embedding in
+        # the notification message — raw tags render as literal text in the
+        # in-app notification dropdown and look unsightly.
+        plain_body = _strip_tags(comment.body)
         create_notification(
             user_id   = user_id,
             notif_type= NotificationType.COMMENT_ADDED,
             title     = f'New Comment on {ticket.ticket_number}',
-            message   = f'{comment.author.full_name} commented: {comment.body[:100]}',
+            message   = f'{comment.author.full_name} commented: {plain_body[:100]}',
             ticket_id = ticket.id,
             link      = f'/tickets/{ticket.id}#comment-{comment.id}',
         )
@@ -267,7 +311,7 @@ def notify_comment_added(comment):
             html = render_template_string(_STATUS_UPDATE_EMAIL,
                 ticket_number = ticket.ticket_number,
                 title         = ticket.title,
-                message       = f'{comment.author.full_name} added a comment: {comment.body[:300]}',
+                message       = f'{comment.author.full_name} added a comment: {plain_body[:300]}',
                 ticket_url    = ticket_url,
             )
             send_email(
@@ -284,6 +328,9 @@ def notify_comment_added(comment):
     if ticket.assigned_to_id and ticket.assigned_to_id != comment.author_id:
         _notify(ticket.assigned_to_id, is_internal=comment.is_internal)
 
+    # Commit all accumulated Notification rows in a single transaction.
+    # create_notification() flushes but does not commit.
+    db.session.commit()
     logger.info(f'[COMMENT ADD] comment_id={comment.id} ticket_id={ticket.id} author_id={comment.author_id} internal={comment.is_internal}')
 
 
@@ -302,6 +349,7 @@ def notify_assignment(ticket, assigned_by):
         ticket_id = ticket.id,
         link      = f'/tickets/{ticket.id}',
     )
+    db.session.commit()
     if ticket.assignee and ticket.assignee.email_notif:
         html = render_template_string(_STATUS_UPDATE_EMAIL,
             ticket_number = ticket.ticket_number,

app/services/validation_service.py

@@ -24,16 +24,26 @@ def validate_password(password: str, confirm: str) -> str | None:
     -----
     - Password and confirmation must match.
     - Minimum length: 8 characters.
+    - Must contain at least one uppercase letter (A-Z).
+    - Must contain at least one digit (0-9).
+    - Must contain at least one special character (!@#$%^&* etc.).
 
     Parameters
     ----------
     password : str  – the candidate password (plain text)
     confirm  : str  – the confirmation field value
     """
+    import re
     if password != confirm:
         return 'Passwords do not match.'
     if len(password) < 8:
         return 'Password must be at least 8 characters.'
+    if not re.search(r'[A-Z]', password):
+        return 'Password must contain at least one uppercase letter.'
+    if not re.search(r'\d', password):
+        return 'Password must contain at least one number.'
+    if not re.search(r'[!@#$%^&*()\-_=+\[\]{};:\'",.<>?/\\|`~]', password):
+        return 'Password must contain at least one special character.'
     return None
 
 
@@ -163,10 +173,22 @@ def _group_by_alternative(
     [[gif87_sig], [gif89_sig]] so the caller can treat each inner list as
     a complete match candidate.
 
-    The rule: each entry with offset=0 starts a new alternative group.
-    Entries with offset>0 are appended to the current group (they are
-    additional constraints on the same file type, e.g. WEBP needs both
-    offset-0 'RIFF' and offset-8 'WEBP').
+    Grouping rule
+    -------------
+    Each entry with offset=0 starts a **new alternative** group.
+    Entries with offset>0 are appended to the **current** group — they
+    represent additional byte constraints that must ALL match alongside
+    the group's offset-0 anchor (e.g. WEBP requires both RIFF at offset 0
+    AND 'WEBP' at offset 8 within the same file).
+
+    ⚠️  Constraint: no two entries in the same alternative group may share
+    offset=0.  If a future signature needs two offset-0 checks as part of
+    ONE alternative (i.e. two different bytes that must both appear at the
+    start of the same file), this function would incorrectly split them
+    into separate alternatives.  In that case, use a combined bytes object
+    covering the full header range instead of two separate entries, or
+    refactor _MAGIC to use a dedicated tuple type that carries an
+    'alternative_id' discriminator.
     """
     groups: list[list[tuple[int, bytes]]] = []
     for offset, magic in signatures:

gunicorn.conf.py

@@ -45,7 +45,17 @@ pidfile           = "/tmp/it_tickets.pid"
 
 
 def post_fork(server, worker):
-    """Called after a worker is forked. Patch eventlet per-worker."""
+    """Called after a worker is forked.
+
+    Defensive monkey-patch — eventlet is already patched by the time this
+    runs (run.py calls eventlet.monkey_patch() at module level, which fires
+    before gunicorn loads the app with preload_app=False), so this call is
+    technically redundant.  It is retained as a belt-and-suspenders guard:
+    if the startup order ever changes (e.g. a future gunicorn version alters
+    worker bootstrap sequencing), the patch here ensures green-thread safety
+    is still applied before any request is served.  Double-patching is safe
+    in eventlet — it is idempotent and produces no side effects.
+    """
     import eventlet
     eventlet.monkey_patch()