From 80fbe16aef0f4536cd4decf202f2a4d037705aea Mon Sep 17 00:00:00 2001 From: ildyria Date: Fri, 23 May 2025 22:15:53 +0200 Subject: [PATCH 1/6] use relative url instead of absolutes --- app/Http/Controllers/SecurePathController.php | 11 +++++++---- app/Models/Extensions/HasUrlGenerator.php | 8 ++++++-- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/app/Http/Controllers/SecurePathController.php b/app/Http/Controllers/SecurePathController.php index 1fcb4c35ab8..66c9e89fac8 100644 --- a/app/Http/Controllers/SecurePathController.php +++ b/app/Http/Controllers/SecurePathController.php @@ -13,6 +13,7 @@ use App\Exceptions\SecurePaths\InvalidSignatureException; use App\Exceptions\SecurePaths\WrongPathException; use App\Models\Configs; +use App\Models\Extensions\HasUrlGenerator; use Illuminate\Contracts\Encryption\DecryptException; use Illuminate\Http\Request; use Illuminate\Routing\Controller; @@ -24,12 +25,10 @@ */ class SecurePathController extends Controller { + use HasUrlGenerator; + public function __invoke(Request $request, ?string $path) { - if (!$request->hasValidSignature()) { - throw new InvalidSignatureException(); - } - if (is_null($path)) { throw new WrongPathException(); } @@ -42,6 +41,10 @@ public function __invoke(Request $request, ?string $path) } } + if (!self::shouldNotUseSignedUrl() && !$request->hasValidSignature(false)) { + throw new InvalidSignatureException(); + } + $file = Storage::disk(StorageDiskType::LOCAL->value)->path($path); if (!file_exists($file)) { throw new WrongPathException(); diff --git a/app/Models/Extensions/HasUrlGenerator.php b/app/Models/Extensions/HasUrlGenerator.php index 6db02b2ea35..4410b686f3a 100644 --- a/app/Models/Extensions/HasUrlGenerator.php +++ b/app/Models/Extensions/HasUrlGenerator.php @@ -44,7 +44,7 @@ public static function pathToUrl(string $short_path, string $storage_disk, SizeV // @codeCoverageIgnoreEnd } - if (self::shouldNotUseSignedUrl()) { + if (self::shouldNotUseSignedUrl() && !Configs::getValueAsBool('secure_image_link_enabled')) { return $image_disk->url($short_path); } @@ -52,10 +52,14 @@ public static function pathToUrl(string $short_path, string $storage_disk, SizeV $short_path = Crypt::encryptString($short_path); } + if (self::shouldNotUseSignedUrl()) { + return Url::route('image', ['path' => $short_path]); + } + $temporary_image_link_life_in_seconds = Configs::getValueAsInt('temporary_image_link_life_in_seconds'); /** @disregard P1013 */ - return URL::temporarySignedRoute('image', now()->addSeconds($temporary_image_link_life_in_seconds), ['path' => $short_path]); + return URL::temporarySignedRoute('image', now()->addSeconds($temporary_image_link_life_in_seconds), ['path' => $short_path], false); } /** From e67d5cc41b30997867006c402a9f73f68a7f15e8 Mon Sep 17 00:00:00 2001 From: ildyria Date: Mon, 26 May 2025 21:51:21 +0200 Subject: [PATCH 2/6] add log lines --- app/Http/Controllers/SecurePathController.php | 5 ++++- app/Models/Extensions/HasUrlGenerator.php | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/app/Http/Controllers/SecurePathController.php b/app/Http/Controllers/SecurePathController.php index 66c9e89fac8..0ac76288e00 100644 --- a/app/Http/Controllers/SecurePathController.php +++ b/app/Http/Controllers/SecurePathController.php @@ -18,6 +18,7 @@ use Illuminate\Http\Request; use Illuminate\Routing\Controller; use Illuminate\Support\Facades\Crypt; +use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Storage; /** @@ -33,6 +34,8 @@ public function __invoke(Request $request, ?string $path) throw new WrongPathException(); } + Log::warning('url:', $request->url()); + if (Configs::getValueAsBool('secure_image_link_enabled')) { try { $path = Crypt::decryptString($path); @@ -41,7 +44,7 @@ public function __invoke(Request $request, ?string $path) } } - if (!self::shouldNotUseSignedUrl() && !$request->hasValidSignature(false)) { + if (!self::shouldNotUseSignedUrl() && !$request->hasValidSignature(true)) { throw new InvalidSignatureException(); } diff --git a/app/Models/Extensions/HasUrlGenerator.php b/app/Models/Extensions/HasUrlGenerator.php index 4410b686f3a..580833f2bb6 100644 --- a/app/Models/Extensions/HasUrlGenerator.php +++ b/app/Models/Extensions/HasUrlGenerator.php @@ -14,6 +14,7 @@ use Illuminate\Contracts\Encryption\EncryptException; use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\Crypt; +use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Storage; use Illuminate\Support\Facades\URL; use League\Flysystem\AwsS3V3\AwsS3V3Adapter; @@ -59,7 +60,9 @@ public static function pathToUrl(string $short_path, string $storage_disk, SizeV $temporary_image_link_life_in_seconds = Configs::getValueAsInt('temporary_image_link_life_in_seconds'); /** @disregard P1013 */ - return URL::temporarySignedRoute('image', now()->addSeconds($temporary_image_link_life_in_seconds), ['path' => $short_path], false); + $url = URL::temporarySignedRoute('image', now()->addSeconds($temporary_image_link_life_in_seconds), ['path' => $short_path], true); + Log::warning('url generated:', $url); + return $url; } /** From 0ccf2f001aed4f80eb05a090639d1ad6bd760f09 Mon Sep 17 00:00:00 2001 From: ildyria Date: Tue, 27 May 2025 00:03:34 +0200 Subject: [PATCH 3/6] fix signature on docker --- .../SecurePaths/SignatureExpiredException.php | 20 +++++ app/Http/Controllers/SecurePathController.php | 73 ++++++++++++++++++- app/Models/Extensions/HasUrlGenerator.php | 5 +- tests/Feature_v2/SecureImageLinksTest.php | 48 ++++++++---- 4 files changed, 126 insertions(+), 20 deletions(-) create mode 100644 app/Exceptions/SecurePaths/SignatureExpiredException.php diff --git a/app/Exceptions/SecurePaths/SignatureExpiredException.php b/app/Exceptions/SecurePaths/SignatureExpiredException.php new file mode 100644 index 00000000000..3409c51dc4d --- /dev/null +++ b/app/Exceptions/SecurePaths/SignatureExpiredException.php @@ -0,0 +1,20 @@ +url()); - if (Configs::getValueAsBool('secure_image_link_enabled')) { try { $path = Crypt::decryptString($path); @@ -44,10 +45,25 @@ public function __invoke(Request $request, ?string $path) } } - if (!self::shouldNotUseSignedUrl() && !$request->hasValidSignature(true)) { + // In theory we should use the `$request->hasCorrectSignature()` method here. + // However, for some stupid unknown reason, the path value is added to the server Query String. + // This completely invalitates the signature check. + // For example the url http://localhost:8000/image/small2x/c3/3d/c661c594a5a781cd44db06828783.png?expires=1748380289 + // will be verified as : + // http://localhost:8000/image/small2x/c3/3d/c661c594a5a781cd44db06828783.png?/image/small2x/c3/3d/c661c594a5a781cd44db06828783.png&expires=1748380289 + // which makes the signature check fail as the hmac does not match. + if (!self::shouldNotUseSignedUrl() && !$this->hasCorrectSignature($request)) { + Log::error('Invalid signature for secure path request. Verify that the url generated for the image match.', [ + 'candidate url' => $this->getUrl($request), + ]); throw new InvalidSignatureException(); } + // On the bright side, we can now differentiate between a missing/failed signature and an expired one. + if (!self::shouldNotUseSignedUrl() && !$this->signatureHasNotExpired($request)) { + throw new SignatureExpiredException(); + } + $file = Storage::disk(StorageDiskType::LOCAL->value)->path($path); if (!file_exists($file)) { throw new WrongPathException(); @@ -55,4 +71,55 @@ public function __invoke(Request $request, ?string $path) return response()->file($file); } + + private function getUrl(Request $request, bool $absolute = true): string + { + $ignore_query = ['signature']; + + $url = $absolute ? $request->url() : ('/' . $request->path()); + + $query_string = ''; + $query_string = (new Collection(explode('&', (string) $request->server->get('QUERY_STRING')))) + ->reject(fn ($parameter) => in_array(\Str::before($parameter, '='), $ignore_query, true)) + ->reject(fn ($parameter) => count(explode('=', $parameter)) === 1) // Ignore parameters without value => avoid problem above mentionned. + ->join('&'); + + return rtrim($url . '?' . $query_string, '?'); + } + + /** + * Determine if the signature from the given request matches the URL. + * + * @param \Illuminate\Http\Request $request + * @param bool $absolute + * + * @return bool + */ + private function hasCorrectSignature(Request $request, bool $absolute = true): bool + { + $original = $this->getUrl($request, $absolute); + $key = new \SensitiveParameterValue(config('app.key')); + if (hash_equals( + hash_hmac('sha256', $original, $key->getValue()), + $request->query('signature', '') + )) { + return true; + } + + return false; + } + + /** + * Determine if the expires timestamp from the given request is not from the past. + * + * @param \Illuminate\Http\Request $request + * + * @return bool + */ + private function signatureHasNotExpired(Request $request) + { + $expires = $request->query('expires'); + + return !($expires !== null && $expires !== '' && Carbon::now()->getTimestamp() > $expires); + } } \ No newline at end of file diff --git a/app/Models/Extensions/HasUrlGenerator.php b/app/Models/Extensions/HasUrlGenerator.php index 580833f2bb6..1a50bc7103d 100644 --- a/app/Models/Extensions/HasUrlGenerator.php +++ b/app/Models/Extensions/HasUrlGenerator.php @@ -14,7 +14,6 @@ use Illuminate\Contracts\Encryption\EncryptException; use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\Crypt; -use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Storage; use Illuminate\Support\Facades\URL; use League\Flysystem\AwsS3V3\AwsS3V3Adapter; @@ -60,9 +59,7 @@ public static function pathToUrl(string $short_path, string $storage_disk, SizeV $temporary_image_link_life_in_seconds = Configs::getValueAsInt('temporary_image_link_life_in_seconds'); /** @disregard P1013 */ - $url = URL::temporarySignedRoute('image', now()->addSeconds($temporary_image_link_life_in_seconds), ['path' => $short_path], true); - Log::warning('url generated:', $url); - return $url; + return URL::temporarySignedRoute('image', now()->addSeconds($temporary_image_link_life_in_seconds), ['path' => $short_path], true); } /** diff --git a/tests/Feature_v2/SecureImageLinksTest.php b/tests/Feature_v2/SecureImageLinksTest.php index f4fc25f5270..2029781956f 100644 --- a/tests/Feature_v2/SecureImageLinksTest.php +++ b/tests/Feature_v2/SecureImageLinksTest.php @@ -24,13 +24,23 @@ class SecureImageLinksTest extends BaseApiWithDataTest { - public function setUp(): void + private function setSecureLink() + { + Configs::set('secure_image_link_enabled', '1'); + Configs::invalidateCache(); + } + + private function setTemporaryLink() { - parent::setUp(); Configs::set('temporary_image_link_enabled', '1'); Configs::invalidateCache(); } + public function setUp(): void + { + parent::setUp(); + } + public function tearDown(): void { Configs::set('temporary_image_link_enabled', '0'); @@ -39,8 +49,9 @@ public function tearDown(): void parent::tearDown(); } - public function testTemporaryImage(): void + public function testSignedImage(): void { + $this->setTemporaryLink(); $response = $this->getJsonWithData('Album', ['album_id' => $this->album4->id]); $this->assertOk($response); $url = $response->json('resource.photos.0.size_variants.medium.url'); @@ -49,6 +60,24 @@ public function testTemporaryImage(): void $response = $this->get($url); $this->assertNotFound($response); $response->assertSeeText('File not found'); // We mocked the file ! + } + + public function testExpiredSignature(): void + { + $this->setTemporaryLink(); + $expired_url = URL::temporarySignedRoute('image', now()->subMinutes(10), ['path' => 'c3/3d/c661c594a5a781cd44db06828783.png']); + $response = $this->get($expired_url); + $this->assertForbidden($response); + $response->assertSeeText('Link expired'); + } + + public function testBrokenSignature(): void + { + $this->setTemporaryLink(); + $response = $this->getJsonWithData('Album', ['album_id' => $this->album4->id]); + $this->assertOk($response); + $url = $response->json('resource.photos.0.size_variants.medium.url'); + $this->assertStringContainsString('/image/medium/', $url); $unsigned_url = explode('?', $url)[0]; $response = $this->get($unsigned_url); @@ -57,8 +86,7 @@ public function testTemporaryImage(): void public function testEncryptedImages(): void { - Configs::set('secure_image_link_enabled', '1'); - Configs::invalidateCache(); + $this->setSecureLink(); $response = $this->getJsonWithData('Album', ['album_id' => $this->album4->id]); $this->assertOk($response); @@ -68,18 +96,12 @@ public function testEncryptedImages(): void $response = $this->get($url); $this->assertNotFound($response); $response->assertSeeText('File not found'); // We mocked the file ! - - $unsigned_url = explode('?', $url)[0]; - $response = $this->get($unsigned_url); - $this->assertForbidden($response); } public function testBrokenEncryption(): void { - Configs::set('secure_image_link_enabled', '1'); - Configs::invalidateCache(); - - $broken_url = URL::temporarySignedRoute('image', now()->addSeconds(10), ['path' => 'broken_path']); + $this->setSecureLink(); + $broken_url = URL::route('image', ['path' => 'broken_path']); $response = $this->get($broken_url); $this->assertForbidden($response); $response->assertSeeText('Invalid payload'); From 05c994bd11ce57c091612f181dbcfbb95c1d3cd6 Mon Sep 17 00:00:00 2001 From: ildyria Date: Tue, 27 May 2025 00:07:01 +0200 Subject: [PATCH 4/6] typo --- app/Http/Controllers/SecurePathController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Http/Controllers/SecurePathController.php b/app/Http/Controllers/SecurePathController.php index 90e348147f9..41eaa17d016 100644 --- a/app/Http/Controllers/SecurePathController.php +++ b/app/Http/Controllers/SecurePathController.php @@ -47,7 +47,7 @@ public function __invoke(Request $request, ?string $path) // In theory we should use the `$request->hasCorrectSignature()` method here. // However, for some stupid unknown reason, the path value is added to the server Query String. - // This completely invalitates the signature check. + // This completely invalidates the signature check. // For example the url http://localhost:8000/image/small2x/c3/3d/c661c594a5a781cd44db06828783.png?expires=1748380289 // will be verified as : // http://localhost:8000/image/small2x/c3/3d/c661c594a5a781cd44db06828783.png?/image/small2x/c3/3d/c661c594a5a781cd44db06828783.png&expires=1748380289 From 1bcdba16a54c10cdba657ab9509928f8398f7cd6 Mon Sep 17 00:00:00 2001 From: ildyria Date: Tue, 27 May 2025 00:11:37 +0200 Subject: [PATCH 5/6] typos and cosmetic --- app/Http/Controllers/SecurePathController.php | 24 +++++++++---------- app/Models/Extensions/HasUrlGenerator.php | 3 +++ tests/Feature_v2/SecureImageLinksTest.php | 5 ---- 3 files changed, 15 insertions(+), 17 deletions(-) diff --git a/app/Http/Controllers/SecurePathController.php b/app/Http/Controllers/SecurePathController.php index 41eaa17d016..bb994d21d0c 100644 --- a/app/Http/Controllers/SecurePathController.php +++ b/app/Http/Controllers/SecurePathController.php @@ -33,18 +33,6 @@ class SecurePathController extends Controller public function __invoke(Request $request, ?string $path) { - if (is_null($path)) { - throw new WrongPathException(); - } - - if (Configs::getValueAsBool('secure_image_link_enabled')) { - try { - $path = Crypt::decryptString($path); - } catch (DecryptException) { - throw new InvalidPayloadException(); - } - } - // In theory we should use the `$request->hasCorrectSignature()` method here. // However, for some stupid unknown reason, the path value is added to the server Query String. // This completely invalidates the signature check. @@ -64,6 +52,18 @@ public function __invoke(Request $request, ?string $path) throw new SignatureExpiredException(); } + if (is_null($path)) { + throw new WrongPathException(); + } + + if (Configs::getValueAsBool('secure_image_link_enabled')) { + try { + $path = Crypt::decryptString($path); + } catch (DecryptException) { + throw new InvalidPayloadException(); + } + } + $file = Storage::disk(StorageDiskType::LOCAL->value)->path($path); if (!file_exists($file)) { throw new WrongPathException(); diff --git a/app/Models/Extensions/HasUrlGenerator.php b/app/Models/Extensions/HasUrlGenerator.php index 1a50bc7103d..74c3fa970bd 100644 --- a/app/Models/Extensions/HasUrlGenerator.php +++ b/app/Models/Extensions/HasUrlGenerator.php @@ -44,14 +44,17 @@ public static function pathToUrl(string $short_path, string $storage_disk, SizeV // @codeCoverageIgnoreEnd } + // If we do not sign the URL or we do not use secure image links, we return the URL directly. if (self::shouldNotUseSignedUrl() && !Configs::getValueAsBool('secure_image_link_enabled')) { return $image_disk->url($short_path); } + // We we use the secure image link, we encrypt the path. if (Configs::getValueAsBool('secure_image_link_enabled')) { $short_path = Crypt::encryptString($short_path); } + // Return the url directly if we do not use signed URLs. if (self::shouldNotUseSignedUrl()) { return Url::route('image', ['path' => $short_path]); } diff --git a/tests/Feature_v2/SecureImageLinksTest.php b/tests/Feature_v2/SecureImageLinksTest.php index 2029781956f..54038e3c8af 100644 --- a/tests/Feature_v2/SecureImageLinksTest.php +++ b/tests/Feature_v2/SecureImageLinksTest.php @@ -36,11 +36,6 @@ private function setTemporaryLink() Configs::invalidateCache(); } - public function setUp(): void - { - parent::setUp(); - } - public function tearDown(): void { Configs::set('temporary_image_link_enabled', '0'); From 66f7b3de4f16d415dde8b21fda02f49a38912bf4 Mon Sep 17 00:00:00 2001 From: ildyria Date: Tue, 27 May 2025 00:17:15 +0200 Subject: [PATCH 6/6] and version 6.6.5 --- .../2025_05_26_221704_bump_version060605.php | 46 +++++++++++++++++++ version.md | 2 +- 2 files changed, 47 insertions(+), 1 deletion(-) create mode 100644 database/migrations/2025_05_26_221704_bump_version060605.php diff --git a/database/migrations/2025_05_26_221704_bump_version060605.php b/database/migrations/2025_05_26_221704_bump_version060605.php new file mode 100644 index 00000000000..c51dabab6a4 --- /dev/null +++ b/database/migrations/2025_05_26_221704_bump_version060605.php @@ -0,0 +1,46 @@ +output = new ConsoleOutput(); + $this->msg_section = $this->output->section(); + } + + /** + * Run the migrations. + * + * @return void + */ + public function up(): void + { + DB::table('configs')->where('key', 'version')->update(['value' => '060605']); + Artisan::call('cache:clear'); + $this->msg_section->writeln('Info: Cleared cache for version 6.6.5'); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down(): void + { + DB::table('configs')->where('key', 'version')->update(['value' => '060604']); + } +}; diff --git a/version.md b/version.md index a08b5f1e88f..653877f1f9b 100644 --- a/version.md +++ b/version.md @@ -1 +1 @@ -6.6.4 \ No newline at end of file +6.6.5 \ No newline at end of file