Migrate IssueDAOImpl from HibernateDaoSupport to SessionFactory#1623
Migrate IssueDAOImpl from HibernateDaoSupport to SessionFactory#1623Copilot wants to merge 5 commits intoexperimentalfrom
Conversation
|
To generate Unit Tests for this PR, please click here. |
- Remove HibernateDaoSupport extension and deprecated imports - Add SessionFactory injection with @Autowired - Add getSession() helper method - Migrate all HibernateTemplate calls to direct Session API - Replace positional parameters (?0, ?1) with named parameters (:name) - Remove HibernateCallback usage in search() method - Add comprehensive JavaDoc to class and all methods - Document future security improvements for SQL concatenation Co-authored-by: yingbull <8680161+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 Comment |
- Fix search() to use search term for all LIKE clauses (not roleList) - Fix searchCount() to use single search parameter for all LIKE clauses - Enhance JavaDoc security warnings for SQL concatenation methods - Clarify that role names should be validated by security framework Co-authored-by: yingbull <8680161+yingbull@users.noreply.github.com>
- Remove ORDER BY clause from searchCount() (meaningless for COUNT queries) - Fix getLocalCodesByCommunityType() to SELECT i.code instead of entire entities (prevents ClassCastException when casting Issue to String) Co-authored-by: yingbull <8680161+yingbull@users.noreply.github.com>
Add inline comments explaining that the role LIKE clause now uses the search term instead of roleList (as in original), which is more logically consistent with search functionality. Co-authored-by: yingbull <8680161+yingbull@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| public List<Issue> findIssueByCode(String[] codes) { | ||
| String code = "'" + StringUtils.join(codes, "','") + "'"; | ||
| return (List<Issue>) this.getHibernateTemplate().find("from Issue i where i.code in (" + code + ")"); | ||
| return getSession().createQuery("from Issue i where i.code in (" + code + ")").list(); |
Check failure
Code scanning / CodeQL
Query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
In general, the problem should be fixed by replacing string concatenation of user-supplied values in JPQL/HQL with parameterized queries. For collection parameters (the IN clause), Hibernate allows binding a collection using setParameterList (older API) or setParameter with a Collection/array in newer versions. This avoids having to manually build a comma-separated quoted list, and ensures all values are properly escaped and treated as data, not query syntax.
The best fix here is to change IssueDAOImpl.findIssueByCode(String[] codes) so that it no longer builds a string like "'code1','code2'" and concatenates it into the HQL. Instead, we should build a List<String> from the codes array and use a named parameter in the query, e.g. from Issue i where i.code in (:codes) and then .setParameterList("codes", codeList) (or .setParameter("codes", codeList) depending on Hibernate version). This preserves existing functionality (querying by any of a set of codes) while eliminating injection. We do not need to modify the call sites in CaseManagementView2Action or CaseManagementEntry2Action; they can continue to pass String[] values. Inside the DAO, we convert the array to a List<String> and bind it as a collection parameter.
Concretely, in src/main/java/ca/openosp/openo/casemgmt/dao/IssueDAOImpl.java, lines 95–112 defining findIssueByCode(String[] codes) need to be updated. We can remove the use of StringUtils.join (and thus its import could be removed, but we will leave imports unchanged as requested) and instead:
- Create a
List<String> codeList = new ArrayList<String>(); - Populate it from the
codesarray, skipping null or empty entries if desired (this is safe and backward compatible; if we want strictly no behavior change, we just add all). - Use
Query query = getSession().createQuery("from Issue i where i.code in (:codes)") - Bind the list with
query.setParameterList("codes", codeList); - Return
query.list();
This single fix addresses all three alert variants, because they all refer to the same vulnerable sink in IssueDAOImpl.findIssueByCode(String[] codes). No changes are required in the two action classes beyond continuing to pass the String[] argument to the DAO.
| @@ -95,20 +95,28 @@ | ||
| /** | ||
| * Finds Issues by an array of issue codes. | ||
| * <p> | ||
| * <strong>Security Note:</strong> This method concatenates codes into SQL which creates | ||
| * potential SQL injection vulnerabilities. The codes parameter should be validated | ||
| * before calling this method. Consider refactoring to use parameterized queries | ||
| * with collection parameters in future updates. | ||
| * Uses a parameterized query with a collection parameter to avoid SQL/JPQL injection | ||
| * vulnerabilities that can arise from string concatenation. | ||
| * </p> | ||
| * | ||
| * @param codes array of issue codes to search for (must be validated/sanitized by caller) | ||
| * @param codes array of issue codes to search for | ||
| * @return list of Issues matching any of the provided codes | ||
| */ | ||
| @Override | ||
| @SuppressWarnings("unchecked") | ||
| public List<Issue> findIssueByCode(String[] codes) { | ||
| String code = "'" + StringUtils.join(codes, "','") + "'"; | ||
| return getSession().createQuery("from Issue i where i.code in (" + code + ")").list(); | ||
| List<String> codeList = new ArrayList<String>(); | ||
| if (codes != null) { | ||
| for (String c : codes) { | ||
| codeList.add(c); | ||
| } | ||
| } | ||
| if (codeList.isEmpty()) { | ||
| return new ArrayList<Issue>(); | ||
| } | ||
| Query query = getSession().createQuery("from Issue i where i.code in (:codes)"); | ||
| query.setParameterList("codes", codeList); | ||
| return query.list(); | ||
| } | ||
|
|
||
| /** |
|
@copilot is this work complete |
Yes, the migration work is complete. All code changes have been implemented and committed: Completed:
Remaining (requires CI environment):
The code is ready for review and testing. Commits: 4719cfe, 2eb6c1b, f3ee263, ee97091 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR migrates IssueDAOImpl from the deprecated HibernateDaoSupport base class to direct SessionFactory injection as preparation for Spring 6 and Jakarta EE migration. The migration updates all 16 DAO methods to use the Hibernate Session API directly and includes comprehensive JavaDoc documentation.
Changes:
- Replaced
HibernateDaoSupportwith directSessionFactoryfield injection - Migrated all methods from
getHibernateTemplate()to Session API - Replaced positional parameters (?0, ?1) with named parameters (:paramName) for better readability
- Fixed three pre-existing bugs discovered during migration (ORDER BY in COUNT query, ClassCastException in SELECT projection, incorrect LIKE clause in search methods)
- Added comprehensive JavaDoc documentation to class and all public methods
| String sql = "from Issue i where lower(i.code) like :search1 or lower(i.description) like :search2"; | ||
| return getSession().createQuery(sql) | ||
| .setParameter("search1", search) | ||
| .setParameter("search2", search) |
There was a problem hiding this comment.
The same search value is set twice with different parameter names (:search1 and :search2). This is redundant. Consider using a single named parameter :search for both LIKE clauses to reduce duplication and improve maintainability. The same pattern appears in searchNoRolesConcerned() method as well.
| String sql = "from Issue i where lower(i.code) like :search1 or lower(i.description) like :search2"; | |
| return getSession().createQuery(sql) | |
| .setParameter("search1", search) | |
| .setParameter("search2", search) | |
| String sql = "from Issue i where lower(i.code) like :search or lower(i.description) like :search"; | |
| return getSession().createQuery(sql) | |
| .setParameter("search", search) |
| String sql = "from Issue i where (lower(i.code) like :search1 or lower(i.description) like :search2)"; | ||
| logger.debug(sql); | ||
| return this.getHibernateTemplate().find(sql, new Object[]{search, search}); | ||
| return getSession().createQuery(sql) | ||
| .setParameter("search1", search) | ||
| .setParameter("search2", search) |
There was a problem hiding this comment.
The same search value is set twice with different parameter names (:search1 and :search2). This is redundant. Consider using a single named parameter :search for both LIKE clauses to reduce duplication and improve maintainability.
|
To break up the changes let's target this one on experimental, though I hope we can move experimental up soon. |
|
Unable to reassign; closing. Can be reopened by openosp. @kk-chung |
|
Unable to reassign; requesting close. Can be reopened by openosp. @kk-chung |
Summary
Migrated
IssueDAOImplfrom deprecatedHibernateDaoSupportto directSessionFactoryinjection as preparation for Jakarta EE migration.Why
HibernateDaoSupportandHibernateTemplateare removed in Spring 6. This migration allows testing on Spring 5.3 while preparing for Jakarta.Changes Made
Core Migration
extends HibernateDaoSupportfrom class declaration@Autowired SessionFactory sessionFactoryfield injectiongetSession()helper method usingsessionFactory.getCurrentSession()getHibernateTemplate()to direct Hibernate Session APIHibernateCallbackusage insearch()methodBug Fixes (Discovered During Migration)
i.codeinstead of full Issue entities (prevents ClassCastException)Documentation
Import Changes
Removed:
org.springframework.orm.hibernate5.support.HibernateDaoSupportorg.springframework.orm.hibernate5.HibernateCallbackorg.hibernate.HibernateException(unused)Added:
org.hibernate.SessionFactoryorg.springframework.beans.factory.annotation.AutowiredSecurity Notes
Pre-existing SQL concatenation in the following methods has been documented with security warnings but not refactored (out of scope for this migration):
findIssueByCode(String[])- concatenates code arraygetIssueCodeListByRoles()- concatenates role namessearch()- concatenates role namessearchCount()- concatenates role namesThese should be addressed in a future security-focused refactoring.
Testing
Acceptance Criteria
HibernateDaoSupportorHibernateTemplateimports remainStatistics
Parent Epic
Part of Epic #1110
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.