r110259 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r110258‎ | r110259 | r110260 >
Date:21:28, 29 January 2012
Author:aaron
Status:ok (Comments)
Tags:filebackend 
Comment:
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.
Modified paths:
  • /trunk/phase3/includes/filerepo/backend/FSFileBackend.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/FileBackend.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/filerepo/FileBackendTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/tests/phpunit/includes/filerepo/FileBackendTest.php
——@@ -2,6 +2,7 @@
33
44 /**
55 * @group FileRepo
 6+ * @group FileBackend
67 */
78 class FileBackendTest extends MediaWikiTestCase {
89 private $backend, $multiBackend;
——@@ -74,6 +75,118 @@
7576 }
7677
7778 /**
 79+ * @dataProvider provider_testIsStoragePath
 80+ */
 81+ public function testIsStoragePath( $path, $isStorePath ) {
 82+ $this->assertEquals( $isStorePath, FileBackend::isStoragePath( $path ),
 83+ "FileBackend::isStoragePath on path '$path'" );
 84+ }
 85+
 86+ function provider_testIsStoragePath() {
 87+ return array(
 88+ array( 'mwstore://', true ),
 89+ array( 'mwstore://backend', true ),
 90+ array( 'mwstore://backend/container', true ),
 91+ array( 'mwstore://backend/container/', true ),
 92+ array( 'mwstore://backend/container/path', true ),
 93+ array( 'mwstore://backend//container/', true ),
 94+ array( 'mwstore://backend//container//', true ),
 95+ array( 'mwstore://backend//container//path', true ),
 96+ array( 'mwstore:///', true ),
 97+ array( 'mwstore:/', false ),
 98+ array( 'mwstore:', false ),
 99+ );
 100+ }
 101+
 102+ /**
 103+ * @dataProvider provider_testSplitStoragePath
 104+ */
 105+ public function testSplitStoragePath( $path, $res ) {
 106+ $this->assertEquals( $res, FileBackend::splitStoragePath( $path ),
 107+ "FileBackend::splitStoragePath on path '$path'" );
 108+ }
 109+
 110+ function provider_testSplitStoragePath() {
 111+ return array(
 112+ array( 'mwstore://backend/container', array( 'backend', 'container', '' ) ),
 113+ array( 'mwstore://backend/container/', array( 'backend', 'container', '' ) ),
 114+ array( 'mwstore://backend/container/path', array( 'backend', 'container', 'path' ) ),
 115+ array( 'mwstore://backend/container//path', array( 'backend', 'container', '/path' ) ),
 116+ array( 'mwstore://backend//container/path', array( null, null, null ) ),
 117+ array( 'mwstore://backend//container//path', array( null, null, null ) ),
 118+ array( 'mwstore://', array( null, null, null ) ),
 119+ array( 'mwstore://backend', array( null, null, null ) ),
 120+ array( 'mwstore:///', array( null, null, null ) ),
 121+ array( 'mwstore:/', array( null, null, null ) ),
 122+ array( 'mwstore:', array( null, null, null ) )
 123+ );
 124+ }
 125+
 126+ /**
 127+ * @dataProvider provider_normalizeStoragePath
 128+ */
 129+ public function testNormalizeStoragePath( $path, $res ) {
 130+ $this->assertEquals( $res, FileBackend::normalizeStoragePath( $path ),
 131+ "FileBackend::normalizeStoragePath on path '$path'" );
 132+ }
 133+
 134+ function provider_normalizeStoragePath() {
 135+ return array(
 136+ array( 'mwstore://backend/container', 'mwstore://backend/container' ),
 137+ array( 'mwstore://backend/container/', 'mwstore://backend/container' ),
 138+ array( 'mwstore://backend/container/path', 'mwstore://backend/container/path' ),
 139+ array( 'mwstore://backend/container//path', 'mwstore://backend/container/path' ),
 140+ array( 'mwstore://backend/container///path', 'mwstore://backend/container/path' ),
 141+ array( 'mwstore://backend/container///path//to///obj', 'mwstore://backend/container/path/to/obj',
 142+ array( 'mwstore://', null ),
 143+ array( 'mwstore://backend', null ),
 144+ array( 'mwstore://backend//container/path', null ),
 145+ array( 'mwstore://backend//container//path', null ),
 146+ array( 'mwstore:///', null ),
 147+ array( 'mwstore:/', null ),
 148+ array( 'mwstore:', null ), )
 149+ );
 150+ }
 151+
 152+ /**
 153+ * @dataProvider provider_testParentStoragePath
 154+ */
 155+ public function testParentStoragePath( $path, $res ) {
 156+ $this->assertEquals( $res, FileBackend::parentStoragePath( $path ),
 157+ "FileBackend::parentStoragePath on path '$path'" );
 158+ }
 159+
 160+ function provider_testParentStoragePath() {
 161+ return array(
 162+ array( 'mwstore://backend/container/path/to/obj', 'mwstore://backend/container/path/to' ),
 163+ array( 'mwstore://backend/container/path/to', 'mwstore://backend/container/path' ),
 164+ array( 'mwstore://backend/container/path', 'mwstore://backend/container' ),
 165+ array( 'mwstore://backend/container', null ),
 166+ array( 'mwstore://backend/container/path/to/obj/', 'mwstore://backend/container/path/to' ),
 167+ array( 'mwstore://backend/container/path/to/', 'mwstore://backend/container/path' ),
 168+ array( 'mwstore://backend/container/path/', 'mwstore://backend/container' ),
 169+ array( 'mwstore://backend/container/', null ),
 170+ );
 171+ }
 172+
 173+ /**
 174+ * @dataProvider provider_testExtensionFromPath
 175+ */
 176+ public function testExtensionFromPath( $path, $res ) {
 177+ $this->assertEquals( $res, FileBackend::extensionFromPath( $path ),
 178+ "FileBackend::extensionFromPath on path '$path'" );
 179+ }
 180+
 181+ function provider_testExtensionFromPath() {
 182+ return array(
 183+ array( 'mwstore://backend/container/path.txt', 'txt' ),
 184+ array( 'mwstore://backend/container/path.svg.png', 'png' ),
 185+ array( 'mwstore://backend/container/path', '' ),
 186+ array( 'mwstore://backend/container/path.', '' ),
 187+ );
 188+ }
 189+
 190+ /**
78191 * @dataProvider provider_testStore
79192 */
80193 public function testStore( $op ) {
——@@ -1060,6 +1173,7 @@
10611174 foreach ( $iter as $iter ) {} // no errors
10621175 }
10631176
 1177+ // test helper wrapper for backend prepare() function
