filter

chris steipp requested that we:

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

i have removed the Php\Filter class and created a GWToolset\Util class, for use
in another request chris made, that contains static methods that replace those
that were used in Php\Filter. while re-factoring the code i realized that the
array handling in Php\Filter was no longer needed, so i did not create a method
for that at this time.

Change-Id: I9ca64ef17199995856a72484230ba92874e64a97
filter

chris steipp requested that we:

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

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

i have removed the Php\Filter class and created a GWToolset\Util class, also for
use in another request chris made, that contains static methods that replace those
that were used in Php\Filter. while re-factoring the code i realized that the
array handling in Php\Filter was no longer needed, so i did not create a method
for that at this time.

Change-Id: I9ca64ef17199995856a72484230ba92874e64a97
functions

chris steipp suggested:

* is there a reason these aren't static methods in a utility class?

Change-Id: I547197cecfa0a7873d257b9882afd4663ec337f7
form-handler

crhis steipp requested that FormHandler->getForm() check that the form
class is an expected form class.

Change-Id: I75833ff75d51aa64079396bff83c3cfad71bfb75
max-file-size

chris steipp requested that:

* we remove MAX_FILE_SIZE since we shouldn’t rely on it, are not using it,
  and don’t want to accidentally suggest to another developer that we
  are using it.

* we not urldecode in getUserOptions() after filtering the $_POSTed data,
  since it invalidates any urlencoding we may have done.

Change-Id: Id23818f914d0d7314e78f2dbf888bffe6343abe9
external-entity

chris steipp requested that:

* XMLReader must disable external entity loading before reading xml

i have included the corresponding libxml_disable_entity_loader() call and added a check
for an XMLReader::DOC_TYPE node in the XML document; if it exists, the extension will
delete the file and notify the user. we don’t need to accept XML metadata files with
<!DOCTYPE> declarations.

i also removed the outdated readXmlAsString method and its related calls and i18n messages.

Change-Id: I3f74c176c7da8fb47a68e4cdb0db0146d6f6a946
functions

chris steipp suggested:

* is there a reason these aren't static methods in a utility class?

Change-Id: I547197cecfa0a7873d257b9882afd4663ec337f7
form-handler

crhis steipp requested that FormHandler->getForm() check that the form
class is an expected form class.

Change-Id: I75833ff75d51aa64079396bff83c3cfad71bfb75
sanitized-exceptions

chris steipp requested that:

* in includes/Specials/SpecialGWToolset.php, line 143: Please log, don't print_r

chris steipp suggested that:

* it would make things easier if you could escape the exception message, or
  demonstrate that it's been properly sanitized.

i augmented the GWTException class so that it now takes in wfMessage() keys and their
parameters and uses its ->parse() method to sanitize the message or assumes a regular
$message string and sanitizes it before use. adjusted all throw GWTExceptions in order
to properly use the new functionality.

i also corrected two bugs i found while working on this issue: the re-upload media file
checkbox wasn’t being picked-up and when no metadata file was provided for upload, a
MWException was being thrown instead of providing a user message.

Change-Id: I64b47093f636a419242e71a1fa8c9c65453083ec
form-handler

chris steipp requested that FormHandler->getForm() check that the form
class is an expected form class.

Change-Id: I4d869cc8fa4c1607f199aa0de4115e7b89d2a1c6
max-file-size

chris steipp requested that:

* we remove MAX_FILE_SIZE since we shouldn’t rely on it, are not using it,
  and don’t want to accidentally suggest to another developer that we
  are using it.

* we not urldecode in getUserOptions() after filtering the $_POSTed data,
  since it invalidates any urlencoding we may have done.

Change-Id: I314de486214c23a90c221924425a81f836ba3082
external-entity

chris steipp requested that:

* XMLReader must disable external entity loading before reading xml

i have included the corresponding libxml_disable_entity_loader() call and added a check
for an XMLReader::DOC_TYPE node in the XML document; if it exists, the extension will
delete the file and notify the user. we don’t need to accept XML metadata files with
<!DOCTYPE> declarations.

i also removed the outdated readXmlAsString method and its related calls and i18n messages.

Change-Id: I638d4294ba759f328388d3e80a72888d58cb8e1d
sanitized-exceptions

chris steipp requested that:

* in includes/Specials/SpecialGWToolset.php, line 143: Please log, don't print_r

chris steipp suggested that:

* it would make things easier if you could escape the exception message, or
  demonstrate that it's been properly sanitized.

i augmented the GWTException class so that it now takes in wfMessage() keys and their
parameters and uses its ->parse() method to sanitize the message or assumes a regular
$message string and sanitizes it before use. adjusted all throw GWTExceptions in order
to properly use the new functionality.

i also corrected two bugs i found while working on this issue: the re-upload media file
checkbox wasn’t being picked-up and when no metadata file was provided for upload, a
MWException was being thrown instead of providing a user message.