-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/member cors #121
Feature/member cors #121
Conversation
…s_BE into feature/member-cors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review by ChatGPT
@@ -45,8 +45,7 @@ public SpringSecurityConfig(TokenProvider tokenProvider, TokenRepository tokenRe | |||
|
|||
private static final String[] AUTH_WHITELIST = { | |||
"/api/v1/member/**", "/swagger-ui/**", "/api-docs", "/swagger-ui-custom.html", | |||
"/v3/api-docs/**", "/api-docs/**", "/swagger-ui.html", "/api/v1/auth/**", | |||
"/api/**" | |||
"/v3/api-docs/**", "/api-docs/**", "/swagger-ui.html" | |||
}; | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다.
-
버그 위험:
- AUTH_WHITELIST에서
/api/v1/auth/**
와/api/**
경로를 제거했습니다. 이는 인증 및 API 요청과 관련된 경로가 보호되지 않을 수 있음을 의미합니다. 이 변경으로 인해 사용자 인증이 필요한 API 엔드포인트에 대한 접근이 허용되면 심각한 보안 위험이 발생할 수 있습니다.
- AUTH_WHITELIST에서
-
개선 제안:
- 추가된 경로를 잘못 제거한 것인지 확인하고, 필요한 경우 원래 경로를 유지하도록 합니다. 구체적으로
/api/v1/auth/**
와/api/**
경로가 실제로 인증이 필요없는 경우인지 검토해야 합니다. - 주석을 추가하여 변경된 경로의 이유를 설명하면 나중에 코드를 이해하는 데 도움이 됩니다. 누군가가 이전 코드와 비교하여 왜 변경되었는지 알기 어려울 수 있기 때문입니다.
- 또한, 이러한 화이트리스트 경로를 관리하는 데 있어 관리 용이성을 높이기 위해, 상수 배열 대신 Set을 사용하는 것을 고려할 수 있으며, 이를 통해 중복을 방지하고 시간 복잡도를 줄일 수 있습니다.
- 추가된 경로를 잘못 제거한 것인지 확인하고, 필요한 경우 원래 경로를 유지하도록 합니다. 구체적으로
이와 같은 점들을 고려하여 코드를 점검하면 좋겠습니다.
"https://api.devlens.work") | ||
|
||
.allowedMethods("GET", "POST", "PUT", "DELETE", "PATCH") | ||
.allowedMethods("GET", "POST", "PUT", "DELETE", "PATCH", "OPTIONS") | ||
.allowedHeaders("*") | ||
.allowCredentials(true); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 리뷰에 대한 의견입니다.
-
허용된 원본 패턴:
allowedOriginPatterns
리스트에서localhost
관련 내용이 제거되었고, 이는 개발 환경에서의 접근을 차단할 수 있습니다. 필요에 따라 개발 환경을 위한 CORS 규칙을 설정하는 것이 좋습니다.
-
안전성 향상:
- 특정 원본들만 허용하고 있는데, 보안상 여러 레벨의 도메인을 허용하는 것이 좋습니다. 예를 들어, 서브도메인이나 변형된 도메인도 고려해 보십시오.
-
HTTP 메서드:
OPTIONS
메서드가 추가된 것은 CORS 정책에 따라 클라이언트가 미리 요청을 검증할 수 있도록 돕기 때문에 좋은 개선입니다. 그러나, 필요하지 않다면 추가적인 메서드는 생략하는 것이 보안상 더 좋을 수 있습니다.
-
주석:
- 코드를 읽기 쉽게 만들기 위해 주석을 추가하는 것이 좋습니다. 이는 특히 팀원들이나 나중에 코드를 유지보수할 때 도움이 됩니다.
-
형식 일관성:
- 코드 포맷이 일관되지 않아 가독성이 떨어질 수 있습니다. 한 줄에 너무 많은 인자를 나열하는 것보다는, 각 인자를 별도의 줄에 배치하여 가독성을 높이는 것이 좋습니다.
-
테스트:
- 변경된 CORS 설정이 의도한 대로 작동하는지 충분히 테스트하는 것이 중요합니다. 특히 다양한 환경에서의 동작을 확인하는 절차를 고려하십시오.
이와 같은 점들을 반영하면 코드 품질을 더욱 향상시킬 수 있습니다.
바로 MERGE하지 마세요! 검토 후 MERGE해야합니다!
✅ 최근 작업 주제 (하나 이상의 주제를 선택해주세요.)
🏆 구현 목표
cors 설정
📋 구현 사항 설명 (작업한 내용을 상세하게 기록합니다.)
cors 설정
🔍 테스트 방법 (변경 사항을 확인하기 위한 테스트 방법을 기술합니다.)
ex)
npm test
실행🛠️ 쓰이는 모듈
ex)
💬 기타 질문 및 특이 사항
ex) 백프 배포 nginx 검토