A couple of weeks ago I gave a presentation showing off some work from the Moodle open Book project. The middle of the presentation was a live demonstration of the Moodle Book and various features. At one point in the presentation members of the audience (including a number of academics who used the Book module in their Moodle sites) gave an audible gasp. This occurred when I showed off the search block for the Book module. A tool that allows the user to search the contents of all the books in a Moodle course. The gasp indicated just how much teachers and students desire this feature. A feature I’ve been calling out for quite some time.
The GitHub repository has been around for 2 years. So why isn’t this block more widely available in Moodle sites? There’s a security flaw in the code and is somewhat unfinished.
The Moodle SQL injection page outlines at least two broad steps that Moodle code should take to prevent a nefarious person from gaining inappropriate access. These are
- Using appropriate parameter cleaning mechanisms on data coming from outside of Moodle (e.g. search terms entered into a form).
- Using provided Moodle functions to retrieve data from the database (e.g. search the database for content in a Moodle book)
The Moodle book search block currently meets #1, but fails #2.
The following aims to explore and hopefully remedy this problem.
Current status is that some initial changes have been made to a local version of the block that borrows lessons from the forum search. Need to spend a bit more time on this, but it’s on the way.
The form for the block currently passes the following information.
Added by the block code
- courseid; and
The user is able to enter data into: bsquery
Apart from standard processing the main searching is done in a function named search which
- Deals with some apparent differences between flavours of SQL between databases.
Seeming to point to a problem in how it’s engaging with databases. DOES FORUM SEARCH DO THIS
- Focuses attention on books the user is allowed to read.
- Generates strings containing SQL statement
See below for the format.
- Supports + and –
- Uses get_records_sql to retrieve any matches
Not using placeholders, which is a problem.
A search for “copyright” generates a SQL statement similar to the following
SELECT DISTINCT bc.* FROM mdl_book_chapters bc, mdl_book b WHERE b.course = 12 AND b.id IN (..long list of book ids) AND bc.bookid = b.id AND bc.hidden = 0 AND (( bc.title ILIKE '%copyright%' ) OR ( bc.content ILIKE '%copyright%' ) ) ORDER BY bc.bookid, bc.pagenum
Each word added to the search phrase adds options to the “ilikes”
( bc.title ILIKE ‘%copyright%’ AND bc.title ILIKE ‘%creative%’
AND bc.title ILIKE ‘%commons%’ )
- Can get_records_sql be replaced with get_records
- Can the database dependency be removed
- Can placeholders be used
Comparison with forum search
The forum search is an accepted part of Moodle. Checking how it works might provide some inspiration to copy.
Has a slight be compartmentalised structure
$forums = forum_get_readable_forums($USER->id, $courseid);
Has to deal with a lot more complexity than the book search.
Makes use of a method search_generate_SQL to do as the name suggests. This is something that should be worked into the Book search block.
Initial testing and that is working. Dig a bit and it makes sense. Also seems to tidy up the code a fair bit.