Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Files API
    • Labels:
      None
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      35777

      Description

      Convert core code still using old file handling to use the new file API

      1. calendar.conversion.20091028.patch
        104 kB
        Sam Hemelryk
      2. calendar.conversion.20091029.patch
        113 kB
        Sam Hemelryk
      3. htmleditor.conversion.20091021.patch
        66 kB
        Sam Hemelryk
      4. htmleditor.conversion.20091102.patch
        138 kB
        Sam Hemelryk

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          Just attached calendar.conversion.20091028.patch
          It is a complete conversion of calendar from html files, to mforms with the new editor. I also reintroduced the full event hooks that must've been lost from calendar/event.php at some point.

          Petr and Martin if you could please review I would be most appreciative.
          I am nearly don't on a patch to convert all other core htmleditor uses but hope to get this in first, just to get it out of the way

          Show
          Sam Hemelryk added a comment - Just attached calendar.conversion.20091028.patch It is a complete conversion of calendar from html files, to mforms with the new editor. I also reintroduced the full event hooks that must've been lost from calendar/event.php at some point. Petr and Martin if you could please review I would be most appreciative. I am nearly don't on a patch to convert all other core htmleditor uses but hope to get this in first, just to get it out of the way
          Hide
          Petr Škoda added a comment -

          There is a potential problem when using new events from PHP code such as in external API- no idea how to solve this though

          1/ calendar_event_description area in pluginfile.php - are all events there public?

          2/ calendar/delete.php
          2a/ missing proper require_login($course), I do not understand the coursid use here much - major
          2b/ CSRF, missing sesskey check - critical
          2c/ if ($confirm !== false) { strange - PARAM_BOOL returns 1 or 0 - cosmetic
          2d/ require_once($CFG->dirroot.'/mod/forum/lib.php'); why does it include forum code? all module interactions should be general through plugin_supports() - minor
          2e/ the calendar_event::load() should probably use new MUST_EXIST flag and throw exception instead - minor

          3/ calendar/event_form.php
          3a/ the "=&" is not needed any more in PHP5 unless arrays are involved - minor

          4/ use groups_is_member() after has_capability() for per resons because has_capability() does not usually access db

          5/ calendar/lib.php
          $firstday = isset($CFG->calendar_startwday) ? $CFG->calendar_startwday : get_string('firstdayofweek'); is wrong, we can not initialise code in lib.php because the language is not initialised yet - major old problem

          Show
          Petr Škoda added a comment - There is a potential problem when using new events from PHP code such as in external API- no idea how to solve this though 1/ calendar_event_description area in pluginfile.php - are all events there public? 2/ calendar/delete.php 2a/ missing proper require_login($course), I do not understand the coursid use here much - major 2b/ CSRF, missing sesskey check - critical 2c/ if ($confirm !== false) { strange - PARAM_BOOL returns 1 or 0 - cosmetic 2d/ require_once($CFG->dirroot.'/mod/forum/lib.php'); why does it include forum code? all module interactions should be general through plugin_supports() - minor 2e/ the calendar_event::load() should probably use new MUST_EXIST flag and throw exception instead - minor 3/ calendar/event_form.php 3a/ the "=&" is not needed any more in PHP5 unless arrays are involved - minor 4/ use groups_is_member() after has_capability() for per resons because has_capability() does not usually access db 5/ calendar/lib.php $firstday = isset($CFG->calendar_startwday) ? $CFG->calendar_startwday : get_string('firstdayofweek'); is wrong, we can not initialise code in lib.php because the language is not initialised yet - major old problem
          Hide
          Petr Škoda added a comment -

          The first patch general review:

          1/ course summary

          The trusttext is not correct there, because we are not enforcing the download of files there, so in fact this results in XSS through files there. We have to use "noclean" instead and remove the summarytrust column from database.

          If we force download flash stops working immediately, only images work, but there is still serious per penalty because the files can not be cached.

          My reasoning in the case of course summary is - for technical reasons you need XSS trust in order to edit activities, so it makes sense to require XSS trust for course editing too.

          2/ course_request_summary

          XSS through the uploaded files because we are not forcing download - 'moodle/course:request' does not have XSS ==> no files with normal headers

          3/ course categories trust - again XSS problem or we need to cripple flash, my +10 to remove trustiest support there and use nucleon

          4/ user_profile - flash files will not work here even when user has trustiest because we must be farcing the download, I am afraid this will not work much and will cause confusion

          5/ multiple XSS in format_text() + trusstext - it must be used correctly when printing the text too - this is the most important part of security in trusttext design! Using 'noclean' option intstead of 'trusted' is a critical security bug - I already proposed to not support trusttext in course and category descriptions which solves this problem

          Show
          Petr Škoda added a comment - The first patch general review: 1/ course summary The trusttext is not correct there, because we are not enforcing the download of files there, so in fact this results in XSS through files there. We have to use "noclean" instead and remove the summarytrust column from database. If we force download flash stops working immediately, only images work, but there is still serious per penalty because the files can not be cached. My reasoning in the case of course summary is - for technical reasons you need XSS trust in order to edit activities, so it makes sense to require XSS trust for course editing too. 2/ course_request_summary XSS through the uploaded files because we are not forcing download - 'moodle/course:request' does not have XSS ==> no files with normal headers 3/ course categories trust - again XSS problem or we need to cripple flash, my +10 to remove trustiest support there and use nucleon 4/ user_profile - flash files will not work here even when user has trustiest because we must be farcing the download, I am afraid this will not work much and will cause confusion 5/ multiple XSS in format_text() + trusstext - it must be used correctly when printing the text too - this is the most important part of security in trusttext design! Using 'noclean' option intstead of 'trusted' is a critical security bug - I already proposed to not support trusttext in course and category descriptions which solves this problem
          Hide
          Sam Hemelryk added a comment -

          Awesome, thanks for the feedback Petr, will get in making those changes and post revised complete patches when done

          Show
          Sam Hemelryk added a comment - Awesome, thanks for the feedback Petr, will get in making those changes and post revised complete patches when done
          Hide
          Sam Hemelryk added a comment -

          Hi guys, here is the revised calendar patch calendar.conversion.20091029.patch, changes are as follows:

          • calendar/delete.php
            • Fixed require_login to use a course if the event is associated with one
            • Added check_sesskey call before undertaking delete
            • changed if ($confirm !== false) -to> if ($confirm)
            • Removed require mod/forum/lib.php
            • Made the calendar_event::load method use MUST_EXIST
          • calendar/event_form.php
            • Remove & in =&
          • pluginfile.php
            • Added more commenting to script
            • Moved the call to groups_is_member after has_cap call
          • calendar/lib.php
            • Fixed up use of get_string in lib.php, added a method and converted uses of the const to use the method
          Show
          Sam Hemelryk added a comment - Hi guys, here is the revised calendar patch calendar.conversion.20091029.patch, changes are as follows: calendar/delete.php Fixed require_login to use a course if the event is associated with one Added check_sesskey call before undertaking delete changed if ($confirm !== false) -to> if ($confirm) Removed require mod/forum/lib.php Made the calendar_event::load method use MUST_EXIST calendar/event_form.php Remove & in =& pluginfile.php Added more commenting to script Moved the call to groups_is_member after has_cap call calendar/lib.php Fixed up use of get_string in lib.php, added a method and converted uses of the const to use the method
          Hide
          Petr Škoda added a comment -

          hi sam, please commit the calendar stuff

          Show
          Petr Škoda added a comment - hi sam, please commit the calendar stuff
          Hide
          Petr Škoda added a comment -

          are you working on new html editor patch?

          Show
          Petr Škoda added a comment - are you working on new html editor patch?
          Hide
          Sam Hemelryk added a comment -

          Alrighty have just attached the core conversion patch... It is getting very close to being ready. All I need to do now is test it thoroughly including on a fresh install and an upgrade.

          The following outlines the changes in respect to the database tables.
          For each table + field below the code has been adjusted to make use of the editor as suggested by the detailed fields below, and the output and formatting of those fields has been corrected to use the *format field and if required rewrite urls.

          Table Field Files Trust Noclean Context Area Delete method
          user description Y - - user user_profile Context
          user_info_field description - - - - - -
          user_info_field defaultdata - - - - - -
          user_info_data data - - - - - -
          tag description Y - - system tag_description Manual
          message fullmessage - - - - - -
          grade_outcomes description Y - - system grade_outcome Manual
          scale description Y - - system grade_scale Manual
          grade_grades feedback - - - - - -
          group description Y - - course course_group_description Manual
          grouping description Y - - course course_grouping_description Manual
          course summary Y - Y course course_summary Context
          course_categories description Y - Y coursecat category_description Context
          course_request summary - - - - - -
          Show
          Sam Hemelryk added a comment - Alrighty have just attached the core conversion patch... It is getting very close to being ready. All I need to do now is test it thoroughly including on a fresh install and an upgrade. The following outlines the changes in respect to the database tables. For each table + field below the code has been adjusted to make use of the editor as suggested by the detailed fields below, and the output and formatting of those fields has been corrected to use the *format field and if required rewrite urls. Table Field Files Trust Noclean Context Area Delete method user description Y - - user user_profile Context user_info_field description - - - - - - user_info_field defaultdata - - - - - - user_info_data data - - - - - - tag description Y - - system tag_description Manual message fullmessage - - - - - - grade_outcomes description Y - - system grade_outcome Manual scale description Y - - system grade_scale Manual grade_grades feedback - - - - - - group description Y - - course course_group_description Manual grouping description Y - - course course_grouping_description Manual course summary Y - Y course course_summary Context course_categories description Y - Y coursecat category_description Context course_request summary - - - - - -
          Hide
          Petr Škoda added a comment -

          very nice table!

          Show
          Petr Škoda added a comment - very nice table!
          Hide
          Petr Škoda added a comment -

          Hmm, there is a problem in /pluginfile.php - the

          send_stored_file($file, 60*60, 0, $forcedownload); 
          

          the $forcedownload parameter must be hardcoded to true for all areas without the "Noclean" flag - this means either add more noclean's or add more force downloads. Each area with noclean or optional force download must be marked with XSS risk (indirectly via capability definition).

          Show
          Petr Škoda added a comment - Hmm, there is a problem in /pluginfile.php - the send_stored_file($file, 60*60, 0, $forcedownload); the $forcedownload parameter must be hardcoded to true for all areas without the "Noclean" flag - this means either add more noclean's or add more force downloads. Each area with noclean or optional force download must be marked with XSS risk (indirectly via capability definition).
          Hide
          Petr Škoda added a comment -

          My +1 for commit after fixing the plugin_file(), the db modification in upgrade.php seems like some new coding style, we should let Eloy review this.

          great job!

          Show
          Petr Škoda added a comment - My +1 for commit after fixing the plugin_file(), the db modification in upgrade.php seems like some new coding style, we should let Eloy review this. great job!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          About upgrade code, np here, as far as it works, IMO is clear enough and supports multiple executions (if fails for any reason). So +1

          The only thing I don't get is why your table above has 14 rows and the fieds added in install/upgrade are only 11. I guess it's because the other 3 already exist.. if so, np.

          About the rest... I don't understand anything... LOL... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - About upgrade code, np here, as far as it works, IMO is clear enough and supports multiple executions (if fails for any reason). So +1 The only thing I don't get is why your table above has 14 rows and the fieds added in install/upgrade are only 11. I guess it's because the other 3 already exist.. if so, np. About the rest... I don't understand anything... LOL... ciao
          Hide
          Sam Hemelryk added a comment -

          Awesome, thanks guys.
          Petr: Thank you for looking through the patch, it is certainly all of the place. I will fix up plugin_file now, finish my testing and commit.
          Eloy: Thank you for looking at the upgrade code, you are correct in that the missing 3 already exist within the database.
          Cheers all will confirm when this has gone through

          Show
          Sam Hemelryk added a comment - Awesome, thanks guys. Petr: Thank you for looking through the patch, it is certainly all of the place. I will fix up plugin_file now, finish my testing and commit. Eloy: Thank you for looking at the upgrade code, you are correct in that the missing 3 already exist within the database. Cheers all will confirm when this has gone through
          Hide
          Martin Dougiamas added a comment -

          Hi Petr, I'm just looking at your comment about pluginfile.php ... can you explain that further?

          Intuitively, I think you mean the opposite, that we should forcedownload for all areas where noclean=true (ie where cleaning is not happening). Right?

          Show
          Martin Dougiamas added a comment - Hi Petr, I'm just looking at your comment about pluginfile.php ... can you explain that further? Intuitively, I think you mean the opposite, that we should forcedownload for all areas where noclean=true (ie where cleaning is not happening). Right?
          Hide
          Martin Dougiamas added a comment -

          Sam: During upgrade, when adding the format field to the course summary, it's currently giving the FORMAT_MOODLE to everything. This is technically correct but it does mean that when someone edits the course settings they won't see an editor for the text there (when they probably usually did before). This is a big usability issue. The workaround (to change to HTML format, save then reload) isn't great.

          I think we might need to parse all those texts looking for HTML tags. If they contain tags we could assume the text was created in the HTML editor and give it FORMAT_HTML.

          We might have to do the same to some/all of the other fields that have had format added.

          Show
          Martin Dougiamas added a comment - Sam: During upgrade, when adding the format field to the course summary, it's currently giving the FORMAT_MOODLE to everything. This is technically correct but it does mean that when someone edits the course settings they won't see an editor for the text there (when they probably usually did before). This is a big usability issue. The workaround (to change to HTML format, save then reload) isn't great. I think we might need to parse all those texts looking for HTML tags. If they contain tags we could assume the text was created in the HTML editor and give it FORMAT_HTML. We might have to do the same to some/all of the other fields that have had format added.
          Hide
          Petr Škoda added a comment - - edited

          no cleaning => any XSS there including flash => we need normal headers => $forcedownload === false, teachers may optionally add forcedownload
          cleaning by default => XSS needs to be removed => download directive in http header disables flash and html with JS => need $forcedownload = true always

          Right, we have the format problem, we also need to relink and move around the used files during upgrade. If we keep NULL there for now we can do the upgrade later. I think the logic for format could be if there is any <p> or <br/>, <br> tag then use FORMAT_HTML, if not use FORMAT_MOODLE which is adding these - probably it could be done in pure SQL.

          Show
          Petr Škoda added a comment - - edited no cleaning => any XSS there including flash => we need normal headers => $forcedownload === false, teachers may optionally add forcedownload cleaning by default => XSS needs to be removed => download directive in http header disables flash and html with JS => need $forcedownload = true always Right, we have the format problem, we also need to relink and move around the used files during upgrade. If we keep NULL there for now we can do the upgrade later. I think the logic for format could be if there is any <p> or <br/>, <br> tag then use FORMAT_HTML, if not use FORMAT_MOODLE which is adding these - probably it could be done in pure SQL.
          Hide
          Sam Hemelryk added a comment -

          Awesome thanks for the info guys, the revised plan is shown below. I will be aiming to commit this tomorrow morning once I have double and triple checked everything, and confirmed that the new upgrade stuff works and is OK.

          Table Field Files Trust Noclean Forcedownload Context Area Delete method Capability
          user description Y - - Y user user_profile Context -
          user_info_field description - - - - - - - -
          user_info_field defaultdata - - - - - - - -
          user_info_data data - - - - - - - -
          tag description Y - - Y system tag_description Manual -
          message fullmessage - - - - - - - -
          grade_outcomes description Y - Y - system grade_outcome Manual moodle/grade:manage
          scale description Y - Y - system grade_scale Manual moodle/course:managescales
          grade_grades feedback - - - - - - - -
          group description Y - Y - course course_group_description Manual moodle/course:managegroups
          grouping description Y - Y - course course_grouping_description Manual moodle/course:managegroups
          course summary Y - Y - course course_summary Context moodle/course:update,moodle/course:create
          course_categories description Y - Y - coursecat category_description Context moodle/category:manage
          course_request summary - - - - - - - -
          Show
          Sam Hemelryk added a comment - Awesome thanks for the info guys, the revised plan is shown below. I will be aiming to commit this tomorrow morning once I have double and triple checked everything, and confirmed that the new upgrade stuff works and is OK. Table Field Files Trust Noclean Forcedownload Context Area Delete method Capability user description Y - - Y user user_profile Context - user_info_field description - - - - - - - - user_info_field defaultdata - - - - - - - - user_info_data data - - - - - - - - tag description Y - - Y system tag_description Manual - message fullmessage - - - - - - - - grade_outcomes description Y - Y - system grade_outcome Manual moodle/grade:manage scale description Y - Y - system grade_scale Manual moodle/course:managescales grade_grades feedback - - - - - - - - group description Y - Y - course course_group_description Manual moodle/course:managegroups grouping description Y - Y - course course_grouping_description Manual moodle/course:managegroups course summary Y - Y - course course_summary Context moodle/course:update,moodle/course:create course_categories description Y - Y - coursecat category_description Context moodle/category:manage course_request summary - - - - - - - -
          Hide
          Sam Hemelryk added a comment -

          Alrighty,
          That is all of the core uses of htmleditor and print_textarea converted. I am now going to start work on question/type/* and mod/* uses.
          The only two that I did not tackle are the blocks/html/edit_form.php (htmleditor), and admin/roles/lib.php (print_textarea).

          • blocks/html/edit_form.php Have to talk to Petr about this but I think conversion of blocks is still on his list.
          • admin/roles/lib.php Petr is workin in the roles area at the moment I think? I will let him get his changes in first, and then do it if it is still there.
          Show
          Sam Hemelryk added a comment - Alrighty, That is all of the core uses of htmleditor and print_textarea converted. I am now going to start work on question/type/* and mod/* uses. The only two that I did not tackle are the blocks/html/edit_form.php (htmleditor), and admin/roles/lib.php (print_textarea). blocks/html/edit_form.php Have to talk to Petr about this but I think conversion of blocks is still on his list. admin/roles/lib.php Petr is workin in the roles area at the moment I think? I will let him get his changes in first, and then do it if it is still there.
          Hide
          Sam Hemelryk added a comment -

          Relies on what is happening there as several file areas will rely on this to clean up files upon removal

          Show
          Sam Hemelryk added a comment - Relies on what is happening there as several file areas will rely on this to clean up files upon removal
          Hide
          Petr Škoda added a comment -

          I have fixed two problems:
          1/ bumped up the version indicating major changes in code - this redirects dev sites from frontpage to /admin/ because the blocks on frontpage throw fatal errors before the ugprade
          2/ fixed fatal sql error in upgrade - missing {} around table name

          ANother potential upgrade issue is that old sites may be using old upper case tags, we should imo searcg also for <P> and <BR> <BR /> and <br>.

          Show
          Petr Škoda added a comment - I have fixed two problems: 1/ bumped up the version indicating major changes in code - this redirects dev sites from frontpage to /admin/ because the blocks on frontpage throw fatal errors before the ugprade 2/ fixed fatal sql error in upgrade - missing {} around table name ANother potential upgrade issue is that old sites may be using old upper case tags, we should imo searcg also for <P> and <BR> <BR /> and <br>.
          Hide
          Sam Hemelryk added a comment -

          Awesome thanks for catching those two Petr, I learnt something new I'd never noticed the version specified in redirect_if_major_upgrade_required()
          I'll have a talk with Eloy and see what ideas he has about searching for the extra tags, the case sensitivity is a good point, but I wonder if there is a better way to do that then using multiple like statements.

          Show
          Sam Hemelryk added a comment - Awesome thanks for catching those two Petr, I learnt something new I'd never noticed the version specified in redirect_if_major_upgrade_required() I'll have a talk with Eloy and see what ideas he has about searching for the extra tags, the case sensitivity is a good point, but I wonder if there is a better way to do that then using multiple like statements.
          Hide
          Sam Hemelryk added a comment -

          Just going through checking old issues, this looks like it is complete as far as all the noted areas have now been converted and/or cleaned up.
          Please re-open if you spot anything missing that doesn't belong to any other file bugs.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Just going through checking old issues, this looks like it is complete as far as all the noted areas have now been converted and/or cleaned up. Please re-open if you spot anything missing that doesn't belong to any other file bugs. Cheers Sam

            People

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

              Dates

              • Created:
                Updated:
                Resolved: