Release 3.6.0 into main #57
Conversation
* fix: add useragent java to skip the token * fix: update the log
fix: amm-1927 res headers based on allowed origins
Release 3.4.0
📝 WalkthroughWalkthroughThe project version is bumped to 3.6.0. CORS configuration now explicitly allows the PATCH HTTP method and replaces wildcard header matching with a specific header list. JWT filter and HTTP interceptor implement origin-based validation with conditional CORS header application and explicit preflight OPTIONS request handling. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Filter as JwtUserIdValidationFilter
participant Interceptor as HTTPRequestInterceptor
participant Config as CorsConfig
Client->>Filter: Request with Origin header
alt OPTIONS Preflight Request
Filter->>Filter: Check if origin is non-null
alt Origin missing
Filter->>Client: 403 Forbidden
else Origin provided
Filter->>Filter: Validate origin against allowed list
alt Origin allowed
Filter->>Filter: addCorsHeaders() with<br/>specific origin
Filter->>Client: 200 OK + CORS headers
else Origin not allowed
Filter->>Client: 403 Forbidden
end
end
else Regular Request (GET/POST/PUT/PATCH/DELETE)
Filter->>Filter: Check if origin is non-null
alt Origin present and not allowed
Filter->>Client: 403 Forbidden
else Origin allowed or absent
Filter->>Filter: addCorsHeaders()<br/>(if origin present)
Filter->>Filter: Proceed to JWT validation
Filter->>Interceptor: Forward validated request
Interceptor->>Interceptor: Additional origin validation<br/>via isOriginAllowed()
alt Origin validation passes
Interceptor->>Interceptor: Set CORS headers conditionally
Interceptor->>Client: Response with CORS headers
else Origin validation fails
Interceptor->>Client: Response without CORS headers
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pom.xml (1)
38-39: Pre-existing issue: Hardcoded credentials in pom.xmlThese deployment credentials (
admin/ad@min123) should be externalized to environment variables or a secrets manager rather than stored in source control. While not introduced by this PR, consider addressing this security concern.src/main/java/com/iemr/common/bengen/utils/JwtUserIdValidationFilter.java (1)
166-183: DuplicatedisOriginAllowedlogic.This method is identical to the one in
HTTPRequestInterceptor.java. Extract to a shared utility class (e.g.,CorsUtils) to avoid maintenance burden and ensure consistent behavior.
🤖 Fix all issues with AI agents
In @src/main/java/com/iemr/common/bengen/utils/JwtUserIdValidationFilter.java:
- Around line 83-90: The else branch in JwtUserIdValidationFilter that checks
for "OPTIONS" is dead code because OPTIONS is already handled earlier; remove
the inner if block that sends SC_FORBIDDEN for OPTIONS (the lines inside the
else that test "OPTIONS".equalsIgnoreCase(method) and call
response.sendError/return) and leave the else to only log the disallowed origin,
ensuring no duplicate OPTIONS handling remains in doFilterInternal (reference:
class JwtUserIdValidationFilter and its doFilterInternal method).
🧹 Nitpick comments (3)
src/main/java/com/iemr/common/bengen/config/CorsConfig.java (1)
22-24: Good security improvement with explicit headers.Adding PATCH to allowed methods and replacing the wildcard header with an explicit list improves security posture.
However, HTTP headers are case-insensitive per RFC 7230, so listing
serverAuthorization,ServerAuthorization,serverauthorization, andServerauthorizationis redundant. A single canonical form should suffice.♻️ Optional: Simplify header list
.allowedMethods("GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS") - .allowedHeaders("Authorization", "Content-Type", "Accept", "Jwttoken", - "serverAuthorization", "ServerAuthorization", "serverauthorization", "Serverauthorization") + .allowedHeaders("Authorization", "Content-Type", "Accept", "Jwttoken", "ServerAuthorization")src/main/java/com/iemr/common/bengen/utils/http/HTTPRequestInterceptor.java (1)
170-184: Duplicated origin validation logic and fragile regex conversion.This
isOriginAllowedmethod is duplicated inJwtUserIdValidationFilter.java. Consider extracting it to a shared utility class.Additionally, the regex conversion approach has issues:
- The replacement chain is order-dependent:
"http://localhost:.*"→"http://localhost:\\d+"only works because*→.*runs first.- Other regex metacharacters (
?,+,[,],(,),^,$) in origins are not escaped, which could causePatternSyntaxExceptionor unintended matches.♻️ Safer approach using Pattern.quote
private boolean isOriginAllowed(String origin) { if (origin == null || allowedOrigins == null || allowedOrigins.trim().isEmpty()) { return false; } return Arrays.stream(allowedOrigins.split(",")) .map(String::trim) .anyMatch(pattern -> { - String regex = pattern - .replace(".", "\\.") - .replace("*", ".*") - .replace("http://localhost:.*", "http://localhost:\\d+"); - return origin.matches(regex); + if (pattern.equals("*")) { + return true; + } + // Handle localhost with wildcard port specially + if (pattern.equals("http://localhost:*")) { + return origin.matches("http://localhost:\\d+"); + } + // For patterns with wildcards, build regex properly + if (pattern.contains("*")) { + String regex = Arrays.stream(pattern.split("\\*", -1)) + .map(java.util.regex.Pattern::quote) + .collect(java.util.stream.Collectors.joining(".*")); + return origin.matches(regex); + } + // Exact match + return origin.equals(pattern); }); }src/main/java/com/iemr/common/bengen/utils/JwtUserIdValidationFilter.java (1)
217-224: Redundant header case variations.Same as noted in
CorsConfig.java: HTTP headers are case-insensitive, so listing multiple case variants ofserverAuthorizationis unnecessary. Additionally, ensure this header list stays in sync withCorsConfig.javato avoid inconsistencies.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pom.xmlsrc/main/java/com/iemr/common/bengen/config/CorsConfig.javasrc/main/java/com/iemr/common/bengen/utils/JwtUserIdValidationFilter.javasrc/main/java/com/iemr/common/bengen/utils/http/HTTPRequestInterceptor.java
🔇 Additional comments (3)
pom.xml (1)
9-9: LGTM - Version bump to 3.6.0The version update aligns with the release branch name and the CORS-related changes in this PR.
src/main/java/com/iemr/common/bengen/utils/http/HTTPRequestInterceptor.java (1)
121-127: CORS headers only applied on error path, not success path.CORS headers are added only within the
catchblock (error responses). For successful requests that pass validation, no CORS headers are set here. Verify that another component (e.g.,CorsConfigorJwtUserIdValidationFilter) consistently adds CORS headers for successful responses, otherwise browsers may block valid responses.src/main/java/com/iemr/common/bengen/utils/JwtUserIdValidationFilter.java (1)
48-66: Origin validation for non-OPTIONS requests may be too strict.For non-OPTIONS requests, if an
Originheader is present but not allowed, the request is blocked (lines 61-65). This is correct for cross-origin requests.However, same-origin requests typically don't include an
Originheader, so they will proceed to line 74 whereorigin != nullis false, meaning no CORS headers are added (correct) but also no blocking occurs (also correct).The logic is sound.
| } else { | ||
| logger.warn("Origin [{}] is NOT allowed. CORS headers NOT added.", origin); | ||
|
|
||
| if ("OPTIONS".equalsIgnoreCase(method)) { | ||
| response.sendError(HttpServletResponse.SC_FORBIDDEN, "Origin not allowed for OPTIONS request"); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Dead code: OPTIONS case already handled above.
If origin is null and the method is OPTIONS, the request already returns at line 52. If origin is not allowed and the method is OPTIONS, the request already returns at line 57. Therefore, the condition at lines 86-89 is unreachable.
🧹 Remove dead code
if (origin != null && isOriginAllowed(origin)) {
addCorsHeaders(response, origin);
logger.info("Origin Validated | Origin: {} | Method: {} | URI: {}", origin, method, uri);
if ("OPTIONS".equalsIgnoreCase(method)) {
// OPTIONS (preflight) - respond with full allowed methods
response.setStatus(HttpServletResponse.SC_OK);
return;
}
} else {
logger.warn("Origin [{}] is NOT allowed. CORS headers NOT added.", origin);
-
- if ("OPTIONS".equalsIgnoreCase(method)) {
- response.sendError(HttpServletResponse.SC_FORBIDDEN, "Origin not allowed for OPTIONS request");
- return;
- }
}🤖 Prompt for AI Agents
In @src/main/java/com/iemr/common/bengen/utils/JwtUserIdValidationFilter.java
around lines 83 - 90, The else branch in JwtUserIdValidationFilter that checks
for "OPTIONS" is dead code because OPTIONS is already handled earlier; remove
the inner if block that sends SC_FORBIDDEN for OPTIONS (the lines inside the
else that test "OPTIONS".equalsIgnoreCase(method) and call
response.sendError/return) and leave the else to only log the disallowed origin,
ensuring no duplicate OPTIONS handling remains in doFilterInternal (reference:
class JwtUserIdValidationFilter and its doFilterInternal method).
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.