Adding validation to check not to have ddl, dml commands in sqlSearch filter text

[nazeer1100126]

No description provided.

[mgeiss]

Looks good to me. Maybe another commiter can look at it too.

[rajuan]

! or <> is not allowed.... inequality or comparison cannot be done as part of sqlSearch

[rajuan]

Code between line 55 and 109 is futile in preventing unauthorised data access (office hierarchy restriction)....
For eg... it will throw an exception for string "' OR '1' = '1'"
but not for
a. "a' OR '1' = '1"
b. "a' OR '2' != '1'"

Basically we can retrieve all the data irrespective of user hierarchy restriction.

[nazeer1100126]

The main purpose of the fix is not to return un authorized data. In above cases, it won't return any data as where condition fails.

[rajuan]

This check should done for all the string params that are received for eg. in this class it should be checked for externalId, name and orderBy etc......

Also for completeness these checks needs to be done across all read platform services

[nazeer1100126]

Instead of we are checking whether operand and value are same. This case will be considered as invalid sql

[rajuan]

I feel supporting sqlSearch is futile..... based on the validation code, we can always find a workaround injection....
I suggest getting rid of this support....

[nazeer1100126]

This use case is legacy and used by some customers. So we can't get rid of this fix.
So we have discussed internally before suggesting this fix.

[rajuan]

Instead of validating input param string.... I would suggest rewriting the query as prepared statements..... Have a list to add/track a value when a sql is appended.....

For eg.... replace extraCriteria.append(" and g.external_id = ").append(ApiParameterHelper.sqlEncodeString(externalId));

with
extraCriteria.append(" and g.external_id = ? ")
paramList.add(externalId)

convert the paramList to array and use it in the jdbcTemplate in the end while executing the query.....

I'm not sure if this approach could be used for order by clause

[nazeer1100126]

My approach is simple. I am accepting any param. But both operand and value should not be equal which is solving the actual purpose of this fix.

[mgeiss]

I'd suggest to parse the params before we hit any depper layer ... so we can provide an early exit instead of relying to a depper level ...