Conversation
develop <- feat/authentication/#22
- 동적 뷰 구성에 필요한 model을 반환하는 DynamicViewResponse 개발 - Handler에서 HandlerResponse 반환 후 공통 관심사를 처리하는 postHandling 메서드 추가 - 동적 뷰에 공통으로 들어가는 유저 정보 반영을 자동화
- AuthenticationInfo에 Attribute를 설정하고 조회할 수 있는 기능 추가
- 동적 뷰를 불러와 로딩하는 Loader 개발 - Html escape를 처리하는 escaper 개발 - Handler에서 반환한 model에서 값을 추출하는 ExpressionResolver 클래스 개발 - 템플릿 엔진에 if 문법과 each 문법 개발
- 템플릿 엔진을 사용하여 동적 뷰를 생성하는 DynamicViewRenderer 개발
- 기본 페이지가 동적 뷰를 반환하도록 동적 뷰를 반환하는 HomeHandler 개발
- 정적 페이지인 기본 페이지를 동적 페이지로 수정 - 기본 페이지 위치 이동: /static -> /templates - 페이지 공통 요소 분할(헤더 분할)
- 로거의 잘못된 클래스 삽입 수정 - 안쓰는 의존성 삭제 - 로그 필터의 rid 생성 시점 변경
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
종합 평가
템플릿 엔진과 동적 뷰 기능 구현이 기본 구조는 잘 설계되었습니다. 간단한 구조를 유지하면서도 include, if, each 문법을 잘 구현했고, postHandling() 메서드로 response 타입별 공통 로직을 처리하는 접근도 깔끔합니다.
다만 몇 가지 보안 및 안정성 측면에서 개선이 필요합니다:
주요 개선 사항
- 템플릿 엔진 안정성: 블록 처리 루프에서 무한 루프 위험, 중첩된 else 태그 검색 문제
- 경로 조회 보안:
TemplateLoader에서 경로 이스케이프 검증 필요 (디렉토리 탐색 공격 방지) - Optional 처리:
getAttribute()에서 실제 null 대소를 구분하지 못하는 문제 - 캐시 전략:
clearCache()의 의도가 불명확하여 성능 최적화 재검토 필요
이러한 사항들은 현재 간단한 테스트 범위에서는 드러나지 않을 수 있지만, 복잡한 템플릿이나 동시 요청이 증가할수록 문제가 될 가능성이 있습니다. 우선순위는 경로 보안 검증 > 템플릿 엔진 안정성 > Optional 처리 순입니다.
| int nextEach = indexOfRegex(s, EACH_OPEN); | ||
|
|
||
| if (nextIf < 0 && nextEach < 0) break; | ||
|
|
||
| if (nextIf >= 0 && (nextEach < 0 || nextIf < nextEach)) { | ||
| s = processIfBlockAt(s, nextIf, model); | ||
| } else { | ||
| s = processEachBlockAt(s, nextEach, model); |
There was a problem hiding this comment.
템플릿 블록(if/each) 처리 루프에서 무한 루프 위험이 있습니다. 만약 정규식이 매칭되지만 processIfBlockAt 또는 processEachBlockAt에서 예외가 발생하거나 문자열이 변경되지 않으면, 같은 위치에서 계속 루프할 수 있습니다.
개선 방안: 각 처리 함수가 반드시 블록을 완전히 제거하고 치환된 문자열을 반환하도록 보장하거나, 최대 반복 횟수를 추가하여 무한 루프를 방지하세요."
| private String processEachBlockAt(String s, int start, Map<String, Object> model) { | ||
| Matcher m = EACH_OPEN.matcher(s); | ||
| if (!m.find(start) || m.start() != start) { | ||
| throw new ErrorException("Template parse error: invalid each open tag"); | ||
| } | ||
|
|
||
| String num = m.group(1); | ||
| String listKey = m.group(2).trim(); | ||
|
|
There was a problem hiding this comment.
else 블록을 찾는 로직에서 indexOf()를 사용하면 같은 번호(예: else1)가 여러 번 나타날 때 잘못된 위치를 찾을 수 있습니다. 중첩된 블록이나 복잡한 템플릿에서 버그의 원인이 될 수 있습니다.
개선 방안: 정규식으로 정확한 else 태그를 찾거나, 매칭된 else 태그인지 검증하는 로직을 추가하세요."
|
|
||
| // 핵심: 현재 아이템을 this로 제공 (DTO든 Map이든 상관 없음) | ||
| overlay.put("this", item); | ||
| overlay.put("index", i); | ||
|
|
||
| // (선택) item이 Map이면 기존처럼 바로 {{key}} 접근도 되게 유지할 수 있음 | ||
| if (item instanceof Map<?, ?> mapItem) { | ||
| @SuppressWarnings("unchecked") | ||
| Map<String, Object> itemMap = (Map<String, Object>) mapItem; | ||
| overlay.putAll(itemMap); // 키 충돌 가능성은 있음 | ||
| } | ||
|
|
||
| out.append(renderText(body, overlay)); | ||
| i++; | ||
| } | ||
| } |
There was a problem hiding this comment.
each 루프에서 model에 직접 item을 추가할 때, overlay.putAll(itemMap)이 수행되면 원래 model의 키를 덮어쓸 수 있습니다. 특히 item 맵에 "this" 또는 "index" 같은 키가 있으면 의도하지 않은 동작이 발생할 수 있습니다.
개선 방안: 맵 병합 전에 키 충돌을 검증하거나, item 맵의 키를 필터링하여 예약어 충돌을 방지하세요."
| for (String root : roots) { | ||
| Path p = Paths.get(root).resolve(relative).normalize(); | ||
| if (Files.exists(p) && Files.isRegularFile(p)) { | ||
| return Optional.of(p); | ||
| } | ||
| } | ||
| return Optional.empty(); |
There was a problem hiding this comment.
경로 정규화 후 normalize() 호출로 ../ 시퀀스를 처리하지만, 생성된 경로가 roots의 범위를 벗어날 수 있습니다. 예를 들어, viewPath가 "../../../etc/passwd"일 수 있고, normalize()가 이를 완전히 방지하지 않으면 의도하지 않은 파일 접근이 가능합니다.
개선 방안: 정규화된 경로가 루트 디렉토리 범위 내에 있는지 검증하는 로직을 추가하세요. (p.startsWith() 사용)"
|
|
||
| public interface AuthenticationInfo { | ||
| Optional<Long> getUserId(); | ||
| Optional<Object> getAttribute(String key); |
There was a problem hiding this comment.
getAttribute() 메서드가 Optional<Object>를 반환하는데, 구현체(UnanimousAuthentication, UserAuthentication)에서 항상 값을 감싼 Optional을 반환합니다. 만약 키가 존재하지 않으면 null을 감싼 Optional이 반환되어 실제 부재를 표현하지 못합니다.
개선 방안: Optional.ofNullable()을 사용하여 null 값을 빈 Optional로 처리하거나, Optional.empty()를 명시적으로 반환하도록 수정하세요."
| public HandlerResponse postHandling(HttpRequest request, HttpResponse httpResponse){ | ||
| AuthenticationInfo authenticationInfo = request.getAuthenticationInfo(); | ||
| if (authenticationInfo instanceof UserAuthentication) { | ||
| addModel("userNickname", authenticationInfo.getAttribute("nickname").orElseThrow( | ||
| () -> new ErrorException("DynamicViewResponse:: user nickname loading error") | ||
| )); | ||
| addModel("isLoginUser", true); | ||
|
|
There was a problem hiding this comment.
postHandling() 메서드가 로그인 사용자인 경우에 attribute("nickname")을 꺼내면서 orElseThrow()를 사용하는데, 만약 attribute가 없으면 예외가 발생합니다. 이는 런타임 에러가 될 수 있습니다.
개선 방안: 요청이 UserAuthentication이면 항상 nickname이 있도록 보장하는 것이 좋지만, 방어적으로 .orElse("")나 .ifPresentOrElse() 패턴을 사용하여 안전성을 높이세요."
|
|
||
| // 변수 치환 패턴 | ||
| private static final Pattern RAW_VAR = Pattern.compile("\\{\\{\\{\\s*([^}]+?)\\s*\\}\\}\\}"); | ||
| private static final Pattern VAR = Pattern.compile("\\{\\{\\s*([^}]+?)\\s*\\}\\}"); |
There was a problem hiding this comment.
clearCache()가 매 렌더링마다 호출되면 캐시의 이점이 사라집니다. 동시성 문제나 의도하지 않은 성능 저하를 초래할 수 있습니다. 캐시 무효화 전략을 명확히 하세요. (예: TTL 기반, 명시적 무효화 등)"
| .findFirst().orElseThrow(() -> new ErrorException("DispatcherError: No adapter matched")); | ||
|
|
||
| HandlerResponse handlerResponse = adapter.handle(request, handler); | ||
| HandlerResponse handlerResponse = adapter.handle(request, handler).postHandling(request, response); |
There was a problem hiding this comment.
postHandling()을 모든 응답에 적용하는 것은 좋은 설계이지만, 응답 타입마다 필요한 처리가 다를 수 있습니다. 현재는 DynamicViewResponse만 구현했지만, 다른 응답 타입에서 필요하지 않은 작업이 있을 경우 성능이나 로직 복잡도가 증가할 수 있습니다. 필요시 선택적 적용 인터페이스를 고려하세요."
- Optional 사용 시 of -> ofNullable 로 수정 - 템플릿 엔진의 캐시 삭제 코드 제거
…e PR #52) develop <- feat/web/dynamic-view#23
1 participant
💻 작업 내용
✨ 리뷰 포인트
템플릿 엔진을 최대한 간단하게 개발하려고 노력했습니다.
일부는 LLM의 도움을 받았는데, 템플릿 엔진의 구조와 코드에 취약점은 없는지 체크해주세요.
또, HandlerResponse에 postHandling 메서드를 추가해 response 타입 별 공통 로직을 처리할 수 있도록 했는데, 이 구조가 어떤지 피드백 부탁드립니다.
공통 로직이 커지면 별도의 계층을 분리하던가 해야겠지만, 지금은 저정도 구조로도 충분하지 않을까 생각합니다.(일단 코드의 변경이 별로 없는게 제일 맘에 듭니다.)
🎯 관련 이슈
closed #23