From e070d137458c26f65bd5c4f1bf1a03ea34fb7fe5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20=C5=A0tancl?= Date: Thu, 27 Jul 2023 05:15:28 +0200 Subject: [PATCH 1/5] update support link --- .github/ISSUE_TEMPLATE/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/ISSUE_TEMPLATE/config.yml b/.github/ISSUE_TEMPLATE/config.yml index 6870a17c..2aab2732 100644 --- a/.github/ISSUE_TEMPLATE/config.yml +++ b/.github/ISSUE_TEMPLATE/config.yml @@ -1,7 +1,7 @@ blank_issues_enabled: false contact_links: - name: Support Questions & Other - url: https://github.com/stancl/tenancy/blob/3.x/SUPPORT.md + url: https://archte.ch/discord about: 'If you have a question or need help using the package.' - name: Documentation Issue url: https://github.com/stancl/tenancy-docs/issues From 395192442d4000fe901888482c7d9b98ead9bff8 Mon Sep 17 00:00:00 2001 From: tamiroh Date: Fri, 18 Aug 2023 14:40:21 +0900 Subject: [PATCH 2/5] Add use (#1103) --- src/Middleware/InitializeTenancyByRequestData.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Middleware/InitializeTenancyByRequestData.php b/src/Middleware/InitializeTenancyByRequestData.php index de75d8c5..ef2132e8 100644 --- a/src/Middleware/InitializeTenancyByRequestData.php +++ b/src/Middleware/InitializeTenancyByRequestData.php @@ -6,6 +6,7 @@ namespace Stancl\Tenancy\Middleware; use Closure; use Illuminate\Http\Request; +use Stancl\Tenancy\Contracts\TenantResolver; use Stancl\Tenancy\Resolvers\RequestDataTenantResolver; use Stancl\Tenancy\Tenancy; From 4af70d302ffcf19306cc16009bd94162f3743ca7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20=C5=A0tancl?= Date: Thu, 24 Aug 2023 18:21:23 +0200 Subject: [PATCH 3/5] add extra $path validation to TenantAssetsController --- .gitignore | 1 + src/Controllers/TenantAssetsController.php | 18 +++++++++++++++++- tests/TenantAssetTest.php | 13 +++++++++++++ 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index f470ba75..c7cf933c 100644 --- a/.gitignore +++ b/.gitignore @@ -10,3 +10,4 @@ coverage/ clover.xml tests/Etc/tmp/queuetest.json docker-compose.override.yml +.DS_Store diff --git a/src/Controllers/TenantAssetsController.php b/src/Controllers/TenantAssetsController.php index 5549da0d..1e2014a7 100644 --- a/src/Controllers/TenantAssetsController.php +++ b/src/Controllers/TenantAssetsController.php @@ -18,7 +18,7 @@ class TenantAssetsController extends Controller public function asset($path = null) { - abort_if($path === null, 404); + $this->validatePath($path); try { return response()->file(storage_path("app/public/$path")); @@ -26,4 +26,20 @@ class TenantAssetsController extends Controller abort(404); } } + + /** + * @throws \Symfony\Component\HttpKernel\Exception\HttpException + */ + protected function validatePath(string|null $path): void + { + abort_if($path === null, 404); + + $allowedRoot = 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); + } + } } diff --git a/tests/TenantAssetTest.php b/tests/TenantAssetTest.php index 703ac65e..c24767cc 100644 --- a/tests/TenantAssetTest.php +++ b/tests/TenantAssetTest.php @@ -141,4 +141,17 @@ class TenantAssetTest extends TestCase $response->assertNotFound(); } + public function test_asset_controller_returns_a_403_when_an_invalid_path_is_provided() + { + TenantAssetsController::$tenancyMiddleware = InitializeTenancyByRequestData::class; + + $tenant = Tenant::create(); + + tenancy()->initialize($tenant); + $response = $this->get(tenant_asset('../foo.txt'), [ + 'X-Tenant' => $tenant->id, + ]); + + $response->assertForbidden(); + } } From caf2267a085dd8ff0c566b3540bbe94f7a89a4cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20=C5=A0tancl?= Date: Sat, 2 Sep 2023 03:19:37 +0200 Subject: [PATCH 4/5] reimplement TenantAssetsController::validatePath() (fixes #1143) --- src/Controllers/TenantAssetsController.php | 36 ++++++++++-- tests/TenantAssetTest.php | 68 ++++++++++++++++++++-- 2 files changed, 92 insertions(+), 12 deletions(-) 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, + ]); } } From 85c7465aca2fb017545a5304dea0ee0006bee5f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20=C5=A0tancl?= Date: Sat, 2 Sep 2023 03:20:42 +0200 Subject: [PATCH 5/5] remove unnecessary import --- tests/TenantAssetTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/TenantAssetTest.php b/tests/TenantAssetTest.php index 8aabc0e2..2852eae2 100644 --- a/tests/TenantAssetTest.php +++ b/tests/TenantAssetTest.php @@ -4,7 +4,6 @@ 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;