refactor(auth): INV-8 imperative auth-guard family — consolidate inline auth wiring (#1310)#1683
Merged
Conversation
…ariant #8 (#1310) Requirements-driven first step (Rule #1) for the INV-8 auth-dependency consolidation. Documents the five leaf helpers (assert_admin, assert_agent_access, assert_agent_owner, assert_owns_or_admin, assert_owns), the preserve-403 rationale (access-first = self-uniform per #186; #1445 precedent), the imperative-vs-path-dependency rule, the assert_agent_owner != delete-authorization footgun, and the permanent exceptions (nevermined/reports intentional-404, sessions compound-404, slack.py deferred). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…or (#1310) Stage 3 — behavioral lock BEFORE any migration (must stay green after). Real-DB db_harness drives the real db.can_user_* predicates (never a mocked auth check — a wholesale-Mock db false-greens a broken gate); only non-auth resource lookups are stubbed. Covers every in-scope site: admin gates, access gates, owner gates, Shape-F owns-or-admin (admin admitted + anti-IDOR binding), public.py strict-self (the admin-non-owner-403 access-widening guard), the agent_files anti-exfil sibling, the schedules #1445 access-before-404 ordering, and the loops composite gate. Asserts exact status + detail + admitted-principal set per site. Also hardens test_186 tier4 to compare the bound access dependency by name, not object identity: the unit conftest pops `dependencies` between tests while already-imported router modules persist, so a co-resident test importing agent_config in an earlier generation left the annotation bound to a stale (no-longer-`is`-equal) helper object. Name comparison proves the same invariant without the re-import fragility. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ent (#1310) Stage 2 — the five leaf helpers in dependencies.py (assert_admin, assert_agent_access, assert_agent_owner, assert_owns_or_admin, assert_owns), each raising 403 and access-first (self-uniform); the agent-name helpers run _enforce_connector_scope first to match the path-dependencies' second fence. No new import edge (dependencies.py already imports db) and no new DB accessor. Also relocate _narrow_to_agent from routers/executions.py to services/agent_service/helpers.py as narrow_to_agent (removing a router->router import edge; reports.py + executions.py repointed). Semantics unchanged. Part B helper-level truth tables added to the characterization suite: connector fence, owner-or-admin, strict-self (no-admin bypass) + owner_id int/int type parity. 39/39 green. Sites are migrated onto these helpers in the next commits. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ire_admin (#1310) Bucket 4a (admin). Replaces inline `role != "admin"` raises with assert_admin (avatar generate-defaults, nevermined settlement-failures/retry, schedules scheduler-status) and deletes the four router-local `require_admin` imperative dupes (subscriptions/system_agent/ops/settings — ~67 call sites swapped to assert_admin). logs.py's local `require_admin` Depends is replaced by the shared dependencies.require_admin. Exact 403 "Admin access required" preserved (custom avatar detail preserved); assert_admin/require_admin additionally reject connector principals — a no-op tightening (connectors are fenced upstream). _check_sdk() ordering before the nevermined retry admin gate preserved. Characterization suite (39) stays green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nt_* (#1310) Bucket 4b (access + owner). Replaces inline db.can_user_access_agent →403 with assert_agent_access (agent_config resources/capacity, avatar identity, public_memory, schedules create, notifications ×3, subscriptions auth-status, loops composite fallback) and inline db.can_user_share_agent →403 with assert_agent_owner (agent_config ×6, agent_files ×3, subscriptions ×2, event_subscriptions ×2, messages). Exact 403 + per-site detail preserved. Load-bearing orderings/siblings preserved verbatim: * schedules.create_schedule — access-403 stays BEFORE the is_agent_live 404 and the #929 timeout read (#1445 anti-enumeration order); * public_memory — the execution-404 / execution.agent_name-403 binding stays; * agent_files.share — the anti-exfil agent-scoped-key sibling check kept; * loops._check_loop_access — admin + initiator allow-returns kept, only the can_user_access fallback migrated. Characterization suite (39) stays green; only slack.py (deferred) + the 4 Shape-F voice/chat sites (next bucket) remain flagged by the guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bucket 4c. Migrates the strict-self-or-admin session gates to assert_owns_or_admin (voice_stop / get_voice_panel / chat get-session-detail / close-session) and the public-link session gate to assert_owns (id-only, NO admin bypass — the access-widening guard: an admin who is not the session owner stays 403; mapping it to assert_owns_or_admin would flip that to 200). Anti-IDOR bindings preserved verbatim (the `session.agent_name != name` / `preview.agent_name != name` 403 on the line above each owner gate — the cross-agent-session-read defense, #600). The WS-dict close(4003) handler (voice_ws) and the admin FILTER branches (chat history/session listing) are correctly left inline — they never raise HTTPException. Characterization suite (39) stays green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… markers (#1310) The durable risk reduction: test_1310_auth_wiring.py forbids, in routers/*.py, a db.can_user_*_agent() call whose negation guards a `raise HTTPException`, and an inline `role != "admin"` deny-If — the shapes the shared dependencies helpers now own. Precise AST matcher (self-tested against planted violations AND the benign shapes: assignments, role=="admin" allow/filter branches, WS close(4003) dict handlers). Per-(file, function) allowlist for the intentional-404 designs (reports.get_report, nevermined._require_*_access) and a `# noqa: inv8` line-exemption for the deferred slack.py sites — so NEW inline auth added to slack.py still trips the guard. slack.py's 10 inline-auth sites carry the `# noqa: inv8` marker (deferred follow-up, coordinate with the channel-adapter owner). Guard is green over the live tree. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…#1310) The INV-8 consolidation moved the inline checks behind dependencies.py helpers, which (a) now read current_user.connector_agent via the connector fence and (b) resolve db.can_user_* through the dependencies module. Existing tests that predated these need small, behavior-neutral fixups: * test_929 / test_1018 / test_117 / test_subscription_reassign — complete the User fakes with connector_agent=None (a real User always carries it — Optional[str]=None; the fakes just predate the field, same convention as the existing agent_name=None). test_subscription_reassign additionally points assert_agent_owner's own `db` global at its fake_db (the owner gate reads dependencies.db, not routers.subscriptions.db). * test_subscription_bola (#182) — asserts the new gate (assert_agent_owner on assign/clear, assert_agent_access on read-only auth-status) instead of the now-moved raw db-call; the behavioral shared-reader-denied case is covered by test_1310_auth_consolidation. No production behavior changed; these lock the same #182/#929/#1445 invariants against the consolidated shape. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…1310) The consolidated settings admin handlers now flow through assert_admin (which reads current_user.connector_agent). Complete the settings-test User fakes with connector_agent=None (test_85_brain_orb_settings, test_retention_floor, test_1638) — same field a real User always carries. test_telegram_webhook_backfill stubs `dependencies` in sys.modules; add the five new helper names so a router that imports them against the stub resolves. No production behavior changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…1310) Fix role-model.md's stale dependencies.py line numbers (ROLE_HIERARCHY 174→503, require_role 177→506, require_admin 158→486, hierarchy check 190→522), add the _reject_connector_principal line to the require_role snippet, and add §1b + a symbol-table row documenting the five imperative auth-guard helpers. agent- permissions.md gains a pointer distinguishing the consolidated router gates from the untouched service-layer inline checks. Feature-flows index gets its Recent-Updates row. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PASS — behavior-preserving refactor; no new bypass, no widening, no enumeration oracle. Documents the two-axis analysis (status + admitted-principal set), the public.py strict-self widening guard (assert_owns, no admin bypass), the connector-fence parity tightening, the intentional-404 exceptions, and the static AST drift guard as the durable control. Full unit tier green modulo one pre-existing failure (test_1474, untouched db/schedules.py summary path). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
….modules leak (#1310) The consolidation suite drives router handler bodies that lazy-import start_agent_internal from services.agent_service (subscriptions admit path). A sibling unit file (test_inject_assigned_credentials) installs a fake services.agent_service package at its own import that persists for the session, so under an adverse collection/execution order the real module was shadowed and the owner-admit path tripped a bare ImportError escaping _raised. Capture the genuine services / services.agent_service / services.docker_service at collection time (evicting any cached fake subtree first) and re-pin all three per test via an autouse monkeypatch.setitem fixture (auto-restores the sibling's stub afterward, so this file stays a well-behaved sys.modules citizen). Verified: 88 passed with test_inject_assigned_credentials collected first. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ge (#1310) The INV-8 imperative auth-guard family spans dependencies.py 740-801 (assert_admin@740 … assert_owns body end@801), not the stale 744-786. ROLE_HIERARCHY@503 / require_role@506 / require_admin@486 already correct. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
AndriiPasternak31
marked this pull request as ready for review
July 17, 2026 17:22
|
Resolve by running |
…e-1310 # Conflicts: # docs/memory/feature-flows.md
Post-merge integration fixes for the origin/dev merge: - test_1310_auth_consolidation.py: adopt the linter-recognized `_STUBBED_MODULE_NAMES` + `_restore_sys_modules` escape hatch for the import-time module-eviction (a module-level `sys.modules.pop` no monkeypatch fixture can reach), so `tests/lint_sys_modules.py` passes without a new baseline entry. Behavior unchanged (same three real service modules re-pinned per test). - test_1310_auth_wiring.py: allowlist `chat.execute_parallel_task` — dev's #1083 resume-session check is a compound `role != "admin" AND NOT db.resume_session_belongs_to_user(...)` raising an intentional 404 (session-id enumeration safety, Invariant #8). No shared helper fits (assert_owns_or_admin takes an owner_id and raises 403), so it stays inline like reports.get_report / nevermined._require_*. - test_1578_task_completion_events.py: the update-route owner check moved into dependencies.assert_agent_owner (#1310), so the #1578 reserved-namespace guard test now no-ops the owner gate via the router's own imported binding (module-identity-robust) instead of patching the router db alone. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…wns_or_admin onto the #1483 chat split; keep both feature-flows rows)
vybe
approved these changes
Jul 19, 2026
vybe
left a comment
vybe
left a comment
Contributor
There was a problem hiding this comment.
Validated via /validate-pr: behavior-preserving auth consolidation with CSO diff PASS (0 findings), static AST guard against inline-auth drift, hard-gates (anti-widening assert_owns on public.py, anti-exfil sibling check, anti-IDOR Shape-F binding) verified. Rebased onto the #1483 chat split — assert_owns_or_admin re-applied to the 2 kept session endpoints; local runs green: auth_wiring 12/12, consolidation+186 68/68, test_1578 37/37. slack.py deferral is noqa-fenced with follow-up. Approving.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1310 (INV-8, split from #654's architecture-invariant drift report; carved off from INV-14/#1308 because it is security-sensitive).
Behavior-PRESERVING consolidation of duplicated inline auth wiring in ~20 FastAPI routers behind a small imperative-guard family in
dependencies.py. Every consolidated site enforces the same verdict (status + detail + admitted-principal set) it did before — proven behaviorally, not by an OpenAPI snapshot.The family (all raise 403, access-first → self-uniform per #186)
assert_admin_reject_connector_principal+role != "admin"require_admindupesassert_agent_access_enforce_connector_scope+can_user_access_agentcan_user_access_agent→ 403assert_agent_owner_enforce_connector_scope(owner_op=True)+can_user_share_agent(owner-or-admin; NOT delete-auth — nois_systemguard)can_user_share_agent→ 403assert_owns_or_adminid != owner AND role != "admin"assert_ownsid != owner(no admin bypass)Each agent-name helper runs
_enforce_connector_scopefirst, matching the path-dependencies' second fence (kills the two-convention divergence)._narrow_to_agentrelocatedrouters/executions.py→services/agent_service/helpers.py(removes a router→router import edge).Preserve-403, not 404: access-first inline handlers are already self-uniform (they check access before any existence lookup → no 404-then-403 enumeration oracle). Precedent:
schedules.pycreate_schedule(#1445).Security hard-gates (the merge-blockers the review flagged — each verified in this branch)
public.py:1134is strict-self, no admin bypass → usesassert_owns(NOTassert_owns_or_admin, which would let an admin read another user's public-link session). ✅ verified:470+) survives the:468migration toassert_agent_owner. ✅ verifiedsession.agent_name != name → 403binding stays on the line above each owner line (voice.py:109/160,chat.py:2358/2389); only the owner line migrated. ✅ verifiedThe durable risk reduction: static AST guard
The extraction is behavior-preserving, so posture is observably identical to today. The lasting protection is
tests/unit/test_1310_auth_wiring.py— a precise AST matcher that forbids, inrouters/*.py, adb.can_user_*_agent()call whose negation guards araise HTTPException, and an inlinerole != "admin"deny-If. Self-tested against planted violations AND the benign shapes (assignments,role == "admin"allow/filter branches, WSclose(4003)dict handlers), with a per-(file, function)allowlist and a# noqa: inv8line-exemption.Honest residual accounting
Threshold was "≤5 in-scope non-exception." True residual inline-auth = 15, bucketed:
slack.py(DEFERRED, see below)internal.py— the INV-8 no-auth exception (agent-to-backend calls)nevermined._require_read_access/_require_write_access+reports.get_report(own consolidated SEC: User and Agent Enumeration via differential API responses #186 designs;sessions._session_or_404is a compound uniform-404 kept as-is — migrating it would regress 404→403 and leak session-id existence)In-scope non-exception residual → ≤5. ✅
The 10
slack.pyinline-auth sites overlap the channel-adapter area (Pavlo's). They are intentionally deferred to a follow-up PR after sign-off. Each deferred line carries a# noqa: inv8marker, so the static guard still trips on new inline auth added to that file. Follow-up: coordinate the slack.py consolidation with the channel-adapter owner (Pavlo) before/after this lands.CurrentUseralias sweep NOT includedThe finalized plan (decision #3) had the mechanical
Depends(get_current_user)→CurrentUsersweep (51 files / 276 sites) as an isolated final commit in this PR. It is not in this branch — only theCurrentUseralias is defined (dependencies.py:813); 275Depends(get_current_user)sites remain untouched. Shipping the security consolidation without the cosmetic sweep keeps this PR reviewable for the auth reviewers (which is what the reviewers originally recommended before the override). @AndriiPasternak31 — confirm whether the sweep should land here as a follow-up commit or a separate PR before this is considered scope-complete.Docs
docs/memory/requirements/auth.md§2.7 (new)docs/memory/architecture.mdInvariant security: implement safe tar extraction with symlink/hardlink validation #8 — the 5 helpers + imperative-vs-path rule +assert_agent_owner≠ delete-auth note (verified against code)docs/memory/feature-flows/role-model.md— §1b + stale line-numbers corrected (ROLE_HIERARCHY@503,require_role@506,require_admin@486, family@740-801)docs/memory/feature-flows/agent-permissions.md— imperative-family pointer + service-layer scope boundarydocs/security-reports/cso-diff-2026-07-17-inv8-auth.md—/cso --diffverdict PASS (no new bypass/widening/oracle; 0 findings)Testing
test_1310_auth_consolidation.py(real-DBdb_harness, 39) — locks status + detail + admitted-principal per migrated site incl. the 3 hard-gate RED→GREEN casestest_1310_auth_wiring.py(static guard, 12) — self-tests + planted violationtest_186_enumeration_uniformity.py— extended for the 5 helperssys.modulescontamination check green (88 with the stub-installing sibling collected first)test_role_model/test_access_controlare server-gated integration tests (covered by CI/verify-localagainst a live stack)No new top-level backend module (no Dockerfile COPY change), no new
os.getenv(no compose/.env.examplechange), no schema/migration change.Reviewers / process
🤖 Generated with Claude Code