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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
