Updated and removed some dependencies for security purpose (develop/dogfish version)#494
Merged
yingbull merged 32 commits intodevelop/dogfishfrom Sep 2, 2025
Merged
Updated and removed some dependencies for security purpose (develop/dogfish version)#494yingbull merged 32 commits intodevelop/dogfishfrom
yingbull merged 32 commits intodevelop/dogfishfrom
Conversation
- Added commons-httpclient and jersey-client dependencies since these are not included in axis2 1.8.0
- Modified some relevant classes to make them fully compatible
- Replaced net.sf.json with jackson-databind in CaseloadContent2Action - Updated StringEscapeUtils usage Co-Authored-By: aider (deepseek/deepseek-chat) <aider@aider.chat>
This reverts commit b779114.
…ing 'Sign Save & Bill' icon in EChart -> encounter note
…Entry2Action.java
…arioMD, will need to review CodeQL fail on rebuild
…d, even though it should be
Reviewer's GuideThis PR systematically upgrades and secures project dependencies, refactors HTTP and XML handling to modern, safer libraries, adds directory-traversal protections on file operations, migrates JSON processing to Jackson, and sanitizes session attribute handling in key web actions. Sequence diagram for secure file validation in TeleplanResponse and TeleplanCodesManagersequenceDiagram
participant TeleplanResponse
participant OscarProperties
participant File
participant Security
TeleplanResponse->>OscarProperties: get DOCUMENT_DIR
TeleplanResponse->>File: get file path
TeleplanResponse->>Security: validate file exists and is within allowed directory
Security-->>TeleplanResponse: throw exception if invalid
TeleplanResponse->>File: proceed with file operation if valid
Sequence diagram for secure XML parsing in OntarioMD and PathNet::ConnectionsequenceDiagram
participant OntarioMD
participant SAXBuilder
participant Security
OntarioMD->>SAXBuilder: create SAXBuilder
OntarioMD->>SAXBuilder: set XXE protection features
OntarioMD->>SAXBuilder: parse XML InputStream
Security-->>OntarioMD: prevent XXE attacks
Sequence diagram for session attribute sanitization in CaseManagementEntry2ActionsequenceDiagram
actor User
participant CaseManagementEntry2Action
participant HttpSession
User->>CaseManagementEntry2Action: submit form with session attributes
CaseManagementEntry2Action->>HttpSession: check for null/empty values
CaseManagementEntry2Action->>HttpSession: set or remove attributes based on validation
HttpSession-->>CaseManagementEntry2Action: attributes updated
Class diagram for refactored TeleplanAPI and related classesclassDiagram
class TeleplanAPI {
- CloseableHttpClient httpclient
- HttpClientContext httpContext
+ changePassword(username, password, newPassword, confirmPassword)
+ login(username, password)
+ logoff()
+ getLog(logname, logtype)
+ getLogList()
+ getRemittance(includeRemittance)
+ getAsciiFile(filetype)
+ getAsciiFileMF(filetype)
+ putAsciiFile(File f)
+ putMSPFile(File f)
+ checkElig(phn, dateofbirthyyyy, dateofbirthmm, dateofbirthdd, dateofserviceyyyy, dateofservicemm, dateofservicedd, patientvisitcharge, lasteyeexam, patientrestriction)
}
class TeleplanResponse {
+ processResponseStream(InputStream in)
+ getFile()
+ getRealFilename()
}
class TeleplanResponseDAO {
+ save(TeleplanResponse tr)
}
TeleplanAPI --> TeleplanResponse
TeleplanAPI --> TeleplanResponseDAO
TeleplanResponse --> File
TeleplanResponseDAO --> TeleplanResponse
Class diagram for updated HTTP and OLISProtocolSocketFactory classesclassDiagram
class HTTP {
- String url
+ Get(queryString)
+ GetString(queryString)
}
class OLISProtocolSocketFactory {
+ OLISProtocolSocketFactory()
+ createSocket(host, port, localAddress, localPort, timeout)
}
OLISProtocolSocketFactory --|> SSLConnectionSocketFactory
Class diagram for updated OntarioMD class (secure XML parsing)classDiagram
class OntarioMD {
+ loginToOntarioMD(username, password, incomingRequestor)
- parseReturn(InputStream is)
}
Class diagram for updated Demographic2Action (Jackson migration)classDiagram
class Demographic2Action {
+ getAddressAndPhoneHistoryAsJson()
+ checkForDuplicates()
}
Class diagram for updated CaseManagementEntry2Action (session attribute sanitization)classDiagram
class CaseManagementEntry2Action {
+ edit()
+ issueNoteSaveJson()
+ issueNoteSave()
+ save()
+ ajaxsave()
+ releaseNoteLock()
+ addNewIssue()
+ issueList()
+ issueSearch()
+ makeIssue()
+ issueAdd()
+ changeDiagnosis()
+ submitChangeDiagnosis()
+ ajaxChangeDiagnosis()
+ issueDelete()
+ issueChange()
+ displayNotes()
+ doDisplayNotes()
+ print()
+ getUnlockedNotesMap()
+ insertReason()
+ convertDateFmt()
+ haveIssue(Long issid, List allNotes)
+ ticklerSaveNote()
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
LiamStanziani
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/main/java/ca/openosp/openo/billings/ca/bc/Teleplan/TeleplanCodesManager.java
Fixed
Show fixed
Hide fixed
src/main/java/ca/openosp/openo/billings/ca/bc/Teleplan/TeleplanCodesManager.java
Fixed
Show fixed
Hide fixed
src/main/java/ca/openosp/openo/billings/ca/bc/Teleplan/TeleplanResponse.java
Fixed
Show fixed
Hide fixed
src/main/java/ca/openosp/openo/billings/ca/bc/Teleplan/TeleplanResponse.java
Fixed
Show fixed
Hide fixed
src/main/java/ca/openosp/openo/billings/ca/bc/pageUtil/ManageTeleplan2Action.java
Fixed
Show fixed
Hide fixed
src/main/java/ca/openosp/openo/billings/ca/bc/pageUtil/ManageTeleplan2Action.java
Fixed
Show fixed
Hide fixed
src/main/java/ca/openosp/openo/billings/ca/bc/pageUtil/ManageTeleplan2Action.java
Fixed
Show fixed
Hide fixed
src/main/java/ca/openosp/openo/billings/ca/bc/pageUtil/ManageTeleplan2Action.java
Fixed
Show fixed
Hide fixed
src/main/java/ca/openosp/openo/billings/ca/bc/pageUtil/ManageTeleplan2Action.java
Fixed
Show fixed
Hide fixed
src/main/java/ca/openosp/openo/billings/ca/bc/pageUtil/ManageTeleplan2Action.java
Fixed
Show fixed
Hide fixed
Collaborator
Author
|
Should be ready to be reviewed, I am going to look into more testing of this branch and comparing it to develop/dogfish to ensure that no new errors are added in with this, if I find errors that are already in the base branch and aren't noted down, I will add them to this ticket for now: #477 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Use of StringEscapeUtils.escapeSql does not prevent SQL injection. (link)
General comments:
- I see repeated file‐path validation code in multiple classes—consider extracting that into a shared utility method to reduce duplication and simplify maintenance.
- In TeleplanAPI (and other HTTP helpers) you create a new default HttpClient per request and ignore the configured httpContext/cookieStore—refactor to reuse the same CloseableHttpClient and context so cookie/session settings and timeouts are consistently applied.
- The secure XML parser settings (disabling external entities, DTDs, etc.) are duplicated across handlers—extract a common factory or builder helper to centralize XXE protection configuration.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- I see repeated file‐path validation code in multiple classes—consider extracting that into a shared utility method to reduce duplication and simplify maintenance.
- In TeleplanAPI (and other HTTP helpers) you create a new default HttpClient per request and ignore the configured httpContext/cookieStore—refactor to reuse the same CloseableHttpClient and context so cookie/session settings and timeouts are consistently applied.
- The secure XML parser settings (disabling external entities, DTDs, etc.) are duplicated across handlers—extract a common factory or builder helper to centralize XXE protection configuration.
## Individual Comments
### Comment 1
<location> `src/main/java/ca/openosp/openo/billings/ca/bc/Teleplan/TeleplanAPI.java:120` </location>
<code_context>
- PostMethod post = new PostMethod(url);
- post.setRequestBody(data);
- httpclient.executeMethod(post);
+ try (CloseableHttpClient httpclient = HttpClients.createDefault()) {
+ HttpPost post = new HttpPost(url);
+ post.setEntity(new UrlEncodedFormEntity(data, "UTF-8"));
- InputStream in = post.getResponseBodyAsStream();
- log.debug("INPUT STREAM " + in + "\n");
+ try (CloseableHttpResponse response = httpclient.execute(post)) {
+ InputStream in = response.getEntity().getContent();
</code_context>
<issue_to_address>
Re-instantiating CloseableHttpClient in processRequest may break cookie/session handling.
Using a new CloseableHttpClient for each request prevents reuse of cookies and session state, which may disrupt authentication or session continuity. Please use the shared httpclient and httpContext fields from getClient() to maintain session consistency.
</issue_to_address>
### Comment 2
<location> `src/main/java/ca/openosp/openo/billings/ca/bc/Teleplan/TeleplanAPI.java:139` </location>
<code_context>
+ private TeleplanResponse processRequest(String url, Map<String, Object> parts) {
</code_context>
<issue_to_address>
Multipart handling in processRequest does not support all possible value types.
Currently, only File and String are processed; other types are ignored. Please validate input types or handle unsupported types explicitly to prevent silent failures.
</issue_to_address>
### Comment 3
<location> `src/main/java/ca/openosp/openo/lab/ca/bc/PathNet/Communication/HTTP.java:58` </location>
<code_context>
- InputStream response = method.getResponseBodyAsStream();
- method.releaseConnection();
- return response;
+ public InputStream Get(String queryString) throws IOException {
+ try (CloseableHttpClient client = HttpClients.createDefault()) {
+ URIBuilder uriBuilder = new URIBuilder(url);
+ uriBuilder.setCustomQuery(queryString);
+ HttpGet get = new HttpGet(uriBuilder.build());
+
+ CloseableHttpResponse response = client.execute(get);
+ HttpEntity entity = response.getEntity();
+
+ logger.error("Status code: " + response.getStatusLine().getStatusCode());
+
+ if (entity != null) {
+ return entity.getContent(); // caller must close this InputStream
+ } else {
</code_context>
<issue_to_address>
Returned InputStream from HttpClient must be closed by caller.
This design may cause resource leaks if callers forget to close the InputStream. Please either document this requirement clearly or refactor to ensure resources are released automatically.
</issue_to_address>
### Comment 4
<location> `src/main/java/ca/openosp/openo/utility/OntarioMD.java:101` </location>
<code_context>
private Hashtable parseReturn(InputStream is) {
Hashtable h = null;
try {
+ // Create secure SAXBuilder with XXE protection
+ SAXBuilder parser = new SAXBuilder();
+ // Disable external entity processing to prevent XXE attacks
+ parser.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
+ parser.setFeature("http://xml.org/sax/features/external-general-entities", false);
+ parser.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+ parser.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
+ parser.setExpandEntities(false);
- SAXBuilder parser = new SAXBuilder();
</code_context>
<issue_to_address>
SAXBuilder feature flags may not be supported by all XML parser implementations.
To prevent runtime errors, handle cases where these features are unsupported by catching ParserConfigurationException or verifying feature support before setting them.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// Disable external entity processing to prevent XXE attacks
parser.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
parser.setFeature("http://xml.org/sax/features/external-general-entities", false);
parser.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
parser.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
parser.setExpandEntities(false);
=======
// Disable external entity processing to prevent XXE attacks
try {
parser.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
} catch (Exception ex) {
MiscUtils.getLogger().warn("Could not set feature disallow-doctype-decl: " + ex.getMessage());
}
try {
parser.setFeature("http://xml.org/sax/features/external-general-entities", false);
} catch (Exception ex) {
MiscUtils.getLogger().warn("Could not set feature external-general-entities: " + ex.getMessage());
}
try {
parser.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
} catch (Exception ex) {
MiscUtils.getLogger().warn("Could not set feature external-parameter-entities: " + ex.getMessage());
}
try {
parser.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
} catch (Exception ex) {
MiscUtils.getLogger().warn("Could not set feature load-external-dtd: " + ex.getMessage());
}
try {
parser.setExpandEntities(false);
} catch (Exception ex) {
MiscUtils.getLogger().warn("Could not set expandEntities: " + ex.getMessage());
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `src/main/java/ca/openosp/openo/commn/web/Demographic2Action.java:274` </location>
<code_context>
+ result.put("hasDuplicates", !duplicateList.isEmpty());
- JSONUtil.jsonResponse(response, JSONObject.fromObject(result));
+ response.setContentType("application/json");
+ response.setCharacterEncoding("UTF-8");
+ try {
+ new ObjectMapper().writeValue(response.getWriter(), result);
+ } catch (Exception e) {
+ e.printStackTrace();
+ }
return null;
</code_context>
<issue_to_address>
Exception handling in checkForDuplicates may leak stack traces to client.
Instead of printing the stack trace, log the error and return a generic error message to the client to prevent exposing internal details.
</issue_to_address>
### Comment 6
<location> `src/main/java/ca/openosp/openo/report/pageUtil/RptByExamplesFavorite2Action.java:79` </location>
<code_context>
+ String favoriteName = this.getFavoriteName();
+ String query = this.getQuery();
+
+ String queryWithEscapeChar = StringEscapeUtils.escapeSql(query);///queryWithEscapeChar);
+ MiscUtils.getLogger().debug("escapeSql: " + queryWithEscapeChar);
+ write2Database(providerNo, favoriteName, queryWithEscapeChar);
}
</code_context>
<issue_to_address>
Use of StringEscapeUtils.escapeSql does not prevent SQL injection.
Use prepared statements or parameterized queries instead of escaping SQL to prevent injection vulnerabilities.
</issue_to_address>
### Comment 7
<location> `src/main/java/ca/openosp/openo/billings/ca/bc/pageUtil/ManageTeleplan2Action.java:164` </location>
<code_context>
REM076 ** **
*/
public List parse(File f) throws Exception {
+ // Validate that the file exists and is within allowed directory
+ if (!f.exists() || !f.isFile()) {
+ throw new IllegalArgumentException("Invalid file");
+ }
+
+ // Define allowed directory (configure this based on your needs)
</code_context>
<issue_to_address>
File path validation logic may not handle symbolic links or case sensitivity.
Using startsWith on normalized paths may not reliably restrict access, especially with symlinks or on case-insensitive file systems. Consider Files.isSameFile or similar methods for robust validation.
</issue_to_address>
### Comment 8
<location> `src/main/java/ca/openosp/openo/billings/ca/bc/Teleplan/TeleplanResponse.java:91` </location>
<code_context>
File file2 = new File(directory + realFilename);
+
+ // Validate that the file exists and is within allowed directory
+ if (!file2.exists() || !file2.isFile()) {
+ throw new IllegalArgumentException("Invalid file");
+ }
+
+ // Define allowed directory (configure this based on your needs)
+ File allowedDir = new File(OscarProperties.getInstance().getProperty("DOCUMENT_DIR"));
+
+ // Convert to Path and normalize
+ Path filePath = file2.toPath().normalize().toAbsolutePath();
+ Path allowedPath = allowedDir.toPath().normalize().toAbsolutePath();
+
+ if (!filePath.startsWith(allowedPath)) {
+ throw new SecurityException("File access not allowed outside designated directory");
+ }
</code_context>
<issue_to_address>
File existence check may fail if file is not yet created.
Since file2 is only created after the rename, checking its existence beforehand will always fail. Instead, validate the existence of the source file or adjust the logic order.
</issue_to_address>
### Comment 9
<location> `src/main/java/ca/openosp/openo/casemgmt/web/CaseManagementEntry2Action.java:490` </location>
<code_context>
- session.setAttribute("filter_roles", request.getParameterValues("filter_roles"));
- session.setAttribute("filter_provider", request.getParameterValues("filter_providers"));
- session.setAttribute("issues", request.getParameterValues("issues"));
+ String noteSort = request.getParameter("note_sort");
+ if (noteSort != null) {
+ session.setAttribute("note_sort", noteSort);
</code_context>
<issue_to_address>
Consider extracting a helper method to handle session attribute setting and removal for null or empty values.
Consider extracting a small helper to collapse all of these null/empty checks. For example, in your Action (or a shared util) add:
```java
private void setOrRemove(HttpSession session, String key, String value) {
if (value != null) {
session.setAttribute(key, value);
}
}
private void setOrRemove(HttpSession session, String key, String[] values) {
if (values == null) {
return;
} else if (values.length > 0) {
session.setAttribute(key, values);
} else {
session.removeAttribute(key);
}
}
```
Then replace the verbose blocks with just:
```java
setOrRemove(session, "note_sort", request.getParameter("note_sort"));
setOrRemove(session, "filter_roles", request.getParameterValues("filter_roles"));
setOrRemove(session, "filter_provider", request.getParameterValues("filter_providers"));
setOrRemove(session, "issues", request.getParameterValues("issues"));
```
</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/billings/ca/bc/Teleplan/TeleplanAPI.java
Outdated
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/lab/ca/bc/PathNet/Communication/HTTP.java
Outdated
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/report/pageUtil/RptByExamplesFavorite2Action.java
Outdated
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/billings/ca/bc/pageUtil/ManageTeleplan2Action.java
Outdated
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/billings/ca/bc/Teleplan/TeleplanResponse.java
Outdated
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/casemgmt/web/CaseManagementEntry2Action.java
Outdated
Show resolved
Hide resolved
Collaborator
Author
|
Looks like I missed the sourcery PR review, I will review these changes first thing in the mourning |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
yingbull
approved these changes
Sep 2, 2025
This was referenced Sep 2, 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
4 participants
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.
This is Kate Yang's PR (#324) but moved to branch off develop/dogfish initially, I have resolved a bunch of conflicts in these changes since develop/coyote and develop/dogfish are pretty different overall.
Summary by Sourcery
Upgrade project dependencies to resolve security vulnerabilities and refactor HTTP and XML handling to modern, secure libraries.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests:
Summary by Sourcery
Upgrade dependencies and harden the codebase against security vulnerabilities by bumping and excluding vulnerable libraries, refactoring HTTP and XML handling to modern, secure APIs, and adding runtime checks for file access and JSON/XML processing.
New Features:
Bug Fixes:
Enhancements:
Tests:
Chores: