From fb99653d4548488cd7ed4cf87618d3ebba9ff698 Mon Sep 17 00:00:00 2001 From: jnsereko Date: Wed, 5 Nov 2025 16:51:43 +0300 Subject: [PATCH 01/25] Add security safeguards for logo path configuration to prevent path traversal attacks --- .../PatientIdStickerXmlReportRenderer.java | 130 +++++++++++++----- readme/PatientIdSticker.md | 10 +- readme/PatientIdStickerXSL.md | 8 +- 3 files changed, 106 insertions(+), 42 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java index 21a547c..eabca02 100644 --- a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java +++ b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java @@ -49,6 +49,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 +65,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 +247,111 @@ 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 with security safeguards. + * + * Logo resolution priority: + * 1. Custom logo from path resolved under {@code OPENMRS_APPLICATION_DATA_DIRECTORY} + * 2. Default OpenMRS logo as base64 data URI + * + * Security measures: + * - All paths are restricted to the application data directory + * - Path traversal attempts (../) are blocked + * - Symbolic links are not followed + * - Only readable files are accepted + * + * @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 if (isNotBlank(logoUrlPath)) { + 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 (SecurityException 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 SecurityException if path traversal or other security violation is detected + */ + private File resolveSecureLogoPath(String logoUrlPath) throws SecurityException { + if (isBlank(logoUrlPath)) { + return null; + } + + final File appDataDir = OpenmrsUtil.getDirectoryInApplicationDataDirectory(""); + if (appDataDir == null) { + log.error("Application data directory is not configured"); + return null; + } + + try { + final String appDataCanonical = appDataDir.getCanonicalPath(); + + // Reject absolute paths early + final File rawPath = new File(logoUrlPath); + if (rawPath.isAbsolute()) { + throw new SecurityException("Absolute paths are not allowed for logo files: " + logoUrlPath); + } + + // Detect traversal and suspicious characters in provided path + if (logoUrlPath.contains("..") || logoUrlPath.contains("./") || logoUrlPath.contains(".\\")) { + throw new SecurityException("Path traversal detected in logo path: " + logoUrlPath); + } + if (logoUrlPath.matches(".*[<>:\"|?*].*")) { + throw new SecurityException("Invalid characters in logo path: " + logoUrlPath); + } + + // Resolve against application data directory and validate canonical location + final File resolved = new File(appDataDir, logoUrlPath); + final String resolvedCanonical = resolved.getCanonicalPath(); + if (!resolvedCanonical.startsWith(appDataCanonical)) { + throw new SecurityException("Logo path escapes application data directory: " + logoUrlPath); + } + + // Warn on potential symlink, but allow if still within app data directory + if (!resolved.getAbsolutePath().equals(resolvedCanonical)) { + log.warn("Potential symbolic link detected for logo: {}", logoUrlPath); + } + + return resolved; + } catch (IOException e) { + log.error("Failed to resolve logo path: {}", logoUrlPath, e); + return null; + } } private Map createConfigKeyMap() { diff --git a/readme/PatientIdSticker.md b/readme/PatientIdSticker.md index 5d43193..655d8b9 100644 --- a/readme/PatientIdSticker.md +++ b/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,7 @@ 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` must be a relative path which is resolved under `OPENMRS_APPLICATION_DATA_DIRECTORY` (e.g., `branding/my_custom_logo.png` resolves to `/openmrs/data/branding/my_custom_logo.png`). + - Absolute paths are rejected for security reasons; path traversal sequences (like `../`) are blocked; non-regular files are ignored. + - 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. \ No newline at end of file diff --git a/readme/PatientIdStickerXSL.md b/readme/PatientIdStickerXSL.md index 4d9173f..3e2fd2a 100644 --- a/readme/PatientIdStickerXSL.md +++ b/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` - - 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. + - Logo path must be relative and is resolved under `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 and embedded as a base64 data URI. The XSL-FO processor can handle both filesystem paths and data URIs. ### 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. Absolute paths are not allowed. ## Technical Requirements From 9466efd171242a9b017818e28d3c9b0c65aebe0c Mon Sep 17 00:00:00 2001 From: jnsereko <58003327+jnsereko@users.noreply.github.com> Date: Fri, 7 Nov 2025 10:09:57 +0300 Subject: [PATCH 02/25] Update readme/PatientIdStickerXSL.md Co-authored-by: Ian <52504170+ibacher@users.noreply.github.com> --- readme/PatientIdStickerXSL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readme/PatientIdStickerXSL.md b/readme/PatientIdStickerXSL.md index 3e2fd2a..38bf3ba 100644 --- a/readme/PatientIdStickerXSL.md +++ b/readme/PatientIdStickerXSL.md @@ -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 `OPENMRS_APPLICATION_DATA_DIRECTORY` via a relative path, or from the servlet context for the default OpenMRS logo. Absolute paths are not allowed. +- **Logo Handling**: Pulled from `OPENMRS_APPLICATION_DATA_DIRECTORY` via a relative path, or from the servlet context for the default OpenMRS logo. ## Technical Requirements From 3a69eada67a5085d427ea1d25a16ef9cf4e24176 Mon Sep 17 00:00:00 2001 From: jnsereko <58003327+jnsereko@users.noreply.github.com> Date: Fri, 7 Nov 2025 10:10:12 +0300 Subject: [PATCH 03/25] Update readme/PatientIdStickerXSL.md Co-authored-by: Ian <52504170+ibacher@users.noreply.github.com> --- readme/PatientIdStickerXSL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readme/PatientIdStickerXSL.md b/readme/PatientIdStickerXSL.md index 38bf3ba..cf572f9 100644 --- a/readme/PatientIdStickerXSL.md +++ b/readme/PatientIdStickerXSL.md @@ -166,7 +166,7 @@ The optional header section can contain: - Logo handling behavior: - Logo path must be relative and is resolved under `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 and embedded as a base64 data URI. The XSL-FO processor can handle both filesystem paths and data URIs. + - If none configured or missing, the default OpenMRS logo is loaded from the servlet context. ### Internationalization Section From 3393fcceb066de2a3ba115cf46abc8a4ad861730 Mon Sep 17 00:00:00 2001 From: jnsereko <58003327+jnsereko@users.noreply.github.com> Date: Fri, 7 Nov 2025 10:10:26 +0300 Subject: [PATCH 04/25] Update readme/PatientIdStickerXSL.md Co-authored-by: Ian <52504170+ibacher@users.noreply.github.com> --- readme/PatientIdStickerXSL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readme/PatientIdStickerXSL.md b/readme/PatientIdStickerXSL.md index cf572f9..1941bd8 100644 --- a/readme/PatientIdStickerXSL.md +++ b/readme/PatientIdStickerXSL.md @@ -164,7 +164,7 @@ 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: - - Logo path must be relative and is 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. From c4ff685fc761d3c51b5e62890f07d869c34bd210 Mon Sep 17 00:00:00 2001 From: jnsereko <58003327+jnsereko@users.noreply.github.com> Date: Fri, 7 Nov 2025 10:10:40 +0300 Subject: [PATCH 05/25] Update readme/PatientIdSticker.md Co-authored-by: Ian <52504170+ibacher@users.noreply.github.com> --- readme/PatientIdSticker.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readme/PatientIdSticker.md b/readme/PatientIdSticker.md index 655d8b9..03cbdc7 100644 --- a/readme/PatientIdSticker.md +++ b/readme/PatientIdSticker.md @@ -93,7 +93,7 @@ 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: - - `report.patientIdSticker.logourl` must be a relative path which is resolved under `OPENMRS_APPLICATION_DATA_DIRECTORY` (e.g., `branding/my_custom_logo.png` resolves to `/openmrs/data/branding/my_custom_logo.png`). + - `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`). - Absolute paths are rejected for security reasons; path traversal sequences (like `../`) are blocked; non-regular files are ignored. - 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. \ No newline at end of file From ef44fbd09d87cc906f2af7d6ea5834fe06738b04 Mon Sep 17 00:00:00 2001 From: jnsereko <58003327+jnsereko@users.noreply.github.com> Date: Fri, 7 Nov 2025 10:10:52 +0300 Subject: [PATCH 06/25] Update api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java Co-authored-by: Ian <52504170+ibacher@users.noreply.github.com> --- .../renderer/PatientIdStickerXmlReportRenderer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java index eabca02..0c27071 100644 --- a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java +++ b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java @@ -312,7 +312,7 @@ private File resolveSecureLogoPath(String logoUrlPath) throws SecurityException return null; } - final File appDataDir = OpenmrsUtil.getDirectoryInApplicationDataDirectory(""); + final File appDataDir = OpenmrsUtil.getApplicationDataDirectoryAsFile(); if (appDataDir == null) { log.error("Application data directory is not configured"); return null; From 5e05736ceaf6ddae2c405f896ec6eb7f36f321df Mon Sep 17 00:00:00 2001 From: jnsereko Date: Tue, 11 Nov 2025 19:17:41 +0300 Subject: [PATCH 07/25] clean LLM documentation and remove hard-corded checks. --- .../PatientIdStickerXmlReportRenderer.java | 81 ++++++++------ ...PatientIdStickerXmlReportRendererTest.java | 104 ++++++++++++++++++ readme/PatientIdSticker.md | 1 - 3 files changed, 150 insertions(+), 36 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java index 0c27071..18462b0 100644 --- a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java +++ b/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.nio.file.InvalidPathException; +import java.nio.file.Path; +import java.nio.file.Paths; import java.util.Arrays; import java.util.Base64; import java.util.HashMap; @@ -35,6 +38,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; @@ -248,18 +252,12 @@ private void configureHeader(Document doc, Element templatePIDElement, byte[] de } /** - * Configures the logo for the sticker document with security safeguards. + * 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 * - * Security measures: - * - All paths are restricted to the application data directory - * - Path traversal attempts (../) are blocked - * - Symbolic links are not followed - * - Only readable files are accepted - * * @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) @@ -270,11 +268,12 @@ private void configureLogo(Document doc, Element header, String logoUrlPath, byt String logoContent = null; try { + // 1. Try custom logo if (isNotBlank(logoUrlPath)) { File logoFile = resolveSecureLogoPath(logoUrlPath); if (logoFile != null && logoFile.exists() && logoFile.canRead() && logoFile.isFile()) { logoContent = logoFile.getAbsolutePath(); - } else if (isNotBlank(logoUrlPath)) { + } else { log.warn("Logo file not found, unreadable, or not a regular file: {}", logoUrlPath); } } @@ -305,54 +304,66 @@ else if (isNotBlank(logoUrlPath)) { * * @param logoUrlPath The user-provided logo path (must be relative) * @return A File object if the path is valid and secure, null otherwise - * @throws SecurityException if path traversal or other security violation is detected + * @throws APIException if path traversal or other security violation is detected */ - private File resolveSecureLogoPath(String logoUrlPath) throws SecurityException { + 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 String appDataCanonical = appDataDir.getCanonicalPath(); - - // Reject absolute paths early - final File rawPath = new File(logoUrlPath); - if (rawPath.isAbsolute()) { - throw new SecurityException("Absolute paths are not allowed for logo files: " + logoUrlPath); - } - - // Detect traversal and suspicious characters in provided path - if (logoUrlPath.contains("..") || logoUrlPath.contains("./") || logoUrlPath.contains(".\\")) { - throw new SecurityException("Path traversal detected in logo path: " + logoUrlPath); + 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(); } - if (logoUrlPath.matches(".*[<>:\"|?*].*")) { - throw new SecurityException("Invalid characters in logo path: " + logoUrlPath); + + // 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 canonical location - final File resolved = new File(appDataDir, logoUrlPath); - final String resolvedCanonical = resolved.getCanonicalPath(); - if (!resolvedCanonical.startsWith(appDataCanonical)) { - throw new SecurityException("Logo path escapes application data directory: " + logoUrlPath); + + // Resolve against application data directory and validate real location + final Path resolved = appDataPath.resolve(logoUrlPath).normalize(); + final Path resolvedReal = resolved.toRealPath(); + + if (!isPathWithinAppDataDirectory(resolvedReal, appDataPath)) { + throw new APIException("Logo path escapes application data directory: " + logoUrlPath); } - + // Warn on potential symlink, but allow if still within app data directory - if (!resolved.getAbsolutePath().equals(resolvedCanonical)) { + if (!resolved.equals(resolvedReal)) { log.warn("Potential symbolic link detected for logo: {}", logoUrlPath); } - - return resolved; + + return resolvedReal.toFile(); + } catch (InvalidPathException 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 createConfigKeyMap() { Map configKeyMap = new HashMap<>(); diff --git a/api/src/test/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRendererTest.java b/api/src/test/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRendererTest.java index 79dd159..81e58d3 100644 --- a/api/src/test/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRendererTest.java +++ b/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"); + }); + } } diff --git a/readme/PatientIdSticker.md b/readme/PatientIdSticker.md index 03cbdc7..99c3a80 100644 --- a/readme/PatientIdSticker.md +++ b/readme/PatientIdSticker.md @@ -94,6 +94,5 @@ The module supports internationalization through message properties. Field label - Available stylesheets include `patientIdStickerFopStylesheet.xsl` (default) and `msfStickerFopStylesheet.xsl` for MSF-specific layouts. - Logo resolution rules: - `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`). - - Absolute paths are rejected for security reasons; path traversal sequences (like `../`) are blocked; non-regular files are ignored. - 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. \ No newline at end of file From 665a16c2551dadf5a65b435a148d1912dc2fd434 Mon Sep 17 00:00:00 2001 From: jnsereko Date: Tue, 11 Nov 2025 19:25:34 +0300 Subject: [PATCH 08/25] clean LLM documentation and remove hard-corded checks. --- .../PatientIdStickerXmlReportRenderer.java | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java index 18462b0..314beb9 100644 --- a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java +++ b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java @@ -325,8 +325,7 @@ private File resolveSecureLogoPath(String logoUrlPath) throws APIException { if (logoPath.isAbsolute()) { final Path logoRealPath = logoPath.toRealPath(); if (!isPathWithinAppDataDirectory(logoRealPath, appDataPath)) { - throw new APIException( - "Absolute path must be within application data directory: " + logoUrlPath); + throw new APIException("Absolute path must be within application data directory: " + logoUrlPath); } return logoRealPath.toFile(); } @@ -340,19 +339,14 @@ private File resolveSecureLogoPath(String logoUrlPath) throws APIException { } // Resolve against application data directory and validate real location - final Path resolved = appDataPath.resolve(logoUrlPath).normalize(); - final Path resolvedReal = resolved.toRealPath(); + final Path resolvedLogoPath = appDataPath.resolve(logoUrlPath).normalize(); + final Path resolvedLogoRealPath = resolvedLogoPath.toRealPath(); - if (!isPathWithinAppDataDirectory(resolvedReal, appDataPath)) { + if (!isPathWithinAppDataDirectory(resolvedLogoRealPath, appDataPath)) { throw new APIException("Logo path escapes application data directory: " + logoUrlPath); } - // Warn on potential symlink, but allow if still within app data directory - if (!resolved.equals(resolvedReal)) { - log.warn("Potential symbolic link detected for logo: {}", logoUrlPath); - } - - return resolvedReal.toFile(); + return resolvedLogoRealPath.toFile(); } catch (InvalidPathException e) { throw new APIException("Invalid characters in logo path: " + logoUrlPath, e); } catch (IOException e) { From 9e50b011441954f312e99aea4dd827e6b8aa9013 Mon Sep 17 00:00:00 2001 From: jnsereko Date: Tue, 11 Nov 2025 19:34:46 +0300 Subject: [PATCH 09/25] catch APIException and not Security Exception --- .../renderer/PatientIdStickerXmlReportRenderer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java index 314beb9..18afc10 100644 --- a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java +++ b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java @@ -277,7 +277,7 @@ private void configureLogo(Document doc, Element header, String logoUrlPath, byt log.warn("Logo file not found, unreadable, or not a regular file: {}", logoUrlPath); } } - } catch (SecurityException e) { + } catch (APIException e) { log.error("Security violation when resolving logo path: {}", logoUrlPath, e); } From 02a5c483c2d8a74fc53e9c69582c5c9f5b42ca9c Mon Sep 17 00:00:00 2001 From: jnsereko Date: Tue, 11 Nov 2025 19:42:09 +0300 Subject: [PATCH 10/25] catch parent IllegalArgumentException and not child InvalidPathException --- .../renderer/PatientIdStickerXmlReportRenderer.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java index 18afc10..d951df2 100644 --- a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java +++ b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java @@ -16,7 +16,6 @@ import java.io.File; import java.io.IOException; import java.io.OutputStream; -import java.nio.file.InvalidPathException; import java.nio.file.Path; import java.nio.file.Paths; import java.util.Arrays; @@ -347,7 +346,7 @@ private File resolveSecureLogoPath(String logoUrlPath) throws APIException { } return resolvedLogoRealPath.toFile(); - } catch (InvalidPathException e) { + } 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); From eb27ff76177e9a48ae06a029e139826280b387fd Mon Sep 17 00:00:00 2001 From: jnsereko Date: Fri, 14 Nov 2025 14:20:54 +0300 Subject: [PATCH 11/25] Improve error messages --- .../PatientIdStickerXmlReportRenderer.java | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java index d951df2..3a39b1f 100644 --- a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java +++ b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java @@ -277,7 +277,14 @@ private void configureLogo(Document doc, Element header, String logoUrlPath, byt } } } catch (APIException e) { - log.error("Security violation when resolving logo path: {}", logoUrlPath, e); + String message = e.getMessage(); + Boolean isSecurityError = message != null + && (message.contains("traversal") || message.contains("must be within")); + if (isSecurityError) { + log.error("Security violation when resolving logo path: {}", logoUrlPath, e); + } else { + log.error("Invalid logo path provided: {}", logoUrlPath, e); + } } if (isBlank(logoContent) && defaultLogoBytes != null && defaultLogoBytes.length > 0) { @@ -294,7 +301,7 @@ private void configureLogo(Document doc, Element header, String logoUrlPath, byt } 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"); + throw new RenderingException("Failed to configure logo: unresolved path '" + logoUrlPath + "' and no default provided"); } } @@ -342,12 +349,12 @@ private File resolveSecureLogoPath(String logoUrlPath) throws APIException { final Path resolvedLogoRealPath = resolvedLogoPath.toRealPath(); if (!isPathWithinAppDataDirectory(resolvedLogoRealPath, appDataPath)) { - throw new APIException("Logo path escapes application data directory: " + logoUrlPath); + throw new APIException("Logo path must be within application data directory: " + logoUrlPath); } return resolvedLogoRealPath.toFile(); } catch (IllegalArgumentException e) { - throw new APIException("Invalid characters in logo path: " + logoUrlPath, e); + throw new APIException("Invalid logo path: " + logoUrlPath, e); } catch (IOException e) { log.error("Failed to resolve logo path: {}", logoUrlPath, e); return null; From 0eae47eb3af02c58cf5f88545524cc8d897882e8 Mon Sep 17 00:00:00 2001 From: jnsereko Date: Fri, 14 Nov 2025 17:32:46 +0300 Subject: [PATCH 12/25] Just log caught errors instead of throwing them --- .../PatientIdStickerXmlReportRenderer.java | 39 +++++++------------ 1 file changed, 13 insertions(+), 26 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java index 3a39b1f..423d0cb 100644 --- a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java +++ b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java @@ -261,29 +261,15 @@ private void configureHeader(Document doc, Element templatePIDElement, byte[] de * @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 logoContent = null; - try { - // 1. Try custom logo - if (isNotBlank(logoUrlPath)) { - 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); - } - } - } catch (APIException e) { - String message = e.getMessage(); - Boolean isSecurityError = message != null - && (message.contains("traversal") || message.contains("must be within")); - if (isSecurityError) { - log.error("Security violation when resolving logo path: {}", logoUrlPath, e); - } else { - log.error("Invalid logo path provided: {}", logoUrlPath, e); + // 1. Try custom logo + if (isNotBlank(logoUrlPath)) { + File logoFile = resolveSecureLogoPath(logoUrlPath); + if (logoFile != null && logoFile.exists() && logoFile.canRead() && logoFile.isFile()) { + logoContent = logoFile.getAbsolutePath(); } } @@ -301,7 +287,7 @@ private void configureLogo(Document doc, Element header, String logoUrlPath, byt } 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 '" + logoUrlPath + "' and no default provided"); + log.error("Failed to configure logo: unresolved path '" + logoUrlPath + "' and no default provided"); } } @@ -312,7 +298,7 @@ else if (isNotBlank(logoUrlPath)) { * @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 { + private File resolveSecureLogoPath(String logoUrlPath) { if (isBlank(logoUrlPath)) { return null; } @@ -331,7 +317,7 @@ private File resolveSecureLogoPath(String logoUrlPath) throws APIException { if (logoPath.isAbsolute()) { final Path logoRealPath = logoPath.toRealPath(); if (!isPathWithinAppDataDirectory(logoRealPath, appDataPath)) { - throw new APIException("Absolute path must be within application data directory: " + logoUrlPath); + log.error("Absolute path must be within application data directory: " + logoUrlPath); } return logoRealPath.toFile(); } @@ -341,7 +327,7 @@ private File resolveSecureLogoPath(String logoUrlPath) throws APIException { final Path logoNormalizedPath = logoAbsolutePath.normalize(); if (!logoAbsolutePath.equals(logoNormalizedPath)) { - throw new APIException("Path traversal detected in logo path: " + logoUrlPath); + log.error("Path traversal detected in logo path: " + logoUrlPath); } // Resolve against application data directory and validate real location @@ -349,14 +335,15 @@ private File resolveSecureLogoPath(String logoUrlPath) throws APIException { final Path resolvedLogoRealPath = resolvedLogoPath.toRealPath(); if (!isPathWithinAppDataDirectory(resolvedLogoRealPath, appDataPath)) { - throw new APIException("Logo path must be within application data directory: " + logoUrlPath); + log.error("Logo path escapes application data directory: " + logoUrlPath); } return resolvedLogoRealPath.toFile(); } catch (IllegalArgumentException e) { - throw new APIException("Invalid logo path: " + logoUrlPath, e); + log.error("Invalid logo path: " + logoUrlPath, e); + return null; } catch (IOException e) { - log.error("Failed to resolve logo path: {}", logoUrlPath, e); + log.error("Failed to access logo file: {}", logoUrlPath, e); return null; } } From b8a325856985bce128d61ed2ef278ec028983daa Mon Sep 17 00:00:00 2001 From: jnsereko <58003327+jnsereko@users.noreply.github.com> Date: Mon, 17 Nov 2025 09:21:49 +0300 Subject: [PATCH 13/25] Update api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java Co-authored-by: Ian <52504170+ibacher@users.noreply.github.com> --- .../renderer/PatientIdStickerXmlReportRenderer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java index 423d0cb..1ced0b0 100644 --- a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java +++ b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java @@ -305,7 +305,7 @@ private File resolveSecureLogoPath(String logoUrlPath) { final File appDataDir = OpenmrsUtil.getApplicationDataDirectoryAsFile(); if (appDataDir == null) { - log.error("Application data directory is not configured"); + log.error("Application data directory could not be found"); return null; } From 5a388fc3744a791e9ff700ea7c6fb43765be0fd7 Mon Sep 17 00:00:00 2001 From: jnsereko <58003327+jnsereko@users.noreply.github.com> Date: Mon, 17 Nov 2025 09:22:06 +0300 Subject: [PATCH 14/25] Update api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java Co-authored-by: Ian <52504170+ibacher@users.noreply.github.com> --- .../renderer/PatientIdStickerXmlReportRenderer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java index 1ced0b0..36a2926 100644 --- a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java +++ b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java @@ -292,7 +292,7 @@ else if (isNotBlank(logoUrlPath)) { } /** - * Securely resolves a logo path, ensuring it stays within the application data directory. + * Ensure that the supplied {@code logoUrlPath} refers to a file in 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 From ee8dd5de01c0476cb92029e018cb09c6d6cfa628 Mon Sep 17 00:00:00 2001 From: jnsereko <58003327+jnsereko@users.noreply.github.com> Date: Mon, 17 Nov 2025 09:22:16 +0300 Subject: [PATCH 15/25] Update api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java Co-authored-by: Ian <52504170+ibacher@users.noreply.github.com> --- .../renderer/PatientIdStickerXmlReportRenderer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java index 36a2926..acd41b2 100644 --- a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java +++ b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java @@ -295,7 +295,7 @@ else if (isNotBlank(logoUrlPath)) { * Ensure that the supplied {@code logoUrlPath} refers to a file in 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 + * @return A File object pointing to the logo if the path is valid otherwise null * @throws APIException if path traversal or other security violation is detected */ private File resolveSecureLogoPath(String logoUrlPath) { From a51180afb13c7db47250b32ee6d276d8f73e1c50 Mon Sep 17 00:00:00 2001 From: jnsereko <58003327+jnsereko@users.noreply.github.com> Date: Mon, 17 Nov 2025 09:22:32 +0300 Subject: [PATCH 16/25] Update api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java Co-authored-by: Ian <52504170+ibacher@users.noreply.github.com> --- .../renderer/PatientIdStickerXmlReportRenderer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java index acd41b2..f20f267 100644 --- a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java +++ b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java @@ -294,7 +294,7 @@ else if (isNotBlank(logoUrlPath)) { /** * Ensure that the supplied {@code logoUrlPath} refers to a file in the application data directory * - * @param logoUrlPath The user-provided logo path (must be relative) + * @param logoUrlPath The user-provided logo path * @return A File object pointing to the logo if the path is valid otherwise null * @throws APIException if path traversal or other security violation is detected */ From e170a559b24e28e175cf14106e1fb891ecca6913 Mon Sep 17 00:00:00 2001 From: jnsereko <58003327+jnsereko@users.noreply.github.com> Date: Mon, 17 Nov 2025 09:22:50 +0300 Subject: [PATCH 17/25] Update readme/PatientIdStickerXSL.md Co-authored-by: Ian <52504170+ibacher@users.noreply.github.com> --- readme/PatientIdStickerXSL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readme/PatientIdStickerXSL.md b/readme/PatientIdStickerXSL.md index 1941bd8..c72fe20 100644 --- a/readme/PatientIdStickerXSL.md +++ b/readme/PatientIdStickerXSL.md @@ -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 `OPENMRS_APPLICATION_DATA_DIRECTORY` via a relative path, or from the servlet context for the default OpenMRS logo. +- **Logo Handling**: A custom logo can be provided as a PNG file stored in the application data directory. By default, this will use the OpenMRS logo. ## Technical Requirements From c2df6d93b5a477f66155e0c95c62d846446c31d6 Mon Sep 17 00:00:00 2001 From: jnsereko <58003327+jnsereko@users.noreply.github.com> Date: Mon, 17 Nov 2025 09:23:01 +0300 Subject: [PATCH 18/25] Update readme/PatientIdSticker.md Co-authored-by: Ian <52504170+ibacher@users.noreply.github.com> --- readme/PatientIdSticker.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readme/PatientIdSticker.md b/readme/PatientIdSticker.md index 99c3a80..b67ba6d 100644 --- a/readme/PatientIdSticker.md +++ b/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 displayed on the sticker (must be a path relative to `OPENMRS_APPLICATION_DATA_DIRECTORY`) | +| `report.patientIdSticker.logourl` | String | Logo path displayed on the sticker (must be a path relative to the 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 | From ccf85920d5c9dc7d35fd9d02afb3a03688cde2a9 Mon Sep 17 00:00:00 2001 From: jnsereko <58003327+jnsereko@users.noreply.github.com> Date: Mon, 17 Nov 2025 09:23:12 +0300 Subject: [PATCH 19/25] Update api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java Co-authored-by: Ian <52504170+ibacher@users.noreply.github.com> --- .../renderer/PatientIdStickerXmlReportRenderer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java index f20f267..348a642 100644 --- a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java +++ b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java @@ -287,7 +287,7 @@ private void configureLogo(Document doc, Element header, String logoUrlPath, byt } 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 '" + logoUrlPath + "' and no default provided"); + log.error("Failed to configure logo: unresolved path '{}' and no default provided", logoUrlPath); } } From 174733a29b68a7ff91711d64b4e5ee7ab7988b85 Mon Sep 17 00:00:00 2001 From: jnsereko <58003327+jnsereko@users.noreply.github.com> Date: Mon, 17 Nov 2025 09:23:44 +0300 Subject: [PATCH 20/25] Update readme/PatientIdSticker.md Co-authored-by: Ian <52504170+ibacher@users.noreply.github.com> --- readme/PatientIdSticker.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readme/PatientIdSticker.md b/readme/PatientIdSticker.md index b67ba6d..9f4628c 100644 --- a/readme/PatientIdSticker.md +++ b/readme/PatientIdSticker.md @@ -93,6 +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: - - `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`). + - `report.patientIdSticker.logourl` should be a path relative to the`OPENMRS_APPLICATION_DATA_DIRECTORY` (e.g., `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. \ No newline at end of file From afe7272fcdfab33bdd78695d6d21c7e8513b982f Mon Sep 17 00:00:00 2001 From: jnsereko <58003327+jnsereko@users.noreply.github.com> Date: Mon, 17 Nov 2025 09:24:05 +0300 Subject: [PATCH 21/25] Update readme/PatientIdSticker.md Co-authored-by: Ian <52504170+ibacher@users.noreply.github.com> --- readme/PatientIdSticker.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readme/PatientIdSticker.md b/readme/PatientIdSticker.md index 9f4628c..d40cf92 100644 --- a/readme/PatientIdSticker.md +++ b/readme/PatientIdSticker.md @@ -94,5 +94,5 @@ The module supports internationalization through message properties. Field label - Available stylesheets include `patientIdStickerFopStylesheet.xsl` (default) and `msfStickerFopStylesheet.xsl` for MSF-specific layouts. - Logo resolution rules: - `report.patientIdSticker.logourl` should be a path relative to the`OPENMRS_APPLICATION_DATA_DIRECTORY` (e.g., `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. + - If no logo is configured or the configured logo is unavailable, the OpenMRS logo will be used as a default (requires the legacyui module). - The barcode is generated from the preferred patient identifier when barcode is enabled. \ No newline at end of file From 6653ea06913bba701ed9988bafb0e74ec128657d Mon Sep 17 00:00:00 2001 From: jnsereko Date: Tue, 18 Nov 2025 21:29:23 +0300 Subject: [PATCH 22/25] Convert OpenMRS logo from classpath into data URI and clean documentation --- .../PatientIdStickerXmlReportRenderer.java | 73 +++++++---- .../reports/PatientIdStickerPdfReport.java | 8 +- ...PatientIdStickerXmlReportRendererTest.java | 115 ++++-------------- ...tientIdStickerDataPdfExportController.java | 34 +----- .../resources/openmrs_logo_white_large.png | Bin 0 -> 8462 bytes ...tIdStickerDataPdfExportControllerTest.java | 10 +- readme/PatientIdSticker.md | 4 +- readme/PatientIdStickerXSL.md | 6 +- 8 files changed, 88 insertions(+), 162 deletions(-) create mode 100644 omod/src/main/webapp/resources/openmrs_logo_white_large.png diff --git a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java index 348a642..5098b34 100644 --- a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java +++ b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java @@ -16,6 +16,7 @@ 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; @@ -37,7 +38,6 @@ 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; @@ -51,6 +51,7 @@ 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; @@ -59,6 +60,7 @@ 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 @@ -69,6 +71,8 @@ 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; @@ -123,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 { @@ -151,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); @@ -224,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) { @@ -253,29 +253,38 @@ private void configureHeader(Document doc, Element templatePIDElement, byte[] de /** * 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 + * 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) - * @param defaultLogoBytes Default logo bytes to use as fallback */ - private void configureLogo(Document doc, Element header, String logoUrlPath, byte[] defaultLogoBytes) { + private void configureLogo(Document doc, Element header, String logoUrlPath) { String logoContent = null; // 1. Try custom logo if (isNotBlank(logoUrlPath)) { File logoFile = resolveSecureLogoPath(logoUrlPath); if (logoFile != null && logoFile.exists() && logoFile.canRead() && logoFile.isFile()) { - logoContent = logoFile.getAbsolutePath(); + 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); + } } } - if (isBlank(logoContent) && defaultLogoBytes != null && defaultLogoBytes.length > 0) { - String base64Image = Base64.getEncoder().encodeToString(defaultLogoBytes); - logoContent = "data:image/png;base64," + base64Image; + if (isBlank(logoContent)) { + byte[] defaultLogoBytes = loadDefaultLogoFromClasspath(); + if (defaultLogoBytes != null && defaultLogoBytes.length > 0) { + String base64Image = Base64.getEncoder().encodeToString(defaultLogoBytes); + logoContent = "data:image/png;base64," + base64Image; + } } if (isNotBlank(logoContent)) { @@ -291,14 +300,27 @@ else if (isNotBlank(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 null - * @throws APIException if path traversal or other security violation is detected + * @return A File object pointing to the logo if the path is valid, otherwise {@code null} */ - private File resolveSecureLogoPath(String logoUrlPath) { + protected File resolveSecureLogoPath(String logoUrlPath) { if (isBlank(logoUrlPath)) { return null; } @@ -317,7 +339,8 @@ private File resolveSecureLogoPath(String logoUrlPath) { if (logoPath.isAbsolute()) { final Path logoRealPath = logoPath.toRealPath(); if (!isPathWithinAppDataDirectory(logoRealPath, appDataPath)) { - log.error("Absolute path must be within application data directory: " + logoUrlPath); + log.error("Absolute path must be within application data directory: {}", logoUrlPath); + return null; } return logoRealPath.toFile(); } @@ -327,7 +350,8 @@ private File resolveSecureLogoPath(String logoUrlPath) { final Path logoNormalizedPath = logoAbsolutePath.normalize(); if (!logoAbsolutePath.equals(logoNormalizedPath)) { - log.error("Path traversal detected in logo path: " + logoUrlPath); + log.error("Path traversal detected in logo path: {}", logoUrlPath); + return null; } // Resolve against application data directory and validate real location @@ -335,7 +359,8 @@ private File resolveSecureLogoPath(String logoUrlPath) { final Path resolvedLogoRealPath = resolvedLogoPath.toRealPath(); if (!isPathWithinAppDataDirectory(resolvedLogoRealPath, appDataPath)) { - log.error("Logo path escapes application data directory: " + logoUrlPath); + log.error("Logo path escapes application data directory: {}", logoUrlPath); + return null; } return resolvedLogoRealPath.toFile(); diff --git a/api/src/main/java/org/openmrs/module/patientdocuments/reports/PatientIdStickerPdfReport.java b/api/src/main/java/org/openmrs/module/patientdocuments/reports/PatientIdStickerPdfReport.java index fb3eb02..ece961f 100644 --- a/api/src/main/java/org/openmrs/module/patientdocuments/reports/PatientIdStickerPdfReport.java +++ b/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(); } } diff --git a/api/src/test/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRendererTest.java b/api/src/test/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRendererTest.java index 81e58d3..886cef4 100644 --- a/api/src/test/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRendererTest.java +++ b/api/src/test/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRendererTest.java @@ -12,134 +12,67 @@ 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.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.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 { Patient badPatient = null; Assertions.assertThrows(IllegalArgumentException.class, () -> { - pdfReport.generatePdf(badPatient, null); + pdfReport.generatePdf(badPatient); }); } @Test - public void resolveSecureLogoPath_shouldAllowAbsolutePathWithinAppDataDir() throws Exception { - Path logoPathInsideAppData = temporaryApplicationDataDirectory.resolve("logos").resolve("logo.png"); - Files.createDirectories(logoPathInsideAppData.getParent()); - Files.createFile(logoPathInsideAppData); + 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 = 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) { } - } - }); + 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_shouldResolveRelativePathWithinAppDataDir() throws Exception { - String relativeLogoPath = "images/logo.png"; - Path expectedLogoPath = temporaryApplicationDataDirectory.resolve(Paths.get(relativeLogoPath)); - Files.createDirectories(expectedLogoPath.getParent()); - Files.createFile(expectedLogoPath); + public void resolveSecureLogoPath_shouldRejectPathTraversalAttempts() throws Exception { + PatientIdStickerXmlReportRenderer renderer = new PatientIdStickerXmlReportRenderer(); - File resolvedLogoFile = invokeResolveSecureLogoPath(relativeLogoPath); - Assertions.assertEquals(expectedLogoPath.toFile().getCanonicalPath(), resolvedLogoFile.getCanonicalPath()); + File resolvedLogoFile = renderer.resolveSecureLogoPath("../malicious-logo.png"); + + Assertions.assertNull(resolvedLogoFile, "Path traversal attempts must be rejected"); } @Test - public void resolveSecureLogoPath_shouldRejectRelativePathTraversal() { - Assertions.assertThrows(APIException.class, () -> { - invokeResolveSecureLogoPath("../logo.png"); - }); + 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); } } diff --git a/omod/src/main/java/org/openmrs/module/patientdocuments/web/rest/controller/PatientIdStickerDataPdfExportController.java b/omod/src/main/java/org/openmrs/module/patientdocuments/web/rest/controller/PatientIdStickerDataPdfExportController.java index 62db228..5469a7a 100644 --- a/omod/src/main/java/org/openmrs/module/patientdocuments/web/rest/controller/PatientIdStickerDataPdfExportController.java +++ b/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 writeResponse(Patient patient, boolean inline, ServletContext servletContext) { + private ResponseEntity 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 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 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 getPatientIdSticker(HttpServletResponse response, return null; } - ServletContext servletContext = request.getSession().getServletContext(); - return writeResponse(patient, inline, servletContext); + return writeResponse(patient, inline); } } diff --git a/omod/src/main/webapp/resources/openmrs_logo_white_large.png b/omod/src/main/webapp/resources/openmrs_logo_white_large.png new file mode 100644 index 0000000000000000000000000000000000000000..2a777daed07794f0ed6c7605e5c8477cd5e6aabe GIT binary patch literal 8462 zcmZu$WmJ^k*Bzupx1F_tK^Qs)sX>rI`i0-O zcdh@I=RE7|ea^k>u5)YOXdNwO+!rrj00017RTTw2000H~_#gNjoHB%ww?je0diz;Rs5uNHQsk3OOP&Y` z9RF0)HK6|t4}-ekP`6&Gw&3LJ0o~hGry#o7*$)ba(E7{XON*YbU{k>|xI!-EnN%Xmr*wD35+j&7+iA%(GEB5_Lbv*B@|$SPIo^Mp2&-#UpAL>BHy6 zVH=OckUQQzm3B_<8=N5Xf~gjw`Q3jkPS{%3~p{xeZD{~3ZO4j^Rdsg=j} zpHlvBrp(cQ(Enu0Y=K5s$j8|-AlkQY76V#&3JU@(#@RO3|I0eGM!$I0!6f(aG-w=F zXc8#v#M%(72ehYz1YMZ?4>Hm$*{+A2#u?-s1DVX)Yqu&=_Vl19c*ue$>buby7R%L$ zl*O2myh+gN@K* zi9e1x_HyfpRV!2NA)yKJ!<)0S`0wh1@KSg}xd9rL8}H z>-=zxp)IRSbQvZ4OF&~o5?vN56ETDAxHsJ@gQ8-m2E&< z&K$BWBF5l#m9*7PbTYHY7JC)BHi&@+_(mF=@zb6YdH!u0W%4dG{%c{`(RzDYFmhJ< zkND#p>t_=R{W~m%-1~T6oTUX2jrKbdEKA--Z#kT4RZX+-F zp!Pg8@~iCPZD0mE4gj}=&9tdGQgXJaBrt$s#TVjuoBo$l8BK}S8*1+%{c(&aS6xr~ z=x)_@P46Z!PX`LF&+pl}c_Akgsh9RAO^}+L6V|?j`GL5+@-zA0U*CC1d(@M_a|l{O1ddh4rUj4K4D2fQy4Eu6WG z-QaK6?(7$*H%tT|tKd0`7UmA~nQ?8*}+)(^{X`PlwiS6p_A;de65j9^(N z*1z-2+m2?uU>lmT^2;QC|JuzSM7uZOoDpEz9ZdeMtWhIHn2av|mssIK4^)7Gxq>WE z`^7Paoj?=lC2UpwRCLsdhKw0}enBrDgucsH1w zzOT=}w%h&3RU#yiK0fzeGr#-yHh+a7DXE*4uZi|Mhd6+Yd4f(Ir?9PU0cG52AoD++ ztTyvYt`cTeyxM8mO)$?doqvpHSxo{$#dsN8)1Zn@ZYn*tX{%vs;F2Xa0EBkcN&ZMjjq0y^eS zwZ_mCp5t$1)~F)|k(|`BbnxVuZlmGhK}ov@JBzWE>&0fFtfL3%ij-nKr~URe2@)K4 z1pFV-+isTE`y6v%n&Te1tSXXWFz;u*;zQ5$Nl$s{{} z_mRLN#kP4TrEWzq@?%$ZqK-t}kdt<>AipTrOh&@Q(V#H@4bvN;q%KEP z&zd)4%U%!=uG=EndgqoX!qQhsPbKH|^B1Fi!ENpe1Ck~_^`FF-H5mpMBhFQ#+o)A`rbdflJneu;#`k+zq@?*yFlsZf&3vZa zw>1dTnkLtifNB!hTgO>?0yQt*0y{odyIRc1ufeSGKMvU}2`~8i76{)t4ak=Xje+;$ z?y{Ls^?iT9J8PBf(7V|Wr_U*}^Qc^hf|y~E_U^qtqU}@ar!)z4#(qrBky;d}v4S(J z@#>=5(qSqi0d#k`{tln)=u$l@sH&vgDQEC`89dVP6$OI!#v+ed#P7TU9V@jg=-@xI zum26`(Jm+|7QXDBZ;NK`97X{h4s3)hxnWE{eK~s2=Eh__Lw{aDEdNP-tu? zMp1DrO2Im4qcf^LCR0>xoh=jkLJ{3;MAH9GOTsI`Hh}0S1aJMPQHJj!17X|&*s^$r zS^PlFvL2%eQR_;LGM@W6e`zb@BVwwEO4^jfX5P-IckE839eX%zVRFj~SHj1j6EWqN z|IDmS@+wk(xN-7cgFq|aR@Orkz5)f01a2+QbZSd6F?M*%=mg$_xMM8wYvKn4-1w4a z)a%5fwcxeB4zam8Oy%a{LU>V{f0M`V2D}xH$BHq@u>p4#Y}J9|{ZVSI5Tz>i0rv>P zR2@4}`19|z^7cjW@72HELDj^^z}7b&hFb%U4o}YZ;{gCC_;E# zl*3(iG)Cv6yDx9FH}=%rND~ksvK-cBW8o8=0(TP)E-W@sMK?I+uX*3AjyF~7Bw5nB zBQ~+sM9eL7-&So!hAF*QO_9}Yxr>~wy+2zPdn?sEZZ^t1>xOHG^SFtMv&d%$v04Ac zc9uv(Hr-pvf?d9cAwMb{&54;i=@GFscPU5u>g@{(--sl~2eD2$-o?PMEvd%UMp?hg zlzP@u8F4$l`b!B#GnMl*DXLouWmXIXPb-i*#${I(yV{?5zD}gvDz9~&eC6G^G~DE? z>Ph`c2lCpgaHoUmg7HojEYs`CcvRwcAXi9<2xuo0JQ4vkA~>>-u&!B1+z;P1rkBmI zKfjoS?R}?ZmAp0^qpjnZ1cI{8L%sQHHR~gG$3Hm%G^0Y=dzmx~=l!c`-{ieTs2eVo zb7?Hp3G!re;?SN9ZJRAP2Qtk|*Pq5TzOO0CaB}Yw9J4W}50&KMQ(}PD?-KzF>CQH6 zGOG1}a*5vy>5-8kwMoAM&zhz<0!mMJJvpcR?d%l?U>YCAY^(;1jz-WA)w6$AL|%7_ zq!u0k_hrZpPcm7Uc(^zCqfe6XN(nJ+TD~=o@Jm!}d`H`IS7(=OW>j~e_W`(3WwXmY zF8qg?0pbPGu-e54(Qr}0B-{P=Uhnt*Jg*mgtN2>|rQYn|0@uNIsrb0$g2UAPB4}n! zjh(etd1FfnA7^<;RrFl{NDY>(vG;_X>2IJ&vjI3 zu6uJOY&Y;VT59#B#NsEoD15+CTHMW5(poeR`j%d~eE-$HzOHs!N?g4*YbfS_$yj!x z!!&I4DQ1Lw-Xm=?@O*BtX3{|=nz{h)js(bnreWK~3oXc#=}dM`)U zskUGC0h1>kS5a~WT~!Ei^^WH8+%FY;o44amyZgG9&Pw%t>R)G= zt5am3KR;u{?$!PhtJtGX(Fv&+_lcz%KVlYsuGYstzqHa?W@&>N^s8a~3 z#z%@n^JnH>%)M9@<>0|Njy}X`Gve(YZ*TwBlcWOg(I=DQ=@(@~))5m&_2jUvtYj(~ zhYL=E-$>2CSXr`sZx5@9B8`qtF*>>=i0a)80mU=w>89HqrcQymFS=^u#faie{bE;I zR9*ZWitG8nO3tbnJNLC}X9_1kyM~aZmDqafbSjY3;G#m7zlVzEJAx_A*wHO5`J-}4 zM7GVrJ8aYWA0AfE7agJkNbcL(e#@vj8@;sa`}VxV*FBbs5LUZ~AQ?zlZXHTu1g#9_ zk$g~xVa``UK=YT9*c|Uh);@pb!t?n977jq)=LT18P5%>lE&WPt6bIOLjS=x%k9J}z z-FbBJ^AG-Vpo?zBSj5Y1a1BFz&+67lC)K<53AtKmvo`;#1rjm5+bXSlh{-Ffsbrw>1P_PUDXSxD@K{tk?*%aL1cUk(E<5 z%jC%Xcp*);+uG|L6r1ZbSoY~Trl2M9(V#J3K7+f*NYmMgY==YOHHKJz+u8`1%iEhsKXzB&v6CY5 z&EJu|Ij6GrS&|R;qprg1f#+e>nTfW)B(D;<6d**1b@TyqrCY;p(8`dp8C7>!#0L|V zTrR&K*FfVi6mn4)g`@F{yYz401W&<*{3HN#J`g;>P$xGwiUo-n#y3&~7n}nDzQjoRJf+Qiu<@ym%INok8zO z)9-yoCLGI@5wTRFAV!=Z!F~%R=#X>k*t>1)_n~aockCRl#^cExb9}!^^Hyj}qUN}V zTa=Db^WnR(Ya6uSv%>+r*zZtvPUjRgQ%g{u2GwYrF+_@4_(5wQ=`$#=&co0@s{D~S!u&)FS`r;!qwnmI~-~O zSzppOn}fQzhP2&CaQ8*Tc3TaQ*`Q(|;2AC9YL6p2BpmE2d%y=>A+0<3;K+DJ@Ik7z z39G|&N6~DdG53??MQ%9xyuF8ViiIF*=X1l)3!-GvUJ>5>knNQ;Vt@S#JR1-dRUGod z-n!!MXyBNE;B;A#Hh25%Nd{a6v=B^3z!n6>f-sHZixEjuF7 zTRRSrBHv?aFfSNNwy$^jgmcByeeubSF4A=s%b`ZaNgK@AbTfoTXF!6NUUaFtWLKK% zK@FA=%UJdUGKY^UO{7OuXl0S`>G-PLu1qlfI$tVcfbtBnZZ2|ih^gnQOCdGM zWc#d(L#8ukVNAbt-6pvwZh`#g$sbxVR#L?#P=;So93>g@72*ZRyfN*PZC#RZuQp`7 zk&yK8NL+7TF;PGW_M_nk{%0qVKd=Jqz^L568d*?n0c8^@C*PR3g{%gf<|XpW*j2C#yMw{qM)6ZToLT?6@-4 zuEE;jv2xRC-a)T&y2jN(zda$&h0S`Ez!L-Zo2GK`d^=9WN*Rq_1CQ)M%XF;@lQw4a z@bt((!P`Km2?i?pb}j7v;bSYrhq?$cFA6<#rY<12OJ#j(~ciBN;GG%0Xk2k7u`5jMY@$>azu{&?8*Bc#8V5p|u}{*b44249tNgL%p7hdosO{Lr1Df8Gtc;hx zgr3Tfd}CqplIbvzn|9{=2qpBb~mZ_#=gxBh_vG=D2W6^3Qyrv;C9M9MZ$+ z6M+KiOA+|Z8!;(V*K=+vmVX)V=5S>^to-5DfS&01dmCvMXtLuqg2QffL^e2)1wAe%nk(kB4@|DeXMPn|2s z7}bIV?0DvJBdN`J{F!1nn(dPs9@X|g5*v+w)C)HhILgnuN3*y19RPsex^Qu23l=j7_ThaZBOIvjgF0o z##NWN9^c8oqu10wFmHurk%ZK#uh^eesG z@jf2UF-BMJ*>)mX%@d?kXi}NTf+gr5!;n@@Eu9@O%NK(tg>Emw|PXp+T&FZ=2Y|4F4SX@#W zahAZUAw-U@Vh)l78Ybl${3_k)TMS=PzMZif@V}&K(N&;~uGmEAZRmIu#((}%3Y|nS zevzJ`cTqeT^oq)fwjaCe-Vn9^NFHdPek$aAZ9R>FpKMsTVvSA8YFRdsXHP+TuHaRa z$xD!KmBeiNbI~!@BQRu*@-)j%u`ryMbNka0N>Nw)dvv}ThOTt`Q0#0NhQ^W*1)y7*Nn^z!UGz&2XclVGN7qQNG&L+ zyD1BsXK0&W*+3wMVKPRcQGUqqwwE%I>#iU-O8IVt;mTx7G8Jc#bw`wM?;e@~93hyb zFv`CjlE7o)S0TE{V!5_@@l-o#uOT`doG+xCLa_ua)C-`wO3Y`e{$A_{-k0rMc4x(aJ>?dc)vQc%BoZ%B6h5mj+CHzz;Nr?Tx{1MgT4>99kY zq25W8BD6W`0Ylw*|74m_;dJ#v$=YJ)ZKAX0txgU7Ib8ndi%l-3*2?U(fX?{012!73 z-aG1*mlns_8#hTQ){7ddT15=NqnS(ow$|HC+_*=qn)(dLK&@GISU3Y=SGwO83eNRe`ct z(n71*LJIX!J!tDe;`(FSB<#6cF~m-WX%59%lDiqRj`>W|Dcp!wrsbyhC#FH~U-usRUu z0&=+JxCyZMSj9IM;wH5~mw3-rtr`OP&Iv%3`e z*>z~U;Dh4F$vol<71gz7D6Q$jLvx#H(*u(QCM&9KMw_ykgzXU=^pI-+Vo+wPq)8pZ z`iOFg*>jlw;^AD3pHff&|3q~3T|PfrS}`vlp^CMc%RhhlNudi6H=5H%1p3~lD3xDD z^p8~oYcmHTLwknO2^u3VsI^%OR#G%|w#h(Pn;bAIaPXPXNu|r%6&0apfSmJX@-Hy5 zww1gGx|k!K0} result = patientStickerController.getPatientIdSticker(response, request, TEST_PATIENT_UUID, false); + ResponseEntity result = patientStickerController.getPatientIdSticker(response, TEST_PATIENT_UUID, false); byte[] pdfContent = result.getBody(); assertNotNull(pdfContent); @@ -94,9 +92,8 @@ public void getPatientIdSticker_shouldReturnValidPdfForEnglishLocale() throws Ex public void getPatientIdSticker_shouldReturnValidPdfForArabicLocale() throws Exception { Context.setLocale(new Locale("ar", "AR")); MockHttpServletResponse response = new MockHttpServletResponse(); - MockHttpServletRequest request = new MockHttpServletRequest(); - ResponseEntity result = patientStickerController.getPatientIdSticker(response, request, TEST_PATIENT_UUID, false); + ResponseEntity result = patientStickerController.getPatientIdSticker(response, TEST_PATIENT_UUID, false); byte[] pdfContent = result.getBody(); assertNotNull(pdfContent); @@ -117,11 +114,10 @@ public void getPatientIdSticker_shouldReturnValidPdfForArabicLocale() throws Exc @Test public void getPatientIdSticker_shouldReturn404ForInvalidPatient() throws Exception { MockHttpServletResponse response = new MockHttpServletResponse(); - MockHttpServletRequest request = new MockHttpServletRequest(); String invalidUuid = "invalid-uuid"; - ResponseEntity responseEntity = patientStickerController.getPatientIdSticker(response, request, invalidUuid, false); + ResponseEntity responseEntity = patientStickerController.getPatientIdSticker(response, invalidUuid, false); assertNull("Response entity should be null", responseEntity); assertEquals("Should return HTTP 404 status", HttpStatus.NOT_FOUND.value(), response.getStatus()); diff --git a/readme/PatientIdSticker.md b/readme/PatientIdSticker.md index d40cf92..f0732b9 100644 --- a/readme/PatientIdSticker.md +++ b/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 displayed on the sticker (must be a path relative to the OpenMRS application data directory) | +| `report.patientIdSticker.logourl` | String | Logo path displayed on the sticker (both relative and absolute paths are supported, but absolute paths must be within the 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 | @@ -94,5 +94,5 @@ The module supports internationalization through message properties. Field label - Available stylesheets include `patientIdStickerFopStylesheet.xsl` (default) and `msfStickerFopStylesheet.xsl` for MSF-specific layouts. - Logo resolution rules: - `report.patientIdSticker.logourl` should be a path relative to the`OPENMRS_APPLICATION_DATA_DIRECTORY` (e.g., `branding/my_custom_logo.png`). - - If no logo is configured or the configured logo is unavailable, the OpenMRS logo will be used as a default (requires the legacyui module). + - If no logo is configured or the configured logo is unavailable, the OpenMRS logo will be used as a default. - The barcode is generated from the preferred patient identifier when barcode is enabled. \ No newline at end of file diff --git a/readme/PatientIdStickerXSL.md b/readme/PatientIdStickerXSL.md index c72fe20..6688a0d 100644 --- a/readme/PatientIdStickerXSL.md +++ b/readme/PatientIdStickerXSL.md @@ -165,8 +165,8 @@ The optional header section can contain: - Custom header text on the right - Logo handling behavior: - 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. + - If none configured or missing, the default OpenMRS logo is loaded from the classpath + - Supported formats: PNG only. ### Internationalization Section @@ -278,6 +278,6 @@ The stylesheet includes several responsive design elements: - Configuration is managed through the Initializer module - Field visibility is controlled by boolean configuration properties - Secondary identifier type is specified by UUID in configuration -- Logo input is either an absolute filesystem path or a base64-encoded data URI (e.g., `data:image/png;base64,...`). Relative paths are resolved under `OPENMRS_APPLICATION_DATA_DIRECTORY`. +- Logo input is an absolute or relative filesystem path resolved under `OPENMRS_APPLICATION_DATA_DIRECTORY`. - Barcode generation uses the preferred patient identifier - Multiple stickers can be generated based on the `pages` configuration From 5bd3ed8fffff47df7f2c7b0fdbd0ac6857667551 Mon Sep 17 00:00:00 2001 From: jnsereko <58003327+jnsereko@users.noreply.github.com> Date: Wed, 19 Nov 2025 09:03:03 +0300 Subject: [PATCH 23/25] Remove check for application data directory existence. (it can never be null)) Removed null check for application data directory. --- .../renderer/PatientIdStickerXmlReportRenderer.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java index 5098b34..77d269a 100644 --- a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java +++ b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java @@ -326,11 +326,6 @@ protected File resolveSecureLogoPath(String logoUrlPath) { } 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); From b49042025884c9966c5724b96f671b29ad90ed66 Mon Sep 17 00:00:00 2001 From: jnsereko Date: Wed, 19 Nov 2025 15:50:21 +0300 Subject: [PATCH 24/25] Reject absolute Paths --- .../renderer/PatientIdStickerXmlReportRenderer.java | 12 ++++-------- .../PatientIdStickerXmlReportRendererTest.java | 6 +++--- readme/PatientIdSticker.md | 2 +- readme/PatientIdStickerXSL.md | 2 +- 4 files changed, 9 insertions(+), 13 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java index 77d269a..1f67eec 100644 --- a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java +++ b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java @@ -253,7 +253,7 @@ private void configureHeader(Document doc, Element templatePIDElement) { /** * Configures the logo for the sticker document. * - * Loads a custom logo from {@code logoUrlPath} (absolute or relative to the {@code OPENMRS_APPLICATION_DATA_DIRECTORY}. + * Loads a custom logo from {@code logoUrlPath} (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 @@ -330,14 +330,10 @@ protected File resolveSecureLogoPath(String logoUrlPath) { final Path appDataPath = appDataDir.toPath().toRealPath(); final Path logoPath = Paths.get(logoUrlPath); - // For absolute paths, verify they're within app data directory + // Reject absolute paths 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(); + log.error("Absolute paths are not allowed for logo files: {}", logoUrlPath); + return null; } // For relative paths, detect path traversal by comparing absolute and normalized paths diff --git a/api/src/test/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRendererTest.java b/api/src/test/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRendererTest.java index 886cef4..a72a037 100644 --- a/api/src/test/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRendererTest.java +++ b/api/src/test/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRendererTest.java @@ -66,13 +66,13 @@ public void resolveSecureLogoPath_shouldRejectPathTraversalAttempts() throws Exc } @Test - public void resolveSecureLogoPath_shouldRejectAbsolutePathsOutsideAppDataDirectory() throws Exception { + public void resolveSecureLogoPath_shouldRejectAbsolutePaths() throws Exception { PatientIdStickerXmlReportRenderer renderer = new PatientIdStickerXmlReportRenderer(); - Path outsideLogo = Files.createTempFile("outside-data-dir-logo", ".png"); + Path outsideLogo = Files.createTempFile("absolute-path-logo", ".png"); File resolvedLogoFile = renderer.resolveSecureLogoPath(outsideLogo.toString()); - Assertions.assertNull(resolvedLogoFile, "Absolute paths outside the app data directory must be rejected"); + Assertions.assertNull(resolvedLogoFile, "Absolute paths must be rejected"); Files.deleteIfExists(outsideLogo); } } diff --git a/readme/PatientIdSticker.md b/readme/PatientIdSticker.md index f0732b9..f5c2297 100644 --- a/readme/PatientIdSticker.md +++ b/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 displayed on the sticker (both relative and absolute paths are supported, but absolute paths must be within the application data directory.) | +| `report.patientIdSticker.logourl` | String | Logo path displayed on the sticker (relative must be within the 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 | diff --git a/readme/PatientIdStickerXSL.md b/readme/PatientIdStickerXSL.md index 6688a0d..5913d66 100644 --- a/readme/PatientIdStickerXSL.md +++ b/readme/PatientIdStickerXSL.md @@ -278,6 +278,6 @@ The stylesheet includes several responsive design elements: - Configuration is managed through the Initializer module - Field visibility is controlled by boolean configuration properties - Secondary identifier type is specified by UUID in configuration -- Logo input is an absolute or relative filesystem path resolved under `OPENMRS_APPLICATION_DATA_DIRECTORY`. +- Logo input is a relative filesystem path resolved under `OPENMRS_APPLICATION_DATA_DIRECTORY`. - Barcode generation uses the preferred patient identifier - Multiple stickers can be generated based on the `pages` configuration From c48e4937954b2ad2cf34b07e968e49ed10d5368f Mon Sep 17 00:00:00 2001 From: jnsereko <58003327+jnsereko@users.noreply.github.com> Date: Wed, 19 Nov 2025 17:07:45 +0300 Subject: [PATCH 25/25] Update error message for logo path validation --- .../renderer/PatientIdStickerXmlReportRenderer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java index 1f67eec..928d389 100644 --- a/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java +++ b/api/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java @@ -350,7 +350,7 @@ protected File resolveSecureLogoPath(String logoUrlPath) { final Path resolvedLogoRealPath = resolvedLogoPath.toRealPath(); if (!isPathWithinAppDataDirectory(resolvedLogoRealPath, appDataPath)) { - log.error("Logo path escapes application data directory: {}", logoUrlPath); + log.error("Logo path must be within the application data directory: {}", logoUrlPath); return null; }