(feat) O3-5186: Add security safeguards for logo path configuration by jnsereko · Pull Request #5 · openmrs/openmrs-module-patientdocuments

jnsereko · 2025-11-05T14:01:46Z

Description

This PR implements security protections for the logo path configuration feature to prevent path traversal attacks.
The changes restrict logo file access to only the OpenMRS application data directory, addressing security concerns that were blocking O3 inclusion.

Screenshot

Related issue

https://openmrs.atlassian.net/browse/O3-5186

…raversal attacks

ibacher

Thanks @jnsereko. While most of this is nitpicking, it's as well to keep our documentation tightly focused. Parts of this look LLM-generated, which is perfectly fine, but it's always important to be reviewing the LLM's work and making sure it conforms to our standards, that it doesn't over-document just for the specific task that it's working on (so that our documentation remains clean and readable and primarily aimed at the concerns of people calling the API).

Finally, some actual tests would be very valuable here.

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

ibacher · 2025-11-06T13:29:22Z

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

+	 * 
+	 * Logo resolution priority:
+	 * 1. Custom logo from path resolved under {@code OPENMRS_APPLICATION_DATA_DIRECTORY}
+	 * 2. Default OpenMRS logo as base64 data URI


Does this code support this?

Yes @ibacher. The code supports the above comments as the logic added with #3. It just changed as i used LLM to improve documentation for me.

What I mean is we encode the data as a base64 data URL, but that's not something that the caller can supply in logoUrlPath, right? The point of Javadoc comments is to explain to the caller how to use a method.

Here our logo resolution algortihm now looks like:

We try to resolve the logoUrlPath to a path relative to the Application Data Directory. If this path cannot be resolved to a file, we fallback to using the bytes passed into the defaultLogoBytes parameter as the logo.

The caller can supply a byte[] representing a default document, but not a base64 data URI.

I'm also wondering now why we're passing around this byte[] for the default logo? It's passed as an argument here, in the caller (configureHeader()) and it's caller (render()). The issue here is that images are relatively large (a little over 8kB in this case) and arguments to functions are persisted on the Java stack, so we're swallowing ~25kB of memory to pass around data that might be used. It makes more sense, I think, for configureLogo() to load the default logo from the classpath if the logoUrlPath doesn't resolve properly.

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

readme/PatientIdSticker.md

readme/PatientIdStickerXSL.md

Co-authored-by: Ian <[email protected]>

…/PatientIdStickerXmlReportRenderer.java Co-authored-by: Ian <[email protected]>

jnsereko · 2025-11-11T16:50:27Z

Thank you @ibacher @dkayiwa
I have cleaned LLM generated documentation, removed hard coded regex and added tests.
Kindly review when you get some time.

denniskigen · 2025-11-13T12:57:12Z

Ping

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

dkayiwa · 2025-11-13T13:21:08Z

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

+			if (logoPath.isAbsolute()) {
+				final Path logoRealPath = logoPath.toRealPath();
+				if (!isPathWithinAppDataDirectory(logoRealPath, appDataPath)) {
+					throw new APIException("Absolute path must be within application data directory: " + logoUrlPath);


How can an absolute path be within the application data directory?

Absolute paths can be within the application data directory eg

Right use

App Data Directory: /openmrs/data/

User provides: /openmrs/data/logos/company-logo.png

This is an absolute path that IS within our app directory ✅

Violation

App Data Directory: /openmrs/data/

User provides: /etc/passwd

This is an absolute path that IS NOT within our app directory ❌

I think that just makes you write more code than you should. Just accept only relative paths within the application data directory. Wouldn't you end up with the same functionality but with less code? 😊

I think that just makes you write more code than you should. Just accept only relative paths within the application data directory. Wouldn't you end up with the same functionality but with less code? 😊

cc @ibacher

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

dkayiwa · 2025-11-13T13:57:00Z

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

+
+			return resolvedLogoRealPath.toFile();
+		} catch (IllegalArgumentException e) {
+			throw new APIException("Invalid characters in logo path: " + logoUrlPath, e);


Is there a possibility of the IllegalArgumentException being thrown while the problem is not that of invalid characters?

Yes. Thats very true. I have generalized it to Invalid logo path: as I think having multiple checks might not be an overkill.

What if the log path is valid but the problem is something else?

Should i change the last catch to catch another error? In this case, null will be returned and the default OpenMRS Logo will be displayed. Something like below.

} catch (Exception e) { log.error("Failed to resolve logo path: {}", logoUrlPath, e); return null; }

If the problem is not with the path itself, then the error message can be misleading. In that case something as generic as i gave above would be better.

I think something like Failed to access logo file: {} on catching IOException should be better.

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

dkayiwa · 2025-11-14T13:02:49Z

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

 		}
