Fix some issues

2026-03-26 13:18:09 -04:00
parent ae6a44199f
commit 169820240a
25 changed files with 319 additions and 38 deletions
@@ -5,6 +5,7 @@ from functools import wraps
 from flask import Blueprint, render_template, redirect, url_for, flash, request, abort, jsonify, current_app, send_from_directory
 from flask_login import login_required, current_user
 from werkzeug.utils import secure_filename
+import bleach
 from app import db
 from app.models import (User, Ticket, Comment, ActivityLog, KnowledgeBase,
                        KBAttachment, UserRole, TicketStatus)
@@ -13,6 +14,39 @@ from app.services.log_service import log_action
 admin_bp = Blueprint('admin', __name__, url_prefix='/admin')
 logger   = logging.getLogger(__name__)

+# ── KB body HTML sanitisation ─────────────────────────────────────────────────
+# TinyMCE produces rich HTML which must be sanitised server-side before
+# persistence to prevent stored XSS attacks. Only tags and attributes that
+# are safe to render are whitelisted; everything else is stripped.
+_KB_ALLOWED_TAGS = set(bleach.sanitizer.ALLOWED_TAGS) | {
+    'p', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6',
+    'pre', 'code', 'blockquote', 'hr', 'br',
+    'table', 'thead', 'tbody', 'tr', 'th', 'td',
+    'ul', 'ol', 'li', 'dl', 'dt', 'dd',
+    'img', 'figure', 'figcaption',
+    'div', 'span', 'section',
+    'strong', 'em', 'u', 's', 'sub', 'sup',
+}
+_KB_ALLOWED_ATTRS = {
+    '*'  : ['class', 'id', 'style'],
+    'a'  : ['href', 'title', 'target', 'rel'],
+    'img': ['src', 'alt', 'width', 'height', 'title'],
+    'td' : ['colspan', 'rowspan'],
+    'th' : ['colspan', 'rowspan'],
+    'col': ['span'],
+}
+
+def _sanitize_kb_body(raw_html):
+    """Strip disallowed tags/attributes from a TinyMCE-produced HTML body."""
+    cleaned = bleach.clean(
+        raw_html or '',
+        tags       = _KB_ALLOWED_TAGS,
+        attributes = _KB_ALLOWED_ATTRS,
+        strip      = True,
+    )
+    logger.debug(f'[KB SANITIZE] input_len={len(raw_html or "")} output_len={len(cleaned)}')
+    return cleaned
+

 def admin_required(f):
    @wraps(f)
@@ -112,10 +146,10 @@ def create_user():
        )
        user.set_password(password)
        db.session.add(user)
-        db.session.commit()
-
        log_action(current_user.id, 'admin_user_create', 'user', user.id,
                   f'email={email} role={role}')
+        db.session.commit()
+
        logger.info(f'[ADMIN USER CREATE] user_id={user.id} email={email} role={role} by admin_id={current_user.id}')
        flash(f'User {full_name} ({email}) created successfully.', 'success')
        return redirect(url_for('admin.users'))
@@ -139,9 +173,9 @@ def edit_user(user_id):
        if new_pw:
            user.set_password(new_pw)
            logger.info(f'[ADMIN PASSWORD RESET] target_user_id={user.id} by admin_id={current_user.id}')
-        db.session.commit()
        log_action(current_user.id, 'admin_user_edit', 'user', user.id,
                   f'role_change={old_role}->{user.role} active={user.is_active}')
+        db.session.commit()
        logger.info(f'[ADMIN USER EDIT] user_id={user.id} by admin_id={current_user.id}')
        flash('User updated.', 'success')
        return redirect(url_for('admin.users'))
@@ -157,8 +191,8 @@ def delete_user(user_id):
        flash('You cannot delete your own account.', 'danger')
        return redirect(url_for('admin.users'))
    user.is_active = False
-    db.session.commit()
    log_action(current_user.id, 'admin_user_deactivate', 'user', user.id)
+    db.session.commit()
    logger.info(f'[ADMIN USER DEACTIVATE] user_id={user.id} by admin_id={current_user.id}')
    flash('User deactivated.', 'success')
    return redirect(url_for('admin.users'))
