# AgentForms Backend Audit Report

**Date:** 2026-06-30
**Scope:** Full backend audit — Security, Architecture, API Design, Database, Payment, Infrastructure, Testing
**Stack:** Flask + SQLite + Redis + RQ + Stripe, Python 3.11, Docker

---

## Summary

| Severity | Count |
|----------|-------|
| Critical | 4 |
| High     | 9 |
| Medium   | 15 |
| Low      | 8 |

---

## 🔴 CRITICAL

### C1 — Stripe E2E Checkout Test Never Completed (Known)
- **Files:** `app/routes/billing.py`, `tests/`
- **Issue:** Both paying tiers (starter/pro) were manually set in DB. No automated end-to-end Stripe checkout test exists. Webhook handling has no replay protection (no `_stripe_event_seen` table or Redis set tracking).
- **Risk:** Payment flow regression goes undetected; webhook replay attacks.
- **Fix:** 
  1. Add Stripe test-mode E2E test using Stripe CLI webhook forwarding.
  2. Add `stripe_event_id` dedup table: `CREATE TABLE IF NOT EXISTS stripe_events_seen (event_id TEXT PRIMARY KEY, processed_at TIMESTAMP)`.
  3. Check for existing `event.id` before processing any webhook.

### C2 — SQLite `check_same_thread=False` + WAL Mode with Concurrent RQ Workers
- **File:** `app/db.py:1`
- **Code:** `sqlite3.connect(DB_PATH, check_same_thread=False, timeout=30)`
- **Issue:** `check_same_thread=False` allows cross-thread access which SQLite does not safely support without careful locking. Combined with WAL mode (`PRAGMA journal_mode=WAL`) and multiple RQ workers, this creates a data corruption risk under concurrent writes.
- **Risk:** Silent data corruption — corrupted DB files, lost writes, split reads.
- **Fix:** Use a threading lock around all SQLite operations. Recommended pattern:
  ```python
  import threading
  _db_lock = threading.Lock()
  
  def get_db():
      conn = sqlite3.connect(...)
      conn.row_factory = sqlite3.Row
      return conn
  
  # In every DB operation:
  with _db_lock:
      conn = get_db()
      try:
          conn.execute(...)
      finally:
          conn.close()
  ```
  Alternatively, switch to a single-threaded connection pool with `queue.Queue` for DB requests.

### C3 — Rate Limiter Fail-Open on Redis Failure for Auth Endpoints
- **File:** `app/services/ratelimit.py:74-75, 100-102`
- **Code:**
  ```python
  if not self._connected:
      return (True, max_requests, 0)
  # ...
  except Exception as e:
      logger.error(...)
      return (True, max_requests, 0)
  ```
- **Issue:** Rate limiter returns `True` (allow all) when Redis is down OR on any Redis exception. Auth endpoints (`/api/login`, `/api/register`) and password reset depend entirely on this. If Redis goes down, all rate limiting disappears.
- **Risk:** Credential stuffing attacks succeed during Redis outage.
- **Fix:** Implement a local (in-memory) fallback rate limiter for auth endpoints:
  ```python
  from collections import defaultdict
  from time import time
  
  _local_limits = defaultdict(list)
  
  def _local_allow(key: str, max_requests: int, window: int) -> bool:
      now = time()
      # Prune old entries
      _local_limits[key] = [t for t in _local_limits[key] if now - t < window]
      if len(_local_limits[key]) >= max_requests:
          return False
      _local_limits[key].append(now)
      return True
  ```

### C4 — Password Verification Timing Oracle
- **File:** `app/routes/auth.py:50-64` (login endpoint)
- **Code:**
  ```python
  user = find_user_by_email(email)
  if not user:
      return jsonify({"status": "error", "message": "Invalid email or password"}), 401
  if not bcrypt.checkpw(password.encode(), user["password_hash"].encode()):
      return jsonify({"status": "error", "message": "Invalid email or password"}), 401
  ```
