Conversation
- HttpResponse::setBody 메서드 내부에서 Content-Length, Content-Type 등의 Body 관련 헤더를 설정하도록 책임 포인트 변경 - Post Handler의 File 읽기 관련 코드 리펙토링
- HttpServlet -> ConnectionHandler - WasServlet -> Dispatcher
- 구성 요소들을 web 패키지 아래에 수평적 구조로 구성 - exception 패키지 분리
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
필드명이 'dispatcher'로 변경되었지만, 초기화 로직에서 'appConfig.wasServlet()'을 호출하고 있습니다. 이는 메서드명이 여전히 'wasServlet()'이므로 리팩토링이 불완전해 보입니다. 일관성을 위해 AppConfig의 메서드명도 'dispatcher()'로 변경하는 것이 좋습니다. 현재 상태에서는 LOADER.dispatcher를 사용하지만 실제 생성 메서드는 wasServlet()이므로 유지보수 시 혼동될 수 있습니다."
There was a problem hiding this comment.
Javadoc 주석이 불완전합니다. '정'만 입력되어 있어 클래스의 목적을 명확히 설명하지 못합니다. 정적 리소스 요청을 처리하는 핸들러임을 명시해주세요."
| File file = resolveStaticFile(path) | ||
| .orElseThrow(() -> new ErrorException("Static content path Error")); |
There was a problem hiding this comment.
에러 메시지가 모호합니다. 'Static content path Error'는 경로 오류, 파일 존재 여부 확인 실패, 또는 다른 이유인지 불명확합니다. 'Static resource file not found: {path}' 같이 구체적인 경로 정보를 포함한 메시지를 제공하면 디버깅이 훨씬 수월해집니다."
| private Optional<File> resolveStaticFile(String path){ | ||
| for(String root : staticResourceRoots){ | ||
| File requestedFile = new File(root + path); | ||
| if(requestedFile.exists() && requestedFile.isFile()) return Optional.of(requestedFile); | ||
|
|
||
| String indexFilePath = path + (path.endsWith("/") ? "index.html" : "/index.html"); | ||
| File indexFile = new File(root + indexFilePath); | ||
| if(indexFile.exists() && indexFile.isFile()) return Optional.of(indexFile); | ||
| } | ||
| return Optional.empty(); | ||
| } | ||
| } |
There was a problem hiding this comment.
'resolveStaticFile' 메서드가 StaticContentResponseHandler와 ViewResponseHandler에서 완전히 동일하게 구현되어 있습니다. 이는 코드 중복입니다. 공통 유틸리티 클래스(예: StaticResourceResolver)를 만들어 두 핸들러가 공유하도록 리팩토링하면 유지보수성이 향상됩니다."
| public class VariableConfig { | ||
| public static final List<String> STATIC_RESOURCE_ROOTS = List.of( | ||
| "./src/main/resources", | ||
| "./src/main/resources/static"); | ||
| } |
There was a problem hiding this comment.
정적 리소스 경로가 하드코딩되어 있습니다. 프로덕션 환경에서는 배포 구조에 따라 경로가 달라질 수 있으므로, 환경 변수나 설정 파일에서 읽도록 개선하는 것이 좋습니다. 또한 './src/main/resources'는 개발 환경 전용 경로로 보이므로, 런타임 환경에서 올바른 리소스 경로를 동적으로 결정하는 로직이 필요합니다."
| public boolean checkEndpoint(HttpMethod method, String path) { | ||
| if(!method.equals(HttpMethod.GET)) return false; | ||
| return staticResourceRoots.stream().anyMatch(root ->{ | ||
| File requestedFile = new File(root + path); | ||
| String indexFilePath = path + (path.endsWith("/") ? "index.html" : "/index.html"); | ||
| File indexFile = new File(root + indexFilePath); | ||
| return (requestedFile.exists() && requestedFile.isFile()) || (indexFile.exists() && indexFile.isFile()); | ||
| }); |
There was a problem hiding this comment.
Path traversal 취약점이 여전히 존재합니다. '../'를 포함한 경로가 정적 리소스 루트 외부에 접근할 수 있습니다. 파일 경로를 정규화하고 정적 리소스 루트 내에 있는지 검증해야 합니다. 예: file.getCanonicalPath().startsWith(rootPath.getCanonicalPath())"
1 participant
💻 작업 내용
✨ 리뷰 포인트
대대적인 패키지구조 리팩터링을 했습니다.
현재 구조가 적절한지 피드백 주세요.
📝 메모
web 패키지쪽은 handler adapter를 사용하도록 리팩터링이 예정되어있습니다.
🎯 관련 이슈
closed #24