diff --git a/src/Controllers/TenantAssetsController.php b/src/Controllers/TenantAssetsController.php index 1e2014a7..356c25dd 100644 --- a/src/Controllers/TenantAssetsController.php +++ b/src/Controllers/TenantAssetsController.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Stancl\Tenancy\Controllers; +use Exception; use Illuminate\Routing\Controller; use Throwable; @@ -28,18 +29,41 @@ class TenantAssetsController extends Controller } /** + * Prevent path traversal attacks. This is generally a non-issue on modern + * webservers but it's still worth handling on the application level as well. + * * @throws \Symfony\Component\HttpKernel\Exception\HttpException */ protected function validatePath(string|null $path): void { - abort_if($path === null, 404); + $this->abortIf($path === null, 'Empty path'); - $allowedRoot = storage_path('app/public'); + $allowedRoot = realpath(storage_path('app/public')); - // Prevent path traversal attacks. This is generally a non-issue on modern - // webservers but it's still worth handling on the application level as well. - if (! str(realpath("{$allowedRoot}/{$path}"))->startsWith($allowedRoot)) { - abort(403); + // `storage_path('app/public')` doesn't exist, so it cannot contain files + $this->abortIf($allowedRoot === false, "Storage root doesn't exist"); + + $attemptedPath = realpath("{$allowedRoot}/{$path}"); + + // User is attempting to access a nonexistent file + $this->abortIf($attemptedPath === false, 'Accessing a nonexistent file'); + + // User is attempting to access a file outside the $allowedRoot folder + $this->abortIf(! str($attemptedPath)->startsWith($allowedRoot), 'Accessing a file outside the storage root'); + } + + protected function abortIf($condition, $exceptionMessage): void + { + if ($condition) { + if (app()->runningUnitTests()) { + // Makes testing the cause of the failure in validatePath() easier + throw new Exception($exceptionMessage); + } else { + // We always use 404 to avoid leaking information about the cause of the error + // e.g. when someone is trying to access a nonexistent file outside of the allowed + // root folder, we don't want to let the user know whether such a file exists or not. + abort(404); + } } } } diff --git a/tests/TenantAssetTest.php b/tests/TenantAssetTest.php index c24767cc..8aabc0e2 100644 --- a/tests/TenantAssetTest.php +++ b/tests/TenantAssetTest.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Stancl\Tenancy\Tests; +use Exception; use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Route; use Illuminate\Support\Facades\Storage; @@ -134,24 +135,79 @@ class TenantAssetTest extends TestCase $tenant = Tenant::create(); tenancy()->initialize($tenant); - $response = $this->get(tenant_asset(null), [ + + $this->withoutExceptionHandling(); + $this->expectExceptionMessage('Empty path'); // outside tests this is a 404 + + $this->get(tenant_asset(null), [ 'X-Tenant' => $tenant->id, ]); - - $response->assertNotFound(); } - public function test_asset_controller_returns_a_403_when_an_invalid_path_is_provided() + public function test_asset_controller_returns_a_404_when_the_storage_root_doesnt_exist() { TenantAssetsController::$tenancyMiddleware = InitializeTenancyByRequestData::class; $tenant = Tenant::create(); tenancy()->initialize($tenant); - $response = $this->get(tenant_asset('../foo.txt'), [ + + $storageRoot = storage_path("app/public"); + + if (is_dir($storageRoot)) { + rmdir(storage_path("app/public")); + } + + $this->withoutExceptionHandling(); + $this->expectExceptionMessage("Storage root doesn't exist"); // outside tests this is a 404 + + $this->get(tenant_asset('foo.txt'), [ 'X-Tenant' => $tenant->id, ]); + } - $response->assertForbidden(); + public function test_asset_controller_returns_a_404_when_accessing_a_nonexistent_file() + { + TenantAssetsController::$tenancyMiddleware = InitializeTenancyByRequestData::class; + + $tenant = Tenant::create(); + + tenancy()->initialize($tenant); + + $storageRoot = storage_path("app/public"); + + if (! is_dir($storageRoot)) { + mkdir(storage_path("app/public"), recursive: true); + } + + $this->withoutExceptionHandling(); + $this->expectExceptionMessage("Accessing a nonexistent file"); // outside tests this is a 404 + + $this->get(tenant_asset('foo.txt'), [ + 'X-Tenant' => $tenant->id, + ]); + } + + public function test_asset_controller_returns_a_404_when_accessing_a_file_outside_the_storage_root() + { + TenantAssetsController::$tenancyMiddleware = InitializeTenancyByRequestData::class; + + $tenant = Tenant::create(); + + tenancy()->initialize($tenant); + + $storageRoot = storage_path("app/public"); + + if (! is_dir($storageRoot)) { + mkdir(storage_path("app/public"), recursive: true); + file_put_contents(storage_path('app/foo.txt'), 'bar'); + } + + $this->withoutExceptionHandling(); + $this->expectExceptionMessage('Accessing a file outside the storage root'); // outside tests this is a 404 + + $this->get(tenant_asset('../foo.txt'), [ + 'X-Tenant' => $tenant->id, + ]); } }