fix(api): bound re-roling in both directions, and give the service principal one permission model - #1750

Closes two advisories (three reports — two were duplicates) from @pavancharak, both submitted as code review with no proof of concept. Every claim was verified against the tree and then reproduced by execution against the real ASGI application, real Postgres with all 157 migrations applied, real JWTs and ENVIRONMENT=test, before anything was changed.

GHSA-hrc5-w9r3-2g8r — re-roling was bounded in one direction only

Submitted twice, 28 seconds apart, with byte-identical descriptions. GHSA-hrc5-w9r3-2g8r is first-filed and already has the reporter credit accepted, so it is kept; GHSA-qcgm-c945-q96h is closed as the duplicate naming it.

Confirmed. authorize_role_change checked that the caller may confer the new role and never that it holds what the target currently holds.

Symmetric authority is the rule but it is not what bites, and that distinction is the finding. Two tenant_admin principals hold identical permissions, so no subset test separates them — administrators managing administrators is what the role is for. What must be unreachable is the end state, and it was reachable:

• The last-administrator guard counted only the wildcard roles, and never_grantable refuses those to every API caller. On a tenant created through the product it therefore protected nothing.
• Reproduced: one tenant_admin demoted its only peer (200) and then itself (200), leaving a tenant with no principal holding users:write and no route to recovery short of database credentials.

Fixes: the subset test now applies to the role being taken away as well as the one conferred; the guard counts role_grants.administrative_roles(), derived from ROLE_PERMISSIONS so a role that gains users:write later is covered without anyone remembering. The second demotion now answers 409 with the row unwritten.

That guard had also never once worked. Its count bound :t and :keep as bare parameters against uuid columns, so asyncpg answered operator does not exist: uuid = character varying. Measured on unmodified main by deactivating the last wildcard administrator: HTTP 500, not the 409 the handler promises. Every bind is now cast, the CAST(:name AS type) spelling check_text_bind_casts.py enforces elsewhere.

Cache invalidation: in. The handler never bumped the tenant's RBAC version, so a principal resolved before the change kept its old set on every replica until the fallback TTL lapsed — for a revocation, the wrong direction.

Session revocation: out, with a reason that is measured rather than asserted. An access token carries no permissions; resolve_jwt_principal re-reads the user row and re-resolves the set on every request. A test demotes a principal and replays a token minted beforehand, and it is refused. The stale channel was the cache. Letting every tenant_admin terminate a peer's sessions would be a new capability, not a fix.

The inverse defect, also fixed. Where a tenant does have RBAC grants the stored role changes and the effective permissions do not, so an administrator is told a demotion worked when it did not. The audit row now carries effective_permissions_follow_role. It also now records from correctly: the UPDATE synchronises the loaded object, so reading user.role afterwards returned the new value and the trail read {"from": "viewer", "to": "viewer"}.

GHSA-ghpm-vc6j-hp5r — two declarations, and the permissive one won

Header-derived tenant: judged separately, and largely by design. A service token identifies a service, not a tenant; the agents container triages for every tenant on the deployment, so the tenant has to come from the work. The API already refuses a service caller that names no tenant and verifies the tenant exists. "Any real tenant is accepted" is the designed behaviour. The reporter's real point — the blast radius of a leaked token — is a legitimate design critique, and per-workload credentials (_resolve_workload_principal, already shipped, with per-credential scopes, attributable audit and rotation with a grace window) are the stronger answer. Migrating internal callers onto them is a larger change than an advisory fix and is not attempted here.

Permission conflict: confirmed, and worse than reported.

The reporter notes no current route exploits it. True for those two permissions — and the same blindness reopened a defect the resolver had already closed, which is reachable: resolve_permissions distinguishes "this tenant has no RBAC" from "this tenant has RBAC and gave this user none", returning the empty set for the second so removing every grant does not restore a static role. has_permission_db saw the same zero rows and could not tell them apart. A deprovisioned tenant_admin answered 403 at every require_permission route and 200 at GET /api/v1/mcp-servers.

Fixes: one object (SERVICE_PRINCIPAL_PERMISSIONS is derived from the role row, rationale living with the row), and require_permission_db delegates to the one decision function — so elevation and the tenant's attribute conditions now bind there too, having previously applied on one path and not the other.

check_one_permission_model.py gains the arm that would have caught this. It asserted everything about the dependency factory 275 routes bind and nothing about the method 27 routes await, so the second model it exists to prevent sat one method over with every check printing OK. Run against the pre-fix tree the new arm exits 1 naming all three defects.

Found while fixing, left open deliberately

The shared service token holds lake:query, and lake:query gates the hunt write routes — so a real service token reaches POST /api/v1/hunts and POST /api/v1/saved-hunts, measured at 422 (past authorization, into body validation) on unmodified main. A test asserted the opposite under the note that api_service "holds no lake permission either"; it passed only because it built a principal with no resolved set and so read the narrower of the two lists — the same two-declaration defect hiding its own consequence.

Closing it means splitting authoring from running into a write permission, which changes what four human roles hold and needs a migration for RBAC tenants. That is its own change, not an advisory fix. Recorded as a strict xfail so whoever does it is told to remove the marker, rather than as a passing test that blesses the exposure.

Evidence

Reproduction first, then the fix, then controls executed and watched.

• Pre-fix: 9 failed, 3 passed — every property assertion failing for the reason its name gives, all three positive controls passing so the harness is known good.
• Post-fix: 13 passed.
• Negative controls, each run and observed: removing the symmetric check → 1 failed; narrowing administrative_roles() back to the wildcard set → 4 failed; restoring the second permission model → 2 failed. Tree restored, baseline re-run at 13 passed.
• Full API suite: 5378 passed, 230 skipped, 1 xfailed.
• Gates: check_one_permission_model (with a 13-case self-test), check_role_grant_scope, check_text_bind_casts, check_claim_gate_matrix (402 rows / 393 GATED / 0 NO GATE), check_claim_matrix_sources, check_gate_coverage, readme_gates, check_machine_paths, check_comment_paths, check_competitor_names, check_attribution, ruff format --check, ruff check — all pass.

Three existing tests changed, each because it measured a declaration rather than the path:

• test_content_route_permissions.py asserted a service principal may submit phishing. Driven with the resolved set the shipped resolver hands out, that route answers 403 both before and after this change, and nothing in the tree calls it as a service — the capability did not exist.
• test_hunting_route_permissions.py — see above.
• test_autonomy_effective_posture.py left db.scalar unstubbed, so an AsyncMock returned a truthy value and the double claimed its tenant administers access through user_roles. A real bootstrap tenant answers 0.