Release 3.6.0 #99
Release 3.6.0 #99
Conversation
Bulk registration
Bulk registration
# Conflicts: # src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java
remove unwanted line
📝 WalkthroughWalkthroughAdds XML-driven bulk user import endpoints and services (parse, validate, persist, Excel error export), signature activation endpoint, Aadhaar/contact lookup methods, repository/service additions, JWT ID-type switch to Integer, stricter CORS/origin handling, and pom/application property updates for XML and compression. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Controller as BulkRegistrationController
participant Service as BulkRegistrationServiceImpl
participant Xml as EmployeeXmlService
participant Repo as EmployeeMasterInter
participant DB as Database
User->>Controller: POST /bulkRegistration (XML, Authorization)
Controller->>Service: registerBulkUser(xml, auth, userName, serviceProviderID)
Service->>Xml: parseXml(xml)
Xml-->>Service: EmployeeList
loop for each Employee
Service->>Service: validate fields (Aadhaar, email, DOB, contact, etc.)
alt valid
Service->>Repo: saveBulkUserEmployee + save demographics/roles
Repo->>DB: persist entities
DB-->>Repo: OK
Repo-->>Service: saved entity
else invalid
Service->>Service: record BulkRegistrationError
end
end
Service-->>Controller: summary (total, registered, errors)
Controller-->>User: JSON response
Note over User,Controller: Error export
User->>Controller: GET /download-error-sheet
Controller->>Service: insertErrorLog()
Service->>Service: generate Excel bytes
Service-->>Controller: byte[]
Controller-->>User: Excel file (application/octet-stream)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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
♻️ Duplicate comments (4)
src/main/java/com/iemr/admin/controller/bulkRegistration/BulkRegistrationController.java (1)
43-88: Previous review comments still apply.The critical issues identified in past reviews remain valid:
- Unused field at line 38
- Security vulnerability with authorization token logging (marked as addressed but should be verified)
- Thread safety issues with shared mutable state (lines 41, 66-79, 86)
- Log injection vulnerabilities (lines 60, 65)
Based on learnings
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (3)
224-227: Inverted Aadhaar validation logic.The
isValidAadharmethod has inverted logic (returns true when invalid). This causes line 224 to add an error when Aadhaar is actually valid.This issue was flagged in previous reviews and remains unresolved. Lines 325-328 and 742-748 have the same problem.
76-76: Remove PII from logs.Logging the full employee list exposes sensitive information including contact numbers, Aadhaar, and PAN.
Based on learnings
Replace with a non-PII summary as recommended in previous reviews.
110-110: Remove PII from logs.Logging the full employee object exposes sensitive PII.
Based on learnings
This issue was marked as addressed in commit 67a4240 but the problematic logging remains in the current code.
🧹 Nitpick comments (5)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (5)
334-334: Hard-coded marital status reduces flexibility.The marital status is always set to 1 regardless of the employee data.
Consider adding a maritalStatus field to the Employee XML schema and mapping it here, or make this value configurable if it's intentionally a default.
330-333: PAN validation method defined but never used.The
isValidPANmethod exists (lines 674-677) but PAN numbers are accepted without validation.Apply this diff to validate PAN when provided:
if (!employee.getPan().isEmpty()) { + if (!isValidPAN(employee.getPan())) { + validationErrors.add("Invalid PAN format"); + } mUser.setpAN(employee.getPan()); }
346-346: Redundant assignment of employeeID.EmployeeID is set twice at lines 310 and 346 with the same value.
Remove the duplicate assignment at line 346.
352-355: Commented-out role mapping code suggests incomplete feature.The role mapping logic is commented out, indicating the feature may be incomplete or intentionally disabled.
Do you want me to help implement the role mapping feature or open an issue to track this work?
547-547: Use logger instead of System.out.println.Console output at lines 547 and 667 should use the configured logger for consistency and proper log management.
Replace
System.out.printlnwithlogger.debugorlogger.info.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/iemr/admin/controller/bulkRegistration/BulkRegistrationController.java(1 hunks)src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationService.java(1 hunks)src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationService.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-13T06:01:56.090Z
Learnt from: SauravBizbRolly
Repo: PSMRI/Admin-API PR: 69
File: src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java:0-0
Timestamp: 2025-03-13T06:01:56.090Z
Learning: Sensitive personal identifiable information (PII) such as contact numbers, addresses, PAN numbers, and other personal details should not be logged in this Java codebase to maintain privacy and security standards.
Applied to files:
src/main/java/com/iemr/admin/controller/bulkRegistration/BulkRegistrationController.javasrc/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java
🧬 Code graph analysis (1)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (3)
src/main/java/com/iemr/admin/service/employeemaster/EmployeeMasterServiceImpl.java (1)
Service(110-1240)src/main/java/com/iemr/admin/service/bulkRegistration/EmployeeXmlService.java (1)
Service(8-21)src/main/java/com/iemr/admin/service/rolemaster/Role_Master_ServiceImpl.java (1)
Service(48-387)
| mUser.setUserName(employee.getContactNo()); | ||
| mUser.setdOB(convertStringIntoDate(employee.getDob())); | ||
| mUser.setEmployeeID(employee.getUserName()); |
There was a problem hiding this comment.
Username and EmployeeID assignment appears reversed.
Line 308 sets userName to the contact number, while line 310 sets employeeID to the username field. This seems backwards—typically userName should be the user's username, not their phone number.
Consider this fix:
- mUser.setUserName(employee.getContactNo());
+ mUser.setUserName(employee.getUserName());
mUser.setdOB(convertStringIntoDate(employee.getDob()));
- mUser.setEmployeeID(employee.getUserName());
+ mUser.setEmployeeID(employee.getContactNo());🤖 Prompt for AI Agents
In
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java
around lines 308 to 310, the assignments are swapped: userName is being set to
employee.getContactNo() and employeeID is set to employee.getUserName(), which
is likely reversed. Change the assignments so
mUser.setUserName(employee.getUserName()) and
mUser.setEmployeeID(employee.getEmployeeID() or employee.getEmpId() — if
employee has no explicit employeeID field use the appropriate identifier field),
keeping the DOB line as-is; ensure you pick the correct source field for
employeeID from the Employee object.
There was a problem hiding this comment.
As per the new requirement, the contact number is enforced as unique and has been configured to be used as the username.
There was a problem hiding this comment.
@SauravBizbRolly, thank you for the clarification! I apologize for the incorrect review comment. Using the contact number as the username is indeed a valid authentication pattern, especially for mobile-first applications. Phone numbers are tougher to fabricate than email addresses, which can help maintain the integrity of your user base, and this approach is used by popular applications like WhatsApp and Telegram.
The implementation is correct as written:
mUser.setUserName(employee.getContactNo())- uses contact number as the login usernamemUser.setEmployeeID(employee.getUserName())- uses the employee username field as the employee ID
I appreciate you explaining the requirement!
✏️ Learnings added
Learnt from: SauravBizbRolly
Repo: PSMRI/Admin-API PR: 99
File: src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java:308-310
Timestamp: 2026-01-14T05:37:52.567Z
Learning: In the PSMRI/Admin-API project's bulk registration service (BulkRegistrationServiceImpl.java), contact numbers are enforced as unique and configured to be used as the username for authentication. The setUserName() method is intentionally set to the contact number, not the employee username field.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| } | ||
| mUserDemographics.setIsPermanent(false); | ||
| if (!employee.getPermanentPincode().isEmpty()) { | ||
| mUserDemographics.setPermPinCode(Integer.valueOf(employee.getPermanentPincode())); |
There was a problem hiding this comment.
Inconsistent pincode type handling.
Permanent pincode is stored as Integer (line 382), but current pincode is stored as String (line 408). This inconsistency may cause issues downstream.
Standardize on one type. If the database field is String, apply this diff:
if (!employee.getPermanentPincode().isEmpty()) {
- mUserDemographics.setPermPinCode(Integer.valueOf(employee.getPermanentPincode()));
+ mUserDemographics.setPermPinCode(employee.getPermanentPincode());
}🤖 Prompt for AI Agents
In
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java
around lines 382 and 408, the permanent pincode is being set as an Integer while
the current pincode is set as a String; standardize both to the same type to
avoid downstream issues—if the DB field is a String, change the permanent
pincode assignment to use the String value (remove Integer.valueOf and pass
employee.getPermanentPincode()), and ensure the mUserDemographics
model/getter/setter uses String for permPincode; alternatively, if the DB
expects Integer, convert the current pincode handling to Integer and update the
model accordingly so both use Integer consistently.
Release 3.6.0
fix: amm-1927 send headers only if the request is from the allowed origin
Release 3.5.0
Release 3.6.1
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 (1)
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java (1)
157-161: Critical: Duplicate error logging and response - will cause IllegalStateException.Lines 160-161 duplicate lines 157-158. The second
response.sendError()call will throwIllegalStateExceptionbecause the response is already committed after the firstsendError().logger.warn("No valid authentication token found"); response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Unauthorized: Invalid or missing token"); - - logger.warn("No valid authentication token found"); - response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Unauthorized: Invalid or missing token");
🧹 Nitpick comments (3)
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java (3)
40-46: Consider reducing debug log verbosity for security.Logging all allowed origins in every request (line 46) could expose security configuration in logs. Consider logging this once at startup instead, or removing it from per-request logging.
logger.debug("Incoming Origin: {}", origin); logger.debug("Request Method: {}", method); logger.debug("Request URI: {}", uri); - logger.debug("Allowed Origins Configured: {}", allowedOrigins);
48-66: Origin validation logic is sound but creates redundancy with later checks.The early validation at lines 48-66 correctly blocks unauthorized origins. However, this means the
elseblock at lines 82-88 (which handles OPTIONS with invalid origin) is unreachable—if origin is invalid, we never reach line 72 because we already returned at line 52 or 57.This redundancy isn't incorrect but adds unnecessary code complexity.
82-89: Unreachable code for OPTIONS with invalid origin.Since lines 48-58 already return early for OPTIONS requests with null or invalid origins, this
elseblock handling OPTIONS at line 85-88 will never execute. Consider removing the dead code:} 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; - } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pom.xml(5 hunks)src/main/java/com/iemr/admin/config/CorsConfig.java(1 hunks)src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureServiceImpl.java(2 hunks)src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java(5 hunks)src/main/java/com/iemr/admin/utils/http/HTTPRequestInterceptor.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureServiceImpl.java
- pom.xml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-17T11:55:39.499Z
Learnt from: 5Amogh
Repo: PSMRI/Admin-API PR: 110
File: src/main/java/com/iemr/admin/utils/http/HTTPRequestInterceptor.java:153-166
Timestamp: 2025-11-17T11:55:39.499Z
Learning: In the Admin-API Java codebase, small self-contained utility methods like `isOriginAllowed` can be tolerated as duplicates across classes (e.g., HTTPRequestInterceptor and JwtUserIdValidationFilter) if it improves local readability and avoids unnecessary abstraction.
Applied to files:
src/main/java/com/iemr/admin/utils/http/HTTPRequestInterceptor.javasrc/main/java/com/iemr/admin/config/CorsConfig.javasrc/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java
🔇 Additional comments (5)
src/main/java/com/iemr/admin/config/CorsConfig.java (1)
22-24: LGTM - Explicit headers and PATCH method are good security improvements.Replacing wildcard
*with an explicit header list and addingPATCHaligns with least-privilege CORS practices. The four case variations ofserverAuthorizationappear intentional for compatibility with clients that may not normalize header case, which is acceptable defensive coding.src/main/java/com/iemr/admin/utils/http/HTTPRequestInterceptor.java (3)
47-48: Field injection for CORS configuration is appropriate.The
@Valueinjection forallowedOriginsproperly externalizes the configuration and enables environment-specific CORS policies.
112-118: LGTM - Conditional CORS headers on error responses.Good security practice to validate the origin before adding CORS headers on error responses, preventing unauthorized origins from receiving credentialed responses.
153-167: Origin validation logic looks correct.The regex transformation handles wildcard patterns appropriately, and the localhost port restriction (
\\d+) is a good security measure. Per learnings, duplication of this helper acrossHTTPRequestInterceptorandJwtUserIdValidationFilteris acceptable for local readability.src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java (1)
169-176: LGTM - Clean CORS header helper.The
addCorsHeadersmethod properly encapsulates CORS header setting and correctly uses the validated origin instead of a wildcard.
| }); | ||
| } | ||
|
|
||
| private boolean isMobileClient(String userAgent) { | ||
| } private boolean isMobileClient(String userAgent) { |
There was a problem hiding this comment.
Formatting error: Missing newline between methods.
The closing brace of isOriginAllowed and the declaration of isMobileClient are on the same line (line 195), which violates standard Java formatting and hinders readability.
- } private boolean isMobileClient(String userAgent) {
+ }
+
+ private boolean isMobileClient(String userAgent) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }); | |
| } | |
| private boolean isMobileClient(String userAgent) { | |
| } private boolean isMobileClient(String userAgent) { | |
| }); | |
| } | |
| private boolean isMobileClient(String userAgent) { |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java around
lines 194 to 195, the closing brace of isOriginAllowed and the declaration of
isMobileClient are on the same line; fix by placing the closing brace on its own
line and starting the private boolean isMobileClient(...) declaration on the
next line (i.e., add a newline between the methods) to restore standard Java
formatting and readability.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureServiceImpl.java (1)
78-90: Avoid raw JSON Strings in services; add validation and domain-level error handling.This method still takes a raw JSON string, parses it with
JSONObject, and throws a genericIllegalArgumentExceptionon not-found; it also doesn’t handle malformed/partial JSON or null input, which can easily turn bad client data into 500s instead of clear 4xx responses. Prefer a typed request/DTO at the controller layer (with bean validation) and a service method taking strongly-typed arguments (e.g.,Long userId, boolean active), and use a domain-specific not-found exception and/or consistent audit updates.
🧹 Nitpick comments (2)
src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureServiceImpl.java (2)
24-30: Remove duplicateJSONObjectimport.
org.json.JSONObjectis imported twice (lines 24 and 30); keep a single import to avoid clutter.
43-48: Clean up commented‑out legacy code.The large commented block in
uploadSignatureis dead code and was also flagged by Sonar; consider removing it or replacing it with a brief explanatory comment if the logic is still relevant historically.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureServiceImpl.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureServiceImpl.java (1)
src/main/java/com/iemr/admin/service/employeemaster/EmployeeMasterServiceImpl.java (1)
Service(110-1240)
🪛 GitHub Check: SonarCloud Code Analysis
src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureServiceImpl.java
[warning] 82-82: This block of commented-out lines of code should be removed.
🔇 Additional comments (2)
src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureServiceImpl.java (2)
67-70: Formatting-only change inexistSignaturelooks fine.The comparison using
> 0is clear and preserves existing behavior.
72-74:isSignatureActiveimplementation is consistent with repo semantics.Using
countByUserIDAndSignatureNotNullAndDeletedFalse(...) > 0correctly reflects “has at least one non-deleted signature”; this looks good and aligns with the intended status check.
src/main/java/com/iemr/admin/controller/bulkRegistration/BulkRegistrationController.java
Fixed
Show fixed
Hide fixed
src/main/java/com/iemr/admin/controller/bulkRegistration/BulkRegistrationController.java
Fixed
Show fixed
Hide fixed
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java
Fixed
Show fixed
Hide fixed
|
@SauravBizbRolly check the vulnerabilities |
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 (1)
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java (1)
129-140: Remove mobile client JWT bypass or add Authorization header validation.Mobile clients (User-Agent containing "okhttp") with an Authorization header bypass all JWT validation entirely. The filter passes the unvalidated Authorization header directly to downstream components without checking its validity. Since User-Agent headers can be spoofed, any attacker can claim to be mobile and provide an arbitrary Authorization header.
Either validate the Authorization header before allowing the request through, or document that downstream components (controllers/services) must strictly validate it for mobile clients. Currently, downstream code extracts this header and uses it without clear validation shown in the codebase.
🤖 Fix all issues with AI agents
In `@src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java`:
- Around line 58-68: The OPTIONS branch in JwtUserIdValidationFilter currently
validates Origin but then falls through to JWT validation; update the
"OPTIONS".equalsIgnoreCase(method) block to call addCorsHeaders(request,
response) (or otherwise add the standard CORS response headers), send an HTTP
200 (OK) or use response.setStatus(HttpServletResponse.SC_OK) and return
immediately so the filter does not proceed to doJwtValidation or other JWT
logic; ensure addCorsHeaders is invoked for both the allowed-origin and
missing-origin error paths where appropriate and that the method returns after
handling the preflight response.
♻️ Duplicate comments (1)
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java (1)
178-178: Formatting error: Missing newline between methods.The closing brace of
isOriginAllowedand the declaration ofisMobileClientare on the same line, violating standard Java formatting.🔧 Fix formatting
- } private boolean isMobileClient(String userAgent) { + } + + private boolean isMobileClient(String userAgent) {
🧹 Nitpick comments (2)
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java (2)
152-159: Unused method:addCorsHeadersis never invoked.This method is defined but never called anywhere in the filter. If CORS headers are handled by
HTTPRequestInterceptororCorsConfig(as mentioned in the AI summary), this dead code should be removed to avoid confusion.If this method is intended to be used for setting CORS headers on successful responses, it needs to be integrated into the request flow.
🗑️ Remove unused method
- private void addCorsHeaders(HttpServletResponse response, String origin) { - response.setHeader("Access-Control-Allow-Origin", origin); // Never use wildcard - 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"); - response.setHeader("Access-Control-Max-Age", "3600"); - } -
167-177: Fragile regex construction for localhost pattern.The chained string replacements work but are order-dependent and hard to reason about. The special case on line 173 assumes the exact intermediate string
http://localhost:.*exists after the glob conversion.Consider using a more explicit pattern matching approach or documenting the expected input/output transformations.
♻️ Suggested improvement for clarity
return Arrays.stream(allowedOrigins.split(",")) .map(String::trim) .anyMatch(pattern -> { - String regex = pattern - .replace(".", "\\.") - .replace("*", ".*") - .replace("http://localhost:.*", "http://localhost:\\d+"); - + String regex; + // Special handling for localhost with port wildcard + if (pattern.equals("http://localhost:*")) { + regex = "http://localhost:\\d+"; + } else { + regex = pattern + .replace(".", "\\.") + .replace("*", ".*"); + } boolean matched = origin.matches(regex); return matched; });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pom.xmlsrc/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java
🚧 Files skipped from review as they are similar to previous changes (1)
- pom.xml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-17T11:55:39.499Z
Learnt from: 5Amogh
Repo: PSMRI/Admin-API PR: 110
File: src/main/java/com/iemr/admin/utils/http/HTTPRequestInterceptor.java:153-166
Timestamp: 2025-11-17T11:55:39.499Z
Learning: In the Admin-API Java codebase, small self-contained utility methods like `isOriginAllowed` can be tolerated as duplicates across classes (e.g., HTTPRequestInterceptor and JwtUserIdValidationFilter) if it improves local readability and avoids unnecessary abstraction.
Applied to files:
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java
🔇 Additional comments (1)
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java (1)
50-76: CORS origin validation logic is sound.The differentiated handling for OPTIONS vs non-OPTIONS requests is correct:
- OPTIONS (preflight) requires Origin header and validates it
- Non-OPTIONS validates Origin only when present (same-origin requests don't include Origin header)
One consideration: the debug logging at line 56 exposes the full
allowedOriginsconfiguration. Ensure DEBUG level is not enabled in production environments to avoid leaking infrastructure details.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if ("OPTIONS".equalsIgnoreCase(method)) { | ||
| if (origin == null) { | ||
| logger.warn("BLOCKED - OPTIONS request without Origin header | Method: {} | URI: {}", method, uri); | ||
| response.sendError(HttpServletResponse.SC_FORBIDDEN, "OPTIONS request requires Origin header"); | ||
| return; | ||
| } | ||
| if (!isOriginAllowed(origin)) { | ||
| logger.warn("BLOCKED - Unauthorized Origin | Origin: {} | Method: {} | URI: {}", origin, method, uri); | ||
| response.sendError(HttpServletResponse.SC_FORBIDDEN, "Origin not allowed"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for OPTIONS and preflight handling across the codebase
echo "=== Searching for OPTIONS and preflight handling ==="
rg -n "OPTIONS|preflight" --type java -C 2
# Also search for CORS configuration
echo -e "\n=== Searching for CORS configuration ==="
rg -n "CorsConfig|cors|CORS" --type java --max-count 20Repository: PSMRI/Admin-API
Length of output: 3788
🏁 Script executed:
#!/bin/bash
# Examine the full JwtUserIdValidationFilter to understand the flow
echo "=== Full JwtUserIdValidationFilter.java ==="
fd -t f "JwtUserIdValidationFilter.java" --exec wc -l {} \;Repository: PSMRI/Admin-API
Length of output: 174
🏁 Script executed:
#!/bin/bash
# Look for the addCorsHeaders method and its usage
echo "=== Searching for addCorsHeaders method ==="
rg -n "addCorsHeaders" --type java -B 2 -A 5Repository: PSMRI/Admin-API
Length of output: 1178
🏁 Script executed:
#!/bin/bash
# Check if there's a CorsConfig or Spring CORS configuration
echo "=== Looking for CorsConfig or CORS configuration files ==="
fd -t f -i "cors" --type f
echo -e "\n=== Looking for Spring configuration files that might handle CORS ==="
fd -t f "ApplicationConfig.java\|WebConfig.java\|SecurityConfig.java" --type f -exec grep -l "CORS\|cors" {} \;Repository: PSMRI/Admin-API
Length of output: 503
🏁 Script executed:
#!/bin/bash
# Get the full doFilter method to understand the flow
echo "=== Full JwtUserIdValidationFilter doFilter method ==="
sed -n '40,100p' src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.javaRepository: PSMRI/Admin-API
Length of output: 2619
🏁 Script executed:
#!/bin/bash
# Continue reading to see what happens after line 68
echo "=== Lines 65-90 to see flow after OPTIONS check ==="
sed -n '65,90p' src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.javaRepository: PSMRI/Admin-API
Length of output: 1102
🏁 Script executed:
#!/bin/bash
# Also examine CorsConfig to understand how Spring handles CORS
echo "=== Full CorsConfig.java ==="
cat src/main/java/com/iemr/admin/config/CorsConfig.javaRepository: PSMRI/Admin-API
Length of output: 1309
OPTIONS requests must return CORS headers, not continue to JWT validation.
When an OPTIONS preflight request passes origin validation (lines 64–68), the filter falls through to JWT validation logic instead of returning a CORS-compliant response. The unused addCorsHeaders method indicates this was intended but incomplete.
🔧 Suggested fix to handle OPTIONS properly
if ("OPTIONS".equalsIgnoreCase(method)) {
if (origin == null) {
logger.warn("BLOCKED - OPTIONS request without Origin header | Method: {} | URI: {}", method, uri);
response.sendError(HttpServletResponse.SC_FORBIDDEN, "OPTIONS request requires Origin header");
return;
}
if (!isOriginAllowed(origin)) {
logger.warn("BLOCKED - Unauthorized Origin | Origin: {} | Method: {} | URI: {}", origin, method, uri);
response.sendError(HttpServletResponse.SC_FORBIDDEN, "Origin not allowed");
return;
}
+ // Handle preflight: set CORS headers and return immediately
+ addCorsHeaders(response, origin);
+ response.setStatus(HttpServletResponse.SC_OK);
+ return;
} else {🤖 Prompt for AI Agents
In `@src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java` around
lines 58 - 68, The OPTIONS branch in JwtUserIdValidationFilter currently
validates Origin but then falls through to JWT validation; update the
"OPTIONS".equalsIgnoreCase(method) block to call addCorsHeaders(request,
response) (or otherwise add the standard CORS response headers), send an HTTP
200 (OK) or use response.setStatus(HttpServletResponse.SC_OK) and return
immediately so the filter does not proceed to doJwtValidation or other JWT
logic; ensure addCorsHeaders is invoked for both the allowed-origin and
missing-origin error paths where appropriate and that the method returns after
handling the preflight response.
fix vulnerabilitie code
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java`:
- Line 40: The PAN_REGEX constant in BulkRegistrationServiceImpl is incorrect
for Indian PANs; update the PAN_REGEX (private static final String PAN_REGEX) to
the correct pattern for 5 letters, 4 digits, and 1 letter (e.g., change to a
regex equivalent to "^[A-Z]{5}[0-9]{4}[A-Z]$") and ensure any validation logic
that uses PAN_REGEX continues to use this constant; run/adjust any unit tests
that assert PAN validation accordingly.
- Around line 634-646: getDistrictId currently dereferences m_districts which
may be null because m_districts is only set in getStateId; add a null-check and
safe initialization or lazy-load to remove the hidden dependency. Specifically,
in getDistrictId(String districtName) guard early: if m_districts is null,
either initialize it (e.g., call the same loader used in getStateId or fetch
districts for the current state) or return 0; also consider refactoring so
getDistrictId accepts the district list or stateId as a parameter instead of
relying on the class field m_districts to avoid implicit ordering between
getStateId and getDistrictId.
♻️ Duplicate comments (14)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (11)
473-489: Hard-coded year 2025 will require maintenance.The year validation will become obsolete. Use dynamic year calculation instead.
private boolean isValidDate(String dateStr) { try { String[] parts = dateStr.split("-"); int year = Integer.parseInt(parts[0]); + int currentYear = LocalDate.now().getYear(); - if (year > 2025) { + if (year > currentYear + 1) { return false; }
42-65: Public mutable fields create thread-safety and encapsulation issues.These public mutable collection fields (
bulkRegistrationErrors,errorLogs,m_bulkUser,m_UserDemographics,totalEmployeeListSize) are shared across requests and can cause race conditions in concurrent environments. Make them private with synchronized accessors or return a result object.
76-76: Remove PII from logs.Logging
employeeList.getEmployees().toString()exposes sensitive personal data including contact numbers, Aadhaar, PAN, and addresses. Based on learnings, PII should not be logged in this codebase.- logger.info("employee_list" + employeeList.getEmployees().toString()); + logger.info("Processing employee list with {} entries", employeeList.getEmployees().size());
224-227: Inverted Aadhaar validation logic.
isValidAadharreturnstruewhen the Aadhaar is invalid. After fixingisValidAadhar, this condition should be negated.
308-310: Username and EmployeeID assignment appears reversed.Line 308 sets
userNameto the contact number, while line 310 setsemployeeIDto the username field. This seems backwards—typicallyuserNameshould be the user's username.
325-328: Double-negation from inverted validation logic.This condition uses
!isValidAadhar()butisValidAadharalready has inverted logic, causing incorrect behavior. FixisValidAadharfirst, then use positive logic here.
502-533: Remove or sanitize PII in debug logging method.This method logs extensive sensitive PII including contact numbers, email, Aadhaar, PAN, and addresses. Either remove this debug method or sanitize all PII fields. Based on learnings, PII should not be logged.
589-592: Hard-coded designation ID ignores input parameter.The method always returns 20 regardless of the
designationStringinput. Implement proper lookup logic or document why this is intentional.
697-708: Add error handling for date conversion.The method assumes input is an Excel serial number but doesn't validate this. If input is in a different format (e.g., "2025-01-15"), it will throw
NumberFormatException.public static Date convertStringIntoDate(String date) { + if (date == null || date.isEmpty()) { + throw new IllegalArgumentException("Date string cannot be null or empty"); + } + try { final long MILLISECONDS_PER_DAY = 86400000L; final long EPOCH_OFFSET = 2209161600000L; long javaMillis = (long) (Double.parseDouble(date) * MILLISECONDS_PER_DAY - EPOCH_OFFSET); return new Date(javaMillis); + } catch (NumberFormatException e) { + throw new IllegalArgumentException("Invalid date format. Expected Excel serial number, got: " + date, e); + } }
740-746: Critical: Inverted Aadhaar validation logic.The method returns
truewhen the pattern does NOT match, which is backwards. This will accept invalid Aadhaar numbers and reject valid ones.public static boolean isValidAadhar(String aadharNumber) { - if (!Pattern.matches("\\d{12}", aadharNumber)) { - return true; - } - return false; + if (aadharNumber == null || aadharNumber.isEmpty()) { + return false; + } + return Pattern.matches("\\d{12}", aadharNumber); }
110-110: Remove PII from logs.Logging the full
employee.toString()exposes sensitive PII including contact numbers, Aadhaar, PAN, and email. Based on learnings, PII should not be logged.- logger.info("employee_list after for loop" + employee.toString()); + logger.info("Processing employee at row {}", row);src/main/java/com/iemr/admin/controller/bulkRegistration/BulkRegistrationController.java (3)
26-41: Prefer interface injection over concrete implementation.Line 29 autowires
BulkRegistrationServiceImpldirectly while line 34 autowires theBulkRegistrationServiceinterface. Use only the interface (BulkRegistrationService) for loose coupling. The direct access tobulkRegistrationServiceimplfields suggests the service API needs refactoring to return a result object instead.Additionally,
errorResponse(line 38) is unused and should be removed, andresponse(line 41) is a class-level map that's shared across requests causing thread-safety issues.
43-70: Critical thread-safety issues with shared state.
- Class-level
responsemap (line 41) is reused across all requests, causing data leaks between concurrent requests.- Direct access to service public fields (lines 54-56, 58-61) is not thread-safe.
The error path (lines 63-66) doesn't clear previous success data from
response, so error responses may include stale data from previous requests.🐛 Proposed fix using local response map
`@PostMapping`(value = "/bulkRegistration", headers = "Authorization") public ResponseEntity<Map<String, Object>> registerBulkUser(`@RequestBody` String m_user, `@RequestHeader` String authorization, `@RequestParam` String userName, HttpServletRequest request, `@RequestParam` Integer serviceProviderID ) { - bulkRegistrationServiceimpl.bulkRegistrationErrors.clear(); + Map<String, Object> response = new HashMap<>(); logger.info("Bulk registration request received. Request payload is omitted from logs."); try { bulkRegistrationService.registerBulkUser(m_user, authorization,userName,serviceProviderID); + // TODO: Refactor service to return a result object instead of accessing public fields response.put("status", "Success"); response.put("statusCode", 200); - response.put("totalUser", bulkRegistrationServiceimpl.totalEmployeeListSize); - response.put("registeredUser", bulkRegistrationServiceimpl.m_bulkUser.size()); - response.put("error", bulkRegistrationServiceimpl.errorLogs.toString()); - - bulkRegistrationServiceimpl.m_bulkUser.clear(); - bulkRegistrationServiceimpl.m_UserDemographics.clear(); - bulkRegistrationServiceimpl.errorLogs.clear(); - bulkRegistrationServiceimpl.totalEmployeeListSize=0; + // ... get values from result object } catch (Exception e) { response.put("message", e.getMessage()); response.put("statusCode", 500); } return ResponseEntity.ok(response); }
72-91: Race condition and poor error handling in error sheet download.
- Race condition: Error list is cleared (lines 80-82) after file generation, but a concurrent request could clear errors while another is still processing.
- Silent failure: Line 89 returns
nullbody with 500 status, providing no error details to the client.🐛 Proposed improvements
`@GetMapping`(value = "/download-error-sheet", headers = "Authorization") public ResponseEntity<byte[]> downloadErrorSheet() { try { + if (bulkRegistrationServiceimpl.bulkRegistrationErrors.isEmpty()) { + return ResponseEntity.status(404) + .body("No errors to download".getBytes()); + } byte[] fileContent = bulkRegistrationServiceimpl.insertErrorLog(); HttpHeaders headers = new HttpHeaders(); headers.add("Content-Disposition", "attachment; filename=error_log.xlsx"); - if(!bulkRegistrationServiceimpl.bulkRegistrationErrors.isEmpty()){ - bulkRegistrationServiceimpl.bulkRegistrationErrors.clear(); - } + // Consider atomic get-and-clear in service method return ResponseEntity.ok() .headers(headers) .contentType(MediaType.APPLICATION_OCTET_STREAM) .body(fileContent); } catch (Exception e) { - return ResponseEntity.status(500).body(null); + logger.error("Failed to generate error sheet", e); + return ResponseEntity.status(500).body("Failed to generate error sheet".getBytes()); } }
🧹 Nitpick comments (2)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (2)
654-670: Method has hidden side effect of modifyingm_districts.
getStateId()modifies the class fieldm_districts(line 666) as a side effect, creating implicit coupling withgetDistrictId(). This violates the principle of least surprise and can cause order-dependent bugs. Consider making this dependency explicit or refactoring the district lookup approach.
749-790: Remove unused variable and improve resource handling.
rowIndex2on line 762 is declared but never used. Also, the workbook should use try-with-resources for proper cleanup.♻️ Proposed fix
public byte[] insertErrorLog() { ByteArrayOutputStream baos = new ByteArrayOutputStream(); - Workbook workbook = new XSSFWorkbook(); - Sheet sheet = workbook.createSheet("Error Log"); - - try { + try (Workbook workbook = new XSSFWorkbook()) { + Sheet sheet = workbook.createSheet("Error Log"); Row headerRow = sheet.createRow(0); headerRow.createCell(0).setCellValue("UserName"); headerRow.createCell(1).setCellValue("Remark"); int rowIndex = 1; - int rowIndex2 = 1; logger.info("Error Size" + bulkRegistrationErrors.size()); for (BulkRegistrationError bulkRegistrationError : bulkRegistrationErrors) { Row row = sheet.createRow(rowIndex++); row.createCell(0).setCellValue(bulkRegistrationError.getUserName()); row.createCell(1).setCellValue(bulkRegistrationError.getError().toString()); } sheet.autoSizeColumn(0); sheet.autoSizeColumn(1); workbook.write(baos); - workbook.close(); - - System.out.println("Error log generated successfully!"); + logger.info("Error log generated successfully"); } catch (IOException e) { logger.error("IOException" + e.getMessage()); - e.printStackTrace(); } return baos.toByteArray(); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/iemr/admin/controller/bulkRegistration/BulkRegistrationController.javasrc/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-13T06:01:56.090Z
Learnt from: SauravBizbRolly
Repo: PSMRI/Admin-API PR: 69
File: src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java:0-0
Timestamp: 2025-03-13T06:01:56.090Z
Learning: Sensitive personal identifiable information (PII) such as contact numbers, addresses, PAN numbers, and other personal details should not be logged in this Java codebase to maintain privacy and security standards.
Applied to files:
src/main/java/com/iemr/admin/controller/bulkRegistration/BulkRegistrationController.javasrc/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java
🔇 Additional comments (1)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (1)
97-100: LGTM!The XML escaping logic correctly handles ampersands that are not already part of valid XML entities.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
|
||
| @Service | ||
| public class BulkRegistrationServiceImpl implements BulkRegistrationService { | ||
| private static final String PAN_REGEX = "^[A-Z]{8}[0-9]{3}$"; |
There was a problem hiding this comment.
Incorrect PAN regex pattern.
The PAN regex ^[A-Z]{8}[0-9]{3}$ expects 8 uppercase letters followed by 3 digits, but Indian PAN format is 5 letters + 4 digits + 1 letter (e.g., ABCDE1234F).
🐛 Proposed fix
- private static final String PAN_REGEX = "^[A-Z]{8}[0-9]{3}$";
+ private static final String PAN_REGEX = "^[A-Z]{5}[0-9]{4}[A-Z]$";🤖 Prompt for AI Agents
In
`@src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java`
at line 40, The PAN_REGEX constant in BulkRegistrationServiceImpl is incorrect
for Indian PANs; update the PAN_REGEX (private static final String PAN_REGEX) to
the correct pattern for 5 letters, 4 digits, and 1 letter (e.g., change to a
regex equivalent to "^[A-Z]{5}[0-9]{4}[A-Z]$") and ensure any validation logic
that uses PAN_REGEX continues to use this constant; run/adjust any unit tests
that assert PAN validation accordingly.
| public int getDistrictId(String districtName) { | ||
| if (!districtName.isEmpty()) { | ||
| return m_districts.stream() | ||
| .filter(m_district -> m_district.getDistrictName().equalsIgnoreCase(districtName)) | ||
| .map(M_District::getDistrictID) | ||
| .findFirst() | ||
| .orElse(0); | ||
| } else { | ||
| return 0; | ||
| } | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
Potential NullPointerException on m_districts.
m_districts is only initialized in getStateId() (line 666). If getDistrictId() is called before getStateId(), a NullPointerException will occur at line 636.
🐛 Proposed fix
public int getDistrictId(String districtName) {
- if (!districtName.isEmpty()) {
+ if (!districtName.isEmpty() && m_districts != null) {
return m_districts.stream()
.filter(m_district -> m_district.getDistrictName().equalsIgnoreCase(districtName))
.map(M_District::getDistrictID)
.findFirst()
.orElse(0);
} else {
return 0;
}
}Additionally, consider refactoring to avoid this implicit state dependency between getStateId and getDistrictId.
🤖 Prompt for AI Agents
In
`@src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java`
around lines 634 - 646, getDistrictId currently dereferences m_districts which
may be null because m_districts is only set in getStateId; add a null-check and
safe initialization or lazy-load to remove the hidden dependency. Specifically,
in getDistrictId(String districtName) guard early: if m_districts is null,
either initialize it (e.g., call the same loader used in getStateId or fetch
districts for the current state) or return 0; also consider refactoring so
getDistrictId accepts the district list or stateId as a parameter instead of
relying on the class field m_districts to avoid implicit ordering between
getStateId and getDistrictId.
fix security hotspots
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java`:
- Around line 42-67: The service exposes mutable instance fields
(totalEmployeeListSize, bulkRegistrationErrors, errorLogs, m_bulkUser,
m_UserDemographics) in singleton BulkRegistrationServiceImpl which causes
cross-request data corruption; refactor methods like registerBulkUser (or
whichever public bulk registration entrypoint) to stop using these shared fields
and instead create and return a new per-request result holder (e.g.,
BulkRegistrationResult with errors, createdUsers, demographics, errorLogs,
totalSize) that the method populates and returns, or alternatively change the
bean scope to request/prototype (annotate BulkRegistrationServiceImpl with
`@Scope`(WebApplicationContext.SCOPE_REQUEST, proxyMode =
ScopedProxyMode.TARGET_CLASS)); remove reliance on the instance collections from
all helper methods (employeeXmlService usage, any methods referencing
bulkRegistrationErrors, errorLogs, m_bulkUser, m_UserDemographics,
totalEmployeeListSize) so all state is passed via the new result object or local
variables.
- Around line 142-151: NullPointerException risk: calls like
employee.getMiddleName().isEmpty() can NPE if the getter returns null; update
the validation in BulkRegistrationServiceImpl to null-safe checks before calling
isEmpty()/length() or isNumeric(). For each optional field referenced
(employee.getMiddleName(), getAadhaarNo(), getPan(), getCommunity(),
getReligion(), etc.) change the condition to first verify the getter is non-null
(e.g., employee.getMiddleName() != null && !employee.getMiddleName().isEmpty())
and then perform length and numeric checks, and consolidate duplicate error
messages where appropriate.
- Around line 749-789: In insertErrorLog, remove the unused rowIndex2 variable
and replace the manual Workbook/ByteArrayOutputStream lifecycle with
try-with-resources (use try (ByteArrayOutputStream baos = new
ByteArrayOutputStream(); Workbook workbook = new XSSFWorkbook()) { ... }) so the
workbook is always closed on exception; keep writing workbook into baos inside
that block and return baos.toByteArray() after the try. Replace the
System.out.println call with logger.info and change the catch to log the full
exception (logger.error("message", e)) and reference bulkRegistrationErrors,
workbook, baos, rowIndex (and remove rowIndex2) to locate where to apply the
changes.
- Around line 97-100: The escapeXmlSpecialChars method currently only handles
'&' and must escape all five XML special chars to avoid malformed XML; update
escapeXmlSpecialChars to perform full XML escaping for &, <, >, ' and " (or
replace its body to call a tested utility such as Apache Commons Text
StringEscapeUtils.escapeXml11(xml)); if implementing manually ensure you replace
'&' first (or use a single pass mapping) then replace '<'->'<', '>'->'>',
'\''->''' and '"'->'"'; keep the method signature
escapeXmlSpecialChars(String xml) and add unit tests covering inputs like "<
City Name >", quotes and ampersands.
♻️ Duplicate comments (10)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (10)
40-41: Incorrect PAN regex pattern remains unaddressed.The PAN regex
^[A-Z]{8}[0-9]{3}$expects 8 uppercase letters followed by 3 digits, but Indian PAN format is 5 letters + 4 digits + 1 letter (e.g.,ABCDE1234F).🐛 Proposed fix
- private static final String PAN_REGEX = "^[A-Z]{8}[0-9]{3}$"; + private static final String PAN_REGEX = "^[A-Z]{5}[0-9]{4}[A-Z]$";
76-76: Remove PII from logs.Logging
employeeList.getEmployees().toString()exposes sensitive personal identifiable information. This was previously flagged but appears unaddressed in the current code.Based on learnings, sensitive PII such as contact numbers, addresses, PAN numbers, and other personal details should not be logged.
- logger.info("employee_list" + employeeList.getEmployees().toString()); + logger.info("Processing employee list with " + employeeList.getEmployees().size() + " entries");
110-110: Remove PII from logs.Logging the full employee object exposes sensitive PII. This was previously flagged.
- logger.info("employee_list after for loop" + employee.toString()); + logger.info("Processing employee at row " + row);
473-489: Year validation is hard-coded to 2025.This was previously flagged. The hard-coded year will make this code obsolete.
private boolean isValidDate(String dateStr) { try { + if (dateStr == null || dateStr.isEmpty()) { + return false; + } String[] parts = dateStr.split("-"); int year = Integer.parseInt(parts[0]); + int currentYear = LocalDate.now().getYear(); - if (year > 2025) { + if (year > currentYear + 1) { return false; }
502-533: Debug method logs extensive PII.This method logs sensitive data including Aadhaar, PAN, contact numbers, and addresses. While currently commented out at line 300, consider removing or sanitizing this method entirely to prevent accidental re-enablement.
589-592: Hard-coded designation ID.Returns 20 regardless of input. This was previously flagged. Implement proper lookup logic.
634-646: Potential NullPointerException on m_districts.This was previously flagged.
m_districtsis only initialized ingetStateId()(line 666). IfgetDistrictId()is called beforegetStateId(), NPE occurs.
697-708: Missing error handling for date conversion.This was previously flagged. The method assumes Excel serial number format without validation and will throw
NumberFormatExceptionfor invalid input.
740-746: Critical bug: Inverted Aadhaar validation logic.This was previously flagged. The method returns
truewhen the pattern does NOT match, accepting invalid Aadhaar numbers and rejecting valid ones.public static boolean isValidAadhar(String aadharNumber) { - if (!Pattern.matches("\\d{12}", aadharNumber)) { - return true; - } - return false; + if (aadharNumber == null) { + return false; + } + return Pattern.matches("\\d{12}", aadharNumber); }
711-727: Weak password hashing parameters.The PBKDF2 implementation uses only 1001 iterations, far below the OWASP recommendation of 210,000 iterations for PBKDF2-HMAC-SHA512. Additionally,
SHA1PRNGis deprecated; usenew SecureRandom()orSecureRandom.getInstanceStrong()for better security practices.🔒 Proposed security improvements
public String generateStrongPassword(String password) throws NoSuchAlgorithmException, InvalidKeySpecException { - int iterations = 1001; + int iterations = 210000; // OWASP recommended minimum for PBKDF2-HMAC-SHA512 char[] chars = password.toCharArray(); byte[] salt = getSalt(); PBEKeySpec spec = new PBEKeySpec(chars, salt, iterations, 512); SecretKeyFactory skf = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA512"); byte[] hash = skf.generateSecret(spec).getEncoded(); return iterations + ":" + toHex(salt) + ":" + toHex(hash); } private byte[] getSalt() throws NoSuchAlgorithmException { - SecureRandom sr = SecureRandom.getInstance("SHA1PRNG"); + SecureRandom sr = new SecureRandom(); byte[] salt = new byte[16]; sr.nextBytes(salt); return salt; }
🧹 Nitpick comments (5)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (5)
414-422: Catching generic Exception hides root cause.Catching
Exceptionat line 414 is too broad. Consider catching specific exceptions (e.g.,DataIntegrityViolationException,PersistenceException) to handle database errors appropriately and let unexpected exceptions propagate.- } catch (Exception e) { + } catch (DataAccessException e) { errorLogs.add("Row :" + (row + 1) + e.getMessage());
544-548: Replace System.out with logger.Debug output should use the logger instead of
System.out.printlnfor consistency and proper log management.if (genderId == 0) { - System.out.println("Gender name not found: " + genderString); + logger.debug("Gender name not found: {}", genderString); } else { - System.out.println("Gender ID: " + genderId); + logger.debug("Gender ID: {}", genderId); }
662-666: Replace System.out with logger and avoid stateful side effects.
System.out.printlnshould use the logger. Additionally, settingm_districtsas a side effect ofgetStateId()creates hidden coupling.if (stateId == 0) { logger.info("State name not found: " + stateName); } else { - System.out.println("State ID: " + stateId); + logger.debug("State ID: {}", stateId); m_districts = locationMasterServiceInter.getAllDistrictByStateId(stateId); }
679-679: Remove debug System.out statement.Debug output should use the logger or be removed.
- System.out.println("Phone Number" + phoneNumber); + logger.debug("Validating phone number");
688-694: Remove unusedgetHeaderMapmethod.The method has no callers in the codebase and can be safely removed.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-14T05:37:52.567Z
Learnt from: SauravBizbRolly
Repo: PSMRI/Admin-API PR: 99
File: src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java:308-310
Timestamp: 2026-01-14T05:37:52.567Z
Learning: In the PSMRI/Admin-API project's bulk registration service (BulkRegistrationServiceImpl.java), contact numbers are enforced as unique and configured to be used as the username for authentication. The setUserName() method is intentionally set to the contact number, not the employee username field.
Applied to files:
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java
📚 Learning: 2025-03-13T06:01:56.090Z
Learnt from: SauravBizbRolly
Repo: PSMRI/Admin-API PR: 69
File: src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java:0-0
Timestamp: 2025-03-13T06:01:56.090Z
Learning: Sensitive personal identifiable information (PII) such as contact numbers, addresses, PAN numbers, and other personal details should not be logged in this Java codebase to maintain privacy and security standards.
Applied to files:
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java
🧬 Code graph analysis (1)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (3)
src/main/java/com/iemr/admin/service/employeemaster/EmployeeMasterServiceImpl.java (1)
Service(110-1240)src/main/java/com/iemr/admin/service/bulkRegistration/EmployeeXmlService.java (1)
Service(8-21)src/main/java/com/iemr/admin/service/rolemaster/Role_Master_ServiceImpl.java (1)
Service(48-387)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| public Integer totalEmployeeListSize = 0; | ||
| private Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName()); | ||
| @Autowired | ||
| JwtUtil jwtUtil; | ||
| public static final String FILE_PATH = "error_log.xlsx"; // Excel file path | ||
| public List<BulkRegistrationError> bulkRegistrationErrors = new ArrayList<>(); | ||
|
|
||
|
|
||
| @Autowired | ||
| private EmployeeMasterInter employeeMasterInter; | ||
| @Autowired | ||
| private Role_MasterInter roleMasterInter; | ||
|
|
||
| @Autowired | ||
| private V_ShowuserRepo showuserRepo; | ||
| @Autowired | ||
| private LocationMasterServiceInter locationMasterServiceInter; | ||
|
|
||
| @Autowired | ||
| EmployeeXmlService employeeXmlService; | ||
|
|
||
| public ArrayList<String> errorLogs = new ArrayList<>(); | ||
| public ArrayList<M_User1> m_bulkUser = new ArrayList<>(); | ||
| public ArrayList<M_UserDemographics> m_UserDemographics = new ArrayList<>(); | ||
|
|
||
| private List<M_District> m_districts; |
There was a problem hiding this comment.
Critical: Singleton service with mutable shared state causes data corruption.
This Spring @Service is a singleton by default. The public mutable collections (bulkRegistrationErrors, errorLogs, m_bulkUser, m_UserDemographics) and totalEmployeeListSize are shared across all requests. Concurrent bulk registration requests will corrupt each other's data.
This was partially flagged before. The issue is not just encapsulation—it's that concurrent requests will mix error logs, user lists, and counts.
🐛 Proposed architectural fix
Option 1: Return a result object instead of storing state in fields
public class BulkRegistrationResult {
private final List<BulkRegistrationError> errors = new ArrayList<>();
private final List<M_User1> createdUsers = new ArrayList<>();
private final List<M_UserDemographics> demographics = new ArrayList<>();
private final List<String> errorLogs = new ArrayList<>();
private int totalSize;
// getters...
}
// Change method signature:
public BulkRegistrationResult registerBulkUser(String xml, String authorization, String userName, Integer serviceProviderID) {
BulkRegistrationResult result = new BulkRegistrationResult();
// Use result object throughout instead of instance fields
return result;
}Option 2: Use request-scoped bean or prototype scope
`@Service`
`@Scope`(value = WebApplicationContext.SCOPE_REQUEST, proxyMode = ScopedProxyMode.TARGET_CLASS)
public class BulkRegistrationServiceImpl implements BulkRegistrationService {Option 1 is preferred as it's more explicit and testable.
🤖 Prompt for AI Agents
In
`@src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java`
around lines 42 - 67, The service exposes mutable instance fields
(totalEmployeeListSize, bulkRegistrationErrors, errorLogs, m_bulkUser,
m_UserDemographics) in singleton BulkRegistrationServiceImpl which causes
cross-request data corruption; refactor methods like registerBulkUser (or
whichever public bulk registration entrypoint) to stop using these shared fields
and instead create and return a new per-request result holder (e.g.,
BulkRegistrationResult with errors, createdUsers, demographics, errorLogs,
totalSize) that the method populates and returns, or alternatively change the
bean scope to request/prototype (annotate BulkRegistrationServiceImpl with
`@Scope`(WebApplicationContext.SCOPE_REQUEST, proxyMode =
ScopedProxyMode.TARGET_CLASS)); remove reliance on the instance collections from
all helper methods (employeeXmlService usage, any methods referencing
bulkRegistrationErrors, errorLogs, m_bulkUser, m_UserDemographics,
totalEmployeeListSize) so all state is passed via the new result object or local
variables.
| public static String escapeXmlSpecialChars(String xml) { | ||
| // Only escape & that are not already part of valid XML entities | ||
| return xml.replaceAll("&(?!amp;|lt;|gt;|apos;|quot;|#\\d+;)", "&"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the file and examine the method context
find . -type f -name "BulkRegistrationServiceImpl.java" | head -5Repository: PSMRI/Admin-API
Length of output: 147
🏁 Script executed:
# Search for usage of the escapeXmlSpecialChars method
rg "escapeXmlSpecialChars" --type javaRepository: PSMRI/Admin-API
Length of output: 339
🏁 Script executed:
# Let's also check if there are any XML processing imports or usage patterns
rg "xml|XML" src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java -i -A 2 -B 2 | head -50Repository: PSMRI/Admin-API
Length of output: 818
🏁 Script executed:
# Find where registerBulkUser is called (likely in a controller)
rg "registerBulkUser" --type java -B 3 -A 3Repository: PSMRI/Admin-API
Length of output: 3663
🏁 Script executed:
# Check the EmployeeXmlService to understand how XML is parsed
find . -type f -name "EmployeeXmlService*.java"Repository: PSMRI/Admin-API
Length of output: 138
🏁 Script executed:
# Look for controller methods handling XML input
rg "BulkRegistrationService|registerBulkUser" --type java -B 5 | head -80Repository: PSMRI/Admin-API
Length of output: 5852
🏁 Script executed:
cat -n ./src/main/java/com/iemr/admin/service/bulkRegistration/EmployeeXmlService.java | head -100Repository: PSMRI/Admin-API
Length of output: 768
🏁 Script executed:
# Get more context on the EmployeeXmlService
wc -l ./src/main/java/com/iemr/admin/service/bulkRegistration/EmployeeXmlService.javaRepository: PSMRI/Admin-API
Length of output: 141
🏁 Script executed:
# Find the EmployeeList class
find . -type f -name "EmployeeList.java"Repository: PSMRI/Admin-API
Length of output: 121
🏁 Script executed:
# Let's also check if there are any test files showing example XML
find . -type f \( -name "*test*.xml" -o -name "*example*.xml" -o -name "*Test*.java" \) | grep -i bulkregistr | head -20Repository: PSMRI/Admin-API
Length of output: 41
🏁 Script executed:
# Look for any documentation or sample data in the project
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.properties" \) | grep -i bulkRepository: PSMRI/Admin-API
Length of output: 41
🏁 Script executed:
cat -n ./src/main/java/com/iemr/admin/data/bulkuser/EmployeeList.javaRepository: PSMRI/Admin-API
Length of output: 716
🏁 Script executed:
# Also check the Employee class
find . -type f -name "Employee.java" | grep bulkuserRepository: PSMRI/Admin-API
Length of output: 117
🏁 Script executed:
cat -n ./src/main/java/com/iemr/admin/data/bulkuser/Employee.javaRepository: PSMRI/Admin-API
Length of output: 3634
Method only escapes & but should escape all five XML special characters.
The method receives user-supplied XML with embedded data fields (names, addresses, emails, etc.). If these contain unescaped <, >, ', or " characters, the XML becomes malformed and Jackson's XmlMapper will fail to parse it. The method name promises to escape all XML special characters but only handles &.
For example, an address field containing "< City Name >" would break parsing:
<CurrentAddressLine1>Plot < City Name ></CurrentAddressLine1> <!-- Invalid -->To fix this, escape all five special characters:
&→&<→<>→>'→'"→"
Consider using a proper XML escaping utility like StringEscapeUtils.escapeXml11() from Apache Commons Text or similar, or implement complete escaping.
🤖 Prompt for AI Agents
In
`@src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java`
around lines 97 - 100, The escapeXmlSpecialChars method currently only handles
'&' and must escape all five XML special chars to avoid malformed XML; update
escapeXmlSpecialChars to perform full XML escaping for &, <, >, ' and " (or
replace its body to call a tested utility such as Apache Commons Text
StringEscapeUtils.escapeXml11(xml)); if implementing manually ensure you replace
'&' first (or use a single pass mapping) then replace '<'->'<', '>'->'>',
'\''->''' and '"'->'"'; keep the method signature
escapeXmlSpecialChars(String xml) and add unit tests covering inputs like "<
City Name >", quotes and ampersands.
| if(!employee.getMiddleName().isEmpty()){ | ||
| if(employee.getMiddleName().length()>50){ | ||
| validationErrors.add("Middle name is invalid."); | ||
|
|
||
| } | ||
| if(isNumeric(employee.getMiddleName())){ | ||
| validationErrors.add("Middle name is invalid."); | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
Potential NullPointerException on optional fields.
getMiddleName() may return null, causing isEmpty() to throw NPE. The same pattern appears for other optional fields.
🐛 Proposed fix
- if(!employee.getMiddleName().isEmpty()){
- if(employee.getMiddleName().length()>50){
+ if(employee.getMiddleName() != null && !employee.getMiddleName().isEmpty()){
+ if(employee.getMiddleName().length() > 50){
validationErrors.add("Middle name is invalid.");
}
- if(isNumeric(employee.getMiddleName())){
+ if(isNumeric(employee.getMiddleName())){
validationErrors.add("Middle name is invalid.");
}
}Review all isEmpty() calls on optional fields (middleName, aadhaarNo, pan, community, religion, etc.) and add null checks.
🤖 Prompt for AI Agents
In
`@src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java`
around lines 142 - 151, NullPointerException risk: calls like
employee.getMiddleName().isEmpty() can NPE if the getter returns null; update
the validation in BulkRegistrationServiceImpl to null-safe checks before calling
isEmpty()/length() or isNumeric(). For each optional field referenced
(employee.getMiddleName(), getAadhaarNo(), getPan(), getCommunity(),
getReligion(), etc.) change the condition to first verify the getter is non-null
(e.g., employee.getMiddleName() != null && !employee.getMiddleName().isEmpty())
and then perform length and numeric checks, and consolidate duplicate error
messages where appropriate.
| public byte[] insertErrorLog() { | ||
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | ||
| Workbook workbook = new XSSFWorkbook(); | ||
| Sheet sheet = workbook.createSheet("Error Log"); | ||
|
|
||
| try { | ||
| // **Create Header Row (Only Once)** | ||
| Row headerRow = sheet.createRow(0); | ||
| headerRow.createCell(0).setCellValue("UserName"); | ||
| headerRow.createCell(1).setCellValue("Remark"); | ||
|
|
||
| // **Start inserting data from row 1** | ||
| int rowIndex = 1; // Start from row 1 (row 0 is header) | ||
| int rowIndex2 = 1; // Start from row 1 (row 0 is header) | ||
| logger.info("Error Size" + bulkRegistrationErrors.size()); | ||
| for (BulkRegistrationError bulkRegistrationError : bulkRegistrationErrors) { | ||
| Row row = sheet.createRow(rowIndex++); | ||
|
|
||
| row.createCell(0).setCellValue(bulkRegistrationError.getUserName()); | ||
| row.createCell(1).setCellValue(bulkRegistrationError.getError().toString()); | ||
|
|
||
|
|
||
| } | ||
|
|
||
|
|
||
| // **Auto-Size Columns for Better Readability** | ||
| sheet.autoSizeColumn(0); | ||
| sheet.autoSizeColumn(1); | ||
|
|
||
| // **Write to ByteArrayOutputStream** | ||
| workbook.write(baos); | ||
| workbook.close(); | ||
|
|
||
| System.out.println("Error log generated successfully!"); | ||
|
|
||
| } catch (IOException e) { | ||
| logger.error("IOException" + e.getMessage()); | ||
| } | ||
|
|
||
| return baos.toByteArray(); | ||
| } |
There was a problem hiding this comment.
Resource leak and unused variable.
rowIndex2(line 762) is declared but never used.workbookis not properly closed if an exception occurs between creation andworkbook.close(). Use try-with-resources.- Line 782 uses
System.out.printlninstead of logger.
🐛 Proposed fix
public byte[] insertErrorLog() {
ByteArrayOutputStream baos = new ByteArrayOutputStream();
- Workbook workbook = new XSSFWorkbook();
- Sheet sheet = workbook.createSheet("Error Log");
-
- try {
+ try (Workbook workbook = new XSSFWorkbook()) {
+ Sheet sheet = workbook.createSheet("Error Log");
+
// **Create Header Row (Only Once)**
Row headerRow = sheet.createRow(0);
headerRow.createCell(0).setCellValue("UserName");
headerRow.createCell(1).setCellValue("Remark");
// **Start inserting data from row 1**
int rowIndex = 1;
- int rowIndex2 = 1;
logger.info("Error Size" + bulkRegistrationErrors.size());
for (BulkRegistrationError bulkRegistrationError : bulkRegistrationErrors) {
Row row = sheet.createRow(rowIndex++);
row.createCell(0).setCellValue(bulkRegistrationError.getUserName());
row.createCell(1).setCellValue(bulkRegistrationError.getError().toString());
}
sheet.autoSizeColumn(0);
sheet.autoSizeColumn(1);
workbook.write(baos);
- workbook.close();
-
- System.out.println("Error log generated successfully!");
+ logger.info("Error log generated successfully!");
} catch (IOException e) {
logger.error("IOException" + e.getMessage());
}
return baos.toByteArray();
}🤖 Prompt for AI Agents
In
`@src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java`
around lines 749 - 789, In insertErrorLog, remove the unused rowIndex2 variable
and replace the manual Workbook/ByteArrayOutputStream lifecycle with
try-with-resources (use try (ByteArrayOutputStream baos = new
ByteArrayOutputStream(); Workbook workbook = new XSSFWorkbook()) { ... }) so the
workbook is always closed on exception; keep writing workbook into baos inside
that block and return baos.toByteArray() after the try. Replace the
System.out.println call with logger.info and change the catch to log the full
exception (logger.error("message", e)) and reference bulkRegistrationErrors,
workbook, baos, rowIndex (and remove rowIndex2) to locate where to apply the
changes.
|
@drtechie There are quite a few major issues flagged by coderabbit, not sure if its entirely saurav's commits, what should we do ? |
📋 Description
✅ 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
Chores
✏️ Tip: You can customize this high-level summary in your review settings.