fix(security): first-run setup refuses when a real admin already exists (#2381)

[obasilakis]

Summary

• On a fresh install setup_completed stayed false while a real admin already existed, and the unauthenticated POST /api/setup/admin-password gated only on that flag — so it would overwrite a live admin's password hash and bind the caller's email as its sign-in identity. The endpoint now refuses whenever a usable admin exists (its own precondition, not a derived flag), fail-closed, and above the bcrypt hash.
• setup_completed is made honest at boot on both backends. PostgreSQL previously had no writer for the key at all.
• Closes the escalation that let the takeover outlive a password reset: /api/auth/email/verify now re-checks the allow-list before redeeming a code.

Why not a migration

setup_completed_backfill asks "does an admin exist?" during run_all_migrations, which on a fresh install runs while users is still empty — then the second pass records it as applied, so it can never answer correctly for anyone. Every affected install has already booted with it recorded, so a new migration would inherit the same once-only semantics and reach none of them. _mark_setup_completed_if_provisioned{,_engine} is a boot-time reconciliation that re-runs every start, so exposed installs converge on their next restart. It never raises — init_database runs at import, and a raise there crash-loops the backend.

Wizard scope is now honest

Group	How they run it	Admin at boot?	Wizard
Hosted / marketplace droplet	prebuilt image, public IP	yes	hidden
Production compose	ADMIN_PASSWORD mandatory (:?)	yes	hidden
start.sh interactive / --unattended	refuses blank / auto-generates	yes	hidden
Dev compose with password set	set by hand	yes	hidden
Dev compose, blank password	—	no	still shown
Hand-rolled backend	—	no	still shown

This closes trinity-enterprise#49's tokenless first-run window without reinstating the setup token. ent#49 priced that tradeoff on the premise there is no admin yet — which now holds exactly where the wizard still renders. Same bug class as #177 (pentest 3.1.6, CVSS 7.5), which the token fixed and ent#49 reopened.

Re-homed side-jobs

The wizard was also the only capture point for two things it inherited six months after it was written:

• Admin sign-in email → dismissible post-login prompt (AdminEmailNudge.vue → Settings → General). Strictly better placed: an unauthenticated wizard's "admin email" can be typed by whoever loads the page first on a hosted install.
• Product-updates opt-in → has no second home; filed as abilityai/trinity-enterprise#463. Unrelated to telemetry sharing (ent#12), which already has its own Settings surface.

Also in scope

• ADMIN_USERNAME was hardcoded as "admin" in routers/setup.py. On an ADMIN_USERNAME=root install that missed the real admin and update_user_password (an upsert) INSERTed a second role='admin' account for the caller. Now resolved through utils/admin_identity.admin_username().
• docker-compose.prod.yml never passed ADMIN_USERNAME (present in docker-compose.yml and .env.example), so the variable was inert in production — the feat(mcp): inline email auth — sign in from an MCP client with no API key (#848) #1707 packaging-gap class. Proven by docker compose config resolution, not grep.
• start.sh's closing summary no longer promises a wizard that will not appear; the --unattended password it prints now actually works.
• Router guard: the "setup complete → /login" branch sat inside if (!to.meta.isSetup) while /setup carries meta.isSetup: true, making it unreachable dead code. /setup rendered the full wizard on a completed install; only the backend 403 stopped a submit.

Deliberately NOT closed

get_or_create_email_user resolves by the email column alone and will return the admin row for any address bound to it. The allow-list re-check removes the path to a platform JWT, but the underlying design question — may an email lookup ever return the admin account? — touches every email-auth consumer and a wrong narrowing locks operators out of their own instances. Out of scope here.

Test Plan

• pytest tests/unit/test_2381_setup_fail_closed.py tests/unit/test_2381_email_verify_whitelist.py — 20 new tests: the exploit verbatim, the blank-ADMIN_PASSWORD flow that must stay open, fail-closed on read error, ADMIN_USERNAME honoured on both halves, refusal precedes bcrypt, already-broken-install convergence, both DB backends, never-raises on both, and test_predicate_halves_agree pinning the two halves against drift.
• Full backend unit suite: 12319 passed. The 21 remaining failures (test_736_a2a_outbound_*, test_mcp_validator, test_ent14_registry_url_ssrf, test_ent399_ipv6_origin) reproduce identically on an untouched checkout — pre-existing macOS IPv6/DNS environment failures, not this change.
• tests/unit/test_2322_mfa_challenge_response.py — its email-route stub now models an allow-listed user (2 tests were failing on the new gate; both are real coverage of the gate, not relaxations).
• tests/unit/test_setup_operator_profile.py — FakeDB grows get_user_by_username; an empty users dict is exactly the no-admin install, so all six existing tests keep exercising the happy path.
• Frontend: npm run build clean, npm run test:unit 1176 passed, raw-color ratchet clean for the new component.
• Structural guards: models-centralized, admin-gate AST, enumeration uniformity, auth wiring, migrations, alembic parity.
• Manual: boot a fresh install with ADMIN_PASSWORD set and confirm /api/setup/admin-password answers 403 before any browser has touched the instance.

Fixes #2381

[dolho]

/review Report

Branch: feature/2381-setup-fail-closed → dev @ 8a8b788
Files Changed: 17 (+1062/−30)
Scope: DRIFT — justified, see below
Plan Completion: both suggested fixes done · the issue's one open question (admin email) resolved explicitly rather than silently · requested test coverage present on both backends

Critical Findings

None. The security fix is correct, and correct in the places it would have been easy to get wrong. Four things I checked rather than took on trust:

• The key rename. db.get_user_by_username returns _row_to_user_dict, which maps row["password_hash"] to the dict key "password" ("Keep as 'password' for backward compat", db/users.py:50). The gate reads existing_admin.get("password") — correct. A .get("password_hash") here would have returned None on every install, made admin_provisioned permanently False, and shipped a fix that silently does nothing while every test that stubs the DB still passes. This is the single highest-consequence line in the PR and it is right.
• Convergence for already-exposed installs. _mark_setup_completed_if_provisioned sits inside init_database's unconditional path on both branches — not gated on fresh-install detection — so the PR's claim that exposed installs converge on next restart holds.
• "This narrows nothing legitimate." /api/auth/email/request does gate on db.is_email_whitelisted before minting (auth.py:~45), so a code obtained through the web flow already passed the identical check. Verified, not assumed.
• The raw INSERT. (key, value, updated_at) matches system_settings' three columns exactly and mirrors backup_primitives.py's existing writes — so the reconcile can't fail into the state described in [I1] through a column mismatch.

Also verified: DatabaseManager really exposes get_user_by_username (database.py:602) — the facade-drops-the-signature class that bit ent#365; UserOperations/SettingsOperations are imported at module scope; the two edits to existing tests are honest (test_2322's stub models an allow-listed user, test_setup_operator_profile's empty users dict is the no-admin install) rather than relaxations; the raw-color ratchet exits 0 with the new component present.

---

Informational Findings

[I1] The mutual-lockout state is now unrecoverable in-product, and the new 403 points at the locked door (Confidence: 8/10)

routers/auth.py:339 refuses /api/token with setup_required while setup_completed is false. The new gate refuses /api/setup/admin-password whenever a usable admin exists. So {usable admin, flag false} is both doors locked — no login, no wizard.

The boot reconcile is what prevents it, and it prevents it well. But note what changed about its failure mode: _mark_setup_completed_if_provisioned catches broadly and prints a warning, and before this PR a skipped reconcile was harmless — the wizard still worked (dangerously). After it, a skipped reconcile means nobody can get in at all. Reachable if the users read succeeds but the system_settings write doesn't (read-only disk, a full volume).

What sharpens it is the refusal copy:

"Sign in with the admin password from your deployment configuration"

In the one state where this refusal is unexpected, that advice sends the operator to an endpoint that also answers 403. Two cheap options, either sufficient: append "restart the backend to re-run reconciliation" to _SETUP_COMPLETED_RECONCILE_SKIPPED, or add a line to DEPLOYMENT.md's new §1. The recovery already exists — it just isn't reachable from either message the operator can see.

[I2] The security comment enumerates 3 of 4 other code producers (Confidence: 9/10)

routers/auth.py names telegram_adapter, whatsapp_adapter and mcp_auth_service. There is a fourth: client_portal/service.py:213, which mints via the same shared create_login_code for any email with agent access — independent of the allow-list — and it is arguably the most reachable of the set, since Workspace clients are external customers by definition.

The fix covers it (the re-check is at the redeemer, so producer count doesn't matter), and nothing legitimate breaks: the Workspace redeems through its own portal_signin_verify, which calls core_db.verify_login_code directly and re-checks email_has_access. So this is a comment accuracy issue, not a hole. It matters because an enumeration in a security comment is exactly what the next person will trust when they add producer five — and this comment's argument would be stronger with the portal case in it.

[I3] start.sh hardcodes admin in the copy, against this PR's own thesis (Confidence: 8/10)

The PR introduces admin_username() because ADMIN_USERNAME is honoured and two callers wrongly assumed "admin". The closing summary it edits still prints Log in as 'admin' (and admin / <generated> in the other branch). On an ADMIN_USERNAME=root install the installer now tells the operator the wrong username — a smaller instance of the bug being fixed, in the file being touched.

[I4] The skip message interpolates the exception; its siblings in this same PR use type(e).__name__ (Confidence: 7/10)

_SETUP_COMPLETED_RECONCILE_SKIPPED % e vs routers/setup.py and routers/auth.py, which both deliberately log type(e).__name__ on the adjacent error paths. Low risk in practice (boot-time print, and the engine sets hide_parameters=True), but the raw-e form is the one the rest of the PR avoids on purpose.

[I5] New component uses raw gray palette classes rather than semantic tokens (Confidence: 6/10)

AdminEmailNudge.vue styles with bg-white dark:bg-gray-800, text-gray-500, etc. Ratchet-clean (gray is the "partially sanctioned chrome" tier) and it matches ActivationChecklist.vue sitting directly above it on the same page, so consistency argues for it. Noting only.

---

Scope

DRIFT, and I'd keep it. The allow-list re-check in routers/auth.py is a second, distinct vulnerability (privilege escalation through code producers that don't gate) touching every email login on the platform. Bundling it means a reviewer judging "does #2381 work" also has to judge that. It earns its place because it closes the path that made the first bug survive a password reset, and the PR says so explicitly — but it's the part of this diff most deserving of a second pair of eyes, and it is the part with the least direct test-to-exploit correspondence.

The other extras (ADMIN_USERNAME plumbing, prod-compose gap, router dead code) are all the same defect's blast radius and belong here.

---

Clean Categories

• Auth boundary — the gate runs first, before the flag and before bcrypt; fails closed on read error; the ordering rationale (unauthenticated, unrate-limited, bcrypt is expensive, refusals must not be timing-distinguishable) is stated and correct.
• Both backends — SQLite raw-cursor and engine twins, each with its own never-raises test; PG previously had no writer for the key at all, which this fixes.
• Enum/value completeness — no new enum or status value.
• Docs — architecture, feature flow, requirements, DEPLOYMENT all updated in the same commit; the wizard-scope table in the PR body is the kind of thing that should be in the flow doc, and is.
• Test gaps — 20 new tests including the exploit verbatim, the blank-ADMIN_PASSWORD flow that must stay open, ADMIN_USERNAME on both halves, refusal-precedes-bcrypt, already-broken-install convergence, and a parametrised test_predicate_halves_agree pinning the two halves against drift. That last one is the right test to have written.

---

Summary

• Critical: 0
• Informational: 5 — [I1] worth acting on before merge (one line of copy), [I2]–[I4] cheap, [I5] note only
• Scope: drift, justified and disclosed
• CI: 23 pass, 3 skipping, 0 failures

Residual: the Test Plan's one unchecked box — booting a genuinely fresh install with ADMIN_PASSWORD set and confirming the 403 before any browser touches it — is the claim no unit test can make, since it depends on init_database's real boot ordering rather than a stubbed DB. Everything around it is verified; that one wants a real fresh boot. Happy to run it against a scratch instance if you want it closed before merge.