Automatic creation of .htaccess files can cause errors

Seems reasonable.

Can we replace the preg_match() with a strstr()?

Also, a test would be nice.

Actually, I think I remember reading that strpos() is faster than strstr(). But either is definitely faster than preg_match(), according to the docs: http://ca2.php.net/preg_match.

See, this is why I'd like tests. :)

Because that code will fail to execute if "Apache" is in the first position (0) in the string. Need to compare to !== FALSE.

Requesting retest.

I don't think we can apply the blanket rule of Apache=.htaccess and non-Apache=no .htaccess.
There are, and will continue to be, exceptions to this rule (eg. IIS + ISAPI_Rewrite supports .htaccess).

Perhaps the best way forward is allowing people to opt-out of enforced .htaccess files.
Patch attached.

For info, I'm using Drupal on an IIS7.5 host with WebsitePanel and any form of .htaccess file breaks the website.
I've used this patch and the following in settings.php to fix the issue:
$conf['no_htaccess'] = TRUE;

Moved to 8.

Added "Needs tests" tag.

Applied Issue summary standards and renamed.

Applying Issue summary standards

Added original title for ease of reference.

Feeling like a bit of a one man army on this one, but oh well. ;)

Test patch - should fail (for the right reason).

Combined patch.

Upgraded to major.

Can we make the variable a positive one, right now it's a double negative.

Like variable_get('automatically_generate_htaccess', TRUE);

Otherwise looks fine and using a configuration setting for this sounds seems better than trying to figure it out.

Thanks catch!!

Rerolled to be a little more up-beat (positive).

First is test only and should fail twice (for the right reason).
Second is test and fix.

Cleanup remaining tasks and add test path.

Sweet!!

Issue summary updated.

Sorry one more thing here:

+ // Don't create .htaccess files if 'auto_create_htaccess' variable is FALSE.

This needs an explanatory note as to why you might want to disable it, although probably in the main function docblock.

New positive patch.

Adding novice.
Removing cross post "Needs tests".

Improved comments as per catch's suggestion.

At the moment, I am unable to persuade either Drupal or Apache to create an .htaccess file in the public:// directory using the 8.x clean install. Status of test thus far:

1. clean Drupal install with a module that allows file uploads (eg. http://drupal.org/project/media) ... OK
2. checked sites/default/files for no .htaccess present ... OK
3. upload a file to public ... OK
4. see that a .htaccess file has been created in the public filesystem path (sites/default/files) ... FAIL

• Added $conf['auto_create_htaccess'] = TRUE; to settings.php
• Repeat step 2 ... FAIL (no .htaccess)
• Repeat step 2 with patch applied ... OK
• checked for .htaccess in sites/default/files ... not present ... FAIL

I am trying to track down the problem. I am not sure if it's in the WAMP stack, or Drupal as yet.

@Paul Simard: Thanks for giving testing this a crack!! ;) Perhaps the test path I wrote is a bit flawed? Will have a play and see.

Improved test path.

Better test path.

Yep. The test path was *completely* wrong. My bad.
In fact, you got further than I did - I don't even know how you installed Media 7.x onto Drupal 8.x. ;)

Updated it in issue summary.

There's a nice automated test here that fails correctly without the fix applied, so no need for manual testing.

There was no need to install Media on 8.x until and unless the patch passed using only core in a standard install. I just added a file field to the Article content type, and uploaded files that way.

Since it didn't pass, i.e. no .htaccess was created, either with or without the patch...

@catch I don't know if the previous was aimed at my inbox (re: automated test). If so, thanks for the info. If not, ignore this line.

Paul

@Paul Simard: Ah. I must admit, I wasn't aware that there was a way of uploading files with just core - I'm still only relatively new to Drupal. Good to know. ;)

Turns out uploading files doesn't "prepare" the directory anyways which is why you didn't get a .htaccess file. Oddly enough, changing colour scheme does so I've updated the test path accordingly.

Up to you whether you test this any further or not as catch reckons it's good to go as is. Thanks again!! ;)

Akaoni,

In all fairness, it was catch who reminded me of 'File' module's presence in core. I merely added the extra field. I'll bow to catch's greater experience and Drupal expertise, as far as continued testing goes, as he is far higher on the food chain than I. I may spend a bit of time in the code trying to ferret out its secrets, though, in my quest to learn Drupal myself. (I'm still very low on the food chain, myself.)

Let me know if I can help any further, as time permits, I'd be interested.

Paul

Instead of a global killswitch variable, wouldn't it be better to check for the existence of Apache and make this behavior automatic?

  $apache = strpos($_SERVER['SERVER_SOFTWARE'], 'Apache') !== FALSE;
  if ($apache) { ... }

I understand there are exceptions to this rule (e.g. some IIS configurations support .htaccess files), but does that really hold up as an argument against automatic behavior? Let's brainstorm about it a bit more ...

