General developer forum

I hadn't looked at it, but I read the thread. Yes, it probably would achieve the desired results, but using separate download code means you may not benefit from other improvements to file.php; it also creates multiple points of failure from a security perspective. I presume the security issue is why there's a single file.php in the first place.

I'd initially been planning to just implement download code within my module, thus sidetracking file.php in a similar way. Then I thought, hang on, why not make file.php not suck? I think this way of doing it fits well with the existing module interface, such as it is. Actually, it could also be used to remove most of the existing module-specific hacks in file.php (e.g. for assessment module) at a later date, although I personally certainly don't plan to change anything there.

I think we (OU development team) may get Moodle cvs access... if we do, and Martin Dougiamas (or someone) doesn't object, I'll put this code in there. He might have another plan for achieving the same thing though in which case, well, we'll just keep this code in our local tree. We already have quite a lot of core changes [i.e. not in a module or other separated code area] in our local tree, though, and we've only been developing on it for a month or so! It would be nice to reduce the difference.

OK, since I haven't been working on the latest version due to technical difficulties here, it won't cleanly patch etc - so rather than attaching the files I will just paste in the added bits.

This first part goes immediately after the require_login check and before 'only editing teachers can access backups':

    // security: prevent direct requests to data for modules that handle
    // their own file requests. This check occurs here rather than lower
    // down, so that the modules themselves can return paths within their
    // moddata. Note that we strtolower components of the path here in
    // order to avoid attacks that depend on case-insensitive filesystems;
    // the 'real' paths are case-sensitive.
    if(count($args)>=3 && strtolower($args[1])=='moddata' &&
      module_handles_file_requests(strtolower($args[2]))) {
        error('Access not allowed');
    }
   
    // Allow modules to handle certain file requests by dealing with
    // remaining parameters and then returning an 'actual' filename
    // to send (this allows for modules to protect downloads
    // themselves). URL syntax:
    // /coursecode/!via/modulename/arbitrary/parameters/for/module
    if(count($args)>=3 && $args[1]=='!via') {
        $pathname=process_module_file_request(
            $args[2],$course->id,array_slice($args,3));
        // Update args array to match new file so that
        // following security still works if needed 
        if(strpos($pathname,$CFG->dataroot)===0) {
            $relativepath=substr($pathname,strlen($CFG->dataroot));
            $args = explode('/', trim($relativepath, '/'));
        } else {
            $args=array();
        }
    }


And here are the two new functions in filelib.php:


/**
 * Processes a request for a file that should be handled via a particular module,
 * which might point to any particular file it wishes to be served.
 *
 * Relies on a module function modulename_process_file_request, which takes
 * as parameters $courseid and $args, and should return either a full path to the
 * file or else FALSE to generate error.
 * @param string $modulename Name of module (already checked to contain only safe
 *   characters, but might not actually exist)
 * @param int $courseid Course ID (already checked, exists)
 * @param array $args Additional arguments in the request (any path components
 *   that came after the /coursecode/!via/module/ bit)
 * @return string Full path to actual file that should be served (otherwise will
 *   call error)
 */
function process_module_file_request($modulename,$courseid,$args) {
    // Check for ordinary characters to avoid security hole by accessing
    // stupid filenames
    if(!preg_match('/^[a-z0-9]+$/',$modulename)) {
        error("Invalid module name in !via file request");
    }
   
    // Get lib.php for module
    global $CFG;
    $modlib = "$CFG->dirroot/mod/$modulename/lib.php";
    if (file_exists($modlib)) {
        include_once($modlib);
    } else {
        error("The $modulename module is missing important code! ($modlib)");
    }
   
    // Find function
    $function = $modulename."_process_file_request";
    if(!function_exists($function)) {
        error("The $modulename module does not provide file access.");       
    }
   
    // Call function to handle request
    $filepath=$function($courseid,$args);
    if($filepath) {
        return $filepath;
    } else {
        error("The $modulename module does not offer a file for that request.");
    }
}

/**
 * Returns true if modules handle their file requests by including a
 * modulename_process_file_request function.
 * @param string $modulename Name of module (not yet checked to
 *   contain only safe characters, but might not actually exist)
 * @return bool True if the module has the given function, false if
 *   it doesn't (or the name wasn't a valid module name)
 */
function module_handles_file_requests($modulename) {
    // The reason this returns false all over the place rather than
    // giving errors is to support legacy behaviour in case this URL
    // isn't to a real module or something weird.
   
    // Check for ordinary characters to avoid security hole by accessing
    // stupid filenames
    if(!preg_match('/^[a-z0-9]+$/',$modulename)) {
        return false;
    }
   
    // Get lib.php for module
    global $CFG;
    $modlib = "$CFG->dirroot/mod/$modulename/lib.php";
    if (file_exists($modlib)) {
        include_once($modlib);
    } else {
        return false; // Not really a module, so doesn't handle requests
    }
   
    // Find function
    $function = $modulename."_process_file_request";
    return function_exists($function);   
}



There you go. Sorry for the delay in response but I was busy with other things!

As for concerns about reducing security, this is less likely to reduce security than using a separate file.php as there are still checks that take place (or can be added) after the module has updated the filepath. It's true that there are some checks that modules can bypass this way, and with injudicious acceptance of the $args parameter, security risks are possible. But frankly since anyone who needed to do anything like this before probably ended up making their own file.php or editing the existing one willy-nilly, this system is a big improvement.

(Also conceptually it is a flaw in the architecture if modules can't control file requests, since modules are supposed to be independent... I know there are lots of other flaws but this one was easy to fix.