Conversation
feat: 영상 댓글 목록 조회 API 구현 (#351)
Summary of ChangesHello @hdg0116, 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은 특정 영상에 대한 모든 댓글을 조회하는 새로운 기능을 백엔드에 도입합니다. 이를 통해 사용자는 영상에 달린 모든 댓글을 한눈에 볼 수 있게 되며, 기존의 타임스탬프 기반 댓글 조회 외에 더 포괄적인 댓글 접근 방식을 제공합니다. 새로운 API 엔드포인트, 관련 서비스 및 레포지토리 로직, 그리고 상세한 API 문서가 포함되어 기능의 완전성을 보장합니다. 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 introduces an API to retrieve all comments for a video. However, it introduces a high-severity Broken Access Control (IDOR) vulnerability because the implementation lacks proper authorization checks for the new endpoint /videos/:videoId/comments/all. While authentication is required, it doesn't verify if the user is authorized to view the comments. Additionally, there are opportunities to improve code clarity and reduce duplication, specifically regarding videoId validation in comment.service.js and the naming/location of DTOs in comment.controller.js. Minor adjustments for Swagger documentation consistency are also noted.
| export const getAllVideoComments = async ({ videoId }) => { | ||
| let vid; | ||
| try { | ||
| vid = BigInt(videoId); | ||
| } catch { | ||
| throw new InvalidParameterError({ videoId }, "videoId가 올바르지 않습니다."); | ||
| } | ||
|
|
||
| if (vid <= 0n) { | ||
| throw new InvalidParameterError({ videoId }, "videoId가 올바르지 않습니다."); | ||
| } | ||
|
|
||
| const videoExists = await findVideoByIdWithProject(vid); | ||
| if (!videoExists) { | ||
| throw new VideoNotFoundError({ videoId: String(videoId) }); | ||
| } | ||
|
|
||
| return findAllVideoComments({ videoId: vid }); | ||
| }; |
There was a problem hiding this comment.
This section of the getAllVideoComments service lacks a crucial authorization check. While it verifies video existence, it fails to confirm if the requesting user has permission to view the comments for the given videoId. This creates a high-severity Broken Access Control (IDOR) vulnerability, allowing any authenticated user to retrieve comments for any video. To fix this, you should pass the userId from the controller and verify ownership or access rights, similar to how getSlideComments is implemented. Additionally, the logic for parsing and validating videoId (lines 213-222) and checking video existence (lines 224-227) is duplicated across functions like createVideoComment and getVideoCommentsByTimestamp. Extracting this into a reusable helper function, such as validateAndFindVideo(videoId), would improve consistency and maintainability.
| const { videoId } = req.params; | ||
|
|
||
| const comments = await getAllVideoComments({ videoId }); |
There was a problem hiding this comment.
The getAllVideoCommentsController does not pass the authenticated user's ID to the service layer for authorization. This contributes to an Insecure Direct Object Reference (IDOR) vulnerability where any logged-in user can access comments for any video.
| * unauthorized: | ||
| * value: | ||
| * resultType: FAILURE | ||
| * error: | ||
| * errorCode: A004 | ||
| * reason: 인증 세션 정보가 없거나 유효하지 않습니다. | ||
| * data: null | ||
| * success: null |
There was a problem hiding this comment.
Swagger 문서에서 401 오류 응답 예시의 resultType과 errorCode 값이 다른 오류 예시들과 형식이 일치하지 않습니다. 스키마 정의 및 다른 예시와 일관성을 맞추기 위해 문자열로 (따옴표로 감싸서) 수정하는 것이 좋겠습니다.
| * unauthorized: | |
| * value: | |
| * resultType: FAILURE | |
| * error: | |
| * errorCode: A004 | |
| * reason: 인증 세션 정보가 없거나 유효하지 않습니다. | |
| * data: null | |
| * success: null | |
| unauthorized: | |
| value: | |
| resultType: "FAILURE" | |
| error: | |
| errorCode: "A004" | |
| reason: 인증 세션 정보가 없거나 유효하지 않습니다. | |
| data: null | |
| success: null |
| res.status(200).json({ | ||
| resultType: "SUCCESS", | ||
| error: null, | ||
| success: getShareCommentsResponseDTO({ |
There was a problem hiding this comment.
getShareCommentsResponseDTO 함수가 사용되고 있습니다. 이 함수의 기능은 적합하지만, 이름과 shareLink.dto.js라는 위치는 공유 기능에 특화된 것으로 보입니다. 일반적인 댓글 조회 엔드포인트에서 이 함수를 사용하면 향후 유지보수 시 혼란을 줄 수 있습니다. getShareCommentsResponseDTO와 shareCommentItemDTO를 comment.dto.js와 같은 보다 일반적인 DTO 파일로 옮기고, formatCommentsForResponseDTO처럼 범용적인 이름으로 변경하는 것을 고려해 보세요. 이렇게 하면 코드의 명확성과 구조가 개선될 것입니다.
1 participant
🔗 관련 이슈