Updated and removed some dependencies for security purpose#324
Updated and removed some dependencies for security purpose#324kateyang1998 wants to merge 22 commits intodevelop/coyotefrom
Conversation
Reviewer's GuideThis PR strengthens security by upgrading several dependencies to safer versions, adding direct management of vulnerable libraries, and systematically excluding risky transitive dependencies in the Maven configuration. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @kateyang1998 - I've reviewed your changes - here's some feedback:
- Consider consolidating repeated transitive exclusions and version overrides into a dependencyManagement section to reduce duplication.
- Align CXF module versions—cxf-rt-ws-security is still at 3.2.0 while transport is 3.3.0—to avoid incompatibilities.
- Remove redundant exclusions for libraries now included directly (e.g., ESAPI, dom4j, Beanutils) to simplify your POM.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider consolidating repeated transitive exclusions and version overrides into a dependencyManagement section to reduce duplication.
- Align CXF module versions—cxf-rt-ws-security is still at 3.2.0 while transport is 3.3.0—to avoid incompatibilities.
- Remove redundant exclusions for libraries now included directly (e.g., ESAPI, dom4j, Beanutils) to simplify your POM.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- 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
| <groupId>com.fasterxml.woodstox</groupId> | ||
| <artifactId>woodstox-core</artifactId> | ||
| </exclusion> | ||
| <exclusion> |
There was a problem hiding this comment.
can you explain this? we are excluding cxf?
There was a problem hiding this comment.
Overall we need more documentation; it helps reviewers and further people working on the code.
There was a problem hiding this comment.
@yingbull, these are the libraries that triggered the security issues, but are not being used. Excluding them won't impact cxf's functionality in our project, I will add some comments.
|
@kateyang1998 could you resolve the conflicts and test this is still good and working when merged in with current coyote, and I will finish the review? Change it from draft to ready to review when that is the case. |
|
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.
|
There are 46 compilation errors after merged with coyote, resolving now |
|
@yingbull all good now. |
- Replaced net.sf.json with jackson-databind in CaseloadContent2Action - Updated StringEscapeUtils usage Co-Authored-By: aider (deepseek/deepseek-chat) <aider@aider.chat>
|
@yingbull there are some error after resolve the conflict, I'm looking into them now. |
There was a problem hiding this comment.
Sorry @kateyang1998, you have reached your 24-hour rate limit for Sourcery. Please try again later
|
Thanks for the work on this! It is generating JSP errors on compile though. @LiamStanziani can you review this when you start back, and see what we've learned here we can apply to a new branch/pr against Dogfish? |
…arioMD, will need to review CodeQL fail on rebuild
src/main/java/oscar/oscarBilling/ca/bc/Teleplan/TeleplanCodesManager.java
Dismissed
Show dismissed
Hide dismissed
src/main/java/oscar/oscarBilling/ca/bc/Teleplan/TeleplanCodesManager.java
Dismissed
Show dismissed
Hide dismissed
src/main/java/oscar/oscarBilling/ca/bc/Teleplan/TeleplanCodesManager.java
Fixed
Show fixed
Hide fixed
src/main/java/oscar/oscarBilling/ca/bc/Teleplan/TeleplanResponse.java
Dismissed
Show dismissed
Hide dismissed
src/main/java/oscar/oscarBilling/ca/bc/Teleplan/TeleplanResponse.java
Dismissed
Show dismissed
Hide dismissed
src/main/java/oscar/oscarBilling/ca/bc/pageUtil/ManageTeleplan2Action.java
Dismissed
Show dismissed
Hide dismissed
src/main/java/oscar/oscarBilling/ca/bc/pageUtil/ManageTeleplan2Action.java
Fixed
Show fixed
Hide fixed
src/main/java/oscar/oscarBilling/ca/bc/pageUtil/ManageTeleplan2Action.java
Dismissed
Show dismissed
Hide dismissed
src/main/java/oscar/oscarBilling/ca/bc/pageUtil/ManageTeleplan2Action.java
Dismissed
Show dismissed
Hide dismissed
src/main/java/oscar/oscarBilling/ca/bc/pageUtil/ManageTeleplan2Action.java
Fixed
Show fixed
Hide fixed
|
PR is fully passing now (fixed up remaining jsp compilation errors, CodeQL, and test failures). I would like to test this to ensure nothing is breaking with these changes. @yingbull would you like me to still move the changes over to a develop/dogfish branch? or would you like this to be merged into develop/coyote and then moved over through a merge? |
|
if this is targeting coyote, confirm it is working well and we can merge to coyote. If you can cleanly enough merge to dogfish, I am okay with it - depending on what files are touched that may be fine, or ugly. In generally my approach here on would be to be pr to just dogfish. |
|
I think the PR is working well, although after further review I think the merge to dogfish will be quite ugly. I am going to create a branch off dogfish and move it over as you said before |
|
Closing this PR since it has been moved and merged to a branch based off of develop/dogfish, merging this would probably cause issues as well. PR: #494 |
Issue described in #322
Summary by Sourcery
Secure project dependencies by bumping library versions, adding security-focused libraries, and excluding vulnerable transitive dependencies
New Features:
Enhancements: