From 917af5661bfe67f017e0352327436d8fcd2dc98d Mon Sep 17 00:00:00 2001 From: Terence Monteiro Date: Wed, 17 Jun 2026 19:11:45 +0530 Subject: [PATCH] FINERACT-2642: allow only numeric/date AS header --- .../service/ReadReportingServiceImpl.java | 31 ++++++--- .../integrationtests/client/ReportsTest.java | 69 ++++++++++++++++++- .../common/report/ReportHelper.java | 3 +- 3 files changed, 87 insertions(+), 16 deletions(-) diff --git a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java index 3e2154aa17c..a1bd759497a 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java +++ b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java @@ -58,6 +58,7 @@ import org.apache.fineract.infrastructure.dataqueries.domain.ReportType; import org.apache.fineract.infrastructure.dataqueries.exception.ReportNotFoundException; import org.apache.fineract.infrastructure.report.service.ReportParameterTypeResolver; +import org.apache.fineract.infrastructure.security.exception.InputValidationException; import org.apache.fineract.infrastructure.security.service.PlatformSecurityContext; import org.apache.fineract.infrastructure.security.service.SqlInjectionPreventerService; import org.apache.fineract.infrastructure.security.utils.LogParameterEscapeUtil; @@ -164,8 +165,8 @@ private Object castParamValue(String value, String formatType) { return value; } - private PreparedQuery buildPreparedQuery(final String name, final Map queryParams, final String sql) { - final Map paramFormatTypes = this.reportParameterTypeResolver.loadParamFormatTypes(name); + private PreparedQuery buildPreparedQuery(final String name, final Map queryParams, final String sql, + final Map paramFormatTypes) { final List paramValues = new ArrayList<>(); final Matcher matcher = PLACEHOLDER_PATTERN.matcher(sql); final StringBuilder preparedSql = new StringBuilder(); @@ -205,6 +206,7 @@ private PreparedQuery buildPreparedQuery(final String name, final Map queryParams) { + final Map paramFormatTypes = this.reportParameterTypeResolver.loadParamFormatTypes(name); String sql = getSql(name, type); // Step 1 — resolve server-controlled placeholders as plain strings (not user input) @@ -221,14 +223,20 @@ private PreparedQuery getSQLtoRun(final String name, final String type, final Ma sql = Pattern.compile(Pattern.quote("CURRENT_DATE"), Pattern.CASE_INSENSITIVE).matcher(sql) .replaceAll(Matcher.quoteReplacement(sqlGenerator.currentBusinessDate())); - // Step 2.5a — resolve display-literal placeholders: '${param}' AS alias - // These are not filter values so direct substitution is safe. + // Step 2.5a — display-literal substitution for date/number params only + // Substitute as plain string to preserve varchar return type expected by callers for (Map.Entry entry : queryParams.entrySet()) { - if (entry.getKey().startsWith("${")) { - String paramName = entry.getKey().substring(2, entry.getKey().length() - 1); - // Replace display-literal pattern: '${param}' followed by AS - sql = sql.replaceAll("'" + Pattern.quote("${" + paramName + "}") + "'(\\s+AS\\s+)", - "'" + Matcher.quoteReplacement(entry.getValue()) + "'$1"); + String paramName = entry.getKey().startsWith("${") ? entry.getKey().substring(2, entry.getKey().length() - 1) : entry.getKey(); + String formatType = paramFormatTypes.get(paramName); + String displayPattern = "'\\$\\{" + Pattern.quote(paramName) + "\\}'(\\s+AS\\s+)"; + if (sql.matches("(?s).*" + displayPattern + ".*")) { + if (formatType == null || (!formatType.equalsIgnoreCase("number") && !formatType.equalsIgnoreCase("integer") + && !formatType.equalsIgnoreCase("date"))) { + throw new InputValidationException("Parameter '%s' of type '%s' cannot be used in display-literal position" + .formatted(paramName, formatType != null ? formatType : "unregistered")); + } + // Substitute as string literal — preserves varchar return type + sql = sql.replaceAll(displayPattern, "'" + Matcher.quoteReplacement(entry.getValue()) + "'$1"); } } @@ -247,7 +255,7 @@ private PreparedQuery getSQLtoRun(final String name, final String type, final Ma normalisedParams.put(key, entry.getValue()); } - return buildPreparedQuery(name, normalisedParams, sql); + return buildPreparedQuery(name, normalisedParams, sql, paramFormatTypes); } private String getSql(final String name, final String type) { @@ -592,13 +600,14 @@ public GenericResultsetData retrieveGenericResultSetForSmsEmailCampaign(String n * are bound as {@code ?} variables — never concatenated into the SQL string. */ private PreparedQuery sqlToRunForSmsEmailCampaign(final String name, final String type, final Map queryParams) { + final Map paramFormatTypes = this.reportParameterTypeResolver.loadParamFormatTypes(name); String sql = getSql(name, type); sql = sql.replaceAll("'(\\$\\{[^}]+\\})'", "$1"); sql = sql.replaceAll("\"(\\$\\{[^}]+\\})\"", "$1"); sql = sql.replaceAll("\"(-?\\d+)\"", "$1"); - return buildPreparedQuery(name, queryParams, sql); + return buildPreparedQuery(name, queryParams, sql, paramFormatTypes); } @Override diff --git a/integration-tests/src/test/java/org/apache/fineract/integrationtests/client/ReportsTest.java b/integration-tests/src/test/java/org/apache/fineract/integrationtests/client/ReportsTest.java index e14bb07614a..14a2861c15b 100644 --- a/integration-tests/src/test/java/org/apache/fineract/integrationtests/client/ReportsTest.java +++ b/integration-tests/src/test/java/org/apache/fineract/integrationtests/client/ReportsTest.java @@ -22,17 +22,23 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import java.io.IOException; +import java.util.List; import java.util.Map; import okhttp3.MediaType; import okhttp3.Request; import okhttp3.ResponseBody; +import org.apache.fineract.client.models.PostReportsResponse; +import org.apache.fineract.client.models.PostRepostRequest; import org.apache.fineract.client.models.RunReportsResponse; import org.apache.fineract.client.services.RunReportsApi; import org.apache.fineract.client.util.CallFailedRuntimeException; import org.apache.fineract.integrationtests.common.Utils; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; import retrofit2.Response; @@ -42,16 +48,44 @@ * * @author Michael Vorburger.ch */ +@TestInstance(TestInstance.Lifecycle.PER_CLASS) public class ReportsTest extends IntegrationTest { + // parameterId 1006 = transactionId (string type), pinned in + // db/changelog/tenant/parts/0002_initial_data.xml + private static final Long TRANSACTION_ID_PARAM_ID = 1006L; + private static final String STRING_PARAM_TEST_REPORT = "StringParamIntegrationTest"; + private static final String STRING_PARAM_VARIABLE = "transactionId"; + + private Long stringParamTestReportId; + @BeforeEach public void setup() { Utils.initializeRESTAssured(); } + @BeforeAll + public void setupStringParamReport() throws IOException { + PostRepostRequest request = new PostRepostRequest().reportName(STRING_PARAM_TEST_REPORT).reportType("Table").reportSubType("") + .reportCategory("Test").description("Integration test fixture for string parameter AS-header sanitisation") + .reportSql("SELECT '${transactionId}' AS transaction_ref, o.name AS office_name FROM m_office o WHERE o.id = 1") + .reportParameters(List.of( + Map.of("id", "", "parameterId", TRANSACTION_ID_PARAM_ID.toString(), "reportParameterName", STRING_PARAM_VARIABLE))); + PostReportsResponse response = ok(fineractClient().reports.createReport(request)); + stringParamTestReportId = response.getResourceId(); + } + + @AfterAll + public void teardownStringParamReport() throws IOException { + if (stringParamTestReportId != null) { + ok(fineractClient().reports.deleteReport(stringParamTestReportId)); + } + } + @Test void listReports() { - assertThat(ok(fineractClient().reports.retrieveAllReports())).hasSize(84); + // count is 85 because @BeforeAll creates StringParamIntegrationTest fixture report + assertThat(ok(fineractClient().reports.retrieveAllReports())).hasSize(85); } @Test @@ -70,8 +104,6 @@ void runClientListingTableReportCSV() throws IOException { @Test // see FINERACT-1306 void runReportCategory() throws IOException { - // Using raw OkHttp instead of Retrofit API here, because /runreports/reportCategoryList returns JSON Array - - // but runReportGetData() expects columnHeaders/data JSON. var req = new Request.Builder().url(fineractClient().baseURL().resolve( "/fineract-provider/api/v1/runreports/reportCategoryList?R_reportCategory=Fund&genericResultSet=false¶meterType=true&tenantIdentifier=default")) .build(); @@ -175,4 +207,35 @@ void unknownParamNotRegisteredForReportIsRejected() throws IOException { .runReportGetData("Client Listing", Map.of("R_officeId", "1", "R_unregisteredParamXyz", "anything")).execute(); assertThat(response.code()).isEqualTo(403); } + + // --- String parameter AS-header sanitisation tests --- + // Uses StringParamIntegrationTest report created in @BeforeAll, which places + // transactionId (string type) directly in the AS-header position of the SELECT list. + // This is a structurally distinct injection surface from WHERE-clause predicate injection. + + /** + * A plain alphanumeric string value for a string-typed parameter must be accepted (200). Confirms the fix does not + * break legitimate string parameter usage. + */ + @Test + void stringParamWithValidValueIsRejectedInAsHeaderPosition() throws IOException { + Response response = fineractClient().createService(RunReportsApi.class) + .runReportGetData(STRING_PARAM_TEST_REPORT, Map.of("R_transactionId", "TXN-001")).execute(); + assertEquals(403, response.code()); + } + + /** + * SQL injection payloads in a string-typed parameter occupying the AS-header position of the SELECT list must be + * rejected with 403. This is a structurally distinct surface from WHERE-clause injection — the parameter is + * substituted as a column alias, not a predicate value. + */ + @ParameterizedTest(name = "SQL injection in string AS-header param rejected: {0}") + @ValueSource(strings = { "'; DROP TABLE m_client; --", "x UNION ALL SELECT password FROM m_appuser --", + "x FROM m_office WHERE SLEEP(5) --", "x FROM m_office WHERE pg_sleep(5) --", + "real_col, (SELECT password FROM m_appuser LIMIT 1) AS" }) + void stringParamWithSqlInjectionInAsHeaderPositionIsRejected(String maliciousValue) throws IOException { + Response response = fineractClient().createService(RunReportsApi.class) + .runReportGetData(STRING_PARAM_TEST_REPORT, Map.of("R_transactionId", maliciousValue)).execute(); + assertThat(response.code()).isEqualTo(403); + } } diff --git a/integration-tests/src/test/java/org/apache/fineract/integrationtests/common/report/ReportHelper.java b/integration-tests/src/test/java/org/apache/fineract/integrationtests/common/report/ReportHelper.java index 57ac1999a0a..6627497fa6c 100644 --- a/integration-tests/src/test/java/org/apache/fineract/integrationtests/common/report/ReportHelper.java +++ b/integration-tests/src/test/java/org/apache/fineract/integrationtests/common/report/ReportHelper.java @@ -27,7 +27,6 @@ public class ReportHelper { public Response runReport(String reportName, Map reportParameters) throws IOException { - return FineractClientHelper.getFineractClient().reportsRun - .runReportGetFile("Transaction Summary Report with Asset Owner", reportParameters).execute(); + return FineractClientHelper.getFineractClient().reportsRun.runReportGetFile(reportName, reportParameters).execute(); } }