fix: 로그아웃 로직 중복 수정 #115
fix: 로그아웃 로직 중복 수정 #115
Conversation
WalkthroughIntroduces Redis-backed token management with access-token blacklisting, updates JWT utilities and filters (validation, header parsing, logout handling), adjusts Spring Security filter ordering and exposes POST /logout, modifies error enums, updates transactional annotations in user-related services, changes UserController constructor to accept TokenService, and aligns tests and configs (adds Docker compose for DB/Redis). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant SF as Spring Security FilterChain
participant JF as JwtFilter
participant TS as TokenService
participant SC as SecurityContext
C->>SF: HTTP Request (Authorization: Bearer ...)
SF->>JF: doFilter
JF->>JF: resolveToken(authHeader)
JF->>JF: isAccessToken && isValid(token)
alt Valid access token
JF->>TS: isBlacklisted(token)?
alt Blacklisted
JF-->>SF: Reject (401 INVALID_ACCESS_TOKEN)
else Not blacklisted
JF->>JF: extract username/roles
JF->>SC: set Authentication
JF-->>SF: continue
end
else Invalid/malformed
JF-->>SF: Reject (401 INVALID_ACCESS_TOKEN)
end
sequenceDiagram
autonumber
participant C as Client
participant SF as Spring Security FilterChain
participant JLF as JwtLogoutFilter
participant TS as TokenService
C->>SF: POST /logout (Authorization + refresh-token headers)
SF->>JLF: doFilterInternal
JLF->>JLF: Validate method/path/headers
alt Missing/invalid headers
JLF-->>C: 401 UNAUTHORIZED (INVALID_LOGOUT_AUTH_TOKEN)
else Headers present
JLF->>TS: revokeUserTokens(accessHeader, refreshHeader)
alt CoreException
JLF-->>C: 401 UNAUTHORIZED (error body)
else Other Exception
JLF-->>C: 500 INTERNAL_SERVER_ERROR
else Success
JLF-->>C: 200 OK { result: "SUCCESS" }
end
end
sequenceDiagram
autonumber
participant C as Client
participant UC as UserController
participant TS as TokenService
C->>UC: Authenticated request (Authorization + refresh-token)
UC->>TS: perform token-related ops (as needed)
TS-->>UC: Result
UC-->>C: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
capturecat-core/src/main/java/com/capturecat/core/service/user/WithdrawLogService.java (1)
21-29: Avoid logging raw ‘reason’ (possible PII) and narrow the catchThe error log currently prints the full, user-supplied reason. That can leak PII into logs. Also, catching Exception is overly broad.
Apply this diff to sanitize logging and narrow the catch:
- } catch (Exception e) { - log.error("Failed to persist withdraw log. userId={}, reason={}", userId, reason, e); + } catch (RuntimeException e) { + // Avoid PII leakage; log only metadata about the reason + int reasonLen = (reason != null ? reason.length() : 0); + log.error("Failed to persist withdraw log. userId={}, reason_len={}", userId, reasonLen, e);Notes:
- If the log save must not block withdrawal, keeping the catch is fine. If a failed log save should fail the whole withdrawal, rethrow after logging instead of swallowing.
capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtFilter.java (1)
69-73: Clear security context on rejectionDefensive: if a previous filter accidentally populated the context, make sure we return 401 with a clean context.
private void rejectInvalidToken(HttpServletResponse response, ErrorType errorType) throws IOException { + SecurityContextHolder.clearContext(); response.setStatus(HttpStatus.UNAUTHORIZED.value()); response.setContentType(MediaType.APPLICATION_JSON_VALUE); objectMapper.writeValue(response.getWriter(), ApiResponse.error(errorType)); }capturecat-core/src/test/java/com/capturecat/core/config/jwt/JwtLoginFilterTest.java (1)
88-89: Avoid header-name drift: use JwtUtil.REFRESH_TOKEN_HEADER constant in assertions.Hardcoding "Refresh-Token" risks test breakage if the header constant changes. Reference the constant to keep tests aligned with production.
- .andExpect(header().exists("Refresh-Token")) - .andExpect(header().string("Refresh-Token", startsWith("Bearer "))) + .andExpect(header().exists(JwtUtil.REFRESH_TOKEN_HEADER)) + .andExpect(header().string(JwtUtil.REFRESH_TOKEN_HEADER, startsWith("Bearer ")))capturecat-core/src/main/java/com/capturecat/core/service/auth/TokenService.java (1)
74-76: Do not log raw refresh tokens.Logging tokens at INFO is a security/PII risk. Remove or mask.
- log.info("Refresh Token: {}", refreshToken); + // Avoid logging raw tokens; optionally log username or a fingerprint + log.debug("Parsed refresh token for user {}", jwtUtil.getUsername(refreshToken));
🧹 Nitpick comments (37)
capturecat-clients/uploader/src/main/java/com/capturecat/client/upload/ErrorCode.java (1)
8-12: Optional: align message style across S3 vs LOCAL errorsLOCAL_* still says “I/O 오류 발생” while S3_* now says generic “오류 발생”. If you want consistent UX, either keep generic across the board or specify the layer (I/O) for both. Not a blocker.
capturecat-clients/uploader/src/main/java/com/capturecat/client/upload/S3FileUploader.java (3)
37-41: Broadened catch + memory usage on upload; prefer streaming and consistent error loggingCatching Exception is acceptable given unified ErrorCode, but consider:
- Avoid buffering entire file with file.getBytes() for large uploads.
- Make the log message uniform and include stack trace (current string has a trailing comma).
Proposed tweak keeps unified handling while improving memory and logs.
- try { - s3Client.putObject(request, RequestBody.fromBytes(file.getBytes())); - } catch (Exception e) { - log.error("S3FileUploader.upload error,", e); - throw new UploadException(ErrorCode.S3_UPLOAD_FAILED, e); - } + try { + // Stream instead of loading whole file into memory + s3Client.putObject(request, RequestBody.fromInputStream(file.getInputStream(), file.getSize())); + } catch (Exception e) { + // Keep unified ErrorCode but ensure consistent message and stack trace + log.error("S3FileUploader.upload error", e); + throw new UploadException(ErrorCode.S3_UPLOAD_FAILED, e); + }
55-58: Log full exception (not only message) on delete; keep error handling consistent with uploadLogging only e.getMessage() drops stack trace and context. Align with upload’s logging.
- } catch (SdkException e) { - log.error("S3FileUploader.delete error = {}", e.getMessage()); - throw new DeleteException(ErrorCode.S3_DELETE_FAILED, e); + } catch (SdkException e) { + log.error("S3FileUploader.delete error", e); + throw new DeleteException(ErrorCode.S3_DELETE_FAILED, e);
44-45: Optional: guard against double slashes in returned URLIf urlPrefix() already ends with “/”, String.join("/", ...) can yield “//”. Consider normalizing.
- return String.join("/", s3Properties.urlPrefix(), key); + return normalizeUrlJoin(s3Properties.urlPrefix(), key);Add once in the class:
private String normalizeUrlJoin(String prefix, String key) { String p = prefix.endsWith("/") ? prefix.substring(0, prefix.length() - 1) : prefix; String k = key.startsWith("/") ? key.substring(1) : key; return p + "/" + k; }capturecat-core/src/main/java/com/capturecat/core/service/user/WithdrawLogService.java (1)
3-5: Import cleanup (only if you keep @transactional without propagation)If you decide not to adopt Propagation.MANDATORY, the Propagation import becomes unused and should be removed to keep the file clean.
-import org.springframework.transaction.annotation.Propagation;capturecat-core/src/docs/asciidoc/user.adoc (2)
20-26: Docs may be slightly stale vs. new propagation rule.
- The text says deletion proceeds even if social unlink fails. With the new propagation change, ensure docs also state that “deletion reason” is not saved if the member deletion itself fails, to prevent confusion during failure triage.
21-21: Verify request-fields snippet and add revocation & propagation notes
Verified in
UserControllerTest.java(line 90) thatrequestFields(fieldWithPath("reason")…)is present, so therequest-fieldssnippet is generated correctly.In
capturecat-core/src/docs/asciidoc/user.adoc, immediately after theoperation::withdraw[…]block, add a short note clarifying token revocation:operation::withdraw[snippets='curl-request,http-request,request-headers,request-fields,http-response,response-fields'] + +.Note +==== +On successful withdrawal, the Authorization token supplied in the request is revoked and can no longer be used. +====In the “실패” section of the same doc, clarify that the deletion reason isn’t persisted on failure, for consistency with the propagation change:
==== 실패 … + +NOTE: If the withdrawal operation fails, the `reason` field is not persisted to avoid propagating an incomplete transaction.capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLoginFilter.java (2)
60-71: Enrich login-success audit log (IP/UA) and consider debug level in high-traffic envs.If you want more actionable auth trails, include IP and User-Agent. Also consider downgrading to debug in very busy systems.
Apply:
@@ @Override protected void successfulAuthentication(HttpServletRequest request, HttpServletResponse response, FilterChain chain, Authentication authResult) throws IOException { @@ //토큰 발급 Map<TokenType, String> tokenMap = tokenIssueService.issue(username, UserRole.fromRoleString(role)); - log.info("[JwtLoginFilter.successfulAuthentication] 사용자 로그인({}), 토큰 발급", username); + String xff = request.getHeader("X-Forwarded-For"); + String ip = (xff != null && !xff.isBlank()) ? xff.split(",")[0].trim() : request.getRemoteAddr(); + String ua = request.getHeader("User-Agent"); + log.info("[JwtLoginFilter.successfulAuthentication] username={}, ip={}, ua={}, token_issued", + username, ip, ua);Note: If you prefer, switch log.info to log.debug to reduce noise.
--- `40-43`: **Minor: consider injecting ObjectMapper to reuse global configuration.** Creating a new ObjectMapper bypasses any global modules (JavaTimeModule, custom serializers) registered as a Spring @Bean. Optional: inject a shared ObjectMapper bean. </blockquote></details> <details> <summary>.github/workflows/deploy-dev.yml (5)</summary><blockquote> `68-70`: **Verify Java 21 is installed on EC2 before starting the app** The runner builds with JDK 21, but EC2 may not have Java 21 installed. Starting a Java 21-compiled app with an older JRE will fail at runtime. Consider installing Temurin 21 JRE at deploy time: ```diff # dev 프로필로 실행 - sudo nohup java ${JAVA_OPTS} -jar project.jar --spring.profiles.active=dev > output.log 2>&1 & + if ! java -version 2>&1 | grep -q 'version "21'; then + sudo apt-get update + sudo apt-get install -y wget gnupg + wget -qO- https://packages.adoptium.net/artifactory/api/gpg/key/public | sudo tee /etc/apt/trusted.gpg.d/adoptium.asc + echo "deb https://packages.adoptium.net/artifactory/deb $(. /etc/os-release && echo $VERSION_CODENAME) main" | sudo tee /etc/apt/sources.list.d/adoptium.list + sudo apt-get update && sudo apt-get install -y temurin-21-jre + fi + sudo nohup java ${JAVA_OPTS} -jar project.jar --spring.profiles.active=dev > output.log 2>&1 &
82-99: Health check should target the local instance to avoid DNS/SSL/proxy couplingChecking https://dev.capture-cat.com/health ties readiness to DNS, certs, and the proxy path. If Nginx is down (e.g., killed earlier), the app may be healthy but the check will fail.
Probe the local instance first (and optionally the public endpoint later):
- if curl -sf https://dev.capture-cat.com/health | grep -q '"status":"UP"'; then + if curl -sf http://localhost:80/health | grep -q '"status":"UP"'; then echo "헬스체크 성공!" break 2 fiIf you keep HTTPS, ensure the proxy remains alive and that you aren’t killing it earlier in the script.
21-25: Safer multi-line secret write for application-dev.ymlecho may mangle newlines/quotes. Use a heredoc to preserve content verbatim.
- - name: application.yml 파일 만들기 - run: | - mkdir -p ./capturecat-core/src/main/resources - echo "${{ secrets.DEV_APPLICATION_PROPERTIES }}" > ./capturecat-core/src/main/resources/application-dev.yml + - name: application.yml 파일 만들기 + run: | + mkdir -p ./capturecat-core/src/main/resources + cat > ./capturecat-core/src/main/resources/application-dev.yml << 'EOF' + ${{ secrets.DEV_APPLICATION_PROPERTIES }} + EOF
33-34: Pin third-party actions to commit SHAs for supply-chain safetyappleboy/* actions are widely used, but best practice is to pin to a specific commit SHA rather than a moving tag.
Example:
- uses: appleboy/[email protected] + uses: appleboy/scp-action@<pinned-commit-sha> ... - uses: appleboy/[email protected] + uses: appleboy/ssh-action@<pinned-commit-sha>Do the same for other third-party actions where feasible.
Also applies to: 42-43
3-7: Consider restricting triggers and adding concurrency controlsDeploying DEV on every push to feature/** can cause noisy or overlapping deploys.
- Add concurrency to cancel previous in-flight runs per branch:
concurrency: group: dev-deploy-${{ github.ref }} cancel-in-progress: true
- Optionally, trigger on PR merge to develop or on tags intended for DEV.
capturecat-core/src/test/java/com/capturecat/core/api/user/UserApiTest.java (1)
49-49: Good consistency: Replaced hard-coded paths with URL_PREFIXReduces string duplication and mistakes in future refactors.
Consider making URL_PREFIX private unless it’s reused externally.
Also applies to: 73-73, 92-92
capturecat-core/src/main/java/com/capturecat/core/config/SecurityConfig.java (2)
49-50: Confirm filter ordering with logout disabledYou disable Spring Security’s logout (logout(AbstractHttpConfigurer::disable)) and then register JwtLogoutFilter before LogoutFilter. In Spring Security 6, ordering is anchored by known filter order even if the anchor filter isn’t present, but it’s worth verifying the actual position.
If you want to be explicit and independent of LogoutFilter presence, consider anchoring relative to UsernamePasswordAuthenticationFilter (or SecurityContextHolderFilter), ensuring your logout filter runs early and only once:
- .addFilterBefore(new JwtLogoutFilter(tokenService), LogoutFilter.class) + .addFilterBefore(new JwtLogoutFilter(tokenService), UsernamePasswordAuthenticationFilter.class)Also verify JwtLogoutFilter short-circuits non-/logout requests and enforces HTTP method semantics (e.g., POST only).
Also applies to: 55-56
58-59: Consider whitelisting the error-code docs endpoint (if exposed in non-test profiles)Tests mock the controller, but in real deployments you might want /v1/error-codes publicly accessible for documentation.
- .requestMatchers("/health", "/docs/**", "/token/reissue", "/v1/auth/**", "/v1/user/join", "/logout") + .requestMatchers("/health", "/docs/**", "/v1/error-codes", "/token/reissue", "/v1/auth/**", "/v1/user/join", "/logout")capturecat-core/src/main/java/com/capturecat/core/support/error/ErrorType.java (1)
25-25: Refine ErrorCode Mapping for INVALID_JWTThe
ErrorType.INVALID_JWTentry is currently mapped toErrorCode.BEAN_VALIDATION_FAIL, which conveys a generic data‐validation error rather than an invalid JWT. To improve clarity and maintain consistent API semantics:• File:
capturecat-core/src/main/java/com/capturecat/core/support/error/ErrorType.java
Line 25:-INVALID_JWT(HttpStatus.BAD_REQUEST, ErrorCode.BEAN_VALIDATION_FAIL, LogLevel.WARN), +INVALID_JWT(HttpStatus.BAD_REQUEST, ErrorCode.INVALID_TOKEN, LogLevel.WARN),Use the existing
INVALID_TOKEN("유효하지 않은 토큰입니다.")for a generic invalid‐token response.• If you prefer a more specific message for JWTs, add a new constant in
ErrorCode.java:INVALID_JWT("유효하지 않은 JWT 토큰입니다."),and then map to that constant instead.
This change is purely an optional refactor to align error codes with their intended semantics.
capturecat-core/src/test/java/com/capturecat/core/config/jwt/JwtFilterTest.java (2)
72-86: Strengthen the negative-path assertions.In the 401 cases, also verify the chain is not invoked to lock in behavior.
// then assertEquals(HttpStatus.UNAUTHORIZED.value(), response.getStatus()); + verify(filterChain, never()).doFilter(any(), any());
88-99: Add a test for blacklisted tokens.Covers the new TokenService blacklist integration and prevents regressions where a valid-but-revoked token passes.
@Test void 블랙리스트_토큰이면_401응답() throws Exception { String token = "revoked.token"; request.addHeader(HttpHeaders.AUTHORIZATION, JwtUtil.BEARER_PREFIX + token); given(jwtUtil.resolveToken(JwtUtil.BEARER_PREFIX + token)).willReturn(token); given(jwtUtil.isAccessToken(token)).willReturn(true); given(jwtUtil.isValid(token)).willReturn(true); given(tokenService.isBlacklisted(token)).willReturn(true); jwtFilter.doFilterInternal(request, response, filterChain); assertEquals(HttpStatus.UNAUTHORIZED.value(), response.getStatus()); verify(filterChain, never()).doFilter(any(), any()); }capturecat-core/src/test/java/com/capturecat/core/api/auth/TokenApiTest.java (2)
80-88: Assert header formats, not just existence, and verify refresh-token deletion.Strengthen expectations and interaction checks to assert Bearer prefix and old refresh-token removal during reissue.
mockMvc.perform(post("/token/reissue") .header(JwtUtil.REFRESH_TOKEN_HEADER, refreshHeaderValue)) .andDo(print()) .andExpect(status().isOk()) - .andExpect(header().exists(HttpHeaders.AUTHORIZATION)) - .andExpect(header().exists(JwtUtil.REFRESH_TOKEN_HEADER)) + .andExpect(header().string(HttpHeaders.AUTHORIZATION, startsWith(JwtUtil.BEARER_PREFIX))) + .andExpect(header().string(JwtUtil.REFRESH_TOKEN_HEADER, startsWith(JwtUtil.BEARER_PREFIX))) .andExpect(jsonPath("$.result").value("SUCCESS")) .andExpect(jsonPath("$.data").doesNotExist()); + + // old refresh token should be removed + verify(redisTemplate).delete("refresh_token:testUser");
15-20: Remove unused imports.Disabled and ArgumentMatchers aren’t used.
- import org.junit.jupiter.api.Disabled; - import org.mockito.ArgumentMatchers;capturecat-core/src/test/java/com/capturecat/core/config/jwt/JwtLogoutFilterTest.java (2)
14-14: Prefer Spring’s HttpStatus for consistency across tests.Other tests use org.springframework.http.HttpStatus. Consider standardizing.
- import org.apache.http.HttpStatus; + import org.springframework.http.HttpStatus; - assertEquals(HttpStatus.SC_UNAUTHORIZED, response.getStatus()); + assertEquals(HttpStatus.UNAUTHORIZED.value(), response.getStatus()); - assertEquals(HttpStatus.SC_UNAUTHORIZED, response.getStatus()); + assertEquals(HttpStatus.UNAUTHORIZED.value(), response.getStatus()); - assertEquals(HttpStatus.SC_OK, response.getStatus()); + assertEquals(HttpStatus.OK.value(), response.getStatus());Also applies to: 77-79, 100-103, 122-124
66-80: Add explicit tests for missing single-token headers.Covers cases where only one of the headers is provided (access or refresh), ensuring 401 and no chain progression.
@Test @DisplayName("POST /logout + Access만 있고 Refresh 없으면 401") void logout_request_without_refresh_token() throws Exception { request.setMethod("POST"); request.setRequestURI("/logout"); request.addHeader(HttpHeaders.AUTHORIZATION, BEARER_PREFIX + "access-only"); jwtLogoutFilter.doFilter(request, response, filterChain); assertEquals(HttpStatus.UNAUTHORIZED.value(), response.getStatus()); verify(tokenService, never()).revokeUserTokens(anyString(), anyString()); verify(filterChain, never()).doFilter(any(), any()); } @Test @DisplayName("POST /logout + Refresh만 있고 Access 없으면 401") void logout_request_without_access_token() throws Exception { request.setMethod("POST"); request.setRequestURI("/logout"); request.addHeader(REFRESH_TOKEN_HEADER, BEARER_PREFIX + "refresh-only"); jwtLogoutFilter.doFilter(request, response, filterChain); assertEquals(HttpStatus.UNAUTHORIZED.value(), response.getStatus()); verify(tokenService, never()).revokeUserTokens(anyString(), anyString()); verify(filterChain, never()).doFilter(any(), any()); }capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtUtil.java (1)
86-91: Expose remaining TTL helper for blacklist TTL computationCallers typically need remaining TTL (exp - now), not the absolute exp timestamp. This utility prevents duplicated math and negative TTL bugs.
Add:
public long getExpiration(String token) { Claims claims = extractClaims(token); return claims.getExpiration().getTime(); } + +/** 현재 시점 기준 남은 만료 시간(ms), 이미 만료면 0 */ +public long getRemainingTtlMillis(String token) { + long exp = getExpiration(token); + long now = System.currentTimeMillis(); + return Math.max(0, exp - now); +}capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtFilter.java (2)
43-45: Tone down noisy logs for missing/malformed Authorization headerThis path will be hit by every public endpoint. Use DEBUG to avoid log spam in production.
- log.info("Authorization header missing or malformed, url={}", request.getRequestURI()); + log.debug("Authorization header missing or malformed, url={}", request.getRequestURI());
75-79: Widen logout skip to handle trailing slashes or versioned paths if applicableIf your logout endpoint could be exposed via multiple servlet paths (e.g., behind a gateway adding a prefix) or with a trailing slash, consider a more flexible match.
- return "/logout".equals(path); // 로그아웃은 스킵 + return "/logout".equals(path) || "/logout/".equals(path);Or use AntPathMatcher if you later need patterns.
capturecat-core/src/test/java/com/capturecat/core/api/auth/LogInOutTest.java (2)
9-9: Prefer Spring’s HttpHeaders/HttpStatus to avoid an extra Apache dependency in testsSmall cleanup to keep test deps consistent with the main code.
- import org.apache.http.HttpHeaders; + import org.springframework.http.HttpHeaders; @@ - .statusCode(org.apache.http.HttpStatus.SC_OK) + .statusCode(org.springframework.http.HttpStatus.OK.value())Also applies to: 57-57
44-47: Verify revokeUserTokens is called with exact header values (including Bearer prefix)Strengthens the test contract and guards accidental changes to header formatting.
- then(tokenService).should(org.mockito.Mockito.times(1)) - .revokeUserTokens(anyString(), anyString()); + then(tokenService).should(org.mockito.Mockito.times(1)) + .revokeUserTokens( + org.mockito.ArgumentMatchers.eq(JwtUtil.BEARER_PREFIX + accessTokenHeader), + org.mockito.ArgumentMatchers.eq(JwtUtil.BEARER_PREFIX + refreshTokenHeader) + );Also applies to: 74-76
capturecat-core/src/test/java/com/capturecat/core/api/user/UserApiMockTest.java (2)
51-64: SecurityContext + @WithMockUser: OK, but consider a custom WithSecurityContext for LoginUserYou’re setting a LoginUser in the context and also using @WithMockUser. That works, but it’s a bit redundant. A custom @WithLoginUser factory would make intent explicit and reduce boilerplate.
I can provide a concise WithSecurityContextFactory if you want to adopt this pattern.
80-93: Tighten interaction verifications to assert exact headers passed to revokeUserTokensPrevents subtle regressions if header formatting changes.
- then(userService).should().withdraw(Mockito.any(LoginUser.class), any()); - then(tokenService).should().revokeUserTokens(anyString(), anyString()); + then(userService).should().withdraw(Mockito.any(LoginUser.class), any()); + then(tokenService).should().revokeUserTokens( + org.mockito.ArgumentMatchers.eq(accessTokenHeader), + org.mockito.ArgumentMatchers.eq(refreshTokenHeader) + );Note: In this test you’re already passing prefixed header values; keep the assertion aligned.
capturecat-core/src/main/java/com/capturecat/core/api/user/UserController.java (1)
66-71: Null-safe token revocation and clearer intentHeaders may be absent in edge cases; guard nulls to avoid passing null down to the service. Optionally log at DEBUG for traceability.
- String accessHeader = headers.getFirst(HttpHeaders.AUTHORIZATION); - String refreshHeader = headers.getFirst(JwtUtil.REFRESH_TOKEN_HEADER); - tokenService.revokeUserTokens(accessHeader, refreshHeader); + String accessHeader = headers.getFirst(HttpHeaders.AUTHORIZATION); + String refreshHeader = headers.getFirst(JwtUtil.REFRESH_TOKEN_HEADER); + if (accessHeader != null || refreshHeader != null) { + tokenService.revokeUserTokens(accessHeader, refreshHeader); + }If TokenService expects raw tokens, consider resolving here and passing tokens instead of headers.
capturecat-core/src/test/java/com/capturecat/core/config/jwt/JwtLoginFilterTest.java (1)
15-16: Unify Mockito matchers usage.You mix static and qualified ArgumentMatchers. Prefer the static import consistently to reduce noise.
-import org.mockito.ArgumentMatchers; +// (optional) rely on the existing static import of org.mockito.ArgumentMatchers.* -willDoNothing().given(valueOps).set( - anyString(), anyString(), anyLong(), ArgumentMatchers.any(TimeUnit.class) -); +willDoNothing().given(valueOps).set( + anyString(), anyString(), anyLong(), any(TimeUnit.class) +);Also applies to: 62-64
capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLogoutFilter.java (2)
69-71: Set content type for the 500 branch as well.Keep response shape consistent across error paths.
} catch (Exception e) { response.setStatus(HttpStatus.INTERNAL_SERVER_ERROR.value()); - objectMapper.writeValue(response.getWriter(), + response.setContentType(MediaType.APPLICATION_JSON_VALUE); + objectMapper.writeValue(response.getWriter(), ApiResponse.error(ErrorType.INTERNAL_SERVER_ERROR)); }
31-33: Prefer injecting Spring’s ObjectMapper to honor global Jackson config (modules, naming, dates).Constructing a new ObjectMapper bypasses your app’s Jackson configuration.
If desired, I can provide a small patch to:
- change this field to
private final ObjectMapper objectMapper;- update SecurityConfig to pass the bean:
new JwtLogoutFilter(tokenService, objectMapper).capturecat-core/src/test/java/com/capturecat/core/service/auth/TokenServiceTest.java (2)
46-53: Reduce coupling to internal Redis key format in tests.Re-declaring
"refresh_token:"and"blacklist:"in tests couples them to private constants. Consider asserting via argument capture and prefix checks instead of exact strings.I can provide a follow-up patch that uses
ArgumentCaptor<String>to capture the keys passed toset()/hasKey()/delete()andassertThat(key).startsWith("refresh_token:").Also applies to: 69-75, 82-87
111-142: Consider adding a focused test for revokeUserTokens().It’s the core path for logout and withdrawal. Verifying that it blacklists access and deletes refresh (in order) would harden the contract.
Want a small unit test that stubs
jwtUtil.resolveToken(...),jwtUtil.isRefreshToken(...),valueOps.get(...)and verifiesredisTemplate.delete(...)+opsForValue().set(...)calls?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (30)
.github/workflows/deploy-dev.yml(1 hunks)capturecat-clients/uploader/src/main/java/com/capturecat/client/upload/ErrorCode.java(1 hunks)capturecat-clients/uploader/src/main/java/com/capturecat/client/upload/S3FileUploader.java(3 hunks)capturecat-core/build.gradle(1 hunks)capturecat-core/src/docs/asciidoc/user.adoc(1 hunks)capturecat-core/src/main/java/com/capturecat/CaptureCatApplication.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/api/user/UserController.java(3 hunks)capturecat-core/src/main/java/com/capturecat/core/config/SecurityConfig.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtFilter.java(4 hunks)capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLoginFilter.java(3 hunks)capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLogoutFilter.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtUtil.java(2 hunks)capturecat-core/src/main/java/com/capturecat/core/domain/auth/RefreshToken.java(0 hunks)capturecat-core/src/main/java/com/capturecat/core/domain/auth/RefreshTokenRepository.java(0 hunks)capturecat-core/src/main/java/com/capturecat/core/service/auth/TokenService.java(5 hunks)capturecat-core/src/main/java/com/capturecat/core/service/user/UserService.java(0 hunks)capturecat-core/src/main/java/com/capturecat/core/service/user/WithdrawLogService.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/support/error/ErrorCode.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/support/error/ErrorType.java(1 hunks)capturecat-core/src/test/java/com/capturecat/core/api/auth/LogInOutTest.java(2 hunks)capturecat-core/src/test/java/com/capturecat/core/api/auth/TokenApiTest.java(2 hunks)capturecat-core/src/test/java/com/capturecat/core/api/error/ErrorCodeControllerTest.java(1 hunks)capturecat-core/src/test/java/com/capturecat/core/api/user/UserApiMockTest.java(1 hunks)capturecat-core/src/test/java/com/capturecat/core/api/user/UserApiTest.java(4 hunks)capturecat-core/src/test/java/com/capturecat/core/api/user/UserControllerTest.java(2 hunks)capturecat-core/src/test/java/com/capturecat/core/config/jwt/JwtFilterTest.java(5 hunks)capturecat-core/src/test/java/com/capturecat/core/config/jwt/JwtLoginFilterTest.java(3 hunks)capturecat-core/src/test/java/com/capturecat/core/config/jwt/JwtLogoutFilterTest.java(4 hunks)capturecat-core/src/test/java/com/capturecat/core/service/auth/TokenServiceTest.java(7 hunks)capturecat-core/src/test/resources/application.yml(1 hunks)
💤 Files with no reviewable changes (3)
- capturecat-core/src/main/java/com/capturecat/core/domain/auth/RefreshTokenRepository.java
- capturecat-core/src/main/java/com/capturecat/core/domain/auth/RefreshToken.java
- capturecat-core/src/main/java/com/capturecat/core/service/user/UserService.java
🧰 Additional context used
🧬 Code graph analysis (7)
capturecat-core/src/test/java/com/capturecat/core/api/user/UserApiMockTest.java (1)
capturecat-core/src/main/java/com/capturecat/core/api/user/dto/UserReqDto.java (1)
UserReqDto(17-62)
capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLoginFilter.java (2)
capturecat-core/src/main/java/com/capturecat/core/config/SecurityConfig.java (1)
Slf4j(28-75)capturecat-core/src/main/java/com/capturecat/core/service/auth/TokenService.java (1)
Slf4j(24-158)
capturecat-core/src/main/java/com/capturecat/core/api/user/UserController.java (3)
capturecat-core/src/main/java/com/capturecat/core/api/auth/Oauth2AuthController.java (1)
RestController(28-59)capturecat-core/src/main/java/com/capturecat/core/api/user/dto/UserReqDto.java (1)
UserReqDto(17-62)capturecat-core/src/main/java/com/capturecat/core/api/user/dto/UserRespDto.java (1)
UserRespDto(10-48)
capturecat-core/src/test/java/com/capturecat/core/config/jwt/JwtFilterTest.java (1)
capturecat-core/src/test/java/com/capturecat/core/service/auth/TokenServiceTest.java (1)
ExtendWith(26-205)
capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLogoutFilter.java (3)
capturecat-core/src/main/java/com/capturecat/core/config/SecurityConfig.java (1)
Slf4j(28-75)capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtFilter.java (1)
Slf4j(28-80)capturecat-core/src/main/java/com/capturecat/core/service/auth/TokenService.java (1)
Slf4j(24-158)
capturecat-core/src/test/java/com/capturecat/core/config/jwt/JwtLoginFilterTest.java (1)
capturecat-core/src/test/java/com/capturecat/core/config/SecurityConfigTest.java (1)
AutoConfigureMockMvc(17-37)
capturecat-core/src/test/java/com/capturecat/core/service/auth/TokenServiceTest.java (1)
capturecat-core/src/test/java/com/capturecat/core/config/jwt/JwtFilterTest.java (1)
ExtendWith(24-110)
🔇 Additional comments (18)
capturecat-clients/uploader/src/main/java/com/capturecat/client/upload/ErrorCode.java (2)
8-12: Good consolidation of S3 error codesUnifying S3 errors to upload/download/delete is cleaner and reduces coupling to transport-layer details. Enum naming and messages look consistent.
8-12: No remaining references to removed ErrorCode constantsI’ve searched the entire repository for usages of the removed constants
ErrorCode.S3_UPLOAD_FAILED_IOandErrorCode.S3_DOWNLOAD_FAILED_SDKand found no occurrences. This confirms there are no lingering references.capturecat-clients/uploader/src/main/java/com/capturecat/client/upload/S3FileUploader.java (2)
19-21: Profile annotation formatting change is fineNo functional change; consistent with common Spring formatting.
47-58: Validate delete(...) only receives S3 object keys
I didn’t find any call sites forS3FileUploader.delete(...)in the codebase, so please manually confirm that every caller is passing only the S3 object key (not a full URL) into this method. If there’s any risk that someone could pass the URL returned byupload(...), we should safeguard against it. You can either:
- Strip the configured
urlPrefix()when present before deleting, or- Rename the
fileNameparameter tokeyand update its Javadoc to explicitly state it must be an object key.Let me know if you’d like me to add a defensive strip inside the
delete(...)method to handle full URLs gracefully.capturecat-core/src/main/java/com/capturecat/core/service/user/WithdrawLogService.java (1)
19-20: Enforce caller’s transaction onsave(...)with Propagation.MANDATORYVerified that the only call site for
WithdrawLogService.save(...)resides inUserService.withdraw(...), which is annotated with@Transactional; no other usages exist. ApplyingPropagation.MANDATORYhere will ensuresavefails fast if ever invoked outside an active transaction, preventing any rogue log persistence.• File:
capturecat-core/src/main/java/com/capturecat/core/service/user/WithdrawLogService.java
Apply:- @Transactional + @Transactional(propagation = Propagation.MANDATORY) public void save(Long userId, String reason) {• No additional callers need annotation changes—
UserService.withdraw(...)at line 100 is already transactional.capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLoginFilter.java (1)
1-88: Verify JwtLogoutFilter setup and user‐deletion transaction behavior
- JwtLogoutFilter inheritance confirmed
JwtLogoutFilterextendsOncePerRequestFilter(capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLogoutFilter.java:28)- Single registration in security chain confirmed
- Registered only once via
.addFilterBefore(new JwtLogoutFilter(tokenService), LogoutFilter.class)inSecurityConfig(capturecat-core/src/main/java/com/capturecat/core/config/SecurityConfig.java:55)- No additional
FilterRegistrationBeanforJwtLogoutFilterfound- User‐deletion propagation requires manual verification
- No
@Transactionalor specificPropagationsettings detected around methods named for user withdrawal or deletion- Please review the service class responsible for user deletion (e.g.,
UserService/MemberService) to ensure that “deletion reason” changes are wrapped in a transaction that properly rolls back when failures occurcapturecat-core/src/main/java/com/capturecat/CaptureCatApplication.java (1)
7-7: @ConfigurationPropertiesScan Scope Verified
- Detected 2 @ConfigurationProperties classes under
com.capturecat:
Oauth2Properties(prefix ="spring.security.oauth2.client")SocialApiProperties(prefix ="social.api")- No duplicate prefixes or unexpected properties classes were found.
Scanning the entire
com.capturecatpackage is safe in the current codebase.
(Optional) In future, if additional properties classes are introduced, you may choose to narrow the scan base or switch to@EnableConfigurationPropertieswith an explicit list to prevent unintended bindings.capturecat-core/src/test/java/com/capturecat/core/api/error/ErrorCodeControllerTest.java (1)
85-86: LGTM: Logout error codes updated to INVALID_AUTH_TOKEN and INTERNAL_SERVER_ERRORMatches the new logout flow and broader server error surface. Ensure the public API docs reflect this change.
If you want, I can regenerate the REST Docs snippets and verify that the rendered docs show the new codes under errorCode/logout.
capturecat-core/src/test/java/com/capturecat/core/api/user/UserApiTest.java (1)
27-28: Nice: Centralized URL prefix constantImproves maintainability across tests; avoids repeating literals.
capturecat-core/src/main/java/com/capturecat/core/config/SecurityConfig.java (1)
51-55: LGTM: JwtFilter placement and JwtLoginFilter registrationJwtFilter before UsernamePasswordAuthenticationFilter and JwtLoginFilter at UsernamePasswordAuthenticationFilter’s slot is a sane ordering for stateless JWT auth.
capturecat-core/src/main/java/com/capturecat/core/support/error/ErrorType.java (1)
43-44: New INTERNAL_SERVER_ERROR enum looks correct; comma fix after MISSING_PARAMETER is appropriate.HttpStatus, ErrorCode, and LogLevel mapping for both entries are sensible. No further changes needed here.
capturecat-core/src/test/java/com/capturecat/core/api/user/UserControllerTest.java (1)
40-47: Constructor injection update to include TokenService is fine.Mocking TokenService and passing it to UserController aligns with the production signature change. No issues spotted in test wiring.
capturecat-core/src/test/java/com/capturecat/core/api/auth/TokenApiTest.java (1)
47-75: Confirmed Spring Boot 3.5.0 supports @MockitoBean – no changes requiredThe project’s
build.gradledeclaresorg.springframework.bootversion 3.5.0, and multiple test classes (including TokenApiTest) already import and useorg.springframework.test.context.bean.override.mockito.MockitoBean. Since Spring Boot 3.5.0 (which pulls in Spring Framework 6.3.x) fully supports@MockitoBean, there’s no need to switch to@MockBean.• Verified Spring Boot version in build.gradle:
id 'org.springframework.boot' version '3.5.0'
• Confirmed existing use of@MockitoBeanacross several tests (e.g., UserApiMockTest, JwtLoginFilterTest)capturecat-core/src/test/java/com/capturecat/core/config/jwt/JwtLogoutFilterTest.java (1)
51-64: Behavior on non-/logout paths is correct.Using GET /not-logout and verifying chain invocation + no token revocation matches OncePerRequestFilter.shouldNotFilter behavior.
capturecat-core/src/test/java/com/capturecat/core/api/user/UserApiMockTest.java (1)
57-64: Stubs for JwtFilter pass-through might be unnecessary in @WebMvcTest(UserController)Since JwtFilter isn’t part of this MVC slice, resolveToken/isValid stubs likely don’t influence this test. If so, remove for clarity; if you plan to include the filter later, keeping them is fine.
Would you like a follow-up PR to introduce a FilterRegistration in this slice and make these stubs meaningful?
capturecat-core/src/main/java/com/capturecat/core/api/user/UserController.java (1)
45-52: Nice improvement: tutorialComplete now returns ApiResponse.success()Aligned with the API’s standard response shape. No issues.
capturecat-core/src/test/java/com/capturecat/core/config/jwt/JwtLoginFilterTest.java (2)
46-50: Good isolation of Redis with @MockitoBean; stubbing covers the token flows.Replacing the real Redis bean with a MockitoBean and stubbing opsForValue()/hasKey()/delete() gives deterministic tests without external dependencies. Nice.
Also applies to: 55-69
67-68: Double-check other Redis key lookups in the login flow.You stub hasKey() only for keys starting with "blacklist:". If any part of the login pipeline checks other Redis keys with hasKey(), Mockito will default to false, which could mask unintended calls. Verify no unexpected hasKey() checks occur during login.
capturecat-core/src/main/java/com/capturecat/core/api/user/UserController.java
Show resolved
Hide resolved
| MISSING_PARAMETER("필수 파라미터 %s(이)가 누락되었습니다."), | ||
| INTERNAL_SERVER_ERROR("서버에서 오류가 발생했습니다."); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
INTERNAL_SERVER_ERROR added correctly; ensure ErrorType and handler mapping updated.
- The enum addition and the trailing comma fix for MISSING_PARAMETER are correct.
- Please confirm ErrorType has a matching INTERNAL_SERVER_ERROR and that your global exception handler maps unhandled exceptions to this code with HTTP 500.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check ErrorType mapping:"
rg -nP --type=java -n 'INTERNAL_SERVER_ERROR' capturecat-core/src/main/java
echo -e "\nCheck controller advice / exception handler uses the new code for 500 paths:"
rg -nP --type=java -n -C3 '(ExceptionHandler|ControllerAdvice|RestControllerAdvice)' capturecat-core/src/main/java | rg -nP -C2 'INTERNAL_SERVER_ERROR|HttpStatus\.INTERNAL_SERVER_ERROR|500' || trueLength of output: 2038
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locating ControllerAdvice and RestControllerAdvice classes with context:"
rg -nP '@RestControllerAdvice|@ControllerAdvice' -C5 capturecat-core/src/main/java
echo -e "\nInspecting ExceptionHandler methods in those classes:"
rg --files-with-matches '@RestControllerAdvice|@ControllerAdvice' -g 'capturecat-core/src/main/java/**/*.java' | xargs -I{} rg -n -C5 '@ExceptionHandler' {}Length of output: 3815
Update global exception handler to use INTERNAL_SERVER_ERROR mapping
The catch-all handler still returns ErrorType.DEFAULT_ERROR rather than the newly added ErrorType.INTERNAL_SERVER_ERROR, so unhandled exceptions aren’t mapped to your new error code. Please update as follows:
• File: capturecat-core/src/main/java/com/capturecat/core/support/handler/CoreExceptionHandler.java
Change the @ExceptionHandler(Exception.class) method (around lines 47–52):
@ExceptionHandler(Exception.class)
public ResponseEntity<ApiResponse<?>> handleException(Exception exception) {
log.error("Exception occurred: {}", exception.getMessage(), exception);
- return ResponseEntity.status(ErrorType.DEFAULT_ERROR.getStatus())
- .body(ApiResponse.error(ErrorType.DEFAULT_ERROR));
+ return ResponseEntity.status(ErrorType.INTERNAL_SERVER_ERROR.getStatus())
+ .body(ApiResponse.error(ErrorType.INTERNAL_SERVER_ERROR));
}This ensures unhandled exceptions return HTTP 500 with your new ErrorCode.INTERNAL_SERVER_ERROR.
📝 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.
| MISSING_PARAMETER("필수 파라미터 %s(이)가 누락되었습니다."), | |
| INTERNAL_SERVER_ERROR("서버에서 오류가 발생했습니다."); | |
| @ExceptionHandler(Exception.class) | |
| public ResponseEntity<ApiResponse<?>> handleException(Exception exception) { | |
| log.error("Exception occurred: {}", exception.getMessage(), exception); | |
| return ResponseEntity.status(ErrorType.INTERNAL_SERVER_ERROR.getStatus()) | |
| .body(ApiResponse.error(ErrorType.INTERNAL_SERVER_ERROR)); | |
| } |
🤖 Prompt for AI Agents
In
capturecat-core/src/main/java/com/capturecat/core/support/handler/CoreExceptionHandler.java
around lines 47–52, the global catch-all @ExceptionHandler(Exception.class)
currently maps unhandled exceptions to ErrorType.DEFAULT_ERROR; change it to use
the new INTERNAL_SERVER_ERROR mapping from ErrorCode (or
ErrorType.INTERNAL_SERVER_ERROR as appropriate), ensure the returned error
payload uses ErrorCode.INTERNAL_SERVER_ERROR and the response status is HTTP
500, and adjust any logging/stack capture accordingly so unhandled exceptions
are mapped to INTERNAL_SERVER_ERROR.
capturecat-core/src/test/java/com/capturecat/core/config/jwt/JwtFilterTest.java
Show resolved
Hide resolved
capturecat-core/src/test/java/com/capturecat/core/service/auth/TokenServiceTest.java
Show resolved
Hide resolved
capturecat-core/src/test/java/com/capturecat/core/service/auth/TokenServiceTest.java
Show resolved
Hide resolved
| data: | ||
| redis: | ||
| host: localhost | ||
| port: 6379 | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Redis test config may fail on CI without a running Redis
With spring.data.redis pointing to localhost:6379, tests that touch Redis will attempt a real connection. The deploy workflow currently runs tests but doesn’t start Redis.
- Start Redis via GitHub Actions services in the deploy job (see my workflow suggestion).
- Or switch tests to Testcontainers and wire host/port dynamically via DynamicPropertyRegistry.
Would you like me to add a minimal Testcontainers-based Redis configuration for tests?
🏁 Script executed:
#!/usr/bin/env bash
set -e
echo "=== Workflow YAML files ==="
find .github/workflows -type f -name "*.yml" || echo "No .github/workflows directory found"
echo -e "\n=== References to services or redis in workflows ==="
grep -R -nE "services:|redis" .github/workflows || echo "No references to services or redis in workflows"
echo -e "\n=== Test files under src ==="
find src -type f -name "*Test.java" || echo "No Java test files found"
echo -e "\n=== @SpringBootTest usage in tests ==="
grep -R -n "@SpringBootTest" src/test || echo "No @SpringBootTest annotations found in tests"Length of output: 996
🏁 Script executed:
#!/usr/bin/env bash
set -e
echo "=== List all Java test files in capturecat-core ==="
find capturecat-core/src/test/java -type f || echo "No tests found"
echo -e "\n=== @SpringBootTest annotations in capturecat-core tests ==="
grep -R -n "@SpringBootTest" capturecat-core/src/test/java || echo "No @SpringBootTest annotations found"
echo -e "\n=== Redis references in capturecat-core test code ==="
grep -R -n -i "redis" capturecat-core/src/test/java || echo "No Redis references found in test code"
echo -e "\n=== Contents of test application.yml ==="
sed -n '1,200p' capturecat-core/src/test/resources/application.yml || echo "Test application.yml not found"Length of output: 11394
🏁 Script executed:
#!/usr/bin/env bash
set -e
echo "=== Display CaptureCatApplicationTest.java ==="
sed -n '1,200p' capturecat-core/src/test/java/com/capturecat/CaptureCatApplicationTest.javaLength of output: 412
Tests Depend on Redis but CI Doesn’t Start It
The application.yml under capturecat-core/src/test/resources configures Spring Data Redis to point at localhost:6379, and you have at least two tests (CaptureCatApplicationTest and TokenApiTest) annotated with @SpringBootTest that will load the full context—including a real LettuceConnectionFactory—causing connection failures when Redis isn’t available in CI. Your GitHub workflows (.github/workflows/ci.yml, deploy.yml, pr-lint.yml, deploy-dev.yml) do not currently declare a Redis service, so these tests will error out on CI.
• In .github/workflows/ci.yml (and any other workflow that runs tests), add under the job definition:
services:
redis:
image: redis:7-alpine
ports:
- 6379:6379• Alternatively, replace the fixed host/port in your test application.yml with a Testcontainers-backed configuration. For example, create a RedisContainer in a @TestConfiguration or use JUnit’s DynamicPropertySource to override:
static final RedisContainer redis = new RedisContainer("redis:7-alpine");
@DynamicPropertySource
static void redisProperties(DynamicPropertyRegistry registry) {
redis.start();
registry.add("spring.data.redis.host", redis::getHost);
registry.add("spring.data.redis.port", () -> redis.getFirstMappedPort());
}Would you like me to add a minimal Testcontainers-based Redis setup to the codebase?
🤖 Prompt for AI Agents
capturecat-core/src/test/resources/application.yml lines 33-37: tests point
Redis to localhost:6379 which fails in CI because workflows don't start Redis;
either add a Redis service to all test-running workflows (e.g.,
.github/workflows/ci.yml, pr-lint.yml, deploy.yml, deploy-dev.yml) by adding a
redis service using image redis:7-alpine and mapping port 6379:6379, or change
tests to use Testcontainers by removing the fixed host/port from test
application.yml and adding a Testcontainers RedisContainer started in test setup
(via a @TestConfiguration or a static container with @DynamicPropertySource)
that registers spring.data.redis.host and spring.data.redis.port dynamically;
implement one of these fixes consistently across CI and test configuration.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
capturecat-core/src/main/java/com/capturecat/core/service/auth/TokenService.java (2)
70-76: Avoid logging raw refresh tokensTokens are credentials. Logging them risks leakage via logs, APMs, and dumps. Log a fingerprint or omit entirely.
Apply:
- String refreshToken = authHeader.substring(JwtUtil.BEARER_PREFIX.length()).trim(); - log.info("Refresh Token: {}", refreshToken); + String refreshToken = authHeader.substring(JwtUtil.BEARER_PREFIX.length()).trim(); + log.debug("Refresh token received (len={})", refreshToken.length());
77-91: Broaden exception handling to JwtException to cover all parser failuresUnsupportedJwtException, WeakKeyException, etc., can escape your current catches and be misreported upstream.
Apply:
+// add import at top of file +import io.jsonwebtoken.JwtException; @@ - } catch (ExpiredJwtException e) { + } catch (ExpiredJwtException e) { throw new CoreException(ErrorType.REFRESH_TOKEN_EXPIRED); - } catch (SignatureException | MalformedJwtException | IllegalArgumentException e) { + } catch (JwtException | IllegalArgumentException e) { throw new CoreException(ErrorType.INVALID_REFRESH_TOKEN); }
♻️ Duplicate comments (6)
capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtUtil.java (2)
71-81: Catch umbrella JwtException in isValid to prevent 500s from uncaught parser errorsparseSignedClaims can throw multiple JwtException subtypes beyond the three listed. Letting any slip will bubble up as 500 during auth. Catch the base JwtException (plus IllegalArgumentException).
Apply:
+// add import +import io.jsonwebtoken.JwtException; @@ public boolean isValid(String token) { try { extractClaims(token); // parseSignedClaims 호출 시 자동 만료 검증 (예외 던짐) - } catch (ExpiredJwtException | SignatureException | MalformedJwtException | IllegalArgumentException e) { + } catch (JwtException | IllegalArgumentException e) { return false; } return true; }
83-89: resolveToken: accept case-insensitive “Bearer”, and reject empty tokens earlyCurrent check is case-sensitive and will accept “Bearer ” (empty token), which later throws deeper. Normalize prefix and guard empty after trim.
Apply:
+// add import at top of file +import org.springframework.util.StringUtils; @@ public String resolveToken(String authHeader) { - if (authHeader == null || !authHeader.startsWith(JwtUtil.BEARER_PREFIX)) { + if (authHeader == null) { + throw new CoreException(ErrorType.INVALID_ACCESS_TOKEN); + } + // Case-insensitive "Bearer " + String prefix = authHeader.length() >= BEARER_PREFIX.length() + ? authHeader.substring(0, BEARER_PREFIX.length()) + : authHeader; + if (!BEARER_PREFIX.equalsIgnoreCase(prefix)) { throw new CoreException(ErrorType.INVALID_ACCESS_TOKEN); } - return authHeader.substring(BEARER_PREFIX.length()).trim(); + String token = authHeader.substring(BEARER_PREFIX.length()).trim(); + if (!StringUtils.hasText(token)) { + throw new CoreException(ErrorType.INVALID_ACCESS_TOKEN); + } + return token; }capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLogoutFilter.java (2)
35-46: Bug: shouldNotFilter should use getServletPath() (context-path safe)With a context path, getRequestURI() won’t equal “/logout” and the filter will be skipped incorrectly.
Apply:
protected boolean shouldNotFilter(HttpServletRequest request) { @@ - // 3) /logout 경로만 통과 (컨텍스트 경로 영향 없는 servletPath 사용) - return !LOGOUT_PATH.equals(request.getRequestURI()); + // 3) /logout 경로만 통과 (컨텍스트 경로 영향 없는 servletPath 사용) + return !LOGOUT_PATH.equals(request.getServletPath()); }
52-56: Validate Bearer prefix of both headers before delegating to TokenServiceFail fast with a clear logout-specific error instead of letting deeper layers convert it to INTERNAL_SERVER_ERROR.
Apply:
String accessHeader = request.getHeader(HttpHeaders.AUTHORIZATION); String refreshHeader = request.getHeader(JwtUtil.REFRESH_TOKEN_HEADER); - if (!StringUtils.hasText(accessHeader) || !StringUtils.hasText(refreshHeader)) { + if (!StringUtils.hasText(accessHeader) || !StringUtils.hasText(refreshHeader) + || !accessHeader.regionMatches(true, 0, JwtUtil.BEARER_PREFIX, 0, JwtUtil.BEARER_PREFIX.length()) + || !refreshHeader.regionMatches(true, 0, JwtUtil.BEARER_PREFIX, 0, JwtUtil.BEARER_PREFIX.length())) { throw new CoreException(ErrorType.INVALID_LOGOUT_AUTH_TOKEN); }capturecat-core/src/main/java/com/capturecat/core/service/auth/TokenService.java (2)
144-153: Harden blacklistAccessToken: guard header, validate token, and avoid logging raw tokensDefensive checks reduce surprising 500s; avoid logging the token value.
Apply:
public void blacklistAccessToken(String authHeader) { - String accessToken = jwtUtil.resolveToken(authHeader); - long remainMillis = jwtUtil.getExpiration(accessToken) - System.currentTimeMillis(); + if (authHeader == null || !authHeader.regionMatches(true, 0, JwtUtil.BEARER_PREFIX, 0, JwtUtil.BEARER_PREFIX.length())) { + throw new CoreException(ErrorType.INVALID_ACCESS_TOKEN); + } + String accessToken = jwtUtil.resolveToken(authHeader); + if (!jwtUtil.isValid(accessToken)) { + throw new CoreException(ErrorType.INVALID_ACCESS_TOKEN); + } + long remainMillis = jwtUtil.getExpiration(accessToken) - System.currentTimeMillis(); if (remainMillis > 0) { redisTemplate.opsForValue() .set(blacklistKey(accessToken), "blacklisted", remainMillis, TimeUnit.MILLISECONDS); } - log.info("Blacklist Token: {}", accessToken); + log.debug("Access token blacklisted (ttlMs={})", remainMillis); }
159-161: Do not use raw tokens as Redis keys; use a deterministic hash/fingerprintRaw keys leak credentials into infra. Store hash(f(token)) instead.
Apply:
- private String blacklistKey(String token) { - return BLACKLIST_PREFIX + token; - } + private String blacklistKey(String token) { + return BLACKLIST_PREFIX + sha256(token); + }Add this helper inside the class (and imports) to support the change:
// imports to add at top of file: // import java.nio.charset.StandardCharsets; // import java.security.MessageDigest; // import java.security.NoSuchAlgorithmException; // import java.util.HexFormat; private String sha256(String input) { try { byte[] digest = MessageDigest.getInstance("SHA-256") .digest(input.getBytes(StandardCharsets.UTF_8)); return HexFormat.of().formatHex(digest); } catch (NoSuchAlgorithmException e) { // Fallback: non-crypto hash to avoid failing the path return Integer.toHexString(input.hashCode()); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLogoutFilter.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtUtil.java(2 hunks)capturecat-core/src/main/java/com/capturecat/core/service/auth/TokenService.java(5 hunks)capturecat-core/src/main/java/com/capturecat/core/support/error/ErrorCode.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/support/error/ErrorType.java(1 hunks)capturecat-core/src/test/java/com/capturecat/core/api/error/ErrorCodeControllerTest.java(2 hunks)capturecat-core/src/test/java/com/capturecat/core/api/user/UserControllerTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- capturecat-core/src/main/java/com/capturecat/core/support/error/ErrorType.java
- capturecat-core/src/test/java/com/capturecat/core/api/user/UserControllerTest.java
- capturecat-core/src/main/java/com/capturecat/core/support/error/ErrorCode.java
🧰 Additional context used
🧬 Code graph analysis (1)
capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLogoutFilter.java (4)
capturecat-core/src/main/java/com/capturecat/core/service/auth/TokenService.java (1)
Slf4j(24-162)capturecat-core/src/main/java/com/capturecat/core/config/SecurityConfig.java (1)
Slf4j(28-75)capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtFilter.java (1)
Slf4j(28-80)capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLoginFilter.java (1)
Slf4j(36-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy-dev
🔇 Additional comments (2)
capturecat-core/src/test/java/com/capturecat/core/api/error/ErrorCodeControllerTest.java (2)
85-86: Verification Complete — Logout Error Flow and Filter Registration Are CorrectAll requested checks have passed:
- ErrorType mappings:
INVALID_LOGOUT_AUTH_TOKEN→HttpStatus.UNAUTHORIZED(401)INTERNAL_SERVER_ERROR→HttpStatus.INTERNAL_SERVER_ERROR(500)JwtLogoutFilterextendsOncePerRequestFilter; noGenericFilterBean‐based logout filter remains.- Security filter chain registers
JwtLogoutFilterexactly once.No further action required.
145-145: Verify withdraw error‐code docs expanded; confirm transactional isolation and exception‐to‐error mappingThe new INVALID_ACCESS_TOKEN (401), INVALID_REFRESH_TOKEN (401), UNLINK_SOCIAL_FAIL (400) and INTERNAL_SERVER_ERROR (500) codes are defined in ErrorType and referenced in the withdraw error‐code documentation. Before approving, please double‐check:
- Exception mapping
• At the withdraw endpoint, ensure your global exception handler (e.g. @RestControllerAdvice) converts CoreException(ErrorType.INVALID_ACCESS_TOKEN/INVALID_REFRESH_TOKEN) into a 401 response, and maps UNLINK_SOCIAL_FAIL and INTERNAL_SERVER_ERROR as documented.- Transactional semantics for “deletion reason”
• Confirm that saving the withdrawal reason uses Propagation.REQUIRES_NEW (or another isolation strategy) so a failure in the member‐deletion step (default REQUIRED) doesn’t roll back the reason record. No custom propagation was detected around UserService.withdraw’s “reason” persistence—verify intended behavior.- Domain/service exception coverage
• Verify that all exceptions thrown by unlinkSocials or user deletion logic (e.g. CoreException(ErrorType.USER_NOT_FOUND)) bubble up as the documented error codes in the withdraw API docs.
capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLogoutFilter.java
Show resolved
Hide resolved
capturecat-core/src/main/java/com/capturecat/core/service/auth/TokenService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLogoutFilter.java (3)
45-46: Fix path check: use getServletPath() so the filter works behind a context pathCurrent check uses getRequestURI() while the comment says “servletPath”. With a context path (e.g., "/api"), POST /api/logout will be skipped.
Apply:
- // 3) /logout 경로만 통과 (컨텍스트 경로 영향 없는 servletPath 사용) - return !LOGOUT_PATH.equals(request.getRequestURI()); + // 3) /logout 경로만 통과 (컨텍스트 경로 영향 없는 servletPath 사용) + return !LOGOUT_PATH.equals(request.getServletPath());
54-56: Validate Bearer prefix for both headers to avoid 500s on client errorsIf a client sends headers without the "Bearer " prefix, TokenService may throw, bubbling up as INTERNAL_SERVER_ERROR and returning 500. Fail fast here with a 401.
- if (!StringUtils.hasText(accessHeader) || !StringUtils.hasText(refreshHeader)) { + if (!StringUtils.hasText(accessHeader) || !StringUtils.hasText(refreshHeader) + || !accessHeader.startsWith(JwtUtil.BEARER_PREFIX) + || !refreshHeader.startsWith(JwtUtil.BEARER_PREFIX)) { throw new CoreException(ErrorType.INVALID_LOGOUT_AUTH_TOKEN); }
64-73: Don’t downgrade server errors to 401; map INTERNAL_SERVER_ERROR to 500CoreException with INTERNAL_SERVER_ERROR should return 500, not 401. Content type is already set correctly.
- } catch (CoreException e) { - response.setStatus(HttpStatus.UNAUTHORIZED.value()); - response.setContentType(MediaType.APPLICATION_JSON_VALUE); - objectMapper.writeValue(response.getWriter(), ApiResponse.error(e.getErrorType())); + } catch (CoreException e) { + HttpStatus status = (e.getErrorType() == ErrorType.INTERNAL_SERVER_ERROR) + ? HttpStatus.INTERNAL_SERVER_ERROR + : HttpStatus.UNAUTHORIZED; + response.setStatus(status.value()); + response.setContentType(MediaType.APPLICATION_JSON_VALUE); + objectMapper.writeValue(response.getWriter(), ApiResponse.error(e.getErrorType())); } catch (Exception e) { response.setStatus(HttpStatus.INTERNAL_SERVER_ERROR.value()); response.setContentType(MediaType.APPLICATION_JSON_VALUE); - objectMapper.writeValue(response.getWriter(), - ApiResponse.error(ErrorType.INTERNAL_SERVER_ERROR)); + objectMapper.writeValue(response.getWriter(), ApiResponse.error(ErrorType.INTERNAL_SERVER_ERROR)); }
🧹 Nitpick comments (2)
capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLogoutFilter.java (2)
35-43: Prefer OncePerRequestFilter dispatch overrides over manual DispatcherType checksUse shouldNotFilterAsyncDispatch/shouldNotFilterErrorDispatch for clarity and to align with Spring’s lifecycle instead of manually checking DispatcherType.
Minimal change inside this block (remove the manual dispatcher check):
- // 1) ERROR/ASYNC 등 요청 아닌 디스패치 스킵 - if (request.getDispatcherType() != DispatcherType.REQUEST) { - return true; - }Add these methods elsewhere in the class:
@Override protected boolean shouldNotFilterAsyncDispatch() { return true; } @Override protected boolean shouldNotFilterErrorDispatch() { return true; }If you adopt this, also remove the now-unused import:
-import jakarta.servlet.DispatcherType;
26-33: Add focused tests for the new logout filter behaviorConsider adding MVC/slice tests to lock behavior:
- POST /logout success path (valid Bearer headers) → 200 and tokenService.revokeUserTokens called.
- Missing headers or wrong prefixes → 401 INVALID_LOGOUT_AUTH_TOKEN.
- TokenService throws INTERNAL_SERVER_ERROR → 500 with error body.
- Non-POST or different path → filter not applied (no interaction with tokenService).
- Context path scenario (e.g., server.servlet.context-path=/api): POST /api/logout still handled.
I can scaffold these with WebMvcTest if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLogoutFilter.java(1 hunks)capturecat-core/src/main/resources/application.yml(0 hunks)
💤 Files with no reviewable changes (1)
- capturecat-core/src/main/resources/application.yml
🧰 Additional context used
🧬 Code graph analysis (1)
capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLogoutFilter.java (4)
capturecat-core/src/main/java/com/capturecat/core/service/auth/TokenService.java (1)
Slf4j(24-162)capturecat-core/src/main/java/com/capturecat/core/config/SecurityConfig.java (1)
Slf4j(28-75)capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtFilter.java (1)
Slf4j(28-80)capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLoginFilter.java (1)
Slf4j(36-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy-dev
🔇 Additional comments (1)
capturecat-core/src/main/java/com/capturecat/core/config/jwt/JwtLogoutFilter.java (1)
61-63: Success response handling looks good and consistent with the rest of the filters200 OK with JSON body via ApiResponse.success(), and Content-Type is set. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
capturecat-core/compose.yml (1)
1-27: Action required: Address critical Docker Compose security and configuration issuesThe sanity-check script confirms the following problems in
capturecat-core/compose.ymlthat must be fixed before merging:
Redis healthcheck does not authenticate
• File: capturecat-core/compose.yml, line 24
• Current:test: [ "CMD", "redis-cli", "ping" ]
• Fix: include-a "$${REDIS_PASSWORD}"(or appropriate env var) in theredis-clicommand so the healthcheck uses the configured password.Unpinned images for Postgres and Redis
• Lines 2 (image: postgres) and 19 (image: redis)
• Fix: specify explicit versions/tags (e.g.postgres:15-alpine,redis:7.2-alpine) to ensure reproducible builds.Stray MySQL environment variables under the Postgres service
• Lines 8–11 (MYSQL_ROOT_PASSWORDandMYSQL_DATABASE)
• Fix: remove these MySQL-specific keys from the Postgres service block.Hard-coded host-path volume for Postgres data
• Line 6 (- /home/ubuntu/capturecat/...:/var/lib/postgresql/data)
• Fix: convert to a named Docker volume to decouple data storage from the host filesystem (e.g.- postgres_data:/var/lib/postgresql/data).Committed sample credentials in plaintext
• Multiple occurrences ofcapturecat77in environment blocks
• Fix: move secrets into a.envfile or a secrets manager, and reference them viaenv_fileor Docker secrets.Please update
capturecat-core/compose.ymlaccordingly to resolve these issues.
🧹 Nitpick comments (6)
capturecat-core/compose.yml (6)
11-11: Hard-coded host path makes the stack non-portable; switch to a named volume.The absolute path ties the compose file to a specific machine. Use a named volume for portability and to work on CI/CD.
Apply this diff:
- - /home/ubuntu/capturecat/database/docker-postgresql/postgresql_data:/var/lib/postgresql/data + - dev-postgres-data:/var/lib/postgresql/dataAdd this at the bottom of the file (top-level) to declare the volume:
volumes: dev-postgres-data:
3-3: Pin images to explicit, supported tags for reproducibility.Floating tags can break local dev unexpectedly when upstream images change.
Apply this diff:
- image: postgres + image: postgres:15-alpine- image: redis + image: redis:7.2-alpineAlso applies to: 19-19
8-9: Remove MySQL variables from Postgres service.These env keys don’t apply to the Postgres image and are noise.
Apply this diff:
- MYSQL_ROOT_PASSWORD: - MYSQL_DATABASE:
2-3: Optional: add a restart policy for smoother local dev.Helps containers recover from transient failures and healthcheck flaps.
Apply this diff:
dev-capturecat-db: - image: postgres + image: postgres:15-alpine + restart: unless-stoppeddev-capturecat-cache-server: - image: redis + image: redis:7.2-alpine + restart: unless-stoppedAlso applies to: 18-19
15-17: Nit: pg_isready vars are correctly escaped; consider adding-qto reduce noise.
$${VAR}ensures expansion inside the container, which is fine.-qexits with proper status without printing.Apply this diff:
- test: ["CMD-SHELL", "pg_isready -U $${POSTGRES_USER} -d $${POSTGRES_DB} -h 127.0.0.1 -p 5432"] + test: ["CMD-SHELL", "pg_isready -q -U $${POSTGRES_USER} -d $${POSTGRES_DB} -h 127.0.0.1 -p 5432"]
1-27: Security hygiene: don’t commit real credentials; prefer .env and examples.Even for dev, committing
capturecat77is not ideal. Use .env (gitignored) for real values and keep acompose.env.examplewith safe placeholders to onboard contributors.I can add
.env, update compose to reference${POSTGRES_PASSWORD}and${REDIS_PASSWORD}, and providecompose.env.example. Want me to push that as a follow-up PR?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
capturecat-core/compose.yml(1 hunks)capturecat-core/src/main/java/com/capturecat/core/service/user/UserService.java(0 hunks)
💤 Files with no reviewable changes (1)
- capturecat-core/src/main/java/com/capturecat/core/service/user/UserService.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: deploy-dev
| command: ["redis-server", "--requirepass", "capturecat77"] | ||
| ports: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding the Redis password in command; pass via env and expand at runtime.
Keeps the password out of the command line and fixes the healthcheck change above (uses $REDIS_PASSWORD).
Apply this diff:
- command: ["redis-server", "--requirepass", "capturecat77"]
+ environment:
+ REDIS_PASSWORD: capturecat77
+ command: ["sh", "-c", "redis-server --requirepass \"$REDIS_PASSWORD\""]Follow-up (recommended): move the secret to an .env file and reference it, so it isn’t committed:
- .env (gitignored):
REDIS_PASSWORD=your-local-only-secret - compose:
environment: { REDIS_PASSWORD: ${REDIS_PASSWORD} }command: ["sh","-c","redis-server --requirepass \"$REDIS_PASSWORD\""]
📝 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.
| command: ["redis-server", "--requirepass", "capturecat77"] | |
| ports: | |
| environment: | |
| REDIS_PASSWORD: capturecat77 | |
| command: ["sh", "-c", "redis-server --requirepass \"$REDIS_PASSWORD\""] | |
| ports: |
🤖 Prompt for AI Agents
In capturecat-core/compose.yml around lines 20-21 the Redis password is
hardcoded in the command, which exposes secrets and breaks the healthcheck
expecting $REDIS_PASSWORD; change the service to accept REDIS_PASSWORD from
environment and run the server via a shell expansion. Add an environment entry
supplying REDIS_PASSWORD (sourced from compose variable or .env), and replace
the command with a shell-invoked form that expands the env var at runtime (e.g.,
sh -c 'redis-server --requirepass "$REDIS_PASSWORD"'). Optionally move the
secret into a gitignored .env file and reference it via compose variable
substitution.
| healthcheck: | ||
| test: [ "CMD", "redis-cli", "ping" ] | ||
| interval: 5s | ||
| retries: 10 |
There was a problem hiding this comment.
Redis healthcheck will always fail due to missing auth; authenticate in the check.
You’ve enabled requirepass on Redis, but the healthcheck runs redis-cli ping without -a <password>. That returns NOAUTH and marks the container unhealthy.
Apply this diff to authenticate in the healthcheck (and avoid noisy auth warnings):
- healthcheck:
- test: [ "CMD", "redis-cli", "ping" ]
- interval: 5s
- retries: 10
+ healthcheck:
+ test: [ "CMD-SHELL", "redis-cli -a \"$REDIS_PASSWORD\" --no-auth-warning ping" ]
+ interval: 5s
+ retries: 10📝 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.
| healthcheck: | |
| test: [ "CMD", "redis-cli", "ping" ] | |
| interval: 5s | |
| retries: 10 | |
| healthcheck: | |
| test: [ "CMD-SHELL", "redis-cli -a \"$REDIS_PASSWORD\" --no-auth-warning ping" ] | |
| interval: 5s | |
| retries: 10 |
🤖 Prompt for AI Agents
In capturecat-core/compose.yml around lines 23-26 the healthcheck runs redis-cli
ping without authentication so it will return NOAUTH; change the healthcheck to
run a shell command that authenticates using an environment variable (e.g.
REDIS_PASSWORD) such as using CMD-SHELL to run redis-cli --no-auth-warning -a
"$REDIS_PASSWORD" ping and verify the output is PONG (exit 0) so the container
is marked healthy, and ensure REDIS_PASSWORD is defined in the service env or
secrets.
📌 관련 이슈 (필수)
📝 작업 내용 (필수)
💬 리뷰 참고 사항 (선택)
Summary by CodeRabbit
New Features
Documentation
Chores