Release 3.6.0 #317
Release 3.6.0 #317
Conversation
* fix: allowed file format for upload * fix: changed application properties * fix: removed firebase enabled * fix: added firebase enabled
Removed allowed origin for CORS configuration.
Merging 3.6.1 into 3.6.0 release
Release 3.7.0
Release 3.8.0
WalkthroughAdds Firebase Cloud Messaging support, OTP-based beneficiary consent, dynamic form management, platform-feedback with Redis rate-limiting, input sanitization and validation, many new JPA entities/repositories/controllers/services, global exception/response handling, configuration/property additions, and bumps project version to 3.6.0. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as BeneficiaryConsentController
participant OTPHandler as BeneficiaryOTPHandlerImpl
participant Cache as GuavaCache
participant SMS as SMS Gateway
Client->>Controller: POST /beneficiaryConsent/sendConsent {mobNo}
Controller->>OTPHandler: sendOTP(BeneficiaryConsentRequest)
OTPHandler->>OTPHandler: generate 6-digit OTP (SecureRandom)
OTPHandler->>OTPHandler: hash OTP (SHA-256)
OTPHandler->>Cache: store hashed OTP (key: mobNo, ttl 5m)
OTPHandler->>SMS: POST send SMS payload (template + OTP)
SMS-->>OTPHandler: response (200 / error)
OTPHandler-->>Controller: status/result
Controller-->>Client: JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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}
| spring.mail.host=smtp.gmail.com | ||
| spring.mail.port=587 | ||
| spring.mail.username=<Enter mail username> | ||
| spring.mail.password=<Enter mail password> |
Check failure
Code scanning / SonarCloud
Credentials should not be hard-coded High
| @Operation(summary = "Resend Consent") | ||
| @RequestMapping(value = "/resendConsent", method = RequestMethod.POST, consumes = MediaType.APPLICATION_JSON, produces = MediaType.APPLICATION_JSON) | ||
| public String resendConsent(@Param(value = "{\"mobNo\":\"String\"}") @RequestBody String requestOBJ) { | ||
| logger.info(requestOBJ.toString()); |
Check notice
Code scanning / SonarCloud
Logging should not be vulnerable to injection attacks Low
| header.put("Authorization", auth); | ||
| } | ||
|
|
||
| logger.info("Request to generate ben IDs: " + request); |
Check notice
Code scanning / SonarCloud
Logging should not be vulnerable to injection attacks Low
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
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 (4)
src/main/java/com/iemr/common/service/scheme/SchemeServiceImpl.java (1)
104-113: Remove dead code.Lines 106-113 construct a complete download URI with credentials, but this URI is never returned (line 114 comments out the return). This dead code should be removed entirely to improve maintainability and eliminate the security risk of embedded credentials.
See the fix suggested in the previous comment for lines 114-116.
src/main/java/com/iemr/common/utils/http/HttpUtils.java (2)
79-84: Critical race condition: shared mutable headers in singleton component.HttpUtils is a Spring
@Component(singleton by default), but methodsgetV1andpost(uri, json)mutate the shared class-levelheadersfield (lines 80, 107). WhenRestTemplateUtil.getJwttokenFromHeaders(headers)adds JWT/Cookie/User-Agent headers, concurrent requests will cause race conditions: threads can see each other's tokens, experience missing headers, or encounter corrupted header state.Apply this diff to use local headers in both methods:
public ResponseEntity<String> getV1(String uri) throws URISyntaxException, MalformedURLException { - RestTemplateUtil.getJwttokenFromHeaders(headers); - HttpEntity<String> requestEntity = new HttpEntity<String>("", headers); + HttpHeaders requestHeaders = new HttpHeaders(); + requestHeaders.add("Content-Type", "application/json"); + RestTemplateUtil.getJwttokenFromHeaders(requestHeaders); + HttpEntity<String> requestEntity = new HttpEntity<String>("", requestHeaders); ResponseEntity<String> responseEntity = rest.exchange(uri, HttpMethod.GET, requestEntity, String.class); return responseEntity; }public String post(String uri, String json) { String body; - RestTemplateUtil.getJwttokenFromHeaders(headers); - HttpEntity<String> requestEntity = new HttpEntity<String>(json, headers); + HttpHeaders requestHeaders = new HttpHeaders(); + requestHeaders.add("Content-Type", "application/json"); + RestTemplateUtil.getJwttokenFromHeaders(requestHeaders); + HttpEntity<String> requestEntity = new HttpEntity<String>(json, requestHeaders); ResponseEntity<String> responseEntity = rest.exchange(uri, HttpMethod.POST, requestEntity, String.class); setStatus((HttpStatus) responseEntity.getStatusCode()); body = responseEntity.getBody(); return body; }Also applies to: 105-113
132-181: Add JWT token injection touploadFilemethod for consistency.All other HTTP methods (
get,post) now inject JWT tokens viaRestTemplateUtil.getJwttokenFromHeaders(), butuploadFiledoes not. AddRestTemplateUtil.getJwttokenFromHeaders(headers);after line 145 (after headers are configured) to maintain consistent authentication handling across all HTTP utility methods.Additionally, note that
uploadFileappears to be unused in the codebase. Consider removing it if it's no longer needed, or update it with JWT injection if it's meant to be retained for future use.src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (1)
140-140: Critical: Password logged in plain text.This log statement includes the user's password, which will be written to log files in clear text. This is a serious security vulnerability.
- logger.info("userAuthenticate request - " + m_User + " " + m_User.getUserName() + " " + m_User.getPassword()); + logger.info("userAuthenticate request for user: {}", m_User.getUserName());
🟠 Major comments (25)
src/main/java/com/iemr/common/service/cti/CTIServiceImpl.java-308-308 (1)
308-308: Remove logging of URLs containing credentials.The URL logged at this line contains the decrypted password, which violates security best practices. Credentials should never be logged as they may be exposed in log files, monitoring systems, or error tracking tools.
Apply this diff to remove or sanitize the log statement:
-logger.info("calling URL " + ctiURI); +logger.info("calling getLoginKey API for username: " + (agentState.getUsername() != null ? agentState.getUsername() : "unknown"));Alternatively, implement a URL sanitization utility that masks sensitive query parameters before logging.
Committable suggestion skipped: line range outside the PR's diff.
src/main/java/com/iemr/common/service/beneficiary/IdentityBeneficiaryServiceImpl.java-409-410 (1)
409-410: Avoid logging potentially sensitive request payloads.Logging the full request payload may expose sensitive beneficiary PII (names, addresses, health IDs, etc.), creating compliance risks under GDPR/CCPA. Consider logging only non-sensitive metadata (e.g., request size, correlation ID) or redacting sensitive fields.
logger.info("Calling URL: {}", apiUrl); - logger.info("Request Payload: {}", request); + logger.debug("Request received for identity creation");src/main/java/com/iemr/common/service/beneficiary/IdentityBeneficiaryServiceImpl.java-534-538 (1)
534-538: Avoid logging sensitive beneficiary data; fix indentation.
- PII Risk: Logging full request and response payloads (lines 535, 538) may expose sensitive beneficiary identifiers, creating compliance risks.
- Formatting: Line 538 lacks proper indentation.
- logger.info("Request to generate ben IDs: " + request); - logger.info("Generating ben IDs API URL: " + BEN_GEN + BEN_GEN_API_URL); + logger.debug("Generating beneficiary IDs"); + logger.info("Generating ben IDs API URL: {}{}", BEN_GEN, BEN_GEN_API_URL); result = httpUtils.post(BEN_GEN + BEN_GEN_API_URL, request, header); -logger.info("Response from generate ben IDs: " + result); + logger.debug("Beneficiary IDs generation completed");Also prefer parameterized logging (
{}) over string concatenation for performance.src/main/java/com/iemr/common/model/beneficiary/BeneficiaryModel.java-81-82 (1)
81-82: Rename field to follow JavaBeans conventions and clarify relationship with existingbeneficiaryConsentfield.Same issue as in CommonIdentityDTO: the field name
isConsentviolates Java naming conventions. With Lombok's@Data, this generatesgetIsConsent()instead of the expectedisConsent()getter.This class already has a
beneficiaryConsentfield at line 330. Clarify the distinction between these two consent-related fields.Apply this diff:
- @Expose - private Boolean isConsent=false; + @Expose + private Boolean consent = false;Note: Document the difference between
consentandbeneficiaryConsentif both are required.src/main/java/com/iemr/common/dto/identity/CommonIdentityDTO.java-57-58 (1)
57-58: RenameisConsenttoconsentto fix JavaBeans naming convention violation.The field name
isConsentviolates Java naming conventions for Boolean properties. With Lombok's@Data, this generatesgetIsConsent()instead ofisConsent(), causing incorrect JSON serialization and API inconsistency. The field is actively used inRegisterBenificiaryServiceImpl.java(line 198), where it's mapped via Gson serialization.Additionally,
CommonIdentityDTOalready has a separatebeneficiaryConsentfield at line 154. TheisConsentfield lacks the@Exposeannotation whilebeneficiaryConsenthas it, suggesting different purposes, but this distinction is undocumented.Apply this diff to fix the naming convention:
- private Boolean isConsent=false; + private Boolean consent = false;Update the corresponding setter call in
RegisterBenificiaryServiceImpl.javaline 198 fromsetIsConsent()tosetConsent().Document the purpose and relationship between
consentandbeneficiaryConsentfields to clarify their distinct roles.Committable suggestion skipped: line range outside the PR's diff.
src/main/java/com/iemr/common/model/beneficiary/BeneficiaryModel.java-84-110 (1)
84-110: Use proper temporal types for date/time fields instead of String.The
dateOfDeath(line 88) andtimeOfDeath(line 94) fields are declared as String, which is inconsistent with the existing pattern in this class where date/time values useTimestamp(e.g.,dOB,marriageDate,createdDate).Using String for dates/times leads to:
- Format inconsistencies across the application
- Loss of type safety and validation
- Parsing errors and timezone handling issues
- Difficulty in date arithmetic and comparisons
This is particularly critical for healthcare data where precise date/time recording is essential.
Additionally, the field name
isDeathValue(line 91) is unclear—the "is" prefix suggests Boolean, but it's declared as String.Apply this diff to use proper temporal types:
@Expose private Boolean isDeath; @Expose - private String dateOfDeath; + @JsonFormat(pattern = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'") + private Timestamp dateOfDeath; @Expose - private String isDeathValue; + private String deathStatus; // or clarify the intended meaning @Expose - private String timeOfDeath; + @JsonFormat(pattern = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'") + private Timestamp timeOfDeath;Include the
@JsonFormatannotation to match the serialization pattern used bydOBandcreatedDatefields.src/main/java/com/iemr/common/service/kmfilemanager/KMFileManagerServiceImpl.java-137-139 (1)
137-139: Catch block swallows validation and IO errors, misreporting upload successBoth catch blocks (lines 141-144 and 230-231) catch generic
Exceptionand log onlygetMessage(), losing stack traces. The method returnssavedFileManagersregardless, allowing callers to mistake partial failures for success.Lines 141-144:
catch(Exception e) { logger.error("error " + e.getMessage()); }Lines 230-231:
catch(Exception e) { logger.error("File upload failed for user " + userId + " " + e.getMessage()); }Replace with specific exception handling that logs the full exception and rethrows:
catch (IOException | NoSuchAlgorithmException e) { logger.error("Error while uploading KM files", e); throw e; } catch (RuntimeException e) { logger.error("Error while uploading KM files", e); throw e; } finally { if (newFile != null) { newFile.close(); } if (fis != null) { fis.close(); } }This preserves stream cleanup, surfaces real failures to callers, and enables production debugging with full stack traces.
src/main/java/com/iemr/common/service/kmfilemanager/KMFileManagerServiceImpl.java-144-176 (1)
144-176: Normalize extension allow-list to handle whitespace and case variationsThe extension validation logic has a case and whitespace sensitivity issue that will cause valid files to be rejected with common configuration patterns. When
allowed.file.extensions=pdf, docx(with spaces) orPDF,DOCX(uppercase), theArrays.asList(allowedFileExtensions.split(","))produces list entries like["pdf", " docx"]with preserved whitespace and case, but the validation checks againstextensionFromName.toLowerCase(). This mismatch causes thecontains()check to fail for entries with leading spaces or different casing.Normalize the configuration list to lowercase and remove whitespace:
- List<String> allowedExtensions = Arrays.asList(allowedFileExtensions.split(",")); + List<String> allowedExtensions = Arrays.asList( + allowedFileExtensions.toLowerCase().split(",")); + allowedExtensions = allowedExtensions.stream() + .map(String::trim) + .collect(Collectors.toList());Or more concisely:
- List<String> allowedExtensions = Arrays.asList(allowedFileExtensions.split(",")); + List<String> allowedExtensions = Arrays.stream(allowedFileExtensions.toLowerCase().split(",")) + .map(String::trim) + .collect(Collectors.toList());This ensures the allow-list matches the normalized extension being checked.
Committable suggestion skipped: line range outside the PR's diff.
src/main/java/com/iemr/common/service/beneficiary/RegisterBenificiaryServiceImpl.java-79-80 (1)
79-80: Harden welcome‑SMS logic against null/empty phone lists.The new
WelcomeBenificarySmsServiceintegration at line 80 is a good addition, but the SMS invocation at lines 226–230 is unsafe:if(beneficiary!=null){ if(beneficiary.getBenPhoneMaps().get(0).getPhoneNo()!=null){ welcomeBenificarySmsService.sendWelcomeSMStoBenificiary(beneficiary.getBenPhoneMaps().get(0).getPhoneNo(),beneficiary.getFirstName()+" "+beneficiary.getLastName(),beneficiary.getBeneficiaryID()); } }This assumes
getBenPhoneMaps()returns a non-null, non-empty list. If a beneficiary has no phone records or the list is null, this will throwNullPointerExceptionorIndexOutOfBoundsException. Elsewhere in the codebase (e.g.,IEMRSearchUserServiceImpl), similar code correctly guards both the list and its size:if (beneficiary.getBenPhoneMaps() != null && beneficiary.getBenPhoneMaps().size() > 0) { // access .get(0) safely }Apply the same defensive pattern here to prevent runtime failures when phone data is absent.
src/main/java/com/iemr/common/exception/ValidationExceptionHandler.java-24-28 (1)
24-28: Unsafe cast may cause ClassCastException.
getAllErrors()returns bothFieldErrorandObjectError(class-level errors). Casting all toFieldErrorwill throwClassCastExceptionfor class-level validation errors (e.g.,@ScriptAssert, cross-field validations).Apply this diff to safely handle both error types:
- ex.getBindingResult().getAllErrors().forEach((error) -> { - String fieldName = ((FieldError) error).getField(); - String errorMessage = error.getDefaultMessage(); - errors.put(fieldName, errorMessage); - }); + ex.getBindingResult().getAllErrors().forEach((error) -> { + String fieldName = (error instanceof FieldError) + ? ((FieldError) error).getField() + : error.getObjectName(); + String errorMessage = error.getDefaultMessage(); + errors.put(fieldName, errorMessage); + });src/main/java/com/iemr/common/repository/beneficiary/BeneficiaryDetailsRmnchRepository.java-1-10 (1)
1-10: Do not commit entirely commented-out code.This file contains only commented-out code, which is effectively dead code. For a production release (v3.6.0), this should either be:
- Uncommented and properly implemented if needed, or
- Removed entirely and added in a future PR when ready
If the entity
RMNCHBeneficiaryDetailsRmnchis being used, uncomment the repository:-//package com.iemr.common.repository.beneficiary; -// -//import com.iemr.common.model.beneficiary.RMNCHBeneficiaryDetailsRmnch; -//import org.springframework.data.jpa.repository.JpaRepository; -//import org.springframework.stereotype.Repository; -// -//@Repository -//public interface BeneficiaryDetailsRmnchRepository extends JpaRepository<RMNCHBeneficiaryDetailsRmnch,Long> { -// -//} +package com.iemr.common.repository.beneficiary; + +import com.iemr.common.model.beneficiary.RMNCHBeneficiaryDetailsRmnch; +import org.springframework.data.jpa.repository.JpaRepository; +import org.springframework.stereotype.Repository; + +@Repository +public interface BeneficiaryDetailsRmnchRepository extends JpaRepository<RMNCHBeneficiaryDetailsRmnch, Long> { + +}Otherwise, remove this file from the release.
src/main/java/com/iemr/common/utils/exception/GlobalExceptionHandler.java-16-20 (1)
16-20: Add logging to the catch-all exception handler.The generic exception handler swallows the actual exception without logging it. This makes production debugging very difficult when unexpected errors occur.
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + @RestControllerAdvice public class GlobalExceptionHandler { + private static final Logger logger = LoggerFactory.getLogger(GlobalExceptionHandler.class); @ExceptionHandler(Exception.class) public ResponseEntity<ApiResponse<Object>> handleException(Exception ex) { + logger.error("Unhandled exception occurred", ex); return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) .body(ApiResponse.error("Something went wrong", HttpStatus.INTERNAL_SERVER_ERROR.value(), null)); }Committable suggestion skipped: line range outside the PR's diff.
pom.xml-115-119 (1)
115-119: Review transitive dependencies of firebase-admin 9.4.3 for vulnerabilities.Version 9.4.3 was released January 9, 2025 and is current. However, it has a transitive dependency vulnerability: CVE-2025-24970 in Netty (SSL handler DoS). Review your dependency tree for affected versions of Netty and consider upgrading to a later firebase-admin release that patches this dependency, or update Netty directly.
src/main/java/com/iemr/common/data/userToken/UserTokenData.java-44-45 (1)
44-45: Add automatic timestamp management for updatedAt.The
updatedAtfield should be automatically maintained by JPA to ensure accurate tracking of token updates. Without automatic management, timestamps must be set manually, which is error-prone.Apply this diff:
+import org.hibernate.annotations.UpdateTimestamp; + @Entity @Table(name = "user_tokens", schema = "db_iemr") @Data public class UserTokenData { @Id @Column(name = "user_id") Integer userId; @Column(name = "token") String token; + @UpdateTimestamp @Column(name = "updated_at") Timestamp updatedAt; }src/main/java/com/iemr/common/controller/beneficiaryConsent/BeneficiaryConsentController.java-83-86 (1)
83-86: Same issue: exception details exposed in response.While the exception is logged (which is good), the error response still exposes raw exception details. Apply the same fix as
sendConsent.} catch (Exception e) { logger.error("error in validating Consent : " + e); - response.setError(500, "error : " + e); + response.setError(500, "Failed to validate consent. Please try again."); }src/main/java/com/iemr/common/controller/beneficiaryConsent/BeneficiaryConsentController.java-108-111 (1)
108-111: Same issue: exception details exposed in response.Apply the same fix to sanitize error responses.
} catch (Exception e) { logger.error("error in re-sending Consent : " + e); - response.setError(500, "error : " + e); + response.setError(500, "Failed to resend consent. Please try again."); }src/main/java/com/iemr/common/controller/beneficiaryConsent/BeneficiaryConsentController.java-93-93 (1)
93-93: Avoid logging raw request body containing sensitive data.Logging the full request body may expose personally identifiable information (mobile numbers) in log files. Consider logging only non-sensitive identifiers or masking the mobile number.
- logger.info(requestOBJ.toString()); + logger.debug("resendConsent request received");src/main/java/com/iemr/common/controller/users/IEMRAdminController.java-83-84 (1)
83-84: Captcha disabled by hardcoding instead of configuration.The captcha enable flag has been changed from a configuration value (
@Value) to a hardcodedfalse. This bypasses a security control without the ability to re-enable it via configuration. If captcha needs to be disabled temporarily, consider keeping the configuration property.-// @Value("${captcha.enable-captcha}") - private boolean enableCaptcha =false; + @Value("${captcha.enable-captcha:false}") + private boolean enableCaptcha;This preserves the ability to enable captcha via configuration while defaulting to
false.src/main/java/com/iemr/common/service/welcomeSms/WelcomeBenificarySmsServiceImpl.java-85-85 (1)
85-85: Avoid logging sensitive data (contact number, beneficiary name/ID).The payload contains PII (contact number, beneficiary name, beneficiary ID). This should not be logged at INFO level, or sensitive fields should be masked.
- logger.info("payload: " + payload); + logger.debug("Sending welcome SMS to destination");src/main/java/com/iemr/common/controller/beneficiaryConsent/BeneficiaryConsentController.java-62-64 (1)
62-64: Avoid exposing raw exception details in API responses.Concatenating the exception object directly into the error message (
"error : " + e) can leak stack traces, internal class names, and potentially sensitive information to API consumers. Use a generic error message for the response and log the full exception server-side.} catch (Exception e) { - response.setError(500, "error : " + e); + logger.error("Error in sendConsent: ", e); + response.setError(500, "Failed to send consent. Please try again."); }Committable suggestion skipped: line range outside the PR's diff.
src/main/java/com/iemr/common/service/platform_feedback/PlatformFeedbackService.java-22-22 (1)
22-22: Package declaration may not match file path.The package is declared as
com.iemr.common.servicebut based on the file path (src/main/java/com/iemr/common/service/platform_feedback/PlatformFeedbackService.java), it should becom.iemr.common.service.platform_feedback.-package com.iemr.common.service; +package com.iemr.common.service.platform_feedback;src/main/java/com/iemr/common/controller/dynamicForm/DynamicFormController.java-92-96 (1)
92-96: Information disclosure in error response.Exposing the exception object
ein the API response leaks internal implementation details and stack traces, which is a security risk.} catch (Exception e) { - return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) - .body(ApiResponse.error("Failed to fetch form structure:"+e.getMessage(), HttpStatus.INTERNAL_SERVER_ERROR.value(), e)); + .body(ApiResponse.error("Failed to fetch form structure", HttpStatus.INTERNAL_SERVER_ERROR.value(), null)); }src/main/java/com/iemr/common/filter/PlatformFeedbackRateLimitFilter.java-57-65 (1)
57-65: Hardcoded limits contradict configurable properties.Constants are hardcoded here, but
common_ci.propertiesdefines environment-driven properties likeplatform.feedback.ratelimit.minute-limit. Inject these values from configuration.- // Limits & TTLs (tweak if needed) - private static final int MINUTE_LIMIT = 10; - private static final int DAY_LIMIT = 100; - private static final int USER_DAY_LIMIT = 50; // for identified users - private static final Duration MINUTE_WINDOW = Duration.ofMinutes(1); - private static final Duration DAY_WINDOW = Duration.ofHours(48); // keep key TTL ~48h - private static final Duration FAIL_COUNT_WINDOW = Duration.ofMinutes(5); - private static final int FAILS_TO_BACKOFF = 3; - private static final Duration BACKOFF_WINDOW = Duration.ofMinutes(15); + private final int minuteLimit; + private final int dayLimit; + private final int userDayLimit; + // ... inject from env in constructorCommittable suggestion skipped: line range outside the PR's diff.
src/main/java/com/iemr/common/service/dynamicForm/FormMasterServiceImpl.java-173-177 (1)
173-177: ReplaceSystem.err.printlnwith proper logging.Use the SLF4J logger for consistent logging instead of
System.err.println.+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; ... + private static final Logger logger = LoggerFactory.getLogger(FormMasterServiceImpl.class); ... } catch (Exception e) { - - System.err.println("JSON Parsing Error in field: " + field.getFieldId()); + logger.error("JSON Parsing Error in field: {}", field.getFieldId(), e); throw new RuntimeException("Failed to parse JSON for field: " + field.getFieldId(), e); }Committable suggestion skipped: line range outside the PR's diff.
src/main/java/com/iemr/common/service/beneficiaryOTPHandler/BeneficiaryOTPHandlerImpl.java-156-161 (1)
156-161: Critical bug: Creating new instance bypasses Spring context.If
otpCacheis null (which shouldn't happen given the constructor), creatingnew BeneficiaryOTPHandlerImpl()won't have autowired dependencies injected, and the new cache won't be shared. This code path is unreachable in normal operation but indicates a design flaw.The
otpCacheis initialized in the constructor, so it should never be null. Remove this fallback:if (otpCache != null) otpCache.put(authKey, generatedPassword); - else { - BeneficiaryOTPHandlerImpl obj = new BeneficiaryOTPHandlerImpl(); - obj.otpCache.put(authKey, generatedPassword); - } return otp;
🟡 Minor comments (15)
src/main/java/com/iemr/common/service/beneficiary/IdentityBeneficiaryServiceImpl.java-45-45 (1)
45-45: Remove duplicate import.The
@Valueannotation import is already present on line 32.-import org.springframework.beans.factory.annotation.Value;src/main/java/com/iemr/common/utils/RestTemplateUtil.java-56-56 (1)
56-56: Minor: improve error message formatting.The error message concatenates the exception message without spacing, making it harder to read.
Apply this diff:
- logger.error("Error while getting jwtToken from Cookie" + e.getMessage()); + logger.error("Error while getting jwtToken from Cookie: {}", e.getMessage());src/main/java/com/iemr/common/exception/ValidationExceptionHandler.java-41-51 (1)
41-51: Consider sanitizing exception messages before returning to clients.Returning
ex.getMessage()directly could expose internal implementation details or sensitive information to API consumers. Consider using a generic message or sanitizing the exception message.- response.put("errorMessage", ex.getMessage()); + response.put("errorMessage", "Invalid argument provided");Alternatively, log the detailed message and return a sanitized version:
logger.error("Illegal argument: {}", ex.getMessage()); Map<String, Object> response = new HashMap<>(); response.put("status", "ERROR"); response.put("statusCode", 5000); - response.put("errorMessage", ex.getMessage()); + response.put("errorMessage", sanitizeMessage(ex.getMessage()));src/main/environment/common_example.properties-214-214 (1)
214-214: Use placeholder syntax for the pepper value and fix inline comment.The pepper is a security-sensitive value used for hashing. It should follow the same placeholder pattern as other secrets in this file (e.g.,
<Enter pepper value>). Additionally, inline comments using#after the value may be parsed as part of the value by some property loaders.-platform.feedback.ratelimit.pepper=dev-pepper-123 # dummy +platform.feedback.ratelimit.pepper=<Enter platform feedback pepper value>src/main/java/com/iemr/common/repo/userToken/UserTokenRepo.java-21-24 (1)
21-24: Malformed comment block in license header.The license header has an improperly closed comment. Line 21 ends the URL, line 22 starts a new comment block with
/*but the previous block wasn't closed with*/. This results in a nested comment.Apply this diff to fix the comment block:
* along with this program. If not, see https://www.gnu.org/licenses/. -* -/* +*/ +/* * AMRIT – Accessible Medical Records via Integrated Technology -*/ + */src/main/java/com/iemr/common/data/userToken/UserTokenData.java-1-24 (1)
1-24: Fix duplicate and malformed license header.The file contains a duplicate license header with a malformed comment block (line 22 has
/*without closing*/before the next comment).Apply this diff to fix:
-/* -* AMRIT – Accessible Medical Records via Integrated Technology -* Integrated EHR (Electronic Health Records) Solution -* -* Copyright (C) "Piramal Swasthya Management and Research Institute" -* -* This file is part of AMRIT. -* -* This program is free software: you can redistribute it and/or modify -* it under the terms of the GNU General Public License as published by -* the Free Software Foundation, either version 3 of the License, or -* (at your option) any later version. -* -* This program is distributed in the hope that it will be useful, -* but WITHOUT ANY WARRANTY; without even the implied warranty of -* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -* GNU General Public License for more details. -* -* You should have received a copy of the GNU General Public License -* along with this program. If not, see https://www.gnu.org/licenses/. -* -/* -* AMRIT – Accessible Medical Records via Integrated Technology -*/ +/* +* AMRIT – Accessible Medical Records via Integrated Technology +* Integrated EHR (Electronic Health Records) Solution +* +* Copyright (C) "Piramal Swasthya Management and Research Institute" +* +* This file is part of AMRIT. +* +* This program is free software: you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation, either version 3 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program. If not, see https://www.gnu.org/licenses/. +*/src/main/java/com/iemr/common/service/welcomeSms/WelcomeBenificarySmsServiceImpl.java-1-21 (1)
1-21: Missing GPL license header.All other files in this PR include the GPL license header. This file should include the same header for consistency and compliance.
src/main/java/com/iemr/common/service/welcomeSms/WelcomeBenificarySmsServiceImpl.java-91-92 (1)
91-92: Incorrect success message for welcome SMS.The return message says "OTP sent successfully" but this is a welcome SMS, not an OTP.
if (response.getStatusCode().value() == 200) { - return "OTP sent successfully on register mobile number"; + return "Welcome SMS sent successfully"; } else {src/main/java/com/iemr/common/controller/beneficiaryConsent/BeneficiaryConsentController.java-28-28 (1)
28-28: Wrong import for@Paramannotation.
io.lettuce.core.dynamic.annotation.Paramis from the Lettuce Redis client and is not intended for controller parameter documentation. This should likely be removed or replaced with a proper Swagger/OpenAPI annotation for request body documentation, which is already covered by@Operationand@io.swagger.v3.oas.annotations.parameters.RequestBody.-import io.lettuce.core.dynamic.annotation.Param;Then remove the
@Paramannotations from lines 50, 70, and 92.Committable suggestion skipped: line range outside the PR's diff.
src/main/java/com/iemr/common/service/dynamicForm/FormMasterServiceImpl.java-185-186 (1)
185-186: Self-assignment is a no-op bug.
singleSection.setSectionTitle(singleSection.getSectionTitle())assigns the field to itself (which isnullfor a new object). This appears to be a copy-paste error.GroupedFieldResponseDTO singleSection = new GroupedFieldResponseDTO(); - singleSection.setSectionTitle(singleSection.getSectionTitle()); // your custom section title + singleSection.setSectionTitle("Fields"); // TODO: Set meaningful section title singleSection.setFields(fieldDtos);src/main/java/com/iemr/common/service/platform_feedback/PlatformFeedbackService.java-108-111 (1)
108-111: Potential NullPointerException ongetScope().If
c.getScope()isnull, the second part of the OR condition will throw NPE. Add a null check.return all.stream() - .filter(c -> "GLOBAL".equals(c.getScope()) || c.getScope().equalsIgnoreCase(serviceLine)) + .filter(c -> "GLOBAL".equals(c.getScope()) || (c.getScope() != null && c.getScope().equalsIgnoreCase(serviceLine))) .map(c -> new CategoryResponse(c.getCategoryId(), c.getSlug(), c.getLabel(), c.getScope(), c.isActive())) .toList();src/main/java/com/iemr/common/service/beneficiaryOTPHandler/BeneficiaryOTPHandlerImpl.java-218-218 (1)
218-218: Avoid logging full payload containing credentials.The payload includes authentication-related fields. Logging it may inadvertently expose sensitive information in logs.
- logger.info("payload: "+payload); + logger.debug("Sending SMS to destination: {}", obj.getMobNo());Committable suggestion skipped: line range outside the PR's diff.
src/main/java/com/iemr/common/service/beneficiaryOTPHandler/BeneficiaryOTPHandlerImpl.java-186-188 (1)
186-188: Remove debug code that fetches all templates.Fetching all SMS templates and logging them is inefficient and may expose sensitive data. This appears to be debug code that should be removed.
Optional<SMSTemplate> smsTemplateData = smsTemplateRepository.findBySmsTemplateName(smsTemplateName); - List<SMSTemplate> smsTemplateAllData = (List<SMSTemplate>) smsTemplateRepository.findAll(); - logger.info("AllData"+smsTemplateAllData.stream().toArray());src/main/java/com/iemr/common/data/platform_feedback/Feedback.java-121-126 (1)
121-126: Trim before checking length to avoid rejecting valid input.The length check occurs before trimming. A string with leading/trailing whitespace that exceeds 2000 characters raw but is valid after trimming will be incorrectly rejected.
public void setComment(String comment) { - if (comment != null && comment.length() > 2000) { - throw new IllegalArgumentException("Comment cannot exceed 2000 characters"); - } - this.comment = (comment == null || comment.trim().isEmpty()) ? null : comment.trim(); + if (comment == null || comment.trim().isEmpty()) { + this.comment = null; + return; + } + String trimmed = comment.trim(); + if (trimmed.length() > 2000) { + throw new IllegalArgumentException("Comment cannot exceed 2000 characters"); + } + this.comment = trimmed; }src/main/java/com/iemr/common/data/platform_feedback/Feedback.java-71-79 (1)
71-79: Use setters consistently in constructor to ensure validation.The constructor uses
setRating,setComment, andsetServiceLinefor validation, but directly assignscategoryanduserId, bypassing the null-check insetCategoryand theisAnonymous/userIdcoupling logic insetUserId.public Feedback(int rating, String comment, String serviceLine, boolean isAnonymous, FeedbackCategory category, Integer userId) { this(); // ensures feedbackId this.setRating(rating); this.setComment(comment); this.setServiceLine(serviceLine); - this.isAnonymous = isAnonymous; - this.category = category; - this.userId = userId; + this.setCategory(category); + this.setUserId(userId); + this.setAnonymous(isAnonymous); // call last to override if explicitly anonymous }
| # # Firebase Configuration | ||
| firebase.enabled=${FIREBASE_ENABLE} | ||
| # # if using file | ||
| firebase.credential-file=${FIREBASE_CREDENTIAL} | ||
|
|
||
|
|
||
| video-call-url=${VIDEO_CALL_URL} | ||
| jibri.output.path={JIBRI_OUTPUT_PATH} | ||
| video.recording.path={VIDEO_RECORDING_PATH} | ||
| video.recording.path={VIDEO_RECORDING_PATH} | ||
|
|
||
| # Platform Feedback module | ||
| platform.feedback.ratelimit.enabled=${PLATFORM_FEEDBACK_RATELIMIT_ENABLED} | ||
| platform.feedback.ratelimit.pepper=${PLATFORM_FEEDBACK_RATELIMIT_PEPPER} | ||
| platform.feedback.ratelimit.trust-forwarded-for=${PLATFORM_FEEDBACK_RATELIMIT_TRUST_FORWARDED_FOR} | ||
| platform.feedback.ratelimit.forwarded-for-header=${PLATFORM_FEEDBACK_RATELIMIT_FORWARDED_FOR_HEADER} | ||
| platform.feedback.ratelimit.minute-limit=${PLATFORM_FEEDBACK_RATELIMIT_MINUTE_LIMIT} | ||
| platform.feedback.ratelimit.day-limit=${PLATFORM_FEEDBACK_RATELIMIT_DAY_LIMIT} | ||
| platform.feedback.ratelimit.user-day-limit=${PLATFORM_FEEDBACK_RATELIMIT_USER_DAY_LIMIT} | ||
| platform.feedback.ratelimit.fail-window-minutes=${PLATFORM_FEEDBACK_RATELIMIT_FAIL_WINDOW_MINUTES} | ||
| platform.feedback.ratelimit.backoff-minutes=${PLATFORM_FEEDBACK_RATELIMIT_BACKOFF_MINUTES} | ||
| generateBeneficiaryIDs-api-url={GEN_BENEFICIARY_IDS_API_URL} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check the specific lines mentioned
fd -t f "common_docker.properties" | head -5Repository: PSMRI/Common-API
Length of output: 105
🏁 Script executed:
# Check lines 189-209 in the file to confirm the content
if [ -f "src/main/environment/common_docker.properties" ]; then
sed -n '189,209p' "src/main/environment/common_docker.properties" | cat -n
else
echo "File not found at src/main/environment/common_docker.properties"
fiRepository: PSMRI/Common-API
Length of output: 1352
🏁 Script executed:
# Search for all property patterns in the file to understand conventions
# Count occurrences of ${...} vs {...} patterns
echo "=== Pattern analysis ==="
rg '\$\{[A-Z_]+\}' src/main/environment/common_docker.properties -c | head -1 && echo "Lines with \${VAR}:"
rg '\{[A-Z_]+\}' src/main/environment/common_docker.properties -c | head -1 && echo "Lines with {VAR}:"Repository: PSMRI/Common-API
Length of output: 127
🏁 Script executed:
# Show specific lines with {VAR} pattern to understand context
echo "=== Lines using {VAR} pattern ==="
rg '\{[A-Z_]+\}' src/main/environment/common_docker.properties -nRepository: PSMRI/Common-API
Length of output: 5657
🏁 Script executed:
# Check if there's a common_example.properties to compare conventions
echo "=== Checking example properties file ==="
fd -t f "common_example.properties"Repository: PSMRI/Common-API
Length of output: 147
🏁 Script executed:
# Look for any documentation or comments about templating/placeholder handling
echo "=== Searching for templating info in README/docs ==="
fd -t f -e md | xargs rg -l "properties|template|placeholder" 2>/dev/null | head -5Repository: PSMRI/Common-API
Length of output: 112
🏁 Script executed:
# Verify Spring Boot is used in this project
fd -t f "pom.xml" | head -1 | xargs grep -i "spring.boot" 2>/dev/null | head -3Repository: PSMRI/Common-API
Length of output: 219
🏁 Script executed:
# Check if there's any documentation about this specific pattern
rg "jibri|VIDEO_RECORDING" src/main/environment/ -A 2 -B 2Repository: PSMRI/Common-API
Length of output: 1311
Fix placeholder syntax for video paths and generateBeneficiaryIDs-api-url
Three properties incorrectly use {VAR} instead of ${VAR} at lines 196, 197, and 209:
jibri.output.path={JIBRI_OUTPUT_PATH}
video.recording.path={VIDEO_RECORDING_PATH}
generateBeneficiaryIDs-api-url={GEN_BENEFICIARY_IDS_API_URL}Spring Boot properties require ${VAR} syntax for environment variable resolution. The {...} pattern will be treated as literal strings and not resolved, breaking Jibri video recording paths and the beneficiary ID generation API URL at runtime. All other environment-driven values in this file correctly use ${VAR}.
Update to:
-jibri.output.path={JIBRI_OUTPUT_PATH}
-video.recording.path={VIDEO_RECORDING_PATH}
-generateBeneficiaryIDs-api-url={GEN_BENEFICIARY_IDS_API_URL}
+jibri.output.path=${JIBRI_OUTPUT_PATH}
+video.recording.path=${VIDEO_RECORDING_PATH}
+generateBeneficiaryIDs-api-url=${GEN_BENEFICIARY_IDS_API_URL}🤖 Prompt for AI Agents
In src/main/environment/common_docker.properties around lines 189 to 209, three
properties use incorrect placeholder syntax with {VAR} instead of the Spring
Boot environment variable format ${VAR}; change
jibri.output.path={JIBRI_OUTPUT_PATH} to use ${JIBRI_OUTPUT_PATH}, change
video.recording.path={VIDEO_RECORDING_PATH} to ${VIDEO_RECORDING_PATH}, and
change generateBeneficiaryIDs-api-url={GEN_BENEFICIARY_IDS_API_URL} to
${GEN_BENEFICIARY_IDS_API_URL} so the values are resolved from environment
variables at runtime.
| @Bean | ||
| @ConditionalOnProperty(name = "firebase.enabled", havingValue = "true") | ||
| public FirebaseMessaging firebaseMessaging() throws IOException { | ||
| if (!firebaseEnabled) { | ||
| logger.error("⚠️ Firebase disabled by config"); | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| if (firebaseCredentialFile == null || firebaseCredentialFile.isBlank()) { | ||
| logger.error("⚠️ No Firebase credentials path provided"); | ||
| return null; // don't throw, app will still start | ||
| } | ||
|
|
||
| GoogleCredentials credentials = GoogleCredentials.fromStream( | ||
| new ClassPathResource(firebaseCredentialFile).getInputStream() | ||
| ); | ||
| FirebaseOptions options = FirebaseOptions.builder() | ||
| .setCredentials(credentials) | ||
| .build(); | ||
|
|
||
| FirebaseApp firebaseApp = FirebaseApp.getApps().isEmpty() | ||
| ? FirebaseApp.initializeApp(options) | ||
| : FirebaseApp.getInstance(); | ||
|
|
||
| return FirebaseMessaging.getInstance(firebaseApp); | ||
|
|
||
| } catch (Exception e) { | ||
| logger.error("⚠️ Firebase init failed: " + e.getMessage()); | ||
| return null; // keep app running | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Spring @Bean method return null BeanCreationException
💡 Result:
Cause
- Spring does not accept a null from a @bean factory: if your @bean method returns null the container treats that as a failed bean creation and you get a BeanCreationException/BeanCurrentlyInCreationException. [1][2]
How to fix (pick one)
- Never return null from a @bean method. Return a real instance or throw an exception when creation fails. [1][2]
- Make the bean conditional so it is not defined when you would otherwise return null (use @conditional / @ConditionalOnX / configuration properties checks) so Spring simply doesn't register the bean. [2]
- Return a wrapper instead of null: Optional, Supplier, or a custom holder object and let callers handle absence. Inject Optional or Supplier where appropriate. [1]
- Programmatically skip registration (BeanDefinitionRegistry / BeanFactoryPostProcessor) so no bean is created when you don't want one.
- If you must signal “no bean” from FactoryBean logic, Spring internally substitutes a NullBean; but returning null directly from a @bean method is not allowed. See FactoryBean/NullBean handling in Spring source. [3]
References
- StackOverflow discussion on returning null from @bean and resulting exceptions. [1]
- Spring Framework reference (bean/factory method behavior). [2]
- Spring source / posts showing FactoryBean/NullBean handling when getObject returns null. [3]
🌐 Web query:
Spring @ConditionalOnProperty bean instantiation behavior
💡 Result:
Summary — how @ConditionalOnProperty works and when beans are created:
-
The condition is evaluated at configuration time (when Spring processes configuration/auto-configuration). If the condition does NOT match, the bean definition is not registered (so the bean is not created). If it matches, the bean definition is registered and the bean is instantiated as usual during context initialization. [1][2]
-
Matching rules:
- By default a property "matches" if it exists and its value is not "false".
- Use havingValue to require a specific value.
- If the property is absent, matchIfMissing (default = false) determines whether absence counts as a match. [1]
-
Class-level vs method-level:
- Placing the annotation on a @Configuration/@autoConfiguration class prevents registration of that configuration as a bean if the condition fails. Placing it on individual @bean methods prevents registration of those bean definitions only. Be careful about order of bean-definition processing — some bean conditions depend on what’s already been processed. [2]
-
Practical note: @ConditionalOnProperty simply prevents registration when the condition fails (it does not throw by default); other configuration and beans continue to be processed. [3]
Sources: Spring Boot API and reference docs for ConditionalOnProperty; community explanation. [1][2][3]
🏁 Script executed:
# First, let's examine the actual file to understand the context better
cat -n src/main/java/com/iemr/common/config/firebase/FirebaseMessagingConfig.javaRepository: PSMRI/Common-API
Length of output: 2803
Remove null returns from @Bean method; they cause BeanCreationException
Spring does not accept null from @Bean factory methods—returning null triggers a BeanCreationException during startup. The current code returns null in three paths (lines 36, 42, 60), which will fail bean initialization and crash the application.
Additionally, the @ConditionalOnProperty(firebase.enabled) annotation already handles the enable/disable logic: if firebase.enabled ≠ true, Spring skips bean registration entirely. The redundant check at line 34–36 is unnecessary.
Replace null returns with exceptions:
- Line 34–36: Remove the redundant
firebaseEnabledcheck (the@ConditionalOnPropertyalready covers this). - Line 42: Throw
IllegalStateException("Firebase credentials path not configured")instead of returningnull. - Line 60: Throw the exception instead of returning
null; optionally wrap with context:throw new IllegalStateException("Firebase initialization failed", e).
This ensures deterministic startup behaviour: either a valid bean is created, or startup fails with a clear error message pointing to the actual misconfiguration.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/config/firebase/FirebaseMessagingConfig.java
around lines 31 to 61, the @Bean method returns null in several branches which
causes BeanCreationException; remove the redundant firebaseEnabled check (the
@ConditionalOnProperty already handles skipping bean registration), replace the
null return at the credentials check with throwing new
IllegalStateException("Firebase credentials path not configured"), and on catch
rethrow a clear exception (for example throw new IllegalStateException("Firebase
initialization failed", e)) instead of returning null so bean creation fails
deterministically with a helpful error.
src/main/java/com/iemr/common/controller/platform_feedback/PlatformFeedbackController.java
Outdated
Show resolved
Hide resolved
| } catch (Exception e) { | ||
| logger.error("File download for userID failed with exception " + e.getMessage(), e); | ||
| throw new Exception("Error while downloading file. Please contact administrator.."); | ||
| } |
There was a problem hiding this comment.
Exception handling swallows ResponseStatusException, breaking 404 responses.
The catch block catches all Exception types, including the ResponseStatusException thrown on lines 71-72 and 93. This converts intended 404 responses into generic 500 errors, defeating the purpose of the new error handling.
- } catch (Exception e) {
+ } catch (ResponseStatusException e) {
+ throw e;
+ } catch (Exception e) {
logger.error("File download for userID failed with exception " + e.getMessage(), e);
throw new Exception("Error while downloading file. Please contact administrator..");
}Alternatively, remove the try-catch entirely and let Spring handle ResponseStatusException natively, or throw ResponseStatusException with HttpStatus.INTERNAL_SERVER_ERROR instead of a generic Exception.
🤖 Prompt for AI Agents
In
src/main/java/com/iemr/common/controller/users/EmployeeSignatureController.java
around lines 99 to 102, the broad catch(Exception) is swallowing
ResponseStatusException and turning intended 404s into 500s; change the error
handling so that if the caught exception is a ResponseStatusException you
rethrow it unchanged, otherwise log the error and throw a
ResponseStatusException with HttpStatus.INTERNAL_SERVER_ERROR (or remove the
try-catch entirely and let Spring propagate ResponseStatusException). Ensure you
do not convert existing ResponseStatusException instances into generic
Exceptions when handling errors.
| package com.iemr.common.model; | ||
|
|
||
| import jakarta.persistence.*; | ||
| import jakarta.validation.constraints.Max; | ||
| import jakarta.validation.constraints.Min; | ||
| import jakarta.validation.constraints.Size; | ||
| import java.time.LocalDateTime; | ||
| import java.util.UUID; |
There was a problem hiding this comment.
Package declaration does not match file path; missing import for FeedbackCategory.
The file is located at src/main/java/com/iemr/common/data/platform_feedback/Feedback.java but declares package com.iemr.common.model. This mismatch will cause a compilation error. Additionally, FeedbackCategory is used but not imported.
-package com.iemr.common.model;
+package com.iemr.common.data.platform_feedback;
import jakarta.persistence.*;
import jakarta.validation.constraints.Max;
import jakarta.validation.constraints.Min;
import jakarta.validation.constraints.Size;
import java.time.LocalDateTime;
import java.util.UUID;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/data/platform_feedback/Feedback.java around
lines 22-29, the declared package com.iemr.common.model does not match the file
path and FeedbackCategory is not imported; change the package declaration to
com.iemr.common.data.platform_feedback and add an import for FeedbackCategory
(e.g., import com.iemr.common.data.platform_feedback.FeedbackCategory;) so the
class compiles and references resolve correctly.
| @ExceptionHandler(MethodArgumentNotValidException.class) | ||
| public ResponseEntity<ApiResponse<Object>> handleValidationException(MethodArgumentNotValidException ex) { | ||
| String errorMessage = ex.getBindingResult().getFieldErrors().stream() | ||
| .map(error -> error.getField() + ": " + error.getDefaultMessage()) | ||
| .collect(Collectors.joining(", ")); | ||
| return ResponseEntity.status(HttpStatus.BAD_REQUEST) | ||
| .body(ApiResponse.error("Validation Error: " + errorMessage, HttpStatus.BAD_REQUEST.value(), null)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "MethodArgumentNotValidException" --type java -C 3Repository: PSMRI/Common-API
Length of output: 3958
Remove duplicate MethodArgumentNotValidException handler to avoid handler precedence conflicts.
Two @RestControllerAdvice handlers for MethodArgumentNotValidException exist:
ValidationExceptionHandler.handleValidationErrors()insrc/main/java/com/iemr/common/exception/(returnsMap<String, Object>)GlobalExceptionHandler.handleValidationException()insrc/main/java/com/iemr/common/utils/exception/(returnsApiResponse<Object>)
Spring will arbitrarily select one based on bean registration order, causing inconsistent error responses. Consolidate into a single handler or explicitly order them using @Order.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/utils/exception/GlobalExceptionHandler.java
around lines 22-29, remove the duplicate MethodArgumentNotValidException handler
to avoid conflicting handlers; keep only the single validation handler (prefer
the one in src/main/java/com/iemr/common/exception/ValidationExceptionHandler)
or consolidate both implementations into one unified handler that returns a
consistent response type across the app; after removing or consolidating, run
tests and ensure controllers return the expected validation response shape.
| @Value("${cors.allowed-origins}") | ||
| private String allowedOrigins; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find CorsConfig class
find . -name "CorsConfig.java" -type fRepository: PSMRI/Common-API
Length of output: 114
🏁 Script executed:
# Find JwtUserIdValidationFilter class
find . -name "JwtUserIdValidationFilter.java" -type fRepository: PSMRI/Common-API
Length of output: 128
🏁 Script executed:
# Search for cors.allowed property keys in the codebase
rg "cors\.allowed" --type java -A 2 -B 2Repository: PSMRI/Common-API
Length of output: 2426
🏁 Script executed:
# Read the HTTPRequestInterceptor file to check lines 198-218
wc -l src/main/java/com/iemr/common/utils/http/HTTPRequestInterceptor.javaRepository: PSMRI/Common-API
Length of output: 132
🏁 Script executed:
# Read CorsConfig.java to see full implementation
cat -n src/main/java/com/iemr/common/config/CorsConfig.javaRepository: PSMRI/Common-API
Length of output: 1476
🏁 Script executed:
# Read HTTPRequestInterceptor.java lines 190-219 to see the full context
sed -n '190,219p' src/main/java/com/iemr/common/utils/http/HTTPRequestInterceptor.javaRepository: PSMRI/Common-API
Length of output: 924
🏁 Script executed:
# Check JwtUserIdValidationFilter for CORS logic
cat -n src/main/java/com/iemr/common/utils/JwtUserIdValidationFilter.javaRepository: PSMRI/Common-API
Length of output: 13174
Fix CorsConfig property key from cors.allowed-origin to cors.allowed-origins and extract origin-matching logic into shared utility
CorsConfig reads @Value("${cors.allowed-origin}") (singular) while the actual property is cors.allowed-origins (plural). The property files define cors.allowed-origins, and both HTTPRequestInterceptor and FilterConfig correctly use the plural form. This mismatch will cause CorsConfig to fail silently with null or default values, leading to misconfigured CORS handling.
Additionally, isOriginAllowed() is duplicated identically in JwtUserIdValidationFilter (lines 210–226) and HTTPRequestInterceptor (lines 198–218), including the same wildcard regex conversion logic. Extract this into a shared utility service to avoid maintenance risk and ensure all three components (CorsConfig, HTTPRequestInterceptor, JwtUserIdValidationFilter) use one source of truth.
- Fix
CorsConfigproperty key tocors.allowed-origins - Create a shared
OriginValidatorutility withisOriginAllowed(String origin, String allowedOrigins)method - Update all three components to use the shared utility
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/utils/http/HTTPRequestInterceptor.java around
lines 50 to 52, update CorsConfig usage and consolidate duplicated origin logic:
change the CorsConfig property key from "cors.allowed-origin" to
"cors.allowed-origins" so it matches property files; create a new shared utility
class OriginValidator with a public static boolean isOriginAllowed(String
origin, String allowedOrigins) that implements the existing wildcard-to-regex
conversion and matching; replace the local isOriginAllowed implementations in
HTTPRequestInterceptor (lines ~198–218), JwtUserIdValidationFilter (lines
~210–226), and CorsConfig to call OriginValidator.isOriginAllowed(origin,
allowedOrigins); ensure all three components inject or access the same
allowed-origins property (plural) and remove the duplicated methods.
| // Additional check for common XSS patterns (case-insensitive) | ||
| String lowerInput = sanitized.toLowerCase(); | ||
| for (String pattern : XSS_PATTERNS) { | ||
| if (lowerInput.contains(pattern.toLowerCase())) { | ||
| // Remove the dangerous pattern | ||
| sanitized = sanitized.replaceAll("(?i)" + pattern, ""); | ||
| } | ||
| } | ||
|
|
||
| return sanitized; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "InputSanitizer.java"Repository: PSMRI/Common-API
Length of output: 117
🏁 Script executed:
cat -n ./src/main/java/com/iemr/common/utils/InputSanitizer.javaRepository: PSMRI/Common-API
Length of output: 3821
Fix sanitizeForXSS regex: patterns with metacharacters like eval( will throw PatternSyntaxException
In sanitizeForXSS (line 40), you concatenate patterns directly into replaceAll without escaping:
sanitized = sanitized.replaceAll("(?i)" + pattern, "");This breaks at runtime because XSS_PATTERNS contains regex metacharacters: eval( and expression( have unbalanced ( characters, creating invalid regexes like (?i)eval( that throw PatternSyntaxException.
Fix: Escape patterns as literals using Pattern.quote():
sanitized = sanitized.replaceAll("(?i)" + Pattern.quote(pattern), "");Additional issue: After HtmlUtils.htmlEscape() on line 33, patterns like <script become <script, so the subsequent pattern matching (lines 38-40) won't find them. Tag-based patterns in XSS_PATTERNS are redundant post-escape and should be removed or the approach reconsidered.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/utils/InputSanitizer.java around lines 35 to
45, the current sanitizeForXSS appends raw patterns into replaceAll which throws
PatternSyntaxException for patterns with regex metacharacters (e.g., "eval(");
change the replaceAll usage to escape patterns as literals (use Pattern.quote on
the pattern before concatenation) so the regex is valid, and review/remove
tag-based patterns (like "<script") because after HtmlUtils.htmlEscape() those
tags become escaped (e.g., "<script") so the tag patterns are redundant or
must be adapted to match the escaped form instead.
| sms-gateway-url = | ||
|
|
||
| #Encryption passphrase used by AESUtil for decrypting CTI passwords. | ||
| encryption.passphrase=Piramal12Piramal |
There was a problem hiding this comment.
Critical security risk: Hardcoded encryption passphrase must be externalized immediately.
Line 197 contains a hardcoded encryption passphrase (Piramal12Piramal) committed to version control. This is a major security vulnerability—any attacker with repository access can decrypt sensitive CTI credentials.
Replace the hardcoded value with an environment variable placeholder, following the pattern established by JWT and other sensitive configs:
-encryption.passphrase=Piramal12Piramal
+encryption.passphrase=<Enter encryption passphrase>Then ensure the actual value is supplied via environment variable at deployment time (e.g., ENCRYPTION_PASSPHRASE injected into the property via Spring's property resolution mechanism or externalized config server).
Based on learnings, sensitive credentials in properties files should use placeholders to prevent accidental exposure.
📝 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.
| encryption.passphrase=Piramal12Piramal | |
| encryption.passphrase=<Enter encryption passphrase> |
🤖 Prompt for AI Agents
In src/main/resources/application.properties around line 197, replace the
hardcoded encryption.passphrase value ("Piramal12Piramal") with an environment
variable placeholder (following the existing JWT/sensitive-config pattern) so
the passphrase is not committed in source; update the property to reference a
Spring property (e.g., an ENV-backed key) and ensure the actual secret is
provided at deploy/runtime via an environment variable or externalized config
(ENCRYPTION_PASSPHRASE) and documented for ops.
fix: cors spell fixes and import of packages updates
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/main/java/com/iemr/common/service/platform_feedback/PlatformFeedbackService.java (1)
77-104: Consider implementing scope validation mentioned in comment.Line 85 notes that scope matching against serviceLine should be checked, but this validation is not implemented in any branch of
resolveCategory. If scope validation is required for data consistency, implement it; otherwise, remove the comment to avoid confusion.src/main/java/com/iemr/common/data/platform_feedback/Feedback.java (1)
165-170: Consider documenting the bidirectional coupling between userId and isAnonymous.Setting
userIdautomatically setsisAnonymous = false(line 168), and settingisAnonymous = trueclearsuserId(line 146). This coupling can lead to surprising behavior: settinguserId, thensetAnonymous(true)clears the userId; but settingsetAnonymous(false)thensetUserId(null)leaves the entity in a state whereisAnonymous = falsewithuserId = null.Consider adding JavaDoc to both setters explaining this behavior, or validating that
isAnonymous = falserequires a non-nulluserId.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/com/iemr/common/config/CorsConfig.java(1 hunks)src/main/java/com/iemr/common/controller/platform_feedback/PlatformFeedbackController.java(1 hunks)src/main/java/com/iemr/common/data/platform_feedback/Feedback.java(1 hunks)src/main/java/com/iemr/common/data/platform_feedback/FeedbackCategory.java(1 hunks)src/main/java/com/iemr/common/dto/platform_feedback/CategoryResponse.java(1 hunks)src/main/java/com/iemr/common/dto/platform_feedback/FeedbackRequest.java(1 hunks)src/main/java/com/iemr/common/dto/platform_feedback/FeedbackResponse.java(1 hunks)src/main/java/com/iemr/common/repository/platform_feedback/PlatformFeedbackCategoryRepository.java(1 hunks)src/main/java/com/iemr/common/repository/platform_feedback/PlatformFeedbackRepository.java(1 hunks)src/main/java/com/iemr/common/service/platform_feedback/PlatformFeedbackService.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/java/com/iemr/common/dto/platform_feedback/FeedbackResponse.java
- src/main/java/com/iemr/common/config/CorsConfig.java
- src/main/java/com/iemr/common/repository/platform_feedback/PlatformFeedbackRepository.java
- src/main/java/com/iemr/common/dto/platform_feedback/FeedbackRequest.java
- src/main/java/com/iemr/common/controller/platform_feedback/PlatformFeedbackController.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/iemr/common/data/platform_feedback/Feedback.java (1)
src/main/java/com/iemr/common/data/platform_feedback/FeedbackCategory.java (1)
Entity(34-153)
🔇 Additional comments (4)
src/main/java/com/iemr/common/dto/platform_feedback/CategoryResponse.java (1)
22-24: LGTM! Package declaration now matches file path.The past review comment regarding package mismatch has been addressed. The record is properly structured as a response DTO.
src/main/java/com/iemr/common/repository/platform_feedback/PlatformFeedbackCategoryRepository.java (1)
28-34: LGTM! Import path corrected and repository follows conventions.The past review comment regarding incorrect import has been addressed. The repository properly imports the entity from the correct package and follows Spring Data JPA query method conventions.
src/main/java/com/iemr/common/data/platform_feedback/Feedback.java (1)
22-22: LGTM! Package declaration corrected.The past review comment regarding package mismatch has been addressed.
src/main/java/com/iemr/common/data/platform_feedback/FeedbackCategory.java (1)
22-92: LGTM! Package corrected and entity well-structured.The past review comment regarding package mismatch has been addressed. The entity includes proper validation constraints, unique constraint enforcement, and defensive UUID generation in the
@PrePersisthook.
| public Feedback(int rating, String comment, String serviceLine, boolean isAnonymous, FeedbackCategory category, Integer userId) { | ||
| this(); // ensures feedbackId | ||
| this.setRating(rating); | ||
| this.setComment(comment); | ||
| this.setServiceLine(serviceLine); | ||
| this.isAnonymous = isAnonymous; | ||
| this.category = category; | ||
| this.userId = userId; | ||
| } |
There was a problem hiding this comment.
Use setCategory in constructor to enforce validation.
Line 77 directly assigns this.category = category, bypassing the null check in setCategory (lines 154-159). This could allow a null category to be set during construction, violating the non-null constraint on the field.
Apply this fix:
public Feedback(int rating, String comment, String serviceLine, boolean isAnonymous, FeedbackCategory category, Integer userId) {
this(); // ensures feedbackId
this.setRating(rating);
this.setComment(comment);
this.setServiceLine(serviceLine);
this.isAnonymous = isAnonymous;
- this.category = category;
+ this.setCategory(category);
this.userId = userId;
}📝 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.
| public Feedback(int rating, String comment, String serviceLine, boolean isAnonymous, FeedbackCategory category, Integer userId) { | |
| this(); // ensures feedbackId | |
| this.setRating(rating); | |
| this.setComment(comment); | |
| this.setServiceLine(serviceLine); | |
| this.isAnonymous = isAnonymous; | |
| this.category = category; | |
| this.userId = userId; | |
| } | |
| public Feedback(int rating, String comment, String serviceLine, boolean isAnonymous, FeedbackCategory category, Integer userId) { | |
| this(); // ensures feedbackId | |
| this.setRating(rating); | |
| this.setComment(comment); | |
| this.setServiceLine(serviceLine); | |
| this.isAnonymous = isAnonymous; | |
| this.setCategory(category); | |
| this.userId = userId; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/data/platform_feedback/Feedback.java around
lines 71 to 79, the constructor assigns this.category = category directly which
bypasses the null-check logic in setCategory; replace the direct assignment with
a call to setCategory(category) so the constructor enforces the same validation
and prevents a null category from being set.
| fb.setCreatedAt(LocalDateTime.now()); | ||
| fb.setUpdatedAt(LocalDateTime.now()); |
There was a problem hiding this comment.
Remove manual timestamp setting—JPA ignores these values.
The Feedback entity marks createdAt and updatedAt with insertable = false, updatable = false (lines 39-43 in Feedback.java), meaning the database manages these fields. Setting them here has no effect and may mislead developers into thinking the application controls these timestamps.
Remove these lines:
Feedback fb = new Feedback();
fb.setFeedbackId(UUID.randomUUID().toString());
- fb.setCreatedAt(LocalDateTime.now());
- fb.setUpdatedAt(LocalDateTime.now());
fb.setRating(req.rating());📝 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.
| fb.setCreatedAt(LocalDateTime.now()); | |
| fb.setUpdatedAt(LocalDateTime.now()); |
🤖 Prompt for AI Agents
In
src/main/java/com/iemr/common/service/platform_feedback/PlatformFeedbackService.java
around lines 64-65, remove the two lines that manually set
fb.setCreatedAt(LocalDateTime.now()) and fb.setUpdatedAt(LocalDateTime.now())
because the Feedback entity marks these columns as insertable = false, updatable
= false and the database manages them; simply delete those assignments so the
service does not attempt to set DB-managed timestamps.
| @Transactional(readOnly = true) | ||
| public List<CategoryResponse> listCategories(String serviceLine) { | ||
| List<FeedbackCategory> all = categoryRepo.findByActiveTrueOrderByLabelAsc(); | ||
| // filter by scope or return all; FE can filter further | ||
| return all.stream() | ||
| .filter(c -> "GLOBAL".equals(c.getScope()) || c.getScope().equalsIgnoreCase(serviceLine)) | ||
| .map(c -> new CategoryResponse(c.getCategoryId(), c.getSlug(), c.getLabel(), c.getScope(), c.isActive())) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
Add null check for serviceLine parameter to prevent NPE.
Line 111 calls c.getScope().equalsIgnoreCase(serviceLine) without checking if serviceLine is null. If a null value is passed, this will throw a NullPointerException.
Apply this fix:
@Transactional(readOnly = true)
public List<CategoryResponse> listCategories(String serviceLine) {
List<FeedbackCategory> all = categoryRepo.findByActiveTrueOrderByLabelAsc();
// filter by scope or return all; FE can filter further
return all.stream()
- .filter(c -> "GLOBAL".equals(c.getScope()) || c.getScope().equalsIgnoreCase(serviceLine))
+ .filter(c -> "GLOBAL".equals(c.getScope()) || (serviceLine != null && c.getScope().equalsIgnoreCase(serviceLine)))
.map(c -> new CategoryResponse(c.getCategoryId(), c.getSlug(), c.getLabel(), c.getScope(), c.isActive()))
.toList();
}📝 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.
| @Transactional(readOnly = true) | |
| public List<CategoryResponse> listCategories(String serviceLine) { | |
| List<FeedbackCategory> all = categoryRepo.findByActiveTrueOrderByLabelAsc(); | |
| // filter by scope or return all; FE can filter further | |
| return all.stream() | |
| .filter(c -> "GLOBAL".equals(c.getScope()) || c.getScope().equalsIgnoreCase(serviceLine)) | |
| .map(c -> new CategoryResponse(c.getCategoryId(), c.getSlug(), c.getLabel(), c.getScope(), c.isActive())) | |
| .toList(); | |
| } | |
| @Transactional(readOnly = true) | |
| public List<CategoryResponse> listCategories(String serviceLine) { | |
| List<FeedbackCategory> all = categoryRepo.findByActiveTrueOrderByLabelAsc(); | |
| // filter by scope or return all; FE can filter further | |
| return all.stream() | |
| .filter(c -> "GLOBAL".equals(c.getScope()) || (serviceLine != null && c.getScope().equalsIgnoreCase(serviceLine))) | |
| .map(c -> new CategoryResponse(c.getCategoryId(), c.getSlug(), c.getLabel(), c.getScope(), c.isActive())) | |
| .toList(); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/com/iemr/common/service/platform_feedback/PlatformFeedbackService.java
around lines 106-114, the stream filter calls
c.getScope().equalsIgnoreCase(serviceLine) which can throw an NPE when
serviceLine is null; update the filter to first check that serviceLine is not
null before calling equalsIgnoreCase (e.g. only call equalsIgnoreCase when
serviceLine != null), so the predicate becomes: accept when scope is "GLOBAL" or
when serviceLine is non-null and matches scope ignoring case.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/data/users/User.java (1)
539-541: Optional: Explicit getter is redundant with Lombok @DaTa.The
getDhistoken()method is explicitly defined, but Lombok's@Dataannotation (line 57) already generates this getter automatically. While not harmful, this creates redundancy.However, if you prefer explicit getters for clarity or to ensure stability regardless of Lombok configuration changes, this is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java(16 hunks)src/main/java/com/iemr/common/data/users/User.java(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-02-21T07:43:03.828Z
Learnt from: sandipkarmakar3
Repo: PSMRI/Common-API PR: 162
File: src/main/java/com/iemr/common/utils/CookieUtil.java:52-66
Timestamp: 2025-02-21T07:43:03.828Z
Learning: In the Common-API project's CookieUtil class, the current implementation of addJwtTokenToCookie using both response.addCookie() and manual Set-Cookie header has been tested and confirmed to work as required for their use case. URL encoding of the JWT token is not necessary.
Applied to files:
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
📚 Learning: 2025-02-03T12:42:38.278Z
Learnt from: indraniBan
Repo: PSMRI/Common-API PR: 138
File: src/main/java/com/iemr/common/utils/JwtAuthenticationUtil.java:0-0
Timestamp: 2025-02-03T12:42:38.278Z
Learning: In the JwtAuthenticationUtil class, catching a generic Exception in the validateUserIdAndJwtToken method is acceptable as per the team's preference.
Applied to files:
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
📚 Learning: 2025-02-21T07:42:36.497Z
Learnt from: sandipkarmakar3
Repo: PSMRI/Common-API PR: 162
File: src/main/java/com/iemr/common/utils/CookieUtil.java:40-47
Timestamp: 2025-02-21T07:42:36.497Z
Learning: In the Common-API project's CookieUtil class, JWT cookies are configured with SameSite=None to support cross-origin requests, which is required for the project's CORS functionality.
Applied to files:
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
🧬 Code graph analysis (1)
src/main/java/com/iemr/common/data/users/User.java (1)
src/main/java/com/iemr/common/data/callhandling/BeneficiaryCall.java (1)
JsonIgnoreProperties(44-300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_check_coverage
🔇 Additional comments (7)
src/main/java/com/iemr/common/data/users/User.java (3)
58-58: LGTM!Adding
@JsonIgnoreProperties(ignoreUnknown = true)improves resilience during deserialization and is consistent with similar entities in the codebase.
220-225: LGTM!The factory method signature correctly includes the new
dhistokenparameter, maintaining consistency with the added field.
252-252: LGTM!The field assignment correctly initializes
dhistokenin the factory method.src/main/java/com/iemr/common/controller/users/IEMRAdminController.java (4)
947-959: Good fix: Early return prevents execution after error.The explicit return statement at line 958 correctly terminates the request when no JWT token is found, preventing unnecessary execution of subsequent validation logic. This improves both correctness and clarity of the error handling flow.
347-350: Good practice: Added braces for consistency.Adding braces to the else block improves code consistency and reduces the risk of future logic errors when modifying the code.
395-395: Reconsider exposingdhistokenin authentication responses without clear justification.The
dhistokenfield appears to be an integration token for an external system (likely District Health Information System) but is being returned to clients in authentication responses without documented purpose. If this token is not needed by clients, it should be excluded from the response to avoid unnecessary exposure of credentials. If clients do require it for legitimate use, add documentation explaining the purpose and ensure appropriate access controls are in place.
288-290: Verify the business intent for allowing "New" status users to refresh tokens.The token refresh logic now accepts both "Active" and "New" user statuses. Confirm whether "New" represents incomplete user setup or is an active operational status that should have full session access. If "New" users have restricted permissions or pending activation requirements elsewhere in the system, token refresh eligibility should align with those constraints.
| // @Value("${captcha.enable-captcha}") | ||
| private boolean enableCaptcha =false; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for enableCaptcha usage and captcha validation logic
echo "=== Checking enableCaptcha usage ==="
rg -n "enableCaptcha" --type=java -B2 -A2
echo -e "\n=== Checking captcha validation service calls ==="
rg -n "captchaValidatorService|CaptchaValidationService" --type=java -B2 -A2
echo -e "\n=== Checking for null token validation ==="
rg -n "token.*null|null.*token" --type=java -B1 -A1 | head -50Repository: PSMRI/Common-API
Length of output: 9769
CAPTCHA protection is disabled and exposes authentication to brute force attacks.
With enableCaptcha hardcoded to false, the login endpoint has no bot protection against brute force or credential stuffing attacks. The null token validation logic exists in the service but is bypassed entirely when CAPTCHA is disabled. If this is intentional during a gradual rollout, document the security justification and establish a timeline for re-enabling CAPTCHA protection, or implement an alternative bot protection mechanism (rate limiting, account lockout policies, etc.).
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
around lines 83-84, the enableCaptcha flag is hardcoded false which disables
CAPTCHA and bypasses null token validation; restore configuration-driven
behavior by re-enabling the @Value("${captcha.enable-captcha}") injection (or
use Environment/ConfigurationProperties) so the flag is read from app config,
set a secure default (true) when the property is absent, and remove the
hardcoded false; if CAPTCHA must stay disabled temporarily, add clear
TODO/comment with a re-enable date and implement an alternative protection (rate
limiting and/or account lockout) and ensure the code path still enforces null
token validation even when CAPTCHA is off.
| @Operation(summary = "Get UserId based on userName") | ||
| @GetMapping(value = "/userName/{userName}", produces = MediaType.APPLICATION_JSON, headers = "Authorization") | ||
| public ResponseEntity<?> getUserDetails(@PathVariable("userName") String userName) { | ||
| try { | ||
| List<User> users = iemrAdminUserServiceImpl.getUserIdbyUserName(userName); | ||
| if (users.isEmpty()) { | ||
| return new ResponseEntity<>(Map.of("error", "UserName Not Found"), HttpStatus.NOT_FOUND); | ||
| } | ||
| User user = users.get(0); | ||
| return new ResponseEntity<>(Map.of("userName", user.getUserName(), "userId", user.getUserID()), HttpStatus.OK); | ||
| } catch (Exception e) { | ||
| return new ResponseEntity<>(Map.of("error", "Internal server error"), HttpStatus.INTERNAL_SERVER_ERROR); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the service implementation
find . -name "*.java" -type f | xargs grep -l "getUserIdbyUserName" | head -5Repository: PSMRI/Common-API
Length of output: 281
🏁 Script executed:
#!/bin/bash
# Check for rate limiting implementations in the codebase
rg -n "RateLimit|@RateLimit|Bucket4j|Resilience4j|SpringLimiter" --type=java | head -20Repository: PSMRI/Common-API
Length of output: 1126
🏁 Script executed:
#!/bin/bash
# Check for input validation on path variables in the controller
rg -n "@Valid|@Validated|@Pattern|@NotBlank" src/main/java/com/iemr/common/controller/users/IEMRAdminController.java | head -20Repository: PSMRI/Common-API
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check database schema or entity definition for userName uniqueness
find . -name "User.java" -o -name "User*.java" | xargs grep -l "class User" | head -3Repository: PSMRI/Common-API
Length of output: 255
🏁 Script executed:
#!/bin/bash
# Read the service implementation
cat src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.javaRepository: PSMRI/Common-API
Length of output: 48455
🏁 Script executed:
#!/bin/bash
# Find and read the User entity class
find . -path "*/data/users/User.java" -o -path "*/entity/User.java" | head -1 | xargs catRepository: PSMRI/Common-API
Length of output: 12916
🏁 Script executed:
#!/bin/bash
# Check if the /userName endpoint might be covered by rate limiting filters
rg -A 5 "PlatformFeedbackRateLimitFilter" src/main/java/com/iemr/common/utils/FilterConfig.javaRepository: PSMRI/Common-API
Length of output: 1251
🏁 Script executed:
#!/bin/bash
# Check the repository implementation for parameterization
find . -name "*UserRepository*.java" -type f | xargs grep -A 5 "findByUserName" | head -40Repository: PSMRI/Common-API
Length of output: 800
🏁 Script executed:
#!/bin/bash
# Check if there are any SQL injection protections or if queries are parameterized
rg -n "findByUserName|@Query|@Param" --type=java src/main/java/com/iemr/common/repository/ | head -30Repository: PSMRI/Common-API
Length of output: 5164
Security: Username enumeration vulnerability enables user enumeration attacks.
This endpoint returns different HTTP status codes (200 vs 404) based on username existence, allowing attackers to enumerate valid usernames in the system. Even with Authorization requirement, authenticated users can perform bulk enumeration without rate limiting.
Recommended mitigations:
- Return consistent HTTP responses regardless of username existence (e.g., always 200 with masked "user not found" response)
- Implement rate limiting on this endpoint to prevent bulk enumeration
- Add logging to track enumeration attempts
- Verify if this endpoint is necessary—consider using alternative mechanisms for the calling code
| @Expose | ||
| @Column(name = "dhistoken") | ||
| private String dhistoken; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for dhistoken usage
rg -n -C3 --type=java 'dhistoken' .Repository: PSMRI/Common-API
Length of output: 3482
🏁 Script executed:
#!/bin/bash
# Check for DHIS-related token patterns
rg -n -C3 --type=java -i 'dhis.*token|token.*dhis' .Repository: PSMRI/Common-API
Length of output: 3578
🏁 Script executed:
#!/bin/bash
# Check how other token fields are handled (look for @JsonIgnore patterns on token fields)
rg -n -B2 '@JsonIgnore' --type=java . | grep -A2 -B2 -i 'token'Repository: PSMRI/Common-API
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Examine the User class structure to understand serialization patterns
head -n 250 src/main/java/com/iemr/common/data/users/User.java | tail -n 50Repository: PSMRI/Common-API
Length of output: 1813
Remove @Expose from dhistoken field and apply @JsonIgnore; also address exposure in IEMRAdminController.
The field is actively exposed in API responses. In IEMRAdminController.java:395, the token is explicitly returned via resMap.put("dhistoken", mUser.getDhistoken()). Combined with @Expose at the field level, this creates a critical security vulnerability where DHIS2 integration tokens are leaked to clients.
Apply @JsonIgnore to prevent serialization and prevent this field from being included in JSON responses. Additionally, remove the explicit exposure at IEMRAdminController.java:395.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/data/users/User.java around lines 212-214,
remove the Gson @Expose annotation from the dhistoken field and annotate it with
Jackson's @JsonIgnore (add the import
com.fasterxml.jackson.annotation.JsonIgnore if missing) so the DHIS2 token is
not serialized in API responses; then update
src/main/java/com/iemr/admin/controller/IEMRAdminController.java at line ~395 to
stop explicitly returning the token by removing the resMap.put("dhistoken",
mUser.getDhistoken()) call (or conditionally omit it), ensuring no code path
exposes dhistoken in responses.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/iemr/common/service/beneficiary/IdentityBeneficiaryServiceImpl.java (1)
154-157: Avoid logging full user-controlled/PII payloads and responsesThese log lines output raw phone numbers (
phoneNo/cleanedPhoneNo), full identity/ben-create request payloads, and full ben-ID responses. Given these are user-controlled and PII-rich (health domain), this is both a privacy risk and aligns with Sonar’s “do not log user-controlled data” warning.Consider masking/truncating and logging only metadata (length/count/last few digits), and avoid logging full responses. For example:
- logger.info("Phone no from getBeneficiaryListByPhone: " + phoneNo); - String cleanedPhoneNo = cleanPhoneNumber(phoneNo); - logger.info("Cleaned phone no: " + cleanedPhoneNo); + String cleanedPhoneNo = cleanPhoneNumber(phoneNo); + String normalizedPhoneDigits = cleanedPhoneNo != null ? cleanedPhoneNo.replaceAll("\\D", "") : null; + String maskedPhoneNo = normalizedPhoneDigits != null && normalizedPhoneDigits.length() > 4 + ? "****" + normalizedPhoneDigits.substring(normalizedPhoneDigits.length() - 4) + : normalizedPhoneDigits; + logger.info("getBeneficiaryListByPhone called; cleaned phone={}", maskedPhoneNo); @@ - - logger.info("Result="+(ConfigProperties.getPropertyByName("identity-api-url-getByPhoneNum") - .replace(IDENTITY_BASE_URL, (is1097 ? identity1097BaseURL : identityBaseURL))) + cleanedPhoneNo); - - result = httpUtils.post((ConfigProperties.getPropertyByName("identity-api-url-getByPhoneNum") - .replace(IDENTITY_BASE_URL, (is1097 ? identity1097BaseURL : identityBaseURL))) + cleanedPhoneNo, "", header); + String identityGetByPhoneBaseUrl = ConfigProperties.getPropertyByName("identity-api-url-getByPhoneNum") + .replace(IDENTITY_BASE_URL, (is1097 ? identity1097BaseURL : identityBaseURL)); + String identityGetByPhoneUrl = identityGetByPhoneBaseUrl + cleanedPhoneNo; + logger.debug("Calling identity getByPhoneNum URL={}", identityGetByPhoneBaseUrl); + + result = httpUtils.post(identityGetByPhoneUrl, "", header); @@ - logger.info("Calling URL: {}", apiUrl); - logger.info("Request Payload: {}", request); + logger.info("Calling Identity benCreate endpoint"); + logger.debug("Identity benCreate URL: {}", apiUrl); + logger.debug("Identity benCreate request payload size={}", request != null ? request.length() : 0); @@ - logger.info("Request to generate ben IDs: " + request); - logger.info("Generating ben IDs API URL: " + BEN_GEN + BEN_GEN_API_URL); + logger.info("Request to generate beneficiary IDs received"); + logger.debug("Generating ben IDs API URL: {}", BEN_GEN + BEN_GEN_API_URL); @@ -result = httpUtils.post(BEN_GEN + BEN_GEN_API_URL, request, header); -logger.info("Response from generate ben IDs: " + result); + result = httpUtils.post(BEN_GEN + BEN_GEN_API_URL, request, header); @@ - JsonArray responseArray = parser.parse(s).getAsJsonArray(); + JsonArray responseArray = parser.parse(s).getAsJsonArray(); + logger.info("Generated {} beneficiary IDs", responseArray.size());This keeps useful observability while substantially reducing exposure of sensitive data and addresses the injection concern.
Also applies to: 169-173, 431-435, 439-441, 559-563
🧹 Nitpick comments (2)
src/main/java/com/iemr/common/service/beneficiary/IdentityBeneficiaryServiceImpl.java (2)
193-210: Phone normalization helper is good; consider handling more formats centrally
cleanPhoneNumbercorrectly strips common Indian country-code prefixes (+91,91for 12-digit numbers) and trims whitespace. If you expect formatted inputs (spaces, dashes, etc.) from multiple entry points, you might want to:
- Strip non-digits (
replaceAll("\\D", "")) before prefix handling.- Reuse this helper wherever phone numbers are passed to identity services, to keep normalization consistent.
Not urgent for this release, but worth centralizing over time if phone formats broaden.
431-452: Add null check on deserializedOutputResponseto prevent NPEIf the Identity API returns the literal JSON
"null",fromJsonwill yieldnull, andidentityResponse.getStatusCode()on line 445 would throw an uncaughtNullPointerException(not caught by theJsonSyntaxExceptionhandler). Add a defensive null check:try { OutputResponse identityResponse = inputMapper.gson().fromJson(result, OutputResponse.class); + if (identityResponse == null) { + throw new IEMRException("Empty response body from Identity API"); + } if (identityResponse.getStatusCode() == OutputResponse.USERID_FAILURE) { throw new IEMRException(identityResponse.getErrorMessage()); } } catch (JsonSyntaxException e) {This converts a potential NPE into a controlled
IEMRException.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/common/service/beneficiary/IdentityBeneficiaryServiceImpl.java(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-03T05:44:08.973Z
Learnt from: srishtigrp78
Repo: PSMRI/Common-API PR: 166
File: src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java:422-431
Timestamp: 2025-03-03T05:44:08.973Z
Learning: In the Common-API project, JsonNode from Jackson library has been found to not work properly for certain JSON parsing tasks, so JSONObject is preferred in those cases, particularly in the GrievanceHandlingServiceImpl class.
Applied to files:
src/main/java/com/iemr/common/service/beneficiary/IdentityBeneficiaryServiceImpl.java
🧬 Code graph analysis (1)
src/main/java/com/iemr/common/service/beneficiary/IdentityBeneficiaryServiceImpl.java (1)
src/main/java/com/iemr/common/utils/response/OutputResponse.java (1)
OutputResponse(38-244)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_check_coverage
🔇 Additional comments (1)
src/main/java/com/iemr/common/service/beneficiary/IdentityBeneficiaryServiceImpl.java (1)
54-58: Config URL injection via@Valueis properly covered across all environment profilesAll four properties (
identity-api-url,identity-1097-api-url,genben-api,generateBeneficiaryIDs-api-url) are defined in all available environment profiles (CI, Docker, Example). No runtime missing-value errors will occur. The configuration follows security best practices by using environment variable references in the CI profile (@env.VAR@).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/main/java/com/iemr/common/service/callhandling/CalltypeServiceImpl.java (2)
51-52: Prefer constructor injection over field injection.Field injection makes testing more difficult and obscures dependencies. Constructor injection is the recommended practice in Spring.
Apply this diff to use constructor injection:
@Service public class CalltypeServiceImpl implements CalltypeService { - @Autowired - private CallTypeMapper callTypeMapper; - private Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName()); InputMapper inputMapper = new InputMapper(); + private final CallTypeMapper callTypeMapper; + private final IEMRCalltypeRepositoryImplCustom iEMRCalltypeRepositoryImplCustom; + @Autowired - private IEMRCalltypeRepositoryImplCustom iEMRCalltypeRepositoryImplCustom; + public CalltypeServiceImpl(CallTypeMapper callTypeMapper, + IEMRCalltypeRepositoryImplCustom iEMRCalltypeRepositoryImplCustom) { + this.callTypeMapper = callTypeMapper; + this.iEMRCalltypeRepositoryImplCustom = iEMRCalltypeRepositoryImplCustom; + }
87-90: Consider using CallTypeMapper consistently across methods.The
getAllCalltypesmethod now usescallTypeMapperfor JSON deserialization (line 62), butgetAllCalltypesV1still usesObjectMapperdirectly. For consistency and maintainability, consider using the same deserialization approach.src/main/java/com/iemr/common/utils/mapper/CallTypeMapper.java (1)
33-42: Adjust logging level and improve exception handling.Two concerns with the current implementation:
Verbose logging: Line 36 logs at INFO level for every deserialization, which can generate excessive logs in production. Consider DEBUG level instead.
Generic exception handling: Lines 38-40 catch and re-throw all exceptions. While logging is good, consider wrapping in a custom exception or declaring specific checked exceptions in the method signature.
Apply this diff:
public <T> T fromJson(String json, Class<T> classOfT) { try { T result = gson().fromJson(json, classOfT); - logger.info("Successfully deserialized to class: {}", classOfT.getSimpleName()); + logger.debug("Successfully deserialized to class: {}", classOfT.getSimpleName()); return result; - } catch (Exception e) { + } catch (JsonSyntaxException | JsonIOException e) { logger.error("Error deserializing JSON to {}: {}", classOfT.getSimpleName(), e.getMessage(), e); - throw e; + throw new IllegalArgumentException("Failed to deserialize JSON to " + classOfT.getSimpleName(), e); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/iemr/common/service/callhandling/CalltypeServiceImpl.java(2 hunks)src/main/java/com/iemr/common/utils/mapper/CallTypeMapper.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/com/iemr/common/utils/mapper/CallTypeMapper.java (1)
src/main/java/com/iemr/common/service/callhandling/CalltypeServiceImpl.java (1)
Service(48-151)
src/main/java/com/iemr/common/service/callhandling/CalltypeServiceImpl.java (1)
src/main/java/com/iemr/common/utils/mapper/CallTypeMapper.java (1)
Service(10-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_check_coverage
| private CallTypeMapper callTypeMapper; | ||
|
|
||
| private Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName()); | ||
| InputMapper inputMapper = new InputMapper(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if inputMapper is used in CalltypeServiceImpl
rg -n 'inputMapper' --type java src/main/java/com/iemr/common/service/callhandling/CalltypeServiceImpl.javaRepository: PSMRI/Common-API
Length of output: 108
Remove unused InputMapper inputMapper field.
The field at line 55 is declared but never referenced in this file. It should be removed.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/service/callhandling/CalltypeServiceImpl.java
around line 55, remove the unused local field declaration "InputMapper
inputMapper = new InputMapper();" because it is never referenced; simply delete
that line and run a build/compile to ensure no remaining references rely on it.
| @Service | ||
| public class CallTypeMapper { | ||
| private static GsonBuilder builder; | ||
| private static Gson gsonInstance; | ||
| private Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName()); | ||
|
|
||
| public CallTypeMapper() { | ||
| if (builder == null) { | ||
| builder = new GsonBuilder(); | ||
| // Only serialize/deserialize fields with @Expose annotation | ||
| builder.excludeFieldsWithoutExposeAnnotation(); | ||
|
|
||
| logger.info("CallTypeMapper initialized - Only @Expose fields will be processed"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Race condition and design anti-pattern with static fields in Spring bean.
This implementation has multiple critical issues:
-
Thread-safety: Lines 17-20 have a race condition where multiple threads can simultaneously check
if (builder == null)and both enter the initialization block. -
Anti-pattern: The class is annotated with
@Service(instance-scoped Spring bean) but uses static fields. When Spring creates multiple instances or the bean is recreated, the static state persists, leading to unpredictable behavior. -
Initialization order: If another thread calls the static
gson()method before any instance is constructed,builderwill be null, causing a NullPointerException.
Apply this diff to fix the design by using proper instance fields or a thread-safe static initialization:
Option 1 (Recommended): Use instance fields
@Service
public class CallTypeMapper {
- private static GsonBuilder builder;
- private static Gson gsonInstance;
+ private final Gson gsonInstance;
private Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName());
public CallTypeMapper() {
- if (builder == null) {
- builder = new GsonBuilder();
- // Only serialize/deserialize fields with @Expose annotation
- builder.excludeFieldsWithoutExposeAnnotation();
-
- logger.info("CallTypeMapper initialized - Only @Expose fields will be processed");
- }
+ GsonBuilder builder = new GsonBuilder();
+ // Only serialize/deserialize fields with @Expose annotation
+ builder.excludeFieldsWithoutExposeAnnotation();
+ this.gsonInstance = builder.create();
+ logger.info("CallTypeMapper initialized - Only @Expose fields will be processed");
}
- public static Gson gson() {
- if (gsonInstance == null) {
- gsonInstance = builder.create();
- }
+ public Gson gson() {
return gsonInstance;
}Option 2: Thread-safe static initialization (if shared instance is required)
@Service
public class CallTypeMapper {
- private static GsonBuilder builder;
- private static Gson gsonInstance;
+ private static final Gson gsonInstance;
private Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName());
+
+ static {
+ GsonBuilder builder = new GsonBuilder();
+ builder.excludeFieldsWithoutExposeAnnotation();
+ gsonInstance = builder.create();
+ }
public CallTypeMapper() {
- if (builder == null) {
- builder = new GsonBuilder();
- // Only serialize/deserialize fields with @Expose annotation
- builder.excludeFieldsWithoutExposeAnnotation();
-
- logger.info("CallTypeMapper initialized - Only @Expose fields will be processed");
- }
+ logger.info("CallTypeMapper initialized - Only @Expose fields will be processed");
}
public static Gson gson() {
- if (gsonInstance == null) {
- gsonInstance = builder.create();
- }
return gsonInstance;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/utils/mapper/CallTypeMapper.java around lines
10 to 24, the class uses static GsonBuilder/gsonInstance with lazy
non-synchronized initialization causing race conditions, NPEs if static access
occurs before an instance is constructed, and is an anti-pattern for a Spring
@Service; fix by making builder and gsonInstance instance fields (remove static)
and initialize them in the constructor (ensuring the public gson() method
becomes an instance method or obtain the bean from Spring), or if a shared
static instance is required, replace the lazy check with a thread-safe static
initialization (e.g., a private static final Gson built in a static initializer
or using an enum/singleton holder) so initialization is atomic and race-free;
also ensure any static-access methods are updated accordingly to avoid NPEs.
|
| sms-gateway-url = | ||
|
|
||
| #Encryption passphrase used by AESUtil for decrypting CTI passwords. | ||
| encryption.passphrase=Piramal12Piramal |
Check notice
Code scanning / SonarCloud
Credentials should not be hard-coded Low
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/dto/sms/SMSTemplateDTO.java (1)
3-114: Consider using Lombok @builder for cleaner DTO construction.The DTO uses standard getters/setters. Consider adding Lombok's
@Builderannotation to reduce boilerplate and enable fluent object construction, which would be helpful when mapping from entities.🔎 View suggested enhancement
+import lombok.Builder; + +@Builder public class SMSTemplateDTO { private Integer smsTemplateID; private String smsTemplateName;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/com/iemr/common/data/mmuDrugHistory/PrescribedMMUDrugDetail.java(1 hunks)src/main/java/com/iemr/common/data/mmuDrugHistory/PrescriptionMMU.java(1 hunks)src/main/java/com/iemr/common/dto/sms/SMSTemplateDTO.java(1 hunks)src/main/java/com/iemr/common/mapper/sms/SMSMapper.java(3 hunks)src/main/java/com/iemr/common/repository/mmuDrugHistory/PrescribedMMUDrugRepository.java(1 hunks)src/main/java/com/iemr/common/service/sms/SMSServiceImpl.java(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/com/iemr/common/data/mmuDrugHistory/PrescriptionMMU.java (1)
src/main/java/com/iemr/common/data/mmuDrugHistory/PrescribedMMUDrugDetail.java (1)
Entity(20-119)
src/main/java/com/iemr/common/data/mmuDrugHistory/PrescribedMMUDrugDetail.java (1)
src/main/java/com/iemr/common/data/mmuDrugHistory/PrescriptionMMU.java (1)
Entity(13-43)
src/main/java/com/iemr/common/service/sms/SMSServiceImpl.java (2)
src/main/java/com/iemr/common/dto/sms/SMSTemplateDTO.java (1)
SMSTemplateDTO(3-114)src/main/java/com/iemr/common/utils/InputSanitizer.java (1)
InputSanitizer(8-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_check_coverage
🔇 Additional comments (9)
src/main/java/com/iemr/common/repository/mmuDrugHistory/PrescribedMMUDrugRepository.java (1)
7-10: LGTM!Clean repository interface following Spring Data JPA conventions. The finder method signature is appropriate for querying by prescribedDrugID.
src/main/java/com/iemr/common/mapper/sms/SMSMapper.java (1)
116-128: LGTM!The explicit field mappings and expression-based date formatting are clear and correct. The mapping properly delegates to the
formatDatehelper for timestamp-to-string conversion.src/main/java/com/iemr/common/service/sms/SMSServiceImpl.java (4)
217-237: Good security practices with input sanitization and transaction management.The method properly sanitizes inputs before persistence and validates template syntax. The
@Transactionalannotation ensures atomicity when saving both the template and parameter maps.
239-276: Excellent input sanitization implementation.The method applies appropriate sanitization strategies for different field types:
- Full sanitization for names
- XSS-only sanitization for templates (preserves placeholders)
- Parameter values sanitized to prevent injection attacks
705-763: MMUPrescription case properly integrated.The new case follows the established pattern and correctly delegates to
getMMUPrescriptionDatafor extracting MMU prescription details.
1328-1427: Well-implemented MMU prescription data extraction.The method properly:
- Fetches prescription details from the repository
- Uses
StringBuilderfor efficient string concatenation- Handles null values safely throughout
- Formats composite fields logically (e.g., "diagnosis, drugName(strength)")
src/main/java/com/iemr/common/data/mmuDrugHistory/PrescriptionMMU.java (1)
13-42: LGTM!Clean JPA entity with proper column mappings. The
insertable=false, updatable=falseon the primary key andinsertable=false, updatable=trueon the deleted field are correctly configured for database-managed columns.src/main/java/com/iemr/common/data/mmuDrugHistory/PrescribedMMUDrugDetail.java (2)
51-53: LGTM!The
@OneToOnerelationship toPrescriptionMMUis correctly configured withinsertable=false, updatable=false, making this a read-only foreign key reference.
91-93: Verify column name spelling - "DuartionUnit" appears to be a typo for "DurationUnit".Line 92 defines the column name as "DuartionUnit", which is almost certainly a spelling error. The correct spelling should be "DurationUnit". This mapping issue will cause a runtime error if the database column uses the correct spelling. Verify the actual database column name in the
t_prescribeddrugtable and correct accordingly.
| default String formatDate(Timestamp timestamp) { | ||
| if (timestamp == null) { | ||
| return null; | ||
| } | ||
| SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"); | ||
| return sdf.format(timestamp); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace SimpleDateFormat with DateTimeFormatter for thread safety and correctness.
Two issues:
- Creating a new
SimpleDateFormaton each call is inefficient - The pattern uses
'Z'as a literal but doesn't set timezone, which may not produce valid ISO-8601 timestamps
Use DateTimeFormatter (thread-safe and more efficient):
🔎 Apply this diff to use DateTimeFormatter:
+import java.time.format.DateTimeFormatter;
+
+ private static final DateTimeFormatter DATE_FORMATTER =
+ DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'");
+
default String formatDate(Timestamp timestamp) {
if (timestamp == null) {
return null;
}
- SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'");
- return sdf.format(timestamp);
+ return timestamp.toInstant()
+ .atZone(java.time.ZoneId.of("UTC"))
+ .format(DATE_FORMATTER);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/common/mapper/sms/SMSMapper.java around lines 130 to
136, replace the per-call SimpleDateFormat with a thread-safe DateTimeFormatter:
make a static final DateTimeFormatter (e.g. DateTimeFormatter.ISO_INSTANT or a
DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'").withZone(ZoneOffset.UTC)),
then in formatDate return null for null input and format timestamp.toInstant()
with that formatter so the output is valid ISO-8601 in UTC and avoids creating a
new formatter each call.
📋 Description
Production release of v 3.6.0
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.