Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughHTTP 서버 구성 요소가 추가되고 라우팅/컨트롤러 구조로 리팩터링되었습니다. 요청 파싱(HttpRequest), 응답 작성(HttpResponse), 상수(enum) 정의, 요청 매핑(RequestMapper), 컨트롤러 구현(홈, 포워드, 리스트, 로그인, 가입)이 도입되었고, 기존 RequestHandler가 이 파이프라인을 사용하도록 변경되었습니다. 일부 파일은 포매팅만 수정되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant WS as WebServer
participant RH as RequestHandler
participant HRq as HttpRequest
participant HRs as HttpResponse
participant RM as RequestMapper
participant C as Controller
Client->>WS: TCP 연결
WS->>RH: 소켓 핸들링
RH->>HRq: from(BufferedReader)
RH->>HRs: new HttpResponse(DataOutputStream)
RH->>RM: new RequestMapper(HRq, HRs)
RM->>RM: 경로 기반 컨트롤러 선택(기본: ForwardController)
RM->>C: execute(request, response)
alt 정적/홈
C->>HRs: response200(path) 또는 302 Location
else 인증 필요
C->>HRs: 302 Location (+Set-Cookie 선택)
end
HRs-->>Client: HTTP 응답 전송
Client-->>WS: 연결 종료
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
|
기능단위로 커밋 하는것이 좋습니다! 등의 컨벤션 지켜서 메세지 작성도 연습해보면 좋을듯합니다 👍 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (6)
src/main/java/constant/HttpStatusCode.java (1)
3-17: 필요 시 추가 상태 코드 확장을 고려하세요.현재 구현은 기본적인 성공, 리다이렉트, 에러 코드만 포함하고 있습니다. 향후 웹 애플리케이션의 요구사항에 따라 400 Bad Request, 401 Unauthorized, 500 Internal Server Error 등의 상태 코드를 추가하는 것을 고려해보세요.
src/main/java/constant/Url.java (1)
3-21: 경로 상수의 명명 규칙을 명확히 하세요.일부 경로는
.html확장자를 포함하고(INDEX, USER_LOGIN_HTML, USER_LOGIN_FAILED), 다른 경로는 포함하지 않습니다(USER_CREATE, USER_LOGIN, USER_LIST). 이는 엔드포인트와 정적 페이지를 구분하기 위한 의도로 보이지만, 명확한 문서화나 명명 규칙이 있으면 코드 가독성이 향상됩니다.예를 들어, 정적 페이지 경로는
_HTML접미사를 일관되게 사용하는 것이 좋습니다.src/main/java/controller/Controller.java (1)
7-7: 제네릭 Exception 대신 구체적인 예외 타입을 사용하세요.
throws Exception은 너무 광범위하여 예외 처리를 어렵게 만듭니다. HTTP 처리와 관련된 구체적인 예외 타입(예:IOException,ServletException등)을 사용하거나 커스텀 예외를 정의하는 것이 좋습니다.다음과 같이 구체적인 예외 타입을 사용하도록 수정하세요:
- void execute(HttpRequest request, HttpResponse response) throws Exception; + void execute(HttpRequest request, HttpResponse response) throws IOException;src/main/java/controller/SignUpController.java (1)
20-27: POST 요청에서 queryString 파싱 로직이 중복됩니다.GET 요청에서 queryString을 파싱하는 것은 올바르지만, POST 요청에서는 body만 파싱하면 됩니다. 현재 코드는 정상 동작하지만 구조를 개선할 수 있습니다.
다음과 같이 리팩터링을 고려하세요:
Map<String, String> params = null; - if (request.getMethod() == HttpMethod.GET) { - String queryString = request.getQueryString(); - if (!queryString.isEmpty()) { - params = HttpRequestUtils.parseQueryParameter(queryString); - } - } else if (request.getMethod() == HttpMethod.POST) { + if (request.getMethod() == HttpMethod.GET && !request.getQueryString().isEmpty()) { + params = HttpRequestUtils.parseQueryParameter(request.getQueryString()); + } else if (request.getMethod() == HttpMethod.POST && !request.getBody().isEmpty()) { params = HttpRequestUtils.parseQueryParameter(request.getBody()); }src/main/java/webserver/RequestMapper.java (1)
15-15: 정적 상수 네이밍 컨벤션을 따르지 않습니다.
static final필드는 일반적으로 대문자와 언더스코어를 사용합니다 (예:ROUTES또는MAPPINGS).다음과 같이 네이밍을 수정하세요:
- private static final Map<String, Controller> mappings = new HashMap<>(); + private static final Map<String, Controller> ROUTES = new HashMap<>(); static { - mappings.put("/", new HomeController()); - mappings.put("/index.html", new ForwardController()); - mappings.put("/user/signup", new SignUpController()); - mappings.put("/user/login", new LoginController()); - mappings.put("/user/userList", new ListController()); + ROUTES.put("/", new HomeController()); + ROUTES.put("/index.html", new ForwardController()); + ROUTES.put("/user/signup", new SignUpController()); + ROUTES.put("/user/login", new LoginController()); + ROUTES.put("/user/userList", new ListController()); } public void proceed() throws Exception { String path = request.getPath(); - Controller controller = mappings.getOrDefault(path, DEFAULT_CONTROLLER); + Controller controller = ROUTES.getOrDefault(path, DEFAULT_CONTROLLER); controller.execute(request, response); }src/main/java/http/HttpRequest.java (1)
55-69: 쿼리 문자열 분리 로직의 중복을 제거하세요.
getPath()와getQueryString()모두path.split("\\?")를 수행하여 중복이 있습니다. 생성자에서 한 번만 분리하거나 캐싱을 고려하세요.다음 diff를 적용하여 중복을 제거하세요:
public class HttpRequest { private final HttpMethod method; private final String path; + private final String queryString; private final String version; private final Map<String, String> headers; private final String body; private HttpRequest(HttpMethod method, String path, String version, Map<String, String> headers, String body) { this.method = method; - this.path = path; + if (path.contains("?")) { + String[] parts = path.split("\\?", 2); + this.path = parts[0]; + this.queryString = parts[1]; + } else { + this.path = path; + this.queryString = ""; + } this.version = version; this.headers = headers; this.body = body; } // --- Getter --- public HttpMethod getMethod() { return method; } public String getPath() { - if (path.contains("?")) { - return path.split("\\?")[0]; - } return path; } public String getVersion() { return version; } public Map<String, String> headers() { return headers; } public String getBody() { return body; } public String getQueryString() { - if (path.contains("?")) { - return path.split("\\?")[1]; - } - return ""; + return queryString; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/main/java/constant/HttpMethod.java(1 hunks)src/main/java/constant/HttpStatusCode.java(1 hunks)src/main/java/constant/Url.java(1 hunks)src/main/java/constant/UserQueryKey.java(1 hunks)src/main/java/controller/Controller.java(1 hunks)src/main/java/controller/ForwardController.java(1 hunks)src/main/java/controller/HomeController.java(1 hunks)src/main/java/controller/ListController.java(1 hunks)src/main/java/controller/LoginController.java(1 hunks)src/main/java/controller/SignUpController.java(1 hunks)src/main/java/db/MemoryUserRepository.java(1 hunks)src/main/java/db/Repository.java(1 hunks)src/main/java/http/HttpRequest.java(1 hunks)src/main/java/http/HttpResponse.java(1 hunks)src/main/java/http/util/HttpRequestUtils.java(1 hunks)src/main/java/http/util/IOUtils.java(1 hunks)src/main/java/model/User.java(1 hunks)src/main/java/webserver/RequestHandler.java(2 hunks)src/main/java/webserver/RequestMapper.java(1 hunks)src/main/java/webserver/WebServer.java(1 hunks)
🔇 Additional comments (6)
src/main/java/http/util/HttpRequestUtils.java (1)
19-19: 마무리 중괄호 정리 감사합니다.불필요한 닫는 중괄호를 제거해 클래스 구조가 정상화되었습니다. 👍
src/main/java/http/util/IOUtils.java (1)
19-19: 주석 추가로 가독성이 향상되었습니다.
char[]을String으로 변환하는 과정을 명확히 하는 주석이 추가되어 코드 이해가 쉬워졌습니다.src/main/java/controller/HomeController.java (1)
7-12: LGTM!
HomeController는Controller인터페이스를 올바르게 구현하고 있으며,Url.INDEX상수를 사용하여 경로를 관리하는 것이 좋은 패턴입니다. 로직이 명확하고 단순합니다.src/main/java/constant/UserQueryKey.java (1)
1-19: 잘 설계된 enum입니다.쿼리 파라미터 키를 타입 안전하게 관리하는 좋은 설계입니다. 캡슐화와 일관성을 제공합니다.
src/main/java/webserver/RequestMapper.java (1)
18-19: 라우트 매핑 유지두 컨트롤러는 '/' 요청을 INDEX 경로로 리다이렉트(HomeController)하고, 정적 파일 요청을 요청 경로 그대로 전달(ForwardController)하도록 역할이 분리되어 있어 변경이 필요 없습니다.
Likely an incorrect or invalid review comment.
src/main/java/webserver/RequestHandler.java (1)
29-35: 파이프라인 구조로의 리팩터링이 적절합니다.HttpRequest/HttpResponse 추상화와 RequestMapper를 통한 요청 처리 흐름이 잘 구성되어 있습니다. 기존 하드코딩된 응답 로직을 제거하고 라우팅 구조를 도입한 점이 좋습니다.
| public static HttpMethod from(String method) { | ||
| return HttpMethod.valueOf(method); | ||
| } |
There was a problem hiding this comment.
예외 처리가 누락되어 있습니다.
valueOf() 메서드는 잘못된 문자열이 전달되면 IllegalArgumentException을 던지므로, 유효하지 않은 HTTP 메서드에 대한 예외 처리가 필요합니다.
다음 diff를 적용하여 명시적인 예외 처리를 추가하세요:
public static HttpMethod from(String method) {
+ try {
return HttpMethod.valueOf(method);
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException("Unsupported HTTP method: " + method, e);
+ }
}📝 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.
| public static HttpMethod from(String method) { | |
| return HttpMethod.valueOf(method); | |
| } | |
| public static HttpMethod from(String method) { | |
| try { | |
| return HttpMethod.valueOf(method); | |
| } catch (IllegalArgumentException e) { | |
| throw new IllegalArgumentException("Unsupported HTTP method: " + method, e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/constant/HttpMethod.java around lines 6 to 8, the from(String)
method currently calls HttpMethod.valueOf(method) without handling invalid
input; wrap the call in a try-catch that catches IllegalArgumentException (and
optionally NullPointerException) and rethrow a new IllegalArgumentException with
a clear message like "Invalid HTTP method: " + method (or use a custom
exception), so invalid or null method strings produce an explicit, informative
error.
| public void execute(HttpRequest request, HttpResponse response) throws Exception { | ||
| response.response200(request.getPath()); | ||
| } |
There was a problem hiding this comment.
경로 검증이 없어 보안 취약점이 존재합니다.
request.getPath()를 검증 없이 직접 사용하면 경로 순회(path traversal) 공격에 취약합니다. 공격자가 ../../etc/passwd 같은 경로로 시스템 파일에 접근할 수 있습니다.
다음과 같이 경로 검증을 추가하세요:
@Override
public void execute(HttpRequest request, HttpResponse response) throws Exception {
- response.response200(request.getPath());
+ String path = request.getPath();
+ // 경로 순회 공격 방지
+ if (path.contains("..") || path.contains("//")) {
+ throw new SecurityException("Invalid path: " + path);
+ }
+ response.response200(path);
}📝 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.
| public void execute(HttpRequest request, HttpResponse response) throws Exception { | |
| response.response200(request.getPath()); | |
| } | |
| @Override | |
| public void execute(HttpRequest request, HttpResponse response) throws Exception { | |
| String path = request.getPath(); | |
| // 경로 순회 공격 방지 | |
| if (path.contains("..") || path.contains("//")) { | |
| throw new SecurityException("Invalid path: " + path); | |
| } | |
| response.response200(path); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/controller/ForwardController.java around lines 10 to 12, the
code uses request.getPath() directly causing a path traversal vulnerability;
validate and canonicalize the requested path before using it: URL-decode the
request path, reject or sanitize any null/empty or absolute paths, resolve it
against a server-side allowed base directory using
java.nio.file.Paths.get(baseDir).resolve(requestPath).normalize(), then ensure
the normalized path startsWith the allowed base directory to block any '..'
traversal; alternatively enforce a strict whitelist or regex of allowed routes
and return a 400/403 if validation fails.
| String cookie = request.getHeaders().get("Cookie"); | ||
|
|
||
| if (cookie != null && cookie.contains("logined=true")) { | ||
| response.response200(Url.USER_LIST.getPath() + ".html"); // ✅ list.html 반환 | ||
| } else { | ||
| response.response302Header(Url.USER_LOGIN_HTML.getPath()); // ✅ 로그인 안되면 redirect | ||
| } |
There was a problem hiding this comment.
쿠키 파싱이 안전하지 않습니다.
Line 12의 cookie.contains("logined=true") 검사는 부분 문자열 매칭을 사용하므로, logined=true123 또는 notlogined=true 같은 잘못된 쿠키 값도 통과할 수 있습니다. 쿠키는 key=value 쌍이 세미콜론으로 구분되는 형식이므로 정확한 파싱이 필요합니다.
다음과 같이 쿠키를 정확하게 파싱하도록 수정하세요:
- if (cookie != null && cookie.contains("logined=true")) {
+ if (cookie != null && isLoggedIn(cookie)) {
response.response200(Url.USER_LIST.getPath() + ".html");
} else {
response.response302Header(Url.USER_LOGIN_HTML.getPath());
}
}
+
+ private boolean isLoggedIn(String cookie) {
+ String[] cookies = cookie.split(";");
+ for (String c : cookies) {
+ String[] pair = c.trim().split("=", 2);
+ if (pair.length == 2 && "logined".equals(pair[0]) && "true".equals(pair[1])) {
+ return true;
+ }
+ }
+ return false;
+ }📝 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.
| String cookie = request.getHeaders().get("Cookie"); | |
| if (cookie != null && cookie.contains("logined=true")) { | |
| response.response200(Url.USER_LIST.getPath() + ".html"); // ✅ list.html 반환 | |
| } else { | |
| response.response302Header(Url.USER_LOGIN_HTML.getPath()); // ✅ 로그인 안되면 redirect | |
| } | |
| String cookie = request.getHeaders().get("Cookie"); | |
| if (cookie != null && isLoggedIn(cookie)) { | |
| response.response200(Url.USER_LIST.getPath() + ".html"); // ✅ list.html 반환 | |
| } else { | |
| response.response302Header(Url.USER_LOGIN_HTML.getPath()); // ✅ 로그인 안되면 redirect | |
| } | |
| } | |
| private boolean isLoggedIn(String cookie) { | |
| String[] cookies = cookie.split(";"); | |
| for (String c : cookies) { | |
| String[] pair = c.trim().split("=", 2); | |
| if (pair.length == 2 | |
| && "logined".equals(pair[0]) | |
| && "true".equals(pair[1])) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/controller/ListController.java around lines 10 to 16, the
current cookie check uses cookie.contains("logined=true") which is unsafe;
replace it with proper parsing: split the Cookie header by ';', trim each token,
split each token on the first '=' into key and value, and then verify there
exists a key exactly equal to "logined" with a value exactly equal to "true"
(handle null header safely); update the if condition to use this exact-match
result so only a true "logined=true" cookie passes and other similar substrings
do not.
There was a problem hiding this comment.
이 부분은 확인해보시고 private boolean isLoggedIn(String cookie) 검증로직 분리하는게 좋아보입니다
There was a problem hiding this comment.
@jyun-KIM 네, 동의합니다! 쿠키 검증 로직을 private boolean isLoggedIn(String cookie) 메서드로 분리하면 다음과 같은 장점이 있습니다:
- 가독성 향상:
execute메서드가 더 간결해지고 의도가 명확해집니다 - 재사용성: 다른 곳에서도 동일한 검증 로직이 필요할 경우 쉽게 재사용할 수 있습니다
- 테스트 용이성: 검증 로직만 독립적으로 테스트할 수 있습니다
- 유지보수성: 쿠키 검증 방식이 변경되어도 한 곳만 수정하면 됩니다
PR 작성자분께서 확인 후 적용해주시면 좋을 것 같습니다! 👍
| String userId = params.get(UserQueryKey.USER_ID.getKey()); | ||
| String password = params.get(UserQueryKey.PASSWORD.getKey()); |
There was a problem hiding this comment.
파라미터 null 체크가 누락되었습니다.
params.get()의 반환값이 null일 수 있으므로 Line 25의 findUserById(userId)나 Line 27의 equals(password) 호출 시 NullPointerException이 발생할 수 있습니다.
다음과 같이 null 체크를 추가하세요:
String userId = params.get(UserQueryKey.USER_ID.getKey());
String password = params.get(UserQueryKey.PASSWORD.getKey());
+
+ if (userId == null || password == null) {
+ response.response302Header(Url.USER_LOGIN_HTML.getPath());
+ return;
+ }📝 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.
| String userId = params.get(UserQueryKey.USER_ID.getKey()); | |
| String password = params.get(UserQueryKey.PASSWORD.getKey()); | |
| String userId = params.get(UserQueryKey.USER_ID.getKey()); | |
| String password = params.get(UserQueryKey.PASSWORD.getKey()); | |
| if (userId == null || password == null) { | |
| response.response302Header(Url.USER_LOGIN_HTML.getPath()); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/controller/LoginController.java around lines 22 to 23, the
calls to params.get(...) for USER_ID and PASSWORD may return null which can
cause NullPointerException later when calling findUserById(userId) or
password.equals(...). Add explicit null checks immediately after retrieving
userId and password: if either is null (or blank) return an appropriate error
response or throw a handled exception before proceeding; alternatively, validate
and provide a safe default only after confirming non-null. Ensure subsequent
calls use the validated non-null values.
|
|
||
| User user = MemoryUserRepository.getInstance().findUserById(userId); | ||
|
|
||
| if (user != null && user.getPassword().equals(password)) { |
There was a problem hiding this comment.
타이밍 공격에 취약한 비밀번호 비교입니다.
equals()를 사용한 문자열 비교는 타이밍 공격(timing attack)에 취약합니다. 공격자가 응답 시간을 분석하여 비밀번호를 추측할 수 있습니다.
다음과 같이 상수 시간 비교를 사용하세요:
- if (user != null && user.getPassword().equals(password)) {
+ if (user != null && constantTimeEquals(user.getPassword(), password)) {
response.response302WithCookie(Url.INDEX.getPath(), "logined=true");
} else {
response.response302Header(Url.USER_LOGIN_FAILED.getPath());
}
}
+
+ private boolean constantTimeEquals(String a, String b) {
+ if (a == null || b == null) {
+ return a == b;
+ }
+ if (a.length() != b.length()) {
+ return false;
+ }
+ int result = 0;
+ for (int i = 0; i < a.length(); i++) {
+ result |= a.charAt(i) ^ b.charAt(i);
+ }
+ return result == 0;
}📝 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.
| if (user != null && user.getPassword().equals(password)) { | |
| if (user != null && constantTimeEquals(user.getPassword(), password)) { | |
| response.response302WithCookie(Url.INDEX.getPath(), "logined=true"); | |
| } else { | |
| response.response302Header(Url.USER_LOGIN_FAILED.getPath()); | |
| } | |
| } | |
| private boolean constantTimeEquals(String a, String b) { | |
| if (a == null || b == null) { | |
| return a == b; | |
| } | |
| if (a.length() != b.length()) { | |
| return false; | |
| } | |
| int result = 0; | |
| for (int i = 0; i < a.length(); i++) { | |
| result |= a.charAt(i) ^ b.charAt(i); | |
| } | |
| return result == 0; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/controller/LoginController.java around line 27, the direct
String.equals(password) comparison is vulnerable to timing attacks; replace it
with a constant-time comparison of credential bytes or, preferably, use a proper
password-hashing verifier. Concretely: obtain the stored password hash (or
stored password bytes) and the supplied password bytes using a fixed charset,
compute/compare hashes using a constant-time method (e.g., MessageDigest.isEqual
on byte[]), or switch to a secure password encoder (e.g.,
BCryptPasswordEncoder.matches) which performs safe, constant-time verification;
also ensure null checks are handled before comparison.
| String body = ""; | ||
| if (headers.containsKey("Content-Length")) { | ||
| int contentLength = Integer.parseInt(headers.get("Content-Length")); | ||
| body = IOUtils.readData(br, contentLength); | ||
| } |
There was a problem hiding this comment.
Content-Length 파싱에 검증을 추가하세요.
Integer.parseInt()가 잘못된 값(예: "abc")에 대해 NumberFormatException을 발생시킬 수 있습니다.
다음 diff를 적용하여 예외 처리를 추가하세요:
// <바디>
String body = "";
if (headers.containsKey("Content-Length")) {
- int contentLength = Integer.parseInt(headers.get("Content-Length"));
+ try {
+ int contentLength = Integer.parseInt(headers.get("Content-Length"));
+ if (contentLength < 0) {
+ throw new IOException("Invalid Content-Length: " + contentLength);
+ }
- body = IOUtils.readData(br, contentLength);
+ body = IOUtils.readData(br, contentLength);
+ } catch (NumberFormatException e) {
+ throw new IOException("Invalid Content-Length format", e);
+ }
}📝 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.
| String body = ""; | |
| if (headers.containsKey("Content-Length")) { | |
| int contentLength = Integer.parseInt(headers.get("Content-Length")); | |
| body = IOUtils.readData(br, contentLength); | |
| } | |
| String body = ""; | |
| if (headers.containsKey("Content-Length")) { | |
| try { | |
| int contentLength = Integer.parseInt(headers.get("Content-Length")); | |
| if (contentLength < 0) { | |
| throw new IOException("Invalid Content-Length: " + contentLength); | |
| } | |
| body = IOUtils.readData(br, contentLength); | |
| } catch (NumberFormatException e) { | |
| throw new IOException("Invalid Content-Length format", e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/http/HttpRequest.java around lines 44 to 48, the code parses
the Content-Length header with Integer.parseInt which can throw
NumberFormatException; validate and handle this by wrapping the parse in a
try/catch (catch NumberFormatException) and only use the parsed value if it's a
non-negative reasonable integer (e.g., > = 0 and below a max threshold),
otherwise treat as missing/zero and do not attempt to read the body; ensure any
parse error is logged or handled gracefully so the server does not crash.
| public void response302WithCookie(String location, String cookie) { | ||
| try { | ||
| dos.writeBytes("HTTP/1.1 " + HttpStatusCode.FOUND.getMessage() + "\r\n"); | ||
| dos.writeBytes("Location: " + location + "\r\n"); | ||
| dos.writeBytes("Set-Cookie: " + cookie + "\r\n"); | ||
| dos.writeBytes("\r\n"); | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| } |
There was a problem hiding this comment.
예외 처리를 Logger로 전환하세요.
printStackTrace()는 프로덕션 환경에서 적절하지 않습니다. 클래스에 이미 Logger가 정의되어 있으므로 이를 사용하세요.
다음 diff를 적용하세요:
public void response302WithCookie(String location, String cookie) {
try {
dos.writeBytes("HTTP/1.1 " + HttpStatusCode.FOUND.getMessage() + "\r\n");
dos.writeBytes("Location: " + location + "\r\n");
dos.writeBytes("Set-Cookie: " + cookie + "\r\n");
dos.writeBytes("\r\n");
} catch (IOException e) {
- e.printStackTrace();
+ log.log(Level.SEVERE, "Failed to write 302 response with cookie", e);
}
}동일한 패턴을 다른 메서드(lines 30-38, 40-64, 66-75)에도 적용하세요.
📝 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.
| public void response302WithCookie(String location, String cookie) { | |
| try { | |
| dos.writeBytes("HTTP/1.1 " + HttpStatusCode.FOUND.getMessage() + "\r\n"); | |
| dos.writeBytes("Location: " + location + "\r\n"); | |
| dos.writeBytes("Set-Cookie: " + cookie + "\r\n"); | |
| dos.writeBytes("\r\n"); | |
| } catch (IOException e) { | |
| e.printStackTrace(); | |
| } | |
| } | |
| public void response302WithCookie(String location, String cookie) { | |
| try { | |
| dos.writeBytes("HTTP/1.1 " + HttpStatusCode.FOUND.getMessage() + "\r\n"); | |
| dos.writeBytes("Location: " + location + "\r\n"); | |
| dos.writeBytes("Set-Cookie: " + cookie + "\r\n"); | |
| dos.writeBytes("\r\n"); | |
| } catch (IOException e) { | |
| log.log(Level.SEVERE, "Failed to write 302 response with cookie", e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/http/HttpResponse.java around lines 19 to 28, the catch block
calls e.printStackTrace(); replace this with the class Logger (already defined)
to log the exception (e.g., logger.error with a concise message and the
exception) so stack traces go to the logging system; apply the same change to
the other catch blocks in this class at lines 30-38, 40-64, and 66-75, ensuring
each catch logs an informative message plus the exception instead of using
printStackTrace().
| public void response200(String path) { | ||
| try { | ||
| File file = new File("./webapp" + path); | ||
| if (!file.exists()) { | ||
| response404Header(); | ||
| return; | ||
| } | ||
|
|
||
| byte[] body = Files.readAllBytes(file.toPath()); | ||
|
|
||
| dos.writeBytes("HTTP/1.1 " + HttpStatusCode.OK.getMessage() + "\r\n"); | ||
| if (path.endsWith(".css")) { | ||
| dos.writeBytes("Content-Type: text/css\r\n"); | ||
| } else { | ||
| dos.writeBytes("Content-Type: text/html;charset=utf-8\r\n"); | ||
| } | ||
| dos.writeBytes("Content-Length: " + body.length + "\r\n"); | ||
| dos.writeBytes("\r\n"); | ||
|
|
||
| dos.write(body, 0, body.length); | ||
| dos.flush(); | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| } |
There was a problem hiding this comment.
경로 검증 및 리소스 관리를 개선하세요.
현재 구현에는 다음 문제가 있습니다:
- 경로 순회(Path Traversal) 취약점:
path에../가 포함되면 웹 루트 외부의 파일에 접근할 수 있습니다. - 메모리 문제: 파일 전체를 메모리로 읽어 큰 파일 시
OutOfMemoryError발생 가능 - Content-Type 감지 부족:
.css만 확인하고.js,.png,.json등은 무시
다음 diff를 적용하여 경로 검증을 추가하세요:
public void response200(String path) {
try {
+ // 경로 순회 방지
+ if (path.contains("..")) {
+ response404Header();
+ return;
+ }
File file = new File("./webapp" + path);
+ // 정규화된 경로가 webapp 디렉토리 내부인지 확인
+ String canonicalPath = file.getCanonicalPath();
+ String webappPath = new File("./webapp").getCanonicalPath();
+ if (!canonicalPath.startsWith(webappPath)) {
+ response404Header();
+ return;
+ }
if (!file.exists()) {
response404Header();
return;
}Content-Type 감지를 개선하려면 다음을 고려하세요:
String contentType = "text/html;charset=utf-8"; // default
if (path.endsWith(".css")) {
contentType = "text/css";
} else if (path.endsWith(".js")) {
contentType = "application/javascript";
} else if (path.endsWith(".json")) {
contentType = "application/json";
} else if (path.endsWith(".png")) {
contentType = "image/png";
} else if (path.endsWith(".jpg") || path.endsWith(".jpeg")) {
contentType = "image/jpeg";
}
dos.writeBytes("Content-Type: " + contentType + "\r\n");또는 Files.probeContentType(file.toPath())를 사용하세요.
| } catch (Exception e) { | ||
| log.log(Level.SEVERE, e.getMessage()); | ||
| System.out.println(Arrays.toString(e.getStackTrace())); |
There was a problem hiding this comment.
예외 처리 방식을 개선하세요.
현재 구현에는 다음 문제가 있습니다:
Exception을 직접 catch하면InterruptedException등 복구 불가능한 예외까지 잡아 스레드 상태를 손상시킬 수 있습니다.Arrays.toString(e.getStackTrace())는 단일 라인 문자열을 생성하여 가독성이 떨어집니다.
다음 diff를 적용하여 예외 처리를 개선하세요:
- } catch (Exception e) {
+ } catch (IOException e) {
log.log(Level.SEVERE, e.getMessage());
- System.out.println(Arrays.toString(e.getStackTrace()));
+ log.log(Level.SEVERE, "Stack trace:", e);
}또는 더 정확한 예외 타입이 필요하다면 멀티 catch를 사용하세요:
- } catch (Exception e) {
+ } catch (IOException | IllegalArgumentException e) {
log.log(Level.SEVERE, e.getMessage());
- System.out.println(Arrays.toString(e.getStackTrace()));
+ log.log(Level.SEVERE, "Stack trace:", e);
}📝 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.
| } catch (Exception e) { | |
| log.log(Level.SEVERE, e.getMessage()); | |
| System.out.println(Arrays.toString(e.getStackTrace())); | |
| } catch (IOException e) { | |
| log.log(Level.SEVERE, e.getMessage()); | |
| log.log(Level.SEVERE, "Stack trace:", e); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/webserver/RequestHandler.java around lines 37 to 39, do not
catch the broad Exception and do not print the stack trace with Arrays.toString;
instead catch only the expected checked exceptions (or use multi-catch for
specific types), remove the System.out.println call, and log the exception with
the logger's throwable-aware API (e.g., log.log(Level.SEVERE, "Descriptive
message", e)); if you must catch InterruptedException ensure you restore the
thread's interrupt status with Thread.currentThread().interrupt() before
returning or rethrowing.
|
|
||
| public void proceed() throws Exception { | ||
| String path = request.getPath(); | ||
| Controller controller = mappings.getOrDefault(path, new ForwardController()); |
There was a problem hiding this comment.
매 요청마다 새로운 ForwardController 인스턴스를 생성합니다.
new ForwardController()를 매번 생성하는 것은 비효율적입니다. 컨트롤러 인스턴스를 재사용하는 것이 좋습니다.
다음과 같이 기본 컨트롤러 인스턴스를 재사용하도록 수정하세요:
+ private static final Controller DEFAULT_CONTROLLER = new ForwardController();
+
static {
mappings.put("/", new HomeController());
mappings.put("/index.html", new ForwardController());
mappings.put("/user/signup", new SignUpController());
mappings.put("/user/login", new LoginController());
mappings.put("/user/userList", new ListController());
}
public void proceed() throws Exception {
String path = request.getPath();
- Controller controller = mappings.getOrDefault(path, new ForwardController());
+ Controller controller = mappings.getOrDefault(path, DEFAULT_CONTROLLER);
controller.execute(request, response);
}📝 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.
| Controller controller = mappings.getOrDefault(path, new ForwardController()); | |
| public class RequestMapper { | |
| private static final Map<String, Controller> mappings = new HashMap<>(); | |
| private static final Controller DEFAULT_CONTROLLER = new ForwardController(); | |
| static { | |
| mappings.put("/", new HomeController()); | |
| mappings.put("/index.html", new ForwardController()); | |
| mappings.put("/user/signup", new SignUpController()); | |
| mappings.put("/user/login", new LoginController()); | |
| mappings.put("/user/userList", new ListController()); | |
| } | |
| public void proceed() throws Exception { | |
| String path = request.getPath(); | |
| Controller controller = mappings.getOrDefault(path, DEFAULT_CONTROLLER); | |
| controller.execute(request, response); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/webserver/RequestMapper.java around line 32, the code creates a
new ForwardController() on every request which is wasteful; change this to reuse
a single shared instance by declaring a private static final ForwardController
DEFAULT_FORWARD_CONTROLLER = new ForwardController() (or similar) in the class
and replace new ForwardController() with DEFAULT_FORWARD_CONTROLLER in the
getOrDefault call so the same controller instance is reused for all requests.
✅ Actions performedReview triggered.
|
| public enum HttpMethod { | ||
| GET, POST, PUT, DELETE; |
|
테스트 코드 작성도 고려해주세요 ! |
2 participants
Summary by CodeRabbit
신규 기능
버그 수정
리팩터링