fix: amm-1927 res headers based on origin via allowed cors #103
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR enhances CORS security by implementing explicit origin validation across two request-handling components. Configuration-driven allowed origins are enforced for both OPTIONS preflight and regular requests, replacing unconditional wildcard headers with origin-specific conditional responses while preserving existing authentication flow. Changes
Sequence DiagramsequenceDiagram
participant Client
participant JwtFilter as JWT Filter
participant Interceptor as HTTP Interceptor
participant App as Application
Client->>JwtFilter: OPTIONS Request + Origin header
activate JwtFilter
JwtFilter->>JwtFilter: Validate origin against allowed list
alt Origin Allowed
JwtFilter-->>Client: 200 + CORS Headers + Methods, Headers, Credentials
JwtFilter->>JwtFilter: Short-circuit (no further processing)
else Origin Not Allowed
JwtFilter-->>Client: 403 Forbidden
end
deactivate JwtFilter
Client->>JwtFilter: GET/POST/etc + Origin header
activate JwtFilter
JwtFilter->>JwtFilter: Validate origin
alt Origin Allowed
JwtFilter->>JwtFilter: Validate JWT Token
alt Valid JWT
JwtFilter->>Interceptor: Forward Request
activate Interceptor
Interceptor->>Interceptor: Validate origin (configured)
Interceptor-->>Client: Response + Conditional CORS Headers
deactivate Interceptor
else Invalid JWT
JwtFilter-->>Client: 401 Unauthorized
end
else Origin Not Allowed
JwtFilter-->>Client: 403 Forbidden
end
deactivate JwtFilter
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java (1)
163-179: Code duplication - see comment on HTTPRequestInterceptor.java lines 149-162.This
isOriginAllowedmethod is identical to the one inHTTPRequestInterceptor.java. Please extract to a shared utility class as suggested in the previous file review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java(2 hunks)src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-22T09:27:49.422Z
Learnt from: ravishanigarapu
Repo: PSMRI/TM-API PR: 79
File: src/main/java/com/iemr/tm/utils/UserAgentContext.java:3-18
Timestamp: 2025-05-22T09:27:49.422Z
Learning: JwtUserIdValidationFilter.java already includes a try-finally block to properly clear UserAgentContext's ThreadLocal resources after use to prevent memory leaks.
Applied to files:
src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java
📚 Learning: 2024-12-16T11:46:15.606Z
Learnt from: ravishanigarapu
Repo: PSMRI/TM-API PR: 58
File: src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java:93-97
Timestamp: 2024-12-16T11:46:15.606Z
Learning: In the `HTTPRequestInterceptor` class (`src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java`), the `validator` is mandatory and always initialized, so null checks are unnecessary.
Applied to files:
src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java
🔇 Additional comments (10)
src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java (3)
24-24: LGTM - Necessary imports added.The new imports support the CORS configuration injection and origin validation logic.
Also applies to: 31-31
46-47: Configuration property verified—code changes are acceptable.The
cors.allowed-originsproperty is defined in your configuration files (common_example.propertiesandcommon_ci.properties) with appropriate values. The@Valueannotation will inject the property correctly at runtime.
104-110: The review comment is incorrect—CORS headers ARE consistently applied to all responses.The HTTPRequestInterceptor's CORS logic (lines 104-110) handles only error responses, but this is intentional and coordinated. The primary CORS handling occurs in
JwtUserIdValidationFilter, which sets the full CORS headers for all requests with allowed origins (lines 68-74 and 80-86). Both components use identical origin validation logic (isOriginAllowed()), confirming this is a deliberate layered approach:
- Filter layer: Applies CORS headers to all successful responses (lines 68-74)
- Interceptor layer: Fallback that applies CORS headers to error responses (lines 104-110)
The review's premise that "most requests will lack proper CORS headers" overlooks the filter's role and mischaracterizes the architecture as incomplete when it is, in fact, properly coordinated across both components.
Likely an incorrect or invalid review comment.
src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java (7)
40-41: LGTM - Variables support enhanced logging.These variables enable better context in logging statements throughout the method.
45-63: Good security practice - OPTIONS requests require origin validation.This block correctly enforces that:
- OPTIONS requests must include an Origin header
- The origin must be in the allowed list
This prevents unauthorized preflight requests and is a security improvement.
68-78: LGTM - Proper origin-based CORS header configuration.The logic correctly:
- Sets origin-specific headers (never wildcard)
- Includes credentials support
- Logs validation results
70-70: PATCH method support added successfully.The inclusion of PATCH in allowed methods addresses a stated PR objective.
72-72: Comprehensive authorization header variants included.The expanded list of serverAuthorization variants (with various capitalizations) ensures compatibility with different client implementations.
74-75: Enhanced CORS configuration with caching and detailed logging.
Access-Control-Max-Age: 3600reduces preflight request overhead by caching for 1 hour- Enhanced logging provides better observability for CORS validation
174-174: Regex pattern change is correct and intentional—no issues found.Verification confirms:
Pattern conversion is correct:
http://localhost:*properly converts to regexhttp://localhost:.*, which matches all localhost ports (8080, 3000, 8089, etc.) while correctly rejecting non-localhost origins.Configuration adequacy confirmed: The
cors.allowed-origins=http://localhost:*setting in properties files correctly handles multiple localhost services across varying ports referenced in the configuration.The simplified pattern using standard wildcard-to-regex conversion (escape dots first, then convert
*to.*) is sound and sufficient. No special-case handling is needed; the generic solution works correctly.
| 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("*", ".*"); | ||
| return origin.matches(regex); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated origin validation to a shared utility.
The isOriginAllowed method is duplicated in both HTTPRequestInterceptor and JwtUserIdValidationFilter (line 163-179 in that file). This violates DRY principles and increases maintenance burden.
Create a shared utility class for CORS validation:
package com.iemr.tm.utils.cors;
import java.util.Arrays;
public class CorsUtil {
/**
* Check if the given origin is allowed based on configured allowedOrigins.
*
* @param origin The origin to validate
* @param allowedOrigins Comma-separated list of allowed origin patterns
* @return true if origin is allowed, false otherwise
*/
public static boolean isOriginAllowed(String origin, String allowedOrigins) {
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("*", ".*");
return origin.matches(regex);
});
}
}Then update both classes to use CorsUtil.isOriginAllowed(origin, allowedOrigins).
🤖 Prompt for AI Agents
In src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java around lines
149 to 162, the origin validation logic is duplicated in
JwtUserIdValidationFilter; extract this into a new shared utility class (e.g.,
package com.iemr.tm.utils.cors.CorsUtil) with a static method
isOriginAllowed(String origin, String allowedOrigins) that implements the same
null/empty checks and pattern matching, then replace the local isOriginAllowed
implementations in both HTTPRequestInterceptor and JwtUserIdValidationFilter to
call CorsUtil.isOriginAllowed(origin, allowedOrigins); ensure imports are added
and remove the duplicated private methods.
| String path = request.getRequestURI(); | ||
| String contextPath = request.getContextPath(); |
There was a problem hiding this comment.
Remove unused variables.
The path and contextPath variables declared here appear to be unused in the immediate context. The path variable is redeclared at line 92 where it's actually used.
Apply this diff:
- String path = request.getRequestURI();
- String contextPath = request.getContextPath();
-Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java around lines
65-66, the local variables "path" and "contextPath" are declared but never used
(the "path" is redeclared later at line 92); remove these two unused variable
declarations to avoid redundancy and dead code, keeping the later correct "path"
declaration at line 92 intact.
There was a problem hiding this comment.
@5Amogh, understood! I'll keep the variables as they are for readability. Thanks for the clarification.
🧠 Learnings used
Learnt from: ravishanigarapu
Repo: PSMRI/TM-API PR: 79
File: src/main/java/com/iemr/tm/utils/UserAgentContext.java:3-18
Timestamp: 2025-05-22T09:27:49.422Z
Learning: JwtUserIdValidationFilter.java already includes a try-finally block to properly clear UserAgentContext's ThreadLocal resources after use to prevent memory leaks.
| if ("OPTIONS".equalsIgnoreCase(request.getMethod())) { | ||
| // OPTIONS (preflight) - respond with full allowed methods | ||
| response.setHeader("Access-Control-Allow-Origin", origin); | ||
| response.setHeader("Access-Control-Allow-Methods", "GET, POST, PUT, PATCH, DELETE, OPTIONS"); | ||
| response.setHeader("Access-Control-Allow-Headers", | ||
| "Authorization, Content-Type, Accept, Jwttoken, serverAuthorization, ServerAuthorization, serverauthorization, Serverauthorization"); | ||
| response.setHeader("Access-Control-Allow-Credentials", "true"); | ||
| logger.info("OPTIONS request - skipping JWT validation"); | ||
| response.setStatus(HttpServletResponse.SC_OK); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Critical security flaw and redundant logic.
This OPTIONS handling block has multiple serious issues:
-
Security vulnerability: Lines 82-86 unconditionally set CORS headers including the origin without validation. This occurs AFTER the origin has already been validated and blocked if unauthorized (lines 45-63). However, the logic flow is confusing and could allow bypass scenarios.
-
Unreachable code: This entire block is redundant because:
- Valid OPTIONS requests already have CORS headers set at lines 68-78
- Invalid OPTIONS requests are already blocked and returned at lines 47-62
- This block should never be reached for unauthorized origins, making lines 82-86 dead code
-
Logic error: Even if an unauthorized origin somehow reaches this block, setting
Access-Control-Allow-Originwith that origin would be a security breach.
Remove this entire redundant OPTIONS block:
- if ("OPTIONS".equalsIgnoreCase(request.getMethod())) {
- // OPTIONS (preflight) - respond with full allowed methods
- response.setHeader("Access-Control-Allow-Origin", origin);
- response.setHeader("Access-Control-Allow-Methods", "GET, POST, PUT, PATCH, DELETE, OPTIONS");
- response.setHeader("Access-Control-Allow-Headers",
- "Authorization, Content-Type, Accept, Jwttoken, serverAuthorization, ServerAuthorization, serverauthorization, Serverauthorization");
- response.setHeader("Access-Control-Allow-Credentials", "true");
- logger.info("OPTIONS request - skipping JWT validation");
- response.setStatus(HttpServletResponse.SC_OK);
- return;
- }
-The OPTIONS handling at lines 45-63 combined with the CORS headers at lines 68-78 already provide complete and secure OPTIONS request handling.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ("OPTIONS".equalsIgnoreCase(request.getMethod())) { | |
| // OPTIONS (preflight) - respond with full allowed methods | |
| response.setHeader("Access-Control-Allow-Origin", origin); | |
| response.setHeader("Access-Control-Allow-Methods", "GET, POST, PUT, PATCH, DELETE, OPTIONS"); | |
| response.setHeader("Access-Control-Allow-Headers", | |
| "Authorization, Content-Type, Accept, Jwttoken, serverAuthorization, ServerAuthorization, serverauthorization, Serverauthorization"); | |
| response.setHeader("Access-Control-Allow-Credentials", "true"); | |
| logger.info("OPTIONS request - skipping JWT validation"); | |
| response.setStatus(HttpServletResponse.SC_OK); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java around lines
80 to 90, remove the redundant OPTIONS handling block (the unconditional setting
of CORS headers and early return) because OPTIONS is already handled securely
earlier (lines ~45-63 and ~68-78); delete this dead code to avoid re-applying
Access-Control-Allow-Origin after origin validation and ensure the filter relies
on the existing validated CORS/header logic and early-return behavior instead.
| String regex = pattern | ||
| .replace(".", "\\.") | ||
| .replace("*", ".*") | ||
| .replace("http://localhost:.*", "http://localhost:\\d+"); // special case for wildcard port |
|
vishwab1
left a comment
vishwab1
left a comment
There was a problem hiding this comment.
please check the OPTIONS logic
| logger.debug("Incoming Origin: {}", origin); | ||
| logger.debug("Allowed Origins Configured: {}", allowedOrigins); | ||
| if ("OPTIONS".equalsIgnoreCase(method)) { | ||
| if (origin == null) { |
There was a problem hiding this comment.
The current implementation returns 200 OK for OPTIONS requests, which is the correct behavior for CORS preflight requests. Could you please check it
📋 Description
JIRA ID: AMM-1927
This PR transforms the CORS implementation from an overly restrictive, maintenance-heavy configuration to a secure, environment-aware, and developer-friendly approach. By removing endpoint-specific restrictions and fixing critical security vulnerabilities, we achieve:
Better Security: Environment-controlled origins, no hardcoded localhost, immutable configuration
Simpler Code: No endpoint-specific configuration, reduced maintenance
Proper Architecture: CORS handles browser security, endpoints handle authorization
Standard Compliance: Full REST API support including PATCH method
Result: A production-safe, maintainable CORS implementation that follows security best practices and reduces operational overhead.
✅ Type of Change
Summary by CodeRabbit