Only show the error “Public files directory Not fully protected” for Apache servers

Thanks Marko for the patch. Patch #4 is working fine for me.

@vishalkhode, thanks for testing the patch. Good to know it works!

I have read the issue and I don't see a review of the code here and there should be an explanation of the fix in the issue summary. I've added the template but can someone else more familiar with this patch complete it? Tagging needs issue summary update

Looking at the patch:

+++ b/core/modules/system/system.install
@@ -511,8 +511,8 @@ function system_requirements($phase) {
+  // Test the contents of the .htaccess files. Use it only on apache servers

The last sentence should end in a period. Since this is just for apache, then let's put that information first in the comment. Something like, 'For apache servers test the contents of the .htaccess files.'

+++ b/core/modules/system/system.install
@@ -511,8 +511,8 @@ function system_requirements($phase) {
+  if ($phase == 'runtime' && (!isset($_SERVER['SERVER_SOFTWARE']) || strpos($_SERVER['SERVER_SOFTWARE'], 'apache') === 0)) {
...
     /** @var \Drupal\Core\File\HtaccessWriterInterface $htaccessWriter */

I'm not sure the best to test for apache but this will fail on my system where

$server['SERVER_SOFTWARE'] = 'Apache/2.4.38 (Debian)';

There is a related issue for 9.3.x: #2906490: Link to useful information about .htaccess and directory protection.

It is about the verbiage in error message itself, not about not showing it on non-Apache servers. I just link to it as a matter of record.

+1
Verifying that this is a false positive,
in my case;
Using nginx with s3fs module, added .htaccess file, in private folder and still see error.

Addressed the point #9
Attached the reroll patch against #4

Line 194 of system.install uses:

193   // Tests clean URL support.
194   if ($phase == 'install' && $install_state['interactive'] && !$request_object->query->has('rewrite') && strpos($software, 'Apache') !== FALSE) {
195     // If the Apache rewrite module is not enabled, Apache version must be >=
196     // 2.2.16 because of the FallbackResource directive in the root .htaccess
197     // file. Since the Apache version reported by the server is dependent on the
198     // ServerTokens setting in httpd.conf, we may not be able to determine if a

Suggest to use that, last condition, instead of global variable. Could not find it in coding standards but thought the usage of global variables was not recommended. The $software is still available for the check on protection.

Current solution implements requirement check for Apache only. That is the wrong thing to do, it should check regardless of which server it is running on. It can use the same check, but shouldn't provide a description that only applies to Apache. It should point to where it is described how to protect these folders for IIS and NGINX.

Updated the patch as per #18

Simple working patch

Simple working patch

@pankaj1390 Why did you remove the comment line from the patch in #20? Your code in #23 and #24 is identical but you removed the documentation comment with no explanation.

I made an MR based on the patch in #20 with a few updates to the language (add a comma, remove an extraneous "the").

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Moving to NW for the issue summary update requested in #9

Also as a bug this will need a test case.

Why this is not include in core because number of user using ngnix?

@pankaj1390 Please read the issue priority guidelines and do not change the priority without a valid reason.

Also bugfixes always go into the working development version of Drupal, which is currently 10.1.

Updating the issue summary and removing the tag.

5 years, and this remains obstructed with astonishingly unclear prescriptions as to why.

It is no wonder folks are reluctant to contribute.

@emjayess The remaining task before this issue can be marked "Needs review" is to add a test as stated in smustgrave's comment.

The issue is also currently tagged with "Needs tests," which hopefully makes it more clear.

@ptmkenny:
system.install @ system_requirements() is 1.5k lines of procedural goo – not exactly lending itself to a unit test. Further, the block emitting this apache-only error to apache and non-apache systems alike, is a system test.

If the hang-up is that this block of twenty lines of (test) code requires additional test coverage elsewhere in a test suite, then it would follow that a test supplying that coverage would pre-exist. I cannot find one. If someone could chime in on the rationale or logic for a test of this test, and where that test needs to live, that would be swell.

As this is merely a "bug" of omission in the first place – since system_requirements() does in fact test the server $software elsewhere for apache, but for some reason (of omission/oversight) not here – it seems to me to be an indication that there is no test coverage for this code now. Probably because it is a system test in and of itself.

Please also note that system.install @ system_requirements() around L219 runs another test for clean url (rewrite module) support, and in fact does already only conditionally proceed for apache webservers, and it conditionally does so as follows:

> && str_contains($software, 'Apache')

If there indeed does exist a separate unit or functional test supplying code coverage for this test, please point at it forthwith, as a guide.

If there is not any existing coverage of all this procedural goo, then this apache-only alarmism has been needlessly nagging maintainers of non-apache systems for years, for no good or justifiable reason, because of a bogus objection and obstruction to adding a simple && str_contains($software, 'Apache') condition to an if statement, to bypass the code for non-apache web servers.

Lower quality software lingers on and on due to insistence on what is perceived to result only in higher quality software... clearly isn't working.

The real chore that evidently is being avoided is to boil away this procedural goo and get security checks much better organized somewhere other than inline'd in system_requirements(). Forking procedurally for all these checks across all the possible web servers using if statements in a multi-thousand LoC function is very non-viable and non-testable, and as is self-evident, a maintenance headache.

Thank you for searching for a test. It's quite possible that there is no test.

A little background may be helpful here: at some point in the Drupal 8? development cycle, it was decided that new commits to core require test coverage, so it is the responsibility of new commits to add test coverage if there isn't any already. That's why this issue is stuck.

The reason for requiring tests is to ensure that "fixed" things don't get broken again.

If you want to debate the rationale for this policy, please don't do it in this issue; a better place would probably be the #contrib channel in the Drupal Slack.

Hopefully someone else better informed than me can describe what kind of test should be written for this.

Instead of figuring out what platform we are on, wouldn't a simple test be to:

• Place a (random hash).php file in the public files directory which will return a value if it is executed
• Do a http request against that url
• See if the result is the output if the file was executed or not

Using a random hash is just so that we don't expose sites with this issue to scanners.

To me this would expose server misconfigurations including on NON-apache servers.

Mistake, ignore.

This issue has been fixed in #360057: Automatic creation of .htaccess files can cause errors

Change record

Sorry for the duplicate posts. This is in fact the correct issue, and you can now disable the check in settings (from Drupal 11.3)