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

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

@@ -16,6 +16,8 @@
 import java.io.File;
 import java.io.IOException;
 import java.io.OutputStream;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.Arrays;
 import java.util.Base64;
 import java.util.HashMap;
@@ -35,6 +37,7 @@
 import javax.xml.transform.stream.StreamResult;
 
 import org.openmrs.annotation.Handler;
+import org.openmrs.api.APIException;
 import org.openmrs.api.context.Context;
 import org.openmrs.messagesource.MessageSourceService;
 import org.openmrs.module.initializer.api.InitializerService;
@@ -49,6 +52,8 @@
 import org.openmrs.module.reporting.report.renderer.ReportDesignRenderer;
 import org.openmrs.module.reporting.report.renderer.ReportRenderer;
 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;
@@ -63,6 +68,8 @@
 @Localized("patientdocuments.patientIdStickerXmlReportRenderer")
 public class PatientIdStickerXmlReportRenderer extends ReportDesignRenderer {
 
+	private static final Logger log = LoggerFactory.getLogger(PatientIdStickerXmlReportRenderer.class);
+
 	private MessageSourceService mss;
 
 	private InitializerService initializerService;
@@ -243,51 +250,112 @@ 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
-     */
+	/**
+	 * Configures the logo for the sticker document.
+	 * 
+	 * Logo resolution priority:
+	 * 1. Custom logo from 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 (must be relative to app data dir)
+	 * @param defaultLogoBytes Default logo bytes to use as fallback
+	 * @throws RenderingException if path traversal is detected or other security violation occurs
+	 */
 	private void configureLogo(Document doc, Element header, String logoUrlPath, byte[] defaultLogoBytes) {
-		String logoPath = "";
+		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();
+				File logoFile = resolveSecureLogoPath(logoUrlPath);
+				if (logoFile != null && logoFile.exists() && logoFile.canRead() && logoFile.isFile()) {
+					logoContent = logoFile.getAbsolutePath();
+				} else {
+					log.warn("Logo file not found, unreadable, or not a regular file: {}", logoUrlPath);
 				}
 			}
-
-			// 2. Fall back to default logo
-			if (isBlank(logoPath) && defaultLogoBytes != null && defaultLogoBytes.length > 0) {
-				String base64Image = Base64.getEncoder().encodeToString(defaultLogoBytes);
-				logoPath = "data:image/png;base64," + base64Image;
-			}
-		} catch (Exception e) {
-			throw new RenderingException("Failed to configure logo", e);
+		} catch (APIException e) {
+			log.error("Security violation when resolving logo path: {}", logoUrlPath, e);
 		}