- **Issue:** The function returns early if the user is not found, before bcrypt.checkpw is called. bcrypt takes ~300ms; the "user not found" path returns immediately. This creates a timing side channel that allows email enumeration.
- **Risk:** Attackers can enumerate valid email addresses by measuring response time.
- **Fix:** Always execute bcrypt.checkpw even for non-existent users:
  ```python
  user = find_user_by_email(email)
  if not user:
      bcrypt.checkpw(password.encode(), b"$2b$12$dummiesalt.dummiesalt.dummiesalt")
      return jsonify({"status": "error", "message": "Invalid email or password"}), 401
  if not bcrypt.checkpw(password.encode(), user["password_hash"].encode()):
      return jsonify({"status": "error", "message": "Invalid email or password"}), 401
  ```

---

## 🟠 HIGH

### H1 — Password Reset Token Not Invalidation After Use
- **File:** `app/models_user.py` (password reset function)
- **Issue:** After a password reset, the token is not cleared from the DB. An attacker who intercepts the reset link can reuse it after the legitimate user resets their password.
- **Risk:** Password reset token reuse.
- **Fix:** Clear the token after successful reset:
  ```sql
  UPDATE users SET password_reset_token = NULL, password_reset_sent_at = NULL WHERE id = ?
  ```

### H2 — Missing Rate Limit on Password Reset & Magic Login Endpoints
- **File:** `app/routes/auth.py`
- **Issue:** Only `/api/login` and `/api/register` have `@limiter.limit`. Password reset and magic login endpoints have no rate limiting.
- **Risk:** Email bombing, token brute-force.
- **Fix:** Add rate limiting decorators:
  ```python
  @auth.route("/api/password-reset", methods=["POST"])
  @limiter.limit("3 per hour", key_func=get_client_ip)
  def request_password_reset():
  ```

### H3 — CSRF Exemptions for Submission Endpoints
- **File:** `app/app.py:501-520` (CSRF config)
- **Code:** `exclude_from_csrf = ["/submit", "/submit/", "/api/..."]`
- **Issue:** All submission endpoints are excluded from CSRF protection. While necessary for public form submissions, this means any page can POST to your submission endpoint without protection.
- **Risk:** Cross-site request forgery on form submissions — spam injection.
- **Mitigation:** This is partially acceptable for public forms, but should be compensated with:
  1. Honeypot field (already implemented: `_check_spam_content`)
  2. IP-based rate limiting on submission endpoints
  3. ReCAPTCHA or hCaptcha integration

### H4 — No File Upload Size Limit
- **File:** `app/app.py`
- **Issue:** `app.config["MAX_CONTENT_LENGTH"]` is not set. Flask defaults to unlimited upload size.
- **Risk:** Denial of service via large file uploads exhausting disk/memory.
- **Fix:** Add to app config:
  ```python
  app.config["MAX_CONTENT_LENGTH"] = 16 * 1024 * 1024  # 16 MB
  ```

### H5 — Stripe Price IDs Fall Through to Empty Strings
- **File:** `app/models.py:98-103`
- **Code:**
  ```python
  STRIPE_PRICE_IDS = {
      "starter": os.environ.get("STRIPE_STARTER_PRICE_ID") or os.environ.get("STRIPE_PRICE_STARTER", ""),
      ...
  }
  ```
- **Issue:** If env vars are not set (empty string), the price ID defaults to `""`. Checkout sessions created with empty price IDs fail silently. No validation at startup.
- **Risk:** Broken payment flow in production if env vars are misconfigured.
- **Fix:** Validate at startup and fail loudly:
  ```python
  for tier, price_id in STRIPE_PRICE_IDS.items():
      if not price_id:
          raise RuntimeError(f"STRIPE_{tier.upper()}_PRICE_ID not configured")
  ```

### H6 — Circular Import Pattern Between Models Modules
- **File:** `app/models_user.py` imports from `app.models` → `app/models.py` imports from `app.models_user.py`
- **Issue:** `models.py` line 172 re-exports from `models_user.py`, while `models_user.py` imports `try_decrypt_user_value` from `app.models`. This creates a circular dependency that works by accident (import ordering) and breaks under test isolation or module reload.
- **Risk:** Import failures in test environment, unpredictable behavior on hot reload.
- **Fix:** Break the cycle by moving `try_decrypt_user_value` directly to `models_user.py` or a shared `app/crypto_utils.py`. Remove the back-reference from `models.py`.