10641178 private function prepare( array $params ) {
10651179 $this->dirsToPrune[] = $params['dir'];
10661180 return $this->backend->prepare( $params );
Index: trunk/phase3/includes/filerepo/backend/FSFileBackend.php
——@@ -58,6 +58,16 @@
5959 }
6060
6161 /**
 62+ * @see FileBackend::resolveContainerName()
 63+ */
 64+ protected function resolveContainerName( $container ) {
 65+ if ( $container !== '.' ) {
 66+ return $container; // container is not a traversal
 67+ }
 68+ return null;
 69+ }
 70+
 71+ /**
6272 * @see FileBackend::resolveContainerPath()
6373 */
6474 protected function resolveContainerPath( $container, $relStoragePath ) {
Index: trunk/phase3/includes/filerepo/backend/FileBackend.php
——@@ -30,9 +30,9 @@
3131 * @since 1.19
3232 */
3333 abstract class FileBackendBase {
34 - protected $name; // unique backend name
35 - protected $wikiId; // unique wiki name
36 - protected $readOnly; // string
 34+ protected $name; // string; unique backend name
 35+ protected $wikiId; // string; unique wiki name
 36+ protected $readOnly; // string; read-only explanation message
3737 /** @var LockManager */
3838 protected $lockManager;
3939
——@@ -265,7 +265,10 @@
266266 }
267267
268268 /**
269 - * Concatenate a list of storage files into a single file on the file system
 269+ * Concatenate a list of storage files into a single file system file.
 270+ * The target path should refer to a file that is already locked or
 271+ * otherwise safe from modification from other processes. Normally,
 272+ * the file will be a new temp file, which should be adequate.
270273 * $params include:
271274 * srcs : ordered source storage paths (e.g. chunk1, chunk2, ...)
272275 * dst : file system path to 0-byte temp file
——@@ -561,6 +564,116 @@
562565 final public function getScopedFileLocks( array $paths, $type, Status $status ) {
563566 return ScopedLock::factory( $this->lockManager, $paths, $type, $status );
564567 }
 568+
 569+ /**
 570+ * Check if a given path is a "mwstore://" path.
 571+ * This does not do any further validation or any existence checks.
 572+ *
 573+ * @param $path string
 574+ * @return bool
 575+ */
 576+ final public static function isStoragePath( $path ) {
 577+ return ( strpos( $path, 'mwstore://' ) === 0 );
 578+ }
 579+
 580+ /**
 581+ * Split a storage path into a backend name, a container name,
 582+ * and a relative file path. The relative path may be the empty string.
 583+ * This does not do any path normalization or traversal checks.
 584+ *
 585+ * @param $storagePath string
 586+ * @return Array (backend, container, rel object) or (null, null, null)
 587+ */
 588+ final public static function splitStoragePath( $storagePath ) {
 589+ if ( self::isStoragePath( $storagePath ) ) {
 590+ // Remove the "mwstore://" prefix and split the path
 591+ $parts = explode( '/', substr( $storagePath, 10 ), 3 );
 592+ if ( count( $parts ) >= 2 && $parts[0] != '' && $parts[1] != '' ) {
 593+ if ( count( $parts ) == 3 ) {
 594+ return $parts; // e.g. "backend/container/path"
 595+ } else {
 596+ return array( $parts[0], $parts[1], '' ); // e.g. "backend/container"
 597+ }
 598+ }
 599+ }
 600+ return array( null, null, null );
 601+ }
 602+
 603+ /**
 604+ * Normalize a storage path by cleaning up directory separators.
 605+ * Returns null if the path is not of the format of a valid storage path.
 606+ *
 607+ * @param $storagePath string
 608+ * @return string|null
 609+ */
 610+ final public static function normalizeStoragePath( $storagePath ) {
 611+ list( $backend, $container, $relPath ) = self::splitStoragePath( $storagePath );
 612+ if ( $relPath !== null ) { // must be for this backend
 613+ $relPath = self::normalizeContainerPath( $relPath );
 614+ if ( $relPath !== null ) {
 615+ return ( $relPath != '' )
 616+ ? "mwstore://{$backend}/{$container}/{$relPath}"
 617+ : "mwstore://{$backend}/{$container}";
 618+ }
 619+ }
 620+ return null;
 621+ }
 622+
 623+ /**
 624+ * Validate and normalize a relative storage path.
 625+ * Null is returned if the path involves directory traversal.
 626+ * Traversal is insecure for FS backends and broken for others.
 627+ *
 628+ * @param $path string Storage path relative to a container
 629+ * @return string|null
 630+ */
 631+ final protected static function normalizeContainerPath( $path ) {
 632+ // Normalize directory separators
 633+ $path = strtr( $path, '\\', '/' );
 634+ // Collapse any consecutive directory separators
 635+ $path = preg_replace( '![/]{2,}!', '/', $path );
 636+ // Remove any leading directory separator
 637+ $path = ltrim( $path, '/' );
 638+ // Use the same traversal protection as Title::secureAndSplit()
 639+ if ( strpos( $path, '.' ) !== false ) {
 640+ if (
 641+ $path === '.' ||
 642+ $path === '..' ||
 643+ strpos( $path, './' ) === 0 ||
 644+ strpos( $path, '../' ) === 0 ||
 645+ strpos( $path, '/./' ) !== false ||
 646+ strpos( $path, '/../' ) !== false
 647+ ) {
 648+ return null;
 649+ }
 650+ }
 651+ return $path;
 652+ }
 653+
 654+ /**
 655+ * Get the parent storage directory of a storage path.
 656+ * This returns a path like "mwstore://backend/container",
 657+ * "mwstore://backend/container/...", or null if there is no parent.
 658+ *
 659+ * @param $storagePath string
 660+ * @return string|null
 661+ */
 662+ final public static function parentStoragePath( $storagePath ) {
 663+ $storagePath = dirname( $storagePath );
 664+ list( $b, $cont, $rel ) = self::splitStoragePath( $storagePath );
 665+ return ( $rel === null ) ? null : $storagePath;
 666+ }
 667+
 668+ /**
 669+ * Get the final extension from a storage or FS path
 670+ *
 671+ * @param $path string
 672+ * @return string
 673+ */
 674+ final public static function extensionFromPath( $path ) {
 675+ $i = strrpos( $path, '.' );
 676+ return strtolower( $i ? substr( $path, $i + 1 ) : '' );
 677+ }
565678 }
566679
567680 /**
——@@ -1300,71 +1413,6 @@
13011414 }
13021415
13031416 /**
1304 - * Get the parent storage directory of a storage path.
1305 - * This returns a path like "mwstore://backend/container",
1306 - * "mwstore://backend/container/...", or null if there is no parent.
1307 - *
1308 - * @param $storagePath string
1309 - * @return string|null
1310 - */
1311 - final public static function parentStoragePath( $storagePath ) {
1312 - $storagePath = dirname( $storagePath );
1313 - list( $b, $cont, $rel ) = self::splitStoragePath( $storagePath );
1314 - return ( $rel === null ) ? null : $storagePath;
1315 - }
1316 -
1317 - /**
1318 - * Check if a given path is a mwstore:// path.
1319 - * This does not do any actual validation or existence checks.
1320 - *
1321 - * @param $path string
1322 - * @return bool
1323 - */
1324 - final public static function isStoragePath( $path ) {
1325 - return ( strpos( $path, 'mwstore://' ) === 0 );
1326 - }
1327 -
1328 - /**
1329 - * Normalize a storage path by cleaning up directory separators.
1330 - * Returns null if the path is not of the format of a valid storage path.
1331 - *
1332 - * @param $storagePath string
1333 - * @return string|null
1334 - */
1335 - final public static function normalizeStoragePath( $storagePath ) {
1336 - list( $backend, $container, $relPath ) = self::splitStoragePath( $storagePath );
1337 - if ( $relPath !== null ) { // must be for this backend
1338 - $relPath = self::normalizeContainerPath( $relPath );
1339 - if ( $relPath !== null ) {
1340 - return ( $relPath != '' )
1341 - ? "mwstore://{$backend}/{$container}/{$relPath}"
1342 - : "mwstore://{$backend}/{$container}";
1343 - }
1344 - }
1345 - return null;
1346 - }
1347 -
1348 - /**
1349 - * Split a storage path into a backend name, a container name,
1350 - * and a relative file path. The relative path may be the empty string.
1351 - *
1352 - * @param $storagePath string
1353 - * @return Array (backend, container, rel object) or (null, null, null)
1354 - */
1355 - final public static function splitStoragePath( $storagePath ) {
1356 - if ( self::isStoragePath( $storagePath ) ) {
1357 - // Note: strlen( 'mwstore://' ) = 10
1358 - $parts = explode( '/', substr( $storagePath, 10 ), 3 );
1359 - if ( count( $parts ) == 3 ) {
1360 - return $parts; // e.g. "backend/container/path"
1361 - } elseif ( count( $parts ) == 2 ) {
1362 - return array( $parts[0], $parts[1], '' ); // e.g. "backend/container"
1363 - }
1364 - }
1365 - return array( null, null, null );
1366 - }
1367 -
1368 - /**
13691417 * Check if a container name is valid.
13701418 * This checks for for length and illegal characters.
13711419 *
——@@ -1380,35 +1428,6 @@
13811429 }
13821430
13831431 /**
1384 - * Validate and normalize a relative storage path.
1385 - * Null is returned if the path involves directory traversal.
1386 - * Traversal is insecure for FS backends and broken for others.
1387 - *
1388 - * @param $path string Storage path relative to a container
1389 - * @return string|null
1390 - */
1391 - final protected static function normalizeContainerPath( $path ) {
1392 - // Normalize directory separators
1393 - $path = strtr( $path, '\\', '/' );
1394 - // Collapse consecutive directory separators
1395 - $path = preg_replace( '![/]{2,}!', '/', $path );
1396 - // Use the same traversal protection as Title::secureAndSplit()
1397 - if ( strpos( $path, '.' ) !== false ) {
1398 - if (
1399 - $path === '.' ||
1400 - $path === '..' ||
1401 - strpos( $path, './' ) === 0 ||
1402 - strpos( $path, '../' ) === 0 ||
1403 - strpos( $path, '/./' ) !== false ||
1404 - strpos( $path, '/../' ) !== false
1405 - ) {
1406 - return null;
1407 - }
1408 - }
1409 - return $path;
1410 - }
1411 -
1412 - /**
14131432 * Splits a storage path into an internal container name,
14141433 * an internal relative file name, and a container shard suffix.
14151434 * Any shard suffix is already appended to the internal container name.
——@@ -1565,17 +1584,6 @@
15661585 protected function resolveContainerPath( $container, $relStoragePath ) {
15671586 return $relStoragePath;
15681587 }
1569 -
1570 - /**
1571 - * Get the final extension from a storage or FS path
1572 - *
1573 - * @param $path string
1574 - * @return string
1575 - */
1576 - final public static function extensionFromPath( $path ) {
1577 - $i = strrpos( $path, '.' );
1578 - return strtolower( $i ? substr( $path, $i + 1 ) : '' );
1579 - }
15801588 }
15811589
15821590 /**

Follow-up revisions

RevisionCommit summaryAuthorDate
r110269* r108353: Made FileBackendMultiWrite consistency checks configurable....aaron08:00, 30 January 2012

Comments

#Comment by Aaron Schulz (talk | contribs)   06:59, 30 January 2012

Technically the FSFileBackend change is redundant.

Status & tagging log