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

      Description

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

        Gliffy Diagrams

        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 Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda added a comment -

            hi sam, please commit the calendar stuff

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

            are you working on new html editor patch?

            Show
            Petr Skoda 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 Skoda added a comment -

            very nice table!

            Show
            Petr Skoda added a comment - very nice table!
            Hide
            Petr Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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: