DeathGun44 commented on code in PR #6513:
URL: https://github.com/apache/fineract/pull/6513#discussion_r4155566140
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/CenterIntegrationTest.java:
##########
@@ -133,38 +121,39 @@ public void testFullCenterCreation() {
public void testListCentersRejectsSqlInjectionInOrderBy() {
final String maliciousOrderBy = "id, (select password from m_appuser
limit 1)-- -";
- final Object response = CenterHelper.listCentersRaw(maliciousOrderBy,
"ASC", false, requestSpec, expectBadRequest);
- assertNotNull(response, "Expected a validation-error response body,
not a silent 200 with leaked data");
+ assertEquals(BAD_REQUEST,
centerHelper.listCentersExpectingError(maliciousOrderBy, "ASC",
false).getStatus(),
+ "Expected a validation error, not a silent 200 with leaked
data");
}
/** Same injection attempt against the paginated listing endpoint, which
the patch modifies separately. */
@Test
public void testPaginatedListCentersRejectsSqlInjectionInOrderBy() {
final String maliciousOrderBy = "id, (select password from m_appuser
limit 1)-- -";
- final Object response = CenterHelper.listCentersRaw(maliciousOrderBy,
"ASC", true, requestSpec, expectBadRequest);
- assertNotNull(response, "Expected a validation-error response body,
not a silent 200 with leaked data");
+ assertEquals(BAD_REQUEST,
centerHelper.listCentersExpectingError(maliciousOrderBy, "ASC",
true).getStatus(),
+ "Expected a validation error, not a silent 200 with leaked
data");
}
/**
* Regression test for the new strict ASC/DESC allow-list on {@code
sortOrder}: any value other than exactly
- * ASC/DESC — including an injection payload appended to a nominally valid
value — must be rejected.
+ * ASC/DESC — including an injection payload appended to a nominally valid
value — must be rejected. The SQL
+ * validator does not catch this payload, so only the allow-list stands
between it and the ORDER BY clause.
*/
@Test
public void testListCentersRejectsInvalidSortOrderValue() {
- final String maliciousSortOrder = "ASC; DROP TABLE m_office; --";
Review Comment:
The original payload was never reaching the server intact: listCentersRaw
URL-encoded it and REST Assured encoded it again, so the server logged
ASC%3B+DROP+TABLE+m_office%3B+-- and rejected it only because that isn't
ASC/DESC. Sent correctly, ASC; DROP TABLE … is caught earlier by
DefaultSqlValidator (403, inject-stacked-query) and never reaches the new
sortOrder allow-list the test is documented to cover. I switched to ASC,
(select password from m_appuser limit 1), which passes the SQL validator and is
rejected only by the allow-list (400).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
