Moodle

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

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

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

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

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

hi sam, please commit the calendar stuff

are you working on new html editor patch?

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.

very nice table!

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).

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!

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

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

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?

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.

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.

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.

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.

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

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>.

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.

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