From 02b3e1c45d0be9dbae60c6c2921e8bf28a384afc Mon Sep 17 00:00:00 2001 From: NguyenND Date: Wed, 6 May 2026 11:47:20 -0400 Subject: [PATCH] 05/06 Fix some notable and quality issues --- app/models/api_token.py | 5 ++ app/models/audit.py | 4 +- app/routes/auth.py | 25 ++------ app/routes/customers.py | 16 +---- app/routes/inspections.py | 5 +- app/routes/scheduled_reports.py | 11 +++- app/utils/decorators.py | 28 ++++++++- app/utils/forms.py | 4 +- app/utils/sla.py | 5 +- .../versions/phase15_audit_log_indexes.py | 62 +++++++++++++++++++ requirements.txt | 6 +- 11 files changed, 125 insertions(+), 46 deletions(-) create mode 100644 migrations/versions/phase15_audit_log_indexes.py diff --git a/app/models/api_token.py b/app/models/api_token.py index 58ef417..9dff3e9 100644 --- a/app/models/api_token.py +++ b/app/models/api_token.py @@ -76,6 +76,11 @@ class RefreshToken(db.Model): """ Look up a refresh token by its raw value. Returns the RefreshToken row if valid and unexpired, else None. + + Expired rows are not deleted here — passive cleanup runs in the + login route (api/auth.py) each time a user authenticates, removing + all expired/revoked tokens for that user. This keeps the table tidy + without requiring a dedicated cron job. """ import hashlib hashed = hashlib.sha256(raw_token.encode()).hexdigest() diff --git a/app/models/audit.py b/app/models/audit.py index 2c3570e..0756dde 100644 --- a/app/models/audit.py +++ b/app/models/audit.py @@ -16,8 +16,8 @@ class AuditLog(db.Model): username = db.Column(db.String(100), nullable=False) # snapshot at time of action user_role = db.Column(db.String(20), nullable=False) # snapshot at time of action # What happened - action = db.Column(db.String(50), nullable=False) # CREATE / UPDATE / DELETE / LOGIN / LOGOUT / EXPORT - entity_type = db.Column(db.String(50), nullable=False) # User / Facility / Area / Template / Inspection / Issue / … + action = db.Column(db.String(50), nullable=False, index=True) # CREATE / UPDATE / DELETE / LOGIN / LOGOUT / EXPORT + entity_type = db.Column(db.String(50), nullable=False, index=True) # User / Facility / Area / Template / Inspection / Issue / … entity_id = db.Column(db.Integer, nullable=True) # PK of the affected record (NULL for bulk ops) entity_label = db.Column(db.String(255), nullable=True) # Human-readable identifier snapshot # Extra context stored as free-text (key=value pairs, comma-separated) diff --git a/app/routes/auth.py b/app/routes/auth.py index 92012d3..c827850 100644 --- a/app/routes/auth.py +++ b/app/routes/auth.py @@ -1,10 +1,9 @@ from flask import Blueprint, render_template, redirect, url_for, flash, request, abort from flask_login import login_user, logout_user, login_required, current_user -from urllib.parse import urlparse from app import db, limiter from app.models.user import User from app.utils.forms import LoginForm, UserForm, ProfileForm -from app.utils.decorators import admin_required, supervisor_required +from app.utils.decorators import admin_required, supervisor_required, safe_redirect_url import logging from app.utils.audit import log_action, ACTION_CREATE, ACTION_UPDATE, ACTION_DELETE, ACTION_LOGIN, ACTION_LOGOUT @@ -13,22 +12,6 @@ logger = logging.getLogger(__name__) bp = Blueprint('auth', __name__, url_prefix='/auth') -def _safe_next(next_url: str | None) -> str: - """ - Validate that the redirect target is a relative URL on this host. - Returns the safe URL, or the dashboard index if the URL is external/invalid. - This prevents open-redirect attacks where an attacker crafts a login link - containing next=https://evil.com to hijack post-login redirects. - """ - if not next_url: - return url_for('dashboard.index') - parsed = urlparse(next_url) - # Reject any URL that specifies a network location (external host) or scheme - if parsed.netloc or parsed.scheme: - return url_for('dashboard.index') - return next_url - - @bp.route('/login', methods=['GET', 'POST']) @limiter.limit('20 per minute; 5 per second') def login(): @@ -52,7 +35,7 @@ def login(): return render_template('auth/login.html', form=form) login_user(user, remember=form.remember_me.data) # Use validated next URL — never redirect blindly to request.args['next'] - next_page = _safe_next(request.args.get('next')) + next_page = safe_redirect_url(request.args.get('next')) log_action(ACTION_LOGIN, 'User', user.id, user.username) flash(f'Welcome back, {user.username}!', 'success') return redirect(next_page) @@ -282,7 +265,7 @@ def toggle_active(user_id): f'account {action_label} by {current_user.username}', ) flash(f'User {user.username} has been {action_label}.', 'success') - return redirect(_safe_next(request.referrer) or url_for('auth.list_users')) + return redirect(safe_redirect_url(request.referrer, fallback=url_for('auth.list_users'))) # ── Notification Matrix ─────────────────────────────────────────────────────── @@ -334,4 +317,4 @@ def notification_matrix(): matrix_roles = MATRIX_ROLES, defaults = MATRIX_DEFAULTS, state = state, - ) \ No newline at end of file + ) diff --git a/app/routes/customers.py b/app/routes/customers.py index 07de082..a868424 100644 --- a/app/routes/customers.py +++ b/app/routes/customers.py @@ -13,7 +13,6 @@ Provides a single screen to: """ import logging -from urllib.parse import urlparse from flask import Blueprint, render_template, redirect, url_for, flash, request, abort from flask_login import login_required, current_user from app import db @@ -21,21 +20,10 @@ from app.models.user import User from app.models.project import Project, CustomerAssignment from app.models.facility import Facility from app.utils.forms import CustomerUserForm, CustomerAssignmentForm, CustomerInviteForm, SetPasswordForm -from app.utils.decorators import admin_required, supervisor_required +from app.utils.decorators import admin_required, supervisor_required, safe_redirect_url from app.utils.audit import log_action, ACTION_CREATE, ACTION_UPDATE, ACTION_DELETE from app.utils.scope import get_customer_scope - -def _safe_referrer(fallback: str) -> str: - """Return request.referrer only if it is a safe relative URL, else fallback.""" - ref = request.referrer - if not ref: - return fallback - parsed = urlparse(ref) - if parsed.netloc or parsed.scheme: - return fallback - return ref - logger = logging.getLogger(__name__) bp = Blueprint('customers', __name__, url_prefix='/customers') @@ -496,7 +484,7 @@ def toggle_active(customer_id): log_action(ACTION_UPDATE, 'User', customer.id, customer.username, f'account {label} via customer_mgmt by {current_user.username}') flash(f'Customer "{customer.username}" has been {label}.', 'success') - return redirect(_safe_referrer(url_for('customers.index'))) + return redirect(safe_redirect_url(request.referrer, fallback=url_for('customers.index'))) # ── AJAX: facilities for a project (used by add-assignment form) ────────────── diff --git a/app/routes/inspections.py b/app/routes/inspections.py index fdb91e1..3626d77 100644 --- a/app/routes/inspections.py +++ b/app/routes/inspections.py @@ -370,7 +370,8 @@ def execute(inspection_id): inspection.completed_at = now_eastern() _save_responses(inspection, responses) - db.session.commit() + # NOTE: do NOT commit here — inspection fields and all notification + # rows are staged together and committed atomically below. inspection_link = url_for('inspections.view', inspection_id=inspection.id) score_display = f'{score:.1f}%' if score is not None else 'N/A' @@ -388,7 +389,7 @@ def execute(inspection_id): facility_id = inspection.facility_id, exclude_user_ids = {current_user.id}, ) - db.session.commit() + db.session.commit() # Single atomic commit: inspection fields + notification rows log_action(ACTION_UPDATE, 'Inspection', inspection.id, f'{inspection.template.name} @ {inspection.facility.name}', f'status=completed; score={score}') diff --git a/app/routes/scheduled_reports.py b/app/routes/scheduled_reports.py index 2304af5..63649e3 100644 --- a/app/routes/scheduled_reports.py +++ b/app/routes/scheduled_reports.py @@ -50,13 +50,20 @@ bp = Blueprint('scheduled_reports', __name__, url_prefix='/scheduled-reports') # ── Helpers ─────────────────────────────────────────────────────────────────── def _compute_next_send(frequency: str, from_dt: datetime = None) -> datetime: - """Return the next send datetime for a given frequency.""" + """Return the next send datetime for a given frequency. + + Monthly cadence always targets the 1st of the next month at 07:00. + The December branch is required because datetime.replace(month=13) + raises ValueError — incrementing the month directly is not safe in + general, but targeting day=1 avoids the separate last-day-of-month + (28/29/30/31) edge case that would affect mid-month scheduling. + """ now = from_dt or now_eastern() if frequency == 'daily': return (now + timedelta(days=1)).replace(hour=7, minute=0, second=0, microsecond=0) if frequency == 'weekly': return (now + timedelta(weeks=1)).replace(hour=7, minute=0, second=0, microsecond=0) - # monthly: first of next month + # monthly: first of next month — December rolls over to Jan of next year. if now.month == 12: return now.replace(year=now.year + 1, month=1, day=1, hour=7, minute=0, second=0, microsecond=0) return now.replace(month=now.month + 1, day=1, hour=7, minute=0, second=0, microsecond=0) diff --git a/app/utils/decorators.py b/app/utils/decorators.py index 19ba838..da52222 100644 --- a/app/utils/decorators.py +++ b/app/utils/decorators.py @@ -1,6 +1,32 @@ from functools import wraps -from flask import flash, redirect, url_for +from flask import flash, redirect, url_for, request from flask_login import current_user +from urllib.parse import urlparse + + +# ── Open-redirect guard ─────────────────────────────────────────────────────── + +def safe_redirect_url(url: str | None, fallback: str | None = None) -> str: + """Return *url* only if it is a safe relative URL on this host. + + Rejects any URL that carries a network location (netloc) or an explicit + scheme, preventing open-redirect attacks where a crafted link contains + next=https://evil.com. + + Parameters + ---------- + url : The candidate redirect target (may be None). + fallback : Returned when *url* is absent or unsafe. + Defaults to the dashboard index. + """ + if fallback is None: + fallback = url_for('dashboard.index') + if not url: + return fallback + parsed = urlparse(url) + if parsed.netloc or parsed.scheme: + return fallback + return url def admin_required(f): @wraps(f) diff --git a/app/utils/forms.py b/app/utils/forms.py index 8b804bc..73bd1f5 100644 --- a/app/utils/forms.py +++ b/app/utils/forms.py @@ -1,5 +1,5 @@ from flask_wtf import FlaskForm -from flask_wtf.file import FileField, FileAllowed +from flask_wtf.file import FileField, FileAllowed, MultipleFileField from wtforms import (StringField, PasswordField, SelectField, TextAreaField, DecimalField, BooleanField, IntegerField, HiddenField, RadioField) @@ -174,7 +174,7 @@ class IssueUpdateForm(FlaskForm): assigned_to = SelectField('Assign To', coerce=int, validators=[Optional()]) update_notes = TextAreaField('Update Notes', validators=[Optional(), Length(max=1000)]) result_notes = TextAreaField('Result Notes', validators=[Optional(), Length(max=2000)]) - result_photos = FileField('Result Photos', validators=[ + result_photos = MultipleFileField('Result Photos', validators=[ Optional(), FileAllowed(['jpg','jpeg','png','gif'], 'Images only.') ]) diff --git a/app/utils/sla.py b/app/utils/sla.py index 266331e..80e9d6a 100644 --- a/app/utils/sla.py +++ b/app/utils/sla.py @@ -110,9 +110,12 @@ def send_sla_alerts(): logger = logging.getLogger(__name__) + # yield_per streams rows in batches of 100 rather than loading all open + # issues into memory at once. At current scale this is a no-op difference, + # but it prevents a memory spike if the issue count grows large. open_issues = Issue.query.filter( Issue.status.in_(['open', 'in_progress', 'pending_verification']) - ).all() + ).yield_per(100) total_sent = 0 diff --git a/migrations/versions/phase15_audit_log_indexes.py b/migrations/versions/phase15_audit_log_indexes.py new file mode 100644 index 0000000..4365763 --- /dev/null +++ b/migrations/versions/phase15_audit_log_indexes.py @@ -0,0 +1,62 @@ +"""phase15 — add indexes on audit_logs action and entity_type + +The audit log filter UI allows filtering by action and entity_type. +Without indexes, every filter invocation performs a full table scan. +As the log grows toward the 180/365-day purge threshold this degrades +noticeably. This migration adds individual indexes on both columns. + +A composite index on (action, entity_type, created_at) would be ideal +for the combined-filter case, but individual indexes are added here to +keep the migration additive and safe for re-run. created_at already +has an index from the model definition. + +Existence checks use information_schema so the migration is safe to +re-run on any MySQL version (compatible back to 5.7). + +Revision ID: phase15_audit_log_indexes +Revises: phase14_facility_created_at +""" + +revision = 'phase15_audit_log_indexes' +down_revision = 'phase14_facility_created_at' +branch_labels = None +depends_on = None + +from alembic import op +from sqlalchemy import text + + +def _index_exists(conn, table: str, index_name: str) -> bool: + """Return True if the named index already exists on the given table.""" + result = conn.execute(text( + "SELECT COUNT(*) FROM information_schema.statistics " + "WHERE table_schema = DATABASE() " + " AND table_name = :table " + " AND index_name = :index" + ), {'table': table, 'index': index_name}) + return result.scalar() > 0 + + +# (table, index_name, column) +INDEXES = [ + ('audit_logs', 'ix_audit_logs_action', 'action'), + ('audit_logs', 'ix_audit_logs_entity_type', 'entity_type'), +] + + +def upgrade(): + conn = op.get_bind() + for table, index_name, column in INDEXES: + if not _index_exists(conn, table, index_name): + op.execute(text( + f'CREATE INDEX {index_name} ON {table} ({column})' + )) + + +def downgrade(): + conn = op.get_bind() + for table, index_name, _column in INDEXES: + if _index_exists(conn, table, index_name): + op.execute(text( + f'DROP INDEX {index_name} ON {table}' + )) diff --git a/requirements.txt b/requirements.txt index 7f3ebc7..d88ecf4 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,3 +1,7 @@ +# Python >= 3.12 required. +# Several route files use nested f-strings with inner single-quoted expressions +# (e.g. f'...{x if x else '—'}...') — valid syntax in Python 3.12+ only. +# Do not downgrade the interpreter without replacing those expressions first. Flask Flask-SQLAlchemy Flask-Login @@ -15,4 +19,4 @@ email-validator gunicorn reportlab pytz -pyJWT \ No newline at end of file +pyJWT