From 07e0ae02c2141a8de349e4e95d2b54ac4cbdd9a3 Mon Sep 17 00:00:00 2001
From: Nguyen HP Laptop <ngodanguyen@gmail.com>
Date: Sun, 24 May 2026 17:49:44 -0400
Subject: [PATCH] 05/24 Fix Security bugs

---
 CLAUDE.md             | 113 +++++++++++++++++++++++++++++++++++++-----
 app.py                |  28 ++++++++++-
 models.py             |  20 +++++++-
 routes/admin_users.py |   2 +
 routes/ai_summary.py  |  32 ++++++++++++
 5 files changed, 179 insertions(+), 16 deletions(-)

diff --git a/CLAUDE.md b/CLAUDE.md
index bfee15a..501ca39 100644
--- a/CLAUDE.md
+++ b/CLAUDE.md
@@ -20,11 +20,11 @@ Browser → Nginx (reverse proxy) → Gunicorn (4 workers) → Flask app
                                                     MySQL (shared with desktop)
 ```
 
-- **Entry point:** `wsgi.py` → `app.py::create_app()`
+- **Entry point:** `wsgi.py` → `app.py::create_app()` — **only `wsgi.py` calls `create_app()`**; the bottom of `app.py` no longer has a module-level call (that caused double initialisation)
 - **No ORM** — all DB access is raw SQL via `mysql-connector-python` in `models.py`
 - **No frontend framework** — vanilla JS, no build step, no npm
 - **Blueprints:** one file per feature area in `routes/`
-- **Templates:** Jinja2, all extend `base.html`
+- **Templates:** Jinja2; most extend `base.html` — **`login.html` is a standalone exception** (see CSRF section)
 - **Static assets:** single `style.css` + `app.js` — no preprocessor
 
 ---
@@ -33,7 +33,7 @@ Browser → Nginx (reverse proxy) → Gunicorn (4 workers) → Flask app
 
 ### 1. Shared Database
 Never rename columns, drop tables, or change column types without verifying the desktop app still works. Key schema facts:
-- `activity_log` timestamp column is **`logged_at`** (not `created_at`)
+- `activity_log` timestamp column is **`logged_at`** (DDL and queries both use `logged_at`; a migration in `config.py` renames `created_at` → `logged_at` for old web-only installs)
 - `app_log` timestamp column is **`logged_at`**
 - All other tables generally use `created_at`
 - The desktop app writes `bid_tracker`, `bid_updates`, `ai_analysis_log`, `ai_criteria`, `app_settings`, `users`, `websites`, `website_credentials`, `shifts`, `shift_users`, `shift_websites`, `shift_checks`, `login_attempts`
@@ -66,19 +66,45 @@ Jinja macros defined in the same file as `{% extends "base.html" %}` cannot be c
 ### 5. CSS Specificity — Local `<style>` vs `style.css`
 The global `static/css/style.css` is loaded in `base.html` and has equal or higher specificity than local `<style>` blocks in child templates. Always fix layout issues in `style.css` — do not rely on local `<style>` overrides.
 
+### 6. CSRF Protection (Flask-WTF)
+`CSRFProtect(app)` is initialised inside `create_app()` in `app.py`. All non-GET requests are validated automatically.
+
+**How tokens reach the server:**
+
+| Request type | Mechanism |
+|---|---|
+| Standard HTML form (in a template that extends `base.html`) | `app.js` IIFE injects a hidden `csrf_token` input into every POST form at page-load time, reading from `<meta name="csrf-token">` in `base.html` |
+| Standalone template (e.g. `login.html`) | Must include `<input type="hidden" name="csrf_token" value="{{ csrf_token() }}">` **directly in the form** — these pages do not load `app.js` or `base.html` |
+| `fetch()` / AJAX POST with `FormData` | Append `fd.append('csrf_token', getCsrfToken())` OR add header `'X-CSRFToken': getCsrfToken()` |
+| `fetch()` / AJAX POST with JSON body | Add header `'X-CSRFToken': getCsrfToken()` (token must be a header; JSON body is not inspected) |
+| JS-built form injected into DOM via `innerHTML` | Inline the token: `'<input type="hidden" name="csrf_token" value="' + getCsrfToken() + '">'` |
+
+**`getCsrfToken()`** is defined in `app.js` and reads `document.querySelector('meta[name="csrf-token"]').content`.
+
+**`SECRET_KEY` must be set in `.env`** — if it is absent, each Gunicorn worker generates its own random key, session cookies signed by one worker are rejected by another, and CSRF validation fails for any cross-worker request.
+
+### 7. `SECRET_KEY` is Required
+```
+# .env
+SECRET_KEY=<long-random-hex>   # generate: python -c "import secrets; print(secrets.token_hex(32))"
+```
+Without this, sessions and CSRF tokens are broken across the 4 Gunicorn workers. `app.py` logs a WARNING at startup if it is missing.
+
 ---
 
 ## Key Files Reference
 
 | File | Purpose | Notes |
 |------|---------|-------|
-| `app.py` | Flask factory | Registers all 11 blueprints; defines `hhmm` template filter for MySQL TIME columns |
-| `config.py` | DB config, DDL, settings | Calls `load_dotenv()` at top of file — **must be before `DB_CONFIG` dict** |
-| `models.py` | All DB queries | ~1,430 lines; no ORM; every function opens/closes its own connection |
+| `app.py` | Flask factory | Registers all 11 blueprints; `CSRFProtect(app)`; `hhmm` template filter; logs WARNING if `SECRET_KEY` not set |
+| `wsgi.py` | Gunicorn entry point | Only place that calls `create_app()` — do not add a second call elsewhere |
+| `config.py` | DB config, DDL, settings | Calls `load_dotenv()` at top — **must be before `DB_CONFIG` dict**; contains safe re-runnable migrations |
+| `models.py` | All DB queries | ~1,430 lines; no ORM; every function opens/closes its own connection; `import datetime` at top |
 | `utils/crypto.py` | Fernet encryption | Must match desktop exactly |
 | `utils/decorators.py` | `@login_required`, `@admin_required` | Simple session checks |
 | `static/css/style.css` | Full design system | Light theme, DM Sans + DM Mono, CSS variables in `:root` |
-| `static/js/app.js` | Global JS utilities | `openModal()`, `closeModal()`, `copyToClipboard()`, session timeout warning |
+| `static/js/app.js` | Global JS utilities | `openModal()`, `closeModal()`, `copyToClipboard()`, session timeout warning, `getCsrfToken()`, CSRF auto-inject IIFE |
+| `templates/login.html` | Standalone login page | Does NOT extend `base.html`; has its own `<head>` and no `app.js`; CSRF token must be a direct hidden input |
 
 ---
 
@@ -90,6 +116,8 @@ The global `static/css/style.css` is loaded in `base.html` and has equal or high
 - Login rate limit: 5 attempts → 15-minute lockout (enforced in `models.check_login_allowed`)
 - `@login_required` — redirects to `/login` if no session
 - `@admin_required` — redirects to login or 403 if not admin
+- **Session fixation prevention** — `session.clear()` is called in `auth.login` immediately before setting `session["user"]` on successful authentication
+- **Keep-alive endpoint** — `GET /ping` (`routes/auth.py`, `@login_required`) touches `session.modified = True` and returns 204; called by the session-timeout warning in `app.js`
 
 ---
 
@@ -124,6 +152,8 @@ Route: `/ai-summary/` (`routes/ai_summary.py`)
 - **API call** — direct `requests.post()` to `https://api.groq.com/openai/v1/chat/completions` — the `groq` Python SDK is **not** installed
 - **Model** — `claude-sonnet-4-20250514` should NOT be used here; use `llama-3.3-70b-versatile` (Groq)
 - **Max tokens** — 4,096; **temperature** — 0.2
