KQMATH / moodle-mod_capquiz

:mushroom: Computer adaptive practice activity module for Moodle
https://moodle.org/plugins/mod_capquiz
GNU General Public License v3.0
4 stars 5 forks source link

invalid use of SQL functions #85

Closed danmarsden closed 5 years ago

danmarsden commented 5 years ago

https://github.com/KQMATH/moodle-mod_capquiz/blob/0d2178b473b71fabc9083d60b66988492b0f878e/classes/matchmaking/n_closest/n_closest_selector.php#L84

injecting parameters into in-line sql is not allowed - please use standard moodle parameter handling provided by the Moodle API functions. You must also pass the raw table name into the sql function - you should not be storing the table name in a separate variable - it must be hard-coded within the db function call.

Please review all use of sql and database functions - I have only given one example from your code but I saw other issues as well.

For more detail please see: https://docs.moodle.org/dev/Data_manipulation_API

sebastsg commented 5 years ago

Fixed that query, as well one other. I can't find any other injections,

danmarsden commented 5 years ago

Thanks @sebastsg stuff like this is frowned on too: $configuration = $DB->get_record(database_meta::$tablequestionselection, $conditions);

mainly because the first param of the db functions is a sql injection vector - a reviewer has to go through your code and check to see how the var is generated, if it can be overwritten by anything etc. It also makes it hard for someone unfamiliar with the full codebase to completely understand what is going on there - it often doesn't provide any large benefit - although I do understand some IDE's will show up a typo in a var name better than a hard-coded string.

I would typically fail a plugin written by our internal staff that does this, but if this is the only thing left I might pass it... - still looking :-)

sebastsg commented 5 years ago

I see. Thanks for your feedback. These will be fixed in the coming week.