Fixed missing image on print card page#749
Merged
yingbull merged 4 commits intodevelop/dogfishfrom Oct 20, 2025
Merged
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR ensures the print card page always displays a clinic logo by loading the OpenOSP logo as a default resource and integrating it into the property lookup flow for CLINIC_LOGO_FILE, with a fallback when no custom logo is provided. Sequence diagram for clinic logo rendering with default fallbacksequenceDiagram
participant User
participant "ImageRenderingServlet"
participant "OscarProperties"
participant "File System"
User->>"ImageRenderingServlet": Request clinic logo
"ImageRenderingServlet"->>"OscarProperties": Get CLINIC_LOGO_FILE property (with default)
"OscarProperties"-->>"ImageRenderingServlet": Return filename or defaultClinicLogo
alt filename is null or empty
"ImageRenderingServlet"->>"File System": Use defaultClinicLogo
else filename exists
"ImageRenderingServlet"->>"File System": Use filename
end
"File System"-->>"ImageRenderingServlet": Return logo image
"ImageRenderingServlet"-->>User: Respond with logo image
Class diagram for ImageRenderingServlet logo rendering changesclassDiagram
class ImageRenderingServlet {
+renderClinicLogoStored(request, response)
-defaultClinicLogo : String
}
class OscarProperties {
+getProperty(key, defaultValue)
}
class FileSystem
ImageRenderingServlet --> OscarProperties : uses
ImageRenderingServlet --> FileSystem : reads logo file
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider loading the default logo resource from the classpath (e.g. via getResourceAsStream) instead of using a hard-coded filesystem path, which may not exist in packaged deployments.
- Mark DEFAULT_CLINIC_LOGO_FILE as static final to reflect that it’s a constant and adhere to naming conventions.
- Add a warning log or fallback behavior if neither the custom nor default logo file is found, to make missing images easier to diagnose.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider loading the default logo resource from the classpath (e.g. via getResourceAsStream) instead of using a hard-coded filesystem path, which may not exist in packaged deployments.
- Mark DEFAULT_CLINIC_LOGO_FILE as static final to reflect that it’s a constant and adhere to naming conventions.
- Add a warning log or fallback behavior if neither the custom nor default logo file is found, to make missing images easier to diagnose.
## Individual Comments
### Comment 1
<location> `src/main/java/ca/openosp/openo/ui/servlet/ImageRenderingServlet.java:67` </location>
<code_context>
local_client, hnr_client, integrator_client, signature_preview, signature_stored, clinic_logo
}
+ private static String DEFAULT_CLINIC_LOGO_FILE = "src/main/resources/loginResource/openosp_logo.png";
+
@Override
</code_context>
<issue_to_address>
**suggestion:** Consider using 'final' for the DEFAULT_CLINIC_LOGO_FILE constant.
Marking DEFAULT_CLINIC_LOGO_FILE as 'private static final' ensures immutability and protects against unintended changes.
```suggestion
private static final String DEFAULT_CLINIC_LOGO_FILE = "src/main/resources/loginResource/openosp_logo.png";
```
</issue_to_address>
### Comment 2
<location> `src/main/java/ca/openosp/openo/ui/servlet/ImageRenderingServlet.java:330` </location>
<code_context>
try {
- String filename = OscarProperties.getInstance().getProperty("CLINIC_LOGO_FILE");
+ String filename = OscarProperties.getInstance().getProperty("CLINIC_LOGO_FILE", DEFAULT_CLINIC_LOGO_FILE);
if (filename != null) {
File f = new File(filename);
</code_context>
<issue_to_address>
**suggestion:** Check for null or empty property values when using a default.
If getProperty returns "", the default won't be used. Consider treating empty strings as unset to ensure the default is applied.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/main/java/ca/openosp/openo/ui/servlet/ImageRenderingServlet.java
Outdated
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/ui/servlet/ImageRenderingServlet.java
Outdated
Show resolved
Hide resolved
…t.java Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
…with .getResource().getPath() through WEB-INF
Collaborator
Author
|
@sourcery-ai resolve |
Sourcery does not seem to be resolving the blocking issue, this was already explained to not be a security vulnerability in the comment to the change, will manually dismiss.
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Blocking issues:
- Detected a potential path traversal. A malicious actor could control the location of this file, to include going backwards in the directory with '../'. To address this, ensure that user-controlled variables in file paths are sanitized. You may also consider using a utility method such as org.apache.commons.io.FilenameUtils.getName(...) to only retrieve the file name from the path. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/main/java/ca/openosp/openo/ui/servlet/ImageRenderingServlet.java:340` </location>
<code_context>
File f = new File(filename);
</code_context>
<issue_to_address>
**security (java.lang.security.httpservlet-path-traversal):** Detected a potential path traversal. A malicious actor could control the location of this file, to include going backwards in the directory with '../'. To address this, ensure that user-controlled variables in file paths are sanitized. You may also consider using a utility method such as org.apache.commons.io.FilenameUtils.getName(...) to only retrieve the file name from the path.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Same false positive dismissed before
yingbull
approved these changes
Oct 20, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixed missing image on print card page by setting default image to openosp logo
Summary by Sourcery
Bug Fixes: