Moodle
  1. Moodle
  2. MDL-38456

Available update plugin deployment fails under Windows

    Details

    • Testing Instructions:
      Hide

      Needs to be tested under Windows in Moodle 2.4 (this won't work in Master I don't think).

      1. Download a plugin (zip) from the Plugins Directory (not Git)
      2. Before installing it, reduce its version in the version.php file
      3. Install the plugin
      4. Visit the Plugins overview page (Site admin > Plugins > Plugins overview)
      5. Click "Check for available plugins"; the newly installed plugin should show an update
      6. Click "Install this update"
      7. Confirm the update
      8. Verify there are no errors during download or update
      9. Go through the normal update process to install the "updated" code
      Show
      Needs to be tested under Windows in Moodle 2.4 (this won't work in Master I don't think). Download a plugin (zip) from the Plugins Directory (not Git) Before installing it, reduce its version in the version.php file Install the plugin Visit the Plugins overview page (Site admin > Plugins > Plugins overview) Click "Check for available plugins"; the newly installed plugin should show an update Click "Install this update" Confirm the update Verify there are no errors during download or update Go through the normal update process to install the "updated" code
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      MDL-38456-mdeploy-paths_24
    • Pull Master Branch:
      MDL-38456-mdeploy-paths
    • Rank:
      48416

      Description

      I was testing the available update system and went to install a plugin through the new interface. After confirming that I did want to install the new plugin, I was presented with an error...

      Oops! It did it again
      
      Moodle deployment utility had a trouble with your request. See the docs page and the debugging information for more details.
      
      exception 'unauthorized_access_exception' with message 'Unable to read the passphrase file.' in D:\xampp\htdocs\24_integration\mdeploy.php:836
      Stack trace:
      #0 D:\xampp\htdocs\24_integration\mdeploy.php(705): worker->authorize()
      #1 D:\xampp\htdocs\24_integration\mdeploy.php(1390): worker->execute()
      #2 {main}
      

      Looking into the value of the passphase file path I discovered that the system is filtering characters from the path including colons (, which means that paths under Windows are being broken. By allowing colons in paths, I was able to download and install a plugin without any trouble. The change I made was to...

      /mdeploy, line 268
      $raw = preg_replace('~[[:cntrl:]]|[&<>"`\|\':]~u', '', $raw);
      

      ...from which I remove the colon from the regex, so it wouldn't be removed.

      /mdeploy, line 268, altered
      $raw = preg_replace('~[[:cntrl:]]|[&<>"`\|\']~u', '', $raw);
      

        Activity

        Hide
        Michael de Raadt added a comment -

        I also noted that the docs page had no information about this error. Perhaps that should be added.

        Show
        Michael de Raadt added a comment - I also noted that the docs page had no information about this error. Perhaps that should be added.
        Hide
        David Mudrak added a comment -

        Hmm. Well, that explains the problem. But ... that regex is an exact copy of what we do in clean_param() with PARAM_FILE, which is used in clean_filename() for example. Which would suggest that Windows users should experience this problem at other places, too.

        Show
        David Mudrak added a comment - Hmm. Well, that explains the problem. But ... that regex is an exact copy of what we do in clean_param() with PARAM_FILE, which is used in clean_filename() for example. Which would suggest that Windows users should experience this problem at other places, too.
        Hide
        David Mudrak added a comment -

        Ah! Stupid me. Thanks Eloy for reminding me that PARAM_FILE is for the file basename only, not the full path.

        It seems that we have no PARAM_* constant that would support absolute paths including the Windows drive label.

        Show
        David Mudrak added a comment - Ah! Stupid me. Thanks Eloy for reminding me that PARAM_FILE is for the file basename only, not the full path. It seems that we have no PARAM_* constant that would support absolute paths including the Windows drive label.
        Hide
        David Mudrak added a comment -

        Submitting for integration. Thanks for the debugging info Michael!

        Show
        David Mudrak added a comment - Submitting for integration. Thanks for the debugging info Michael!
        Hide
        David Mudrak added a comment -

        Note: as you can see, there was a typo in the mdeploy unit test (missing colon in the C/WINDOWS/...) so we did not catch this before.

        Show
        David Mudrak added a comment - Note: as you can see, there was a typo in the mdeploy unit test (missing colon in the C/WINDOWS/...) so we did not catch this before.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Damyon Wiese added a comment -

        Thanks David,

        The fix looks correct to me. Integrated to 24 and master.

        Show
        Damyon Wiese added a comment - Thanks David, The fix looks correct to me. Integrated to 24 and master.
        Hide
        Michael de Raadt added a comment -

        Test result: Success!

        I tested this functionally in 2.4 only. It worked and I was able to install an update.

        I also ran the unit tests under 2.4 and master...

        D:\xampp\htdocs\24_integration>phpunit --no-configuration mdeploytest
        PHPUnit 3.7.9 by Sebastian Bergmann.
        
        ...............................
        
        Time: 0 seconds, Memory: 2.25Mb
        
        OK (31 tests, 49 assertions)
        
        D:\xampp\htdocs\master_integration>phpunit --no-configuration mdeploytest
        PHPUnit 3.7.9 by Sebastian Bergmann.
        
        ...............................
        
        Time: 0 seconds, Memory: 2.25Mb
        
        OK (31 tests, 49 assertions)
        
        Show
        Michael de Raadt added a comment - Test result: Success! I tested this functionally in 2.4 only. It worked and I was able to install an update. I also ran the unit tests under 2.4 and master... D:\xampp\htdocs\24_integration>phpunit --no-configuration mdeploytest PHPUnit 3.7.9 by Sebastian Bergmann. ............................... Time: 0 seconds, Memory: 2.25Mb OK (31 tests, 49 assertions) D:\xampp\htdocs\master_integration>phpunit --no-configuration mdeploytest PHPUnit 3.7.9 by Sebastian Bergmann. ............................... Time: 0 seconds, Memory: 2.25Mb OK (31 tests, 49 assertions)
        Hide
        Damyon Wiese added a comment -

        This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

        Thanks for your contributions!

        Show
        Damyon Wiese added a comment - This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads). Thanks for your contributions!

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: