Merged
Conversation
- useSSEConnect: SSE 연결 및 수신 Data 상태 저장만 담당 - useSSEEvent: data를 이용한 이벤트 처리만 담당 - NotificationProvider: useEffect의 connect, disconnect는 useSSEConnect 훅으로 이관
Contributor
WalkthroughSSE 연결 로직을 재구조화하여 단일 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎭 Playwright Report✨ E2E Test가 성공적으로 완료되었습니다. Test 요약 내용을 확인해주세요.
📊 Test Summary
📜 Test Details✅ Passed Tests (3)
|
📊 Coverage Report
📈 #391을 main에 병합하면 coverage가 Coverage 요약@@ Coverage Diff @@
## main #391 +/- ##
===========================================
+ Coverage 35.40% 35.58% +0.18%
===========================================
Files 262 264 +2
Lines 12119 12116 -3
Branches 467 467 0
===========================================
+ Hits 4291 4312 +21
- Misses 7828 7804 -24 영향받은 파일
|
🚀 PR Preview Report✨ Build가 성공적으로 완료되었습니다. Preview에서 변경사항을 확인하세요.
|
🎨 Storybook Report✅ 변경 사항이 없습니다 모든 Story가 이전 빌드와 동일합니다.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/hooks/use-sse/use-sse-connect/index.ts (1)
15-92: 프로덕션 배포 전[DEBUG]로그 제거 필요파일 전체에 걸쳐
[DEBUG]접두사가 붙은console.log/console.error구문이 다수 존재합니다(Line 18, 28, 38, 47, 56, 62, 71, 80, 82, 88). 프로덕션 환경에서 민감한 SSE 연결 상태 정보가 콘솔에 노출됩니다. 제거하거나 구조화된 로깅 유틸리티로 교체하는 것을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/use-sse/use-sse-connect/index.ts` around lines 15 - 92, Replace all raw console.log/console.error calls in connect, disconnect, reconnect, and setupSSEConnection with production-safe logging: either remove the “[DEBUG]” messages or route them through the app’s structured logger (e.g., logger.debug/info/error) and guard verbose debug logs behind an environment check (NODE_ENV !== 'production' or a feature flag). Update uses around eventSourceRef and retryRefreshRef (all console.* occurrences inside connect, disconnect, reconnect, and setupSSEConnection) so no sensitive SSE state is printed in production; ensure error paths still record enough context via the structured logger.src/hooks/use-sse/use-sse-event/index.ts (1)
28-37:queryClient를useEffect의존성 배열에 추가하는 것을 고려하세요.
queryClient가 effect 내부에서 사용되지만 의존성 배열에 누락되어react-hooks/exhaustive-deps린트 경고가 발생합니다.@tanstack/react-queryv5에서queryClient는 안정적인 참조이므로 동작에는 문제없으나, 관례상 포함하는 것이 권장됩니다.♻️ 제안: 의존성 배열에 queryClient 추가
- }, [data]); + }, [data, queryClient]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/use-sse/use-sse-event/index.ts` around lines 28 - 37, The useEffect uses queryClient (and SSE_INVALIDATION_MAP, notificationKeys, data) but omits queryClient from its dependency array, causing a react-hooks/exhaustive-deps lint warning; update the useEffect dependency array to include queryClient (e.g., change [data] to [data, queryClient]) so the effect declares all external references used inside (no other behavioral changes required since QueryClient is stable in `@tanstack/react-query` v5).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/use-sse/use-sse-connect/index.ts`:
- Around line 87-91: The onerror handler for the EventSource (es.onerror) lacks
a guard for retryRefreshRef.current, which allows infinite reconnect loops;
update the es.onerror callback to check retryRefreshRef.current before calling
reconnect() (e.g., if (!retryRefreshRef.current) { reconnect(); }) so you only
trigger the reconnect path when allowed; locate es.onerror in use-sse-connect's
setupSSEConnection and reference retryRefreshRef and reconnect() to implement
the guard and preserve the existing console log and es.close() behavior.
- Around line 36-50: The reconnect function (reconnect) can resume after await
when the component may have unmounted; fix by checking the sentinel
retryRefreshRef.current immediately after the await API.authService.refresh()
(and before calling setupSSEConnection) and aborting if it has been cleared
(false), to avoid creating an EventSource for an unmounted component; also
ensure you still call disconnect() inside the catch path and only call
setupSSEConnection(token) when token exists and retryRefreshRef.current is true.
- Around line 63-65: The EventSource instantiation in use-sse-connect is leaking
the access token via URL query; remove the accessToken query parameter from the
EventSource URL and create the EventSource with credentials enabled so the
HttpOnly cookie is sent automatically (update the EventSource call in
use-sse-connect/index.ts where the EventSource is constructed to call the
subscribe endpoint without the token query and pass { withCredentials: true } as
the second argument).
---
Nitpick comments:
In `@src/hooks/use-sse/use-sse-connect/index.ts`:
- Around line 15-92: Replace all raw console.log/console.error calls in connect,
disconnect, reconnect, and setupSSEConnection with production-safe logging:
either remove the “[DEBUG]” messages or route them through the app’s structured
logger (e.g., logger.debug/info/error) and guard verbose debug logs behind an
environment check (NODE_ENV !== 'production' or a feature flag). Update uses
around eventSourceRef and retryRefreshRef (all console.* occurrences inside
connect, disconnect, reconnect, and setupSSEConnection) so no sensitive SSE
state is printed in production; ensure error paths still record enough context
via the structured logger.
In `@src/hooks/use-sse/use-sse-event/index.ts`:
- Around line 28-37: The useEffect uses queryClient (and SSE_INVALIDATION_MAP,
notificationKeys, data) but omits queryClient from its dependency array, causing
a react-hooks/exhaustive-deps lint warning; update the useEffect dependency
array to include queryClient (e.g., change [data] to [data, queryClient]) so the
effect declares all external references used inside (no other behavioral changes
required since QueryClient is stable in `@tanstack/react-query` v5).
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app/providers.tsxsrc/hooks/use-notification/use-notification-connect-sse/index.tssrc/hooks/use-sse/index.tssrc/hooks/use-sse/use-sse-connect/index.tssrc/hooks/use-sse/use-sse-event/index.tssrc/providers/provider-notification/index.tsxsrc/types/service/notification.ts
💤 Files with no reviewable changes (1)
- src/hooks/use-notification/use-notification-connect-sse/index.ts
Comment on lines
+63
to
+65
| const es = new EventSource( | ||
| `${process.env.NEXT_PUBLIC_API_BASE_URL}/api/v1/notifications/subscribe?accessToken=${token}`, | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
EventSource API custom headers support authentication best practices
💡 Result:
Does the browser EventSource API support custom headers?
No. The native browser EventSource(url, { withCredentials }) does not let you set arbitrary request headers like Authorization. This is a known/long-standing spec + implementation limitation. [1][2]
Authentication best practices for SSE when you can’t set headers
- Prefer cookie-based auth with
HttpOnlysession cookies (and usewithCredentials: truewhen you need cookies on cross-origin SSE). This is the main thing nativeEventSourcesupports beyond the URL. [2][3] - Avoid putting access tokens in the URL query string (
/sse?token=...) because URLs get logged/cached and can leak via referrers/history. [4] - If you use cookies, treat SSE like any other credentialed request:
- Use TLS everywhere.
- Lock down CORS (specific origins,
Access-Control-Allow-Credentials: trueonly when needed). - Mitigate CSRF (e.g.,
SameSitecookies, CSRF tokens on state-changing endpoints; SSE itself is usually GET/read-only, but your overall session still matters).
If you need Authorization: Bearer ... (custom headers)
Use a fetch-based SSE client (recommended) or a polyfill:
- Fetch-based SSE client (recommended):
@microsoft/fetch-event-source(or equivalents) lets you send headers/body/method because it’s built onfetch(). [3] - Polyfill approach:
event-source-polyfill/Yaffle’s EventSource polyfill supports custom headers via an init option. [5]
Token lifecycle best practice (important for long-lived streams)
Even with header-capable clients, plan for token expiry/refresh: long-lived connections + short-lived JWTs require a strategy (reconnect with a fresh token, or prefer session cookies). [2]
Sources: WHATWG html issue on headers [1], StackOverflow confirmation of native limitation + cookie suggestion [2], @microsoft/fetch-event-source README (custom headers via fetch) [3], discussion of URL token leakage risk [4], EventSource polyfill custom headers example [5].
🏁 Script executed:
# Check the implementation context
cat -n src/hooks/use-sse/use-sse-connect/index.ts | head -80Repository: WeGo-Together/WeGo_FrontEnd
Length of output: 2780
EventSource 요청에서 accessToken을 URL 쿼리 파라미터로 전달하지 마세요.
토큰이 이미 HttpOnly 쿠키에 저장되어 있으므로, 다시 추출하여 URL에 노출하면 안 됩니다. 쿼리 파라미터로 전달된 토큰은 다음과 같이 유출될 수 있습니다:
- 서버 액세스 로그에 기록
- 브라우저 히스토리에 저장
Referer헤더를 통해 제3자에게 유출
해결 방법: EventSource 생성 시 withCredentials: true 옵션을 추가하여 브라우저가 자동으로 쿠키를 전송하도록 변경하고, URL에서 accessToken 파라미터를 제거하세요.
const es = new EventSource(
`${process.env.NEXT_PUBLIC_API_BASE_URL}/api/v1/notifications/subscribe`,
{ withCredentials: true }
);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/use-sse/use-sse-connect/index.ts` around lines 63 - 65, The
EventSource instantiation in use-sse-connect is leaking the access token via URL
query; remove the accessToken query parameter from the EventSource URL and
create the EventSource with credentials enabled so the HttpOnly cookie is sent
automatically (update the EventSource call in use-sse-connect/index.ts where the
EventSource is constructed to call the subscribe endpoint without the token
query and pass { withCredentials: true } as the second argument).
Member
Author
There was a problem hiding this comment.
추후 백엔드 요청하여 수정진행하겠습니다. 이슈 생성 부탁드립니다.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
@Chiman2937, 네, 이슈를 생성해드리겠습니다.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the SSE token security concern
gh issue create \
--title "SSE 연결에서 accessToken URL 노출 보안 문제 해결" \
--body "## 문제 설명
현재 SSE(Server-Sent Events) 연결 설정 시 accessToken을 URL 쿼리 파라미터로 전달하고 있습니다. 이는 다음과 같은 보안 위험을 초래합니다:
- 서버 액세스 로그에 토큰 기록
- 브라우저 히스토리에 토큰 저장
- Referer 헤더를 통한 제3자 유출 가능성
## 해결 방안
백엔드에서 EventSource 연결 시 HttpOnly 쿠키를 통한 인증을 지원하도록 수정이 필요합니다. 프론트엔드에서는 \`withCredentials: true\` 옵션을 사용하여 쿠키가 자동으로 전송되도록 설정할 수 있습니다.
## 관련 정보
- PR: https://github.com/WeGo-Together/WeGo_FrontEnd/pull/391
- 리뷰 코멘트: https://github.com/WeGo-Together/WeGo_FrontEnd/pull/391#discussion_r2840385123
- 요청자: `@Chiman2937`
- 파일: \`src/hooks/use-sse/use-sse-connect/index.ts\`" \
--assignee "Chiman2937"
Length of output: 57
@Chiman2937, 이슈를 생성했습니다: #392
백엔드 수정 요청 후 진행하시면 됩니다. 감사합니다!
Chiman2937
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 변경 사항
알림 SSE 훅 리팩토링 변경사항
커밋 히스토리
5a070524854e4c97472815fe306d822081e1. useConnectSSE 역할 분리
기존
useConnectSSE훅이 SSE 연결 관리와 알림 이벤트 처리(쿼리 무효화)를 모두 담당하고 있었으나, 이를 연결 전용(useSSEConnect)과 이벤트 처리 전용(useSSEEvent) 두 훅으로 분리.Before - 단일 훅에서 모든 역할 수행
After - 역할별 분리
useSSEConnect{ data: NotificationItem | null }useSSEEventuseSSEConnect 변경점
isAuthenticated체크 로직 제거 → 연결 관리에만 집중hasRefreshToken파라미터 제거receivedNewNotification상태 →data상태로 변경 (수신된NotificationItem자체를 관리)connect()/disconnect()를 외부에 노출하지 않고 내부useEffect에서 자동 관리useSSEEvent - 쿼리 무효화 로직을 선언적 Map으로 전환
기존
switch-case구문을SSE_INVALIDATION_MAP객체로 변환하여 선언적으로 관리.2. Notification Type 간소화
src/types/service/notification.ts에서 Discriminated Union 패턴을 제거하고 단일NotificationItem인터페이스로 통합.Before - Discriminated Union (3개 타입)
After - 단일 인터페이스
3. NotificationProvider 간소화
src/providers/provider-notification/index.tsx에서 분리된 훅을 사용하도록 변경.변경점
hasRefreshTokenprops 제거connect()/disconnect()수동 호출 제거 →useSSEConnect내부에서 자동 관리useEffect제거 → SSE 연결 생명주기가 훅 내부로 캡슐화됨4. useSSEConnect 안정성 개선
SSE 연결 오류 시 재연결 로직에 가드를 추가하여 안정성 개선.
onError retry 가드 추가
이미 retry 중일 때
onerror가 다시 발생해도reconnect()가 중복 실행되지 않도록 가드 추가.언마운트 가드 추가
컴포넌트 언마운트 이후 비동기
reconnect()가 완료되어setupSSEConnection()이 호출되는 것을 방지하기 위해isMountedRef가드 추가.변경된 파일 목록
src/hooks/use-notification/use-notification-connect-sse/index.tssrc/hooks/use-sse/index.tssrc/hooks/use-sse/use-sse-connect/index.tssrc/hooks/use-sse/use-sse-event/index.tssrc/providers/provider-notification/index.tsxsrc/app/providers.tsxsrc/types/service/notification.ts🔗 관련 이슈
Closes #
🧪 테스트 방법
📸 스크린샷 (선택)
📋 체크리스트
💬 추가 코멘트
CodeRabbit Review는 자동으로 실행되지 않습니다.
Review를 실행하려면 comment에 아래와 같이 작성해주세요
Summary by CodeRabbit
Release Notes