### H7 — Duplicate `get_user_tier` Function Definition
- **File:** `app/models.py:234` and `app/models_user.py`
- **Issue:** `get_user_tier` is defined in `models_user.py` AND redefined in `models.py:234`. The version in `models.py` shadows the one from `models_user.py` for any code importing from `models`.
- **Risk:** Subtle behavioral differences if the two implementations diverge. Maintenance confusion.
- **Fix:** Remove the duplicate from `models.py` (line 234-252). Use only the version from `models_user.py`.

### H8 — Duplicate `get_site_owner_tier` Function
- **File:** `app/models.py:255` and `app/models_user.py`
- **Issue:** Same pattern as H7. Two implementations of `get_site_owner_tier` exist.
- **Risk:** Same as H7.
- **Fix:** Remove duplicate from `models.py` (line 255-274).

### H9 — No Idempotency on Site Creation / User Registration
- **File:** `app/routes/api.py`, `app/models_site.py`, `app/models_user.py`
- **Issue:** Network retries during user registration or site creation can result in duplicate records. No idempotency keys or dedup logic.
- **Risk:** Duplicate user accounts, duplicate sites on retry.
- **Fix:** Accept an `Idempotency-Key` header. Store processed keys:
  ```python
  CREATE TABLE IF NOT EXISTS idempotency_keys (
      key TEXT PRIMARY KEY,
      response_status INTEGER,
      response_body TEXT,
      created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP
  );
  ```

---

## 🟡 MEDIUM

### M1 — `models.py` Is a 3,429-Line God Module
- **File:** `app/models.py`
- **Issue:** Despite splitting into 9 domain modules, `models.py` still contains ~3,000 lines of active code including duplicate functions (`create_or_update_session`, `create_variant`, `get_variants`, etc.) that were supposed to be extracted. The C2 refactor was incomplete.
- **Risk:** Maintenance burden, merge conflicts, slow test imports, cognitive overload.
- **Fix:** Complete the extraction. Every function currently in `models.py` that isn't a re-export or shared config (TIERS, STRIPE_*) should be moved to its domain module. `models.py` should be < 50 lines.

### M2 — `print()` Used for Error Logging Instead of Logger
- **Files:** `app/models.py:336`, `app/helpers.py:2102`, `app/helpers.py:2580`, and many more
- **Code:** `print(f"[model] create_or_update_session error: {e}")`
- **Issue:** Production errors are sent to stdout via `print()`, not through the logging framework. Structured logging exists (`app/services/logging.py`) but is underutilized.
- **Risk:** Errors lost in Docker logs, no correlation IDs, no severity levels.
- **Fix:** Replace all `print()` calls with `logging.getLogger(__name__).error(...)`.

### M3 — SQL Injection Risk in Dynamic UPDATE Statements
- **Files:** `app/models.py:484`, `app/model_email_campaigns.py`, and others
- **Code:**
  ```python
  clauses = ", ".join(f"{k} = ?" for k in updates)
  conn.execute(f"UPDATE form_variants SET {clauses} WHERE id = ?", values)
  ```
- **Issue:** While `k` comes from a hardcoded `allowed` set (safe now), the pattern is fragile — if `allowed` is ever populated from user input, this becomes SQL injection.
- **Risk:** SQL injection if `allowed` set is compromised.
- **Fix:** Use a whitelist validation function that explicitly checks each key against a constant:
  ```python
  ALLOWED_VARIANT_FIELDS = frozenset({"name", "field_config", "success_message", "weight", "active"})
  updates = {k: v for k, v in kwargs.items() if k in ALLOWED_VARIANT_FIELDS}
  ```

