diff --git a/DOCKER.md b/DOCKER.md index c858b3cc..262738a3 100644 --- a/DOCKER.md +++ b/DOCKER.md @@ -121,11 +121,18 @@ Without it every visitor appears to come from the proxy. The login rate limiter your users as one attacker, and the download log records the proxy's address instead of the person's. `compose.example.yaml` already sets this. -What it does **not** affect is whether a request succeeds at all. `TRUSTED_PROXIES` only changes how -the application reads `X-Forwarded-*` headers on a request that already arrived, so getting it wrong -gives you wrong client IPs, wrong generated links, or a redirect loop — never a `502`. A 502 means -your proxy could not get a usable response out of the container, which is a different problem with a -different fix. +Leaving it unset does not cause a `502` — that means your proxy could not get a usable response out +of the container at all, which is a different problem with a different fix. It does cause a **419 +"page expired"**. Without it the application never learns the proxy terminated TLS, so it builds +its links and redirects with `http://` while the browser is on `https://`, and marks the session +cookie as non-secure. The browser declines to send that cookie back to what it now reads as a +different, less secure origin, the session arrives empty, and the first thing you submit — usually +the create-your-admin-account form — is rejected as a stale token. After that you get returned to +the login screen at random, because each redirect leaves and re-enters over the wrong scheme. + +Your proxy also has to pass the original `Host` header through, or the links come out naming the +container instead of your domain. Most do by default: `passHostHeader=true` in Traefik, +`proxy_set_header Host $host;` in nginx. ### Give the proxy header headroom diff --git a/INSTALL.md b/INSTALL.md index 55c40994..821f49f0 100644 --- a/INSTALL.md +++ b/INSTALL.md @@ -463,28 +463,16 @@ sudo -u www-data php artisan event:cache You only run these once: `projectsend:update` notices they are in place and rebuilds them for you after every update. If you change your mind, `php artisan optimize:clear` undoes all three. -#### One command to skip: `config:cache` +#### `config:cache` and your `.env` -Every Laravel deployment guide on the internet lists `php artisan config:cache` alongside those -three, and `php artisan optimize` runs it for you. **Don't** — not on this application. +Every Laravel deployment guide also lists `php artisan config:cache`, and `php artisan optimize` +runs it for you. It is safe here, with one thing to remember. -Here is why. Caching the configuration writes every resolved setting into one PHP file, and from -then on the framework stops reading your `.env` at all, on the entirely reasonable grounds that -everything in it has already been baked in. That holds for settings read the normal way, through -`config()`. ProjectSend reads one value earlier than that — `TRUSTED_PROXIES`, which has to be -known before the middleware stack is assembled, so it is read straight from the environment. Cache -the config and that read returns nothing. - -Nothing breaks loudly. The site comes up, you log in, everything looks fine. But if there is a -proxy or CDN in front of the server, ProjectSend goes back to believing every visitor is the proxy: -the login rate limiter now counts all of your users as one attacker and locks the whole site out -after five wrong passwords, and every row in the download log records the proxy's address instead -of the person who actually downloaded the file. Both are the kind of thing you discover weeks -later, from a complaint. - -If you have already run it — or ran `php artisan optimize`, which includes it — `php artisan -config:clear` puts things back immediately, and every update clears it too, saying why. The three commands above are safe and give you nearly -all of the speed anyway; `config:cache` was always the smallest win of the four. +Caching the configuration writes every resolved setting into one PHP file, and from then on the +framework stops reading your `.env` at all — everything in it has already been baked in. So +**re-run `php artisan config:cache` every time you edit `.env`**, or the edit does nothing and you +are left staring at a setting that is plainly there and plainly ignored. `php artisan config:clear` +goes back to reading `.env` directly, and every update clears it too, saying why. --- @@ -577,13 +565,22 @@ applies to a non-standard port; behind a TLS proxy on 443 you do not need it. **A change I made in `.env` has no effect.** Run `php artisan optimize:clear`, then restart PHP-FPM and the worker. Both hold the old values until they are restarted. If it *still* has no effect, someone has run `php artisan config:cache` -(or `optimize`) on this install — see [One command to skip](#one-command-to-skip-configcache). +(or `optimize`) on this install — re-run it to pick the new value up, or `php artisan config:clear` +to go back to reading `.env` directly. **Everyone is locked out of the login form at once, or the download log shows the same IP for every download.** ProjectSend is seeing your proxy or CDN instead of your visitors. Set `TRUSTED_PROXIES` in `.env` -(step 3) — and make sure `config:cache` has not been run, which stops that value from being read -at all. Same section as above. +(step 3) and restart PHP-FPM. + +**Behind a reverse proxy: 419 "page expired" when you log in or save a form, or you land back on +the login screen at random.** +`TRUSTED_PROXIES` again (step 3). Without it ProjectSend never learns the proxy terminated TLS, so +it builds its links and redirects with `http://` while the browser is on `https://`, and marks the +session cookie as non-secure. The browser then declines to send that cookie back, the session +arrives empty, and the write fails with a 419 that reads as an expired session. Make sure your +proxy passes the original `Host` header through as well — `proxy_set_header Host $host;` in nginx, +`passHostHeader=true` in Traefik (its default). Still stuck? Ask in the [community forum](https://www.projectsend.org/) or open an issue on [GitHub](https://github.com/projectsend/projectsend/issues), and include the last few lines of diff --git a/bootstrap/app.php b/bootstrap/app.php index cb526408..12dceeb7 100644 --- a/bootstrap/app.php +++ b/bootstrap/app.php @@ -32,17 +32,14 @@ return Application::configure(basePath: dirname(__DIR__)) health: '/up', ) ->withMiddleware(function (Middleware $middleware) { - // Unset = trust nothing, which is correct for the shipped topology - // (nginx talks to PHP-FPM directly and passes the real REMOTE_ADDR). - // Behind anything else — a load balancer, Cloudflare, the hosted - // Cloud ingress — this MUST name the proxy, or every client appears - // to come from it: per-IP throttles collapse into one shared bucket - // and the download IP log records the proxy instead of the client. - $proxies = env('TRUSTED_PROXIES'); - - if (is_string($proxies) && $proxies !== '') { - $middleware->trustProxies(at: $proxies === '*' ? '*' : explode(',', $proxies)); - } + // Trusted proxies are configured in config/trustedproxy.php, NOT + // here. This closure runs when the HTTP kernel is resolved, which is + // before the dotenv bootstrapper has read .env, so env() returns null + // here for anything that is not already a real environment variable — + // silently, and only on web requests (artisan bootstraps in the other + // order, so a CLI check reports the setting as working). The framework's + // TrustProxies middleware is in the global stack either way and falls + // back to that config key on its own. $middleware->web(append: [ // Binds every session to the password hash it was created under, diff --git a/config/trustedproxy.php b/config/trustedproxy.php new file mode 100644 index 00000000..52a26f58 --- /dev/null +++ b/config/trustedproxy.php @@ -0,0 +1,12 @@ + env('TRUSTED_PROXIES'), +]; diff --git a/tests/Feature/Groups/PublicGroupsTest.php b/tests/Feature/Groups/PublicGroupsTest.php index cb6c8e7f..5b9b5250 100644 --- a/tests/Feature/Groups/PublicGroupsTest.php +++ b/tests/Feature/Groups/PublicGroupsTest.php @@ -11,10 +11,8 @@ use App\Modules\Files\Models\Folder; use App\Modules\Groups\Models\Group; use App\Modules\Platform\Settings\Setting; use App\Modules\Platform\Settings\Settings; -use Illuminate\Http\UploadedFile; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Storage; -use Illuminate\Support\Str; use Illuminate\Testing\TestResponse; /** @@ -27,38 +25,6 @@ function publicPageProps(TestResponse $response): array return $page['props']; } -function publicListingFile(array $overrides = []): File -{ - return File::factory()->create(array_merge([ - 'uploaded_by' => User::factory()->create()->id, - 'name' => 'Report', - 'original_name' => 'report.pdf', - 'path' => '2026/08/'.Str::uuid()->toString().'.pdf', - 'mime_type' => 'application/pdf', - 'size' => 2048, - 'public' => true, - ], $overrides)); -} - -/** - * A real, thumbnailable public image on the faked "files" disk — unlike - * publicListingFile()'s bare PDF row, this one has actual bytes GD can - * decode, needed to exercise the thumbnail-generation path. - */ -function publicListingImageFile(User $uploader): File -{ - test()->actingAs($uploader)->post('/files', [ - 'file' => UploadedFile::fake()->image('photo.jpg', 200, 100), - 'name' => '', - 'description' => '', - ]); - - $file = File::query()->latest('id')->firstOrFail(); - $file->update(['public' => true]); - - return $file; -} - beforeEach(function () { Storage::fake('files'); diff --git a/tests/Feature/Platform/TrustedProxiesTest.php b/tests/Feature/Platform/TrustedProxiesTest.php new file mode 100644 index 00000000..12fbf925 --- /dev/null +++ b/tests/Feature/Platform/TrustedProxiesTest.php @@ -0,0 +1,88 @@ + response()->json([ + 'root' => request()->getSchemeAndHttpHost(), + 'ip' => request()->ip(), + 'secure' => request()->isSecure(), + ])); +}); + +test('forwarded scheme, host and client address are honoured when the proxy is trusted', function () { + config()->set('trustedproxy.proxies', '*'); + + $this->get('/__proxy-probe', [ + 'X-Forwarded-Proto' => 'https', + 'X-Forwarded-Host' => 'files.example.com', + 'X-Forwarded-For' => '203.0.113.9', + ])->assertOk()->assertJson([ + 'root' => 'https://files.example.com', + 'ip' => '203.0.113.9', + 'secure' => true, + ]); +}); + +test('forwarded headers are ignored when no proxy is trusted', function () { + config()->set('trustedproxy.proxies', null); + + // Asserted against the forwarded values rather than a literal expected + // host: the test environment's APP_URL supplies the host here, and + // isSecure() is already true from it, so neither is a signal on its own. + // What discriminates is that the proxy's claims are not adopted. + $json = $this->get('/__proxy-probe', [ + 'X-Forwarded-Proto' => 'https', + 'X-Forwarded-Host' => 'files.example.com', + 'X-Forwarded-For' => '203.0.113.9', + ])->assertOk()->json(); + + expect($json['ip'])->toBe('127.0.0.1') + ->and($json['root'])->not->toContain('files.example.com'); +}); + +test('the config key the framework falls back to is wired to TRUSTED_PROXIES', function () { + // Illuminate\Http\Middleware\TrustProxies reads `trustedproxy.proxies` + // when nothing called trustProxies(at:). That is the only path that sees + // a value coming from .env, so the key has to stay spelled this way and + // has to keep reading that variable. Evaluated directly rather than + // through config(), which already holds the value loaded at boot. + $_ENV['TRUSTED_PROXIES'] = '10.0.0.1,10.0.0.2'; + $_SERVER['TRUSTED_PROXIES'] = '10.0.0.1,10.0.0.2'; + + try { + expect(require base_path('config/trustedproxy.php')) + ->toBe(['proxies' => '10.0.0.1,10.0.0.2']); + } finally { + unset($_ENV['TRUSTED_PROXIES'], $_SERVER['TRUSTED_PROXIES']); + } +}); + +test('bootstrap/app.php does not read TRUSTED_PROXIES from the environment', function () { + // Reintroducing this read is the regression: it works when the value is + // a real environment variable (Docker `environment:`), and silently does + // nothing when it comes from .env, which is the documented way to set it. + expect(file_get_contents(base_path('bootstrap/app.php'))) + ->not->toContain('TRUSTED_PROXIES'); +}); diff --git a/tests/Helpers.php b/tests/Helpers.php index 3e43bd02..76447a15 100644 --- a/tests/Helpers.php +++ b/tests/Helpers.php @@ -209,3 +209,39 @@ function activateExternalStorage(): void ])->save(); app(ExternalStorageConfigApplier::class)->flush(); } + +/** + * A public file row, with no bytes behind it — enough for any listing or + * permission assertion that never opens the file. + */ +function publicListingFile(array $overrides = []): File +{ + return File::factory()->create(array_merge([ + 'uploaded_by' => User::factory()->create()->id, + 'name' => 'Report', + 'original_name' => 'report.pdf', + 'path' => '2026/08/'.Str::uuid()->toString().'.pdf', + 'mime_type' => 'application/pdf', + 'size' => 2048, + 'public' => true, + ], $overrides)); +} + +/** + * A real, thumbnailable public image on the faked "files" disk — unlike + * publicListingFile()'s bare PDF row, this one has actual bytes GD can + * decode, needed to exercise the thumbnail-generation path. + */ +function publicListingImageFile(User $uploader): File +{ + test()->actingAs($uploader)->post('/files', [ + 'file' => UploadedFile::fake()->image('photo.jpg', 200, 100), + 'name' => '', + 'description' => '', + ]); + + $file = File::query()->latest('id')->firstOrFail(); + $file->update(['public' => true]); + + return $file; +}