Support open_basedir

Further to what I commented on the original issue #comment-7965763, is there some reason why you don't want to make it possible for Drupal to work on a system where open_basedir is active ? Or is this a don't-care for Drupal ?

I am coming to Drupal from the Symfony world, and I don't know a lot about the typical hosting scenario for Drupal, so perhaps there is a good reason why open_basedir is not normally needed. All I can say is that every single non-Drupal customer server, website and subdomain that we manage uses open_basedir to restrict what the application code can do. Of course there are other ways to stop applications misbehaving, but open_basedir gives precise, self-documenting control over what files an application can access.

So, my preference would be for Drupal to support open_basedir.

If the decision is to add support for open_basedir, I'd be happy to help work on this issue, but frankly right now I'm a bit baffled by the error messages Drupal 8 generates when open_basedir is active. Does anyone have a rough idea where to start looking for the root cause ?

I think the intent from #2019911: Add install requirement that open_basedir = Off since 8.x currently doesn't support it was to flag the problem for now and determine/provide support here. This will keep people from repeatedly debugging a known problem. If you look at all the related issues, including those closed as duplicates on #201991: Starting a site in Drupal 6 beta?, you'll find the suggested causes. If support for open_basedir can be solved, #2019911: Add install requirement that open_basedir = Off since 8.x currently doesn't support it will be reverted.

@dcrocks, perhaps I wasn't clear in #1. I'm all in favor of finding a fix for open_basedir in Drupal, and am happy to help work on a solution. My hesitation in #1 is just because I'm relatively new to Drupal, so there could be good reasons that I don't know about, that mean lack of open_basedir support is a non issue for 99% of Drupal websites. From my perspective, open-basedir support is pretty much a must-have, if you want to play nice on shared servers or in other multi-application / multi-customer situations.

I completely agree with cpj, open_basedir should be supported. Dropping the support would mean my clients won't switch to D8 and stay on D7 as long as they can until they build on alternatives. And I'm sure this is not just a 1% situation.

I also completely agree with cpj and hauptm, open_basedir should be supported!!!

If not, I'll have no chance to install D8 on our shared website provider. :-(((
grrrrr.

Okay I thought it should be discussed here whether to drop support or support it. Seems no one shares the opinion that dropping support is reasonable?

Based on this php bug report glob returns error, should be empty array() and this suggested work-around Behaviour of glob function in php is different with open_basedir, I created the attached patch.

If you enable open_basedir, this patch gets the install process past the initial open_basedir errors, but after the database is created, before it starts the main install it now throws "Fatal error: Call to undefined function theme() in xxx/core/includes/install.core.inc on line 992."

Any suggestions ?

I have debugged this part of the install process both with and without open_basedir set. It seems that the execution takes a different path if open_basedir is set, and theme.inc is not included.

I don't understand enough about the Drupal install process to get to the bottom of this. Can someone else help here ?

The main hunk of #9 (for InstallStorage.php) doesn't apply any more, due to #2138239: Use GlobIterator instead of glob, but it also doesn't seem to be needed any more. The other two hunks are useful for debugging this.

The theme() error message is a failure when catching an exception, which is itself caused by a failure to log a php E_WARNING triggered in file_directory_os_temp(). I'm filing issues for the poor error handling.

We can probably work around those issues by surpressing the original warning, but I'm not sure yet whether that's a good idea or not.

- if (is_dir($directory) && is_writable($directory))
+ if (@is_dir($directory) && @is_writable($directory))

I don't think Globiterater has the same bug as glob(glob returns error ). See comment #9 above. I thought that #2138239: Use GlobIterator instead of glob might fix the initial error.

The two error handling bugs I filed now both have patches needing review. If you want this bug fixed, then reviewing those would be a big help.

#2091501: When install fails: Fatal error: Call to undefined function theme() in install.core.inc on line 954
#2153401: Uncaught InvalidArgumentException sometimes thrown by _drupal_error_handler during install

I am trying Drupal 8 alpha 7 on a shared hosting company (where I can not request open_basedir to be turned off) and the error still persists :

"Your PHP installation has open_basedir enabled. Drupal currently requires the open_basedir option to be turned off. See the PHP manual for details of how to do this. This issue is currently under discussion at drupal.org."

Is the patch #9 already committed to Drupal 8 alpha 7?

No, #9 has not been committed and isn't enough to fix the problem anyway.

Presumably you're just thinking about Drupal 8 for personal use / curiosity at the moment, so I'd recommend setting up a local environment where you can control things like open_basedir. See https://drupal.org/node/157602

We have two testing environments:
1st is on http://www.virtualbox.org/ and http://www.apachefriends.org/en/xampp.html
2nd is on a real shared hosting company.

For a while, we are only using Drupal 8 for testing and studying.
Since Drupal 8 provide many advantages related to Drupal 7, our idea is to migrate as soon as possible. That's why we are also testing on a real environment.

1) It's an alpha, expect problems.
2) Drupal 8 will support open_basedir, it's just broken at the moment.

@ianthomas_uk

I applied

2091501-4-undefined-function-theme.patch

and

2153401-1-option-a-watchdog-exception.patch

After disabling the open_basedir check in core/install.php, the install completes but when I go to the newly installed website I get

Fatal error: Call to undefined function cache() in /var/www/clients/client0/web11/Drupal8Test/web/core/includes/module.inc on line 33

If I then try to run core/update.php, it throws several open_basedir errors on line 80 of core/lib/Drupal/Component/phpStorage/FileStorage.php -> ensureDirectory($directory, $mode = 0777) because this does a file_exists on all nodes in the directory tree of the requested directory and this fails on nodes that are outside (i.e. above) the directories in the open_basedir list.

Hope this helps...

Chris

I have found & tested a possible solution, and will now create a patch for /Drupal/Component/phpStorage/FileStorage.php -> ensureDirectory($directory, $mode = 0777). Since I'm new to the Drupal way of doing patches, I'm also pasting the proposed solution below for feedback:

  /**
   * current version
   *
  protected function ensureDirectory($directory, $mode = 0777) {
    if (!file_exists($directory)) {
      // mkdir() obeys umask() so we need to mkdir() and chmod() manually.
      $parts = explode('/', $directory);
      $path = '';
      $delimiter = '';
      do {
        $part = array_shift($parts);
        $path .= $delimiter . $part;
        $delimiter = '/';
        // For absolute paths the first part will be empty.
        if ($part && !file_exists($path)) {
          mkdir($path);
          chmod($path, $mode);
        }
      } while ($parts);
    }
  }
  */

  /**
   * Proposed version
   */
  protected function ensureDirectory($directory, $mode = 0777)
    {
        if ((@mkdir($directory) && chmod($directory, $mode)) || file_exists($directory)) {
            return true;
        }

        return ($this->ensureDirectory(dirname($directory), $mode) && mkdir($directory));
    }

The current version can't work with open_basedir active because it always starts from the root folder, which will not be in the allowed path. Actually, the current version of the function has to be a potential security risk, because if open_basedir is inactive, this function could attempt to create a folder anywhere in the file system.

The proposed version works from the end of the path recursively back towards the root folder, so must hit an existing & write-able parent folder. If it does not, then the requested path can't be created anyway.

I'm experiencing the very same problem. Is this patch merged into the main dev release?

As far as I can tell it works and it passed testing. The patch status is "Needs review"...

To which release should the patch be manually applied?

It should work on alpha8 or the latest 8.x

@catch - I've tested the patch and as far as I can tell it works. I don't know how well the testing exercises the code, but it passes. The code only changes the way directories are created, which is fairly isolated from the rest of Drupal, so it should not be impacted by all the other changes that are active. I've tried a couple of different Apache based configurations so I think it will work on most hosters, but ideally it should be tested on a variety of hosting scenarios - for example Apache mod_php, mod_fastcgi, php-fpm, nginx, and IIS. I don't have access to all these options...

Is there something I can do to help progress this further ?

Thanks

According to https://bugs.php.net/bug.php?id=47358, the earlier comments here about glob() returning FALSE should be obsolete with PHP 5.4.

The error suppression for @is_dir() in DrupalKernel::buildContainer() seems to be obsolete.

The change to FileStorage::ensureDirectory() looks interesting. I assume that the same could be achieved by passing a relative directory path into PhpStorage, but of course, that would break again if you'd want to use an absolute path that is outside of the Drupal/document root.

The proposed approach makes sense. The recursion logic idea is actually pretty smart.

The gist of that function actually originates from drupal_mkdir(), which we should probably update in the identical way here.

(For the sake of completeness, LocalStream::mkdir() also calls into drupal_mkdir().)

Now, on the FileStorage::ensureDirectory() change itself:

1. All of the error suppression wasn't there before and should thus be removed.
2. The file_exists() in the initial condition needs to come first, so you do not try to create a directory that exists already.
3. While being there, it might be a good idea to change the file_exists() into is_dir().
4. The whole coding style needs to be revised to follow the rest of the file; also, Booleans are always notated in uppercase in Drupal. See https://drupal.org/coding-standards
5. The additional phpDoc describes what the function is doing, but not really the full backwards/forward recursion logic concept, and even more importantly, it does not document *why* it works in the way it works (→ a short summary of the root cause).

