Conversation
develop <- feat/app/login-post/#37
- 응답 쿠키 인터페이스 정의 - 응답 쿠키 구현체 구현 - 핸들러 응답과 렌더러에 응답 쿠키 관련 기능 추가
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
코드 리뷰 완료. 주요 이슈 7가지를 인라인 코멘트로 첨부했습니다.
핵심 요약:
- NPE 위험:
HttpRequest.cookies가 null로 초기화되어getCookieValue()호출 시 crash 위험 - 타입 불일치:
HandlerResponse.setCookie()이CookieBuilder만 수용하여SimpleCookieBuilder미지원 - 보안 취약점 (XSS/Injection): 쿠키 값이 검증 없이 HTTP 헤더에 추가되어 Response Splitting 공격 가능
- SameSite=None 미검증: Secure 플래그 필수 조건을 강제하지 않음
이 이슈들을 해결하면 더 안정적이고 안전한 구현이 될 것입니다.
| parts[1].strip(), | ||
| parts[2].strip()); | ||
| } catch (IllegalArgumentException e) { | ||
| throw new ServiceException(ErrorCode.METHOD_NOT_ALLOWED); | ||
| } catch (NullPointerException e){ | ||
| throw new ErrorException("Http method error"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
NPE 위험: this.cookies가 초기화되지 않은 상태에서 getCookieValue() 메서드가 호출되면 NullPointerException이 발생합니다.
HttpRequest생성자에서cookies를 초기화하지 않습니다 (기본값이 null).setHeader("Cookie", value)가 호출되지 않으면cookies는 null로 유지됩니다.getCookieValue(String key)호출 시this.cookies.get(key)에서 NPE 발생.
해결책: 생성자에서 this.cookies = Cookies.empty();로 초기화하세요.
| } catch (NullPointerException e){ | ||
| throw new ErrorException("Http method error"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
NPE 위험: getCookies() 메서드도 null을 반환할 수 있습니다.
getCookies()에서this.cookies가 null일 수 있습니다.StaticViewRenderer라인 33에서handlerResponse.getCookies().forEach(...)가 호출되는데,getCookies()가 null이면 NPE 발생.
해결책: Cookies.empty()를 기본값으로 초기화하세요.
| return status; | ||
| } | ||
|
|
||
| public void setCookie(CookieBuilder cookie){ | ||
| this.cookies.add(cookie); |
There was a problem hiding this comment.
타입 불일치 위험: setCookie() 메서드의 매개변수 타입이 CookieBuilder로 고정되어 있습니다.
- 인터페이스
ResponseCookie가 정의되어 있지만,setCookie()는CookieBuilder타입만 수용합니다. SimpleCookieBuilder도ResponseCookie를 구현하지만,setCookie()로 전달할 수 없습니다.
해결책: 메서드를 public void setCookie(ResponseCookie cookie)로 변경하세요. 이렇게 하면 모든 구현체를 수용할 수 있습니다.
| } | ||
|
|
||
| @Override | ||
| public String toHeaderValue() { | ||
| StringBuilder sb = new StringBuilder(); | ||
| sb.append(name).append("=").append(value); | ||
|
|
||
| if (domain != null && !domain.isBlank()) sb.append("; Domain=").append(domain); | ||
| if (path != null && !path.isBlank()) sb.append("; Path=").append(path); |
There was a problem hiding this comment.
XSS/Injection 위험: 쿠키 값이 검증되지 않은 채로 직접 HTTP 헤더에 추가됩니다.
toHeaderValue()에서name,value,domain,path등을 그대로StringBuilder에 추가합니다.- 악의적인 값(예: 개행, 세미콜론)이 포함되면 HTTP 응답 헤더 주입 공격(Response Splitting)이 가능합니다.
- 예:
path = "/\r\nSet-Cookie: admin=true"와 같은 입력으로 추가 쿠키 주입 가능.
해결책: 쿠키 값을 검증하거나 인코딩하세요. RFC 6265 표준에 따라 유효한 문자인지 확인하고, 필요시 URL 인코딩을 적용하세요.
| if (rawHeader == null || rawHeader.isBlank()) return empty(); | ||
|
|
||
| Map<String, String> map = new LinkedHashMap<>(); | ||
| String[] parts = rawHeader.split(";"); | ||
| for (String part : parts) { | ||
| String token = part.trim(); | ||
| if (token.isEmpty()) continue; | ||
|
|
||
| String[] kv = token.split("=", 2); | ||
| String name = kv[0].trim(); | ||
| String value = kv.length == 2 ? kv[1].trim() : ""; | ||
|
|
||
| if (!name.isEmpty()) { | ||
| map.put(name, value); // 동일 name은 마지막 값으로 덮어씀(브라우저/서버 실무 관행) | ||
| } |
There was a problem hiding this comment.
XSS 위험: 요청 쿠키 값이 검증되지 않습니다.
parse()메서드가 쿠키 헤더 값을 그대로 분석하여 저장합니다.- RFC 6265에서 쿠키 값은 특정 문자만 허용되지만, 여기서는 검증이 없습니다.
- 악의적인 값이
values맵에 저장되어 나중에 사용될 때 문제가 될 수 있습니다.
해결책: 쿠키 값의 유효성을 검증하거나, 사용 시점에 필요한 인코딩/디코딩을 적용하세요.
| httpResponse.setBody(file, body); | ||
| handlerResponse.getCookies().forEach(cookie->httpResponse.addHeader("Set-Cookie", cookie)); | ||
| return httpResponse; |
There was a problem hiding this comment.
NPE 위험: getCookies()가 null을 반환할 수 있습니다.
HandlerResponse.getCookies()는 초기화된 List를 반환해야 하지만, 부모 클래스에서cookies가 제대로 초기화되지 않으면 문제가 발생할 수 있습니다.- 현재 코드는
HandlerResponse생성자에서cookies = new ArrayList<>()로 초기화하므로 안전하지만, 다른 곳에서 null로 덮어쓸 가능성이 있습니다.
해결책: defensive programming으로 handlerResponse.getCookies() 결과를 체크하거나, getCookies()가 항상 empty list를 반환하도록 보장하세요.
| return CookieBuilder.of(name, "") | ||
| .path("/") // 보통 명시 | ||
| .maxAge(0); | ||
| } | ||
|
|
There was a problem hiding this comment.
논리 오류: SameSite=None일 때 Secure 플래그가 필수인데, 이를 강제하지 않습니다.
- HTTP 명세상 SameSite=None은 반드시 Secure 쿠키와 함께 사용되어야 합니다.
- 현재 코드는
sameSite(SameSite.NONE).toHeaderValue()를 호출해도secure가 false이면 검증이 없습니다.
해결책: toHeaderValue() 메서드 시작 부분에서 sameSite == SameSite.NONE && !secure일 때 예외를 던지거나, 자동으로 secure = true로 설정하세요.
- NPE 위험 제거 - 추상화 에러 제거
1 participant
💻 작업 내용
🎯 관련 이슈
closed #41