+- **Text limit** — combined document text is truncated to 14,000 characters before being sent; the JSON response includes `"truncated": true` when this occurs so the UI can warn the user
+- **`sort_order` input** — always wrap `int(request.form.get("sort_order", 0) or 0)` in `try/except (ValueError, TypeError)` — bad input must degrade gracefully to 0, not raise a 500
 
 ---
 
@@ -134,12 +164,16 @@ Route: `/bids/` (`routes/bid_tracker.py`)
 Split-pane UI — left: filterable bid list, right: detail panel with update timeline.
 
 Key AJAX endpoints (all return JSON, no page reload):
-- `GET /bids/list/json?status=` — paginated bid list
+- `GET /bids/list/json?status=` — bid list filtered by status
 - `GET /bids/<id>/json` — bid detail + updates + `can_edit` flag
 - `POST /bids/<id>/updates/json` — post new update
 - `POST /bids/updates/<id>/delete/json` — delete update
 
-Ownership rule: users can only edit/delete their own bids. Admins can edit/delete any bid. This is enforced in both the route (`can_edit = is_admin or bid.added_by == user.id`) and the rendered detail panel.
+Ownership rule: users can only edit/delete their own bids. Admins can edit/delete any bid. Enforced via `can_edit = is_admin or bid.added_by == user.id`.
+
+**`_ser(row)`** — module-level helper that converts a DB dict to JSON-serialisable form (dates/times → ISO strings). Do not redefine it inline inside individual functions.
+
+All write operations (create, edit, delete bid; add, delete update — both form and JSON variants) call `log_action()`.
 
 ---
 
@@ -184,26 +218,49 @@ Route: `/dashboard/` (`routes/user_dashboard.py`)
 ```
 Open with `openModal('modal-mymodal')` from `app.js`.
 
+### Adding a new POST form (CSRF checklist)
+- **Template extends `base.html`** → nothing extra needed; `app.js` IIFE injects the token automatically
+- **Standalone template** (like `login.html`) → add `<input type="hidden" name="csrf_token" value="{{ csrf_token() }}">` inside the `<form>`
+- **`fetch()` POST** → add `'X-CSRFToken': getCsrfToken()` to the `headers` object
+- **JS-built form string** → concatenate `'<input type="hidden" name="csrf_token" value="' + getCsrfToken() + '">'`
+
 ### Serialising DB rows to JSON
-MySQL connector returns `datetime`, `date`, `timedelta` objects which are not JSON-serialisable. Always convert:
+MySQL connector returns `datetime`, `date`, `timedelta` objects which are not JSON-serialisable. Use the shared helper (already defined at module level in `bid_tracker.py`; replicate it where needed):
 ```python
-def ser(row):
+def _ser(row):
     return {k: v.isoformat() if hasattr(v, 'isoformat') else v for k, v in row.items()}
 ```
 
+### Adding a new model query with optional search/filter
+Do **not** use f-strings to interpolate WHERE clauses into SQL. Build the query with string concatenation and always pass user values through the `%s` parameter list:
+```python
+sql = "SELECT ... FROM table"
+params = []
+if search:
+    sql += " WHERE col LIKE %s"
+    params.append(f"%{search}%")
+sql += " ORDER BY id DESC LIMIT %s"
+params.append(limit)
+cur.execute(sql, params)
+```
+
 ---
 
 ## Known Gotchas
 
 | Gotcha | Detail |
 |--------|--------|
-| `activity_log.logged_at` | Column is `logged_at` in the production DB, not `created_at`. Queried as `al.logged_at AS created_at` |
+| `activity_log.logged_at` | DDL now creates `logged_at`. A migration in `config.py` renames `created_at` → `logged_at` for old web-only installs. Queried as `al.logged_at AS created_at` in `get_activity_log()` |
 | `TIME` columns → `timedelta` | MySQL returns `TIME` as `datetime.timedelta`. Use the `\|hhmm` filter in templates |
 | `GROQ_API_KEY` in `.env` | Read directly via `os.environ.get("GROQ_API_KEY")` in the analyze route — not via `get_setting()` alone — because `get_setting()` depends on the DB row being populated |
 | `div.modal` vs `.modal-overlay` | All modal backdrops must use `class="modal-overlay"`. Inner dialog boxes use `class="modal"` or `class="modal-dialog"`. Never use `div.modal` as a backdrop |
 | Jinja macros in child templates | Cannot call `{{ macro_name() }}` before the `{% macro %}` definition is parsed. Inline the HTML instead |
 | `INSERT IGNORE` for settings seed | Use `ON DUPLICATE KEY UPDATE value = IF(value='', VALUES(value), value)` — `INSERT IGNORE` silently skips rows that already exist, even with empty values |
 | `bcrypt` not in original requirements | Added as `bcrypt==4.1.3`. Required by `models.py` for password hashing |
+| `login.html` is standalone | Does NOT extend `base.html`. Has no `app.js`, no CSRF meta tag. Any form on this page needs `{{ csrf_token() }}` as a direct hidden input |
+| `SECRET_KEY` must be fixed | `os.urandom(32)` fallback generates a different key per Gunicorn worker — sessions and CSRF break across workers. Always set `SECRET_KEY` in `.env` |
+| `create_app()` called once | Only `wsgi.py` calls `create_app()`. Do not add a module-level call to `app.py` — it causes double initialisation (double `initialize_database()`, double blueprint registration) |
+| `import datetime` in `models.py` | Already imported at the top of the file. Do not add inline `import datetime` inside functions |
 
 ---
 
@@ -213,6 +270,9 @@ def ser(row):
 # Activate venv
 source /home/webchecker/venv/bin/activate
 
+# Install / update dependencies (e.g. after adding Flask-WTF)
+pip install -r requirements.txt
+
 # After changing Python files — reload Gunicorn (zero-downtime)
 sudo systemctl reload webchecker
 
@@ -242,6 +302,8 @@ mysql -u webchecker_user -p webchecker -e "SELECT key_name, value FROM app_setti
 /etc/nginx/sites-available/webchecker
 ```
 
