galovics commented on a change in pull request #2205: URL: https://github.com/apache/fineract/pull/2205#discussion_r838695671
########## File path: fineract-provider/src/main/resources/esapi-java-logging.properties ########## @@ -0,0 +1,24 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +handlers= java.util.logging.ConsoleHandler Review comment: Can't we hook it up with SLF4J? ########## File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java ########## @@ -248,7 +250,8 @@ 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"))); + String validatedFileLocation = ESAPI.encoder().encodeForOS(new UnixCodec(), fileLocation + reportName + ".pdf"); Review comment: Have you been able to test this? I wanna make sure we are not breaking anything. ########## File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/PaginationHelper.java ########## @@ -41,18 +43,32 @@ 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 List<E> items = jt.query(connection -> { + final PreparedStatement preparedStatement = connection.prepareStatement(sqlFetchRows); Review comment: Why do we need to construct the prepared statement manually? We could use the jdbcTemplate's query method with pure SQL strings. Internally that'll be turned into prepared statements. ########## File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/utils/EsapiUtils.java ########## @@ -0,0 +1,49 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.fineract.infrastructure.security.utils; + +import java.sql.SQLException; +import org.apache.fineract.infrastructure.core.service.database.DatabaseTypeResolver; +import org.owasp.esapi.ESAPI; +import org.owasp.esapi.codecs.Codec; +import org.owasp.esapi.codecs.MySQLCodec; +import org.postgresql.core.Utils; + +public final class EsapiUtils { Review comment: Also, I'd probably rename it and not call it EsapiUtils since the PostgreSQL encoding doesn't involve ESAPI. ########## File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/utils/EsapiUtils.java ########## @@ -0,0 +1,49 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.fineract.infrastructure.security.utils; + +import java.sql.SQLException; +import org.apache.fineract.infrastructure.core.service.database.DatabaseTypeResolver; +import org.owasp.esapi.ESAPI; +import org.owasp.esapi.codecs.Codec; +import org.owasp.esapi.codecs.MySQLCodec; +import org.postgresql.core.Utils; + +public final class EsapiUtils { + + private static final Codec<?> MYSQL_CODEC = new MySQLCodec(MySQLCodec.Mode.STANDARD); + + private EsapiUtils() { + throw new IllegalStateException("Utility class"); + } + + public static String encodeSql(String literal, DatabaseTypeResolver databaseTypeResolver) { + if (databaseTypeResolver.isMySQL()) { + return ESAPI.encoder().encodeForSQL(MYSQL_CODEC, literal); + } else if (databaseTypeResolver.isPostgreSQL()) { + try { + return Utils.escapeLiteral(null, literal, true).toString(); + } catch (SQLException e) { + return literal; Review comment: You sure? If there's an exception, that should indicate some error case, wouldn't it? Probably not the best idea to swallow here. What do you think? ########## File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/utils/EsapiUtils.java ########## @@ -0,0 +1,49 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.fineract.infrastructure.security.utils; + +import java.sql.SQLException; +import org.apache.fineract.infrastructure.core.service.database.DatabaseTypeResolver; +import org.owasp.esapi.ESAPI; +import org.owasp.esapi.codecs.Codec; +import org.owasp.esapi.codecs.MySQLCodec; +import org.postgresql.core.Utils; + +public final class EsapiUtils { Review comment: I mean since this class needs the DatabaseTypeResolver bean anyway, why don't we just make this a Spring Bean as well so we can simply autowire it instead of passing it over and over again for each invocation. Thoughts? -- 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]
