FINERACT-2461: Refactor EmailReadPlatformServiceImpl to use Prepared Statements by Saifulhuq01 · Pull Request #5417 · apache/fineract

Saifulhuq01 · 2026-01-30T05:29:19Z

Description

Refactored EmailReadPlatformServiceImpl.java to replace legacy SQL string concatenation with JDBC Prepared Statements.

This change prevents potential SQL injection vulnerabilities by using ? placeholders and passing parameters dynamically via JdbcTemplate.

Resolves FINERACT-2461.
Also related to FINERACT-2459.

Changes

Refactored queries in retrieveAllPending, retrieveAllSent, and other read methods to use ? placeholders.
Implemented List<Object> to pass parameters dynamically.
Applied Spotless formatting.

Checklist

Commit message follows guidelines
Coding conventions followed
Build is passing

Aman-Mittal · 2026-01-30T16:35:09Z

...org/apache/fineract/infrastructure/campaigns/email/service/EmailReadPlatformServiceImpl.java

- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file


Why this removed?

Why this removed?

Apologies, that was accidental during the refactor. I have restored the License Header in the latest push.

oleksii-novikov-onix · 2026-01-30T20:59:38Z

These changes really improved the code, but in my opinion it would be much more beneficial to rewrite it from jdbcTemplate to Spring Data JPA.

Saifulhuq01 · 2026-01-31T04:50:27Z

These changes really improved the code, but in my opinion it would be much more beneficial to rewrite it from jdbcTemplate to Spring Data JPA.

Thank you for the review! I agree that moving to Spring Data JPA is the better long-term architectural goal.

However, since this PR is focused on a critical Security Fix (SQL Injection), I would prefer to keep the scope limited to jdbcTemplate for now to ensure a faster merge. I can open a separate Jira ticket to handle the JPA migration as a follow-up task. Does that sound reasonable?

Aman-Mittal · 2026-01-31T14:53:35Z