@@ -316,7 +350,7 @@ def kb_new():
        try:
            article = KnowledgeBase(
                title       = request.form.get('title', '').strip(),
-                body        = request.form.get('body', '').strip(),
+                body        = _sanitize_kb_body(request.form.get('body', '')),
                category    = request.form.get('category', ''),
                tags        = request.form.get('tags', ''),
                author_id   = current_user.id,
@@ -330,9 +364,9 @@ def kb_new():
                    att = _save_kb_file(f, article.id)
                    db.session.add(att)

-            db.session.commit()
            log_action(current_user.id, 'kb_create', 'knowledge_base', article.id,
                       f'title={article.title}')
+            db.session.commit()
            logger.info(f'[KB CREATE] article_id={article.id} by user_id={current_user.id}')
            flash('Article created successfully.', 'success')
            return jsonify({'ok': True, 'article_id': article.id, 'redirect': url_for('admin.kb_list')})
@@ -353,7 +387,7 @@ def kb_edit(article_id):
    if request.method == 'POST':
        try:
            article.title       = request.form.get('title', article.title).strip()
-            article.body        = request.form.get('body', article.body).strip()
+            article.body        = _sanitize_kb_body(request.form.get('body', article.body))
            article.category    = request.form.get('category', article.category)
            article.tags        = request.form.get('tags', article.tags)
            article.is_published= bool(request.form.get('is_published')) and not bool(request.form.get('_save_as_draft'))
@@ -363,8 +397,8 @@ def kb_edit(article_id):
                    att = _save_kb_file(f, article.id)
                    db.session.add(att)

-            db.session.commit()
            log_action(current_user.id, 'kb_edit', 'knowledge_base', article.id)
+            db.session.commit()
            logger.info(f'[KB EDIT] article_id={article.id} by user_id={current_user.id}')
            flash('Article updated successfully.', 'success')
            return jsonify({'ok': True, 'article_id': article.id, 'redirect': url_for('admin.kb_list')})
@@ -382,10 +416,10 @@ def kb_toggle_publish(article_id):
    """Quick publish/unpublish toggle — callable from the article list."""
    article = KnowledgeBase.query.get_or_404(article_id)
    article.is_published = not article.is_published
-    db.session.commit()
    state = 'published' if article.is_published else 'unpublished'
    log_action(current_user.id, f'kb_{state}', 'knowledge_base', article.id,
               f'title={article.title}')
+    db.session.commit()
    logger.info(f'[KB TOGGLE PUBLISH] article_id={article.id} is_published={article.is_published} by user_id={current_user.id}')
    flash(f'Article "{article.title}" has been {state}.', 'success')
    return redirect(url_for('admin.kb_list'))
@@ -395,7 +429,18 @@ def kb_toggle_publish(article_id):
@login_required
@it_required
 def kb_delete(article_id):
-    article = KnowledgeBase.query.get_or_404(article_id)
+    article    = KnowledgeBase.query.get_or_404(article_id)
+    upload_dir = current_app.config['UPLOAD_FOLDER']
+
+    # Remove physical files before the cascade deletes the KBAttachment rows.
+    # Without this step the DB records disappear but the files remain on disk
+    # with no pointer to them — unrecoverable orphans.
+    for att in article.attachments.all():
+        filepath = os.path.join(upload_dir, att.stored_name)
+        if os.path.exists(filepath):
+            os.remove(filepath)
+            logger.info(f'[KB DELETE FILE] stored_name={att.stored_name} article_id={article_id} by user_id={current_user.id}')
+
    log_action(current_user.id, 'kb_delete', 'knowledge_base', article.id,
               f'title={article.title}')
    logger.info(f'[KB DELETE] article_id={article.id} by user_id={current_user.id}')
@@ -1,8 +1,9 @@
 import logging
 from datetime import datetime
+from urllib.parse import urlparse, urljoin
 from flask import Blueprint, render_template, redirect, url_for, flash, request
 from flask_login import login_user, logout_user, login_required, current_user
-from app import db
+from app import db, limiter
 from app.models import User, UserRole
 from app.services.log_service import log_action

@@ -10,7 +11,23 @@ auth_bp = Blueprint('auth', __name__, url_prefix='/auth')
 logger  = logging.getLogger(__name__)


+def _is_safe_url(target):
+    """Return True only when *target* points back to this same host.
+
+    Prevents open-redirect attacks where an attacker crafts a login URL
+    with ?next=https://evil.com — without this check the user would be
+    silently forwarded to an external site after authentication.
+    """
+    ref_url  = urlparse(request.host_url)
+    test_url = urlparse(urljoin(request.host_url, target))
+    return (
+        test_url.scheme in ('http', 'https') and
+        ref_url.netloc == test_url.netloc
+    )
+
+
@auth_bp.route('/login', methods=['GET', 'POST'])
+@limiter.limit('10 per minute; 50 per hour')
 def login():
    if current_user.is_authenticated:
        return redirect(url_for('tickets.dashboard'))
@@ -24,10 +41,13 @@ def login():
        if user and user.check_password(password) and user.is_active:
            login_user(user, remember=remember)
            user.last_login = datetime.utcnow()
-            db.session.commit()
            log_action(user.id, 'user_login', 'user', user.id, f'email={email}')
+            db.session.commit()
            logger.info(f'[AUTH LOGIN] user_id={user.id} email={email}')
            next_page = request.args.get('next')
+            if next_page and not _is_safe_url(next_page):
+                logger.warning(f'[AUTH OPEN-REDIRECT BLOCKED] next={next_page} user_id={user.id}')
+                next_page = None
            return redirect(next_page or url_for('tickets.dashboard'))
        else:
            logger.warning(f'[AUTH FAILED] email={email} ip={request.remote_addr}')
@@ -37,6 +57,7 @@ def login():


@auth_bp.route('/register', methods=['GET', 'POST'])
+@limiter.limit('5 per minute; 20 per hour')
 def register():
    if current_user.is_authenticated:
        return redirect(url_for('tickets.dashboard'))
@@ -69,8 +90,8 @@ def register():
            )
            user.set_password(password)
            db.session.add(user)
-            db.session.commit()
            log_action(user.id, 'user_register', 'user', user.id, f'email={email}')
+            db.session.commit()
            logger.info(f'[AUTH REGISTER] user_id={user.id} email={email}')
            flash('Account created! You may now log in.', 'success')
            return redirect(url_for('auth.login'))
@@ -116,8 +137,8 @@ def profile():
            current_user.set_password(new_pw)
            logger.info(f'[AUTH PASSWORD CHANGE] user_id={current_user.id}')

-        db.session.commit()
        log_action(current_user.id, 'user_profile_update', 'user', current_user.id)
+        db.session.commit()
        logger.info(f'[AUTH PROFILE UPDATE] user_id={current_user.id}')
        flash('Profile updated successfully.', 'success')

@@ -76,11 +76,33 @@ def chat():
            end   = reply_text.rfind('}') + 1
            parsed = json.loads(reply_text[start:end])
            if parsed.get('action') == 'create_ticket':
+                # Validate AI-provided enum values against allowed sets to
+                # prevent arbitrary strings reaching the database.
+                _valid_categories = {
+                    TicketCategory.HARDWARE, TicketCategory.SOFTWARE,
+                    TicketCategory.NETWORK,  TicketCategory.ACCESS,
+                    TicketCategory.EMAIL,    TicketCategory.PRINTER,
+                    TicketCategory.PHONE,    TicketCategory.SECURITY,
+                    TicketCategory.OTHER,
+                }
+                _valid_priorities = {
+                    TicketPriority.LOW,    TicketPriority.MEDIUM,
+                    TicketPriority.HIGH,   TicketPriority.CRITICAL,
+                }
+                raw_category = parsed.get('category', TicketCategory.OTHER)
+                raw_priority = parsed.get('priority', TicketPriority.MEDIUM)
+                safe_category = raw_category if raw_category in _valid_categories else TicketCategory.OTHER
+                safe_priority = raw_priority if raw_priority in _valid_priorities else TicketPriority.MEDIUM
+                if raw_category != safe_category:
+                    logger.warning(f'[CHATBOT VALIDATION] invalid category="{raw_category}" coerced to "{safe_category}"')
+                if raw_priority != safe_priority:
+                    logger.warning(f'[CHATBOT VALIDATION] invalid priority="{raw_priority}" coerced to "{safe_priority}"')
+
                ticket = Ticket(
                    title         = parsed.get('title', 'Untitled Issue'),
                    description   = parsed.get('description', ''),
-                    category      = parsed.get('category', TicketCategory.OTHER),
-                    priority      = parsed.get('priority', TicketPriority.MEDIUM),
+                    category      = safe_category,
+                    priority      = safe_priority,
                    location      = parsed.get('location', ''),
                    asset_tag     = parsed.get('asset_tag', ''),
                    created_by_id = current_user.id,
@@ -89,10 +111,9 @@ def chat():
                )
                ticket.ticket_number = ticket.generate_ticket_number()
                db.session.add(ticket)
-                db.session.commit()
-
                log_action(current_user.id, 'ticket_create_chatbot', 'ticket', ticket.id,
                           f'ticket_number={ticket.ticket_number} ai_generated=True')
+                db.session.commit()
                logger.info(f'[CHATBOT TICKET CREATE] ticket_id={ticket.id} number={ticket.ticket_number} user_id={current_user.id}')
                notify_new_ticket(ticket)

@@ -117,9 +117,9 @@ def create_ticket():
            if f and f.filename and allowed_file(f.filename):
                save_attachment(f, ticket_id=ticket.id, uploader_id=current_user.id)

-        db.session.commit()
        log_action(current_user.id, 'ticket_create', 'ticket', ticket.id,
                   f'ticket_number={ticket.ticket_number} priority={priority} category={category}')
+        db.session.commit()
        logger.info(f'[TICKET CREATE] ticket_id={ticket.id} number={ticket.ticket_number} by user_id={current_user.id}')
        notify_new_ticket(ticket)
        flash(f'Ticket {ticket.ticket_number} created successfully!', 'success')
@@ -197,9 +197,9 @@ def ticket_detail(ticket_id):
                    save_attachment(f, ticket_id=ticket.id,
                                    comment_id=comment.id, uploader_id=current_user.id)

-            db.session.commit()
            log_action(current_user.id, 'comment_create', 'comment', comment.id,
                       f'ticket_id={ticket.id} internal={is_internal}')
+            db.session.commit()
            logger.info(f'[COMMENT CREATE] comment_id={comment.id} ticket_id={ticket.id} by user_id={current_user.id}')
            notify_comment_added(comment)
            flash('Comment added.', 'success')
@@ -275,9 +275,9 @@ def update_ticket(ticket_id):
        except ValueError:
            pass

-    db.session.commit()
    log_action(current_user.id, 'ticket_update', 'ticket', ticket.id,
               f'changes=[{"; ".join(changes)}]')
+    db.session.commit()
    logger.info(f'[TICKET UPDATE] ticket_id={ticket.id} changes={changes} by user_id={current_user.id}')

    if new_status != old_status: