fix(agent-terminal): refuse binding a conversation the creator does not own (#17422)

[mrveiss]

Thinking Path

POST /api/agent-terminal/sessions takes conversation_id from the request body. The route passed the JWT username as an explicit owner, and SessionManager.create_session only resolved the conversation's owner when owner is None (#17053). The REST path never takes that branch. So nothing compared the conversation to the caller. The session record then named the caller as owner, which means every #17053 check passed. An authenticated user could bind another tenant's conversation, and _restore_pending_approval would load that conversation's pending approval into their session (CWE-639).

The comment above the call ("owner and tenant_id both come from the JWT only") was accurate but beside the point: the untrusted input was conversation_id.

The fix is in the service layer, so no caller can skip it. create_session now resolves the conversation's owner on every call. An explicit owner must match it, and it narrows authority rather than replacing the check. The route maps the refusal to the same 404 a missing session gets (THREAT_MODEL.md §2: a response never confirms who owns what).

Decisions made on purpose, where the brief left them open:

• Unowned or unknown conversation + explicit owner → refused. Agent-terminal session routes have no ownership check: any signed-in user can list, read, execute in or delete another user's session #17053 made unowned conversations admin-only, and binding one would stamp the caller as its owner. A failed owner read also resolves to None in conversation_owner(), so this path fails closed.
• No admin bypass. SessionManager has no role, and AgentTerminalService is at its file-size ceiling (951 lines), so it couldn't take a new parameter without a split. An admin who creates a session on someone else's conversation would be stamped as that session's owner, which misattributes it anyway. If an admin flow needs this, it is a follow-up. I found no such flow: the only frontend caller (useTerminalStore.createAgentTerminalSession) binds the user's current chat conversation.
• The "implicit owner" case. Internal callers (the chat workflow, terminal_tool) pass no owner, so the session is stamped with the conversation's own owner. There is no caller identity that could differ from it, so there is nothing to refuse. The check is not on the owner is None branch. The test pins the implicit path to the conversation's owner.

Discoverability of another tenant's conversation_id (severity): partly established. Server-minted IDs are UUID4 (utils/chat_utils.generate_chat_session_id), and ChatHistoryManager.create_session falls back to chat-{ms}-{8 hex}. Neither can be guessed. Client-minted IDs pass a format check only (validate_chat_session_id). I did not measure whether conversation IDs leak through other channels (logs, shared sessions, org indexes, WebSocket channel names), so moderate vs. severe is still undetermined.

What Changed

• services/agent_terminal/session_manager.py: new _owner_for(conversation_id, owner). It always resolves the recorded owner, returns it when owner is None, and otherwise raises ConversationNotOwnedError unless the recorded owner equals owner. create_session calls it in place of the owner is None branch, before PTY setup, session storage and _restore_pending_approval.
• services/agent_terminal/conversation_owner.py: ConversationNotOwnedError(PermissionError). The same error covers someone else's conversation and an unowned one.
• api/agent_terminal_access.py: create_session_for_caller(service, user, **fields) stamps owner and tenant from the JWT and maps the refusal to 404 ERR_SESSION_NOT_FOUND.
• api/agent_terminal.py: the create route calls the helper. No net line change, because the file is at its KNOWN_LARGE ceiling (773).
• tests/security/test_agent_terminal_conversation_binding_17422.py (new, 8 tests): the real route runs over a real SessionManager. Only PTY setup and _restore_pending_approval are mocked.
• session_manager_conversation_owner_17053_test.py: test_an_explicit_creator_is_never_replaced asserted the defect (carol bound alice's conversation). It now covers an explicit creator with no conversation.
• changelog/unreleased/17422-agent-terminal-conversation-binding.md.

Verification

Mapped to the acceptance criteria:

• Exact status code: test_binding_a_conversation_you_do_not_own_is_404_and_nothing_is_created asserts == 404 and detail == ERR_SESSION_NOT_FOUND for someone else's, unowned, unknown, and admin-on-someone-else's conversations.
• Refusal regardless of how the owner arrives: the check is in _owner_for, off the owner is None branch. test_an_explicit_owner_is_checked_against_the_conversation covers the service directly, and test_an_implicit_owner_is_the_conversations_own_so_it_cannot_name_someone_else pins the implicit path.
• _restore_pending_approval not reached: assert_not_awaited() in the refusal tests, and assert_awaited_once_with(session, "conv-alice") for the owner.

Targeted run (new tests plus every test file that mentions the agent terminal, 24 files):

$ python -m pytest -q tests/security/test_agent_terminal_conversation_binding_17422.py $(grep -rl "agent_terminal\|AgentTerminal" --include=*test*.py .)
=============== 333 passed, 2110 skipped, 78 warnings in 32.68s ================

The skips come from the local environment (optional deps absent; the conftest reports it). tests/security/test_agent_terminal_ownership_17052.py runs in full: 92 passed.

Mutation: I removed the two lines if conversation_id and recorded != owner: raise ConversationNotOwnedError(conversation_id) and ran the new file:

E   AssertionError: mallory bound conv-alice, which they do not own: {"status":"created",...,"conversation_id":"conv-alice",...}
E   AssertionError: mallory bound conv-unowned, which they do not own: {"status":"created",...,"conversation_id":"conv-unowned",...}
E   AssertionError: mallory bound conv-never-existed, which they do not own: {"status":"created",...}
E   AssertionError: root bound conv-alice, which they do not own: {"status":"created",...}
E   Failed: DID NOT RAISE ConversationNotOwnedError
FAILED ...::test_binding_a_conversation_you_do_not_own_is_404_and_nothing_is_created[someone-elses]
FAILED ...::test_binding_a_conversation_you_do_not_own_is_404_and_nothing_is_created[unowned]
FAILED ...::test_binding_a_conversation_you_do_not_own_is_404_and_nothing_is_created[unknown]
FAILED ...::test_binding_a_conversation_you_do_not_own_is_404_and_nothing_is_created[admin-on-someone-elses]
FAILED ...::test_an_explicit_owner_is_checked_against_the_conversation
==================== 5 failed, 3 passed, 1 warning in 0.82s ====================

After restoring the check: 8 passed. pre-commit run --files <changed> passed with exit 0, and check_python_file_size.py passed on every changed file.

Risks

• Admins can no longer create an agent-terminal session bound to another user's conversation or to an unowned one. This is deliberate (see Thinking Path), and I found no caller that does it.
• Legacy conversations with no recorded owner are refused over REST until the chat ownership gate claims them. That happens on their first message (legacy_migration), and Agent-terminal session routes have no ownership check: any signed-in user can list, read, execute in or delete another user's session #17053 made the same conversations admin-only.
• Out of scope, not touched: refactor(events): retire the duplicate event socket — /api/ws vs /api/ws/live #14822 (legacy /api/ws channel scoping) and security(events): captcha timeout/resolved publish the target URL to the 'global' channel, and the #17354 guard cannot see it #17414 (global publishes in the captcha path).

Rollback: revert this single commit.

Model Used

Claude (Anthropic), via Claude Code

Issue Link

Closes #17422

Single-issue rationale: a security fix to the agent-terminal ownership path; the adjacent issues (#14822, #17414) are different code paths with different risk, and the brief scopes this to one defect.

Changelog fragment

• Added changelog/unreleased/17422-agent-terminal-conversation-binding.md

Checklist

• Code follows AutoBot patterns from CLAUDE.md
• Tests added or updated
• Documentation updated if behavior changed (docstrings; changelog fragment)
• Pre-commit hooks pass
• PR targets main (not release)
• No secrets or credentials in the diff

Summary by CodeRabbit

• Bug Fixes
  • Agent-terminal sessions can only be created for conversations owned by the caller. Attempts to use another person’s, unowned or unknown conversation return the same not-found response.
  • Sessions created without a conversation remain available. When no owner is specified, sessions linked to a conversation use its recorded owner.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

📝 Walkthrough

Walkthrough

Agent-terminal session creation now verifies that the caller owns the supplied conversation. The API returns the missing-session 404 response when ownership verification fails. Tests cover rejected and permitted requests, including unreadable and unowned conversations.

Changes

Conversation ownership enforcement

Layer / File(s)	Summary
Service-level conversation ownership autobot-backend/services/agent_terminal/conversation_owner.py, autobot-backend/services/agent_terminal/session_manager.py, autobot-backend/tests/security/test_agent_terminal_conversation_binding_17422.py, autobot-backend/services/agent_terminal/session_manager_conversation_owner_17053_test.py	The service resolves the recorded conversation owner when no owner is supplied. When an explicit owner and conversation ID are supplied, it rejects a mismatch or an owner that cannot be verified. Tests cover owner resolution, rejection, and Redis grant cases.
API ownership response autobot-backend/api/agent_terminal_access.py, autobot-backend/api/agent_terminal.py, autobot-backend/tests/security/test_agent_terminal_conversation_binding_17422.py, changelog/unreleased/17422-agent-terminal-conversation-binding.md	The API passes the authenticated caller to session creation and maps ownership failures to ERR_SESSION_NOT_FOUND with HTTP 404. Route tests cover rejected and permitted requests. The changelog records the 404 behaviour.

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: 🔵 Low · up to 85423

Conversation ownership checks appear intact. Tightening the refusal tests would improve regression detection, but the remaining gap does not prevent merging.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 85423

The change strengthens a cross-tenant ownership check before a terminal session is created. No new security weakness was established, though the behavior of existing sessions during an ownership transfer remains uncertain.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The boundary matters across users and potentially tenants: a caller supplies the conversation ID, while a successful binding creates a caller-owned terminal session linked to that conversation. The new comparison reduces that reachability for explicit-owner callers.

Trust Boundaries and Controls

• observed — The caller helper takes owner and tenant ID from authenticated user data; the service compares that owner with the owner resolved for the request's conversation ID. Refusal precedes PTY registration and approval restoration.

Resilience and Maintainability Implications

• observed — The owner-omitted path still derives an owner through the earlier resolver, which can return no owner after a failed durable read. This pre-existing path is distinct from the new fail-closed explicit-owner check; the inspected evidence does not show that the PR expanded its exposure.

Hardening Proposals

• proposed — If conversation ownership can be deliberately transferred, define whether existing and in-flight terminal sessions must be revoked or revalidated on transfer. Creation checks one owner snapshot; the examined evidence does not establish transfer-time session handling or a PR-introduced exploit.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The implementation meets the other coding requirements in #17422. SessionManager._owner_for checks an explicit owner against the verified conversation owner before PTY creation, and the route maps r…	Run mutation testing against the ownership check. Record evidence that the ownership-specific tests fail because of the ownership mismatch when the check is removed.
Docstring Coverage	⚠️ Warning	Docstring coverage is 64.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 6 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarises the main security fix: it prevents agent-terminal sessions from binding conversations that the creator does not own. It is specific and concise.
Out of Scope Changes check	✅ Passed	The changes remain within #17422. They enforce conversation ownership in the session service, map refusal to the existing 404 route response, add focused route and service tests, update the affected o…

Full details: Linked Issues check

Explanation

The implementation meets the other coding requirements in #17422. SessionManager._owner_for checks an explicit owner against the verified conversation owner before PTY creation, and the route maps refusal to exact 404 behaviour. The route tests cover another user's, unowned, unknown, and administrator claims. They also verify that session creation and _restore_pending_approval are not reached after refusal. The implicit-owner path remains covered. However, the reviewed changes and available reports contain no mutation-test result showing that removing the ownership check causes the ownership-specific tests to fail.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

[mrveiss]

autofix-types is red, and the cause is outside this PR.

The job fails while building the SLM backend, which this diff does not touch:

[dump-slm-openapi] FAILED to build app: The SQLAlchemy asyncio module requires that the Python 'greenlet' library is installed.

SQLAlchemy 2.1.0 was just released, and autobot-slm-backend/requirements.txt pins it only as sqlalchemy>=2.0.54, with no upper bound. In the 2.1.0 wheel, greenlet is now only pulled in by the asyncio extra (Requires-Dist: greenlet>=1; extra == "asyncio"). Evidence it is not this PR's:

• The same job passed on this PR's previous head (c305201, 19:55Z).
• It failed identically on fix(plugins): stop passing grant_capabilities= to loader that rejects it (#17420) #17427 (plugins, unrelated) at 23:54Z.

The same fresh install would also break the SLM backend at runtime, not only CI.

Proposed fix (one line, its own PR, not folded in here):

--- a/autobot-slm-backend/requirements.txt
-sqlalchemy>=2.0.54
+sqlalchemy[asyncio]>=2.0.54

No fix PR exists yet; I searched open PRs for greenlet. I'm not re-running the job, because it will fail the same way until the requirement changes. Every other check on this head is independent of this failure.

---

Generated by Claude Code

[mrveiss]

hardened-smoke-test is red for the same reason as autofix-types (see above). The autobot-slm container restart-loops, and the job fails with dependency failed to start: container autobot-slm is unhealthy. Its log:

autobot-slm  | ModuleNotFoundError: No module named 'greenlet'
autobot-slm  | ImportError: The SQLAlchemy asyncio module requires that the Python 'greenlet' library is installed. ... 'pip install sqlalchemy[asyncio]'

The backend container from this PR came up healthy (GET /api/system/health 200). The fix is the same one-line sqlalchemy[asyncio]>=2.0.54 change in autobot-slm-backend/requirements.txt, which does not exist yet. So this confirms that change fixes a real runtime break in the SLM image, not just a CI step.

---

Generated by Claude Code

[mrveiss]

This PR now carries the greenlet fix from #17433 (#17432), in d8bcd61. It adds only the requirement lines, with blobs byte-identical to #17433's (autobot-backend/requirements.txt 6a0587f, autobot-slm-backend/requirements.txt 2a5413e). So it no-ops once #17433 merges. The repo test that comes with them stays in #17433. This should clear autofix-types, smoke-test and hardened-smoke-test. On 54f0d02, every other check was already green, including all 12 python-suite shards.

---

Generated by Claude Code

[mrveiss]

I dropped d8bcd61 (the greenlet port). #17433 owns that fix, and this PR is back to the single agent-terminal commit 54f0d02, which has no trailers. I did not rebase onto main, because base is still red. Once #17433 merges, I'll rebase onto main and autofix-types, smoke-test and hardened-smoke-test should clear with it.

On the four metadata gates (Check PR links to its issue, Check same-scope batching, No open blocks-merge issues reference this PR, Validate PR template sections): on 54f0d02 each has two check runs. One is cancelled (runs 36075022576 / 36075022570 / 36075022664, superseded when the body edit re-triggered them). The other is success (runs 36075057238 / 36075057233 / 36075057290). So none of them failed. The body has all four required headings (Thinking Path, What Changed, Verification, Model Used), scripts/validate_pr_body.py --file reports all four OK, and the CI Validate PR template sections run passed.

On 54f0d02 the only failures were the three caused by the greenlet base break. Everything else passed, including all 12 python-suite shards and No commit trailers.

---

Generated by Claude Code

[mrveiss]

migration-matrix (run 36080287265) failed without running its checks. The log's only error is:

##[error]path detection cancelled — cannot tell whether migrations changed, so this gate cannot pass

Its path-detection job was cancelled, most likely because my force-pushes (d8bcd61, then back to 54f0d02) superseded the run. This PR changes no migrations, and the same gate passed on 54f0d02 in the earlier run (migration-matrix success, 00:00:58Z). I tried to re-run it and got 403 Resource not accessible by integration, so I have no way to re-run it from here. It will run again when I rebase onto main after #17433 merges. If you'd like it green sooner, a manual re-run of 36080287265 should do it.

---

Generated by Claude Code

[mrveiss]

verify-precommit-config (run 36120218259) on cc4403d failed during dependency install, before any check ran:

ERROR: Could not install packages due to an OSError: ("Connection broken: ConnectionResetError(104, 'Connection reset by peer')", ...)
##[error]Process completed with exit code 1.

This was a network reset on the runner, not a failure in this PR's change. The same check passed on 800010f at 06:17Z, and cc4403d only adds the main merge. I tried to re-run the failed job and got 403 Resource not accessible by integration, so I can't re-run it from here. A manual "Re-run failed jobs" on run 36120218259 should turn it green.

---

Generated by Claude Code

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: c1417088-e63c-4d03-8d82-1197b0e40e9a

📥 Commits

Reviewing files that changed from the base of the PR and between cc4403d and b383873.

📒 Files selected for processing (4)

• autobot-backend/api/agent_terminal.py
• autobot-backend/api/agent_terminal_access.py
• autobot-backend/services/agent_terminal/conversation_owner.py
• autobot-backend/services/agent_terminal/session_manager.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

[mrveiss]

CI on c3909d1 (the unreadable-owner fix) was cancelled, not failed. Between 10:49Z and 10:52Z nearly every job in the batch finished as cancelled: python-suite, smoke tests, code-quality, the template/trailer gates and the path detectors. The single red, migration-matrix, is the aggregate reporting path detection cancelled. The head did not move afterwards, so another push is not the cause. I have not identified what cancelled the batch. I did not verify the likely cause: CodeRabbit's PR-description edit at ~10:49Z re-triggering the same concurrency groups.

Nothing has run against this commit yet, so there is no verdict on it. Locally: pre-commit exit 0, and the agent-terminal suite passes 335. I get 403 Resource not accessible by integration when I try to re-run jobs, so this commit needs a manual re-run (re-running the cancelled workflows on c3909d1 is enough). One Detect changed paths job (run 36125994327) was queued at 10:55Z and may be the start of a fresh batch. I'll keep checking.

---

Generated by Claude Code

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: a0e04147-0a5d-4e50-9191-518cd1f69712

📥 Commits

Reviewing files that changed from the base of the PR and between b383873 and 85423a8.

📒 Files selected for processing (5)

• autobot-backend/api/agent_terminal.py
• autobot-backend/api/agent_terminal_access.py
• autobot-backend/services/agent_terminal/conversation_owner.py
• autobot-backend/services/agent_terminal/session_manager.py
• autobot-backend/tests/security/test_agent_terminal_conversation_binding_17422.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.