Skip to content

fix(security): close guest-surface findings on agent run entry paths - #411

Closed
esafwan wants to merge 5 commits into
developfrom
task/SEC-gap1-fix
Closed

fix(security): close guest-surface findings on agent run entry paths#411
esafwan wants to merge 5 commits into
developfrom
task/SEC-gap1-fix

Conversation

@esafwan

@esafwan esafwan commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Problem

Follow-up to the security gaps salvaged from closed #118 (full audit on file, every claim file:line-verified against develop @ 95daa90):

  • 1a (high): cross-user conversation hijack. run_agent_sync passed caller-supplied conversation_id straight to ConversationManager.get_or_create_conversation, which loaded ANY Agent Conversation by name with no agent/owner/session check — any authenticated user (or Guest on a guest agent) could read and append to another user's conversation.
  • 1b: agent-name enumeration oracle for Guests — doc load preceded the guest check (DoesNotExistError vs PermissionError).
  • 1c: caller-controlled provider/model override — Guests (on guest agents) could redirect execution to any configured provider/model against site API keys.
  • 1d: capability RBAC never consulted on the run_agent_sync/run_agent_stream/chat API entry paths.

Scope (3 files)

  • agent_integration.py (run_agent_sync, run_agent_stream), chat_api.py: resolve allow_guest via frappe.db.get_value BEFORE any Agent doc load; one generic 'Agent not found or access denied' for missing and not-permitted; provider/model override ignored for Guest; has_capability(user, 'agent.use') enforced for non-Guest callers.
  • conversation_manager.py: explicit conversation_id must match agent AND (owner == session user OR session_id matches OR chat.view_all capability) — and a missing id and an inaccessible id now fail identically (no existence oracle).

Key decisions

  • Ownership check lives in get_or_create_conversation so every caller (sync, stream, chat, hooks, flows) is protected at one point.
  • Behavior change (intended): doc-event hooks and flows run by users WITHOUT agent.use will now be denied — flagged for release notes. Scheduler/Administrator paths unaffected (Administrator has all capabilities).

Agent contributions

Audit + fix by the release-orchestration pipeline; an independent second review REJECTED the first cut (found a remaining existence oracle for missing explicit IDs); oracle closed by the pipeline; second commit.

Tests executed

  • py_compile on all 3 files; rg-verified guest check precedes every doc load on the 3 entry paths.
  • Isolated bench 16_ro: live check — missing explicit conversation_id raises the generic PermissionError (oracle closed); no-id create path unaffected.
  • Baseline suite on this branch crashes identically to develop (pre-existing runner bug, fixed in fix(tests): migrate off deprecated FrappeTestCase so the suite runs on Frappe 16 #407).

Risks and limitations

  • Users without agent.use lose doc-event/flow agent execution (intended; needs release note).
  • Guest sessions share owner 'Guest'; ownership for guests is only as strong as channel session_id scoping.
  • Gap 2 (MCP tool validation) and Gap 3 (execution identity) are NOT covered here — separate items.

Rollback

Revert the 2 commits; entry paths return to entry-point-only enforcement.

Remaining work

esafwan added 2 commits July 20, 2026 08:14
@esafwan
fix(security): close Gap-1 findings on agent run entry paths
a57d4b5 
- run_agent_sync / run_agent_stream / run_agent_sync_chat: resolve
  allow_guest via frappe.db.get_value before loading the Agent doc and
  return one generic 'Agent not found or access denied' PermissionError
  for both missing and guest-denied agents (closes the enumeration oracle)
- ConversationManager.get_or_create_conversation: a conversation loaded
  by id must belong to the requested agent and be owned by the session
  user, match the session_id, or the caller must hold chat.view_all
  (closes cross-user conversation hijack for every caller)
- ignore caller-supplied provider/model for Guest sessions
- enforce has_capability(user, 'agent.use') for non-guest callers on all
  three entry paths
@esafwan
fix(security): fail generic for missing explicit conversation_id (exi…
453dc15 
…stence oracle)

Cross-family review (codex) found that a supplied-but-nonexistent
conversation_id fell through to the latest-or-create path, distinguishing
'missing' from 'exists but forbidden'. Both now fail with the same generic
PermissionError.
@esafwan esafwan changed the title security: close guest-surface findings on agent run entry paths (conversation hijack, enumeration, provider override, capability bypass) fix(security): close guest-surface findings on agent run entry paths Aug 1, 2026
@Sanjusha-tridz
Sanjusha-tridz marked this pull request as ready for review August 12, 2026 07:44
esafwan added a commit that referenced this pull request Aug 12, 2026
@esafwan
fix: port PR #411's guest-oracle, capability, and conversation-hijack…
2f94a15 
… fixes onto agent_access.py

PR #597's assert_agent_access/check_agent_access unify allow_guest/allowed_users/
allowed_roles enforcement, but branched before #411 landed three narrower fixes
that #597 doesn't otherwise cover:

- Agent-name enumeration oracle: run_agent_sync/run_agent_stream/run_agent_sync_chat
  loaded the Agent doc before checking allow_guest, so a Guest caller could tell
  "no such agent" (DoesNotExistError) from "exists but guests aren't allowed"
  (PermissionError) apart by exception type. Now both collapse to one generic
  PermissionError for Guest callers. Implemented inline per call site (not via a
  new agent_access.py helper) because agent_access.py has its own `frappe` import
  distinct from each caller module's — a shared helper doing frappe.db.get_value
  there would bypass the @patch("huf.ai.agent_integration.frappe")-style mocks
  these call sites' existing tests rely on (the same class of bug #597's own
  7646fe7 fixed for assert_agent_access).
- agent.use capability (RBAC) was never consulted on these three entry points;
  now enforced for all non-Guest callers.
- Guest callers could pass provider/model overrides that were resolved (and thus
  billed against site API keys) before the guest check ran; now nulled for Guest
  before resolution.
- ConversationManager.get_or_create_conversation trusted a caller-supplied
  conversation_id with no ownership check, letting any authenticated user (or
  Guest on a guest agent) read/append to another user's conversation. Now
  requires conversation.agent match plus owner/session_id/chat.view_all, and a
  missing id and an inaccessible id fail identically (no existence oracle).

Adds ai/tests/test_conversation_manager_access.py (pure-mock unit tests, no live
bench required) covering the new ownership branch, mirroring test_agent_access.py's
style since no test previously existed for this method.

Supersedes #411 (SEC-gap1-fix), which will be closed in favor of this branch.
@esafwan

esafwan commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favor of #597, which supersedes this PR.

#597 replaces the duplicated _is_user_allowed()/Agent.has_permission() logic this PR patched with a single shared helper (huf/ai/agent_access.py) and covers a much wider surface (gateway/webhook bypasses, agent_chat.py endpoints, conversation_fork.py, frontend permissions UI), but its branch predates this PR's merge and didn't carry these three fixes forward. They've now been ported onto fix/agent-permissions-hardening in 2f94a15:

  • The Guest agent-name enumeration oracle (doc load before the allow_guest check) on run_agent_sync, run_agent_stream, and run_agent_sync_chat.
  • The agent.use capability (RBAC) check on those same three entry points.
  • Guest callers no longer get their provider/model override resolved (and billed) before the guest check runs.
  • The cross-user conversation hijack in ConversationManager.get_or_create_conversation (caller-supplied conversation_id now requires an ownership/session/chat.view_all check), plus a new unit test file for that method.

Rollback/history for this PR's original approach is preserved on task/SEC-gap1-fix if needed for reference.

@esafwan esafwan closed this Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@esafwan @Sanjusha-tridz