Conversation
Summary of ChangesHello @1000hyehyang, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 Pull Request는 캠페인 제안 관련 이벤트 처리 로직을 개선하고 채팅방 생성 기능을 명확하게 분리하여 시스템의 견고성과 유연성을 높입니다. 특히, 캠페인 제안 이벤트에 행위자 정보를 추가하고, 채팅방 생성 시 사용자 권한 검증 여부에 따라 다른 메서드를 사용하도록 재설계함으로써, 이벤트 기반의 자동 채팅방 생성과 사용자 요청에 의한 채팅방 생성을 구분합니다. 또한, 트랜잭션 후 실행 로직의 안정성을 향상시켜 예상치 못한 오류를 줄였습니다. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR aims to improve campaign proposal event processing and chat room creation logic. However, a high-severity broken access control vulnerability was identified. The new createOrGetRoomSystem method bypasses authorization checks, which, when used by event listeners, could allow an attacker to create chat rooms between arbitrary users if they can trigger the corresponding application events. This poses a significant privacy risk and requires remediation by ensuring authorization is performed in all chat room creation paths. On the positive side, the refactoring clarifies the proposal subject by adding actorUserId to CampaignProposalSentEvent, impressively separates ChatRoomCommandService for user requests and system events, and enhances stability by using Propagation.REQUIRES_NEW for transaction isolation in event listeners. Additionally, a suggestion to extract duplicate validation logic, aligning with best practices for maintainability, was noted. Overall, code clarity and stability have greatly improved, but the security vulnerability needs immediate attention.
| @Override | ||
| @Transactional(propagation = Propagation.REQUIRES_NEW) | ||
| public ChatRoomCreateResponse createOrGetRoomSystem(Long brandId, Long creatorId) { | ||
| validateSystemRequest(brandId, creatorId); | ||
| return findOrCreateRoom(brandId, creatorId); | ||
| } |
There was a problem hiding this comment.
The new method createOrGetRoomSystem is explicitly designed to bypass authorization checks, as confirmed by its implementation of validateSystemRequest which only performs basic validation, not permission verification. This method is intended for internal system/event use, but it creates a dangerous backdoor for creating chat rooms. Any part of the application that can call this method can create chat rooms between arbitrary users. This violates the principle of least privilege and secure design. The previous implementation enforced that the user creating the room was a member of that room.
| private Long ensureRoomAndGetId(Long brandUserId, Long creatorUserId) { | ||
| return chatRoomCommandService | ||
| .createOrGetRoom(brandUserId, brandUserId, creatorUserId) | ||
| .createOrGetRoomSystem(brandUserId, creatorUserId) | ||
| .roomId(); |
There was a problem hiding this comment.
The ensureRoomAndGetId method was changed to use createOrGetRoomSystem, which, as noted in the comments, bypasses authorization checks. This method is called by the handleCampaignApplySent event listener. The listener trusts the brandUserId and creatorUserId from the CampaignApplySentEvent without verifying if the user who triggered the event is authorized to create a chat room for those users. This could allow an attacker who finds a way to publish a malicious CampaignApplySentEvent to create chat rooms between arbitrary users, leading to a breach of privacy. The previous implementation correctly passed the actor's userId to enforce authorization.
| private Long ensureRoomAndGetId(CampaignProposalSentEvent event) { | ||
| return chatRoomCommandService | ||
| .createOrGetRoom(brandUserId, brandUserId, creatorUserId) | ||
| .createOrGetRoomSystem(event.brandUserId(), event.creatorUserId()) | ||
| .roomId(); |
There was a problem hiding this comment.
The ensureRoomAndGetId method uses createOrGetRoomSystem, which bypasses authorization checks, based on data from a CampaignProposalSentEvent. Although the event was updated to include an actorUserId, this ID is not used to perform any authorization checks before creating the chat room. The listener implicitly trusts the event's content. This could allow an attacker who finds a way to publish a malicious CampaignProposalSentEvent to create chat rooms between arbitrary users, leading to a breach of privacy.
| private Long ensureRoomAndGetId(CampaignProposalSentEvent event) { | |
| return chatRoomCommandService | |
| .createOrGetRoom(brandUserId, brandUserId, creatorUserId) | |
| .createOrGetRoomSystem(event.brandUserId(), event.creatorUserId()) | |
| .roomId(); | |
| private Long ensureRoomAndGetId(CampaignProposalSentEvent event) { | |
| return chatRoomCommandService | |
| .createOrGetRoomAsMember(event.actorUserId(), event.brandUserId(), event.creatorUserId()) | |
| .roomId(); | |
| } |
| private void validateMemberRequest(Long userId, Long brandId, Long creatorId) { | ||
| if (brandId == null || creatorId == null || brandId.equals(creatorId)) { | ||
| throw new CustomException(ChatErrorCode.INVALID_ROOM_REQUEST); | ||
| } | ||
| if (!userId.equals(brandId) && !userId.equals(creatorId)) { | ||
| throw new CustomException(ChatErrorCode.NOT_ROOM_MEMBER); | ||
| } | ||
| } | ||
|
|
||
| private void validateSystemRequest(Long brandId, Long creatorId) { | ||
| if (brandId == null || creatorId == null || brandId.equals(creatorId)) { | ||
| throw new CustomException(ChatErrorCode.INVALID_ROOM_REQUEST); | ||
| } | ||
| } |
There was a problem hiding this comment.
validateMemberRequest와 validateSystemRequest 메서드에 중복된 유효성 검사 로직이 있습니다. 이 공통 로직을 별도의 private 메서드로 추출하여 중복을 제거하고 코드의 유지보수성을 높이는 것을 제안합니다.
private void validateRoomParticipants(Long brandId, Long creatorId) {
if (brandId == null || creatorId == null || brandId.equals(creatorId)) {
throw new CustomException(ChatErrorCode.INVALID_ROOM_REQUEST);
}
}
private void validateMemberRequest(Long userId, Long brandId, Long creatorId) {
validateRoomParticipants(brandId, creatorId);
if (!userId.equals(brandId) && !userId.equals(creatorId)) {
throw new CustomException(ChatErrorCode.NOT_ROOM_MEMBER);
}
}
private void validateSystemRequest(Long brandId, Long creatorId) {
validateRoomParticipants(brandId, creatorId);
}References
- Extract duplicate validation logic into a separate component (e.g., a validator class) to reduce code duplication and improve maintainability.
1 participant
Summary
Changes
Type of Change
Related Issues
참고 사항