-
-		// Create and append logo elements if valid
-		if (isNotBlank(logoPath)) {
+
+		if (isBlank(logoContent) && defaultLogoBytes != null && defaultLogoBytes.length > 0) {
+			String base64Image = Base64.getEncoder().encodeToString(defaultLogoBytes);
+			logoContent = "data:image/png;base64," + base64Image;
+		}
+
+		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
+			throw new RenderingException("Failed to configure logo: unresolved path and no default provided");
+		}
+	}
+
+	/**
+	 * Securely resolves a logo path, ensuring it stays within the application data directory.
+	 * 
+	 * @param logoUrlPath The user-provided logo path (must be relative)
+	 * @return A File object if the path is valid and secure, null otherwise
+	 * @throws APIException if path traversal or other security violation is detected
+	 */
+	private File resolveSecureLogoPath(String logoUrlPath) throws APIException {
+		if (isBlank(logoUrlPath)) {
+			return null;
+		}
+
+		final File appDataDir = OpenmrsUtil.getApplicationDataDirectoryAsFile();
+		if (appDataDir == null) {
+			log.error("Application data directory is not configured");
+			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)) {
+					throw new APIException("Absolute path must be within application data directory: " + logoUrlPath);
+				}
+				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)) {
+				throw new APIException("Path traversal detected in logo path: " + logoUrlPath);
+			}
+
+			// 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)) {
+				throw new APIException("Logo path escapes application data directory: " + logoUrlPath);
+			}
+
+			return resolvedLogoRealPath.toFile();
+		} catch (IllegalArgumentException e) {
+			throw new APIException("Invalid characters in logo path: " + logoUrlPath, e);
+		} catch (IOException e) {
+			log.error("Failed to resolve logo path: {}", logoUrlPath, e);
+			return null;
+		}
+	}
+
+	private boolean isPathWithinAppDataDirectory(Path path, Path appDataPath) {
+		return path.startsWith(appDataPath);
 	}
 
 	private Map<String, String> createConfigKeyMap() {

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

@@ -12,23 +12,78 @@
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.AfterEach;
 import org.openmrs.Patient;
+import org.openmrs.api.APIException;
 import org.openmrs.module.patientdocuments.reports.PatientIdStickerPdfReport;
 import org.openmrs.test.jupiter.BaseModuleContextSensitiveTest;
 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.lang.reflect.Method;
+import java.lang.reflect.InvocationTargetException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
 public class PatientIdStickerXmlReportRendererTest extends BaseModuleContextSensitiveTest {
 
 	@Autowired
 	private PatientIdStickerPdfReport pdfReport;
 
+	private Path temporaryApplicationDataDirectory;
+
+	private String originalApplicationDataDirectoryProperty;
+
 	@BeforeEach
 	public void setup() throws Exception {
 		executeDataSet("org/openmrs/module/patientdocuments/include/patientIdStickerManagerTestDataset.xml");
 		ReportManagerUtil.setupReport(new PatientIdStickerReportManager());
 	}
+
+	@BeforeEach
+	public void setupApplicationDataDirectory() throws Exception {
+		originalApplicationDataDirectoryProperty = System.getProperty("OPENMRS_APPLICATION_DATA_DIRECTORY");
+		temporaryApplicationDataDirectory = Files.createTempDirectory("openmrs-appdata-");
+		System.setProperty("OPENMRS_APPLICATION_DATA_DIRECTORY", temporaryApplicationDataDirectory.toFile().getAbsolutePath());
+	}
+
+	@AfterEach
+	public void tearDownApplicationDataDirectory() throws Exception {
+		if (originalApplicationDataDirectoryProperty != null) {
+			System.setProperty("OPENMRS_APPLICATION_DATA_DIRECTORY", originalApplicationDataDirectoryProperty);
+		} else {
+			System.clearProperty("OPENMRS_APPLICATION_DATA_DIRECTORY");
+		}
+		if (temporaryApplicationDataDirectory != null) {
+			try {
+				Files.walk(temporaryApplicationDataDirectory)
+					.sorted((pathA, pathB) -> pathB.compareTo(pathA))
+					.forEach(pathToDelete -> {
+						try {
+							Files.deleteIfExists(pathToDelete);
+						} catch (Exception ignored) { }
+					});
+			} catch (Exception ignored) { }
+		}
+	}
+
+	private File invokeResolveSecureLogoPath(String logoPath) throws Exception {
+		PatientIdStickerXmlReportRenderer renderer = new PatientIdStickerXmlReportRenderer();
+		Method resolveMethod = PatientIdStickerXmlReportRenderer.class.getDeclaredMethod("resolveSecureLogoPath", String.class);
+		resolveMethod.setAccessible(true);
+		try {
+			return (File) resolveMethod.invoke(renderer, logoPath);
+		} catch (InvocationTargetException invocationException) {
+			Throwable actualCause = invocationException.getCause();
+			if (actualCause instanceof APIException) {
+				throw (APIException) actualCause;
+			}
+			throw invocationException;
+		}
+	}
 
 	@Test
 	public void generatePdf_shouldThrowWhenPatientIsMissing() throws Exception {
@@ -38,4 +93,53 @@ public void generatePdf_shouldThrowWhenPatientIsMissing() throws Exception {
 		});
 	}
 
+	@Test
+	public void resolveSecureLogoPath_shouldAllowAbsolutePathWithinAppDataDir() throws Exception {
+		Path logoPathInsideAppData = temporaryApplicationDataDirectory.resolve("logos").resolve("logo.png");
+		Files.createDirectories(logoPathInsideAppData.getParent());
+		Files.createFile(logoPathInsideAppData);
+
+		File resolvedLogoFile = invokeResolveSecureLogoPath(logoPathInsideAppData.toFile().getAbsolutePath());
+		Assertions.assertEquals(logoPathInsideAppData.toFile().getCanonicalPath(), resolvedLogoFile.getCanonicalPath());
+	}
+
+	@Test
+	public void resolveSecureLogoPath_shouldRejectAbsolutePathOutsideAppDataDir() {
+		Assertions.assertThrows(APIException.class, () -> {
+			Path externalTempDirectory = Files.createTempDirectory("outside-appdata-");
+			try {
+				Path logoPathOutsideAppData = externalTempDirectory.resolve("logo.png");
+				Files.createFile(logoPathOutsideAppData);
+				invokeResolveSecureLogoPath(logoPathOutsideAppData.toFile().getAbsolutePath());
+			} finally {
+				try {
+					Files.walk(externalTempDirectory)
+						.sorted((pathA, pathB) -> pathB.compareTo(pathA))
+						.forEach(pathToDelete -> { 
+							try { 
+								Files.deleteIfExists(pathToDelete); 
+							} catch (Exception ignored) { } 
+						});
+				} catch (Exception ignored) { }
+			}
+		});
+	}
+
+	@Test
+	public void resolveSecureLogoPath_shouldResolveRelativePathWithinAppDataDir() throws Exception {
+		String relativeLogoPath = "images/logo.png";
+		Path expectedLogoPath = temporaryApplicationDataDirectory.resolve(Paths.get(relativeLogoPath));
+		Files.createDirectories(expectedLogoPath.getParent());
+		Files.createFile(expectedLogoPath);
+
+		File resolvedLogoFile = invokeResolveSecureLogoPath(relativeLogoPath);
+		Assertions.assertEquals(expectedLogoPath.toFile().getCanonicalPath(), resolvedLogoFile.getCanonicalPath());
+	}
+
+	@Test
+	public void resolveSecureLogoPath_shouldRejectRelativePathTraversal() {
+		Assertions.assertThrows(APIException.class, () -> {
+			invokeResolveSecureLogoPath("../logo.png");
+		});
+	}
 }

