fix: amm-1927 http options vulnerability fixed through cors #312
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 WalkthroughTighten CORS security across the application by restricting allowed headers to explicit values in CorsConfig, implementing endpoint-specific HTTP method validation in JwtUserIdValidationFilter, and adding origin-aware CORS filtering in HTTPRequestInterceptor. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant JwtFilter as JwtUserIdValidationFilter
participant CorsConfig
participant App as Application<br/>Endpoint
Client->>JwtFilter: HTTP Request
Note over JwtFilter: Extract origin and<br/>HTTP method
alt Origin Not Allowed
JwtFilter-->>Client: 403 Forbidden
Note right of JwtFilter: Origin validation failed
else OPTIONS Preflight
JwtFilter->>JwtFilter: Resolve allowed methods<br/>for endpoint
JwtFilter-->>Client: 200 OK + CORS Headers
Note right of JwtFilter: Preflight response,<br/>skip JWT validation
else Origin Allowed & Method Valid
JwtFilter->>JwtFilter: Validate JWT token
alt JWT Valid
JwtFilter->>App: Forward request
App-->>JwtFilter: Response
JwtFilter-->>Client: Response + CORS Headers
else JWT Invalid
JwtFilter-->>Client: 401 Unauthorized
end
else Origin Allowed & Method Invalid
JwtFilter-->>Client: 405 Method Not Allowed
Note right of JwtFilter: Method not in<br/>endpoint allowlist
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 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: 2
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/utils/http/HTTPRequestInterceptor.java (1)
198-219: Extract duplicated origin validation to shared utility.The
isOriginAllowedmethod is duplicated identically inJwtUserIdValidationFilter(lines 189-206). Consider extracting this logic to a shared utility class (e.g.,CorsUtilsorOriginValidator) to maintain DRY principles and ensure consistent origin validation behavior across the application.Example shared utility:
public class OriginValidator { 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("*", ".*") .replace("http://localhost:.*", "http://localhost:\\d+"); return origin.matches(regex); }); } }Then both classes can call:
OriginValidator.isOriginAllowed(origin, allowedOrigins)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/iemr/common/config/CorsConfig.java(2 hunks)src/main/java/com/iemr/common/utils/JwtUserIdValidationFilter.java(5 hunks)src/main/java/com/iemr/common/utils/http/HTTPRequestInterceptor.java(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: sandipkarmakar3
Repo: PSMRI/Common-API PR: 162
File: src/main/java/com/iemr/common/utils/CookieUtil.java:40-47
Timestamp: 2025-02-21T07:42:36.497Z
Learning: In the Common-API project's CookieUtil class, JWT cookies are configured with SameSite=None to support cross-origin requests, which is required for the project's CORS functionality.
📚 Learning: 2025-02-21T07:42:36.497Z
Learnt from: sandipkarmakar3
Repo: PSMRI/Common-API PR: 162
File: src/main/java/com/iemr/common/utils/CookieUtil.java:40-47
Timestamp: 2025-02-21T07:42:36.497Z
Learning: In the Common-API project's CookieUtil class, JWT cookies are configured with SameSite=None to support cross-origin requests, which is required for the project's CORS functionality.
Applied to files:
src/main/java/com/iemr/common/utils/JwtUserIdValidationFilter.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). (1)
- GitHub Check: build_and_check_coverage
🔇 Additional comments (7)
src/main/java/com/iemr/common/config/CorsConfig.java (1)
15-27: Good documentation of two-layer security approach.The JavaDoc clearly explains the division of responsibility between Spring's CORS configuration and the filter-based enforcement, which will help future maintainers understand the design.
src/main/java/com/iemr/common/utils/http/HTTPRequestInterceptor.java (2)
50-51: LGTM!The origin configuration injection aligns with the filter's approach and enables consistent origin validation across authentication layers.
148-155: LGTM - Secure error response handling.The conditional CORS header injection ensures that only validated origins receive CORS headers in error responses, preventing information leakage and maintaining consistent security posture.
src/main/java/com/iemr/common/utils/JwtUserIdValidationFilter.java (4)
88-100: LGTM - Proper endpoint-specific method validation.The method validation correctly returns 405 for disallowed methods and includes helpful logging. The validation order (origin → method → JWT) follows security best practices.
103-111: LGTM - Dynamic CORS headers per endpoint.The endpoint-specific
Access-Control-Allow-Methodsand origin-specificAccess-Control-Allow-Originprovide granular control. Note: TheserverAuthorizationheaders in line 106-107 need to be added toCorsConfig.java(flagged in that file's review).
208-236: DELETE endpoint is properly configured—no action required.The existing DELETE endpoint at
/dynamicForm/delete/*/fieldis already configured inENDPOINT_ALLOWED_METHODS(lines 38-42) with DELETE method explicitly allowed. No PUT endpoints were found in the codebase. The default method restrictions will not break existing functionality.
68-73: Verify that this strict Origin requirement aligns with your application's usage patterns.The code explicitly documents this as "STRICT Origin Validation" (line 67 comment). The implementation deliberately requires the
Originheader for OPTIONS requests while making it optional for non-OPTIONS requests—this asymmetry appears intentional for CORS preflight handling. However, without unit tests or documented usage patterns, verify that this strict requirement does not break legitimate same-origin OPTIONS requests from testing tools, health checks, or other internal services in your environment.
src/main/java/com/iemr/common/utils/JwtUserIdValidationFilter.java
Outdated
Show resolved
Hide resolved
| String regex = pattern | ||
| .replace(".", "\\.") | ||
| .replace("*", ".*") | ||
| .replace("http://localhost:.*", "http://localhost:\\d+"); // special case for wildcard port |
There was a problem hiding this comment.
I didn't get why localhost is defined in code. This ideally should be present in the env variable.
For Dev and UAT, we might support localhost as an allowed origin for UI devs.
But in production that shouldn't be allowed.
There was a problem hiding this comment.
yes, have removed it now, thansk for pointing it out.. the code infact did not have an effect if we're restricting even specific ports from the env as tested via postman
|
|
Updated code doc based on latest changes based on reviews OverviewThis PR addresses critical CORS security vulnerabilities (CWE-942: Insecure HTTP Method) and simplifies HTTP method handling by removing unnecessary endpoint-specific restrictions. The implementation now follows security best practices with environment-based configuration and proper separation of concerns. 🔒 Security Vulnerabilities AddressedCWE-942: Insecure HTTP Method
🔧 Changes Made1. JwtUserIdValidationFilter.javaFile: Security Fixes:
Simplifications:
Key Changes: // BEFORE: Endpoint-specific method restrictions
ENDPOINT_ALLOWED_METHODS.put("/dynamicForm/delete/*/field", dynamicFormMethods);
// AFTER: Universal method support
response.setHeader("Access-Control-Allow-Methods", "GET, POST, PUT, PATCH, DELETE, OPTIONS");// BEFORE: Hardcoded localhost (security risk)
.replace("http://localhost:.*", "http://localhost:\\d+");
// AFTER: Environment-controlled (secure)
// Removed hardcoded localhost handling2. CorsConfig.javaFile: Changes:
Before: .allowedHeaders("*")
.allowedMethods("GET", "POST", "PUT", "DELETE", "OPTIONS")After: .allowedHeaders("Authorization", "Content-Type", "Accept", "Jwttoken",
"serverAuthorization", "ServerAuthorization", "serverauthorization", "Serverauthorization")
.allowedMethods("GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS")Rationale: Spring CORS configuration provides framework-level defaults, but 3. HTTPRequestInterceptor.javaFile: Changes:
Key Features:
🔐 Security ImprovementsBefore Implementation
After Implementation
🌍 Environment ConfigurationRequired Environment VariableCORS_ALLOWED_ORIGINS=<comma-separated-list-of-origins>Environment-Specific ConfigurationDevelopmentCORS_ALLOWED_ORIGINS=http://localhost:*,https://dev.example.com
UATCORS_ALLOWED_ORIGINS=http://localhost:*,https://uat.example.com
ProductionCORS_ALLOWED_ORIGINS=https://prod.example.com
Wildcard Pattern SupportThe implementation supports wildcard patterns in origins:
📊 Architecture ChangesCORS Security LayersSeparation of Concerns
🚀 BenefitsFor Development Teams
For Security
For Operations
🧪 TestingManual Testing ScenariosTest 1: Unauthorized Origincurl -X POST https://api.example.com/endpoint \
-H "Origin: https://malicious.com" \
-H "Content-Type: application/json"Expected: Test 2: Allowed Origincurl -X POST https://api.example.com/endpoint \
-H "Origin: https://prod.example.com" \
-H "Authorization: Bearer <token>"Expected: Test 3: OPTIONS Preflightcurl -X OPTIONS https://api.example.com/endpoint \
-H "Origin: https://prod.example.com" \
-H "Access-Control-Request-Method: PUT"Expected: Test 4: Localhost in Dev (Allowed)# With CORS_ALLOWED_ORIGINS=http://localhost:*
curl -X GET https://dev.example.com/endpoint \
-H "Origin: http://localhost:3000"Expected: Test 5: Localhost in Production (Blocked)# With CORS_ALLOWED_ORIGINS=https://prod.example.com (no localhost)
curl -X GET https://api.example.com/endpoint \
-H "Origin: http://localhost:3000"Expected: 📝 Migration Notes
|
📋 Description
JIRA ID: AMM-1927
Overview
This summarizes the granular CORS security implementation to fix CWE-942 vulnerability (Insecure HTTP Method). The implementation enforces strict origin validation, endpoint-specific HTTP method control, and eliminates wildcard CORS headers.
Security Vulnerability Addressed
CWE-942: Insecure HTTP Method
Changes Made
1. JwtUserIdValidationFilter.java
File:
src/main/java/com/iemr/common/utils/JwtUserIdValidationFilter.javaChanges:
ENDPOINT_ALLOWED_METHODSmap/dynamicForm/delete/*/fieldGET, POST, OPTIONSonlyKey Features:
/dynamicForm/delete/*/field)2. CorsConfig.java
File:
src/main/java/com/iemr/common/config/CorsConfig.javaChanges:
allowedHeadersfrom*to specific headers:Authorization, Content-Type, Accept, JwttokenallowedMethodsasGET, POST, PUT, DELETE, OPTIONS(Spring level - actual enforcement in filter)Rationale: Spring CORS config is permissive at framework level, but
JwtUserIdValidationFilterenforces granular restrictions. This two-layer approach allows Spring to handle CORS preflight requests while the filter enforces security policies.3. HTTPRequestInterceptor.java
File:
src/main/java/com/iemr/common/utils/http/HTTPRequestInterceptor.javaChanges:
Access-Control-Allow-Origin: *)@Value("${cors.allowed-origins}")for origin validationisOriginAllowed()helper method (same logic as filter for consistency)Impact: Error responses (401 Unauthorized) now include CORS headers only for allowed origins, preventing frontend CORS errors while maintaining security.
Endpoints Requiring PUT/DELETE
DELETE Endpoints
DynamicFormController.javadeleteField/dynamicForm/delete/{fieldId}/fieldConfiguration:
PUT Endpoints
None found - No PUT endpoints exist in the codebase.
Security Improvements
Before Implementation
After Implementation
Test Cases Covered
Manual Testing
Test unauthorized origin:
Test allowed origin:
Test DELETE on unconfigured endpoint:
Test DELETE on configured endpoint:
Environment Variables
Production
Development
CORS_ALLOWED_ORIGINS=http://localhost:*Docker
CORS_ALLOWED_ORIGINS=${CORS_ALLOWED_ORIGINS}Security Posture
Backward Compatibility
Breaking Changes
cors.allowed-originsvaluesNon-Breaking Changes
Error response CORS headers: Now origin-specific (previously wildcard)
Method restrictions: Most endpoints default to GET, POST, OPTIONS only
ENDPOINT_ALLOWED_METHODSAdding New Endpoints with PUT/DELETE
To add a new endpoint that requires PUT or DELETE:
JwtUserIdValidationFilter.java:Rollback Plan
If issues arise, changes are isolated to 3 files:
JwtUserIdValidationFilter.javaCorsConfig.javaHTTPRequestInterceptor.javaRevert these files to their previous versions to rollback.
Monitoring
Enhanced logging has been added to help identify issues:
Origin Validated | Origin: {origin} | Method: {method} | URI: {uri}BLOCKED - Unauthorized Origin | Origin: {origin} | Method: {method} | URI: {uri}BLOCKED - Method Not Allowed | Method: {method} | URI: {uri}Monitor application logs for these indicators to track security enforcement.
Summary
The granular CORS security implementation successfully addresses the CWE-942 vulnerability by:
Summary by CodeRabbit