diff --git a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/DatatableUtil.java b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/DatatableUtil.java index 6577513d3e4..e4c68b6b3cd 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/DatatableUtil.java +++ b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/DatatableUtil.java @@ -25,6 +25,9 @@ import java.util.ArrayList; import java.util.List; +import java.util.Locale; +import java.util.Set; +import java.util.stream.Collectors; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.StringUtils; @@ -40,7 +43,6 @@ import org.apache.fineract.infrastructure.dataqueries.exception.DatatableSystemErrorException; import org.apache.fineract.infrastructure.security.service.PlatformSecurityContext; import org.apache.fineract.infrastructure.security.service.SqlValidator; -import org.apache.fineract.infrastructure.security.utils.ColumnValidator; import org.apache.fineract.portfolio.search.service.SearchUtil; import org.apache.fineract.useradministration.domain.AppUser; import org.springframework.jdbc.core.JdbcTemplate; @@ -61,7 +63,9 @@ public class DatatableUtil { private final PlatformSecurityContext context; private final GenericDataService genericDataService; private final DatabaseSpecificSQLGenerator sqlGenerator; - private final ColumnValidator columnValidator; + private static final char DOUBLE_QUOTE = '"'; + private static final char BACKTICK = '`'; + private static final Set ALLOWED_SORT_DIRECTIONS = Set.of("ASC", "DESC"); public boolean isMultirowDatatable(final List columnHeaders) { return searchUtil.findFiltered(columnHeaders, e -> e.isNamed(TABLE_FIELD_ID)) != null; @@ -232,6 +236,96 @@ public String getClientOfficeJoinCondition(String appTableAlias) { return " join m_client c on c.id = " + appTableAlias + ".client_id " + getOfficeJoinCondition("c"); } + private PlatformDataIntegrityException invalidOrderException(final String value) { + return new PlatformDataIntegrityException("error.msg.datatables.orderby.invalid", "Invalid order by parameter: " + value, "order", + value); + } + + private String unescapeQuotedIdentifier(final String raw, final char quoteChar) { + final String doubled = "" + quoteChar + quoteChar; + return raw.replace(doubled, String.valueOf(quoteChar)); + } + + /** + * Finds the index of the closing quote matching the opening quote at index 0, honoring the SQL-standard + * doubled-quote escape (e.g. {@code "a""b"} represents the identifier {@code a"b}). Returns -1 if unterminated. + */ + private int findClosingQuote(final String input, final char quoteChar) { + int i = 1; + while (i < input.length()) { + if (input.charAt(i) == quoteChar) { + if (i + 1 < input.length() && input.charAt(i + 1) == quoteChar) { + i += 2; // escaped quote inside identifier, keep scanning + continue; + } + return i; + } + i++; + } + return -1; + } + + /** + * Validates the {@code order} query parameter against the datatable's actual columns and returns a dialect-escaped, + * safe-to-concatenate ORDER BY clause fragment (without the "order by" keywords), or {@code null} if {@code order} + * is blank. + * + * Supported grammar: a single column name, optionally double-quoted (Postgres/ANSI) or backtick-quoted + * (MySQL/MariaDB) — quoting is required only when the column name contains a space or other character that would + * otherwise be ambiguous — followed by an optional ASC/DESC direction token. + * + * Examples: {@code client_id}, {@code client_id DESC}, {@code "Birth Date"}, {@code `Birth Date` ASC}. + */ + String validateAndBuildOrderClause(final String order, final List columnHeaders) { + if (StringUtils.isBlank(order)) { + return null; + } + final String trimmedOrder = order.trim(); + if (trimmedOrder.isEmpty()) { + return null; + } + + final Set allowedColumns = columnHeaders.stream().map(ResultsetColumnHeaderData::getColumnName).collect(Collectors.toSet()); + + final String columnName; + final String remainder; + + final char firstChar = trimmedOrder.charAt(0); + if (firstChar == DOUBLE_QUOTE || firstChar == BACKTICK) { + final int closingIndex = findClosingQuote(trimmedOrder, firstChar); + if (closingIndex < 0) { + throw invalidOrderException(order); + } + columnName = unescapeQuotedIdentifier(trimmedOrder.substring(1, closingIndex), firstChar); + remainder = trimmedOrder.substring(closingIndex + 1).trim(); + } else { + final int spaceIndex = trimmedOrder.indexOf(' '); + if (spaceIndex < 0) { + columnName = trimmedOrder; + remainder = ""; + } else { + columnName = trimmedOrder.substring(0, spaceIndex).trim(); + remainder = trimmedOrder.substring(spaceIndex + 1).trim(); + } + } + + if (!allowedColumns.contains(columnName)) { + throw invalidOrderException(order); + } + + String direction = null; + if (StringUtils.isNotBlank(remainder)) { + final String upperRemainder = remainder.toUpperCase(Locale.ROOT); + if (!ALLOWED_SORT_DIRECTIONS.contains(upperRemainder)) { + throw invalidOrderException(order); + } + direction = upperRemainder; + } + + final String escapedColumn = sqlGenerator.escape(columnName); + return direction != null ? escapedColumn + " " + direction : escapedColumn; + } + public GenericResultsetData retrieveDataTableGenericResultSet(final EntityTables entityTable, final String dataTableName, final Long appTableId, final String order, final Long id) { final List columnHeaders = genericDataService.fillResultsetColumnHeaders(dataTableName); @@ -247,8 +341,10 @@ public GenericResultsetData retrieveDataTableGenericResultSet(final EntityTables params.add(id); } if (StringUtils.isNotBlank(order)) { - columnValidator.validateSqlInjection(sql, order); - sql = sql + " order by " + order; + String validatedOrder = validateAndBuildOrderClause(order, columnHeaders); + if (validatedOrder != null) { + sql = sql + " order by " + validatedOrder; + } } final List result = genericDataService.fillResultsetRowData(sql, columnHeaders, params.toArray()); diff --git a/fineract-provider/src/test/java/org/apache/fineract/infrastructure/dataqueries/service/DatatableUtilTest.java b/fineract-provider/src/test/java/org/apache/fineract/infrastructure/dataqueries/service/DatatableUtilTest.java index 635e73cd205..19656a7a0ef 100644 --- a/fineract-provider/src/test/java/org/apache/fineract/infrastructure/dataqueries/service/DatatableUtilTest.java +++ b/fineract-provider/src/test/java/org/apache/fineract/infrastructure/dataqueries/service/DatatableUtilTest.java @@ -20,21 +20,23 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import java.util.ArrayList; import java.util.List; +import org.apache.fineract.infrastructure.core.exception.PlatformDataIntegrityException; import org.apache.fineract.infrastructure.core.service.database.DatabaseSpecificSQLGenerator; import org.apache.fineract.infrastructure.core.service.database.DatabaseType; import org.apache.fineract.infrastructure.dataqueries.data.EntityTables; import org.apache.fineract.infrastructure.dataqueries.data.ResultsetColumnHeaderData; import org.apache.fineract.infrastructure.security.service.PlatformSecurityContext; import org.apache.fineract.infrastructure.security.service.SqlValidator; -import org.apache.fineract.infrastructure.security.utils.ColumnValidator; import org.apache.fineract.organisation.office.domain.Office; import org.apache.fineract.portfolio.search.service.SearchUtil; import org.apache.fineract.useradministration.domain.AppUser; @@ -69,14 +71,12 @@ class DatatableUtilTest { private GenericDataService genericDataService; @Mock private DatabaseSpecificSQLGenerator sqlGenerator; - @Mock - private ColumnValidator columnValidator; private DatatableUtil underTest; @BeforeEach void setUp() { - underTest = new DatatableUtil(searchUtil, jdbcTemplate, sqlValidator, context, genericDataService, sqlGenerator, columnValidator); + underTest = new DatatableUtil(searchUtil, jdbcTemplate, sqlValidator, context, genericDataService, sqlGenerator); setupSecurityContext(); } @@ -232,4 +232,113 @@ private List createMultiRowHeaders() { headers.add(ResultsetColumnHeaderData.basic("loan_id", "bigint", DatabaseType.MYSQL)); return headers; } + + // ---- order-by allowlist validation (fixes SQL injection via ORDER BY) ---- + + private List createOrderByTestHeaders() { + List headers = new ArrayList<>(); + headers.add(ResultsetColumnHeaderData.basic("client_id", "bigint", DatabaseType.MYSQL)); + headers.add(ResultsetColumnHeaderData.basic("Gender_cd_Question", "int", DatabaseType.MYSQL)); + headers.add(ResultsetColumnHeaderData.basic("Some Decimal", "decimal", DatabaseType.MYSQL)); + headers.add(ResultsetColumnHeaderData.basic("Birth Date", "date", DatabaseType.MYSQL)); + return headers; + } + + private void setupOrderByTestMocks() { + // identity escape so assertions can check for the raw column name in the captured SQL + when(sqlGenerator.escape(anyString())).thenAnswer(invocation -> invocation.getArgument(0)); + when(genericDataService.fillResultsetColumnHeaders(anyString())).thenReturn(createOrderByTestHeaders()); + when(searchUtil.findFiltered(any(), any())).thenReturn(null); // single-row datatable, keeps SQL simple + when(genericDataService.fillResultsetRowData(anyString(), any(), any(Object[].class))).thenReturn(new ArrayList<>()); + } + + @Test + void testRetrieveDataTableGenericResultSetAppendsValidColumnOrderBy() { + setupOrderByTestMocks(); + + underTest.retrieveDataTableGenericResultSet(EntityTables.LOAN, "test_datatable", APP_TABLE_ID, "client_id DESC", null); + + ArgumentCaptor sqlCaptor = ArgumentCaptor.forClass(String.class); + verify(genericDataService).fillResultsetRowData(sqlCaptor.capture(), any(), any(Object[].class)); + assertTrue(sqlCaptor.getValue().contains("order by client_id DESC"), "SQL should contain the validated order by clause"); + } + + @Test + void testRetrieveDataTableGenericResultSetRejectsUnquotedColumnNameContainingSpace() { + setupOrderByTestMocks(); + + // unquoted "Birth Date" is no longer valid -- it parses as column="Birth", direction="Date", + // and "Date" isn't ASC/DESC, so this must be rejected. Quoting is now required for space-containing names. + assertThrows(PlatformDataIntegrityException.class, + () -> underTest.retrieveDataTableGenericResultSet(EntityTables.LOAN, "test_datatable", APP_TABLE_ID, "Birth Date", null)); + verify(genericDataService, never()).fillResultsetRowData(anyString(), any(), any(Object[].class)); + } + + @Test + void testRetrieveDataTableGenericResultSetOrderByIsCaseInsensitiveForDirection() { + setupOrderByTestMocks(); + + underTest.retrieveDataTableGenericResultSet(EntityTables.LOAN, "test_datatable", APP_TABLE_ID, "client_id desc", null); + + ArgumentCaptor sqlCaptor = ArgumentCaptor.forClass(String.class); + verify(genericDataService).fillResultsetRowData(sqlCaptor.capture(), any(), any(Object[].class)); + assertTrue(sqlCaptor.getValue().contains("order by client_id DESC"), "Direction token should be normalized to uppercase"); + } + + @Test + void testRetrieveDataTableGenericResultSetRejectsUnknownColumnInOrderBy() { + setupOrderByTestMocks(); + + assertThrows(PlatformDataIntegrityException.class, () -> underTest.retrieveDataTableGenericResultSet(EntityTables.LOAN, + "test_datatable", APP_TABLE_ID, "not_a_real_column", null)); + verify(genericDataService, never()).fillResultsetRowData(anyString(), any(), any(Object[].class)); + } + + @Test + void testRetrieveDataTableGenericResultSetRejectsClassicSqlInjectionInOrderBy() { + setupOrderByTestMocks(); + + assertThrows(PlatformDataIntegrityException.class, () -> underTest.retrieveDataTableGenericResultSet(EntityTables.LOAN, + "test_datatable", APP_TABLE_ID, "client_id; DROP TABLE m_client;--", null)); + verify(genericDataService, never()).fillResultsetRowData(anyString(), any(), any(Object[].class)); + } + + @Test + void testRetrieveDataTableGenericResultSetRejectsInjectionDisguisedWithTrailingDirectionToken() { + setupOrderByTestMocks(); + + // guards against a naive "strip trailing ASC/DESC" implementation trusting everything before it + assertThrows(PlatformDataIntegrityException.class, () -> underTest.retrieveDataTableGenericResultSet(EntityTables.LOAN, + "test_datatable", APP_TABLE_ID, "client_id; DROP TABLE m_client;-- ASC", null)); + verify(genericDataService, never()).fillResultsetRowData(anyString(), any(), any(Object[].class)); + } + + @Test + void testRetrieveDataTableGenericResultSetRejectsWhenAnyColumnInMultiListIsInvalid() { + setupOrderByTestMocks(); + + assertThrows(PlatformDataIntegrityException.class, () -> underTest.retrieveDataTableGenericResultSet(EntityTables.LOAN, + "test_datatable", APP_TABLE_ID, "client_id,not_a_real_column", null)); + verify(genericDataService, never()).fillResultsetRowData(anyString(), any(), any(Object[].class)); + } + + @Test + void testRetrieveDataTableGenericResultSetOmitsOrderByWhenOrderIsBlank() { + setupOrderByTestMocks(); + + underTest.retrieveDataTableGenericResultSet(EntityTables.LOAN, "test_datatable", APP_TABLE_ID, " ", null); + + ArgumentCaptor sqlCaptor = ArgumentCaptor.forClass(String.class); + verify(genericDataService).fillResultsetRowData(sqlCaptor.capture(), any(), any(Object[].class)); + assertFalse(sqlCaptor.getValue().contains("order by"), "Blank order param should not produce an order by clause"); + } + + @Test + void testRetrieveDataTableGenericResultSetTreatsLiteralPlusAsPlusNotSpace() { + setupOrderByTestMocks(); // uses createOrderByTestHeaders(), which doesn't include a "+"-containing column + + assertThrows(PlatformDataIntegrityException.class, + () -> underTest.retrieveDataTableGenericResultSet(EntityTables.LOAN, "test_datatable", APP_TABLE_ID, "Birth+Date", null)); + verify(genericDataService, never()).fillResultsetRowData(anyString(), any(), any(Object[].class)); + } } diff --git a/integration-tests/src/test/java/org/apache/fineract/integrationtests/common/system/DatatableHelper.java b/integration-tests/src/test/java/org/apache/fineract/integrationtests/common/system/DatatableHelper.java index fefd37aaeec..c0f70473f85 100644 --- a/integration-tests/src/test/java/org/apache/fineract/integrationtests/common/system/DatatableHelper.java +++ b/integration-tests/src/test/java/org/apache/fineract/integrationtests/common/system/DatatableHelper.java @@ -233,6 +233,31 @@ public Date readDatatableEntry(final String datatableName, final Integer resourc return Utils.convertJsonElementAsDate(jsonElement); } + // TODO: Rewrite to use fineract-client instead! + // Example: org.apache.fineract.integrationtests.common.loans.LoanTransactionHelper.disburseLoan(java.lang.Long, + // org.apache.fineract.client.models.PostLoansLoanIdRequest) + @Deprecated(forRemoval = true) + public T readDatatableEntryWithOrder(final String datatableName, final Integer resourceId, final boolean genericResultset, + final String order, final String jsonAttributeToGetBack) { + final String orderParam = order == null ? "" : "&order=" + order; + return Utils.performServerGet(this.requestSpec, this.responseSpec, DATATABLE_URL + "/" + datatableName + "/" + resourceId + + "?genericResultSet=" + genericResultset + orderParam + "&" + Utils.TENANT_IDENTIFIER, jsonAttributeToGetBack); + } + + // TODO: Rewrite to use fineract-client instead! + // Example: org.apache.fineract.integrationtests.common.loans.LoanTransactionHelper.disburseLoan(java.lang.Long, + // org.apache.fineract.client.models.PostLoansLoanIdRequest) + @Deprecated(forRemoval = true) + public T readDatatableManyEntryWithOrder(final String datatableName, final Integer apptableId, final Long datatableId, + final boolean genericResultSet, final String order, final String jsonAttributeToGetBack) { + final String orderParam = order == null ? "" : "&order=" + order; + return Utils + .performServerGet( + this.requestSpec, this.responseSpec, DATATABLE_URL + "/" + datatableName + "/" + apptableId + "/" + datatableId + + "?genericResultSet=" + genericResultSet + "&" + Utils.TENANT_IDENTIFIER + orderParam, + jsonAttributeToGetBack); + } + // TODO: Rewrite to use fineract-client instead! // Example: org.apache.fineract.integrationtests.common.loans.LoanTransactionHelper.disburseLoan(java.lang.Long, // org.apache.fineract.client.models.PostLoansLoanIdRequest) diff --git a/integration-tests/src/test/java/org/apache/fineract/integrationtests/datatable/DatatableIntegrationTest.java b/integration-tests/src/test/java/org/apache/fineract/integrationtests/datatable/DatatableIntegrationTest.java index 54aecf76432..c2fc66e62ce 100644 --- a/integration-tests/src/test/java/org/apache/fineract/integrationtests/datatable/DatatableIntegrationTest.java +++ b/integration-tests/src/test/java/org/apache/fineract/integrationtests/datatable/DatatableIntegrationTest.java @@ -1051,4 +1051,128 @@ public void testDropNullColumnWithData() { this.datatableHelper.deleteDatatable(datatableName); } + @Test + public void validateOrderParameterOnDatatableEntryRead() { + // given: a single-row client datatable with a column name that requires quoting + final List> datatableColumnsList = new ArrayList<>(); + String plainColumn = "plaincolumn"; + String spacedColumn = "Spaced Column"; + addDatatableColumn(datatableColumnsList, plainColumn, "Number", false, null, null); + addDatatableColumn(datatableColumnsList, spacedColumn, "String", false, 20, null); + + final HashMap columnMap = new HashMap<>(); + String datatableName = Utils.uniqueRandomStringGenerator(CLIENT_APP_TABLE_NAME + "_", 5).toLowerCase(); + columnMap.put("datatableName", datatableName); + columnMap.put("apptableName", CLIENT_APP_TABLE_NAME); + columnMap.put("entitySubType", CLIENT_PERSON_SUBTYPE_NAME); + columnMap.put("multiRow", false); + columnMap.put("columns", datatableColumnsList); + + String dtJson = new Gson().toJson(columnMap); + HashMap datatableResponse = this.datatableHelper.createDatatable(dtJson, ""); + String assignedDatatableName = (String) datatableResponse.get("resourceIdentifier"); + assertEquals(datatableName, assignedDatatableName); + + final Integer clientID = ClientHelper.createClientAsPerson(requestSpec, responseSpec); + + final HashMap entryMap = new HashMap<>(); + entryMap.put(plainColumn, Utils.randomNumberGenerator(3)); + entryMap.put(spacedColumn, Utils.randomStringGenerator("", 8)); + entryMap.put("locale", "en"); + this.datatableHelper.createDatatableEntry(datatableName, clientID, true, new Gson().toJson(entryMap)); + + // valid: bare column, no direction + HashMap result = this.datatableHelper.readDatatableEntryWithOrder(datatableName, clientID, true, plainColumn, ""); + assertNotNull(result); + + // valid: bare column with direction + result = this.datatableHelper.readDatatableEntryWithOrder(datatableName, clientID, true, plainColumn + " DESC", ""); + assertNotNull(result); + + // valid: double-quoted column with a space + result = this.datatableHelper.readDatatableEntryWithOrder(datatableName, clientID, true, "\"" + spacedColumn + "\"", ""); + assertNotNull(result); + + // valid: backtick-quoted column with a space, plus direction + result = this.datatableHelper.readDatatableEntryWithOrder(datatableName, clientID, true, "`" + spacedColumn + "` ASC", ""); + assertNotNull(result); + + // invalid: unknown column -- expect 403 + ResponseSpecification responseSpecError403 = new ResponseSpecBuilder().expectStatusCode(403).build(); + DatatableHelper error403Helper = new DatatableHelper(this.requestSpec, responseSpecError403); + error403Helper.readDatatableEntryWithOrder(datatableName, clientID, true, "not_a_real_column", ""); + + // invalid: classic SQL injection payload -- expect 403 + error403Helper.readDatatableEntryWithOrder(datatableName, clientID, true, plainColumn + "; DROP TABLE m_client;--", ""); + + // invalid: unquoted column name containing a space -- expect 403 (quoting is required) + error403Helper.readDatatableEntryWithOrder(datatableName, clientID, true, spacedColumn, ""); + + // cleanup + this.datatableHelper.deleteDatatableEntries(datatableName, clientID, "clientId"); + this.datatableHelper.deleteDatatable(datatableName); + } + + @Test + public void validateOrderParameterOnDatatableManyEntryRead() { + // given: a multi-row client datatable so we can obtain a datatableId to query against + final List> datatableColumnsList = new ArrayList<>(); + String plainColumn = "plaincolumn"; + String spacedColumn = "Spaced Column"; + addDatatableColumn(datatableColumnsList, plainColumn, "Number", false, null, null); + addDatatableColumn(datatableColumnsList, spacedColumn, "String", false, 20, null); + + final HashMap columnMap = new HashMap<>(); + String datatableName = Utils.uniqueRandomStringGenerator(CLIENT_APP_TABLE_NAME + "_", 5).toLowerCase(); + columnMap.put("datatableName", datatableName); + columnMap.put("apptableName", CLIENT_APP_TABLE_NAME); + columnMap.put("entitySubType", CLIENT_PERSON_SUBTYPE_NAME); + columnMap.put("multiRow", true); + columnMap.put("columns", datatableColumnsList); + + HashMap datatableResponse = this.datatableHelper.createDatatable(new Gson().toJson(columnMap), ""); + assertEquals(datatableName, datatableResponse.get("resourceIdentifier")); + + final Integer clientID = ClientHelper.createClientAsPerson(requestSpec, responseSpec); + + final HashMap entryMap = new HashMap<>(); + entryMap.put(plainColumn, Utils.randomNumberGenerator(3)); + entryMap.put(spacedColumn, Utils.randomStringGenerator("", 8)); + entryMap.put("locale", "en"); + PostDataTablesAppTableIdResponse entryResponse = this.datatableHelper.addDatatableEntry(datatableName, clientID, true, + new Gson().toJson(entryMap)); + Long datatableId = entryResponse.getResourceId(); + assertNotNull(datatableId); + + // valid: bare column, no direction + HashMap result = this.datatableHelper.readDatatableManyEntryWithOrder(datatableName, clientID, datatableId, true, + plainColumn, ""); + assertNotNull(result); + + // valid: bare column with direction + result = this.datatableHelper.readDatatableManyEntryWithOrder(datatableName, clientID, datatableId, true, plainColumn + " DESC", + ""); + assertNotNull(result); + + // valid: double-quoted column with a space + result = this.datatableHelper.readDatatableManyEntryWithOrder(datatableName, clientID, datatableId, true, + "\"" + spacedColumn + "\"", ""); + assertNotNull(result); + + // invalid: unknown column -- expect 403 + ResponseSpecification responseSpecError403 = new ResponseSpecBuilder().expectStatusCode(403).build(); + DatatableHelper error403Helper = new DatatableHelper(this.requestSpec, responseSpecError403); + error403Helper.readDatatableManyEntryWithOrder(datatableName, clientID, datatableId, true, "not_a_real_column", ""); + + // invalid: classic SQL injection payload -- expect 403 + error403Helper.readDatatableManyEntryWithOrder(datatableName, clientID, datatableId, true, plainColumn + "; DROP TABLE m_client;--", + ""); + + // invalid: subquery-based injection (the exact class of payload from the security report) -- expect 403 + error403Helper.readDatatableManyEntryWithOrder(datatableName, clientID, datatableId, true, "(SELECT 1 FROM pg_sleep(3))", ""); + + // cleanup + this.datatableHelper.deleteDatatableEntries(datatableName, clientID, "clientId"); + this.datatableHelper.deleteDatatable(datatableName); + } }