|
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 3/ calendar/event_form.php 4/ use groups_is_member() after has_capability() for per resons because has_capability() does not usually access db 5/ calendar/lib.php 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:
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.
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).
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sub-task
In Progress
Major
 |
|
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