Michael Vorburger created FINERACT-1095:
-------------------------------------------
Summary: 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
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)
