mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-17 17:15:08 +00:00
a5496d24cd
`FileDelivery::describe()` from a console returned
`{"method":"php","detected":true}` on every installation, whatever its web
server. detect() reads SERVER_SOFTWARE, which only exists inside a
request, so a console process has nothing to look at and falls to the
`php` default — and `detected: true` then vouched for it.
The value is right for that process and wrong as a statement about the
installation, which is how anybody running it from `artisan tinker` will
read it. Somebody verifying a healthy nginx tenant hit exactly that, spent
an afternoon on it, and only recognised it as an artefact of *where* the
question was asked after reading `nginx -T` in the container.
There is now a third field. `observed` is false only outside a request,
where `method` is a default rather than a finding. Both screens that read
this run in a request and always see true; it exists for whoever asks from
a shell, which is the one place the answer could mislead.
The two web paths are unchanged and were never wrong — `projectsend:status`
does not report delivery at all, so no fleet ever reported this
incorrectly. What was wrong was a confident answer to a question that
could not be answered from where it was asked.
Three tests: a console reading says not observed and still says php,
because php is what that process would actually do; a reading during a
request observes nginx; and a stated method is observed wherever it is
read, since a decision needs nothing detected to be true.
Found by the session verifying the 2.4.0 canary, not by me.
253 lines
9.8 KiB
PHP
253 lines
9.8 KiB
PHP
<?php
|
|
|
|
declare(strict_types=1);
|
|
|
|
use App\Models\User;
|
|
use App\Modules\Files\Delivery\FileDelivery;
|
|
use App\Modules\Files\Models\File;
|
|
use Illuminate\Support\Facades\Storage;
|
|
|
|
/**
|
|
* How a file's bytes leave the server.
|
|
*
|
|
* The fast path answers with an empty body and a header telling the web
|
|
* server to send the file. A server that does not recognise that header
|
|
* sends the empty body instead, which is a 0-byte download with every
|
|
* other page working perfectly — the shape of
|
|
* https://github.com/projectsend/projectsend/issues/1765, where an Apache
|
|
* install had broken thumbnails and empty downloads.
|
|
*
|
|
* So these are mostly about the *body*: the assertion that catches the
|
|
* bug is "the bytes actually arrived", not "the header said the right
|
|
* thing".
|
|
*/
|
|
beforeEach(function () {
|
|
Storage::fake('files');
|
|
$this->admin = User::factory()->create();
|
|
});
|
|
|
|
/** A file whose bytes are really on the fake disk. */
|
|
function storedFile(string $contents = 'the actual bytes'): File
|
|
{
|
|
$file = File::factory()->create(['size' => strlen($contents), 'mime_type' => 'application/pdf']);
|
|
Storage::disk('files')->put($file->path, $contents);
|
|
|
|
return $file;
|
|
}
|
|
|
|
function deliverAs(string $method): void
|
|
{
|
|
config(['projectsend.file_delivery' => $method]);
|
|
}
|
|
|
|
test('nginx is handed the file and PHP sends no bytes', function () {
|
|
deliverAs('nginx');
|
|
$file = storedFile();
|
|
|
|
$response = $this->actingAs($this->admin)->get("/files/{$file->id}/download");
|
|
|
|
$response->assertOk()->assertHeader('X-Accel-Redirect', '/protected-files/'.$file->path);
|
|
expect($response->getContent())->toBe('');
|
|
});
|
|
|
|
test('PHP streaming actually sends the bytes', function () {
|
|
// The regression that matters. Before this existed, an installation
|
|
// whose server did not understand X-Accel-Redirect served this empty.
|
|
deliverAs('php');
|
|
$file = storedFile('the actual bytes');
|
|
|
|
$response = $this->actingAs($this->admin)->get("/files/{$file->id}/download");
|
|
|
|
$response->assertOk()
|
|
->assertHeaderMissing('X-Accel-Redirect')
|
|
->assertHeader('Content-Disposition', 'attachment; filename="'.$file->original_name.'"');
|
|
|
|
expect($response->streamedContent())->toBe('the actual bytes');
|
|
});
|
|
|
|
test('PHP streaming answers a range request rather than resending the file', function () {
|
|
// What makes seeking through a long video work. nginx does this for
|
|
// itself on the fast path, so a hand-rolled readfile here would break
|
|
// scrubbing on exactly the installations this fallback exists for.
|
|
deliverAs('php');
|
|
$file = storedFile('0123456789');
|
|
|
|
$response = $this->actingAs($this->admin)
|
|
->get("/files/{$file->id}/download", ['Range' => 'bytes=2-5']);
|
|
|
|
expect($response->getStatusCode())->toBe(206)
|
|
->and($response->streamedContent())->toBe('2345');
|
|
});
|
|
|
|
test('X-Sendfile is handed an absolute path, not a URL path', function () {
|
|
// The two headers are not interchangeable: nginx maps a URL through an
|
|
// internal location, Apache and LiteSpeed open a filesystem path.
|
|
// Renaming the header without changing the value is the obvious way to
|
|
// "add Apache support" and produces a second broken install.
|
|
deliverAs('xsendfile');
|
|
$file = storedFile();
|
|
|
|
$response = $this->actingAs($this->admin)->get("/files/{$file->id}/download");
|
|
|
|
$response->assertOk()->assertHeaderMissing('X-Accel-Redirect');
|
|
|
|
expect($response->headers->get('X-Sendfile'))
|
|
->toBe(Storage::disk('files')->path($file->path))
|
|
->and($response->getContent())->toBe('');
|
|
});
|
|
|
|
test('auto uses the fast path when the server says it is nginx', function () {
|
|
deliverAs('auto');
|
|
$file = storedFile();
|
|
|
|
$response = $this->actingAs($this->admin)
|
|
->withServerVariables(['SERVER_SOFTWARE' => 'nginx/1.24.0'])
|
|
->get("/files/{$file->id}/download");
|
|
|
|
$response->assertOk()->assertHeader('X-Accel-Redirect', '/protected-files/'.$file->path);
|
|
});
|
|
|
|
test('auto falls back to PHP on a server it cannot hand files to', function () {
|
|
// Apache, and the reason the issue was filed. Slow beats empty.
|
|
deliverAs('auto');
|
|
$file = storedFile('apache bytes');
|
|
|
|
$response = $this->actingAs($this->admin)
|
|
->withServerVariables(['SERVER_SOFTWARE' => 'Apache/2.4.62 (AlmaLinux)'])
|
|
->get("/files/{$file->id}/download");
|
|
|
|
$response->assertOk()->assertHeaderMissing('X-Accel-Redirect');
|
|
expect($response->streamedContent())->toBe('apache bytes');
|
|
});
|
|
|
|
test('auto never chooses X-Sendfile on its own', function () {
|
|
// mod_xsendfile also needs XSendFilePath to allow the storage
|
|
// directory, which cannot be seen from here. Choosing it because the
|
|
// module might be loaded would swap a silent failure an administrator
|
|
// can diagnose from the dashboard for one nobody can.
|
|
deliverAs('auto');
|
|
$file = storedFile();
|
|
|
|
$response = $this->actingAs($this->admin)
|
|
->withServerVariables(['SERVER_SOFTWARE' => 'Apache/2.4.62'])
|
|
->get("/files/{$file->id}/download");
|
|
|
|
$response->assertHeaderMissing('X-Sendfile');
|
|
});
|
|
|
|
test('an unrecognised setting falls back to detection rather than breaking every download', function () {
|
|
// A typo in an environment variable should cost speed, not the
|
|
// installation's downloads.
|
|
deliverAs('nginx-x-accel-redirect');
|
|
$file = storedFile('still works');
|
|
|
|
$response = $this->actingAs($this->admin)
|
|
->withServerVariables(['SERVER_SOFTWARE' => 'Apache/2.4.62'])
|
|
->get("/files/{$file->id}/download");
|
|
|
|
$response->assertOk();
|
|
expect($response->streamedContent())->toBe('still works');
|
|
});
|
|
|
|
test('a path trying to climb out of the storage directory is refused', function () {
|
|
deliverAs('nginx');
|
|
$file = storedFile();
|
|
// Paths come from rows this application wrote, so this cannot happen
|
|
// today. It is refused anyway because the cost of being wrong once is
|
|
// handing over any file the web server can read — and nginx resolves
|
|
// `..` in the URL it is given exactly as happily as PHP would.
|
|
$file->forceFill(['path' => '../../../../etc/passwd'])->save();
|
|
|
|
$this->actingAs($this->admin)->get("/files/{$file->id}/download")->assertNotFound();
|
|
});
|
|
|
|
test('a path carrying a control character is refused, on every method', function () {
|
|
// Not traversal -- header injection. The path is written into
|
|
// X-Accel-Redirect or X-Sendfile, and a CR or LF in a header value
|
|
// splits the response. PHP's header() refuses to emit one, so the real
|
|
// effect is a 500 on every download, preview and thumbnail of that
|
|
// file: a file permanently broken by its own name.
|
|
//
|
|
// Paths are generated here as Y/m/{uuid}.{ext} -- but the extension is
|
|
// not generated, it comes from the uploader's filename, and on a
|
|
// migrated installation from a v1 database.
|
|
$file = storedFile();
|
|
$file->forceFill(['path' => "2026/08/x.pd\r\nX-Injected: yes"])->save();
|
|
|
|
foreach (['nginx', 'xsendfile', 'php'] as $method) {
|
|
deliverAs($method);
|
|
|
|
$this->actingAs($this->admin)->get("/files/{$file->id}/download")->assertNotFound();
|
|
}
|
|
});
|
|
|
|
test('PHP streaming reports a missing file as missing rather than failing', function () {
|
|
deliverAs('php');
|
|
$file = File::factory()->create();
|
|
|
|
$this->actingAs($this->admin)->get("/files/{$file->id}/download")->assertNotFound();
|
|
});
|
|
|
|
test('previews, thumbnails and zips travel the same way downloads do', function () {
|
|
// The four routes that hand over bytes each used to decide this for
|
|
// themselves, and all four hard-coded nginx. Centralising them is the
|
|
// fix; this is what stops one drifting back out.
|
|
deliverAs('php');
|
|
|
|
$image = File::factory()->create(['mime_type' => 'image/png', 'original_name' => 'shot.png']);
|
|
Storage::disk('files')->put($image->path, base64_decode(
|
|
'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg=='
|
|
));
|
|
|
|
$response = $this->actingAs($this->admin)->get("/files/{$image->id}/preview");
|
|
|
|
$response->assertOk()->assertHeaderMissing('X-Accel-Redirect');
|
|
expect(strlen($response->streamedContent()))->toBeGreaterThan(0);
|
|
});
|
|
|
|
/*
|
|
|--------------------------------------------------------------------------
|
|
| Asking from a console
|
|
|--------------------------------------------------------------------------
|
|
|
|
|
| detect() reads SERVER_SOFTWARE, which only exists inside a request. A
|
|
| console process therefore has nothing to look at and falls to the `php`
|
|
| default — correct for that process, and wrong as a statement about the
|
|
| installation, which is exactly how somebody running `artisan tinker` on
|
|
| a healthy nginx box will read it. It cost somebody an afternoon before
|
|
| it was recognised as an artefact of where the question was asked.
|
|
*/
|
|
|
|
test('a console reading says the method was not observed', function () {
|
|
config()->set('projectsend.file_delivery', null);
|
|
|
|
$described = app(FileDelivery::class)->describe();
|
|
|
|
expect($described['observed'])->toBeFalse()
|
|
// Still `php`, because that is what this process would actually do.
|
|
->and($described['method'])->toBe('php');
|
|
});
|
|
|
|
test('a reading taken during a request is observed', function () {
|
|
config()->set('projectsend.file_delivery', null);
|
|
|
|
request()->server->set('SERVER_SOFTWARE', 'nginx/1.27.0');
|
|
|
|
$described = app(FileDelivery::class)->describe();
|
|
|
|
expect($described['observed'])->toBeTrue()
|
|
->and($described['method'])->toBe('nginx');
|
|
});
|
|
|
|
// An explicit setting is somebody's decision and needs nothing observed to
|
|
// be true — the value stands wherever it is read from.
|
|
test('a stated method is always observed, console or not', function () {
|
|
config()->set('projectsend.file_delivery', 'xsendfile');
|
|
|
|
$described = app(FileDelivery::class)->describe();
|
|
|
|
expect($described['method'])->toBe('xsendfile')
|
|
->and($described['detected'])->toBeFalse()
|
|
->and($described['observed'])->toBeTrue();
|
|
});
|