Repository navigation
security(auth): enumerate router gating by mechanism, before fixing anything (#15745) - #16170
Conversation
…nything (#15745) Two prior sweeps reported 30 ungated routers, then 15, then fewer -- each internally consistent and each wrong, always in the same direction. You cannot fix a set you cannot enumerate, so this is the enumerator; the fixes follow. THE CAUSE WAS FOUR MECHANISMS, NOT CARELESSNESS. This codebase gates via router-level APIRouter(dependencies=[...]), per-route Depends, inline calls in the handler body, and middleware. Every detector so far modelled fewer than exist, and a detector that models three of four reports the fourth as a hole. Measured on base: 94 registered routers, 80 gated, 13 ungated, 1 unreadable. THE VOCABULARY IS DERIVED, NEVER TYPED. A hand-written regex omitted check_admin_permission -- 140 imports, the most-used auth dependency in the backend -- and that one omission produced fifteen false findings. Names now come from what the auth modules export. A test asserts the derivation still contains it, so a narrowing cannot silently reintroduce the same false findings. Derivation gets the names; PURPOSE still has to be decided. NOT_A_GATE records twenty exports that live in auth modules and do not gate a request -- validate_path protects a filesystem argument, not an identity. Six of them were measured contributing to GATED verdicts, and none was the sole evidence for any router, so no verdict was wrong today. They are excluded anyway: a name that can contribute to GATED without gating anything is a false-GATED waiting for the file where it appears alone, and false GATED hides a hole where false UNGATED merely wastes an investigation. CONTROLS ARE ONE PER MECHANISM, NOT ONE PER DIRECTION. The sweep that reported 15 ran two controls, one gated and one ungated, and both passed while it was wrong, because both were shapes its regex already matched. A control drawn from the detector's own model tests the model against itself. MIDDLEWARE IS DECLARED UNMODELLED rather than quietly skipped. middleware.py assumes request.state.user is already populated upstream and #15758 found there is no upstream for at least some routes, so it may cover nothing, something or everything. UNGATED here means "not gated by mechanisms 1-3", which is weaker than "reachable unauthenticated", and a test fails if anyone models it without re-verifying every control. Reproduces all five confirmed findings from #15745: api/redis.py, api/developer.py, api/wake_word.py, services/knowledge_sync_service.py, api/knowledge_suggestions.py. They are recorded in KNOWN_UNGATED so the guard can be turned on NOW rather than waiting on the fixes; the list may only shrink and a test fails if it goes stale. The enumerator design, the four mechanisms, the derived vocabulary and the control set are all autobot-ai-b9's, handed over in full when worktree capacity blocked them from building it.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
|
| before | after | |
|---|---|---|
| gated | 80 | 81 |
| ungated + undocumented | 12 | 12 |
| documented-open | 1 | 1 |
| unreadable | 1 | 0 |
The twelve are unaffected — I re-derived them independently and they hold.
Suggested control, since this is now a demonstrated shape
Add api.redis_mcp as a GATED control for the package form, alongside the four mechanism controls. It is the only package-form router in the registry, so if the resolver ever regresses to .py-only, that control is the one thing that notices. A bucket labelled "unreadable" is the dangerous kind of output — it looks examined.
One more, unrelated to the resolver
api/user_provider_credentials.py has three routes — :66 POST, :153 GET, :206 DELETE — but its decorators wrap onto the next line (@router.post( then arguments below). A route counter matching @router\.(get|post|...)\(" on one line reports zero routes for it. That does not change its UNGATED verdict, but it would make it look empty in any per-router route count, and "0 routes, ungated" reads as harmless when it is a credentials surface with a DELETE.
Everything else in this PR I checked stands: the five reproduce exactly, MIDDLEWARE_UNKNOWN = True with a test guarding it is the right way to encode an unresolved question, and NOT_A_GATE recording twenty decisions rather than a pattern is the correct response to validate_path/encode_jwt/hash_password being auth-module exports that gate arguments rather than identity.
…15745) reach_declarations_test runs every declared discover() against a scratch directory and requires a ReachFloorError from the floor below. Mine raised FileNotFoundError from discovery itself, so the mutation test failed on two counts: test_no_guard_can_succeed_against_an_empty_tree and test_discovery_honours_the_root_it_is_given. A guard that crashes reports "broken" where it should report "reached nothing", and those are different findings — which is the distinction this whole file exists to enforce, so getting it wrong here mattered more than usual. registered_routers now returns [] on an absent or unparseable registry, which relocates the failure to the reach floor where it is typed. Verified against a tempdir (returns []) and against the real tree (94 routers), not reasoned about.
comment_line_number_citations_test flagged two comments in the router enumerator citing api/agent_org.py:53 and initialization/middleware.py:287. A line number is the referent kind that goes stale fastest — the file moves, the citation still resolves, and it now points at something else entirely. Replaced with symbol references. Both comments say the same thing and survive an edit to the file they name. Worth noting the guard caught this in a file whose whole subject is detectors that report cleanly about the wrong thing.
…ounted itself (#15745) Review ran the enumerator against a reconstructed tree and found four defects. Two were FALSE FINDINGS I had already published, one against a credentials surface. FALSE UNGATED, both real gates: api/service_messages.py Depends(_check_admin) on all 3 routes. _check_admin is defined IN THAT FILE and calls is_admin_role(). api/user_provider_credentials.py Depends(get_current_user_id) on all 3 routes. Imported from api.user_management.dependencies, which itself does Depends(get_current_user) and raises 401. Both were invisible because the vocabulary was derived from the IMPORT PATH containing auth/security/permission/rbac, not from whether the name performs a check. That is the same defect as the hand-written regex missing check_admin_permission — in a new spelling, produced by the fix for the old one. The issue body singled out user_provider_credentials as "wants looking at first". It was already gated. One-hop resolution now reaches a dependency defined locally or imported from any path, if its own body reaches an auth primitive. THAT FIX IMMEDIATELY CREATED A FALSE GATED, which is the dangerous direction. api/jwks.py:_build_jwks calls get_auth_middleware() to fetch the SIGNING KEY and publishes a public key set — it authenticates nobody. Measured: _check_admin reaches {get_auth_middleware, is_admin_role}, _build_jwks reaches {get_auth_middleware} alone. ACCESSOR_NOT_DECISION excludes the accessor, which keeps the first a gate and correctly drops the second. jwks is now classified documented-open on its own docstring. THE COUNT WAS WRONG TOO: 98 routers, not 94. registered_routers matched `alias.name == "router"` literally, so four mounted routers exported under other names — api.jwks.auth_router, api.voice.realtime_router, api.voice_bundle_admin.bundle_admin_router and bundle_me_router — were dropped from EVERY bucket. Not gated, not ungated, not documented, not unreadable: absent. An enumerator that silently omits its subject is this module's own defect, in the module. All four are gated on inspection, so no hole was hidden — but a future router following that export pattern would have been permanently invisible. 98 routers · 85 gated · 10 ungated · 2 documented-open · 1 unreadable floor 80 -> 90, KNOWN_UNGATED 12 -> 10 TWO STRUCTURAL LIMITS ARE NOW PINNED AS TESTS RATHER THAN DESCRIBED: `intentional` is file-scoped while gating is route-scoped. api/system.py carries both at once — one documented public health check, the rest gated. If its gates regressed it would flip to UNGATED-BY-DESIGN rather than UNGATED and pass silently. Recorded against that real file. GATED means "a gate runs", not "the caller was identified". voice_stream checks WS ORIGIN, websockets authenticates the USER, and both report GATED because the model cannot tell them apart — which is precisely what #15745 AC3 is about. Asserted so that teaching the model the difference fails this test and forces the claim to be updated with it.
1 participant
Thinking Path
You cannot fix a set you cannot enumerate. Two sweeps of this reported 30
ungated routers, then 15, then fewer — each internally consistent, each wrong,
always in the same direction. So this PR is the enumerator only. The fixes follow
it, and #15745's AC5 already asks for the guard first.
The cause was four mechanisms, not carelessness
This codebase gates four ways, and a detector modelling three reports the fourth
as a hole:
APIRouter(dependencies=[...])api/agent_org.py:53— gates 12 routes at once, with noDependsanywhereDepends(X)dependencies=[...]api/websockets.pyawaitsenforce_ws_origin(websocket)The vocabulary is derived, never typed
A hand-written regex omitted
check_admin_permission— 140 imports, themost-used auth dependency in the backend — and that single omission produced
fifteen false findings. Names now come from what the auth modules export.
Derivation gets the names; purpose still has to be decided.
NOT_A_GATErecords twenty exports that live in auth modules and do not gate a request —
validate_pathprotects a filesystem argument, not an identity. Six of them weremeasured contributing to
GATEDverdicts, and none was the sole evidence forany router, so no verdict was wrong. They are excluded anyway: a name that can
contribute to
GATEDwithout gating anything is a false-GATEDwaiting for thefile where it appears alone — and false
GATEDhides a hole, where falseUNGATEDmerely wastes an investigation.What Changed
repo_tests/router_auth_enumerator.pyandrouter_auth_coverage_test.py.Measured on base: 94 registered routers — 80 gated, 12 ungated-and-undocumented,
1 documented-open, 1 unreadable.
Verification
Controls are one per mechanism, not one per direction. The sweep that reported
15 ran a gated and an ungated control and both passed while it was wrong, because
both were shapes its regex already matched — a control drawn from the detector's
own model tests the model against itself.
api.agent_orgapi.systemcheck_admin_permissionapi.websocketsapi.redisapi.chat_embedThe last one failed on the first run and the fix is instructive: the marker
pattern matched
#comments only, andchat_embeddocuments its posture in themodule docstring. A detector modelling one shape of a thing that has several —
the same defect this PR exists to catch.
Openness must be documented in the file. No name allowlist: allowlisting here
would make this module the record, and the record would drift from the code
(#15737).
api/frontend_config.pydocuments nothing and is therefore a finding,not an exemption.
It found seven the hand-inspection had not reached
The five confirmed on #15745 reproduce exactly. Seven more, of which two are
independent corroboration of open issues:
api.transcriber— the subject of security(auth): transcriber routes authenticate no one -- every caller resolves to DEFAULT_USER #15758api.user_management— the subject of security(auth): create/update/delete_user carry no admin or ownership gate #15738api.user_provider_credentials— handles provider credentials; wants looking at firstapi.frontend_config,api.knowledge_search,api.knowledge_search_aggregator,api.service_messagesAll twelve are recorded in
KNOWN_UNGATEDso the guard turns on now ratherthan waiting on the fixes. The list may only shrink, and a test fails if it goes
stale.
api.redis_mcpis registered and its module does not exist on disk. It getsits own test rather than sitting in an unreadable bucket that looks examined.
The middleware gap is declared, not implied
initialization/middleware.pyassumesrequest.state.useris already populatedupstream, and #15758 found there is no upstream for at least some routes. So
middleware may cover nothing, something, or everything.
UNGATEDhere means "not gated by mechanisms 1–3", which is weaker than"reachable unauthenticated" — every finding needs that last step confirmed by
hand. A test fails if anyone models middleware without re-verifying every control,
because the meaning of
UNGATEDwould change underneath them.Model Used
Claude Opus 5 (1M context)
Single-issue rationale: this is the enumerator that #15758 and #15738 both depend
on for their scope, so it wants to land ahead of either rather than inside one of
them.
Credit: the design, the four mechanisms, the derived vocabulary and the control
set are autobot-ai-b9's, handed over in full when worktree capacity blocked
them from building it.
Closes #15745