Files
Ten31-Portal/SECURITY-AUDIT.md
Jonathan Kirkwood 3a4b7dfd8b 0.2.46: security hardening (external-fund scope, role guards, forced password rotation, headers)
Pre-release audit fixes:
- External Administrators can only act on investors already in their own funds
  (grant_access, create_statement, set_partner_exited, import match), closing a
  cross-fund account-takeover chain.
- Role and is_active changes are gated: no self-escalation, non-approvers cannot
  mint or alter approver accounts, and the Service Admin is protected.
- must_change_password is enforced server-side, not just in the client.
- Session cookie is Secure with a 12h max-age and SameSite; CSP and standard
  security headers on every response; the app refuses to boot on a known or short
  session secret; uvicorn runs with proxy-headers so login throttling keys on the
  real client IP behind the platform proxy.
- Import preview no longer confirms cross-fund investors; no-store on document
  downloads.
- Adds backend/tests/test_security_hardening.py (15 tests); full suite 76 passing.
- SECURITY-AUDIT.md records the swarm audit, findings, and remediation status.
2026-08-28 09:53:16 -05:00

18 KiB

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

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.