vidakovic commented on PR #5916: URL: https://github.com/apache/fineract/pull/5916#issuecomment-4602321672
@terencemo Question: why didn't we use just this service https://github.com/apache/fineract/blob/9e37bc3b66f82322ce50ee4d4bef32694d8a19b6/fineract-core/src/main/java/org/apache/fineract/infrastructure/security/service/SqlValidator.java#L23 to validate? I think that thing should cover most if not all what is done here... and in case something would be missing: just add a pattern in the application.properties at https://github.com/apache/fineract/blob/9e37bc3b66f82322ce50ee4d4bef32694d8a19b6/fineract-provider/src/main/resources/application.properties#L225 If I understood the code here correctly then the only thing left would be the type checking for the columns, that could stay... but in any way, why not re-use the SQL validator, add any missing pattern and then the entire code base could profit? Should also simplify the proposed changes significantly. As for the "allow-list" that @IOhacker mentioned: we could use the JDBC driver's metadata, extract all existing tables in the database at boot time and collect a static final list of table and column names in a `Map<String, List<String>>` variable (key = allowed table name, value = list of allowed column names in that table) and validate against that. Additionally we could again use the SqlValidator and maybe configure a separate profile with regex patterns to check the table and column names for any malicious characters... but I think that whitelist based on driver's metadata should be enough. -- 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]