readme/PatientIdSticker.md

@@ -28,7 +28,7 @@ These flags control which patient information is displayed on each sticker.
 | Key                                                           | Type    | Description                                      |
 |---------------------------------------------------------------|---------|--------------------------------------------------|
 | `report.patientIdSticker.stylesheet`                          | String  | XSL stylesheet to use for rendering stickers     |
-| `report.patientIdSticker.logourl`                             | String  | Logo path or URL displayed on the sticker (supports a path relative to `OPENMRS_APPLICATION_DATA_DIRECTORY`) |
+| `report.patientIdSticker.logourl`                             | String  | Logo path displayed on the sticker (must be a path relative to `OPENMRS_APPLICATION_DATA_DIRECTORY`) |
 | `report.patientIdSticker.header`                              | Boolean | Show a header section on each sticker            |
 | `report.patientIdSticker.barcode`                             | Boolean | Show a barcode section on each sticker           |
 | `report.patientIdSticker.pages`                               | Number  | Number of sticker pages to generate              |
@@ -58,7 +58,7 @@ These flags control which patient information is displayed on each sticker.
     "report.patientIdSticker.fields.gender": "true",
     "report.patientIdSticker.fields.fulladdress": "true",
     "report.patientIdSticker.stylesheet": "patientIdStickerFopStylesheet.xsl",
-    "report.patientIdSticker.logourl": "http://example.com/logo.png",
+    "report.patientIdSticker.logourl": "branding/logo.png",
     "report.patientIdSticker.pages": "10",
     "report.patientIdSticker.header": "true",
     "report.patientIdSticker.barcode": "true",
@@ -93,7 +93,6 @@ The module supports internationalization through message properties. Field label
 - The secondary identifier type can be configured using `report.patientIdSticker.fields.identifier.secondary.type` if needed.
 - Available stylesheets include `patientIdStickerFopStylesheet.xsl` (default) and `msfStickerFopStylesheet.xsl` for MSF-specific layouts.
 - Logo resolution rules:
-  - If `report.patientIdSticker.logourl` is a relative path, it is resolved under `OPENMRS_APPLICATION_DATA_DIRECTORY` (e.g., `/openmrs/data/my_custom_logo.png`).
-  - If no logo is configured or the configured logo is unavailable, the default OpenMRS logo is loaded from the servlet context.
-  - Supported formats: an absolute filesystem path (for a custom logo) or a base64-encoded data URI (for the default OpenMRS logo), both of which are accepted by the renderer/XSL-FO processor.
+  - `report.patientIdSticker.logourl` should be a path relative to the`OPENMRS_APPLICATION_DATA_DIRECTORY` (e.g., `branding/my_custom_logo.png` resolves to `/openmrs/data/branding/my_custom_logo.png`).
+  - If no logo is configured or the configured logo is unavailable, a default logo (provided by the web layer) is used via a base64 data URI.
 - The barcode is generated from the preferred patient identifier when barcode is enabled.

readme/PatientIdStickerXSL.md

@@ -164,9 +164,9 @@ The optional header section can contain:
 - An organizational logo on the left (from a file path under `OPENMRS_APPLICATION_DATA_DIRECTORY`)
 - Custom header text on the right
 - Logo handling behavior:
-  - Relative paths are resolved under `OPENMRS_APPLICATION_DATA_DIRECTORY`
+  - Logo path is resolved relative to `OPENMRS_APPLICATION_DATA_DIRECTORY`.
+  - Absolute paths are rejected; path traversal sequences are blocked; non-regular files are ignored.
   - If none configured or missing, the default OpenMRS logo is loaded from the servlet context.
-  - Supported formats: absolute filesystem path (custom logo) or base64 data URI (default logo). The XSL-FO processor can handle both.
 
 ### Internationalization Section
 
@@ -208,7 +208,7 @@ The stylesheet includes several responsive design elements:
 - **Demographic Grouping**: In MSF layout, groups Gender, DOB, and Age fields in a single row
 - **Secondary Identifier**: Special handling for secondary patient identifiers
 - **Internationalization**: Support for translated field labels and messages
-- **Logo Handling**: Pulled from the `OPENMRS_APPLICATION_DATA_DIRECTORY` or servlet context for the default OpenMRS logo.
+- **Logo Handling**: Pulled from `OPENMRS_APPLICATION_DATA_DIRECTORY` via a relative path, or from the servlet context for the default OpenMRS logo.
 
 ## Technical Requirements