Ten31-Portal/SECURITY-AUDIT.md

# Ten31Portal pre-release security audit

## Remediation status (2026-08-28)

Fixed and covered by tests in `backend/tests/test_security_hardening.py` (76 tests pass):

- **#1 Cross-tenant investor takeover (Critical)** — fixed. A shared `check_target_investor_scope`
  now gates every write that could mint an access grant or a statement for an arbitrary investor:
  `grant_access`, `create_statement`, `set_partner_exited`, and the import "match" action. An
  external Administrator can only ever act on an investor already inside their funds.
- **#2 Internal role/is_active mass-assignment (Critical)** — fixed. `_enforce_privileged_fields`
  blocks self-promotion, minting or tampering with approver accounts by non-approvers, and any
  change to the Service Admin's role or status.
- **#3 Shared default password bypass (Critical)** — fixed. `must_change_password` is now enforced
  server-side: such an account may reach only `me`, `change-password`, and `logout` until it
  rotates. (The shared default is now safe; moving to per-investor one-time secrets is still worth
  doing and is left as a follow-up.)
- **#4 Global login-throttle DoS (High)** — fixed. `start.sh` runs uvicorn with `--proxy-headers`
  so the limiter keys on the real client IP behind the platform proxy. Depends on the StartOS
  proxy forwarding `X-Forwarded-For`; verify on the box.
- **#5 Session-secret fallback (High)** — fixed. The app refuses to boot on a known/short secret;
  the `start.sh` placeholder list now matches `config.py`.
- **#6 Cookie hardening (Medium)** — partly fixed. `Secure` (via `TEN31_SESSION_COOKIE_SECURE`,
  on in the image), `SameSite=lax`, and a 12-hour max-age are set. Server-side session revocation
  is still a follow-up (sessions remain stateless signed cookies).
- **#7 Security headers (Medium)** — fixed. CSP, `X-Frame-Options: DENY`, `nosniff`,
  `Referrer-Policy`, and HSTS are set on every response; CSP verified against the built SPA with no
  violations.
- **#9 Import-preview cross-fund leak (Medium)** — fixed. External Administrators must name a fund
  and only see matches against investors in their scope.
