Skip to content

[REFACTOR]: 문서 CRUD 트레이스 로깅 및 requestId 연계 예외 로깅 수정 #153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
masonkimseoul wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

[REFACTOR]: 문서 CRUD 트레이스 로깅 및 requestId 연계 예외 로깅 수정 #153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 45 additions & 3 deletions src/main/java/com/wooteco/wiki/global/exception/WikiExceptionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@

import com.wooteco.wiki.global.common.ApiResponse;
import com.wooteco.wiki.global.common.ApiResponseGenerator;
import jakarta.servlet.http.HttpServletRequest;
import lombok.extern.slf4j.Slf4j;
import org.springframework.http.HttpStatus;
import org.springframework.web.context.request.RequestContextHolder;
import org.springframework.web.context.request.ServletRequestAttributes;
import org.springframework.web.bind.MethodArgumentNotValidException;
import org.springframework.web.bind.annotation.ControllerAdvice;
import org.springframework.web.bind.annotation.ExceptionHandler;
Expand All @@ -14,21 +17,60 @@ public class WikiExceptionHandler {

@ExceptionHandler(WikiException.class)
public ApiResponse<ApiResponse.FailureBody> handle(WikiException exception) {
log.error(exception.getMessage(), exception);
logError(
exception,
exception.getErrorCode().name(),
exception.getMessage()
);
return ApiResponseGenerator.failure(exception.getErrorCode(), exception.getMessage(),
exception.getHttpStatus());
}

@ExceptionHandler(Exception.class)
public ApiResponse<ApiResponse.FailureBody> handle(Exception exception) {
log.error(exception.getMessage(), exception);
logError(
exception,
ErrorCode.UNKNOWN_ERROR.name(),
"An unknown error occurred."
);
return ApiResponseGenerator.failure(ErrorCode.UNKNOWN_ERROR, "An unknown error occurred.",
HttpStatus.INTERNAL_SERVER_ERROR);
}

@ExceptionHandler(MethodArgumentNotValidException.class)
public ApiResponse<ApiResponse.FailureBody> handle(MethodArgumentNotValidException exception) {
log.error(exception.getMessage(), exception);
logError(
exception,
ErrorCode.VALIDATION_ERROR.name(),
exception.getMessage()
);
return ApiResponseGenerator.failure(ErrorCode.VALIDATION_ERROR);
}

private void logError(Exception exception, String errorCode, String message) {
RequestInfo requestInfo = getRequestInfo();
log.error(
"api_exception requestId={} httpMethod={} uri={} errorCode={} message={}",
requestInfo.requestId,
requestInfo.httpMethod,
requestInfo.uri,
errorCode,
message,
exception
);
Comment on lines +52 to +60
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

The logError method logs the uri and message directly without sanitization. While uri (from request.getRequestURI()) is generally safe in most environments, the message parameter can contain unsanitized user input, especially when handling MethodArgumentNotValidException. If the input contains CRLF characters, an attacker can perform log injection, potentially misleading administrators or bypassing log monitoring systems. It is recommended to sanitize these values by replacing or removing CRLF characters before logging.

}

private RequestInfo getRequestInfo() {
try {
HttpServletRequest request = ((ServletRequestAttributes) RequestContextHolder.currentRequestAttributes())
.getRequest();
String requestId = request.getAttribute("requestId") instanceof String id ? id : "N/A";
return new RequestInfo(requestId, request.getMethod(), request.getRequestURI());
Comment on lines +67 to +68
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

예외 로그에 URI를 기록할 때 쿼리 문자열(query string)이 누락되어 있습니다. 디버깅 시 전체 요청 URI 정보가 중요할 수 있으므로, BusinessLogicLogger에서와 같이 쿼리 문자열을 포함하는 것이 좋습니다. 이렇게 하면 로깅 정보의 일관성도 개선됩니다.

            String requestId = request.getAttribute("requestId") instanceof String id ? id : "N/A";
            String uri = request.getRequestURI();
            String queryString = request.getQueryString();
            if (queryString != null && !queryString.isBlank()) {
                uri += "?" + queryString;
            }
            return new RequestInfo(requestId, request.getMethod(), uri);

} catch (IllegalStateException e) {
return new RequestInfo("N/A", "N/A", "N/A");
}
}

private record RequestInfo(String requestId, String httpMethod, String uri) {
}
}
219 changes: 182 additions & 37 deletions src/main/java/com/wooteco/wiki/logging/BusinessLogicLogger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
package com.wooteco.wiki.logging;

import com.fasterxml.jackson.databind.ObjectMapper;
import jakarta.servlet.http.HttpServletRequest;
import java.lang.reflect.Array;
import java.lang.reflect.Method;
import java.util.Collection;
import java.util.Locale;
import java.util.Map;
import java.util.StringJoiner;
import java.util.UUID;
import lombok.extern.slf4j.Slf4j;
import org.aspectj.lang.ProceedingJoinPoint;
import org.aspectj.lang.annotation.Around;
Expand All @@ -10,66 +16,205 @@
import org.springframework.stereotype.Component;
import org.springframework.web.context.request.RequestContextHolder;
import org.springframework.web.context.request.ServletRequestAttributes;
import org.springframework.web.util.ContentCachingRequestWrapper;

