Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/
public record CampaignProposalSentEvent(
Long proposalId,
Long actorUserId,
Long brandUserId,
Long creatorUserId,
Long campaignId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,7 @@ private void publishProposalSentEvent(CampaignProposal proposal, boolean isRePro

CampaignProposalSentEvent event = new CampaignProposalSentEvent(
proposal.getId(),
proposal.getSenderUserId(),
brandUserId,
creatorUserId,
campaignId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,11 @@ public void handleCampaignApplySent(CampaignApplySentEvent event) {

/**
* 채팅방이 없으면 생성하고, roomId를 반환합니다.
* 이 리스너는 AFTER_COMMIT 컨텍스트에서 실행되므로 createOrGetRoom이 별도 트랜잭션으로 처리됩니다.
* 이벤트 기반 자동 생성이므로 createOrGetRoomSystem 사용 (권한 검증 없음).
*/
private Long ensureRoomAndGetId(Long brandUserId, Long creatorUserId) {
return chatRoomCommandService
.createOrGetRoom(brandUserId, brandUserId, creatorUserId)
.createOrGetRoomSystem(brandUserId, creatorUserId)
.roomId();
Comment on lines 55 to 58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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.

}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public void handleCampaignProposalSent(CampaignProposalSentEvent event) {
event.proposalId(), event.isReProposal());

// 채팅방이 없으면 생성
Long roomId = ensureRoomAndGetId(event.brandUserId(), event.creatorUserId());
Long roomId = ensureRoomAndGetId(event);
ChatProposalCardPayloadResponse payload = createPayload(event);
String eventId = ProposalSentEvent.generateEventId(event.proposalId(), event.isReProposal());

Expand Down Expand Up @@ -80,11 +80,11 @@ private ChatProposalDecisionStatus toChatDecisionStatus(ProposalStatus proposalS

/**
* 채팅방이 없으면 생성하고, roomId를 반환합니다.
* 이 리스너는 AFTER_COMMIT 컨텍스트에서 실행되므로 createOrGetRoom이 별도 트랜잭션으로 처리됩니다.
* 이벤트 기반 자동 생성이므로 createOrGetRoomSystem 사용 (권한 검증 없음).
*/
private Long ensureRoomAndGetId(Long brandUserId, Long creatorUserId) {
private Long ensureRoomAndGetId(CampaignProposalSentEvent event) {
return chatRoomCommandService
.createOrGetRoom(brandUserId, brandUserId, creatorUserId)
.createOrGetRoomSystem(event.brandUserId(), event.creatorUserId())
.roomId();
Comment on lines +85 to 88
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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.

Suggested change
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();
}

}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,14 @@
import com.example.RealMatch.chat.presentation.dto.response.ChatRoomCreateResponse;

public interface ChatRoomCommandService {
ChatRoomCreateResponse createOrGetRoom(Long userId, Long brandId, Long creatorId);

/**
* 사용자 요청 시 사용. 요청자(userId)가 brandId 또는 creatorId일 때만 허용
*/
ChatRoomCreateResponse createOrGetRoomAsMember(Long userId, Long brandId, Long creatorId);

/**
* 이벤트/시스템에서 사용. 권한 검증 없음.
*/
ChatRoomCreateResponse createOrGetRoomSystem(Long brandId, Long creatorId);
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import org.slf4j.LoggerFactory;
import org.springframework.dao.DataIntegrityViolationException;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Propagation;
import org.springframework.transaction.annotation.Transactional;

import com.example.RealMatch.chat.application.cache.ChatCacheInvalidationService;
Expand Down Expand Up @@ -35,9 +36,34 @@ public class ChatRoomCommandServiceImpl implements ChatRoomCommandService {

@Override
@Transactional
public ChatRoomCreateResponse createOrGetRoom(Long userId, Long brandId, Long creatorId) {
validateRequest(userId, brandId, creatorId);
public ChatRoomCreateResponse createOrGetRoomAsMember(Long userId, Long brandId, Long creatorId) {
validateMemberRequest(userId, brandId, creatorId);
return findOrCreateRoom(brandId, creatorId);
}

@Override
@Transactional(propagation = Propagation.REQUIRES_NEW)
public ChatRoomCreateResponse createOrGetRoomSystem(Long brandId, Long creatorId) {
validateSystemRequest(brandId, creatorId);
return findOrCreateRoom(brandId, creatorId);
}
Comment on lines +44 to +49
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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 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);
}
}
Comment on lines +51 to +64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

validateMemberRequestvalidateSystemRequest 메서드에 중복된 유효성 검사 로직이 있습니다. 이 공통 로직을 별도의 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
  1. Extract duplicate validation logic into a separate component (e.g., a validator class) to reduce code duplication and improve maintainability.


private ChatRoomCreateResponse findOrCreateRoom(Long brandId, Long creatorId) {
String roomKey = ChatRoomKeyGenerator.createDirectRoomKey(brandId, creatorId);

ChatRoom room = chatRoomRepository.findByRoomKey(roomKey).orElse(null);
Expand All @@ -52,15 +78,6 @@ public ChatRoomCreateResponse createOrGetRoom(Long userId, Long brandId, Long cr
);
}

private void validateRequest(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 ChatRoom createRoomWithMembers(
String roomKey,
Long brandId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,12 @@ public void afterCommit() {
return;
}

String message = "AfterCommitExecutor must be used within an active transaction. " +
"hasTransaction=%s, hasSynchronization=%s".formatted(hasTransaction, hasSynchronization);
LOG.error(message);
throw new IllegalStateException(message);
LOG.warn("[AfterCommitExecutor] No active transaction. Executing immediately. hasTransaction={}, hasSynchronization={}",
hasTransaction, hasSynchronization);
try {
task.run();
} catch (Exception ex) {
LOG.error("Exception occurred in fallback task execution (no transaction).", ex);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public CustomResponse<ChatRoomCreateResponse> createOrGetRoom(
Long userId = user.getUserId();
Long brandId = request.brandId();
Long creatorId = request.creatorId();
return CustomResponse.ok(chatRoomCommandService.createOrGetRoom(userId, brandId, creatorId));
return CustomResponse.ok(chatRoomCommandService.createOrGetRoomAsMember(userId, brandId, creatorId));
}

@GetMapping("/rooms")
Expand Down
Loading