Skip to content

Conversations are readable and writable by any caller who knows the id #80

Description

@ExtraToast

Problem Statement

A user's Conversation is readable and writable by anyone who knows its id.

GET /api/v1/conversations/{id}, POST /api/v1/conversations/{id}/messages and POST /api/v1/conversations/{id}/messages/stream take no caller identity and perform no ownership check. A caller who has any Conversation id can read its entire Transcript, append Turns attributed to the User, the Agent or the System, and drive answer generation against someone else's Conversation.

Conversation ids are UUIDs, so this is not trivially enumerable — but ids appear in URLs, logs and client state, and "hard to guess" is not an access control.

This is pre-existing. It is not introduced by the Chat Session to Conversation rename in #68; that rename moves the same handlers onto the canonical path.

Solution

A Conversation is visible and modifiable only to the user it belongs to. A request for someone else's Conversation is indistinguishable from a request for one that does not exist.

User Stories

  1. As a user, I want my Conversations readable only by me, so that a leaked or guessed id does not expose what I discussed with an agent.
  2. As a user, I want nobody else appending Turns to my Conversation, so that my Transcript is a truthful record of my own exchange.
  3. As a user, I want nobody else able to append a Turn attributed to the Agent or the System in my Conversation, so that I cannot be shown fabricated agent output as though the system produced it.
  4. As a user, I want nobody else able to start answer streaming on my Conversation, so that my Conversation's history and token spend are not driven by a third party.
  5. As a user, I want a request for another user's Conversation to look exactly like a request for a Conversation that does not exist, so that the response does not confirm which ids are real.
  6. As a user, I want listing my Conversations to return only mine, so that the collection view never leaks another user's titles.
  7. As a user, I want archiving to apply only to my own Conversations, so that nobody can archive mine.
  8. As a user, I want the deprecated /api/v1/chat-sessions alias to enforce the same rule as the canonical path, so that the older route is not a way around the check.
  9. As an operator, I want identity taken from the edge-injected header rather than a request body or query parameter, so that the identity used for the check is the one the SSO proxy asserted.
  10. As an operator, I want a refused request to be distinguishable in logs from a genuine miss, so that probing is visible even though the response body is not.
  11. As a developer, I want ownership enforced at one seam rather than in each handler, so that a future endpoint cannot forget the check.
  12. As a developer, I want a test that fails if a handler is added without the check, so that this regression cannot return quietly.
  13. As a reviewer, I want the rule stated once in the domain vocabulary, so that "a Conversation belongs to exactly one user" is not re-derived per endpoint.

Implementation Decisions

  • One seam. ConversationQueryService.get becomes user-scoped: it takes the requesting user alongside the Conversation id and returns nothing when the Conversation belongs to someone else. Callers already treat a nothing-result as a 404, so no handler grows a new branch and no handler can skip the check by forgetting to call it. list is already user-scoped and is unchanged.
  • Identity comes from the X-User-Id header that the SSO proxy injects at the edge, consistent with WorkspaceController, SessionStatusController, AdminRunnerController and WorkspaceRunnerEventsController. The three handlers that currently take no identity gain it. This is the estate's existing identity mechanism, not a new one.
  • Refusal is indistinguishable from absence. A Conversation owned by another user returns the same 404 as a non-existent one, following WorkspaceRunnerEventsController, which already resolves a Workspace and returns 404 rather than 403 on an owner mismatch. No 403, because a 403 confirms the id exists.
  • Commands check before dispatching. Appending a Turn and starting a stream resolve ownership before any command reaches the bus, so a refused request has no side effect. Today the append handler dispatches first and reads back afterwards; that order inverts.
  • The deprecated alias enforces the same rule. /api/v1/chat-sessions delegates to the same service, so it inherits the check rather than reimplementing it.
  • A refusal is logged at warn with the Conversation id and the requesting user, so probing is observable. The response body stays empty.
  • No schema change. Conversation already carries the owning user.
  • API contract: three handlers gain a required X-User-Id header. The canonical path is already being reshaped in Rename Chat Session to Conversation and split navigation into Workspaces and Conversations #68, so this rides along rather than adding a separate breaking change.

Testing Decisions

A good test here asserts what a caller observes — the status code and whether state changed — not how the check is implemented. It should still pass if ownership moves from the query service into a filter or a policy object.

  • Controller tests covering, for each of the six handlers: the owner succeeds; a different user gets 404; and for the two write paths, that no Turn was appended and no stream began. Prior art: the existing Conversation controller tests, and WorkspaceRunnerEventsController's owner-mismatch test, which is the closest analogue.
  • Query service tests covering: the owner gets the Conversation; a different user gets nothing; an unknown id gets nothing — so the two cases are indistinguishable at the seam as well as at the edge.
  • One architecture test asserting every handler on the Conversation controllers takes the identity header, so a new endpoint added without it fails the build rather than shipping unchecked. Prior art: the ArchUnit suite already enforces structural rules of this kind — note its existing predicate is name-based and passes vacuously when nothing matches, so this test must assert a non-empty match set.
  • Integration coverage through the existing flow tests against a real database, asserting a second user cannot read or append to the first user's Conversation.

Out of Scope

  • Any change to how the edge authenticates, or to the X-User-Id mechanism itself.
  • The internal bearer-guarded endpoints under /api/v1/internal/*, which have their own mechanism.
  • Workspaces and Agent Sessions. WorkspaceRunnerEventsController already checks ownership; a wider audit of other surfaces is worth doing separately.
  • Rate limiting or id unguessability. Both are orthogonal to an access check.

Further Notes

Found by a background security review of the #68 rename and verified against main: the identical gap exists there on the pre-rename handlers, so this is not a regression introduced by that work.

Two details that shaped the decisions above. First, agents-ui deliberately strips X-User-Id from outgoing requests so the edge-injected value is the one that arrives — the header is trustworthy on the edge path specifically because the client cannot set it. Second, the legacy surface removed in #68 did require x-user-id on its messages endpoint, which is why that removal shows up in the contract diff; restoring identity on the renamed handlers brings the canonical path back in line.

Because #68 is already reshaping these exact handlers and their contract, folding this fix into that branch avoids a second breaking change to the same endpoints.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area: agentsAgent runtime, agent APIs, tools, prompts, or UI.component: securitySecurity, permissions, secrets, or vulnerability handling.priority: P1High; important and should be handled in the current iteration.type: bugSomething is broken or behaving incorrectly.

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions