Skip to content
Merged
48 changes: 31 additions & 17 deletions apps/client/src/pages/onBoarding/GoogleCallback.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,16 @@ import LoadingChippi from '@shared/components/loadingChippi/LoadingChippi';
import { useEffect } from 'react';
import { useNavigate, useSearchParams } from 'react-router-dom';

const sendTokenToExtension = (token: string) => {
window.postMessage(
{
type: 'SET_TOKEN',
token,
},
window.location.origin
);
};

const GoogleCallback = () => {
const navigate = useNavigate();
const [searchParams] = useSearchParams();
Expand All @@ -21,24 +31,20 @@ const GoogleCallback = () => {

const handleUserLogin = (
isUser: boolean,
accessToken: string | undefined,
accessToken: string | null,
refreshToken: string | null,
Comment on lines +34 to +35
Copy link

Choose a reason for hiding this comment

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

โš ๏ธ Potential issue | ๐ŸŸ  Major

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.

hasJob?: boolean
) => {
if (isUser) {
if (accessToken) {
localStorage.setItem('token', accessToken);

const sendTokenToExtension = (token: string) => {
window.postMessage(
{
type: 'SET_TOKEN',
token,
},
window.location.origin
);
};
sendTokenToExtension(accessToken);
}

if (refreshToken) {
localStorage.setItem('refreshToken', refreshToken);
}

if (typeof hasJob === 'boolean') {
localStorage.setItem('hasJob', String(hasJob));
}
Expand All @@ -54,16 +60,24 @@ const GoogleCallback = () => {

const loginWithCode = async (code: string) => {
try {
const res = await apiRequest.post('/api/v3/auth/google', {
code,
uri: redirectUri,
});
const { isUser, userId, email, accessToken, hasJob } = res.data.data;
const res = await apiRequest.post(
'/api/v3/auth/google',
{
code,
uri: redirectUri,
},
{
withCredentials: true,
}
);

const { isUser, userId, email, accessToken, refreshToken, hasJob } =
res.data.data;

localStorage.setItem('email', email);
localStorage.setItem('userId', userId);

handleUserLogin(isUser, accessToken, hasJob);
handleUserLogin(isUser, accessToken, refreshToken, hasJob);
} catch (error) {
console.error('๋กœ๊ทธ์ธ ์˜ค๋ฅ˜:', error);
navigate('/onboarding?step=SOCIAL_LOGIN');
Expand Down
33 changes: 29 additions & 4 deletions apps/client/src/shared/apis/setting/axiosInstance.ts
Copy link
Member

Choose a reason for hiding this comment

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

์ง€๊ธˆ 401์ฒ˜๋ฆฌ์— refreshํ•˜๋Š” ๋กœ์ง์„ ์ž˜ ์ž‘์„ฑ์„ ํ•ด์ฃผ์‹œ๊ธด ํ–ˆ์ง€๋งŒ ํ•œ๊ฐ€์ง€ ๋ฌธ์ œ๊ฐ€ ์žˆ์„ ๊ฒƒ ๊ฐ™์•„์š”.
๋งŒ์•ฝ ์—ฌ๋Ÿฌ API์—์„œ ๋™์‹œ์— 401 ๋“ฑ์˜ ์—๋Ÿฌ๊ฐ€ ๋‚˜ํƒ€๋‚˜๊ฒŒ ๋˜๋ฉด,์ด๋•Œ ํ•œ๋ฒˆ์˜ refresh๊ฐ€ ์ผ์–ด๋‚˜์•ผ ํ•˜๋Š”๋ฐ ํ˜„์žฌ ์ฝ”๋“œ ๊ธฐ์ค€์œผ๋กœ๋Š” ์ด ๋ชจ๋“  401์ด ์˜จ API๊ฐ€ ๋‹ค refresh๋ฅผ ํ˜ธ์ถœํ•˜๋Š” ์ƒํ™ฉ์ด ์ผ์–ด๋‚  ๊ฒƒ ๊ฐ™์•„์š”.

ํ˜„์žฌ ์žˆ๋Š” !originalRequest._retry && ์กฐ๊ฑด์€ ํ•œ API์—๋งŒ ํ•ด๋‹น๋˜๋Š” ๊ฒƒ์œผ๋กœ ์•Œ์•„์„œ ์—ฌ๋Ÿฌ API์—์„œ 401์ด ํ„ฐ์ง€๋ฉด ๋ชจ๋‘ ์ € refresh๋ฅผ ํ•  ๊ฒƒ์œผ๋กœ ์˜ˆ์ƒ์ด ๋ผ์š”.

๋”ฐ๋ผ์„œ ์ด๋ฅผ refreshPromise ๋ฐฉ์‹์ด๋‚˜, isRefreshing + queue ํŒจํ„ด ๋“ฑ์„ ํ†ตํ•ด ์ด๋ฅผ ํ•œ๋ฒˆ๋งŒ refreshํ•˜๋„๋ก ํ•  ์ˆ˜ ์žˆ์„ ๊ฒƒ ๊ฐ™์•„์š”!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refresh๊ฐ€ ์ฒ˜์Œ์ด๋ผ ์ด ๋ถ€๋ถ„์€ ์˜ˆ์ƒํ•˜์ง€ ๋ชปํ–ˆ๋Š”๋ฐ ์ด์ŠˆํŒŒ์„œ ๊ณต๋ถ€ํ•˜๊ณ  ์ž‘์—…ํ•ด๋ณด๊ฒ ์Šต๋‹ˆ๋‹ค-!

Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ apiRequest.interceptors.response.use(
const noAuthNeeded = [
'/api/v1/auth/token',
'/api/v3/auth/signup',
'/api/v2/auth/google',
'/api/v3/auth/google',
'/api/v3/auth/reissue',
];

const isNoAuth = noAuthNeeded.some((url) =>
Expand All @@ -46,10 +47,34 @@ apiRequest.interceptors.response.use(
) {
originalRequest._retry = true;

// localStorage.removeItem('token');
window.location.href = '/onboarding?step=SOCIAL_LOGIN';
try {
const res = await axios.post(
`${import.meta.env.VITE_BASE_URL}/api/v3/auth/reissue`,
{},
{
withCredentials: true,
}
Comment on lines +51 to +56
Copy link
Member

Choose a reason for hiding this comment

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

ํ•ด๋‹น v3/auth/reissue API ํ˜ธ์ถœ์€ ํ•จ์ˆ˜๋กœ ๋ถ„๋ฆฌํ•ด์„œ ์‚ฌ์šฉํ•˜๋Š” ๊ฒƒ์ด ์ปจ๋ฒค์…˜์—๋„ ๋งž์„ ๊ฒƒ ๊ฐ™์•„์š”!
๊ฐ™์€ ํŒŒ์ผ์— ๋‘ฌ๋„ ๊ดœ์ฐฎ์„ ๊ฒƒ ๊ฐ™์•„์š”.

);

return Promise.reject(error);
const newAccessToken = res.data.data.token;
localStorage.setItem('token', newAccessToken);

window.postMessage(
{ type: 'SET_TOKEN', token: newAccessToken },
window.location.origin
);
Comment on lines +60 to +65
Copy link
Member

Choose a reason for hiding this comment

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

์‚ฌ์‹ค ์ €ํฌ ํ”„๋กœ์ ํŠธ๊ฐ€ ํ† ํฐ์„ extension์ด๋ž‘ client๊ฐ€ ์ฃผ๊ณ  ๋ฐ›์•„์•ผ ํ•˜๋Š” ๊ตฌ์กฐ๋‹ค๋ณด๋‹ˆ๊นŒ, localStorage ๊ด€๋ จ ์ถ”๊ฐ€/์‚ญ์ œ ๋กœ์ง๊ณผ window.postMessage๋กœ ๋ฉ”์‹œ์ง€ ๋ณด๋‚ด๋Š” ๋กœ์ง์ด ๊ฑฐ์˜ ํ•จ๊ป˜ ์ž‘๋™์„ ํ•˜๋Š” ๊ฒƒ ๊ฐ™์•„์š”.
์ด๋Ÿฐ ๋‘ ๋กœ์ง์ด ๋™์‹œ์— ๋ฌถ์—ฌ์„œ ์žฌ์‚ฌ์šฉ ๋˜๋Š” ๊ฒฝ์šฐ๊ฐ€ ๋งŽ์œผ๋‹ˆ ์•„์˜ˆ ๊ฐ™์ด ๋ฌถ์ธ Util๋กœ ๋นผ๋„ ์ข‹์„ ๊ฒƒ ๊ฐ™์€๋ฐ ์–ด๋–ป๊ฒŒ ์ƒ๊ฐํ•˜์‹œ๋‚˜์š”?

Copy link
Member

Choose a reason for hiding this comment

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

์ด๊ฑด ๋‹ค์‹œ ์ƒ๊ฐํ•ด๋ณด๋‹ˆ๊นŒ ์ด ๋‘๊ฐœ๋ฅผ ๋ฌถ์–ด์„œ ์ถ”์ƒํ™”ํ•˜๊ธฐ๊ฐ€ ์กฐ๊ธˆ ์• ๋งคํ•œ ๊ฑฐ ๊ฐ™๊ธฐ๋„ํ•ด์š”.
์ด์œ ๋Š” ๋‘ ๋กœ์ง์ด ๋„ˆ๋ฌด ๋‹ค๋ฅธ ์—ญํ• (extension send message + localStorage ์„ค์ •)์ด๋ผ, ๋‹จ์ˆœ ๋‘๊ฐœ๋ฅผ ๋ฌถ๊ธฐ๋ณด๋‹ค extension ์ชฝ message ๋กœ์ง๊ณผ localStorage ๋กœ์ง์„ ๊ฐ๊ฐ ๋ฌถ์–ด์„œ ์กฐํ•ฉํ•ด์„œ ์‚ฌ์šฉํ•˜๋Š” ๊ฒƒ์ด ๋” ์ข‹์„ ๊ฒƒ ๊ฐ™๊ธฐ๋„ ํ•ฉ๋‹ˆ๋‹ค.

์ •๋ฏผ๋‹˜์€ ์–ด๋–ป๊ฒŒ ์ƒ๊ฐํ•˜์‹œ๋‚˜์š”!??!
@jjangminii


originalRequest.headers.Authorization = `Bearer ${newAccessToken}`;
return apiRequest(originalRequest);
} catch (reissueError) {
console.error('ํ† ํฐ ์žฌ๋ฐœ๊ธ‰ ์‹คํŒจ. ๋‹ค์‹œ ๋กœ๊ทธ์ธํ•ด์ฃผ์„ธ์š”.', reissueError);

localStorage.removeItem('token');
localStorage.removeItem('refreshToken');
Comment on lines +72 to +73
Copy link
Member

Choose a reason for hiding this comment

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

clearAuthStorage ๋“ฑ์˜ ๋„ค์ด๋ฐ์„ ๋ถ™์ธ ํ•˜๋‚˜์˜ ํ•จ์ˆ˜๋กœ ๋ถ„๋ฆฌํ•˜๋Š” ๊ฒƒ๋„ ๊ดœ์ฐฎ์„ ๊ฒƒ ๊ฐ™์€๋ฐ ์–ด๋–ป๊ฒŒ ์ƒ๊ฐํ•˜์‹œ๋‚˜์š”?

window.location.href = '/onboarding?step=SOCIAL_LOGIN';

return Promise.reject(reissueError);
}
Comment on lines +50 to +77
Copy link

Choose a reason for hiding this comment

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

โš ๏ธ Potential issue | ๐ŸŸ  Major

ํ† ํฐ ์žฌ๋ฐœ๊ธ‰์„ ๋‹จ์ผ ํ”Œ๋กœ์šฐ๋กœ ์ง๋ ฌํ™”ํ•ด ์ฃผ์„ธ์š”.

์—ฌ๊ธฐ์„œ๋Š” 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.

Suggested change
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.

}

return Promise.reject(error);
Expand Down
Loading