### M4 — No Index on `api_keys` Table Columns
- **File:** `app/models_initdb.py` (schema) + migrations
- **Issue:** `api_keys.user_id` and `api_keys.key_hash` have no indexes. API key validation does a full table scan on every authenticated request.
- **Risk:** Performance degradation as key count grows. 1000+ keys = noticeable latency.
- **Fix:** 
  ```sql
  CREATE INDEX IF NOT EXISTS idx_api_keys_user_id ON api_keys(user_id);
  CREATE INDEX IF NOT EXISTS idx_api_keys_key_hash ON api_keys(key_hash);
  ```

### M5 — Missing Index on `submissions.customer_email`
- **File:** Schema (initdb + migrations)
- **Issue:** Email-based lookup queries on submissions table scan the entire table.
- **Risk:** Slow email-based submission lookups.
- **Fix:** `CREATE INDEX idx_submissions_customer_email ON submissions(customer_email);`

### M6 — No Cascading Delete for Related Records
- **File:** Schema definition
- **Issue:** `sites` → `submissions` → `webhook_logs` → `form_sessions` → `form_analytics` → `rate_limits` → `usage` — none use `ON DELETE CASCADE`. Manual cleanup required for data deletion (GDPR compliance).
- **Risk:** Orphaned records on user/site deletion. GDPR non-compliance.
- **Fix:** Add `ON DELETE CASCADE` to all FK constraints or implement a cascade-delete helper in the user deletion flow.

### M7 — No Database Backup Strategy
- **File:** `docker-compose.yml`
- **Issue:** SQLite DB lives in a Docker volume with no backup mechanism. No automated `sqlite3 .dump` pipeline.
- **Risk:** Data loss on container/volume destruction.
- **Fix:** Add a cron job or scheduled RQ task:
  ```python
  # Daily backup
  import subprocess
  subprocess.run(["sqlite3", DB_PATH, ".backup", f"/backups/relay-{date.today()}.db"])
  ```

### M8 — N+1 Query Pattern in Submission List Endpoints
- **File:** `app/routes/api.py` (submission list) + `app/helpers.py`
- **Issue:** When listing submissions, each submission's encrypted fields are decrypted individually in a Python loop rather than in a single batch operation.
- **Risk:** Performance degradation with large submission counts.
- **Fix:** Batch decryption or decrypt only when rendering, not in the API list response.

### M9 — No Transaction Rollback on Multi-Step Operations
- **File:** `app/routes/api.py`, `app/models_site.py`
- **Issue:** Site creation, user registration, and document generation involve multiple DB operations with no transaction boundaries. If the second operation fails, the first is not rolled back.
- **Risk:** Partial state — e.g., site created but usage not incremented.
- **Fix:** Wrap multi-operation sequences in transactions:
  ```python
  conn.execute("BEGIN")
  try:
      # operation 1
      # operation 2
      conn.commit()
  except:
      conn.rollback()
      raise
  ```

### M10 — No Migration Version Tracking
- **File:** `app/models_initdb.py:109-140`
- **Issue:** 37 migrations are imported and called sequentially. There's no version table to track which migrations have run. While they claim idempotency (`IF NOT EXISTS`), this is fragile — any migration that modifies data (not just schema) will re-execute on every restart.
- **Risk:** Data corruption from re-running non-idempotent migrations (e.g., `migrate_encrypt_existing_data`, `migrate_encrypt_user_pii`).
- **Fix:** Add an `alembic_version`-style table:
  ```sql
  CREATE TABLE IF NOT EXISTS migration_versions (version INTEGER PRIMARY KEY);
  -- Before each migration:
  -- SELECT version FROM migration_versions WHERE version = N
  ```

### M11 — Email Verification Token Reuse
- **File:** `app/models_user.py` (verify_email_token)
- **Issue:** After email verification, the token is not invalidated. Re-sending verification email generates a new token but doesn't invalidate the old one.
- **Risk:** Old verification links remain valid until they expire.
- **Fix:** Clear token after verification:
  ```sql
  UPDATE users SET email_verification_token = NULL WHERE id = ?
  ```