...org/apache/fineract/infrastructure/campaigns/email/service/EmailReadPlatformServiceImpl.java

    public Collection<EmailData> retrieveAllFailed(final SearchParameters searchParameters) {
-        final String sqlPlusLimit = (searchParameters.getLimit() > 0) ? " " + sqlGenerator.limit(searchParameters.getLimit()) : "";
-        final String sql = "select " + this.emailRowMapper.schema() + " where emo.status_enum = ?" + sqlPlusLimit;
+        String sql = "select " + this.emailRowMapper.schema() + " where emo.status_enum = ?";


I see a pattern here i think you can create helper function here

I see a pattern here i think you can create helper function here

Good catch. I have refactored the duplicate logic into a private helper method retrieveEmailByStatus to reduce code repetition

adamsaghy

This PR does NOT change the EmailReadPlatformServiceImpl.java‎ to use prepared statements.

Saifulhuq01 · 2026-02-03T04:20:06Z

This PR does NOT change the EmailReadPlatformServiceImpl.java‎ to use prepared statements.

Apologies @adamsaghy, I believe I accidentally reverted the changes while fixing the license header. I have just force-pushed the correct version with the prepared statements and helper method. Please verify

adamsaghy · 2026-02-03T09:58:02Z

...org/apache/fineract/infrastructure/campaigns/email/service/EmailReadPlatformServiceImpl.java

+        args.add(status);
+
+        if (sqlPlusLimit != null) {
+            sql += sqlPlusLimit;


it is still using string concatenation...

it is still using string concatenation...

Ah, I missed converting the sql variable to a StringBuilder. I'll update it to use sql.append(sqlPlusLimit) instead of string concatenation. Pushing the fix shortly.

No, that is not the problem.
The PR title says: use prepared statements. However, you are not using that for the sqlPlusLimit... you are still just concatenate a string to the SQL query itself. Please rewrite it to use prepared statements only and provide the arguments only.

adamsaghy · 2026-02-03T10:33:21Z

...org/apache/fineract/infrastructure/campaigns/email/service/EmailReadPlatformServiceImpl.java

+        if (sqlPlusLimit != null) {
+            sql.append(sqlPlusLimit);
+        }


The problem is:
you are not concatenating to the SQL the following: LIMIT ? but you are concatenating: LIMIT <whatever the user provided in query param> which is wrong...

The problem is: you are not concatenating to the SQL the following: LIMIT ? but you are concatenating: LIMIT <whatever the user provided in query param> which is wrong...

The sqlGenerator.limit() method returns a literal string (e.g. 'LIMIT 10'). I've cleaned up the code to pass the Integer limit down, but sticking to the generator to ensure MSSQL compatibility.

…Statements

adamsaghy

I havent seen any existing test cases for this functionality.

Would you mind to write a unit test which tests thoroughly these changes?

You should focus whether the appropriate status is added to the sql and also the appropriate LIMIT added to the sql.

Saifulhuq01 · 2026-02-03T11:07:14Z

I havent seen any existing test cases for this functionality.

Would you mind to write a unit test which tests thoroughly these changes?

You should focus whether the appropriate status is added to the sql and also the appropriate LIMIT added to the sql.

Done. I have added EmailReadPlatformServiceImplTest to verify the logic.

The new tests cover:

testRetrieveAllPendingWithLimit(): Verifies that the LIMIT clause is correctly appended when provided.

testRetrieveAllSentWithoutLimit(): Verifies that no LIMIT clause is added when the limit is null/zero, and checks that the correct status_enum parameter is bound.

I verified the tests locally: Test testRetrieveAllPendingWithLimit() PASSED Test testRetrieveAllSentWithoutLimit() PASSED

adamsaghy · 2026-02-03T12:11:48Z

...apache/fineract/infrastructure/campaigns/email/service/EmailReadPlatformServiceImplTest.java

+    private JdbcTemplate jdbcTemplate;
+
+    @Mock
+    private DatabaseSpecificSQLGenerator sqlGenerator;


Please dont mock the sqlGenerator. Mocking this makes the tests are completely useless...

You can mock the DatabaseTypeResolver only... and you can easily test for Mysql and Postgres as well.

Please make sure to test for positive test cases, like limit is "10" and "1" and for negative where limit is "random text", "-5", "5.6"

Please dont mock the sqlGenerator. Mocking this makes the tests are completely useless...

You can mock the DatabaseTypeResolver only... and you can easily test for Mysql and Postgres as well.

Please make sure to test for positive test cases, like limit is "10" and "1" and for negative where limit is "random text", "-5", "5.6"

Thanks for the detailed feedback, Adam. I have updated the PR with the following changes:

Refactoring: EmailReadPlatformServiceImpl now passes the Integer limit directly to the helper method instead of concatenating strings. This ensures limit is strictly an Integer, preventing injection of random text or floats by the compiler itself.

Testing Strategy: I removed the mock for DatabaseSpecificSQLGenerator. The new tests use the real generator and mock only the underlying DatabaseTypeResolver to simulate database dialects accurately.

Test Coverage: I added comprehensive test cases:

testRetrieveAllPendingWithMySQL: Verifies LIMIT 10 syntax for MySQL.

testRetrieveAllSentWithPostgres: Verifies LIMIT 5 syntax for PostgreSQL.

testRetrieveWithNegativeLimit & testRetrieveWithZeroLimit: Verifies that invalid limits (-5, 0) result in no LIMIT clause being appended.

Status: All 5 tests passed locally.

…Statements (apache#5417) * FINERACT-2461: Refactor EmailReadPlatformServiceImpl to use Prepared Statements * FINERACT-2461: Add unit tests for EmailReadPlatformServiceImpl

Aman-Mittal reviewed Jan 30, 2026

View reviewed changes

Saifulhuq01 force-pushed the fix-email-sql-injection branch from 4c731e2 to 7eb8e83 Compare January 31, 2026 04:42

Saifulhuq01 requested a review from Aman-Mittal January 31, 2026 04:51

Saifulhuq01 force-pushed the fix-email-sql-injection branch from 7eb8e83 to 948f2b5 Compare January 31, 2026 09:44

Aman-Mittal reviewed Jan 31, 2026

View reviewed changes

Saifulhuq01 force-pushed the fix-email-sql-injection branch 2 times, most recently from 7eb4a6d to c925217 Compare January 31, 2026 17:09

Saifulhuq01 requested a review from Aman-Mittal January 31, 2026 17:10

Saifulhuq01 force-pushed the fix-email-sql-injection branch 5 times, most recently from e78692f to 097d594 Compare February 2, 2026 04:32

adamsaghy requested changes Feb 2, 2026

View reviewed changes

Saifulhuq01 force-pushed the fix-email-sql-injection branch from 097d594 to 5946091 Compare February 3, 2026 04:18

Saifulhuq01 requested a review from adamsaghy February 3, 2026 04:20

adamsaghy reviewed Feb 3, 2026

View reviewed changes

Saifulhuq01 force-pushed the fix-email-sql-injection branch from 5946091 to cb18b0e Compare February 3, 2026 10:31

adamsaghy reviewed Feb 3, 2026

View reviewed changes

FINERACT-2461: Refactor EmailReadPlatformServiceImpl to use Prepared …

23fb382

…Statements

Saifulhuq01 force-pushed the fix-email-sql-injection branch from cb18b0e to 23fb382 Compare February 3, 2026 10:45

adamsaghy requested changes Feb 3, 2026

View reviewed changes

Saifulhuq01 requested a review from adamsaghy February 3, 2026 11:24

adamsaghy reviewed Feb 3, 2026

View reviewed changes

Saifulhuq01 force-pushed the fix-email-sql-injection branch 2 times, most recently from 986770c to 2c62dc8 Compare February 3, 2026 14:43

Saifulhuq01 requested a review from adamsaghy February 3, 2026 14:47

FINERACT-2461: Add unit tests for EmailReadPlatformServiceImpl

dcbb3b7

Saifulhuq01 force-pushed the fix-email-sql-injection branch from 2c62dc8 to dcbb3b7 Compare February 3, 2026 15:10

adamsaghy merged commit f5951f1 into apache:develop Feb 6, 2026
36 checks passed

Conversation

Saifulhuq01 commented Jan 30, 2026

Description

Changes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleksii-novikov-onix commented Jan 30, 2026

Uh oh!

Saifulhuq01 commented Jan 31, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

Saifulhuq01 commented Feb 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

Saifulhuq01 commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Saifulhuq01 commented Feb 3, 2026 •

edited

Loading