middleware added through FilterConfig for JWT Token Validation. #87
Conversation
WalkthroughThe pull request adds new project dependencies and multiple Java classes to enable JSON processing and JWT-based authentication. Enhancements include a new user entity with its repository interface, and several utility classes for handling cookies, generating and validating JWT tokens. Additionally, a servlet filter along with its configuration is introduced to intercept HTTP requests and enforce JWT validation. CORS configuration is centralized and dynamic CORS filtering is implemented. Cross-origin annotations are removed from controllers, and new environment properties for CORS allowed origins are added. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant F as JwtUserIdValidationFilter
participant CU as CookieUtil
participant JA as JwtAuthenticationUtil
participant DB as EmployeeMasterRepo
participant RC as Redis Cache
C->>F: Send HTTP request with JWT (cookie/header)
F->>CU: Extract JWT token from request
CU-->>F: Return extracted token
F->>JA: Validate JWT token
alt Token valid
JA->>RC: Check if user data is cached
alt User not found in cache
JA->>DB: Retrieve user details from DB
DB-->>JA: Return user information
JA->>RC: Cache user data
end
JA-->>F: Return success response
else Token invalid
JA-->>F: Return error response
end
F->>C: Proceed with request / Return unauthorized error
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (15)
✅ Files skipped from review due to trivial changes (11)
🔇 Additional comments (4)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 8
🧹 Nitpick comments (12)
src/main/java/com/iemr/common/identity/repo/iemr/EmployeeMasterRepo.java (1)
8-11: Consider consolidating redundant methods.The interface defines two methods (
findByUserIDandgetUserByUserID) that appear to perform the same function - retrieving a user by ID. Spring Data JPA will automatically implementfindByUserIDbased on naming convention, making the second method potentially redundant.If these methods serve different purposes (e.g., different query implementations), consider:
- Adding documentation to clarify their distinct roles
- Or consolidating into a single method if they're functionally identical
@Repository public interface EmployeeMasterRepo extends JpaRepository<M_User,Integer> { M_User findByUserID(Integer userID); - M_User getUserByUserID(Integer parseLong); }src/main/java/com/iemr/common/identity/utils/FilterConfig.java (1)
7-18: Add documentation and consider filter ordering.The configuration correctly registers the JWT validation filter, but lacks documentation about its purpose and behavior.
Consider adding:
- Javadoc comments explaining the filter's purpose
- Filter ordering to control execution sequence if multiple filters exist
- Conditional activation through properties
@Configuration +/** + * Configuration for JWT token validation filter. + * Registers a filter that intercepts all requests to validate JWT tokens. + */ public class FilterConfig { @Bean public FilterRegistrationBean<JwtUserIdValidationFilter> jwtUserIdValidationFilter( JwtAuthenticationUtil jwtAuthenticationUtil) { FilterRegistrationBean<JwtUserIdValidationFilter> registrationBean = new FilterRegistrationBean<>(); registrationBean.setFilter(new JwtUserIdValidationFilter(jwtAuthenticationUtil)); registrationBean.addUrlPatterns("/*"); // Apply filter to all API endpoints + registrationBean.setOrder(1); // Set filter order return registrationBean; } }src/main/java/com/iemr/common/identity/domain/iemr/M_User.java (2)
132-134: Remove TODO comments from constructors.Both constructors contain TODO placeholder comments that should be resolved or removed before merging. These comments don't provide any information and suggest incomplete implementation.
public M_User() { - // TODO Auto-generated constructor stub } public M_User(Integer userID, String userName) { - // TODO Auto-generated constructor stub this.userID=userID; this.userName=userName; }Also applies to: 137-138
18-133: Follow Java naming conventions for field names.Several field names don't follow standard Java naming conventions, which typically use camelCase starting with lowercase. Current fields like
dOB,dOJ, andpANuse unconventional capitalization patterns.While these likely match the database column names, consider using proper Java naming conventions in the entity class:
@Expose @Column(name="DOB") - private LocalDate dOB; + private LocalDate dob; @Expose @Column(name="DOJ") - private LocalDate dOJ; + private LocalDate doj; @Expose @Column(name="PAN") - private String pAN; + private String pan;You can maintain the correct column mapping via the
@Columnannotation while using proper Java conventions.pom.xml (1)
215-220: Remove explicit spring-web version override.The project explicitly specifies version 6.1.12 for spring-web, which could override the version managed by Spring Boot parent. This can lead to version conflicts if not carefully managed.
Unless there's a specific reason to use this version, let Spring Boot manage the version:
<dependency> <groupId>org.springframework</groupId> <artifactId>spring-web</artifactId> - <version>6.1.12</version> </dependency>src/main/java/com/iemr/common/identity/utils/JwtAuthenticationUtil.java (4)
22-29: Adopt a consistent dependency injection approach.
Mixing field-level@Autowiredinjection (lines 22-29) with constructor injection (lines 32-35) can be confusing. You should pick one pattern (preferably constructor injection) to ensure clarity and easier testability.
37-62: Avoid returningResponseEntityfrom a utility method.
Returning a SpringResponseEntityfrom a utility class fuses your business logic with the web layer. Instead, consider throwing a custom exception or returning a domain object, leaving HTTP-related formatting to the controller/filter layer.
64-90: Consider validating additional user attributes.
Right now, you only verify the user's existence. If needed, ensure you also confirm the user's active status or not locked/disabled in the database to enhance security.
105-123: Make parse errors more explicit.
WhenInteger.parseInt(userId)fails, it is wrapped in a generic exception. Consider catchingNumberFormatExceptionor validatinguserIdbefore parsing for clearer error handling.src/main/java/com/iemr/common/identity/utils/JwtUtil.java (3)
18-19: Secure your JWT secret key properlyThe
SECRET_KEYfield stores sensitive information and should be handled carefully. Consider making it final for immutability once initialized.- private String SECRET_KEY; + private final String SECRET_KEY;Additionally, verify the secret key is sufficiently complex and long enough (at least 256 bits for HS256) in your application properties.
56-74: Fix whitespace and improve error handling in extractUserId methodThere are inconsistent whitespaces in this method and the error message could be more specific.
-public Integer extractUserId(String jwtToken) throws IEMRException { +public Integer extractUserId(String jwtToken) throws IEMRException { try { // Validate JWT token and extract claims Claims claims = validateToken(jwtToken); if (claims == null) { - throw new IEMRException("Invalid JWT token."); + throw new IEMRException("Invalid or expired JWT token."); } String userId = claims.get("userId", String.class); - return Integer.parseInt(userId); + return Integer.parseInt(userId); } catch (NumberFormatException e) { + throw new IEMRException("Invalid userId format in JWT token: " + e.getMessage(), e); } catch (Exception e) { throw new IEMRException("Validation error: " + e.getMessage(), e); } }
31-40: Consider adding token refresh functionalityThe current implementation sets a fixed 24-hour expiration time for tokens, but doesn't provide a way to refresh tokens.
Consider adding a refresh token mechanism to improve security while maintaining user convenience. This typically involves:
- Generating a separate refresh token with longer expiration
- Adding a method to validate refresh tokens and generate new access tokens
- Storing refresh token identifiers in a database or cache to allow revocation
This approach allows for shorter-lived access tokens with the ability to generate new ones without requiring users to re-authenticate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pom.xml(1 hunks)src/main/java/com/iemr/common/identity/domain/iemr/M_User.java(1 hunks)src/main/java/com/iemr/common/identity/repo/iemr/EmployeeMasterRepo.java(1 hunks)src/main/java/com/iemr/common/identity/utils/CookieUtil.java(1 hunks)src/main/java/com/iemr/common/identity/utils/FilterConfig.java(1 hunks)src/main/java/com/iemr/common/identity/utils/JwtAuthenticationUtil.java(1 hunks)src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java(1 hunks)src/main/java/com/iemr/common/identity/utils/JwtUtil.java(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
src/main/java/com/iemr/common/identity/utils/JwtAuthenticationUtil.java (2)
src/main/java/com/iemr/common/identity/utils/JwtUtil.java (1)
Component(15-85)src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (1)
Component(13-106)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (2)
src/main/java/com/iemr/common/identity/utils/JwtAuthenticationUtil.java (1)
Component(19-124)src/main/java/com/iemr/common/identity/utils/JwtUtil.java (1)
Component(15-85)
🪛 ast-grep (0.31.1)
src/main/java/com/iemr/common/identity/utils/CookieUtil.java
[warning] 33-33: A cookie was detected without setting the 'secure' flag. The 'secure' flag for cookies prevents the client from transmitting the cookie over insecure channels such as HTTP. Set the 'secure' flag by calling '.setSecure(true);'.
Context: response.addCookie(cookie);
Note: [CWE-614] Sensitive Cookie in HTTPS Session Without 'Secure' Attribute. [REFERENCES]
- https://owasp.org/www-community/controls/SecureCookieAttribute
(cookie-missing-secure-flag-java)
[warning] 33-33: The application does not appear to verify inbound requests which can lead to a Cross-site request forgery (CSRF) vulnerability. If the application uses cookie-based authentication, an attacker can trick users into sending authenticated HTTP requests without their knowledge from any arbitrary domain they visit. To prevent this vulnerability start by identifying if the framework or library leveraged has built-in features or offers plugins for CSRF protection. CSRF tokens should be unique and securely random. The Synchronizer Token or Double Submit Cookie patterns with defense-in-depth mechanisms such as the sameSite cookie flag can help prevent CSRF. For more information, see: [Cross-site request forgery prevention](https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Req\ uest_Forgery_Prevention_Cheat_Sheet.html).
Context: response.addCookie(cookie);
Note: [CWE-352] Cross-Site Request Forgery (CSRF). [REFERENCES]
- https://stackoverflow.com/questions/42717210/samesite-cookie-in-java-application
(cookie-missing-samesite-java)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java
[warning] 103-103: The application does not appear to verify inbound requests which can lead to a Cross-site request forgery (CSRF) vulnerability. If the application uses cookie-based authentication, an attacker can trick users into sending authenticated HTTP requests without their knowledge from any arbitrary domain they visit. To prevent this vulnerability start by identifying if the framework or library leveraged has built-in features or offers plugins for CSRF protection. CSRF tokens should be unique and securely random. The Synchronizer Token or Double Submit Cookie patterns with defense-in-depth mechanisms such as the sameSite cookie flag can help prevent CSRF. For more information, see: [Cross-site request forgery prevention](https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Req\ uest_Forgery_Prevention_Cheat_Sheet.html).
Context: response.addCookie(cookie);
Note: [CWE-352] Cross-Site Request Forgery (CSRF). [REFERENCES]
- https://stackoverflow.com/questions/42717210/samesite-cookie-in-java-application
(cookie-missing-samesite-java)
🔇 Additional comments (1)
src/main/java/com/iemr/common/identity/utils/FilterConfig.java (1)
15-15: Verify if filtering all URLs is appropriate.The current configuration applies JWT validation to all URLs including static resources with the pattern "/*". Verify if this is intentional or if the filter should target only specific API endpoints.
Consider restricting the filter to only API endpoints if authentication isn't required for all resources:
- registrationBean.addUrlPatterns("/*"); // Apply filter to all API endpoints + registrationBean.addUrlPatterns("/api/*"); // Apply filter only to API endpoints
| @Expose | ||
| @Column(name="Password") | ||
| private String password; |
There was a problem hiding this comment.
Secure sensitive information storage.
The password field is currently exposed via the @Expose annotation, which means it could be included in serialized output. Additionally, there's no indication the password is being stored securely (hashed and salted).
Consider:
- Removing
@Exposeannotation from the password field - Add a comment or annotation indicating password handling mechanism
- Ensure the actual storage implementation follows security best practices for passwords
- @Expose
@Column(name="Password")
private String 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.
| @Expose | |
| @Column(name="Password") | |
| private String password; | |
| @Column(name="Password") | |
| private String password; |
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| <!-- Jersey JSON Processing --> | ||
| <dependency> | ||
| <groupId>org.glassfish.jersey.media</groupId> | ||
| <artifactId>jersey-media-json-processing</artifactId> | ||
| <version>2.30.1</version> | ||
| </dependency> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update Jersey JSON Processing version for compatibility.
The version of jersey-media-json-processing (2.30.1) is outdated and may have compatibility issues with Spring Boot 3.x and Jakarta EE.
Consider updating to a version that supports Jakarta EE:
<dependency>
<groupId>org.glassfish.jersey.media</groupId>
<artifactId>jersey-media-json-processing</artifactId>
- <version>2.30.1</version>
+ <version>3.1.3</version>
</dependency>📝 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.
| <!-- Jersey JSON Processing --> | |
| <dependency> | |
| <groupId>org.glassfish.jersey.media</groupId> | |
| <artifactId>jersey-media-json-processing</artifactId> | |
| <version>2.30.1</version> | |
| </dependency> | |
| <!-- Jersey JSON Processing --> | |
| <dependency> | |
| <groupId>org.glassfish.jersey.media</groupId> | |
| <artifactId>jersey-media-json-processing</artifactId> | |
| <version>3.1.3</version> | |
| </dependency> |
| <dependency> | ||
| <groupId>org.glassfish.jaxb</groupId> | ||
| <artifactId>jaxb-runtime</artifactId> | ||
| <version>2.3.1</version> | ||
| </dependency> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update JAXB Runtime version for compatibility.
The project is using JAXB Runtime 2.3.1, which is quite old and may not be fully compatible with Jakarta EE in Spring Boot 3.x. Spring Boot 3 uses Jakarta EE 9+ which requires newer JAXB versions.
Consider updating to a newer version that aligns with Jakarta EE:
<dependency>
<groupId>org.glassfish.jaxb</groupId>
<artifactId>jaxb-runtime</artifactId>
- <version>2.3.1</version>
+ <version>4.0.0</version>
</dependency>📝 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.
| <dependency> | |
| <groupId>org.glassfish.jaxb</groupId> | |
| <artifactId>jaxb-runtime</artifactId> | |
| <version>2.3.1</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.glassfish.jaxb</groupId> | |
| <artifactId>jaxb-runtime</artifactId> | |
| <version>4.0.0</version> | |
| </dependency> |
| public void addJwtTokenToCookie(String Jwttoken, HttpServletResponse response, HttpServletRequest request) { | ||
| // Create a new cookie with the JWT token | ||
| Cookie cookie = new Cookie("Jwttoken", Jwttoken); | ||
| cookie.setHttpOnly(true); // Prevent JavaScript access for security | ||
| cookie.setMaxAge(60 * 60 * 24); // 1 day expiration time | ||
| cookie.setPath("/"); // Make the cookie available for the entire application | ||
| if ("https".equalsIgnoreCase(request.getScheme())) { | ||
| cookie.setSecure(true); // Secure flag only on HTTPS | ||
| } | ||
| response.addCookie(cookie); // Add the cookie to the response | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add CSRF mitigation measures to cookies.
While you already set HttpOnly and conditionally set Secure, consider adding the SameSite attribute to prevent CSRF attacks. For modern Java Servlet APIs, you may need custom headers or frameworks that support setting SameSite=Lax or Strict.
🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 33-33: A cookie was detected without setting the 'secure' flag. The 'secure' flag for cookies prevents the client from transmitting the cookie over insecure channels such as HTTP. Set the 'secure' flag by calling '.setSecure(true);'.
Context: response.addCookie(cookie);
Note: [CWE-614] Sensitive Cookie in HTTPS Session Without 'Secure' Attribute. [REFERENCES]
- https://owasp.org/www-community/controls/SecureCookieAttribute
(cookie-missing-secure-flag-java)
[warning] 33-33: The application does not appear to verify inbound requests which can lead to a Cross-site request forgery (CSRF) vulnerability. If the application uses cookie-based authentication, an attacker can trick users into sending authenticated HTTP requests without their knowledge from any arbitrary domain they visit. To prevent this vulnerability start by identifying if the framework or library leveraged has built-in features or offers plugins for CSRF protection. CSRF tokens should be unique and securely random. The Synchronizer Token or Double Submit Cookie patterns with defense-in-depth mechanisms such as the sameSite cookie flag can help prevent CSRF. For more information, see: [Cross-site request forgery prevention](https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Req\ uest_Forgery_Prevention_Cheat_Sheet.html).
Context: response.addCookie(cookie);
Note: [CWE-352] Cross-Site Request Forgery (CSRF). [REFERENCES]
- https://stackoverflow.com/questions/42717210/samesite-cookie-in-java-application
(cookie-missing-samesite-java)
| @Override | ||
| public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain) | ||
| throws IOException, ServletException { | ||
| HttpServletRequest request = (HttpServletRequest) servletRequest; | ||
| HttpServletResponse response = (HttpServletResponse) servletResponse; | ||
|
|
||
| String path = request.getRequestURI(); | ||
| String contextPath = request.getContextPath(); | ||
| logger.info("JwtUserIdValidationFilter invoked for path: " + path); | ||
|
|
||
| // Log cookies for debugging | ||
| Cookie[] cookies = request.getCookies(); | ||
| if (cookies != null) { | ||
| for (Cookie cookie : cookies) { | ||
| if ("userId".equals(cookie.getName())) { | ||
| logger.warn("userId found in cookies! Clearing it..."); | ||
| clearUserIdCookie(response); // Explicitly remove userId cookie | ||
| } | ||
| } | ||
| } else { | ||
| logger.info("No cookies found in the request"); | ||
| } | ||
|
|
||
| // Log headers for debugging | ||
| String jwtTokenFromHeader = request.getHeader("Jwttoken"); | ||
| logger.info("JWT token from header: "); | ||
|
|
||
| // Skip login and public endpoints | ||
| if (path.equals(contextPath + "/user/userAuthenticate") | ||
| || path.equalsIgnoreCase(contextPath + "/user/logOutUserFromConcurrentSession") | ||
| || path.startsWith(contextPath + "/public")) { | ||
| logger.info("Skipping filter for path: " + path); | ||
| filterChain.doFilter(servletRequest, servletResponse); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| // Retrieve JWT token from cookies | ||
| String jwtTokenFromCookie = getJwtTokenFromCookies(request); | ||
| logger.info("JWT token from cookie: "); | ||
|
|
||
| // Determine which token (cookie or header) to validate | ||
| String jwtToken = jwtTokenFromCookie != null ? jwtTokenFromCookie : jwtTokenFromHeader; | ||
| if (jwtToken == null) { | ||
| response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "JWT token not found in cookies or headers"); | ||
| return; | ||
| } | ||
|
|
||
| // Validate JWT token and userId | ||
| boolean isValid = jwtAuthenticationUtil.validateUserIdAndJwtToken(jwtToken); | ||
|
|
||
| if (isValid) { | ||
| // If token is valid, allow the request to proceed | ||
| filterChain.doFilter(servletRequest, servletResponse); | ||
| } else { | ||
| response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Invalid JWT token"); | ||
| } | ||
| } catch (Exception e) { | ||
| logger.error("Authorization error: ", e); | ||
| response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Authorization error: "); | ||
| } | ||
| } |
There was a problem hiding this comment.
Implement CSRF protection.
This filter enforces JWT validation but does not address CSRF. Attackers can forge user requests since cookies are implicitly sent with each request. Consider introducing a CSRF token mechanism, SameSite cookies, or additional server-side checks.
There was a problem hiding this comment.
🧩 Analysis chain
The code in CookieUtil.java shows that cookies are indeed being created, but I need to verify if they have the SameSite attribute set:
Let me check the content of the addJwtTokenToCookie method more carefully to see how cookies are configured:
@drtechie I looked through the codebase but couldn't find where the SameSite attribute is being set on cookies. In the CookieUtil class, the addJwtTokenToCookie method creates cookies but doesn't appear to configure the SameSite attribute.
To properly set SameSite, you could add something like:
Cookie cookie = new Cookie("Jwttoken", Jwttoken);
cookie.setHttpOnly(true);
cookie.setMaxAge(24 * 60 * 60); // 1 day
cookie.setPath("/");
if (request.isSecure()) {
cookie.setSecure(true);
}
// Add SameSite attribute
cookie.setAttribute("SameSite", "Strict"); // or "Lax" depending on your requirementsThis would strengthen CSRF protection as mentioned in the previous comment. Is this configured somewhere else in the application?
🏁 Script executed:
#!/bin/bash
# Check if CookieUtil.java exists and view its content
fd -t f "CookieUtil.java" --exec cat {} \;
Length of output: 1679
🏁 Script executed:
#!/bin/bash
# Look specifically for SameSite attribute being set in CookieUtil or related classes
rg -i "samesite" --include "*.java"
Length of output: 477
🏁 Script executed:
#!/bin/bash
# Look for cookie creation and configuration in CookieUtil
rg -A 15 "addJwtTokenToCookie" --include "*.java"
Length of output: 491
🏁 Script executed:
#!/bin/bash
# Look specifically for SameSite attribute being set in any Java file
rg -i "samesite"
# Try to find cookie configuration in the codebase
rg "cookie\.set"
Length of output: 1023
🏁 Script executed:
#!/bin/bash
# Look for the clearUserIdCookie method implementation
rg -A 10 "clearUserIdCookie"
Length of output: 2220
@drtechie I've checked the codebase but couldn't find where the SameSite attribute is being set on cookies. In both CookieUtil.java and JwtUserIdValidationFilter.java, the cookies are configured with HttpOnly, Secure, and Path attributes, but there's no SameSite configuration.
To properly implement SameSite cookies and address the CSRF concern, you could add:
// In CookieUtil.java - addJwtTokenToCookie method
Cookie cookie = new Cookie("Jwttoken", Jwttoken);
cookie.setHttpOnly(true);
cookie.setMaxAge(60 * 60 * 24);
cookie.setPath("/");
if ("https".equalsIgnoreCase(request.getScheme())) {
cookie.setSecure(true);
}
// Add SameSite attribute (depending on your Jakarta version)
cookie.setAttribute("SameSite", "Lax"); // or "Strict" for more security
// For older Jakarta/Servlet versions, use header
response.setHeader("Set-Cookie", response.getHeader("Set-Cookie") + "; SameSite=Lax");Similar updates would be needed in the clearUserIdCookie method as well. This would strengthen the CSRF protection mentioned in the earlier comment.
✏️ Learnings added
Learnt from: drtechie
PR: PSMRI/Identity-API#87
File: src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java:23-84
Timestamp: 2025-04-10T05:08:55.382Z
Learning: SameSite cookie attribute should be set to enhance CSRF protection in the Identity-API project. This can be configured using cookie.setAttribute("SameSite", "Strict") or cookie.setAttribute("SameSite", "Lax") depending on requirements.
Learnt from: drtechie
PR: PSMRI/Identity-API#87
File: src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java:23-84
Timestamp: 2025-04-10T05:08:55.382Z
Learning: In the Identity-API project, SameSite cookie attribute should be set to enhance CSRF protection. This can be configured using cookie.setAttribute("SameSite", "Strict") or cookie.setAttribute("SameSite", "Lax") depending on requirements. For older Jakarta/Servlet versions, it may require using response.setHeader("Set-Cookie", response.getHeader("Set-Cookie") + "; SameSite=Lax").
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| public Claims validateToken(String token) { | ||
| try { | ||
| return Jwts.parser().setSigningKey(getSigningKey()).build().parseClaimsJws(token).getBody(); | ||
| } catch (Exception e) { | ||
| return null; // Handle token parsing/validation errors | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve token validation error handling
The current implementation swallows all exceptions and returns null, which can lead to null pointer exceptions and makes debugging difficult.
Consider:
- Logging validation errors
- Using more specific exception handling
- Potentially differentiating between different validation failure reasons
public Claims validateToken(String token) {
try {
return Jwts.parser().setSigningKey(getSigningKey()).build().parseClaimsJws(token).getBody();
} catch (Exception e) {
- return null; // Handle token parsing/validation errors
+ // Log the exception
+ logger.error("JWT validation error: {}", e.getMessage());
+ return null; // Handle token parsing/validation errors
}
}For more detailed error handling:
public Claims validateToken(String token) {
try {
return Jwts.parser().setSigningKey(getSigningKey()).build().parseClaimsJws(token).getBody();
} catch (ExpiredJwtException e) {
logger.warn("JWT token expired: {}", e.getMessage());
return null;
} catch (MalformedJwtException e) {
logger.error("Invalid JWT token: {}", e.getMessage());
return null;
} catch (UnsupportedJwtException e) {
logger.error("Unsupported JWT token: {}", e.getMessage());
return null;
} catch (Exception e) {
logger.error("JWT validation error: {}", e.getMessage());
return null;
}
}| package com.iemr.common.identity.utils; | ||
|
|
||
| import com.iemr.common.identity.utils.exception.IEMRException; | ||
| import io.jsonwebtoken.Claims; | ||
| import io.jsonwebtoken.Jwts; | ||
| import io.jsonwebtoken.SignatureAlgorithm; | ||
| import io.jsonwebtoken.security.Keys; | ||
| import org.springframework.beans.factory.annotation.Value; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
| import java.security.Key; | ||
| import java.util.Date; | ||
| import java.util.function.Function; | ||
|
|
||
| @Component | ||
| public class JwtUtil { | ||
|
|
||
| @Value("${jwt.secret}") | ||
| private String SECRET_KEY; | ||
|
|
||
| private static final long EXPIRATION_TIME = 24L * 60 * 60 * 1000; // 1 day in milliseconds | ||
|
|
||
| // Generate a key using the secret | ||
| private Key getSigningKey() { | ||
| if (SECRET_KEY == null || SECRET_KEY.isEmpty()) { | ||
| throw new IllegalStateException("JWT secret key is not set in application.properties"); | ||
| } | ||
| return Keys.hmacShaKeyFor(SECRET_KEY.getBytes()); | ||
| } | ||
|
|
||
| // Generate JWT Token | ||
| public String generateToken(String username, String userId) { | ||
| Date now = new Date(); | ||
| Date expiryDate = new Date(now.getTime() + EXPIRATION_TIME); | ||
|
|
||
| // Include the userId in the JWT claims | ||
| return Jwts.builder().setSubject(username).claim("userId", userId) // Add userId as a claim | ||
| .setIssuedAt(now).setExpiration(expiryDate).signWith(getSigningKey(), SignatureAlgorithm.HS256) | ||
| .compact(); | ||
| } | ||
|
|
||
| // Validate and parse JWT Token | ||
| public Claims validateToken(String token) { | ||
| try { | ||
| return Jwts.parser().setSigningKey(getSigningKey()).build().parseClaimsJws(token).getBody(); | ||
| } catch (Exception e) { | ||
| return null; // Handle token parsing/validation errors | ||
| } | ||
| } | ||
|
|
||
|
|
||
| public String extractUsername(String token) { | ||
| return extractClaim(token, Claims::getSubject); | ||
| } | ||
|
|
||
| public Integer extractUserId(String jwtToken) throws IEMRException { | ||
| try { | ||
| // Validate JWT token and extract claims | ||
| Claims claims = validateToken(jwtToken); | ||
|
|
||
| if (claims == null) { | ||
| throw new IEMRException("Invalid JWT token."); | ||
| } | ||
|
|
||
| String userId = claims.get("userId", String.class); | ||
|
|
||
| return Integer.parseInt(userId); | ||
|
|
||
| } catch (Exception e) { | ||
| throw new IEMRException("Validation error: " + e.getMessage(), e); | ||
| } | ||
|
|
||
|
|
||
| } | ||
|
|
||
|
|
||
| public <T> T extractClaim(String token, Function<Claims, T> claimsResolver) { | ||
| final Claims claims = extractAllClaims(token); | ||
| return claimsResolver.apply(claims); | ||
| } | ||
|
|
||
| private Claims extractAllClaims(String token) { | ||
| return Jwts.parser().setSigningKey(getSigningKey()).build().parseClaimsJws(token).getBody(); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add unit tests for JWT token functionality
This is a critical security component that requires thorough testing to ensure correct behavior.
Please verify if there are unit tests covering:
- Token generation with valid inputs
- Token validation with valid/invalid tokens
- Claim extraction with various scenarios
- Error handling for malformed tokens
🏁 Script executed:
#!/bin/bash
# Search for test files related to JwtUtil
echo "Searching for test files related to JwtUtil..."
find src/test -name "*JwtUtil*Test.java" -o -name "Test*JwtUtil*.java" -o -name "*JwtTest*.java"
# Search for any tests that might use JwtUtil
echo "Searching for tests that might test JWT functionality..."
grep -r "JwtUtil" --include="*.java" src/test 2>/dev/null || echo "No references found"Length of output: 419
Action Required: Add Comprehensive Unit Tests for JWT Functionality
It appears that the JwtUtil class is not currently covered by any unit tests. Please add tests that validate the following scenarios:
- Token Generation: Verify that tokens are generated correctly when provided with valid username and userId inputs.
- Token Validation: Ensure that valid tokens are correctly parsed, while invalid, expired, or malformed tokens result in appropriate error handling.
- Claim Extraction: Confirm that claims (especially the username and userId) are accurately extracted from the token.
- Error Handling: Test that errors (such as a missing or empty secret key) are properly handled and that the corresponding exceptions or error messages are produced.
Consider creating a new file (e.g., src/test/java/com/iemr/common/identity/utils/JwtUtilTest.java) to house these tests.
| private Claims extractAllClaims(String token) { | ||
| return Jwts.parser().setSigningKey(getSigningKey()).build().parseClaimsJws(token).getBody(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent duplicate code and add error handling in extractAllClaims
The extractAllClaims method duplicates code from validateToken without the same error handling.
private Claims extractAllClaims(String token) {
- return Jwts.parser().setSigningKey(getSigningKey()).build().parseClaimsJws(token).getBody();
+ Claims claims = validateToken(token);
+ if (claims == null) {
+ throw new IllegalArgumentException("Invalid token");
+ }
+ return claims;
}This approach ensures consistent token validation behavior across the application and proper error handling.
📝 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.
| private Claims extractAllClaims(String token) { | |
| return Jwts.parser().setSigningKey(getSigningKey()).build().parseClaimsJws(token).getBody(); | |
| } | |
| private Claims extractAllClaims(String token) { | |
| - return Jwts.parser().setSigningKey(getSigningKey()).build().parseClaimsJws(token).getBody(); | |
| + Claims claims = validateToken(token); | |
| + if (claims == null) { | |
| + throw new IllegalArgumentException("Invalid token"); | |
| + } | |
| + return claims; | |
| } |
There was a problem hiding this comment.
@kevalkanp1011 are all these attributes required for JWT checks?
There was a problem hiding this comment.
not all but username, password, userid are required. i followed similar implementation available in other repositories
|
@ravishanigarapu can you check this PR? |
There was a problem hiding this comment.
@kevalkanp1011 Please Add jwtToken as header in Common API where we are calling identity API's
CC : @drtechie
|
jwtToken is already configured through middlewares in Common-API. @ravishanigarapu . please review asap so i can raise a new PR for cors config |
|
4 participants
📋 Description
JIRA ID:
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit