[Security] Missing ownership check on /conversations/{conversation_id}/code-changes/sync allows injecting code changes into another user's conversation

[geo-chen]

Component/Module

Conversations

Environment

Development (isDevelopmentMode=enabled)

Severity

High (Major feature broken)

Operating System

Environment Information

Description

Hi, is there a place I can report a potential security vulnerability please?

Expected Behavior

Actual Behavior

No response

Steps to Reproduce

No response

Relevant Logs/Error Messages

No response

Additional Context

No response

[linear-code]

POT-1551

[coderabbitai]

Coding Plan

Summary

• Extend the existing sync_code_change_from_local handler to use the established ownership verification pattern already implemented across all other conversation endpoints
• Leverage check_conversation_access() from the conversation service layer, which correctly handles owner, shared, and public access scenarios
• Require WRITE access since syncing code changes is a mutating operation that should only be permitted for conversation owners
• No new abstractions or architectural changes required—this fix aligns the endpoint with existing security patterns

Implementation Steps

Add Ownership Verification to Code Changes Sync Endpoint

This phase addresses the security vulnerability by integrating the existing access control pattern into the sync endpoint, ensuring only conversation owners can sync code changes.

Implement ownership check in sync endpoint

Add conversation access verification to sync_code_change_from_local before any code change operations are performed.

• Add async_db: AsyncDbSession as a dependency parameter to the handler (required for async access check)
• Instantiate ConversationController(db, async_db, user_id, user_email) following the pattern used by other endpoints in the same router
• Call the service layer's check_conversation_access() method to verify the authenticated user has WRITE access to the conversation
• Raise AccessTypeNotFoundError if access level is NOT_FOUND (maps to HTTP 401/404)
• Raise AccessTypeReadError if access level is READ (maps to HTTP 403, since this is a mutating operation)
• Only proceed with the existing CodeChangesManager logic if access level is WRITE
• The existing exception handlers in the controller/router will map these errors to appropriate HTTP responses

File to modify: legacy/app/modules/conversations/conversations_router.py (lines 654–747)

Reference pattern: See how delete_conversation, rename_conversation, or update_conversation_agent handlers in the same file implement the controller instantiation and access verification flow.

🤖 Prompt for AI agents

This phase fixes a security vulnerability in `sync_code_change_from_local` by
integrating the existing conversation ownership verification pattern before any
code change operations are processed.

In `legacy/app/modules/conversations/conversations_router.py` (lines 654–747),
modify the `sync_code_change_from_local` handler as follows:

- Add `async_db: AsyncDbSession` as a new dependency parameter to the handler
function (required for the async access check)
- Instantiate `ConversationController(db, async_db, user_id, user_email)` at the
start of the handler, following the same pattern used by `delete_conversation`,
`rename_conversation`, or `update_conversation_agent` in the same file
- Call the service layer's `check_conversation_access()` method to determine the
authenticated user's access level for the given conversation
- After the access check:
  - Raise `AccessTypeNotFoundError` if access level is `NOT_FOUND`
- Raise `AccessTypeReadError` if access level is `READ` (since this is a
mutating operation)
- Only proceed with the existing `CodeChangesManager` logic if access level is
`WRITE`
- Do not change any of the existing `CodeChangesManager` logic — only gate it
behind the access check
- The existing exception handlers in the controller/router will map these errors
to the appropriate HTTP responses automatically

Research

The codebase follows a consistent pattern for conversation ownership verification: route handlers extract user_id and user_email from the authenticated user, instantiate a ConversationController, and delegate to service methods that call check_conversation_access(). This method returns WRITE (owner), READ (shared/public), or NOT_FOUND (no access). The vulnerable sync_code_change_from_local endpoint in conversations_router.py bypasses this pattern entirely—it extracts user_id but never uses it, and the injected db session goes unused. The Conversation model has a user_id foreign key as its ownership field.

---

🚀 Next Steps

🤖 All AI agent prompts combined

