Description of problem:
*Every* SearchQuery is improperly evaluated as unsafe expression.
Version-Release number of selected component (if applicable):
3.5
How reproducible:
100%
Steps to Reproduce:
1. call GET on http://localhost:8080/api/vms or any other such 'getter' method and follow through mentioned methods.
Additional info:
in method
org.ovirt.engine.core.bll.SearchQuery#initQueryData
is this line:
isSafe = SearchObjects.isSafeExpression(searchText);
this line is currently always false, because internally it consists of searching through Map which is filled with <some_uppercase_constant> transformed to lowercase and then concatenated with colon. Example "vms:", "macpool:". The searching code looks like this:
<map>.contains(<input>.toLowerCase());
but that input is calculated here:
org.ovirt.engine.api.restapi.resource.AbstractBackendCollectionResource#getBackendCollection(org.ovirt.engine.core.common.interfaces.SearchType)
using code:
QueryHelper.getConstraint(getUriInfo(), "", modelType)
which instead of previously mentioned examples produces "vms : ", "macpool : ". Reason is in incompatibilities among:
org.ovirt.engine.api.common.util.QueryHelper#RETURN_TYPE_SEPARTOR
and
org/ovirt/engine/core/searchbackend/SearchObjects.java:63 //another definition of separator.
Comment: trying to solve sql injection is very improper, related code is extremely inferior and all of this should be thrown away as a whole.
in
http://gerrit.ovirt.org/#/c/28343/
I've selected " : " to be the right suffix — it seemed to me, that this value is more important for the system, since ':' was only used in determining 'query safeness'. So I've changed ':' to " : ". If it should be the other case, please let me know.
I did a quick scan through code and I do not think this is testable in a black-box way.
Internals seems too complex to me, so we cannot prove it's ok just upon changing logging level or some other externally observable change like this. Which is, on top of that, not present in code. I.e. I did not find such 'property', but even if it exist somewhere in code, based on it's complexity I do not think it's safe to pronounce it "ok" based just on such observation.
Bug was actually caused by two duplicate constants which weren't in sync, so fix was rather easy.