+		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");


Is having a logo a requirement for printing these documents? In other words, is your plan to abort the printing if the only missing part is the logo?

Logically, each field should be optional so i think we should just log errors instead of throwing them and also basing on the fact that we have a fallback logo.

dkayiwa · 2025-11-14T13:10:35Z

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

+
+			return resolvedLogoRealPath.toFile();
+		} catch (IllegalArgumentException e) {
+			throw new APIException("Invalid logo path: " + logoUrlPath, e);


What criteria do you use to determine whether to throw an exception or log an error?

Since the logo is an option, like many other sticker configurations, i have just logged the caught errors.

ibacher

There's a bunch of comments here, but the real blocker is that the tests (it's great that we have them!) must pass on CI.

ibacher · 2025-11-14T15:50:38Z

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

+	 * 
+	 * Logo resolution priority:
+	 * 1. Custom logo from path resolved under {@code OPENMRS_APPLICATION_DATA_DIRECTORY}
+	 * 2. Default OpenMRS logo as base64 data URI


What I mean is we encode the data as a base64 data URL, but that's not something that the caller can supply in logoUrlPath, right? The point of Javadoc comments is to explain to the caller how to use a method.

Here our logo resolution algortihm now looks like:

We try to resolve the logoUrlPath to a path relative to the Application Data Directory. If this path cannot be resolved to a file, we fallback to using the bytes passed into the defaultLogoBytes parameter as the logo.

The caller can supply a byte[] representing a default document, but not a base64 data URI.

ibacher · 2025-11-14T15:57:49Z

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

+	 * 
+	 * Logo resolution priority:
+	 * 1. Custom logo from path resolved under {@code OPENMRS_APPLICATION_DATA_DIRECTORY}
+	 * 2. Default OpenMRS logo as base64 data URI


I'm also wondering now why we're passing around this byte[] for the default logo? It's passed as an argument here, in the caller (configureHeader()) and it's caller (render()). The issue here is that images are relatively large (a little over 8kB in this case) and arguments to functions are persisted on the Java stack, so we're swallowing ~25kB of memory to pass around data that might be used. It makes more sense, I think, for configureLogo() to load the default logo from the classpath if the logoUrlPath doesn't resolve properly.

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

readme/PatientIdStickerXSL.md

readme/PatientIdSticker.md

ibacher · 2025-11-14T16:11:43Z

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