### M12 — `app/__init__.py` Is Empty
- **File:** `app/__init__.py`
- **Issue:** The package init file is empty. No app factory pattern, no module-level configuration.
- **Risk:** Import path confusion, no clear package entry point.
- **Fix:** Add minimal package metadata:
  ```python
  __version__ = "1.0.0"
  __author__ = "AgentForms"
  ```

### M13 — Redis Dependency Not Declared as Hard Dependency
- **File:** `docker-compose.yml`
- **Issue:** RQ worker depends on Redis but the Flask app silently degrades when Redis is unavailable. Rate limiter fails open (H3), and webhook delivery silently drops.
- **Risk:** Silent data loss (webhooks, emails) during Redis downtime.
- **Fix:** Add a Redis health check at app startup that blocks until Redis is available for critical services, or implement a local queue fallback.

### M14 — Encryption Key Loaded at Import Time
- **File:** `app/crypto.py:1`
- **Code:** `ENCRYPTION_KEY = os.environ.get("ENCRYPTION_KEY", "")`
- **Issue:** Key is loaded at module import time (before app initialization). If the key changes (e.g., hot reload, key rotation), the old key remains in memory.
- **Risk:** Stale encryption key in memory after rotation.
- **Fix:** Load key lazily or provide a `set_encryption_key()` function for key rotation.

### M15 — No API Request Logging
- **File:** `app/app.py`
- **Issue:** No middleware logs incoming API requests (method, path, status, duration, user_id). The structured logger exists but isn't wired to request/response hooks.
- **Risk:** Cannot debug production issues, audit user actions, or detect suspicious patterns.
- **Fix:** Add before/after request hooks:
  ```python
  @app.before_request
  def log_request():
      request._start_time = time.time()
  
  @app.after_request
  def log_response(response):
      duration = time.time() - request._start_time
      struct_log.info("http_request",
          method=request.method, path=request.path,
          status=response.status_code, duration=duration)
      return response
  ```

---

## 🟢 LOW

### L1 — Hardcoded API Key Prefix `afk_live_` with No Test Variant
- **File:** `app/models.py:105`
- **Issue:** `API_KEY_PREFIX = "afk_live_"` is hardcoded. No `afk_test_` prefix for test environments. Cannot distinguish test keys from production keys.
- **Risk:** Accidental use of production API keys in test environment.
- **Fix:** Support both prefixes:
  ```python
  API_KEY_PREFIXES = ["afk_live_", "afk_test_"]
  API_KEY_PREFIX = os.environ.get("API_KEY_PREFIX", "afk_live_")
  ```

### L2 — OpenAPI/Swagger Spec Typo
- **File:** `app/routes/api.py:226`
- **Issue:** `content-type` header should be `Content-Type` (proper capitalization in OpenAPI spec).
- **Fix:** Trivial capitalization fix in spec definition.

### L3 — Missing `Content-Type` Validation on API Endpoints
- **File:** `app/routes/api.py`
- **Issue:** API endpoints accept POST/PUT without validating `Content-Type: application/json`. Non-JSON POSTs produce confusing errors.
- **Fix:** Add `@require_json` decorator or check `request.is_json` before processing.

### L4 — `models.py` Re-exports All Migration Functions
- **File:** `app/models.py:108-124`
- **Issue:** `models.py` re-exports 37+ migration functions. These are never called from `models.py` — they're called from `models_initdb.py`. The re-export serves no purpose.
- **Fix:** Remove migration re-exports from `models.py`.

### L5 — `app/helpers.py` Is 2,595 Lines (God Module)
- **File:** `app/helpers.py`
- **Issue:** Contains spam detection, PII decryption helpers, geo resolution, email template generation, and more — all in one file. Violates single responsibility principle.
- **Fix:** Split into: `spam_detection.py`, `pii_helpers.py`, `geo_utils.py`, `email_templates.py`.

### L6 — No API Versioning
- **File:** `app/routes/api.py`
- **Issue:** All API routes are under `/api/` with no version prefix (`/api/v1/`). Breaking changes cannot be deployed without affecting all clients.
- **Fix:** Prefix routes with `/api/v1/` and plan for `/api/v2/`.

