Inspection for improper usage of findOne()

[cebe]

It would be great if we could add a warning for cases where findOne() is used in unsafe context as described in

https://www.yiiframework.com/news/168/releasing-yii-2-0-15-and-database-extensions-with-security-fixes#is-my-application-affected

e.g. warn, when a variable is passed to findOne() that could be of type array.

Not sure how hard it is to implement it, but I just got the idea for this case so I thought I'd share it.

[kalessil]

With some limitations, we can report the case. Do you have a PR/commits with the fix - so I get a better idea?

[cebe]

There is no fix for the case that needs to be reported. The problem is that if someone passes a variable from user input to findOne() the user might be able to inject an array instead of a scalar value and thus may be able to filter records by different columns. We can not fix this in the framework because we do not know if user input is passed or the array is explicitly created by the calling function.

So if we have code like findOne($var) and $var comes from either $_GET, $_POST, $_REQUEST, or Yii::$app->request->..., we should consider it unsafe for passing it directly to findOne($var). If $var is a scalar value everything is fine, findOne() will do a query by primary key in that case. If $var is an array, it will search by other columns, e.g. ['name' => 'cebe'] will query WHERE name='cebe'.

The following cases should be reported:

$id = Yii::$app->request->get($id);
Post::findOne($id);

$id = $_GET['id']; // same with $_POST, $_REQUEST
Post::findOne($id);

There is no problem if the field to filter by is explicitly specified:

Post::findOne(['id' => $_GET['id']]); // will generate WHERE id IN (1,2,3) in case an array is passed, but returns the first matching record, so its safe

If the variable comes from the funtion argument its not clear if its okay to pass it:

public function findModel($id) {
    return Post::findOne($id);
}

In case of a controller however, the controller will only allow scalar values:

public function actionView($id) {
    $model = Post::findOne($id); // no warning needed here, because $id is string. Otherwise HTTP 400 Invalid requrest is generated by the controller
    // ...
}

let me know if you need more information.

[kalessil]

Thank you @cebe : that's sufficient volume of information =)