Removes POST /admin/validate-and-allowlist — an unauthenticated endpoint, live in production, that wrote directly to the Clerk allowlist. Adds a regression guard so the shape cannot return.
−73 / +413 across 2 files (removal + hardened guard). Route count 686 → 685.
> Please prioritise this one. It is an authentication bypass rather than ordinary dead-code cleanup. Everything below is verified against the code and production, not inferred.
## The finding
The handler had no auth dependency of any kind — no dependencies= on the decorator, no Depends(...) parameter, and admin_router has no router-level dependency either. It POSTed straight to https://api.clerk.com/v1/allowlist_identifiers using CLERK_SECRET_KEY.
It was reachable from the public internet. An unauthenticated POST to the production host reached the handler and was rejected only by the application's own allowed-signup-domains check:
POST https://adoption-api.klairvoyant.ai/admin/validate-and-allowlist→ HTTP 403 {"detail":"Email domain '…' is not allowed. Please use your work email."}
That 403 is the finding, not a reassurance: the request arrived at application logic with zero credentials.
I probed with a deliberately invalid domain so the request could not mutate the allowlist.
### Allowlisting is enough to obtain access
There is no second approval gate behind it. utils/auth.py:337 auto-creates a Klair user record for any allowlisted identity that authenticates:
if not user_data:logger.warning(f"User {user_id} authenticated but not found in DynamoDB - creating default user")
user_data = await UserService.create_default_user(user_id, email, jwt_issuer)
Auto-created users land on the Default team with no page overrides — so the blast radius is Default-team visibility, not admin. But it is a real account, obtained without approval.
### Realistic exploit
The domain gate means this is not open to arbitrary attackers — an attacker needs an address in an allowed domain that they control or can predict. The practical abuse is pre-allowlisting an address that does not exist yet, or a departed employee's address, bypassing the approval workflow entirely.
How wide the exposure actually was is still unconfirmed, and this is the one claim in this PR I cannot close from the code. is_internal_domain validates against ALLOWED_SIGNUP_DOMAINS, which merely *defaults* to trilogy.com; the repo never sets it, so the deployed value is what actually bounded this. If any environment widened that list, the historical exposure was correspondingly wider.
Corrected from an earlier revision of this description, which asserted a @trilogy.com bound as settled — @ashwanth1109 caught that in review (finding 11). Someone with access to the production environment should confirm the deployed value.
## It was superseded one day after it was written
| Date | Commit | What happened |
|---|---|---|
| 2025-09-22 | 24e1f44b5 | Added the route and its only caller (SignUpModal.tsx:50) |
| 2025-09-23 | ac81e6929 | Replaced the caller with the access-request workflow — left the endpoint registered |
The replacement commit shows both sides of the swap in one diff:
- const response = await fetch(${...}/admin/validate-and-allowlist, {+ import { submitAccessRequest, validateAccessRequestForm } from '../services/accessRequestApi';
It has been live, unauthenticated and caller-less for ~10 months. Zero callers repo-wide — backend, frontend, MCP package, crons.
The two paths differ in exactly the way that matters:
- removed route — *writes* to the allowlist → access granted immediately
- /admin/submit-access-request (:1462) — *checks* the allowlist, then files a pending request requiring human approval
So leaving the endpoint reachable undid the control its replacement exists to impose. The identical Clerk write in /admin/create-user is gated by Depends(require_super_admin) — the same operation was protected in one place and open in another.
## Evidence of exploitation: none found, but the window is limited
Nginx access logs on the EC2 host across the full ~15-day retention window contain one hit on this path — my own probe (curl/8.7.1, my IP, during this investigation).
This does not rule out earlier activity. Retention is ~15 days against a ~10-month exposure, and ALB access logging is disabled on both load balancers. If anyone wants certainty, the Clerk allowlist should be audited directly for identifiers that never came through the approval workflow. Happy to help with that — flagging it rather than assuming it is covered.
## The regression guard (+156)
Deletion fixes today; the test is what stops the shape coming back. tests/routers/test_admin_allowlist_write_guard.py fails if any admin route writes to the Clerk allowlist without enforcing a caller check.
Writing it surfaced something my manual review had missed. A naive "must have Depends" check flags approve_access_request and process_approval_with_options — both write to the allowlist with no FastAPI dependency. They are in fact safe: they are reached from notification-email links where the recipient has no session, so a dependency cannot apply, and they verify a signed token in the body, raising 401 on every failure path.
So the guard accepts two auth shapes — a named auth callable in dependencies=[...] or a Depends(...) default, and in-body verify_admin_token() plus a raised 401.
The first revision of this guard was considerably weaker on both counts; see the review round below for what changed and why.
## Verification
| Check | Result |
|---|---|
| Route absent from the live app.routes table | ✅ 686 → 685 |
| Replacement workflow intact | ✅ submit / approve / deny / create-user all present |
| Shared helpers retained | ✅ check_user_in_allowlist (:1462), is_internal_domain (:866, :1436) |
| AST symbol diff vs main | ✅ exactly one name removed |
| Tests | ✅ 320 pass (314 before + 6 guard tests) |
| ruff · pyright · boot check | ✅ clean; boot matches main |
One note on method: a local curl to the removed path returns 403, not 404 — that is the global auth middleware answering before routing. I confirmed removal against the live route table instead, since the 403 would have been a false confirmation either way.
## Review round: all 14 findings addressed (fcd0cfe66)
@ashwanth1109's automated review raised 14 findings, all on the guard test rather than the endpoint removal. I reproduced each before fixing it; every one held up. Test-only commit — the removal is unchanged.
Detection gaps (1, 2, 5). The guard only counted a write when the Clerk URL was a bare positional literal in a .post(...) attribute call. I built each evading shape and confirmed all four slipped past while the control was caught: requests.post(url=...), an f-string URL, a URL in a variable, and from requests import post; post(...). The aggravating detail was real — the old loop *found* the URL constant, then discarded that evidence and returned False when the call-shape match failed, failing open exactly where it should fail loudly.
Detection is now inverted: any mention of the allowlist path inside a route counts as a possible write unless every call carrying it is provably a read.
False auth (3, 4). _has_auth accepted any dependencies= kwarg and any default whose ast.dump contained the substring "Depends" — so dependencies=[], Depends(get_db) and Query(None, description="Depends on the team") all read as gated. Finding 4 is the one I'd single out: my inline comment claimed the check proved the route "actually rejects", which substring co-presence cannot do — the comment promised more than the code delivered. Auth callables are now name-matched against an explicit set, and the token branch requires a real ast.Call plus a raised status_code=401.
Blind spots are now pinned by tests, not comments — a comment cannot fail. Three new tests fail if a write moves into a helper, if the file adopts add_api_route, or if the signed-token branch regresses.
Mutation-checked six ways:
| Mutation | Caught by |
|---|---|
| Restore the original endpoint | test_removed_endpoint_stays_removed |
| Reintroduce via post(url=...) | main guard — evaded the previous version |
| Reintroduce behind Depends(get_db) | main guard — evaded the previous version |
| Move a write into a helper | inline-assumption test |
| Switch to add_api_route | registration test |
| Neuter the token 401 | token-branch control |
Each failed the intended test and restored green. Simplifications 6–10 applied (single-walk detection verified behaviour-identical across all 32 handlers before replacing it); comment corrections 11–14 applied.
Post-review: 6 guard tests, 320 tests across tests/routers/ and the access-request suite, ruff clean.
## Follow-ups (not in this PR)
1. Audit the Clerk allowlist for identifiers not traceable to an approved access request — the log window cannot cover the full exposure.
2. Confirm the production ALLOWED_SIGNUP_DOMAINS value — it bounds how wide the historical exposure actually was, and cannot be determined from the repo.
3. Surfaced by the same audit: unused-endpoints-audit.md §3d lists 24 more UNCERTAIN routes never resolved. This one took ~15 minutes of git archaeology to settle definitively, which suggests the others are resolvable too rather than genuinely ambiguous. Two look worth an early look — /passive-investments/portfolio-analytics returns fabricated data (documented violation V-023-01), and /upload-acquisition_performance is a live manual Redshift write.
