Fix/security audit idor remediation

docs/changelog.md

@@ -4,6 +4,89 @@
 
 ---
 
+## 🛡️ Security Audit Remediation — IDOR Prevention & Ownership Validation (2026-06-10)
+
+**Repo:** EDDI (`fix/security-audit-idor-remediation`)
+**What changed:** Addressed 5 findings from a comprehensive security audit. Added resource ownership validation across all conversation, user memory, and group conversation REST endpoints. Hardened GDPR, A2A, and MCP annotations.
+
+### Finding: IDOR — Conversations (HIGH → FIXED)
+- **Problem:** Any authenticated user with `eddi-user` role could read/modify ANY conversation by guessing the conversationId. No ownership validation existed despite `ConversationDescriptor` having a `userId` field.
+- **Fix:** `RestAgentEngine` now injects `SecurityIdentity`, `OwnershipValidator`, and `IConversationDescriptorStore`. All conversation-scoped endpoints (`readConversation`, `say`, `endConversation`, `undo`, `redo`, `rerun`, `readConversationLog`, `getConversationState`) validate that the caller owns the conversation. `startConversation` validates that the provided `userId` matches the caller's identity (admins can set any userId).
+
+### Finding: IDOR — User Memory (HIGH → FIXED)
+- **Problem:** Any authenticated user could read/delete another user's persistent memories via the `/usermemorystore/memories/{userId}` endpoints.
+- **Fix:** `RestUserMemoryStore` now injects `SecurityIdentity` and `OwnershipValidator`. All endpoints validate that the `{userId}` path parameter matches the authenticated caller. `upsertMemory` validates against the `userId` in the request body.
+
+### Finding: IDOR — Group Conversations (HIGH → FIXED)
+- **Problem:** Any authenticated user could read/delete any group conversation.
+- **Fix:** `RestGroupConversation` now validates ownership on `readGroupConversation` and `deleteGroupConversation`. `listGroupConversations` filters results to only the caller's conversations. `discuss`/`discussStreaming` validate the provided userId.
+
+### Finding: GDPR Annotation on Implementation Only (MEDIUM → FIXED)
+- **Problem:** `@RolesAllowed("eddi-admin")` was only on `RestGdprAdmin` implementation, not the `IRestGdprAdmin` interface. Fragile to refactoring.
+- **Fix:** Moved `@RolesAllowed("eddi-admin")` to the interface level.
+
+### Finding: A2A Endpoint Annotation Clarity (MEDIUM → FIXED)
+- **Problem:** A2A GET discovery endpoints had no explicit security annotations, making intent unclear.
+- **Fix:** Added `@PermitAll` to all 5 GET discovery endpoints to document intentional public access per A2A protocol spec.
+
+### Finding: MCP Memory Ownership (NEW → FIXED)
+- **Problem:** MCP memory read tools (`list_user_memories`, `get_visible_memories`, etc.) accepted `userId` as a tool parameter without validating against the caller's identity.
+- **Fix:** `McpMemoryTools` now injects `OwnershipValidator` and calls `validateUserAccess()` in all 5 read-only MCP memory tools (initially via `McpToolUtils.requireOwnerOrAdmin()`, consolidated to direct `OwnershipValidator` use in code review hardening below).
+
+### New Component: OwnershipValidator
+- Centralized `@ApplicationScoped` utility for ownership checks
+- Three methods: `validateUserAccess()`, `validateAndResolveUserId()`, `requireOwnerOrAdmin()`
+- All checks are no-ops when `authorization.enabled=false` (dev mode)
+- `eddi-admin` role bypasses all ownership checks
+- Legacy data without ownership (null/blank userId) is allowed through gracefully
+- WARN-level audit logging on all ownership check failures
+
+### Dropped Finding: MCP Unauthenticated by Default
+- **Rationale:** When OIDC is disabled, ALL endpoints are unauthenticated — MCP is not uniquely vulnerable. `AuthStartupGuard` already prevents accidental unauthenticated production deployments. Not a finding.
+
+**Files:** `OwnershipValidator.java` [NEW], `RestAgentEngine.java`, `RestUserMemoryStore.java`, `RestGroupConversation.java`, `IRestGdprAdmin.java`, `RestGdprAdmin.java`, `RestA2AEndpoint.java`, `McpToolUtils.java`, `McpMemoryTools.java`
+
+### Code Review Hardening (2026-06-10)
+
+**Repo:** EDDI (`fix/security-audit-idor-remediation`)
+**What changed:** Addressed all findings from the post-implementation code review.
+
+- **M1 — MCP ownership consolidation:** Removed duplicate `requireOwnerOrAdmin` static method from `McpToolUtils`. `McpMemoryTools` now injects `OwnershipValidator` directly and calls `validateUserAccess()` — single source of truth for ownership logic.
+- **M3 — PII in WARN logs:** `OwnershipValidator` WARN messages no longer include caller/user IDs. Full details are logged at DEBUG level only, reducing compliance risk.
+- **M4 — Narrow catch clause:** `RestAgentEngine.validateConversationOwnership()` now catches `ResourceNotFoundException` and `ResourceStoreException` specifically instead of generic `Exception`, preventing unexpected errors from being silently swallowed.
+- **BUG-2 — deleteMemory ownership:** Added `findEntryById(String entryId)` to `IUserMemoryStore` with MongoDB and PostgreSQL implementations. `RestUserMemoryStore.deleteMemory()` now looks up the entry, validates ownership via `validateUserAccess()`, and returns 404 if not found.
+
+**Files:** `OwnershipValidator.java`, `RestAgentEngine.java`, `RestUserMemoryStore.java`, `McpToolUtils.java`, `McpMemoryTools.java`, `IUserMemoryStore.java`, `MongoUserMemoryStore.java`, `PostgresUserMemoryStore.java`
+
+### Test Coverage for Security Fixes (2026-06-10)
+
+**Repo:** EDDI (`fix/security-audit-idor-remediation`)
+**What changed:** Added 36 new tests covering all security-critical ownership validation logic.
+
+- **OwnershipValidatorTest [NEW]:** 24 tests across 4 nested groups — `validateUserAccess`, `validateAndResolveUserId`, `requireOwnerOrAdmin`, `isAuthEnabled`. Covers auth on/off, admin bypass, legacy null owner, caller mismatch → ForbiddenException.
+- **RestAgentEngineTest — OwnershipValidation:** 5 tests — admin userId override, impersonation rejection, non-owner read/end, descriptor-not-found graceful skip.
+- **RestUserMemoryStoreTest — DeleteMemory:** 3 tests — owner match → 204, not found → 404, non-owner → ForbiddenException.
+- **RestGroupConversationTest — OwnershipValidation:** 4 tests — non-owner read/delete, userId resolution in discuss, list filtering for non-admin.
+- **Existing test fixes:** Updated `RestAgentEngineTest`, `RestGroupConversationTest`, `McpMemoryToolsTest` stubs for new constructor parameters and ownership lookup patterns.
+
+**Total:** 184 security-related tests, 0 failures, 0 errors.
+**Files:** `OwnershipValidatorTest.java` [NEW], `RestAgentEngineTest.java`, `RestUserMemoryStoreTest.java`, `RestGroupConversationTest.java`, `McpMemoryToolsTest.java`
+
+### GitHub Advanced Security / CodeQL Remediation (2026-06-10)
+
+**Repo:** EDDI (`fix/security-audit-idor-remediation`)
+**What changed:** Addressed 12 CodeQL "Log Injection" findings and 5 Copilot validation-order findings from automated PR review.
+
+- **Log Injection — RestAgentEngine:** `validateConversationOwnership()` now sanitizes `conversationId` via `LogSanitizer.sanitize()` before logging.
+- **Log Injection — OwnershipValidator:** All 3 debug-level log statements (`validateUserAccess`, `validateAndResolveUserId`, `requireOwnerOrAdmin`) now sanitize user-provided values (`callerId`, `requestedUserId`, `resourceOwnerId`, `resourceType`) via `LogSanitizer.sanitize()`.
+- **Fail-closed ownership check:** `RestAgentEngine.validateConversationOwnership()` now throws `ForbiddenException` on `ResourceStoreException` instead of silently skipping the ownership check. Previous fail-open behavior could allow unauthorized access during transient DB errors.
+- **MCP validation order:** In `McpMemoryTools`, all 5 read-only tools (`listUserMemories`, `getVisibleMemories`, `searchUserMemories`, `getMemoryByKey`, `countUserMemories`) now validate `userId` is non-null/non-blank **before** calling `ownershipValidator.validateUserAccess()`. Previously, a missing `userId` with auth enabled would throw `ForbiddenException` instead of the intended `"userId is required"` error JSON.
+- **Changelog clarity:** Updated MCP ownership entry (line 32-35) to reflect final state — `OwnershipValidator.validateUserAccess()` is the sole mechanism, not `requireOwnerOrAdmin()` in `McpToolUtils`.
+
+**Files:** `RestAgentEngine.java`, `OwnershipValidator.java`, `McpMemoryTools.java`, `docs/changelog.md`
+
+---
+
 ## 🐛 Fix: Swagger UI Broken by CSP — Per-Path Filter Override (2026-06-03)
 
 **Repo:** EDDI (`fix/swagger-ui-csp`)

src/main/java/ai/labs/eddi/configs/properties/IUserMemoryStore.java

@@ -53,6 +53,15 @@ public interface IUserMemoryStore {
 
     void deleteEntry(String entryId) throws IResourceStore.ResourceStoreException;
 
+    /**
+     * Finds a memory entry by its ID. Used for ownership validation before
+     * deletion.
+     *
+     * @return the entry, or empty if not found
+     * @since 6.1.0
+     */
+    Optional<UserMemoryEntry> findEntryById(String entryId) throws IResourceStore.ResourceStoreException;
+
     // === Queries ===
 
     /**

src/main/java/ai/labs/eddi/configs/properties/mongo/MongoUserMemoryStore.java

@@ -186,6 +186,13 @@ public void deleteEntry(String entryId) throws IResourceStore.ResourceStoreExcep
         memoriesCollection.deleteOne(eq("_id", new ObjectId(entryId)));
     }
 
+    @Override
+    public Optional<UserMemoryEntry> findEntryById(String entryId) throws IResourceStore.ResourceStoreException {
+        RuntimeUtilities.checkNotNull(entryId, "entryId");
+        Document doc = memoriesCollection.find(eq("_id", new ObjectId(entryId))).first();
+        return doc != null ? Optional.of(documentToEntry(doc)) : Optional.empty();
+    }
+
     // === Queries ===
 
     @Override

src/main/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStore.java

@@ -8,6 +8,8 @@
 import ai.labs.eddi.configs.properties.IUserMemoryStore;
 import ai.labs.eddi.configs.properties.model.UserMemoryEntry;
 import ai.labs.eddi.datastore.IResourceStore;
+import ai.labs.eddi.engine.security.OwnershipValidator;
+import io.quarkus.security.identity.SecurityIdentity;
 import org.jboss.logging.Logger;
 
 import jakarta.enterprise.context.ApplicationScoped;
@@ -28,16 +30,23 @@
 public class RestUserMemoryStore implements IRestUserMemoryStore {
 
     private final IUserMemoryStore userMemoryStore;
+    private final SecurityIdentity identity;
+    private final OwnershipValidator ownershipValidator;
 
     private static final Logger LOGGER = Logger.getLogger(RestUserMemoryStore.class);
 
     @Inject
-    public RestUserMemoryStore(IUserMemoryStore userMemoryStore) {
+    public RestUserMemoryStore(IUserMemoryStore userMemoryStore,
+            SecurityIdentity identity,
+            OwnershipValidator ownershipValidator) {
         this.userMemoryStore = userMemoryStore;
+        this.identity = identity;
+        this.ownershipValidator = ownershipValidator;
     }
 
     @Override
     public List<UserMemoryEntry> getAllMemories(String userId) {
+        ownershipValidator.validateUserAccess(identity, userId);
         try {
             return userMemoryStore.getAllEntries(userId);
         } catch (IResourceStore.ResourceStoreException e) {
@@ -48,6 +57,7 @@ public List<UserMemoryEntry> getAllMemories(String userId) {
 
     @Override
     public List<UserMemoryEntry> getVisibleMemories(String userId, String agentId, List<String> groupIds, String recallOrder, int maxEntries) {
+        ownershipValidator.validateUserAccess(identity, userId);
         try {
             return userMemoryStore.getVisibleEntries(userId, agentId, groupIds != null ? groupIds : List.of(), recallOrder, maxEntries);
         } catch (IResourceStore.ResourceStoreException e) {
@@ -58,6 +68,7 @@ public List<UserMemoryEntry> getVisibleMemories(String userId, String agentId, L
 
     @Override
     public List<UserMemoryEntry> searchMemories(String userId, String query) {
+        ownershipValidator.validateUserAccess(identity, userId);
         try {
             return userMemoryStore.filterEntries(userId, query);
         } catch (IResourceStore.ResourceStoreException e) {
@@ -68,6 +79,7 @@ public List<UserMemoryEntry> searchMemories(String userId, String query) {
 
     @Override
     public List<UserMemoryEntry> getMemoriesByCategory(String userId, String category) {
+        ownershipValidator.validateUserAccess(identity, userId);
         try {
             return userMemoryStore.getEntriesByCategory(userId, category);
         } catch (IResourceStore.ResourceStoreException e) {
@@ -78,6 +90,7 @@ public List<UserMemoryEntry> getMemoriesByCategory(String userId, String categor
 
     @Override
     public Response getMemoryByKey(String userId, String key) {
+        ownershipValidator.validateUserAccess(identity, userId);
         try {
             var entry = userMemoryStore.getByKey(userId, key);
             if (entry.isPresent()) {
@@ -105,6 +118,7 @@ public Response upsertMemory(UserMemoryEntry entry) {
         if (entry.key().length() > 255) {
             return Response.status(Response.Status.BAD_REQUEST).entity(Map.of("error", "key must not exceed 255 characters")).build();
         }
+        ownershipValidator.validateUserAccess(identity, entry.userId());
         try {
             String id = userMemoryStore.upsert(entry);
             return Response.ok(Map.of("id", id)).build();
@@ -117,8 +131,15 @@ public Response upsertMemory(UserMemoryEntry entry) {
     @Override
     public Response deleteMemory(String entryId) {
         try {
+            var entry = userMemoryStore.findEntryById(entryId);
+            if (entry.isEmpty()) {
+                throw new NotFoundException("Memory entry not found: " + entryId);
+            }
+            ownershipValidator.validateUserAccess(identity, entry.get().userId());
             userMemoryStore.deleteEntry(entryId);
             return Response.noContent().build();
+        } catch (NotFoundException e) {
+            throw e;
         } catch (IResourceStore.ResourceStoreException e) {
             LOGGER.error("Failed to delete memory entry: " + entryId, e);
             throw new InternalServerErrorException(e.getLocalizedMessage());
@@ -127,6 +148,7 @@ public Response deleteMemory(String entryId) {
 
     @Override
     public Response deleteAllForUser(String userId) {
+        ownershipValidator.validateUserAccess(identity, userId);
         try {
             userMemoryStore.deleteAllForUser(userId);
             return Response.noContent().build();
@@ -138,6 +160,7 @@ public Response deleteAllForUser(String userId) {
 
     @Override
     public Response countMemories(String userId) {
+        ownershipValidator.validateUserAccess(identity, userId);
         try {
             long count = userMemoryStore.countEntries(userId);
             return Response.ok(Map.of("userId", userId, "count", count)).build();

src/main/java/ai/labs/eddi/datastore/postgres/PostgresUserMemoryStore.java

@@ -245,6 +245,23 @@ public void deleteEntry(String entryId) throws IResourceStore.ResourceStoreExcep
         }
     }
 
+    @Override
+    public Optional<UserMemoryEntry> findEntryById(String entryId) throws IResourceStore.ResourceStoreException {
+        ensureSchema();
+        String sql = "SELECT * FROM usermemories WHERE id = ?";
+        try (Connection conn = dataSourceInstance.get().getConnection(); PreparedStatement ps = conn.prepareStatement(sql)) {
+            ps.setString(1, entryId);
+            try (ResultSet rs = ps.executeQuery()) {
+                if (rs.next()) {
+                    return Optional.of(resultSetToEntry(rs));
+                }
+            }
+        } catch (SQLException e) {
+            throw new IResourceStore.ResourceStoreException("Failed to find memory entry by id", e);
+        }
+        return Optional.empty();
+    }
+
     // === Queries ===
 
     @Override

src/main/java/ai/labs/eddi/engine/a2a/RestA2AEndpoint.java

@@ -7,6 +7,7 @@
 import ai.labs.eddi.configs.agents.CapabilityRegistryService;
 import ai.labs.eddi.configs.agents.CapabilityRegistryService.CapabilityMatch;
 import ai.labs.eddi.engine.a2a.A2AModels.*;
+import jakarta.annotation.security.PermitAll;
 import jakarta.inject.Inject;
 import jakarta.ws.rs.*;
 import jakarta.ws.rs.core.MediaType;
@@ -63,6 +64,7 @@ public RestA2AEndpoint(AgentCardService agentCardService, A2ATaskHandler taskHan
     /**
      * Default Agent Card — returns the first A2A-enabled agent's card.
      */
+    @PermitAll
     @GET
     @Path(".well-known/agent.json")
     public Response getDefaultAgentCard() {
@@ -81,6 +83,7 @@ public Response getDefaultAgentCard() {
     /**
      * Per-agent Agent Card.
      */
+    @PermitAll
     @GET
     @Path("a2a/agents/{agentId}/agent.json")
     public Response getAgentCard(@PathParam("agentId") String agentId) {
@@ -99,6 +102,7 @@ public Response getAgentCard(@PathParam("agentId") String agentId) {
     /**
      * List all A2A-enabled agents.
      */
+    @PermitAll
     @GET
     @Path("a2a/agents")
     public Response listA2AAgents() {
@@ -117,6 +121,7 @@ public Response listA2AAgents() {
      * Path follows the well-known URI convention, same auth model as
      * {@code /.well-known/agent.json}.
      */
+    @PermitAll
     @GET
     @Path(".well-known/capabilities")
     @Tag(name = "06. Capability Registry", description = "A2A agent capability discovery")
@@ -145,6 +150,7 @@ public Response searchCapabilities(@QueryParam("skill") String skill,
      * Public endpoint listing all registered skill names. Gated behind
      * {@code eddi.a2a.capabilities.public} (default {@code false}).
      */
+    @PermitAll
     @GET
     @Path(".well-known/capabilities/skills")
     @Tag(name = "06. Capability Registry", description = "A2A agent capability discovery")

src/main/java/ai/labs/eddi/engine/gdpr/IRestGdprAdmin.java

@@ -4,6 +4,7 @@
  */
 package ai.labs.eddi.engine.gdpr;
 
+import jakarta.annotation.security.RolesAllowed;
 import jakarta.ws.rs.*;
 import jakarta.ws.rs.core.MediaType;
 import org.eclipse.microprofile.openapi.annotations.Operation;
@@ -23,6 +24,7 @@
 @Path("/admin/gdpr")
 @Tag(name = "GDPR / Privacy")
 @Produces(MediaType.APPLICATION_JSON)
+@RolesAllowed("eddi-admin")
 public interface IRestGdprAdmin {
 
     @DELETE

src/main/java/ai/labs/eddi/engine/gdpr/RestGdprAdmin.java

@@ -4,7 +4,6 @@
  */
 package ai.labs.eddi.engine.gdpr;
 
-import jakarta.annotation.security.RolesAllowed;
 import jakarta.enterprise.context.ApplicationScoped;
 import jakarta.inject.Inject;
 import jakarta.ws.rs.BadRequestException;
@@ -20,7 +19,6 @@
  * @since 6.0.0
  */
 @ApplicationScoped
-@RolesAllowed("eddi-admin")
 public class RestGdprAdmin implements IRestGdprAdmin {
 
     private static final Logger LOGGER = Logger.getLogger(RestGdprAdmin.class);