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

• Type: Improvement
• Status: In Progress
• Priority: Major
• Resolution: Unresolved
• Fix Version/s: 1.10.0
• Assignee: Mihaly Dallos
• Reporter: Michael Vorburger
• Created: 22/Jul/20 23:49
• Updated: 06/Jan/24 15:29

Description

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 :

_sqlSearch
String optional
Use an sql fragment valid for the underlying client schema to filter results. e.g. display_name like %K%

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 for MIFOSX-2712. It's noteworthy that the Find on https://cui.fineract.dev/.../clients does NOT use sqlSearch but the /search API
• 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.

Issue Links

• Blocked: FINERACT-1058 Add support for "limit" and "order by" query in SQLBuilder
• blocks: FINERACT-1123 StringIndexOutOfBoundsException at ColumnValidator
• blocks: FINERACT-854 Use prepared statements instead of string concatenated SQL everywhere
• is duplicated by: FINERACT-2025 SQL Query Improvement
• links to: GitHub Pull Request #1207
• links to: GitHub Pull Request #1251