mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-12 06:48:55 +00:00
b838036a9a
FILES_WEB_SERVER_READABLE asks for 0755 on the directories a download has to be traversed through, and asks for it from a key that is never consulted. FilesystemManager::createLocalDriver passes `directory_visibility ?? visibility ?? private` to PortableVisibilityConverter::fromArray() as the default visibility for directories. This disk sets `visibility` to public two lines above, and no `directory_visibility`, so directories are public and the converter reads `dir.public`. The configuration names only `dir.private`. The mode is 0755 regardless, because 0755 is Flysystem's default for a public directory -- the right answer from the wrong place. Adding `directory_visibility` to this disk, or a change to that default, is all it would take for the flag to stop doing what it says. Measured on main, with the flag on: dir.private 0755 → 0750 directory stays 0755 (nothing reads it) dir.public 0755 → 0750 directory becomes 0750 (this is the key) Both are named now, so the intent survives either way round. FilePermissionsTest could not have caught this, because it was not testing this configuration. filesDiskWith() restated the shipped branch inline, verbatim down to the 0755, so it went on passing against its own copy however the real one changed. It now requires config/filesystems.php and replaces only the root, which is what makes the mutation above visible to it. Two more things in the same helper, both about the suite rather than the subject: the scratch root is per worker now (Tests\TestCase does the same for upload parts, and eight workers sharing one directory means one worker's afterEach deletes another's tree mid-test), and it is cleared before each test as well as after, so a killed run does not poison the next one.
120 lines
4.3 KiB
PHP
120 lines
4.3 KiB
PHP
<?php
|
|
|
|
declare(strict_types=1);
|
|
|
|
// What the `files` disk writes to disk, in modes rather than in config keys.
|
|
//
|
|
// A download is not served by PHP: PHP authorizes it and hands the web server
|
|
// the path with X-Accel-Redirect. So on a host where those are different
|
|
// users the mode on a directory decides whether downloads work at all, and
|
|
// nothing else on the site notices (#1668).
|
|
//
|
|
// The umask cases are the point of this file. `visibility` makes Flysystem
|
|
// chmod each file after writing it, so it holds regardless; a directory is
|
|
// created by mkdir(), which masks its mode argument, so the same setting is a
|
|
// ceiling there rather than a guarantee. That asymmetry is invisible from the
|
|
// configuration and is exactly what someone would "simplify" away.
|
|
|
|
use Illuminate\Support\Env;
|
|
use Illuminate\Support\Facades\File;
|
|
use Illuminate\Support\Facades\ParallelTesting;
|
|
use Illuminate\Support\Facades\Storage;
|
|
|
|
/**
|
|
* This worker's scratch root.
|
|
*
|
|
* Per worker, like the upload parts directory in Tests\TestCase: eight of
|
|
* them run this file at once, and the afterEach below deletes the tree it
|
|
* is given. Shared, one worker's cleanup lands in the middle of another
|
|
* worker's test, and the failure is a mode read from a directory that was
|
|
* removed underneath it.
|
|
*/
|
|
function filesPermissionRoot(): string
|
|
{
|
|
return storage_path('app/files-permission-test/w'.(ParallelTesting::token() ?: '0'));
|
|
}
|
|
|
|
/**
|
|
* Points the `files` disk at that root, configured the way
|
|
* config/filesystems.php configures it for the given flag.
|
|
*
|
|
* The configuration is *read from that file*, with only the root replaced.
|
|
* Restating its branch here — which is what this used to do, verbatim down
|
|
* to the 0755 — meant the test went on passing against its own copy after
|
|
* somebody changed the shipped one, which is the single thing it exists to
|
|
* notice.
|
|
*/
|
|
function filesDiskWith(bool $webServerReadable): string
|
|
{
|
|
$root = filesPermissionRoot();
|
|
|
|
Env::getRepository()->set('FILES_WEB_SERVER_READABLE', $webServerReadable ? 'true' : 'false');
|
|
|
|
// Required rather than read through config(), so the env() call in it
|
|
// is evaluated now, against the flag just set.
|
|
$shipped = require base_path('config/filesystems.php');
|
|
|
|
config(['filesystems.disks.files' => [...$shipped['disks']['files'], 'root' => $root]]);
|
|
|
|
Storage::forgetDisk('files');
|
|
|
|
return $root;
|
|
}
|
|
|
|
function modeOf(string $path): string
|
|
{
|
|
clearstatcache(true, $path);
|
|
|
|
return substr(sprintf('%o', fileperms($path)), -4);
|
|
}
|
|
|
|
beforeEach(function () {
|
|
$this->originalUmask = umask();
|
|
|
|
// Also before: a run killed mid-test leaves its tree behind, and these
|
|
// cases read modes off directories they expect to have created.
|
|
File::deleteDirectory(filesPermissionRoot());
|
|
});
|
|
|
|
afterEach(function () {
|
|
umask($this->originalUmask);
|
|
File::deleteDirectory(filesPermissionRoot());
|
|
Env::getRepository()->clear('FILES_WEB_SERVER_READABLE');
|
|
Storage::forgetDisk('files');
|
|
});
|
|
|
|
test('by default an upload lands in a directory only its owner can traverse', function () {
|
|
umask(0022);
|
|
$root = filesDiskWith(webServerReadable: false);
|
|
|
|
Storage::disk('files')->put('2026/08/report.pdf', 'contents');
|
|
|
|
// 0700: a web server running as another user cannot open anything
|
|
// underneath this, whatever the file's own mode says.
|
|
expect(modeOf($root.'/2026/08'))->toBe('0700');
|
|
});
|
|
|
|
test('the flag opens both the file and the directory for another user', function () {
|
|
umask(0022);
|
|
$root = filesDiskWith(webServerReadable: true);
|
|
|
|
Storage::disk('files')->put('2026/08/report.pdf', 'contents');
|
|
|
|
expect(modeOf($root.'/2026/08/report.pdf'))->toBe('0644')
|
|
->and(modeOf($root.'/2026/08'))->toBe('0755');
|
|
});
|
|
|
|
// Both halves of the same claim, on a pool that denies group and other by
|
|
// default. The file still comes out readable because it is chmod'ed after the
|
|
// write; the directory does not, because mkdir() masked it — so the flag alone
|
|
// does not rescue a host like this and INSTALL.md has to say so.
|
|
test('a restrictive umask still caps the directory, though not the file', function () {
|
|
umask(0077);
|
|
$root = filesDiskWith(webServerReadable: true);
|
|
|
|
Storage::disk('files')->put('2026/08/report.pdf', 'contents');
|
|
|
|
expect(modeOf($root.'/2026/08/report.pdf'))->toBe('0644')
|
|
->and(modeOf($root.'/2026/08'))->toBe('0700');
|
|
});
|