### L7 — Dockerfile Python Version Hardcoded to 3.11
- **File:** `Dockerfile:1`
- **Issue:** `FROM python:3.11-slim` but pip resolves to python3.13. Version mismatch between base image and runtime.
- **Fix:** Align Dockerfile base image with actual Python version, or pin explicitly.

### L8 — No Graceful Shutdown Signal Handling
- **File:** `worker.py`, `app/app.py`
- **Issue:** No `SIGTERM`/`SIGINT` handlers. Docker stop sends SIGTERM but workers may drop in-flight tasks.
- **Fix:** Add signal handlers:
  ```python
  import signal
  signal.signal(signal.SIGTERM, lambda s, f: shutdown_gracefully())
  ```

---

## Appendix A: Database Index Audit

### Existing indexes (from initdb + migrations):
| Table | Index |
|-------|-------|
| submissions | `idx_submissions_site_id` |
| submissions | `idx_submissions_created` |
| sites | `idx_sites_user_id` |
| api_keys | `idx_api_keys_key_hash` (from migration) |
| usage | `idx_usage_user_month` |
| form_analytics | `idx_form_analytics_site_date` |
| rate_limits | `idx_rate_limits_key_window` |

### Missing indexes:
| Table | Column | Priority |
|-------|--------|----------|
| api_keys | `user_id` | High |
| submissions | `customer_email` | Medium |
| form_actions | `chain_id` | Medium |
| invoice_schedules | `email_encrypted` | Medium |
| email_campaigns | `user_id` | Medium |
| teams | `user_id` (member lookups) | Medium |
| webhook_destinations | `site_id` | Medium |
| form_sessions | `site_id + created_at` | Low |
| users | `email_verified` | Low |
| users | `stripe_customer_id` | Low |

---

## Appendix B: Rate Limit Coverage Audit

| Endpoint | Rate Limited | Config |
|----------|-------------|--------|
| POST /api/register | ✅ Yes | 5/hour |
| POST /api/login | ✅ Yes | 5/minute |
| POST /api/password-reset | ❌ No | — |
| POST /api/magic-login | ❌ No | — |
| POST /api/api-keys | ❌ No | — |
| POST /submit | Partial (IP-based in helpers) | — |
| GET /api/sites | ❌ No | — |
| GET /api/submissions | ❌ No | — |
| POST /api/stripe/checkout | ❌ No | — |
| POST /api/webhook/stripe | ❌ No | — |

---

## Appendix C: Auth Flow Diagram

```
Register: POST /api/register → validate → create user (bcrypt hash) → send verification email
Verify: GET /api/verify-email/{token} → validate token → set email_verified=1
Login: POST /api/login → find_user → bcrypt.checkpw → create_session
Magic Login: POST /api/magic-login → find_user → generate token → send email
Magic Verify: GET /api/verify-magic/{token} → validate → create_session
Password Reset: POST /api/password-reset → find_user → generate token → send email
Password Reset Apply: POST /api/reset-password/{token} → validate → update password
API Key Auth: GET request → parse Bearer header → validate_api_key() → set g.current_user
```

---

## Appendix D: Recommendations Priority Matrix

| # | ID | Effort | Impact | Priority |
|---|-----|--------|--------|----------|
| 1 | C1 | Medium | Critical | P0 |
| 2 | C2 | Medium | Critical | P0 |
| 3 | C3 | Small | Critical | P0 |
| 4 | C4 | Small | Critical | P0 |
| 5 | H1 | Small | High | P1 |
| 6 | H2 | Small | High | P1 |
| 7 | H5 | Small | High | P1 |
| 8 | H9 | Medium | High | P1 |
| 9 | M10 | Medium | Medium | P2 |
| 10 | M1 | Large | Medium | P2 |
| 11 | M4 | Small | Medium | P2 |
| 12 | M6 | Medium | Medium | P2 |
| 13 | M7 | Small | Medium | P2 |