This phase fixes a security vulnerability in `sync_code_change_from_local` by
integrating the existing conversation ownership verification pattern before any
code change operations are processed.

In `legacy/app/modules/conversations/conversations_router.py` (lines 654–747),
modify the `sync_code_change_from_local` handler as follows:

- Add `async_db: AsyncDbSession` as a new dependency parameter to the handler
function (required for the async access check)
- Instantiate `ConversationController(db, async_db, user_id, user_email)` at the
start of the handler, following the same pattern used by `delete_conversation`,
`rename_conversation`, or `update_conversation_agent` in the same file
- Call the service layer's `check_conversation_access()` method to determine the
authenticated user's access level for the given conversation
- After the access check:
  - Raise `AccessTypeNotFoundError` if access level is `NOT_FOUND`
- Raise `AccessTypeReadError` if access level is `READ` (since this is a
mutating operation)
- Only proceed with the existing `CodeChangesManager` logic if access level is
`WRITE`
- Do not change any of the existing `CodeChangesManager` logic — only gate it
behind the access check
- The existing exception handlers in the controller/router will map these errors
to the appropriate HTTP responses automatically

💡 Iterate on the plan with: `@coderabbitai` <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!

[geo-chen]

Oh wow. Impressive. Here goes the details.

Affected Versions: confirmed on commit 3cb6fca (legacy/ backend)
CVSS Vector: CVSS:3.1/AV:N/AC:H/PR:L/UI:N/S:U/C:L/I:H/A:N (Score 6.0)
CWE: CWE-862 - Missing Authorization (also CWE-639 IDOR)

Summary

The endpoint POST /conversations/{conversation_id}/code-changes/sync does not verify that the
authenticated user owns conversation_id. The code-change store it writes to is keyed only by
conversation_id, so an authenticated user can inject arbitrary file changes into another
user's conversation. Those changes are later read and acted on by the victim's agent for that
conversation.

Details

Every other conversation route builds an ownership-aware controller and (for the read/session
routes) gates on get_conversation_info first
(legacy/app/modules/conversations/conversations_router.py):

@router.get(".../active-session")
async def get_active_session(..., user=Depends(...)):
    user_id = user["user_id"]
    controller = ConversationController(db, async_db, user_id, user_email)
    await controller.get_conversation_info(conversation_id)   # ownership gate (raises 403 if not owner)
    ...

ConversationController passes self.user_id into the service, which enforces ownership
(conversation_controller.py methods raise 403 on mismatch). The sync endpoint skips all of
this:

@router.post("/conversations/{conversation_id}/code-changes/sync")
async def sync_code_change_from_local(conversation_id: str, request: ..., user=Depends(...)):
    from app.modules.intelligence.tools.code_changes_manager import (
        _get_code_changes_manager, _set_conversation_id)
    _set_conversation_id(conversation_id)          # attacker-supplied id, no ownership check
    manager = _get_code_changes_manager()
    ... manager.add_file(file_path, content) ...   # writes a change for that conversation

user is read from the token but never used to check that conversation_id belongs to the
caller. The manager persists to Redis under a key built only from the conversation id
(code_changes_manager/manager.py):

return f"{CODE_CHANGES_KEY_PREFIX}:{self._conversation_id}"   # "code_changes:<conversation_id>", no user_id

So a change written by user B under user A's conversation_id lands in the same store that
user A's agent reads for that conversation.

PoC

Validated end to end against the real backend (Postgres + Neo4j + Redis booted from
legacy/compose.yaml, dev mode). Two users were created, userA and userB, and a
conversation convA owned by userA. Authenticated as userB, using the verbatim handler
with the real get_db session and the real code_changes_manager/Redis:

DB owner of conversation convA: userA
userB -> POST /conversations/convA/code-changes/sync
   body {"file_path":"app/auth.py","change_type":"add","content":"BACKDOOR=True # injected by userB"}
