Add drupal/core-filesecurity component for writing htaccess files

Related: #3057094: Add Composer vendor/ hardening plugin to core

In that issue, we're using Drupal\Component\PhpStorage\FileStorage::htaccessLines() in the context of a Composer plugin. This replaces the same usage in the current Composer script: https://git.drupalcode.org/project/drupal/blob/8.8.x/core/lib/Drupal/Cor...

This is great because the vendor hardening plugin can just require drupal/core-php-storage and automatically get the same .htaccess file it had in the script. It's a DRY win.

So basically the issue title seems wrong to me: The dependencies are not currently tangled and don't need untangling. :-)

+1 on moving the htaccess writer to be a component. That way the Composer plugin can use it with minimal dependency hell, and core's file system can wrap it up in services context all it wants.

Alternately, the htaccess template method could be in, for instance, drupal/core-utility or somewhere else that's more granular.

This issue is now a blocker for #3057094-101: Add Composer vendor/ hardening plugin to core and thus the Composer initiative.

+1 in favor of making an htaccess/web.config generating component that can be used in a Composer plugin context.

This might require some rearchitecting in parent issue #2620304: htaccess functions should be a service

Nice, thanks.

+++ b/core/lib/Drupal/Component/FileSecurity/FileSecurity.php
@@ -0,0 +1,110 @@
+  /**
+   * Ensures an .htaccess file exists for the given directory.
+   *
+   * @param string $directory
+   *   The directory.
+   * @param bool $private
+   *   (optional) Set to FALSE to ensure an .htaccess file for an open and
+   *   public directory. Default is TRUE.
+   */
+  public static function ensureHtaccess($directory, $private = TRUE) {
...
+  /**
+   * Ensures a web.config file exists for the given directory.
+   *
+   * @param string $directory
+   *   The directory.
+   */
+  public static function ensureWebConfig($directory) {

We should return a value so that callers can know whether it happened or not.

Thanks.

+++ b/core/tests/Drupal/Tests/Component/FileSecurity/FileSecurityTest.php
@@ -0,0 +1,51 @@
+class FileSecurityTest extends TestCase {

Right, so now that we have a return value that can be FALSE, we need a test case where that happens.

Also needs the same tests for ensureWebConfig().

I'd do it but then I can't review. :-)

+++ b/core/lib/Drupal/Component/FileSecurity/FileSecurity.php
@@ -0,0 +1,143 @@
+    if (file_exists($directory) && is_writable($directory)) {
+      if (file_put_contents($file_path, $contents . "\n")) {

could this be one if instead of two?

LGTM assuming the tests pass.

+++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.php
@@ -64,46 +66,15 @@ public function save($name, $code) {
+    @trigger_error("htaccessLines() is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use \Drupal\Component\FileSecurity\FileSecurity::htaccessLines() instead. See https://www.drupal.org/node/3075098", E_USER_DEPRECATED);

shouldn't we be adding a deprecation test for this?

1. +++ b/core/lib/Drupal/Component/FileSecurity/FileSecurity.php
   @@ -0,0 +1,141 @@
   +   * @param bool $private
   +   *   (optional) Set to FALSE to ensure an .htaccess file for an open and
   +   *   public directory. Default is TRUE.
   

   I've read this three times and I don't understand the connection between the name of the parameter and the description.

   Does it mean 'only act for private directories'?

   Oh, wait reading the code further down, it's to do with what is written in the file itself.

   The use of 'ensure' is what makes it unclear here, because it makes it sound like for some values of the parameter, it's not written. 'write' would be better.

   While we're at it, I don't think a boolean value here is very clear. What about having class constants self::PUBLIC_ACCESS and self::PRIVATE_ACCESS?

2. +++ b/core/lib/Drupal/Component/FileSecurity/FileSecurity.php
   @@ -0,0 +1,141 @@
   +    if ($private) {
   

   This should be an if/else rather than a clobber.

3. +++ b/core/lib/Drupal/Component/FileSecurity/FileSecurity.php
   @@ -0,0 +1,141 @@
   +   * @see file_create_htaccess()
   

   This doesn't exist in 8.x as far as I can tell. It looks like it maybe hasn't existed since D6! Let's not bring obsolete documentation into the new class. Besides, you don't need a @see to the method that writes the files, since now it's in the same class.

4. +++ b/core/lib/Drupal/Component/FileSecurity/FileSecurity.php
   @@ -0,0 +1,141 @@
   +    // Don't overwrite if the file exists.
   

   This comment is not true, since we can force. Should append 'unless told to force.'

And a couple of things for follow-ups:

+++ b/core/lib/Drupal/Component/FileSecurity/FileSecurity.php
@@ -0,0 +1,141 @@
+    if (file_exists($directory) && is_writable($directory) && file_put_contents($file_path, $contents)) {
+      return @chmod($file_path, 0444);

This looks weird. If the file_put_contents() succeeds, but the chmod() fails, we return FALSE, even though the file has been written.

Not one to be fixed here, since this issue is about moving the existing code and keeping the same behaviour.

Also, it seems to me like most of the methods on this new class could be private. You're only going to call writeHtAccess() and writeWebConfig(), right? We need the others public for now for the deprecated methods to call, but we could deprecate them as public.

I added the new drupal/core-filesecurity component to the replace section of the drupal/core composer.json file, like the other Components.

I've read this three times and I don't understand the connection between the name of the parameter and the description.

I updated the variable name and code comment for increased clarity.

What about having class constants self::PUBLIC_ACCESS and self::PRIVATE_ACCESS?

I didn't put this in, because $deny_public_access = self::DENY_PUBLIC_ACCESS looked redundant to me. I am not opposed to doing this if it is the consensus.

This should be an if/else rather than a clobber.

This is not a clobber; $lines appears in the second set of lines (prepend). If this were a clobber, I would prefer early-exit (return <<<EOF) over an else. We could move the conditional up and make $lines always append to improve clarity a bit. I didn't do this because it's more lines, and I think the existing code is clear enough, but I wouldn't be opposed to do it that way if it was the consensus.

@see file_create_htaccess()

Good catch. This method is now called file_save_htaccess, which still calls htaccessLines in order to present the error message.

This comment is not true, since we can force. Should append 'unless told to force.'

Updated as suggested.

This looks weird. If the file_put_contents() succeeds, but the chmod() fails, we return FALSE, even though the file has been written.

Being able to write the file but not chmod it is an edge case. Perhaps this could be covered in a follow-up issue; however, if it is fixed, then the resolution should be to add a specific error message if the chmod fails. In the current behavior, if the chmod fails after the file is written, then you get an error message; the error message is wrong, but it does at least draw your attention to the file. It would be undesirable to simply return TRUE and silently fail if the chmod did not succeed.

Also, it seems to me like most of the methods on this new class could be private.

The contents of the htaccessLines and webConfigLines are made available so that they may be included in error messages should the automatic creation of these files fail. These methods therefore should remain public.

Sigh, accidentally posted the interdiff twice. Here's the patch that should have been #27.

> This is not a clobber; $lines appears in the second set of lines (prepend).

Ooops, my mistake. I don't think it's very clear, but I can't think of a way of making it clearer.

> The contents of the htaccessLines and webConfigLines are made available so that they may be included in error messages should the automatic creation of these files fail. These methods therefore should remain public.

I missed the code that calls them. Thanks for the explanation.

> I didn't put this in, because $deny_public_access = self::DENY_PUBLIC_ACCESS looked redundant to me. I am not opposed to doing this if it is the consensus.

I was thinking for the improved clarity in code that calls this:

FileSecurity::writeHtaccess($vendor_dir, TRUE);

// or:

FileSecurity::writeHtaccess($vendor_dir, FileSecurity::DENY_PUBLIC_ACCESS);

The only place outside of tests that uses the 2nd parameter is this in file_save_htaccess():

+  if (FileSecurity::writeHtaccess($directory, $private, $force_overwrite)) {

and it's passing on a $private that it gets as its OWN parameter. But then that too could use constants FileSecurity::DENY_PUBLIC_ACCESS / FileSecurity::ALLOW_PUBLIC_ACCESS.

mod_php5

Maybe it's time to clean it up?

Btw not sure constant is needed, I'd better split methods to writeAllow() and writeDeny() - this is very fragile way to bet on bool for this difference in behaviour

Also if this so much bound to specific apache + php versions this this class could detect used env and be more smart to detect IIS and not write apache for it.

I think it good time to review current webservers and their EOL

Maybe it's time to clean [mod_php5] up?

Like deprecations of code, I would imagine this should be removed in Drupal 9. If that's not the case, I'd rather discuss timing as a separate issue anyway.

Btw not sure constant is needed, I'd better split methods to writeAllow() and writeDeny()

Yeah, I think that might work. Of course, the existing (deprecated) methods will need to maintain the boolean. I'll look at it a bit and see if it's an improvement.

I split out the htaccess snippets into their own functions to make the logic easier to read in htaccessLines. I did not try to get rid of the $deny_public_access boolean, as I did not want to propagate the method bifurcation up the call stack. It didn't seem to make sense to have two writeHtaccess methods, etc., and I think with this minor refactor, the existing code is now clear enough.

I used 3v4l.org to make sure that return <<<EOT is still okay in old PHP versions.

Here's another patch that renames core-filesecurity to core-file-security, as discussed with @mixologic in slack. Forgot that for a minute.

Sure. The original code had these elements broken out, perhaps because the logger line is so long. However, inlining variables is the normal style for Drupal logging, so let's go with consistency.

Fixing code style errors from the test bot.

Diggit. LGTM.

+++ b/core/lib/Drupal/Component/FileSecurity/FileSecurity.php
@@ -0,0 +1,160 @@
+# If we know how to do it safely, disable the PHP engine entirely.
+<IfModule mod_php5.c>
+  php_flag engine off
+</IfModule>

Filed follow-up for #30 #3075999: Remove php5 from htaccess writer

Add 'composer initiative' tag.

Committed 00b3ed6 and pushed to 8.8.x. Thanks!

Published change record