Fix/mcp conversation ownership - #600
Merged
Merged
Conversation
Every conversation-scoped tool in McpConversationTools was gated on the coarse eddi-viewer role and nothing else, while the equivalent REST endpoints all enforce requireOwnerOrAdmin. With authorization.enabled=true any viewer could, over MCP, read ANY user's conversation memory and transcript, enumerate all users' conversations, read another conversation's audit trail (prompts, tool calls, costs) and server logs, inject turns into someone else's conversation and read the agent's reply, and take over a managed conversation by naming another userId. The read half also defeated the group-conversation ownership gate: group member conversations are ordinary conversations, so list_conversations + read_conversation_log reached transcripts that read_group_conversation denies. A naive gate would have broken MCP outright: MCP created conversations with a null userId, which the engine turns into a generated anonymous-<uuid> — a non-blank owner matching no principal — so the creator itself could never read its own conversation back. The fix therefore also stamps the caller as owner at creation, which is what makes the gate effective instead of merely restrictive. - New ConversationAccessGuard (engine.security), the non-HITL sibling of HitlAccessGuard: requireConversationOwner (owner-or-admin, skip on missing descriptor, fail-closed on store error), canAccessConversation / seesAllConversations for listings, resolveOwnerUserId for creation. This is RestAgentEngine's private ownership check lifted out, so REST and MCP cannot drift apart on who may read or drive a conversation. - RestAgentEngine delegates to the guard (behavior identical; its now-dead IConversationDescriptorStore dependency dropped). - McpConversationTools gates all eight conversation-scoped tools, returning a uniform non-leaking "Access denied" that never distinguishes "not yours" from "does not exist"; list_conversations owner-filters and over-fetches the store's full page so a personal list is not starved by other users' conversations. Default deployments (authorization.enabled=false) are unaffected — every check no-ops. With auth on, pre-existing anonymous-* conversations become invisible to non-admins over MCP: they provably belong to nobody. Tests: ConversationAccessGuardTest and McpConversationToolsOwnershipTest, which asserts per tool that a non-owner is denied AND the underlying service is never reached, while owner and admin pass.
… page Self-review of the ownership fix: list_conversations filtered a single 100-row page, which silently starves a personal list. On a shared agent the newest page is often entirely other users' conversations, so a non-admin caller got count: 0 — indistinguishable from "you have no conversations" — and the requested limit stopped meaning anything (asking for 20 could return 3 while 50 existed). - Scan forward page by page until the limit is filled or a 500-descriptor budget is spent, instead of filtering only the newest page. - Dedupe by resource URI: the store's own paging skips deleted rows, so its cursor can outrun the rows it hands back and an offset-based scan can re-read one, listing a conversation twice. - Report incomplete: true (with a note) when the scan stops on its budget rather than on the store running out — AGENTS.md "no silent caps", rather than passing a partial list off as complete. - chat_managed's denial message said "you cannot chat as another user", but the same catch also fires when a stale intent→conversation mapping points at a conversation the caller does not own. One accurate message now covers both without disclosing which. Tests: list_conversations gains scan-past-foreign-pages, budget-exhausted (incomplete), and cross-page dedupe cases.
read_agent_logs enforced ownership only when a conversationId filter was supplied; without one (unfiltered, or filtered by agentId alone) it still returned the shared cross-user server-log buffer — workflow logs, LLM provider errors, internal diagnostics that can quote other users' conversation data — to any caller holding eddi-viewer. Require eddi-admin for the unscoped/agent-only path, matching the REST log endpoint IRestLogAdmin (@RolesAllowed("eddi-admin")) so MCP is not the more permissive door. The conversation-scoped path is unchanged (owner-or-admin via ConversationAccessGuard); BoundedLogStore filters by exact conversationId, so a scoped read returns only that one conversation's lines. The admin check sits before the try so a role denial surfaces as an honest role error rather than the ownership "Access denied" message. Closes the residual read_agent_logs gap filed by the ownership commit.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughConversation ownership authorization is centralized in ChangesConversation access authorization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant MCPConversationTools
participant ConversationAccessGuard
participant ConversationService
MCPClient->>MCPConversationTools: invoke conversation operation
MCPConversationTools->>ConversationAccessGuard: authorize conversation
ConversationAccessGuard-->>MCPConversationTools: allow or deny
MCPConversationTools->>ConversationService: perform authorized operation
ConversationService-->>MCPConversationTools: return result
MCPConversationTools-->>MCPClient: result or access-denied response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR tightens authorization for MCP conversation tools by enforcing conversation ownership (or admin) checks and aligning MCP behavior with the stricter REST access policies via a shared ConversationAccessGuard.
Changes:
- Introduces
ConversationAccessGuardas a shared owner-or-admin gate used by both REST (RestAgentEngine) and MCP (McpConversationTools). - Updates MCP conversation flows to (a) stamp the caller as the owner on new conversations and (b) enforce ownership checks before reading/driving conversations, audit trails, and conversation-scoped logs.
- Expands unit tests to cover these security boundaries and documents the changes in the changelog.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/ai/labs/eddi/engine/security/ConversationAccessGuard.java | New shared owner-or-admin guard for conversation access and owner stamping. |
| src/main/java/ai/labs/eddi/engine/mcp/McpConversationTools.java | Applies ownership/admin checks across MCP conversation tools; adds owner-scoped listing behavior and log gating. |
| src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java | Refactors REST engine to delegate conversation ownership checks to ConversationAccessGuard. |
| src/test/java/ai/labs/eddi/engine/security/ConversationAccessGuardTest.java | New unit tests for guard behavior (owner/admin/unowned/auth-disabled/store errors). |
| src/test/java/ai/labs/eddi/engine/mcp/McpConversationToolsOwnershipTest.java | New MCP security regression tests ensuring non-owners are denied and services aren’t reached. |
| src/test/java/ai/labs/eddi/engine/mcp/McpConversationToolsTest.java | Updates tool wiring to pass a ConversationAccessGuard. |
| src/test/java/ai/labs/eddi/engine/mcp/McpConversationToolsHitlTest.java | Updates tool wiring to pass a ConversationAccessGuard while keeping HITL behavior covered. |
| src/test/java/ai/labs/eddi/engine/mcp/McpConversationToolsExtendedTest.java | Updates tool wiring to pass a ConversationAccessGuard. |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java | Updates REST engine construction to include ConversationAccessGuard. |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineHitlTest.java | Updates REST engine construction to include ConversationAccessGuard. |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineToolPauseDetailsTest.java | Updates REST engine construction to include ConversationAccessGuard. |
| docs/changelog.md | Adds rationale, behavior notes, and testing notes for the security changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java (1)
98-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelegate owner resolution to
ConversationAccessGuard.Since
ConversationAccessGuardwas introduced to centralize conversation ownership logic and specifically providesresolveOwnerUserId()to stamp new conversations, you can delegate this call directly to the guard instead of invokingOwnershipValidator.♻️ Proposed refactor
try { - String resolvedUserId = ownershipValidator.validateAndResolveUserId(identity, userId); + String resolvedUserId = conversationAccessGuard.resolveOwnerUserId(userId); var result = conversationService.startConversation(environment, agentId, resolvedUserId, context);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java` around lines 98 - 100, In the conversation-start flow, replace the direct ownershipValidator.validateAndResolveUserId call with ConversationAccessGuard.resolveOwnerUserId(), passing the same identity and userId inputs, and use its result when calling conversationService.startConversation. Remove the now-unneeded direct OwnershipValidator dependency for this path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/changelog.md`:
- Line 42: Update the `docs/changelog.md` entry to remove or correct the claim
that `McpConversationTools` provides uniform non-disclosing access denial; state
the actual behavior until missing and foreign conversation paths are normalized.
In `@src/main/java/ai/labs/eddi/engine/mcp/McpConversationTools.java`:
- Around line 90-97: Inject MeterRegistry into McpConversationTools and register
counters for conversation ownership denials and exhausted listing scans.
Increment the ownership counter wherever ConversationAccessGuard rejects access
and increment the scan counter when listing reaches its exhaustion condition,
using descriptive metric names and the existing MeterRegistry conventions.
- Around line 682-686: Update the managed-conversation tool flow around
conversationAccessGuard.resolveOwnerUserId to stop accepting or trusting userId
as an explicit tool argument. Obtain the conversation owner from
IConversationMemory instead, while preserving authorization for the current
conversation; move any cross-user impersonation behavior into a separate
administrative operation.
- Around line 114-121: Update McpConversationTools call paths that invoke
ConversationAccessGuard.requireConversationOwner() to treat a null conversation
descriptor the same as ForbiddenException by returning accessDenied(...),
preserving the uniform denial response. Update docs/changelog.md at the affected
entry to document that missing conversation descriptors now fail closed with the
same access-denied behavior.
- Around line 408-449: Add the existing `@Blocking` annotation to the
list_conversations MCP tool method, matching the annotation style used by other
synchronous MCP tools. Keep its owner-scoped scanning and store-read behavior
unchanged.
- Around line 423-452: Fix the pagination loop around
readConversationDescriptors so deleted descriptors cannot cause offset re-reads
or premature exhaustion. Use an absolute offset/continuation mechanism supported
by the store, advance it according to the store’s actual paging contract rather
than page.size(), and remove the page-size exhaustion assumption unless the API
explicitly guarantees it; preserve URI deduplication, access filtering, the scan
budget, and incomplete reporting.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java`:
- Around line 98-100: In the conversation-start flow, replace the direct
ownershipValidator.validateAndResolveUserId call with
ConversationAccessGuard.resolveOwnerUserId(), passing the same identity and
userId inputs, and use its result when calling
conversationService.startConversation. Remove the now-unneeded direct
OwnershipValidator dependency for this path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0542748d-280a-4019-9b5a-055694f0c27e
📒 Files selected for processing (12)
docs/changelog.mdsrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.javasrc/main/java/ai/labs/eddi/engine/mcp/McpConversationTools.javasrc/main/java/ai/labs/eddi/engine/security/ConversationAccessGuard.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineHitlTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineToolPauseDetailsTest.javasrc/test/java/ai/labs/eddi/engine/mcp/McpConversationToolsExtendedTest.javasrc/test/java/ai/labs/eddi/engine/mcp/McpConversationToolsHitlTest.javasrc/test/java/ai/labs/eddi/engine/mcp/McpConversationToolsOwnershipTest.javasrc/test/java/ai/labs/eddi/engine/mcp/McpConversationToolsTest.javasrc/test/java/ai/labs/eddi/engine/security/ConversationAccessGuardTest.java
RestConversationStore.readConversationDescriptors (GET /conversationstore/conversations) had no @RolesAllowed and no ownership filter, so any authenticated caller could enumerate every user's conversation descriptors (id, agent, state, owner). Inject ConversationAccessGuard and owner-filter the listing inside the existing paging do-while: admins and auth-disabled callers short-circuit via seesAllConversations(), and the loop back-fills across pages so a personal list is never starved. This is the REST twin of the MCP list_conversations ownership fix; direct callers (incl. the EDDI-Manager UI) hit the endpoint with their own identity, so filtering is correct. Also simplify McpConversationTools.listConversations to a single store call, removing its owner-scoping over-fetch loop. That loop's only runtime path (auth-on, non-admin) 401s at the unauthenticated internal loopback before it runs, and it carried a latent page-index bug (scanned row-count passed as the store's page index). Under auth-off the store returns all and the tool relays it, unchanged from before. Tests: new RestConversationStoreOwnershipTest (own-only, intruder sees nothing, admin-all, legacy-unowned visible, personal list back-filled across foreign pages); wire the guard through the existing RestConversationStore(Filter)Test constructors; reduce the MCP ownership listing test to a delegation check.
Adversarial review of the previous commit found that owner-filtering GET /conversationstore/conversations turned the default non-admin Manager list view into an O(total-conversations) scan under authorization.enabled=true: the ownership gate ran AFTER populateDataToDescriptor (a full memory-document load per row) and the back-fill had no scan budget, so a caller owning few/none of a large shared store paged the entire descriptor collection, one snapshot load per row — a repeatable authenticated DoS-amplification primitive. The access-control result was correct; the cost was not. - Split the ownership check around the snapshot load: for the common case (owner recorded on the descriptor since v5.1.6) decide on descriptor.getUserId() BEFORE populateDataToDescriptor, so a foreign row is skipped without loading its memory document; only a legacy null-owner row falls through to the post-populate re-check that resolves the owner from the snapshot. - Cap the back-fill at MAX_OWNER_SCAN=500 (mirroring the MCP twin's budget) so a sparse owner cannot force a full-collection scan. Admins and auth-disabled callers are never filtered and never reach the bound. Tests: RestConversationStoreOwnershipTest gains a foreign-row-skipped- without-snapshot-load case, a legacy null-owner-resolved-to-foreign-owner case (guards the post-populate ordering so a reorder above populate now fails), and a scan-budget-bounded case. Changelog updated with the cost tradeoff and an owner-scoped-query follow-up.
The two verify(never()).loadConversationMemorySnapshot(anyString()) assertions were vacuous: the test descriptors used non-hex ids, so extractResourceId(...).getId() returned null and a reordered populate would call loadConversationMemorySnapshot(null), which anyString() does not match — so the assertion stayed green even under the DoS reorder. Give those descriptors valid hex ids so getId() is non-null; a mutation test (moving populateDataToDescriptor before the ownership check) now makes both foreignRowSkippedWithoutSnapshotLoad and scanIsBoundedForSparseOwner fail, so the O(store) DoS reorder can no longer ship green.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/ai/labs/eddi/engine/mcp/McpConversationTools.java (1)
389-404: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winForward the caller auth context to the delegated conversation-store request
RestInterfaceFactory.get(IRestConversationStore.class)always creates a new REST client tohttp://127.0.0.1:<port>, so this call does not automatically carry the MCP caller’sSecurityIdentity. That leavesConversationAccessGuardrunning under the wrong principal on the receiving side and can break the owner-filtering guarantee forlist_conversations. Forward the caller’sAuthorizationheader here, or inject the store directly instead of going through HTTP.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/engine/mcp/McpConversationTools.java` around lines 389 - 404, The conversation-store lookup in the list_conversations flow loses the MCP caller’s authorization context when RestInterfaceFactory.get creates its HTTP client. Update the request path around RestInterfaceFactory.get(IRestConversationStore.class) to forward the caller’s Authorization header to the delegated REST request, or replace the REST lookup with direct store injection, ensuring ConversationAccessGuard evaluates the original caller and owner filtering remains enforced.
🧹 Nitpick comments (1)
src/main/java/ai/labs/eddi/engine/memory/rest/RestConversationStore.java (1)
77-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Micrometer metrics for the new ownership-filtering path.
This listing now enforces owner-scoping and a hard scan budget (
MAX_OWNER_SCAN), but there's no counter/gauge tracking denied/foreign rows skipped or budget-exhaustion events. Without this, a caller silently hitting the scan cap (getting an incomplete list) is operationally invisible.As per coding guidelines, "Add Micrometer metrics to new features, using counters, timers, or gauges registered through
MeterRegistry."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/engine/memory/rest/RestConversationStore.java` around lines 77 - 104, Inject a Micrometer MeterRegistry into RestConversationStore and add metrics for the ownership-filtering path: count denied or foreign rows skipped and count events where MAX_OWNER_SCAN is exhausted. Increment these metrics at the corresponding filtering and scan-cap branches so incomplete listings are observable, using the project’s established metric naming and registration conventions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/changelog.md`:
- Around line 18-19: The changelog's “never starved” claim must be qualified by
the owner-scan limit. Update the first bullet around readDescriptors and the
endpoint’s do-while to state that back-filling prevents starvation only within
the MAX_OWNER_SCAN budget, while preserving the existing explanation that no
resource-URI deduplication is required.
In `@src/main/java/ai/labs/eddi/engine/memory/rest/RestConversationStore.java`:
- Around line 55-61: Update the javadoc for the owner-scan limit in
RestConversationStore to remove the stale reference to an MCP-specific cap and
state that this store provides the sole owner-scan budget used by delegated
filtering. Preserve the existing explanation of the bound and the
admin/auth-disabled exception.
---
Outside diff comments:
In `@src/main/java/ai/labs/eddi/engine/mcp/McpConversationTools.java`:
- Around line 389-404: The conversation-store lookup in the list_conversations
flow loses the MCP caller’s authorization context when RestInterfaceFactory.get
creates its HTTP client. Update the request path around
RestInterfaceFactory.get(IRestConversationStore.class) to forward the caller’s
Authorization header to the delegated REST request, or replace the REST lookup
with direct store injection, ensuring ConversationAccessGuard evaluates the
original caller and owner filtering remains enforced.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/memory/rest/RestConversationStore.java`:
- Around line 77-104: Inject a Micrometer MeterRegistry into
RestConversationStore and add metrics for the ownership-filtering path: count
denied or foreign rows skipped and count events where MAX_OWNER_SCAN is
exhausted. Increment these metrics at the corresponding filtering and scan-cap
branches so incomplete listings are observable, using the project’s established
metric naming and registration conventions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: caf35910-9f10-4b37-a123-26a2eee41295
📒 Files selected for processing (7)
docs/changelog.mdsrc/main/java/ai/labs/eddi/engine/mcp/McpConversationTools.javasrc/main/java/ai/labs/eddi/engine/memory/rest/RestConversationStore.javasrc/test/java/ai/labs/eddi/engine/mcp/McpConversationToolsOwnershipTest.javasrc/test/java/ai/labs/eddi/engine/memory/rest/RestConversationStoreFilterTest.javasrc/test/java/ai/labs/eddi/engine/memory/rest/RestConversationStoreOwnershipTest.javasrc/test/java/ai/labs/eddi/engine/memory/rest/RestConversationStoreTest.java
…doc accuracy Triaged the Copilot + CodeRabbit review of this branch. Most bot findings targeted the MCP-side list_conversations over-fetch/scan loop that 009ca0f already deleted (its page-index bug was the reason for the delete), so they were moot against current HEAD. The substantive, still-valid items: - Observability (Micrometer), per the repo "always add metrics to new features" convention — the new authorization paths were operationally invisible. Two counters via field-injected MeterRegistry (AGENTS.md metrics pattern, with a SimpleMeterRegistry default so unit tests that construct the bean directly stay non-null): * eddi.mcp.conversation.access.denied{tool} on every MCP ownership denial (the six gated read/drive tools via accessDenied, plus chat_managed's impersonation denial). MCP denials return a 200 error-body, so unlike REST 403s they are not visible in http.server.requests. * eddi.conversations.listing.owner_scan_exhausted when a non-admin listing stops on the MAX_OWNER_SCAN budget with fewer than `limit` results — the truncation the List return type cannot signal to the caller. - Fail-open on a missing descriptor: kept deliberately. requireConversationOwner returns null (operation proceeds → 404) rather than denying when the descriptor is absent; a missing descriptor means the conversation is genuinely not found, and flipping the SHARED guard to deny would change REST 404→403 and contradict its documented "let the operation handle the 404" contract. Softened accessDenied's javadoc, which had over-claimed a denial is indistinguishable from "does not exist". - Doc accuracy: RestConversationStore.MAX_OWNER_SCAN javadoc no longer says it "mirrors the MCP owner-scan cap" (MCP has none since 009ca0f — this is now the sole budget); the prior changelog entry's "never starved" line is qualified. Declined: removing userId from chat_managed. The cited rule exempts external interfaces (MCP/REST) that operate outside a conversation, which is exactly what chat_managed is (it routes to a per-intent+userId managed conversation rather than running inside one); resolveOwnerUserId already rejects impersonation. Tests: metric assertions for both counters (denial tagged by tool; budget exhaustion). 215 affected tests green.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/ai/labs/eddi/engine/memory/rest/RestConversationStore.java (1)
145-145: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winEnforce
MAX_OWNER_SCANwithin the descriptor loop.The current check runs only after a full page is processed, so a non-admin request can scan up to
MAX_OWNER_SCAN + limit - 1descriptors. That violates the documented upper bound and can make the exhaustion metric report after more work than intended.Proposed fix
for (var conversationDescriptor : conversationDescriptors) { + if (!seesAllConversations && scannedDescriptors >= MAX_OWNER_SCAN) { + break; + } scannedDescriptors++;Also applies to: 153-154, 226-238
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/engine/memory/rest/RestConversationStore.java` at line 145, Move the MAX_OWNER_SCAN enforcement into the descriptor-processing loop that uses scannedDescriptors, checking the limit before processing each additional descriptor so non-admin requests never scan beyond the configured bound. Preserve the existing pagination and exhaustion-metric behavior while stopping immediately when MAX_OWNER_SCAN is reached.docs/changelog.md (1)
28-28: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the ownership-check ordering description.
The implementation checks descriptors with a recorded
userIdbeforepopulateDataToDescriptor; only legacy descriptors without an owner are populated and then re-checked. Update this sentence to reflect both paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/changelog.md` at line 28, Update the RestConversationStore changelog description to state that descriptors with a recorded userId are checked before populateDataToDescriptor, while legacy descriptors without an owner are populated first and then re-checked. Preserve the existing ownership and visibility behavior details.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/changelog.md`:
- Line 33: Update the changelog entry’s description of MAX_OWNER_SCAN to remove
the claim that it mirrors an MCP budget. State that it is solely the REST
listing’s owner-scan budget, keeping the surrounding behavior and truncation
details unchanged.
---
Outside diff comments:
In `@docs/changelog.md`:
- Line 28: Update the RestConversationStore changelog description to state that
descriptors with a recorded userId are checked before populateDataToDescriptor,
while legacy descriptors without an owner are populated first and then
re-checked. Preserve the existing ownership and visibility behavior details.
In `@src/main/java/ai/labs/eddi/engine/memory/rest/RestConversationStore.java`:
- Line 145: Move the MAX_OWNER_SCAN enforcement into the descriptor-processing
loop that uses scannedDescriptors, checking the limit before processing each
additional descriptor so non-admin requests never scan beyond the configured
bound. Preserve the existing pagination and exhaustion-metric behavior while
stopping immediately when MAX_OWNER_SCAN is reached.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07890f18-207f-44e8-b614-5a0672a944fb
📒 Files selected for processing (5)
docs/changelog.mdsrc/main/java/ai/labs/eddi/engine/mcp/McpConversationTools.javasrc/main/java/ai/labs/eddi/engine/memory/rest/RestConversationStore.javasrc/test/java/ai/labs/eddi/engine/mcp/McpConversationToolsOwnershipTest.javasrc/test/java/ai/labs/eddi/engine/memory/rest/RestConversationStoreOwnershipTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/ai/labs/eddi/engine/mcp/McpConversationToolsOwnershipTest.java
- src/test/java/ai/labs/eddi/engine/memory/rest/RestConversationStoreOwnershipTest.java
- src/main/java/ai/labs/eddi/engine/mcp/McpConversationTools.java
…ccuracy Second CodeRabbit pass (its first pass predated the metrics/doc commit and was already moot). Two valid, current items: - Enforce MAX_OWNER_SCAN per-descriptor, not per-page. The budget was checked only in the do-while condition (after a full page), so a non-admin scan could reach MAX_OWNER_SCAN + limit - 1 before stopping — over the documented bound. Added an in-loop break. Impact is small (the overrun rows are within an already-fetched page, and foreign rows skip the snapshot load either way), but it makes the bound exact and owner_scan_exhausted fire at 500 rather than up to a page late. Not separately unit-tested: with all-foreign pages the store returns the same empty list and page-read count with or without the break, so the tightening is not observable through the store interface; the existing bounded-scan tests guard against regression. - Changelog accuracy: the 009ca0f entry said the ownership check "runs after populateDataToDescriptor" — stale since 8bb304b split it (common case decides before the snapshot load; only a legacy null-owner row is re-checked after). Corrected, and dropped a second stale "mirroring the MCP twin's budget" reference (MCP has no scan cap since 009ca0f).
aisabella-ai
approved these changes
Jul 20, 2026
…ownership # Conflicts: # docs/changelog.md
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
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
This pull request addresses major security gaps in the MCP (Management Control Plane) conversation tools by enforcing ownership and admin checks for all conversation-related operations, aligning them with the stricter REST API policies. The changes ensure that only conversation owners or admins can read or modify conversations and related logs, thus preventing unauthorized access or modification by users with only the general viewer role. Additionally, the implementation centralizes access control logic to prevent future drift and improves test coverage for these security boundaries.
Security and Access Control Tightening:
Introduced
ConversationAccessGuardto enforce owner-or-admin checks for all MCP conversation tools, ensuring only authorized users can read, list, or modify conversations and their logs. This closes gaps whereeddi-viewerusers could previously access or inject into any conversation. [1] [2] [3] [4] [5] [6] [7] [8] [9]Updated conversation creation in MCP to stamp the caller as the conversation owner, preventing orphaned conversations that would otherwise be unreadable due to ownership checks. [1] [2] [3]
Added uniform, non-leaking error handling for access denials, ensuring error messages do not reveal the existence of other users’ conversations. [1] [2] [3]
Alignment and Refactoring:
RestAgentEngineto delegate ownership validation toConversationAccessGuard, ensuring consistent access control logic between REST and MCP surfaces. Removed the now-redundant direct descriptor store dependency and validation logic. [1] [2] [3] [4] [5]Security for Log Access:
read_agent_logsto requireeddi-adminfor unscoped or agent-scoped log reads, preventing cross-user log exposure. Conversation-scoped log reads remain available to owners or admins, preserving self-service diagnostics without cross-user leakage.Tests and Documentation:
McpConversationToolsOwnershipTest,ConversationAccessGuardTest) to cover all new access restrictions and ensure no data is leaked to unauthorized users. Updated documentation and changelog with detailed rationale and design decisions.These changes close critical authorization gaps, align access policies across all interfaces, and improve maintainability by centralizing access logic.
Type of Change
Checklist
./mvnw clean verify -DskipITs)Summary by CodeRabbit