@Slf4j
@Aspect
@Component
@Order(1)
public class BusinessLogicLogger {

@Around("execution(* com.wooteco.wiki.controller.*Controller.*(..)) "
+ "|| execution(* com.wooteco.wiki.service.*Service.*(..)) "
+ "|| execution(* com.wooteco.wiki.domain.*.*(..)) "
+ "|| execution(* com.wooteco.wiki.repository.*Repository.*(..)) ")
public Object printLog(ProceedingJoinPoint joinPoint) throws Throwable {
private static final String NOT_AVAILABLE = "N/A";
private static final String DOCUMENT_SERVICE_PACKAGE = "com.wooteco.wiki.document.service.";
private static final String ORGANIZATION_DOCUMENT_SERVICE_PACKAGE = "com.wooteco.wiki.organizationdocument.service.";

@Around("execution(* com.wooteco.wiki..service..*(..))")
public Object traceCrud(ProceedingJoinPoint joinPoint) throws Throwable {
String className = joinPoint.getSignature().getDeclaringTypeName();
String methodName = joinPoint.getSignature().getName();
String type = getBusinessType(className);
if (className.contains("Controller")) {
printRequestBody();

if (!isDocumentService(className)) {
return joinPoint.proceed();
}

CrudAction crudAction = resolveCrudAction(methodName);
if (crudAction == CrudAction.NONE) {
return joinPoint.proceed();
}

RequestInfo requestInfo = getRequestInfo();
String arguments = summarizeArguments(joinPoint.getArgs());
long start = System.currentTimeMillis();

try {
HttpServletRequest request = getRequest();
String requestId = (String) request.getAttribute("requestId");
log.info("{} = {} {} {}.{}()", "RequestId", requestId, type, className, methodName);
} catch (IllegalStateException e) {
log.info("{} = {} {} {}.{}()", "Scheduler", className, type, className, methodName);
Object result = joinPoint.proceed();
long durationMs = System.currentTimeMillis() - start;

log.info(
"crud_trace status=SUCCESS action={} class={} method={} requestId={} httpMethod={} uri={} durationMs={} args={}",
crudAction,
className,
methodName,
requestInfo.requestId(),
requestInfo.httpMethod(),
requestInfo.uri(),
durationMs,
arguments
);
Comment on lines +52 to +62
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

The traceCrud method logs the request URI and method arguments, which may contain unsanitized user input. The uri includes the query string (see lines 185-189), and the arguments string includes CharSequence arguments that are not sanitized (see lines 136-138). If these contain CRLF characters, an attacker can inject fake log entries. Although a sanitize method exists in this class, it is not applied to the uri or to CharSequence arguments. It is recommended to apply sanitization to all user-controlled data before logging.

return result;
} catch (Throwable throwable) {
long durationMs = System.currentTimeMillis() - start;

log.error(
"crud_trace status=FAILED action={} class={} method={} requestId={} httpMethod={} uri={} durationMs={} errorType={} errorMessage={} args={}",
crudAction,
className,
methodName,
requestInfo.requestId(),
requestInfo.httpMethod(),
requestInfo.uri(),
durationMs,
throwable.getClass().getSimpleName(),
sanitize(throwable.getMessage()),
arguments
);
Comment on lines +68 to +79
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

Similar to the success log, the error log in traceCrud also logs the unsanitized uri and arguments. This allows for log injection if the query string or method arguments contain CRLF characters. Ensure all user-controlled data is sanitized before being passed to the logger.

throw throwable;
}
}

private boolean isDocumentService(String className) {
return className.startsWith(DOCUMENT_SERVICE_PACKAGE)
|| className.startsWith(ORGANIZATION_DOCUMENT_SERVICE_PACKAGE);
}

private CrudAction resolveCrudAction(String methodName) {
String normalized = methodName.toLowerCase(Locale.ROOT);

if (startsWithAny(normalized, "post", "create", "add", "link")) {
return CrudAction.CREATE;
}
if (startsWithAny(normalized, "put", "update", "modify", "flush")) {
return CrudAction.UPDATE;
}
if (startsWithAny(normalized, "delete", "remove", "unlink")) {
return CrudAction.DELETE;
}
if (startsWithAny(normalized, "get", "find", "search", "read")) {
return CrudAction.READ;
}

return CrudAction.NONE;
}

private boolean startsWithAny(String value, String... prefixes) {
for (String prefix : prefixes) {
if (value.startsWith(prefix)) {
return true;
}
}
return false;
}
Comment on lines +108 to +115
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

startsWithAny 메서드는 Java Stream API를 사용하면 더 간결하고 표현력 있게 작성할 수 있습니다. 가독성 향상을 위해 스트림을 사용하는 것을 제안합니다. java.util.Arrays를 import해야 합니다.

    private boolean startsWithAny(String value, String... prefixes) {
        return java.util.Arrays.stream(prefixes).anyMatch(value::startsWith);
    }


private String summarizeArguments(Object[] args) {
if (args == null || args.length == 0) {
return "[]";
}

StringJoiner stringJoiner = new StringJoiner(", ", "[", "]");
for (Object arg : args) {
stringJoiner.add(summarizeArgument(arg));
}
return joinPoint.proceed();
return stringJoiner.toString();
}

private String getBusinessType(String className) {
if (className.contains("Controller")) {
return "<<Controller>>";
private String summarizeArgument(Object arg) {
if (arg == null) {
return "null";
}
if (arg instanceof UUID || arg instanceof Number || arg instanceof Boolean) {
return arg.toString();
}
if (arg instanceof CharSequence text) {
return "'" + truncate(text.toString(), 80) + "'";
}
if (arg instanceof Collection<?> collection) {
return arg.getClass().getSimpleName() + "(size=" + collection.size() + ")";
}
if (className.contains("Service")) {
return "<<Service>>";
if (arg instanceof Map<?, ?> map) {
return arg.getClass().getSimpleName() + "(size=" + map.size() + ")";
}
if (className.contains("Repository")) {
return "<<Repository>>";
if (arg.getClass().isArray()) {
return arg.getClass().getSimpleName() + "(length=" + Array.getLength(arg) + ")";
}
if (className.contains("Scheduler")) {
return "<<Scheduler>>";

String identifier = extractIdentifier(arg);
if (identifier.isBlank()) {
return arg.getClass().getSimpleName();
}
return arg.getClass().getSimpleName() + "(" + identifier + ")";
}

private String extractIdentifier(Object target) {
StringJoiner joiner = new StringJoiner(", ");
appendIdentifier(joiner, target, "getUuid", "uuid");
appendIdentifier(joiner, target, "getId", "id");
appendIdentifier(joiner, target, "getTitle", "title");
appendIdentifier(joiner, target, "getWriter", "writer");
return joiner.toString();
}

private void appendIdentifier(StringJoiner joiner, Object target, String getterName, String label) {
try {
Method method = target.getClass().getMethod(getterName);
if (method.getParameterCount() != 0) {
return;
}
Object value = method.invoke(target);
if (value != null) {
joiner.add(label + "=" + sanitize(value.toString()));
}
} catch (Exception ignored) {
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

appendIdentifier 메서드에서 모든 Exception을 잡아 무시하고 있습니다. 이는 예기치 않은 다른 오류까지 숨길 수 있어 좋지 않은 패턴입니다. 리플렉션 관련 예외만 명시적으로 처리하도록 ReflectiveOperationException을 사용하는 것이 더 안전합니다.

        } catch (ReflectiveOperationException ignored) {

}
return "";
}

private void printRequestBody() {
HttpServletRequest request = getRequest();
final ContentCachingRequestWrapper cachingRequest = (ContentCachingRequestWrapper) request;
String requestId = (String) cachingRequest.getAttribute("requestId");
String logType = "<<Request>>";
ObjectMapper objectMapper = new ObjectMapper();
private RequestInfo getRequestInfo() {
try {
log.info("{} = {} {} {} = \n{}", "RequestId", requestId, logType, "Body",
objectMapper.readTree(cachingRequest.getContentAsByteArray()).toPrettyString());
} catch (Exception e) {
log.info("{} = {} {} {} = \n{}", "RequestId", requestId, logType, "Body", " ");
HttpServletRequest request = getRequest();
String requestId = request.getAttribute("requestId") instanceof String id
? id
: NOT_AVAILABLE;
String queryString = request.getQueryString();
String uri = request.getRequestURI();
if (queryString != null && !queryString.isBlank()) {
uri = uri + "?" + queryString;
}
return new RequestInfo(requestId, request.getMethod(), uri);
} catch (IllegalStateException exception) {
return new RequestInfo(NOT_AVAILABLE, NOT_AVAILABLE, NOT_AVAILABLE);
}
}

private static HttpServletRequest getRequest() {
private HttpServletRequest getRequest() {
return ((ServletRequestAttributes) RequestContextHolder.currentRequestAttributes()).getRequest();
}

private String sanitize(String value) {
if (value == null || value.isBlank()) {
return NOT_AVAILABLE;
}
return truncate(value.replace('\n', ' ').replace('\r', ' '), 120);
}

private String truncate(String value, int maxLength) {
if (value.length() <= maxLength) {
return value;
}
return value.substring(0, maxLength) + "...";
}

private enum CrudAction {
Copy link
Copy Markdown
Collaborator

@Starlight258 Starlight258 Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

CREATE, READ, UPDATE, DELETE, NONE
}

private record RequestInfo(String requestId, String httpMethod, String uri) {
}
}
Loading