Plugins traffic

Some common issues in submitted plugins

 
 
Picture of David Mudrák
Some common issues in submitted plugins
Group DevelopersGroup Documentation writersGroup Moodle HQGroup Particularly helpful MoodlersGroup Translators

In this post I would like to highlight some common issues I recently spotted while providing the initial acceptance review of plugins submitted into the Plugins directory.

Database access and MySQLisms

It seems that quite a lot of authors of contributed plugins use MySQL database only when they are developing and testing their plugin. Moodle provides solid data manipulation API to deal with tiny differences in the syntax of SQL queries for all supported database engines (MySQL, PostgreSQL, MS SQL and Oracle). To make the plugin cross-db compatible, it is necessary to avoid DB engine specific syntax.

  • Always use the $DB to access the database, avoid functions like mysql_query() to execute your query.
  • Aim to use the most specific $DB method to do the job. That is, use $DB->get_records() if you can instead of $DB->get_records_sql() if you don't really need to.
  • Do not use MySQL specific backtick quotes in the queries. It makes the query fail badly on other databases.
  • Keep in mind that when using aggregations via GROUP BY, the SELECT part of the statement must explicitly contain all the fields your are grouping by. MySQL allows you to avoid them, which I personally really hate as it goes against some fundamental aspects of RDBMS - such as being deterministic.
  • Do not use LIMIT in your queries. The $DB methods have explicit parameters for them.
  • Neither use $CFG->prefix in your queries nor use hard-coded prefix like mdl_user. Use the {tablename} syntax and let the $DB driver to replace it with the actual table name.
  • In order to prevent SQL injection, always use data placeholders in your queries (? or :named) to pass data from users into the queries.
  • Always use the XMLDB editor to perform the schema changes via db/upgrade.php.

Our experience says that having the code tested against MySQL and PostgreSQL is usually enough to make sure it's cross-db compatible.

Prevent the cross-site request forgery

I was quite surprised how often this happens. Using the sesskey() feature to prevent CSRF attacks is a basic security protection you should include in your code whenever you are performing an action. When using the forms API, session key is checked implicitly. In all other cases it's your duty to perform the check. See this page for more details.

Check capabilities

