Release 3.6.2#153
Conversation
story: amm-1668 task - 1754
…tData. (#96) * Update version in pom.xml to 3.4.0 * story: amm-1668 task - 1754 * story: amm-1668 task - 1754 dto updated (#92) * story: amm-1668 task - 1754 dto updated (#93) * story: amm-1668 task - 1754 dto updated * story: amm-1668 task - 1754 * fix: amm-1879 doctor signature was not coming for ncdcare --------- Co-authored-by: Amoghavarsh <93114621+5Amogh@users.noreply.github.com> Co-authored-by: 5Amogh <amoghavarsh@navadhiti.com>
fix: aam-1896 prescribed quantity was not coming in the casesheet
3.4.0 to 3.4.1
fix: amm-1919 fix for update doctor data for higher refferal data
Fix the WASA Issue : IDOR Vulnerability
* fix: add @PreAuthorize to RBAC * fix: wasa RBAC implementation * fix: remove duplicate dependency * fix: coderabbit comments * fix: update role * fix: enable the request matcher
Sn/3.5.1
Implement Role-Based Access Control with JWT and Auth Integration
add role in register api
fix: amm-2063 added beneficiarytype but not reflecting in the DB level
Updating version from 3.6.0 to 3.6.2
fix: amm-2063 updated the updateBeneficiary flow
feat: amm-2175 dockendra ecg abnormal findings feature added
Amm 2175 api issue fix
Rebase 3.6.2 on 3.6.1
* fix: add labtech role for ecg controller * fix: add role
* fix: add labtech role for ecg controller * fix: add role * fix: url issue in download document * Remove authorization check from getKMFile method Removed authorization check for getKMFile endpoint.
* fix: beneficiary update url * fix: edit url * fix: update the url
fix: 2346 handling duplicate entry of clinical observation
📝 WalkthroughWalkthroughThe PR adds Spring Security role checks and authentication-based request handling across controllers, introduces health and version endpoints with git metadata generation, updates environment route values, and adds ECG abnormal findings support plus doctor-signature flag propagation through clinical flow updates. ChangesSecurity and access control
Runtime support and utilities
Clinical data and workflow updates
Sequence Diagram(s)sequenceDiagram
participant Client
participant RoleAuthenticationFilter
participant JwtUtil
participant RedisStorage
participant JwtAuthenticationUtil
participant SecurityContextHolder
Client->>RoleAuthenticationFilter: request
RoleAuthenticationFilter->>JwtUtil: validate token and extract userId
RoleAuthenticationFilter->>RedisStorage: load cached roles or session data
RoleAuthenticationFilter->>JwtAuthenticationUtil: getUserRoles(userId)
RoleAuthenticationFilter->>SecurityContextHolder: set Authentication
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.0)src/main/java/com/iemr/tm/service/common/transaction/CommonNurseServiceImpl.javaThanks 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}
| csrfTokenRepository.setCookieHttpOnly(true); | ||
| csrfTokenRepository.setCookiePath("/"); | ||
| http | ||
| .csrf(csrf -> csrf.disable()) |
|
|
||
| } | ||
| JSONObject obj = new JSONObject(comingRequest); | ||
| logger.info("getUserVanSpDetails request {}", comingRequest); |
![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
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/iemr/tm/controller/registrar/main/RegistrarController.java (1)
289-321: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep registrar access on the new beneficiary detail/image endpoints.
The legacy equivalents include
REGISTRAR, but the new left-panel and image endpoints omit it, which can block registrar users after switching to the new flow.🔐 Proposed fix
- `@PreAuthorize`("hasRole('NURSE') || hasRole('DOCTOR') || hasRole('LABTECHNICIAN') || hasRole('LAB_TECHNICIAN') || hasRole('PHARMACIST')") + `@PreAuthorize`("hasRole('NURSE') || hasRole('DOCTOR') || hasRole('LABTECHNICIAN') || hasRole('LAB_TECHNICIAN') || hasRole('PHARMACIST') || hasRole('REGISTRAR')") public String getBenDetailsForLeftSidePanelByRegID(- `@PreAuthorize`("hasRole('NURSE') || hasRole('DOCTOR') || hasRole('LABTECHNICIAN') || hasRole('LAB_TECHNICIAN') || hasRole('PHARMACIST')") + `@PreAuthorize`("hasRole('NURSE') || hasRole('DOCTOR') || hasRole('LABTECHNICIAN') || hasRole('LAB_TECHNICIAN') || hasRole('PHARMACIST') || hasRole('REGISTRAR')") public String getBenImage(`@RequestBody` String requestObj,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/controller/registrar/main/RegistrarController.java` around lines 289 - 321, The new beneficiary detail/image endpoints in RegistrarController are missing REGISTRAR access compared with the legacy flow. Update the `@PreAuthorize` rules on getBenDetailsForLeftSidePanelByRegID and the new getBenImage endpoint to include REGISTRAR alongside the existing roles so registrar users are not blocked after the switch.src/main/java/com/iemr/tm/service/labtechnician/LabTechnicianServiceImpl.java (1)
421-460: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist and validate
abnormalFindingsat the procedure level.Right now the new ECG field is copied onto each component row, but those rows are only materialized when a component has a result value or
stripsNotAvailable=true. That means an ECG submission carrying onlyabnormalFindingsis silently dropped, and non-ECG submissions can also populate this column because the assignment is unconditional. Please validate the procedure type and persist this field independently of the component-value gate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/labtechnician/LabTechnicianServiceImpl.java` around lines 421 - 460, The handling in LabTechnicianServiceImpl’s lab result assembly currently copies abnormalFindings onto every LabResultEntry and still only adds rows when a component has testResultValue or stripsNotAvailable, so ECG-only submissions can be dropped and non-ECG procedures can accidentally carry this field. Update the loop that builds labCompResult to validate the procedure type before setting abnormalFindings, and make sure procedure-level abnormalFindings is persisted independently of the component-value condition rather than being tied to the component gate.
🟠 Major comments (28)
src/main/java/com/iemr/tm/controller/foetalmonitor/FoetalMonitorController.java-31-31 (1)
31-31: 🔒 Security & Privacy | 🟠 MajorAdd
@PreAuthorizeannotations to all endpoints; the import is unused.The
@PreAuthorizeimport on line 31 is not used anywhere in the controller. None of the handler methods include@PreAuthorizechecks, leaving these endpoints vulnerable to access by any authenticated user rather than restricting them to the required clinical role.Exposed Methods
`@Operation`(...) `@PostMapping`(...) // Missing `@PreAuthorize` public ResponseEntity<String> sendANCMotherTestDetailsToFoetalMonitor(...) {} `@Operation`(...) `@PostMapping`(...) // Missing `@PreAuthorize` public String saveMother(...) {} // ... and other exposed methods🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/controller/foetalmonitor/FoetalMonitorController.java` at line 31, The imported `@PreAuthorize` in FoetalMonitorController is unused because none of the controller handler methods have authorization checks. Add the appropriate `@PreAuthorize` annotations to each exposed endpoint method in FoetalMonitorController with the required clinical role expression, and keep the import only if it is actually used; otherwise remove it.src/main/java/com/iemr/tm/repo/login/UserLoginRepo.java-18-19 (1)
18-19: 🔒 Security & Privacy | 🟠 MajorAdd a check for soft-deleted mappings in the role lookup query
The query at
src/main/java/com/iemr/tm/repo/login/UserLoginRepo.java:18currently retrieves roles without filtering out deleted entries fromm_userservicerolemapping. Other native queries touching this table (e.g., inV_getVanLocDetailsRepo) explicitly verifyDeleted = false. Update the subquery to includeand Deleted = falseto prevent stale mappings from granting access.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/repo/login/UserLoginRepo.java` around lines 18 - 19, The role lookup in UserLoginRepo.getRoleNamebyUserId currently reads mappings from m_userservicerolemapping without excluding soft-deleted rows. Update the native query so the subquery filters the mapping table with Deleted = false, matching the pattern used by other repos like V_getVanLocDetailsRepo. Keep the change scoped to the getRoleNamebyUserId query and ensure only active role mappings are considered.src/main/java/com/iemr/tm/controller/common/main/WorklistController.java-710-712 (1)
710-712: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd specialist role checks to TC specialist worklists.
These endpoints only require an authenticated principal, unlike the other specialist routes. Add
@PreAuthorizeso any authenticated non-specialist role cannot access TC specialist worklist data.Proposed fix
`@Operation`(summary = "Get teleconsultation specialist worklist") + `@PreAuthorize`("hasRole('TC_SPECIALIST') || hasRole('TCSPECIALIST')") `@GetMapping`(value = { "/getTCSpecialistWorklist/{providerServiceMapID}/{serviceID}" }) ... `@Operation`(summary = "Get teleconsultation specialist worklist for patient app") + `@PreAuthorize`("hasRole('TC_SPECIALIST') || hasRole('TCSPECIALIST')") `@GetMapping`(value = { "/getTCSpecialistWorklistPatientApp/{providerServiceMapID}/{serviceID}/{vanID}" }) ... `@Operation`(summary = "Get teleconsultation specialist future scheduled") + `@PreAuthorize`("hasRole('TC_SPECIALIST') || hasRole('TCSPECIALIST')") `@GetMapping`(value = { "/getTCSpecialistWorklistFutureScheduled/{providerServiceMapID}/{serviceID}" })Also applies to: 744-747, 776-779
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/controller/common/main/WorklistController.java` around lines 710 - 712, Add role-based access control to the TC specialist worklist endpoints so they are not reachable by any authenticated user; update the specialist route handlers such as getTCSpecialistWorkListNew and the other TC worklist methods in WorklistController to include the same `@PreAuthorize` check used by the other specialist routes, ensuring only specialist roles can access these endpoints.src/main/java/com/iemr/tm/controller/videoconsultationcontroller/VideoConsultationController.java-59-75 (1)
59-75: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftApply the principal/userID binding to all from-user video actions.
The new check prevents logging in as another
userID, but the same controller still exposes call actions that takefromuserIDfrom the path. Add the same authenticated-principal check there too, or authenticated users can initiate video actions as another user.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/controller/videoconsultationcontroller/VideoConsultationController.java` around lines 59 - 75, The principal-to-userID authorization check added in VideoConsultationController.login must also be applied to the other from-user video action handlers in this controller. Locate the endpoints that accept fromuserID and Authentication, and verify the authenticated principal matches the path user ID before calling videoConsultationService methods; otherwise return the same unauthorized response path used in login. Keep the check consistent across all from-user actions so authenticated users cannot initiate video operations as another user.src/main/java/com/iemr/tm/utils/mapper/RoleAuthenticationFilter.java-73-81 (1)
73-81: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not log authentication tokens.
Lines 74 and 81 log the full legacy auth token, which can leak reusable credentials through application logs. Log only token presence or a non-reversible fingerprint.
Proposed fix
- logger.info("Resolved authToken: {}", authToken); + logger.debug("Legacy auth token present: {}", authToken != null && !authToken.isBlank()); ... - logger.warn("No Redis session found for authToken: {}", authToken); + logger.warn("No Redis session found for supplied legacy auth token");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/utils/mapper/RoleAuthenticationFilter.java` around lines 73 - 81, The RoleAuthenticationFilter currently logs the full legacy auth token in both the resolveAuthToken and Redis warning paths, which can expose reusable credentials. Update the logging around resolveAuthToken(request) and redisService.getObject(...) to avoid printing the raw token; instead log only that a token was present or use a non-reversible fingerprint. Keep the rest of the authentication flow unchanged and make sure any logger.info/logger.warn calls in this block no longer include authToken directly.src/main/java/com/iemr/tm/utils/JwtUtil.java-62-68 (1)
62-68: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep public claim extraction aligned with token validation.
Line 62 now exposes a parser path that bypasses the denylist check in
validateToken, so revoked JWTs can still yield claims if callers use this public method. Delegate tovalidateTokenor keep the raw parser private.Proposed fix
public Claims extractAllClaims(String token) { - return Jwts.parser() - .verifyWith(getSigningKey()) - .build() - .parseSignedClaims(token) - .getPayload(); + return validateToken(token); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/utils/JwtUtil.java` around lines 62 - 68, The public claim extraction path in JwtUtil.extractAllClaims currently bypasses the denylist logic enforced by validateToken, so revoked tokens can still be parsed for claims. Update extractAllClaims to reuse validateToken (or otherwise perform the denylist check before parsing) and keep any raw parsing helper private so only validated JWTs can expose Claims.src/main/java/com/iemr/tm/utils/redis/RedisStorage.java-102-107 (1)
102-107: 🔒 Security & Privacy | 🟠 MajorEnsure atomic persistence of role cache and TTL
The current
cacheUserRolesmethod executesdelete,rightPushAll, andexpireas separate, non-atomic operations. IfrightPushAllsucceeds butexpirefails (e.g., due to a transient network issue), the cached roles persist indefinitely, potentially serving stale authorization data.Wrap these operations in a transaction or pipeline to ensure the key and its expiration are updated together.
public void cacheUserRoles(Long userId, List<String> roles) { try { String key = "roles:" + userId; redisTemplate.executePipelined((RedisCallback<Object>) connection -> { connection.del(key.getBytes()); connection.rPush(key.getBytes(), roles.toArray()); connection.expire(key.getBytes(), 1800); // 30 minutes return null; }); } catch (Exception e) { logger.warn("Failed to cache role for user {} : {} ", userId, e.getMessage()); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/utils/redis/RedisStorage.java` around lines 102 - 107, The cacheUserRoles method in RedisStorage currently performs delete, rightPushAll, and expire as separate Redis calls, so the role list can be written without a TTL if expire fails. Update this method to use a single atomic Redis pipeline or transaction (for example via redisTemplate.executePipelined or a transaction-based callback) so the key removal, list write, and 30-minute expiration are applied together. Keep the existing cacheUserRoles symbol and preserve the current error handling/logging path.src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java-154-161 (1)
154-161: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject global wildcard origins when credentials are enabled.
With lines 105-107 echoing the request origin and setting
Access-Control-Allow-Credentials: true, a configured*becomes credentialed CORS for any origin after thisreplace("*", ".*"). Treat*as invalid here and apply the same guard to the duplicateJwtUserIdValidationFilterhelper.🛡️ Proposed fix
return Arrays.stream(allowedOrigins.split(",")) .map(String::trim) + .filter(pattern -> !pattern.isEmpty() && !"*".equals(pattern)) .anyMatch(pattern -> { String regex = pattern .replace(".", "\\.") .replace("*", ".*") .replace("http://localhost:.*", "http://localhost:\\d+"); return origin.matches(regex); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java` around lines 154 - 161, The CORS origin matching in HTTPRequestInterceptor currently turns a configured "*" into a regex that matches any origin while credentials are enabled, so update the allowedOrigins handling to explicitly reject or ignore "*" before building the match pattern and keep only specific origins. Apply the same validation in the duplicate helper used by JwtUserIdValidationFilter so both paths behave consistently, and make sure the origin echo plus Access-Control-Allow-Credentials logic can never combine with a wildcard config.src/main/java/com/iemr/tm/controller/location/LocationController.java-45-45 (1)
45-45: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAllow ASHA to read location masters used during registration.
/registrar/registrarBeneficaryRegistrationNewpermitsASHA, but this controller’s beneficiary location master endpoints omit that role, so ASHA users can be blocked before submitting registration.🔐 Proposed fix
-@PreAuthorize("hasRole('NURSE') || hasRole('PHARMACIST') || hasRole('LABTECHNICIAN') || hasRole('REGISTRAR') || hasRole('DATASYNC') || hasRole('DATA_SYNC') || hasRole('DOCTOR') || hasRole('LAB_TECHNICIAN') || hasRole('TC_SPECIALIST') || hasRole('TCSPECIALIST') || hasRole('ONCOLOGIST') || hasRole('RADIOLOGIST')") +@PreAuthorize("hasRole('NURSE') || hasRole('PHARMACIST') || hasRole('LABTECHNICIAN') || hasRole('REGISTRAR') || hasRole('DATASYNC') || hasRole('DATA_SYNC') || hasRole('DOCTOR') || hasRole('LAB_TECHNICIAN') || hasRole('TC_SPECIALIST') || hasRole('TCSPECIALIST') || hasRole('ONCOLOGIST') || hasRole('RADIOLOGIST') || hasRole('ASHA')") public class LocationController {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/controller/location/LocationController.java` at line 45, The LocationController security restriction is missing ASHA access, which can block ASHA users from reading the location master data needed during registration. Update the `@PreAuthorize` expression on the LocationController endpoint(s) to include ASHA alongside the existing roles, matching the registration flow permissions. Keep the change localized to the location master access checks so ASHA can load the beneficiary location data without affecting other authorization logic.src/main/java/com/iemr/tm/utils/exception/CustomAuthenticationEntryPoint.java-21-21 (1)
21-21: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSerialize the auth error instead of concatenating exception text.
authException.getMessage()is written directly into JSON, which can break the response format and expose internal auth failure details. Return a fixed client message through Jackson, matching the access-denied handler.🛡️ Proposed fix
+import java.util.Map; + +import com.fasterxml.jackson.databind.ObjectMapper; + `@Component` public class CustomAuthenticationEntryPoint implements AuthenticationEntryPoint { + private static final ObjectMapper mapper = new ObjectMapper(); `@Override` public void commence(HttpServletRequest request, HttpServletResponse response, AuthenticationException authException) throws IOException { response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); // 401 response.setContentType("application/json"); - response.getWriter().write("{\"error\": \"Unauthorized\", \"message\": \"" + authException.getMessage() + "\"}"); + response.getWriter().write(mapper.writeValueAsString( + Map.of("error", "Unauthorized", "message", "Authentication required"))); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/utils/exception/CustomAuthenticationEntryPoint.java` at line 21, In CustomAuthenticationEntryPoint, stop building the unauthorized response JSON by concatenating authException.getMessage() into the string. Use the same Jackson-based serialization approach as the access-denied handler so the response always returns a fixed client-facing message and cannot be broken by exception text. Locate the change in the entry point’s write/commence logic and replace the manual JSON write with an object serialized by the existing ObjectMapper pattern.Source: Linters/SAST tools
src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java-542-545 (1)
542-545: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestrict oncologist diagnosis updates to the oncologist role.
This endpoint updates oncologist-specific diagnosis data, but Line 544 also grants
NURSEandDOCTORwrite access. Unless that is intentional, narrow this toONCOLOGIST.Suggested fix
- `@PreAuthorize`("hasRole('NURSE') || hasRole('DOCTOR') || hasRole('ONCOLOGIST') ") + `@PreAuthorize`("hasRole('ONCOLOGIST')")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.java` around lines 542 - 545, The updateCancerDiagnosisDetailsByOncologist endpoint is currently too permissive because its `@PreAuthorize` expression allows NURSE and DOCTOR roles to perform an oncologist-specific update; tighten the authorization on this method so only ONCOLOGIST can access it, and keep the change localized to updateCancerDiagnosisDetailsByOncologist in CancerScreeningController.src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java-45-75 (1)
45-75: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winShort-circuit successful CORS preflights before JWT validation.
Allowed
OPTIONSrequests set CORS headers, then continue into JWT validation; protected endpoints will return 401 before the browser can send the real request. Return immediately after setting CORS headers forOPTIONS.Suggested fix
if (origin != null && isOriginAllowed(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"); logger.info("Origin Validated | Origin: {} | Method: {} | URI: {}", origin, method, uri); + if ("OPTIONS".equalsIgnoreCase(method)) { + response.setStatus(HttpServletResponse.SC_OK); + return; + } } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.java` around lines 45 - 75, In JwtUserIdValidationFilter, allowed CORS preflight handling for OPTIONS requests currently continues into the JWT checks, which can trigger a 401 before the browser sends the actual request. Update the filter so that after setting the CORS headers for a valid OPTIONS request, it exits immediately and does not reach the JWT validation path. Keep the origin validation logic in the same filter flow, and use the existing JwtUserIdValidationFilter method and the OPTIONS branch to place the early return.src/main/java/com/iemr/tm/service/health/HealthService.java-92-97 (1)
92-97: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound the client calls, not just the
Futurewait.
mysqlFuture.get(...)/redisFuture.get(...)only stop waiting in the request thread.dataSource.getConnection()andredisTemplate.execute(...)can keep blocking aftercancel(true), andExecutors.newFixedThreadPool(6)adds an unbounded queue behind them. During a DB/Redis outage,/healthrequests can pile up stuck tasks until the executor is saturated and every subsequent probe starts timing out.Also applies to: 123-125, 140-149, 194-230
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` around lines 92 - 97, The health checks in HealthService are only timing out the caller via Future.get(), but the underlying dataSource.getConnection() and redisTemplate.execute() calls can keep blocking and pile up on the fixed thread pool. Refactor the MySQL/Redis probe methods (the ones wrapped by mysqlFuture and redisFuture) so the client operations themselves are bounded with real timeouts or non-blocking timeout-aware APIs, and replace the unbounded executor setup in the HealthService constructor with a bounded execution strategy so stuck probes cannot accumulate during outages.src/main/java/com/iemr/tm/service/common/transaction/CommonNurseServiceImpl.java-2890-2891 (1)
2890-2891: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the double-space typo in the new single-dose frequency literals.
"Single Dose Before Food"/"After Food"won't match the normal"Before Food"/"After Food"values. When that happens,getQtyForOneDay()stays0andcalculateQtyPrescribed()writes a zero quantity for a valid prescription.Suggested change
- if (frequency.equalsIgnoreCase("Single Dose") || frequency.equalsIgnoreCase("Stat Dose")|| - frequency.equalsIgnoreCase("Single Dose Before Food") || frequency.equalsIgnoreCase("Single Dose After Food")) { + if (frequency.equalsIgnoreCase("Single Dose") || frequency.equalsIgnoreCase("Stat Dose")|| + frequency.equalsIgnoreCase("Single Dose Before Food") || frequency.equalsIgnoreCase("Single Dose After Food")) { ... - if (frequency.equalsIgnoreCase("Single Dose") || frequency.equalsIgnoreCase("Stat Dose")|| - frequency.equalsIgnoreCase("Single Dose Before Food") || frequency.equalsIgnoreCase("Single Dose After Food")) { + if (frequency.equalsIgnoreCase("Single Dose") || frequency.equalsIgnoreCase("Stat Dose")|| + frequency.equalsIgnoreCase("Single Dose Before Food") || frequency.equalsIgnoreCase("Single Dose After Food")) {Also applies to: 3010-3011
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/common/transaction/CommonNurseServiceImpl.java` around lines 2890 - 2891, The new single-dose frequency checks in CommonNurseServiceImpl use misspelled literals with double spaces, so they will not match the normal “Before Food” and “After Food” values. Update the frequency comparisons in the affected branches, including the logic around getQtyForOneDay() and calculateQtyPrescribed(), to use the exact standard literals so valid prescriptions are recognized and assigned the correct quantity.src/main/java/com/iemr/tm/service/common/transaction/CommonServiceImpl.java-566-576 (1)
566-576: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove the OpenKM URL/response logs.
These
infologs can capture internal endpoints and credential- or token-bearing document URLs, plus patient-linked file identifiers. That turns routine document fetches into secret/PII leakage through application logs.Suggested change
- logger.info("fileUUID for fileID " + obj.getInt("fileID") + " is " + fileUUID); - logger.info("openkmDocUrl is " + openkmDocUrl); + logger.debug("Fetching OpenKM document URL for fileID={}", obj.getInt("fileID")); ... - logger.info("Response=" + response.getBody());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/common/transaction/CommonServiceImpl.java` around lines 566 - 576, The logging in CommonServiceImpl around the OpenKM fetch flow is leaking sensitive data; remove the info logs that print openkmDocUrl and the response body, and keep only non-sensitive operational logging around the file retrieval path in the block that builds the request with RestTemplateUtil.createRequestEntity and calls restTemplate.exchange.src/main/java/com/iemr/tm/service/common/transaction/CommonServiceImpl.java-578-595 (1)
578-595: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep the old raw-body fallback for non-JSON OpenKM responses.
new JSONObject(responseBody)now throws for plain-text/HTML error bodies, so this path 500s instead of returning the original body. That makes the integration less tolerant than before.Suggested change
String responseBody = response.getBody(); if (responseBody != null) { - JSONObject responseObj = new JSONObject(responseBody); - if (responseObj.has("data")) { - Object dataVal = responseObj.get("data"); - if (dataVal instanceof JSONObject) { - JSONObject dataObj = (JSONObject) dataVal; - if (dataObj.has("response")) { - String fileUrl = dataObj.getString("response"); - // Fix malformed URL: https://user:pass@https://host -> https://user:pass@host - fileUrl = fileUrl.replaceAll("`@https`?://", "@"); - return fileUrl; - } - } - return dataVal.toString(); - } + try { + JSONObject responseObj = new JSONObject(responseBody); + if (responseObj.has("data")) { + Object dataVal = responseObj.get("data"); + if (dataVal instanceof JSONObject) { + JSONObject dataObj = (JSONObject) dataVal; + if (dataObj.has("response")) { + String fileUrl = dataObj.getString("response"); + fileUrl = fileUrl.replaceAll("`@https`?://", "@"); + return fileUrl; + } + } + return dataVal.toString(); + } + } catch (JSONException ex) { + return responseBody; + } } return responseBody;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/common/transaction/CommonServiceImpl.java` around lines 578 - 595, The response parsing in CommonServiceImpl should preserve the old raw-body fallback for non-JSON OpenKM responses. Update the logic around responseBody parsing so JSONObject construction is only attempted when the body is actually JSON, and if parsing fails or the payload is plain text/HTML, return the original responseBody instead of throwing. Keep the existing data/response extraction path and URL fix, but make sure the fallback behavior in this method remains tolerant for malformed or non-JSON bodies.src/main/java/com/iemr/tm/utils/StringListConverter.java-39-50 (1)
39-50: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve empty lists instead of collapsing them to
null.This converter is lossy today: an empty list is stored as
NULLand read back asnull. That breaks round-tripping and makes “unset” indistinguishable from “explicitly empty”.Suggested change
+import java.util.Collections; ... `@Override` public String convertToDatabaseColumn(List<String> attribute) { - if (attribute == null || attribute.isEmpty()) { + if (attribute == null) { return null; } return gson.toJson(attribute); } `@Override` public List<String> convertToEntityAttribute(String dbData) { - if (dbData == null || dbData.trim().isEmpty()) { + if (dbData == null) { return null; } - return gson.fromJson(dbData, new TypeToken<List<String>>(){}.getType()); + if (dbData.trim().isEmpty()) { + return Collections.emptyList(); + } + List<String> values = gson.fromJson(dbData, new TypeToken<List<String>>(){}.getType()); + return values != null ? values : Collections.emptyList(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/utils/StringListConverter.java` around lines 39 - 50, The StringListConverter is collapsing empty lists into null, which breaks round-tripping between entity and database values. Update convertToDatabaseColumn in StringListConverter so that an empty List<String> is serialized instead of returning null, while still handling a null attribute separately. Keep convertToEntityAttribute aligned with this behavior so an empty persisted value restores to an empty list rather than null, preserving the distinction between unset and explicitly empty.src/main/java/com/iemr/tm/controller/common/master/CommonMasterController.java-42-44 (1)
42-44: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winScope the lab-tech role to the new endpoint only.
Making
LABTECHNICIAN/LAB_TECHNICIANa class-level allowance also opens the existing nurse/doctor master-data endpoints to that role. If the goal is only to expose/master/ecgAbnormalFindings, keep the broader access on that method instead of the whole controller.Suggested access split
-@PreAuthorize("hasRole('NURSE') || hasRole('DOCTOR') || hasRole('LABTECHNICIAN') || hasRole('LAB_TECHNICIAN')") +@PreAuthorize("hasRole('NURSE') || hasRole('DOCTOR')") public class CommonMasterController {+ `@PreAuthorize`("hasRole('NURSE') || hasRole('DOCTOR') || hasRole('LABTECHNICIAN') || hasRole('LAB_TECHNICIAN')") `@Operation`(summary = "Get ECG abnormal findings master data") `@GetMapping`(value = "/ecgAbnormalFindings", produces = MediaType.APPLICATION_JSON) public String getECGAbnormalFindings() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/controller/common/master/CommonMasterController.java` around lines 42 - 44, The class-level access on CommonMasterController currently grants LABTECHNICIAN/LAB_TECHNICIAN to every master-data endpoint, which unintentionally broadens access beyond the new ECG route. Move the lab-tech roles off the controller-level `@PreAuthorize` and apply them only to the ECG-specific handler (the method for /master/ecgAbnormalFindings), while keeping the existing nurse/doctor restrictions on the rest of the controller. Use the controller class annotation and the ECG endpoint method name to locate the split and preserve the broader access only where intended.src/main/java/com/iemr/tm/service/anc/ANCServiceImpl.java-1496-1500 (1)
1496-1500: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not treat an omitted signature flag as
falseon update.For update flows, defaulting an absent
doctorSignatureFlagtofalsecan overwrite an existing saved signature. Preserve the current value when the field is omitted; use explicitfalseonly when the client sends it.Also applies to: 1608-1609
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/anc/ANCServiceImpl.java` around lines 1496 - 1500, The update handling in ANCServiceImpl is defaulting doctorSignatureFlag to false when the field is omitted, which can overwrite an existing saved signature; adjust the requestOBJ parsing in the update flow so omitted doctorSignatureFlag values preserve the current stored value, and only set false when the client explicitly sends false. Apply the same fix to the other doctorSignatureFlag handling near the related update logic so the behavior is consistent.src/main/java/com/iemr/tm/service/covid19/Covid19ServiceImpl.java-1195-1198 (1)
1195-1198: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve existing signature state when the update payload omits the flag.
Defaulting
doctorSignatureFlagtofalsemakes older/partial update clients clear a previously stored signature. Pass a nullable “not provided” value or otherwise preserve the existing DB value unless the client explicitly sendsfalse.Also applies to: 1317-1318
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/covid19/Covid19ServiceImpl.java` around lines 1195 - 1198, The update flow in Covid19ServiceImpl is overwriting an existing doctor signature state by defaulting doctorSignatureFlag to false when the payload omits it. Change the handling in the update logic so the flag stays nullable or otherwise indicates “not provided,” and only apply a change when the request explicitly includes doctorSignatureFlag; use the existing update paths in Covid19ServiceImpl around the doctorSignatureFlag assignment sites to preserve the stored DB value for partial updates.src/main/java/com/iemr/tm/service/ncdCare/NCDCareServiceImpl.java-1211-1214 (1)
1211-1214: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve existing signature state when the update payload omits the flag.
Missing
doctorSignatureFlagcurrently becomesfalseand is propagated to the flow update, which can unintentionally reset a previously true signature flag. Preserve existing state unless the field is explicitly supplied.Also applies to: 1324-1325
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/ncdCare/NCDCareServiceImpl.java` around lines 1211 - 1214, The update flow in NCDCareServiceImpl is defaulting doctorSignatureFlag to false when the request omits it, which can overwrite an existing true value; change the parsing logic so the flag is only updated when requestOBJ explicitly contains doctorSignatureFlag, and otherwise preserve the current flow state. Apply the same behavior in both the signature handling block around doctorSignatureFlag and the later update path that also reads this field, using the existing NCDCareServiceImpl update methods/variables to keep the prior signature value unchanged unless provided.src/main/java/com/iemr/tm/service/common/transaction/CommonDoctorServiceImpl.java-198-203 (1)
198-203: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake duplicate suppression atomic.
Line 199 checks for an existing observation and Line 202 inserts afterward, so concurrent saves for the same beneficiary/visit can both see
nulland create duplicates. Enforce this with a DB uniqueness/atomic insert strategy instead of a non-atomic pre-check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/common/transaction/CommonDoctorServiceImpl.java` around lines 198 - 203, The duplicate check in CommonDoctorServiceImpl’s observation save flow is non-atomic, so concurrent calls can both pass getBenClinicalObservationStatus and then save duplicates. Replace the pre-check in this save block with an atomic database-backed guard, such as a unique constraint on beneficiaryRegID plus visitCode and handling the insert conflict in the getBenClinicalObservations/save path. Keep the logic centered around CommonDoctorServiceImpl and benClinicalObservationsRepo, but rely on the database to enforce one record per beneficiary/visit.src/main/java/com/iemr/tm/service/generalOPD/GeneralOPDServiceImpl.java-1368-1371 (1)
1368-1371: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve existing signature state when the update payload omits the flag.
This update path treats absence as
false, so an update request that does not includedoctorSignatureFlagcan clear an existing signed state. Keep absence distinct from explicitfalse.Also applies to: 1486-1487
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/generalOPD/GeneralOPDServiceImpl.java` around lines 1368 - 1371, The update logic in GeneralOPDServiceImpl currently defaults doctorSignatureFlag to false when the field is missing, which can unintentionally clear an existing signed state. In the update paths around the doctorSignatureFlag handling (including the later occurrence noted in the same service), distinguish “missing” from explicit false by only changing the stored signature state when the payload actually contains doctorSignatureFlag, and otherwise leave the current value unchanged. Use the existing doctorSignatureFlag variable and the requestOBJ JSON checks to gate the assignment rather than applying a default false.src/main/java/com/iemr/tm/service/pnc/PNCServiceImpl.java-1389-1392 (1)
1389-1392: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve existing signature state when the update payload omits the flag.
An omitted
doctorSignatureFlagis currently indistinguishable from an explicitfalse, so partial update requests can clear an existing signature. Use nullable/preserve semantics for update paths.Also applies to: 1501-1502
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/pnc/PNCServiceImpl.java` around lines 1389 - 1392, The update path in PNCServiceImpl is treating a missing doctorSignatureFlag the same as false, which can clear an existing signature during partial updates. Adjust the logic around the doctorSignatureFlag handling in the relevant update methods so omitted values are represented as null or otherwise preserved, and only apply a boolean change when the field is actually present in the request payload. Use the existing doctorSignatureFlag variable and the related update flow near the repeated assignment sites to keep the current signature state intact when the flag is absent.src/main/java/com/iemr/tm/service/quickConsultation/QuickConsultationServiceImpl.java-529-532 (1)
529-532: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve existing signature state when the update payload omits the flag.
The update path defaults absence to
falseand then persists it through the flow update. That can reset an existing signed state for clients that do not send the new field.Also applies to: 620-621
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/quickConsultation/QuickConsultationServiceImpl.java` around lines 529 - 532, The update flow in QuickConsultationServiceImpl is resetting doctorSignatureFlag to false when the request omits it, which can overwrite an existing signed state. Change the handling around quickConsultDoctorOBJ so the flag is only updated when the payload explicitly includes doctorSignatureFlag, and otherwise preserve the current persisted value through the flow update. Apply the same fix in both the signature extraction and downstream update/persistence logic that uses doctorSignatureFlag.src/main/java/com/iemr/tm/service/common/transaction/CommonDoctorServiceImpl.java-685-689 (1)
685-689: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAvoid inserting a second referral row after updating existing rows.
Lines 640-652 already update existing referral rows; this fallback then saves a fresh
referDetailswhen no additional-service list is present, causing duplicate referral rows on update. Only add the fallback row when there were no existing statuses to update.Proposed fix
- } else { + } else if (benReferDetailsStatuses == null || benReferDetailsStatuses.isEmpty()) { if (referDetails.getReferredToInstituteName() != null || referDetails.getRevisitDate() != null || referDetails.getReferralReason() != null) { referDetailsList.add(referDetails); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/common/transaction/CommonDoctorServiceImpl.java` around lines 685 - 689, The fallback in CommonDoctorServiceImpl should not add a new referDetails row when the earlier referral-status update path has already found existing rows to update. In the method handling referral persistence, guard the else branch so referDetailsList only gets a fresh entry when there were no existing statuses/records updated (and the additional-service list is absent), using the existing referral update flow and referDetailsList as the key points to adjust.src/main/java/com/iemr/tm/service/anc/ANCServiceImpl.java-353-356 (1)
353-356: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove signature parsing inside the existing null guard.
Line 354 dereferences
requestOBJbefore Line 358 checks it, so a null input now throws immediately instead of following the existing null-handling path.Proposed fix
- Boolean doctorSignatureFlag = false; - if (requestOBJ.has("doctorSignatureFlag") && !requestOBJ.get("doctorSignatureFlag").isJsonNull()) { - doctorSignatureFlag = requestOBJ.get("doctorSignatureFlag").getAsBoolean(); - } - if (requestOBJ != null) { + Boolean doctorSignatureFlag = false; + if (requestOBJ.has("doctorSignatureFlag") && !requestOBJ.get("doctorSignatureFlag").isJsonNull()) { + doctorSignatureFlag = requestOBJ.get("doctorSignatureFlag").getAsBoolean(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/anc/ANCServiceImpl.java` around lines 353 - 356, Move the doctor signature parsing in ANCServiceImpl into the existing request null-handling path so requestOBJ is never dereferenced before the null check; update the logic around doctorSignatureFlag to first verify requestOBJ is non-null, then read the doctorSignatureFlag field only within that guard, preserving the existing fallback behavior when the request is null.src/main/java/com/iemr/tm/service/cancerScreening/CSServiceImpl.java-814-820 (1)
814-820: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove
doctorSignatureFlagparsing behind the null guard.Line 815 dereferences
requestOBJbefore Line 820 checks it for null, so a null request now fails with an NPE instead of the existing invalid-input path.Proposed fix
- Boolean doctorSignatureFlag = false; - if (requestOBJ.has("doctorSignatureFlag") && !requestOBJ.get("doctorSignatureFlag").isJsonNull()) { - doctorSignatureFlag = requestOBJ.get("doctorSignatureFlag").getAsBoolean(); - } - - - if (requestOBJ != null && requestOBJ.has("diagnosis") && !requestOBJ.get("diagnosis").isJsonNull()) { + Boolean doctorSignatureFlag = false; + if (requestOBJ.has("doctorSignatureFlag") && !requestOBJ.get("doctorSignatureFlag").isJsonNull()) { + doctorSignatureFlag = requestOBJ.get("doctorSignatureFlag").getAsBoolean(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/cancerScreening/CSServiceImpl.java` around lines 814 - 820, The doctorSignatureFlag parsing in CSServiceImpl should be moved behind the existing requestOBJ null guard because it currently dereferences requestOBJ before the null check. Update the request handling logic so the doctorSignatureFlag extraction happens only after confirming requestOBJ is non-null, using the same pattern already applied to the diagnosis parsing block, to preserve the invalid-input path and avoid an NPE.
🟡 Minor comments (3)
src/main/java/com/iemr/tm/controller/common/main/WorklistController.java-1031-1031 (1)
1031-1031: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude the
TCSPECIALISTalias for session extension.Other TC specialist endpoints accept both
TC_SPECIALISTandTCSPECIALIST, but this role list only includesTC_SPECIALIST. Users with the alias can be denied session extension.Proposed fix
- `@PreAuthorize`("hasRole('NURSE') || hasRole('PHARMACIST') || hasRole('LABTECHNICIAN') || hasRole('REGISTRAR') || hasRole('DATASYNC') || hasRole('DATA_SYNC') || hasRole('DOCTOR') || hasRole('LAB_TECHNICIAN') || hasRole('TC_SPECIALIST') || hasRole('ONCOLOGIST') || hasRole('RADIOLOGIST')") + `@PreAuthorize`("hasRole('NURSE') || hasRole('PHARMACIST') || hasRole('LABTECHNICIAN') || hasRole('REGISTRAR') || hasRole('DATASYNC') || hasRole('DATA_SYNC') || hasRole('DOCTOR') || hasRole('LAB_TECHNICIAN') || hasRole('TC_SPECIALIST') || hasRole('TCSPECIALIST') || hasRole('ONCOLOGIST') || hasRole('RADIOLOGIST')")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/controller/common/main/WorklistController.java` at line 1031, The session extension role check in WorklistController is missing the TCSPECIALIST alias, so users with that alias can be blocked even though other TC specialist endpoints allow it. Update the `@PreAuthorize` expression on the session extension endpoint to include both TC_SPECIALIST and TCSPECIALIST, matching the existing role-alias pattern used elsewhere in the controller.src/main/java/com/iemr/tm/controller/login/IemrMmuLoginController.java-49-49 (1)
49-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep TC specialist role aliases consistent.
Line 49 allows
TC_SPECIALISTbut notTCSPECIALIST, while other controller methods support both aliases. Add the missing alias to avoid denying valid TC specialist users.Proposed fix
-@PreAuthorize("hasRole('NURSE') || hasRole('PHARMACIST') || hasRole('LABTECHNICIAN') || hasRole('REGISTRAR') || hasRole('DATASYNC') || hasRole('DATA_SYNC') || hasRole('DOCTOR') || hasRole('LAB_TECHNICIAN') || hasRole('TC_SPECIALIST') || hasRole('ONCOLOGIST') || hasRole('RADIOLOGIST') || hasRole('ASHA')") +@PreAuthorize("hasRole('NURSE') || hasRole('PHARMACIST') || hasRole('LABTECHNICIAN') || hasRole('REGISTRAR') || hasRole('DATASYNC') || hasRole('DATA_SYNC') || hasRole('DOCTOR') || hasRole('LAB_TECHNICIAN') || hasRole('TC_SPECIALIST') || hasRole('TCSPECIALIST') || hasRole('ONCOLOGIST') || hasRole('RADIOLOGIST') || hasRole('ASHA')")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/controller/login/IemrMmuLoginController.java` at line 49, The role check in IemrMmuLoginController is missing the TC specialist alias used elsewhere, so valid users with that role can be denied access. Update the `@PreAuthorize` expression on the login controller method to include both TC_SPECIALIST and TCSPECIALIST, matching the alias handling already used in the other controller methods and keeping the role names consistent.src/main/java/com/iemr/tm/repo/labModule/ECGAbnormalFindingMasterRepo.java-31-34 (1)
31-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn the master list in a deterministic order.
findByDeleted(Boolean deleted)leaves row order up to the database, so the ECG findings list can reshuffle between calls. Prefer an ordered query such asfindByDeletedFalseOrderByFindingNameAsc()(or byFindingIDif that is the canonical sequence).Suggested repository signature
-public interface ECGAbnormalFindingMasterRepo extends CrudRepository<ECGAbnormalFindingMaster, Integer> { - - List<ECGAbnormalFindingMaster> findByDeleted(Boolean deleted); +public interface ECGAbnormalFindingMasterRepo extends CrudRepository<ECGAbnormalFindingMaster, Integer> { + + List<ECGAbnormalFindingMaster> findByDeletedFalseOrderByFindingNameAsc(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/repo/labModule/ECGAbnormalFindingMasterRepo.java` around lines 31 - 34, The ECG abnormal findings repository currently returns rows in an undefined order, which can cause the master list to reshuffle between calls. Update ECGAbnormalFindingMasterRepo by replacing or supplementing findByDeleted(Boolean deleted) with an ordered derived query such as findByDeletedFalseOrderByFindingNameAsc() (or by FindingID if that is the canonical sort), so callers always receive a deterministic list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fac3adae-e57e-4436-84c0-9aae64a676c5
📒 Files selected for processing (67)
pom.xmlsrc/main/environment/common_ci.propertiessrc/main/environment/common_docker.propertiessrc/main/environment/common_example.propertiessrc/main/java/com/iemr/tm/controller/anc/AntenatalCareController.javasrc/main/java/com/iemr/tm/controller/cancerscreening/CancerScreeningController.javasrc/main/java/com/iemr/tm/controller/common/main/WorklistController.javasrc/main/java/com/iemr/tm/controller/common/master/CommonMasterController.javasrc/main/java/com/iemr/tm/controller/covid19/CovidController.javasrc/main/java/com/iemr/tm/controller/dataSyncActivity/StartSyncActivity.javasrc/main/java/com/iemr/tm/controller/dataSyncLayerCentral/MMUDataSyncVanToServer.javasrc/main/java/com/iemr/tm/controller/foetalmonitor/FoetalMonitorController.javasrc/main/java/com/iemr/tm/controller/generalOPD/GeneralOPDController.javasrc/main/java/com/iemr/tm/controller/health/HealthController.javasrc/main/java/com/iemr/tm/controller/labtechnician/LabtechnicianController.javasrc/main/java/com/iemr/tm/controller/location/LocationController.javasrc/main/java/com/iemr/tm/controller/login/IemrMmuLoginController.javasrc/main/java/com/iemr/tm/controller/ncdCare/NCDCareController.javasrc/main/java/com/iemr/tm/controller/ncdscreening/NCDScreeningController.javasrc/main/java/com/iemr/tm/controller/nurse/vitals/AnthropometryVitalsController.javasrc/main/java/com/iemr/tm/controller/patientApp/master/PatientAppCommonMasterController.javasrc/main/java/com/iemr/tm/controller/pnc/PostnatalCareController.javasrc/main/java/com/iemr/tm/controller/quickconsult/QuickConsultController.javasrc/main/java/com/iemr/tm/controller/registrar/main/RegistrarController.javasrc/main/java/com/iemr/tm/controller/report/CRMReportController.javasrc/main/java/com/iemr/tm/controller/snomedct/SnomedController.javasrc/main/java/com/iemr/tm/controller/teleconsultation/TeleConsultationController.javasrc/main/java/com/iemr/tm/controller/version/VersionController.javasrc/main/java/com/iemr/tm/controller/videoconsultationcontroller/VideoConsultationController.javasrc/main/java/com/iemr/tm/data/benFlowStatus/BeneficiaryFlowStatus.javasrc/main/java/com/iemr/tm/data/labModule/ECGAbnormalFindingMaster.javasrc/main/java/com/iemr/tm/data/labModule/LabResultEntry.javasrc/main/java/com/iemr/tm/data/ncdcare/NCDCareDiagnosis.javasrc/main/java/com/iemr/tm/repo/benFlowStatus/BeneficiaryFlowStatusRepo.javasrc/main/java/com/iemr/tm/repo/labModule/ECGAbnormalFindingMasterRepo.javasrc/main/java/com/iemr/tm/repo/login/UserLoginRepo.javasrc/main/java/com/iemr/tm/repo/nurse/ncdcare/NCDCareDiagnosisRepo.javasrc/main/java/com/iemr/tm/repo/quickConsultation/BenClinicalObservationsRepo.javasrc/main/java/com/iemr/tm/service/anc/ANCServiceImpl.javasrc/main/java/com/iemr/tm/service/benFlowStatus/CommonBenStatusFlowServiceImpl.javasrc/main/java/com/iemr/tm/service/cancerScreening/CSServiceImpl.javasrc/main/java/com/iemr/tm/service/common/master/CommonMasterServiceImpl.javasrc/main/java/com/iemr/tm/service/common/master/CommonMaterService.javasrc/main/java/com/iemr/tm/service/common/transaction/CommonDoctorServiceImpl.javasrc/main/java/com/iemr/tm/service/common/transaction/CommonNurseServiceImpl.javasrc/main/java/com/iemr/tm/service/common/transaction/CommonServiceImpl.javasrc/main/java/com/iemr/tm/service/covid19/Covid19ServiceImpl.javasrc/main/java/com/iemr/tm/service/generalOPD/GeneralOPDServiceImpl.javasrc/main/java/com/iemr/tm/service/health/HealthService.javasrc/main/java/com/iemr/tm/service/labtechnician/LabTechnicianServiceImpl.javasrc/main/java/com/iemr/tm/service/ncdCare/NCDCareServiceImpl.javasrc/main/java/com/iemr/tm/service/ncdscreening/NCDSCreeningDoctorServiceImpl.javasrc/main/java/com/iemr/tm/service/ncdscreening/NCDScreeningServiceImpl.javasrc/main/java/com/iemr/tm/service/pnc/PNCServiceImpl.javasrc/main/java/com/iemr/tm/service/quickConsultation/QuickConsultationServiceImpl.javasrc/main/java/com/iemr/tm/utils/CookieUtil.javasrc/main/java/com/iemr/tm/utils/IntegerListConverter.javasrc/main/java/com/iemr/tm/utils/JwtAuthenticationUtil.javasrc/main/java/com/iemr/tm/utils/JwtUserIdValidationFilter.javasrc/main/java/com/iemr/tm/utils/JwtUtil.javasrc/main/java/com/iemr/tm/utils/StringListConverter.javasrc/main/java/com/iemr/tm/utils/exception/CustomAccessDeniedHandler.javasrc/main/java/com/iemr/tm/utils/exception/CustomAuthenticationEntryPoint.javasrc/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.javasrc/main/java/com/iemr/tm/utils/mapper/RoleAuthenticationFilter.javasrc/main/java/com/iemr/tm/utils/mapper/SecurityConfig.javasrc/main/java/com/iemr/tm/utils/redis/RedisStorage.java
| @RequestMapping(value = "/dataSyncActivity", headers = "Authorization", consumes = "application/json", produces = "application/json") | ||
| @PreAuthorize("hasRole('DATASYNC') || hasRole('DATA_SYNC') ") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for data-sync role seeds, mappings, tests, or auth-token handling around sync callers.
rg -n -C4 --iglob '*.java' --iglob '*.sql' --iglob '*.properties' --iglob '*.xml' \
'(DATASYNC|DATA_SYNC|ServerAuthorization|dataSyncActivity|/dataSync|m_userservicerolemapping)' .Repository: PSMRI/TM-API
Length of output: 26817
🏁 Script executed:
rg -n "ServerAuthorization" --type java -A 5 -B 5 | rg -i "filter|context|security|authentication" -A 10 -B 10Repository: PSMRI/TM-API
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Check for any SecurityFilter, Filter, or Interceptor that handles ServerAuthorization
rg -l "Filter" --type java | xargs -I {} sh -c 'echo "=== {} ==="; cat {}' | rg -C 5 "ServerAuthorization"Repository: PSMRI/TM-API
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Check for any SecurityFilter, Filter, or Interceptor that handles ServerAuthorization explicitly
rg -n "ServerAuthorization" --type java -A 10 | rg -C 10 "SecurityContext|Authentication"Repository: PSMRI/TM-API
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Search for security configuration or filters handling ServerAuthorization
rg -n "ServerAuthorization" --type java -A 5 -B 5Repository: PSMRI/TM-API
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Search for security configuration or filters handling ServerAuthorization (case-insensitive)
rg -in "serverauthorization" --type java -A 5 -B 5Repository: PSMRI/TM-API
Length of output: 150
Verify role resolution for ServerAuthorization-based sync calls.
The @PreAuthorize check at lines 49-50 relies on the Spring Authentication derived from the Authorization (JWT) header, but the sync logic delegates to services using the ServerAuthorization header. There is no evidence of a filter or interceptor mapping ServerAuthorization credentials to the DATASYNC or DATA_SYNC security roles. This disconnect implies that valid sync requests failing to provide a JWT with these roles will be incorrectly blocked, or the check is only protecting endpoints that use JWT roles and not the ServerAuthorization mechanism. Ensure the security configuration correctly maps sync credentials to these roles or remove the role check if ServerAuthorization is the intended authority.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/tm/controller/dataSyncActivity/StartSyncActivity.java`
around lines 49 - 50, The `StartSyncActivity` endpoint’s `@PreAuthorize` guard
is tied to Spring `Authentication` from the `Authorization` JWT, but the sync
flow uses `ServerAuthorization`, so the role check may not reflect the real auth
mechanism. Update the security setup around `StartSyncActivity` to either map
`ServerAuthorization` credentials into `DATASYNC`/`DATA_SYNC` authorities before
`@PreAuthorize` runs, or remove/replace the role check if `ServerAuthorization`
is the intended source of trust. Verify the behavior in the controller entry
point and the related security filter/interceptor configuration.
| CookieCsrfTokenRepository csrfTokenRepository = new CookieCsrfTokenRepository(); | ||
| csrfTokenRepository.setCookieHttpOnly(true); | ||
| csrfTokenRepository.setCookiePath("/"); | ||
| http | ||
| .csrf(csrf -> csrf.disable()) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether authentication can be derived from cookies and whether CSRF config is intentionally unused.
fd -i 'CookieUtil\.java|Jwt.*Filter\.java|RoleAuthenticationFilter\.java|JwtAuthenticationUtil\.java|SecurityConfig\.java' src/main/java --exec sh -c '
echo "### $1"
rg -n -C2 "Cookie|cookies|getCookies|Authorization|csrf|CookieCsrfTokenRepository|setCookie|addCookie" "$1"
' sh {}Repository: PSMRI/TM-API
Length of output: 7179
🏁 Script executed:
#!/bin/bash
# Read the full SecurityConfig.java to verify the context and line numbers.
cat -n src/main/java/com/iemr/tm/utils/mapper/SecurityConfig.javaRepository: PSMRI/TM-API
Length of output: 3098
Disable CSRF protection while using cookies for authentication creates a security vulnerability.
The code configures CookieCsrfTokenRepository (lines 35-37) but immediately disables CSRF globally at line 39. Since CookieUtil and JwtUserIdValidationFilter extract the Jwttoken from cookies for authentication, disabling CSRF exposes the application to cross-site request forgery attacks on all authenticated state-changing endpoints.
- Remove Dead Code: The
CookieCsrfTokenRepositoryconfiguration is unused and should be removed. - Enable CSRF: Remove
.csrf(csrf -> csrf.disable())to enforce CSRF protection for cookie-based authentication.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 37-38: Do not disable CSRF
Context: http
.csrf(csrf -> csrf.disable())
Note: [CWE-352] Cross-Site Request Forgery (CSRF).
(spring-csrf-disable)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/tm/utils/mapper/SecurityConfig.java` around lines 35 -
39, The SecurityConfig setup is configuring a CookieCsrfTokenRepository but then
disabling CSRF entirely, leaving cookie-based authentication exposed; update the
HttpSecurity configuration to keep CSRF protection enabled and remove the unused
CookieCsrfTokenRepository setup if it is not being applied anywhere. Make the
fix in SecurityConfig where the security filter chain is built, and ensure the
cookie-based Jwttoken flow used by CookieUtil and JwtUserIdValidationFilter
remains protected by CSRF rather than bypassed.
Source: Linters/SAST tools
| .csrf(csrf -> csrf.disable()) | ||
| .sessionManagement(session -> session.sessionCreationPolicy(SessionCreationPolicy.STATELESS)) | ||
| .authorizeHttpRequests(auth -> auth | ||
| .requestMatchers("/user/*").permitAll() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List user/login controller mappings to verify what /user/* exposes publicly.
fd -i 'IemrMmuLoginController\.java|.*User.*Controller\.java|.*Login.*Controller\.java' src/main/java --exec sh -c '
echo "### $1"
rg -n -C3 "@(RequestMapping|GetMapping|PostMapping|PutMapping|DeleteMapping|PatchMapping)|/user|password|profile|role|admin" "$1"
' sh {}Repository: PSMRI/TM-API
Length of output: 2057
Restrict public access to /user/* endpoints internally protected by roles
The pattern .requestMatchers("/user/*").permitAll() is too broad. While the base path for IemrMmuLoginController includes a headers constraint, the route /user/* encompasses internal business logic endpoints (e.g., getUserServicePointVanDetails) that require role-based authentication (hasRole) and are not intended for public access. Relying on method-level @PreAuthorize to block anonymous traffic allowed by the URL matcher introduces a security risk.
Replace the broad wildcard with specific public endpoints or remove permitAll() for this path entirely.
Diff context
.requestMatchers("/user/*").permitAll()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/tm/utils/mapper/SecurityConfig.java` at line 42,
Restrict the overly broad public matcher in SecurityConfig: the current
requestMatchers("/user/*").permitAll() exposes internal user endpoints that
should be role-protected. Update the security configuration to remove this
wildcard permitAll or replace it with only the specific public routes used by
IemrMmuLoginController, and leave protected user operations to the existing
hasRole / `@PreAuthorize` controls so endpoints like getUserServicePointVanDetails
are not anonymously reachable.
7 participants
📋 Description
JIRA ID:
Moving 3.6.2 to main
✅ 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