Skip to content

FINERACT-2642: allow only numeric/date AS header #5996

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adamsaghy merged 1 commit into from
Jun 17, 2026
+87 −16
Merged

FINERACT-2642: allow only numeric/date AS header #5996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 20 additions & 11 deletions ...java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -164,8 +165,8 @@ private Object castParamValue(String value, String formatType) {
return value;
}

private PreparedQuery buildPreparedQuery(final String name, final Map<String, String> queryParams, final String sql) {
final Map<String, String> paramFormatTypes = this.reportParameterTypeResolver.loadParamFormatTypes(name);
private PreparedQuery buildPreparedQuery(final String name, final Map<String, String> queryParams, final String sql,
final Map<String, String> paramFormatTypes) {
final List<Object> paramValues = new ArrayList<>();
final Matcher matcher = PLACEHOLDER_PATTERN.matcher(sql);
final StringBuilder preparedSql = new StringBuilder();
Expand Down Expand Up @@ -205,6 +206,7 @@ private PreparedQuery buildPreparedQuery(final String name, final Map<String, St
*/
private PreparedQuery getSQLtoRun(final String name, final String type, final Map<String, String> queryParams) {

final Map<String, String> paramFormatTypes = this.reportParameterTypeResolver.loadParamFormatTypes(name);
String sql = getSql(name, type);

// Step 1 — resolve server-controlled placeholders as plain strings (not user input)
Expand All @@ -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<String, String> 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");
}
}

Expand All @@ -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) {
Expand Down Expand Up @@ -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<String, String> queryParams) {
final Map<String, String> 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
Expand Down
69 changes: 66 additions & 3 deletions integration-tests/src/test/java/org/apache/fineract/integrationtests/client/ReportsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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&parameterType=true&tenantIdentifier=default"))
.build();
Expand Down Expand Up @@ -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<RunReportsResponse> 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<RunReportsResponse> response = fineractClient().createService(RunReportsApi.class)
.runReportGetData(STRING_PARAM_TEST_REPORT, Map.of("R_transactionId", maliciousValue)).execute();
assertThat(response.code()).isEqualTo(403);
}
}
3 changes: 1 addition & 2 deletions ...-tests/src/test/java/org/apache/fineract/integrationtests/common/report/ReportHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
public class ReportHelper {

public Response<ResponseBody> runReport(String reportName, Map<String, String> reportParameters) throws IOException {
return FineractClientHelper.getFineractClient().reportsRun
.runReportGetFile("Transaction Summary Report with Asset Owner", reportParameters).execute();
return FineractClientHelper.getFineractClient().reportsRun.runReportGetFile(reportName, reportParameters).execute();
}
}