Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standalone install - check upload directory is writable #29354

Merged
merged 1 commit into from
Mar 6, 2024
Merged

Standalone install - check upload directory is writable #29354

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions setup/plugins/installFiles/StandaloneCivicrmFilesPath.civi-setup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<?php
/**
* @file
*
* Validate and create the civicrm.files folder.
*/

if (!defined('CIVI_SETUP')) {
exit("Installation plugins must only be loaded by the installer.\n");
}

\Civi\Setup::dispatcher()
->addListener('civi.setup.checkRequirements', function (\Civi\Setup\Event\CheckRequirementsEvent $e) {
\Civi\Setup::log()->info(sprintf('[%s] Handle %s', basename(__FILE__), 'checkRequirements'));
$m = $e->getModel();

$civicrmFilesDirectory = $m->paths['civicrm.files']['path'] ?? '';

if (!$civicrmFilesDirectory) {
$e->addError('system', 'civicrmFilesPath', 'The civicrm.files directory path is undefined.');
}
else {
$e->addInfo('system', 'civicrmFilesPath', sprintf('The civicrm.files directory path is defined ("%s").', $civicrmFilesDirectory));
}

if ($civicrmFilesDirectory && !file_exists($civicrmFilesDirectory) && !\Civi\Setup\FileUtil::isCreateable($civicrmFilesDirectory)) {
$e->addError('system', 'civicrmFilesPathWritable', sprintf('The civicrm files dir "%s" does not exist and cannot be created. Ensure it exists or the parent folder is writable.', $civicrmFilesDirectory));
}
elseif ($civicrmFilesDirectory && !file_exists($civicrmFilesDirectory)) {
$e->addInfo('system', 'civicrmFilesPathWritable', sprintf('The civicrm files dir "%s" can be created.', $civicrmFilesDirectory));
}
});

\Civi\Setup::dispatcher()
->addListener('civi.setup.installFiles', function (\Civi\Setup\Event\InstallFilesEvent $e) {
\Civi\Setup::log()->info(sprintf('[%s] Handle %s', basename(__FILE__), 'installFiles'));
$m = $e->getModel();

$civicrmFilesDirectory = $m->paths['civicrm.files']['path'] ?? '';

if ($civicrmFilesDirectory && !file_exists($civicrmFilesDirectory)) {
Civi\Setup::log()->info('[StandaloneCivicrmFilesPath.civi-setup.php] mkdir "{path}"', [
'path' => $civicrmFilesDirectory,
]);
mkdir($civicrmFilesDirectory, 0777, TRUE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0777 is very open; it makes the uploaded data world-read-writable. If the ancestor dirs do not provide protection against access this could mean some other process (e.g. another website on same server) could write files in the civi upload dir.

Also, I note that the following line calls FileUtil which does the same thing- we don't need that call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my eye, the parameter here looks like a formality necessitated by setting $recursive=TRUE. 0777 is the PHP default, and it's still subject to umask. It's consistent with how CreateTemplateCompilePath does it.

Due to umask handling, mkdir(...0777...) doesn't have the same meaning as chmod(...0777...) (in FileUtil::makeWebWritable()):

  • mkdir() speaks softly: "I'm fine with anything up to 0777, but the local system might prefer to change it via umask."
  • chmod() speaks loudly: "Set this thing to 0777. It will be public!"

The part that's being overly prescriptive is makeWebWritable(). Fixing that would kinda be a different feature, tho, wouldn't it? (e.g. "configurable permissioning"?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full disclosure: I just blindly copied from CreateTemplateCompilePath ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the webserver is able to create the file, can we assume it's the file owner, and so 700 or 770 be ok?

Or is this also called by CLI install, in which case you might be running as a different user. Still in this case 770 and make sure you have a shared group seems better place to get to?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point @totten re mkdir respecting umask.

I think we just ditch the 2nd call that applies 0777 and we're done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(@ufundo) Or is this also called by CLI install, in which case you might be running as a different user.

Quite right.

(@totten) The part that's being overly prescriptive is makeWebWritable(). Fixing that would kinda be a different feature, tho, wouldn't it? (e.g. "configurable permissioning"?)

(@ufundo) Still in this case 770 and make sure you have a shared group seems better place to get to?

(@artfulrobot) I think we just ditch the 2nd call that applies 0777 and we're done.

Ugh, messy area.

Unfortunately, in the wild, 0770, 0700, and inherited-umask are just as likely to leave a broken environment. Even if the person knows about the console-user/web-user split, their primary-group is usually mismatched (myuser:staff or myuser:myuser not myuser:www-data not myuser:apache), and they usually don't have any suitable policy/inheritance mechanism (like sticky bits or umasks or setfacl). And even if they did, it's hard to -preconfigure enough nuance to distinguish "gid/perms for new code files" (re:unzip) and "gid/perms for new data files" (re:cv core:install).

The thing about 0777 for initializing data-folders is.... it just works. It's not awful (in the common contexts of a dedicated web-server or personal workstation or container), but it's not good either (wrt defense-in-depth or consistently-mutually access). For a professional admin, it's usually not what you want. But it's simple; it's a universally recognized quantity; and you can tune post-install.

IMHO:

  • The KISS thing is basically what you have already (call makeWebWritable()).
  • A bonus thing would be an option in the install-model ($model->dataDirMode). Teach makeWebWritable() to abide. The web-based installer and CLI-installer can set this differently. Folks doing system-automation can tune it.
  • Another bonus thing would be to write more detailed detection/training/advice. ("Your source-code is owned by a different user? Do X..." or "Your server has a user named www-data? Do Y...") You would probably target this first as a status-check (which applies to post-install as well as system-restores, migrations, etc).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kiss

Makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks both.

On that basis I'd be quite keen to leave the makeWebWritable for now.. its consistent with the templates_c behaviour and if anyone comes along to improve that they'll pick up this occurence?

\Civi\Setup\FileUtil::makeWebWriteable($civicrmFilesDirectory);
}
});