From 9a1452ae92f65ea334df4796438dbb6a9d714d66 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Sun, 29 Jan 2012 21:28:31 +0000 Subject: [PATCH] In FileBackendBase/FileBackend: * Moved some public static functions from FileBackend to FileBackendBase as the later defines the public API. * Made splitStoragePath() return null if the backend or container name is empty. * Made normalizeContainerPath() kill leading directory separators. * Added more unit tests and made some documentation tweaks. In FSFileBackend: * Added resolveContainerName() to disallow '.' a container name, since this would cause a traversal. --- includes/filerepo/backend/FSFileBackend.php | 10 + includes/filerepo/backend/FileBackend.php | 226 +++++++++--------- .../includes/filerepo/FileBackendTest.php | 114 +++++++++ 3 files changed, 241 insertions(+), 109 deletions(-) diff --git a/includes/filerepo/backend/FSFileBackend.php b/includes/filerepo/backend/FSFileBackend.php index 0c3ac7491c..a4ad3d4bbe 100644 --- a/includes/filerepo/backend/FSFileBackend.php +++ b/includes/filerepo/backend/FSFileBackend.php @@ -57,6 +57,16 @@ class FSFileBackend extends FileBackend { : 0644; } + /** + * @see FileBackend::resolveContainerName() + */ + protected function resolveContainerName( $container ) { + if ( $container !== '.' ) { + return $container; // container is not a traversal + } + return null; + } + /** * @see FileBackend::resolveContainerPath() */ diff --git a/includes/filerepo/backend/FileBackend.php b/includes/filerepo/backend/FileBackend.php index 152baebe7f..971bb76968 100644 --- a/includes/filerepo/backend/FileBackend.php +++ b/includes/filerepo/backend/FileBackend.php @@ -30,9 +30,9 @@ * @since 1.19 */ abstract class FileBackendBase { - protected $name; // unique backend name - protected $wikiId; // unique wiki name - protected $readOnly; // string + protected $name; // string; unique backend name + protected $wikiId; // string; unique wiki name + protected $readOnly; // string; read-only explanation message /** @var LockManager */ protected $lockManager; @@ -265,7 +265,10 @@ abstract class FileBackendBase { } /** - * Concatenate a list of storage files into a single file on the file system + * Concatenate a list of storage files into a single file system file. + * The target path should refer to a file that is already locked or + * otherwise safe from modification from other processes. Normally, + * the file will be a new temp file, which should be adequate. * $params include: * srcs : ordered source storage paths (e.g. chunk1, chunk2, ...) * dst : file system path to 0-byte temp file @@ -561,6 +564,116 @@ abstract class FileBackendBase { final public function getScopedFileLocks( array $paths, $type, Status $status ) { return ScopedLock::factory( $this->lockManager, $paths, $type, $status ); } + + /** + * Check if a given path is a "mwstore://" path. + * This does not do any further validation or any existence checks. + * + * @param $path string + * @return bool + */ + final public static function isStoragePath( $path ) { + return ( strpos( $path, 'mwstore://' ) === 0 ); + } + + /** + * Split a storage path into a backend name, a container name, + * and a relative file path. The relative path may be the empty string. + * This does not do any path normalization or traversal checks. + * + * @param $storagePath string + * @return Array (backend, container, rel object) or (null, null, null) + */ + final public static function splitStoragePath( $storagePath ) { + if ( self::isStoragePath( $storagePath ) ) { + // Remove the "mwstore://" prefix and split the path + $parts = explode( '/', substr( $storagePath, 10 ), 3 ); + if ( count( $parts ) >= 2 && $parts[0] != '' && $parts[1] != '' ) { + if ( count( $parts ) == 3 ) { + return $parts; // e.g. "backend/container/path" + } else { + return array( $parts[0], $parts[1], '' ); // e.g. "backend/container" + } + } + } + return array( null, null, null ); + } + + /** + * Normalize a storage path by cleaning up directory separators. + * Returns null if the path is not of the format of a valid storage path. + * + * @param $storagePath string + * @return string|null + */ + final public static function normalizeStoragePath( $storagePath ) { + list( $backend, $container, $relPath ) = self::splitStoragePath( $storagePath ); + if ( $relPath !== null ) { // must be for this backend + $relPath = self::normalizeContainerPath( $relPath ); + if ( $relPath !== null ) { + return ( $relPath != '' ) + ? "mwstore://{$backend}/{$container}/{$relPath}" + : "mwstore://{$backend}/{$container}"; + } + } + return null; + } + + /** + * Validate and normalize a relative storage path. + * Null is returned if the path involves directory traversal. + * Traversal is insecure for FS backends and broken for others. + * + * @param $path string Storage path relative to a container + * @return string|null + */ + final protected static function normalizeContainerPath( $path ) { + // Normalize directory separators + $path = strtr( $path, '\\', '/' ); + // Collapse any consecutive directory separators + $path = preg_replace( '![/]{2,}!', '/', $path ); + // Remove any leading directory separator + $path = ltrim( $path, '/' ); + // Use the same traversal protection as Title::secureAndSplit() + if ( strpos( $path, '.' ) !== false ) { + if ( + $path === '.' || + $path === '..' || + strpos( $path, './' ) === 0 || + strpos( $path, '../' ) === 0 || + strpos( $path, '/./' ) !== false || + strpos( $path, '/../' ) !== false + ) { + return null; + } + } + return $path; + } + + /** + * Get the parent storage directory of a storage path. + * This returns a path like "mwstore://backend/container", + * "mwstore://backend/container/...", or null if there is no parent. + * + * @param $storagePath string + * @return string|null + */ + final public static function parentStoragePath( $storagePath ) { + $storagePath = dirname( $storagePath ); + list( $b, $cont, $rel ) = self::splitStoragePath( $storagePath ); + return ( $rel === null ) ? null : $storagePath; + } + + /** + * Get the final extension from a storage or FS path + * + * @param $path string + * @return string + */ + final public static function extensionFromPath( $path ) { + $i = strrpos( $path, '.' ); + return strtolower( $i ? substr( $path, $i + 1 ) : '' ); + } } /** @@ -1299,71 +1412,6 @@ abstract class FileBackend extends FileBackendBase { } } - /** - * Get the parent storage directory of a storage path. - * This returns a path like "mwstore://backend/container", - * "mwstore://backend/container/...", or null if there is no parent. - * - * @param $storagePath string - * @return string|null - */ - final public static function parentStoragePath( $storagePath ) { - $storagePath = dirname( $storagePath ); - list( $b, $cont, $rel ) = self::splitStoragePath( $storagePath ); - return ( $rel === null ) ? null : $storagePath; - } - - /** - * Check if a given path is a mwstore:// path. - * This does not do any actual validation or existence checks. - * - * @param $path string - * @return bool - */ - final public static function isStoragePath( $path ) { - return ( strpos( $path, 'mwstore://' ) === 0 ); - } - - /** - * Normalize a storage path by cleaning up directory separators. - * Returns null if the path is not of the format of a valid storage path. - * - * @param $storagePath string - * @return string|null - */ - final public static function normalizeStoragePath( $storagePath ) { - list( $backend, $container, $relPath ) = self::splitStoragePath( $storagePath ); - if ( $relPath !== null ) { // must be for this backend - $relPath = self::normalizeContainerPath( $relPath ); - if ( $relPath !== null ) { - return ( $relPath != '' ) - ? "mwstore://{$backend}/{$container}/{$relPath}" - : "mwstore://{$backend}/{$container}"; - } - } - return null; - } - - /** - * Split a storage path into a backend name, a container name, - * and a relative file path. The relative path may be the empty string. - * - * @param $storagePath string - * @return Array (backend, container, rel object) or (null, null, null) - */ - final public static function splitStoragePath( $storagePath ) { - if ( self::isStoragePath( $storagePath ) ) { - // Note: strlen( 'mwstore://' ) = 10 - $parts = explode( '/', substr( $storagePath, 10 ), 3 ); - if ( count( $parts ) == 3 ) { - return $parts; // e.g. "backend/container/path" - } elseif ( count( $parts ) == 2 ) { - return array( $parts[0], $parts[1], '' ); // e.g. "backend/container" - } - } - return array( null, null, null ); - } - /** * Check if a container name is valid. * This checks for for length and illegal characters. @@ -1379,35 +1427,6 @@ abstract class FileBackend extends FileBackendBase { return preg_match( '/^[a-z0-9][a-z0-9-_]{0,199}$/i', $container ); } - /** - * Validate and normalize a relative storage path. - * Null is returned if the path involves directory traversal. - * Traversal is insecure for FS backends and broken for others. - * - * @param $path string Storage path relative to a container - * @return string|null - */ - final protected static function normalizeContainerPath( $path ) { - // Normalize directory separators - $path = strtr( $path, '\\', '/' ); - // Collapse consecutive directory separators - $path = preg_replace( '![/]{2,}!', '/', $path ); - // Use the same traversal protection as Title::secureAndSplit() - if ( strpos( $path, '.' ) !== false ) { - if ( - $path === '.' || - $path === '..' || - strpos( $path, './' ) === 0 || - strpos( $path, '../' ) === 0 || - strpos( $path, '/./' ) !== false || - strpos( $path, '/../' ) !== false - ) { - return null; - } - } - return $path; - } - /** * Splits a storage path into an internal container name, * an internal relative file name, and a container shard suffix. @@ -1565,17 +1584,6 @@ abstract class FileBackend extends FileBackendBase { protected function resolveContainerPath( $container, $relStoragePath ) { return $relStoragePath; } - - /** - * Get the final extension from a storage or FS path - * - * @param $path string - * @return string - */ - final public static function extensionFromPath( $path ) { - $i = strrpos( $path, '.' ); - return strtolower( $i ? substr( $path, $i + 1 ) : '' ); - } } /** diff --git a/tests/phpunit/includes/filerepo/FileBackendTest.php b/tests/phpunit/includes/filerepo/FileBackendTest.php index d5823d877b..f7aeddcc4a 100644 --- a/tests/phpunit/includes/filerepo/FileBackendTest.php +++ b/tests/phpunit/includes/filerepo/FileBackendTest.php @@ -2,6 +2,7 @@ /** * @group FileRepo + * @group FileBackend */ class FileBackendTest extends MediaWikiTestCase { private $backend, $multiBackend; @@ -73,6 +74,118 @@ class FileBackendTest extends MediaWikiTestCase { return get_class( $this->backend ); } + /** + * @dataProvider provider_testIsStoragePath + */ + public function testIsStoragePath( $path, $isStorePath ) { + $this->assertEquals( $isStorePath, FileBackend::isStoragePath( $path ), + "FileBackend::isStoragePath on path '$path'" ); + } + + function provider_testIsStoragePath() { + return array( + array( 'mwstore://', true ), + array( 'mwstore://backend', true ), + array( 'mwstore://backend/container', true ), + array( 'mwstore://backend/container/', true ), + array( 'mwstore://backend/container/path', true ), + array( 'mwstore://backend//container/', true ), + array( 'mwstore://backend//container//', true ), + array( 'mwstore://backend//container//path', true ), + array( 'mwstore:///', true ), + array( 'mwstore:/', false ), + array( 'mwstore:', false ), + ); + } + + /** + * @dataProvider provider_testSplitStoragePath + */ + public function testSplitStoragePath( $path, $res ) { + $this->assertEquals( $res, FileBackend::splitStoragePath( $path ), + "FileBackend::splitStoragePath on path '$path'" ); + } + + function provider_testSplitStoragePath() { + return array( + array( 'mwstore://backend/container', array( 'backend', 'container', '' ) ), + array( 'mwstore://backend/container/', array( 'backend', 'container', '' ) ), + array( 'mwstore://backend/container/path', array( 'backend', 'container', 'path' ) ), + array( 'mwstore://backend/container//path', array( 'backend', 'container', '/path' ) ), + array( 'mwstore://backend//container/path', array( null, null, null ) ), + array( 'mwstore://backend//container//path', array( null, null, null ) ), + array( 'mwstore://', array( null, null, null ) ), + array( 'mwstore://backend', array( null, null, null ) ), + array( 'mwstore:///', array( null, null, null ) ), + array( 'mwstore:/', array( null, null, null ) ), + array( 'mwstore:', array( null, null, null ) ) + ); + } + + /** + * @dataProvider provider_normalizeStoragePath + */ + public function testNormalizeStoragePath( $path, $res ) { + $this->assertEquals( $res, FileBackend::normalizeStoragePath( $path ), + "FileBackend::normalizeStoragePath on path '$path'" ); + } + + function provider_normalizeStoragePath() { + return array( + array( 'mwstore://backend/container', 'mwstore://backend/container' ), + array( 'mwstore://backend/container/', 'mwstore://backend/container' ), + array( 'mwstore://backend/container/path', 'mwstore://backend/container/path' ), + array( 'mwstore://backend/container//path', 'mwstore://backend/container/path' ), + array( 'mwstore://backend/container///path', 'mwstore://backend/container/path' ), + array( 'mwstore://backend/container///path//to///obj', 'mwstore://backend/container/path/to/obj', + array( 'mwstore://', null ), + array( 'mwstore://backend', null ), + array( 'mwstore://backend//container/path', null ), + array( 'mwstore://backend//container//path', null ), + array( 'mwstore:///', null ), + array( 'mwstore:/', null ), + array( 'mwstore:', null ), ) + ); + } + + /** + * @dataProvider provider_testParentStoragePath + */ + public function testParentStoragePath( $path, $res ) { + $this->assertEquals( $res, FileBackend::parentStoragePath( $path ), + "FileBackend::parentStoragePath on path '$path'" ); + } + + function provider_testParentStoragePath() { + return array( + array( 'mwstore://backend/container/path/to/obj', 'mwstore://backend/container/path/to' ), + array( 'mwstore://backend/container/path/to', 'mwstore://backend/container/path' ), + array( 'mwstore://backend/container/path', 'mwstore://backend/container' ), + array( 'mwstore://backend/container', null ), + array( 'mwstore://backend/container/path/to/obj/', 'mwstore://backend/container/path/to' ), + array( 'mwstore://backend/container/path/to/', 'mwstore://backend/container/path' ), + array( 'mwstore://backend/container/path/', 'mwstore://backend/container' ), + array( 'mwstore://backend/container/', null ), + ); + } + + /** + * @dataProvider provider_testExtensionFromPath + */ + public function testExtensionFromPath( $path, $res ) { + $this->assertEquals( $res, FileBackend::extensionFromPath( $path ), + "FileBackend::extensionFromPath on path '$path'" ); + } + + function provider_testExtensionFromPath() { + return array( + array( 'mwstore://backend/container/path.txt', 'txt' ), + array( 'mwstore://backend/container/path.svg.png', 'png' ), + array( 'mwstore://backend/container/path', '' ), + array( 'mwstore://backend/container/path.', '' ), + ); + } + /** * @dataProvider provider_testStore */ @@ -1060,6 +1173,7 @@ class FileBackendTest extends MediaWikiTestCase { foreach ( $iter as $iter ) {} // no errors } + // test helper wrapper for backend prepare() function private function prepare( array $params ) { $this->dirsToPrune[] = $params['dir']; return $this->backend->prepare( $params ); -- 2.20.1