Open
Conversation
WalkthroughIDE VCS/Gradle config updates; introduction of HTTP request/response abstractions; addition of routing via RequestMapper; new controller interface and implementations (Home, Forward, List, Login, SignUp); WebServer comments; signup form method changed to POST. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant WS as WebServer
participant RH as RequestHandler
participant HRq as HttpRequest
participant HRs as HttpResponse
participant RM as RequestMapper
participant CTR as Controller
C->>WS: TCP 연결
WS->>RH: 소켓 핸들러 실행
RH->>HRq: BufferedReader로 from(...) 파싱
RH->>HRs: DataOutputStream으로 생성
RH->>RM: new RequestMapper(HRq, HRs)
RM->>RM: URL→컨트롤러 매핑 조회
RM->>CTR: execute(request, response)
alt Forward
CTR->>HRs: forward(request.getUrl())
else Home
CTR->>HRs: forward("/index.html")
else Login
CTR->>HRs: redirect("/index.html?logined=true")
else SignUp
CTR->>HRs: redirect("/index.html")
else List
CTR->>HRs: redirect("/user/login.html") or redirect("/user/list.html")
end
HRs-->>C: HTTP 응답 전송
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ 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[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 (7)
src/main/java/enums/HttpHeader.java (1)
16-18: 메서드 이름이 HttpMethod와 일관성이 없습니다.
HttpHeader.value()와HttpMethod.getValue()의 메서드 명명이 다릅니다. API 일관성을 위해 둘 중 하나로 통일하는 것이 좋습니다.옵션 1: HttpHeader를 getValue()로 변경
- public String value() { + public String getValue() { return value; }옵션 2: HttpMethod를 value()로 변경 (HttpMethod.java에서)
- public String getValue() { + public String value() { return value; }권장:
getValue()로 통일 (Java 관례상 더 명확함)src/main/java/controller/LoginController.java (1)
15-15: 필드를 final로 선언하세요.
memoryUserRepository는 재할당되지 않으므로final로 선언하여 불변성을 보장하는 것이 좋습니다.- MemoryUserRepository memoryUserRepository = MemoryUserRepository.getInstance(); + private final MemoryUserRepository memoryUserRepository = MemoryUserRepository.getInstance();src/main/java/webserver/RequestMapper.java (2)
16-24: 생성자에서 null 체크 필요
httpRequest또는httpResponse파라미터가 null인 경우 NPE가 발생할 수 있습니다. 생성자 시작 부분에서 null 체크를 추가하는 것을 권장합니다.public RequestMapper(HttpRequest httpRequest, HttpResponse httpResponse) { + if (httpRequest == null || httpResponse == null) { + throw new IllegalArgumentException("HttpRequest and HttpResponse must not be null"); + } this.request = httpRequest; this.response = httpResponse;
14-14: 컨트롤러 맵을 정적 final 필드로 고려컨트롤러 맵은 모든 요청에 대해 동일하므로, 매 요청마다 새로 생성하는 대신 정적 final 필드로 선언하여 메모리 효율성을 개선할 수 있습니다.
+private static final Map<String, Controller> CONTROLLERS = new HashMap<>(); + +static { + CONTROLLERS.put("/", new HomeController()); + CONTROLLERS.put("/user/signup", new SignUpController()); + CONTROLLERS.put("/user/login", new LoginController()); + CONTROLLERS.put("/user/userList", new ListController()); +} + HttpRequest request; HttpResponse response; -Map<String, Controller> controllers = new HashMap<>(); public RequestMapper(HttpRequest httpRequest, HttpResponse httpResponse) { this.request = httpRequest; this.response = httpResponse; - - controllers.put("/", new HomeController()); - controllers.put("/user/signup", new SignUpController()); - controllers.put("/user/login", new LoginController()); - controllers.put("/user/userList", new ListController()); }src/main/java/webserver/RequestHandler.java (1)
97-100: 예외 스택 트레이스 로깅 개선
Arrays.toString(e.getStackTrace())보다는e를 직접 로깅하거나log.log(Level.SEVERE, "Error", e)를 사용하는 것이 스택 트레이스를 더 명확하게 출력합니다.} catch (Exception e) { - log.log(Level.SEVERE, e.getMessage()); - System.out.println(Arrays.toString(e.getStackTrace())); + log.log(Level.SEVERE, "Error processing request", e); }src/main/java/webserver/HttpRequest.java (1)
12-16: 필드를 private final로 변경 권장현재 필드들이 package-private이어서 같은 패키지 내에서 직접 수정 가능합니다. 불변성을 보장하기 위해
private final로 선언하는 것을 권장합니다.-String method; -String path; -String version; -Map<String,String> headers; -String body; +private final String method; +private final String path; +private final String version; +private final Map<String,String> headers; +private final String body;src/main/java/webserver/HttpResponse.java (1)
13-13: 필드를 private final로 변경 권장
dos필드가 package-private이며 재할당 가능합니다. 불변성을 위해private final로 선언하는 것을 권장합니다.-DataOutputStream dos; +private final DataOutputStream dos;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.idea/gradle.xml(1 hunks).idea/vcs.xml(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/enums/HttpHeader.java(1 hunks)src/main/java/enums/HttpMethod.java(1 hunks)src/main/java/webserver/HttpRequest.java(1 hunks)src/main/java/webserver/HttpResponse.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)webapp/user/form.html(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/main/java/controller/ListController.java (2)
src/main/java/webserver/HttpRequest.java (1)
HttpRequest(10-77)src/main/java/webserver/HttpResponse.java (1)
HttpResponse(11-102)
src/main/java/controller/Controller.java (2)
src/main/java/webserver/HttpRequest.java (1)
HttpRequest(10-77)src/main/java/webserver/HttpResponse.java (1)
HttpResponse(11-102)
src/main/java/webserver/HttpRequest.java (1)
src/main/java/http/util/IOUtils.java (1)
IOUtils(6-21)
src/main/java/controller/SignUpController.java (5)
src/main/java/db/MemoryUserRepository.java (1)
MemoryUserRepository(9-35)src/main/java/http/util/HttpRequestUtils.java (1)
HttpRequestUtils(8-20)src/main/java/model/User.java (1)
User(5-46)src/main/java/webserver/HttpRequest.java (1)
HttpRequest(10-77)src/main/java/webserver/HttpResponse.java (1)
HttpResponse(11-102)
src/main/java/controller/HomeController.java (2)
src/main/java/webserver/HttpRequest.java (1)
HttpRequest(10-77)src/main/java/webserver/HttpResponse.java (1)
HttpResponse(11-102)
src/main/java/webserver/RequestHandler.java (5)
src/main/java/db/MemoryUserRepository.java (1)
MemoryUserRepository(9-35)src/main/java/http/util/HttpRequestUtils.java (1)
HttpRequestUtils(8-20)src/main/java/http/util/IOUtils.java (1)
IOUtils(6-21)src/main/java/model/User.java (1)
User(5-46)src/main/java/webserver/HttpRequest.java (1)
HttpRequest(10-77)
src/main/java/controller/ForwardController.java (2)
src/main/java/webserver/HttpRequest.java (1)
HttpRequest(10-77)src/main/java/webserver/HttpResponse.java (1)
HttpResponse(11-102)
src/main/java/controller/LoginController.java (4)
src/main/java/db/MemoryUserRepository.java (1)
MemoryUserRepository(9-35)src/main/java/http/util/HttpRequestUtils.java (1)
HttpRequestUtils(8-20)src/main/java/model/User.java (1)
User(5-46)src/main/java/webserver/HttpResponse.java (1)
HttpResponse(11-102)
🔇 Additional comments (6)
.idea/gradle.xml (1)
3-3: Gradle 마이그레이션 설정 추가 문제없음IDE 메타데이터에
GradleMigrationSettings가 추가된 것으로 보이며, 실행 동작에 영향이 없어 보입니다.webapp/user/form.html (1)
58-58: 회원가입 폼 메서드를 POST로 변경한 점 좋습니다.민감한 사용자 정보(비밀번호 등)를 URL에 노출시키지 않기 위해 GET 대신 POST를 사용하는 것이 보안상 올바른 선택입니다.
src/main/java/webserver/WebServer.java (1)
24-30: 한글 주석이 코드 이해에 도움이 됩니다.ServerSocket, accept 루프, 스레드 풀 제출 과정에 대한 설명이 명확합니다.
src/main/java/controller/HomeController.java (1)
8-13: 깔끔한 구현입니다.홈 페이지로 포워딩하는 간단하고 명확한 컨트롤러 구현입니다.
src/main/java/enums/HttpMethod.java (1)
3-23: HttpMethod 열거형 구현이 잘 되어 있습니다.표준 HTTP 메서드들을 타입 안전하게 관리할 수 있는 열거형입니다. 현재 코드에서는 사용되지 않지만 향후
HttpRequest에서 문자열 대신 이 enum을 사용하면 타입 안전성이 향상됩니다.참고: Line 12의 후행 쉼표(trailing comma)는 Java에서 유효한 문법이며, 향후 enum 값 추가 시 diff를 깔끔하게 유지하는 장점이 있습니다.
src/main/java/controller/Controller.java (1)
8-12: 인터페이스 설계 우수컨트롤러 인터페이스가 명확하고 간결합니다. execute 메서드의 시그니처가 적절하며 IOException 선언도 올바릅니다.
Comment on lines
+11
to
+12
| public void execute(HttpRequest request, HttpResponse response) throws IOException { | ||
| response.forward(request.getUrl()); |
There was a problem hiding this comment.
🧩 Analysis chain
경로 탐색(Path Traversal) 취약점 가능성이 있습니다.
request.getUrl()의 값을 검증 없이 response.forward()에 전달하면 ../ 같은 상대 경로를 통해 웹 애플리케이션 루트 디렉토리 외부의 파일에 접근할 수 있습니다.
HttpResponse.forward()가 new File("./webapp" + path)를 사용하므로, 악의적인 요청 (예: GET /../../etc/passwd)이 시스템 파일을 노출시킬 수 있습니다.
다음 스크립트로 HttpResponse.forward의 경로 검증 여부를 확인하세요:
경로 검증이 없다면 다음과 같이 수정하세요:
@Override
public void execute(HttpRequest request, HttpResponse response) throws IOException {
- response.forward(request.getUrl());
+ String path = request.getUrl();
+ // 경로 탐색 방지: 상대 경로 제거
+ if (path.contains("..") || !path.startsWith("/")) {
+ response.forward("/error/400.html");
+ return;
+ }
+ response.forward(path);
}또는 HttpResponse.forward() 내부에서 경로를 정규화하여 보안을 강화하세요.
🏁 Script executed:
#!/bin/bash
# HttpResponse.forward 메서드의 경로 검증 로직 확인
rg -A 10 'public void forward' src/main/java/webserver/HttpResponse.javaLength of output: 489
경로 탐색(Path Traversal) 취약점 확인 및 방어 구현 필요
request.getUrl()에서 전달받은 경로가 검증·정규화되지 않아 ../ 입력으로 웹앱 외부 파일(예: /etc/passwd)에 접근될 수 있습니다.
HttpResponse.forward() 내부 또는 호출부에서 입력 경로를 Path.normalize() 등으로 정규화하고, 허용된 웹앱 디렉터리 경로로만 제한(예: normalizedPath.startsWith(webappRoot))한 뒤 처리하세요.
🤖 Prompt for AI Agents
In src/main/java/controller/ForwardController.java around lines 11-12, the
controller forwards using request.getUrl() without validation which allows path
traversal; normalize and resolve the requested path against the webapp root and
reject any paths that escape it: obtain the webapp root Path, resolve the
request URL against it, call normalize() (and preferably toRealPath() or
toAbsolutePath() to canonicalize), verify the resolved path startsWith the
webapp root canonical path, and if not respond with an error (400/403) and log
the event; only call response.forward() with the validated normalized path.
Comment on lines
+14
to
+20
| String cookie = request.getHeaders().get(HttpHeader.COOKIE.value()); | ||
|
|
||
| if(cookie == null) { | ||
| response.redirect("/user/login.html", null); | ||
| }else if(cookie.equals("logined=true")) { | ||
| response.redirect("/user/list.html", null); | ||
| } |
There was a problem hiding this comment.
쿠키 기반 인증에 심각한 보안 취약점이 있습니다.
현재 구현은 다음 문제들을 가지고 있습니다:
- 인증 우회 가능: 클라이언트가 브라우저에서
logined=true쿠키를 직접 설정하면 인증 없이 사용자 목록에 접근할 수 있습니다. - 서버 측 검증 부재: 세션 토큰이나 서명된 쿠키 없이 단순 문자열만 비교합니다.
- 불완전한 로직: 쿠키가 존재하지만
"logined=true"가 아닌 경우 응답을 보내지 않습니다.
다음 개선 사항을 적용하세요:
+import java.util.UUID;
+// 서버 측에 세션 저장소 추가 필요
+
public class ListController implements Controller {
@Override
public void execute(HttpRequest request, HttpResponse response) throws IOException {
String cookie = request.getHeaders().get(HttpHeader.COOKIE.value());
- if(cookie == null) {
+ // 세션 ID 검증 로직 추가
+ String sessionId = extractSessionId(cookie);
+ if(sessionId == null || !isValidSession(sessionId)) {
response.redirect("/user/login.html", null);
- }else if(cookie.equals("logined=true")) {
+ return;
+ }
+
+ if(isValidSession(sessionId)) {
response.redirect("/user/list.html", null);
+ return;
}
-
+
+ response.redirect("/user/login.html", null);
}
+
+ private String extractSessionId(String cookie) {
+ // 쿠키에서 세션 ID 추출
+ }
+
+ private boolean isValidSession(String sessionId) {
+ // 서버 측 세션 저장소에서 검증
+ }
}또는 최소한 다음과 같이 수정하여 빠진 else 케이스를 처리하세요:
if(cookie == null) {
response.redirect("/user/login.html", null);
+ return;
}else if(cookie.equals("logined=true")) {
response.redirect("/user/list.html", null);
+ return;
}
-
+
+ // 쿠키가 있지만 유효하지 않은 경우
+ response.redirect("/user/login.html", null);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/controller/ListController.java around lines 14 to 20 the
current logic trusts a plain "logined=true" cookie and omits handling for other
cookie values, creating an authentication bypass and missing server-side
validation; replace the simple string check with server-side session validation
(e.g., read a session ID or signed token from the cookie and validate it against
your session store or verify its signature and expiry), ensure the else branch
handles invalid/missing tokens by redirecting to /user/login.html or returning
401, and set secure cookie attributes (HttpOnly, Secure, SameSite) when issuing
auth cookies; in short: validate tokens server-side, handle the missing/invalid
cookie path, and enforce secure cookie properties.
Comment on lines
+21
to
+27
| Map<String, String> queryParameter = HttpRequestUtils.parseQueryParameter(request.getBody()); | ||
| User loginUser = memoryUserRepository.findUserById(queryParameter.get("username")); | ||
|
|
||
| if(loginUser != null) { | ||
| response.redirect("/index.html", "logined=true"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
비밀번호 검증이 누락되어 인증 우회가 가능합니다.
현재 구현은 사용자 존재 여부만 확인하고 비밀번호를 검증하지 않습니다. 데이터베이스에 존재하는 임의의 사용자 이름만 알면 비밀번호 없이 로그인할 수 있는 심각한 보안 취약점입니다.
다음과 같이 수정하세요:
Map<String, String> queryParameter = HttpRequestUtils.parseQueryParameter(request.getBody());
- User loginUser = memoryUserRepository.findUserById(queryParameter.get("username"));
+ String username = queryParameter.get("username");
+ String password = queryParameter.get("password");
+
+ User loginUser = memoryUserRepository.findUserById(username);
- if(loginUser != null) {
+ if(loginUser != null && loginUser.getPassword().equals(password)) {
response.redirect("/index.html", "logined=true");
return;
}추가 권장사항: 실제 프로덕션 환경에서는 평문 비밀번호 비교 대신 bcrypt 등의 해시 함수를 사용해야 합니다.
📝 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
| Map<String, String> queryParameter = HttpRequestUtils.parseQueryParameter(request.getBody()); | |
| User loginUser = memoryUserRepository.findUserById(queryParameter.get("username")); | |
| if(loginUser != null) { | |
| response.redirect("/index.html", "logined=true"); | |
| return; | |
| } | |
| Map<String, String> queryParameter = HttpRequestUtils.parseQueryParameter(request.getBody()); | |
| String username = queryParameter.get("username"); | |
| String password = queryParameter.get("password"); | |
| User loginUser = memoryUserRepository.findUserById(username); | |
| if (loginUser != null && loginUser.getPassword().equals(password)) { | |
| response.redirect("/index.html", "logined=true"); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/controller/LoginController.java around lines 21 to 27, the code
only checks that a user exists and skips password verification, allowing
authentication bypass; retrieve the password from queryParameter (e.g.,
"password"), compare it against the stored password for loginUser and only call
response.redirect("/index.html", "logined=true") when the passwords match; on
mismatch or missing password return an appropriate failure response (e.g.,
redirect back with an error flag or set 401), and in comments or docs note to
use a secure hash (bcrypt) and compare hashed values in production instead of
plain-text comparison.
Comment on lines
+23
to
+28
| if (request.getMethod().equals(HttpMethod.GET.getValue())) { | ||
| String[] tokens = request.getUrl().split("\\?"); | ||
| queryString = tokens[1]; | ||
| } else if (request.getMethod().equals(HttpMethod.POST.getValue())) { | ||
| queryString = request.getBody(); | ||
| } |
There was a problem hiding this comment.
URL 파싱 시 ArrayIndexOutOfBoundsException 위험
GET 요청 시 URL에 ?가 없으면 Line 25에서 tokens[1]에 접근할 때 ArrayIndexOutOfBoundsException이 발생합니다.
//get 방식인 경우
if (request.getMethod().equals(HttpMethod.GET.getValue())) {
String[] tokens = request.getUrl().split("\\?");
- queryString = tokens[1];
+ if (tokens.length > 1) {
+ queryString = tokens[1];
+ }
} else if (request.getMethod().equals(HttpMethod.POST.getValue())) {📝 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
| if (request.getMethod().equals(HttpMethod.GET.getValue())) { | |
| String[] tokens = request.getUrl().split("\\?"); | |
| queryString = tokens[1]; | |
| } else if (request.getMethod().equals(HttpMethod.POST.getValue())) { | |
| queryString = request.getBody(); | |
| } | |
| if (request.getMethod().equals(HttpMethod.GET.getValue())) { | |
| String[] tokens = request.getUrl().split("\\?"); | |
| if (tokens.length > 1) { | |
| queryString = tokens[1]; | |
| } | |
| } else if (request.getMethod().equals(HttpMethod.POST.getValue())) { | |
| queryString = request.getBody(); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/controller/SignUpController.java around lines 23 to 28, the GET
branch assumes request.getUrl() contains a '?' and uses tokens[1], risking
ArrayIndexOutOfBoundsException; change the logic to check for the presence of
'?' (or verify tokens.length > 1) before accessing tokens[1], and if absent set
queryString to an empty string (or null) so the code safely handles URLs without
query parameters; also defensively ensure the POST branch uses a non-null body
(fallback to empty string) to avoid NPEs.
Comment on lines
+30
to
+38
| Map<String, String> queryParameter = HttpRequestUtils.parseQueryParameter(queryString); | ||
|
|
||
| User user = new User( | ||
| queryParameter.get("username"), | ||
| queryParameter.get("password"), | ||
| queryParameter.get("email"), | ||
| queryParameter.get("phone") | ||
| ); | ||
| memoryUserRepository.addUser(user); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
입력 값 검증 및 중복 사용자 체크 누락
사용자 입력에 대한 검증(null 체크, 빈 문자열 체크)이 없으며, 이미 존재하는 사용자 ID에 대한 중복 체크도 없습니다.
Map<String, String> queryParameter = HttpRequestUtils.parseQueryParameter(queryString);
+String username = queryParameter.get("username");
+String password = queryParameter.get("password");
+String name = queryParameter.get("name");
+String email = queryParameter.get("email");
+
+// 입력 값 검증
+if (username == null || username.trim().isEmpty() ||
+ password == null || password.trim().isEmpty()) {
+ response.redirect("/user/form.html", null);
+ return;
+}
+
+// 중복 사용자 체크
+if (memoryUserRepository.findUserById(username) != null) {
+ response.redirect("/user/form.html", null);
+ return;
+}
+
User user = new User(
- queryParameter.get("username"),
- queryParameter.get("password"),
- queryParameter.get("email"),
- queryParameter.get("phone")
+ username,
+ password,
+ name,
+ email
);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/controller/SignUpController.java around lines 30 to 38, the
code constructs a User directly from parsed query parameters without validating
inputs or checking for duplicate usernames; update the handler to (1) validate
each required parameter (username, password, email, phone) for null and empty
values and return a client error if any are missing/invalid, (2) optionally
validate formats (e.g., email regex, minimal password length) and reject invalid
formats, (3) query memoryUserRepository to detect an existing user with the same
username and return a conflict error if found, and (4) only create and add the
new User when all validations pass; ensure error responses include a clear
message and appropriate HTTP status codes.
Comment on lines
+22
to
+35
| public void forward(String path) throws IOException { | ||
|
|
||
| File file = new File("./webapp"+path); | ||
|
|
||
| if(file.exists()) { //존재한다면 파일 출력 | ||
| byte[] body = java.nio.file.Files.readAllBytes(file.toPath()); | ||
| response200Header(dos, body.length, getContentType(path)); | ||
| responseBody(dos, body); | ||
| } else { //존재하지 않는다면 404 not found 출력 | ||
| byte[] body = "<h1>404 Not Found</h1>".getBytes(); | ||
| response404Header(dos, body.length); | ||
| responseBody(dos, body); | ||
| } | ||
| } |
There was a problem hiding this comment.
경로 순회(Path Traversal) 취약점
Line 24에서 path 파라미터를 검증 없이 파일 경로로 사용하고 있습니다. 공격자가 ../../../etc/passwd 같은 경로를 전달하면 웹 루트 외부의 파일에 접근할 수 있습니다.
public void forward(String path) throws IOException {
+ // 경로 정규화 및 검증
+ String normalizedPath = new File(path).getCanonicalPath();
+ String webappPath = new File("./webapp").getCanonicalPath();
+
+ if (!normalizedPath.startsWith(webappPath)) {
+ byte[] body = "<h1>403 Forbidden</h1>".getBytes();
+ response404Header(dos, body.length);
+ responseBody(dos, body);
+ return;
+ }
- File file = new File("./webapp"+path);
+ File file = new File(webappPath + path);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/webserver/HttpResponse.java around lines 22 to 35, the forward
method uses the incoming path directly to construct a File, allowing path
traversal (e.g. ../../../etc/passwd). Normalize and validate the requested path
before opening the file: URL-decode the input, reject any null bytes, resolve it
against a fixed web root (e.g. new File(webRoot, path)), obtain the canonical
path of both the web root and the resolved file, and only serve the file if the
file's canonical path startsWith the web root's canonical path; otherwise return
404 or 400. Also disallow segments like ".." after decoding and consider mapping
directory requests to an index file.
Comment on lines
+58
to
+67
| private void response200Header(DataOutputStream dos, int lengthOfBodyContent, String contentType) { | ||
| try { | ||
| dos.writeBytes("HTTP/1.1 200 OK \r\n"); | ||
| dos.writeBytes(HttpHeader.CONTENT_TYPE.value() + ": " + contentType + ";charset=utf-8\r\n"); | ||
| dos.writeBytes( HttpHeader.CONTENT_LENGTH.value() + ": " + lengthOfBodyContent + "\r\n"); | ||
| dos.writeBytes("\r\n"); | ||
| } catch (IOException e) { | ||
| log.log(Level.SEVERE, e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
IOException 처리 방식 개선 필요
Lines 64-66에서 IOException을 catch하여 로깅만 하고 있습니다. 이는 호출자에게 오류를 전달하지 않아 응답이 불완전하게 전송될 수 있습니다. 또한 flush()가 누락되어 있습니다.
-private void response200Header(DataOutputStream dos, int lengthOfBodyContent, String contentType) {
+private void response200Header(DataOutputStream dos, int lengthOfBodyContent, String contentType) throws IOException {
- try {
- dos.writeBytes("HTTP/1.1 200 OK \r\n");
- dos.writeBytes(HttpHeader.CONTENT_TYPE.value() + ": " + contentType + ";charset=utf-8\r\n");
- dos.writeBytes( HttpHeader.CONTENT_LENGTH.value() + ": " + lengthOfBodyContent + "\r\n");
- dos.writeBytes("\r\n");
- } catch (IOException e) {
- log.log(Level.SEVERE, e.getMessage());
- }
+ dos.writeBytes("HTTP/1.1 200 OK \r\n");
+ dos.writeBytes(HttpHeader.CONTENT_TYPE.value() + ": " + contentType + ";charset=utf-8\r\n");
+ dos.writeBytes( HttpHeader.CONTENT_LENGTH.value() + ": " + lengthOfBodyContent + "\r\n");
+ dos.writeBytes("\r\n");
+ dos.flush();
}response404Header와 responseBody 메서드도 동일하게 수정이 필요합니다.
📝 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
| private void response200Header(DataOutputStream dos, int lengthOfBodyContent, String contentType) { | |
| try { | |
| dos.writeBytes("HTTP/1.1 200 OK \r\n"); | |
| dos.writeBytes(HttpHeader.CONTENT_TYPE.value() + ": " + contentType + ";charset=utf-8\r\n"); | |
| dos.writeBytes( HttpHeader.CONTENT_LENGTH.value() + ": " + lengthOfBodyContent + "\r\n"); | |
| dos.writeBytes("\r\n"); | |
| } catch (IOException e) { | |
| log.log(Level.SEVERE, e.getMessage()); | |
| } | |
| } | |
| private void response200Header(DataOutputStream dos, int lengthOfBodyContent, String contentType) throws IOException { | |
| dos.writeBytes("HTTP/1.1 200 OK \r\n"); | |
| dos.writeBytes(HttpHeader.CONTENT_TYPE.value() + ": " + contentType + ";charset=utf-8\r\n"); | |
| dos.writeBytes(HttpHeader.CONTENT_LENGTH.value() + ": " + lengthOfBodyContent + "\r\n"); | |
| dos.writeBytes("\r\n"); | |
| dos.flush(); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/webserver/HttpResponse.java around lines 58 to 67, the methods
catch IOException and only log it (and omit flush), which hides errors and can
leave incomplete responses; update response200Header (and mirror changes in
response404Header and responseBody) to propagate the IOException instead of
swallowing it by either declaring throws IOException and removing the try/catch
or rethrowing the caught exception after logging, and ensure you call
dos.flush() after writing headers and after writing the body so data is sent;
also adjust any callers to handle or declare the propagated IOException.
Comment on lines
+20
to
+74
| //public class RequestHandler implements Runnable { | ||
| // Socket connection; | ||
| // private static final Logger log = Logger.getLogger(RequestHandler.class.getName()); | ||
| // | ||
| // private final Repository repository; | ||
| // private Controller controller = new ForwardController(); | ||
| // | ||
| // | ||
| // public RequestHandler(Socket connection) { | ||
| // this.connection = connection; | ||
| // repository = MemoryUserRepository.getInstance(); | ||
| // } | ||
| // | ||
| // @Override | ||
| // public void run() { | ||
| // log.log(Level.INFO, "New Client Connect! Connected IP : " + connection.getInetAddress() + ", Port : " + connection.getPort()); | ||
| // try (InputStream in = connection.getInputStream(); OutputStream out = connection.getOutputStream()) { | ||
| // BufferedReader br = new BufferedReader(new InputStreamReader(in)); | ||
| // DataOutputStream dos = new DataOutputStream(out); | ||
| // | ||
| // // Header 분석 | ||
| // HttpRequest httpRequest = HttpRequest.from(br); | ||
| // HttpResponse httpResponse = new HttpResponse(dos); | ||
| // | ||
| // // 요구 사항 1번 | ||
| // if (httpRequest.getMethod().equals(HttpMethod.GET.getValue()) && httpRequest.getUrl().endsWith(".html")) { | ||
| // controller = new ForwardController(); | ||
| // } | ||
| // | ||
| // if (httpRequest.getUrl().equals("/")) { | ||
| // controller = new HomeController(); | ||
| // } | ||
| // | ||
| // // 요구 사항 2,3,4번 | ||
| // if (httpRequest.getUrl().equals("/user/signup")) { | ||
| // controller = new SignUpController(); | ||
| // } | ||
| // | ||
| // // 요구 사항 5번 | ||
| // if (httpRequest.getUrl().equals("/user/login")) { | ||
| // controller = new LoginController(); | ||
| // } | ||
| // | ||
| // // 요구 사항 6번 | ||
| // if (httpRequest.getUrl().equals("/user/userList")) { | ||
| // controller = new ListController(); | ||
| // } | ||
| // controller.execute(httpRequest, httpResponse); | ||
| // | ||
| // } catch (Exception e) { | ||
| // log.log(Level.SEVERE, e.getMessage()); | ||
| // System.out.println(Arrays.toString(e.getStackTrace())); | ||
| // } | ||
| // } | ||
| //} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
주석 처리된 코드 제거 필요
이전 구현이 주석으로 남아있습니다. 버전 관리 시스템(Git)에서 이력을 확인할 수 있으므로, 주석 처리된 코드는 제거하는 것이 좋습니다.
-//public class RequestHandler implements Runnable {
-// Socket connection;
-// private static final Logger log = Logger.getLogger(RequestHandler.class.getName());
-//
-// private final Repository repository;
-// private Controller controller = new ForwardController();
-//
-//
-// public RequestHandler(Socket connection) {
-// this.connection = connection;
-// repository = MemoryUserRepository.getInstance();
-// }
-//
-// @Override
-// public void run() {
-// log.log(Level.INFO, "New Client Connect! Connected IP : " + connection.getInetAddress() + ", Port : " + connection.getPort());
-// try (InputStream in = connection.getInputStream(); OutputStream out = connection.getOutputStream()) {
-// BufferedReader br = new BufferedReader(new InputStreamReader(in));
-// DataOutputStream dos = new DataOutputStream(out);
-//
-// // Header 분석
-// HttpRequest httpRequest = HttpRequest.from(br);
-// HttpResponse httpResponse = new HttpResponse(dos);
-//
-// // 요구 사항 1번
-// if (httpRequest.getMethod().equals(HttpMethod.GET.getValue()) && httpRequest.getUrl().endsWith(".html")) {
-// controller = new ForwardController();
-// }
-//
-// if (httpRequest.getUrl().equals("/")) {
-// controller = new HomeController();
-// }
-//
-// // 요구 사항 2,3,4번
-// if (httpRequest.getUrl().equals("/user/signup")) {
-// controller = new SignUpController();
-// }
-//
-// // 요구 사항 5번
-// if (httpRequest.getUrl().equals("/user/login")) {
-// controller = new LoginController();
-// }
-//
-// // 요구 사항 6번
-// if (httpRequest.getUrl().equals("/user/userList")) {
-// controller = new ListController();
-// }
-// controller.execute(httpRequest, httpResponse);
-//
-// } catch (Exception e) {
-// log.log(Level.SEVERE, e.getMessage());
-// System.out.println(Arrays.toString(e.getStackTrace()));
-// }
-// }
-//}
-📝 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
| //public class RequestHandler implements Runnable { | |
| // Socket connection; | |
| // private static final Logger log = Logger.getLogger(RequestHandler.class.getName()); | |
| // | |
| // private final Repository repository; | |
| // private Controller controller = new ForwardController(); | |
| // | |
| // | |
| // public RequestHandler(Socket connection) { | |
| // this.connection = connection; | |
| // repository = MemoryUserRepository.getInstance(); | |
| // } | |
| // | |
| // @Override | |
| // public void run() { | |
| // log.log(Level.INFO, "New Client Connect! Connected IP : " + connection.getInetAddress() + ", Port : " + connection.getPort()); | |
| // try (InputStream in = connection.getInputStream(); OutputStream out = connection.getOutputStream()) { | |
| // BufferedReader br = new BufferedReader(new InputStreamReader(in)); | |
| // DataOutputStream dos = new DataOutputStream(out); | |
| // | |
| // // Header 분석 | |
| // HttpRequest httpRequest = HttpRequest.from(br); | |
| // HttpResponse httpResponse = new HttpResponse(dos); | |
| // | |
| // // 요구 사항 1번 | |
| // if (httpRequest.getMethod().equals(HttpMethod.GET.getValue()) && httpRequest.getUrl().endsWith(".html")) { | |
| // controller = new ForwardController(); | |
| // } | |
| // | |
| // if (httpRequest.getUrl().equals("/")) { | |
| // controller = new HomeController(); | |
| // } | |
| // | |
| // // 요구 사항 2,3,4번 | |
| // if (httpRequest.getUrl().equals("/user/signup")) { | |
| // controller = new SignUpController(); | |
| // } | |
| // | |
| // // 요구 사항 5번 | |
| // if (httpRequest.getUrl().equals("/user/login")) { | |
| // controller = new LoginController(); | |
| // } | |
| // | |
| // // 요구 사항 6번 | |
| // if (httpRequest.getUrl().equals("/user/userList")) { | |
| // controller = new ListController(); | |
| // } | |
| // controller.execute(httpRequest, httpResponse); | |
| // | |
| // } catch (Exception e) { | |
| // log.log(Level.SEVERE, e.getMessage()); | |
| // System.out.println(Arrays.toString(e.getStackTrace())); | |
| // } | |
| // } | |
| //} |
🤖 Prompt for AI Agents
In src/main/java/webserver/RequestHandler.java around lines 20 to 74, remove the
large block of commented-out legacy RequestHandler implementation (the entire
commented class) to clean up the file; after removal, tidy imports and any
unused variables or logger references introduced by the cleanup, ensure the
active implementation remains intact and compiles, and run the build/tests to
verify no regressions before committing.
Comment on lines
+91
to
+95
| HttpRequest httpRequest = HttpRequest.from(br); | ||
| HttpResponse httpResponse = new HttpResponse(dos); | ||
|
|
||
| } catch (IOException e) { | ||
| log.log(Level.SEVERE,e.getMessage()); | ||
| } | ||
| } | ||
| RequestMapper requestMapper = new RequestMapper(httpRequest, httpResponse); | ||
| requestMapper.proceed(); |
There was a problem hiding this comment.
HttpRequest가 null인 경우 처리 필요
HttpRequest.from(br)은 Line 47에서 null을 반환할 수 있습니다 (빈 라인이거나 null인 경우). null 체크 없이 RequestMapper를 생성하면 NPE가 발생합니다.
HttpRequest httpRequest = HttpRequest.from(br);
+if (httpRequest == null) {
+ log.log(Level.WARNING, "Invalid HTTP request received");
+ return;
+}
HttpResponse httpResponse = new HttpResponse(dos);
RequestMapper requestMapper = new RequestMapper(httpRequest, httpResponse);🤖 Prompt for AI Agents
In src/main/java/webserver/RequestHandler.java around lines 91 to 95,
HttpRequest.from(br) can return null (e.g., for empty lines) and the code
currently constructs RequestMapper without a null check causing an NPE; add a
null check immediately after obtaining httpRequest and if it's null, either
close the socket/streams or write an appropriate error response (e.g., 400 Bad
Request) via the existing HttpResponse/dos and return without creating
RequestMapper, ensuring resources are cleaned up.
Comment on lines
+26
to
+35
| public void proceed() throws IOException { | ||
|
|
||
| Controller controller = controllers.get(request.getUrl()); | ||
|
|
||
| if(controller == null) { | ||
| controller = new ForwardController(); | ||
| } | ||
|
|
||
| controller.execute(request, response); | ||
| } |
There was a problem hiding this comment.
request.getUrl() 호출 전 null 체크 필요
Line 28에서 request.getUrl()을 호출하기 전에 request가 null이 아닌지 확인해야 합니다. HttpRequest.from()이 null을 반환할 수 있으므로 NPE 위험이 있습니다.
public void proceed() throws IOException {
+ if (request == null) {
+ throw new IllegalStateException("HttpRequest is null");
+ }
Controller controller = controllers.get(request.getUrl());📝 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
| public void proceed() throws IOException { | |
| Controller controller = controllers.get(request.getUrl()); | |
| if(controller == null) { | |
| controller = new ForwardController(); | |
| } | |
| controller.execute(request, response); | |
| } | |
| public void proceed() throws IOException { | |
| if (request == null) { | |
| throw new IllegalStateException("HttpRequest is null"); | |
| } | |
| Controller controller = controllers.get(request.getUrl()); | |
| if (controller == null) { | |
| controller = new ForwardController(); | |
| } | |
| controller.execute(request, response); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/webserver/RequestMapper.java around lines 26 to 35, the code
calls request.getUrl() without checking if request is null; add a null check for
request before invoking getUrl() and handle the null case (e.g., set controller
= new ForwardController() or return an appropriate error response), then only
call controllers.get(request.getUrl()) when request is non-null; also optionally
log the null-request case for diagnostics.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit