Issue #2508666 by alexpott, pwolanin, benjy: Drupal 8 .htaccess rule to...

Issue #2508666 by alexpott, pwolanin, benjy: Drupal 8 .htaccess rule to prevent php file access can be easily bypassed

.htaccess

@@ -3,7 +3,7 @@
 #
 
 # Protect files and directories from prying eyes.
-<FilesMatch "\.(engine|inc|install|make|module|profile|po|sh|.*sql|theme|twig|tpl(\.php)?|xtmpl|yml)(~|\.sw[op]|\.bak|\.orig|\.save)?$|^(\..*|Entries.*|Repository|Root|Tag|Template)$|^#.*#$|\.php(~|\.sw[op]|\.bak|\.orig\.save)$">
+<FilesMatch "\.(engine|inc|install|make|module|profile|po|sh|.*sql|theme|twig|tpl(\.php)?|xtmpl|yml)(~|\.sw[op]|\.bak|\.orig|\.save)?$|^(\..*|Entries.*|Repository|Root|Tag|Template)$|^#.*#$|\.php(~|\.sw[op]|\.bak|\.orig|\.save)$">
   <IfModule mod_authz_core.c>
     Require all denied
   </IfModule>
@@ -139,14 +139,14 @@ AddEncoding gzip svgz
   # Allow access to PHP files in /core (like authorize.php or install.php):
   RewriteCond %{REQUEST_URI} !/core/[^/]*\.php$
   # Allow access to test-specific PHP files:
-  RewriteCond %{REQUEST_URI} !/core/modules/system/tests/https?.php$
+  RewriteCond %{REQUEST_URI} !/core/modules/system/tests/https?.php
   # Allow access to Statistics module's custom front controller.
   # Copy and adapt this rule to directly execute PHP files in contributed or
   # custom modules or to run another PHP application in the same directory.
   RewriteCond %{REQUEST_URI} !/core/modules/statistics/statistics.php$
   # Deny access to any other PHP files that do not match the rules above.
   # Specifically, disallow autoload.php from being served directly.
-  RewriteRule "^(.+/.*|autoload)\.php$" - [F]
+  RewriteRule "^(.+/.*|autoload)\.php($|/)" - [F]
 
   # Rules to correctly serve gzip compressed CSS and JS files.
   # Requires both mod_rewrite and mod_headers to be enabled.

core/modules/system/src/Tests/System/HtaccessTest.php

@@ -15,20 +15,44 @@
  * @group system
  */
 class HtaccessTest extends WebTestBase {
+
+  /**
+   * Modules to enable.
+   *
+   * @var array
+   */
+  public static $modules = array('node', 'path');
+
   /**
    * Get an array of file paths for access testing.
    *
-   * @return array
-   *   An array of file paths to be access-tested.
+   * @return int[]
+   *   An array keyed by file paths. Each value is the expected response code,
+   *   for example, 200 or 403.
    */
   protected function getProtectedFiles() {
     $path = drupal_get_path('module', 'system') . '/tests/fixtures/HtaccessTest';
-    $file_exts = [
+
+    // Tests the FilesMatch directive which denies access to certain file
+    // extensions.
+    $file_exts_to_deny = [
       'engine',
       'inc',
       'install',
       'make',
       'module',
+      'module~',
+      'module.bak',
+      'module.orig',
+      'module.save',
+      'module.swo',
+      'module.swp',
+      'php~',
+      'php.bak',
+      'php.orig',
+      'php.save',
+      'php.swo',
+      'php.swp',
       'profile',
       'po',
       'sh',
@@ -40,13 +64,28 @@ protected function getProtectedFiles() {
       'yml',
     ];
 
-    foreach ($file_exts as $file_ext) {
-      $file_paths[] = "$path/access_test.$file_ext";
+    foreach ($file_exts_to_deny as $file_ext) {
+      $file_paths["$path/access_test.$file_ext"] = 403;
     }
 
+    // Tests the .htaccess file in core/vendor and created by a Composer script.
     // Try and access a non PHP file in the vendor directory.
-    $file_paths[] = 'core/vendor/composer/installed.json';
+    // @see Drupal\\Core\\Composer\\Composer::ensureHtaccess
+    $file_paths['core/vendor/composer/installed.json'] = 403;
+
+    // Tests the rewrite conditions and rule that denies access to php files.
+    $file_paths['core/lib/Drupal.php'] = 403;
+    $file_paths['core/vendor/autoload.php'] = 403;
+    $file_paths['autoload.php'] = 403;
+
+    // Test extensions that should be permitted.
+    $file_exts_to_allow = [
+      'php-info.txt'
+    ];
 
+    foreach ($file_exts_to_allow as $file_ext) {
+      $file_paths["$path/access_test.$file_ext"] = 200;
+    }
     return $file_paths;
   }
 
@@ -54,21 +93,52 @@ protected function getProtectedFiles() {
    * Iterates over protected files and calls assertNoFileAccess().
    */
   public function testFileAccess() {
-    foreach ($this->getProtectedFiles() as $file) {
-      $this->assertNoFileAccess($file);
+    foreach ($this->getProtectedFiles() as $file => $response_code) {
+      $this->assertFileAccess($file, $response_code);
     }
+
+    // Test that adding "/1" to a .php URL does not make it accessible.
+    $this->drupalGet('core/lib/Drupal.php/1');
+    $this->assertResponse(403, "Access to core/lib/Drupal.php/1 is denied.");
+
+    // Test that is it possible to have path aliases containing .php.
+    $type = $this->drupalCreateContentType();
+
+    // Create an node aliased to test.php.
+    $node = $this->drupalCreateNode([
+      'title' => 'This is a node',
+      'type' => $type->id(),
+      'path' => 'test.php'
+    ]);
+    $node->save();
+    $this->drupalGet('test.php');
+    $this->assertResponse(200);
+    $this->assertText('This is a node');
+
+    // Update node's alias to test.php/test.
+    $node->path = 'test.php/test';
+    $node->save();
+    $this->drupalGet('test.php/test');
+    $this->assertResponse(200);
+    $this->assertText('This is a node');
   }
 
   /**
-   * Asserts that a file exists but not accessible via HTTP.
+   * Asserts that a file exists and requesting it returns a specific response.
    *
    * @param string $path
    *   Path to file. Without leading slash.
+   * @param int $response_code
+   *   The expected response code. For example: 200, 403 or 404.
+   *
+   * @return bool
+   *   TRUE if the assertion succeeded, FALSE otherwise.
    */
-  protected function assertNoFileAccess($path) {
-    $this->assertTrue(file_exists(\Drupal::root() . '/' . $path));
+  protected function assertFileAccess($path, $response_code) {
+    $result = $this->assertTrue(file_exists(\Drupal::root() . '/' . $path), "The file $path exists.");
     $this->drupalGet($path);
-    $this->assertResponse(403);
+    $result = $result && $this->assertResponse($response_code, "Response code to $path is $response_code.");
+    return $result;
   }
 
   /**