From 383c8f46130caf9f2ef4397a5607bfaf4e214e34 Mon Sep 17 00:00:00 2001 From: "Ronald A. Richardson" Date: Tue, 6 Oct 2026 13:05:58 +0800 Subject: [PATCH] fix(files): make uploads work with a private S3 media bucket - Drop the 'public' visibility from Utils::urlToStorefrontFile: a bucket with BucketOwnerEnforced rejects any PUT carrying an ACL, so put() returned false. - Add File::signStoredUrl()/s3KeyFromUrl(): turn absolute URLs stored as strings (legacy unsigned bucket URLs, expired signed URLs) back into a key and re-sign. - Re-sign template builder image src at render time. - Cache signed URLs for 60 of their 120 minutes so every URL handed out has at least an hour left; cut Extension icon_url cache from 24h to 30m. --- src/Models/Extension.php | 3 +- src/Models/File.php | 102 +++++++++++++++++++++---- src/Services/TemplateRenderService.php | 4 +- src/Support/Utils.php | 5 +- tests/Unit/Models/FileModelTest.php | 47 ++++++++++++ 5 files changed, 142 insertions(+), 19 deletions(-) diff --git a/src/Models/Extension.php b/src/Models/Extension.php index aab27aec..86b9b7f2 100644 --- a/src/Models/Extension.php +++ b/src/Models/Extension.php @@ -187,7 +187,8 @@ public function getAuthorNameAttribute() */ public function getIconUrlAttribute() { - return static::attributeFromCache($this, 'file.url', 'https://s3.ap-southeast-1.amazonaws.com/flb-assets/static/no-avatar.png'); + // short TTL: file.url is a signed URL that expires, a day-long cache would serve dead links + return static::attributeFromCache($this, 'file.url', 'https://s3.ap-southeast-1.amazonaws.com/flb-assets/static/no-avatar.png', 30 * 60); } /** diff --git a/src/Models/File.php b/src/Models/File.php index 05e3cc59..1eaa37e3 100644 --- a/src/Models/File.php +++ b/src/Models/File.php @@ -147,30 +147,102 @@ public function getUrlAttribute() /** @var Storage $filesystem */ $filesystem = $this->getFilesystem(); - $cacheKey = "file_url_{$this->uuid}"; - $bufferTime = 5; // Buffer time in minutes + if ($disk === 's3' || $disk === 'gcs') { + return static::cachedTemporaryUrl($filesystem, $this->path, "file_url_{$this->uuid}"); + } + + $url = $filesystem->url($this->path); + + if ($disk === 'local') { + return asset($url, !app()->environment(['development', 'local'])); + } + + return $url; + } + + /** + * Generate a signed URL for an object, cached for slightly less than its lifetime. + */ + protected static function cachedTemporaryUrl($filesystem, string $path, string $cacheKey): string + { + // Cache for half the signature's lifetime, so every URL handed out has at least an hour left. + // Callers (browser tabs, short-lived caches) hold the string after we return it. + $bufferTime = 60; // Buffer time in minutes $urlExpiration = 120; // URL expiration time in minutes (2 hours) - if ($disk === 's3' || $disk === 'gcs') { - // Check if the URL is already cached - if (Cache::has($cacheKey)) { - return Cache::get($cacheKey); - } + // Check if the URL is already cached + if (Cache::has($cacheKey)) { + return Cache::get($cacheKey); + } + + // Generate a new temporary URL + $url = $filesystem->temporaryUrl($path, now()->addMinutes($urlExpiration)); + + // Cache the URL with a reduced expiration time for buffer + Cache::put($cacheKey, $url, now()->addMinutes($urlExpiration - $bufferTime)); + + return $url; + } - // Generate a new temporary URL - $url = $filesystem->temporaryUrl($this->path, now()->addMinutes($urlExpiration)); + /** + * Re-sign an absolute URL that was stored as a string and points into the configured S3 bucket. + * + * Some columns (e.g. legacy `avatar_url` values, cart item image URLs) hold a URL rather than a + * File reference. Plain bucket URLs only work while the bucket is publicly readable, and stored + * signed URLs stop working once their signature expires, so both are turned back into an object + * key and signed afresh. Anything else (other hosts, flb-assets, relative paths, UUIDs) is + * returned unchanged. + */ + public static function signStoredUrl(?string $url): ?string + { + $key = static::s3KeyFromUrl($url); + if ($key === null) { + return $url; + } - // Cache the URL with a reduced expiration time for buffer - Cache::put($cacheKey, $url, now()->addMinutes($urlExpiration - $bufferTime)); + return static::cachedTemporaryUrl(Storage::disk('s3'), $key, 'file_url_key_' . sha1($key)); + } + + /** + * Extract the object key from a URL that points into the configured S3 bucket, or null. + * + * Recognises virtual-hosted (`bucket.s3.region.amazonaws.com/key`, `bucket.s3-region...`), + * path-style (`s3.region.amazonaws.com/bucket/key`) and the disk's configured `url` (AWS_URL). + */ + public static function s3KeyFromUrl(?string $url): ?string + { + if (!is_string($url) || !preg_match('#^https?://#i', $url)) { + return null; + } + + $bucket = config('filesystems.disks.s3.bucket'); + if (!is_string($bucket) || $bucket === '') { + return null; + } + + $key = null; + $withoutQs = preg_split('/[?#]/', $url, 2)[0]; + $configured = config('filesystems.disks.s3.url'); + + if (is_string($configured) && $configured !== '' && Str::startsWith($withoutQs, rtrim($configured, '/') . '/')) { + $key = Str::after($withoutQs, rtrim($configured, '/') . '/'); } else { - $url = $filesystem->url($this->path); + $host = strtolower((string) parse_url($withoutQs, PHP_URL_HOST)); + $path = ltrim((string) parse_url($withoutQs, PHP_URL_PATH), '/'); + $quoted = preg_quote(strtolower($bucket), '#'); + + if (preg_match('#^' . $quoted . '\.s3([.-][a-z0-9-]+)?\.amazonaws\.com$#', $host)) { + $key = $path; + } elseif (preg_match('#^s3([.-][a-z0-9-]+)?\.amazonaws\.com$#', $host) && Str::startsWith($path, $bucket . '/')) { + $key = Str::after($path, $bucket . '/'); + } } - if ($disk === 'local') { - return asset($url, !app()->environment(['development', 'local'])); + if ($key === null || $key === '') { + return null; } - return $url; + return rawurldecode($key); } /** diff --git a/src/Services/TemplateRenderService.php b/src/Services/TemplateRenderService.php index c0583b4f..e6348435 100644 --- a/src/Services/TemplateRenderService.php +++ b/src/Services/TemplateRenderService.php @@ -2,6 +2,7 @@ namespace Fleetbase\Services; +use Fleetbase\Models\File; use Fleetbase\Models\Template; use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Carbon; @@ -297,7 +298,8 @@ protected function renderElement(array $element): string return "
{$content}
\n"; case 'image': - $src = data_get($element, 'src', ''); + // the builder stores the upload's URL; re-sign it so old (expired or unsigned) bucket URLs still render + $src = File::signStoredUrl((string) data_get($element, 'src', '')); return "\"\"\n"; diff --git a/src/Support/Utils.php b/src/Support/Utils.php index a26b916f..473f9269 100644 --- a/src/Support/Utils.php +++ b/src/Support/Utils.php @@ -1791,8 +1791,9 @@ public static function urlToStorefrontFile($url, $type = 'source', ?Model $owner $bucketPath = 'uploads/storefront/' . $owner->uuid . '/' . Str::slug($type) . '/' . $fileName; $pathInfo = pathinfo($bucketPath); - // upload to bucket - Storage::disk('s3')->put($bucketPath, $contents, 'public'); + // upload to bucket. No 'public' visibility: the media bucket is private and enforces + // bucket-owner object ownership, which rejects any request carrying an ACL. + Storage::disk('s3')->put($bucketPath, $contents); $fileInfo = [ 'company_uuid' => $owner->company_uuid ?? null, diff --git a/tests/Unit/Models/FileModelTest.php b/tests/Unit/Models/FileModelTest.php index 6a89779c..1dfe327f 100644 --- a/tests/Unit/Models/FileModelTest.php +++ b/tests/Unit/Models/FileModelTest.php @@ -343,6 +343,53 @@ function bind_file_model_filesystem(array $config = []): FileModelFilesystemFake expect($file->url)->toBe('https://cdn.example.test/cached-report.csv'); }); +it('extracts object keys only from urls that point into the configured s3 bucket', function () { + bind_file_model_filesystem([ + 'filesystems.disks.s3.bucket' => 'fleetbase-production-media', + 'filesystems.disks.s3.url' => 'https://media.example.test/assets', + ]); + + expect(File::s3KeyFromUrl('https://fleetbase-production-media.s3.amazonaws.com/uploads/a/photo.png'))->toBe('uploads/a/photo.png') + ->and(File::s3KeyFromUrl('https://fleetbase-production-media.s3.ap-southeast-1.amazonaws.com/uploads/a/photo.png?X-Amz-Signature=old'))->toBe('uploads/a/photo.png') + ->and(File::s3KeyFromUrl('https://fleetbase-production-media.s3-ap-southeast-1.amazonaws.com/custom-avatars/vehicles/c/My%20Van.png'))->toBe('custom-avatars/vehicles/c/My Van.png') + ->and(File::s3KeyFromUrl('https://s3.ap-southeast-1.amazonaws.com/fleetbase-production-media/uploads/b/logo.png'))->toBe('uploads/b/logo.png') + ->and(File::s3KeyFromUrl('https://media.example.test/assets/uploads/c/doc.pdf'))->toBe('uploads/c/doc.pdf') + // other buckets, other hosts and non-urls are left alone + ->and(File::s3KeyFromUrl('https://flb-assets.s3.ap-southeast-1.amazonaws.com/static/no-avatar.png'))->toBeNull() + ->and(File::s3KeyFromUrl('https://s3.ap-southeast-1.amazonaws.com/flb-assets/static/no-avatar.png'))->toBeNull() + ->and(File::s3KeyFromUrl('https://evil.example.test/fleetbase-production-media.s3.amazonaws.com/x.png'))->toBeNull() + ->and(File::s3KeyFromUrl('https://fleetbase-production-media.s3.amazonaws.com.evil.test/x.png'))->toBeNull() + ->and(File::s3KeyFromUrl('https://fleetbase-production-media.s3.amazonaws.com/'))->toBeNull() + ->and(File::s3KeyFromUrl('5f1c2a10-0000-4000-8000-000000000000'))->toBeNull() + ->and(File::s3KeyFromUrl(null))->toBeNull(); + + // config is shared across tests: drop the url override here as well + bind_file_model_filesystem(['filesystems.disks.s3.bucket' => null, 'filesystems.disks.s3.url' => null]); + + expect(File::s3KeyFromUrl('https://fleetbase-production-media.s3.amazonaws.com/uploads/a/photo.png'))->toBeNull(); +}); + +it('re-signs stored bucket urls and leaves every other value unchanged', function () { + $filesystem = bind_file_model_filesystem([ + 'filesystems.disks.s3.bucket' => 'fleetbase-production-media', + ]); + + $stored = 'https://fleetbase-production-media.s3.ap-southeast-1.amazonaws.com/custom-avatars/vehicles/c/van.png?X-Amz-Expires=7200&X-Amz-Signature=expired'; + + expect(File::signStoredUrl($stored))->toBe('https://s3.example.test/custom-avatars/vehicles/c/van.png?temporary=1') + ->and($filesystem->disk('s3')->temporaryUrls)->toHaveKey('custom-avatars/vehicles/c/van.png') + ->and(File::signStoredUrl('https://flb-assets.s3.ap-southeast-1.amazonaws.com/static/vehicle-icons/mini_bus.svg')) + ->toBe('https://flb-assets.s3.ap-southeast-1.amazonaws.com/static/vehicle-icons/mini_bus.svg') + ->and(File::signStoredUrl(null))->toBeNull() + ->and(File::signStoredUrl(''))->toBe(''); + + // the signed url is cached per object key, like File::url + $filesystem->disk('s3')->temporaryUrls = []; + + expect(File::signStoredUrl($stored))->toBe('https://s3.example.test/custom-avatars/vehicles/c/van.png?temporary=1') + ->and($filesystem->disk('s3')->temporaryUrls)->toBe([]); +}); + it('assigns uploaders subjects and file types through model mutators', function () { bind_test_container();