vorburger commented on issue #723: FINERACT-808 FIXES: Some Action names do not filter audit trails URL: https://github.com/apache/fineract/pull/723#issuecomment-596211460 > Since, it is in where clause this probably need not be verified (I have researched, but would still require a confirmation from mentors). Reading e.g. https://www.sqlinjection.net/tutorial/, I don't think it's true in general that "Since, it is in where clause this probably need not be verified" ... however, looking more into the code: For the change (removal of SQL validation to prevent injection) in `AuditReadPlatformServiceImpl` we can see that `retrievePaginatedAuditEntries()` is only ever called from `AuditsApiResource`'s `retrieveAuditEntries()`, where the `extraCriteria` are constructed in `getExtraCriteria()`, which itself already uses `ApiParameterHelper.sqlEncodeString()` for all String args, so removing `validateSqlInjection()` for `extraCriteria` looks safe, to me (but, again, **NOT** because "it's just a WHERE", but because it seems to previously have been checked already). For the other proposed change in `AuditsApiResource.getExtraCriteria()` I'm much more sceptical, because that `actionName` which is currently `ApiParameterHelper.sqlEncodeString` which does a `SQLInjectionValidator.validateSQLInput`, but which with this change wouldn't do that anymore, comes straight from the outside world user as String parameter {{actionName}} in {{retrieveAuditEntries()}} ... removing that seems dangerous, to me. I'm not entirely sure what a better solution is. I'd welcome more eyes on this though, and will email the list for additional input. PS: Whatever solution we end up ultimately identifying and accepting, IMHO it could be useful for future code archaeology to include (a summary or copy/paste of) above into the commit message?
---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
