Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
With #1118520: Add inclusion of a settings.local.php file in settings.php in core, we should probably add a test for an unprotected (writeable) settings.local.php to system_requirements()
. I'll get a patch up shortly.
Comments
Comment #1
TravisCarden CreditAttribution: TravisCarden commentedHere'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:
Multiple problems:
Comment #2
TravisCarden CreditAttribution: TravisCarden commentedOops. I forgot to test for the existence of the file before testing whether it was writeable. Here's a new patch.
Comment #3
sunugh. I was rather thinking of removing this babysitting entirely for D8...
Comment #4
dww<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>
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.
Comment #5
dwwFixed my concern from #4, and did some other simplications and cleanup of the code to make it (IMHO) more readable.
Comment #6
dwwInstead 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...
Comment #7
tstoeckler%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.)
Any reason to not simply use $conf_errors[0]?
Marking "needs work" for the first item.
Comment #8
TravisCarden CreditAttribution: TravisCarden commentedGreat feedback. Here's an updated patch.
Comment #9
dww@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
Comment #10
TravisCarden CreditAttribution: TravisCarden commentedLooks great! Let's take it. :)
Comment #11
sunThe 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.
Comment #12
Dries CreditAttribution: Dries commented@sun: It sounds like you have a problem with
drupal_verify_install_file()
instead ofsystem_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.
Comment #13
sunCreated follow-up: #1616266: Do not automatically rewrite file and directory permissions (only check and ask instead)
Comment #14
catchWorks for me, followed up on the other issue as well. Committed/pushed to 8.x.