From e9cf754b372f2e3ba228ce00b0b68aace355abcf Mon Sep 17 00:00:00 2001 From: Gregor Klevze Date: Sat, 29 Aug 2026 12:25:37 +0200 Subject: [PATCH] Enable nginx X-Accel for original artwork downloads. Hand originals to nginx after auth so PHP is not in the byte path. Keep DOWNLOAD_ACCEL_ENABLED off until the internal location is verified. --- .env.example | 5 + .../Controllers/ArtworkDownloadController.php | 14 +- deploy/nginx/download-accel.conf | 42 ++--- docs/optimization-m13-x-accel-downloads.md | 80 +++++++++ tests/Feature/ArtworkDownloadAccelTest.php | 170 ++++++++++++++++++ tests/Feature/ArtworkDownloadTest.php | 4 +- .../Feature/Http/ArtworkDownloadM15CTest.php | 131 ++++++++++++++ 7 files changed, 411 insertions(+), 35 deletions(-) create mode 100644 docs/optimization-m13-x-accel-downloads.md create mode 100644 tests/Feature/ArtworkDownloadAccelTest.php create mode 100644 tests/Feature/Http/ArtworkDownloadM15CTest.php diff --git a/.env.example b/.env.example index 8129f6ac..bc8108fd 100644 --- a/.env.example +++ b/.env.example @@ -4,6 +4,11 @@ APP_KEY= APP_DEBUG=true APP_URL=http://localhost +# Artwork original downloads: PHP fallback unless nginx X-Accel is configured. +# Leave false until the internal location exists and has been verified. +DOWNLOAD_ACCEL_ENABLED=false +DOWNLOAD_ACCEL_PATH=/internal/originals + SECURITY_REPORT_ENABLED=true SECURITY_REPORT_NOTIFY_EMAIL= SECURITY_REPORT_SCAN_NPM=true diff --git a/app/Http/Controllers/ArtworkDownloadController.php b/app/Http/Controllers/ArtworkDownloadController.php index fa6c1db4..41069b28 100644 --- a/app/Http/Controllers/ArtworkDownloadController.php +++ b/app/Http/Controllers/ArtworkDownloadController.php @@ -48,14 +48,14 @@ final class ArtworkDownloadController extends Controller $artwork = Artwork::query()->find($id); if (! $artwork) { - abort(404); + return $this->notFound(); } $filePath = $this->originalFiles->resolveLocalPath($artwork); $ext = strtolower(ltrim((string) pathinfo($filePath, PATHINFO_EXTENSION), '.')); if ($filePath === '' || ! in_array($ext, self::ALLOWED_EXTENSIONS, true)) { - abort(404); + return $this->notFound(); } if (! File::isFile($filePath)) { @@ -65,7 +65,7 @@ final class ArtworkDownloadController extends Controller 'resolved_path' => $filePath, ]); - abort(404); + return $this->notFound(); } $this->recordDownload($request, $artwork->id); @@ -98,6 +98,14 @@ final class ArtworkDownloadController extends Controller return response()->download($filePath, $downloadName); } + private function notFound(): Response + { + return response('', 404, [ + 'Content-Type' => 'text/plain; charset=UTF-8', + 'Cache-Control' => 'no-store', + ]); + } + private function resolveAccelUri(string $filePath): ?string { if (! config('app.download_accel_enabled')) { diff --git a/deploy/nginx/download-accel.conf b/deploy/nginx/download-accel.conf index 40b109c0..8c317550 100644 --- a/deploy/nginx/download-accel.conf +++ b/deploy/nginx/download-accel.conf @@ -1,36 +1,18 @@ -# ----------------------------------------------------------------------- -# nginx X-Accel-Redirect for artwork original file downloads +# M13 — nginx X-Accel-Redirect for GET /download/artwork/{id} # -# Problem: PHP streams the file body through FPM → nginx, causing -# "[warn] upstream response is buffered to a temporary file" -# for large downloads because FastCGI buffers are too small. +# Production vhost is NOT in this repository (live file: +# /etc/nginx/sites-enabled/skinbase.org.conf). +# Insert this location inside the HTTPS server { } block, before the +# catch-all `location /` and any regex locations that could steal the URI. # -# Solution: PHP sets X-Accel-Redirect, nginx serves the file body -# directly from disk — FPM is only used for the header response. +# Trailing slashes are required: location prefix and alias both end with /. +# Use the canonical shared storage tree so release switches do not break nginx. +# Do not use `root` here. # -# Setup: -# 1. Set DOWNLOAD_ACCEL_PATH=/internal/originals in .env (production). -# 2. Include this file inside your server {} block: -# include /etc/nginx/conf.d/download-accel.conf; -# 3. Make sure the nginx worker has read access to the originals path. -# -# The /internal/originals prefix MUST match DOWNLOAD_ACCEL_PATH in .env. -# The alias path MUST match ARTWORKS_LOCAL_ORIGINALS_ROOT on the server. -# ----------------------------------------------------------------------- +# Direct GET /internal/originals/... must 404 (internal;). +# Laravel still authorizes/counts; only the file body is offloaded. -location /internal/originals/ { - # Block direct client access — only X-Accel-Redirect headers trigger this. +location ^~ /internal/originals/ { internal; - - # Replace this with the actual absolute path to artwork originals on disk. - # Typically: /var/www/skinbase/storage/originals or /var/www/skinbase/public/files/originals - alias /var/www/skinbase/public/files/originals/; - - # Let nginx send the file efficiently with sendfile + tcp_nopush. - sendfile on; - tcp_nopush on; - tcp_nodelay on; - - # No nginx-level caching for download responses (private files). - add_header Cache-Control "private, no-store"; + alias /opt/www/virtual/SkinbaseNova.releases/shared/storage/app/originals/artworks/; } diff --git a/docs/optimization-m13-x-accel-downloads.md b/docs/optimization-m13-x-accel-downloads.md new file mode 100644 index 00000000..a3ab5329 --- /dev/null +++ b/docs/optimization-m13-x-accel-downloads.md @@ -0,0 +1,80 @@ +# M13 — Artwork Download X-Accel-Redirect Offload + +Application tests and nginx snippet only. **Do not enable in production until Stage A nginx is verified.** + +## Pre-M13 evidence + +`/download/artwork/{id}` (M12 analyzer, skinbase.org): + +| | | +| --- | --- | +| count | 2363 | +| p50 / p95 / p99 / max | 0.535 / 1.526 / 3.335 / **16.037** s | +| avg request_time | 0.7175 s | +| avg upstream_response_time | 0.3762 s | +| avg bytes | ~528 KB | + +Slow requests show PHP finishing in ~0.3–0.5 s while `request_time` stays large: FPM is stuck sending the body. + +After X-Accel: `upstream_response_time` is still Laravel work; `request_time` can still include slow clients. The win is **freeing PHP-FPM**, not always a tiny nginx `request_time`. + +## Existing application + +- Route: `GET /download/artwork/{id}` → `ArtworkDownloadController` +- Flag: `DOWNLOAD_ACCEL_ENABLED` default **false** +- Path: `DOWNLOAD_ACCEL_PATH=/internal/originals` +- Laravel originals root: `uploads.local_originals_root` (symlink `SkinbaseNova/storage` → shared) +- Fallback: `response()->download()` — **must stay** + +Controller already authorizes, checks file, records analytics, then optionally sets `X-Accel-Redirect`. + +## nginx mapping + +Production vhost is **not** in this repo: `/etc/nginx/sites-enabled/skinbase.org.conf`. + +Snippet: `deploy/nginx/download-accel.conf` + +```nginx +location ^~ /internal/originals/ { + internal; + alias /opt/www/virtual/SkinbaseNova.releases/shared/storage/app/originals/artworks/; +} +``` + +Alias uses the **canonical shared tree** so a release switch cannot point nginx at a missing release directory. Laravel still uses `local_originals_root`; the X-Accel URI is only the relative hash path (`/27/21/….zip`). + +`internal;` is mandatory: a browser GET to `/internal/originals/...` must 404. + +No sendfile/tcp/buffering changes in M13. + +## Rollout (operator — not executed here) + +### Stage A — nginx only (`DOWNLOAD_ACCEL_ENABLED=false`) + +1. Backup: `sudo cp /etc/nginx/sites-enabled/skinbase.org.conf /etc/nginx/sites-enabled/skinbase.org.conf.bak-$(date +%Y%m%d-%H%M%S)` +2. Insert the location inside the HTTPS `server { }` (before `location /`). +3. `sudo nginx -t` — stop if it fails. +4. `sudo systemctl reload nginx` — **not** restart. +5. Direct access: `curl -I https://skinbase.org/internal/originals/` → **404**. +6. Optional: `sudo -u www-data test -r && echo READABLE` + +### Stage B — Laravel flag + +7. Set `DOWNLOAD_ACCEL_ENABLED=true` (path already `/internal/originals`). +8. `sudo -u skinbase php artisan config:cache` if config is cached. +9. Verify with tinker as `skinbase` (flag true, path `/internal/originals`). +10. Public: `curl -fL https://skinbase.org/download/artwork/` → 200, attachment, size matches original. +11. Range: `curl -H 'Range: bytes=0-99'` → **206**, `Content-Range: bytes 0-99/`, 100-byte body. +12. Repeat direct internal URL → still 404. + +Application deploy: `bash ./sync.sh`. Never `git pull` on production. + +## Rollback + +First: `DOWNLOAD_ACCEL_ENABLED=false` then `sudo -u skinbase php artisan config:cache` if needed. PHP fallback returns immediately. Nginx location can stay. + +If nginx must be reverted: restore the `.bak-*` vhost, `nginx -t`, reload. + +## Analyzer + +Keep M12 `skinbase-performance.log`. After activation, `upstream_response_time` should stay near Laravel work; do not expect `request_time` to collapse for slow clients. diff --git a/tests/Feature/ArtworkDownloadAccelTest.php b/tests/Feature/ArtworkDownloadAccelTest.php new file mode 100644 index 00000000..13b6b160 --- /dev/null +++ b/tests/Feature/ArtworkDownloadAccelTest.php @@ -0,0 +1,170 @@ + $root, + 'uploads.local_originals_root' => $root, + 'app.download_accel_enabled' => false, + 'app.download_accel_path' => '/internal/originals', + 'app.url' => 'https://skinbase.test', + ]); + + if (File::exists($root)) { + File::deleteDirectory($root); + } + + File::makeDirectory($root, 0755, true); +}); + +afterEach(function () { + $root = storage_path('framework/testing/artwork-downloads-accel'); + if (File::exists($root)) { + File::deleteDirectory($root); + } +}); + +function accelOriginal(string $hash, string $ext, string $content = 'original-bytes'): string +{ + $root = rtrim((string) config('uploads.local_originals_root'), DIRECTORY_SEPARATOR); + $dir = $root.DIRECTORY_SEPARATOR.substr($hash, 0, 2).DIRECTORY_SEPARATOR.substr($hash, 2, 2); + File::makeDirectory($dir, 0755, true, true); + $path = $dir.DIRECTORY_SEPARATOR.$hash.'.'.$ext; + File::put($path, $content); + + return $path; +} + +function resolveAccelUri(string $filePath): ?string +{ + $controller = app(ArtworkDownloadController::class); + $method = new ReflectionMethod(ArtworkDownloadController::class, 'resolveAccelUri'); + + return $method->invoke($controller, $filePath); +} + +it('uses the PHP download fallback when acceleration is disabled', function () { + $hash = '2721722cb2f407cc53539ee8718cc89c4cdfc579'; + accelOriginal($hash, 'zip', 'zip-body'); + + $artwork = Artwork::factory()->create([ + 'file_name' => 'Nested Original', + 'hash' => $hash, + 'file_ext' => 'zip', + ]); + + $response = $this->get('/download/artwork/'.$artwork->id); + + $response->assertOk(); + expect($response->headers->get('X-Accel-Redirect'))->toBeNull() + ->and($response->baseResponse)->toBeInstanceOf(BinaryFileResponse::class); +}); + +it('returns X-Accel-Redirect for a nested original when acceleration is enabled', function () { + config(['app.download_accel_enabled' => true]); + $hash = '2721722cb2f407cc53539ee8718cc89c4cdfc579'; + accelOriginal($hash, 'zip', 'zip-body'); + + $artwork = Artwork::factory()->create([ + 'file_name' => 'Nested Original', + 'hash' => $hash, + 'file_ext' => 'zip', + ]); + + $this->mock(ArtworkStatsService::class, function ($mock) use ($artwork): void { + $mock->shouldReceive('incrementDownloads')->once()->with($artwork->id, 1, false); + }); + + $response = $this->get('/download/artwork/'.$artwork->id); + + $response->assertOk() + ->assertHeader('X-Accel-Redirect', '/internal/originals/27/21/2721722cb2f407cc53539ee8718cc89c4cdfc579.zip') + ->assertHeader('Content-Type', 'application/octet-stream') + ->assertHeader('X-Content-Type-Options', 'nosniff'); + + $disposition = (string) $response->headers->get('Content-Disposition'); + expect($disposition)->toContain('attachment') + ->and($disposition)->toContain('Nested Original') + ->and($disposition)->toContain('.zip') + ->and($response->baseResponse)->not->toBeInstanceOf(BinaryFileResponse::class); + + expect(DB::table('artwork_downloads')->where('artwork_id', $artwork->id)->count())->toBe(1); +}); + +it('maps nested originals under the accel prefix without duplicate slashes', function () { + config(['app.download_accel_enabled' => true]); + $root = rtrim((string) config('uploads.local_originals_root'), DIRECTORY_SEPARATOR); + $relative = '27'.DIRECTORY_SEPARATOR.'21'.DIRECTORY_SEPARATOR.'2721722cb2f407cc53539ee8718cc89c4cdfc579.zip'; + $path = $root.DIRECTORY_SEPARATOR.$relative; + + expect(resolveAccelUri($path))->toBe('/internal/originals/27/21/2721722cb2f407cc53539ee8718cc89c4cdfc579.zip'); +}); + +it('does not emit an accel URI for a file outside the originals root', function () { + config(['app.download_accel_enabled' => true]); + $outside = sys_get_temp_dir().DIRECTORY_SEPARATOR.'not-original.zip'; + File::put($outside, 'nope'); + + expect(resolveAccelUri($outside))->toBeNull(); + + File::delete($outside); +}); + +it('returns 404 for an unsupported original extension', function () { + $hash = 'aa11bb22cc33dd44ee55ff6677889900aabbccdd'; + accelOriginal($hash, 'exe', 'not-allowed'); + + $artwork = Artwork::factory()->create([ + 'hash' => $hash, + 'file_ext' => 'exe', + 'file_name' => 'bad.exe', + ]); + + $this->mock(ArtworkStatsService::class, function ($mock): void { + $mock->shouldReceive('incrementDownloads')->never(); + }); + + $this->get('/download/artwork/'.$artwork->id)->assertNotFound(); + expect(DB::table('artwork_downloads')->where('artwork_id', $artwork->id)->count())->toBe(0); +}); + +it('still records download analytics when acceleration is enabled', function () { + config(['app.download_accel_enabled' => true]); + $hash = 'bb22cc33dd44ee55ff6677889900aabbccddeeff'; + accelOriginal($hash, 'jpg', 'jpeg-bytes'); + + $artwork = Artwork::factory()->create([ + 'hash' => $hash, + 'file_ext' => 'jpg', + 'file_name' => 'Photo', + ]); + + DB::table('artwork_stats')->insertOrIgnore([ + 'artwork_id' => $artwork->id, + 'views' => 0, + 'views_24h' => 0, + 'views_7d' => 0, + 'downloads' => 0, + 'downloads_24h' => 0, + 'downloads_7d' => 0, + 'favorites' => 0, + 'rating_avg' => 0, + 'rating_count' => 0, + ]); + + $this->get('/download/artwork/'.$artwork->id) + ->assertOk() + ->assertHeader('X-Accel-Redirect'); + + expect(DB::table('artwork_downloads')->where('artwork_id', $artwork->id)->count())->toBe(1) + ->and((int) DB::table('artwork_stats')->where('artwork_id', $artwork->id)->value('downloads'))->toBe(1); +}); diff --git a/tests/Feature/ArtworkDownloadTest.php b/tests/Feature/ArtworkDownloadTest.php index 410b5b59..0e23775b 100644 --- a/tests/Feature/ArtworkDownloadTest.php +++ b/tests/Feature/ArtworkDownloadTest.php @@ -57,7 +57,7 @@ it('downloads an existing artwork file', function () { $response = $this->get("/download/artwork/{$artwork->id}"); $response->assertOk(); - $response->assertDownload('Sky Sunset.png'); + $response->assertDownload('Sky Sunset (skinbase.org).png'); }); it('forces the download filename using file_name and extension', function () { @@ -74,7 +74,7 @@ it('forces the download filename using file_name and extension', function () { $response = $this->get("/download/artwork/{$artwork->id}"); $response->assertOk(); - $response->assertDownload('My Original Name.jpg'); + $response->assertDownload('My Original Name (skinbase.org).jpg'); }); it('returns 404 for a missing artwork', function () { diff --git a/tests/Feature/Http/ArtworkDownloadM15CTest.php b/tests/Feature/Http/ArtworkDownloadM15CTest.php new file mode 100644 index 00000000..3eafa2df --- /dev/null +++ b/tests/Feature/Http/ArtworkDownloadM15CTest.php @@ -0,0 +1,131 @@ + $root, + 'uploads.local_originals_root' => $root, + 'app.download_accel_enabled' => false, + 'app.url' => 'https://skinbase.test', + 'skinbase-sessions.enabled' => true, + 'skinbase-sessions.debug_header' => true, + 'skinbase-sessions.skip_anonymous_public_get' => true, + ]); + + if (File::exists($root)) { + File::deleteDirectory($root); + } + + File::makeDirectory($root, 0755, true); +}); + +afterEach(function () { + $root = storage_path('framework/testing/artwork-downloads-m15c'); + if (File::exists($root)) { + File::deleteDirectory($root); + } +}); + +function m15cOriginal(string $hash, string $ext, string $content = 'bytes'): string +{ + $root = rtrim((string) config('uploads.local_originals_root'), DIRECTORY_SEPARATOR); + $dir = $root.DIRECTORY_SEPARATOR.substr($hash, 0, 2).DIRECTORY_SEPARATOR.substr($hash, 2, 2); + File::makeDirectory($dir, 0755, true, true); + $path = $dir.DIRECTORY_SEPARATOR.$hash.'.'.$ext; + File::put($path, $content); + + return $path; +} + +it('returns a cheap 404 for a nonexistent artwork without analytics', function () { + $this->mock(ArtworkStatsService::class, function ($mock): void { + $mock->shouldReceive('incrementDownloads')->never(); + }); + + $this->get('/download/artwork/999999')->assertNotFound(); + + expect(DB::table('artwork_downloads')->count())->toBe(0); +}); + +it('returns a cheap 404 for a missing original without analytics', function () { + $artwork = Artwork::factory()->create([ + 'hash' => 'c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f6a7b8c9d0', + 'file_ext' => 'webp', + 'file_name' => 'gone.webp', + ]); + + $this->mock(ArtworkStatsService::class, function ($mock): void { + $mock->shouldReceive('incrementDownloads')->never(); + }); + + $this->get('/download/artwork/'.$artwork->id)->assertNotFound(); + + expect(DB::table('artwork_downloads')->where('artwork_id', $artwork->id)->count())->toBe(0); +}); + +it('keeps valid anonymous downloads and X-Accel-Redirect when enabled', function () { + config(['app.download_accel_enabled' => true, 'app.download_accel_path' => '/internal/originals']); + $hash = '2721722cb2f407cc53539ee8718cc89c4cdfc579'; + m15cOriginal($hash, 'zip'); + + $artwork = Artwork::factory()->create([ + 'hash' => $hash, + 'file_ext' => 'zip', + 'file_name' => 'Anon File', + ]); + + $this->get('/download/artwork/'.$artwork->id) + ->assertOk() + ->assertHeader('X-Accel-Redirect', '/internal/originals/27/21/2721722cb2f407cc53539ee8718cc89c4cdfc579.zip'); +}); + +it('still records ArtworkDownload.user_id for authenticated downloads', function () { + $hash = 'aa11bb22cc33dd44ee55ff6677889900aabbccdd'; + m15cOriginal($hash, 'png'); + $user = User::factory()->create(); + $artwork = Artwork::factory()->create([ + 'hash' => $hash, + 'file_ext' => 'png', + 'file_name' => 'Auth File', + ]); + + $this->actingAs($user)->get('/download/artwork/'.$artwork->id)->assertOk(); + + $this->assertDatabaseHas('artwork_downloads', [ + 'artwork_id' => $artwork->id, + 'user_id' => $user->id, + ]); +}); + +it('skips session for anonymous download GET without a session cookie', function () { + $this->get('/download/artwork/999999') + ->assertNotFound() + ->assertHeader('X-Skinbase-Session', 'skipped'); +}); + +it('starts session for download GET when a session cookie is already present', function () { + $cookie = (string) config('session.cookie'); + + $this->withCookie($cookie, 'existing-session-cookie') + ->get('/download/artwork/999999') + ->assertNotFound() + ->assertHeader('X-Skinbase-Session', 'started'); +}); + +it('does not record presence for artwork download requests', function () { + $this->mock(OnlineVisitorRepository::class, function ($mock): void { + $mock->shouldReceive('track')->never(); + }); + + $this->get('/download/artwork/999999')->assertNotFound(); +});