fix(security): mask patient PII in logs and return correct HTTP status codes (AMRIT#153)#213
Open
Piyush0000 wants to merge 4 commits into
Open
Conversation
…I#153) GeneralOPDController previously returned 'String' from every endpoint, which forced Spring to respond with HTTP 200 regardless of outcome and embedded error details only in the body. The same controllers also INFO-logged the full request payload, exposing beneficiary vitals, history, examinations and prescriptions to log aggregators. Changes: * OutputResponse: BAD_REQUEST is now 400 per RFC 9110; introduce NOT_FOUND (404) so existing callers using the literal 404 for not-found semantics keep working. TM_EXCEPTION (5011) is now distinct from SWYMED_EXCEPTION (5010). toStringWithHttpStatus() maps every internal code to its appropriate HTTP status instead of defaulting unknown codes to 503. * GlobalExceptionHandler: new @RestControllerAdvice translating IEMRException, TMException and standard Spring web exceptions to correct HTTP status codes. Exception messages are not echoed into log lines because they can carry beneficiary values; stack traces remain captured via SLF4J for diagnosis. The catch-all returns 500 with a generic body so internal details do not leak. * LogMasker: JSON-aware masker for AMRIT-domain PII / PHI fields (Aadhaar, ABHA, names, DOB, address, contact, free-text remarks). Matching is case-insensitive; unparseable payloads are replaced with a length-only summary so raw bytes never reach logs. * GeneralOPDController: every endpoint now returns ResponseEntity<String> via toStringWithHttpStatus(); request-body logs route through LogMasker.maskJson; catch-block error logs use the (msg, throwable) overload instead of concatenating exception.getMessage(). Tests cover the OutputResponse status mapping (including legacy 404 callers), the LogMasker behaviour for nested objects, arrays, case-insensitive keys and non-JSON fallback, and the GlobalExceptionHandler mapping for each handled exception type plus the non-echo guarantee on generic exceptions.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR standardizes HTTP status code responses and secures patient data logging across the HWC-API. GeneralOPDController endpoints now return ChangesHTTP Status Codes & Secure Logging
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
…ionHandler The seven @ExceptionHandler methods all repeated the same four-line sequence (log via SLF4J, instantiate OutputResponse, setError, return toStringWithHttpStatus()), which SonarCloud flagged as 3.1% duplication on new code in PR PSMRI#213. Collapse the duplicated body into two private helpers: * respond(ex, logMessage) - for exceptions whose mapping is fully described by OutputResponse.setError(Throwable) (IEMRException, TMException). * respond(ex, logMessage, statusCode, publicMessage) - for the Spring-framework exceptions where the handler chooses the public status code and message. Each handler now reduces to a single return statement. External behaviour is identical: the same OutputResponse mutations and the same ResponseEntity are produced, so the existing GlobalExceptionHandlerTest assertions about HTTP status and body content remain valid without any test changes.
…eturn SonarCloud continued to report 3.1% duplication on new code after the GlobalExceptionHandler cleanup. The remaining duplicate token block was the eight 'return ResponseEntity.status(HttpStatus.X).body(output);' returns inside OutputResponse.toStringWithHttpStatus() - structurally identical except for the HttpStatus argument. Replace the legacy statement switch with a Java 17 switch expression that maps each internal status code to its HttpStatus once, assigns it to resolvedStatus, then issues a single 'return ResponseEntity.status(resolvedStatus).body(output);'. Behaviour matrix is unchanged: GENERIC_FAILURE and CODE_EXCEPTION still resolve to INTERNAL_SERVER_ERROR (now via the default arm, which was already INTERNAL_SERVER_ERROR), and every other mapping is identical. Existing OutputResponseTest assertions about each HttpStatus, the unknown-code fallback to 500, and the response body shape continue to hold without test changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/iemr/hwc/controller/generalOPD/GeneralOPDController.java (1)
93-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFailure branches still return HTTP 200 due to
setResponse(...).In these branches, failure strings are set via
setResponse(...), which marks status as success. That reintroduces the “200 with error text” behavior this PR is fixing.Proposed fix
- response.setResponse("Invalid request"); + response.setError(OutputResponse.BAD_REQUEST, "Invalid request"); ... - response.setResponse("Unable to save data"); + response.setError(OutputResponse.GENERIC_FAILURE, "Unable to save data"); ... - response.setResponse("Invalid request"); + response.setError(OutputResponse.BAD_REQUEST, "Invalid request");Also applies to: 145-151
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/hwc/controller/generalOPD/GeneralOPDController.java` around lines 93 - 97, The controller currently returns failure messages by calling response.setResponse(...) which still results in an HTTP 200; update the failure branches in GeneralOPDController (the branch around the saveNurseData(jsnOBJ, Authorization) call and the similar branch at the later block) to return an actual error HTTP status instead of setResponse("Invalid request"). Replace the setResponse(...) on failure with either setting the response status/code (e.g., response.setStatus(...) or response.setHttpStatus(...)) or throw a framework exception like new ResponseStatusException(HttpStatus.BAD_REQUEST, "Invalid request") so the HTTP status is non-200 and the error message is preserved; ensure saveNurseData success path still sets the normal response body.
🧹 Nitpick comments (1)
src/test/java/com/iemr/hwc/utils/exception/GlobalExceptionHandlerTest.java (1)
57-69: ⚡ Quick winAdd non-echo assertions for IEMR/TM exception bodies.
These tests verify status only; please also assert response bodies do not contain the raw exception messages to lock in PII-safe behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/iemr/hwc/utils/exception/GlobalExceptionHandlerTest.java` around lines 57 - 69, The tests iemrExceptionMapsToHttp401WithGenericBody and tmExceptionMapsToHttp400 only assert status codes but must also assert that the response bodies do not echo raw exception messages; update both tests to call handler.handleIEMRException(...) and handler.handleTMException(...), capture response.getBody(), assert it does not contain the original exception strings ("password rejected" and "ben-id mismatch") and instead verify a generic/PII-safe message is present (or at least not null/empty) to ensure PII is not leaked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/iemr/hwc/utils/exception/GlobalExceptionHandler.java`:
- Around line 92-98: handleMethodNotSupported currently sets
OutputResponse.BAD_REQUEST causing a 400; change it to use a new
OutputResponse.METHOD_NOT_ALLOWED constant (value 405) and update
OutputResponse.toStringWithHttpStatus() so that when the response code equals
METHOD_NOT_ALLOWED it maps to HttpStatus.METHOD_NOT_ALLOWED; specifically: add
METHOD_NOT_ALLOWED = 405 to OutputResponse, change
GlobalExceptionHandler.handleMethodNotSupported to
setError(OutputResponse.METHOD_NOT_ALLOWED, "..."), and extend
toStringWithHttpStatus() to return HttpStatus.METHOD_NOT_ALLOWED for that code.
- Around line 52-66: The handlers handleIEMRException and handleTMException
currently call OutputResponse.setError(ex) which copies ex.getMessage() into the
API body; change them to avoid echoing internal exception messages by setting a
sanitized/generic error on the OutputResponse instead (e.g., set a generic error
code and a non-sensitive message like "Internal error" or populate only an
errorCode field), or call a new OutputResponse method that accepts only
non-sensitive details; update calls in handleIEMRException and handleTMException
to use that sanitized setter (or clear the message before passing) rather than
passing the original exception instance.
---
Outside diff comments:
In `@src/main/java/com/iemr/hwc/controller/generalOPD/GeneralOPDController.java`:
- Around line 93-97: The controller currently returns failure messages by
calling response.setResponse(...) which still results in an HTTP 200; update the
failure branches in GeneralOPDController (the branch around the
saveNurseData(jsnOBJ, Authorization) call and the similar branch at the later
block) to return an actual error HTTP status instead of setResponse("Invalid
request"). Replace the setResponse(...) on failure with either setting the
response status/code (e.g., response.setStatus(...) or
response.setHttpStatus(...)) or throw a framework exception like new
ResponseStatusException(HttpStatus.BAD_REQUEST, "Invalid request") so the HTTP
status is non-200 and the error message is preserved; ensure saveNurseData
success path still sets the normal response body.
---
Nitpick comments:
In `@src/test/java/com/iemr/hwc/utils/exception/GlobalExceptionHandlerTest.java`:
- Around line 57-69: The tests iemrExceptionMapsToHttp401WithGenericBody and
tmExceptionMapsToHttp400 only assert status codes but must also assert that the
response bodies do not echo raw exception messages; update both tests to call
handler.handleIEMRException(...) and handler.handleTMException(...), capture
response.getBody(), assert it does not contain the original exception strings
("password rejected" and "ben-id mismatch") and instead verify a
generic/PII-safe message is present (or at least not null/empty) to ensure PII
is not leaked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8d47e97-e58d-48f4-8bd0-f68e28817f88
📒 Files selected for processing (7)
src/main/java/com/iemr/hwc/controller/generalOPD/GeneralOPDController.javasrc/main/java/com/iemr/hwc/utils/exception/GlobalExceptionHandler.javasrc/main/java/com/iemr/hwc/utils/logging/LogMasker.javasrc/main/java/com/iemr/hwc/utils/response/OutputResponse.javasrc/test/java/com/iemr/hwc/utils/exception/GlobalExceptionHandlerTest.javasrc/test/java/com/iemr/hwc/utils/logging/LogMaskerTest.javasrc/test/java/com/iemr/hwc/utils/response/OutputResponseTest.java
Comment on lines
+52
to
+66
| @ExceptionHandler(IEMRException.class) | ||
| public ResponseEntity<String> handleIEMRException(IEMRException ex) { | ||
| LOGGER.error("IEMRException raised at controller boundary", ex); | ||
| OutputResponse response = new OutputResponse(); | ||
| response.setError(ex); | ||
| return response.toStringWithHttpStatus(); | ||
| } | ||
|
|
||
| @ExceptionHandler(TMException.class) | ||
| public ResponseEntity<String> handleTMException(TMException ex) { | ||
| LOGGER.error("TMException raised at controller boundary", ex); | ||
| OutputResponse response = new OutputResponse(); | ||
| response.setError(ex); | ||
| return response.toStringWithHttpStatus(); | ||
| } |
There was a problem hiding this comment.
Avoid echoing exception messages in API bodies for IEMR/TM handlers.
Line 56 and Line 64 currently delegate to setError(Throwable), which propagates ex.getMessage() into errorMessage. That can expose internal/sensitive values to clients.
Proposed fix
`@ExceptionHandler`(IEMRException.class)
public ResponseEntity<String> handleIEMRException(IEMRException ex) {
LOGGER.error("IEMRException raised at controller boundary", ex);
OutputResponse response = new OutputResponse();
- response.setError(ex);
+ response.setError(OutputResponse.USERID_FAILURE, "Authentication failed");
return response.toStringWithHttpStatus();
}
`@ExceptionHandler`(TMException.class)
public ResponseEntity<String> handleTMException(TMException ex) {
LOGGER.error("TMException raised at controller boundary", ex);
OutputResponse response = new OutputResponse();
- response.setError(ex);
+ response.setError(OutputResponse.TM_EXCEPTION, GENERIC_BAD_REQUEST);
return response.toStringWithHttpStatus();
}🧰 Tools
🪛 PMD (7.24.0)
[Low] 54-54: InvalidLogMessageFormat (Error Prone): Too many arguments, expected 0 argument but found 1
(InvalidLogMessageFormat (Error Prone))
[Low] 62-62: InvalidLogMessageFormat (Error Prone): Too many arguments, expected 0 argument but found 1
(InvalidLogMessageFormat (Error Prone))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/hwc/utils/exception/GlobalExceptionHandler.java`
around lines 52 - 66, The handlers handleIEMRException and handleTMException
currently call OutputResponse.setError(ex) which copies ex.getMessage() into the
API body; change them to avoid echoing internal exception messages by setting a
sanitized/generic error on the OutputResponse instead (e.g., set a generic error
code and a non-sensitive message like "Internal error" or populate only an
errorCode field), or call a new OutputResponse method that accepts only
non-sensitive details; update calls in handleIEMRException and handleTMException
to use that sanitized setter (or clear the message before passing) rather than
passing the original exception instance.
Comment on lines
+92
to
+98
| @ExceptionHandler(HttpRequestMethodNotSupportedException.class) | ||
| public ResponseEntity<String> handleMethodNotSupported(HttpRequestMethodNotSupportedException ex) { | ||
| LOGGER.error("Unsupported HTTP method", ex); | ||
| OutputResponse response = new OutputResponse(); | ||
| response.setError(OutputResponse.BAD_REQUEST, "HTTP method not supported"); | ||
| return response.toStringWithHttpStatus(); | ||
| } |
There was a problem hiding this comment.
Map unsupported HTTP methods to 405 instead of 400.
Line 96 sets BAD_REQUEST, so this path returns HTTP 400. For HttpRequestMethodNotSupportedException, expected status is 405 Method Not Allowed.
Proposed direction
- response.setError(OutputResponse.BAD_REQUEST, "HTTP method not supported");
+ response.setError(OutputResponse.METHOD_NOT_ALLOWED, "HTTP method not supported");Also add METHOD_NOT_ALLOWED = 405 in OutputResponse and map it to HttpStatus.METHOD_NOT_ALLOWED in toStringWithHttpStatus().
🧰 Tools
🪛 PMD (7.24.0)
[Low] 94-94: InvalidLogMessageFormat (Error Prone): Too many arguments, expected 0 argument but found 1
(InvalidLogMessageFormat (Error Prone))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/hwc/utils/exception/GlobalExceptionHandler.java`
around lines 92 - 98, handleMethodNotSupported currently sets
OutputResponse.BAD_REQUEST causing a 400; change it to use a new
OutputResponse.METHOD_NOT_ALLOWED constant (value 405) and update
OutputResponse.toStringWithHttpStatus() so that when the response code equals
METHOD_NOT_ALLOWED it maps to HttpStatus.METHOD_NOT_ALLOWED; specifically: add
METHOD_NOT_ALLOWED = 405 to OutputResponse, change
GlobalExceptionHandler.handleMethodNotSupported to
setError(OutputResponse.METHOD_NOT_ALLOWED, "..."), and extend
toStringWithHttpStatus() to return HttpStatus.METHOD_NOT_ALLOWED for that code.
…ror calls
Eight new lines in GeneralOPDController contained the identical token
sequence
response.setError(OutputResponse.GENERIC_FAILURE, "Unable to modify data");
across the four update endpoints (each method invoked it once in the
fallback else-branch and once in the catch block). SonarCloud's
clone detector flagged that pattern as the largest residual duplicated
block on new code (PR PSMRI#213, 3.11% > 3.0% gate).
Replace those eight call sites with a one-line private helper:
markUnableToModify(response);
Each call site is now a 4-token method invocation - well below
SonarCloud's minimum clone size - and the failure literal lives in a
single place. Behaviour is identical: the helper performs exactly the
same OutputResponse.setError(GENERIC_FAILURE, "Unable to modify data")
call the eight inline statements used to make.
No tests touched and no other file changed.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes PSMRI/AMRIT#153
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes