From 85e4c16f39a8b00b229644bcd175663541dfd51a Mon Sep 17 00:00:00 2001 From: Steve Clay Date: Mon, 4 Feb 2013 20:37:25 -0500 Subject: Doc fixes and inline type hints for variables (big static analysis cleanup) --- engine/classes/ElggXMLElement.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'engine/classes/ElggXMLElement.php') diff --git a/engine/classes/ElggXMLElement.php b/engine/classes/ElggXMLElement.php index 65a13912c..4e4b7e63c 100644 --- a/engine/classes/ElggXMLElement.php +++ b/engine/classes/ElggXMLElement.php @@ -32,7 +32,7 @@ class ElggXMLElement { } /** - * @return array:string The attributes + * @return string[] The attributes */ public function getAttributes() { //include namespace declarations as attributes @@ -64,7 +64,7 @@ class ElggXMLElement { } /** - * @return array:ElggXMLElement Child elements + * @return ElggXMLElement[] Child elements */ public function getChildren() { $children = $this->_element->children(); -- cgit v1.2.3 From ea4ce20b3632a3c55ffedfad1ad53845db5a7e12 Mon Sep 17 00:00:00 2001 From: cash Date: Sat, 16 Mar 2013 12:03:21 -0400 Subject: coding standards --- engine/classes/ElggAccess.php | 4 ++++ engine/classes/ElggCache.php | 4 ++++ engine/classes/ElggData.php | 2 ++ engine/classes/ElggFileCache.php | 4 ++++ engine/classes/ElggXMLElement.php | 8 ++++++++ 5 files changed, 22 insertions(+) (limited to 'engine/classes/ElggXMLElement.php') diff --git a/engine/classes/ElggAccess.php b/engine/classes/ElggAccess.php index 6f8d9bb4b..0aed477fc 100644 --- a/engine/classes/ElggAccess.php +++ b/engine/classes/ElggAccess.php @@ -16,6 +16,7 @@ class ElggAccess { */ private $ignore_access; + // @codingStandardsIgnoreStart /** * Get current ignore access setting. * @@ -26,6 +27,7 @@ class ElggAccess { elgg_deprecated_notice('ElggAccess::get_ignore_access() is deprecated by ElggAccess::getIgnoreAccess()', 1.8); return $this->getIgnoreAccess(); } + // @codingStandardsIgnoreEnd /** * Get current ignore access setting. @@ -36,6 +38,7 @@ class ElggAccess { return $this->ignore_access; } + // @codingStandardsIgnoreStart /** * Set ignore access. * @@ -49,6 +52,7 @@ class ElggAccess { elgg_deprecated_notice('ElggAccess::set_ignore_access() is deprecated by ElggAccess::setIgnoreAccess()', 1.8); return $this->setIgnoreAccess($ignore); } + // @codingStandardsIgnoreEnd /** * Set ignore access. diff --git a/engine/classes/ElggCache.php b/engine/classes/ElggCache.php index 4317f4be9..909eab39b 100644 --- a/engine/classes/ElggCache.php +++ b/engine/classes/ElggCache.php @@ -21,6 +21,7 @@ abstract class ElggCache implements ArrayAccess { $this->variables = array(); } + // @codingStandardsIgnoreStart /** * Set a cache variable. * @@ -35,6 +36,7 @@ abstract class ElggCache implements ArrayAccess { elgg_deprecated_notice('ElggCache::set_variable() is deprecated by ElggCache::setVariable()', 1.8); $this->setVariable($variable, $value); } + // @codingStandardsIgnoreEnd /** * Set a cache variable. @@ -52,6 +54,7 @@ abstract class ElggCache implements ArrayAccess { $this->variables[$variable] = $value; } + // @codingStandardsIgnoreStart /** * Get variables for this cache. * @@ -65,6 +68,7 @@ abstract class ElggCache implements ArrayAccess { elgg_deprecated_notice('ElggCache::get_variable() is deprecated by ElggCache::getVariable()', 1.8); return $this->getVariable($variable); } + // @codingStandardsIgnoreEnd /** * Get variables for this cache. diff --git a/engine/classes/ElggData.php b/engine/classes/ElggData.php index 426248ca3..4f843cde4 100644 --- a/engine/classes/ElggData.php +++ b/engine/classes/ElggData.php @@ -26,6 +26,7 @@ abstract class ElggData implements */ protected $attributes = array(); + // @codingStandardsIgnoreStart /** * Initialise the attributes array. * @@ -44,6 +45,7 @@ abstract class ElggData implements elgg_deprecated_notice('initialise_attributes() is deprecated by initializeAttributes()', 1.8); } } + // @codingStandardsIgnoreEnd /** * Initialize the attributes array. diff --git a/engine/classes/ElggFileCache.php b/engine/classes/ElggFileCache.php index e654f1db2..94143f777 100644 --- a/engine/classes/ElggFileCache.php +++ b/engine/classes/ElggFileCache.php @@ -26,6 +26,7 @@ class ElggFileCache extends ElggCache { } } + // @codingStandardsIgnoreStart /** * Create and return a handle to a file. * @@ -41,6 +42,7 @@ class ElggFileCache extends ElggCache { return $this->createFile($filename, $rw); } + // @codingStandardsIgnoreEnd /** * Create and return a handle to a file. @@ -72,6 +74,7 @@ class ElggFileCache extends ElggCache { return fopen($path . $filename, $rw); } + // @codingStandardsIgnoreStart /** * Create a sanitised filename for the file. * @@ -86,6 +89,7 @@ class ElggFileCache extends ElggCache { return $filename; } + // @codingStandardsIgnoreEnd /** * Create a sanitised filename for the file. diff --git a/engine/classes/ElggXMLElement.php b/engine/classes/ElggXMLElement.php index 4e4b7e63c..d7e912035 100644 --- a/engine/classes/ElggXMLElement.php +++ b/engine/classes/ElggXMLElement.php @@ -76,6 +76,10 @@ class ElggXMLElement { return $result; } + /** + * @param string $name Property name + * @return mixed + */ function __get($name) { switch ($name) { case 'name': @@ -94,6 +98,10 @@ class ElggXMLElement { return null; } + /** + * @param string $name Property name + * @return boolean + */ function __isset($name) { switch ($name) { case 'name': -- cgit v1.2.3 From 00819122111a081c17f1ae4c53974b0deb50757c Mon Sep 17 00:00:00 2001 From: cash Date: Sat, 16 Mar 2013 12:41:16 -0400 Subject: more coding standard fixes --- engine/classes/ElggAttributeLoader.php | 30 ++++++++- engine/classes/ElggAutoP.php | 24 +++++--- engine/classes/ElggDiskFilestore.php | 9 +++ engine/classes/ElggVolatileMetadataCache.php | 92 +++++++++++++++------------- engine/classes/ElggXMLElement.php | 4 ++ engine/lib/opendd.php | 4 ++ 6 files changed, 110 insertions(+), 53 deletions(-) (limited to 'engine/classes/ElggXMLElement.php') diff --git a/engine/classes/ElggAttributeLoader.php b/engine/classes/ElggAttributeLoader.php index 2d1c1abde..d1e15008e 100644 --- a/engine/classes/ElggAttributeLoader.php +++ b/engine/classes/ElggAttributeLoader.php @@ -4,6 +4,9 @@ * Loads ElggEntity attributes from DB or validates those passed in via constructor * * @access private + * + * @package Elgg.Core + * @subpackage DataModel */ class ElggAttributeLoader { @@ -65,9 +68,11 @@ class ElggAttributeLoader { public $full_loader = ''; /** - * @param string $class class of object being loaded - * @param string $required_type entity type this is being used to populate - * @param array $initialized_attrs attributes after initializeAttributes() has been run + * Constructor + * + * @param string $class class of object being loaded + * @param string $required_type entity type this is being used to populate + * @param array $initialized_attrs attributes after initializeAttributes() has been run * @throws InvalidArgumentException */ public function __construct($class, $required_type, array $initialized_attrs) { @@ -87,14 +92,33 @@ class ElggAttributeLoader { $this->secondary_attr_names = array_diff($all_attr_names, self::$primary_attr_names); } + /** + * Get primary attributes missing that are missing + * + * @param stdClass $row Database row + * @return array + */ protected function isMissingPrimaries($row) { return array_diff(self::$primary_attr_names, array_keys($row)) !== array(); } + /** + * Get secondary attributes that are missing + * + * @param stdClass $row Database row + * @return array + */ protected function isMissingSecondaries($row) { return array_diff($this->secondary_attr_names, array_keys($row)) !== array(); } + /** + * Check that the type is correct + * + * @param stdClass $row Database row + * @return void + * @throws InvalidClassException + */ protected function checkType($row) { if ($row['type'] !== $this->required_type) { $msg = elgg_echo('InvalidClassException:NotValidElggStar', array($row['guid'], $this->class)); diff --git a/engine/classes/ElggAutoP.php b/engine/classes/ElggAutoP.php index f3c7cc972..71536c433 100644 --- a/engine/classes/ElggAutoP.php +++ b/engine/classes/ElggAutoP.php @@ -7,6 +7,9 @@ * * In DIV elements, Ps are only added when there would be at * least two of them. + * + * @package Elgg.Core + * @subpackage Output */ class ElggAutoP { @@ -51,8 +54,12 @@ class ElggAutoP { protected $_alterList = 'article aside blockquote body details div footer header section'; + /** @var string */ protected $_unique = ''; + /** + * Constructor + */ public function __construct() { $this->_blocks = preg_split('@\\s+@', $this->_blocks); $this->_descendList = preg_split('@\\s+@', $this->_descendList); @@ -98,7 +105,7 @@ class ElggAutoP { $html = str_replace('&', $this->_unique . 'AMP', $html); $this->_doc = new DOMDocument(); - + // parse to DOM, suppressing loadHTML warnings // http://www.php.net/manual/en/domdocument.loadhtml.php#95463 libxml_use_internal_errors(true); @@ -112,7 +119,7 @@ class ElggAutoP { $this->_xpath = new DOMXPath($this->_doc); // start processing recursively at the BODY element $nodeList = $this->_xpath->query('//body[1]'); - $this->_addParagraphs($nodeList->item(0)); + $this->addParagraphs($nodeList->item(0)); // serialize back to HTML $html = $this->_doc->saveHTML(); @@ -187,15 +194,16 @@ class ElggAutoP { /** * Add P and BR elements as necessary * - * @param DOMElement $el + * @param DOMElement $el DOM element + * @return void */ - protected function _addParagraphs(DOMElement $el) { + protected function addParagraphs(DOMElement $el) { // no need to call recursively, just queue up $elsToProcess = array($el); $inlinesToProcess = array(); while ($el = array_shift($elsToProcess)) { // if true, we can alter all child nodes, if not, we'll just call - // _addParagraphs on each element in the descendInto list + // addParagraphs on each element in the descendInto list $alterInline = in_array($el->nodeName, $this->_alterList); // inside affected elements, we want to trim leading whitespace from @@ -229,8 +237,8 @@ class ElggAutoP { if ($alterInline) { $isText = ($node->nodeType === XML_TEXT_NODE); $isLastInline = (! $node->nextSibling - || ($node->nextSibling->nodeType === XML_ELEMENT_NODE - && in_array($node->nextSibling->nodeName, $this->_blocks))); + || ($node->nextSibling->nodeType === XML_ELEMENT_NODE + && in_array($node->nextSibling->nodeName, $this->_blocks))); if ($isElement) { $isFollowingBr = ($node->nodeName === 'br'); } @@ -263,7 +271,7 @@ class ElggAutoP { if ($isBlock) { if (in_array($node->nodeName, $this->_descendList)) { $elsToProcess[] = $node; - //$this->_addParagraphs($node); + //$this->addParagraphs($node); } } $openP = true; diff --git a/engine/classes/ElggDiskFilestore.php b/engine/classes/ElggDiskFilestore.php index 7374aad35..29547d83b 100644 --- a/engine/classes/ElggDiskFilestore.php +++ b/engine/classes/ElggDiskFilestore.php @@ -254,6 +254,7 @@ class ElggDiskFilestore extends ElggFilestore { } } + // @codingStandardsIgnoreStart /** * Create a directory $dirroot * @@ -268,6 +269,7 @@ class ElggDiskFilestore extends ElggFilestore { return $this->makeDirectoryRoot($dirroot); } + // @codingStandardsIgnoreEnd /** * Create a directory $dirroot @@ -287,6 +289,7 @@ class ElggDiskFilestore extends ElggFilestore { return true; } + // @codingStandardsIgnoreStart /** * Multibyte string tokeniser. * @@ -318,7 +321,9 @@ class ElggDiskFilestore extends ElggFilestore { return str_split($string); } } + // @codingStandardsIgnoreEnd + // @codingStandardsIgnoreStart /** * Construct a file path matrix for an entity. * @@ -332,6 +337,7 @@ class ElggDiskFilestore extends ElggFilestore { return $this->makefileMatrix($identifier); } + // @codingStandardsIgnoreEnd /** * Construct a file path matrix for an entity. @@ -351,7 +357,9 @@ class ElggDiskFilestore extends ElggFilestore { return "$time_created/$entity->guid/"; } + // @codingStandardsIgnoreEnd + // @codingStandardsIgnoreStart /** * Construct a filename matrix. * @@ -370,6 +378,7 @@ class ElggDiskFilestore extends ElggFilestore { return $this->makeFileMatrix($guid); } + // @codingStandardsIgnoreEnd /** * Returns a list of attributes to save to the database when saving diff --git a/engine/classes/ElggVolatileMetadataCache.php b/engine/classes/ElggVolatileMetadataCache.php index 8a33c198d..4acda7cee 100644 --- a/engine/classes/ElggVolatileMetadataCache.php +++ b/engine/classes/ElggVolatileMetadataCache.php @@ -33,9 +33,11 @@ class ElggVolatileMetadataCache { protected $ignoreAccess = null; /** - * @param int $entity_guid - * - * @param array $values + * Cache metadata for an entity + * + * @param int $entity_guid The GUID of the entity + * @param array $values The metadata values to cache + * @return void */ public function saveAll($entity_guid, array $values) { if (!$this->getIgnoreAccess()) { @@ -45,8 +47,9 @@ class ElggVolatileMetadataCache { } /** - * @param int $entity_guid - * + * Get the metadata for an entity + * + * @param int $entity_guid The GUID of the entity * @return array */ public function loadAll($entity_guid) { @@ -61,15 +64,17 @@ class ElggVolatileMetadataCache { * Declare that there may be fetch-able metadata names in storage that this * cache doesn't know about * - * @param int $entity_guid + * @param int $entity_guid The GUID of the entity + * @return void */ public function markOutOfSync($entity_guid) { unset($this->isSynchronized[$entity_guid]); } /** - * @param $entity_guid - * + * Have all the metadata for this entity been cached? + * + * @param int $entity_guid The GUID of the entity * @return bool */ public function isSynchronized($entity_guid) { @@ -77,13 +82,15 @@ class ElggVolatileMetadataCache { } /** - * @param int $entity_guid - * - * @param string $name - * - * @param array|int|string|null $value null means it is known that there is no - * fetch-able metadata under this name - * @param bool $allow_multiple + * Cache a piece of metadata + * + * @param int $entity_guid The GUID of the entity + * @param string $name The metadata name + * @param array|int|string|null $value The metadata value. null means it is + * known that there is no fetch-able + * metadata under this name + * @param bool $allow_multiple Can the metadata be an array + * @return void */ public function save($entity_guid, $name, $value, $allow_multiple = false) { if ($this->getIgnoreAccess()) { @@ -115,10 +122,8 @@ class ElggVolatileMetadataCache { * function's return value should be trusted (otherwise a null return value * is ambiguous). * - * @param int $entity_guid - * - * @param string $name - * + * @param int $entity_guid The GUID of the entity + * @param string $name The metadata name * @return array|string|int|null null = value does not exist */ public function load($entity_guid, $name) { @@ -133,9 +138,9 @@ class ElggVolatileMetadataCache { * Forget about this metadata entry. We don't want to try to guess what the * next fetch from storage will return * - * @param int $entity_guid - * - * @param string $name + * @param int $entity_guid The GUID of the entity + * @param string $name The metadata name + * @return void */ public function markUnknown($entity_guid, $name) { unset($this->values[$entity_guid][$name]); @@ -145,10 +150,8 @@ class ElggVolatileMetadataCache { /** * If true, load() will return an accurate value for this name * - * @param int $entity_guid - * - * @param string $name - * + * @param int $entity_guid The GUID of the entity + * @param string $name The metadata name * @return bool */ public function isKnown($entity_guid, $name) { @@ -163,10 +166,8 @@ class ElggVolatileMetadataCache { /** * Declare that metadata under this name is known to be not fetch-able from storage * - * @param int $entity_guid - * - * @param string $name - * + * @param int $entity_guid The GUID of the entity + * @param string $name The metadata name * @return array */ public function markEmpty($entity_guid, $name) { @@ -176,7 +177,8 @@ class ElggVolatileMetadataCache { /** * Forget about all metadata for an entity * - * @param int $entity_guid + * @param int $entity_guid The GUID of the entity + * @return void */ public function clear($entity_guid) { $this->values[$entity_guid] = array(); @@ -185,6 +187,8 @@ class ElggVolatileMetadataCache { /** * Clear entire cache and mark all entities as out of sync + * + * @return void */ public function flush() { $this->values = array(); @@ -197,7 +201,8 @@ class ElggVolatileMetadataCache { * * This setting makes this component a little more loosely-coupled. * - * @param bool $ignore + * @param bool $ignore Whether to ignore access or not + * @return void */ public function setIgnoreAccess($ignore) { $this->ignoreAccess = (bool) $ignore; @@ -205,12 +210,16 @@ class ElggVolatileMetadataCache { /** * Tell the cache to call elgg_get_ignore_access() to determing access status. + * + * @return void */ public function unsetIgnoreAccess() { $this->ignoreAccess = null; } /** + * Get the ignore access value + * * @return bool */ protected function getIgnoreAccess() { @@ -225,12 +234,10 @@ class ElggVolatileMetadataCache { * Invalidate based on options passed to the global *_metadata functions * * @param string $action Action performed on metadata. "delete", "disable", or "enable" - * - * @param array $options Options passed to elgg_(delete|disable|enable)_metadata - * - * "guid" if given, invalidation will be limited to this entity - * - * "metadata_name" if given, invalidation will be limited to metadata with this name + * @param array $options Options passed to elgg_(delete|disable|enable)_metadata + * "guid" if given, invalidation will be limited to this entity + * "metadata_name" if given, invalidation will be limited to metadata with this name + * @return void */ public function invalidateByOptions($action, array $options) { // remove as little as possible, optimizing for common cases @@ -254,7 +261,10 @@ class ElggVolatileMetadataCache { } /** - * @param int|array $guids + * Populate the cache from a set of entities + * + * @param int|array $guids Array of or single GUIDs + * @return void */ public function populateFromEntities($guids) { if (empty($guids)) { @@ -318,9 +328,7 @@ class ElggVolatileMetadataCache { * cache if RAM usage becomes an issue. * * @param array $guids GUIDs of entities to examine - * - * @param int $limit Limit in characters of all metadata (with ints casted to strings) - * + * @param int $limit Limit in characters of all metadata (with ints casted to strings) * @return array */ public function filterMetadataHeavyEntities(array $guids, $limit = 1024000) { diff --git a/engine/classes/ElggXMLElement.php b/engine/classes/ElggXMLElement.php index d7e912035..6f2633e25 100644 --- a/engine/classes/ElggXMLElement.php +++ b/engine/classes/ElggXMLElement.php @@ -77,6 +77,8 @@ class ElggXMLElement { } /** + * Override -> + * * @param string $name Property name * @return mixed */ @@ -99,6 +101,8 @@ class ElggXMLElement { } /** + * Override isset + * * @param string $name Property name * @return boolean */ diff --git a/engine/lib/opendd.php b/engine/lib/opendd.php index f00ea6aab..7d635a295 100644 --- a/engine/lib/opendd.php +++ b/engine/lib/opendd.php @@ -7,6 +7,8 @@ * @version 0.4 */ +// @codingStandardsIgnoreStart + /** * Attempt to construct an ODD object out of a XmlElement or sub-elements. * @@ -103,3 +105,5 @@ function ODD_Import($xml) { function ODD_Export(ODDDocument $document) { return "$document"; } + +// @codingStandardsIgnoreEnd -- cgit v1.2.3 From d53447f7e6b3277f3249d9a70e56ec01a90c3a60 Mon Sep 17 00:00:00 2001 From: Steve Clay Date: Thu, 11 Jul 2013 13:24:01 -0400 Subject: Disable loading external entities during XML parsing --- engine/classes/ElggAutoP.php | 14 ++++++++++++++ engine/classes/ElggXMLElement.php | 8 ++++++-- engine/tests/regression/trac_bugs.php | 10 ++++++++++ engine/tests/test_files/xxe/external_entity.txt | 1 + engine/tests/test_files/xxe/request.xml | 8 ++++++++ 5 files changed, 39 insertions(+), 2 deletions(-) create mode 100644 engine/tests/test_files/xxe/external_entity.txt create mode 100644 engine/tests/test_files/xxe/request.xml (limited to 'engine/classes/ElggXMLElement.php') diff --git a/engine/classes/ElggAutoP.php b/engine/classes/ElggAutoP.php index 71536c433..05842d1b2 100644 --- a/engine/classes/ElggAutoP.php +++ b/engine/classes/ElggAutoP.php @@ -110,12 +110,19 @@ class ElggAutoP { // http://www.php.net/manual/en/domdocument.loadhtml.php#95463 libxml_use_internal_errors(true); + // Do not load entities. May be unnecessary, better safe than sorry + $disable_load_entities = libxml_disable_entity_loader(true); + if (!$this->_doc->loadHTML("{$html}" . "")) { + + libxml_disable_entity_loader($disable_load_entities); return false; } + libxml_disable_entity_loader($disable_load_entities); + $this->_xpath = new DOMXPath($this->_doc); // start processing recursively at the BODY element $nodeList = $this->_xpath->query('//body[1]'); @@ -135,9 +142,16 @@ class ElggAutoP { // re-parse so we can handle new AUTOP elements + // Do not load entities. May be unnecessary, better safe than sorry + $disable_load_entities = libxml_disable_entity_loader(true); + if (!$this->_doc->loadHTML($html)) { + libxml_disable_entity_loader($disable_load_entities); return false; } + + libxml_disable_entity_loader($disable_load_entities); + // must re-create XPath object after DOM load $this->_xpath = new DOMXPath($this->_doc); diff --git a/engine/classes/ElggXMLElement.php b/engine/classes/ElggXMLElement.php index 6f2633e25..cbd3fc5ce 100644 --- a/engine/classes/ElggXMLElement.php +++ b/engine/classes/ElggXMLElement.php @@ -20,7 +20,12 @@ class ElggXMLElement { if ($xml instanceof SimpleXMLElement) { $this->_element = $xml; } else { + // do not load entities + $disable_load_entities = libxml_disable_entity_loader(true); + $this->_element = new SimpleXMLElement($xml); + + libxml_disable_entity_loader($disable_load_entities); } } @@ -123,5 +128,4 @@ class ElggXMLElement { } return false; } - -} \ No newline at end of file +} diff --git a/engine/tests/regression/trac_bugs.php b/engine/tests/regression/trac_bugs.php index ef1348cf6..e6773c8af 100644 --- a/engine/tests/regression/trac_bugs.php +++ b/engine/tests/regression/trac_bugs.php @@ -373,4 +373,14 @@ class ElggCoreRegressionBugsTest extends ElggCoreUnitTest { //delete group and annotations $group->delete(); } + + public function test_ElggXMLElement_does_not_load_external_entities() { + $payload = file_get_contents(dirname(dirname(__FILE__)) . '/test_files/xxe/request.xml'); + $payload = sprintf($payload, 'file://' . realpath(dirname(dirname(__FILE__)) . '/test_files/xxe/external_entity.txt')); + + $el = new ElggXMLElement($payload); + $chidren = $el->getChildren(); + $content = $chidren[0]->getContent(); + $this->assertNoPattern('/secret/', $content); + } } diff --git a/engine/tests/test_files/xxe/external_entity.txt b/engine/tests/test_files/xxe/external_entity.txt new file mode 100644 index 000000000..536aca34d --- /dev/null +++ b/engine/tests/test_files/xxe/external_entity.txt @@ -0,0 +1 @@ +secret \ No newline at end of file diff --git a/engine/tests/test_files/xxe/request.xml b/engine/tests/test_files/xxe/request.xml new file mode 100644 index 000000000..4390f9db2 --- /dev/null +++ b/engine/tests/test_files/xxe/request.xml @@ -0,0 +1,8 @@ + + + +]> + + test&xxe;test + -- cgit v1.2.3