TRUNK-6587: Add unit tests for ModuleResourcesServlet#6103
Conversation
Adds ModuleResourcesServletTest covering every branch of
ModuleResourcesServlet.getFile(), the request-path-to-File resolver
that backs /WEB-INF/view/module/<module>/resources/... lookups. The
class previously had zero test coverage despite holding all of the
servlet's security-sensitive path-resolution logic.
While writing the tests, two minor security gaps in getFile() became
visible. Both were already listed as expected behaviour in the JIRA
ticket but neither was actually enforced in code:
1. A null request path NPE'd inside ModuleUtil.getModuleForPath
instead of returning null. That surfaces to the caller as a 500
rather than a 404 on bad input.
2. A null byte injected into the path was not stripped before being
passed to Path.of, which throws InvalidPathException. Null bytes
in web paths are a well-known poison-NUL pattern: some downstream
APIs treat the byte as a string terminator and bypass suffix-based
checks.
Both are now rejected up front with a WARN log line, matching the
behaviour the ticket described. A malformed path (one that ModuleUtil
rejects with IllegalArgumentException) is also caught and converted
into a null return for the same reason.
New tests cover:
- null path -> null
- null-byte injection -> null (verifies module lookup is skipped)
- malformed path "/" -> null
- no module matches path -> null
- directory traversal in production mode -> null
- directory traversal in dev mode -> null
- missing file on disk -> null
- production-mode resolution from <webappRoot>/WEB-INF/view/...
- dev-mode resolution from <devDir>/omod/target/classes/web/...
📝 WalkthroughWalkthroughThis PR hardens module resource serving by validating request paths before module resolution. The servlet now rejects null paths, embedded null bytes, and malformed paths upfront; it also catches and logs exceptions from failed module lookups rather than allowing them to propagate. A comprehensive test suite validates both rejection and successful resolution paths. ChangesModule Resource Path Security
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PMD (7.24.0)web/src/test/java/org/openmrs/module/web/ModuleResourcesServletTest.java[ERROR] Cannot load ruleset rulesets/java/android.xml/DoNotHardCodeSDCard: Cannot resolve rule/ruleset reference 'rulesets/java/android.xml/DoNotHardCodeSDCard'. Make sure the resource is a valid file or URL and is on the CLASSPATH. Use --debug (or a fine log level) to see the current classpath. web/src/main/java/org/openmrs/module/web/ModuleResourcesServlet.java[ERROR] Cannot load ruleset rulesets/java/android.xml/DoNotHardCodeSDCard: Cannot resolve rule/ruleset reference 'rulesets/java/android.xml/DoNotHardCodeSDCard'. Make sure the resource is a valid file or URL and is on the CLASSPATH. Use --debug (or a fine log level) to see the current classpath. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/main/java/org/openmrs/module/web/ModuleResourcesServlet.java (1)
118-120:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle malformed filesystem paths to prevent 500 errors during normalization.
Your new guard at line 94 catches module-path lookup failures early—that's good! However,
Path.of(realPath)andPath.of(basePath)at lines 118–119 can still throwInvalidPathExceptionif the filesystem path contains invalid characters (e.g., null bytes, control characters, or OS-specific invalid chars). This exception isn't caught, so a malformed input returns a 500 instead of a cleannullrejection.Wrap the normalization in the same try-catch pattern to keep the endpoint fail-closed:
Suggested fix
+import java.nio.file.InvalidPathException; import java.nio.file.Path; @@ - Path normalizedPath = Path.of(realPath).normalize(); - Path normalizedBase = Path.of(basePath).normalize(); + Path normalizedPath; + Path normalizedBase; + try { + normalizedPath = Path.of(realPath).normalize(); + normalizedBase = Path.of(basePath).normalize(); + } + catch (InvalidPathException ex) { + log.warn("Rejected request with invalid filesystem path: " + path); + return null; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/main/java/org/openmrs/module/web/ModuleResourcesServlet.java` around lines 118 - 120, Wrap the Path.of(...).normalize() calls in ModuleResourcesServlet in a try-catch for InvalidPathException so malformed filesystem paths do not throw a 500; specifically, surround the creation/normalization of normalizedPath and normalizedBase (the Path.of(realPath).normalize() and Path.of(basePath).normalize() expressions) with a try { ... } catch (InvalidPathException e) { return null; } or equivalent fail-closed behavior, and ensure any logging includes the exception so the method cleanly returns null instead of propagating the exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@web/src/main/java/org/openmrs/module/web/ModuleResourcesServlet.java`:
- Around line 118-120: Wrap the Path.of(...).normalize() calls in
ModuleResourcesServlet in a try-catch for InvalidPathException so malformed
filesystem paths do not throw a 500; specifically, surround the
creation/normalization of normalizedPath and normalizedBase (the
Path.of(realPath).normalize() and Path.of(basePath).normalize() expressions)
with a try { ... } catch (InvalidPathException e) { return null; } or equivalent
fail-closed behavior, and ensure any logging includes the exception so the
method cleanly returns null instead of propagating the exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5cf0b0fb-76e3-4165-995d-a11a83c7d250
📒 Files selected for processing (2)
web/src/main/java/org/openmrs/module/web/ModuleResourcesServlet.javaweb/src/test/java/org/openmrs/module/web/ModuleResourcesServletTest.java
|
1 participant
Description of what I changed
Adds
ModuleResourcesServletTestcovering every branch ofModuleResourcesServlet.getFile()— the request-path-to-Fileresolver thatbacks
/WEB-INF/view/module/<module>/resources/...lookups. The classpreviously had zero test coverage despite holding all of the servlet's
security-sensitive path-resolution logic.
While writing the tests, two minor security gaps in
getFile()surfaced.Both were already described in the JIRA ticket as expected behaviour but
neither was actually enforced in code:
nullrequest path NPE'd insideModuleUtil.getModuleForPathinsteadof returning
null. That surfaces to the caller as a 500 rather than a404 on bad input.
%00) inside the path was not strippedbefore being passed to
Path.of, which throwsInvalidPathException.Null bytes in web paths are a well-known poison-NUL pattern — some
downstream APIs treat the byte as a string terminator, allowing
suffix-based checks to be bypassed.
Both are now rejected up front, with a
WARNlog line, matching thebehaviour the ticket described. A malformed path (one that
ModuleUtilrejects with
IllegalArgumentException) is also caught and converted intoa
nullreturn for the same reason.Production change
Test class
ModuleResourcesServletTestuses JUnit Jupiter + Mockito 5MockedStaticto stub
ModuleUtil, plus@TempDirto stage real files for thefilesystem-resolution paths. The 9 test methods cover, one per branch:
getFile_shouldReturnNullWhenRequestPathIsNullgetFile_shouldReturnNullWhenPathContainsANullBytegetFile_shouldReturnNullForMalformedPathWithoutLeadingModuleSegmentgetFile_shouldReturnNullWhenNoModuleHandlesThePathgetFile_shouldRejectPathTraversalAttemptsgetFile_shouldRejectTraversalEvenInDevelopmentModegetFile_shouldReturnNullWhenResolvedFileDoesNotExistgetFile_shouldResolveFromWebappRootInProductionModegetFile_shouldResolveFromDevelopmentDirectoryWhenDevModeIsActiveVerified with:
Issue I worked on
see https://openmrs.atlassian.net/browse/TRUNK-6587
Checklist: I completed these to help reviewers :)
mvn clean packageright before creating this pull request and added all formatting changes to my commit.Summary by CodeRabbit
Bug Fixes
Tests