Tried running tests on my shared hosting account (the server is a tad more powerful than my laptop). The server has an open_basedir restriction that interferes with the following 2 curl options:

CURLOPT_COOKIEJAR => NULL
CURLOPT_FOLLOWLOCATION => TRUE

I got round the first by setting the cookie jar to a random filename e.g.
protected $cookie_file = './cook';
The file didn't seem to get created, no errors were reported ...

However the FOLLOWLOCATION setting is more of a problem since PHP reports thatthis is incompatible with open_basedir. Running the syslog tests with this option set to FALSE I get a test failure after each POST (2 in my case), which makes sense. I guess simpletest would need to do handle the redirections itself?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Title: simpletest.module incompatible with open_basedir » Testing incompatible with open_basedir restriction
gpk’s picture

Update:
#554106: Module enable/disable tests failing on testing servers temporarily set CURLOPT_FOLLOWLOCATION => FALSE, but it was reverted.

The other issue has a bit of useful info on this at http://drupal.org/node/360417#comment-1207025 - specifically see http://www.php.net/manual/en/function.curl-setopt.php#71313 (the other comment referenced on the php.net page seems to have gone..!)

Also (for my info) the cookie file is defined here:
http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/simpletest/drupal_...

and then the jar is set here: http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/simpletest/drupal_...

sun’s picture

Priority: Normal » Critical

HOLY COW.

I spent like the entire day to figure out what's wrong with my server, checked cURL installation/configuration, PHP configuration, extensions, DNS resolver, hacked drupal_web_test_case.php inserting CURLOPT_RETURNTRANSFER => TRUE in various places (which at least made GET requests return more than 0-1 bytes) and whatnot...

So, yeah, hours later I find this issue, altered my httpd.conf:

  php_admin_value open_basedir none

and suddenly it works. :-/

Anonymous’s picture

Status: Active » Needs review
FileSize
1.52 KB

Created a patch for simpletest.install adding a requirement saying that open_basedir has to be set to none!

sun’s picture

Status: Needs review » Needs work

Good idea.

+++ simpletest.install	8 Dec 2009 21:10:03 -0000
@@ -223,8 +223,9 @@ function simpletest_requirements($phase)
-
+  

