Jwttoken implementation #106
Conversation
WalkthroughThis change introduces centralized JWT-based authentication and CORS management. It adds new utility, configuration, and domain classes for JWT creation, validation, denylisting, and user management. CORS is now globally configured via a dedicated config class and properties, removing all Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Filter as JwtUserIdValidationFilter
participant JwtUtil
participant JwtAuthUtil as JwtAuthenticationUtil
participant Redis
participant DB as Database
Client->>Filter: HTTP Request (with JWT cookie/header)
Filter->>Filter: Check Origin, Set CORS headers
alt OPTIONS request
Filter-->>Client: 200 OK (CORS preflight)
else Other request
Filter->>JwtAuthUtil: Validate JWT token
JwtAuthUtil->>CookieUtil: Extract JWT from cookie
JwtAuthUtil->>JwtUtil: Validate token signature/claims
JwtAuthUtil->>Redis: Get User by userId
alt User not in Redis
JwtAuthUtil->>DB: Query User by userId
DB-->>JwtAuthUtil: User object
JwtAuthUtil->>Redis: Cache User object
end
JwtAuthUtil-->>Filter: Validation result
alt Valid
Filter->>Filter: Clear Authorization header
Filter->>Client: Continue filter chain (request proceeds)
else Invalid
Filter-->>Client: 401 Unauthorized
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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). (2)
✨ 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. 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: 26
🧹 Nitpick comments (20)
src/main/java/com/iemr/common/identity/utils/http/AuthorizationHeaderRequestWrapper.java (3)
10-42: Security consideration: Document intended usage.This wrapper can override authorization headers, which has security implications. Consider adding JavaDoc to document its intended usage and security considerations.
+/** + * A request wrapper that overrides the Authorization header with a provided value. + * This should only be used in controlled scenarios such as JWT token processing + * where the authorization header needs to be modified after validation. + * + * Security Note: This wrapper can override authorization headers and should be + * used with caution to prevent security vulnerabilities. + */ public class AuthorizationHeaderRequestWrapper extends HttpServletRequestWrapper {
26-32: Handle null authorization value consistently.The
getHeadersmethod returns a single-item enumeration even whenAuthorizationis null. Consider returning an empty enumeration when the authorization value is null for consistency.@Override public Enumeration<String> getHeaders(String name) { if ("Authorization".equalsIgnoreCase(name)) { - return Collections.enumeration(Collections.singletonList(Authorization)); + return Authorization != null ? + Collections.enumeration(Collections.singletonList(Authorization)) : + Collections.emptyEnumeration(); } return super.getHeaders(name); }
34-41: Consider consistency with null authorization values.The method adds "Authorization" to header names even when the authorization value is null. This might be misleading since
getHeader("Authorization")would return null in that case.@Override public Enumeration<String> getHeaderNames() { List<String> names = Collections.list(super.getHeaderNames()); - if (!names.contains("Authorization")) { + if (Authorization != null && !names.contains("Authorization")) { names.add("Authorization"); } return Collections.enumeration(names); }src/main/java/com/iemr/common/identity/utils/FilterConfig.java (2)
11-12: Consider centralizing CORS configuration to avoid duplication.The
cors.allowed-originsproperty is injected in bothCorsConfigandFilterConfig, creating potential maintenance issues. Consider:
- Centralized configuration: Create a
@ConfigurationPropertiesclass for CORS settings- Shared bean: Inject the CORS configuration as a bean rather than duplicating the property injection
Create a centralized CORS properties class:
@ConfigurationProperties(prefix = "cors") @Component public class CorsProperties { private String allowedOrigins; // getter and setter public String getAllowedOrigins() { return allowedOrigins; } public void setAllowedOrigins(String allowedOrigins) { this.allowedOrigins = allowedOrigins; } }
21-22: Consider narrowing the filter scope and documenting precedence implications.The filter applies to all URL patterns (
/*) with highest precedence, which may impact performance and other filters. Consider if this broad scope is necessary.Consider these alternatives:
- Apply filter only to protected endpoints:
/api/**- Document why highest precedence is required
- Consider excluding static resources and health endpoints
- registrationBean.setOrder(Ordered.HIGHEST_PRECEDENCE); - registrationBean.addUrlPatterns("/*"); // Apply filter to all API endpoints + registrationBean.setOrder(Ordered.HIGHEST_PRECEDENCE); // Required for JWT validation before security filters + registrationBean.addUrlPatterns("/api/*"); // Apply filter to API endpoints onlysrc/main/java/com/iemr/common/identity/utils/redis/RedisConfig.java (2)
21-33: Consider adding Redis connection configuration and error handling.The current configuration relies on default Redis connection settings. For production deployment, consider:
- Connection configuration: Redis host, port, password, SSL settings
- Connection pooling: Configure Lettuce or Jedis connection pool
- Timeouts: Set appropriate connection and read timeouts
- Error handling: Add Redis connection failure handling
Example configuration addition:
@Bean public LettuceConnectionFactory redisConnectionFactory() { RedisStandaloneConfiguration config = new RedisStandaloneConfiguration(); config.setHostName("${spring.redis.host:localhost}"); config.setPort("${spring.redis.port:6379}"); // Add password, SSL, etc. as needed LettucePoolingClientConfiguration poolConfig = LettucePoolingClientConfiguration.builder() .poolConfig(GenericObjectPoolConfig.defaultConfig()) .build(); return new LettuceConnectionFactory(config, poolConfig); }
16-17: Document the purpose of ConfigureRedisAction.NO_OP.The NO_OP configuration should be documented to explain why automatic Redis keyspace notifications are disabled.
@Bean + // Disable automatic Redis keyspace notifications configuration for performance public ConfigureRedisAction configureRedisAction() { return ConfigureRedisAction.NO_OP; }src/main/java/com/iemr/common/identity/utils/TokenDenylist.java (4)
14-14: Consider making the prefix configurable.The hard-coded prefix
"denied_"should be configurable for different environments or to avoid key collisions:-private static final String PREFIX = "denied_"; +@Value("${jwt.denylist.prefix:denied_}") +private String prefix; private String getKey(String jti) { - return PREFIX + jti; + return prefix + jti; }
23-36: Add retry mechanism for Redis operations resilience.Redis operations can fail due to network issues or temporary unavailability. Consider adding retry logic for critical operations like token denylisting:
Consider using Spring Retry or implementing a simple retry mechanism:
@Retryable(value = {Exception.class}, maxAttempts = 3, backoff = @Backoff(delay = 100)) public void addTokenToDenylist(String jti, Long expirationTime) { // existing implementation }This ensures better resilience for security-critical operations.
46-46: Optimize logging to avoid string concatenation.Use parameterized logging to improve performance and avoid unnecessary string concatenation.
- logger.error("Failed to check denylist status for jti: " + jti, e); + logger.error("Failed to check denylist status for jti: {}", jti, e);
32-32: Consider using a more meaningful value for denylisted tokens.Storing a single space character (" ") as the value is unclear. Consider using a more descriptive value or just "true".
- redisTemplate.opsForValue().set(key, " ", expirationTime, TimeUnit.MILLISECONDS); + redisTemplate.opsForValue().set(key, "denylisted", expirationTime, TimeUnit.MILLISECONDS);src/main/java/com/iemr/common/identity/utils/CookieUtil.java (1)
18-28: Simplify cookie retrieval using Java 8 streamsApply this diff for a more concise implementation:
public Optional<String> getCookieValue(HttpServletRequest request, String cookieName) { - Cookie[] cookies = request.getCookies(); - if (cookies != null) { - for (Cookie cookie : cookies) { - if (cookieName.equals(cookie.getName())) { - return Optional.of(cookie.getValue()); - } - } - } - return Optional.empty(); + if (request.getCookies() == null) { + return Optional.empty(); + } + return Arrays.stream(request.getCookies()) + .filter(cookie -> cookieName.equals(cookie.getName())) + .map(Cookie::getValue) + .findFirst(); }src/main/java/com/iemr/common/identity/domain/User.java (2)
136-138: Remove empty TODO commentsThe TODO comments in constructors serve no purpose and should be removed.
Apply this diff:
public User() { - // TODO Auto-generated constructor stub } public User(Integer userID, String userName) { - // TODO Auto-generated constructor stub this.userID=userID; this.userName=userName; }Also applies to: 140-144
51-52: Fix field naming to follow Java conventionsThe field name
pANviolates Java naming conventions. It should bepanin camelCase.Apply this diff:
@Column(name="PAN") -private String pAN; +private String pan;Note: This will require updates to any getters/setters generated by Lombok and any code referencing this field.
src/main/java/com/iemr/common/identity/utils/JwtUtil.java (1)
13-13: Remove unused importThe
ch.qos.logback.classic.Loggerimport is not used in this class.-import ch.qos.logback.classic.Logger;src/main/java/com/iemr/common/identity/utils/JwtAuthenticationUtil.java (1)
39-42: Remove redundant constructorThe constructor is unnecessary since the fields are already annotated with
@Autowired.-public JwtAuthenticationUtil(CookieUtil cookieUtil, JwtUtil jwtUtil) { - this.cookieUtil = cookieUtil; - this.jwtUtil = jwtUtil; -}src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (2)
93-93: Fix typo in log message-logger.info("Common-API incoming userAget : " + userAgent); +logger.info("Common-API incoming userAgent : " + userAgent);
41-48: Consider setting CORS headers after authenticationCurrently, CORS headers are set before JWT validation, which exposes your allowed origins configuration to unauthorized requests. Consider moving CORS header setting after successful authentication for enhanced security.
src/main/java/com/iemr/common/identity/utils/UserAgentContext.java (2)
6-8: Add input validation for user agent.The setter method should validate input to prevent storing null or problematic values.
public static void setUserAgent(String userAgent) { - userAgentHolder.set(userAgent); + userAgentHolder.set(userAgent != null ? userAgent.trim() : null); }
3-4: Document memory leak prevention responsibility.ThreadLocal usage requires proper cleanup to prevent memory leaks. Document that callers must ensure
clear()is called.+/** + * Thread-local context for storing user agent information. + * + * WARNING: Callers must ensure clear() is called to prevent memory leaks, + * typically in a finally block or filter cleanup. + */ public class UserAgentContext {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
pom.xml(1 hunks)src/main/environment/1097_ci.properties(1 hunks)src/main/environment/1097_docker.properties(1 hunks)src/main/environment/1097_example.properties(1 hunks)src/main/environment/common_ci.properties(1 hunks)src/main/environment/common_docker.properties(1 hunks)src/main/environment/common_example.properties(1 hunks)src/main/java/com/iemr/common/identity/config/CorsConfig.java(1 hunks)src/main/java/com/iemr/common/identity/controller/IdentityController.java(0 hunks)src/main/java/com/iemr/common/identity/controller/familyTagging/FamilyTaggingController.java(0 hunks)src/main/java/com/iemr/common/identity/controller/version/VersionController.java(0 hunks)src/main/java/com/iemr/common/identity/domain/User.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)src/main/java/com/iemr/common/identity/utils/TokenDenylist.java(1 hunks)src/main/java/com/iemr/common/identity/utils/UserAgentContext.java(1 hunks)src/main/java/com/iemr/common/identity/utils/http/AuthorizationHeaderRequestWrapper.java(1 hunks)src/main/java/com/iemr/common/identity/utils/http/HTTPRequestInterceptor.java(1 hunks)src/main/java/com/iemr/common/identity/utils/redis/RedisConfig.java(1 hunks)src/main/resources/application.properties(1 hunks)
💤 Files with no reviewable changes (3)
- src/main/java/com/iemr/common/identity/controller/version/VersionController.java
- src/main/java/com/iemr/common/identity/controller/familyTagging/FamilyTaggingController.java
- src/main/java/com/iemr/common/identity/controller/IdentityController.java
🧰 Additional context used
🧠 Learnings (3)
src/main/java/com/iemr/common/identity/utils/CookieUtil.java (2)
Learnt from: drtechie
PR: #87
File: src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java:23-84
Timestamp: 2025-04-10T05:08:55.537Z
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").
Learnt from: drtechie
PR: #87
File: src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java:23-84
Timestamp: 2025-04-10T05:08:55.537Z
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.
src/main/java/com/iemr/common/identity/domain/User.java (1)
Learnt from: ravishanigarapu
PR: #85
File: src/main/java/com/iemr/common/identity/repo/BenMappingRepo.java:125-138
Timestamp: 2025-02-17T07:29:05.121Z
Learning: In the Identity-API project, tables involved in beneficiary mapping queries (MBeneficiaryregidmapping, MBeneficiarycontact, MBeneficiarydetail, MBeneficiaryaddress, MBeneficiaryconsent, MBeneficiaryImage, MBeneficiaryAccount, MBeneficiaryidentity, MBeneficiaryfamilymapping, MBeneficiaryservicemapping) have indexes created on join columns (vanID, vanSerialNo) to optimize query performance.
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (1)
Learnt from: drtechie
PR: #87
File: src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java:23-84
Timestamp: 2025-04-10T05:08:55.537Z
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").
🧬 Code Graph Analysis (1)
src/main/java/com/iemr/common/identity/config/CorsConfig.java (1)
src/main/java/com/iemr/common/identity/utils/FilterConfig.java (1)
Configuration(9-26)
🪛 ast-grep (0.38.6)
src/main/java/com/iemr/common/identity/utils/CookieUtil.java
[warning] 37-37: 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] 37-37: 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] 150-150: 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 (17)
src/main/java/com/iemr/common/identity/utils/http/HTTPRequestInterceptor.java (2)
105-105: LGTM: Good defensive programming improvement.The enhanced null and empty check ensures that
sessionObject.updateSessionObject()is only called when there's a meaningful authorization value, preventing unnecessary processing of empty authorization headers.
105-105: Good defensive programming improvement.The additional empty string check prevents unnecessary session object updates when the authorization header is present but empty, which aligns well with the JWT implementation being introduced in this PR.
src/main/environment/common_example.properties (2)
19-19: LGTM: Standard Redis configuration.The Redis host configuration is appropriate for local development.
19-21: LGTM! Good configuration for local development.The Redis host configuration and CORS allowed origins with port wildcard are appropriate for local development environments. The
http://localhost:*pattern provides flexibility for testing on different ports.src/main/environment/common_docker.properties (2)
25-25: LGTM: Proper environment variable configuration.Using
${CORS_ALLOWED_ORIGINS}environment variable for CORS configuration is the correct approach for containerized deployments. This allows for proper environment-specific configuration without hardcoded values.
23-26: Excellent externalized configuration for Docker deployments.Using environment variables for Redis host and CORS allowed origins follows Docker best practices, allowing configuration flexibility without image rebuilds. The property names are consistent with other environment files.
src/main/resources/application.properties (2)
54-55: LGTM: Appropriate JWT token expiration settings.The JWT expiration configurations follow security best practices:
- 24 hours for access tokens provides good security with reasonable user experience
- 7 days for refresh tokens balances security with user convenience
The values are well-chosen and the property names are clear and consistent.
54-56: Appropriate JWT token expiration settings.The access token expiration of 24 hours and refresh token expiration of 7 days follow security best practices. These durations provide a good balance between security (shorter access token lifetime) and user experience (reasonable refresh token duration).
src/main/java/com/iemr/common/identity/utils/http/AuthorizationHeaderRequestWrapper.java (2)
18-24: LGTM: Proper header override implementation.The case-insensitive comparison and delegation pattern is implemented correctly.
10-16: Well-designed request wrapper for authorization header manipulation.The class properly extends
HttpServletRequestWrapperand follows the wrapper pattern correctly. The field is appropriately marked as final and the constructor properly delegates to the parent.src/main/environment/1097_docker.properties (1)
22-24: VerifyCORS_ALLOWED_ORIGINSis injected at runtime
The container image will crash at startup if${CORS_ALLOWED_ORIGINS}is not supplied. Double-check your Helm chart / docker-compose to make sure the env var is present, otherwise fall back to a sensible default.pom.xml (1)
223-242: JWT libs added – confirm compatibility & CVE status
jjwt-api/impl/jackson 0.12.6is the latest release and should work with Java 17, but make sure:
- No transitive pull-in of outdated
jackson-databindversions (CVE prone).spring-boot-starter-securityis on the class-path to avoid class-loading issues.- License (& crypto export) policies are acceptable for your organization.
If all three are checked, 👍🏻
src/main/java/com/iemr/common/identity/utils/FilterConfig.java (2)
22-22: FilterConfig URL mapping is safe with current endpoints
I’ve verified the codebase: the only exposed endpoints are/getByAbhaAddressand/getByAbhaIdNo(no health/actuator, static resources, or other public paths). Applying the JWT filter to/*therefore only affects secured API calls and does not introduce unnecessary processing. We can revisit and tighten this mapping if new public or management endpoints (static assets, Swagger-UI, actuator, etc.) are added in the future.
11-12: Good practice: Consistent property usage across components.The same
cors.allowed-originsproperty is used in bothCorsConfigandFilterConfig, ensuring consistent CORS handling.src/main/java/com/iemr/common/identity/utils/redis/RedisConfig.java (1)
28-30: Update deprecated Jackson2JsonRedisSerializer usage.
Jackson2JsonRedisSerializer<User>is deprecated in newer Spring Data Redis versions. Consider usingGenericJackson2JsonRedisSerializerfor better flexibility and future compatibility.-// Use Jackson2JsonRedisSerializer for values (Users objects) -Jackson2JsonRedisSerializer<User> serializer = new Jackson2JsonRedisSerializer<>(User.class); -template.setValueSerializer(serializer); +// Use GenericJackson2JsonRedisSerializer for values (flexible object serialization) +template.setValueSerializer(new GenericJackson2JsonRedisSerializer()); +template.setHashValueSerializer(new GenericJackson2JsonRedisSerializer());This change also makes the configuration more flexible for caching different object types beyond just
User.Likely an incorrect or invalid review comment.
src/main/java/com/iemr/common/identity/config/CorsConfig.java (1)
17-21: Ensure safe CORS origin patterns and document custom headersWe’ve located
cors.allowed-originsin your environment configs:
- src/main/environment/common_example.properties →
http://localhost:*- src/main/environment/common_ci.properties →
@env.CORS_ALLOWED_ORIGINS@- src/main/environment/1097_example.properties →
http://localhost:*- src/main/environment/1097_docker.properties →
${CORS_ALLOWED_ORIGINS}- src/main/environment/1097_ci.properties →
@env.CORS_ALLOWED_ORIGINS@- src/main/environment/common_docker.properties →
${CORS_ALLOWED_ORIGINS}Please verify that in production
CORS_ALLOWED_ORIGINSonly contains explicitly trusted domains (no overly broad wildcards) before allowing credentials. Also add documentation for the custom “Jwttoken” header so consumers understand its purpose.src/main/java/com/iemr/common/identity/utils/TokenDenylist.java (1)
19-21: Good practice: Consistent key prefixing.The helper method for key generation with prefix ensures consistent Redis key management.
| spring.redis.host=localhost | ||
| cors.allowed-origins=http://localhost:* |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Wildcard port in cors.allowed-origins is likely ignored by Spring
http://localhost:* is not interpreted as a valid wildcard by Spring’s CorsConfiguration#setAllowedOrigins, so no origin will actually match and CORS will still fail in local testing.
Suggested fix – either enumerate the concrete ports you use, or switch the Java config to setAllowedOriginPatterns and keep a wildcard there:
-cors.allowed-origins=http://localhost:*
+# Example: explicit ports
+cors.allowed-origins=http://localhost:3000,http://localhost:4200or update CorsConfig to rely on setAllowedOriginPatterns instead.
Fix CORS allowed-origins: wildcard port is ignored
Spring’s CorsConfiguration#setAllowedOrigins only matches exact origins. The pattern http://localhost:* will not match any origin, so CORS requests will still be blocked.
Please update src/main/environment/1097_example.properties (lines 20–21) using one of these options:
- Enumerate explicit ports:
- cors.allowed-origins=http://localhost:* + cors.allowed-origins=http://localhost:3000,http://localhost:4200
- Switch to origin patterns (requires Spring Framework 5.3+):
Or update your Java CORS config to call
- cors.allowed-origins=http://localhost:* + cors.allowed-origin-patterns=http://localhost:*
setAllowedOriginPatterns(...)instead ofsetAllowedOrigins(...).
📝 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.
| spring.redis.host=localhost | |
| cors.allowed-origins=http://localhost:* | |
| spring.redis.host=localhost | |
| cors.allowed-origins=http://localhost:3000,http://localhost:4200 |
| spring.redis.host=localhost | |
| cors.allowed-origins=http://localhost:* | |
| spring.redis.host=localhost | |
| cors.allowed-origin-patterns=http://localhost:* |
🤖 Prompt for AI Agents
In src/main/environment/1097_example.properties at lines 20-21, the CORS
allowed-origins setting uses a wildcard port (http://localhost:*), which
Spring's setAllowedOrigins does not support and causes CORS requests to be
blocked. Fix this by either listing explicit allowed origins with specific ports
instead of a wildcard or, if using Spring Framework 5.3 or newer, switch to
using origin patterns by updating the configuration to use
setAllowedOriginPatterns instead of setAllowedOrigins to allow wildcard ports.
| # Redis Config | ||
| spring.redis.host=localhost | ||
| spring.redis.host=localhost | ||
| cors.allowed-origins=http://localhost:* |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Security concern: Wildcard port in CORS configuration.
Using http://localhost:* allows cross-origin requests from any service running on any port on localhost. This could be a security risk as it permits requests from potentially malicious local services.
Consider specifying explicit ports or using environment-specific configurations instead of wildcards.
-cors.allowed-origins=http://localhost:*
+cors.allowed-origins=http://localhost:3000,http://localhost:4200Verify if this wildcard configuration is used in production environments:
🏁 Script executed:
#!/bin/bash
# Check if wildcard CORS origins are used in production configs
echo "Checking for wildcard CORS configurations in production-like environments..."
rg -n "cors\.allowed-origins.*\*" --type propertiesLength of output: 251
🏁 Script executed:
#!/bin/bash
echo "Searching for wildcard CORS origins in all .properties files..."
# Use glob filter instead of --type to include .properties files
rg -n "cors\.allowed-origins.*\*" -g '*.properties'Length of output: 368
Restrict wildcard ports in CORS configuration
Allowing http://localhost:* opens CORS to any local service on any port, which can be exploited by malicious processes. Please tighten these entries to only the necessary development ports or use environment-specific overrides.
Files to update:
src/main/environment/common_example.properties(line 20)src/main/environment/1097_example.properties(line 21)
Suggested change (example ports—adjust as needed):
-cors.allowed-origins=http://localhost:*
+cors.allowed-origins=http://localhost:3000,http://localhost:4200📝 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.
| cors.allowed-origins=http://localhost:* | |
| cors.allowed-origins=http://localhost:3000,http://localhost:4200 |
🤖 Prompt for AI Agents
In src/main/environment/common_example.properties at line 20, the CORS allowed
origins entry uses a wildcard port with http://localhost:*, which is insecure.
Replace the wildcard with specific allowed ports needed for development, such as
http://localhost:3000 or http://localhost:8080, to restrict access. Apply a
similar change in src/main/environment/1097_example.properties at line 21 to
ensure consistent and secure CORS configuration.
| registry.addMapping("/**") | ||
| .allowedOriginPatterns( | ||
| Arrays.stream(allowedOrigins.split(",")).map(String::trim).toArray(String[]::new)) | ||
| .allowedMethods("GET", "POST", "PUT", "DELETE", "OPTIONS").allowedHeaders("*") | ||
| .exposedHeaders("Authorization", "Jwttoken").allowCredentials(true).maxAge(3600); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Review CORS security configuration for production readiness.
The CORS configuration allows credentials with potentially dynamic origin patterns, which requires careful validation of the cors.allowed-origins property. Consider these security aspects:
- Origin validation: The comma-separated origins are trimmed but not validated for format
- Wildcard risk: If origins contain wildcards, ensure they're intentionally configured
- Custom header exposure: "Jwttoken" header exposure should be documented as part of the API contract
Verify the origin patterns are properly validated:
🏁 Script executed:
#!/bin/bash
# Check cors.allowed-origins configuration across environment files
echo "Checking CORS allowed origins configuration:"
fd -e properties -x grep -l "cors.allowed-origins" {} \; | head -10
echo
echo "Sample configurations:"
fd -e properties -x grep "cors.allowed-origins" {} \; | head -5Length of output: 794
Review CORS configuration and tighten origin patterns before production deployment
The application currently injects cors.allowed-origins via placeholders (@env.CORS_ALLOWED_ORIGINS@, ${CORS_ALLOWED_ORIGINS}) and defaults to wildcard patterns such as http://localhost:*. In production, this poses a security risk:
- Restrict origin patterns to explicit, trusted hostnames and ports (avoid
*in host or port). - Add runtime validation of each parsed origin (e.g., via
java.net.URIor a regex) and fail fast on malformed or overly broad entries. - Document the exposure of the custom
"Jwttoken"header in your API specification.
Files defining cors.allowed-origins in your repo:
• src/main/environment/common_docker.properties
• src/main/environment/1097_docker.properties
• src/main/environment/common_ci.properties
• src/main/environment/1097_ci.properties
• src/main/environment/1097_example.properties
• src/main/environment/common_example.properties
No code diffs provided; please incorporate the above validations and tighten your environment configurations.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/identity/config/CorsConfig.java around lines 17
to 21, the CORS configuration currently allows broad origin patterns including
wildcards, which is insecure for production. To fix this, replace wildcard
origin patterns with explicit, trusted hostnames and ports only. Add runtime
validation for each origin string parsed from the configuration using
java.net.URI or regex to ensure they are well-formed and not overly broad, and
fail fast if invalid entries are detected. Also, update environment property
files to restrict allowed origins accordingly and document the exposure of the
custom "Jwttoken" header in the API specification.
src/main/java/com/iemr/common/identity/utils/TokenDenylist.java
Outdated
Show resolved
Hide resolved
| } catch (Exception e) { | ||
| throw new RuntimeException("Failed to denylist token", e); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use more specific exception types for better error handling.
Throwing generic RuntimeException makes it difficult for calling code to handle specific Redis failures appropriately. Consider using more specific exceptions:
} catch (Exception e) {
- throw new RuntimeException("Failed to denylist token", e);
+ logger.error("Failed to denylist token with jti: {}", jti, e);
+ throw new TokenDenylistException("Failed to denylist token", e);
}Create a custom exception class for better error handling:
public class TokenDenylistException extends RuntimeException {
public TokenDenylistException(String message, Throwable cause) {
super(message, cause);
}
}🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/identity/utils/TokenDenylist.java around lines
33 to 35, replace the generic RuntimeException with a custom exception class
named TokenDenylistException to improve error specificity. Create the
TokenDenylistException class extending RuntimeException with a constructor
accepting a message and cause, then throw this custom exception instead of
RuntimeException in the catch block.
| public class UserAgentContext { | ||
| private static final ThreadLocal<String> userAgentHolder = new ThreadLocal<>(); | ||
|
|
||
| public static void setUserAgent(String userAgent) { | ||
| userAgentHolder.set(userAgent); | ||
| } | ||
|
|
||
| public static String getUserAgent() { | ||
| return userAgentHolder.get(); | ||
| } | ||
|
|
||
| public static void clear() { | ||
| userAgentHolder.remove(); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Document ThreadLocal memory leak prevention.
ThreadLocal variables can cause memory leaks in long-running applications if not properly cleared. Ensure the calling code (like JwtUserIdValidationFilter) always calls clear() in a finally block.
Verify that the filter properly clears the context:
Consider adding documentation about proper usage patterns to prevent memory leaks.
🏁 Script executed:
#!/bin/bash
# Check if JwtUserIdValidationFilter properly clears UserAgentContext
echo "Checking UserAgentContext usage in filters:"
ast-grep --pattern $'class $_ {
$$$
UserAgentContext.clear()
$$$
}'
echo
echo "Looking for try-finally patterns with UserAgentContext:"
rg -A 10 -B 5 "UserAgentContext" --type javaLength of output: 3786
Add Javadoc to document ThreadLocal usage in UserAgentContext
We've confirmed that JwtUserIdValidationFilter correctly wraps set/clear in a try…finally. To make this pattern clear and prevent future leaks, add class-level Javadoc to UserAgentContext explaining the required usage:
• File: src/main/java/com/iemr/common/identity/utils/UserAgentContext.java
• Location: Immediately above the public class UserAgentContext declaration
Suggested diff:
+/**
+ * Holds the User-Agent header for the current thread.
+ * <p>
+ * IMPORTANT: To avoid ThreadLocal memory leaks in long-running environments,
+ * callers must wrap usage in a try/finally:
+ * <pre>
+ * try {
+ * UserAgentContext.setUserAgent(request.getHeader("User-Agent"));
+ * filterChain.doFilter(request, response);
+ * } finally {
+ * UserAgentContext.clear();
+ * }
+ * </pre>
+ */
public class UserAgentContext {
private static final ThreadLocal<String> userAgentHolder = new ThreadLocal<>();
…
}📝 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.
| public class UserAgentContext { | |
| private static final ThreadLocal<String> userAgentHolder = new ThreadLocal<>(); | |
| public static void setUserAgent(String userAgent) { | |
| userAgentHolder.set(userAgent); | |
| } | |
| public static String getUserAgent() { | |
| return userAgentHolder.get(); | |
| } | |
| public static void clear() { | |
| userAgentHolder.remove(); | |
| } | |
| } | |
| /** | |
| * Holds the User-Agent header for the current thread. | |
| * <p> | |
| * IMPORTANT: To avoid ThreadLocal memory leaks in long-running environments, | |
| * callers must wrap usage in a try/finally: | |
| * <pre> | |
| * try { | |
| * UserAgentContext.setUserAgent(request.getHeader("User-Agent")); | |
| * filterChain.doFilter(request, response); | |
| * } finally { | |
| * UserAgentContext.clear(); | |
| * } | |
| * </pre> | |
| */ | |
| public class UserAgentContext { | |
| private static final ThreadLocal<String> userAgentHolder = new ThreadLocal<>(); | |
| public static void setUserAgent(String userAgent) { | |
| userAgentHolder.set(userAgent); | |
| } | |
| public static String getUserAgent() { | |
| return userAgentHolder.get(); | |
| } | |
| public static void clear() { | |
| userAgentHolder.remove(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/identity/utils/UserAgentContext.java at lines 3
to 17, add a class-level Javadoc comment immediately above the public class
UserAgentContext declaration. The Javadoc should explain that this class uses a
ThreadLocal to store user agent information and emphasize the importance of
setting and clearing the ThreadLocal value properly, ideally in a try-finally
block, to prevent memory leaks and ensure thread safety.
src/main/java/com/iemr/common/identity/utils/UserAgentContext.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/main/java/com/iemr/common/identity/utils/CookieUtil.java (1)
28-37: Add SameSite attribute for CSRF protectionThe cookie is missing the SameSite attribute, which is crucial for CSRF protection. Based on the project's security requirements, this should be set to either "Strict" or "Lax".
Apply this diff to add SameSite protection:
public void addJwtTokenToCookie(String Jwttoken, HttpServletResponse response, HttpServletRequest request) { Cookie cookie = new Cookie("Jwttoken", Jwttoken); cookie.setHttpOnly(true); cookie.setMaxAge(60 * 60 * 24); cookie.setPath("/"); if ("https".equalsIgnoreCase(request.getScheme())) { cookie.setSecure(true); } + cookie.setAttribute("SameSite", "Strict"); response.addCookie(cookie); }Note: For older servlet versions, you might need to use:
response.setHeader("Set-Cookie", response.getHeader("Set-Cookie") + "; SameSite=Strict");src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (1)
111-124: Fix wildcard-port handling in isOriginAllowedThe special-case replacement for "http://localhost:.*" is applied after escaping dots, causing it to never match and fail localhost port wildcard handling.
Apply this diff to fix the replacement order:
private boolean isOriginAllowed(String origin) { if (origin == null || allowedOrigins == null || allowedOrigins.trim().isEmpty()) { logger.warn("No allowed origins configured or origin is null"); return false; } return Arrays.stream(allowedOrigins.split(",")).map(String::trim).anyMatch(pattern -> { - String regex = pattern.replace(".", "\\.").replace("*", ".*").replace("http://localhost:.*", - "http://localhost:\\d+"); // special case for wildcard port + String regex; + if (pattern.equals("http://localhost:.*")) { + regex = "http://localhost:\\d+"; + } else { + regex = pattern.replace(".", "\\.").replace("*", ".*"); + } - boolean matched = origin.matches(regex); + boolean matched = origin.matches("^" + regex + "$"); return matched; }); }src/main/java/com/iemr/common/identity/utils/TokenDenylist.java (2)
25-42: Good implementation of token denylisting.The method properly validates inputs, handles errors with the custom exception, and includes appropriate logging.
44-55: Excellent error handling improvement.The method now throws
TokenDenylistExceptionon Redis failures instead of silently returning false, which addresses the security concern from the previous review about failing securely.
🧹 Nitpick comments (3)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (1)
89-100: Consider expanding mobile client detectionThe current implementation only detects "okhttp" clients. Consider supporting other common mobile user agents.
private boolean isMobileClient(String userAgent) { if (userAgent == null) return false; userAgent = userAgent.toLowerCase(); - return userAgent.contains("okhttp"); // iOS (custom clients) + return userAgent.contains("okhttp") || + userAgent.contains("android") || + userAgent.contains("ios") || + userAgent.contains("mobile"); }src/main/java/com/iemr/common/identity/utils/exception/TokenDenylistException.java (1)
3-7: Add serialVersionUID and additional constructors for completeness.Exception classes should include a
serialVersionUIDfor serialization compatibility and provide standard constructors for flexibility.+import java.io.Serial; + +/** + * Exception thrown when token denylist operations fail. + */ public class TokenDenylistException extends RuntimeException { + @Serial + private static final long serialVersionUID = 1L; + + public TokenDenylistException(String message) { + super(message); + } + public TokenDenylistException(String message, Throwable cause) { super(message, cause); } + + public TokenDenylistException(Throwable cause) { + super(cause); + } }src/main/java/com/iemr/common/identity/utils/TokenDenylist.java (1)
14-14: Consider making the logger static final.The logger instance should be static to avoid creating multiple logger instances per object instance.
- private final Logger logger = LoggerFactory.getLogger(this.getClass().getName()); + private static final Logger logger = LoggerFactory.getLogger(TokenDenylist.class);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/environment/1097_ci.properties(1 hunks)src/main/environment/common_ci.properties(1 hunks)src/main/java/com/iemr/common/identity/config/CorsConfig.java(1 hunks)src/main/java/com/iemr/common/identity/utils/CookieUtil.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)src/main/java/com/iemr/common/identity/utils/TokenDenylist.java(1 hunks)src/main/java/com/iemr/common/identity/utils/UserAgentContext.java(1 hunks)src/main/java/com/iemr/common/identity/utils/exception/TokenDenylistException.java(1 hunks)src/main/java/com/iemr/common/identity/utils/http/AuthorizationHeaderRequestWrapper.java(1 hunks)src/main/java/com/iemr/common/identity/utils/redis/RedisConfig.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/iemr/common/identity/utils/UserAgentContext.java
🚧 Files skipped from review as they are similar to previous changes (7)
- src/main/environment/common_ci.properties
- src/main/java/com/iemr/common/identity/config/CorsConfig.java
- src/main/environment/1097_ci.properties
- src/main/java/com/iemr/common/identity/utils/redis/RedisConfig.java
- src/main/java/com/iemr/common/identity/utils/JwtAuthenticationUtil.java
- src/main/java/com/iemr/common/identity/utils/JwtUtil.java
- src/main/java/com/iemr/common/identity/utils/http/AuthorizationHeaderRequestWrapper.java
🧰 Additional context used
🧠 Learnings (2)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (2)
Learnt from: drtechie
PR: #87
File: src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java:23-84
Timestamp: 2025-04-10T05:08:55.537Z
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").
Learnt from: drtechie
PR: #87
File: src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java:23-84
Timestamp: 2025-04-10T05:08:55.537Z
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.
src/main/java/com/iemr/common/identity/utils/CookieUtil.java (2)
Learnt from: drtechie
PR: #87
File: src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java:23-84
Timestamp: 2025-04-10T05:08:55.537Z
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").
Learnt from: drtechie
PR: #87
File: src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java:23-84
Timestamp: 2025-04-10T05:08:55.537Z
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.
🧬 Code Graph Analysis (2)
src/main/java/com/iemr/common/identity/utils/TokenDenylist.java (2)
src/main/java/com/iemr/common/identity/utils/JwtUtil.java (1)
Component(20-167)src/main/java/com/iemr/common/identity/utils/exception/TokenDenylistException.java (1)
TokenDenylistException(3-7)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (2)
src/main/java/com/iemr/common/identity/utils/http/AuthorizationHeaderRequestWrapper.java (1)
AuthorizationHeaderRequestWrapper(10-44)src/main/java/com/iemr/common/identity/utils/UserAgentContext.java (1)
UserAgentContext(3-20)
🪛 ast-grep (0.38.6)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java
[warning] 151-151: 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/CookieUtil.java
[warning] 35-35: 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] 35-35: 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)
⏰ 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_and_analyse
- GitHub Check: Analyze (java)
🔇 Additional comments (3)
src/main/java/com/iemr/common/identity/utils/CookieUtil.java (2)
16-26: LGTM!Good use of Optional for null safety.
39-41: LGTM!Clean implementation reusing the getCookieValue method.
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (1)
145-153: LGTM!Proper cookie clearing implementation with all necessary security attributes including SameSite.
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java
Show resolved
Hide resolved
| @Autowired | ||
| private RedisTemplate<String, Object> redisTemplate; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Redis configuration for production use.
Ensure that Redis connection pooling, timeout settings, and error recovery mechanisms are properly configured in RedisConfig.java for production workloads.
🏁 Script executed:
#!/bin/bash
# Check Redis configuration for connection pooling and timeout settings
ast-grep --pattern 'class RedisConfig {
$$$
}'
# Look for Redis connection factory configuration
rg -A 10 "JedisConnectionFactory|LettuceConnectionFactory"
# Check for Redis properties in configuration files
fd -e properties -e yml -e yaml --exec grep -l "redis\." {} \;Length of output: 4212
Configure Redis connection factory for production workloads
The connectionFactory() method in src/main/java/com/iemr/common/identity/utils/IEMRApplBeans.java currently returns a plain new LettuceConnectionFactory(redisHost, redisPort), which relies on defaults. For a production‐ready setup, you should:
- Extract Redis beans into a dedicated
@Configurationclass (e.g.RedisConfig.java). - Build a
LettuceClientConfiguration(orLettucePoolingClientConfiguration) to set:- Command and shutdown timeouts (
commandTimeout(Duration…),shutdownTimeout(Duration…)) - Connection‐pool parameters via
GenericObjectPoolConfig(e.g.maxTotal,maxIdle,minIdle,maxWait)
- Command and shutdown timeouts (
- Wire a
RedisStandaloneConfiguration(host/port/SSL/password) into the factory. - Optionally wrap operations in retry/backoff or a circuit-breaker to handle transient failures.
Without explicit pooling, timeouts and error-recovery, you risk connection exhaustion, unbounded resource usage or hung commands under load.
| // Remove a token's jti from the denylist (Redis) | ||
| public void removeTokenFromDenylist(String jti) { | ||
| if (jti != null && !jti.trim().isEmpty()) { | ||
| String key = getKey(jti); | ||
| redisTemplate.delete(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and logging to removeTokenFromDenylist.
The method lacks error handling for Redis failures and doesn't log the operation, making it inconsistent with other methods in the class.
- // Remove a token's jti from the denylist (Redis)
+ /**
+ * Remove a token's jti from the denylist (Redis)
+ * @param jti the token identifier to remove
+ * @throws TokenDenylistException if removal fails
+ */
public void removeTokenFromDenylist(String jti) {
if (jti != null && !jti.trim().isEmpty()) {
- String key = getKey(jti);
- redisTemplate.delete(key);
+ try {
+ String key = getKey(jti);
+ redisTemplate.delete(key);
+ logger.debug("Removed jti from denylist: {}", jti);
+ } catch (Exception e) {
+ logger.error("Failed to remove token from denylist with jti: {}", jti, e);
+ throw new TokenDenylistException("Failed to remove token from denylist", e);
+ }
+ } else {
+ logger.warn("Attempted to remove null or empty jti from denylist");
}
}📝 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.
| // Remove a token's jti from the denylist (Redis) | |
| public void removeTokenFromDenylist(String jti) { | |
| if (jti != null && !jti.trim().isEmpty()) { | |
| String key = getKey(jti); | |
| redisTemplate.delete(key); | |
| } | |
| } | |
| /** | |
| * Remove a token's jti from the denylist (Redis) | |
| * @param jti the token identifier to remove | |
| * @throws TokenDenylistException if removal fails | |
| */ | |
| public void removeTokenFromDenylist(String jti) { | |
| if (jti != null && !jti.trim().isEmpty()) { | |
| try { | |
| String key = getKey(jti); | |
| redisTemplate.delete(key); | |
| logger.debug("Removed jti from denylist: {}", jti); | |
| } catch (Exception e) { | |
| logger.error("Failed to remove token from denylist with jti: {}", jti, e); | |
| throw new TokenDenylistException("Failed to remove token from denylist", e); | |
| } | |
| } else { | |
| logger.warn("Attempted to remove null or empty jti from denylist"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/identity/utils/TokenDenylist.java around lines
57 to 63, the removeTokenFromDenylist method lacks error handling and logging.
Add a try-catch block around the Redis delete operation to catch any exceptions,
log an error message if an exception occurs, and also add an info-level log
before attempting to delete the token to indicate the operation is starting.
This will make the method consistent with other methods in the class.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (1)
113-126: Fix wildcard-port handling in isOriginAllowed (duplicate issue).The special-case replacement for
"http://localhost:.*"is applied after escaping dots, causing it to never match and fail localhost port wildcard handling. This is the same issue identified in previous reviews.The replacement chain:
pattern.replace(".", "\\.") // turns "http://localhost:.*" into "http://localhost:\\.*" .replace("*", ".*") // → "http://localhost:\\..*" .replace("http://localhost:.*", "http://localhost:\\d+") // no match, never appliedMove the localhost replacement before escaping:
-String regex = pattern.replace(".", "\\.").replace("*", ".*").replace("http://localhost:.*", - "http://localhost:\\d+"); +String regex = pattern.replace("http://localhost:.*", "http://localhost:\\d+") + .replace(".", "\\.").replace("*", ".*"); -boolean matched = origin.matches(regex); +boolean matched = origin.matches("^" + regex + "$");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java(1 hunks)src/main/java/com/iemr/common/identity/utils/TokenDenylist.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/iemr/common/identity/utils/TokenDenylist.java
🧰 Additional context used
🧠 Learnings (1)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (2)
Learnt from: drtechie
PR: #87
File: src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java:23-84
Timestamp: 2025-04-10T05:08:55.537Z
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").
Learnt from: drtechie
PR: #87
File: src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java:23-84
Timestamp: 2025-04-10T05:08:55.537Z
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.
🪛 ast-grep (0.38.6)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java
[warning] 145-145: 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)
🪛 GitHub Actions: Package
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java
[warning] Method doFilter length is 78 lines (max allowed is 50).
🪛 GitHub Actions: Call Checkstyle
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java
[warning] 26-129: Method doFilter length is 78 lines (max allowed is 50); lines longer than 120 characters; 'if' constructs must use braces.
🪛 GitHub Actions: Build and Static Code Analysis On Pull Request
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java
[warning] 22-129: Variable separation, line length, and method length warnings.
🪛 GitHub Actions: CodeQL
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java
[warning] 32-109: Method length exceeds maximum allowed (78 lines vs 50). Multiple line length and brace usage warnings. [MethodLength, LineLength, NeedBraces]
🔇 Additional comments (4)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (4)
20-30: LGTM - Well-structured class with proper dependency injection.The class declaration, fields, and constructor follow good practices with proper dependency injection and immutable fields.
128-133: LGTM - Clean mobile client detection logic.The method properly handles null checks and has clear logic for detecting mobile clients.
135-137: LGTM - Code duplication addressed.The method now properly delegates to
CookieUtil, addressing the previous code duplication concern.
139-147: LGTM - Proper security attributes including SameSite.The cookie clearing method correctly sets all security attributes including
SameSite=Strict, which addresses CSRF protection requirements mentioned in the retrieved learnings.
| @Override | ||
| public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain) | ||
| throws IOException, ServletException { | ||
| HttpServletRequest request = (HttpServletRequest) servletRequest; | ||
| HttpServletResponse response = (HttpServletResponse) servletResponse; | ||
|
|
||
| String origin = request.getHeader("Origin"); | ||
|
|
||
| logger.debug("Incoming Origin: {}", origin); | ||
| logger.debug("Allowed Origins Configured: {}", allowedOrigins); | ||
|
|
||
| if (origin != null && isOriginAllowed(origin)) { | ||
| response.setHeader("Access-Control-Allow-Origin", origin); | ||
| response.setHeader("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS"); | ||
| response.setHeader("Access-Control-Allow-Headers", "Authorization, Content-Type, Accept, Jwttoken"); | ||
| response.setHeader("Access-Control-Allow-Credentials", "true"); | ||
| } else { | ||
| logger.warn("Origin [{}] is NOT allowed. CORS headers NOT added.", origin); | ||
| } | ||
|
|
||
| if ("OPTIONS".equalsIgnoreCase(request.getMethod())) { | ||
| logger.info("OPTIONS request - skipping JWT validation"); | ||
| response.setStatus(HttpServletResponse.SC_OK); | ||
| return; | ||
| } | ||
|
|
||
| String path = request.getRequestURI(); | ||
| logger.info("JwtUserIdValidationFilter invoked for path: " + path); | ||
|
|
||
| // Log cookies for debugging | ||
| Cookie[] cookies = request.getCookies(); | ||
| if (cookies != null) { | ||
| for (Cookie cookie : cookies) { | ||
| if ("userId".equalsIgnoreCase(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 | ||
| logger.info("JWT token from header: "); | ||
|
|
||
| try { | ||
| String jwtFromCookie = getJwtTokenFromCookies(request); | ||
| String jwtFromHeader = request.getHeader("JwtToken"); | ||
| String authHeader = request.getHeader("Authorization"); | ||
| String jwtToken = jwtFromCookie != null ? jwtFromCookie : jwtFromHeader; | ||
| if (jwtToken != null) { | ||
| logger.info("Validating JWT token from cookie"); | ||
| if (jwtAuthenticationUtil.validateUserIdAndJwtToken(jwtToken)) { | ||
| AuthorizationHeaderRequestWrapper authorizationHeaderRequestWrapper = new AuthorizationHeaderRequestWrapper( | ||
| request, ""); | ||
| filterChain.doFilter(authorizationHeaderRequestWrapper, servletResponse); | ||
| return; | ||
| } | ||
| } else { | ||
| String userAgent = request.getHeader("User-Agent"); | ||
| logger.info("User-Agent: " + userAgent); | ||
| if (userAgent != null && isMobileClient(userAgent) && authHeader != null) { | ||
| try { | ||
| logger.info("Common-API incoming userAget : " + userAgent); | ||
| UserAgentContext.setUserAgent(userAgent); | ||
| filterChain.doFilter(servletRequest, servletResponse); | ||
| } finally { | ||
| UserAgentContext.clear(); | ||
| } | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| logger.warn("No valid authentication token found"); | ||
| response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Unauthorized: Invalid or missing token"); | ||
| } catch (Exception e) { | ||
| logger.error("Authorization error: ", e); | ||
| response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Authorization error: " + e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Extract methods to reduce complexity and fix pipeline violations.
The doFilter method is 78 lines long (exceeds 50-line limit) and handles multiple responsibilities, violating single responsibility principle and causing pipeline failures.
Consider extracting the following methods:
handleCorsHeaders(HttpServletRequest request, HttpServletResponse response)handleOptionsRequest(HttpServletResponse response)inspectAndClearCookies(HttpServletRequest request, HttpServletResponse response)handleJwtAuthentication(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)handleMobileClientAuthentication(HttpServletRequest request, ServletResponse servletResponse, FilterChain filterChain)
@Override
public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain)
throws IOException, ServletException {
HttpServletRequest request = (HttpServletRequest) servletRequest;
HttpServletResponse response = (HttpServletResponse) servletResponse;
- String origin = request.getHeader("Origin");
- logger.debug("Incoming Origin: {}", origin);
- logger.debug("Allowed Origins Configured: {}", allowedOrigins);
-
- if (origin != null && isOriginAllowed(origin)) {
- response.setHeader("Access-Control-Allow-Origin", origin);
- response.setHeader("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS");
- response.setHeader("Access-Control-Allow-Headers", "Authorization, Content-Type, Accept, Jwttoken");
- response.setHeader("Access-Control-Allow-Credentials", "true");
- } else {
- logger.warn("Origin [{}] is NOT allowed. CORS headers NOT added.", origin);
- }
+ handleCorsHeaders(request, response);
- if ("OPTIONS".equalsIgnoreCase(request.getMethod())) {
- logger.info("OPTIONS request - skipping JWT validation");
- response.setStatus(HttpServletResponse.SC_OK);
- return;
- }
+ if (handleOptionsRequest(response, request.getMethod())) {
+ return;
+ }
String path = request.getRequestURI();
logger.info("JwtUserIdValidationFilter invoked for path: " + path);
- // Cookie inspection and clearing logic
+ inspectAndClearCookies(request, response);
try {
- // JWT and mobile authentication logic
+ if (!handleJwtAuthentication(request, response, filterChain)) {
+ handleMobileClientAuthentication(request, servletResponse, filterChain);
+ }
} catch (Exception e) {
logger.error("Authorization error: ", e);
response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Authorization error: " + e.getMessage());
}
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Package
[warning] Method doFilter length is 78 lines (max allowed is 50).
🪛 GitHub Actions: CodeQL
[warning] 32-109: Method length exceeds maximum allowed (78 lines vs 50). Multiple line length and brace usage warnings. [MethodLength, LineLength, NeedBraces]
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java
from lines 32 to 111, the doFilter method is too long and handles multiple
responsibilities, causing complexity and pipeline failures. Refactor by
extracting the following methods: handleCorsHeaders to set CORS headers based on
origin, handleOptionsRequest to respond to OPTIONS requests,
inspectAndClearCookies to log and clear userId cookies, handleJwtAuthentication
to validate JWT tokens and proceed with the filter chain, and
handleMobileClientAuthentication to handle mobile client authentication logic.
Replace the corresponding code blocks in doFilter with calls to these new
methods to reduce method length and improve clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (1)
123-129: Wildcard-localhost replacement still executed too late – regex never matches
pattern.replace(".", "\\.")runs before the special-casereplace("http://localhost:.*", …)so the literal dots are already escaped and the second replacement never triggers.
The filter therefore rejects legitimatehttp://localhost:<port>origins and the earlier issue reported in PR #87 survives unchanged.-String regex = pattern.replace(".", "\\.") - .replace("*", ".*") - .replace("http://localhost:.*", "http://localhost:\\d+"); +String regex; +if ("http://localhost:.*".equals(pattern)) { + // wildcard port – allow any numeric port + regex = "http://localhost:\\d+"; +} else { + regex = pattern.replace(".", "\\.") + .replace("*", ".*"); +} +// Anchor to avoid partial matches +return origin.matches("^" + regex + "$");
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (1)
61-67: Debug log message misleading
logger.info("JWT token from header: ");logs nothing and may confuse operators. Either log presence/absence (without the token value) or remove the line.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (2)
Learnt from: drtechie
PR: #87
File: src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java:23-84
Timestamp: 2025-04-10T05:08:55.537Z
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").
Learnt from: drtechie
PR: #87
File: src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java:23-84
Timestamp: 2025-04-10T05:08:55.537Z
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.
🪛 ast-grep (0.38.6)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java
[warning] 149-149: 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)
⏰ 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_and_analyse
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (1)
78-84: Mobile-client detection is brittleRelying on
User-Agentcontaining “okhttp” will miss many legitimate mobile clients and can be spoofed trivially. Consider an explicit header or token exchange instead of UA sniffing.
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/common/identity/utils/redis/RedisConfig.java(1 hunks)
⏰ 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: Analyze (java)
- GitHub Check: Build_and_analyse
🔇 Additional comments (1)
src/main/java/com/iemr/common/identity/utils/redis/RedisConfig.java (1)
18-21: LGTM!The
ConfigureRedisAction.NO_OPconfiguration is appropriate for disabling automatic Redis keyspace notifications, which is a common pattern when notifications aren't required or the Redis server doesn't support them.
| @Bean | ||
| public RedisTemplate<String, Object> redisTemplate(RedisConnectionFactory factory) { | ||
| RedisTemplate<String, Object> template = new RedisTemplate<>(); | ||
| template.setConnectionFactory(factory); | ||
|
|
||
| Jackson2JsonRedisSerializer<User> serializer = new Jackson2JsonRedisSerializer<>(User.class); | ||
| ObjectMapper mapper = new ObjectMapper(); | ||
| mapper.registerModule(new JavaTimeModule()); | ||
| mapper.disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS); | ||
| serializer.setObjectMapper(mapper); | ||
| template.setValueSerializer(serializer); | ||
|
|
||
| return template; | ||
| } |
There was a problem hiding this comment.
Incomplete Redis serializer configuration.
The RedisTemplate configuration is missing key serializers and hash serializers, which can lead to functionality and security issues:
- Missing key serializer: Without explicit key serialization, Redis uses the default
JdkSerializationRedisSerializer, making keys unreadable and potentially causing security issues. - Missing hash serializers: Hash operations will use default serialization, causing inconsistency.
- Security concern: The previous review suggested security configurations that aren't fully implemented.
Jackson2JsonRedisSerializer<User> serializer = new Jackson2JsonRedisSerializer<>(User.class);
ObjectMapper mapper = new ObjectMapper();
mapper.registerModule(new JavaTimeModule());
mapper.disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS);
+ mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
serializer.setObjectMapper(mapper);
+
+ // Configure key serializer
+ template.setKeySerializer(new StringRedisSerializer());
template.setValueSerializer(serializer);
+
+ // Configure hash serializers
+ template.setHashKeySerializer(new StringRedisSerializer());
+ template.setHashValueSerializer(serializer);Note: Import com.fasterxml.jackson.databind.DeserializationFeature is needed.
📝 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.
| @Bean | |
| public RedisTemplate<String, Object> redisTemplate(RedisConnectionFactory factory) { | |
| RedisTemplate<String, Object> template = new RedisTemplate<>(); | |
| template.setConnectionFactory(factory); | |
| Jackson2JsonRedisSerializer<User> serializer = new Jackson2JsonRedisSerializer<>(User.class); | |
| ObjectMapper mapper = new ObjectMapper(); | |
| mapper.registerModule(new JavaTimeModule()); | |
| mapper.disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS); | |
| serializer.setObjectMapper(mapper); | |
| template.setValueSerializer(serializer); | |
| return template; | |
| } | |
| @Bean | |
| public RedisTemplate<String, Object> redisTemplate(RedisConnectionFactory factory) { | |
| RedisTemplate<String, Object> template = new RedisTemplate<>(); | |
| template.setConnectionFactory(factory); | |
| Jackson2JsonRedisSerializer<User> serializer = new Jackson2JsonRedisSerializer<>(User.class); | |
| ObjectMapper mapper = new ObjectMapper(); | |
| mapper.registerModule(new JavaTimeModule()); | |
| mapper.disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS); | |
| mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false); | |
| serializer.setObjectMapper(mapper); | |
| // Configure key serializer | |
| template.setKeySerializer(new StringRedisSerializer()); | |
| template.setValueSerializer(serializer); | |
| // Configure hash serializers | |
| template.setHashKeySerializer(new StringRedisSerializer()); | |
| template.setHashValueSerializer(serializer); | |
| return template; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/identity/utils/redis/RedisConfig.java around
lines 23 to 36, the RedisTemplate bean configuration lacks explicit key and hash
serializers, which can cause unreadable keys and inconsistent hash
serialization. To fix this, set the key serializer to StringRedisSerializer and
also set the hash key and hash value serializers appropriately, for example
using StringRedisSerializer for hash keys and Jackson2JsonRedisSerializer for
hash values. Additionally, configure the ObjectMapper to disable
DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES for safer deserialization.
Ensure all these serializers are set on the RedisTemplate before returning it.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (1)
119-132: Fix wildcard port handling regression in isOriginAllowedThe regex replacement order issue identified in previous reviews still exists. The special case for
http://localhost:.*never matches because dots are escaped before this replacement occurs.Apply this fix to handle the special case before other replacements:
private boolean isOriginAllowed(String origin) { if (origin == null || allowedOrigins == null || allowedOrigins.trim().isEmpty()) { logger.warn("No allowed origins configured or origin is null"); return false; } return Arrays.stream(allowedOrigins.split(",")).map(String::trim).anyMatch(pattern -> { - String regex = pattern.replace(".", "\\.").replace("*", ".*").replace("http://localhost:.*", - "http://localhost:\\d+"); // special case for wildcard port + String regex; + if (pattern.equals("http://localhost:.*")) { + regex = "http://localhost:\\d+"; + } else { + regex = pattern.replace(".", "\\.").replace("*", ".*"); + } - boolean matched = origin.matches(regex); + boolean matched = origin.matches("^" + regex + "$"); return matched; }); }Additionally, anchor the regex with
^and$to prevent partial matches.
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (1)
134-139: Correct misleading comment about okhttpThe comment mentions "iOS (custom clients)" but okhttp is primarily used by Android applications, not iOS.
private boolean isMobileClient(String userAgent) { if (userAgent == null) return false; userAgent = userAgent.toLowerCase(); - return userAgent.contains("okhttp"); // iOS (custom clients) + return userAgent.contains("okhttp"); // Android HTTP client }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (2)
Learnt from: drtechie
PR: #87
File: src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java:23-84
Timestamp: 2025-04-10T05:08:55.537Z
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").
Learnt from: drtechie
PR: #87
File: src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java:23-84
Timestamp: 2025-04-10T05:08:55.537Z
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.
🪛 ast-grep (0.38.6)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java
[warning] 151-151: 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)
⏰ 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: Analyze (java)
- GitHub Check: Build_and_analyse
🔇 Additional comments (2)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (2)
141-143: Good use of CookieUtil delegationThis implementation properly delegates to CookieUtil as suggested in previous reviews, avoiding code duplication.
145-153: Secure cookie clearing implementationThe implementation properly sets all security attributes including SameSite as recommended in previous reviews and the retrieved learnings.
|
2 participants
📋 Description
JIRA ID: AMM-1187
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
New Features
Configuration
Improvements