Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TravisCarden’s picture

Title: Add check for unprotected settings.local.php » Add test for unprotected settings.local.php
Status: Active » Needs review
FileSize
2.86 KB
6.28 KB
12.27 KB

Here's a patch. Since there are now three components to the configuration files test, I changed it to display them in bullets if more than one fails—like we do with messages.

One problem:
status-report-single.gif
Multiple problems:
status-report-multiple.gif

TravisCarden’s picture

Oops. I forgot to test for the existence of the file before testing whether it was writeable. Here's a new patch.

sun’s picture

ugh. I was rather thinking of removing this babysitting entirely for D8...

dww’s picture

Status: Needs review » Needs work

<hat class="security-team">+1 for babysitting. On what basis do you want to remove this? Too much code to maintain? The benefit is a much higher likelihood that sites will be more securely configured. The cost is what, 10 lines of code? Seems like a totally obvious win to me.</hat>

+    if (!drupal_verify_install_file(conf_path() . '/settings.php', FILE_EXIST|FILE_READABLE|FILE_NOT_WRITABLE)) {
+      $conf_errors[] = $t("The file %file is not protected from modifications and poses a security risk. You must change the file's permissions to be non-writable.", array('%file' => conf_path() . '/settings.php'));
+    }
+    if (file_exists(conf_path() . '/settings.local.php') && !drupal_verify_install_file(conf_path() . '/settings.local.php', FILE_EXIST|FILE_READABLE|FILE_NOT_WRITABLE)) {
+      $conf_errors[] = $t("The file %file is not protected from modifications and poses a security risk. You must change the file's permissions to be non-writable.", array('%file' => conf_path() . '/settings.local.php'));
+    }

This seems a bit too much copy-paste code for me. I don't mind checking that settings.php exists, too, if that means we can share this code.

dww’s picture

Status: Needs work » Needs review
FileSize
2.67 KB

Fixed my concern from #4, and did some other simplications and cleanup of the code to make it (IMHO) more readable.

dww’s picture

Instead of calling conf_path() over and over, maybe we should just do that once to create a $full_path variable and then keep using that...

tstoeckler’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/system.install
@@ -226,29 +226,37 @@ function system_requirements($phase) {
+      $full_path = conf_path() . '/' . $conf_file;
...
+        $conf_errors[] = $t("The file %file is not protected from modifications and poses a security risk. You must change the file's permissions to be non-writable.", array('%file' => conf_path() . $full_path));

%file now gets conf_path() twice. (It is already contained in $full_path.)

Also, if we're already going with a local variable, why not simply define $conf_path upfront to avoid calling it a bunch of times. (I realize this is debatable and very minor.)

+++ b/core/modules/system/system.install
@@ -226,29 +226,37 @@ function system_requirements($phase) {
+        $description = array_pop($conf_errors);

Any reason to not simply use $conf_errors[0]?

Marking "needs work" for the first item.

TravisCarden’s picture

Status: Needs work » Needs review
FileSize
1.5 KB
2.67 KB

Great feedback. Here's an updated patch.

dww’s picture

@tstoeckler: Thanks for catching my 1:15am coding blunder there. ;)

#8 looks good to me, and could be RTBC, but here's another version that does what tsoeckler suggested of just calling conf_path() once at the start of this little code block. Probably a meaningless performance optimization, but it can't hurt.

Cheers,
-Derek

TravisCarden’s picture

Status: Needs review » Reviewed & tested by the community

Looks great! Let's take it. :)

sun’s picture

The reason I want to ditch this is that drupal_verify_install_file() does not only check the permission masks; it also goes ahead and tries to "fix" them for me. This is pure babysitting. And it constantly gets in my way, rewriting the permissions of my files and directories. No, Drupal, I changed those permissions purposively, and you have no effin' business to rewrite them and no one asked you to do that, kthxbye.

Dries’s picture

@sun: It sounds like you have a problem with drupal_verify_install_file() instead of system_requirements() which this file touches. It is important to give people a warning when there is a security risk.

A better pattern may be this: if Drupal identifies that the files are not properly secured, there could be a "Fix permissions" link or button in the warning message, that executes a callback that will try to fix the permissions. Like that, Drupal doesn't automatically fix the permissions but still makes it very easy for people to (try and) get their permissions fixed.

sun’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed

Works for me, followed up on the other issue as well. Committed/pushed to 8.x.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.