Uploaded image for project: 'Apache Fineract'
  1. Apache Fineract
  2. FINERACT-1095

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

Attach filesAttach ScreenshotAdd voteVotersWatch issueWatchersCreate sub-taskLinkCloneUpdate Comment AuthorReplace String in CommentUpdate Comment VisibilityDelete Comments
    XMLWordPrintableJSON

    Details

    • Type: Improvement
    • Status: Open
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      While code reviewing PRs from Manthan Surkar 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:

      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 .

      Petri Tuomola I thought this kind of thing may interest you, feel free to chime in.

        Attachments

        Issue Links

          Activity

            People

            • Assignee:
              Unassigned
              Reporter:
              vorburger Michael Vorburger

              Dates

              • Created:
                Updated:

                Issue deployment