From 38405b46c973ddc63252dd63ed81488d3ba4baab Mon Sep 17 00:00:00 2001 From: gggeek Date: Wed, 25 Jan 2023 11:29:51 +0000 Subject: [PATCH] review BC of parser --- NEWS.md | 13 +++++-- src/Helper/XMLParser.php | 79 +++++++++++++++++++++++----------------- 2 files changed, 54 insertions(+), 38 deletions(-) diff --git a/NEWS.md b/NEWS.md index 03a49fc1..7350994c 100644 --- a/NEWS.md +++ b/NEWS.md @@ -130,6 +130,7 @@ - a regular expression has been introduced to check incoming methodname elements. In the default configuration it will trigger error messages in the logs, but not reject the calls. It can be tweaked via use of `PhpXmlRpc\PhpXmlRpc::$xmlrpc_methodname_format` + - an error message will now be generated if, in incoming data, a STRUCT element has no NAME - parameters `$timeout` and `$method` are now considered deprecated in `Client::send()` and `Client::multicall()` - Client properties `$errno` and `$errstring` are now deprecated - direct access to `Wrapper::$objHolder` is now deprecated @@ -140,15 +141,19 @@ - the `$options` argument passed to `XMLParser::parse` will now contain both options intended to be passed down to the php xml parser, and further options used to tweak the parsing results. If you have subclassed `XMLParser` - and reimplemented the `parse` methods, or wholesale replaced it, you will have to adapt your code + and reimplemented the `parse` methods, or wholesale replaced it, you will have to adapt your code: both for that, + and for making sure that it sets `$this->current_parsing_options['xmlrpc_null_extension']` from + `PhpXmlRpc::$xmlrpc_null_extension` - also, if you had reimplemented `XMLParser::parse`, be warned that the callers now treat differently results when `_xh['isf'] > 3` - - new methods in helper classes: `Charset::knownCharsets`, `Http::parseAcceptHeader`, `XMLParser::truncateForLog` + - new methods in helper classes: `Charset::knownCharsets`, `Http::parseAcceptHeader`, `XMLParser::truncateValueForLog` - if you had been somehow interacting with private method `Client::_try_multicall`, be warned its returned data has changed: it now returns a Response for the cases in which it previously returned false, and an array of Response objects for the cases in which it previously returned a string - - if you subclassed the `Client` class, take care of new static variables `$requestClass` and `$responseClass` - - if you replaced the Logger class, take care that you will have to implement methods `error` and `debug` + - if you subclassed the `Client` class, take care of new static variables `$requestClass` and `$responseClass`, + which should be used to instantiate requests and responses + - if you replaced the Logger class, take care that you will have to implement methods `error` and `debug` (all is ok + if you subclassed id) - traits have been introduced for all classes dealing with Logger, XMLParser and CharsetEncoder; method `setCharsetEncoder` is now static diff --git a/src/Helper/XMLParser.php b/src/Helper/XMLParser.php index e4198dda..ddfeefd2 100644 --- a/src/Helper/XMLParser.php +++ b/src/Helper/XMLParser.php @@ -106,8 +106,16 @@ class XMLParser /** @var int $maxChunkLength 4 MB by default. Any value below 10MB should be good */ protected $maxChunkLength = 4194304; - /** @var array used keys: accept, target_charset, methodname_callback, xmlrpc_null_extension, xmlrpc_return_datetimes */ - protected $current_parsing_options = array(); + /** @var array + * Used keys: accept, target_charset, methodname_callback, plus the ones set here. + * We initialize it partially to help keep BC with subclasses which might have reimplemented `parse()` but not + * the element handler methods + */ + protected $current_parsing_options = array( + 'xmlrpc_null_extension' => false, + 'xmlrpc_return_datetimes' => false, + 'xmlrpc_reject_invalid_values' => false + ); /** * @param array $options integer keys: options passed to the inner xml parser @@ -302,6 +310,7 @@ class XMLParser // top level element can only be of 2 types /// @todo optimization creep: save this check into a bool variable, instead of using count() every time: /// there is only a single top level element in xml anyway + // BC if ($acceptSingleVals === false) { $accept = $this->current_parsing_options['accept']; @@ -379,11 +388,11 @@ class XMLParser return; } // create an empty array to hold child values, and push it onto appropriate stack - $curVal = array(); - $curVal['values'] = array(); - $curVal['type'] = $name; - // check for out-of-band information to rebuild php objs - // and in case it is found, save it + $curVal = array( + 'values' => array(), + 'type' => $name, + ); + // check for out-of-band information to rebuild php objs and, in case it is found, save it if (@isset($attrs['PHP_CLASS'])) { $curVal['php_class'] = $attrs['PHP_CLASS']; } @@ -437,7 +446,7 @@ class XMLParser return; } - // reset the accumulator - q: is this necessary at all here? + // reset the accumulator - q: is this necessary at all here? we don't use it on _ee anyway for NILs $this->_xh['ac'] = ''; } else { @@ -581,11 +590,11 @@ class XMLParser if ($this->_xh['ac'] != '0' && strcasecmp($this->_xh['ac'], 'false') !== 0) { if ($this->current_parsing_options['xmlrpc_reject_invalid_values']) { $this->_xh['isf'] = 2; - $this->_xh['isf_reason'] = 'Invalid data received in BOOLEAN value: ' . $this->truncateForLog($this->_xh['ac']); + $this->_xh['isf_reason'] = 'Invalid data received in BOOLEAN value: ' . $this->truncateValueForLog($this->_xh['ac']); return; } else { $this->getLogger()->error('XML-RPC: ' . __METHOD__ . ': invalid data received in BOOLEAN value: ' . - $this->truncateForLog($this->_xh['ac'])); + $this->truncateValueForLog($this->_xh['ac'])); } } $this->_xh['value'] = false; @@ -606,11 +615,11 @@ class XMLParser if ($this->current_parsing_options['xmlrpc_reject_invalid_values']) { $this->_xh['isf'] = 2; - $this->_xh['isf_reason'] = 'Non numeric data received in INT value: ' . $this->truncateForLog($this->_xh['ac']); + $this->_xh['isf_reason'] = 'Non numeric data received in INT value: ' . $this->truncateValueForLog($this->_xh['ac']); return; } else { $this->getLogger()->error('XML-RPC: ' . __METHOD__ . ': non numeric data received in INT: ' . - $this->truncateForLog($this->_xh['ac'])); + $this->truncateValueForLog($this->_xh['ac'])); } /// @todo: find a better way of reporting an error value than this! Use NaN? $this->_xh['value'] = 'ERROR_NON_NUMERIC_FOUND'; @@ -627,11 +636,11 @@ class XMLParser if ($this->current_parsing_options['xmlrpc_reject_invalid_values']) { $this->_xh['isf'] = 2; $this->_xh['isf_reason'] = 'Non numeric data received in DOUBLE value: ' . - $this->truncateForLog($this->_xh['ac']); + $this->truncateValueForLog($this->_xh['ac']); return; } else { $this->getLogger()->error('XML-RPC: ' . __METHOD__ . ': non numeric data received in DOUBLE value: ' . - $this->truncateForLog($this->_xh['ac'])); + $this->truncateValueForLog($this->_xh['ac'])); } $this->_xh['value'] = 'ERROR_NON_NUMERIC_FOUND'; @@ -647,11 +656,11 @@ class XMLParser if (!preg_match(PhpXmlRpc::$xmlrpc_datetime_format, $this->_xh['ac'])) { if ($this->current_parsing_options['xmlrpc_reject_invalid_values']) { $this->_xh['isf'] = 2; - $this->_xh['isf_reason'] = 'Invalid data received in DATETIME value: ' . $this->truncateForLog($this->_xh['ac']); + $this->_xh['isf_reason'] = 'Invalid data received in DATETIME value: ' . $this->truncateValueForLog($this->_xh['ac']); return; } else { $this->getLogger()->error('XML-RPC: ' . __METHOD__ . ': invalid data received in DATETIME value: ' . - $this->truncateForLog($this->_xh['ac'])); + $this->truncateValueForLog($this->_xh['ac'])); } } if ($this->current_parsing_options['xmlrpc_return_datetimes']) { @@ -674,7 +683,7 @@ class XMLParser $v = base64_decode($this->_xh['ac'], true); if ($v === false) { $this->_xh['isf'] = 2; - $this->_xh['isf_reason'] = 'Invalid data received in BASE64 value: '. $this->truncateForLog($this->_xh['ac']); + $this->_xh['isf_reason'] = 'Invalid data received in BASE64 value: '. $this->truncateValueForLog($this->_xh['ac']); return; } } else { @@ -682,7 +691,7 @@ class XMLParser if ($v === '' && $this->_xh['ac'] !== '') { // only the empty string should decode to the empty string $this->getLogger()->error('XML-RPC: ' . __METHOD__ . ': invalid data received in BASE64 value: ' . - $this->truncateForLog($this->_xh['ac'])); + $this->truncateValueForLog($this->_xh['ac'])); } } $this->_xh['value'] = $v; @@ -754,11 +763,11 @@ class XMLParser if (!preg_match(PhpXmlRpc::$xmlrpc_methodname_format, $this->_xh['ac'])) { if ($this->current_parsing_options['xmlrpc_reject_invalid_values']) { $this->_xh['isf'] = 2; - $this->_xh['isf_reason'] = 'Invalid data received in METHODNAME: '. $this->truncateForLog($this->_xh['ac']); + $this->_xh['isf_reason'] = 'Invalid data received in METHODNAME: '. $this->truncateValueForLog($this->_xh['ac']); return; } else { $this->getLogger()->error('XML-RPC: ' . __METHOD__ . ': invalid data received in METHODNAME: '. - $this->truncateForLog($this->_xh['ac'])); + $this->truncateValueForLog($this->_xh['ac'])); } } $methodName = trim($this->_xh['ac']); @@ -995,6 +1004,20 @@ class XMLParser return false; } + /** + * Truncates unsafe data + * @param string $data + * @return string + */ + protected function truncateValueForLog($data) + { + if (strlen($data) > $this->maxLogValueLength) { + return substr($data, 0, $this->maxLogValueLength - 3) . '...'; + } + + return $data; + } + // BC layer public function __set($name, $value) @@ -1002,6 +1025,7 @@ class XMLParser //trigger_error('setting property Response::' . $name . ' is deprecated', E_USER_DEPRECATED); switch ($name) { + // this should only ever be called by subclasses which overtook `parse()` case 'accept': $this->current_parsing_options['accept'] = $value; break; @@ -1026,6 +1050,7 @@ class XMLParser public function __unset($name) { switch ($name) { + // q: does this make sense at all? case 'accept': unset($this->current_parsing_options['accept']); break; @@ -1034,18 +1059,4 @@ class XMLParser trigger_error('Undefined property via __unset(): ' . $name . ' in ' . $trace[0]['file'] . ' on line ' . $trace[0]['line'], E_USER_WARNING); } } - - /** - * Truncates unsafe data - * @param string $data - * @return string - */ - protected function truncateForLog($data) - { - if (strlen($data) > $this->maxLogValueLength) { - return substr($data, 0, $this->maxLogValueLength - 3) . '...'; - } - - return $data; - } } -- 2.47.0