(feat) O3-5186: Add security safeguards for logo path configuration

api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

@@ -16,6 +16,9 @@
 import java.io.File;
 import java.io.IOException;
 import java.io.OutputStream;
+import java.io.InputStream;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.Arrays;
 import java.util.Base64;
 import java.util.HashMap;
@@ -48,12 +51,16 @@
 import org.openmrs.module.reporting.report.renderer.RenderingException;
 import org.openmrs.module.reporting.report.renderer.ReportDesignRenderer;
 import org.openmrs.module.reporting.report.renderer.ReportRenderer;
+import org.openmrs.util.OpenmrsClassLoader;
 import org.openmrs.util.OpenmrsUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import org.springframework.stereotype.Component;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.commons.io.IOUtils;
 
 /**
  * ReportRenderer that renders to a default XML format
@@ -63,6 +70,10 @@
 @Localized("patientdocuments.patientIdStickerXmlReportRenderer")
 public class PatientIdStickerXmlReportRenderer extends ReportDesignRenderer {
 
+	private static final Logger log = LoggerFactory.getLogger(PatientIdStickerXmlReportRenderer.class);
+
+	private static final String DEFAULT_LOGO_CLASSPATH = "web/module/resources/openmrs_logo_white_large.png";
+
 	private MessageSourceService mss;
 
 	private InitializerService initializerService;
@@ -116,10 +127,6 @@ protected String getStringValue(DataSetRow row, DataSetColumn column) {
 
 	@Override
 	public void render(ReportData results, String argument, OutputStream out) throws IOException, RenderingException {
-		render(results, argument, out, null);
-	}
-
-	public void render(ReportData results, String argument, OutputStream out, byte[] defaultLogoBytes) throws IOException, RenderingException {
 		DocumentBuilderFactory docFactory = DocumentBuilderFactory.newInstance();
 		DocumentBuilder docBuilder;
 		try {
@@ -144,7 +151,7 @@ public void render(ReportData results, String argument, OutputStream out, byte[]
 		Element templatePIDElement = createStickerTemplate(doc);
 
 		// Handle header configuration
-		configureHeader(doc, templatePIDElement, defaultLogoBytes);
+		configureHeader(doc, templatePIDElement);
 
 		// Process data set fields
 		processDataSetFields(results, doc, templatePIDElement);
@@ -217,11 +224,11 @@ private Element createStickerTemplate(Document doc) {
 		return templatePIDElement;
 	}
 
-	private void configureHeader(Document doc, Element templatePIDElement, byte[] defaultLogoBytes) {
+	private void configureHeader(Document doc, Element templatePIDElement) {
 		Element header = doc.createElement("header");
-		// Handle logo if configured		
+		// Handle logo if configured
 		String logoUrlPath = getInitializerService().getValueFromKey("report.patientIdSticker.logourl");
-		configureLogo(doc, header, logoUrlPath, defaultLogoBytes);
+		configureLogo(doc, header, logoUrlPath);
 
 		boolean useHeader = Boolean.TRUE.equals(getInitializerService().getBooleanFromKey("report.patientIdSticker.header"));
 		if (useHeader) {
@@ -243,51 +250,131 @@ private void configureHeader(Document doc, Element templatePIDElement, byte[] de
 		templatePIDElement.appendChild(i18nStrings);
 	}
 
-    /**
-     * Configures the logo for the sticker document.
-     * 
-     * Logo resolution priority:
-     * 1. Custom logo from absolute filesystem path resolved under {@code OPENMRS_APPLICATION_DATA_DIRECTORY}
-     * 2. Default OpenMRS logo as base64 data URI
-     * 
-     * @param doc The XML document
-     * @param header The header element to append the logo to
-     * @param logoUrlPath User-configured logo path (can be null, absolute, or relative)
-     * @throws RenderingException if no valid logo can be found
-     */
-	private void configureLogo(Document doc, Element header, String logoUrlPath, byte[] defaultLogoBytes) {
-		String logoPath = "";
+	/**
+	 * Configures the logo for the sticker document.
+	 * 
+	 * Loads a custom logo from {@code logoUrlPath} (absolute or relative to the {@code OPENMRS_APPLICATION_DATA_DIRECTORY}.
+	 * If not found, falls back to the OpenMRS logo from the classpath.
+	 * 
+	 * @param doc The XML document
+	 * @param header The header element to append the logo to
+	 * @param logoUrlPath User-configured logo path (must be relative to app data dir)
+	 */
+	private void configureLogo(Document doc, Element header, String logoUrlPath) {
+		String logoContent = null;
 
-		try {
-			// 1. Try custom logo
-			if (isNotBlank(logoUrlPath)) {
-				File logoFile = new File(logoUrlPath);
-				if (!logoFile.isAbsolute()) {
-					File appDataDir = OpenmrsUtil.getDirectoryInApplicationDataDirectory("");
-					logoFile = new File(appDataDir, logoUrlPath);
-				}
-				if (logoFile.exists() && logoFile.canRead()) {
-					logoPath = logoFile.getAbsolutePath();
+		// 1. Try custom logo
+		if (isNotBlank(logoUrlPath)) {
+			File logoFile = resolveSecureLogoPath(logoUrlPath);
+			if (logoFile != null && logoFile.exists() && logoFile.canRead() && logoFile.isFile()) {
+				try {
+					byte[] customLogoBytes = OpenmrsUtil.getFileAsBytes(logoFile);
+					if (customLogoBytes != null && customLogoBytes.length > 0) {
+						String base64Image = Base64.getEncoder().encodeToString(customLogoBytes);
+						logoContent = "data:image/png;base64," + base64Image;
+					}
+				} catch (IOException e) {
+					log.error("Failed to load custom logo from file: {}", logoFile.getAbsolutePath(), e);
 				}
 			}
-
-			// 2. Fall back to default logo
-			if (isBlank(logoPath) && defaultLogoBytes != null && defaultLogoBytes.length > 0) {
+		}
+
+		if (isBlank(logoContent)) {
+			byte[] defaultLogoBytes = loadDefaultLogoFromClasspath();
+			if (defaultLogoBytes != null && defaultLogoBytes.length > 0) {
 				String base64Image = Base64.getEncoder().encodeToString(defaultLogoBytes);
-				logoPath = "data:image/png;base64," + base64Image;
+				logoContent = "data:image/png;base64," + base64Image;
 			}
-		} catch (Exception e) {
-			throw new RenderingException("Failed to configure logo", e);
 		}
-
-		// Create and append logo elements if valid
-		if (isNotBlank(logoPath)) {
+
+		if (isNotBlank(logoContent)) {
 			Element branding = doc.createElement("branding");
 			Element image = doc.createElement("logo");
-			image.setTextContent(logoPath);
+			image.setTextContent(logoContent);
 			branding.appendChild(image);
 			header.appendChild(branding);
 		}
+		else if (isNotBlank(logoUrlPath)) {
+			// If a path was provided but we could not resolve or fall back, surface an error
+			log.error("Failed to configure logo: unresolved path '{}' and no default provided", logoUrlPath);
+		}
+	}
+
+	private byte[] loadDefaultLogoFromClasspath() {
+		try (InputStream logoStream = OpenmrsClassLoader.getInstance().getResourceAsStream(DEFAULT_LOGO_CLASSPATH)) {
+			if (logoStream == null) {
+				log.warn("Default logo not found on classpath at: {}", DEFAULT_LOGO_CLASSPATH);
+				return null;
+			}
+			return IOUtils.toByteArray(logoStream);
+		}
+		catch (IOException e) {
+			log.error("Failed to load default logo from classpath at: {}", DEFAULT_LOGO_CLASSPATH, e);
+			return null;
+		}
+	}
+
+	/**
+	 * Ensure that the  supplied {@code logoUrlPath} refers to a file in the application data directory
+	 * 
+	 * @param logoUrlPath The user-provided logo path
+	 * @return A File object pointing to the logo if the path is valid, otherwise {@code null}
+	 */
+	protected File resolveSecureLogoPath(String logoUrlPath) {
+		if (isBlank(logoUrlPath)) {
+			return null;
+		}
+
+		final File appDataDir = OpenmrsUtil.getApplicationDataDirectoryAsFile();
+		if (appDataDir == null) {
+			log.error("Application data directory could not be found");
+			return null;
+		}
+
+		try {
+			final Path appDataPath = appDataDir.toPath().toRealPath();
+			final Path logoPath = Paths.get(logoUrlPath);
+
+			// For absolute paths, verify they're within app data directory
+			if (logoPath.isAbsolute()) {
+				final Path logoRealPath = logoPath.toRealPath();
+				if (!isPathWithinAppDataDirectory(logoRealPath, appDataPath)) {
+					log.error("Absolute path must be within application data directory: {}", logoUrlPath);
+					return null;
+				}
+				return logoRealPath.toFile();
+			}
+
+			// For relative paths, detect path traversal by comparing absolute and normalized paths
+			final Path logoAbsolutePath = logoPath.toAbsolutePath();
+			final Path logoNormalizedPath = logoAbsolutePath.normalize();
+
+			if (!logoAbsolutePath.equals(logoNormalizedPath)) {
+				log.error("Path traversal detected in logo path: {}", logoUrlPath);
+				return null;
+			}
+
+			// Resolve against application data directory and validate real location
+			final Path resolvedLogoPath = appDataPath.resolve(logoUrlPath).normalize();
+			final Path resolvedLogoRealPath = resolvedLogoPath.toRealPath();
+
+			if (!isPathWithinAppDataDirectory(resolvedLogoRealPath, appDataPath)) {
+				log.error("Logo path escapes application data directory: {}", logoUrlPath);
+				return null;
+			}
+
+			return resolvedLogoRealPath.toFile();
+		} catch (IllegalArgumentException e) {
+			log.error("Invalid logo path: " + logoUrlPath, e);
+			return null;
+		} catch (IOException e) {
+			log.error("Failed to access logo file: {}", logoUrlPath, e);
+			return null;
+		}
+	}
+
+	private boolean isPathWithinAppDataDirectory(Path path, Path appDataPath) {
+		return path.startsWith(appDataPath);
 	}
 
 	private Map<String, String> createConfigKeyMap() {

api/src/main/java/org/openmrs/module/patientdocuments/reports/PatientIdStickerPdfReport.java

@@ -58,12 +58,12 @@ public class PatientIdStickerPdfReport {
 	@Autowired
 	private InitializerService initializerService;
 
-	public byte[] generatePdf(Patient patient, byte[] defaultLogoBytes) throws RuntimeException {
+	public byte[] generatePdf(Patient patient) throws RuntimeException {
 		validatePatientAndPrivileges(patient);
 
 		try {
 			ReportData reportData = createReportData(patient);
-			byte[] xmlBytes = renderReportToXml(reportData, defaultLogoBytes);
+			byte[] xmlBytes = renderReportToXml(reportData);
 			return transformXmlToPdf(xmlBytes);
 		}
 		catch (Exception e) {
@@ -95,10 +95,10 @@ private ReportData createReportData(Patient patient) throws EvaluationException
 		return reportData;
 	}
 
-	private byte[] renderReportToXml(ReportData reportData, byte[] defaultLogoBytes) throws IOException {
+	private byte[] renderReportToXml(ReportData reportData) throws IOException {
 		PatientIdStickerXmlReportRenderer renderer = new PatientIdStickerXmlReportRenderer();
 		try (ByteArrayOutputStream xmlOutputStream = new ByteArrayOutputStream()) {
-			renderer.render(reportData, null, xmlOutputStream, defaultLogoBytes);
+			renderer.render(reportData, null, xmlOutputStream);
 			return xmlOutputStream.toByteArray();
 		}
 	}

api/src/test/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRendererTest.java

@@ -15,10 +15,15 @@
 import org.openmrs.Patient;
 import org.openmrs.module.patientdocuments.reports.PatientIdStickerPdfReport;
 import org.openmrs.test.jupiter.BaseModuleContextSensitiveTest;
+import org.openmrs.util.OpenmrsUtil;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.openmrs.module.reporting.report.manager.ReportManagerUtil;
 import org.openmrs.module.patientdocuments.reports.PatientIdStickerReportManager;
 
+import java.io.File;
+import java.nio.file.Files;
+import java.nio.file.Path;
+
 public class PatientIdStickerXmlReportRendererTest extends BaseModuleContextSensitiveTest {
 
 	@Autowired
@@ -34,8 +39,40 @@ public void setup() throws Exception {
 	public void generatePdf_shouldThrowWhenPatientIsMissing() throws Exception {
 		Patient badPatient = null;
 		Assertions.assertThrows(IllegalArgumentException.class, () -> {
-			pdfReport.generatePdf(badPatient, null);
+			pdfReport.generatePdf(badPatient);
 		});
 	}
 
+	@Test
+	public void resolveSecureLogoPath_shouldReturnFileWithinAppDataDirectory() throws Exception {
+		PatientIdStickerXmlReportRenderer renderer = new PatientIdStickerXmlReportRenderer();
+		Path logosDirectory = Files.createDirectories(OpenmrsUtil.getApplicationDataDirectoryAsFile().toPath().resolve("logos"));
+		Path logoFile = logosDirectory.resolve("custom-logo.png");
+		Files.write(logoFile, "image-data".getBytes());
+
+		File resolvedLogoFile = renderer.resolveSecureLogoPath("logos/custom-logo.png");
+
+		Assertions.assertNotNull(resolvedLogoFile, "Expected logo file within app data directory to be resolved");
+		Assertions.assertEquals(logoFile.toRealPath(), resolvedLogoFile.toPath().toRealPath());
+	}
+
+	@Test
+	public void resolveSecureLogoPath_shouldRejectPathTraversalAttempts() throws Exception {
+		PatientIdStickerXmlReportRenderer renderer = new PatientIdStickerXmlReportRenderer();
+
+		File resolvedLogoFile = renderer.resolveSecureLogoPath("../malicious-logo.png");
+
+		Assertions.assertNull(resolvedLogoFile, "Path traversal attempts must be rejected");
+	}
+
+	@Test
+	public void resolveSecureLogoPath_shouldRejectAbsolutePathsOutsideAppDataDirectory() throws Exception {
+		PatientIdStickerXmlReportRenderer renderer = new PatientIdStickerXmlReportRenderer();
+		Path outsideLogo = Files.createTempFile("outside-data-dir-logo", ".png");
+
+		File resolvedLogoFile = renderer.resolveSecureLogoPath(outsideLogo.toString());
+
+		Assertions.assertNull(resolvedLogoFile, "Absolute paths outside the app data directory must be rejected");
+		Files.deleteIfExists(outsideLogo);
+	}
 }

omod/src/main/java/org/openmrs/module/patientdocuments/web/rest/controller/PatientIdStickerDataPdfExportController.java

@@ -12,14 +12,8 @@
 import static org.openmrs.module.patientdocuments.common.PatientDocumentsConstants.PATIENT_ID_STICKER_ID;
 import static org.openmrs.module.patientdocuments.common.PatientDocumentsConstants.MODULE_ARTIFACT_ID;
 
-import java.io.IOException;
-import java.io.InputStream;
-
-import javax.servlet.ServletContext;
 import javax.servlet.http.HttpServletResponse;
-import javax.servlet.http.HttpServletRequest;
 
-import org.apache.commons.io.IOUtils;
 import org.openmrs.Patient;
 import org.openmrs.api.PatientService;
 import org.openmrs.module.patientdocuments.reports.PatientIdStickerPdfReport;
@@ -43,8 +37,6 @@
 public class PatientIdStickerDataPdfExportController extends BaseRestController {
 
 	private static final Logger logger = LoggerFactory.getLogger(PatientIdStickerDataPdfExportController.class);
-
-	private static final String DEFAULT_LOGO_CLASSPATH = "/images/openmrs_logo_white_large.png";
 
 	private PatientIdStickerPdfReport pdfReport;
 
@@ -57,10 +49,9 @@ public PatientIdStickerDataPdfExportController(@Qualifier("patientService") Pati
 		this.pdfReport = pdfReport;
 	}
 
-	private ResponseEntity<byte[]> writeResponse(Patient patient, boolean inline, ServletContext servletContext) {
+	private ResponseEntity<byte[]> writeResponse(Patient patient, boolean inline) {
 		try {
-			byte[] defaultLogoBytes = loadDefaultLogo(servletContext);
-			byte[] pdfBytes = pdfReport.generatePdf(patient, defaultLogoBytes);
+			byte[] pdfBytes = pdfReport.generatePdf(patient);
 
 			HttpHeaders headers = new HttpHeaders();
 			headers.set("Content-Type", "application/pdf");
@@ -76,27 +67,9 @@ private ResponseEntity<byte[]> writeResponse(Patient patient, boolean inline, Se
 			        .body("Error generating PDF".getBytes());
 		}
 	}
-
-	private byte[] loadDefaultLogo(ServletContext servletContext) {
-		if (servletContext == null) {
-			return null;
-		}
-
-		try (InputStream logoStream = servletContext.getResourceAsStream(DEFAULT_LOGO_CLASSPATH)) {
-			if (logoStream == null) {
-				logger.warn("Logo file not found at: {}", DEFAULT_LOGO_CLASSPATH);
-				return null;
-			}
-			return IOUtils.toByteArray(logoStream);
-		} catch (IOException e) {
-			logger.error("Failed to load logo from: {}", DEFAULT_LOGO_CLASSPATH, e);
-			return null;
-		}
-	}
 
 	@RequestMapping(method = RequestMethod.GET)
 	public ResponseEntity<byte[]> getPatientIdSticker(HttpServletResponse response,
-	        HttpServletRequest request,
 	        @RequestParam(value = "patientUuid") String patientUuid,
 	        @RequestParam(value = "inline", required = false, defaultValue = "true") boolean inline) {
 
@@ -106,7 +79,6 @@ public ResponseEntity<byte[]> getPatientIdSticker(HttpServletResponse response,
 			return null;
 		}
 
-		ServletContext servletContext = request.getSession().getServletContext();
-		return writeResponse(patient, inline, servletContext);
+		return writeResponse(patient, inline);
 	}
 }