Skip to content

Commit de63f58

Browse files
ildyriaildyria
authored
Fix temporary URL on docker setup (#3373)
* and version 6.6.5
1 parent e143237 commit de63f58

File tree

6 files changed

+180
-17
lines changed

6 files changed

+180
-17
lines changed

‎app/Exceptions/SecurePaths/SignatureExpiredException.php‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
<?php
2+
3+
/**
4+
* SPDX-License-Identifier: MIT
5+
* Copyright (c) 2017-2018 Tobias Reich
6+
* Copyright (c) 2018-2025 LycheeOrg.
7+
*/
8+
9+
namespace App\Exceptions\SecurePaths;
10+
11+
use App\Exceptions\BaseLycheeException;
12+
use Symfony\Component\HttpFoundation\Response;
13+
14+
class SignatureExpiredException extends BaseLycheeException
15+
{
16+
public function __construct(?\Throwable $previous = null)
17+
{
18+
parent::__construct(Response::HTTP_FORBIDDEN, 'Link expired', $previous);
19+
}
20+
}

‎app/Http/Controllers/SecurePathController.php‎

Lines changed: 74 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,25 +11,47 @@
1111
use App\Enum\StorageDiskType;
1212
use App\Exceptions\SecurePaths\InvalidPayloadException;
1313
use App\Exceptions\SecurePaths\InvalidSignatureException;
14+
use App\Exceptions\SecurePaths\SignatureExpiredException;
1415
use App\Exceptions\SecurePaths\WrongPathException;
1516
use App\Models\Configs;
17+
use App\Models\Extensions\HasUrlGenerator;
1618
use Illuminate\Contracts\Encryption\DecryptException;
1719
use Illuminate\Http\Request;
1820
use Illuminate\Routing\Controller;
21+
use Illuminate\Support\Carbon;
22+
use Illuminate\Support\Collection;
1923
use Illuminate\Support\Facades\Crypt;
24+
use Illuminate\Support\Facades\Log;
2025
use Illuminate\Support\Facades\Storage;
2126

2227
/**
2328
* Controller responsible for serving files securely.
2429
*/
2530
class SecurePathController extends Controller
2631
{
32+
use HasUrlGenerator;
33+
2734
public function __invoke(Request $request, ?string $path)
2835
{
29-
if (!$request->hasValidSignature()) {
36+
// In theory we should use the `$request->hasCorrectSignature()` method here.
37+
// However, for some stupid unknown reason, the path value is added to the server Query String.
38+
// This completely invalidates the signature check.
39+
// For example the url http://localhost:8000/image/small2x/c3/3d/c661c594a5a781cd44db06828783.png?expires=1748380289
40+
// will be verified as :
41+
// http://localhost:8000/image/small2x/c3/3d/c661c594a5a781cd44db06828783.png?/image/small2x/c3/3d/c661c594a5a781cd44db06828783.png&expires=1748380289
42+
// which makes the signature check fail as the hmac does not match.
43+
if (!self::shouldNotUseSignedUrl() && !$this->hasCorrectSignature($request)) {
44+
Log::error('Invalid signature for secure path request. Verify that the url generated for the image match.', [
45+
'candidate url' => $this->getUrl($request),
46+
]);
3047
throw new InvalidSignatureException();
3148
}
3249

50+
// On the bright side, we can now differentiate between a missing/failed signature and an expired one.
51+
if (!self::shouldNotUseSignedUrl() && !$this->signatureHasNotExpired($request)) {
52+
throw new SignatureExpiredException();
53+
}
54+
3355
if (is_null($path)) {
3456
throw new WrongPathException();
3557
}
@@ -49,4 +71,55 @@ public function __invoke(Request $request, ?string $path)
4971

5072
return response()->file($file);
5173
}
74+
75+
private function getUrl(Request $request, bool $absolute = true): string
76+
{
77+
$ignore_query = ['signature'];
78+
79+
$url = $absolute ? $request->url() : ('/' . $request->path());
80+
81+
$query_string = '';
82+
$query_string = (new Collection(explode('&', (string) $request->server->get('QUERY_STRING'))))
83+
->reject(fn ($parameter) => in_array(\Str::before($parameter, '='), $ignore_query, true))
84+
->reject(fn ($parameter) => count(explode('=', $parameter)) === 1) // Ignore parameters without value => avoid problem above mentionned.
85+
->join('&');
86+
87+
return rtrim($url . '?' . $query_string, '?');
88+
}
89+
90+
/**
91+
* Determine if the signature from the given request matches the URL.
92+
*
93+
* @param \Illuminate\Http\Request $request
94+
* @param bool $absolute
95+
*
96+
* @return bool
97+
*/
98+
private function hasCorrectSignature(Request $request, bool $absolute = true): bool
99+
{
100+
$original = $this->getUrl($request, $absolute);
101+
$key = new \SensitiveParameterValue(config('app.key'));
102+
if (hash_equals(
103+
hash_hmac('sha256', $original, $key->getValue()),
104+
$request->query('signature', '')
105+
)) {
106+
return true;
107+
}
108+
109+
return false;
110+
}
111+
112+
/**
113+
* Determine if the expires timestamp from the given request is not from the past.
114+
*
115+
* @param \Illuminate\Http\Request $request
116+
*
117+
* @return bool
118+
*/
119+
private function signatureHasNotExpired(Request $request)
120+
{
121+
$expires = $request->query('expires');
122+
123+
return !($expires !== null && $expires !== '' && Carbon::now()->getTimestamp() > $expires);
124+
}
52125
}

‎app/Models/Extensions/HasUrlGenerator.php‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,18 +44,25 @@ public static function pathToUrl(string $short_path, string $storage_disk, SizeV
4444
// @codeCoverageIgnoreEnd
4545
}
4646

47-
if (self::shouldNotUseSignedUrl()) {
47+
// If we do not sign the URL or we do not use secure image links, we return the URL directly.
48+
if (self::shouldNotUseSignedUrl() && !Configs::getValueAsBool('secure_image_link_enabled')) {
4849
return $image_disk->url($short_path);
4950
}
5051

52+
// We we use the secure image link, we encrypt the path.
5153
if (Configs::getValueAsBool('secure_image_link_enabled')) {
5254
$short_path = Crypt::encryptString($short_path);
5355
}
5456

57+
// Return the url directly if we do not use signed URLs.
58+
if (self::shouldNotUseSignedUrl()) {
59+
return Url::route('image', ['path' => $short_path]);
60+
}
61+
5562
$temporary_image_link_life_in_seconds = Configs::getValueAsInt('temporary_image_link_life_in_seconds');
5663

5764
/** @disregard P1013 */
58-
return URL::temporarySignedRoute('image', now()->addSeconds($temporary_image_link_life_in_seconds), ['path' => $short_path]);
65+
return URL::temporarySignedRoute('image', now()->addSeconds($temporary_image_link_life_in_seconds), ['path' => $short_path], true);
5966
}
6067

6168
/**

‎database/migrations/2025_05_26_221704_bump_version060605.php‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
<?php
2+
3+
/**
4+
* SPDX-License-Identifier: MIT
5+
* Copyright (c) 2017-2018 Tobias Reich
6+
* Copyright (c) 2018-2025 LycheeOrg.
7+
*/
8+
9+
use Illuminate\Database\Migrations\Migration;
10+
use Illuminate\Support\Facades\Artisan;
11+
use Illuminate\Support\Facades\DB;
12+
use Symfony\Component\Console\Output\ConsoleOutput;
13+
use Symfony\Component\Console\Output\ConsoleSectionOutput;
14+
15+
return new class() extends Migration {
16+
private ConsoleOutput $output;
17+
private ConsoleSectionOutput $msg_section;
18+
19+
public function __construct()
20+
{
21+
$this->output = new ConsoleOutput();
22+
$this->msg_section = $this->output->section();
23+
}
24+
25+
/**
26+
* Run the migrations.
27+
*
28+
* @return void
29+
*/
30+
public function up(): void
31+
{
32+
DB::table('configs')->where('key', 'version')->update(['value' => '060605']);
33+
Artisan::call('cache:clear');
34+
$this->msg_section->writeln('<info>Info:</info> Cleared cache for version 6.6.5');
35+
}
36+
37+
/**
38+
* Reverse the migrations.
39+
*
40+
* @return void
41+
*/
42+
public function down(): void
43+
{
44+
DB::table('configs')->where('key', 'version')->update(['value' => '060604']);
45+
}
46+
};

‎tests/Feature_v2/SecureImageLinksTest.php‎

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,14 @@
2424

2525
class SecureImageLinksTest extends BaseApiWithDataTest
2626
{
27-
public function setUp(): void
27+
private function setSecureLink()
28+
{
29+
Configs::set('secure_image_link_enabled', '1');
30+
Configs::invalidateCache();
31+
}
32+
33+
private function setTemporaryLink()
2834
{
29-
parent::setUp();
3035
Configs::set('temporary_image_link_enabled', '1');
3136
Configs::invalidateCache();
3237
}
@@ -39,8 +44,9 @@ public function tearDown(): void
3944
parent::tearDown();
4045
}
4146

42-
public function testTemporaryImage(): void
47+
public function testSignedImage(): void
4348
{
49+
$this->setTemporaryLink();
4450
$response = $this->getJsonWithData('Album', ['album_id' => $this->album4->id]);
4551
$this->assertOk($response);
4652
$url = $response->json('resource.photos.0.size_variants.medium.url');
@@ -49,6 +55,24 @@ public function testTemporaryImage(): void
4955
$response = $this->get($url);
5056
$this->assertNotFound($response);
5157
$response->assertSeeText('File not found'); // We mocked the file !
58+
}
59+
60+
public function testExpiredSignature(): void
61+
{
62+
$this->setTemporaryLink();
63+
$expired_url = URL::temporarySignedRoute('image', now()->subMinutes(10), ['path' => 'c3/3d/c661c594a5a781cd44db06828783.png']);
64+
$response = $this->get($expired_url);
65+
$this->assertForbidden($response);
66+
$response->assertSeeText('Link expired');
67+
}
68+
69+
public function testBrokenSignature(): void
70+
{
71+
$this->setTemporaryLink();
72+
$response = $this->getJsonWithData('Album', ['album_id' => $this->album4->id]);
73+
$this->assertOk($response);
74+
$url = $response->json('resource.photos.0.size_variants.medium.url');
75+
$this->assertStringContainsString('/image/medium/', $url);
5276

5377
$unsigned_url = explode('?', $url)[0];
5478
$response = $this->get($unsigned_url);
@@ -57,8 +81,7 @@ public function testTemporaryImage(): void
5781

5882
public function testEncryptedImages(): void
5983
{
60-
Configs::set('secure_image_link_enabled', '1');
61-
Configs::invalidateCache();
84+
$this->setSecureLink();
6285

6386
$response = $this->getJsonWithData('Album', ['album_id' => $this->album4->id]);
6487
$this->assertOk($response);
@@ -68,18 +91,12 @@ public function testEncryptedImages(): void
6891
$response = $this->get($url);
6992
$this->assertNotFound($response);
7093
$response->assertSeeText('File not found'); // We mocked the file !
71-
72-
$unsigned_url = explode('?', $url)[0];
73-
$response = $this->get($unsigned_url);
74-
$this->assertForbidden($response);
7594
}
7695

7796
public function testBrokenEncryption(): void
7897
{
79-
Configs::set('secure_image_link_enabled', '1');
80-
Configs::invalidateCache();
81-
82-
$broken_url = URL::temporarySignedRoute('image', now()->addSeconds(10), ['path' => 'broken_path']);
98+
$this->setSecureLink();
99+
$broken_url = URL::route('image', ['path' => 'broken_path']);
83100
$response = $this->get($broken_url);
84101
$this->assertForbidden($response);
85102
$response->assertSeeText('Invalid payload');

‎version.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
6.6.4
1+
6.6.5

0 commit comments

Comments
 (0)