It is important to highlight that permission checks must be done at both places. Not only there where your code is deciding whether or not a certain action link should be displayed (has_capability() is typically used there). It is essential to also check it just before you actually perform the action (that's where you want to call require_capability()). Remember, your code is open source. If require_capability() is missing, users can easily bypass your "protection" by submitting the data directly.

Sanitize user's input

You should never need to access superglobals $_GET, $_POST and $_COOKIE directly. Moodle provides wrappers for getting HTTP parameters - such as optional_param() and required_param(). Make sure you pick the most specific PARAM type for each of the parameters.

 
Average of ratings:Useful (12)
Picture of Justin Hunt
Re: Some common issues in submitted plugins
Group Particularly helpful Moodlers

That is a really great post! I am going to get more liberal with using sesskey(). I had noticed it but not given it much thought before.

 
Average of ratings: -
Picture of Marcus Green
Re: Some common issues in submitted plugins
Group Particularly helpful Moodlers
Good points David, I have some code that had been downloaded over 10,000 times and appeared to work perfectly.  Then I got a message from a Microsoft  SQL server user because my alter table upgrade code was MySQL specific sad. Fortunatly the user knew his stuff and provided me with a workaround, but I should have been using XMLDB from the start.
 
Average of ratings:Useful (1)
David
Re: Some common issues in submitted plugins
 

sesskey is a very fake security example as any place where you can inject javascript (and given by the latest releases being plagues by XSS exploits there are probably still many left) can grab the sesskey from the page source by simply executing the following code:

alert(M.cfg.sesskey);

End of the story, don't trust user input, always use the correct PHP calls to validate/sanitize the user's input.

 
Average of ratings: -
Picture of David Mudrák
Re: Some common issues in submitted plugins
Group DevelopersGroup Documentation writersGroup Moodle HQGroup Particularly helpful MoodlersGroup Translators
The sesskey is not supposed to sanitize user's input. It helps to protect the user from the CSRF. The incoming input can be 100% sanitized and valid but what's the point if the user did not actually intent to send it?

I did not say that sesskey is the holy grail of the web security. It's just an important piece in the puzzle. And my point was that it's often being ignored completely when an action is executed based on passed HTTP arguments.
 
Average of ratings:Useful (1)
Tim at Lone Pine Koala Sanctuary
Re: Some common issues in submitted plugins
Group DevelopersGroup Documentation writersGroup Particularly helpful Moodlers

Off topic, but please can we have the 'Useful' rating scale in this forum.

 
Average of ratings: -
C'est moi :-)
Re: Some common issues in submitted plugins
Group Documentation writersGroup Particularly helpful MoodlersGroup TestersGroup Translators

Very useful post smile

Tim : it seems it's now done wink

 
Average of ratings: -
Picture of Nicolas Martignoni
Re: Some common issues in submitted plugins
Group DevelopersGroup Documentation writersGroup Particularly helpful MoodlersGroup TestersGroup Translators

This is a very useful poste indeed, and it could be more useful yet with examples of good (and bad) Moodle code, or pointers to source codes, somewhere in the dev Moodledocs.

Anyway, is this info somewhere to find in Moodledocs?

 
Average of ratings: -
Picture of David Mudrák
Re: Some common issues in submitted plugins
Group DevelopersGroup Documentation writersGroup Moodle HQGroup Particularly helpful MoodlersGroup Translators
Thanks Nicolas for the feedback and suggestions. I'll keep it in mind for further similar posts here.

The coding guidelines, API docs and security related docs are all there. I don't think it would help to create yet another page / section to repeat that all again.
 
Average of ratings: -
David
Re: Some common issues in submitted plugins
 

Unfortunately I have to admit that even me as a long time coder are often baffled by the things that are in Moodle, and other obvious things which are not. The example given about $DB->get_records() versus $DB->get_records_sql() is a strange one, because under the hood these functions are aliased so executing $DB->get_records() will construct a SQL query which is then executed with $DB->get_records_sql(), the result is then parsed back and delivered in a different way than executing $DB->get_records_sql() directly in some cases.

 

I therefor do not think there are some things that are often done wrong in plugins, but are fundamentally wrong with Moodle. It should be trivial to use the correct syntaxes, classes and functions, and I often seem to find that the Moodle way is more cluttered, less secure, undocumented in anything else than code, confusing and gives much, much, much worse performance than expected or necessary.

Compared to any modern PHP based application Moodle provides a poor example of what quality code should be.

 
Average of ratings: -
Picture of David Mudrák
Re: Some common issues in submitted plugins
Group DevelopersGroup Documentation writersGroup Moodle HQGroup Particularly helpful MoodlersGroup Translators

Can you provide some specific examples of things "fundamentally wrong with Moodle"? Not saying I am not aware of some of them, I'm just trying to clarify things here.

The point of the $DB API calls is that the more specific method you use (and let the DB drivers to actually construct the query for you), the more robust result you get. Methods like get_records() are less prone to cross-db incompatibilities. These wrappers were introduced so that developers don't need to pay attention to details but can focus on fundamentals. I see this as a good thing, not a strange one.

Also, comparison with "any modern PHP based application" is not actually fair. Moodle code has been evolving for more than twelve years now. Related technologies, including web servers, databases and PHP itself, evolved since then (and, let's admit, we all learnt a lot during that time, too). If Moodle core team was rewriting the legacy code over and over again to follow the modern patterns, there would not be much resources (if any) left for further development.

 
Average of ratings: -
Picture of David Mudrák
Re: Some common issues in submitted plugins
Group DevelopersGroup Documentation writersGroup Moodle HQGroup Particularly helpful MoodlersGroup Translators
For the reference, the conversation continues at https://moodle.org/mod/forum/discuss.php?d=267920
 
Average of ratings: -