It's common for sites in version control to include a "settings.inc" file in sites/default that is used to version control site configuration, while keeping sensitive information (keys, passwords, etc) in settings.php. When system_requirements() runs, it will automatically change the permissions of the site directory to remove write permissions. Then, when running git pull, the merge will break because write permissions are missing on the directory.
Here's a patch that adds a "drupal_install_fix_file_disable" variable that can be used to disable chmod'ing files. system_requirements() still throws an error, but I think that's desirable. I'd be open to downgrading the requirement to a warning if this variable is set.
If this approach is sound, we'd still need to add an example to settings.php so it's known that the variable exists.
Comment | File | Size | Author |
---|---|---|---|
#93 | 1232572-92.patch | 9 KB | mcdruid |
#93 | interdiff-1232572-91-92.txt | 2.5 KB | mcdruid |
Comments
Comment #1
q0rban CreditAttribution: q0rban commentedYes, this is a common problem, and essentially breaks upgrades during deployment. I think the patch needs work, though, as it looks like drupal_verify_install_file() is used on more things than just the site directory.
Comment #2
deviantintegral CreditAttribution: deviantintegral commentedHere's a different approach that only disables permission checking for the site directory. I've also include the update to default.settings.php.
Comment #3
q0rban CreditAttribution: q0rban commentedThis looks MUCH better. Great job!
Comment #4
kenorb CreditAttribution: kenorb commentedMarked as duplicate: #1630276: The right way to disable changing permissions of sites/default and settings.php
Comment #5
kenorb CreditAttribution: kenorb commentedTested and it works fine.
Comment #6
kenorb CreditAttribution: kenorb commentedWhat about settings.php file:
Should we have another variable 'ignore_site_settings_permissions'?
I'm not interested in changing the permissions on my local, because it's breaking my git environment.
Comment #7
kenorb CreditAttribution: kenorb commentedMy version of patch which includes settings.php file and in my opinion it should show in Reports that the site permissions are not protected. But it's for 7.x
Comment #8
kenorb CreditAttribution: kenorb commented#2: 1232572.2-disable-site-directory-permission-check.patch queued for re-testing.
Comment #9
kenorb CreditAttribution: kenorb commentedVersion against the current 8.x
Comment #10
deviantintegral CreditAttribution: deviantintegral commentedThe 8.x patch needs to be converted to CMI.
Here's a 7.x patch that downgrades the error to a warning if the variable is used. As well, it changes the message for the warning which I think will make sense for 8.x but I'm not sure if we can add that string into 7.x
Comment #11
deviantintegral CreditAttribution: deviantintegral commentedComment #12
chx CreditAttribution: chx commentedThis should be Settings() and not $conf
Comment #13
jenlampton#11: system.install-1232572.11.patch queued for re-testing.
Comment #15
bsztreha CreditAttribution: bsztreha commentedI have problem with that too, i am happy if will be a solution. I think many developers dont need to change settings.php chmod, if visit the report page... GIT, deployment etc..
Comment #16
deviantintegral CreditAttribution: deviantintegral commentedHere's an update against the 7.x patch that also avoids changing permissions during site installs.
Comment #17
deviantintegral CreditAttribution: deviantintegral commentedAnd following the above, let's skip settings.php too.
Comment #18
Elijah LynnPatch in #17 works great, yay!
If this gets committed I fixed a duplicate word in the doc block.
Comment #19
bleen CreditAttribution: bleen commentedThis has been working for us for a little while now. RTBC++
Comment #23
deviantintegral CreditAttribution: deviantintegral commentedHere's a reroll for 8.x, as well as a conversion to use Settings().
Comment #24
deviantintegral CreditAttribution: deviantintegral commentedComment #26
Shivam Agarwal CreditAttribution: Shivam Agarwal commentedWorking now! Great work by #24.
Comment #27
bleen CreditAttribution: bleen at NBCUniversal commented....just a couple grammar improvements in the comment. This patch does not change any functionality so if it passes tests it should go right back to RTBC.
Comment #28
bsztreha CreditAttribution: bsztreha commentedWhat about Drupal 7? Will ever committed to core? #17 looks good
Comment #30
cweagansMan, that's a really long running test.
Comment #32
cweagansRe-uploading patch to run the test on Drupal CI, instead of the legacy testbots.
Comment #34
sriharsha.uppuluri CreditAttribution: sriharsha.uppuluri at Azri Solutions commentedPrevious patch failing to apply.
Comment #36
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #39
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedFixed the failing test.
Comment #40
cweagansI changed the variable name to be more descriptive and moved it to example.settings.local.php, which is probably a reasonable place for this to live by default. It's also now turned on by default in that file.
Comment #41
joachim CreditAttribution: joachim commentedI think it would be good if the docs for the settings also mentioned the other use case for this, which is probably more common: if you're working on Drupal core itself, your git checkout of core gets broken when a git pull changes any of the tracked files in that directory.
Comment #42
cweagans@joachim, This is already covered in the patch, I think:
Comment #43
deviantintegral CreditAttribution: deviantintegral at Lullabot commentedI went to update this to handle the string format changes and fix a comment copy-paste typo, and ended up finding a few new bugs with this and some test cleanup that needed to be done. Compared to #40:
Comment #45
johnennew CreditAttribution: johnennew at Deeson commentedThink the fail was just the interdiff having a .patch extension.
Rerolled the patch in #43 against latest 8.0.x branch and provided an interdiff against #40
Comment #46
cweagansRe-uploading patch without the /sites changes for people using subtree splits of /core + cweagans/composer-patches. Please disregard.
Comment #47
BerdirYes please!
Nitpick review:
Contains \Drupal\...
unnecessary double empty line.
we don't use t() anymore for assertion messages.
description of a method should be < 80 characters.
Checks, Makes, Returns...
nobody has a settings.inc anymore :)
I think this should just say settings.php, as the documented standard these days is to have that committed and have a non-committed settings.local.php
There is no leading hash sign, so just remove this.
Comment #48
joachim CreditAttribution: joachim commentedAll the changes from 48 done.
Comment #50
joachim CreditAttribution: joachim commentedWhoops. Over-enthusiastic find & replace!
Comment #51
BerdirOk, this would be a great thing to have, it always causes a lot of confusion, especially with not very experienced git users.
Not sure if it can get into 8.0.x, leaving that for the committers to decide :)
Comment #52
catchThis feels to me like an 8.1.x -> 7.x change to me. New strings, new setting etc.
Moving there for now. We can always cherry-pick to 8.0.x if we change our minds.
Shouldn't this be max()?
Comment #53
joachim CreditAttribution: joachim commentedYup. REQUIREMENTS_FOO constants are integers, and larger ones are worse, so we should always take the maximum.
Although:
By the time we get to the min() line, $severity has been set to either REQUIREMENT_WARNING or REQUIREMENT_ERROR anyway, so if at the min() line we want REQUIREMENT_ERROR, we can just set that.
But there's more, I think: we have an 'if() elseif()'. So it's possible for $severity to not be set at all at this point -- wouldn't that cause the min() to produce an error?
And finally, this function uses the variable $severity again further on, in a different context. That's a potential bug, I think. Changing this to $settings_permissions_severity would avoid that.
Comment #54
joachim CreditAttribution: joachim commented> Shouldn't this be max()?
> Yup. REQUIREMENTS_FOO constants are integers, and larger ones are worse, so we should always take the maximum.
Looking at the code again, no, min() is correct.
The reasoning is this:
- if the 'skip_permissions_hardening' config is set, then file permissions being writable are merely a warning, not an error, because it's assumed the site administrator has purposefully set this. So remind them of the situation as a warning, but don't make it an error.
- if 'skip_permissions_hardening' isn't set, then it's an error if settings files are writable.
That said, I don't find the use of min() to be really clear. What's actually going on here is:
- is skip_permissions_hardening set? If so, all file permissions issues should be warnings. If not, they're errors.
- check dirs and files and flag any issues, using for each one the level already set
I'll reroll for 8.1.x.
Comment #55
joachim CreditAttribution: joachim commentedI've changed the variable name, changed the logic around the error level, and rebased to 8.1.x.
Comment #56
joachim CreditAttribution: joachim commentedComment #57
deviantintegral CreditAttribution: deviantintegral at Lullabot commentedThe above changes look good to me.
I've replaced SafeMarkup in the test with FormattableMarkup since SafeMarkup is depreciated. Almost there?
Comment #58
Saphyel CreditAttribution: Saphyel commentedWorks like a charm! Tested on my local.
Comment #59
Saphyel CreditAttribution: Saphyel commentedI tested again and if you use $settings['skip_permissions_hardening'] = FALSE; it's still doing the same that TRUE..
I tested with Drush.
Comment #60
BerdirI don't know what you tested, this works fine for me. Note that only certain operations, like visiting admin/reports/status triggers the file permission check and automated attempt at fixing it. With it not set or set to FALSE, write permission is removed for me, with TRUE, I get a warning and it's not changed. Exactly as it should be.
Comment #61
catchCommitted/pushed to 8.1.x, thanks!
Moving to 7.x for backport.
Comment #62
catchComment #64
joachim CreditAttribution: joachim commentedWe should add something about this to our developer documentation on d.org, but I'm not sure where. The only page I can find that mentions using local.settings.php is in the theme guide: https://www.drupal.org/node/1903374
Comment #65
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedRetitling a bit to match what the patch does.
I have to ask though: Instead of adding a variable, are we sure we can't remove this behavior entirely?
It makes sense that Drupal tries to fix this for you at the end of the installer (and I'm linking to a related issue regarding that)...
But doing it randomly during normal site operation whenever the requirements check happens to run - that seems pretty weird.
I guess it's possible that we're trying to protect against the situation where someone went into the operating system and made the file writeable (in order to add something to settings.php, e.g. for a module they just installed) and then forgot to change the permissions back, but why isn't the warning enough in that case?
Comment #66
deviantintegral CreditAttribution: deviantintegral at Lullabot commentedIt looks like the code hasn't changed much since the initial D5 installer patch over at #68926: Installer for Drupal Core.
I am OK with just removing it - but is there any value for this for those using ftp / sftp / rsync to push up Drupal? Shared hosting? I have no idea to be honest, but I also haven't dealt with a small scale site in years. Anyone from the security team have comments on removing this?
Comment #67
esod CreditAttribution: esod at Memorial Sloan Kettering Cancer Center commentedFor use in 8.0.x, I rerolled the patch in #50 against latest 8.0.x branch and provided an interdiff against #50.
Comment #68
joachim CreditAttribution: joachim commentedComment #71
Lennard WesterveldAdding a 7.x fix for this issue a reroll of the latest 7.x patch.
Patch is tested on version: 7.53
Comment #72
Lennard WesterveldOops removed code formatting of the patch...
Comment #75
joachim CreditAttribution: joachim commentedPatch is working well, just a couple of details:
This bit isn't in the D8 patch, so should it be in this backport?
At any rate, the text has typos: 'disableD', and missing a final full stop.
Also, the text that the D8 patch adds to sites/example.settings.local.php should probably be added to default.settings.php for documentation. That already has other developer-only config examples in it, such as theme_debug.
Comment #76
Lennard Westerveld- Variable is now skip_permissions_hardening same as in D8
- Fixed some typo's
- Added variable description to default.settings.php
Comment #77
Lennard WesterveldFor me is it running nicely in several production websites.
Some people more can review this? instead of reviewing my own patch :)
Comment #78
Rene BakxBeen using the #76 patch on development for a few weeks, works like advertised. RTBC for me.
Comment #79
hanoiiRe-roll for 7.64+
Comment #80
ron_s CreditAttribution: ron_s commentedThere is an unnecessary extra line break in the
install_configure_form
changes.See the attached update to #79.
Comment #81
ron_s CreditAttribution: ron_s commentedI'm not having a problem applying the patch in #80... can anyone else confirm if there is an issue?
Comment #82
hkirsman CreditAttribution: hkirsman commentedThe last patch does not work for me on 7.66.
But the #79 does
Comment #83
hanoiiLet's stick with #79 for now, not sure if an extra line needs a re-roll, but otherwise feel free to send a fixed one.
Comment #84
AaronBaumanbump
Comment #85
cman9090 CreditAttribution: cman9090 commentedReroll for 7.78
Comment #86
csmdgl CreditAttribution: csmdgl at MTech, LLC commentedReroll for 7.79
Comment #87
hanoiiRe-roll for 7.81
Comment #88
mcdruidReroll of the D7 patch.
It doesn't look like D8/9 have a stanza in default.settings.php for the
skip_permissions_hardening
setting.I'm not sure if that's deliberate; if not, I think we need a follow-up to add it to D9.
Comment #89
mcdruidWe should backport the changes from #2950851: invalid conf file warnings when skip_permissions_hardening is on to this patch.
Comment #90
mcdruid1st pass at backporting those changes.
Comment #91
mcdruidOops - manual testing revealed a mistake.
It'd be good to have more (any?) automated test coverage here.
Comment #92
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC - I looked very carefully at the logic for the boolean change, but it makes perfect sense.
Comment #93
mcdruidMore manual testing turned up at least one more problem (where
$conf_dir
would be undefined ifskip_permissions_hardening
is set to TRUE).This patch is closer to a straight backport of D9's
system_requirements()
.The logic is unchanged, so still RTBC.
Comment #97
mcdruidThank you everyone!
This backport could do with a CR for D7.
Comment #99
Collins405 CreditAttribution: Collins405 commentedThis caused me a lot of grief - we have a multisite environment that uses bash scripts to create new instances programmatically by cloning the default folder and editing settings.php. Each time cron ran, it would change settings.php to 444 and stop it from being edited by our script, which relies on it being 644.
The fix for anyone running into an issue with Drupal automatically updating the permissions for settings.php is to set this variable... variable_set('skip_permissions_hardening', TRUE);