@@ -250,6 +251,14 @@ function simpletest_requirements($phase)
-
+  
...
+  if ($open_basedir) {  
...
+    

Trailing white-space here.

+++ simpletest.install	8 Dec 2009 21:10:03 -0000
@@ -250,6 +251,14 @@ function simpletest_requirements($phase)
+  $requirements['open_basedir'] = array(
+	'title' => $t('open basedir'),
+	'value' => $open_basedir ? $t('None') : $t('Set'),
+  );

Wrong indentation.

+++ simpletest.install	8 Dec 2009 21:10:03 -0000
@@ -250,6 +251,14 @@ function simpletest_requirements($phase)
+  if ($open_basedir) {  
+    $requirements['open_basedir']['severity'] = REQUIREMENT_ERROR;
+    $requirements['open_basedir']['description'] = t('Simpletest requires open_basedir to be set to none. Please check your webserver configuration');
+    
   return $requirements;

Missing closing brace for if statement.

This review is powered by Dreditor.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
1.53 KB

Fixed the issues, attaching updated patch file.

sun’s picture

Status: Needs review » Needs work
+++ simpletest.install	8 Dec 2009 21:41:48 -0000
@@ -250,6 +251,15 @@ function simpletest_requirements($phase)
-
+  

Still trailing white-space here.

+++ simpletest.install	8 Dec 2009 21:41:48 -0000
@@ -250,6 +251,15 @@ function simpletest_requirements($phase)
+    'title' => $t('open basedir'),

Proper capitalization? Perhaps also prefix with "PHP", so people know what this belongs to?

+++ simpletest.install	8 Dec 2009 21:41:48 -0000
@@ -250,6 +251,15 @@ function simpletest_requirements($phase)
+    $requirements['open_basedir']['description'] = t('Simpletest requires open_basedir to be set to none. Please check your webserver configuration');

SimpleTest was partially renamed to "Testing", i.e. as in "testing framework".

A further rename of the internal module name is likely to happen.

In any way, this user-facing message should not use "SimpleTest".

This review is powered by Dreditor.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
1.35 KB

Fixed thoose issues, attaching updated patch.

Anonymous’s picture

Found an indentation error, corrected in attached patch.

sun’s picture

+++ simpletest.install	8 Dec 2009 22:11:02 -0000
@@ -251,5 +252,14 @@ function simpletest_requirements($phase)
+  $requirements['open_basedir'] = array(

Looking at the other keys, let's rename this to "php_open_basedir".

+++ simpletest.install	8 Dec 2009 22:11:02 -0000
@@ -251,5 +252,14 @@ function simpletest_requirements($phase)
+    'title' => $t('PHP Open Basedir'),

Let's rename this to "PHP open_basedir value".

+++ simpletest.install	8 Dec 2009 22:11:02 -0000
@@ -251,5 +252,14 @@ function simpletest_requirements($phase)
+    'value' => $open_basedir ? $t('None') : $t('Set'),

This logic looks wrong/reversed...?

+++ simpletest.install	8 Dec 2009 22:11:02 -0000
@@ -251,5 +252,14 @@ function simpletest_requirements($phase)
+    $requirements['open_basedir']['description'] = t('Testing requires open_basedir to be set to none. Please check your webserver configuration');

Missing trailing period/full-stop.

Let's also re-phrase that into:

The testing framework requires PHP's open_basedir to be set to 'none'. Please check your webserver configuration.

I'm on crack. Are you, too?

Status: Needs review » Needs work

The last submitted patch failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
1.4 KB

Another update! :)

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

+++ simpletest.install	8 Dec 2009 22:11:02 -0000
@@ -251,5 +252,14 @@ function simpletest_requirements($phase)
+    $requirements['php_open_basedir']['description'] = t('The testing framework requires PHP's open_basedir to be set to 'none'. Please check your webserver configuration.');

You need to use double-quotes here.

I'm on crack. Are you, too?

Anonymous’s picture

Status: Needs work » Needs review
FileSize
1.49 KB

Had used wrong CVS release for previous patch. Attaching proper one.

sun’s picture

Status: Needs review » Reviewed & tested by the community

well done.

Welcome to the core development team! :)

chx’s picture

Welcome indeed -- I gave nenne this issue on IRC and some instructions -- thanks sun a lot for guiding him towards this nice solution!

Anonymous’s picture

Thank you both for the help! Learned alot from this!

Status: Reviewed & tested by the community » Needs review

Re-test of update5-323110-simpletest-openbasedir-issue-D7.patch from comment #15 was requested by webchick.

sun’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Needs work

Great work, nenne! :) Some comments...

+++ modules/simpletest/simpletest.install	9 Dec 2009 00:16:13 -0000
@@ -154,6 +155,15 @@ function simpletest_requirements($phase)
+    $requirements['php_open_basedir']['description'] = t("The testing framework requires PHP's open_basedir to be set to 'none'. Please check your webserver configuration.");

Would it be possible to expand this very slightly to include why? Or at least what happens when this is not set properly? I know this creates inconsistency with the other ones I can guess at (cURL is needed for the browser, DOM for HTML/XML parsing...) I have no idea why open_basedir would be screwing things up. I've never in my life needed to look at that setting before, in all the PHP development I've done.

Alternately, should we be investigating why open_basedir is screwing things up? Are we simply identifying a symptom rather than treating the cause?

I'm on crack. Are you, too?

gpk’s picture

>I have no idea why open_basedir would be screwing things up.
I think the answer might be in the original post?

>Are we simply identifying a symptom rather than treating the cause?
Yes; I was planning to open a separate issue for adding open_basedir support once/if this workaround gets committed. Since open_basedir support might take a while to arrange (because it's something few core devs need ... they don't use shared hosting) I think it would be helpful to commit this patch, perhaps with an additional comment explaining why open_basedir is not currently supported) to make sure that others avoid the pickle that sun got into :P, until such time as the symptom is properly treated.

