Fix/security audit idor remediation#530
Conversation
…ions, memories, groups - NEW: OwnershipValidator utility (validateUserAccess, validateAndResolveUserId, requireOwnerOrAdmin) - RestAgentEngine: ownership checks on all conversation-scoped endpoints, userId resolution on startConversation - RestUserMemoryStore: ownership checks on all userId-scoped endpoints - RestGroupConversation: ownership checks on read/delete, userId validation on discuss, filtered list for non-admins - IRestGdprAdmin: moved @RolesAllowed to interface level (was implementation-only) - RestA2AEndpoint: added @permitAll to GET discovery endpoints (documents intentional public access) - McpToolUtils: added requireOwnerOrAdmin() for MCP-level ownership validation - McpMemoryTools: added ownership checks to all read tools (userId param validated against caller) Admin role (eddi-admin) bypasses all ownership checks. All checks are no-ops when auth is disabled.
Updated 4 test files to pass SecurityIdentity and OwnershipValidator mocks to the modified constructors of RestAgentEngine, RestUserMemoryStore, and RestGroupConversation.
…nsolidation, deleteMemory ownership - M1: Remove duplicate requireOwnerOrAdmin from McpToolUtils; McpMemoryTools injects OwnershipValidator directly - M3: Sanitize PII in WARN logs; full details at DEBUG level only - M4: Narrow catch in validateConversationOwnership to ResourceNotFoundException + ResourceStoreException - BUG-2: Add findEntryById to IUserMemoryStore (Mongo + Postgres); deleteMemory validates ownership before deletion
…new tests) - OwnershipValidatorTest [NEW]: 24 tests for all 4 public methods - RestAgentEngineTest: 5 ownership validation tests + default descriptor stub - RestUserMemoryStoreTest: 3 deleteMemory ownership tests - RestGroupConversationTest: 4 ownership validation tests (IDOR, list filtering) - McpMemoryToolsTest: updated constructor for OwnershipValidator injection
📝 WalkthroughWalkthroughAdds a centralized OwnershipValidator and enforces ownership checks across user memory stores, conversation and group conversation REST endpoints, and MCP memory tools; hardens GDPR and A2A endpoint annotations; updates constructors and tests; and documents the changes in the changelog. ChangesSecurity Audit Remediation: IDOR Prevention & Ownership Validation
🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull request overview
This PR remediates security-audit IDOR findings by centralizing resource ownership enforcement and applying it across conversation, group conversation, user memory REST endpoints, and MCP tools, while also hardening security annotations for GDPR admin and public A2A discovery endpoints.
Changes:
- Introduces a centralized
OwnershipValidatorand wires it into REST + MCP entry points to enforce owner/admin access control. - Updates user-memory deletion to validate ownership via an ID lookup (
findEntryById) before deleting and return 404 when the entry does not exist. - Hardens security annotations by moving GDPR admin
@RolesAllowedto the interface and explicitly marking A2A discovery endpoints@PermitAll, with accompanying test updates/additions.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/ai/labs/eddi/engine/security/OwnershipValidator.java | Adds centralized ownership validation (owner/admin checks, userId resolution) with audit logging. |
| src/main/java/ai/labs/eddi/engine/mcp/McpMemoryTools.java | Enforces ownership checks for MCP memory read tools via OwnershipValidator. |
| src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java | Adds ownership validation on read/delete and filters list results for non-admin callers; resolves userId via validator. |
| src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java | Adds conversation ownership validation by loading the conversation descriptor and enforcing owner/admin access; resolves start userId via validator. |
| src/main/java/ai/labs/eddi/engine/gdpr/RestGdprAdmin.java | Removes implementation-level @RolesAllowed after moving it to the interface. |
| src/main/java/ai/labs/eddi/engine/gdpr/IRestGdprAdmin.java | Adds interface-level @RolesAllowed("eddi-admin") for GDPR admin endpoints. |
| src/main/java/ai/labs/eddi/engine/a2a/RestA2AEndpoint.java | Marks A2A discovery GET endpoints as intentionally public with @PermitAll. |
| src/main/java/ai/labs/eddi/datastore/postgres/PostgresUserMemoryStore.java | Implements findEntryById for ownership validation prior to delete. |
| src/main/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStore.java | Adds ownership validation to all endpoints and validates delete via entry lookup prior to deletion. |
| src/main/java/ai/labs/eddi/configs/properties/mongo/MongoUserMemoryStore.java | Implements findEntryById for ownership validation prior to delete. |
| src/main/java/ai/labs/eddi/configs/properties/IUserMemoryStore.java | Adds findEntryById to support ownership validation on delete. |
| src/test/java/ai/labs/eddi/engine/security/OwnershipValidatorTest.java | New unit tests covering validator behavior (auth on/off, admin bypass, mismatch failures). |
| src/test/java/ai/labs/eddi/engine/mcp/McpMemoryToolsTest.java | Updates test wiring for new OwnershipValidator dependency. |
| src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.java | Adds ownership-validation tests and updates constructor dependencies. |
| src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationExtendedTest.java | Updates constructor dependencies to include identity + validator. |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java | Adds ownership-validation tests and updates constructor dependencies to include descriptor store + validator. |
| src/test/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStoreTest.java | Updates delete-memory tests to cover 204/404/403 behavior with ownership validation. |
| docs/changelog.md | Documents the security audit remediation, ownership validator, and test coverage changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/ai/labs/eddi/engine/mcp/McpMemoryToolsTest.java (1)
27-35: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd ownership validation integration tests for MCP memory tools.
The test setup now includes
OwnershipValidator, but no tests verify the ownership validation integration. For a security feature preventing IDOR attacks, explicit test coverage is essential.Consider adding tests for:
validateUserAccessis called with correct identity and userId parameters- Non-owner receives
ForbiddenExceptionwhen attempting to access another user's memories- Admin bypass behavior (if applicable to MCP tools)
- Owner can successfully access their own memories after ownership check
Similar ownership tests were added to
RestGroupConversationTest(lines 226-299 in that file) and can serve as a pattern.🤖 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/test/java/ai/labs/eddi/engine/mcp/McpMemoryToolsTest.java` around lines 27 - 35, Add unit tests that exercise McpMemoryTools' ownership integration: in the test class (setUp uses mocked OwnershipValidator and SecurityIdentity) add tests that 1) verify OwnershipValidator.validateUserAccess is invoked with the SecurityIdentity mock and the target userId when calling the MCP methods that load user memories, using Mockito.verify; 2) stub validateUserAccess to throw a ForbiddenException and assert the MCP method propagates/throws ForbiddenException for non-owners; 3) stub validateUserAccess to allow access and assert owners can read their memories; and 4) if admin bypass behavior exists, stub SecurityIdentity as admin and assert validateUserAccess is not blocking (or verify bypass path). Target methods/classes: McpMemoryTools and OwnershipValidator.validateUserAccess; use the existing userMemoryStore and jsonSerialization mocks to drive responses and Mockito.verify/when to assert behavior.
🤖 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 32-35: Update the changelog to remove ambiguity by clarifying that
the addition of requireOwnerOrAdmin() in McpToolUtils was the initial fix and
was later replaced by direct use of OwnershipValidator, or alternatively change
the earlier entry to reflect the final approach: locate the entries referencing
McpToolUtils and requireOwnerOrAdmin and the later entry mentioning
OwnershipValidator and either annotate the first as "initial approach" or
rewrite it to state the final solution (direct OwnershipValidator usage) so the
document shows a single, unambiguous final security posture.
In `@src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java`:
- Around line 248-267: The current validateConversationOwnership method silently
skips ownership checks when conversationDescriptorStore.readDescriptor(...)
throws ResourceStoreException, creating a fail-open; change the
ResourceStoreException handling so the method fails-closed by throwing a
ForbiddenException (or otherwise denying access) instead of just logging; ensure
the thrown ForbiddenException references that ownership could not be verified
and includes the original ResourceStoreException as the cause so callers see the
underlying error; keep the existing handling for ResourceNotFoundException if
you want the actual operation to handle missing descriptors.
In `@src/main/java/ai/labs/eddi/engine/mcp/McpMemoryTools.java`:
- Around line 62-67: In listUserMemories, getVisibleMemories,
searchUserMemories, getMemoryByKey, and countUserMemories, move the parameter
null/blank checks for userId (and any other required params like key) to occur
immediately after requireRole(...) and before calling
ownershipValidator.validateUserAccess(identity, userId); specifically, validate
userId (and key where applicable) and return errorJson("userId is required") or
appropriate errorJson before invoking ownershipValidator.validateUserAccess in
the methods listUserMemories, getVisibleMemories, searchUserMemories,
getMemoryByKey, and countUserMemories so the ownership validator never receives
null/blank values.
---
Outside diff comments:
In `@src/test/java/ai/labs/eddi/engine/mcp/McpMemoryToolsTest.java`:
- Around line 27-35: Add unit tests that exercise McpMemoryTools' ownership
integration: in the test class (setUp uses mocked OwnershipValidator and
SecurityIdentity) add tests that 1) verify OwnershipValidator.validateUserAccess
is invoked with the SecurityIdentity mock and the target userId when calling the
MCP methods that load user memories, using Mockito.verify; 2) stub
validateUserAccess to throw a ForbiddenException and assert the MCP method
propagates/throws ForbiddenException for non-owners; 3) stub validateUserAccess
to allow access and assert owners can read their memories; and 4) if admin
bypass behavior exists, stub SecurityIdentity as admin and assert
validateUserAccess is not blocking (or verify bypass path). Target
methods/classes: McpMemoryTools and OwnershipValidator.validateUserAccess; use
the existing userMemoryStore and jsonSerialization mocks to drive responses and
Mockito.verify/when to assert behavior.
🪄 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: d5e024c6-b7b2-4f64-8001-36ff7f9fb5c2
📒 Files selected for processing (18)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/properties/IUserMemoryStore.javasrc/main/java/ai/labs/eddi/configs/properties/mongo/MongoUserMemoryStore.javasrc/main/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStore.javasrc/main/java/ai/labs/eddi/datastore/postgres/PostgresUserMemoryStore.javasrc/main/java/ai/labs/eddi/engine/a2a/RestA2AEndpoint.javasrc/main/java/ai/labs/eddi/engine/gdpr/IRestGdprAdmin.javasrc/main/java/ai/labs/eddi/engine/gdpr/RestGdprAdmin.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.javasrc/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.javasrc/main/java/ai/labs/eddi/engine/mcp/McpMemoryTools.javasrc/main/java/ai/labs/eddi/engine/security/OwnershipValidator.javasrc/test/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStoreTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestGroupConversationExtendedTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.javasrc/test/java/ai/labs/eddi/engine/mcp/McpMemoryToolsTest.javasrc/test/java/ai/labs/eddi/engine/security/OwnershipValidatorTest.java
💤 Files with no reviewable changes (1)
- src/main/java/ai/labs/eddi/engine/gdpr/RestGdprAdmin.java
…nd MCP validation order - RestAgentEngine: sanitize conversationId in debug logs via LogSanitizer - OwnershipValidator: sanitize all user-provided values in debug logs - RestAgentEngine: fail-closed on ResourceStoreException in ownership check (previously silently skipped, allowing unauthorized access during DB errors) - McpMemoryTools: reorder validation — null/blank check before ownership check in all 5 read-only tools (prevents ForbiddenException on missing input) - Changelog: clarify MCP ownership fix chronology, add remediation entry
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 (1)
src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java (1)
139-146:⚠️ Potential issue | 🟠 Major | ⚡ Quick winArm the
AsyncResponsetimeout before the ownership lookup.Lines 143 and 163 call
validateConversationOwnership(conversationId)before Lines 171-172 register the timeout handler. Since that helper does a synchronousconversationDescriptorStore.readDescriptor(...), a slow or hung descriptor lookup can keepsay/rerunoutside theagentTimeoutwindow and block the request thread before the async timeout is even active.♻️ Suggested change
+ private void configureAsyncTimeout(AsyncResponse response) { + response.setTimeout(agentTimeout, TimeUnit.SECONDS); + response.setTimeoutHandler(asyncResp -> asyncResp.resume(Response.status(Response.Status.REQUEST_TIMEOUT).build())); + } + `@Override` public void rerunLastConversationStep(String conversationId, String language, Boolean returnDetailed, Boolean returnCurrentStepOnly, List<String> returningFields, final AsyncResponse response) { checkNotNull(conversationId, "conversationId"); checkNotEmpty(language, "language"); + configureAsyncTimeout(response); validateConversationOwnership(conversationId); sayInternal(conversationId, returnDetailed, returnCurrentStepOnly, returningFields, new InputData("", Map.of(KEY_LANG, new Context(string, language))), true, response); } @@ public void sayWithinContext(final String conversationId, final Boolean returnDetailed, final Boolean returnCurrentStepOnly, final List<String> returningFields, final InputData inputData, final AsyncResponse response) { checkNotNull(conversationId, "conversationId"); checkNotNull(inputData, "inputData"); checkNotNull(inputData.getInput(), "inputData.input"); + configureAsyncTimeout(response); validateConversationOwnership(conversationId); sayInternal(conversationId, returnDetailed, returnCurrentStepOnly, returningFields, inputData, false, response); } @@ private void sayInternal(String conversationId, Boolean returnDetailed, Boolean returnCurrentStepOnly, List<String> returningFields, InputData inputData, boolean rerunOnly, AsyncResponse response) { - - response.setTimeout(agentTimeout, TimeUnit.SECONDS); - response.setTimeoutHandler(asyncResp -> asyncResp.resume(Response.status(Response.Status.REQUEST_TIMEOUT).build())); - try { conversationService.say(conversationId, returnDetailed, returnCurrentStepOnly, returningFields, inputData, rerunOnly, response::resume);As per coding guidelines, "Code must be thread-safe and non-blocking. REST endpoints use JAX-RS AsyncResponse. Tasks execute synchronously but must not block for extended periods."
Also applies to: 160-173
🤖 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 139 - 146, The AsyncResponse timeout must be armed before any synchronous blocking calls like validateConversationOwnership(conversationId); modify rerunLastConversationStep (and the other affected endpoint method) so that you register the AsyncResponse timeout/timeout handler (the code that sets the agent timeout on the provided AsyncResponse) immediately upon entry, then perform validateConversationOwnership(conversationId) and the subsequent sayInternal(...) call; ensure sayInternal(...) and validateConversationOwnership(...) remain unchanged but are executed after the timeout is registered so a hung conversationDescriptorStore.readDescriptor(...) cannot prevent the JAX-RS timeout from firing.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 `@src/main/java/ai/labs/eddi/engine/mcp/McpMemoryTools.java`:
- Around line 64-67: In McpMemoryTools, the current requireRole(identity,
authEnabled, "eddi-viewer") calls block callers who only have eddi-admin; update
those viewer-gated checks to allow either eddi-viewer or eddi-admin before
calling ownershipValidator.validateUserAccess(...). Concretely, in the
McpMemoryTools methods containing the requireRole calls (the occurrences around
the existing requireRole(...) at lines shown), replace the single-role gate with
a check that succeeds if identity.hasRole("eddi-viewer") OR
identity.hasRole("eddi-admin") (or use an existing helper that accepts multiple
roles), then proceed to ownershipValidator.validateUserAccess(identity, userId)
as before. Ensure the change is applied to every occurrence of requireRole in
this class so eddi-admin can bypass the viewer check consistently.
---
Outside diff comments:
In `@src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java`:
- Around line 139-146: The AsyncResponse timeout must be armed before any
synchronous blocking calls like validateConversationOwnership(conversationId);
modify rerunLastConversationStep (and the other affected endpoint method) so
that you register the AsyncResponse timeout/timeout handler (the code that sets
the agent timeout on the provided AsyncResponse) immediately upon entry, then
perform validateConversationOwnership(conversationId) and the subsequent
sayInternal(...) call; ensure sayInternal(...) and
validateConversationOwnership(...) remain unchanged but are executed after the
timeout is registered so a hung conversationDescriptorStore.readDescriptor(...)
cannot prevent the JAX-RS timeout from firing.
🪄 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: 6c24dae9-2bf6-4f76-bdac-514fd24c9afa
📒 Files selected for processing (4)
docs/changelog.mdsrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.javasrc/main/java/ai/labs/eddi/engine/mcp/McpMemoryTools.javasrc/main/java/ai/labs/eddi/engine/security/OwnershipValidator.java
✅ Files skipped from review due to trivial changes (1)
- docs/changelog.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/ai/labs/eddi/engine/security/OwnershipValidator.java
| requireRole(identity, authEnabled, "eddi-viewer"); | ||
| if (userId == null || userId.isBlank()) | ||
| return errorJson("userId is required"); | ||
| ownershipValidator.validateUserAccess(identity, userId); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== McpToolUtils.requireRole implementation =="
fd 'McpToolUtils.java' . -x sh -c 'echo "--- $1 ---"; sed -n "1,240p" "$1"' sh {}
echo
echo "== Viewer/admin role references around MCP tools =="
rg -n -C2 'requireRole\s*\(|eddi-viewer|eddi-admin' .Repository: labsai/EDDI
Length of output: 50367
Fix MCP viewer-gated role checks to allow intended eddi-admin access
McpToolUtils.requireRole(...) only passes when identity.hasRole(role) (no role hierarchy), so the viewer-gated MCP methods in src/main/java/ai/labs/eddi/engine/mcp/McpMemoryTools.java (lines 64-67, 94-97, 122-125, 146-149, 234-237) will reject callers with only eddi-admin before ownershipValidator.validateUserAccess(...) can run. Since OwnershipValidator does allow an eddi-admin bypass for ownership checks, these gates are inconsistent—either grant eddi-admin the eddi-viewer role (in the IdP) or change those methods to also accept eddi-admin.
🤖 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/McpMemoryTools.java` around lines 64 -
67, In McpMemoryTools, the current requireRole(identity, authEnabled,
"eddi-viewer") calls block callers who only have eddi-admin; update those
viewer-gated checks to allow either eddi-viewer or eddi-admin before calling
ownershipValidator.validateUserAccess(...). Concretely, in the McpMemoryTools
methods containing the requireRole calls (the occurrences around the existing
requireRole(...) at lines shown), replace the single-role gate with a check that
succeeds if identity.hasRole("eddi-viewer") OR identity.hasRole("eddi-admin")
(or use an existing helper that accepts multiple roles), then proceed to
ownershipValidator.validateUserAccess(identity, userId) as before. Ensure the
change is applied to every occurrence of requireRole in this class so eddi-admin
can bypass the viewer check consistently.
Summary
This pull request addresses multiple high- and medium-severity findings from a recent security audit, focusing on preventing Insecure Direct Object Reference (IDOR) vulnerabilities and strengthening ownership validation throughout the EDDI platform. Key changes include the introduction of a centralized
OwnershipValidatorcomponent, comprehensive ownership checks across all conversation and user memory REST endpoints, annotation hardening for GDPR and A2A endpoints, and improved test coverage for all new security logic. Additionally, the codebase has been refactored to ensure a single source of truth for ownership checks and to reduce compliance risks in logging.Security Audit Remediation and Ownership Validation
Centralized ownership validation:
OwnershipValidatorcomponent with methods for user access validation, admin bypass, and audit logging. All ownership checks are no-ops when authorization is disabled (dev mode).Interface and annotation hardening:
@RolesAllowed("eddi-admin")from the implementation to theIRestGdprAdmininterface for more robust GDPR admin protection. [1] [2] [3] [4]@PermitAllto all 5 A2A GET discovery endpoints to clarify intentional public access. [1] [2] [3] [4] [5] [6]Codebase and API improvements
findEntryByIdto theIUserMemoryStoreinterface and implementing it for both MongoDB and PostgreSQL backends. [1] [2] [3] [4]requireOwnerOrAdminlogic fromMcpToolUtils; now, all MCP memory tools use the centralized validator.Test coverage and compliance
These changes collectively close all critical and medium security findings, enforce strong resource ownership throughout the platform, and improve compliance and maintainability.
Type of Change
Checklist
./mvnw clean verify -DskipITs)Summary by CodeRabbit