From a0a08ddd2ee804638efbe0ecd04b74c764e699da Mon Sep 17 00:00:00 2001 From: Lingeswaran Subramaniyam Date: Thu, 13 Aug 2026 12:20:00 +0530 Subject: [PATCH 1/9] Path traversal to arbitrary Groovy execution via the flowSheet --- .../extensions/BahmniExtensions.java | 21 ++++- .../extensions/BahmniExtensionsTest.java | 86 +++++++++++++++++++ 2 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/extensions/BahmniExtensionsTest.java diff --git a/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/extensions/BahmniExtensions.java b/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/extensions/BahmniExtensions.java index dd6e2f1d9b..f4587157b4 100644 --- a/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/extensions/BahmniExtensions.java +++ b/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/extensions/BahmniExtensions.java @@ -1,6 +1,7 @@ package org.bahmni.module.bahmnicore.extensions; import groovy.lang.GroovyClassLoader; +import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.bahmni.module.bahmnicore.dao.ApplicationDataDirectory; @@ -9,6 +10,7 @@ import java.io.File; import java.io.IOException; +import java.nio.file.Path; @Component public class BahmniExtensions { @@ -26,8 +28,23 @@ public BahmniExtensions() { } public Object getExtension(String directory, String fileName) { - File groovyFile = applicationDataDirectory - .getFileFromConfig("openmrs" + File.separator + directory + File.separator + fileName); + if (StringUtils.isBlank(fileName)) { + log.error("Extension file name is required for directory " + directory); + return null; + } + + Path extensionDir = applicationDataDirectory + .getFileFromConfig("openmrs" + File.separator + directory).toPath().normalize(); + Path groovyFilePath = applicationDataDirectory + .getFileFromConfig("openmrs" + File.separator + directory + File.separator + fileName) + .toPath().normalize(); + + if (!groovyFilePath.startsWith(extensionDir)) { + log.error("Rejected extension file resolving outside of the allowed directory for extension type " + directory); + return null; + } + + File groovyFile = groovyFilePath.toFile(); if (!groovyFile.exists()) { log.error("File not found " + groovyFile.getAbsolutePath()); } else { diff --git a/bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/extensions/BahmniExtensionsTest.java b/bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/extensions/BahmniExtensionsTest.java new file mode 100644 index 0000000000..6064995286 --- /dev/null +++ b/bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/extensions/BahmniExtensionsTest.java @@ -0,0 +1,86 @@ +package org.bahmni.module.bahmnicore.extensions; + +import org.apache.commons.io.FileUtils; +import org.bahmni.module.bahmnicore.dao.ApplicationDataDirectory; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; +import org.springframework.test.util.ReflectionTestUtils; + +import java.io.File; + +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.mockito.Mockito.when; + +public class BahmniExtensionsTest { + + @Rule + public TemporaryFolder temporaryFolder = new TemporaryFolder(); + + @Mock + private ApplicationDataDirectory applicationDataDirectory; + + private BahmniExtensions bahmniExtensions; + private File extensionDir; + + @Before + public void setUp() throws Exception { + MockitoAnnotations.initMocks(this); + bahmniExtensions = new BahmniExtensions(); + ReflectionTestUtils.setField(bahmniExtensions, "applicationDataDirectory", applicationDataDirectory); + + extensionDir = temporaryFolder.newFolder("openmrs", "flowsheetExtension"); + when(applicationDataDirectory.getFileFromConfig("openmrs" + File.separator + "flowsheetExtension")) + .thenReturn(extensionDir); + } + + @Test + public void shouldLoadExtensionThatIsInsideTheConfiguredDirectory() throws Exception { + File groovyFile = new File(extensionDir, "MyExtension.groovy"); + FileUtils.writeStringToFile(groovyFile, "class MyExtension { }"); + when(applicationDataDirectory.getFileFromConfig( + "openmrs" + File.separator + "flowsheetExtension" + File.separator + "MyExtension.groovy")) + .thenReturn(groovyFile); + + Object extension = bahmniExtensions.getExtension("flowsheetExtension", "MyExtension.groovy"); + + assertNotNull(extension); + } + + @Test + public void shouldRejectPathTraversalAttemptOutsideConfiguredDirectory() throws Exception { + File outsideMarker = new File(temporaryFolder.getRoot(), "Pwn.groovy"); + FileUtils.writeStringToFile(outsideMarker, + "class Pwn { public Pwn() { throw new RuntimeException(\"pwned\") } }"); + String traversalFileName = ".." + File.separator + ".." + File.separator + "Pwn.groovy"; + when(applicationDataDirectory.getFileFromConfig( + "openmrs" + File.separator + "flowsheetExtension" + File.separator + traversalFileName)) + .thenReturn(new File(extensionDir, traversalFileName)); + + Object extension = bahmniExtensions.getExtension("flowsheetExtension", traversalFileName); + + assertNull(extension); + } + + @Test + public void shouldReturnNullWithoutThrowingWhenFileDoesNotExist() throws Exception { + when(applicationDataDirectory.getFileFromConfig( + "openmrs" + File.separator + "flowsheetExtension" + File.separator + "DoesNotExist.groovy")) + .thenReturn(new File(extensionDir, "DoesNotExist.groovy")); + + Object extension = bahmniExtensions.getExtension("flowsheetExtension", "DoesNotExist.groovy"); + + assertNull(extension); + } + + @Test + public void shouldReturnNullWhenFileNameIsBlank() { + assertNull(bahmniExtensions.getExtension("flowsheetExtension", null)); + assertNull(bahmniExtensions.getExtension("flowsheetExtension", "")); + assertNull(bahmniExtensions.getExtension("flowsheetExtension", " ")); + } +} From 9a8a3acbbcc5a98fe443745e7a5454c7854e1e61 Mon Sep 17 00:00:00 2001 From: Lingeswaran Subramaniyam Date: Thu, 13 Aug 2026 18:02:29 +0530 Subject: [PATCH 2/9] BAH-4971 SQL injection via concatenated into a native REGEXP literal --- .../bahmnicore/dao/impl/ObsDaoImpl.java | 13 +++--- .../bahmnicore/dao/impl/ObsDaoImplIT.java | 43 +++++++++++++++++++ .../formBuilderFormNamesTestData.xml | 14 ++++++ 3 files changed, 64 insertions(+), 6 deletions(-) create mode 100644 bahmnicore-api/src/test/resources/formBuilderFormNamesTestData.xml diff --git a/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/dao/impl/ObsDaoImpl.java b/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/dao/impl/ObsDaoImpl.java index 15a276bee0..d5ce0840da 100644 --- a/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/dao/impl/ObsDaoImpl.java +++ b/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/dao/impl/ObsDaoImpl.java @@ -301,7 +301,7 @@ public List getObsForFormBuilderForms(String patientUuid, List form "JOIN encounter ON encounter.encounter_id = obs.encounter_id AND encounter.voided = 0 " + encounterFilter + "JOIN visit ON visit.visit_id = encounter.visit_id AND visit.visit_id IN :visitIds "); - queryString.append(String.format("where obs.form_namespace_and_path REGEXP '%s' ", commaSeparatedFormNamesPattern(formNames))); + queryString.append("where obs.form_namespace_and_path REGEXP :formNamesPattern "); if (startDate != null) queryString.append("and obs.obs_datetime >= :startDate "); if (startDate != null && endDate != null) queryString.append("and obs.obs_datetime <= :endDate "); queryString.append("order by obs_datetime asc "); @@ -309,6 +309,7 @@ public List getObsForFormBuilderForms(String patientUuid, List form .createSQLQuery(queryString.toString()).addEntity(Obs.class); queryToGetObs.setParameter("patientUuid", patientUuid); queryToGetObs.setParameterList("visitIds", listOfVisitIds); + queryToGetObs.setParameter("formNamesPattern", commaSeparatedFormNamesPattern(formNames)); if (nonNull(startDate)) queryToGetObs.setParameter("startDate", new SimpleDateFormat("yyyy-MM-dd HH:mm:ss").format(startDate)); if (nonNull(endDate)) @@ -319,7 +320,7 @@ public List getObsForFormBuilderForms(String patientUuid, List form private String commaSeparatedFormNamesPattern(List formNames) { ArrayList formPatterns = new ArrayList<>(); - formNames.forEach(form -> formPatterns.add("\\\\^" + form + "\\\\.")); + formNames.forEach(form -> formPatterns.add("\\^" + form + "\\.")); return StringUtils.join(formPatterns, OR); } @@ -362,10 +363,10 @@ public List getObsByPatientProgramUuidAndConceptNames(String patientProgram "AND cn.concept_name_type='FULLY_SPECIFIED' " + "AND cn.name IN (:conceptNames) " + "AND cn.locale = :locale"); - if(null != startDate) { + if (null != startDate) { queryString.append(" AND o.obs_datetime >= STR_TO_DATE(:startDate, '%Y-%m-%d')"); } - if(null != endDate) { + if (null != endDate) { queryString.append(" AND o.obs_datetime <= STR_TO_DATE(:endDate, '%Y-%m-%d')"); } if (sortOrder == OrderBy.ASC) { @@ -380,10 +381,10 @@ public List getObsByPatientProgramUuidAndConceptNames(String patientProgram queryToGetObs.setParameterList("conceptNames", conceptNames); queryToGetObs.setString("patientProgramUuid", patientProgramUuid); queryToGetObs.setString("locale", Context.getLocale().getLanguage()); - if(null != startDate) { + if (null != startDate) { queryToGetObs.setString("startDate", dateFormat.format(startDate)); } - if(null != endDate) { + if (null != endDate) { queryToGetObs.setString("endDate", dateFormat.format(endDate)); } diff --git a/bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/dao/impl/ObsDaoImplIT.java b/bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/dao/impl/ObsDaoImplIT.java index e4edc9d6c6..d29365054c 100644 --- a/bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/dao/impl/ObsDaoImplIT.java +++ b/bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/dao/impl/ObsDaoImplIT.java @@ -36,6 +36,7 @@ public class ObsDaoImplIT extends BaseIntegrationTest { public void setUp() throws Exception { executeDataSet("obsTestData.xml"); executeDataSet("patientProgramTestData.xml"); + executeDataSet("formBuilderFormNamesTestData.xml"); conceptToObsMap.put(9012, 5); conceptToObsMap.put(9011, 4); @@ -178,4 +179,46 @@ public void shouldRetrieveObsFromPatientProgramIdAndConceptNamesInAscendingOrder assertEquals("2015-08-18 15:09:05.0", observations.get(0).getObsDatetime().toString()); assertEquals("2016-08-18 15:09:05.0", observations.get(1).getObsDatetime().toString()); } + + @Test + public void shouldReturnObsMatchingBenignFormNamesFilter() throws Exception { + String patientUuid = "86526ed5-3c11-11de-a0ba-001e378eb67a"; + List listOfVisitIds = Collections.singletonList(902); + + List observations = obsDao.getObsForFormBuilderForms( + patientUuid, Arrays.asList("Vitals"), listOfVisitIds, + Collections.EMPTY_LIST, null, null); + + assertEquals(1, observations.size()); + assertEquals("Bahmni^Vitals.1/5-0", observations.get(0).getFormNamespaceAndPath()); + + List bloodSampleObs = obsDao.getObsForFormBuilderForms( + patientUuid, Arrays.asList("BloodSample"), listOfVisitIds, + Collections.EMPTY_LIST, null, null); + assertEquals(1, bloodSampleObs.size()); + assertEquals("Bahmni^BloodSample.2/1-0", bloodSampleObs.get(0).getFormNamespaceAndPath()); + + List bothForms = obsDao.getObsForFormBuilderForms( + patientUuid, Arrays.asList("Vitals", "BloodSample"), listOfVisitIds, + Collections.EMPTY_LIST, null, null); + assertEquals(2, bothForms.size()); + } + + @Test + public void shouldTreatMaliciousFormNamesPayloadAsInertDataNotSql() throws Exception { + String patientUuid = "86526ed5-3c11-11de-a0ba-001e378eb67a"; + List listOfVisitIds = Collections.singletonList(902); + + String maliciousPayload = "x' OR '1'='1' -- "; + List observations = obsDao.getObsForFormBuilderForms( + patientUuid, Arrays.asList(maliciousPayload), listOfVisitIds, + Collections.EMPTY_LIST, null, null); + assertEquals(0, observations.size()); + + String unionPayload = "x' UNION SELECT username, password, salt FROM users -- "; + List unionAttempt = obsDao.getObsForFormBuilderForms( + patientUuid, Arrays.asList(unionPayload), listOfVisitIds, + Collections.EMPTY_LIST, null, null); + assertEquals(0, unionAttempt.size()); + } } \ No newline at end of file diff --git a/bahmnicore-api/src/test/resources/formBuilderFormNamesTestData.xml b/bahmnicore-api/src/test/resources/formBuilderFormNamesTestData.xml new file mode 100644 index 0000000000..996cf2e4a9 --- /dev/null +++ b/bahmnicore-api/src/test/resources/formBuilderFormNamesTestData.xml @@ -0,0 +1,14 @@ + + + + + + + + From 4f2ad3fc9337cbd10e5c4b73f8d7be76fabc26b4 Mon Sep 17 00:00:00 2001 From: Lingeswaran Subramaniyam Date: Fri, 14 Aug 2026 15:49:30 +0530 Subject: [PATCH 3/9] F03 Path traversal to arbitrary Groovy execution via the import patientMatchingAlgorithm parameter --- .../csv/service/PatientMatchService.java | 12 ++- .../csv/service/PatientMatchServiceTest.java | 80 +++++++++++++++++++ 2 files changed, 90 insertions(+), 2 deletions(-) create mode 100644 admin/src/test/java/org/bahmni/module/admin/csv/service/PatientMatchServiceTest.java diff --git a/admin/src/main/java/org/bahmni/module/admin/csv/service/PatientMatchService.java b/admin/src/main/java/org/bahmni/module/admin/csv/service/PatientMatchService.java index c10a94e696..fb456d1bfc 100644 --- a/admin/src/main/java/org/bahmni/module/admin/csv/service/PatientMatchService.java +++ b/admin/src/main/java/org/bahmni/module/admin/csv/service/PatientMatchService.java @@ -16,6 +16,8 @@ import java.io.File; import java.io.IOException; +import java.nio.file.Path; +import java.nio.file.Paths; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -25,7 +27,7 @@ public class PatientMatchService { @Autowired private BahmniPatientService patientService; - private static final String PATIENT_MATCHING_ALGORITHM_DIRECTORY = "/patientMatchingAlgorithm/"; + private static final String PATIENT_MATCHING_ALGORITHM_DIRECTORY_NAME = "patientMatchingAlgorithm"; private static final Logger log = LogManager.getLogger(PatientMatchService.class); // Mujir - an implementation could use multiple patient matching algorithms @@ -56,7 +58,13 @@ private PatientMatchingAlgorithm getPatientMatchingAlgorithm(String matchingAlgo } private String getAlgorithmClassPath(String matchingAlgorithmClassName) { - return OpenmrsUtil.getApplicationDataDirectory() + PATIENT_MATCHING_ALGORITHM_DIRECTORY + matchingAlgorithmClassName; + Path baseDir = Paths.get(OpenmrsUtil.getApplicationDataDirectory(), PATIENT_MATCHING_ALGORITHM_DIRECTORY_NAME).normalize(); + Path resolvedPath = baseDir.resolve(matchingAlgorithmClassName).normalize(); + if (!resolvedPath.startsWith(baseDir)) { + log.error("Rejected unsafe patientMatchingAlgorithm value: " + matchingAlgorithmClassName); + throw new IllegalArgumentException("Invalid patientMatchingAlgorithm: " + matchingAlgorithmClassName); + } + return resolvedPath.toString(); } } diff --git a/admin/src/test/java/org/bahmni/module/admin/csv/service/PatientMatchServiceTest.java b/admin/src/test/java/org/bahmni/module/admin/csv/service/PatientMatchServiceTest.java new file mode 100644 index 0000000000..cb8ec71f67 --- /dev/null +++ b/admin/src/test/java/org/bahmni/module/admin/csv/service/PatientMatchServiceTest.java @@ -0,0 +1,80 @@ +package org.bahmni.module.admin.csv.service; + +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; +import org.junit.rules.TemporaryFolder; + +import java.io.File; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; + +import static org.junit.Assert.assertEquals; + +public class PatientMatchServiceTest { + + private static final String APP_DATA_DIR_PROPERTY = "OPENMRS_APPLICATION_DATA_DIRECTORY"; + private static final String ALGORITHM_DIR_NAME = "patientMatchingAlgorithm"; + + @Rule + public TemporaryFolder appDataDir = new TemporaryFolder(); + + @Rule + public ExpectedException expectedEx = ExpectedException.none(); + + private String originalAppDataDirProperty; + private Method getAlgorithmClassPath; + + @Before + public void setUp() throws Exception { + originalAppDataDirProperty = System.getProperty(APP_DATA_DIR_PROPERTY); + System.setProperty(APP_DATA_DIR_PROPERTY, appDataDir.getRoot().getAbsolutePath()); + appDataDir.newFolder(ALGORITHM_DIR_NAME); + + getAlgorithmClassPath = PatientMatchService.class.getDeclaredMethod("getAlgorithmClassPath", String.class); + getAlgorithmClassPath.setAccessible(true); + } + + @After + public void tearDown() { + if (originalAppDataDirProperty == null) { + System.clearProperty(APP_DATA_DIR_PROPERTY); + } else { + System.setProperty(APP_DATA_DIR_PROPERTY, originalAppDataDirProperty); + } + } + + @Test + public void shouldResolveASimpleNameInsideTheAlgorithmDirectory() throws Exception { + String resolvedPath = invoke("SimpleAlgorithm.groovy"); + String expectedPath = new File(new File(appDataDir.getRoot(), ALGORITHM_DIR_NAME), "SimpleAlgorithm.groovy").getPath(); + assertEquals(expectedPath, resolvedPath); + } + + @Test + public void shouldRejectRelativePathTraversalOutsideTheAlgorithmDirectory() throws Exception { + expectedEx.expect(IllegalArgumentException.class); + expectedEx.expectMessage("Invalid patientMatchingAlgorithm"); + invoke("../../../../tmp/evil/Pwn.groovy"); + } + + @Test + public void shouldRejectAbsolutePathPayload() throws Exception { + expectedEx.expect(IllegalArgumentException.class); + expectedEx.expectMessage("Invalid patientMatchingAlgorithm"); + invoke("/etc/passwd"); + } + + private String invoke(String matchingAlgorithmClassName) throws Exception { + try { + return (String) getAlgorithmClassPath.invoke(new PatientMatchService(), matchingAlgorithmClassName); + } catch (InvocationTargetException e) { + if (e.getCause() instanceof RuntimeException) { + throw (RuntimeException) e.getCause(); + } + throw e; + } + } +} From 29337ae13bac156d662b8ed7eac2ede9f0255efe Mon Sep 17 00:00:00 2001 From: Lingeswaran Subramaniyam Date: Mon, 17 Aug 2026 11:54:36 +0530 Subject: [PATCH 4/9] F04 Path-traversal file read and IDOR on the patient image endpoint --- .../impl/PatientDocumentServiceImpl.java | 17 +++++++-- .../impl/PatientDocumentServiceImplTest.java | 20 +++++++++++ .../BahmniPatientImageController.java | 20 ++++++----- .../BahmniPatientImageControllerTest.java | 36 ++++++++++++++++++- 4 files changed, 81 insertions(+), 12 deletions(-) diff --git a/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImpl.java b/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImpl.java index 211f812444..76a1f146f5 100644 --- a/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImpl.java +++ b/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImpl.java @@ -253,15 +253,26 @@ private void deleteFile(File file) { } private File getPatientImageFile(String patientUuid) { - File file = new File(String.format("%s/%s.%s", BahmniCoreProperties.getProperty("bahmnicore.images.directory"), patientUuid, patientImagesFormat)); - if (file.exists() && file.isFile()) { + File file = resolveContainedImageFile(patientUuid); + if (file != null && file.exists() && file.isFile()) { return file; } return new File(BahmniCoreProperties.getProperty("bahmnicore.images.directory.defaultImage")); } private File getPatientImageFileWithoutDefault(String patientUuid) { - return new File(String.format("%s/%s.%s", BahmniCoreProperties.getProperty("bahmnicore.images.directory"), patientUuid, patientImagesFormat)); + File file = resolveContainedImageFile(patientUuid); + return file != null ? file : new File(""); + } + + private File resolveContainedImageFile(String patientUuid) { + String imagesDir = BahmniCoreProperties.getProperty("bahmnicore.images.directory"); + Path base = Paths.get(imagesDir).normalize(); + Path candidate = Paths.get(imagesDir, String.format("%s.%s", patientUuid, patientImagesFormat)).normalize(); + if (!candidate.startsWith(base)) { + return null; + } + return candidate.toFile(); } private ResponseEntity readImage(File file) { diff --git a/bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImplTest.java b/bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImplTest.java index c1611333bb..91192a1de9 100644 --- a/bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImplTest.java +++ b/bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImplTest.java @@ -103,6 +103,26 @@ public void shouldGetImageNotFoundForIfNoImageCapturedForPatientAndNoDefaultImag assertEquals(404, responseEntity.getStatusCode().value()); } + @Test + public void shouldNotReadFileOutsideImagesDirectoryViaPathTraversal() throws Exception { + File imagesDir = temporaryFolder.newFolder("images"); + File outsideDir = temporaryFolder.newFolder("outside"); + File secretFile = new File(outsideDir, "secret.jpeg"); + byte[] secretBytes = "TOP-SECRET-CONTENT-NOT-A-PATIENT-PHOTO".getBytes(); + FileUtils.writeByteArrayToFile(secretFile, secretBytes); + + PowerMockito.mockStatic(BahmniCoreProperties.class); + when(BahmniCoreProperties.getProperty("bahmnicore.images.directory")).thenReturn(imagesDir.getAbsolutePath()); + when(BahmniCoreProperties.getProperty("bahmnicore.images.directory.defaultImage")).thenReturn(""); + + patientDocumentService = new PatientDocumentServiceImpl(); + + String traversalPayload = "../outside/secret"; + ResponseEntity responseEntity = patientDocumentService.retriveImageWithoutDefault(traversalPayload); + + assertEquals(404, responseEntity.getStatusCode().value()); + } + @Test public void shouldThrowExceptionWhenVideoFormatIsNotSupported() throws Exception { PowerMockito.mockStatic(BahmniCoreProperties.class); diff --git a/bahmnicore-omod/src/main/java/org/bahmni/module/bahmnicore/web/v1_0/controller/BahmniPatientImageController.java b/bahmnicore-omod/src/main/java/org/bahmni/module/bahmnicore/web/v1_0/controller/BahmniPatientImageController.java index bbbf09ccd3..b3bcb58693 100644 --- a/bahmnicore-omod/src/main/java/org/bahmni/module/bahmnicore/web/v1_0/controller/BahmniPatientImageController.java +++ b/bahmnicore-omod/src/main/java/org/bahmni/module/bahmnicore/web/v1_0/controller/BahmniPatientImageController.java @@ -1,6 +1,8 @@ package org.bahmni.module.bahmnicore.web.v1_0.controller; +import org.bahmni.module.bahmnicore.security.PrivilegeConstants; import org.bahmni.module.bahmnicore.service.PatientDocumentService; +import org.openmrs.Patient; import org.openmrs.api.context.Context; import org.openmrs.api.context.UserContext; import org.openmrs.module.webservices.rest.web.RestConstants; @@ -15,11 +17,6 @@ import org.springframework.web.bind.annotation.ResponseBody; -/** - * @deprecated This API is deprecated because it returns a default image when patient image doesn't exist. - * If you don't want a default image, use V2: /openmrs/ws/rest/v2/patientImage - */ -@Deprecated @Controller @RequestMapping(value = "/rest/" + RestConstants.VERSION_1 + "/patientImage") public class BahmniPatientImageController extends BaseRestController { @@ -35,10 +32,17 @@ public BahmniPatientImageController(PatientDocumentService patientDocumentServic @ResponseBody public ResponseEntity getImage(@RequestParam(value = "patientUuid", required = true) String patientUuid) { UserContext userContext = Context.getUserContext(); - if (userContext.isAuthenticated()) { - return patientDocumentService.retriveImage(patientUuid); + if (!userContext.isAuthenticated()) { + return new ResponseEntity(new Object(), HttpStatus.UNAUTHORIZED); } - return new ResponseEntity(new Object(), HttpStatus.UNAUTHORIZED); + if (!userContext.hasPrivilege(PrivilegeConstants.GET_PATIENT_PHOTO)) { + return new ResponseEntity(new Object(), HttpStatus.FORBIDDEN); + } + Patient patient = Context.getPatientService().getPatientByUuid(patientUuid); + if (patient == null) { + return new ResponseEntity(new Object(), HttpStatus.NOT_FOUND); + } + return patientDocumentService.retriveImage(patientUuid); } } diff --git a/bahmnicore-omod/src/test/java/org/bahmni/module/bahmnicore/web/v1_0/controller/BahmniPatientImageControllerTest.java b/bahmnicore-omod/src/test/java/org/bahmni/module/bahmnicore/web/v1_0/controller/BahmniPatientImageControllerTest.java index a77c58d0a8..056b3df71c 100644 --- a/bahmnicore-omod/src/test/java/org/bahmni/module/bahmnicore/web/v1_0/controller/BahmniPatientImageControllerTest.java +++ b/bahmnicore-omod/src/test/java/org/bahmni/module/bahmnicore/web/v1_0/controller/BahmniPatientImageControllerTest.java @@ -1,11 +1,14 @@ package org.bahmni.module.bahmnicore.web.v1_0.controller; +import org.bahmni.module.bahmnicore.security.PrivilegeConstants; import org.bahmni.module.bahmnicore.service.PatientDocumentService; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.Mockito; +import org.openmrs.Patient; +import org.openmrs.api.PatientService; import org.openmrs.api.context.Context; import org.openmrs.api.context.UserContext; import org.powermock.api.mockito.PowerMockito; @@ -35,18 +38,24 @@ public class BahmniPatientImageControllerTest { @Mock private UserContext userContext; + @Mock + private PatientService patientService; + @Before public void setUp() throws IOException { PowerMockito.mockStatic(Context.class); PowerMockito.when(Context.getUserContext()).thenReturn(userContext); + PowerMockito.when(Context.getPatientService()).thenReturn(patientService); bahmniPatientImageController = new BahmniPatientImageController(patientDocumentService); } @Test public void shouldRespondWithFileNotFoundStatusCodeIfTheImageIsNotFound() throws Exception { + String patientUuid = "patientUuid"; Mockito.when(userContext.isAuthenticated()).thenReturn(true); + Mockito.when(userContext.hasPrivilege(PrivilegeConstants.GET_PATIENT_PHOTO)).thenReturn(true); + Mockito.when(patientService.getPatientByUuid(patientUuid)).thenReturn(new Patient()); when(patientDocumentService.retriveImage(anyString())).thenReturn(new ResponseEntity(new Object(), HttpStatus.OK)); - String patientUuid = "patientUuid"; ResponseEntity responseEntity = bahmniPatientImageController.getImage(patientUuid); @@ -65,4 +74,29 @@ public void shouldRespondWithNotAuthorizeStatusCodeIfTheImageIsNotFound() throws verify(patientDocumentService, never()).retriveImage(patientUuid); assertEquals(HttpStatus.UNAUTHORIZED, responseEntity.getStatusCode()); } + + @Test + public void shouldRespondWithForbiddenWhenUserLacksGetPatientPhotoPrivilege() throws Exception { + String patientUuid = "patientUuid"; + Mockito.when(userContext.isAuthenticated()).thenReturn(true); + Mockito.when(userContext.hasPrivilege(PrivilegeConstants.GET_PATIENT_PHOTO)).thenReturn(false); + + ResponseEntity responseEntity = bahmniPatientImageController.getImage(patientUuid); + + verify(patientDocumentService, never()).retriveImage(patientUuid); + assertEquals(HttpStatus.FORBIDDEN, responseEntity.getStatusCode()); + } + + @Test + public void shouldRespondWithNotFoundWhenPatientUuidDoesNotResolveToAPatient() throws Exception { + String patientUuid = "../outside/secret"; + Mockito.when(userContext.isAuthenticated()).thenReturn(true); + Mockito.when(userContext.hasPrivilege(PrivilegeConstants.GET_PATIENT_PHOTO)).thenReturn(true); + Mockito.when(patientService.getPatientByUuid(patientUuid)).thenReturn(null); + + ResponseEntity responseEntity = bahmniPatientImageController.getImage(patientUuid); + + verify(patientDocumentService, never()).retriveImage(patientUuid); + assertEquals(HttpStatus.NOT_FOUND, responseEntity.getStatusCode()); + } } \ No newline at end of file From 0eeb5a6d06c9308b31e3588bb6a4689b29dd4946 Mon Sep 17 00:00:00 2001 From: Lingeswaran Subramaniyam Date: Mon, 17 Aug 2026 15:06:11 +0530 Subject: [PATCH 5/9] F07 Groovy class loaded from an encounter-modifier concept-set name --- .../BahmniEncounterModifierServiceImpl.java | 13 +- ...ahmniEncounterModifierServiceImplTest.java | 112 ++++++++++++++++++ 2 files changed, 123 insertions(+), 2 deletions(-) create mode 100644 bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/service/impl/BahmniEncounterModifierServiceImplTest.java diff --git a/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/BahmniEncounterModifierServiceImpl.java b/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/BahmniEncounterModifierServiceImpl.java index 30ccaa0cb1..693048e9eb 100644 --- a/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/BahmniEncounterModifierServiceImpl.java +++ b/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/BahmniEncounterModifierServiceImpl.java @@ -14,6 +14,7 @@ import java.io.File; import java.io.IOException; +import java.nio.file.Path; import java.util.HashMap; import java.util.Map; @@ -57,8 +58,16 @@ private EncounterModifier loadGroovyClass(String encounterModifierClassName) thr return (EncounterModifier) clazz.newInstance(); } - private String getEncounterModifierClassPath(String encounterModifierClassName) { - return OpenmrsUtil.getApplicationDataDirectory() + ENCOUNTER_MODIFIER_ALGORITHM_DIRECTORY + encounterModifierClassName ; + private String getEncounterModifierClassPath(String encounterModifierClassName) throws IOException { + Path encounterModifierDirectory = new File(OpenmrsUtil.getApplicationDataDirectory() + ENCOUNTER_MODIFIER_ALGORITHM_DIRECTORY) + .toPath().normalize(); + Path encounterModifierClassFile = new File(OpenmrsUtil.getApplicationDataDirectory() + ENCOUNTER_MODIFIER_ALGORITHM_DIRECTORY + encounterModifierClassName) + .toPath().normalize(); + + if (!encounterModifierClassFile.startsWith(encounterModifierDirectory)) { + throw new IOException("Rejected encounter modifier class name resolving outside of the allowed directory: " + encounterModifierClassName); + } + return encounterModifierClassFile.toString(); } private boolean isGroovyCachingAllowed(){ diff --git a/bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/service/impl/BahmniEncounterModifierServiceImplTest.java b/bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/service/impl/BahmniEncounterModifierServiceImplTest.java new file mode 100644 index 0000000000..588245e405 --- /dev/null +++ b/bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/service/impl/BahmniEncounterModifierServiceImplTest.java @@ -0,0 +1,112 @@ +package org.bahmni.module.bahmnicore.service.impl; + +import org.apache.commons.io.FileUtils; +import org.bahmni.module.bahmnicore.contract.encounter.data.ConceptData; +import org.bahmni.module.bahmnicore.contract.encounter.data.EncounterModifierData; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.openmrs.api.AdministrationService; +import org.openmrs.api.context.Context; +import org.openmrs.util.OpenmrsUtil; +import org.powermock.api.mockito.PowerMockito; +import org.powermock.core.classloader.annotations.PrepareForTest; +import org.powermock.modules.junit4.PowerMockRunner; + +import java.io.File; +import java.io.IOException; + +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.fail; +import static org.mockito.MockitoAnnotations.initMocks; +import static org.powermock.api.mockito.PowerMockito.when; + +@RunWith(PowerMockRunner.class) +@PrepareForTest({Context.class, OpenmrsUtil.class}) +public class BahmniEncounterModifierServiceImplTest { + + @Rule + public TemporaryFolder temporaryFolder = new TemporaryFolder(); + + @Mock + private AdministrationService administrationService; + + private File appDataDir; + private BahmniEncounterModifierServiceImpl service; + + private void setUp(String allowCaching) throws IOException { + initMocks(this); + + appDataDir = temporaryFolder.newFolder("openmrsAppData"); + new File(appDataDir, "encounterModifier").mkdirs(); + + PowerMockito.mockStatic(OpenmrsUtil.class); + when(OpenmrsUtil.getApplicationDataDirectory()).thenReturn(appDataDir.getAbsolutePath()); + + PowerMockito.mockStatic(Context.class); + when(Context.getAdministrationService()).thenReturn(administrationService); + when(administrationService.getGlobalProperty("encounterModifier.groovy.allowCaching")).thenReturn(allowCaching); + + service = new BahmniEncounterModifierServiceImpl(); + } + + @Test + public void shouldLoadEncounterModifierThatIsInsideTheConfiguredDirectory() throws Throwable { + setUp("false"); + + File legit = new File(appDataDir, "encounterModifier/DiabetesPanel.groovy"); + FileUtils.writeStringToFile(legit, + "import org.bahmni.module.bahmnicore.encounterModifier.EncounterModifier\n" + + "import org.bahmni.module.bahmnicore.contract.encounter.data.EncounterModifierData\n" + + "class DiabetesPanel extends EncounterModifier {\n" + + " EncounterModifierData run(EncounterModifierData d) { return d }\n" + + "}\n"); + + EncounterModifierData request = requestFor("Diabetes Panel"); + + EncounterModifierData result = service.getModifiedEncounter(request); + + assertNotEquals(null, result); + } + + @Test + public void shouldRejectConceptSetNameThatTraversesOutsideTheConfiguredDirectory() throws Throwable { + setUp("false"); + + File outsideDir = temporaryFolder.newFolder("someOtherDirectory"); + File canary = new File(outsideDir, "Pwn.groovy"); + FileUtils.writeStringToFile(canary, + "import org.bahmni.module.bahmnicore.encounterModifier.EncounterModifier\n" + + "import org.bahmni.module.bahmnicore.contract.encounter.data.EncounterModifierData\n" + + "class Pwn extends EncounterModifier {\n" + + " static {\n" + + " System.setProperty('poc.pwned', 'true')\n" + + " }\n" + + " EncounterModifierData run(EncounterModifierData d) { return d }\n" + + "}\n"); + System.clearProperty("poc.pwned"); + + EncounterModifierData request = requestFor("../../someOtherDirectory/Pwn"); + + try { + service.getModifiedEncounter(request); + fail("Expected an IOException rejecting the path traversal attempt"); + } catch (IOException e) { + // expected + } + + assertNull("Groovy file outside /encounterModifier/ must never be loaded/executed", System.getProperty("poc.pwned")); + } + + private EncounterModifierData requestFor(String conceptSetName) { + ConceptData conceptSetData = new ConceptData(); + conceptSetData.setName(conceptSetName); + + EncounterModifierData request = new EncounterModifierData(); + request.setConceptSetData(conceptSetData); + return request; + } +} From 61082ec9343368e3ea8e07becd6471828acc1a21 Mon Sep 17 00:00:00 2001 From: Lingeswaran Subramaniyam Date: Mon, 17 Aug 2026 15:27:12 +0530 Subject: [PATCH 6/9] F08 Missing authorization on the import-status endpoint --- .../controller/AdminImportController.java | 8 ++++-- .../controller/AdminImportControllerTest.java | 28 +++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/bahmnicore-omod/src/main/java/org/bahmni/module/bahmnicore/web/v1_0/controller/AdminImportController.java b/bahmnicore-omod/src/main/java/org/bahmni/module/bahmnicore/web/v1_0/controller/AdminImportController.java index 06604ac168..a58f3757c3 100644 --- a/bahmnicore-omod/src/main/java/org/bahmni/module/bahmnicore/web/v1_0/controller/AdminImportController.java +++ b/bahmnicore-omod/src/main/java/org/bahmni/module/bahmnicore/web/v1_0/controller/AdminImportController.java @@ -341,10 +341,14 @@ public ResponseEntity uploadRelationship(@RequestParam(value = "fi @RequestMapping(value = baseUrl + "/status", method = RequestMethod.GET) @ResponseBody - public List status(@RequestParam(required = false) Integer numberOfDays) throws SQLException { + public ResponseEntity status(@RequestParam(required = false) Integer numberOfDays) throws SQLException { + if (!hasRequiredPrivilege()) { + return insufficientUserPrivilegeResponse(); + } numberOfDays = numberOfDays == null ? DEFAULT_NUMBER_OF_DAYS : numberOfDays; ImportStatusDao importStatusDao = new ImportStatusDao(new CurrentThreadConnectionProvider()); - return importStatusDao.getImportStatusFromDate(DateUtils.addDays(new Date(), (numberOfDays * -1))); + List result = importStatusDao.getImportStatusFromDate(DateUtils.addDays(new Date(), (numberOfDays * -1))); + return new ResponseEntity<>((Serializable) result, HttpStatus.OK); } private boolean importCsv(String filesDirectory, MultipartFile file, EntityPersister persister, diff --git a/bahmnicore-omod/src/test/java/org/bahmni/module/bahmnicore/web/v1_0/controller/AdminImportControllerTest.java b/bahmnicore-omod/src/test/java/org/bahmni/module/bahmnicore/web/v1_0/controller/AdminImportControllerTest.java index 47d95e85f7..d86e5b7bba 100644 --- a/bahmnicore-omod/src/test/java/org/bahmni/module/bahmnicore/web/v1_0/controller/AdminImportControllerTest.java +++ b/bahmnicore-omod/src/test/java/org/bahmni/module/bahmnicore/web/v1_0/controller/AdminImportControllerTest.java @@ -4,6 +4,7 @@ import org.bahmni.fileimport.FileImporter; import org.bahmni.module.admin.csv.persister.PatientPersister; import org.bahmni.module.bahmnicore.security.PrivilegeConstants; +import org.hibernate.HibernateException; import org.hibernate.SessionFactory; import org.junit.Before; import org.junit.Test; @@ -30,9 +31,12 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @RunWith(PowerMockRunner.class) @@ -129,4 +133,28 @@ public void shouldStoreUploadedFileWithRandomNameNotOriginalFilename() throws Ex assertTrue("a randomly named patient CSV should exist in the upload directory", uploadedFiles != null && uploadedFiles.length > 0); } + + @Test + public void shouldReturn403WhenFetchingStatusWithoutRequiredPrivilege() throws Exception { + when(mockUserContext.hasPrivilege(PrivilegeConstants.IMPORT_CSV_FILE_PRIVILEGE)).thenReturn(false); + + ResponseEntity response = controller.status(30); + + assertEquals(HttpStatus.FORBIDDEN, response.getStatusCode()); + verifyNoInteractions(sessionFactory); + } + + @Test + public void shouldReachDaoLayerWhenFetchingStatusWithRequiredPrivilege() throws Exception { + when(sessionFactory.getCurrentSession()).thenThrow(new HibernateException("DAO layer reached")); + + try { + controller.status(30); + fail("Expected the DAO layer to be reached and throw"); + } catch (HibernateException e) { + assertEquals("DAO layer reached", e.getMessage()); + } + + verify(mockUserContext).hasPrivilege(PrivilegeConstants.IMPORT_CSV_FILE_PRIVILEGE); + } } From 241dae01917654996ccec163c992d338fdfa2159 Mon Sep 17 00:00:00 2001 From: Lingeswaran Subramaniyam Date: Mon, 17 Aug 2026 16:36:30 +0530 Subject: [PATCH 7/9] F09 Do not start fix first try to reproduce then i will try by myself then may plan to fix. --- .../impl/PatientDocumentServiceImpl.java | 18 +++++++++++++- .../impl/PatientDocumentServiceImplTest.java | 24 +++++++++++++++---- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImpl.java b/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImpl.java index 76a1f146f5..b060d82097 100644 --- a/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImpl.java +++ b/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImpl.java @@ -78,6 +78,7 @@ public String saveDocument(Integer patientId, String encounterTypeName, String c String relativeFilePath = createFilePath(basePath, patientId, encounterTypeName, format, fileName); File outputFile = new File(String.format("%s/%s", basePath, relativeFilePath)); + validateOutputFileIsContained(basePath, outputFile); saveDocumentInFile(content, format, outputFile, fileType); return relativeFilePath; @@ -87,6 +88,14 @@ public String saveDocument(Integer patientId, String encounterTypeName, String c } } + private void validateOutputFileIsContained(String basePath, File outputFile) { + Path base = Paths.get(basePath).normalize(); + Path resolved = outputFile.toPath().normalize(); + if (!resolved.startsWith(base)) { + throw new BahmniCoreException("Invalid file path"); + } + } + private String getBasePathByEncounterType(String encounterTypeName) { String basePath; if(encounterTypeName.equalsIgnoreCase(LAB_RESULT_ENCOUNTER_TYPE)){ @@ -101,12 +110,19 @@ private String getBasePath() { return BahmniCoreProperties.getProperty("bahmnicore.documents.baseDirectory"); } + private String sanitizePathComponent(String value) { + if (value == null) return null; + return value.replaceAll("[/\\\\]", "").replace("..", ""); + } + private String createFileName(Integer patientId, String encounterTypeName, Object format, String originalFileName) { String uuid = UUID.randomUUID().toString(); + String safeEncounterTypeName = sanitizePathComponent(encounterTypeName); + String safeFormat = sanitizePathComponent(String.valueOf(format)); if (StringUtils.isNotBlank(originalFileName)) { originalFileName = "__" + originalFileName; } - return String.format("%s-%s-%s%s.%s", patientId, encounterTypeName, uuid, originalFileName, format); + return String.format("%s-%s-%s%s.%s", patientId, safeEncounterTypeName, uuid, originalFileName, safeFormat); } protected String createFilePath(String basePath, Integer patientId, String encounterTypeName, String format, String originalFileName) { diff --git a/bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImplTest.java b/bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImplTest.java index 91192a1de9..1db7c95e10 100644 --- a/bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImplTest.java +++ b/bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImplTest.java @@ -2,6 +2,7 @@ import org.apache.commons.io.FileUtils; import org.apache.xerces.impl.dv.util.Base64; +import org.bahmni.module.bahmnicore.BahmniCoreException; import org.bahmni.module.bahmnicore.bahmniexceptions.FileTypeNotSupportedException; import org.bahmni.module.bahmnicore.bahmniexceptions.VideoFormatNotSupportedException; import org.bahmni.module.bahmnicore.model.VideoFormats; @@ -126,7 +127,7 @@ public void shouldNotReadFileOutsideImagesDirectoryViaPathTraversal() throws Exc @Test public void shouldThrowExceptionWhenVideoFormatIsNotSupported() throws Exception { PowerMockito.mockStatic(BahmniCoreProperties.class); - when(BahmniCoreProperties.getProperty("bahmnicore.documents.baseDirectory")).thenReturn(""); + when(BahmniCoreProperties.getProperty("bahmnicore.documents.baseDirectory")).thenReturn(temporaryFolder.getRoot().getAbsolutePath()); PowerMockito.mockStatic(FileUtils.class); Patient patient = new Patient(); @@ -144,7 +145,7 @@ public void shouldThrowExceptionWhenVideoFormatIsNotSupported() throws Exception @Test public void shouldSavePDF() throws Exception { PowerMockito.mockStatic(BahmniCoreProperties.class); - when(BahmniCoreProperties.getProperty("bahmnicore.documents.baseDirectory")).thenReturn(""); + when(BahmniCoreProperties.getProperty("bahmnicore.documents.baseDirectory")).thenReturn(temporaryFolder.getRoot().getAbsolutePath()); PowerMockito.mockStatic(FileUtils.class); Patient patient = new Patient(); @@ -161,7 +162,7 @@ public void shouldSavePDF() throws Exception { public void shouldSaveImage() throws Exception { PowerMockito.mockStatic(BahmniCoreProperties.class); PowerMockito.mockStatic(ImageIO.class); - when(BahmniCoreProperties.getProperty("bahmnicore.documents.baseDirectory")).thenReturn(""); + when(BahmniCoreProperties.getProperty("bahmnicore.documents.baseDirectory")).thenReturn(temporaryFolder.getRoot().getAbsolutePath()); BufferedImage bufferedImage = new BufferedImage(1,2, 2); when(ImageIO.read(Matchers.any(ByteArrayInputStream.class))).thenReturn(bufferedImage); when(ImageIO.write(eq(bufferedImage),eq("jpg"), Matchers.any(File.class))).thenReturn(true); @@ -176,10 +177,23 @@ public void shouldSaveImage() throws Exception { assertTrue(url.matches(".*1-Consultation-.*.jpg")); } + @Test + public void shouldNotWriteDocumentOutsideBaseDirectoryViaPathTraversal() throws Exception { + PowerMockito.mockStatic(BahmniCoreProperties.class); + when(BahmniCoreProperties.getProperty("bahmnicore.documents.baseDirectory")) + .thenReturn(temporaryFolder.getRoot().getAbsolutePath()); + + patientDocumentService = new PatientDocumentServiceImpl(); + + expectedException.expect(BahmniCoreException.class); + patientDocumentService.saveDocument(1, "Consultation", "pdfContent", "pdf", "file", + "../../../../pwned_by_bahmni_poc"); + } + @Test public void shouldThrowExceptionWhenFileTypeIsNotSupported() throws Exception { PowerMockito.mockStatic(BahmniCoreProperties.class); - when(BahmniCoreProperties.getProperty("bahmnicore.documents.baseDirectory")).thenReturn(""); + when(BahmniCoreProperties.getProperty("bahmnicore.documents.baseDirectory")).thenReturn(temporaryFolder.getRoot().getAbsolutePath()); PowerMockito.mockStatic(FileUtils.class); Patient patient = new Patient(); @@ -196,7 +210,7 @@ public void shouldThrowExceptionWhenFileTypeIsNotSupported() throws Exception { @Test public void shouldThrowExceptionWhenImageTypeOtherThanPngJpegGif() throws Exception { PowerMockito.mockStatic(BahmniCoreProperties.class); - when(BahmniCoreProperties.getProperty("bahmnicore.documents.baseDirectory")).thenReturn(""); + when(BahmniCoreProperties.getProperty("bahmnicore.documents.baseDirectory")).thenReturn(temporaryFolder.getRoot().getAbsolutePath()); PowerMockito.mockStatic(FileUtils.class); PowerMockito.mockStatic(ImageIO.class); BufferedImage bufferedImage = new BufferedImage(1,2, 2); From bae85bd5b78f47ab31c16facb3f127cb90d7e07b Mon Sep 17 00:00:00 2001 From: Lingeswaran Subramaniyam Date: Wed, 19 Aug 2026 10:45:46 +0530 Subject: [PATCH 8/9] BAH-4971 Reverting patient image v2 endpoint which deleted --- .../web/v1_0/controller/BahmniPatientImageController.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/bahmnicore-omod/src/main/java/org/bahmni/module/bahmnicore/web/v1_0/controller/BahmniPatientImageController.java b/bahmnicore-omod/src/main/java/org/bahmni/module/bahmnicore/web/v1_0/controller/BahmniPatientImageController.java index b3bcb58693..eefc863774 100644 --- a/bahmnicore-omod/src/main/java/org/bahmni/module/bahmnicore/web/v1_0/controller/BahmniPatientImageController.java +++ b/bahmnicore-omod/src/main/java/org/bahmni/module/bahmnicore/web/v1_0/controller/BahmniPatientImageController.java @@ -17,6 +17,11 @@ import org.springframework.web.bind.annotation.ResponseBody; +/** + * @deprecated This API is deprecated because it returns a default image when patient image doesn't exist. + * If you don't want a default image, use V2: /openmrs/ws/rest/v2/patientImage + */ +@Deprecated @Controller @RequestMapping(value = "/rest/" + RestConstants.VERSION_1 + "/patientImage") public class BahmniPatientImageController extends BaseRestController { From dc728cc7f71c5c414768daf81cdcd68e380fdb2c Mon Sep 17 00:00:00 2001 From: Lingeswaran Subramaniyam Date: Wed, 26 Aug 2026 15:01:23 +0530 Subject: [PATCH 9/9] BAH-4991 Code review changes --- .../bahmni/module/bahmnicore/dao/impl/ObsDaoImpl.java | 6 +++++- .../service/impl/PatientDocumentServiceImpl.java | 9 --------- .../web/v1_0/controller/AdminImportController.java | 3 ++- 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/dao/impl/ObsDaoImpl.java b/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/dao/impl/ObsDaoImpl.java index d5ce0840da..89fec801c6 100644 --- a/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/dao/impl/ObsDaoImpl.java +++ b/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/dao/impl/ObsDaoImpl.java @@ -320,10 +320,14 @@ public List getObsForFormBuilderForms(String patientUuid, List form private String commaSeparatedFormNamesPattern(List formNames) { ArrayList formPatterns = new ArrayList<>(); - formNames.forEach(form -> formPatterns.add("\\^" + form + "\\.")); + formNames.forEach(form -> formPatterns.add("\\^" + escapeRegexMetacharacters(form) + "\\.")); return StringUtils.join(formPatterns, OR); } + private String escapeRegexMetacharacters(String value) { + return value.replaceAll("([\\\\^$.|?*+()\\[\\]{}])", "\\\\$1"); + } + private String commaSeparatedEncounterIds(Collection encounters) { ArrayList encounterIds = new ArrayList<>(); for (Encounter encounter : encounters) { diff --git a/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImpl.java b/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImpl.java index b060d82097..33231e9831 100644 --- a/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImpl.java +++ b/bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImpl.java @@ -78,7 +78,6 @@ public String saveDocument(Integer patientId, String encounterTypeName, String c String relativeFilePath = createFilePath(basePath, patientId, encounterTypeName, format, fileName); File outputFile = new File(String.format("%s/%s", basePath, relativeFilePath)); - validateOutputFileIsContained(basePath, outputFile); saveDocumentInFile(content, format, outputFile, fileType); return relativeFilePath; @@ -88,14 +87,6 @@ public String saveDocument(Integer patientId, String encounterTypeName, String c } } - private void validateOutputFileIsContained(String basePath, File outputFile) { - Path base = Paths.get(basePath).normalize(); - Path resolved = outputFile.toPath().normalize(); - if (!resolved.startsWith(base)) { - throw new BahmniCoreException("Invalid file path"); - } - } - private String getBasePathByEncounterType(String encounterTypeName) { String basePath; if(encounterTypeName.equalsIgnoreCase(LAB_RESULT_ENCOUNTER_TYPE)){ diff --git a/bahmnicore-omod/src/main/java/org/bahmni/module/bahmnicore/web/v1_0/controller/AdminImportController.java b/bahmnicore-omod/src/main/java/org/bahmni/module/bahmnicore/web/v1_0/controller/AdminImportController.java index a58f3757c3..6f54bf79b2 100644 --- a/bahmnicore-omod/src/main/java/org/bahmni/module/bahmnicore/web/v1_0/controller/AdminImportController.java +++ b/bahmnicore-omod/src/main/java/org/bahmni/module/bahmnicore/web/v1_0/controller/AdminImportController.java @@ -67,6 +67,7 @@ import java.sql.Connection; import java.sql.SQLException; import java.text.SimpleDateFormat; +import java.util.ArrayList; import java.util.Date; import java.util.List; import java.util.UUID; @@ -348,7 +349,7 @@ public ResponseEntity status(@RequestParam(required = false) Integ numberOfDays = numberOfDays == null ? DEFAULT_NUMBER_OF_DAYS : numberOfDays; ImportStatusDao importStatusDao = new ImportStatusDao(new CurrentThreadConnectionProvider()); List result = importStatusDao.getImportStatusFromDate(DateUtils.addDays(new Date(), (numberOfDays * -1))); - return new ResponseEntity<>((Serializable) result, HttpStatus.OK); + return new ResponseEntity<>(new ArrayList<>(result), HttpStatus.OK); } private boolean importCsv(String filesDirectory, MultipartFile file, EntityPersister persister,