security: migrate from Apache Commons BeanUtils to Spring BeanUtils (Phase 1)#2241
security: migrate from Apache Commons BeanUtils to Spring BeanUtils (Phase 1)#2241
Conversation
…Phase 1) - Migrated 9 files from org.apache.commons.beanutils.BeanUtils to org.springframework.beans.BeanUtils - Reversed parameter order in copyProperties() calls (Spring uses source-first convention) - Migrated BeanUtilHlp to use Spring BeanWrapper for property access - Removed explicit commons-beanutils 1.11.0 dependency from pom.xml - Simplified exception handling (generic Exception instead of specific reflection exceptions) This migration eliminates the unmaintained commons-beanutils library and its associated CVE exposure (CVE-2014-0114, CVE-2019-10086, CVE-2025-48734). Spring BeanUtils is actively maintained and already included in our Spring Framework 5.3.39 dependency. Files modified: - src/main/java/ca/openosp/openo/util/BeanUtilHlp.java - src/main/java/ca/openosp/openo/prescript/web/CopyFavorites2Action.java - src/main/java/ca/openosp/openo/integration/mchcv/HCValidationResult.java - src/main/java/ca/openosp/openo/form/pharmaForms/formBPMH/business/BpmhForm2Handler.java - src/main/java/ca/openosp/openo/dashboard/handler/TicklerHandler.java - src/main/java/ca/openosp/openo/dashboard/factory/DashboardBeanFactory.java - src/main/java/ca/openosp/openo/dashboard/factory/DrilldownBeanFactory.java - src/main/java/ca/openosp/openo/commn/model/Demographic.java - src/main/java/ca/openosp/openo/PMmodule/web/ClientManager2Action.java - pom.xml Fixes #2202 Co-authored-by: Michael Yingbull <yingbull@users.noreply.github.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the
📝 WalkthroughWalkthroughThis pull request removes the Apache Commons BeanUtils dependency from the project and migrates all bean property copying operations to Spring BeanUtils. The pom.xml dependency is removed, and eleven Java files are updated to use Spring's API, with argument order adjustments to accommodate the different method signature (source, target vs. destination, original). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
This PR implements Phase 1 of migrating from Apache Commons BeanUtils to Spring BeanUtils to address security vulnerabilities (CVE-2014-0114, CVE-2019-10086, CVE-2025-48734). The migration focuses on replacing direct BeanUtils.copyProperties() and BeanUtils.getProperty() usages with Spring's equivalents.
Changes:
- Migrated 9 Java files from
org.apache.commons.beanutils.BeanUtilstoorg.springframework.beans.BeanUtils - Updated parameter order from
copyProperties(dest, source)tocopyProperties(source, dest)for Spring compatibility - Simplified exception handling from specific reflection exceptions to generic
Exception - Migrated
BeanUtilHlp.javato use Spring'sBeanWrapperfor property access - Removed explicit commons-beanutils 1.11.0 dependency from pom.xml
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| BeanUtilHlp.java | Migrated from BeanUtils.getProperty() to Spring BeanWrapper with potential null handling behavior change |
| CopyFavorites2Action.java | Updated copyProperties parameter order for favorites copying |
| HCValidationResult.java | Updated copyProperties parameter order for health card validation fee service details |
| BpmhForm2Handler.java | Updated copyProperties parameter order and simplified exception handling for pharmacy medication history |
| TicklerHandler.java | Updated copyProperties parameter order for tickler creation |
| DashboardBeanFactory.java | Updated copyProperties parameter order and simplified exception handling for dashboard beans |
| DrilldownBeanFactory.java | Updated copyProperties parameter order for drilldown indicator beans |
| Demographic.java | Updated copy constructor to use Spring BeanUtils with reversed parameter order |
| ClientManager2Action.java | Updated copyProperties parameter order in 2 locations for program management |
| pom.xml | Removed explicit commons-beanutils dependency (CRITICAL ISSUE: premature removal) |
| } catch (IllegalAccessException ex) { | ||
| BeanWrapper beanWrapper = new BeanWrapperImpl(bean); | ||
| Object propertyValue = beanWrapper.getPropertyValue(fieldName); | ||
| value = propertyValue != null ? propertyValue.toString() : ""; |
There was a problem hiding this comment.
The migration from Apache Commons BeanUtils.getProperty() to Spring BeanWrapper changes null handling behavior.
Apache Commons BeanUtils.getProperty() returns the string "null" when a property value is null, whereas the new implementation returns an empty string "".
Current implementation:
Object propertyValue = beanWrapper.getPropertyValue(fieldName);
value = propertyValue != null ? propertyValue.toString() : "";This means code that relies on BeanUtilHlp.getPropertyValue() will now get "" instead of "null" for null properties. This could affect business logic in files using BeanUtilHlp, particularly MSPReconcile.java which uses it for billing status checks.
Consider either:
- Maintaining backward compatibility by returning "null" string:
value = propertyValue != null ? propertyValue.toString() : "null"; - Verifying that all callers can handle empty strings correctly
- Documenting this behavior change in the PR description
| value = propertyValue != null ? propertyValue.toString() : ""; | |
| value = propertyValue != null ? propertyValue.toString() : "null"; |
|
@claude review for any other issues where we need to match old behaviour to avoid breaking changes as copilot pointed out relating to the null vs "" default - should we fix this anywhere else? |
|
Claude finished @yingbull's task in 1m 20s —— View job Review Complete: Behavioral Analysis for Breaking ChangesI've analyzed all 9 migrated files for behavioral differences between Apache Commons BeanUtils and Spring BeanUtils. Here's what I found: ✅ No Additional Breaking Changes FoundAfter thorough review, Copilot's finding about BeanUtilHlp.java is the ONLY behavioral difference that requires attention. All other migrations are safe. Detailed Analysis by File🔴 BeanUtilHlp.java - BREAKING CHANGE (Already Identified)Line 57: // OLD (Apache Commons): returned "null" string for null properties
// NEW (Spring): returns "" for null properties
value = propertyValue != null ? propertyValue.toString() : "";Impact: Any code calling Known usage: MSPReconcile.java uses this for billing status checks. Recommendation: Change line 57 to maintain backward compatibility: value = propertyValue != null ? propertyValue.toString() : "null";✅ All Other Files Are Safe1. Demographic.java (Copy Constructor)Line 197: BeanUtils.copyProperties(d, this); // Spring: (source, dest)
2. DashboardBeanFactory.javaLine 88: BeanUtils.copyProperties(getDashboardEntity(), dashboardBean);
3. DrilldownBeanFactory.javaLine 109: BeanUtils.copyProperties(getIndicatorTemplate(), drilldownBean);
4. TicklerHandler.javaLine 127: BeanUtils.copyProperties(getMasterTickler(), ticklerCopy);
5. BpmhForm2Handler.javaLine 487: BeanUtils.copyProperties(drug, bpmhDrug);
6. HCValidationResult.javaLine 268: BeanUtils.copyProperties(feeServiceDetail, selectedFeeServiceDetails);
7. CopyFavorites2Action.javaLine 112: BeanUtils.copyProperties(f, copy);
8. ClientManager2Action.java (2 locations)Line 562 & 605: BeanUtils.copyProperties(cachedProgram, program);
Key Differences Between Libraries
🎯 Final RecommendationOnly one fix needed: Update All other migrations are correct and safe. The parameter order reversals were done correctly across all files, and none of the other use cases involve individual property retrieval where null handling matters. --- | |
|
@claude commit a fix to correct this. |
|
Claude finished @yingbull's task in 1m 21s —— View job ✅ Fixed null handling in BeanUtilHlp.javaCorrected the backward compatibility issue identified by Copilot in the BeanUtils migration. Tasks Completed
SummaryIssue: The migration from Apache Commons BeanUtils to Spring BeanUtils changed the null handling behavior in Impact: This breaking change could affect code like Fix: Changed line 57 in value = propertyValue != null ? propertyValue.toString() : "";to: value = propertyValue != null ? propertyValue.toString() : "null";This restores backward compatibility with the Apache Commons BeanUtils behavior, ensuring that existing code continues to function correctly. Commit: | Branch |
Change BeanUtilHlp.getPropertyValue() to return "null" string for null properties instead of empty string, matching Apache Commons BeanUtils behavior. This maintains compatibility with existing code like MSPReconcile.java that expects the string "null" for null values. Co-authored-by: Michael Yingbull <yingbull@users.noreply.github.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/main/java/ca/openosp/openo/util/BeanUtilHlp.java">
<violation number="1" location="src/main/java/ca/openosp/openo/util/BeanUtilHlp.java:57">
P2: Returning the literal string "null" when a property is missing introduces a behavioral regression (callers now get the text "null" instead of a blank). Keep the empty-string fallback or return null to avoid persisting/displaying "null".</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@claude review |
|
Claude finished @yingbull's task in 2m 59s —— View job Code Review CompleteAnalysis SummaryI've reviewed all 10 changed files and identified one critical blocking issue that prevents merging this PR. 🔴 BLOCKING ISSUE: Build Will Fail Due to Missing BeanComparatorFiles affected: 9 source files still actively use Evidence of Active Usage: // These 9 files will fail to compile:
src/main/java/ca/openosp/openo/mds/data/ProviderData.java:84
Collections.sort(providers, new BeanComparator("formattedName"));
src/main/java/ca/openosp/openo/providers/bean/ProviderNameBeanHandler.java:74
Collections.sort(groups, new BeanComparator("firstName"));
src/main/java/ca/openosp/openo/provider/web/ProviderProperty2Action.java:1614,1777
Collections.sort(ps, new BeanComparator("lastName"));
// ... and 5 more filesWhy This is Blocking:
Recommendation: <dependency>
<groupId>commons-beanutils</groupId>
<artifactId>commons-beanutils</artifactId>
<version>1.9.4</version>
</dependency>Phase 1b Follow-up (separate PR):
Copilot correctly identified this issue → ✅ Issue #2: Null Handling - Already Fixed CorrectlyFile: What Happened:
Critical Usage ( String beanStatus = beanut.getPropertyValue(bill, "status");
if (beanStatus.equals(status)) { // Needs "null" string, not ""
amt += Double.valueOf(bill.getAmount()).doubleValue();
}Verdict: ✅ The fix is correct. Keep Note: cubic-dev-ai's suggestion to revert to ✅ All Other Changes Look GoodParameter Order Reversals (9 files)All files correctly reversed parameter order from Apache Commons (
Exception Handling SimplificationThe migration from specific exceptions to generic 📋 Action Items Before MergeRequired Changes:
For Phase 1b (separate PR):
🎯 Final Verdict❌ DO NOT MERGE until commons-beanutils dependency is restored. Rationale: The current PR will cause immediate compilation failures in 9 files due to missing BeanComparator class. The build will break before any runtime issues occur. Once dependency is restored: ✅ All other changes are correct and the migration is properly implemented. |
|
@claude but that dependency is an issue and the whole point is to get away from it? What else can we do? What have others done doing this migration as a best practise relating to this? |
|
Claude finished @yingbull's task in 1m 19s —— View job BeanComparator Migration Strategy AnalysisYou're absolutely right to question this - restoring commons-beanutils defeats the purpose. Let me explain what I found and provide better alternatives. Tasks
🔍 Current SituationGood news: Your PR removes the explicit dependency correctly. The issue is that 9 files still use
Critical finding: commons-beanutils is explicitly excluded from 5 transitive dependencies in pom.xml (lines 255, 282, 633, 795, 956), so it was ONLY available through the now-removed explicit dependency. This means: ❌ The build will fail with "cannot find symbol: class BeanComparator" errors. ✅ Best Practice Migration StrategyIndustry best practice: Replace BeanComparator with Java 8+ Comparators (no Spring alternative needed) Why This is the Modern Approach:
Migration Examples:OLD (Apache Commons BeanComparator): Collections.sort(providers, new BeanComparator("lastName"));NEW (Java Comparator.comparing): Collections.sort(providers, Comparator.comparing(Provider::getLastName));For reversed sorting: // OLD
Collections.sort(appts, new ReverseComparator(new BeanComparator("startTime")));
// NEW
Collections.sort(appts, Comparator.comparing(Appointment::getStartTime).reversed());For null-safe sorting: Collections.sort(providers,
Comparator.comparing(Provider::getLastName, Comparator.nullsLast(String::compareTo)));📋 Recommended Action PlanOption 1: Complete the Migration in This PR (Recommended)
Option 2: Two-Phase Approach
I recommend Option 1 - the migration is straightforward and keeps this PR focused. 🎯 What I Can Do Right NowI can:
This would give you:
Want me to implement the complete migration now? Just say the word and I'll:
|
|
@claude add a commit to do these migrations |
|
Claude finished @yingbull's task in 4m 52s —— View job ✅ BeanComparator Migration CompleteSuccessfully migrated all 9 files from Apache Commons BeanComparator to modern Java Comparator API, completing the full commons-beanutils removal. Migration SummaryMigration Pattern Applied: // OLD (Apache Commons BeanUtils - reflection-based)
Collections.sort(providers, new BeanComparator("lastName"));
Collections.sort(appts, new ReverseComparator(new BeanComparator("startTime")));
// NEW (Java 8+ Comparator - type-safe method references)
Collections.sort(providers, Comparator.comparing(Provider::getLastName));
Collections.sort(appts, Comparator.comparing(Appointment::getStartTime).reversed());Files Migrated (9 files, 11 BeanComparator usages)
Benefits AchievedSecurity:
Code Quality:
CVEs Eliminated:
What ChangedCommit:
VerificationThis PR now completes Phase 1 of commons-beanutils migration:
Ready for:
--- | Branch |
Complete migration from Apache Commons BeanComparator to modern Java 8+ Comparator API. This completes Phase 1 of commons-beanutils removal by eliminating all remaining Apache Commons BeanUtils dependencies. Changes: - Replaced BeanComparator with Comparator.comparing() in 9 files - Replaced ReverseComparator with .reversed() method - Removed all org.apache.commons.beanutils imports - Zero new dependencies (uses built-in Java Comparator) - Type-safe property references instead of string-based reflection Benefits: - Eliminates CVE exposure (CVE-2014-0114, CVE-2019-10086, CVE-2025-48734) - Better performance (no reflection overhead) - Compile-time safety (method references vs string property names) - More maintainable modern Java code Files migrated: 1. ProviderData.java (2 usages - formattedName sorting) 2. ProviderProperty2Action.java (2 usages - lastName sorting) 3. RptMeasurementTypesBeanHandler.java (2 usages - typeDisplayName, typeDescription) 4. RptLabReportData.java (1 usage - lastName sorting) 5. ThirdApptTimeReporter.java (1 usage - startTime reversed) 6. EctStyleSheetBeanHandler.java (1 usage - id sorting) 7. AppointmentStatusMgrImpl.java (1 usage - id sorting) 8. ProviderNameBeanHandler.java (1 usage - firstName sorting) 9. MsgMessengerGroupData.java (1 usage - lastName sorting) Related-to: #2202 Co-authored-by: Michael Yingbull <yingbull@users.noreply.github.com>
There was a problem hiding this comment.
2 issues found across 9 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/main/java/ca/openosp/openo/messenger/config/data/MsgMessengerGroupData.java">
<violation number="1" location="src/main/java/ca/openosp/openo/messenger/config/data/MsgMessengerGroupData.java:211">
P2: Comparator.comparing(Provider::getLastName) is not null-safe; if any provider has a null lastName, sorting will throw a NullPointerException. Use a null-tolerant comparator or map nulls to an empty string.</violation>
</file>
<file name="src/main/java/ca/openosp/openo/report/data/RptLabReportData.java">
<violation number="1" location="src/main/java/ca/openosp/openo/report/data/RptLabReportData.java:76">
P2: `Comparator.comparing(Provider::getLastName)` will throw a NullPointerException if any provider has a null lastName. Use a null-safe comparator to avoid runtime failures when data is incomplete.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@claude review |
|
Claude finished @yingbull's task in 3m 10s —— View job ✅ Code Review CompleteI've thoroughly reviewed all 19 changed files across the 3 commits in this PR. Here's my comprehensive analysis: SummaryThis PR successfully migrates from Apache Commons BeanUtils to Spring BeanUtils and Java Comparators, eliminating the unmaintained commons-beanutils dependency and its associated CVEs. The migration is well-executed and ready to merge after addressing one documentation clarification. Detailed Analysis✅ BeanUtils.copyProperties() Migrations (9 files)All parameter order reversals are correct:
Verification: All migrations correctly reversed from Apache Commons' ✅ BeanUtilHlp.java Null Handling (Line 57)Current implementation: value = propertyValue != null ? propertyValue.toString() : "null";Status: ✅ CORRECT - Maintains backward compatibility Analysis:
Recommendation: cubic-dev-ai's suggestion to revert to ✅ BeanComparator → Java Comparator Migrations (9 files, 11 usages)All migrations are correct and safe: Example migration: // OLD (Apache Commons - reflection-based)
Collections.sort(ps, new BeanComparator("lastName"));
// NEW (Java 8+ - type-safe)
Collections.sort(ps, Comparator.comparing(Provider::getLastName));Files migrated: ProviderData.java, ProviderProperty2Action.java, RptMeasurementTypesBeanHandler.java, RptLabReportData.java, ThirdApptTimeReporter.java, EctStyleSheetBeanHandler.java, AppointmentStatusMgrImpl.java, ProviderNameBeanHandler.java, MsgMessengerGroupData.java 🔍 Addressing cubic-dev-ai's Null-Safety Concernscubic-dev-ai flagged potential NPE in 2 files:
Status: ❌ False positive - No null-safety issue exists Evidence from database schema ( CREATE TABLE IF NOT EXISTS provider (
provider_no varchar(6) NOT NULL default '',
last_name varchar(30) NOT NULL default '', -- NOT NULL constraint
first_name varchar(30) NOT NULL default '',
...Key findings:
Recommendation: Ignore cubic-dev-ai's suggestion to add Benefits AchievedSecurity:
Code Quality:
Minor Issues📝 PR Description ClarityIssue: The PR description mentions "Phase 1" but doesn't clarify the scope after completing both Recommendation: Update PR description to clarify:
This is a documentation clarification only - the code is correct. Final Verdict✅ APPROVE - Ready to merge Summary:
Optional follow-up: Update PR description to clarify this completes the full commons-beanutils removal (not just Phase 1a). |
Summary
This PR implements Phase 1 of the commons-beanutils migration strategy (#2202): migrating all direct usage from Apache Commons BeanUtils to Spring BeanUtils.
Changes
Migration Details
org.apache.commons.beanutils.BeanUtilstoorg.springframework.beans.BeanUtilscopyProperties(source, dest)vs Commons'copyProperties(dest, source)Exceptioninstead of specific reflection exceptionsBeanWrapperfor property accessFiles Modified
src/main/java/ca/openosp/openo/util/BeanUtilHlp.javasrc/main/java/ca/openosp/openo/prescript/web/CopyFavorites2Action.javasrc/main/java/ca/openosp/openo/integration/mchcv/HCValidationResult.javasrc/main/java/ca/openosp/openo/form/pharmaForms/formBPMH/business/BpmhForm2Handler.javasrc/main/java/ca/openosp/openo/dashboard/handler/TicklerHandler.javasrc/main/java/ca/openosp/openo/dashboard/factory/DashboardBeanFactory.javasrc/main/java/ca/openosp/openo/dashboard/factory/DrilldownBeanFactory.javasrc/main/java/ca/openosp/openo/commn/model/Demographic.javasrc/main/java/ca/openosp/openo/PMmodule/web/ClientManager2Action.javapom.xmlSecurity Impact
This migration eliminates the unmaintained commons-beanutils library and its associated CVE exposure:
Spring BeanUtils is actively maintained as part of Spring Framework 5.3.39 (already in our stack) and provides equivalent functionality without the security vulnerabilities.
Testing
Related
Next Steps
Phase 2 (recommended in #2202):
🤖 Generated with Claude Code
Summary by cubic
Migrated all direct uses of Apache Commons BeanUtils to Spring BeanUtils and replaced BeanComparator with Java Comparator (Phase 1 of #2202) to remove a vulnerable, unmaintained library. Behavior remains the same and the app compiles cleanly.
Refactors
Dependencies
Written for commit 10235b0. Summary will update on new commits.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.