I guess it all depends a bit how important open_basedir support is deemed to be. As things stand at the moment, a large proportion of Drupal 7.x users will be unable to run tests without either changing web host or setting up a local dev environment. Hardly the best way of encouraging people to get involved in reviewing patches/testing/best practices etc.!

gpk’s picture

Status: Needs work » Needs review
FileSize
1.62 KB

Slight update including:
- comment added in response to #21, which also references new issue #674304: Make testing compatible with open_basedir restriction
- 'none' is how you turn off open_basedir in httpd.conf, but is not a valid value

sun’s picture

Thanks! It would really be nice to get this one in to save other developers from the nightmares I had to go through. Even if it's only temporary until the real cause can be fixed (if it can be fixed at all).

+++ modules/simpletest/simpletest.install	Mon Jan 04 23:21:44 2010
@@ -63,6 +64,18 @@ function simpletest_requirements($phase)
+    $requirements['php_open_basedir']['description'] = t('SimpleTest requires the PHP <a href="@open_basedir-url">open_basedir</a> restriction not to be set. Please check your webserver configuration or contact your web host.', array('@open_basedir-url' => url('http://php.net/manual/en/ini.core.php#ini.open-basedir')));

s/not to be set./to be disabled./

9 days to code freeze. Better review yourself.

gpk’s picture

> s/not to be set./to be disabled./
Yes I think I was trying to reflect the fact that open_basedir is usually disabled by default ... your php.ini must have turned it on or maybe another directive somewhere in the Apache config. However since the message is only displayed if there is a problem your suggestion makes sense.

>if it can be fixed at all
I don't see why not.. requires 2 curl options to be changed ... NULL for cookiejar (which looks like a bit of a "trick" anyway), and also redirects would need to be processed explicitly instead of automatically.

sun’s picture

+++ modules/simpletest/simpletest.install	Mon Jan 04 23:21:44 2010
@@ -63,6 +64,18 @@ function simpletest_requirements($phase)
+  // See also http://drupal.org/node/674304.

s/See also/@see/

+++ modules/simpletest/simpletest.install	Mon Jan 04 23:21:44 2010
@@ -63,6 +64,18 @@ function simpletest_requirements($phase)
+    'title' => $t('PHP open_basedir value'),
+    'value' => $open_basedir ? $t('Set') : $t('Not set'),

Following the new wording, we should use "PHP open_basedir restriction" along with "Enabled" and "Disabled" here as well.

+++ modules/simpletest/simpletest.install	Mon Jan 04 23:21:44 2010
@@ -63,6 +64,18 @@ function simpletest_requirements($phase)
+    $requirements['php_open_basedir']['description'] = t('SimpleTest requires the PHP <a href="@open_basedir-url">open_basedir</a> restriction to be disabled. Please check your webserver configuration or contact your web host.', array('@open_basedir-url' => url('http://php.net/manual/en/ini.core.php#ini.open-basedir')));

I think that we can remove the url() here...?

Afterwards, RTBC.

9 days to code freeze. Better review yourself.

sun’s picture

+++ modules/simpletest/simpletest.install	Mon Jan 04 23:21:44 2010
@@ -63,6 +64,18 @@ function simpletest_requirements($phase)
+    $requirements['php_open_basedir']['description'] = t('SimpleTest requires the PHP <a href="@open_basedir-url">open_basedir</a> restriction to be disabled. Please check your webserver configuration or contact your web host.', array('@open_basedir-url' => url('http://php.net/manual/en/ini.core.php#ini.open-basedir')));

In addition, "SimpleTest" should not appear in user-facing text. We replaced it with "The testing framework" earlier.

9 days to code freeze. Better review yourself.

gpk’s picture

Status: Needs review » Needs work

Largely fair enough ;)

re. url() and SimpleTest, I was using those formats for consistency with the rest of http://api.drupal.org/api/function/simpletest_requirements/7. So what to do?

sun’s picture

The other external URLs are not using url(). The other strings should be updated to not use "SimpleTest", but that rather belongs into a separate issue.

gpk’s picture

Status: Needs work » Needs review
FileSize
1.63 KB

Doh! I was using the link to the PHP info page as an example!

sun’s picture

Status: Needs review » Reviewed & tested by the community

Awesome!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Thanks. I ran this past Jimmy last night and he was cool with it.

Committed to HEAD.

gpk’s picture

Status: Closed (fixed) » Fixed

Thanks webchick!

@sun: is it actually too late to change, for 7.x, the other mentions of "Simpletest" to "The testing framework" in simpletest_requirements()..?

[update] They were fixed here #678050: Strings tidy-up for simpletest_requirements

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

leonardjo’s picture

Status: Fixed » Closed (fixed)

The current wording of the error in the status page gives the impression that open_basedir restrictions are a bad thing. This sends entirely the wrong signal to the user.

The error message should be reworded to indicate removing open_basedir restrictions actually decreases system security and that open_basedir restrictions should be reinstated when no testing is being done.

nedjo’s picture

salvis’s picture

@leonardjo: I couldn't agree more!

Asking the admin to turn off the open_basedir restriction is a work-around (better than leaving him in the dark about why Testing is not working), but it's not a solution.

open_basedir is a useful security feature.

Damien Tournoud’s picture

open_basedir is part of the deprecated (and removed from trunk) security mascarade that is the Safe Mode feature. The current wording is fine. No one in his right mind should need to run with open_basedir.

salvis’s picture

@Damien Tournoud: Please provide pointers to some factual evidence that supports your strong words.

Dave Reid’s picture

Status: Closed (fixed) » Active

All I've tried searching for just now shows that open_basedir is a different item from Safe mode, which the latter is indeed deprecated, but not the former. Other things like disable_functions and disable_classes show nothing about being a part of safe mode or being deprecated.

catch’s picture

Category: bug » task
Priority: Critical » Normal
sun’s picture

Category: task » bug
Priority: Normal » Critical

I was equally confused/surprised by @Damien Tournoud's follow-up, and am pretty sure that safe_mode was deprecated in favor of open_basedir.

Docs on safe_mode contain a bold warning about being deprecated, also referring to open_basedir as replacement.

sun’s picture

Priority: Critical » Normal

Sorry, x-post. Didn't someone post a workaround for underlying cURL problem that needs open_basedir to be unset?

maths222’s picture

Status: Active » Needs review

#30: simpletest_openbasedir.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, simpletest_openbasedir.patch, failed testing.

sun’s picture

Issue tags: +Testing system
regilero’s picture

open_basedir is certainly not a "security mascarade", it the first step of a good installation. Am I wrong or none ever tried to fix the problem?
We can find on the net some workaround where the FOLLOWONLOCATION is handled by some curl php code in case of open_basedir restrictions http://stackoverflow.com/questions/2511410/curl-follow-location-error

This seems a lot more serious than simply asking for any PHP security settings removal before running tests (how do you test the security restrictions then?), it's like a big "please chmod 777" :-)

fantom84’s picture

subscribe
removing open_basedir is a bad idea

Rabikumar’s picture

Status: Needs work » Needs review
Issue tags: -Testing system
rehanmerchant’s picture

Issue tags: +Testing system
chx’s picture

Title: Testing incompatible with open_basedir restriction » Remove the open_basedir requirement check
Category: bug » task
Status: Needs review » Active
Issue tags: +Novice

This issue is a mess lately. It's not clear what it is about and noone is helping by filing a clear report on what's desired instead just clicks around destroying useful past testing results.

As far as I can see, both conflicting settings has been resolved since and so we can remove this check, effectively reverting. This requires someone to do more work than write a nonsense comment: one needs to remove the requirements check, set open_basedir and run tests. I think they would pass now.

chx’s picture

Issue tags: -Novice

Maybe not novice because of the php.ini edit requirement.

MrHaroldA’s picture

I ran into the open_basedir requirement too, and found this in simpletest.install:

  // SimpleTest currently needs 2 cURL options which are incompatible with
  // having PHP's open_basedir restriction set.
  // See http://drupal.org/node/674304.
  $requirements['php_open_basedir'] = array(
    'title' => $t('PHP open_basedir restriction'),
    'value' => $open_basedir ? $t('Enabled') : $t('Disabled'),
  );

