[jira] [Assigned] (FINERACT-1095) Remove direct sqlSearch support from /clients and all other APIs [Security & Performance]

["Aleksandar Vidakovic (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/FINERACT-1095?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Aleksandar Vidakovic reassigned FINERACT-1095:
----------------------------------------------

    Assignee: Aleksandar Vidakovic  (was: Manthan Surkar)

> 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: Aleksandar Vidakovic
>            Priority: Major
>             Fix For: 1.4.0
>
>
> 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.3.4#803005)