Inboxintel/AUDIT_REPORT.md

# InboxIntel — Full Application Audit Report

**Date:** 2026-07-02 · **Scope:** entire repository (backend `src/`, `frontend/`, migrations,
CI/CD `.gitea/workflows/`, `docker-compose*`, `deploy/`, git history) · **Phase:** 1 (read-only)

**Stack:** .NET 8 / ASP.NET Core (Clean Architecture) · EF Core + PostgreSQL (pgvector image)
· React 18 + Vite SPA · Google OAuth2 (cookie session) · Gitea Actions CI/CD · self-hosted
Docker Compose (single-operator deployment).

## Executive summary
This codebase is **markedly more hardened than typical** — previous hardening phases (V-01…V-12,
visible in code comments) already addressed the classic killers: IDOR (EF global query filters),
SSRF (a genuinely thorough `SafeHttpGuard` with DNS-rebinding defence), token encryption at rest,
non-root containers, stack-trace suppression, forwarded-header trust, secret hygiene (nothing in
git history), and confirmed-destructive-actions. **No Critical findings.** The most significant
issues are: **registered input validators that never execute (H-1)**, **no rate limiting (H-2)**,
and a **data-at-rest posture** for email bodies that should be a deliberate, documented decision
(H-3). CI has real security gates (gitleaks + dependency audit) already.

Severity counts: **Critical 0 · High 3 · Medium 6 · Low 7**

---

## 1. Security

### H-1 · FluentValidation validators are registered but NEVER executed
- **File:** `src/InboxIntel.Application/DependencyInjection.cs:11` (registration) vs
  `src/InboxIntel.Api/Program.cs` (no `AddFluentValidationAutoValidation()` anywhere; no
  controller injects `IValidator<>`).
- **Why it's a problem:** `Validators.cs` defines real safety rules — `CleanupRequestValidator`
  requires `Confirmed=true` for Trash/HardDelete, `SearchRequestValidator` bounds paging, etc. —
  but FluentValidation 11.x requires explicit auto-validation enablement, which is absent. The
  rules are dead code; requests reach services unvalidated. *Mitigating factor:* the services
  re-check the most dangerous rules (`CleanupService.cs:48` re-enforces `Confirmed`), and the
  controller clamps paging — so this is defence-in-depth loss, not an open hole. But rules that
  exist **only** in the validators (e.g. `From <= To`, LabelId-required) are silently unenforced.
- **Fix:** add `services.AddFluentValidationAutoValidation()` (package already referenced) in
  `Program.cs`; add a regression test posting an invalid DTO and asserting 400.
- **Blast radius:** isolated (1 line + tests). Verify no existing client sends payloads that
  would now 400.

### H-2 · No rate limiting on any endpoint
- **File:** `src/InboxIntel.Api/Program.cs` (no `AddRateLimiter`/middleware anywhere).
- **Why:** login (`/auth/login` → OAuth), search (now with FTS + trigram + future AI), export
  (PDF generation), unsubscribe (server-side outbound HTTP), and AI endpoints are all
  unthrottled. A single authenticated user (or an unauthenticated client hammering
  `/auth/login`/`/appinfo`) can exhaust CPU/DB/outbound quota. For the future multi-user platform
  (see `docs/discovery/multi-provider/`), this is a prerequisite.
- **Fix:** ASP.NET Core's built-in `RateLimiter` — a global fixed-window per-IP policy + stricter
  policies on `auth`, `export`, `unsubscribe`, `ai`. Return 429 with Retry-After.
- **Blast radius:** isolated (Program.cs + policy constants + tests).

### Verified-good (no finding)
- **Authorization / IDOR:** `[Authorize]` on `ApiControllerBase`; only `AppInfo` + `Auth.login`
  are `[AllowAnonymous]` (correct). Global query filters on every tenant-scoped entity enforce
  per-user isolation even if a query forgets its `Where` — covered by `TenantIsolationTests`.
- **Injection:** all data access via EF parameterisation; no raw SQL string concatenation in app
  code; FTS uses `websearch_to_tsquery` parameters. Frontend has **zero**
  `dangerouslySetInnerHTML`/`innerHTML`/`eval`; the search-highlight feature deliberately uses
  non-HTML sentinels rendered as escaped React elements (XSS-safe by construction).