-> 200 {"message":"Change synced successfully","conversation_id":"convA","file_path":"app/auth.py"}

The request from the non-owner succeeded (no 403). The real Redis store for the victim's
conversation, code_changes:convA, then contained the attacker's change:

code_changes:convA = {"conversation_id":"convA","changes":[
   {"file_path":"app/auth.py","change_type":"add","content":"BACKDOOR=True # injected by userB", ...}]}

The victim's agent reads code changes for convA from exactly this key, so it consumes the
attacker-injected change. (Only the authentication step was injected as userB in the harness;
the handler code, the database, the Redis store, and the code-changes manager are the real
ones. The vulnerability is the handler never comparing the authenticated user to the
conversation owner.)

On a live deployment the same request is:

curl -X POST "$TARGET/conversations/<victim_conversation_id>/code-changes/sync" \
  -H "Authorization: Bearer <user B token>" -H 'Content-Type: application/json' \
  -d '{"file_path":"app/auth.py","change_type":"add","content":"BACKDOOR=True"}'

Impact

An authenticated user who knows or obtains another user's conversation_id injects arbitrary
file content/code changes into that user's conversation. The victim's coding agent reads and
acts on these changes (it can apply them to the victim's repository workspace / propose them in
a PR), so the impact is integrity of the victim's code and agent actions. Exploitation requires
the victim's conversation_id (a UUID), which can leak through shared chats or other endpoints.

Suggested remediation: in sync_code_change_from_local, construct a ConversationController
with the authenticated user_id and call get_conversation_info(conversation_id) (or an
equivalent ownership check) before touching the code-changes manager, matching the other
conversation endpoints.

[sukritgoyal]

Hi maintainers! I see CodeRabbit generated a plan but no PR is open yet. I'd love to implement the suggested fix using the ConversationController ownership check. May I be assigned to this?

[Prateeks16]

Hi @geo-chen , I am willing to do the fix for this issue, can you assign this issue to me. I'd appreciate that.

[geo-chen]

hey, im not a maintainer so i probably have no say in that

[Prateeks16]

whom can i approach if i want open contribute to potpie @geo-chen

[Souptik96]

Re-checked this against today's main (legacy/app/modules/conversations/conversations_router.py, blob badec14): the missing check is still there.

sync_code_change_from_local (L654-747) takes user: AuthenticatedUser and binds user_id = user["user_id"] at L679 — then never uses it. It calls _set_conversation_id(conversation_id) (L683) with the caller-supplied id and goes straight on to manager.add_file(...) / manager.update_file(...). For contrast, the same router constructs ConversationController(...) 14 times and gates on controller.get_conversation_info(...) 5 times, so the ownership pattern is well established here and this one route is simply skipping it.

One correction to the CodeRabbit plan above, in case anyone works from it: check_conversation_access() is not a method on ConversationController. It lives on ConversationService (conversation_service.py:278) and returns ConversationAccessType.WRITE / READ / NOT_FOUND. The controller only exposes get_conversation_info(), which already maps AccessTypeNotFoundError to 401 and ConversationNotFoundError to 404. media_controller.py is the in-repo precedent for calling the access check directly. So the fix is either:

1. gate the route on controller.get_conversation_info(conversation_id), exactly like the sibling routes in this file, or
2. call the service-level check and require WRITE — which needs user_email plumbed through and an async_db dependency added to the handler signature.

Either way there's a maintainer decision worth making explicitly: check_conversation_access() opens with if not user_email: return ConversationAccessType.WRITE (L281-282), so whichever path you pick, a missing email shouldn't turn into an implicit write grant on this route.

@sukritgoyal and @Prateeks16 both offered to take this (June 10 and June 17) and neither got a reply — if either of you is still on it, it's yours and I'll stay out of the way. If not, and the maintainers are happy for me to pick it up, I'll open a PR with the ownership gate plus a regression test asserting that a second user is rejected before _set_conversation_id is ever reached. Happy to follow whichever of the two shapes above you prefer.