This should cut it.

Reconsidered and not touching drupal_mkdir() here, only adding a @todo, since that function additionally caters for stream wrapper URIs.

Hm. Reverting is_dir() back to file_exists() to see whether that helps.

Oh, I think I get it now. Before trying to create a subdirectory, we need to check whether the parent directory exists.

35: drupal8.open-basedir.35.patch queued for re-testing.

@sun - in case it wasn't clear, the reason why the code doesn't make use of the recursive mode of mkdir is that IIS ignores the mkdir mode parameter. So the recursion needs to be implemented in code, and the chmod needs to be done separately from the mkdir, as shown here.

wont work with current version

wont work with current version

@undead67: Not sure what your comment tried to say - could you clarify?

@cpj:

Yeah, the recursive mode of mkdir() is known to have issues — first and foremost, the existing/documented nature of mkdir() obeying to the umask of the PHP process pretty much prevents its usage, because we still need to chmod() the parent directories that may be created.

That part of the patch hasn't been changed; it is still using the clever backwards/forward recursion construct. (Not sure who's to credit for that, did you invent that yourself? :))

That said, attached patch fixes some edge-case oversights in the backwards recursion logic.

+++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.php
@@ -58,31 +58,43 @@ public function save($name, $code) {
+    // from there, recursively create the sub-directories. If that recursion
+    // succeeds, create the final subdirectory.
+    return $this->ensureDirectory($parent, $mode) && mkdir($directory) && chmod($directory, $mode);

It might be worth to clarify via code syntax/logic/comments that this final line of creating the originally requested directory is only executed if the parent directory of the requested directory did not exist yet.

If the parent directory of the requested directory already exists, then the function ends in the is_dir($parent) condition already.

One problem with the non-recursion case is that the return value does not reflect the status of the chmod() operation → we need to add a recursion argument in order to return the actual status when not recursing.

@sun
#30 - the @is_dir() in DrupalKernel::buildContainer() line 513 is required because if open_basedir is enabled you get an error when it tries to check web/core/lib/Drupal/Core/SystemListing.php/Plugin. In the front end you'll get that confusing "missing theme" message, but in XDEBUG you'll see it actually thows an open_basedir error on the call to is_dir.

#41 - I've used this recursive method here because I've successfully used it for similar problems in the past but I didn't invent it...

@sun - as per the explanation in #44, @is_dir() in DrupalKernel::buildContainer() line 513 is required. #45 re-applies this change to buildContainer(), the rest is the same.

Thanks, I see what you mean now.

That's actually a bug on its own and has little to do with open_basedir - but we can happily fix it here.

Attached patch includes the proper fix for that. Again, error suppression is almost always wrong and should ring all alarm bells. Instead, always check why an error is thrown in the first place. :-)

I would like to test these patches. To which version should they be applied? The daily dev or the alpha8? Will they be merged into the main daily dev release?

Re #49: Try them out with the daily dev. Once this issue has been reviewed and approved it will be committed to the Git repository and then show up in the daily devs.

Re #50: Should I apply the patch whenever I do upgrade my Drupal? Or just on setup time?

@cesareaugusto: The only remaining task here is to test whether the latest patch in #47 fixes the open_basedir compatibility problem with the latest development snapshot of Drupal 8.x-dev.

You can download 8.x-dev from https://drupal.org/node/572834 (or alternatively follow the instructions on https://drupal.org/project/drupal/git-instructions) and apply the latest patch here, following the instructions from https://drupal.org/patch/apply

As soon as we have a confirmation that this patch fixes the problem, this issue should be marked as "Reviewed and Tested By the Community" (RTBC), so that it can be committed. Once this patch has been committed, you no longer need to apply it manually.

@cpj et al who experienced the problem: Does the latest patch work for you?

@sun - yes, just tested it and version #47 of this patch works for me against the latest version of 8.x-dev on our standard debian + apache + open_basedir configuration. Ideally someone with a Windows IIS setup and open_basedir should also try to install Drupal 8.x-dev with this patch applied. As I worked on this issue, to exercise the common places where open_basedir threw an error, I found you need to do a complete fresh install from scratch, right to the end of the install process, followed by a login to the new Drupal website, and a cache clear.

So from my perspective, we're done - see you on to the next critical issue ?

Chris

Thanks for confirming! Let's simplify further testing.

In case additional issues with open_basedir will arise, we can do follow-up patches here or in separate follow-up issues.