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 @@ +hasValidSignature()) { + // 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. + // 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(); + } + if (is_null($path)) { throw new WrongPathException(); } @@ -49,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 6db02b2ea35..74c3fa970bd 100644 --- a/app/Models/Extensions/HasUrlGenerator.php +++ b/app/Models/Extensions/HasUrlGenerator.php @@ -44,18 +44,25 @@ public static function pathToUrl(string $short_path, string $storage_disk, SizeV // @codeCoverageIgnoreEnd } - if (self::shouldNotUseSignedUrl()) { + // 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]); + } + $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], true); } /** 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/tests/Feature_v2/SecureImageLinksTest.php b/tests/Feature_v2/SecureImageLinksTest.php index f4fc25f5270..54038e3c8af 100644 --- a/tests/Feature_v2/SecureImageLinksTest.php +++ b/tests/Feature_v2/SecureImageLinksTest.php @@ -24,9 +24,14 @@ 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(); } @@ -39,8 +44,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 +55,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 +81,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 +91,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'); 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