terencemo commented on PR #5916: URL: https://github.com/apache/fineract/pull/5916#issuecomment-4624293919
> @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. Correct me if I'm wrong, but SqlValidator purports to validate SQL and I'm not trying to do that. As explained on the PMC mail thread, my approach is to validate input parameters at the gate based on parameter type. I'm all for re-use, but SqlValidator takes a statement as input, right? My validation is before the statement is constructed. > 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. Again, I'm looking at something far simpler than SQL here. e.g For parameter R_officeId=1, I'm just doing numeric validation to ensure we don't accept 1-SLEEP(10) for instance. -- 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]
