From 2785cdb5023d39d3c33d783ef2e44fa33ee71f91 Mon Sep 17 00:00:00 2001 From: gggeek Date: Wed, 25 Jan 2023 16:01:47 +0000 Subject: [PATCH] introduce structured exceptions; make addParam and Response::__construct stricter; allow serialization of responses built out of php values --- NEWS.md | 6 ++++ doc/manual/phpxmlrpc_manual.adoc | 18 ++++++++++ lib/xmlrpc.inc | 12 ++++++- src/Exception.php | 7 ++++ src/Exception/FaultResponseException.php | 12 +++++++ src/Exception/HttpException.php | 5 ++- src/Exception/NoSuchMethodException.php | 7 ++++ src/Exception/ParsingException.php | 12 +++++++ src/Exception/PhpXmlrpcException.php | 7 ++-- src/Exception/ServerException.php | 12 +++++++ src/Exception/StateErrorException.php | 12 +++++++ src/Exception/TransportException.php | 12 +++++++ src/Exception/TypeErrorException.php | 12 +++++++ src/Exception/ValueErrorException.php | 12 +++++++ src/Exception/XmlException.php | 7 ++++ src/Exception/XmlRpcException.php | 7 ++++ src/Helper/Charset.php | 9 ++--- src/Helper/XMLParser.php | 11 +++--- src/Request.php | 1 + src/Response.php | 46 ++++++++++++++---------- src/Server.php | 9 ++--- src/Value.php | 29 ++++++++------- src/Wrapper.php | 11 +++--- 23 files changed, 220 insertions(+), 56 deletions(-) create mode 100644 src/Exception.php create mode 100644 src/Exception/FaultResponseException.php create mode 100644 src/Exception/NoSuchMethodException.php create mode 100644 src/Exception/ParsingException.php create mode 100644 src/Exception/ServerException.php create mode 100644 src/Exception/StateErrorException.php create mode 100644 src/Exception/TransportException.php create mode 100644 src/Exception/TypeErrorException.php create mode 100644 src/Exception/ValueErrorException.php create mode 100644 src/Exception/XmlException.php create mode 100644 src/Exception/XmlRpcException.php diff --git a/NEWS.md b/NEWS.md index 7350994c..5923099a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -25,6 +25,9 @@ NB: if the received strings are not parseable as dates, NULL will be returned instead of an object (but that can be avoided by setting `PhpXmlRpc\PhpXmlRpc::$xmlrpc_reject_invalid_values = true`, see below). +* improved: be more strict in the `Response` constructor and in `Request::addParam`: both of those will now generate + an error message in the log if passed unexpected values + * improved: be more strict in the data accepted as valid for dateTime xml-rpc values. Clearly invalid dates such as a month '13', day '32' or hour '25' will cause an error message to be logged or the value to be rejected, depending on configuration @@ -89,6 +92,8 @@ * improved: made sure all debug output goes through the logger at response parsing time (there was one printf call left) +* improved: all the Exceptions thrown by the library are now `\PhpXmlRpc\Exception` or subclasses thereof + * improved: all the Client's `setSomething()` methods now return the client object, allowing for usage of fluent style calling. The same applies to `Request::setDebug` @@ -156,6 +161,7 @@ if you subclassed id) - traits have been introduced for all classes dealing with Logger, XMLParser and CharsetEncoder; method `setCharsetEncoder` is now static + - exception `\PhpXmlRpc\Exception\PhpXmlRpcException` is deprecated. Use `\PhpXmlRpc\Exception` instead ## XML-RPC for PHP version 4.9.5 - 2023/01/11 diff --git a/doc/manual/phpxmlrpc_manual.adoc b/doc/manual/phpxmlrpc_manual.adoc index 1e6ed30f..540a34ed 100644 --- a/doc/manual/phpxmlrpc_manual.adoc +++ b/doc/manual/phpxmlrpc_manual.adoc @@ -1880,3 +1880,21 @@ the demo files are also available for download as a separate tarball from the re *Note* when downloading the demo files, make sure that the demo folder is not directly accessible from the internet, i.e. it is not within the webserver root directory. + +== Exception hierarchy + + Exception + `-PhpXmlRpc/Exception + |-PhpXmlRpc/Exception/FaultResponseException.php + |-PhpXmlRpc/Exception/ParsingException.php + | |-PhpXmlRpc/Exception/XmlException.php + | `-PhpXmlRpc/Exception/XmlRpcException.php + |-PhpXmlRpc/Exception/TransportException.php + | `-PhpXmlRpc/Exception/HttpException.php + |-PhpXmlRpc/Exception/ServerException.php + | `-PhpXmlRpc/Exception/NoSuchMethodException.php + |-PhpXmlRpc/Exception/StateErrorException.php + |-PhpXmlRpc/Exception/TypeErrorException.php + `-PhpXmlRpc/Exception/ValueErrorException.php + +*Note* not all of the above exceptions are in use at the moment, but they might be in the future diff --git a/lib/xmlrpc.inc b/lib/xmlrpc.inc index b2e1ea96..5201320e 100644 --- a/lib/xmlrpc.inc +++ b/lib/xmlrpc.inc @@ -54,8 +54,18 @@ include_once(__DIR__.'/../src/PhpXmlRpc.php'); include_once(__DIR__.'/../src/Request.php'); include_once(__DIR__.'/../src/Response.php'); include_once(__DIR__.'/../src/Value.php'); -include_once(__DIR__.'/../src/Exception/PhpXmlrpcException.php'); +include_once(__DIR__.'/../src/Exception.php'); +include_once(__DIR__.'/../src/Exception/FaultResponseException.php'); +include_once(__DIR__.'/../src/Exception/ParsingException.php'); +include_once(__DIR__.'/../src/Exception/XmlException.php'); +include_once(__DIR__.'/../src/Exception/XmlRpcException.php'); +include_once(__DIR__.'/../src/Exception/TransportException.php'); include_once(__DIR__.'/../src/Exception/HttpException.php'); +include_once(__DIR__.'/../src/Exception/ServerException.php'); +include_once(__DIR__.'/../src/Exception/NoSuchMethodException.php'); +include_once(__DIR__.'/../src/Exception/StateErrorException.php'); +include_once(__DIR__.'/../src/Exception/TypeErrorException.php'); +include_once(__DIR__.'/../src/Exception/ValueErrorException.php'); include_once(__DIR__.'/../src/Helper/Charset.php'); include_once(__DIR__.'/../src/Helper/Date.php'); include_once(__DIR__.'/../src/Helper/Http.php'); diff --git a/src/Exception.php b/src/Exception.php new file mode 100644 index 00000000..d5d85b48 --- /dev/null +++ b/src/Exception.php @@ -0,0 +1,7 @@ +xml_iso88591_Entities; default: - throw new \Exception('Unsupported charset: ' . $charset); + throw new ValueErrorException('Unsupported charset: ' . $charset); } } } diff --git a/src/Helper/XMLParser.php b/src/Helper/XMLParser.php index ddfeefd2..45f2bcea 100644 --- a/src/Helper/XMLParser.php +++ b/src/Helper/XMLParser.php @@ -172,8 +172,6 @@ class XMLParser return; } - //$prevAccept = $this->accept; - //$this->accept = $accept; $this->current_parsing_options = array('accept' => $accept); $mergedOptions = $this->parsing_options; @@ -272,17 +270,22 @@ class XMLParser break; } } + /// @todo bump minimum php version to 5.5 and use a finally clause instead of doing cleanup 3 times } catch (\Exception $e) { xml_parser_free($parser); $this->current_parsing_options = array(); - //$this->accept = $prevAccept; /// @todo should we set $this->_xh['isf'] and $this->_xh['isf_reason'] ? throw $e; + } catch (\Error $e) { + xml_parser_free($parser); + $this->current_parsing_options = array(); + //$this->accept = $prevAccept; + /// @todo should we set $this->_xh['isf'] and $this->_xh['isf_reason'] ? + throw $e; } xml_parser_free($parser); $this->current_parsing_options = array(); - //$this->accept = $prevAccept; } /** diff --git a/src/Request.php b/src/Request.php index 311c2417..8a5fcaf0 100644 --- a/src/Request.php +++ b/src/Request.php @@ -141,6 +141,7 @@ class Request return true; } else { + $this->getLogger()->error('XML-RPC: ' . __METHOD__ . ': value passed in must be a PhpXmlRpc\Value'); return false; } } diff --git a/src/Response.php b/src/Response.php index 0ccb46af..b4ab39bc 100644 --- a/src/Response.php +++ b/src/Response.php @@ -2,7 +2,9 @@ namespace PhpXmlRpc; +use PhpXmlRpc\Exception\StateErrorException; use PhpXmlRpc\Traits\CharsetEncoderAware; +use PhpXmlRpc\Traits\LoggerAware; /** * This class provides the representation of the response of an XML-RPC server. @@ -16,6 +18,7 @@ use PhpXmlRpc\Traits\CharsetEncoderAware; class Response { use CharsetEncoderAware; + use LoggerAware; /// @todo: do these need to be public? /** @internal */ @@ -32,18 +35,20 @@ class Response protected $httpResponse = array('headers' => array(), 'cookies' => array(), 'raw_data' => '', 'status_code' => null); /** - * @param Value|string|mixed $val either a Value object, a php value or the xml serialization of an xml-rpc value (a string) + * @param Value|string|mixed $val either a Value object, a php value or the xml serialization of an xml-rpc value (a string). + * Note that using anything other than a Value object wll have an impact on serialization. * @param integer $fCode set it to anything but 0 to create an error response. In that case, $val is discarded * @param string $fString the error string, in case of an error response * @param string $valType The type of $val passed in. Either 'xmlrpcvals', 'phpvals' or 'xml'. Leave empty to let - * the code guess the correct type. + * the code guess the correct type by looking at $val - in which case strings are assumed + * to be serialized xml * @param array|null $httpResponse this should be set when the response is being built out of data received from * http (i.e. not when programmatically building a Response server-side). Array * keys should include, if known: headers, cookies, raw_data, status_code * - * @todo add check that $val / $fCode / $fString is of correct type??? + * @todo add check that $val / $fCode / $fString is of correct type? We could at least log a warning for fishy cases... * NB: as of now we do not do it, since it might be either an xml-rpc value or a plain php val, or a complete - * xml chunk, depending on usage of Client::send() inside which the constructor is called... + * xml chunk, depending on usage of Client::send() inside which the constructor is called. */ public function __construct($val, $fCode = 0, $fString = '', $valType = '', $httpResponse = null) { @@ -64,8 +69,12 @@ class Response $this->valtyp = 'phpvals'; } } else { - // user declares type of resp value: believe him $this->valtyp = $valType; + // user declares the type of resp value: we "almost" trust it... but log errors just in case + if (($this->valtyp == 'xmlrpcvals' && (!is_a($this->val, 'PhpXmlRpc\Value'))) || + ($this->valtyp == 'xml' && (!is_string($this->val)))) { + $this->getLogger()->error('XML-RPC: ' . __METHOD__ . ': value passed in does not match type ' . $valType); + } } } @@ -138,7 +147,7 @@ class Response * * @param string $charsetEncoding the charset to be used for serialization. If null, US-ASCII is assumed * @return string the xml representation of the response - * @throws \Exception + * @throws StateErrorException if the response was built out of a value of an unsupported type */ public function serialize($charsetEncoding = '') { @@ -157,22 +166,21 @@ class Response $result .= "\n" . "\nfaultCode\n" . $this->errno . "\n\n\nfaultString\n" . - $this->getCharsetEncoder()->encodeEntities($this->errstr, PhpXmlRpc::$xmlrpc_internalencoding, $charsetEncoding) . "\n\n" . - "\n\n"; + $this->getCharsetEncoder()->encodeEntities($this->errstr, PhpXmlRpc::$xmlrpc_internalencoding, $charsetEncoding) . + "\n\n\n\n"; } else { - if (!is_object($this->val) || !is_a($this->val, 'PhpXmlRpc\Value')) { - if (is_string($this->val) && $this->valtyp == 'xml') { - $result .= "\n\n" . - $this->val . - "\n"; - } else { - /// @todo try to build something serializable using the Encoder... - throw new \Exception('cannot serialize xmlrpc response objects whose content is native php values'); - } - } else { + if (is_object($this->val) && is_a($this->val, 'PhpXmlRpc\Value')) { + $result .= "\n\n" . $this->val->serialize($charsetEncoding) . "\n"; + } else if (is_string($this->val) && $this->valtyp == 'xml') { $result .= "\n\n" . - $this->val->serialize($charsetEncoding) . + $this->val . "\n"; + } else if ($this->valtyp == 'phpvals') { + $encoder = new Encoder(); + $val = $encoder->encode($this->val); + $result .= "\n\n" . $val->serialize($charsetEncoding) . "\n"; + } else { + throw new StateErrorException('cannot serialize xmlrpc response objects whose content is native php values'); } } $result .= "\n"; diff --git a/src/Server.php b/src/Server.php index 5d7b2bbc..fdec46ce 100644 --- a/src/Server.php +++ b/src/Server.php @@ -2,7 +2,7 @@ namespace PhpXmlRpc; -use PhpXmlRpc\Exception\PhpXmlrpcException; +use PhpXmlRpc\Exception\NoSuchMethodException; use PhpXmlRpc\Helper\Http; use PhpXmlRpc\Helper\Interop; use PhpXmlRpc\Helper\Logger; @@ -567,7 +567,7 @@ class Server $xmlRpcParser = $this->getParser(); try { $xmlRpcParser->parse($data, $this->functions_parameters_type, XMLParser::ACCEPT_REQUEST, $options); - } catch (PhpXmlrpcException $e) { + } catch (NoSuchMethodException $e) { return new Response(0, $e->getCode(), $e->getMessage()); } @@ -751,6 +751,7 @@ class Server $r = new Response($encoder->encode($r, $this->phpvals_encoding_options)); } } + /// @todo bump minimum php version to 7.1 and use a single catch clause instead of the duplicate blocks } catch (\Exception $e) { // (barring errors in the lib) an uncaught exception happened in the called function, we wrap it in a // proper error-response @@ -822,7 +823,7 @@ class Server * @param XMLParser $xmlParser * @param resource $parser * @return void - * @throws PhpXmlrpcException + * @throws NoSuchMethodException * * @todo feature creep - we could validate here that the method in the dispatch map is valid, but that would mean * dirtying a lot the logic, as we would have back to both parseRequest() and execute() methods the info @@ -835,7 +836,7 @@ class Server if (!isset($dmap[$methodName]['function'])) { // No such method - throw new PhpXmlrpcException(PhpXmlRpc::$xmlrpcstr['unknown_method'], PhpXmlRpc::$xmlrpcerr['unknown_method']); + throw new NoSuchMethodException(PhpXmlRpc::$xmlrpcstr['unknown_method'], PhpXmlRpc::$xmlrpcerr['unknown_method']); } // alter on-the-fly the config of the xml parser if needed diff --git a/src/Value.php b/src/Value.php index a12c98f6..276aea0d 100644 --- a/src/Value.php +++ b/src/Value.php @@ -2,6 +2,9 @@ namespace PhpXmlRpc; +use PhpXmlRpc\Exception\StateErrorException; +use PhpXmlRpc\Exception\TypeErrorException; +use PhpXmlRpc\Exception\ValueErrorException; use PhpXmlRpc\Traits\CharsetEncoderAware; use PhpXmlRpc\Traits\LoggerAware; @@ -533,26 +536,26 @@ class Value implements \Countable, \IteratorAggregate, \ArrayAccess * @param mixed $value * @return void * - * @throws \Exception + * @throws ValueErrorException|TypeErrorException */ #[\ReturnTypeWillChange] public function offsetSet($offset, $value) { switch ($this->mytype) { case 3: - if (!($value instanceof \PhpXmlRpc\Value)) { - throw new \Exception('It is only possible to add Value objects to an XML-RPC Struct'); + if (!($value instanceof Value)) { + throw new TypeErrorException('It is only possible to add Value objects to an XML-RPC Struct'); } if (is_null($offset)) { // disallow struct members with empty names - throw new \Exception('It is not possible to add anonymous members to an XML-RPC Struct'); + throw new ValueErrorException('It is not possible to add anonymous members to an XML-RPC Struct'); } else { $this->me['struct'][$offset] = $value; } return; case 2: - if (!($value instanceof \PhpXmlRpc\Value)) { - throw new \Exception('It is only possible to add Value objects to an XML-RPC Array'); + if (!($value instanceof Value)) { + throw new TypeErrorException('It is only possible to add Value objects to an XML-RPC Array'); } if (is_null($offset)) { $this->me['array'][] = $value; @@ -566,13 +569,13 @@ class Value implements \Countable, \IteratorAggregate, \ArrayAccess reset($this->me); $type = key($this->me); if ($type != $offset) { - throw new \Exception(''); + throw new ValueErrorException(''); } $this->me[$type] = $value; return; default: // it would be nice to allow empty values to be turned into non-empty ones this way, but we miss info to do so - throw new \Exception("XML-RPC Value is of type 'undef' and its value can not be set using array index"); + throw new ValueErrorException("XML-RPC Value is of type 'undef' and its value can not be set using array index"); } } @@ -604,7 +607,7 @@ class Value implements \Countable, \IteratorAggregate, \ArrayAccess * @param mixed $offset * @return void * - * @throws \Exception + * @throws ValueErrorException|StateErrorException */ #[\ReturnTypeWillChange] public function offsetUnset($offset) @@ -618,9 +621,9 @@ class Value implements \Countable, \IteratorAggregate, \ArrayAccess return; case 1: // can not remove value from a scalar - throw new \Exception("XML-RPC Value is of type 'scalar' and its value can not be unset using array index"); + throw new StateErrorException("XML-RPC Value is of type 'scalar' and its value can not be unset using array index"); default: - throw new \Exception("XML-RPC Value is of type 'undef' and its value can not be unset using array index"); + throw new StateErrorException("XML-RPC Value is of type 'undef' and its value can not be unset using array index"); } } @@ -629,7 +632,7 @@ class Value implements \Countable, \IteratorAggregate, \ArrayAccess * * @param mixed $offset * @return mixed|Value|null - * @throws \Exception + * @throws StateErrorException */ #[\ReturnTypeWillChange] public function offsetGet($offset) @@ -646,7 +649,7 @@ class Value implements \Countable, \IteratorAggregate, \ArrayAccess return $type == $offset ? $value : null; default: // return null or exception? - throw new \Exception("XML-RPC Value is of type 'undef' and can not be accessed using array index"); + throw new StateErrorException("XML-RPC Value is of type 'undef' and can not be accessed using array index"); } } } diff --git a/src/Wrapper.php b/src/Wrapper.php index bf613c5b..04a0a833 100644 --- a/src/Wrapper.php +++ b/src/Wrapper.php @@ -7,6 +7,7 @@ namespace PhpXmlRpc; +use PhpXmlRpc\Exception\ValueErrorException; use PhpXmlRpc\Traits\LoggerAware; /** @@ -908,7 +909,7 @@ class Wrapper if (is_string($throwFault)) { throw new $throwFault($resp->faultString(), $resp->faultCode()); } else { - throw new \Exception($resp->faultString(), $resp->faultCode()); + throw new \PhpXmlRpc\Exception($resp->faultString(), $resp->faultCode()); } } else if ($decodeFault) { if (is_string($faultResponse) && ((strpos($faultResponse, '%faultCode%') !== false) || @@ -1017,7 +1018,7 @@ class Wrapper $plist = implode(', ', $plist); $mDesc .= ' * @return ' . $this->xmlrpc2PhpType($mSig[0]); if ($throwFault) { - $mDesc .= "\n * @throws " . (is_string($throwFault) ? $throwFault : '\\Exception'); + $mDesc .= "\n * @throws " . (is_string($throwFault) ? $throwFault : '\\PhpXmlRpc\\Exception'); } else if ($decodeFault) { $mDesc .= '|' . gettype($faultResponse) . " (a " . gettype($faultResponse) . " if call fails)"; } else { @@ -1028,7 +1029,7 @@ class Wrapper $innerCode .= " \$res = \${$this_}client->send(\$req, $timeout, '$protocol');\n"; if ($throwFault) { if (!is_string($throwFault)) { - $throwFault = '\\Exception'; + $throwFault = '\\PhpXmlRpc\\Exception'; } $respCode = "throw new $throwFault(\$res->faultString(), \$res->faultCode())"; } else if ($decodeFault) { @@ -1219,7 +1220,7 @@ class Wrapper /** * @param string $index * @return object - * @throws \Exception + * @throws ValueErrorException */ public static function getHeldObject($index) { @@ -1227,6 +1228,6 @@ class Wrapper return self::$objHolder[$index]; } - throw new \Exception("No object held for index '$index'"); + throw new ValueErrorException("No object held for index '$index'"); } } -- 2.47.0