- **P3 quick wins** — fixed: `Cache-Control: no-store` on document downloads; `nosniff` (via #7).

Still outstanding (documented below, not yet coded): **#8** XLSX zip-bomb / dimension memory
exhaustion and unthrottled argon2 on change-password / TOTP-disable; the money-as-float / Decimal
and range-validation cleanup; TOTP secret-at-rest encryption and replay protection; list-endpoint
pagination. These are Medium/Low and were left as a reviewed backlog rather than rushed in.

---

# Original findings

Date: 2026-08-28. Method: 10-agent adversarial swarm (one finder plus one independent
verifier per attack surface), followed by hand re-verification of every Critical and High
finding against the source. Overlapping findings are deduped below. Line numbers are from the
audited tree.

Scope reviewed: backend routers, auth/session/2FA, file storage and document serving,
CSV/XLSX import, money handling, rate limiting, StartOS deploy, and the React frontend.

## The short version

The app is written defensively (argon2, timing-safe login, path-traversal guards, upload
caps, privilege-dropping container). The real exposure is in **authorization logic**, not
memory-safety or injection. Three issues would let one account reach another account's money
and documents, or let one staff member seize the whole system. Fix the three P0 items before
any investor logs in. The P1/P2 items should follow before or immediately after launch.

| # | Severity | Issue | Core location |
|---|----------|-------|---------------|
| 1 | Critical | A fund administrator can take over any investor account in any fund | `user_router.py:144,157,522` |
| 2 | Critical | operations/cfo can self-promote to approver and disable other admins | `user_router.py:405-457,362-402`; `schemas.py:79-80` |
| 3 | Critical | Shared default investor password, forced-change enforced only in the browser | `config.py:17`; `capital_import_router.py:324-335`; `auth_router.py:85-98` |
| 4 | High | Login throttle is effectively global behind the proxy: 10 bad tries locks out everyone | `auth_router.py:33,49`; `ratelimit.py` |
| 5 | High | Session signing key silently falls back to a published constant | `config.py:7`; `start.sh:5`; `Dockerfile:44` |
| 6 | Medium | Signed-cookie sessions cannot be revoked; no Secure flag; up to 14-day life | `main.py:37`; `auth_router.py:144` |
| 7 | Medium | No security headers (CSP, X-Frame-Options, nosniff, Referrer-Policy, HSTS) | `main.py:36-52` |
| 8 | Medium | Malicious XLSX (zip bomb / huge declared dimension) exhausts the single worker | `import_router.py:72-83`; `capital_import_router.py:230-239` |
| 9 | Medium | Import preview leaks investor usernames across funds when entity_id is omitted | `capital_import_router.py:221-267` |
| 10 | Low set | Money-as-float, doc caching, latent stored XSS, self-approval, dev-server entrypoint, etc. | see below |

---

## P0 - fix before release

### 1. Cross-tenant investor account takeover by a fund administrator (Critical)

`fund_administrator` is an external, entity-scoped, semi-trusted third party. It should never
reach an investor outside its own funds, and never seize an investor login. It can do both.

The scope model has a gap: several write endpoints check that the *entity* is in the caller's
grants, but not that the *target investor* is already in scope. Because "in scope" is derived
from `EntityAccess` and `CapitalAccountStatement` rows (`_visible_investor_ids`,
`user_router.py:27-40`), any endpoint that *creates* one of those rows for an arbitrary
investor pulls that investor into the attacker's scope. After that, the stricter checks pass.

Exploit chain:

1. Attacker is any `fund_administrator`, holding a grant on their own entity `E`.
2. `PUT /api/users/{victim_id}/access/{E}` (`grant_access`, `user_router.py:137`). It calls
   only `check_administrator_scope(admin, E)` (entity check) and requires the target be an
   investor. It creates `EntityAccess(victim_id, E)` for any investor id, enumerable.
3. The victim is now returned by `_visible_investor_ids`, so they are "in scope."
4. `POST /api/users/{victim_id}/reset-password` (`reset_password`, `user_router.py:512`). Its
   `_check_target_in_scope` now passes. It sets `password_hash` to an attacker-chosen value,
   `login_enabled=True`, `must_change_password=False` (`user_router.py:523-526`).
5. Attacker logs in as the victim. Unless the victim self-enrolled TOTP, this is full takeover:
   their capital account, statements, and private documents (K-1s).

The same "mint a scope-granting row for an arbitrary investor" gap exists at three more
injection points, so fixing only `grant_access` is not enough:

- `create_statement` (`capital_account_router.py:126-173`): writes a `CapitalAccountStatement`
  for any investor id on the caller's entity, both fabricating capital figures under the
  victim's name and bringing them into scope.
- `set_partner_exited` (`entity_router.py:162-197`): creates an `EntityAccess` row for an
  arbitrary investor when marking an exit.
- `commit_import` "match" (`capital_import_router.py:299,339-382`): binds an arbitrary existing
  investor to the caller's fund.

Fix: in every one of these endpoints, when `accessible_entity_ids(admin)` is not None, require
the target investor to already be within scope before writing (call `_check_target_in_scope`,
or verify membership in `_visible_investor_ids`). The clean invariant: an external
Administrator can only ever act on an investor whose footprint is already a subset of their
grants, exactly the check `delete_user` already performs at `user_router.py:554-567`. Consider
also decoupling "reset password" from "is currently visible" so a freshly minted grant cannot
immediately unlock a credential reset.

### 2. Internal privilege escalation via mass-assignment (Critical)

`update_user` (`PATCH /api/users/{id}`, `user_router.py:405`) and `create_user`
(`POST /api/users`, `user_router.py:362`) are gated by `require_admin`, which includes
`operations` and `cfo`. `UserUpdate` exposes `role` and `is_active` as client-settable fields
(`schemas.py:79-80`); `UserCreate` exposes `role` (`schemas.py:70`).

The only role guard sits inside `if scope is not None` (`user_router.py:421-424`). For every
internal role `accessible_entity_ids` returns None (`auth.py:71`), so that block is skipped and
execution falls through to `setattr(user, key, val)` (`user_router.py:445-446`), writing `role`
and `is_active` verbatim. There is no self-edit guard and no `is_service_admin` guard on this
path (unlike `delete_user`).

Consequences for any authenticated `operations` or `cfo` account:

- `PATCH /api/users/{own_id}` with `{"role":"approver"}` grants final valuation sign-off, the
  one authority `operations` is explicitly denied (`models.py:16-17`, `round_router.py:182`).
  Segregation of duties collapses.
- `POST /api/users` with `{"role":"approver", ...}` mints brand-new approver accounts.
- `PATCH` with `{"is_active":false}` on any approver or the Service Admin locks them out.

Fix: strip `role` and `is_active` from the client-settable set for everyone except an explicit
role-management path guarded by `require_approver`; forbid self-role-change and any change to
`is_service_admin` accounts; treat `UserUpdate`/`UserCreate` role changes as a privileged
operation, not a field write.

### 3. Shared default investor password, forced change enforced only in the browser (Critical)

Imported and CLI-created investor accounts are minted with `login_enabled=True` and
`password_hash = hash(DEFAULT_INVESTOR_PASSWORD)` where `DEFAULT_INVESTOR_PASSWORD = "Ten31Portal"`
(`config.py:17`, `capital_import_router.py:324-335`). `must_change_password` is set True, but
nothing on the server enforces it: `login` issues a full authenticated session regardless
(`auth_router.py:85-98`), and no dependency in `auth.py` blocks API calls when the flag is set.
The forced-change screen exists only in the React client (`frontend/src/App.tsx`).

So during the window between account creation and the investor's first manual password change,
anyone who knows or guesses an investor's username or email (investor emails are frequently
known) can log in with the constant `"Ten31Portal"` and, by calling the API directly, skip the
change-password screen entirely and read that investor's capital account and documents.

Fix: enforce `must_change_password` server-side. A user with the flag set should get a session
that is allowed to hit only `change-password`, `me`, and `logout`, and be 403'd everywhere else
until they rotate. Better still, do not set a shared password at all: generate a unique random
initial secret per investor (as `start.sh` already does for the admin) and surface it once to
the operator, or drive onboarding through a one-time invite token instead of a standing default.

---

## P1 - fix before or right at launch

### 4. Login throttle is effectively global behind the proxy (High, availability)

The limiter is keyed on `request.client.host` (`auth_router.py:49,108`). StartOS serves the
service behind its own reverse proxy and the app does not parse forwarded headers, so
`request.client.host` is the proxy's single address for every user. The limiter
(`SlidingWindowLimiter(max_attempts=10, window=300)`) therefore behaves as one global bucket.

Impact: any unauthenticated visitor can send 10 bad logins and lock out *every* user for five
minutes, repeatable indefinitely, a trivial login denial-of-service against the whole firm. The
same coupling means real per-source throttling is impossible.

Fix: terminate identity at the proxy and pass a trusted client IP (uvicorn `--proxy-headers`
with a locked-down trusted-hosts set), then key the limiter on that. Add a separate per-account
failure counter with backoff (not a hard lockout by name) so one noisy source cannot deny
everyone, and apply the same throttle to `login/verify-totp` per account.

### 5. Session signing key can silently fall back to a published constant (High)

`config.SESSION_SECRET` defaults to the literal `"change-me-in-production"` (`config.py:7`).
`start.sh` only replaces it when the value is exactly `"change-me"` (`start.sh:5`), the sentinel
the Dockerfile sets (`Dockerfile:44`). The two sentinels do not match. If the service is ever
run with `TEN31_SESSION_SECRET` unset, empty, or set to anything other than `"change-me"`
(a different entrypoint, a compose override, a future packaging change), the guard does not
fire and the app boots on the hardcoded, source-visible key. Anyone can then forge a valid
session cookie for any user id and bypass authentication entirely.

Fix: make the app refuse to start when `SESSION_SECRET` is the known default or shorter than a
safe length, rather than silently accepting it. Align the `start.sh` sentinel with the config
default (or generate unconditionally when it matches either sentinel).

---

## P2 - fix soon after launch

### 6. Signed-cookie sessions cannot be revoked; cookie hardening (Medium)

`SessionMiddleware` stores the session in a client-side signed cookie (`main.py:37`), so the
SPEC's "server-side sessions" claim does not hold. There is no server-side session store and no
revocation: `logout` (`auth_router.py:144`) only clears the client's copy, and password change
or TOTP disable do not invalidate an already-stolen cookie. With the Starlette default max-age
(14 days), a captured cookie stays valid for up to two weeks. The cookie is also added without
`https_only=True`, so it has no `Secure` flag and can leak over any plaintext hop. Deactivating
a user is honored on each request (`auth.py:33`), which is the one saving grace.

