Issue #3320606: False Postive/Negative from preventCrossSchemeAccess() (80e92945) · Commits · project / s3fs

src/Traits/S3fsPathsTrait.php

+25 −5

Original line number	Diff line number	Diff line
		@@ -58,9 +58,11 @@ trait S3fsPathsTrait {
		$stream_wrapper_manager = \Drupal::service('stream_wrapper_manager');
		$uri = $stream_wrapper_manager->normalizeUri($uri);

		$public_folder = !empty($this->config['public_folder']) ? $this->config['public_folder'] : 's3fs-public';
		$config = \Drupal::config('s3fs.settings')->get();

		$public_folder = !empty($config['public_folder']) ? $config['public_folder'] : 's3fs-public';
		$public_folder = trim($public_folder, '/');
		$private_folder = !empty($this->config['private_folder']) ? $this->config['private_folder'] : 's3fs-private';
		$private_folder = !empty($config['private_folder']) ? $config['private_folder'] : 's3fs-private';
		$private_folder = trim($private_folder, '/');

		$scheme = StreamWrapperManager::getScheme($uri);
		@@ -68,7 +70,14 @@ trait S3fsPathsTrait {
		switch ($scheme) {
		case 's3':
		// Attempt to use s3:// to access public:// or private:// path.
		if (mb_strpos($uri, 's3://' . $public_folder) === 0 \|\| mb_strpos($uri, 's3://' . $private_folder) === 0) {
		if (
		// The 'folder' with a file path.
		mb_strpos($uri, 's3://' . $public_folder . '/') === 0
		\|\| mb_strpos($uri, 's3://' . $private_folder . '/') === 0
		// Just the root 'folder'.
		\|\| $uri === 's3://' . $public_folder
		\|\| $uri === 's3://' . $private_folder
		) {
		throw new CrossSchemeAccessException("Cross scheme access attempt blocked");
		}
		break;
		@@ -80,7 +89,13 @@ trait S3fsPathsTrait {
		$prefix = preg_replace($public_search, '', $private_folder);
		$prefix = trim($prefix, '/');

		if (mb_strpos($uri, 'public://' . $prefix . '/') === 0) {
		if (
		mb_strpos(
		// The 'folder' with a path.
		$uri, 'public://' . $prefix . '/') === 0
		// Just the root 'folder'.
		\|\| $uri === 'public://' . $prefix
		) {
		throw new CrossSchemeAccessException("Cross scheme access attempt blocked");
		}
		}
		@@ -92,7 +107,12 @@ trait S3fsPathsTrait {
		$private_search = '#^' . $private_folder . '#';
		$prefix = preg_replace($private_search, '', $public_folder);
		$prefix = trim($prefix, '/');
		if (mb_strpos($uri, 'private://' . $prefix . '/') === 0) {
		if (
		// The 'folder' with a path.
		mb_strpos($uri, 'private://' . $prefix . '/') === 0
		// Just the root 'folder'.
		\|\| $uri === 'private://' . $prefix
		) {
		throw new CrossSchemeAccessException("Cross scheme access attempt blocked");
		}
		}

tests/src/Functional/S3fsStreamIsolationTest.php

+54 −68

Original line number	Diff line number	Diff line
		@@ -2,6 +2,7 @@

		namespace Drupal\Tests\s3fs\Functional;

		use Drupal\Core\StreamWrapper\StreamWrapperManager;
		use Drupal\s3fs\Exceptions\CrossSchemeAccessException;
		use Drupal\s3fs\StreamWrapper\S3fsStream;

		@@ -24,13 +25,45 @@ class S3fsStreamIsolationTest extends S3fsTestBase {
		* Coverage test for the stream wrapper.
		*
		* @param string $path
		* Path to validate against.
		* Path to test against.
		* @param bool $expect_exception
		* Should this path throw exceptions.
		* Should this path trigger an exception.
		* @param array $new_config
		* S3fs config to override from defaults.
		*
		* @dataProvider schemaExceptionsDataProvider
		* @dataProvider schemaDirectoryExceptionsDataProvider
		*/
		public function testSchemaExceptionsForFiles(string $path, bool $expect_exception) {
		public function testSchemaExceptionsForFiles(string $path, bool $expect_exception, array $new_config = []) {

		// Enable public/private takeover.
		$settings['settings']['s3fs.use_s3_for_public'] = (object) [
		'value' => TRUE,
		'required' => TRUE,
		];
		$settings['settings']['s3fs.use_s3_for_private'] = (object) [
		'value' => TRUE,
		'required' => TRUE,
		];
		$this->writeSettings($settings);

		if (!empty($new_config)) {
		$config = $this->config('s3fs.settings');
		foreach ($new_config as $key => $item) {
		$config->set($key, $item);
		}
		$this->prepareConfig($config);
		drupal_static_reset();
		}

		$this->rebuildAll();

		// Prevent 'scheme://' from becomming 'scheme:/'.
		if (StreamWrapperManager::getTarget($path)) {
		$path = rtrim($path, '/');
		}

		$path = $path . '/test.txt';

		/** @var \Drupal\s3fs\S3fsFileService $file_system */
		$file_system = $this->container->get('file_system');

		@@ -71,8 +104,10 @@ class S3fsStreamIsolationTest extends S3fsTestBase {
		}

		try {
		file_put_contents('s3://rename_source.txt', 'source file for rename test');
		rename("s3://rename_source.txt", $path);
		$scheme = StreamWrapperManager::getScheme($path);
		$source_file = $scheme . '://rename_source.txt';
		file_put_contents($source_file, 'source file for rename test');
		rename($source_file, $path);
		$this->assertFalse($expect_exception, 'no exception on rename');
		}
		catch (CrossSchemeAccessException $e) {
		@@ -80,8 +115,10 @@ class S3fsStreamIsolationTest extends S3fsTestBase {
		}

		try {
		file_put_contents('s3://rename_source.txt', 'source file for move file_system test');
		$file_system->move("s3://rename_source.txt", $path);
		$scheme = StreamWrapperManager::getScheme($path);
		$source_file = $scheme . '://rename_source.txt';
		file_put_contents($source_file, 'source file for rename test');
		$file_system->move($source_file, $path);
		$this->assertFalse($expect_exception, 'no exception on file_system move');
		}
		catch (CrossSchemeAccessException $e) {
		@@ -107,42 +144,6 @@ class S3fsStreamIsolationTest extends S3fsTestBase {

		}

		/**
		* Data provider for testSchemaExceptionsForFiles.
		*
		* @return array[]
		* - path
		* - Should exception be thrown.
		*/
		public function schemaExceptionsDataProvider() {
		return [
		'file under s3fs-private' => [
		's3://s3fs-private/test.txt',
		TRUE,
		],
		'file under s3fs-public' => [
		's3://s3fs-public/test.txt',
		TRUE,
		],
		'file under s3fs-private with extra slashes' => [
		's3:////s3fs-private//test.txt',
		TRUE,
		],
		'file under s3fs-public with extra slashes' => [
		's3:///s3fs-public//test.txt',
		TRUE,
		],
		's3fs-private folder not under root' => [
		's3://subdir/s3fs-private/test.txt',
		FALSE,
		],
		's3fs-public folder not under root' => [
		's3://subdir/s3fs-public/test.txt',
		FALSE,
		],
		];
		}

		/**
		* Test directory functions for access exception.
		*
		@@ -197,6 +198,11 @@ class S3fsStreamIsolationTest extends S3fsTestBase {
		/**
		* Provide data for testDirectoriesAccess.
		*
		* This is intended to just be a functional validation that
		* S3fsPathsTrait::preventCrossSchemeAccess() is called to block access.
		*
		* The S3fsPathsTraitsTest does the more in depth scenarios.
		*
		* @return array[]
		* - Path
		* - Should throw exception
		@@ -220,35 +226,15 @@ class S3fsStreamIsolationTest extends S3fsTestBase {
		's3://subdir/s3fs-public/somdir',
		FALSE,
		],
		'Allow Directory carrying same name as private://' => [
		's3://subdir/s3fs-private/somdir',
		FALSE,
		],
		's3:// validate public:// has renamed' => [
		'validate s3://s3fs-public/ does not block when renamed' => [
		's3://s3fs-public/somdir',
		FALSE,
		['public_folder' => 'test1'],
		],
		's3:// validate private:// has renamed' => [
		's3://s3fs-private/somdir',
		FALSE,
		['private_folder' => 'test1'],
		],
		'Deny private:// under public://' => [
		'public://s3fs-private/text.txt',
		'validate s3://renamed-public/ does block when renamed' => [
		's3://renamed-public/somdir',
		TRUE,
		[
		'private_folder' => 'test1/s3fs-private',
		'public_folder' => 'test1',
		],
		],
		'Deny public:// under private://' => [
		'private://s3fs-public/test.txt',
		TRUE,
		[
		'private_folder' => 'test1',
		'public_folder' => 'test1/s3fs-public',
		],
		['public_folder' => 'renamed-public'],
		],
		];
		}

tests/src/Unit/S3fsPathsTraitsTest.php

0 → 100644

+227 −0

Original line number	Diff line number	Diff line
		<?php

		namespace Drupal\Tests\s3fs\Unit;

		use Drupal\Core\DependencyInjection\ContainerBuilder;
		use Drupal\Core\StreamWrapper\StreamWrapperManager;
		use Drupal\Core\StreamWrapper\StreamWrapperManagerInterface;
		use Drupal\s3fs\Exceptions\CrossSchemeAccessException;
		use Drupal\s3fs\Traits\S3fsPathsTrait;
		use Drupal\Tests\UnitTestCase;

		/**
		* S3 File System Scheme Isolation Tests.
		*
		* Ensure that the remote file system functionality provided by S3 File System
		* isolates the nested folders for schemes.
		*
		* @group s3fs
		*
		* @coversDefaultClass \Drupal\s3fs\Traits\S3fsPathsTrait
		*/
		class S3fsPathsTraitsTest extends UnitTestCase {

		use S3fsPathsTrait;

		/**
		* Coverage test for the preventCrossSchemeAccess().
		*
		* @param string $directory_path
		* Directory path to test against.
		* @param bool $expect_exception
		* Should this path trigger an exception.
		* @param array $new_config
		* S3fs config to override from defaults.
		*
		* @dataProvider schemaDirectoryExceptionsDataProvider
		*
		* @covers ::preventCrossSchemeAccess
		*/
		public function testCrossSchemeAccessException(string $directory_path, bool $expect_exception, array $new_config = []) {

		// Stream_wrapper_manager service mock that assumes scheme is always valid.
		$stream_wrapper_manager_mock = $this->createMock(StreamWrapperManagerInterface::class);
		$stream_wrapper_manager_mock->method('normalizeUri')->willReturnCallback(
		function ($uri) {
		$scheme = StreamWrapperManager::getScheme($uri);

		$target = StreamWrapperManager::getTarget($uri);

		if ($target !== FALSE) {
		$uri = $scheme . '://' . $target;
		}
		return $uri;
		}
		);

		// Mock s3fs.settings.
		$s3fs_config = [];
		if (!empty($new_config)) {
		foreach ($new_config as $key => $item) {
		$s3fs_config[$key] = $item;
		}
		}
		$config_factory_mock = $this->getConfigFactoryStub([
		's3fs.settings' => $s3fs_config,
		]);

		// Set mock services into global container.
		$container = new ContainerBuilder();
		\Drupal::setContainer($container);
		$container->set('config.factory', $config_factory_mock);
		$container->set('stream_wrapper_manager', $stream_wrapper_manager_mock);

		// Prevent 'scheme://test.txt' from becoming
		// 'scheme:/test.txt' or scheme:///test.txt.
		if (StreamWrapperManager::getTarget($directory_path)) {
		$directory_path = rtrim($directory_path, '/') . '/';

		}
		$file_path = $directory_path . 'test.txt';

		// Test the directory path.
		try {
		$this->preventCrossSchemeAccess($directory_path);
		$this->assertFalse($expect_exception, 'no exception from preventCrossSchemeAccess for directory');
		}
		catch (CrossSchemeAccessException $e) {
		$this->assertTrue($expect_exception, "exception thrown from preventCrossSchemeAccess for directory");
		}

		// Test the path with a file under directory.
		try {
		$this->preventCrossSchemeAccess($file_path);
		$this->assertFalse($expect_exception, 'no exception from preventCrossSchemeAccess for file under directory');
		}
		catch (CrossSchemeAccessException $e) {
		$this->assertTrue($expect_exception, "exception thrown from preventCrossSchemeAccess for file under directory");
		}

		}

		/**
		* Provide data for testDirectoriesAccess.
		*
		* @return array[]
		* - Path
		* - Should throw exception
		* - array config to override.
		*/
		public function schemaDirectoryExceptionsDataProvider() {
		return [
		'Allow s3:// root level access' => [
		's3://',
		FALSE,
		],
		'Allow public:// root level' => [
		'public://',
		FALSE,
		],
		'Allow private:// root level' => [
		'private://',
		FALSE,
		],
		'Deny public:// under s3://' => [
		's3://s3fs-public',
		TRUE,
		],
		'Deny private:// under s3://' => [
		's3://s3fs-private',
		TRUE,
		],
		'Deny public://something under s3://' => [
		's3://s3fs-public/something',
		TRUE,
		],
		'Deny private://something under s3://' => [
		's3://s3fs-private/something',
		TRUE,
		],
		'Allow s3:// sub-path contains public:// prefix' => [
		's3://sub-path/s3fs-public',
		FALSE,
		],
		'Allow s3:// sub-path contains private:// prefix' => [
		's3://sub-path/s3fs-private',
		FALSE,
		],
		'Allow s3:// directory carrying same prefix as public://' => [
		's3://s3fs-public-sub_directory',
		FALSE,
		],
		'Allow s3:// directory carrying same prefix as private://' => [
		's3://s3fs-private-sub_directory',
		FALSE,
		],
		's3://s3fs-public does not block when prefix renamed' => [
		's3://s3fs-public',
		FALSE,
		['public_folder' => 'test1'],
		],
		's3://s3fs-private does not block when prefix renamed' => [
		's3://s3fs-private',
		FALSE,
		['private_folder' => 'test1'],
		],
		's3://renamed-public does block with renamed prefix' => [
		's3://renamed-public',
		TRUE,
		['public_folder' => 'renamed-public'],
		],
		's3://renamed-private does block with renamed prefix' => [
		's3://renamed-private',
		TRUE,
		['private_folder' => 'renamed-private'],
		],
		'Deny private:// under public://' => [
		'public://s3fs-private',
		TRUE,
		[
		'private_folder' => 'test1/s3fs-private',
		'public_folder' => 'test1',
		],
		],
		'Deny private://something under public://' => [
		'public://s3fs-private/something',
		TRUE,
		[
		'private_folder' => 'test1/s3fs-private',
		'public_folder' => 'test1',
		],
		],
		'Deny public:// under private://' => [
		'private://s3fs-public',
		TRUE,
		[
		'private_folder' => 'test1',
		'public_folder' => 'test1/s3fs-public',
		],
		],
		'Deny public://something under private://' => [
		'private://s3fs-public/something',
		TRUE,
		[
		'private_folder' => 'test1',
		'public_folder' => 'test1/s3fs-public',
		],
		],
		'Allow public:// with path that partially matches nested private:// prefix' => [
		'private://s3fs-public-partial-match',
		FALSE,
		[
		'private_folder' => 'test1',
		'public_folder' => 'test1/s3fs-public',
		],
		],
		'Allow private:// with path that partially matches nested public:// prefix' => [
		'private://s3fs-private-partial-match',
		FALSE,
		[
		'private_folder' => 'test1/s3fs-private',
		'public_folder' => 'test1',
		],
		],
		];
		}

		}