+		if (isNotBlank(logoUrlPath)) {
+			File logoFile = resolveSecureLogoPath(logoUrlPath);
+			if (logoFile != null && logoFile.exists() && logoFile.canRead() && logoFile.isFile()) {
+				logoContent = logoFile.getAbsolutePath();


Wouldn't it be better to load the image here and convert it into a data URI, as we do for the default logo, rather than passing it as a URL and assuming it will be readable when rendered?

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

…/PatientIdStickerXmlReportRenderer.java Co-authored-by: Ian <[email protected]>

Co-authored-by: Ian <[email protected]>

…/PatientIdStickerXmlReportRenderer.java Co-authored-by: Ian <[email protected]>

Co-authored-by: Ian <[email protected]>

…tion

jnsereko · 2025-11-18T18:34:38Z

Thank you @dkayiwa and @ibacher for looking at this. I have fixed tests, improved documentation and loaded the OpenMRS from the classpath instead of Servlet context.
Kindly re-review.

dkayiwa · 2025-11-18T18:37:50Z

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java


+	private static final Logger log = LoggerFactory.getLogger(PatientIdStickerXmlReportRenderer.class);
+
+	private static final String DEFAULT_LOGO_CLASSPATH = "web/module/resources/openmrs_logo_white_large.png";


Are you still loading the logo from the module?

I thought that we had agreed not to duplicate this logo in the module.

Yes. We definitely had agreed that but i am out of options.

@ibacher I'm also wondering now why we're passing around this byte[] for the default logo? It's passed as an argument here, in the caller (configureHeader()) and it's caller (render()). The issue here is that images are relatively large (a little over 8kB in this case) and arguments to functions are persisted on the Java stack, so we're swallowing ~25kB of memory to pass around data that might be used. It makes more sense, I think, for configureLogo() to load the default logo from the classpath if the logoUrlPath doesn't resolve properly.

We had two options

Get OpenMRS logo from servlet context (only be one in controllers)

pass OpenMRS logo data from controllers to images (not recommended~ increasing memory in method calls) ❌

save as a temporary or permanent file (not recommended ~ duplicate file on disc) ❌

Using classpath

get OpenMRS logo from legacy UI resources OpenmrsClassLoader.getInstance().getResourceAsStream() (not working) ❌

duplicate OpenMRS logo in this module and access it using classpath (only option left) ✅

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

dkayiwa · 2025-11-18T18:54:23Z

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

+			final Path appDataPath = appDataDir.toPath().toRealPath();
+			final Path logoPath = Paths.get(logoUrlPath);
+
+			// For absolute paths, verify they're within app data directory


Didn't we agree to remove this?

@ibacher suggested that we can allow absolute paths so long as they are with in the application data directory.
something like /openmrs/data/custom_logo.png

I'm happy to leave this off, but we've had requests for supporting this for similar features where we've tried to lock-down where files are loaded from.

…be null)) Removed null check for application data directory.

jnsereko · 2025-11-19T09:18:52Z

hey @dkayiwa and @ibacher,
Could we handle logo retrieval performance and optimization in a separate Ticket that i have created here?
That case, we can keep this logic only focused on ensuring that the way we access the custom logo is secure?

dkayiwa · 2025-11-19T09:47:05Z

@jnsereko is there a reason why you are still accepting absolute paths for the logo? I thought we agreed to only allow relative paths.

jnsereko · 2025-11-19T12:04:22Z

@jnsereko is there a reason why you are still accepting absolute paths for the logo? I thought we agreed to only allow relative paths.

@ibacher had suggested that we can allow absolute paths as long as they are within the App data directory but let me remove that

jnsereko · 2025-11-19T12:58:24Z

Thank you @dkayiwa
I have rejected absolute paths from the logo configuration.

dkayiwa · 2025-11-19T13:08:15Z

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