Fix: pass `https_only=True` and a shorter `max_age` to `SessionMiddleware`. For real
revocation, add a server-side session table or a per-user `session_epoch` that increments on
logout, password change, and TOTP disable, and reject cookies minted before it.

### 7. Missing HTTP security headers (Medium)

No CSP, `X-Frame-Options`/frame-ancestors (clickjacking), `X-Content-Type-Options: nosniff`,
`Referrer-Policy`, or HSTS are set anywhere (`main.py:36-52`, SPA served via bare
`FileResponse`). Fix: add a small middleware that sets these on every response; nosniff also
hardens the document-download path against content sniffing (see item 10).

### 8. XLSX decompression / dimension memory exhaustion (Medium, availability)

The 50 MB upload cap (`storage.py`) bounds bytes on the wire, but a small crafted `.xlsx`
(zip bomb, or a tiny sheet declaring an enormous used range) can expand to gigabytes when
parsed (`import_router.py:72-83,145`; `capital_import_router.py:230-239`). On the single-worker
deployment this takes the portal down. Related: `change-password` and `totp-disable` run an
unthrottled argon2 verify (`auth_router.py`), a cheap way to pin CPU. Fix: parse with
read-only/streaming mode and cap row/column counts and decompressed size; throttle the argon2
endpoints.

### 9. Import-preview cross-fund username disclosure (Medium)

The capital-import preview path skips the administrator scope check when `entity_id` is omitted
(`capital_import_router.py:221-267`), letting an external Administrator confirm whether given
investor usernames exist in other funds. Related lower-severity leak: `get_user` returns an
investor's full cross-fund entity ids and linked-account names to an in-scope external
Administrator (`user_router.py:285-308`). Fix: require and enforce entity scope on preview;
filter `_entity_ids_for` and `linked_accounts` to the caller's scope.

---

## P3 - hardening backlog (Low / defense-in-depth)

- **Document downloads lack `Cache-Control: no-store`** (`document_router.py:153`): K-1s and
  statements can linger in shared-machine browser caches.
- **Uploaded Content-Type is stored unvalidated and echoed on download**
  (`document_router.py:120,155`; `storage.py:47`): currently defused because `FileResponse`
  forces `Content-Disposition: attachment`. Keep the attachment disposition, add `nosniff`, and
  consider allow-listing upload types so this stays latent.
- **Money handled as float**: `round(dollars*100)` instead of integer/Decimal across
  `capital_account_router.py:22-23`, `position_router.py:19-21`, `import_router.py:50,65-67`.
  No sign/range validation on `value_cents`/cents inputs (`schemas.py:202,255-263`), `abs()`
  forces distributions positive and flips legitimate negative reversals
  (`capital_import_router.py:158`), and NaN/Infinity floats can reach `round()` and persist.
  Validate sign/range and parse money with `Decimal`.
- **No maker-checker on valuation sign-off** (`round_router.py:179-202`): the same user can
  submit and approve. Enforce a different approver.
- **Packaged `ten31portal` console entrypoint runs `uvicorn --reload` on 0.0.0.0**
  (`main.py:103-105`, `pyproject.toml:28-29`). Confirm production uses `start.sh` (it does) and
  drop or guard the reload entrypoint so it cannot be the prod launch by accident.
- **TOTP**: secret stored in plaintext (`models.py:86`); codes replayable within the validation
  window (no last-used timestep, `totp.py:37-39`); brute-force throttle is global-per-IP only,
  no per-account cap. Encrypt the secret at rest, record and reject the last-used step, add a
  per-account 2FA counter.
- **Upload size enforced only after Starlette buffers the whole multipart body**
  (`storage.py:40-63`): add a global request-size limit at the proxy.
- **Unbounded, un-paginated list endpoints with N+1 queries**
  (`document_router.py:48-80`, `entity_router.py:39-112`): paginate and batch.

---

## Method note and honest caveats

The swarm's automated verifier confirmed all 38 raw findings and refuted none, which is weaker
skepticism than ideal. Because of that, every Critical and High item above was re-checked by
hand against the source before being written here, and the raw findings were deduped (the
default-password issue, cookie flags, security headers, session-secret fallback, and
money-as-float each surfaced from multiple angles and are listed once). The P3 items are lower
confidence and lower impact; treat them as a backlog, not blockers. This review is static
analysis of the code; it is not a penetration test against a running instance, and it does not
cover the StartOS platform's own network exposure or the reverse proxy's TLS configuration.