fix: routes available without authentication#134
Open
driedpampas wants to merge 10 commits intomainfrom
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughCentralizes JWT extraction/validation in JwtAuthFilter, adds WebSocket/STOMP JWT guards (JwtHandshakeInterceptor, StompJwtAuthInterceptor), wires them into WebSocketConfig, permits unauthenticated access to Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WS as "WebSocket Endpoint"
participant Handshake as "JwtHandshakeInterceptor"
participant Channel as "StompJwtAuthInterceptor"
participant Auth as "JwtAuthFilter"
Client->>WS: GET /ws?token=...
WS->>Handshake: beforeHandshake(request)
Handshake->>Auth: authenticateToken(token)
Auth-->>Handshake: Authentication or null
alt token valid
Handshake->>WS: store wsJwtToken, allow handshake
else token invalid
Handshake->>WS: set 401, reject handshake
end
Client->>WS: STOMP CONNECT (headers / session)
WS->>Channel: preSend(CONNECT)
alt header token present
Channel->>Auth: authenticateToken(headerToken)
else session token present
Channel->>Auth: authenticateToken(sessionToken)
end
Auth-->>Channel: Authentication or null
alt auth valid
Channel->>WS: attach User to message
else auth invalid
Channel-->>Client: throw BadCredentialsException / deny
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/p2ps/auth/security/SecurityConfig.java`:
- Around line 60-61: The STOMP/WebSocket layer lacks authentication so any
client can connect and subscribe; implement JWT validation at the STOMP
handshake to prevent unauthorized subscriptions. Modify the WebSocket handshake
or add a ChannelInterceptor: extract the JWT from the WebSocket query param
(e.g., /ws?token=...) or from CONNECT frame headers, validate it using the same
logic as JwtAuthFilter (reuse its token validation methods), populate the
SecurityContext/Principal on successful validation, and reject the CONNECT (or
close the channel) on failure; update or replace RoomSubscriptionInterceptor to
trust the authenticated Principal rather than just validating listId format.
Ensure you reference RoomSubscriptionInterceptor, JwtAuthFilter, and a new
ChannelInterceptor (or handshake extractor) when making the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa548f40-c276-4d1d-8c94-3c143b113f1e
📒 Files selected for processing (1)
src/main/java/com/p2ps/auth/security/SecurityConfig.java
…tompJwtAuthInterceptor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/main/java/com/p2ps/auth/security/JwtAuthFilter.java (2)
72-77: Don't clear a pre-populated context here.This is mostly a no-op while the
SecurityContextis empty, but a valid JWT still falls into this branch whenever something earlier has already authenticated the request. In that case the filter de-authenticates the request instead of leaving the current principal alone.Suggested fix
try { UsernamePasswordAuthenticationToken authToken = authenticateToken(token); if (authToken != null && SecurityContextHolder.getContext().getAuthentication() == null) { authToken.setDetails(new WebAuthenticationDetailsSource().buildDetails(request)); SecurityContextHolder.getContext().setAuthentication(authToken); - } else if (token != null) { - SecurityContextHolder.clearContext(); } } catch (Exception ex) { SecurityContextHolder.clearContext(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/p2ps/auth/security/JwtAuthFilter.java` around lines 72 - 77, The JwtAuthFilter branch that calls SecurityContextHolder.clearContext() when token != null can de-authenticate a request if some earlier filter already populated the SecurityContext; remove the else-if clearing logic so the filter only sets the auth when authToken != null and no existing authentication (i.e., keep the authToken.setDetails(...) and setAuthentication(...) path and delete the else if (token != null) { SecurityContextHolder.clearContext(); } branch) to avoid clearing a pre-populated context.
26-29: Use servlet-relative matching for/wsto avoid overly-broad path matching and ensure forward compatibility.The current
startsWith("/ws")check will unintentionally match paths like/wspor/wsadmin. Additionally,getRequestURI()includes the context path, which causes misalignment once the app is deployed under a prefix (e.g.,/api). UsegetServletPath()with explicit matching to align with the registered/wsendpoint and SecurityConfig's/ws/**permit rule.Suggested fix
`@Override` protected boolean shouldNotFilter(HttpServletRequest request) { - String path = request.getRequestURI(); - return path.startsWith("/ws"); + String path = request.getServletPath(); + return "/ws".equals(path) || path.startsWith("/ws/"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/p2ps/auth/security/JwtAuthFilter.java` around lines 26 - 29, The shouldNotFilter method in JwtAuthFilter uses request.getRequestURI() and startsWith("/ws"), which can over-match and ignores servlet context; change it to use request.getServletPath() and perform an exact or prefix match against "/ws" (e.g., equals("/ws") or startsWith("/ws/") depending on intent) so it aligns with the SecurityConfig’s "/ws/**" rule and avoids matching "/wsp" or "/wsadmin". Locate JwtAuthFilter.shouldNotFilter and replace the getRequestURI()/startsWith logic with servlet-path-based matching using getServletPath().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/p2ps/config/JwtHandshakeInterceptor.java`:
- Around line 32-47: The handshake currently extracts a JWT from the URL query
and stores it (JwtHandshakeInterceptor -> SESSION_TOKEN_ATTRIBUTE), which risks
leaking credentials; change this so URL-token support is only enabled when an
explicit feature flag/config (e.g., a new boolean allowUrlToken or
websocket.compatibility.enableUrlToken) is true, otherwise skip reading/setting
the token and rely on StompJwtAuthInterceptor for CONNECT headers; if you must
keep URL-token support, ensure any logging/redaction around
jwtAuthFilter.authenticateToken and request URIs removes or masks the token
value before it reaches access/ingress logs and do not persist the raw token in
attributes unless the flag is enabled.
In `@src/main/java/com/p2ps/config/StompJwtAuthInterceptor.java`:
- Around line 38-46: The interceptor currently mutates the thread-local
SecurityContextHolder by calling
SecurityContextHolder.getContext().setAuthentication(authentication) which can
leak authentication across reused channel threads; remove that mutation and rely
solely on propagating the identity via the STOMP USER_HEADER when building the
message (i.e., keep the
MessageBuilder.fromMessage(message).setHeader(SimpMessageHeaderAccessor.USER_HEADER,
authentication).build()) so no
SecurityContextHolder.clearContext()/setAuthentication(...) calls remain in
StompJwtAuthInterceptor and authentication is delivered only through the
USER_HEADER.
- Line 31: The overridden preSend method in StompJwtAuthInterceptor currently
lacks a `@Nullable` annotation on its return type; update the method signature for
Message<?> preSend(Message<?> message, MessageChannel channel) to annotate the
return as `@Nullable` (use org.jspecify.annotations.Nullable) to match
ChannelInterceptor#preSend in Spring 7.0.6, and add the import for
org.jspecify.annotations.Nullable if missing; keep the method body unchanged.
---
Nitpick comments:
In `@src/main/java/com/p2ps/auth/security/JwtAuthFilter.java`:
- Around line 72-77: The JwtAuthFilter branch that calls
SecurityContextHolder.clearContext() when token != null can de-authenticate a
request if some earlier filter already populated the SecurityContext; remove the
else-if clearing logic so the filter only sets the auth when authToken != null
and no existing authentication (i.e., keep the authToken.setDetails(...) and
setAuthentication(...) path and delete the else if (token != null) {
SecurityContextHolder.clearContext(); } branch) to avoid clearing a
pre-populated context.
- Around line 26-29: The shouldNotFilter method in JwtAuthFilter uses
request.getRequestURI() and startsWith("/ws"), which can over-match and ignores
servlet context; change it to use request.getServletPath() and perform an exact
or prefix match against "/ws" (e.g., equals("/ws") or startsWith("/ws/")
depending on intent) so it aligns with the SecurityConfig’s "/ws/**" rule and
avoids matching "/wsp" or "/wsadmin". Locate JwtAuthFilter.shouldNotFilter and
replace the getRequestURI()/startsWith logic with servlet-path-based matching
using getServletPath().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 87543b2b-21ef-4dde-915b-5bf25a8f148d
📒 Files selected for processing (10)
build.gradle.ktssrc/main/java/com/p2ps/auth/security/JwtAuthFilter.javasrc/main/java/com/p2ps/config/JwtHandshakeInterceptor.javasrc/main/java/com/p2ps/config/RoomSubscriptionInterceptor.javasrc/main/java/com/p2ps/config/StompJwtAuthInterceptor.javasrc/main/java/com/p2ps/config/WebSocketConfig.javasrc/test/java/com/p2ps/config/JwtHandshakeInterceptorTest.javasrc/test/java/com/p2ps/config/RoomSubscriptionInterceptorTest.javasrc/test/java/com/p2ps/config/StompJwtAuthInterceptorTest.javasrc/test/java/com/p2ps/config/WebSocketConfigTest.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/p2ps/config/StompJwtAuthInterceptor.java (1)
39-48:⚠️ Potential issue | 🟠 MajorAvoid
SecurityContextHoldermutation in STOMP channel interceptors.Line 45 sets authentication in
SecurityContextHolder, which can leak identity across reused channel threads since cleanup isn't guaranteed in the STOMP message processing flow. TheUSER_HEADERpropagation at lines 46-48 is the correct mechanism for STOMP authentication.🔒 Suggested fix: remove SecurityContextHolder mutation
if (authentication == null) { - SecurityContextHolder.clearContext(); logger.warn("Rejecting websocket CONNECT without a valid JWT"); throw new AccessDeniedException("WebSocket JWT authentication failed"); } - SecurityContextHolder.getContext().setAuthentication(authentication); return org.springframework.messaging.support.MessageBuilder.fromMessage(message) .setHeader(SimpMessageHeaderAccessor.USER_HEADER, authentication) .build();Also remove the unused import at line 15:
-import org.springframework.security.core.context.SecurityContextHolder;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/p2ps/config/StompJwtAuthInterceptor.java` around lines 39 - 48, In StompJwtAuthInterceptor, remove mutations to SecurityContextHolder (both SecurityContextHolder.clearContext() and SecurityContextHolder.getContext().setAuthentication(authentication)) and instead only set the SimpMessageHeaderAccessor.USER_HEADER by returning the MessageBuilder with the authentication; this prevents identity leaks across reused channel threads. Also delete the unused import referenced at line 15; ensure the method (preSend or the interceptor method where authentication is handled) only validates the token, throws AccessDeniedException when authentication is null, and propagates the Authentication via the USER_HEADER without touching SecurityContextHolder.
🧹 Nitpick comments (1)
src/main/java/com/p2ps/auth/security/JwtAuthFilter.java (1)
26-29: Path matching may be overly permissive.
startsWith("/ws")matches unintended paths like/wsanything. Consider a more precise check.♻️ Suggested fix
protected boolean shouldNotFilter(HttpServletRequest request) { String path = request.getRequestURI(); - return path.startsWith("/ws"); + return path.equals("/ws") || path.startsWith("/ws/"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/p2ps/auth/security/JwtAuthFilter.java` around lines 26 - 29, The path check in shouldNotFilter(HttpServletRequest request) is too permissive because path.startsWith("/ws") will match "/wsanything"; change the logic to only exempt the websocket base and its subpaths by checking request.getRequestURI() matches "^/ws(/|$)" — e.g., replace the startsWith check with a precise match that allows exactly "/ws" or any path beginning with "/ws/". Ensure the change is made inside the shouldNotFilter method using request.getRequestURI() so no other behavior is affected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/p2ps/config/StompJwtAuthInterceptor.java`:
- Around line 84-95: Remove the duplicate private method extractBearerToken from
StompJwtAuthInterceptor and update resolveToken to call the already-public
JwtAuthFilter.extractBearerToken via the injected jwtAuthFilter field;
specifically delete the private extractBearerToken(String) method in
StompJwtAuthInterceptor and replace any internal calls with
jwtAuthFilter.extractBearerToken(authorizationHeader) (ensure the jwtAuthFilter
field is present and used).
---
Duplicate comments:
In `@src/main/java/com/p2ps/config/StompJwtAuthInterceptor.java`:
- Around line 39-48: In StompJwtAuthInterceptor, remove mutations to
SecurityContextHolder (both SecurityContextHolder.clearContext() and
SecurityContextHolder.getContext().setAuthentication(authentication)) and
instead only set the SimpMessageHeaderAccessor.USER_HEADER by returning the
MessageBuilder with the authentication; this prevents identity leaks across
reused channel threads. Also delete the unused import referenced at line 15;
ensure the method (preSend or the interceptor method where authentication is
handled) only validates the token, throws AccessDeniedException when
authentication is null, and propagates the Authentication via the USER_HEADER
without touching SecurityContextHolder.
---
Nitpick comments:
In `@src/main/java/com/p2ps/auth/security/JwtAuthFilter.java`:
- Around line 26-29: The path check in shouldNotFilter(HttpServletRequest
request) is too permissive because path.startsWith("/ws") will match
"/wsanything"; change the logic to only exempt the websocket base and its
subpaths by checking request.getRequestURI() matches "^/ws(/|$)" — e.g., replace
the startsWith check with a precise match that allows exactly "/ws" or any path
beginning with "/ws/". Ensure the change is made inside the shouldNotFilter
method using request.getRequestURI() so no other behavior is affected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db258f73-14dc-4102-8d99-07d667ad0148
📒 Files selected for processing (2)
src/main/java/com/p2ps/auth/security/JwtAuthFilter.javasrc/main/java/com/p2ps/config/StompJwtAuthInterceptor.java
…nd add tests for new behavior
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/p2ps/auth/security/JwtAuthFilter.java`:
- Around line 26-29: The shouldNotFilter override in JwtAuthFilter currently
bypasses JWT processing for any path under /ws/** which is too broad; update it
to only bypass the exact WebSocket handshake endpoints (e.g., the plain "/ws"
and the SockJS transport paths used by your WebSocketConfig) or remove the
override entirely so doFilterInternal handles absent Authorization headers,
ensuring SecurityConfig.requestMatchers("/ws/**").permitAll() does not
inadvertently skip authorization for non-handshake HTTP routes; locate the
shouldNotFilter method in JwtAuthFilter and change its logic to match only the
concrete handshake paths (or delete the method) and verify doFilterInternal and
WebSocketConfig's registered endpoints remain consistent.
In `@src/main/java/com/p2ps/config/StompJwtAuthInterceptor.java`:
- Around line 45-48: resolveAuthentication currently conflates "no token" and
"invalid token" by returning null for both; change the logic so you first call
resolveToken(accessor) and if it returns null return null (allow guest CONNECT),
but if a token is present call jwtAuthFilter.authenticateToken(token) and if
that returns null treat it as an invalid credential and reject the CONNECT
(e.g., throw an AuthenticationException/BadCredentialsException from
resolveAuthentication or from preSend) so bad tokens are not silently downgraded
to anonymous; update references in preSend to rely on this distinction.
- Around line 34-36: The current return rebuilds the Message and sets
SimpMessageHeaderAccessor.USER_HEADER but doesn't register the Principal with
the STOMP session; update StompJwtAuthInterceptor to use StompHeaderAccessor (or
obtain the accessor used earlier) and call accessor.setUser(authentication)
before calling MessageBuilder.fromMessage(message).setHeader(...).build() so the
authenticated Principal (authentication) is persisted to the session and later
accessor.getUser() calls (e.g., in RoomSubscriptionInterceptor) return the
registered user.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c2cfa41a-2728-4a86-897a-d5b92c72b918
📒 Files selected for processing (5)
src/main/java/com/p2ps/auth/security/JwtAuthFilter.javasrc/main/java/com/p2ps/config/JwtHandshakeInterceptor.javasrc/main/java/com/p2ps/config/StompJwtAuthInterceptor.javasrc/test/java/com/p2ps/config/JwtHandshakeInterceptorTest.javasrc/test/java/com/p2ps/config/StompJwtAuthInterceptorTest.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/com/p2ps/config/JwtHandshakeInterceptorTest.java
…nce StompJwtAuthInterceptor with improved authentication handling
…ption handling in StompJwtAuthInterceptorTest
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/p2ps/config/StompJwtAuthInterceptor.java (1)
86-97: 🛠️ Refactor suggestion | 🟠 MajorRemove duplicate
extractBearerTokenmethod; reuse fromJwtAuthFilter.This method duplicates
JwtAuthFilter.extractBearerToken()which is alreadypublicand accessible via the injectedjwtAuthFilterfield. Update line 74 to calljwtAuthFilter.extractBearerToken(headerToken)instead.♻️ Suggested fix: remove duplication
private String resolveToken(StompHeaderAccessor accessor) { String headerToken = accessor.getFirstNativeHeader("Authorization"); if (headerToken == null) { headerToken = accessor.getFirstNativeHeader("authorization"); } if (headerToken == null) { headerToken = accessor.getFirstNativeHeader("token"); } if (headerToken == null) { headerToken = accessor.getFirstNativeHeader("access_token"); } if (headerToken != null) { - return extractBearerToken(headerToken); + return jwtAuthFilter.extractBearerToken(headerToken); } Map<String, Object> sessionAttributes = accessor.getSessionAttributes(); if (sessionAttributes == null) { return null; } Object sessionToken = sessionAttributes.get(JwtHandshakeInterceptor.SESSION_TOKEN_ATTRIBUTE); return sessionToken instanceof String string ? string : null; } - - private String extractBearerToken(String authorizationHeader) { - if (authorizationHeader == null || authorizationHeader.isBlank()) { - return null; - } - - String token = authorizationHeader.trim(); - if (token.regionMatches(true, 0, "Bearer ", 0, 7)) { - return token.substring(7).trim(); - } - - return token; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/p2ps/config/StompJwtAuthInterceptor.java` around lines 86 - 97, The private extractBearerToken in StompJwtAuthInterceptor duplicates JwtAuthFilter.extractBearerToken; delete the private method and update callers in StompJwtAuthInterceptor (e.g., where headerToken is processed) to call jwtAuthFilter.extractBearerToken(headerToken) instead, ensuring the injected jwtAuthFilter field is used and removing any now-unused imports or warnings.
🧹 Nitpick comments (3)
src/main/java/com/p2ps/auth/security/JwtAuthFilter.java (1)
63-72: Outer try-catch is currently unreachable.Since
authenticateToken()internally catches all exceptions and returnsnull(lines 50-52), the outer catch block at lines 70-72 will never execute under normal operation. This is defensive but amounts to dead code.Consider removing the redundant try-catch, or if you want to keep it for future-proofing (e.g., if
authenticateTokencontract changes), add a comment explaining the intent.♻️ Optional simplification
- try { - UsernamePasswordAuthenticationToken authToken = authenticateToken(token); + UsernamePasswordAuthenticationToken authToken = authenticateToken(token); - if (authToken != null && SecurityContextHolder.getContext().getAuthentication() == null) { - authToken.setDetails(new WebAuthenticationDetailsSource().buildDetails(request)); - SecurityContextHolder.getContext().setAuthentication(authToken); - } - } catch (Exception _) { - SecurityContextHolder.clearContext(); + if (authToken != null && SecurityContextHolder.getContext().getAuthentication() == null) { + authToken.setDetails(new WebAuthenticationDetailsSource().buildDetails(request)); + SecurityContextHolder.getContext().setAuthentication(authToken); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/p2ps/auth/security/JwtAuthFilter.java` around lines 63 - 72, The outer try-catch around authenticateToken(token) in JwtAuthFilter is redundant because authenticateToken already swallows exceptions and returns null; remove the outer try-catch and directly call authenticateToken(token), performing the null check and setting SecurityContextHolder as currently done, OR if you intend to retain defensive safety, keep the try-catch but replace it with a clear comment explaining that the catch is deliberate to guard against future changes in authenticateToken's contract; reference authenticateToken, SecurityContextHolder.getContext(), and authToken.setDetails(...) when making the change.src/test/java/com/p2ps/config/StompJwtAuthInterceptorTest.java (1)
44-60: Unnecessary mock setup forauthenticateToken(null).The
when(jwtAuthFilter.authenticateToken(null)).thenReturn(null)at line 54 is never invoked becauseresolveAuthenticationreturnsnulldirectly whenresolveTokenreturnsnull(line 50-52 in implementation), without callingauthenticateToken.This isn't a bug but the mock setup is misleading. Consider removing it or adding a
verify(jwtAuthFilter, never()).authenticateToken(any())to explicitly document that authenticateToken is not called in this scenario.♻️ Suggested improvement
`@Test` void preSend_ConnectWithoutTokenPassesThrough() { JwtAuthFilter jwtAuthFilter = mock(JwtAuthFilter.class); StompJwtAuthInterceptor interceptor = new StompJwtAuthInterceptor(jwtAuthFilter); StompHeaderAccessor accessor = StompHeaderAccessor.create(StompCommand.CONNECT); accessor.setLeaveMutable(true); accessor.setSessionAttributes(new HashMap<>()); Message<?> message = MessageBuilder.createMessage(new byte[0], accessor.getMessageHeaders()); MessageChannel channel = mock(MessageChannel.class); - when(jwtAuthFilter.authenticateToken(null)).thenReturn(null); - Message<?> result = interceptor.preSend(message, channel); assertNotNull(result); assertNull(StompHeaderAccessor.wrap(result).getUser()); + verify(jwtAuthFilter, never()).authenticateToken(any()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/p2ps/config/StompJwtAuthInterceptorTest.java` around lines 44 - 60, The test preSend_ConnectWithoutTokenPassesThrough contains an unnecessary mock setup when(jwtAuthFilter.authenticateToken(null)).thenReturn(null) because StompJwtAuthInterceptor.resolveAuthentication returns early when resolveToken is null and never calls authenticateToken; remove that when(...) line OR replace it with an explicit verification like verify(jwtAuthFilter, never()).authenticateToken(any()) to document the expected behavior. Update the test in StompJwtAuthInterceptorTest to reference JwtAuthFilter.authenticateToken and StompJwtAuthInterceptor.resolveAuthentication/resolveToken accordingly so the test is clearer and not misleading.src/main/java/com/p2ps/config/StompJwtAuthInterceptor.java (1)
35-40:accessor.setUser()andUSER_HEADERsetting may be redundant.You call
accessor.setUser(authentication)at line 36, then also explicitly setSimpMessageHeaderAccessor.USER_HEADERin the rebuilt message at line 38. ThesetUser()call already stores the principal in the accessor's headers. The explicitUSER_HEADERsetting appears redundant sinceMessageBuilder.fromMessage(message)would carry the accessor's state.This is minor and works correctly, but could be simplified.
♻️ Optional simplification
if (authentication != null) { accessor.setUser(authentication); - return MessageBuilder.fromMessage(message) - .setHeader(SimpMessageHeaderAccessor.USER_HEADER, authentication) - .build(); + return MessageBuilder.createMessage(message.getPayload(), accessor.getMessageHeaders()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/p2ps/config/StompJwtAuthInterceptor.java` around lines 35 - 40, The accessor.setUser(authentication) and then also calling MessageBuilder.fromMessage(message).setHeader(SimpMessageHeaderAccessor.USER_HEADER, authentication) is redundant; update StompJwtAuthInterceptor to only call accessor.setUser(authentication) and return MessageBuilder.fromMessage(message).build() (or alternatively remove the accessor.setUser and keep the explicit header, but prefer keeping accessor.setUser), ensuring you still use the existing variables accessor, authentication and MessageBuilder.fromMessage(message) so the principal is propagated without duplicating the USER_HEADER population.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/java/com/p2ps/auth/security/SecurityConfigTest.java`:
- Around line 20-75: SecurityConfigTest is missing integration tests that assert
the core authorization rules (permitAll for "/" and "/ws/**" and auth required
for protected endpoints). Add MockMvc or WebTestClient integration tests (e.g.,
in SecurityConfigIntegrationTest) that boot the web layer with the
SecurityConfig (mock JwtAuthFilter or disable filter as needed) and assert: an
unauthenticated GET to "/" returns 200 OK; an unauthenticated GET to a
"/ws/some" path returns 200 OK; and an unauthenticated request to a protected
endpoint (e.g., "/api/protected") returns 401/403. Name tests clearly like
testPermitAllRootAndWs() and testProtectedRequiresAuth() and use assertions on
status codes to catch regressions.
---
Duplicate comments:
In `@src/main/java/com/p2ps/config/StompJwtAuthInterceptor.java`:
- Around line 86-97: The private extractBearerToken in StompJwtAuthInterceptor
duplicates JwtAuthFilter.extractBearerToken; delete the private method and
update callers in StompJwtAuthInterceptor (e.g., where headerToken is processed)
to call jwtAuthFilter.extractBearerToken(headerToken) instead, ensuring the
injected jwtAuthFilter field is used and removing any now-unused imports or
warnings.
---
Nitpick comments:
In `@src/main/java/com/p2ps/auth/security/JwtAuthFilter.java`:
- Around line 63-72: The outer try-catch around authenticateToken(token) in
JwtAuthFilter is redundant because authenticateToken already swallows exceptions
and returns null; remove the outer try-catch and directly call
authenticateToken(token), performing the null check and setting
SecurityContextHolder as currently done, OR if you intend to retain defensive
safety, keep the try-catch but replace it with a clear comment explaining that
the catch is deliberate to guard against future changes in authenticateToken's
contract; reference authenticateToken, SecurityContextHolder.getContext(), and
authToken.setDetails(...) when making the change.
In `@src/main/java/com/p2ps/config/StompJwtAuthInterceptor.java`:
- Around line 35-40: The accessor.setUser(authentication) and then also calling
MessageBuilder.fromMessage(message).setHeader(SimpMessageHeaderAccessor.USER_HEADER,
authentication) is redundant; update StompJwtAuthInterceptor to only call
accessor.setUser(authentication) and return
MessageBuilder.fromMessage(message).build() (or alternatively remove the
accessor.setUser and keep the explicit header, but prefer keeping
accessor.setUser), ensuring you still use the existing variables accessor,
authentication and MessageBuilder.fromMessage(message) so the principal is
propagated without duplicating the USER_HEADER population.
In `@src/test/java/com/p2ps/config/StompJwtAuthInterceptorTest.java`:
- Around line 44-60: The test preSend_ConnectWithoutTokenPassesThrough contains
an unnecessary mock setup
when(jwtAuthFilter.authenticateToken(null)).thenReturn(null) because
StompJwtAuthInterceptor.resolveAuthentication returns early when resolveToken is
null and never calls authenticateToken; remove that when(...) line OR replace it
with an explicit verification like verify(jwtAuthFilter,
never()).authenticateToken(any()) to document the expected behavior. Update the
test in StompJwtAuthInterceptorTest to reference JwtAuthFilter.authenticateToken
and StompJwtAuthInterceptor.resolveAuthentication/resolveToken accordingly so
the test is clearer and not misleading.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a803d5ce-800a-445f-9cb0-fb1e931bfd57
📒 Files selected for processing (7)
src/main/java/com/p2ps/auth/security/JwtAuthFilter.javasrc/main/java/com/p2ps/config/StompJwtAuthInterceptor.javasrc/test/java/com/p2ps/auth/JwtAuthFilterTest.javasrc/test/java/com/p2ps/auth/security/JwtAuthFilterTest.javasrc/test/java/com/p2ps/auth/security/SecurityConfigTest.javasrc/test/java/com/p2ps/config/JwtHandshakeInterceptorTest.javasrc/test/java/com/p2ps/config/StompJwtAuthInterceptorTest.java
💤 Files with no reviewable changes (1)
- src/test/java/com/p2ps/auth/JwtAuthFilterTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/com/p2ps/config/JwtHandshakeInterceptorTest.java
…e test assertions in StompJwtAuthInterceptorTest
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/test/java/com/p2ps/auth/security/SecurityConfigTest.java (1)
10-10: Remove unused import.
SecurityFilterChainis imported but never used in this test class.🧹 Suggested fix
-import org.springframework.security.web.SecurityFilterChain;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/p2ps/auth/security/SecurityConfigTest.java` at line 10, Remove the unused import of SecurityFilterChain from SecurityConfigTest.java: locate the import statement "import org.springframework.security.web.SecurityFilterChain;" and delete it so unused imports are cleaned up and the test compiles without unnecessary dependencies.src/test/java/com/p2ps/config/StompJwtAuthInterceptorTest.java (1)
22-42: Consider consolidating header variation tests into a parameterized test.The tests for different header names (
Authorization,authorization,token,access_token) share identical structure. A@ParameterizedTestwith@MethodSourceor@CsvSourcewould reduce duplication while maintaining the same coverage.♻️ Example parameterized approach
`@ParameterizedTest` `@CsvSource`({ "Authorization, Bearer valid-token, valid-token", "authorization, Bearer lower-token, lower-token", "token, token-header-value, token-header-value", "access_token, access-token-value, access-token-value" }) void preSend_ConnectWithVariousHeadersAuthenticates(String headerName, String headerValue, String expectedToken) { JwtAuthFilter jwtAuthFilter = mock(JwtAuthFilter.class); StompJwtAuthInterceptor interceptor = new StompJwtAuthInterceptor(jwtAuthFilter); StompHeaderAccessor accessor = StompHeaderAccessor.create(StompCommand.CONNECT); accessor.addNativeHeader(headerName, headerValue); accessor.setLeaveMutable(true); accessor.setSessionAttributes(new HashMap<>()); Message<?> message = MessageBuilder.createMessage(new byte[0], accessor.getMessageHeaders()); MessageChannel channel = mock(MessageChannel.class); UsernamePasswordAuthenticationToken authentication = new UsernamePasswordAuthenticationToken("user@test.com", null, List.of()); when(jwtAuthFilter.authenticateToken(expectedToken)).thenReturn(authentication); Message<?> result = interceptor.preSend(message, channel); assertNotNull(result); assertSame(authentication, StompHeaderAccessor.wrap(result).getUser()); }Also applies to: 100-121, 123-144, 146-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/p2ps/config/StompJwtAuthInterceptorTest.java` around lines 22 - 42, Several near-duplicate tests call StompJwtAuthInterceptor.preSend with different header names; convert these into a single `@ParameterizedTest` (using `@CsvSource` or `@MethodSource`) that supplies headerName, headerValue, and expectedToken, instantiate JwtAuthFilter and StompJwtAuthInterceptor once per invocation, build the StompHeaderAccessor with accessor.addNativeHeader(headerName, headerValue), stub jwtAuthFilter.authenticateToken(expectedToken) to return the UsernamePasswordAuthenticationToken, call interceptor.preSend(message, channel) and assert the returned message user equals that authentication; update all groups of duplicated tests (the blocks referencing preSend_ConnectWithHeaderTokenAuthenticates and the similar blocks at lines noted in the review) to use this parameterized approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/java/com/p2ps/auth/security/SecurityConfigTest.java`:
- Line 10: Remove the unused import of SecurityFilterChain from
SecurityConfigTest.java: locate the import statement "import
org.springframework.security.web.SecurityFilterChain;" and delete it so unused
imports are cleaned up and the test compiles without unnecessary dependencies.
In `@src/test/java/com/p2ps/config/StompJwtAuthInterceptorTest.java`:
- Around line 22-42: Several near-duplicate tests call
StompJwtAuthInterceptor.preSend with different header names; convert these into
a single `@ParameterizedTest` (using `@CsvSource` or `@MethodSource`) that supplies
headerName, headerValue, and expectedToken, instantiate JwtAuthFilter and
StompJwtAuthInterceptor once per invocation, build the StompHeaderAccessor with
accessor.addNativeHeader(headerName, headerValue), stub
jwtAuthFilter.authenticateToken(expectedToken) to return the
UsernamePasswordAuthenticationToken, call interceptor.preSend(message, channel)
and assert the returned message user equals that authentication; update all
groups of duplicated tests (the blocks referencing
preSend_ConnectWithHeaderTokenAuthenticates and the similar blocks at lines
noted in the review) to use this parameterized approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0996d7ca-6199-4aff-b50c-a54877a78c3c
📒 Files selected for processing (7)
src/main/java/com/p2ps/auth/security/JwtAuthFilter.javasrc/main/java/com/p2ps/config/StompJwtAuthInterceptor.javasrc/test/java/com/p2ps/auth/JwtAuthFilterTest.javasrc/test/java/com/p2ps/auth/security/JwtAuthFilterTest.javasrc/test/java/com/p2ps/auth/security/SecurityConfigTest.javasrc/test/java/com/p2ps/config/JwtHandshakeInterceptorTest.javasrc/test/java/com/p2ps/config/StompJwtAuthInterceptorTest.java
💤 Files with no reviewable changes (1)
- src/test/java/com/p2ps/auth/JwtAuthFilterTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/com/p2ps/auth/security/JwtAuthFilterTest.java
- src/test/java/com/p2ps/config/JwtHandshakeInterceptorTest.java
… tests and remove redundant test cases
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/java/com/p2ps/config/StompJwtAuthInterceptorTest.java`:
- Around line 92-106: The test preSend_NonConnectCommandsPassThrough currently
only asserts message identity but doesn't verify that authentication wasn't
invoked; after the loop add a Mockito verification that the mocked
JwtAuthFilter's authenticateToken(...) (reference JwtAuthFilter and its
authenticateToken method and the StompJwtAuthInterceptor.preSend test) was never
called, e.g. verify(jwtAuthFilter, never()).authenticateToken(any()), to ensure
non-CONNECT StompCommand messages do not trigger authentication.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9ee9ab4-7ed9-47ce-a13c-531c71f39a7e
📒 Files selected for processing (2)
src/test/java/com/p2ps/auth/security/SecurityConfigTest.javasrc/test/java/com/p2ps/config/StompJwtAuthInterceptorTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/com/p2ps/auth/security/SecurityConfigTest.java
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
bmbianca
approved these changes
Apr 2, 2026
…hance README with local development instructions
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Some routes should be available without authentication.
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests
Documentation
Chores