#674304: Make testing compatible with open_basedir restriction and this issue both mention that these cURL options conflict with open_basedir:

CURLOPT_COOKIEJAR => NULL
CURLOPT_FOLLOWLOCATION => TRUE

When searching Drupal's codebase for these options, I only found these in drupal_web_test_case.php

// Some versions/configurations of cURL break on a NULL cookie jar, so
// supply a real file.
if (empty($this->cookieFile)) {
  $this->cookieFile = $this->public_files_directory . '/cookie.jar';
}
$curl_options = array(
  CURLOPT_COOKIEJAR => $this->cookieFile,
  CURLOPT_URL => $base_url,
  CURLOPT_FOLLOWLOCATION => FALSE,
  CURLOPT_RETURNTRANSFER => TRUE,
  CURLOPT_SSL_VERIFYPEER => FALSE, // Required to make the tests run on HTTPS.
  CURLOPT_SSL_VERIFYHOST => FALSE, // Required to make the tests run on HTTPS.
  CURLOPT_HEADERFUNCTION => array(&$this, 'curlHeaderCallback'),
  CURLOPT_USERAGENT => $this->databasePrefix,
);

... so I guess the whole requirement can be omitted from simpletest_requirements()? I've attached a patch that removes it.

MrHaroldA’s picture

Status: Active » Needs review
BarisW’s picture

Status: Needs review » Needs work

The last submitted patch, 55: 323110_55_remove_open_basedir_requirement.patch, failed testing.

BarisW’s picture

Version: 7.x-dev » 8.1.x-dev
Status: Needs work » Needs review

Duh

BarisW’s picture

Status: Needs review » Active
BarisW’s picture

Status: Active » Needs review

Sorry for the noise to re-activate the testbot.

webflo’s picture

Status: Needs review » Needs work

The last submitted patch, 60: 323110_55_remove_open_basedir_requirement.patch, failed testing.

BarisW’s picture

Status: Needs work » Needs review
FileSize
1.68 KB

The previous patch was made against 8.0.x-dev, this one is against 8.1-dev

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed 1b8b2f5 on 8.3.x
    #323110 by nenne, gpk, and sun: Add requirement check for open_basedir...

  • webchick committed 1b8b2f5 on 8.3.x
    #323110 by nenne, gpk, and sun: Add requirement check for open_basedir...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed 1b8b2f5 on 8.4.x
    #323110 by nenne, gpk, and sun: Add requirement check for open_basedir...

  • webchick committed 1b8b2f5 on 8.4.x
    #323110 by nenne, gpk, and sun: Add requirement check for open_basedir...

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joseph.olstad’s picture

Status: Needs review » Fixed

D8 fixed.

Backport to D7 is ready, see backport issue:
#2926150: D7 Remove the open_basedir requirement check

David_Rothstein’s picture

Status: Fixed » Needs review

This is not fixed - the code which #62 removes is still in Drupal 8.

joseph.olstad’s picture

Thanks David for verifying.

#47 says it all, this open_dir check needs to be removed. It was put in as a hack for something that was fixed long, long ago.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joseph.olstad’s picture

Pancho’s picture

Rerolled #62 against 8.7.x-dev.
Don't know if simpletest can properly test itself, so I'm also going to test it manually on shared hosting.

Pancho’s picture

Pretty slow on shared hosting but it works flawlessly despite of open_basedir restrictions being active. :)
Let's some more people test it on their systems, and then we should finally go forward.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mirroar’s picture

Status: Needs review » Reviewed & tested by the community

I can confirm that the patch from #78 works and looks good to me. I was able to enable the testing module and run tests successfully. Carefully setting this to RTBC.

  • catch committed f9f013c on 8.8.x
    Issue #323110 by gpk, BarisW, MrHaroldA, Pancho, webflo, sun: Remove the...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Glad we do separate 7.x and 8.x issues these days.

Committed f9f013c and pushed to 8.8.x. Thanks!

kim.pepper’s picture

Can we update the IS as to why we can safely remove this?

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.