I'd think that the automatic behavior avoids errors/problems, whereas the global killswitch variable doesn't. Because the variable defaults to generating .htaccess files (i.e. variable_get('auto_create_htaccess', TRUE)), it still causes problems on installations that don't support .htaccess files. Hence, it requires people to research the problem, and to find out about the existence of the global killswitch variable. That is non-trivial and most people might have given up by the time they learn about the global killswitch variable ... in other words, I find the variable kind of a poor solution.

Sounds like this needs some more work then

Automatic detection with no killswitch is going to break things for those IIS installs, all I can think of as a compromise is this:

Automatically create the .htaccess if it's Apache.

Don't create it automatically for anything else, however, warn in system_requirements() if the .htaccess doesn't exist. For sites that really don't want it, keep the killswitch which will mainly function to disable the system_requirements() warning.

@Dries: Good point on automation being much better for users. I agree with catch though, in that we still need a killswitch/override. There's so much variation out there on how web servers and even web applications handle .htaccess files. As such, let's automate what we know and allow people to override for the edge cases.

New test only patch as described by catch (should fail twice for the right reason).

New test and fix patch as described by catch.

+++ b/includes/file.incundefined
@@ -470,15 +470,45 @@ function file_ensure_htaccess() {
+  if (!variable_get('auto_create_htaccess', stripos($_SERVER['SERVER_SOFTWARE'], 'apache') !== FALSE)) {
+    return;

This and the next check:

+++ b/modules/system/system.installundefined
@@ -356,6 +356,13 @@ function system_requirements($phase) {
+    // variable hasn't been overridden.
+    if (stripos($_SERVER['SERVER_SOFTWARE'], 'apache') === FALSE && is_null(variable_get('auto_create_htaccess', NULL))) {
+      $requirements['file system']['description'] = $t('Your web server has been detected as different from Apache so creation of .htaccess files is not being enforced. As such your file system may not be secure. See the !link for more information.', array('!link' => l('API documentation', 'http://api.drupal.org/api/drupal/includes--file.inc/function/file_create_htaccess')));

The different arguments to variable_get() can make things harder to debug, as well as likely being incompatible as soon as CMI comes on - since we'll no longer pass defaults every time we check a variable (at least I hope so).

1 days to next Drupal core point release.

New patch fixes different variable_get() arguments and cleans things up a bit.
Test code hasn't changed.

Made it more better after IRC chat with catch.
Please let this be the last one... ;)

OK this looks good to me. It's hard to explain what's going on here, but the alternative is completely failing out on people and/or risking a security issue on sites that aren't using apache but are relying on .htaccess.

one, the variable_get is repeated needlessly. Two, I think the api.drupal.org link needs to be file_save_htaccess not file_create_htaccess otherwise we are good here.

@catch, chx Thanks, guys!!

chx, another patch, just for you. ;)

Good to go.

+++ b/modules/system/system.install
@@ -356,6 +356,15 @@ function system_requirements($phase) {
+      $requirements['file system']['description'] = $t('Your web server has been detected as different from Apache so creation of .htaccess files is not being enforced. As such your file system may not be secure. See the !link for more information.', array('!link' => l('API documentation', 'http://api.drupal.org/file_save_htaccess')));

This is potentially one of the first messages/warnings/errors that users are going to see when installing Drupal.

It's unlikely that users are going to (be able to) switch to another web server when reading this, so the entire drama about "Apache" is information clutter.

"Files on this web server may not be stored securely. Configure your server securely."

Give or take grammar errors. ;)

We also need to link to a handbook page, not api.d.o.

-2 days to next Drupal core point release.

Link to new patch.

New fix.

Issue summary updated.

In **further** IRC discussions with sun we could:

• Expand the File Directories section of the handbook (http://drupal.org/node/394704) and link to that instead of api.drupal.org
• Add automated creation of web.config file in private:// for IIS to prevent direct access

*sigh* The rabbit hole gets deeper...

Glad to see sun pick up on this very wordy warning message. I think it would be useful to use the ".htaccess file" keyword somewhere in this message. Maybe the link in suns' example could be "Secure your server with .htaccess" or similar:

Files on this web server may not be stored securely. [link]Secure your server with .htaccess files[/link]

This approach looks good.

+++ b/includes/file.inc
@@ -470,15 +470,46 @@ function file_ensure_htaccess() {
+  // Don't create .htaccess files if auto_create_htaccess variable is FALSE.
+  // For non-Apache web servers, don't create .htaccess files unless
+  // auto_create_htaccess variable has been explicitly set to TRUE.
+  $not_apache = stripos($_SERVER['SERVER_SOFTWARE'], 'apache') === FALSE;
+  $htaccess_variable = variable_get('auto_create_htaccess');
+  if ($htaccess_variable === FALSE || ($not_apache && !isset($htaccess_variable))) {
+    return;
+  }

Instead of checking this over and over again, why don't we set variable_get('auto_create_htaccess') at install time? Then, we can streamline this code to just check variable_get('auto_create_htaccess'). I think that would streamline the code a bit.

Just a reroll of Akaoni's patch from above to apply to the /core directory structure. Still needs changes based on comments from Sun in 44.

Thanks, rbayliss.

Unfortunately I can't commit anymore time to this one. :(
Hopefully someone can pick it up or we settle for a less ideal, but implementable now solution.

Includes the suggestions in #44 (sun), #45 (Akaoni), and #46 (yoroy).
("Files on this web server may not be stored securely. Secure your server with .htaccess files.")

I think that the message is fine in #50 but we still need to apply/discuss #47

Here is a new patch, which sets the variable in file.install per #47.

0 byte patch.

Oops. We should make the bot complain about that maybe. New patch attached. Note that I did not add a hook_uninstall() because so far as I can tell, File module can not be uninstalled.

Doing that in hook_install() seems like it would cause problems if you ever move your site to a different server. I think it's better to just check the server software each time (like in the earlier patches). It's not like this is code which runs often...

***

Overall, I'm somewhat concerned about the security implications of this patch. Currently, Drupal guarantees that if your server is configured to support .htaccess, it will provide this protection in the files directory. But this patch is going to make this security feature "opt-in" for anyone who isn't running Apache. I don't think that's a good change.

I think we need to go back to something like #22, which forces you to opt-out if you are having problems. The concern with that was that it's not discoverable, but couldn't we use a status report message to make it discoverable (i.e. combine the two approaches)? If someone is running into trouble with their site, they'll probably go to the status report page eventually, and if the warning there links to somewhere which tells them how to turn off .htaccess protection, we get the best of both worlds.

Although... that all depends on the person being able to make it to the status report page in the first place :) This issue refers to "errors" caused by .htaccess when using IIS + WebsitePanel, but I don't see any explanation of what the errors are. That seems like useful information for this issue. I did a little searching and came up with http://forum.arvixe.com/smf/general/installed-drupal-dotnetpanel-iis-mod... but I'm not even sure that's the same problem; it refers to a conflict between Drupal's .htaccess and one which is automatically created by the control panel software. If that's the problem then the solution may be different also.

Instead of saying that anything not apache gets a .htaccess file created why not instead list the known exceptions? I did not do this because I'm not sure which ones will create the error.
I also updated this to default to creating the .htaccess file and assume that admin can turn it off if they need to.
Fix a typo in the install function.

Patch in #57 doesn't even apply anymore to current D8 (2013-03-09).
Can we remove the "novice" tag from this issue in the meantime?

Removing novice tag until its decided what approach will be used.

Not RTbC.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Moving to 8.1 for now. It might be accepted into 8.0 since it is classified as a bug if somebody wants to pick this back up.

There's no new API here, so it could feasibly be done in a patch release, moving back for now.

This little patch just adds Options +FollowSymLinks to .htaccess for those who want to add it using composer patch.

EDIT: Mike Potter has a working solution to this issue: https://github.com/acquia/blt/issues/1135#issuecomment-285404408

Here is my application of his advice: https://github.com/rjzaar/opencourse/blob/8.6.x/composer.json
and the patch: https://github.com/rjzaar/opencourse/blob/8.6.x/scripts/Add-FollowSymLin...

Here's a patch that builds on #2620304: htaccess functions should be a service to add a settings check to see if auto create is enabled/disabled.

This came up as a daily #bugsmash triage target.

The filename of the latest patch is "360057-combined-74.patch" and the comment says:

Here's a patch that builds on #2620304: htaccess functions should be a service to add a settings check to see if auto create is enabled/disabled.

So I think that includes all of what was committed for #2620304 plus the additional stuff to satisfy the needs of this issue. If @kim.pepper still has the Git branches lying around and can untangle that from what's already committed, great. Otherwise, someone will have to do that manually (which will probably be interesting / fun). 😉

Re-wrote this for Drupal 9.4.x so there's no real useful interdiff.

Fixes incorrect comment.

Fix wrong service in constructor.

@kim.pepper, thanks for updating the patch.

Some things I noticed

1. It would be very helpful to have steps to manually test this.
2. The change record needs an update
3. The new setting needs to be added to settings.php with a comment.

I did some testing with ddev.
I installed the patch, removed sites/default/files/.htaccess, and then installed Drupal. After the install that .htaccess file existed.
I tried again, I added dded $settings['auto_create_htaccess'] = FALSE; to settings.local.php, removed sites/default/files/.htaccess, and then installed Drupal. After the install that .htaccess file did not exist. There is no warning in the logs. And the Status report has an error that the public directory is not fully protected.

Setting to NW for the above.

No need for reroll

+++ b/core/lib/Drupal/Core/File/HtaccessWriter.php
@@ -35,16 +42,27 @@ class HtaccessWriter implements HtaccessWriterInterface {
+    if (!$this->settings->get('auto_create_htaccess', TRUE)) {
+      $this->logger->warning('Auto-creating htaccess disabled.');
+      return;
+    }

Do we need both warnings in ensure and write functions? Also, should it really be a warning?

So this one is coming as the "One" for bugsmash so thought I'd give it a quick look.

Took the patch in #82 and only slightly fixed up with today's checks: Constructor promotion, return types, deprecation version.

#87 I agree and changed that to an error()

Also tweaked the CR some but it was pretty straight forward.

Still NW for

The new setting needs to be added to settings.php with a comment.

Rebased on 11.x and added docs to default.settings.php

Settings read well to me.

16 years and this one may be good

Back to NR for the feedback from @acbramley

I reviewed the MR and added some comments.

I think this needs an issue summary update, because these parts from IS are not implemented:

• When Apache is reported as the web server, automatically create .htaccess files. Otherwise, do not create them and warn the user about the possible implications of this
• New warning message in File System section of Status Report.

When manually tested this, I was unable to find the warning entry in the log (Auto-creating htaccess disabled.), as the file creation is blocked in the parent function (HtaccessWriter::ensure()), so if the auto_create_htaccess config is set to FALSE, it will never reach the code in HtaccessWriter::write(). Not sure it was the intent. Personally I think that we do not need this logging at all and it will be sufficient to create a follow-up to sort-out the errors in the status report, which are displayed if .htaccess files are missing and the auto_create_htaccess config is set to FALSE:

Private files directory - Not fully protected
See https://www.drupal.org/SA-CORE-2013-003 for information about the recommended .htaccess file which should be added to the private:// directory to help protect against arbitrary code execution.

I think it does not make sense to have it as errors anymore, when you use this new switch and disable it's creation on purpose.

Moving to NW for these.

Thanks!

Addressed feedback and updated the IS.

I think we can create a new requirements check for the config setting. However, there was some pushback (#44_) on showing warnings on a standard install, so we need to be sure this is considered.

However, there was some pushback (#44_) on showing warnings on a standard install, so we need to be sure this is considered.

Yes, I was thinking more about adjusting the existing error messages, which are displayed when .htaccess files are missing, so that these have a different wording when the auto_create_htaccess config is set to FALSE, or hide these messages entirely. But I think that is not in scope of this issue, thus I mentioned the potential follow-up. I agree that adding a new requirements check with another warning or so is not ideal.

Believe feedback has been addressed here.

If we need a follow up ping me and I’ll happily do that

Added a CR about the new setting.

Updating issue credits.

I recreated @quietone's testing from #83 and yielded the same result.

Steps to test

1. Delete sites/default/files/.htaccess
2. sudo touch sites/default/files/.htaccess so that the webserver cannot write to it
3. Set $settings['auto_create_htaccess'] = FALSE; in settings.php
4. Visit status report - I still get an error about public files beingnot fully protected because I have the file, but it is empty - this is coming from this hunk in system_requirements
   

    foreach ($htaccessWriter->defaultProtectedDirs() as $protected_dir) {
         $htaccess_file = $protected_dir->getPath() . '/.htaccess';
         // Check for the string which was added to the recommended .htaccess file
         // in the latest security update.
         if (!file_exists($htaccess_file) || !($contents = @file_get_contents($htaccess_file)) || !str_contains($contents, 'Drupal_Security_Do_Not_Remove_See_SA_2013_003')) {
           $url = 'https://www.drupal.org/SA-CORE-2013-003';
           $requirements[$htaccess_file] = [
             // phpcs:ignore Drupal.Semantics.FunctionT.NotLiteralString
             'title' => new TranslatableMarkup($protected_dir->getTitle()),
             'value' => t('Not fully protected'),
             'severity' => REQUIREMENT_ERROR,
             'description' => t('See <a href=":url">@url</a> for information about the recommended .htaccess file which should be added to the %directory directory to help protect against arbitrary code execution.', [':url' => $url, '@url' => $url, '%directory' => $protected_dir->getPath()]),
           ];
         }
       } 

   - I think we need that code to be skipped if this flag is set to FALSE

   

Added a check for the setting in system_requirements().

Fixed conflict with #3493718: Convert system_requirements() into OOP hooks and install time class and added check for setting in SystemRequirements::checkRequirements() instead of system_requirements().

Made some changes to the CR, https://www.drupal.org/node/3525119

Believe feedback has been addressed regarding system_requirements

Committed to 11.x - congrats folks, this was the oldest open bug in Drupal core, AKA 'the one ⭕️' in #bugsmash parlance

Published change record.