+			final Path resolvedLogoRealPath = resolvedLogoPath.toRealPath();
+
+			if (!isPathWithinAppDataDirectory(resolvedLogoRealPath, appDataPath)) {
+				log.error("Logo path escapes application data directory: {}", logoUrlPath);


What does escapes mean?

I have changed the log to Logo path must be within the application data directory

Add security safeguards for logo path configuration to prevent path t…

fb99653

…raversal attacks

jnsereko marked this pull request as ready for review November 5, 2025 14:33

jnsereko requested review from dkayiwa and ibacher November 5, 2025 14:34

jnsereko mentioned this pull request Nov 5, 2025

Fix logo and address not showing in Sticker #3

Merged

ibacher requested changes Nov 6, 2025

View reviewed changes

jnsereko and others added 7 commits November 7, 2025 10:09

Update readme/PatientIdStickerXSL.md

9466efd

Co-authored-by: Ian <[email protected]>

Update readme/PatientIdStickerXSL.md

3a69ead

Co-authored-by: Ian <[email protected]>

Update readme/PatientIdStickerXSL.md

3393fcc

Co-authored-by: Ian <[email protected]>

Update readme/PatientIdSticker.md

c4ff685

Co-authored-by: Ian <[email protected]>

Update api/src/main/java/org/openmrs/module/patientdocuments/renderer…

ef44fbd

…/PatientIdStickerXmlReportRenderer.java Co-authored-by: Ian <[email protected]>

clean LLM documentation and remove hard-corded checks.

5e05736

clean LLM documentation and remove hard-corded checks.

665a16c

jnsereko requested a review from ibacher November 11, 2025 16:29

jnsereko added 2 commits November 11, 2025 19:34

catch APIException and not Security Exception

9e50b01

catch parent IllegalArgumentException and not child InvalidPathException

02a5c48

dkayiwa reviewed Nov 13, 2025

View reviewed changes

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java Outdated Show resolved Hide resolved

dkayiwa reviewed Nov 13, 2025

View reviewed changes

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java Outdated Show resolved Hide resolved

dkayiwa reviewed Nov 13, 2025

View reviewed changes

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java Outdated Show resolved Hide resolved

dkayiwa reviewed Nov 13, 2025

View reviewed changes

Improve error messages

eb27ff7

jnsereko requested a review from dkayiwa November 14, 2025 11:21

dkayiwa reviewed Nov 14, 2025

View reviewed changes

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java Outdated Show resolved Hide resolved

dkayiwa reviewed Nov 14, 2025

View reviewed changes

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java Outdated Show resolved Hide resolved

dkayiwa reviewed Nov 14, 2025

View reviewed changes

Just log caught errors instead of throwing them

0eae47e

jnsereko requested a review from dkayiwa November 14, 2025 14:34

ibacher reviewed Nov 14, 2025

View reviewed changes

jnsereko and others added 10 commits November 17, 2025 09:21

Update api/src/main/java/org/openmrs/module/patientdocuments/renderer…

b8a3258

…/PatientIdStickerXmlReportRenderer.java Co-authored-by: Ian <[email protected]>

Update api/src/main/java/org/openmrs/module/patientdocuments/renderer…

5a388fc

…/PatientIdStickerXmlReportRenderer.java Co-authored-by: Ian <[email protected]>

Update api/src/main/java/org/openmrs/module/patientdocuments/renderer…

ee8dd5d

…/PatientIdStickerXmlReportRenderer.java Co-authored-by: Ian <[email protected]>

Update api/src/main/java/org/openmrs/module/patientdocuments/renderer…

a51180a

…/PatientIdStickerXmlReportRenderer.java Co-authored-by: Ian <[email protected]>

Update readme/PatientIdStickerXSL.md

e170a55

Co-authored-by: Ian <[email protected]>

Update readme/PatientIdSticker.md

c2df6d9

Co-authored-by: Ian <[email protected]>

Update api/src/main/java/org/openmrs/module/patientdocuments/renderer…

ccf8592

…/PatientIdStickerXmlReportRenderer.java Co-authored-by: Ian <[email protected]>

Update readme/PatientIdSticker.md

174733a

Co-authored-by: Ian <[email protected]>

Update readme/PatientIdSticker.md

afe7272

Co-authored-by: Ian <[email protected]>

Convert OpenMRS logo from classpath into data URI and clean documenta…

6653ea0

…tion

jnsereko requested a review from ibacher November 18, 2025 18:31

dkayiwa reviewed Nov 18, 2025

View reviewed changes

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java Outdated Show resolved Hide resolved

dkayiwa reviewed Nov 18, 2025

View reviewed changes

Remove check for application data directory existence. (it can never …

5bd3ed8

…be null)) Removed null check for application data directory.

jnsereko requested a review from dkayiwa November 19, 2025 09:02

Reject absolute Paths

b490420

dkayiwa reviewed Nov 19, 2025

View reviewed changes

Update error message for logo path validation

c48e493

jnsereko requested a review from dkayiwa November 19, 2025 14:10

dkayiwa merged commit adcb9b0 into openmrs:main Nov 19, 2025
1 check passed


		private static final Logger log = LoggerFactory.getLogger(PatientIdStickerXmlReportRenderer.class);

		private static final String DEFAULT_LOGO_CLASSPATH = "web/module/resources/openmrs_logo_white_large.png";

Conversation

jnsereko commented Nov 5, 2025

Description

Screenshot

Related issue

Uh oh!

ibacher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnsereko commented Nov 11, 2025

Uh oh!

denniskigen commented Nov 13, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnsereko Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnsereko Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dkayiwa Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibacher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnsereko Nov 14, 2025 •

edited

Loading

jnsereko Nov 14, 2025 •

edited

Loading

dkayiwa Nov 14, 2025 •

edited

Loading