-
Notifications
You must be signed in to change notification settings - Fork 54
fix(security): mask patient PII in logs and return correct HTTP status codes (AMRIT#153) #213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
8270694
5e470ae
38c9f02
a332041
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| /* | ||
| * 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/. | ||
| */ | ||
| package com.iemr.hwc.utils.exception; | ||
|
|
||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.http.converter.HttpMessageNotReadableException; | ||
| import org.springframework.web.HttpRequestMethodNotSupportedException; | ||
| import org.springframework.web.bind.MethodArgumentNotValidException; | ||
| import org.springframework.web.bind.MissingServletRequestParameterException; | ||
| import org.springframework.web.bind.annotation.ExceptionHandler; | ||
| import org.springframework.web.bind.annotation.RestControllerAdvice; | ||
|
|
||
| import com.iemr.hwc.utils.response.OutputResponse; | ||
|
|
||
| /** | ||
| * Centralised exception translation for HWC-API REST controllers. Every | ||
| * uncaught exception is converted into an {@link OutputResponse} body with a | ||
| * correct HTTP status code instead of the legacy behaviour of returning HTTP | ||
| * 200 with an error embedded in the body. Exception messages are deliberately | ||
| * not echoed into log messages because they may contain values originating | ||
| * from beneficiary payloads; the stack trace is still captured by SLF4J for | ||
| * server-side diagnosis. | ||
| */ | ||
| @RestControllerAdvice | ||
| public class GlobalExceptionHandler { | ||
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(GlobalExceptionHandler.class); | ||
| private static final String GENERIC_BAD_REQUEST = "Invalid request"; | ||
| private static final String GENERIC_SERVER_ERROR = "Unexpected server error"; | ||
|
|
||
| @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(); | ||
| } | ||
|
|
||
| @ExceptionHandler(HttpMessageNotReadableException.class) | ||
| public ResponseEntity<String> handleHttpMessageNotReadable(HttpMessageNotReadableException ex) { | ||
| LOGGER.error("Malformed request body received", ex); | ||
| OutputResponse response = new OutputResponse(); | ||
| response.setError(OutputResponse.BAD_REQUEST, GENERIC_BAD_REQUEST); | ||
| return response.toStringWithHttpStatus(); | ||
| } | ||
|
|
||
| @ExceptionHandler(MissingServletRequestParameterException.class) | ||
| public ResponseEntity<String> handleMissingParameter(MissingServletRequestParameterException ex) { | ||
| LOGGER.error("Missing required request parameter", ex); | ||
| OutputResponse response = new OutputResponse(); | ||
| response.setError(OutputResponse.BAD_REQUEST, "Missing required parameter: " + ex.getParameterName()); | ||
| return response.toStringWithHttpStatus(); | ||
| } | ||
|
|
||
| @ExceptionHandler(MethodArgumentNotValidException.class) | ||
| public ResponseEntity<String> handleValidation(MethodArgumentNotValidException ex) { | ||
| LOGGER.error("Request payload failed validation", ex); | ||
| OutputResponse response = new OutputResponse(); | ||
| response.setError(OutputResponse.BAD_REQUEST, "Request payload failed validation"); | ||
| return response.toStringWithHttpStatus(); | ||
| } | ||
|
|
||
| @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(); | ||
| } | ||
|
Comment on lines
+80
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Map unsupported HTTP methods to 405 instead of 400. Line 96 sets Proposed direction- response.setError(OutputResponse.BAD_REQUEST, "HTTP method not supported");
+ response.setError(OutputResponse.METHOD_NOT_ALLOWED, "HTTP method not supported");Also add 🧰 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 |
||
|
|
||
| @ExceptionHandler(Exception.class) | ||
| public ResponseEntity<String> handleAny(Exception ex) { | ||
| LOGGER.error("Unhandled exception at controller boundary", ex); | ||
| OutputResponse response = new OutputResponse(); | ||
| response.setError(OutputResponse.GENERIC_FAILURE, GENERIC_SERVER_ERROR); | ||
| return response.toStringWithHttpStatus(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| /* | ||
| * 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/. | ||
| */ | ||
| package com.iemr.hwc.utils.logging; | ||
|
|
||
| import java.util.Arrays; | ||
| import java.util.Iterator; | ||
| import java.util.Locale; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| import com.fasterxml.jackson.core.JsonProcessingException; | ||
| import com.fasterxml.jackson.databind.JsonNode; | ||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import com.fasterxml.jackson.databind.node.ObjectNode; | ||
| import com.fasterxml.jackson.databind.node.TextNode; | ||
|
|
||
| /** | ||
| * Redacts beneficiary personally identifiable information (PII) and personal | ||
| * health information (PHI) from JSON payloads before they reach application | ||
| * logs. Field matching is case-insensitive against a curated AMRIT-domain set. | ||
| * Unparseable payloads are replaced with a length-only summary so raw bodies | ||
| * never leak even when JSON parsing fails. | ||
| */ | ||
| public final class LogMasker { | ||
|
|
||
| private static final ObjectMapper MAPPER = new ObjectMapper(); | ||
| private static final String MASK = "***"; | ||
| private static final String NULL_PAYLOAD = "[null payload]"; | ||
|
|
||
| private static final Set<String> SENSITIVE_KEYS = Arrays.stream(new String[] { | ||
| // government identifiers | ||
| "aadhaar", "aadhaarno", "aadharno", "aadhaarnumber", "aadharnumber", "aadhaarcardno", | ||
| "pan", "panno", "pannumber", | ||
| "voterid", "voteridnumber", "epicno", | ||
| "drivinglicence", "drivinglicense", "drivinglicensenumber", | ||
| "passportno", "passportnumber", | ||
| // health identifiers | ||
| "abha", "abhaaddress", "healthid", "healthidnumber", "healthaccountnumber", | ||
| "hwid", "hpid", | ||
| // contact | ||
| "mobile", "mobileno", "mobilenumber", "phone", "phoneno", "phonenumber", | ||
| "email", "emailid", "emailaddress", | ||
| "emergencycontactname", "emergencycontactnumber", | ||
| "fatheremergencycontactname", "fathercontactnumber", | ||
| // names | ||
| "firstname", "lastname", "middlename", "fullname", "name", | ||
| "fathername", "mothername", "husbandname", "spousename", "guardianname", | ||
| // dob | ||
| "dob", "dateofbirth", | ||
| // address | ||
| "address", "address1", "address2", "addressline1", "addressline2", | ||
| "city", "district", "state", "pincode", "zipcode", "village", | ||
| // free text likely to contain PII | ||
| "remarks", "notes" }) | ||
| .collect(Collectors.toUnmodifiableSet()); | ||
|
|
||
| private LogMasker() { | ||
| // no-instantiation | ||
| } | ||
|
|
||
| /** | ||
| * Returns the input JSON with sensitive field values replaced by {@value #MASK}. | ||
| * Non-JSON payloads are summarised by length rather than echoed verbatim. | ||
| * | ||
| * @param payload raw request or response body; may be {@code null} | ||
| * @return masked payload safe for logging | ||
| */ | ||
| public static String maskJson(String payload) { | ||
| if (payload == null) { | ||
| return NULL_PAYLOAD; | ||
| } | ||
| if (payload.isEmpty()) { | ||
| return payload; | ||
| } | ||
| try { | ||
| JsonNode root = MAPPER.readTree(payload); | ||
| maskNode(root); | ||
| return MAPPER.writeValueAsString(root); | ||
| } catch (JsonProcessingException e) { | ||
| return "[REDACTED non-json payload, length=" + payload.length() + "]"; | ||
| } | ||
| } | ||
|
|
||
| private static void maskNode(JsonNode node) { | ||
| if (node == null) { | ||
| return; | ||
| } | ||
| if (node.isObject()) { | ||
| ObjectNode obj = (ObjectNode) node; | ||
| Iterator<Map.Entry<String, JsonNode>> fields = obj.fields(); | ||
| while (fields.hasNext()) { | ||
| Map.Entry<String, JsonNode> entry = fields.next(); | ||
| if (isSensitive(entry.getKey()) && !entry.getValue().isNull()) { | ||
| obj.set(entry.getKey(), TextNode.valueOf(MASK)); | ||
| } else { | ||
| maskNode(entry.getValue()); | ||
| } | ||
| } | ||
| } else if (node.isArray()) { | ||
| for (JsonNode item : node) { | ||
| maskNode(item); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private static boolean isSensitive(String fieldName) { | ||
| if (fieldName == null) { | ||
| return false; | ||
| } | ||
| return SENSITIVE_KEYS.contains(fieldName.toLowerCase(Locale.ROOT)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid echoing exception messages in API bodies for IEMR/TM handlers.
Line 56 and Line 64 currently delegate to
setError(Throwable), which propagatesex.getMessage()intoerrorMessage. 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