fix(chat-history): only delete message file for owned sessions - #2043
Conversation
deleteSessionLocal removed the session from the user's list (no-op if not owned) but then unconditionally Files.deleteIfExists the message file, so a caller could delete another user's file by session id. Only delete when removeIf actually removed an owned session. Fixes OtterMind#2034 Co-Authored-By: Claude <noreply@anthropic.com>
openai0229
left a comment
There was a problem hiding this comment.
Reviewed the ownership guard and added a focused regression test covering non-owner and owner deletion behavior. The security issue is addressed and the test passes locally.
|
Maintainer follow-up: I added a focused regression test on the contributor branch. It creates an owner session and message, verifies a different user cannot delete the message file or session data, then verifies the owner can delete it. Local verification: |
|
Additional maintainer review found the same ownership gap on the write path: the HTTP adapter supplies the authenticated user ID, but accepts a client-provided session ID, and |
openai0229
left a comment
There was a problem hiding this comment.
Final review after maintainer hardening and current-main CI. Session ownership is now enforced for reads, writes, and deletion; focused tests cover non-owner write/delete behavior, owner deletion, and Spring constructor selection. All checks, including Java CodeQL, passed.
Related issue
Closes #2034
Summary
AiChatHistoryServiceImpl.deleteSessionLocalcalledsessions.removeIf(s -> Objects.equals(s.getId(), sessionId))against the current user's session list — a no-op if the session does not belong to this user — then unconditionallyFiles.deleteIfExists(messagesPath(sessionId))wheremessagesPathisbaseDir.resolve(sessionId + ".json"). A user could therefore delete another user's chat message file by supplying that session's id, regardless of ownership. Now the message file is deleted only whenremoveIfactually removed the session (i.e. it was owned by the requesting user).Affected surfaces
Verification
mvn -B -q -f chat2db-community-server/pom.xml -pl chat2db-community-domain/chat2db-community-domain-core -am -Dmaven.test.skip=true compile-> BUILD SUCCESS.List.removeIfreturnstrueiff the list changed (a session with that id belonged to the user); when it returnsfalse, the method returns without touching the filesystem. The owned path is unchanged.Risk and compatibility
Reviewer map
AiChatHistoryServiceImpl.deleteSessionLocal— captureboolean removed = sessions.removeIf(...)andif (!removed) return;before deleting the message file.sessionId.jsonby supplying that id.Contributor declaration
AI assistance: The fix, verification, and PR description were produced with Claude Code assistance.