From 169820240ac13aadb30ab84e6e64ce96c0ed4253 Mon Sep 17 00:00:00 2001 From: NguyenND Date: Thu, 26 Mar 2026 13:18:09 -0400 Subject: [PATCH] Fix some issues --- .env | 9 ++- app/__init__.py | 26 +++++++- app/models.py | 4 +- app/routes/admin.py | 65 ++++++++++++++++--- app/routes/auth.py | 29 +++++++-- app/routes/chatbot.py | 29 +++++++-- app/routes/tickets.py | 6 +- app/services/log_service.py | 21 +++++- app/templates/admin/create_user.html | 1 + app/templates/admin/edit_user.html | 1 + app/templates/admin/kb_edit.html | 1 + app/templates/admin/kb_list.html | 2 + app/templates/admin/users.html | 2 +- app/templates/auth/login.html | 1 + app/templates/auth/profile.html | 1 + app/templates/auth/register.html | 1 + app/templates/base.html | 31 +++++++-- app/templates/tickets/create.html | 1 + app/templates/tickets/detail.html | 3 + app/templates/tickets/notifications.html | 1 + config/config.py | 11 ++++ gunicorn.conf.py | 17 ++++- migrations/env.py | 31 +++++++++ .../001_widen_ticket_history_values.py | 61 +++++++++++++++++ requirements.txt | 2 + 25 files changed, 319 insertions(+), 38 deletions(-) create mode 100644 migrations/env.py create mode 100644 migrations/versions/001_widen_ticket_history_values.py diff --git a/.env b/.env index 5f4ae1a..be271e6 100644 --- a/.env +++ b/.env @@ -11,9 +11,12 @@ DB_USER=it_ticket DB_PASSWORD=IT.t1ck3t.5ys # Mail Configuration (SMTP) +# Port 465 uses implicit SSL — MAIL_USE_SSL must be True, MAIL_USE_TLS must be False. +# If your SMTP server uses port 587, swap these: MAIL_USE_TLS=True, MAIL_USE_SSL=False. MAIL_SERVER=mail.ltservicesinc.com MAIL_PORT=465 -MAIL_USE_TLS=True +MAIL_USE_TLS=False +MAIL_USE_SSL=True MAIL_USERNAME=jqc.noreply@ltservicesinc.com MAIL_PASSWORD=jQc.4utoMail$ MAIL_DEFAULT_SENDER=IT Helpdesk @@ -28,7 +31,9 @@ APP_BASE_URL=https://tickets.ltservicesinc.com ANTHROPIC_API_KEY=your-anthropic-api-key-here # File Upload Configuration -UPLOAD_FOLDER=app/static/uploads +# Must be an absolute path so all gunicorn workers resolve the same directory +# regardless of their working directory. Adjust to match your deployment path. +UPLOAD_FOLDER=/home/it-ticket/myapp/app/static/uploads MAX_CONTENT_LENGTH=16777216 # Admin default credentials (change after first login) diff --git a/app/__init__.py b/app/__init__.py index a6990ab..7835dad 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -8,6 +8,9 @@ from flask_login import LoginManager from flask_mail import Mail from flask_migrate import Migrate from flask_socketio import SocketIO +from flask_wtf.csrf import CSRFProtect +from flask_limiter import Limiter +from flask_limiter.util import get_remote_address from config.config import config db = SQLAlchemy() @@ -15,6 +18,13 @@ login_manager= LoginManager() mail = Mail() migrate = Migrate() socketio = SocketIO() +csrf = CSRFProtect() +# limiter is a module-level name so blueprints can do `from app import limiter`, +# but the actual Limiter object is constructed inside create_app() — AFTER +# gunicorn has called eventlet.monkey_patch() in run.py. Constructing it here +# (at import time) creates a threading.RLock before the patch runs, which +# triggers: "1 RLock(s) were not greened". +limiter: Limiter = None # type: ignore[assignment] def create_app(config_name=None): @@ -32,10 +42,24 @@ def create_app(config_name=None): login_manager.init_app(app) mail.init_app(app) migrate.init_app(app, db) + csrf.init_app(app) + + # Limiter is constructed here — NOT at module level — so that + # eventlet.monkey_patch() (called in run.py before any imports) has already + # replaced threading.RLock with a green-thread-safe version. Constructing + # Limiter at module import time creates a real OS RLock before the patch + # runs, which produces: "1 RLock(s) were not greened". + global limiter + limiter = Limiter( + key_func = get_remote_address, + default_limits = [], + storage_uri = app.config.get('RATELIMIT_STORAGE_URI'), + ) + limiter.init_app(app) socketio.init_app( app, async_mode = 'eventlet', - cors_allowed_origins = '*', + cors_allowed_origins = app.config.get('APP_BASE_URL', ''), # Ping settings: server sends a ping every 25s, client has 60s to respond. # This ensures dead connections are detected and closed cleanly rather than # being torn down by nginx timeouts, which causes [Errno 9] Bad file descriptor. diff --git a/app/models.py b/app/models.py index 96e3388..bd5a7ba 100644 --- a/app/models.py +++ b/app/models.py @@ -218,8 +218,8 @@ class TicketHistory(db.Model): ticket_id = db.Column(db.Integer, db.ForeignKey('tickets.id'), nullable=False) changed_by = db.Column(db.Integer, db.ForeignKey('users.id'), nullable=False) field_name = db.Column(db.String(50), nullable=False) - old_value = db.Column(db.String(200)) - new_value = db.Column(db.String(200)) + old_value = db.Column(db.Text) + new_value = db.Column(db.Text) changed_at = db.Column(db.DateTime, default=datetime.utcnow) changer = db.relationship('User', foreign_keys=[changed_by]) diff --git a/app/routes/admin.py b/app/routes/admin.py index d12bc3e..5341d06 100644 --- a/app/routes/admin.py +++ b/app/routes/admin.py @@ -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}') diff --git a/app/routes/auth.py b/app/routes/auth.py index 4028142..48f2d5c 100644 --- a/app/routes/auth.py +++ b/app/routes/auth.py @@ -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') diff --git a/app/routes/chatbot.py b/app/routes/chatbot.py index 7d7fda8..064d51c 100644 --- a/app/routes/chatbot.py +++ b/app/routes/chatbot.py @@ -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) diff --git a/app/routes/tickets.py b/app/routes/tickets.py index e017dc4..0e6fa19 100644 --- a/app/routes/tickets.py +++ b/app/routes/tickets.py @@ -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: diff --git a/app/services/log_service.py b/app/services/log_service.py index fb2bdc0..e1afa0a 100644 --- a/app/services/log_service.py +++ b/app/services/log_service.py @@ -46,6 +46,14 @@ def log_action(user_id, action, entity_type=None, entity_id=None, details=None): entity_type : str | None – 'ticket', 'comment', 'user', etc. entity_id : int | None – primary key of the affected entity details : str | None – free-form JSON or human-readable details + + Transaction note + ---------------- + This function deliberately does NOT call db.session.commit(). The entry + is added to the current session and committed by the caller alongside its + own business objects. This ensures the log entry is only persisted when + the parent operation succeeds — an independent commit here would leave + orphaned log entries for operations that were subsequently rolled back. """ ip = _get_real_ip() @@ -59,7 +67,8 @@ def log_action(user_id, action, entity_type=None, entity_id=None, details=None): ip_address = ip, ) db.session.add(entry) - db.session.commit() + # flush to surface constraint violations early without committing + db.session.flush() logger.info( f'[ACTIVITY] action={action} entity={entity_type}:{entity_id} ' f'user_id={user_id} ip={ip}' @@ -70,7 +79,13 @@ def log_action(user_id, action, entity_type=None, entity_id=None, details=None): def log_ticket_history(ticket, field_name, old_value, new_value, changed_by_id): - """Record a granular field-level change on a ticket.""" + """Record a granular field-level change on a ticket. + + Transaction note + ---------------- + Like log_action, this function does NOT commit — the caller is responsible + for committing the session after all field changes have been recorded. + """ from app.models import TicketHistory try: entry = TicketHistory( @@ -81,7 +96,7 @@ def log_ticket_history(ticket, field_name, old_value, new_value, changed_by_id): new_value = str(new_value) if new_value is not None else None, ) db.session.add(entry) - db.session.commit() + db.session.flush() logger.info( f'[TICKET HISTORY] ticket_id={ticket.id} field={field_name} ' f'"{old_value}" -> "{new_value}" by user_id={changed_by_id}' diff --git a/app/templates/admin/create_user.html b/app/templates/admin/create_user.html index 6010678..4bcfc74 100644 --- a/app/templates/admin/create_user.html +++ b/app/templates/admin/create_user.html @@ -18,6 +18,7 @@
+
diff --git a/app/templates/admin/edit_user.html b/app/templates/admin/edit_user.html index 3d3e60e..63453b0 100644 --- a/app/templates/admin/edit_user.html +++ b/app/templates/admin/edit_user.html @@ -10,6 +10,7 @@
Edit: {{ user.full_name }}
+
diff --git a/app/templates/admin/kb_edit.html b/app/templates/admin/kb_edit.html index e49b09c..4ee0c24 100644 --- a/app/templates/admin/kb_edit.html +++ b/app/templates/admin/kb_edit.html @@ -43,6 +43,7 @@ +
+ {% if art.is_published %}
diff --git a/app/templates/admin/users.html b/app/templates/admin/users.html index 575a702..5c47a9b 100644 --- a/app/templates/admin/users.html +++ b/app/templates/admin/users.html @@ -61,7 +61,7 @@ {% if u.id != current_user.id and u.is_active %}
-
diff --git a/app/templates/auth/login.html b/app/templates/auth/login.html index 0c46b41..9391a39 100644 --- a/app/templates/auth/login.html +++ b/app/templates/auth/login.html @@ -53,6 +53,7 @@ {% endwith %}
+
diff --git a/app/templates/auth/profile.html b/app/templates/auth/profile.html index 1f28d01..d4743c0 100644 --- a/app/templates/auth/profile.html +++ b/app/templates/auth/profile.html @@ -9,6 +9,7 @@
Account Settings
+
diff --git a/app/templates/auth/register.html b/app/templates/auth/register.html index 26b1951..1b92bf2 100644 --- a/app/templates/auth/register.html +++ b/app/templates/auth/register.html @@ -45,6 +45,7 @@ {% endwith %} +
diff --git a/app/templates/base.html b/app/templates/base.html index 9179c66..bde97a5 100644 --- a/app/templates/base.html +++ b/app/templates/base.html @@ -3,6 +3,7 @@ + {% block title %}IT Helpdesk{% endblock %} — TechDesk @@ -438,6 +439,27 @@