Repository navigation
[FLYW-140] CSRF 설정 분리 (JSP Web 활성화 / API Origin 검증 적용) - #261
Hidden character warning
Conversation
- refresh token 검증 실패/만료/재사용/레이스(이미 회전됨) 등 예외 케이스에서 단순 에러 반환 대신 forceLogout()로 보안 컨텍스트/세션/쿠키를 일괄 정리하도록 개선 - refreshToken 쿠키 path를 '/'로 통일하고, 과거 '/auth' 경로로 남아있는 레거시 쿠키도 함께 삭제 (deleteRefreshCookies로 두 경로 동시 정리) - WEB(SecurityConfigWeb) 체인에 RefreshTokenSessionSyncFilter를 추가하여, 로그인 상태인데 refresh token 쿠키/DB 상태가 불일치할 경우 즉시 강제 로그아웃 후 /login 리다이렉트 - 필터 적용 범위에서 정적 리소스/퍼블릭 엔드포인트(/error 포함)는 제외하여 불필요한 로그아웃 방지 Motivation: - 하이브리드(Session + JWT Cookie) 구조에서 refresh token 유실/폐기 상태로 인증 컨텍스트만 남는 경우 처리
- API 요청에서 JWT로 인증된 요청인지 명확히 구분하기 위해 `JWT_AUTHENTICATED_ATTR` 마커를 도입 - `authenticate()` 시점에 - `req.setAttribute(JWT_AUTHENTICATED_ATTR, true)`로 **요청 단위** 마킹 - `auth.setDetails(JWT_AUTHENTICATED_ATTR)`로 **SecurityContext 인증 객체**에도 마킹 - 기존 `isAlreadyAuthenticated()`(Anonymous 제외) 방식 대신, - “이미 JWT로 인증된 요청”만 스킵하도록 `isJwtAuthenticatedRequest()`로 로직 변경
- /login 요청에서 returnUrl 파라미터를 Model에 전달하도록 수정 - returnUrl sanitize 처리로 외부 URL(Open Redirect) 위험 차단 - login.jsp에 returnUrl hidden input 추가하여 로그인 요청 시 전달
- 로그인 성공 시 returnUrl 파라미터가 존재하면 우선적으로 redirect 처리 - returnUrl sanitize 검증 추가하여 악성 redirect 방지 - 기존 request attribute 기반 redirect 로직과 병행 처리
- WebLoginRedirectEntryPoint 추가하여 인증 실패 시 /login?returnUrl=... redirect 처리 - SecurityConfigWeb exceptionHandling에 entrypoint 등록 - /login 요청에 대한 무한 redirect 방지 로직 포함
- JwtWebAuthFilter에서 인증 실패 시 entrypoint 대신 login redirect 처리 - RefreshTokenSessionSyncFilter 강제 로그아웃 시 returnUrl 포함 redirect 적용 - GET 요청에 한해 returnUrl 생성, 외부 redirect 방지 검증 로직 포함 - /login 경로는 JwtWebAuthFilter에서 제외하여 무한 루프 방지
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCSRF 쿠키 기반 보호와 Origin/Referer 검증 필터(API·Web), 리프레시 토큰 기반 세션 동기화 필터가 도입되었고, 로그인 리다이렉트(returnUrl) 검증 및 강제 로그아웃(forceLogout) 흐름이 추가되었습니다. 프론트엔드에 csrfFetch 유틸이 추가되고 JSP 폼에 CSRF 입력이 삽입되었습니다. Changes
Sequence DiagramssequenceDiagram
participant Client
participant OriginFilter as OriginReferer\n(API)
participant CsrfFilter as CSRF\n(CookieRepo)
participant Controller as AuthController
Client->>OriginFilter: POST /api/... (Origin/Referer)
activate OriginFilter
OriginFilter->>OriginFilter: isStateChanging? / isExcluded?
alt Included & Not excluded
OriginFilter->>OriginFilter: validate Origin/Referer
alt Allowed
OriginFilter->>CsrfFilter: forward request
activate CsrfFilter
CsrfFilter->>CsrfFilter: validate CsrfToken (cookie/header)
alt Valid
CsrfFilter->>Controller: forward
Controller-->>Client: 2xx / delegate
else Invalid
CsrfFilter-->>Client: 403 Forbidden
end
deactivate CsrfFilter
else Blocked
OriginFilter-->>Client: 403 Forbidden
end
else Excluded
OriginFilter->>CsrfFilter: forward
end
deactivate OriginFilter
sequenceDiagram
participant Browser
participant JwtFilter as JWT\nAuthFilter
participant RefreshSync as RefreshToken\nSessionSyncFilter
participant RefreshRepo as RefreshToken\nRepository
participant AuthService as AuthTokenService
Browser->>JwtFilter: GET /protected (with JWT)
activate JwtFilter
JwtFilter->>JwtFilter: isJwtAuthenticatedRequest?
JwtFilter->>RefreshSync: forward
deactivate JwtFilter
activate RefreshSync
RefreshSync->>RefreshSync: isAuthenticated?
alt Authenticated
RefreshSync->>RefreshSync: readCookie(refreshToken)
RefreshSync->>RefreshRepo: lookup(hashedToken)
alt Token valid
RefreshSync-->>Browser: proceed (200)
else Token invalid/missing
RefreshSync->>AuthService: forceLogout(request,response)
activate AuthService
AuthService->>AuthService: delete cookies, invalidate session, clear SecurityContext
AuthService-->>Browser: redirect /login?returnUrl=...
deactivate AuthService
end
else Not authenticated
RefreshSync-->>Browser: proceed (skip)
end
deactivate RefreshSync
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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 |
gaeunnlee
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/flyway/auth/service/AuthTokenServiceImpl.java (1)
78-136:⚠️ Potential issue | 🔴 Critical
refresh()트랜잭션 내에서forceLogout()호출 시 DB 변경이 롤백됩니다.
refresh()는@Transactional로 선언되어 있어,forceLogout()→logout()→revokeById()/revokeAllByUserId()등의 DB 변경이 수행된 후BusinessException이 throw되면 트랜잭션 전체가 롤백됩니다.특히 재사용 탐지 경로(Line 102-106)에서
revokeAllByUserId()는 보안상 중요한 조치인데, 롤백으로 인해 실제 DB에 반영되지 않습니다. 결과적으로 클라이언트 측 쿠키/세션은 정리되지만 서버 측 토큰은 유효한 상태로 남게 됩니다.해결 방안:
forceLogout내 DB 작업을REQUIRES_NEW전파 수준의 별도 트랜잭션으로 분리- 또는
refresh()내에서 throw 전에forceLogout을 호출하지 않고, 호출자(컨트롤러)에서 예외 catch 후forceLogout실행
🤖 Fix all issues with AI agents
In `@src/main/java/com/flyway/auth/service/AuthTokenServiceImpl.java`:
- Around line 15-19: AuthTokenServiceImpl 내 import 목록에 SecurityContextHolder가 중복
선언돼 있으니 중복된 import 한 줄을 삭제해 단일 선언만 남기세요 (참조 식별자: SecurityContextHolder, 클래스:
AuthTokenServiceImpl).
In `@src/main/java/com/flyway/security/filter/OriginRefererCheckFilter.java`:
- Around line 180-186: normalizeOrigin currently trims and strips a trailing
slash but doesn't normalize case, so origins like "HTTPS://FLYWAY.KR" won't
match the allowlist; update the normalizeOrigin method to trim, remove any
trailing slash, and convert the result to a consistent case (use
toLowerCase(Locale.ROOT)) before returning so scheme/host comparisons are
case-insensitive (keep the method name normalizeOrigin and update its return
value accordingly).
- Around line 26-29: The DEFAULT_ALLOWED_ORIGINS constant in
OriginRefererCheckFilter currently hardcodes "http://localhost:8080", which is
unsafe; change the filter to load allowed origins from external configuration
(e.g., application properties or an environment variable) instead of relying on
the hardcoded DEFAULT_ALLOWED_ORIGINS: refactor OriginRefererCheckFilter to read
a configured List<String> (inject via constructor or
`@Value/`@ConfigurationProperties) and use that for origin checks, keep a minimal
safe default if none provided but do not include localhost in production
defaults, and update any references to DEFAULT_ALLOWED_ORIGINS to use the
injected/loaded value.
In `@src/main/java/com/flyway/security/jwt/JwtWebAuthFilter.java`:
- Around line 52-56: The current check in JwtWebAuthFilter using
path.startsWith("/login") is too broad and skips routes like /loginProc; update
the condition in the code around resolvePath(request) so it only bypasses the
exact login page or subpaths under /login (e.g. change path.startsWith("/login")
to path.equals("/login") || path.startsWith("/login/")); adjust the branch where
filterChain.doFilter(request, response) is invoked so only true /login or
/login/* paths are skipped and other paths such as /loginProc still go through
JWT validation.
In `@src/main/webapp/resources/common/js/csrfFetch.js`:
- Around line 87-95: Race condition: non-module scripts may call
window.csrfFetch before the ES module assigns it; to fix, immediately set
window.csrfFetch at module initialization to a defensive wrapper that queues
calls (or returns a rejected Promise with a clear error) and then replace it
with the real exported csrfFetch once ready; specifically, add a top-level early
assignment for window.csrfFetch in the csrfFetch module that references
csrfFetch (the exported function), and inside the real csrfFetch continue using
isStateChanging, ensureCsrfCookie, toFetchUrl, and withCsrfHeader so queued
calls are executed with the correct behavior once the module finishes
initialization.
🧹 Nitpick comments (16)
src/main/java/com/flyway/security/jwt/JwtWebAuthFilter.java (2)
125-139:buildReturnUrl검증 로직이 4곳에 중복됨
buildReturnUrl(이 파일),WebLoginRedirectEntryPoint.buildReturnUrl,LoginSuccessHandler.sanitizeReturnUrl,AuthViewController.sanitizeReturnUrl— 거의 동일한 open-redirect 방지 로직이 4곳에 복사되어 있습니다. 검증 규칙이 변경되면 모든 곳을 동시에 수정해야 하므로 유지보수 리스크가 있습니다.공통 유틸리티 클래스(예:
ReturnUrlSanitizer)로 추출하면 일관성과 유지보수성이 향상됩니다.
34-35:entryPoint필드는 사용되지 않으므로 제거하세요
redirectToLogin()메서드로 전환되면서entryPoint.commence()호출이 모두 제거되었습니다. 해당 필드는 더 이상 코드 내에서 참조되지 않으므로 불필요한 의존성입니다. 함께 불필요한 import도 제거할 수 있습니다.src/main/java/com/flyway/security/handler/WebLoginRedirectEntryPoint.java (1)
27-30: 무한 루프 방지 로직 —/login요청 시에도 redirect 발생
/login은SecurityConfigWeb에서 public endpoint로 설정되어 있으므로 일반적으로 이 EntryPoint가 호출되지 않을 것입니다. 하지만 방어적 코드로 유지하는 것은 합리적입니다.다만, 현재 로직은
/login경로일 때 다시/login으로 redirect하는데, 이미/login에 있는 사용자가 인증 실패로 여기에 도달한 경우 불필요한 redirect가 발생합니다.response.sendRedirect대신response.sendError(HttpServletResponse.SC_UNAUTHORIZED)또는 단순히filterChain을 통과시키는 것도 고려해 볼 수 있습니다.src/main/webapp/resources/common/js/csrfFetch.js (1)
53-66:ensureCsrfCookie에서 bootstrap fetch 실패를 무시함Line 63의 빈
catch (e) {}블록이 네트워크 에러나 서버 에러를 완전히 삼킵니다. CSRF 토큰 부트스트랩이 실패한 이유를 디버깅하기 어렵습니다.제안: 최소한의 경고 로그 추가
- } catch (e) {} + } catch (e) { + console.warn("[csrfFetch] CSRF bootstrap failed", e); + }src/main/webapp/resources/search/js/filtering.js (1)
22-30:loadAirlines에 에러 처리가 없습니다.
csrfFetch전환 자체는 적절하지만, 네트워크 오류나 비정상 응답 시res.json()이 예외를 발생시킬 수 있고, 이 함수는DOMContentLoaded의await에서 호출되므로 unhandled rejection이 될 수 있습니다. 기존 코드의 문제이지만, 이번 변경과 함께 방어 로직을 추가하는 것을 고려해 보세요.♻️ 에러 처리 추가 제안
async function loadAirlines() { - const res = await csrfFetch(`${CONTEXT_PATH}/api/public/airlines`); - const data = await res.json(); - - AIRLINES = data.map(a => ({ - code: a.airlineId, - name: a.airlineName - })); + try { + const res = await csrfFetch(`${CONTEXT_PATH}/api/public/airlines`); + if (!res.ok) return; + const data = await res.json(); + AIRLINES = data.map(a => ({ + code: a.airlineId, + name: a.airlineName + })); + } catch (e) { + console.error("항공사 목록 로딩 실패:", e); + } }src/main/webapp/resources/mypage/js/render/bookings.js (1)
27-34:getAirlineMap이detail.js와 중복됩니다.
bookings.js의getAirlineMap(Lines 27-34)과detail.js의getAirlineMap(Lines 26-33)이toAssetUrl헬퍼와 함께 거의 동일한 구현입니다. 공통 모듈로 추출하면 유지보수가 용이해집니다.src/main/webapp/resources/auth/signup.js (1)
50-58: 응답 Body 이중 소비 가능성이 있습니다.Line 53의
res.json()이 실패하면 Body 스트림이 이미 소비된 상태이므로, Line 56의res.text()도 실패할 수 있습니다. 기존 코드의 문제이지만, 이번 변경 시 함께 수정하는 것을 고려해 보세요.♻️ 응답 텍스트를 먼저 읽고 JSON 파싱하는 방식 제안
- let data = null; - try { - data = await res.json(); - attemptIdHidden.value = data.data.attemptId; - } catch (e) { - const text = await res.text().catch(() => ""); - data = text ? { message: text } : {}; - } + let data = null; + const text = await res.text(); + try { + data = JSON.parse(text); + attemptIdHidden.value = data.data.attemptId; + } catch (e) { + data = text ? { message: text } : {}; + }src/main/webapp/WEB-INF/views/common/head.jsp (1)
29-32: 모듈 스크립트와 일반 스크립트 간 실행 순서 주의
type="module"스크립트는 항상 deferred로 실행됩니다.window.csrfFetch는 문서 파싱 완료 후에 할당되므로, 일반(non-module) 스크립트에서 파싱 시점에csrfFetch를 호출하면undefined에러가 발생합니다.현재 사용처(이벤트 핸들러,
DOMContentLoaded콜백 등)에서는 문제가 없지만, 향후 일반 스크립트에서 즉시 호출하는 경우 문제가 될 수 있습니다. 이 점을 인지하고 계시면 됩니다.src/main/webapp/WEB-INF/views/payment/refund-test.jsp (1)
40-43:head.jsp를 포함하지 않아 csrfFetch 임포트가 중복됩니다.이 페이지는
head.jsp를 include하지 않고 자체<head>를 사용하고 있어, csrfFetch 모듈 임포트 코드가 중복됩니다. 가능하다면 공통head.jsp를 include하여 중복을 줄이는 것을 고려해 보세요.src/main/webapp/resources/common/js/authFetch.js (1)
10-31:csrfFetch.js와 URL 유틸리티 함수 중복.
getBasePath,isAbsoluteHttpUrl,joinBasePath,normalizeInputToUrl,toFetchUrl등의 함수가csrfFetch.js에도 동일하게 존재합니다. 공통 모듈로 추출하면 유지보수성이 향상됩니다.src/main/java/com/flyway/security/jwt/JwtApiAuthFilter.java (1)
93-98:auth.setDetails()에 문자열 상수를 설정하고 있습니다.Line 96에서
auth.setDetails(JWT_AUTHENTICATED_ATTR)는"JWT_AUTHENTICATED"문자열을 설정합니다. 그런데 Line 123에서Boolean.TRUE.equals(details)검사는 도달 불가능한 코드입니다. 일관성을 위해Boolean.TRUE를 사용하거나, 불필요한 검사를 제거하는 것이 좋습니다.♻️ 일관성 개선 제안
- auth.setDetails(JWT_AUTHENTICATED_ATTR); + auth.setDetails(Boolean.TRUE);또는 Line 123의
Boolean.TRUE.equals(details)분기를 제거:- return JWT_AUTHENTICATED_ATTR.equals(details) || Boolean.TRUE.equals(details); + return JWT_AUTHENTICATED_ATTR.equals(details);src/test/java/com/flyway/security/filter/CsrfProtectionTest.java (1)
67-76:ArgumentMatchers에 static import 사용 권장
org.mockito.ArgumentMatchers.any()가 줄마다 반복됩니다. 이미mock,doNothing,verify에는 static import를 사용하고 있으므로 일관성을 위해ArgumentMatchers.any()도 static import로 정리하면 가독성이 개선됩니다.♻️ 리팩토링 제안
import 추가:
+import static org.mockito.ArgumentMatchers.any;- doNothing().when(authTokenService).refresh(org.mockito.ArgumentMatchers.any(), org.mockito.ArgumentMatchers.any()); + doNothing().when(authTokenService).refresh(any(), any());- verify(authTokenService).refresh(org.mockito.ArgumentMatchers.any(), org.mockito.ArgumentMatchers.any()); + verify(authTokenService).refresh(any(), any());src/main/java/com/flyway/security/config/SecurityConfigWeb.java (1)
93-104:excludePatterns목록 구성 — 간소화 가능기능적으로 문제는 없으나,
ArrayList+addAll대신 보다 간결한 방식으로 작성할 수 있습니다.♻️ 간소화 제안
`@Bean` public RefreshTokenSessionSyncFilter refreshTokenSessionSyncFilter() { - List<String> excludes = new ArrayList<>(); - excludes.addAll(Arrays.asList(STATIC_RESOURCES)); - excludes.addAll(Arrays.asList(PUBLIC_ENDPOINTS)); + List<String> excludes = new ArrayList<>(STATIC_RESOURCES.length + PUBLIC_ENDPOINTS.length); + Collections.addAll(excludes, STATIC_RESOURCES); + Collections.addAll(excludes, PUBLIC_ENDPOINTS); return new RefreshTokenSessionSyncFilter( refreshTokenRepository, tokenHasher, authTokenService, excludes ); }src/main/java/com/flyway/security/filter/RefreshTokenSessionSyncFilter.java (3)
58-69: 인증된 요청마다 DB 조회 발생 — 성능 고려 필요
refreshTokenRepository.findByTokenHash(hash)가 인증된 모든 비공개 경로 요청마다 실행됩니다. 정적 리소스와 공개 엔드포인트는 제외되지만, 트래픽이 많은 인증 페이지에서는 DB 부하가 될 수 있습니다.세션 속성에 마지막 검증 시각을 저장하고 짧은 TTL(예: 30초~1분) 동안 재검증을 건너뛰는 방식으로 DB 호출을 줄일 수 있습니다.
111-118: 커스텀 경로 매칭 대신 Spring의AntPathMatcher사용 권장현재
matches메서드는/**suffix와 정확한 문자열 비교만 지원합니다.excludePatterns에 현재 사용 중인 패턴은 커버되지만, 향후/*와 같은 단일 레벨 와일드카드가 추가될 경우 동작하지 않습니다.Spring의
AntPathMatcher를 사용하면 모든 Ant 패턴을 지원하며 Spring Security의 경로 매칭과 일관성을 유지할 수 있습니다.♻️ AntPathMatcher 적용 제안
+import org.springframework.util.AntPathMatcher; + `@Slf4j` `@RequiredArgsConstructor` public class RefreshTokenSessionSyncFilter extends OncePerRequestFilter { private static final String REFRESH_COOKIE = "refreshToken"; + private static final AntPathMatcher pathMatcher = new AntPathMatcher(); // ... - private boolean matches(String path, String pattern) { - if (pattern == null || pattern.isEmpty()) return false; - if (pattern.endsWith("/**")) { - String prefix = pattern.substring(0, pattern.length() - 3); - return path.startsWith(prefix); - } - return path.equals(pattern); - } + private boolean matches(String path, String pattern) { + if (pattern == null || pattern.isEmpty()) return false; + return pathMatcher.match(pattern, path); + }
120-127:readCookie헬퍼가AuthTokenServiceImpl과 중복됩니다.
AuthTokenServiceImpl.readCookie와 동일한 로직이 여기에도 존재합니다. 공통 유틸리티로 추출하면 중복을 제거할 수 있습니다.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/main/java/com/flyway/auth/service/AuthTokenServiceImpl.java`:
- Around line 161-181: forceLogout(...) calls this.logout(...) without
`@Transactional` so Spring AOP self-invocation bypasses the transactional proxy
and logout()'s DB call (e.g., refreshTokenRepository.revokeById() invoked from
logout()) may run outside a transaction; fix by annotating forceLogout(...) with
`@Transactional` (importing
org.springframework.transaction.annotation.Transactional if needed) so that
external calls (e.g., from RefreshTokenSessionSyncFilter) get a proper
transactional boundary and logout()'s DB operations are executed within a
transaction.
In `@src/main/java/com/flyway/security/filter/OriginRefererCheckFilter.java`:
- Around line 121-129: isAllowedReferer currently compares the raw referer (only
trimmed) against normalized allowedOrigins, causing case/misc mismatches; update
isAllowedReferer to normalize the incoming referer using the same
normalizeOrigin method (and handle null/empty after trim) before comparing
against allowedOrigins (same checks as in isAllowedOrigin: equality or
startsWith(allowedOrigin + "/")) so case and formatting are consistent between
isAllowedOrigin and isAllowedReferer.
In `@src/main/java/com/flyway/security/jwt/JwtWebAuthFilter.java`:
- Line 32: JWT_AUTHENTICATED_ATTR is declared public but never referenced
externally; change its declaration inside JwtWebAuthFilter from public static
final to private static final to encapsulate it (update the field visibility
only, keep name and value intact).
🧹 Nitpick comments (5)
src/main/java/com/flyway/security/filter/OriginRefererCheckFilter.java (2)
104-110: 로그에 사용자 제어 가능한 헤더 값이 직접 기록되어 로그 인젝션 위험이 있습니다.
Origin과Referer는 클라이언트가 조작할 수 있는 헤더입니다. 개행 문자(\r,\n) 등이 포함되면 로그 위조(log injection/forging)가 가능합니다. 로그에 기록하기 전에 개행 문자를 제거하거나 치환하는 것이 좋습니다.🛡️ 제안: 로그 출력 전 헤더 값 새니타이즈
+ private String sanitizeForLog(String value) { + if (value == null) return "null"; + return value.replaceAll("[\\r\\n]", "_"); + } + if (!allowed) { log.warn("[OriginRefererCheck] blocked. method={}, requestURI={}, origin={}, referer={}", - request.getMethod(), request.getRequestURI(), origin, referer); + request.getMethod(), request.getRequestURI(), + sanitizeForLog(origin), sanitizeForLog(referer));
56-72: 팩토리 메서드forApi()/forWeb()이 정적으로 생성되어 Spring 빈으로 관리되지 않습니다.
SecurityConfigWeb에서OriginRefererCheckFilter.forWeb()을 호출하여 필터를 생성하고 있어, 이 인스턴스는 Spring 컨텍스트 밖에서 생성됩니다. 현재 코드에서는 외부 의존성이 없어 문제가 되지 않지만, 향후 허용 Origin 목록을 외부 설정에서 주입(@Value등)하려면 이 구조를 변경해야 합니다. 설정 외부화 시 팩토리 메서드 대신@Bean등록 방식으로 전환하는 것을 고려해 주세요.src/main/webapp/resources/common/js/csrfFetch.js (2)
78-91:ensureCsrfCookie: bootstrap 실패 시 에러가 완전히 무시됩니다.Line 88의 빈
catch (e) {}가 네트워크 오류를 삼키고 있습니다.csrfFetchLine 117에서 토큰 부재 시 throw하므로 기능적으로는 안전하지만, 디버깅 시 bootstrap 실패 원인을 파악하기 어렵습니다. 최소한console.warn정도는 남기는 것을 권장합니다.♻️ 제안: 경고 로그 추가
- } catch (e) {} + } catch (e) { + console.warn("[csrfFetch] CSRF bootstrap failed:", e); + }
32-57: URL 유틸리티 함수들이authFetch.js와 완전히 중복됩니다.
getBasePath,isAbsoluteHttpUrl,joinBasePath,normalizeInputToUrl,toFetchUrl총 5개 함수가authFetch.js에도 동일하게 존재합니다. 한 쪽을 수정할 때 다른 쪽도 동기화해야 하는 유지보수 부담이 생깁니다.csrfFetch.js에서 export하고authFetch.js에서 import하는 방식으로 통합하는 것을 권장합니다.src/main/webapp/resources/common/js/authFetch.js (1)
10-31:csrfFetch.js와 동일한 URL 유틸리티 5개가 중복 정의되어 있습니다.
getBasePath,isAbsoluteHttpUrl,joinBasePath,normalizeInputToUrl,toFetchUrl이csrfFetch.js와 완전히 동일합니다.csrfFetch.js에서 이 함수들을 named export하고 여기서 import하면 코드 중복을 제거할 수 있습니다.♻️ 제안: csrfFetch.js에서 유틸리티 import
csrfFetch.js에서 유틸리티를 export합니다:export { toFetchUrl, getBasePath, joinBasePath, normalizeInputToUrl, isAbsoluteHttpUrl };
authFetch.js에서 import하여 로컬 정의를 제거합니다:-import { csrfFetch } from "./csrfFetch.js"; +import { csrfFetch, toFetchUrl } from "./csrfFetch.js"; const CONFIG = { REFRESH_URL: "/auth/refresh", LOGIN_URL: "/login", }; let refreshPromise = null; -function getBasePath() { - return window.APP?.contextPath ?? ""; -} -function isAbsoluteHttpUrl(url) { - return typeof url === "string" && /^https?:\/\//i.test(url); -} -function joinBasePath(base, path) { - if (!base) return path; - const b = base.endsWith("/") ? base.slice(0, -1) : base; - const p = path.startsWith("/") ? path : `/${path}`; - return `${b}${p}`; -} -function normalizeInputToUrl(input) { - if (input instanceof Request) return input.url; - if (typeof input === "string") return input; - return String(input); -} -function toFetchUrl(input) { - const raw = normalizeInputToUrl(input); - if (isAbsoluteHttpUrl(raw)) return raw; - return joinBasePath(getBasePath(), raw); -}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/main/java/com/flyway/auth/service/AuthTokenServiceImpl.java`:
- Around line 100-105: The revokeAllByUserId call inside refresh() is rolled
back when BusinessException is thrown because refresh() is `@Transactional`; to
fix, move the revoke logic into a separate Spring bean method annotated with
`@Transactional`(propagation = Propagation.REQUIRES_NEW) (e.g., create a
RefreshTokenRevocationService with a method revokeAllByUserTokens(userId, now)
that calls refreshTokenRepository.revokeAllByUserId), and replace the direct
repository call in AuthTokenServiceImpl.refresh() with a call to that new
service method so revocation commits immediately even if refresh() rolls back
due to BusinessException.
In `@src/main/resources/config/application-prod.properties`:
- Around line 62-63: The security.allowed-origins property currently only lists
https://flyway.kr which will block requests from https://www.flyway.kr; update
the allowed origins value (property name security.allowed-origins) in
application-prod.properties to include both host variants (e.g., add
https://www.flyway.kr, comma-separated) so OriginRefererCheckFilter will accept
requests from the www subdomain as well.
🧹 Nitpick comments (4)
src/main/java/com/flyway/security/jwt/JwtWebAuthFilter.java (2)
132-133:raw == null체크는 도달 불가능한 코드입니다.Line 130의
resolvePath는request.getRequestURI()(null 반환 없음)를 기반으로 하고, Line 132의 삼항 연산자도 항상 non-null 문자열을 생성합니다.raw == null조건은 절대 true가 될 수 없습니다. 가독성을 위해 제거하거나, 방어적 코딩 의도라면 그대로 두셔도 무방합니다.
125-139:buildReturnUrl로직이 세 개의 클래스에서 중복되고 있습니다.
JwtWebAuthFilter,RefreshTokenSessionSyncFilter,WebLoginRedirectEntryPoint세 클래스 모두에서 거의 동일한 open-redirect 방어 로직이 반복되고 있습니다. 향후 유지보수 시 한쪽만 수정하는 실수를 방지하기 위해, 공통 유틸리티 클래스(예:LoginRedirectUtils)로 추출하는 것을 고려해 주세요.src/test/java/com/flyway/security/filter/OriginRefererCheckFilterTest.java (1)
16-174: 테스트 커버리지가 양호합니다. 몇 가지 추가 케이스를 고려해 주세요.주요 시나리오(허용/차단 Origin, Referer 폴백, OPTIONS 우회, 경로 포함/제외, contextPath)가 잘 커버되어 있습니다. 다음 케이스도 추가하면 더 견고해집니다:
- GET 요청 우회 테스트 —
shouldNotFilter에서 state-changing method가 아닌 요청은 필터를 건너뛰는지 확인- 대소문자 무관 Origin 매칭 —
HTTPS://FLYWAY.KR같은 변형이 통과하는지 확인 (이전 리뷰에서normalizeOrigin에toLowerCase추가를 제안한 바 있음)src/main/java/com/flyway/security/filter/OriginRefererCheckFilter.java (1)
83-109: API 응답 형식 고려: JSON 에러 바디가 더 적합할 수 있습니다.API 체인(
/api/**)에서도text/plain응답("Forbidden")이 반환됩니다. API 클라이언트가 JSON 응답을 기대하는 경우 파싱 오류가 발생할 수 있습니다. 현재 구조에서 API/Web 필터가 같은 클래스를 공유하므로, 필터 유형에 따라 응답 형식을 분기하거나 현재 상태를 유지하되 클라이언트 측에서 403 상태 코드 기반 처리를 하는 것이 좋겠습니다.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/flyway/auth/service/AuthTokenServiceImpl.java (1)
185-188:⚠️ Potential issue | 🟠 Major
revokeAllRefreshTokens를RefreshTokenRevocationService로 통합하세요.이 메서드는
refreshTokenRepository.revokeAllByUserId()를 호출하며,RefreshTokenRevocationService.revokeAllByUserTokens()와 동일한 작업을 수행합니다. 주요 차이점은 트랜잭션 전파 설정입니다:
AuthTokenServiceImpl.revokeAllRefreshTokens():@Transactional(기본값 = REQUIRED)RefreshTokenRevocationService.revokeAllByUserTokens():@Transactional(propagation = REQUIRES_NEW)
UserWithdrawalServiceImpl.withdraw()에서 현재 이 메서드를 호출하고 있는데, 같은 트랜잭션 내에서 실행되므로 사용자 탈퇴 처리가 롤백되면 토큰 폐기도 함께 롤백됩니다. 이는 데이터 일관성 문제를 야기할 수 있습니다.RefreshTokenRevocationService를 사용하거나, 명시적으로 트랜잭션 전파 전략을 정의하세요.
🧹 Nitpick comments (2)
src/main/java/com/flyway/auth/service/AuthTokenServiceImpl.java (1)
84-86:refresh()내에서forceLogout()자기 호출(self-invocation) 시 트랜잭션 경계 확인 필요
refresh()는@Transactional이 적용되어 있고,forceLogout()도@Transactional이지만 동일 빈 내 자기 호출이므로forceLogout()의 트랜잭션 어노테이션은 프록시를 우회하여 적용되지 않습니다. 이후BusinessException이 throw되면refresh()의 트랜잭션이 롤백되어,forceLogout()→logout()내의revokeById()도 함께 롤백됩니다.현재 시나리오에서 실질적인 보안 문제는 제한적입니다:
- 재사용 탐지 (Line 103):
refreshTokenRevocationService.revokeAllByUserTokens()가REQUIRES_NEW로 별도 커밋되므로 안전- 기타 에러 경로: 이미 무효/만료/누락된 토큰이므로
revokeById롤백의 실질적 영향 미미- 쿠키 삭제/세션 무효화: HTTP 응답 작업이므로 DB 롤백과 무관하게 정상 수행
다만, 향후
logout()로직이 복잡해질 경우를 대비해 이 동작을 인지하고 있는 것이 중요합니다.Also applies to: 91-93, 97-99, 103-106, 134-136
src/test/java/com/flyway/auth/service/AuthTokenServiceImplTest.java (1)
100-123: 재사용 토큰 시나리오 테스트가 핵심 보안 로직을 잘 검증합니다.
rotatedAt이 설정된 토큰으로 재사용 탐지 경로를 테스트하고,REQUIRES_NEW트랜잭션의 revocation 서비스 호출과 예외 발생을 모두 검증합니다.추가적으로
forceLogout의 부수 효과(쿠키 삭제, 세션 무효화)도 검증하면 커버리지가 더 강화됩니다. 예를 들어:// forceLogout으로 인한 쿠키 삭제 검증 List<String> cookies = response.getHeaders(HttpHeaders.SET_COOKIE); assertThat(cookies).anyMatch(c -> c.contains("accessToken=;") || c.contains("accessToken=\n")); assertThat(cookies).anyMatch(c -> c.contains("refreshToken=;") || c.contains("refreshToken=\n"));
3 participants
📌 PR 설명
쿠키 기반 인증(JWT Cookie) 환경에서 상태 변경 요청(POST/PUT/PATCH/DELETE)에 대한 CSRF 방어를 강화하고, 추가로 Origin/Referer 검증 레이어를 도입하여 교차 출처 요청 위조를 이중 방어하도록 개선했습니다.
또한 CSRF 토큰을 안정적으로 전달하기 위해 토큰 부트스트랩 엔드포인트 및 프론트 유틸을 추가하고, 검증 필터에 대한 단위 테스트를 함께 작성했습니다.
상태 변경 요청에 대한 동작 과정
팀 공용
csrfFetch함수 추가csrfFetch사용fetchWithRefresh사용fetchWithRefresh내부에는 이미 CSRF 검증 로직(XSRF 쿠키 확인 + X-XSRF-TOKEN 헤더 첨부) 이 포함되어 있으므로인증 요청에서는
csrfFetch를 따로 사용할 필요 없습니다.✅ 완료한 기능 명세
/api/**)에도 CSRF 보호 활성화GET /auth/csrf호출 시 CSRF 토큰 쿠키 발급/갱신 가능csrfFetch.js추가XSRF-TOKEN쿠키 존재 여부 확인/auth/csrf호출하여 토큰 부트스트랩X-XSRF-TOKEN헤더 자동 첨부 후 fetch 실행authFetch.js가csrfFetch를 사용하도록 변경<sec:csrfInput/>적용OriginRefererCheckFilter신규 구현OPTIONS요청은 CORS preflight 고려하여 검사 제외Origin헤더가 존재하면 Origin 우선 검증Referer기반 fallback 검증403 Forbidden응답 처리/api/**요청에서CsrfFilter이전 단계로 Origin/Referer 검증 필터 실행/mypage,/reservations,/payment,/payments/oauth/**,/auth/**,/loginProc📸 스크린샷
API Chain - Origin/Referer + CSRF 검증 흐름
CSRF 토큰 발급 과정
💭 고민과 해결과정
1) CSRF 보호 적용 범위 확장
쿠키 기반 인증(JWT Cookie) 구조에서는 브라우저가 자동으로 쿠키를 포함해 요청을 전송하기 때문에, 상태 변경 요청이 CSRF 공격에 취약해질 수 있었습니다.
이를 해결하기 위해 다음을 적용했습니다.
CookieCsrfTokenRepository.withHttpOnlyFalse()적용/api/**)에도 동일하게 CSRF 활성화 적용GET /auth/csrf)XSRF-TOKEN쿠키 확보 후X-XSRF-TOKEN헤더 자동 첨부하도록csrfFetch.js유틸 추가authFetch.js가csrfFetch를 사용하도록 반영<sec:csrfInput/>적용 (login, signup, home, reservations 등)2) Origin/Referer 검증 필터 추가 (이중 방어)
CSRF 토큰 검증만으로는 토큰 탈취/오용 가능성을 완전히 배제하기 어렵기 때문에, 추가적으로 요청 출처를 확인하는 방식을 도입했습니다.
OriginRefererCheckFilter신규 구현OPTIONS는 제외Origin헤더 우선 검증, 없으면Refererfallback 검증403 Forbidden처리CsrfFilter이전 단계로 필터 적용또한 Web 환경에서는 전체 경로를 무조건 검사할 경우 불필요한 영향이 생길 수 있어,
검사 범위를 제한했습니다.
/mypage,/reservations,/payment,/payments/oauth/**,/auth/**,/loginProc🔗 관련 이슈
Closes #100
Summary by CodeRabbit
새로운 기능
보안 개선
테스트