https://bugzilla.wikimedia.org/show_bug.cgi?id=56178
--- Comment #4 from Chris Steipp <[email protected]> --- Hi Dan, I'm working through the review. There are a couple of things I'd like to see fixed at their root, so I don't have to document every occurrence, so this is just a partial list for now. Let me know if you have questions. includes/Php/Filter.php * $filter_sanitize === null in sanitize() should not be allowed. Since you use Filter::evaluate() to sanitize for xss in several places, there can't be a chance that would return without any filtering. If you can remove setting a custom filter, that would be even better. * Split array handling into another class that calls the scalar version, so all filtering logic is in one class, but when you filter, it's explicit if you're expecting an array or scalar. includes/Handlers/Forms/FormHandler.php Line 109: getForm is called without validating the class beyond existence. Please check this is a form class you're expecting (have them all inherit and check instanceof?) includes/Handlers/Forms/MetadataDetectHandler.php * getUserOptions() - doing urldecode after filtering invalidates filtering * You have MAX_FILE_SIZE come in from the form, although it looks like you never actually use it. You should obviously never rely on user-submitted data to check sizes. You can probably just remove that field, in case another developer decides to trust it? includes/functions/functions.php * is there a reason these aren't static methods in a utility class? * Attack can get arbitrary values through getWhitelistedPost by passing in a whitelisted name as an array with 'filter-sanitize'=null. Fixing Filter should prevent this from being an issue. includes/Specials/SpecialGWToolset.php * Line 143: Please log, don't print_r * Line 164: I would make things easier if you could escape the exception message, or demonstrate that it's been properly sanitized. Otherwise all exceptions have to be sanitized for xss (more places to get something wrong). includes/Handlers/Xml/XmlHandler.php * XMLReader must disable external entity loading before reading xml -- You are receiving this mail because: You are on the CC list for the bug. _______________________________________________ Wikibugs-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
