[REFACTOR] Redisearch로 매칭 알고리즘 리팩토링#374
Conversation
Summary of ChangesHello @Yoonchulchung, 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! 이 PR은 기존에 애플리케이션 서버에서 처리하던 매칭 로직을 Redisearch를 활용하여 Redis에서 직접 연산을 수행하도록 변경하는 대규모 리팩토링을 포함합니다. 이를 통해 매칭 프로세스의 효율성을 높이고, 사용자 유형을 더욱 정교하게 분류하는 새로운 알고리즘을 도입하여 개인화된 매칭 경험을 제공하는 데 기여합니다. 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
|
There was a problem hiding this comment.
Code Review
This PR is a great refactoring that moves the matching logic from the application server to RediSearch to significantly improve performance, and it also introduces new user type determination logic. The RediSearch client implementation is robust, including a fallback mechanism, and its query construction appears safe from injection. However, a significant security issue was identified in MatchServiceImpl: the match method lacks authorization checks on the userId parameter, allowing for Insecure Direct Object Reference (IDOR). This could enable an attacker to access or modify matching data for other users, so it is highly recommended to add ownership verification for the userId before proceeding with the match operation. Additionally, an inconsistency in the test data generator was noted. Some refactoring suggestions for code maintainability have been identified but are noted as potential future tasks, aligning with the scope of this already complex refactoring.
I am having trouble creating individual review comments. Click here to see my feedback.
src/main/java/com/example/RealMatch/match/application/service/MatchServiceImpl.java (131-133)
The match method uses the userId parameter to access and modify user-specific data without verifying that the userId belongs to the currently authenticated user. This is an Insecure Direct Object Reference (IDOR) vulnerability. An attacker could potentially call this method with an arbitrary userId to view another user's matching results (like their nickname) and, more critically, modify their matching details, SNS URL, and tags (via the replaceUserMatchingDetailAndTags call, which also uses the unvalidated userId).
To remediate this, implement an authorization check to ensure that the userId provided in the request matches the ID of the authenticated user. Alternatively, retrieve the user ID directly from the security context instead of accepting it as a parameter.
data/generators/redis_matching_generator.py (389)
인덱스 스키마에 preferredContentsAverageViewsTags 필드가 정의되어 있지만, generate_brand_documents와 generate_campaign_documents 메서드에서 이 필드를 생성하는 로직이 누락되었습니다. 이로 인해 MatchScoreCalculator에서 매칭 점수가 의도와 다르게 계산될 수 있습니다.
데이터 생성 로직에 해당 필드를 추가하는 것을 권장합니다. 예를 들어, 다음과 같이 추가할 수 있습니다:
# in generate_brand_documents and generate_campaign_documents
...
views_tags = self._get_random_tag_ids(['영상 조회수'], 1, 2)
doc = {
...
'preferredContentsAverageViewsTags': self._to_str_list(views_tags),
...
}
Summary
기존 Redis를 캐싱 형태로 사용하여, 모든 연산을 어플리케이션 서버에서 진행하던 것을 Redisearch를 이용해
Redis에서 연산을 수행하도록 로직을 수정했습니다.
Changes
Type of Change
Related Issues
#372