You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2022/03/29 07:00:48 UTC
[GitHub] [fineract] galovics commented on a change in pull request #2205: FINERACT-1483: Fix sonar vulnerabilities with Blocker and Critical severity
galovics commented on a change in pull request #2205:
URL: https://github.com/apache/fineract/pull/2205#discussion_r837090141
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/PaginationHelper.java
##########
@@ -41,15 +43,17 @@ public PaginationHelper(DatabaseSpecificSQLGenerator sqlGenerator, DatabaseTypeR
public <E> Page<E> fetchPage(final JdbcTemplate jt, final String sqlFetchRows, final Object[] args, final RowMapper<E> rowMapper) {
- final List<E> items = jt.query(sqlFetchRows, rowMapper, args);
+ final String validatedSqlFetchRows = ESAPI.encoder().encodeForSQL(new MySQLCodec(MySQLCodec.Mode.STANDARD), sqlFetchRows);
Review comment:
This is probably not the best idea here.
On one hand, let's not duplicate this logic everywhere (getting an encoder and passing a MySQL codec everywhere) but let's extract this to somewhere.
Plus, on the other hand, this codec is encoding for MySQL only while Fineract supports PostgreSQL as well. The encoding could simply result in incorrectly formatted SQL strings which is not what we want.
As far as I can tell the OWASP library doesn't have a PostgreSQL specific codec but another thing I quickly found was this: https://stackoverflow.com/questions/43645748/is-using-org-postgresql-core-utils-escapeliteral-enough-to-prevent-sql-injection
Take a look please. Thanks!
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java
##########
@@ -368,10 +376,10 @@ public void deregisterDatatable(final String datatable) {
final String deleteFromConfigurationSql = "delete from c_configuration where name ='" + datatable + "'";
String[] sqlArray = new String[4];
- sqlArray[0] = deleteRolePermissionsSql;
- sqlArray[1] = deletePermissionsSql;
- sqlArray[2] = deleteRegisteredDatatableSql;
- sqlArray[3] = deleteFromConfigurationSql;
+ sqlArray[0] = ESAPI.encoder().encodeForSQL(new MySQLCodec(MySQLCodec.Mode.STANDARD), deleteRolePermissionsSql);
Review comment:
Same deal as above.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java
##########
@@ -187,21 +190,27 @@ public DatatableData retrieveDatatable(final String datatable) {
final String sql = "select application_table_name, registered_table_name, entity_subtype" + " from x_registered_table "
+ " where exists" + " (select 'f'" + " from m_appuser_role ur " + " join m_role r on r.id = ur.role_id"
+ " left join m_role_permission rp on rp.role_id = r.id" + " left join m_permission p on p.id = rp.permission_id"
- + " where ur.appuser_id = " + this.context.authenticatedUser().getId() + " and registered_table_name='" + datatable + "'"
- + " and (p.code in ('ALL_FUNCTIONS', 'ALL_FUNCTIONS_READ') or p.code = concat('READ_', registered_table_name))) "
+ + " where ur.appuser_id = ? and registered_table_name=? and (p.code in ('ALL_FUNCTIONS', "
+ + "'ALL_FUNCTIONS_READ') or p.code = concat('READ_', registered_table_name))) "
+ " order by application_table_name, registered_table_name";
- final SqlRowSet rs = this.jdbcTemplate.queryForRowSet(sql);
-
DatatableData datatableData = null;
- while (rs.next()) {
- final String appTableName = rs.getString("application_table_name");
- final String registeredDatatableName = rs.getString("registered_table_name");
- final String entitySubType = rs.getString("entity_subtype");
- final List<ResultsetColumnHeaderData> columnHeaderData = this.genericDataService
- .fillResultsetColumnHeaders(registeredDatatableName);
-
- datatableData = DatatableData.create(appTableName, registeredDatatableName, entitySubType, columnHeaderData);
+ try (Connection connection = jdbcTemplate.getDataSource().getConnection()) {
Review comment:
Same as above.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java
##########
@@ -248,7 +248,7 @@ public String retrieveReportPDF(final String reportName, final String type, fina
final Document document = new Document(PageSize.B0.rotate());
- PdfWriter.getInstance(document, new FileOutputStream(new File(fileLocation + reportName + ".pdf")));
+ PdfWriter.getInstance(document, new FileOutputStream(fileLocation + reportName + ".pdf")); // NOSONAR
Review comment:
Wouldn't the ESAPI UnixCodec help here? https://www.javadoc.io/doc/org.owasp.esapi/esapi/2.1.0.1/org/owasp/esapi/codecs/UnixCodec.html
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java
##########
@@ -146,34 +152,31 @@ public ReadWriteNonCoreDataServiceImpl(final JdbcTemplate jdbcTemplate, final Na
@Override
public List<DatatableData> retrieveDatatableNames(final String appTable) {
- String andClause;
- if (appTable == null) {
- andClause = "";
- } else {
- validateAppTable(appTable);
- SQLInjectionValidator.validateSQLInput(appTable);
- andClause = " and application_table_name = '" + appTable + "'";
- }
-
// PERMITTED datatables
final String sql = "select application_table_name, registered_table_name, entity_subtype " + " from x_registered_table "
+ " where exists" + " (select 'f'" + " from m_appuser_role ur " + " join m_role r on r.id = ur.role_id"
+ " left join m_role_permission rp on rp.role_id = r.id" + " left join m_permission p on p.id = rp.permission_id"
- + " where ur.appuser_id = " + this.context.authenticatedUser().getId()
- + " and (p.code in ('ALL_FUNCTIONS', 'ALL_FUNCTIONS_READ') or p.code = concat('READ_', registered_table_name))) "
- + andClause + " order by application_table_name, registered_table_name";
-
- final SqlRowSet rs = this.jdbcTemplate.queryForRowSet(sql);
+ + " where ur.appuser_id = ? and (p.code in ('ALL_FUNCTIONS', 'ALL_FUNCTIONS_READ') or p.code = concat"
+ + "('READ_', registered_table_name))) "
+ + " and application_table_name like ? order by application_table_name, registered_table_name";
final List<DatatableData> datatables = new ArrayList<>();
- while (rs.next()) {
- final String appTableName = rs.getString("application_table_name");
- final String registeredDatatableName = rs.getString("registered_table_name");
- final String entitySubType = rs.getString("entity_subtype");
- final List<ResultsetColumnHeaderData> columnHeaderData = this.genericDataService
- .fillResultsetColumnHeaders(registeredDatatableName);
-
- datatables.add(DatatableData.create(appTableName, registeredDatatableName, entitySubType, columnHeaderData));
+ try (Connection connection = jdbcTemplate.getDataSource().getConnection()) {
Review comment:
Question, why do we want to use a plain connection here instead of executing the PreparedStatement through the JdbcTemplate itself since internally it creates its own?
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org