Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughGoogle OAuth 콜백에서 refreshToken을 수신·저장하도록 handleUserLogin 및 loginWithCode 호출을 변경하고, axios 인스턴터의 401/403 처리 흐름을 즉시 온보딩으로 리다이렉트하지 않고 Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client (브라우저)
participant API as API 서버
participant Storage as localStorage
participant PostMsg as window.postMessage
Browser->>API: 보호된 요청 (Bearer accessToken)
API-->>Browser: 401/403
Browser->>API: POST /api/v3/auth/reissue (withCredentials: true)
alt 재발급 성공
API-->>Browser: { accessToken, refreshToken? }
Browser->>Storage: accessToken 저장
Browser->>Storage: refreshToken 저장 (있다면)
Browser->>PostMsg: window.postMessage('SET_TOKEN', accessToken)
Browser->>API: 원래 요청 재시도 (업데이트된 Authorization)
API-->>Browser: 200 OK
else 재발급 실패
API-->>Browser: 에러
Browser->>Storage: accessToken, refreshToken 제거
Browser->>Browser: 온보딩 페이지로 리다이렉트 (SOCIAL_LOGIN)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
jjangminii
changed the title
jjangminii
changed the title
|
✅ Storybook chromatic 배포 확인: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/client/src/shared/apis/setting/axiosInstance.ts`:
- Around line 50-77: The token reissue calls must be serialized: introduce a
shared module-level Promise (e.g., reissuePromise) that is set when calling
axios.post(`${import.meta.env.VITE_BASE_URL}/api/v3/auth/reissue`, ...) and
reused by concurrent interceptors so only one network reissue occurs; in the
interceptor code handling originalRequest use await reissuePromise if it exists,
otherwise create it, then on success update localStorage and postMessage, set
originalRequest.headers.Authorization = `Bearer ${newAccessToken}` and retry via
apiRequest(originalRequest), and on failure clear reissuePromise, remove tokens
and redirect (same logic as currently in the catch using reissueError) before
rejecting—ensure reissuePromise is cleared after completion so subsequent 401s
can trigger a new reissue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1a5f8a24-e4dd-4e4e-bd23-b2d159b4b382
📒 Files selected for processing (2)
apps/client/src/pages/onBoarding/GoogleCallback.tsxapps/client/src/shared/apis/setting/axiosInstance.ts
| try { | ||
| const res = await axios.post( | ||
| `${import.meta.env.VITE_BASE_URL}/api/v3/auth/reissue`, | ||
| {}, | ||
| { | ||
| withCredentials: true, | ||
| } | ||
| ); | ||
|
|
||
| return Promise.reject(error); | ||
| const newAccessToken = res.data.data.token; | ||
| localStorage.setItem('token', newAccessToken); | ||
|
|
||
| window.postMessage( | ||
| { type: 'SET_TOKEN', token: newAccessToken }, | ||
| window.location.origin | ||
| ); | ||
|
|
||
| originalRequest.headers.Authorization = `Bearer ${newAccessToken}`; | ||
| return apiRequest(originalRequest); | ||
| } catch (reissueError) { | ||
| console.error('토큰 재발급 실패. 다시 로그인해주세요.', reissueError); | ||
|
|
||
| localStorage.removeItem('token'); | ||
| localStorage.removeItem('refreshToken'); | ||
| window.location.href = '/onboarding?step=SOCIAL_LOGIN'; | ||
|
|
||
| return Promise.reject(reissueError); | ||
| } |
There was a problem hiding this comment.
토큰 재발급을 단일 플로우로 직렬화해 주세요.
여기서는 401/403이 동시에 여러 개 들어오면 각 요청이 /api/v3/auth/reissue를 따로 호출합니다. refresh token rotation을 쓰는 서버라면 한 요청은 성공하고 다른 요청은 실패해서, 결국 Line 72-74의 강제 로그아웃으로 이어질 수 있습니다. 재발급 Promise를 공유하고 대기 중인 요청은 그 결과를 재사용하도록 막는 편이 안전합니다.
예시 수정안
+let refreshRequest: Promise<string> | null = null;
+
apiRequest.interceptors.response.use(
(response) => response,
async (error) => {
@@
- try {
- const res = await axios.post(
- `${import.meta.env.VITE_BASE_URL}/api/v3/auth/reissue`,
- {},
- {
- withCredentials: true,
- }
- );
-
- const newAccessToken = res.data.data.token;
+ try {
+ refreshRequest ??= axios
+ .post(
+ `${import.meta.env.VITE_BASE_URL}/api/v3/auth/reissue`,
+ {},
+ { withCredentials: true }
+ )
+ .then((res) => res.data.data.token)
+ .finally(() => {
+ refreshRequest = null;
+ });
+
+ const newAccessToken = await refreshRequest;
localStorage.setItem('token', newAccessToken);
@@
- originalRequest.headers.Authorization = `Bearer ${newAccessToken}`;
+ originalRequest.headers = {
+ ...originalRequest.headers,
+ Authorization: `Bearer ${newAccessToken}`,
+ };
return apiRequest(originalRequest);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const res = await axios.post( | |
| `${import.meta.env.VITE_BASE_URL}/api/v3/auth/reissue`, | |
| {}, | |
| { | |
| withCredentials: true, | |
| } | |
| ); | |
| return Promise.reject(error); | |
| const newAccessToken = res.data.data.token; | |
| localStorage.setItem('token', newAccessToken); | |
| window.postMessage( | |
| { type: 'SET_TOKEN', token: newAccessToken }, | |
| window.location.origin | |
| ); | |
| originalRequest.headers.Authorization = `Bearer ${newAccessToken}`; | |
| return apiRequest(originalRequest); | |
| } catch (reissueError) { | |
| console.error('토큰 재발급 실패. 다시 로그인해주세요.', reissueError); | |
| localStorage.removeItem('token'); | |
| localStorage.removeItem('refreshToken'); | |
| window.location.href = '/onboarding?step=SOCIAL_LOGIN'; | |
| return Promise.reject(reissueError); | |
| } | |
| let refreshRequest: Promise<string> | null = null; | |
| apiRequest.interceptors.response.use( | |
| (response) => response, | |
| async (error) => { | |
| if (error.response?.status === 401 || error.response?.status === 403) { | |
| try { | |
| refreshRequest ??= axios | |
| .post( | |
| `${import.meta.env.VITE_BASE_URL}/api/v3/auth/reissue`, | |
| {}, | |
| { withCredentials: true } | |
| ) | |
| .then((res) => res.data.data.token) | |
| .finally(() => { | |
| refreshRequest = null; | |
| }); | |
| const newAccessToken = await refreshRequest; | |
| localStorage.setItem('token', newAccessToken); | |
| window.postMessage( | |
| { type: 'SET_TOKEN', token: newAccessToken }, | |
| window.location.origin | |
| ); | |
| originalRequest.headers = { | |
| ...originalRequest.headers, | |
| Authorization: `Bearer ${newAccessToken}`, | |
| }; | |
| return apiRequest(originalRequest); | |
| } catch (reissueError) { | |
| console.error('토큰 재발급 실패. 다시 로그인해주세요.', reissueError); | |
| localStorage.removeItem('token'); | |
| localStorage.removeItem('refreshToken'); | |
| window.location.href = '/onboarding?step=SOCIAL_LOGIN'; | |
| return Promise.reject(reissueError); | |
| } | |
| } | |
| return Promise.reject(error); | |
| } | |
| ); |
🧰 Tools
🪛 ast-grep (0.41.0)
[warning] 59-59: Detected potential storage of sensitive information in browser localStorage. Sensitive data like email addresses, personal information, or authentication tokens should not be stored in localStorage as it's accessible to any script.
Context: localStorage.setItem('token', newAccessToken)
Note: [CWE-312] Cleartext Storage of Sensitive Information [REFERENCES]
- https://owasp.org/www-community/vulnerabilities/HTML5_Security_Cheat_Sheet
- https://cwe.mitre.org/data/definitions/312.html
(browser-storage-sensitive-data)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/client/src/shared/apis/setting/axiosInstance.ts` around lines 50 - 77,
The token reissue calls must be serialized: introduce a shared module-level
Promise (e.g., reissuePromise) that is set when calling
axios.post(`${import.meta.env.VITE_BASE_URL}/api/v3/auth/reissue`, ...) and
reused by concurrent interceptors so only one network reissue occurs; in the
interceptor code handling originalRequest use await reissuePromise if it exists,
otherwise create it, then on success update localStorage and postMessage, set
originalRequest.headers.Authorization = `Bearer ${newAccessToken}` and retry via
apiRequest(originalRequest), and on failure clear reissuePromise, remove tokens
and redirect (same logic as currently in the catch using reissueError) before
rejecting—ensure reissuePromise is cleared after completion so subsequent 401s
can trigger a new reissue.
constantly-dev
left a comment
constantly-dev
left a comment
There was a problem hiding this comment.
크리티컬한 백로그는 아니었는데 엄청 빨리 작업해주셨네요!! 수고하셨습니다~~ 👍
리뷰 달았으니 코멘트 몇 개 확인해주세요!!
| localStorage.removeItem('token'); | ||
| localStorage.removeItem('refreshToken'); |
There was a problem hiding this comment.
clearAuthStorage 등의 네이밍을 붙인 하나의 함수로 분리하는 것도 괜찮을 것 같은데 어떻게 생각하시나요?
There was a problem hiding this comment.
현재 localStorage에 저장하거나 삭제, 변경하는 로직이 적지 않다고 생각하는데 localStorage를 관리하는 함수나 유틸을 따로 파일로 만들어도 좋을 것 같아요-!
| const res = await axios.post( | ||
| `${import.meta.env.VITE_BASE_URL}/api/v3/auth/reissue`, | ||
| {}, | ||
| { | ||
| withCredentials: true, | ||
| } |
There was a problem hiding this comment.
해당 v3/auth/reissue API 호출은 함수로 분리해서 사용하는 것이 컨벤션에도 맞을 것 같아요!
같은 파일에 둬도 괜찮을 것 같아요.
| localStorage.setItem('token', newAccessToken); | ||
|
|
||
| window.postMessage( | ||
| { type: 'SET_TOKEN', token: newAccessToken }, | ||
| window.location.origin | ||
| ); |
There was a problem hiding this comment.
사실 저희 프로젝트가 토큰을 extension이랑 client가 주고 받아야 하는 구조다보니까, localStorage 관련 추가/삭제 로직과 window.postMessage로 메시지 보내는 로직이 거의 함께 작동을 하는 것 같아요.
이런 두 로직이 동시에 묶여서 재사용 되는 경우가 많으니 아예 같이 묶인 Util로 빼도 좋을 것 같은데 어떻게 생각하시나요?
There was a problem hiding this comment.
이건 다시 생각해보니까 이 두개를 묶어서 추상화하기가 조금 애매한 거 같기도해요.
이유는 두 로직이 너무 다른 역할(extension send message + localStorage 설정)이라, 단순 두개를 묶기보다 extension 쪽 message 로직과 localStorage 로직을 각각 묶어서 조합해서 사용하는 것이 더 좋을 것 같기도 합니다.
정민님은 어떻게 생각하시나요!??!
@jjangminii
There was a problem hiding this comment.
저도 각각 묶어서 조합하는데 좋을것 같다고 생각합니다.
현재까지 localStorage에 저장하는 정보가 꽤 바뀌었다고 생각합니다. 다음 스프린트 때도 localStorage에 저장이 필요한 정보가 바뀔 수 있다고 생각하고요.
그래서 localStorage와 extension send message를 각각 묶는 방식이 좋다고 생각합니다-!
There was a problem hiding this comment.
지금 401처리에 refresh하는 로직을 잘 작성을 해주시긴 했지만 한가지 문제가 있을 것 같아요.
만약 여러 API에서 동시에 401 등의 에러가 나타나게 되면,이때 한번의 refresh가 일어나야 하는데 현재 코드 기준으로는 이 모든 401이 온 API가 다 refresh를 호출하는 상황이 일어날 것 같아요.
현재 있는 !originalRequest._retry && 조건은 한 API에만 해당되는 것으로 알아서 여러 API에서 401이 터지면 모두 저 refresh를 할 것으로 예상이 돼요.
따라서 이를 refreshPromise 방식이나, isRefreshing + queue 패턴 등을 통해 이를 한번만 refresh하도록 할 수 있을 것 같아요!
There was a problem hiding this comment.
refresh가 처음이라 이 부분은 예상하지 못했는데 이슈파서 공부하고 작업해보겠습니다-!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/client/src/pages/onBoarding/GoogleCallback.tsx`:
- Around line 24-25: The code is persisting a high-value refreshToken into
localStorage (the accessToken/refreshToken fields in GoogleCallback), which
widens XSS attack surface; remove any localStorage writes/reads for refreshToken
(e.g., delete calls that setItem/getItem for refreshToken in the GoogleCallback
flow) and stop passing/storing refreshToken in the client side payloads—keep
only the accessToken if needed or rely solely on cookie-based refresh
(axiosInstance already uses withCredentials). Also remove the refreshToken
parameter from local state/props or any helper functions you control (references
to refreshToken in the GoogleCallback component) so the client no longer
duplicates the server-managed refresh token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7fd2dd38-57d6-4147-8ba9-4855cf8daf0b
📒 Files selected for processing (1)
apps/client/src/pages/onBoarding/GoogleCallback.tsx
| accessToken: string | null, | ||
| refreshToken: string | null, |
There was a problem hiding this comment.
Refresh Token은 localStorage에 저장하지 않는 편이 안전합니다.
apps/client/src/shared/apis/setting/axiosInstance.ts의 Line 12-19는 refreshToken을 전혀 읽지 않고, Line 50-68의 재발급도 withCredentials 쿠키만 사용합니다. 지금 변경은 사용되지도 않는 고가치 토큰을 JS 접근 가능한 저장소에 한 번 더 복제하는 셈이라 XSS 노출면만 넓힙니다. 전달/저장 체인 자체를 제거하는 쪽이 안전합니다.
🔒 제안 수정안
const handleUserLogin = (
isUser: boolean,
accessToken: string | null,
- refreshToken: string | null,
hasJob?: boolean
) => {
if (isUser) {
if (accessToken) {
localStorage.setItem('token', accessToken);
@@
sendTokenToExtension(accessToken);
}
-
- if (refreshToken) {
- localStorage.setItem('refreshToken', refreshToken);
- }
if (typeof hasJob === 'boolean') {
localStorage.setItem('hasJob', String(hasJob));
}
navigate('/');
@@
- const { isUser, userId, email, accessToken, refreshToken, hasJob } =
+ const { isUser, userId, email, accessToken, hasJob } =
res.data.data;
@@
- handleUserLogin(isUser, accessToken, refreshToken, hasJob);
+ handleUserLogin(isUser, accessToken, hasJob);Also applies to: 44-46, 74-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/client/src/pages/onBoarding/GoogleCallback.tsx` around lines 24 - 25,
The code is persisting a high-value refreshToken into localStorage (the
accessToken/refreshToken fields in GoogleCallback), which widens XSS attack
surface; remove any localStorage writes/reads for refreshToken (e.g., delete
calls that setItem/getItem for refreshToken in the GoogleCallback flow) and stop
passing/storing refreshToken in the client side payloads—keep only the
accessToken if needed or rely solely on cookie-based refresh (axiosInstance
already uses withCredentials). Also remove the refreshToken parameter from local
state/props or any helper functions you control (references to refreshToken in
the GoogleCallback component) so the client no longer duplicates the
server-managed refresh token.
2 participants
📌 Related Issues
📄 Tasks
⭐ PR Point (To Reviewer)
📷 Screenshot
Summary by CodeRabbit
릴리스 노트
버그 수정
리팩토링