- **SSRF:** `SafeHttpGuard` validates scheme, resolves DNS and checks **every** address against
  loopback/private/link-local/CGNAT/metadata/multicast (v4+v6, v4-mapped), fails closed on
  unknown families, and is paired with `AllowAutoRedirect=false` (`DependencyInjection.cs:65`).
  This is better than most production code.
- **Secrets:** none hardcoded (empty placeholders in `appsettings.json`); no `.env` ever
  committed (git history checked); real secrets live in git-ignored `deploy/.env*` and Gitea
  Actions secrets; pre-commit hook + CI gitleaks scan both guard regressions.
- **Session cookies:** HttpOnly, SameSite=Lax, Secure-always outside dev, 401-not-redirect for
  XHR. Forwarded headers only trusted from configured proxy CIDRs (V-08).
- **Headers:** nosniff, X-Frame-Options DENY, Referrer-Policy, COOP on API; HSTS in prod.

### M-1 · No absolute session lifetime (sliding-only)
- **File:** `Program.cs:58-59` — `ExpireTimeSpan = 7d` with `SlidingExpiration = true`.
- **Why:** a session that's touched at least weekly renews forever; a stolen cookie never
  expires as long as the attacker uses it. No server-side revocation list exists either
  (cookie is self-contained).
- **Fix:** add an absolute cap via an `issued-at` claim checked in `OnValidatePrincipal`
  (e.g. re-auth after 30 days), and/or a session-stamp validated against the DB to enable
  revocation. (The multi-provider design doc 02/06 already specs DB-backed sessions — this
  aligns.)
- **Blast radius:** isolated.

### M-2 · No Content-Security-Policy on the SPA or API
- **File:** `Program.cs:156-167` (comment says CSP "report-only for now" but none is actually
  set); `frontend/nginx.conf` serves the SPA without security headers.
- **Why:** CSP is the main mitigation layer against any future XSS slip; currently absent.
- **Fix:** add CSP (default-src 'self'; connect-src API origin; etc.) + nosniff/XFO to the SPA
  nginx config; optionally a report-only CSP on the API.
- **Blast radius:** isolated (nginx conf + one middleware line), needs SPA smoke-test (Chart.js
  inline styles etc.).

### L-1 · CSRF: no antiforgery tokens (accepted-risk, documented here)
- SameSite=Lax cookies + strict CORS allowlist + JSON-only POST bodies make classic CSRF
  impractical in modern browsers. Acceptable for now; revisit if cookie SameSite is ever
  relaxed or non-JSON form endpoints are added.

### L-2 · `AllowedHosts: "*"` (`appsettings.json`)
- Host-header filtering disabled; low risk behind the proxy but set it to the real hostnames
  at production deployment.

---

## 2. Data protection (top priority)

**Inventory of sensitive data:**
| Data | Where | At-rest protection | Verdict |
|---|---|---|---|
| Google OAuth **refresh/access tokens** | `users.EncryptedRefreshToken` | **Encrypted** (Data Protection API, keys on `/keys` volume) | ✅ good |
| **Passwords** | — | **None stored** (OAuth-only login; no password column exists) | ✅ best-possible |
| **Email bodies / subjects / snippets** | `emails.BodyText` etc. | **Plaintext** in Postgres | ⚠️ **H-3** below |
| Sender names/addresses (PII) | `senders`, `emails` | Plaintext | part of H-3 |
| User email + display name | `users` | Plaintext | part of H-3 |
| DB password | `deploy/.env` (git-ignored) + Actions secret | not in repo | ✅ |
| Data Protection keys | `/keys` Docker volume | filesystem | M-3 below |
| IPs / payment / health data | — | not collected | ✅ n/a |

### H-3 · Email content stored in plaintext at rest, with unbounded retention
- **File:** `src/InboxIntel.Domain/Entities/Email.cs` (`BodyText`, `Snippet`, `Subject`);
  Postgres `pgdata` volume; also flows into PDF/CSV/JSON **exports** and (when enabled) to the
  local Ollama process.
- **Why:** the entire product is a copy of the user's mailbox. On this self-hosted,
  single-operator deployment the DB lives on the operator's own disk — a defensible posture —
  but: (a) anyone with disk/volume/backup access reads all mail; (b) there is **no retention or
  purge policy** (mail persists even after unsubscribe/cleanup in Gmail; "remove account"
  flows don't exist yet); (c) the planned **multi-user platform** (docs/discovery/multi-provider)
  makes plaintext-bodies-readable-by-host-admin a real privacy issue (its own security doc
  promises "admins never read users' mail" — the DB must back that up).
- **Fix (phased):** 1) *Document* the current posture in README/threat model (deliberate,
  local-first). 2) Enable **pgcrypto/field-level encryption or full-disk/volume encryption**
  before any multi-user deployment. 3) Add a **data-retention setting** + purge job and an
  account-deletion path (GDPR-style erasure). 4) Ensure DB **backups** inherit the same
  protection.
- **Blast radius:** documentation = trivial; field-level encryption = **large** (touches search
  — FTS can't index encrypted columns; would need architectural decision). Recommend
  volume-level encryption + retention/deletion first.

### M-3 · Data Protection keys stored unencrypted on the `/keys` volume
- **File:** `Program.cs:26-28` — `PersistKeysToFileSystem` without `ProtectKeysWith*`.
- **Why:** whoever reads the volume can decrypt all stored refresh tokens. Same-disk-as-DB
  caveat applies, but keys and ciphertext living side-by-side weakens the encryption's value.
- **Fix:** `ProtectKeysWithCertificate(...)` (cert from env/secret), or OS-level DPAPI on
  Windows hosts; at minimum document the volume-permissions requirement.
- **Blast radius:** isolated, but requires a key-migration step for existing tokens.

### M-4 · Default DB credentials in `appsettings.json`
- **File:** `appsettings.json:3` — `Password=inboxintel` as the fallback connection string.
- **Why:** if a deployment forgets the env override, the app happily connects with a guessable
  password (compose enforces `POSTGRES_PASSWORD` but a non-compose deployment might not).
- **Fix:** empty the default and fail fast at startup with a clear message when unset.
- **Blast radius:** isolated (plus updating dev docs to use user-secrets).

### Logging & transit — verified
- **Logs:** no token/password logging found; the one PII-ish log is `SmtpEmailSender.cs:28`
  (recipient address + subject at Info when SMTP is unconfigured) — acceptable, downgrade to
  Debug if desired (L-3). Serilog request logging does not include query strings or bodies.
- **Transit:** TLS terminates at nginx (HSTS enabled in prod); API+DB bound to loopback/compose
  network only. **DB connection itself is non-TLS** — fine while Postgres is co-located on the
  compose network; **must add `SSL Mode=Require` if the DB ever moves to another host** (L-4).

---

## 3. Dependencies & supply chain
- **`dotnet list package --vulnerable --include-transitive`:** **clean** (all projects) — the
  earlier round of CVE pins (System.Text.Json 8.0.6 etc.) is holding, and CI re-checks on every PR.
- **`npm audit` (production deps):** **0 vulnerabilities**.
- **M-5 · `npm audit` (dev deps):** vite 5.x depends on a vulnerable **esbuild** (1 moderate,
  1 high, dev-server-only vectors). Not shipped to users and CI already scopes to prod deps —
  but the fix is a routine **vite 5→6/7 upgrade**. Effort S.
- **Duplication (L-5):** both `MailKit` (Infrastructure) and the frontend carry sizeable deps;
  `react-grid-layout` exists solely for the draggable dashboard the redesign plans to retire —
  candidate for removal with the Analytics migration. No abandoned packages spotted.
- CI supply-chain posture: gitleaks (full history) + NuGet/npm audits are **required checks** —
  good. No SAST/CodeQL (L-6, nice-to-have).

## 4. Error handling & reliability
- **Good:** global `UseExceptionHandler` + RFC7807 ProblemDetails (no stack traces to clients);
  Polly retry/backoff on Gmail sync; idempotent sync upserts; AI calls wrapped in try/catch with
  non-AI fallbacks ("sync must never fail because the AI provider is down").
- **M-6 · Known EF model warning:** `Email` has a global query filter but is the required end
  of the `Email↔EmailLabel` relationship — logged on every boot; can yield surprising results
  when filtered parents are excluded. Fix: matching filter on `EmailLabel` (one line) + test.
  (Long-noted in logs; this is the nudge to actually do it.)
- **L-7 · Multi-step writes without explicit transactions:** e.g. `GoogleAuthEvents` upsert and
  sync batches rely on EF's single-SaveChanges transactionality — mostly fine; the sync
  cursor-advance + batch-upsert pairing is the one place an explicit transaction would guard a
  mid-batch crash (currently self-heals via idempotent re-sync — acceptable).

## 5. Performance
- **Fixed this cycle (verified live):** relevance-ranked FTS with weighted tsvector + GIN;
  trigram GIN indexes for the previously non-sargable sender/domain `.Contains()`; HNSW vector
  index ready for semantic search.
- **Remaining (all Low, roadmapped):** offset pagination degrades on deep pages (keyset planned
  in `docs/discovery/05`); frontend ships one **678 KB JS bundle** (no code-splitting — vite
  `manualChunks`/dynamic import, effort S); no HTTP caching/ETags on read-heavy endpoints
  (sidebar counts are fetched per page-load); `docker-compose` frontend nginx lacks gzip/brotli
  confirmation for the bundle.
- No N+1 patterns found (queries project with joins; aggregates precomputed in
  `AnalyticsAggregate`).

## 6. Code quality & architecture
- Clean Architecture discipline is genuinely observed (dependency rule intact; thin controllers;
  DTO mapping at boundaries). Config is env-driven throughout.
- **L-8:** `AiService.GenerateQueryAsync` returns raw LLM output as a search query (advisory-only,
  becomes a search string — harmless today; keep it that way when NL search lands: model output
  must stay data, never an executable/action).
- Dead-ish code: none significant; `docs/specs/*` and discovery docs are current.

## 7. Testing & CI
- **41 tests** (unit: parser/guard/unsubscribe/embeddings-null; integration: authz challenge,
  tenant isolation, pagination clamp, search fallback ordering). Critical security invariants
  (IDOR filter, SSRF guard, confirmed-destructive) **are tested** — better than most.
- **Gap (M-7):** no live-Postgres test harness — FTS/ranking/fuzzy/pgvector paths are verified
  manually against staging (documented in commit messages) but not repeatably in CI. A
  Testcontainers-Postgres (pgvector image) job would convert those throwaway verifications into
  permanent regression tests. Effort M.
- Gap (L-9): no coverage for `CleanupService`/`SyncService` beyond compilation; no E2E of the
  OAuth flow (hard without creds — acceptable).
- CI: build+test+secrets+deps as **required** PR checks; auto-deploy to staging with a
  post-deploy health gate; prod is tag-gated. Solid. Missing: lint/format check in CI (the
  pre-commit hook enforces locally; add `dotnet format --verify-no-changes` job, effort S) (L-10).

---

## Prioritized remediation plan

| # | Finding | Sev | Fix effort | Blast radius |
|---|---------|-----|-----------|--------------|
| 1 | **H-1** Enable FluentValidation auto-validation + 400 tests | High | S | Isolated (1 line + tests; verify clients) |
| 2 | **H-2** Rate limiting (global + auth/export/unsub/AI policies) | High | S–M | Isolated (Program.cs + tests) |
| 3 | **H-3** Data-at-rest posture: document now; retention setting + purge job + account-deletion; volume-encryption guidance; (defer field-level encryption decision) | High | S (doc) → M (retention) → L (encryption) | Doc: none · Retention: moderate · Encryption: large/architectural |
| 4 | **M-3** Protect Data Protection keys at rest | Med | S–M | Isolated + key migration |
| 5 | **M-1** Absolute session lifetime (+ revocation groundwork) | Med | S | Isolated |
| 6 | **M-4** Remove default DB password; fail fast | Med | S | Isolated |
| 7 | **M-2** CSP + security headers on the SPA nginx | Med | S | Isolated (needs SPA smoke test) |
| 8 | **M-6** Fix EF query-filter warning (EmailLabel filter) | Med | S | Isolated + test |
| 9 | **M-5** Vite upgrade (dev-dep CVEs) | Med | S | Frontend build only |
| 10 | **M-7** Testcontainers live-Postgres CI job | Med | M | CI + new test project wiring |
| 11 | L-2/L-3/L-4/L-10 (AllowedHosts, SMTP log level, DB TLS note, CI format check) | Low | S each | Isolated |
| 12 | L-5 bundle split / grid-layout removal (with redesign) | Low | S–M | Frontend |

---

# Phase 2 — Remediation status (2026-07-02, all items approved & implemented)

Shipped as four PRs (#20–#23), each with tests, green CI, and a verified staging deploy.

| Finding | Status | How it was resolved | PR |
|---------|--------|---------------------|----|
| **H-1** validators never ran | ✅ **Resolved** | `AddFluentValidationAutoValidation()`; invalid DTOs now 400 at the boundary. Proven by 2 integration tests (pageSize=0, From>To → 400) via a new test-auth scheme | #20 |
| **H-2** no rate limiting | ✅ **Resolved** | Global 300/min per-user (per-IP anonymous) + `auth` 10/min + `expensive` 20/min (export/unsubscribe/AI), config-driven, 429/no-queue. Proven by a 429-on-3rd-request test | #20 |
| **H-3** plaintext bodies + unbounded retention | ✅ **Resolved (as scoped)** | `SECURITY.md` documents the deliberate posture (volume-encryption + backup guidance, delete-my-data procedure); **opt-in retention** (`DataRetention:*`, default off) + daily purge worker with 3 tests. Field-level encryption deliberately deferred (FTS can't index encrypted columns) — revisit before any multi-user deployment | #22 |
| **M-1** sliding-only sessions | ✅ **Resolved** | Absolute 30 d cap (`Auth:AbsoluteSessionDays`) via issued-at stamp checked in `OnValidatePrincipal`; 3 unit tests (incl. missing-stamp = expired) | #20 |
| **M-2** no CSP on the SPA | ✅ **Resolved** | Full CSP (`script-src 'self'`, `frame-ancestors 'none'`, …) + nosniff/XFO/Referrer-Policy + gzip on the SPA nginx; inline theme script moved to `/theme-init.js` to keep `script-src 'self'` honest. **Verified serving on staging** | #21 |
| **M-3** unprotected DP key ring | ✅ **Resolved (opt-in)** | `DataProtection:CertificatePath/Password` → `ProtectKeysWithCertificate`; documented in SECURITY.md as recommended for shared hosts | #22 |
| **M-4** default DB password | ✅ **Resolved** | Guessable default removed from `appsettings.json`; startup fails fast with a clear message when the connection string has no password | #20 |
| **M-5** dev-dep esbuild CVEs | ✅ **Resolved** | vite 5→8 + plugin-react 6; `npm audit` now clean **including dev deps**; build verified | #21 |
| **M-6** EF query-filter warning | ✅ **Resolved** | Matching tenant filter on `EmailLabel` (via Email navigation); boot warning confirmed gone from staging logs; cross-user invisibility test added | #20 |
| **M-7** no live-Postgres tests | ✅ **Resolved** | CI `db-tests` job with a `pgvector/pg16` service container runs 3 permanent `Category=LiveDb` regression tests (FTS weighting, ts_headline sentinels, trigram typo fallback, pgvector cosine). **Confirmed green on the actual runner** | #23 |
| **L-2** AllowedHosts `*` | ✅ Documented | Production checklist item in SECURITY.md (set at deployment) | #22 |
| **L-3** SMTP recipient at Info | ✅ **Resolved** | Downgraded to Debug | #20 |
| **L-4** DB TLS note | ✅ Documented | SECURITY.md: add `SSL Mode=Require` if Postgres ever leaves the host | #22 |
| **L-10** no CI format gate | ✅ **Resolved** | `format` CI job (`dotnet format --verify-no-changes`) — `--no-verify` pushes can no longer bypass formatting | #23 |
| L-1 CSRF (accepted risk) | ✅ Documented | SECURITY.md rationale (SameSite=Lax + CORS + JSON) with revisit conditions | #22 |
| L-5/L-6/L-7/L-8/L-9 | ⏸ Deferred by design | Bundle-split & grid-layout removal ride the UI redesign; SAST/CodeQL, sync transaction hardening, and broader service coverage are Phase 3 recommendation candidates | — |

**Test suite: 40 → 54 tests** (48 always-on + 3 retention + 3 live-DB in CI).
Every deploy through the pipeline stayed green; staging verified after each batch.