[
https://issues.apache.org/jira/browse/FINERACT-1095?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Mihaly Dallos reassigned FINERACT-1095:
---------------------------------------
Assignee: Mihaly Dallos
> Remove direct sqlSearch support from /clients and all other APIs [Security &
> Performance]
> -----------------------------------------------------------------------------------------
>
> Key: FINERACT-1095
> URL: https://issues.apache.org/jira/browse/FINERACT-1095
> Project: Apache Fineract
> Issue Type: Improvement
> Reporter: Michael Vorburger
> Assignee: Mihaly Dallos
> Priority: Major
>
> While code reviewing PRs from [~Manthan] such as
> [https://github.com/apache/fineract/pull/1171/files] and
> [https://github.com/apache/fineract/pull/1123/files] re. FINERACT-854, I've
> learnt about Fineract's support for an {{sqlSearch}} parameter on a number of
> its APIs, such as {{/clients}} (and others).
> According to
> [https://demo.fineract.dev/fineract-provider/api-docs/apiLive.htm] :
> {quote}_sqlSearch
> String optional
> Use an sql fragment valid for the underlying client schema to filter
> results. e.g. display_name like %K%
> {quote}
> [https://github.com/apache/fineract/search?p=2&q=sqlSearch&unscoped_q=sqlSearch]
> shows all current occurrences. There are a number, but not THAT many either.
> (By far not every API supports this, only a handful.)
> This can be used e.g. like this:
> [https://demo.fineract.dev/fineract-provider/api/v1/clients?paged=true&sqlSearch=c.account_no=000000003&tenantIdentifier=default]
> To me, this is an egregious violation of "layering architecture". Basically,
> the REST API gives you direct SQL database access! You apparently have to
> know the exact name of not the SQL table but the alias used in the respective
> internally hard-coded query (note the use of {{c.}} in the example above, NOT
> {{m_client}}), and the internal SQL column name (note the use of
> {{account_no}}, NOT {{accountNo}}). There is no real documentation how to use
> this, and even if there were, in my tests I've noticed its fairly easy to
> provoke _500 Internal Server Errors_ when using {{sqlSearch}} with a slightly
> wrong syntax.
> From a security point of view, it's not quite as bad as it looks, because
> there is code with heuristics to "validate" the {{sqlSearch}} and prevent SQL
> Injections. But that could have holes (I don't want to know!), so... this
> still isn't great, at all, IMHO.
> From a performance point of view, permitting clients to run arbitrary queries
> isn't great either. You can't really reliable offer performance guarantees,
> or offer tuning with indices, if at the end of the day the wide open API just
> lets a client "query whatever they want" anyway.
> Use of {{sqlSearch}} by (public) API clients isn't that hard to find. I did
> some digging, and:
> * The new web-app UI doesn't use sqlSearch (or not yet), see
> [https://github.com/openMF/web-app/search?q=sqlSearch&unscoped_q=sqlSearch]
> * The old community-app UI does use sqlSearch, see
> [https://github.com/openMF/community-app/search?p=1&q=sqlSearch&unscoped_q=sqlSearch].
> But only in 2 very specific places, for Loans' {{l.loan_status_id in
> (100,200)}} and Clients' {{c.status_enum=100}}. This was apparently
> introduced by [~vishwasbabu] in
> [https://github.com/openMF/community-app/pull/1582|https://github.com/openMF/community-app/pull/1582/files]
> for [MIFOSX-2712.|https://mifosforge.jira.com/browse/MIFOSX-2712.] It's
> noteworthy that the Find on
> [https://cui.fineract.dev/.../clients|https://cui.fineract.dev/?baseApiUrl=https://demo.fineract.dev&tenantIdentifier=default#/clients]
> does NOT use {{sqlSearch}} but the [/search
> API|https://demo.fineract.dev/fineract-provider/api-docs/apiLive.htm#search]
> * other repos on openMF, such as Mobile Apps & Co, don't realy seem to
> actively use {{sqlSearch}}, looking at
> [https://github.com/search?p=7&q=org%3AopenMF+sqlSearch&type=Code]
> Other than that, I don't know if anyone actively using {{sqlSearch}} would
> have any major objections to... just simply altogether removing this
> entirely? Folks may of course be using this in their own client UIs, but...
> they really shouldn't, this is a "bad" API. If you are missing a query
> facility, just contribute to the upstream project and raise a pull request to
> add whatever query option you are missing to whatever Fineract API (such as
> e.g. by status for Loans and Clients).
> Let's further discuss on the developer mailing list on thread "Use of
> sqlSearch argument in Groups/Clients List" if anyone wants to strongly defend
> {{sqlSearch}}. If not, let's just completely remove it. We do have to first
> replace the small current use in the community-app.
> PS: Nota bene that this issue isn't stating that a REST API with query
> capabilities is bad per se. The point here is that an "SQL pass-through" is
> wrong. We can, and to replace current existing use of {{sqlSearch}} in the
> community-app must, add additional query parameters to API, as needed. Just
> need a wide open "anything goes" too general query parameter like
> {{sqlSearch}} .
> [~ptuomola] I thought this kind of thing may interest you, feel free to chime
> in.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
