Skip to content

FINERACT-2735: Validate datatables order input on actual columns #6230

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

Open
terencemo wants to merge 1 commit into
base: develop
Choose a base branch
from
+362 −8
Open

FINERACT-2735: Validate datatables order input on actual columns #6230

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
104 changes: 100 additions & 4 deletions ...r/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/DatatableUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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<String> ALLOWED_SORT_DIRECTIONS = Set.of("ASC", "DESC");

public boolean isMultirowDatatable(final List<ResultsetColumnHeaderData> columnHeaders) {
return searchUtil.findFiltered(columnHeaders, e -> e.isNamed(TABLE_FIELD_ID)) != null;
Expand Down Expand Up @@ -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<ResultsetColumnHeaderData> columnHeaders) {
if (StringUtils.isBlank(order)) {
return null;
}
final String trimmedOrder = order.trim();
if (trimmedOrder.isEmpty()) {
return null;
}

final Set<String> 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<ResultsetColumnHeaderData> columnHeaders = genericDataService.fillResultsetColumnHeaders(dataTableName);
Expand All @@ -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<ResultsetRowData> result = genericDataService.fillResultsetRowData(sql, columnHeaders, params.toArray());
Expand Down
117 changes: 113 additions & 4 deletions ...c/test/java/org/apache/fineract/infrastructure/dataqueries/service/DatatableUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -232,4 +232,113 @@ private List<ResultsetColumnHeaderData> createMultiRowHeaders() {
headers.add(ResultsetColumnHeaderData.basic("loan_id", "bigint", DatabaseType.MYSQL));
return headers;
}

// ---- order-by allowlist validation (fixes SQL injection via ORDER BY) ----

private List<ResultsetColumnHeaderData> createOrderByTestHeaders() {
List<ResultsetColumnHeaderData> 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<String> 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<String> 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<String> 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));
}
}
25 changes: 25 additions & 0 deletions ...sts/src/test/java/org/apache/fineract/integrationtests/common/system/DatatableHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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> 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> 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)
Expand Down
Loading
Loading