+---
+
 ## Change Philosophy
 
 1. **Surgical, additive patches** — smallest possible change to achieve the goal
@@ -251,4 +313,29 @@ mysql -u webchecker_user -p webchecker -e "SELECT key_name, value FROM app_setti
 5. **Migration existence checks** — all migrations safe to re-run
 6. **Full file contents for 1–3 file changes**; deployment map for larger changesets
 7. **Explicit deploy instructions** — migration steps separated from code steps
-8. **Root cause analysis** on errors — never apply temporary workarounds
\ No newline at end of file
+8. **Root cause analysis** on errors — never apply temporary workarounds
+
+---
+
+## Pending Improvements (To-Do)
+
+### Security
+- [x] **File upload size cap** — `MAX_CONTENT_LENGTH = 20 MB` set in `create_app()`; `RequestEntityTooLarge` handler returns JSON for `/ai-summary/` paths and flash+redirect for form routes
+- [x] **HTTP security headers** — `@app.after_request` in `app.py` sets `X-Frame-Options`, `X-Content-Type-Options`, `Referrer-Policy` on every response
+- [x] **Prevent last-admin demotion/deactivation** — `update_user()` in `models.py` checks that demoting or deactivating an admin won't leave zero active admins; raises `ValueError` surfaced as a flash message
+- [x] **File MIME type validation** — `_validate_magic()` in `ai_summary.py` checks magic bytes for PDF (`%PDF`), DOCX/XLSX (`PK\x03\x04`), DOC/XLS (OLE header), and UTF-8 decodability for text types; no extra dependency needed
+
+### UI / UX
+- [ ] **Mobile-responsive sidebar** — No media queries exist; the sidebar + main-content layout is unusable on phones; implement a hamburger toggle that collapses the sidebar on small screens
+- [ ] **Bid due-date urgency badges** — Client-side only: highlight bids due within 7 days with a warning badge and bids past due with a red "Overdue" pill in the bid list
+- [ ] **Relative timestamps in Activity Log** — Format raw `YYYY-MM-DD HH:MM:SS` timestamps as "2 hours ago" / "3 days ago" using a small JS formatter
+- [ ] **Empty state for AI analysis history** — When no analyses exist, show a call-to-action ("Upload your first document ↑") instead of a blank panel
+- [ ] **Paginate bid list** — `get_all_bids()` fetches all rows with no limit; add `LIMIT`/`OFFSET` to the model query and a "load more" button in the split-pane list
+
+### Functionality
+- [ ] **Missed-shift alerting** — Query or report that flags shifts where zero `shift_checks` records exist for a given date, surfaced on the admin dashboard or via email
+- [ ] **Bid deadline email reminders** — Use the existing `email.smtp_*` settings to send a daily digest of bids with `due_date` within the next 7 days
+- [ ] **Server-side health checks** — Replace the Google favicon proxy in the user dashboard with a `/dashboard/health/<id>` route that makes a server-side `HEAD` request (with short timeout) for a real reachability signal
+- [ ] **Password reset via email** — Time-limited token flow so users can self-service instead of requiring an admin edit; needs a `password_reset_tokens` table and SMTP integration
+- [ ] **"Copy password" button in Credentials modal** — Wire `copyToClipboard()` (already in `app.js`) to the password field in the user dashboard credentials modal
+- [ ] **Shift calendar view** — Weekly grid (Mon–Sun columns, shifts as rows) on the admin shifts page to make schedule gaps and overlaps visible at a glance
diff --git a/app.py b/app.py
index 1701a25..01778a8 100644
--- a/app.py
+++ b/app.py
@@ -29,6 +29,9 @@ def create_app():
         )
     app.secret_key = secret_key or os.urandom(32)
 
+    # ── Upload size limit (20 MB) ──────────────────────────────────────────────
+    app.config["MAX_CONTENT_LENGTH"] = 20 * 1024 * 1024
+
     # ── Session timeout (30 minutes) ──────────────────────────────────────────
     from datetime import timedelta
     app.permanent_session_lifetime = timedelta(minutes=30)
@@ -70,10 +73,31 @@ def create_app():
     from flask_wtf.csrf import CSRFProtect
     CSRFProtect(app)
 
-    # ── Template context processors ────────────────────────────────────────────
-    from flask import session, redirect, url_for, g
+    # ── Security response headers ──────────────────────────────────────────────
+    from flask import session, redirect, url_for, g, request, jsonify, flash
     import functools
 
