diff --git a/app/__init__.py b/app/__init__.py index 458e508..a6dcdf0 100644 --- a/app/__init__.py +++ b/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}') \ No newline at end of file + app.logger.info(f'[SEED] Default admin account created: {admin.email} (username={seed_username})') \ No newline at end of file diff --git a/app/models.py b/app/models.py index 5756ec2..bde4a7b 100644 --- a/app/models.py +++ b/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) diff --git a/app/routes/admin.py b/app/routes/admin.py index 912c6e5..77f158a 100644 --- a/app/routes/admin.py +++ b/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/') +@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. 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 diff --git a/app/routes/chatbot.py b/app/routes/chatbot.py index ba998b3..ce677e1 100644 --- a/app/routes/chatbot.py +++ b/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, diff --git a/app/routes/tickets.py b/app/routes/tickets.py index b570d6b..4361feb 100644 --- a/app/routes/tickets.py +++ b/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 'bold' 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 + # 'bold' 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() @@ -389,12 +411,32 @@ def update_ticket(ticket_id): @tickets_bp.route('/comments//delete', methods=['POST']) @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) + comment = db.session.get(Comment, comment_id) or abort(404) 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() diff --git a/app/services/notification_service.py b/app/services/notification_service.py index 8895e81..7dafc64 100644 --- a/app/services/notification_service.py +++ b/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 ,

+ 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, diff --git a/app/services/validation_service.py b/app/services/validation_service.py index 0f796a3..cbaaaa0 100644 --- a/app/services/validation_service.py +++ b/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: @@ -225,4 +247,4 @@ def render_comment_body(raw_text: str) -> str: attributes = _COMMENT_ALLOWED_ATTRS, strip = True, ) - return cleaned + return cleaned \ No newline at end of file diff --git a/gunicorn.conf.py b/gunicorn.conf.py index 470c899..16ced3d 100644 --- a/gunicorn.conf.py +++ b/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()