Open
Conversation
Walkthrough
Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as Client
participant RH as RequestHandler
participant Req as HttpRequest
participant Map as RequestMapping
participant Ctrl as Controller
participant Res as HttpResponse
Note over RH,Res: 신규 요청/응답 추상화 기반 흐름
C->>RH: TCP 연결 수립 후 요청 전송
RH->>Req: 입력 스트림으로 HttpRequest 생성
Req-->>RH: 메서드, 경로, 파라미터, 쿠키, 로그인여부
RH->>Map: getController(method, path)
alt 매핑 존재
Map-->>RH: Controller 인스턴스
RH->>Res: HttpResponse 생성
RH->>Ctrl: process(request, response)
alt ForwardController
Ctrl->>Res: forward(path)
Res-->>C: 200 OK + 정적 컨텐츠
else CreateUser/Login/ListUser
Ctrl->>Res: sendRedirect(...)[+옵션 쿠키]
Res-->>C: 302 Found (+Set-Cookie)
end
else 매핑 없음
RH->>Res: forward(정적 경로)
Res-->>C: 200 OK 또는 404
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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: 6
🧹 Nitpick comments (1)
src/main/java/http/util/HttpRequestUtils.java (1)
23-34: 쿠키 파싱에 명시적인 중복 키 처리 전략을 추가하세요.
parseQueryString과 달리parseCookies에는 중복 키에 대한 병합 전략이 명시되지 않았습니다. 일반적으로 쿠키 이름은 중복되지 않지만, 일관성과 방어적 프로그래밍을 위해 명시적인 병합 전략을 추가하는 것을 권장합니다.다음과 같이 수정할 수 있습니다:
return Arrays.stream(cookieString.split("; ")) .map(pair -> pair.split("=", 2)) .filter(parts -> parts.length == 2) .collect(Collectors.toMap( parts -> parts[0], - parts -> parts[1] + parts -> parts[1], + (oldValue, newValue) -> newValue ));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.idea/gradle.xml(1 hunks)src/main/java/controller/Controller.java(1 hunks)src/main/java/controller/CreateUserController.java(1 hunks)src/main/java/controller/ForwardController.java(1 hunks)src/main/java/controller/ListUserController.java(1 hunks)src/main/java/controller/LoginController.java(1 hunks)src/main/java/db/MemoryUserRepository.java(1 hunks)src/main/java/http/util/HttpRequestUtils.java(1 hunks)src/main/java/model/User.java(2 hunks)src/main/java/webserver/HttpRequest.java(1 hunks)src/main/java/webserver/HttpResponse.java(1 hunks)src/main/java/webserver/RequestHandler.java(1 hunks)src/main/java/webserver/RequestMapping.java(1 hunks)webapp/user/form.html(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/main/java/controller/Controller.java (2)
src/main/java/webserver/HttpRequest.java (1)
HttpRequest(11-59)src/main/java/webserver/HttpResponse.java (1)
HttpResponse(11-80)
src/main/java/controller/ListUserController.java (2)
src/main/java/webserver/HttpRequest.java (1)
HttpRequest(11-59)src/main/java/webserver/HttpResponse.java (1)
HttpResponse(11-80)
src/main/java/controller/CreateUserController.java (4)
src/main/java/db/MemoryUserRepository.java (1)
MemoryUserRepository(9-34)src/main/java/model/User.java (1)
User(5-51)src/main/java/webserver/HttpRequest.java (1)
HttpRequest(11-59)src/main/java/webserver/HttpResponse.java (1)
HttpResponse(11-80)
src/main/java/controller/ForwardController.java (2)
src/main/java/webserver/HttpRequest.java (1)
HttpRequest(11-59)src/main/java/webserver/HttpResponse.java (1)
HttpResponse(11-80)
src/main/java/controller/LoginController.java (4)
src/main/java/db/MemoryUserRepository.java (1)
MemoryUserRepository(9-34)src/main/java/model/User.java (1)
User(5-51)src/main/java/webserver/HttpRequest.java (1)
HttpRequest(11-59)src/main/java/webserver/HttpResponse.java (1)
HttpResponse(11-80)
src/main/java/webserver/RequestHandler.java (4)
src/main/java/db/MemoryUserRepository.java (1)
MemoryUserRepository(9-34)src/main/java/model/User.java (1)
User(5-51)src/main/java/http/util/HttpRequestUtils.java (1)
HttpRequestUtils(8-35)src/main/java/webserver/RequestMapping.java (1)
RequestMapping(8-23)
src/main/java/webserver/HttpRequest.java (1)
src/main/java/http/util/HttpRequestUtils.java (1)
HttpRequestUtils(8-35)
🔇 Additional comments (7)
src/main/java/db/MemoryUserRepository.java (1)
9-13: 코드 스타일 개선이 적절합니다.클래스 선언과 생성자의 포맷팅이 일관성 있게 수정되었습니다.
src/main/java/http/util/HttpRequestUtils.java (1)
9-21: 쿼리 스트링 파싱 로직이 올바릅니다.null/empty 입력 검증과 중복 키에 대한 명시적인 병합 전략(
newValue우선)이 적절하게 구현되었습니다.webapp/user/form.html (1)
58-58: 폼 메서드를 POST로 변경한 것이 적절합니다.회원가입과 같이 서버 상태를 변경하는 작업에는 POST 메서드가 올바르며, 비밀번호와 같은 민감 정보가 URL에 노출되지 않습니다.
src/main/java/controller/ListUserController.java (2)
9-11: 인증 체크 로직이 올바릅니다.로그인하지 않은 사용자를 로그인 페이지로 리다이렉트하는 처리가 적절합니다.
13-13: 사용자 목록 데이터 전달을 확인하세요.
response.forward("/user/list.html")는 정적 HTML 파일만 전송합니다. 실제 사용자 목록 데이터(MemoryUserRepository.getInstance().findAll())를 응답에 포함하는 메커니즘이 누락된 것으로 보입니다./user/list.html이 동적으로 렌더링되는 템플릿인지, 아니면 별도의 데이터 전달 방식(예: JSON 응답 또는 템플릿 엔진)이 필요한지 확인하세요.src/main/java/controller/Controller.java (1)
6-7: 컨트롤러 인터페이스 설계가 명확합니다.단일 책임 원칙을 따르는 간결한 인터페이스로, 모든 컨트롤러가 일관된 방식으로 요청을 처리할 수 있도록 합니다.
src/main/java/controller/CreateUserController.java (1)
21-21: 로그에 민감 정보가 포함되지 않는지 확인하세요.현재
User.toString()은 비밀번호를 포함하지 않지만, 향후toString()메서드가 변경될 경우 민감 정보가 로그에 노출될 위험이 있습니다. 명시적으로 로깅할 필드를 지정하는 것을 권장합니다.예시:
- log.info("New User Created: " + user); + log.info("New User Created: userId=" + user.getUserId() + ", name=" + user.getName());
Comment on lines
+14
to
+19
| User user = new User( | ||
| request.getParameter("userId"), | ||
| request.getParameter("password"), | ||
| request.getParameter("name"), | ||
| request.getParameter("email") | ||
| ); |
There was a problem hiding this comment.
입력 검증을 추가하세요.
사용자 입력에 대한 검증이 전혀 없습니다. 다음 검증이 필요합니다:
userId,password,name,email이 null이거나 비어있지 않은지 확인email이 유효한 이메일 형식인지 검증password강도 요구사항 확인 (최소 길이, 복잡도 등)userId가 허용된 문자만 포함하는지 확인
입력 검증 예시:
+ String userId = request.getParameter("userId");
+ String password = request.getParameter("password");
+ String name = request.getParameter("name");
+ String email = request.getParameter("email");
+
+ if (userId == null || userId.trim().isEmpty()) {
+ response.sendRedirect("/user/form.html?error=invalid_userid");
+ return;
+ }
+ // 추가 검증 로직...
+
User user = new User(
- request.getParameter("userId"),
- request.getParameter("password"),
- request.getParameter("name"),
- request.getParameter("email")
+ userId.trim(),
+ password,
+ name.trim(),
+ email.trim()
);📝 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
| User user = new User( | |
| request.getParameter("userId"), | |
| request.getParameter("password"), | |
| request.getParameter("name"), | |
| request.getParameter("email") | |
| ); | |
| String userId = request.getParameter("userId"); | |
| String password = request.getParameter("password"); | |
| String name = request.getParameter("name"); | |
| String email = request.getParameter("email"); | |
| if (userId == null || userId.trim().isEmpty()) { | |
| response.sendRedirect("/user/form.html?error=invalid_userid"); | |
| return; | |
| } | |
| // TODO: 추가 검증 로직 (password 강도, email 포맷, name/userId 문자 검사 등) | |
| User user = new User( | |
| userId.trim(), | |
| password, | |
| name.trim(), | |
| email.trim() | |
| ); |
| request.getParameter("name"), | ||
| request.getParameter("email") | ||
| ); | ||
| MemoryUserRepository.getInstance().addUser(user); |
There was a problem hiding this comment.
중복 사용자 ID 체크를 추가하세요.
기존에 동일한 userId가 존재하는지 확인하지 않고 추가합니다. Map.put()은 기존 값을 덮어쓰므로 의도치 않게 기존 사용자 정보가 손실될 수 있습니다.
다음과 같이 중복 체크를 추가하세요:
+ if (MemoryUserRepository.getInstance().findUserById(user.getUserId()) != null) {
+ response.sendRedirect("/user/form.html?error=duplicate_userid");
+ return;
+ }
MemoryUserRepository.getInstance().addUser(user);🤖 Prompt for AI Agents
In src/main/java/controller/CreateUserController.java around line 20, the
controller currently calls MemoryUserRepository.getInstance().addUser(user)
without checking for an existing userId which will overwrite existing users;
change the flow to first query the repository (e.g., hasUser / findById /
containsKey) to detect if user.getUserId() already exists, and if it does return
an appropriate error response (e.g., HTTP 409 Conflict or throw a
DuplicateUserException) instead of calling addUser; only call addUser when the
id is confirmed absent to avoid unintentional overwrites.
| public void process(HttpRequest request, HttpResponse response) { | ||
| User user = MemoryUserRepository.getInstance().findUserById(request.getParameter("userId")); | ||
|
|
||
| if (user != null && user.getPassword().equals(request.getParameter("password"))) { |
There was a problem hiding this comment.
타이밍 공격 취약성을 고려하세요.
String.equals()는 문자열 비교 시간이 입력 값에 따라 달라져 타이밍 공격(timing attack)에 취약합니다. 보안이 중요한 비밀번호 비교에서는 MessageDigest.isEqual() 또는 상수 시간 비교 함수를 사용하여 타이밍 공격을 방어해야 합니다.
예시:
// 해시화된 비밀번호를 상수 시간 비교
MessageDigest.isEqual(
storedHashedPassword.getBytes(),
hashPassword(request.getParameter("password")).getBytes()
)🤖 Prompt for AI Agents
In src/main/java/controller/LoginController.java around line 13, the current
password check uses user.getPassword().equals(request.getParameter("password")),
which is vulnerable to timing attacks; replace this with a constant-time
comparison of fixed-length hashes: compute the stored password hash and the hash
of the incoming password using the same algorithm and charset, handle nulls
safely, then compare the two byte arrays with MessageDigest.isEqual (or another
constant-time comparator) and proceed only if that comparison returns true.
평문 비밀번호 비교는 심각한 보안 취약점입니다.
비밀번호를 평문으로 비교하고 있습니다. 프로덕션 환경에서는 반드시 다음과 같은 보안 조치가 필요합니다:
- 비밀번호는 bcrypt, scrypt, Argon2 등의 안전한 해시 알고리즘으로 해시화하여 저장
- 로그인 시 입력된 비밀번호를 해시화하여 저장된 해시와 비교
- 솔트(salt)를 사용하여 레인보우 테이블 공격 방어
학습 목적의 프로토타입이더라도, 프로덕션 수준의 보안 구현 방법을 문서화해두는 것을 권장합니다.
🤖 Prompt for AI Agents
In src/main/java/controller/LoginController.java around line 13, the code
compares plaintext passwords directly which is a critical security issue;
replace plaintext comparison with a secure hash check by using a
password-hashing strategy (e.g., BCrypt): ensure user passwords are stored as
bcrypt hashes (salted by bcrypt) at registration, inject or instantiate a
BCryptPasswordEncoder (or equivalent) and replace
user.getPassword().equals(request.getParameter("password")) with
passwordEncoder.matches(rawPasswordFromRequest, storedHashedPassword), and
update registration/storage flows to hash passwords before persisting; also
handle nulls and failed-match paths appropriately.
Comment on lines
+48
to
+52
| if (contentLength > 0) { | ||
| char[] body = new char[contentLength]; | ||
| br.read(body); | ||
| this.params.putAll(HttpRequestUtils.parseQueryString(new String(body))); | ||
| } |
There was a problem hiding this comment.
요청 본문을 완전히 읽지 못할 수 있습니다
BufferedReader.read는 한 번에 contentLength만큼 보장하지 않아 폼 데이터가 잘리는 경우가 생깁니다. 반복 읽기로 Content-Length 전부를 확보해야 합니다.
- char[] body = new char[contentLength];
- br.read(body);
- this.params.putAll(HttpRequestUtils.parseQueryString(new String(body)));
+ char[] body = new char[contentLength];
+ int offset = 0;
+ while (offset < contentLength) {
+ int read = br.read(body, offset, contentLength - offset);
+ if (read == -1) {
+ break;
+ }
+ offset += read;
+ }
+ this.params.putAll(HttpRequestUtils.parseQueryString(new String(body, 0, offset)));📝 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 (contentLength > 0) { | |
| char[] body = new char[contentLength]; | |
| br.read(body); | |
| this.params.putAll(HttpRequestUtils.parseQueryString(new String(body))); | |
| } | |
| if (contentLength > 0) { | |
| char[] body = new char[contentLength]; | |
| int offset = 0; | |
| while (offset < contentLength) { | |
| int read = br.read(body, offset, contentLength - offset); | |
| if (read == -1) { | |
| break; | |
| } | |
| offset += read; | |
| } | |
| this.params.putAll( | |
| HttpRequestUtils.parseQueryString( | |
| new String(body, 0, offset) | |
| ) | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/webserver/HttpRequest.java around lines 48 to 52, the current
single call to br.read(body) may not read the full Content-Length and can
truncate the request body; replace it with a loop that repeatedly calls
br.read(buffer, offset, remaining) until the total number of chars read equals
contentLength (or read() returns -1), accumulating into the char array (or a
StringBuilder) and then convert the fully-read data to a String before passing
to HttpRequestUtils.parseQueryString; also handle IOException and edge cases
where fewer bytes are available.
Comment on lines
+19
to
+27
| public void forward(String path) { | ||
| try { | ||
| byte[] body = Files.readAllBytes(new File("./webapp" + path).toPath()); | ||
| response200Header(body.length, path); | ||
| responseBody(body); | ||
| } catch (IOException e) { | ||
| response404(); | ||
| } | ||
| } |
There was a problem hiding this comment.
디렉터리 트래버설 취약점 차단 필요
요청 경로에 포함된 .. 등을 그대로 허용하면 RequestHandler에서 전달된 임의의 경로가 ./webapp 밖의 파일까지 노출될 수 있습니다. 반드시 기본 경로를 기준으로 정규화한 뒤 범위를 벗어나면 차단해야 합니다.
-import java.io.File;
+import java.io.File;
import java.io.IOException;
import java.io.OutputStream;
import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
...
- byte[] body = Files.readAllBytes(new File("./webapp" + path).toPath());
+ Path basePath = Paths.get("./webapp").toAbsolutePath().normalize();
+ Path requestedPath = basePath.resolve(path.startsWith("/") ? path.substring(1) : path).normalize();
+ if (!requestedPath.startsWith(basePath)) {
+ response404();
+ return;
+ }
+ byte[] body = Files.readAllBytes(requestedPath);📝 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 forward(String path) { | |
| try { | |
| byte[] body = Files.readAllBytes(new File("./webapp" + path).toPath()); | |
| response200Header(body.length, path); | |
| responseBody(body); | |
| } catch (IOException e) { | |
| response404(); | |
| } | |
| } | |
| public void forward(String path) { | |
| try { | |
| Path basePath = Paths.get("./webapp").toAbsolutePath().normalize(); | |
| Path requestedPath = basePath.resolve(path.startsWith("/") ? path.substring(1) : path).normalize(); | |
| if (!requestedPath.startsWith(basePath)) { | |
| response404(); | |
| return; | |
| } | |
| byte[] body = Files.readAllBytes(requestedPath); | |
| response200Header(body.length, path); | |
| responseBody(body); | |
| } catch (IOException e) { | |
| response404(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/webserver/HttpResponse.java around lines 19 to 27, the forward
method currently reads files using "./webapp" + path which allows
directory-traversal (e.g. "../") and can expose files outside the webapp;
normalize and resolve the requested path against a fixed base directory and
block requests that escape it: obtain the canonical/real path of the base
directory (./webapp), resolve the request path (normalize/remove redundant
segments), compute the resolved real path and verify it starts with the base
real path; if it does not, call response404(), otherwise read the file and
proceed with response200Header/responseBody; handle IO errors as before.
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
신규 기능
변경 사항
유지 보수