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.

diff --git a/includes/filerepo/backend/FSFileBackend.php b/includes/filerepo/backend/FSFileBackend.php
index 0c3ac74..a4ad3d4 100644 (file)
--- 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 152baeb..971bb76 100644 (file)
--- 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 d5823d8..f7aeddc 100644 (file)
--- 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 );