+    @app.after_request
+    def set_security_headers(response):
+        response.headers["X-Frame-Options"] = "SAMEORIGIN"
+        response.headers["X-Content-Type-Options"] = "nosniff"
+        response.headers["Referrer-Policy"] = "strict-origin-when-cross-origin"
+        return response
+
+    # ── 413 handler: file too large ────────────────────────────────────────────
+    from werkzeug.exceptions import RequestEntityTooLarge
+
+    @app.errorhandler(RequestEntityTooLarge)
+    def handle_too_large(e):
+        max_mb = app.config.get("MAX_CONTENT_LENGTH", 0) // (1024 * 1024)
+        msg = f"Upload too large. Maximum allowed size is {max_mb} MB."
+        if request.path.startswith("/ai-summary/"):
+            return jsonify({"error": msg}), 413
+        flash(msg, "danger")
+        return redirect(request.referrer or url_for("index")), 413
+
+    # ── Template context processors ────────────────────────────────────────────
+
     # ── Custom template filter: timedelta/time → "HH:MM" ──────────────────────
     import datetime as _dt
 
diff --git a/models.py b/models.py
index a923780..a141db7 100644
--- a/models.py
+++ b/models.py
@@ -274,7 +274,25 @@ def update_user(admin_id, user_id, username, role, full_name, is_active, passwor
     conn = None
     try:
         conn = get_connection()
-        cur = conn.cursor()
+        cur = conn.cursor(dictionary=True)
+
+        # Guard: prevent demoting or deactivating the last active admin.
+        cur.execute("SELECT role FROM users WHERE id=%s", (user_id,))
+        target = cur.fetchone()
+        if target and target["role"] == "admin":
+            removing_admin = (role != "admin") or (not is_active)
+            if removing_admin:
+                cur.execute(
+                    "SELECT COUNT(*) AS n FROM users WHERE role='admin' AND is_active=1"
+                )
+                if cur.fetchone()["n"] <= 1:
+                    cur.close()
+                    raise ValueError(
+                        "Cannot demote or deactivate the last active administrator. "
+                        "Promote another user to admin first."
+                    )
+
+        cur = conn.cursor()   # switch back to plain cursor for the UPDATE
         if password:
             cur.execute(
                 "UPDATE users SET username=%s, role=%s, full_name=%s, is_active=%s, "
diff --git a/routes/admin_users.py b/routes/admin_users.py
index cc487bb..241a4a2 100644
--- a/routes/admin_users.py
+++ b/routes/admin_users.py
@@ -58,6 +58,8 @@ def edit(user_id):
         update_user(admin_id, user_id, username, role, full_name, is_active, password, email)
         flash(f"User '{username}' updated successfully.", "success")
         logger.info(f"User id={user_id} updated by admin_id={admin_id}.")
+    except ValueError as e:
+        flash(str(e), "danger")
     except Exception as e:
         logger.error(f"update_user error: {e}")
         flash(f"Error updating user: {e}", "danger")
diff --git a/routes/ai_summary.py b/routes/ai_summary.py
index 20f3814..36d2761 100644
--- a/routes/ai_summary.py
+++ b/routes/ai_summary.py
@@ -111,9 +111,41 @@ def analyze():
                    "file_count": len(file_names), **result})
 
 
+# Magic-byte signatures for each supported binary format.
+# Office Open XML (docx/xlsx) and legacy OLE (doc/xls) share these headers.
+_FILE_MAGIC: dict = {
+    ".pdf":  b"%PDF",
+    ".docx": b"PK\x03\x04",
+    ".xlsx": b"PK\x03\x04",
+    ".doc":  b"\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1",
+    ".xls":  b"\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1",
+}
+
+
+def _validate_magic(data: bytes, ext: str) -> None:
+    """Raise ValueError if the file's actual bytes don't match its extension."""
+    magic = _FILE_MAGIC.get(ext)
+    if magic:
+        if data[: len(magic)] != magic:
+            raise ValueError(
+                f"File content does not match the declared {ext} format "
+                "(possible disguised upload)."
+            )
+    elif ext in (".txt", ".csv", ".md"):
+        # Text files must be decodable; binary data masquerading as text is rejected.
+        try:
+            data[:512].decode("utf-8")
+        except UnicodeDecodeError:
+            raise ValueError(
+                "Text file does not appear to be valid UTF-8. "
+                "Ensure the file is a plain text document."
+            )
+
+
 def _extract_text(file_obj, ext: str) -> str:
     """Extract plain text from an uploaded file object."""
     data = file_obj.read()
+    _validate_magic(data, ext)
 
     if ext in (".txt", ".md", ".csv"):
         return data.decode("utf-8", errors="replace")