From 34d30056a3eb697730026b16e3068db7bd27329f Mon Sep 17 00:00:00 2001 From: Geoffroy Letournel Date: Thu, 9 Jan 2014 16:24:40 +0100 Subject: [PATCH] #7 Sanitizing sources --- src/Grale/WebDav/Client.php | 3 +- src/Grale/WebDav/Exception/HttpException.php | 23 ++++++++------- src/Grale/WebDav/Lock.php | 8 +++++- src/Grale/WebDav/MultiStatus.php | 7 +++-- src/Grale/WebDav/Property/SupportedLock.php | 2 -- src/Grale/WebDav/PropertySet.php | 2 +- src/Grale/WebDav/Resource.php | 1 + src/Grale/WebDav/Response.php | 2 +- src/Grale/WebDav/StreamWrapper.php | 24 ++++------------ tests/Grale/WebDav/ClientTest.php | 8 ++++-- tests/Grale/WebDav/LockTest.php | 5 +++- .../WebDav/Property/LockDiscoveryTest.php | 3 ++ tests/Grale/WebDav/PropertySetTest.php | 8 +++++- tests/Grale/WebDav/ResourceTest.php | 16 ++++++++++- tests/Grale/WebDav/StreamWrapperTest.php | 28 ++++++++----------- 15 files changed, 81 insertions(+), 59 deletions(-) diff --git a/src/Grale/WebDav/Client.php b/src/Grale/WebDav/Client.php index 6894e8e..29a874d 100644 --- a/src/Grale/WebDav/Client.php +++ b/src/Grale/WebDav/Client.php @@ -600,8 +600,9 @@ protected function resolveUrl($uri) * * @param HttpRequest $request The request * + * @throws Exception\NoSuchResourceException + * @throws Exception\HttpException * @return HttpResponse Returns the server response - * @throws HttpException If an HTTP error is returned */ protected function doRequest(HttpRequest $request) { diff --git a/src/Grale/WebDav/Exception/HttpException.php b/src/Grale/WebDav/Exception/HttpException.php index f5f2760..4c41a16 100644 --- a/src/Grale/WebDav/Exception/HttpException.php +++ b/src/Grale/WebDav/Exception/HttpException.php @@ -89,23 +89,26 @@ public static function factory(BadResponseException $cause) if ($response->isClientError()) { $class = __NAMESPACE__ . '\\ClientFailureException'; - } elseif ($response->isServerError()) { + } else { $class = __NAMESPACE__ . '\\ServerFailureException'; } - $statusCode = $response->getStatusCode(); - $httpMethod = $request->getMethod(); + $exception = new $class($response->getReasonPhrase(), null, $cause); - $e = new $class($response->getReasonPhrase(), null, $cause); - $e->setStatusCode($statusCode); - $e->setResponse($response); - $e->setRequest($request); + if ($exception instanceof HttpException) { + $statusCode = $response->getStatusCode(); + $httpMethod = $request->getMethod(); - if (isset(self::$descriptionMapping[$httpMethod][$statusCode])) { - $e->setDescription(self::$descriptionMapping[$httpMethod][$statusCode]); + $exception->setStatusCode($statusCode); + $exception->setResponse($response); + $exception->setRequest($request); + + if (isset(self::$descriptionMapping[$httpMethod][$statusCode])) { + $exception->setDescription(self::$descriptionMapping[$httpMethod][$statusCode]); + } } - return $e; + return $exception; } /** diff --git a/src/Grale/WebDav/Lock.php b/src/Grale/WebDav/Lock.php index 9cef61d..436b777 100644 --- a/src/Grale/WebDav/Lock.php +++ b/src/Grale/WebDav/Lock.php @@ -136,6 +136,8 @@ public function getDepth() * Accepted values are zero or {@link Header\DepthHeader::INFINITY} * * @param int $depth The depth of lock + * + * @throws \InvalidArgumentException */ public function setDepth($depth) { @@ -203,8 +205,11 @@ public function setTimeout($timeout) } /** - * @return self + * @param \DOMElement $element + * * @throws \InvalidArgumentException + * @return self + * * @todo define exception message */ public static function fromXml(\DOMElement $element) @@ -251,6 +256,7 @@ public static function fromXml(\DOMElement $element) * @param Client $client * @param string $xml * + * @throws \RuntimeException * @return self * * @todo diff --git a/src/Grale/WebDav/MultiStatus.php b/src/Grale/WebDav/MultiStatus.php index e382511..4fa33d9 100644 --- a/src/Grale/WebDav/MultiStatus.php +++ b/src/Grale/WebDav/MultiStatus.php @@ -68,7 +68,7 @@ public function getDescription() } /** - * @return Iterator Returns an iterator to be used in loop functions + * @return \Iterator Returns an iterator to be used in loop functions */ public function getIterator() { @@ -85,8 +85,9 @@ public function count() /** * @param Client $client - * @param string $xml The multi-status response as an XML string + * @param string $xml The multi-status response as an XML string * + * @throws \RuntimeException * @return self Returns the parsed multi-status response as an object * * @todo @@ -158,7 +159,9 @@ public static function parse(Client $client, $xml) /** * @param string $statusLine + * @throws \Exception * @return int + * @todo define exception here */ protected static function parseHttpStatus($statusLine) { diff --git a/src/Grale/WebDav/Property/SupportedLock.php b/src/Grale/WebDav/Property/SupportedLock.php index 0774b86..b4e81cb 100644 --- a/src/Grale/WebDav/Property/SupportedLock.php +++ b/src/Grale/WebDav/Property/SupportedLock.php @@ -55,8 +55,6 @@ public function getValue() */ public function isLockable($type = null, $scope = null) { - $result = false; - if ($type === null) { $result = count($this->capabilities) > 0; } elseif ($scope === null) { diff --git a/src/Grale/WebDav/PropertySet.php b/src/Grale/WebDav/PropertySet.php index b060699..7c2dcc2 100644 --- a/src/Grale/WebDav/PropertySet.php +++ b/src/Grale/WebDav/PropertySet.php @@ -104,7 +104,7 @@ public function getNames() /** * Returns an iterator over all properties in this set. * - * @return Iterator An iterator over {@link PropertyInterface} + * @return \Iterator An iterator over {@link PropertyInterface} */ public function getIterator() { diff --git a/src/Grale/WebDav/Resource.php b/src/Grale/WebDav/Resource.php index dda697b..6018ee9 100644 --- a/src/Grale/WebDav/Resource.php +++ b/src/Grale/WebDav/Resource.php @@ -35,6 +35,7 @@ class Resource implements LockableInterface /** * @param string $href * @param PropertySet|array + * @throws \InvalidArgumentException */ public function __construct($href, $properties = null) { diff --git a/src/Grale/WebDav/Response.php b/src/Grale/WebDav/Response.php index 7a6cd67..ce50047 100644 --- a/src/Grale/WebDav/Response.php +++ b/src/Grale/WebDav/Response.php @@ -48,7 +48,7 @@ class Response protected $status; /** - * @var array A list of resource properties, grouped by HTTP status code + * @var PropertySet[] A list of resource properties, grouped by HTTP status code */ protected $properties = array(); diff --git a/src/Grale/WebDav/StreamWrapper.php b/src/Grale/WebDav/StreamWrapper.php index 0c3f5fc..8757a1f 100644 --- a/src/Grale/WebDav/StreamWrapper.php +++ b/src/Grale/WebDav/StreamWrapper.php @@ -55,7 +55,7 @@ class StreamWrapper protected $stream; /** - * @var Iterator An iterator used to iterate the response elements of a WebDAV multi-status response + * @var \Iterator An iterator used to iterate the response elements of a WebDAV multi-status response */ protected $iterator; @@ -166,7 +166,7 @@ public static function getDefaultClient() */ public function stream_open($path, $mode, $options, &$openedPath) { - $result = false; + $url = null; try { // We don't care about text-mode translation and binary-mode flags @@ -334,8 +334,6 @@ public function stream_seek($offset, $whence = SEEK_SET) */ public function stream_lock($operation) { - $result = false; - try { // We don't care about LOCK_NB $operation = $operation & ~LOCK_NB; @@ -410,8 +408,6 @@ public function stream_close() */ public function rename($old, $new) { - $result = false; - try { // Retrieve the context options $this->setClientConfig(); @@ -441,8 +437,6 @@ public function rename($old, $new) */ public function unlink($path) { - $result = false; - try { // Retrieve the context options $this->setClientConfig(); @@ -477,8 +471,6 @@ public function unlink($path) */ public function mkdir($path, $mode, $options) { - $result = false; - try { if ($options & STREAM_MKDIR_RECURSIVE) { throw new \RuntimeException('WebDAV stream wrapper does not allow to create directories recursively'); @@ -516,8 +508,6 @@ public function mkdir($path, $mode, $options) */ public function rmdir($path, $options) { - $result = false; - try { // Retrieve the context options $this->setClientConfig(); @@ -591,7 +581,7 @@ public function dir_readdir() $result = $resource->getFilename(); // Cache the resource statistics for quick url_stat lookups - $url = (string)$this->openedPath->combine($resource->getHref()); + $url = (string)Url::factory($this->openedPath)->combine($resource->getHref()); self::$statCache[$url] = $resource->getStat(); $this->iterator->next(); @@ -764,8 +754,6 @@ protected function lock($scope) */ protected function releaseLock() { - $result = false; - try { $result = self::$client->releaseLock($this->openedPath, $this->locktoken); $this->locktoken = null; @@ -806,7 +794,7 @@ protected function openAppendMode($url) } catch (NoSuchResourceException $exception) { // The resource does not exist, so use a simple write stream - return $this->openWriteOnly($path); + return $this->openWriteOnly($url); } return true; @@ -869,10 +857,10 @@ protected function clearStatCache($path = null) * Trigger an error * * @param \Exception|string $error Error to trigger - * @param int $quiet If set to true, then no error or exception occurs + * @param bool|int $quiet If set to true, then no error or exception occurs * + * @throws Exception\StreamException * @return bool Returns false - * @throws StreamException if throw_exceptions is true */ protected function triggerError($error, $quiet = false) { diff --git a/tests/Grale/WebDav/ClientTest.php b/tests/Grale/WebDav/ClientTest.php index 85e35d1..0e03f9a 100644 --- a/tests/Grale/WebDav/ClientTest.php +++ b/tests/Grale/WebDav/ClientTest.php @@ -10,6 +10,7 @@ namespace Grale\WebDav; +use Guzzle\Http\Message\Request; use Guzzle\Http\Message\Response; use Guzzle\Http\Message\RequestFactory; use Guzzle\Http\Exception\BadResponseException; @@ -257,7 +258,8 @@ public function testMoveLockedCollection() $this->assertContains('Destination: http://www.foo.bar/othercontainer/', $request); $this->assertContains( 'If: ()' . - ' ()', $request + ' ()', + $request ); } @@ -468,7 +470,6 @@ public function testSimpleLockRequest() 'timeout' => 4100000000 )); - $status = $client->getLastResponseStatus(); $request = $client->getLastRequest(); $this->assertInstanceOf('Grale\\WebDav\\Lock', $lock); @@ -506,7 +507,7 @@ public function testRefreshingWriteLock() 'opaquelocktoken:e71d4fae-5dec-22d6-fea5-00a0c91e6be4', 4100000000 ); - $status = $client->getLastResponseStatus(); + $request = $client->getLastRequest(); $this->assertInstanceOf('Grale\\WebDav\\Lock', $result); @@ -592,6 +593,7 @@ public function testUnlock() * @param string $name * @param bool $asString * + * @throws \RuntimeException * @return Request|Response */ protected function getFixture($name, $asString = false) diff --git a/tests/Grale/WebDav/LockTest.php b/tests/Grale/WebDav/LockTest.php index db03bd8..b1bd6d8 100644 --- a/tests/Grale/WebDav/LockTest.php +++ b/tests/Grale/WebDav/LockTest.php @@ -18,6 +18,9 @@ */ class LockTest extends \PHPUnit_Framework_TestCase { + /** + * @var Lock + */ protected $lock; public function setUp() @@ -113,7 +116,7 @@ public function testInvalidLockScope() public function testFromInvalidXml() { $dom = new \DOMDocument(); - $dom->loadXML(''); + $dom->loadXML(''); Lock::fromXml($dom->documentElement); } diff --git a/tests/Grale/WebDav/Property/LockDiscoveryTest.php b/tests/Grale/WebDav/Property/LockDiscoveryTest.php index fd9ca99..3181130 100644 --- a/tests/Grale/WebDav/Property/LockDiscoveryTest.php +++ b/tests/Grale/WebDav/Property/LockDiscoveryTest.php @@ -17,6 +17,9 @@ */ class LockDiscoveryTest extends \PHPUnit_Framework_TestCase { + /** + * @var LockDiscovery + */ protected $property; public function setUp() diff --git a/tests/Grale/WebDav/PropertySetTest.php b/tests/Grale/WebDav/PropertySetTest.php index 7494e62..4fb1839 100644 --- a/tests/Grale/WebDav/PropertySetTest.php +++ b/tests/Grale/WebDav/PropertySetTest.php @@ -21,6 +21,9 @@ class PropertySetTest extends \PHPUnit_Framework_TestCase 'D:getcontenttype' ); + /** + * @var PropertySet + */ protected $props; public function setUp() @@ -58,7 +61,10 @@ public function testOffsetExists() public function testOffsetUnset() { unset($this->props['R:bigbox']); - $this->assertFalse(isset($this->props['R:bigbox']), 'Unable to assert that R:bigbox property has been deleted!'); + $this->assertFalse( + isset($this->props['R:bigbox']), + 'Unable to assert that R:bigbox property has been deleted!' + ); } /** Testing Countable **/ diff --git a/tests/Grale/WebDav/ResourceTest.php b/tests/Grale/WebDav/ResourceTest.php index dd5dac1..ca28ee4 100644 --- a/tests/Grale/WebDav/ResourceTest.php +++ b/tests/Grale/WebDav/ResourceTest.php @@ -20,8 +20,19 @@ */ class ResourceTest extends \PHPUnit_Framework_TestCase { + /** + * @var \Grale\WebDav\Resource + */ protected $collection; + + /** + * @var \Grale\WebDav\Resource + */ protected $resource; + + /** + * @var Lock + */ protected $lock; public function setUp() @@ -160,7 +171,10 @@ public function testHasLockToken() public function testGetLock() { - $this->assertEquals($this->lock, $this->resource->getLock('opaquelocktoken:e71d4fae-5dec-22d6-fea5-00a0c91e6be4')); + $this->assertEquals( + $this->lock, + $this->resource->getLock('opaquelocktoken:e71d4fae-5dec-22d6-fea5-00a0c91e6be4') + ); } public function testGetNoLocks() diff --git a/tests/Grale/WebDav/StreamWrapperTest.php b/tests/Grale/WebDav/StreamWrapperTest.php index 49f6d43..0e601e5 100644 --- a/tests/Grale/WebDav/StreamWrapperTest.php +++ b/tests/Grale/WebDav/StreamWrapperTest.php @@ -17,6 +17,9 @@ */ class StreamWrapperTest extends \PHPUnit_Framework_TestCase { + /** + * @var \PHPUnit_Framework_MockObject_MockObject + */ protected $client; public function setUp() @@ -29,7 +32,10 @@ public function setUp() $stream = EntityBody::fromString('Hello World!'); $wdavClient->expects($this->any())->method('getStream')->will($this->returnValue($stream)); - $propfind = MultiStatus::parse($wdavClient, file_get_contents(__DIR__ . '/../../fixtures/streamwrapper.opendir.xml')); + $propfind = MultiStatus::parse( + $wdavClient, + file_get_contents(__DIR__ . '/../../fixtures/streamwrapper.opendir.xml') + ); $wdavClient->expects($this->any())->method('setThrowExceptions')->will($this->returnValue($this->client)); $wdavClient->expects($this->any())->method('propfind')->will($this->returnValue($propfind)); @@ -82,12 +88,12 @@ public function testStreamOpenWithUnsupportedPlusMode($mode) /** * @dataProvider getUnsupportedModes - * @expectedException PHPUnit_Framework_Error_Warning + * @expectedException \PHPUnit_Framework_Error_Warning * @expectedExceptionMessage failed to open stream */ public function testStreamOpenWithUnsupportedModeTriggersError($mode) { - $result = fopen('webdav://test', $mode); + fopen('webdav://test', $mode); } public function getUnsupportedModes() @@ -205,7 +211,7 @@ public function testMakeDirectory() } /** - * @expectedException PHPUnit_Framework_Error_Warning + * @expectedException \PHPUnit_Framework_Error_Warning * @expectedExceptionMessage WebDAV stream wrapper does not allow to create directories recursively */ public function testMakeDirectoryRecursively() @@ -280,7 +286,7 @@ public function testRefreshLock() ) ->will($this->returnValue($lock)); - $result = flock($fd, LOCK_EX); + flock($fd, LOCK_EX); $result = flock($fd, LOCK_EX); $this->assertTrue($result); @@ -323,16 +329,4 @@ public function testIsDir() $result = is_dir('webdav://www.foo.bar/container'); $this->assertTrue($result); } - - public function testIsDirWithCache() - { - $dir = dir('webdav://www.foo.bar/container'); - - $files = array(); - while (($file = $dir->read()) !== false) { - $files[] = $file; - } - - $result = is_dir('webdav://www.foo.bar/container/othercontainer'); - } }