Test the interactive installer

See: #630446: Allow SimpleTest to test the non-interactive installer

Talked to @alexpott and I'm closing #630446: Allow SimpleTest to test the non-interactive installer as a duplicate. However, in that issue, @alexpott raised a concern about this hunk (which this patch just removes, but that issue moved to later in the request with a drupal_valid_test_ua() check):

+++ b/core/includes/install.core.incundefined
@@ -263,13 +264,6 @@ function install_begin_request(&$install_state) {
-  // The user agent header is used to pass a database prefix in the request when
-  // running tests. However, for security reasons, it is imperative that no
-  // installation be permitted using such a prefix.
-  elseif (isset($_SERVER['HTTP_USER_AGENT']) && strpos($_SERVER['HTTP_USER_AGENT'], "simpletest") !== FALSE) {
-    header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden');
-    exit;
-  }

.

Updated issue summary.

Patch attached adds the 403 for security back in and adds the test from #630446: Allow SimpleTest to test the non-interactive installer and tidies up a couple of things.

@chx this work is awesome...

I think it is a bug we don't test the interactive installer... look at the followup issues listed (thanks @xjm)

Does this:

-  elseif (isset($_SERVER['HTTP_USER_AGENT']) && strpos($_SERVER['HTTP_USER_AGENT'], "simpletest") !== FALSE) {
...
+  if (isset($_SERVER['HTTP_USER_AGENT']) && strpos($_SERVER['HTTP_USER_AGENT'], "simpletest") !== FALSE && !drupal_valid_test_ua()) {

actually provide any more security than this?

     // Do not install over a configured settings.php.
-    if (!empty($GLOBALS['databases'])) {
+    if (!empty($GLOBALS['databases']) && !drupal_valid_test_ua()) {
+      throw new Exception(install_already_done_error());
+    }

In other words, if we believe checking drupal_valid_test_ua() is good enough, can't we just leave the first check out after all?

It seems like drupal_valid_test_ua() might be good enough, but the reason the installer is more sensitive is that if you can access that you can completely take over the server at any time (because once you install you automatically get user 1 privileges). Meanwhile, the other checks of drupal_valid_test_ua() throughout core might really only need to protect against the situation where an attacker stumbles across an in-progress (or never cleaned up) simpletest run and tries to spoof visiting that? (Which is itself only dangerous if that simpletest run also happened to configured the test site insecurely for the purposes of the test, e.g. gave anonymous or authenticated users high level permissions... so overall a lot more far-fetched.)

The security of drupal_valid_test_ua() is also weaker than other tokens in Drupal since by definition it cannot use anything stored in the database to construct it (only the filesystem):

function drupal_generate_test_ua($prefix) {
....
    // We use the salt from settings.php to make the HMAC key, since
    // the database is not yet initialized and we can't access any Drupal variables.
    // The file properties add more entropy not easily accessible to others.
    $key = $drupal_hash_salt . filectime(__FILE__) . fileinode(__FILE__);

On the other hand, I'm having trouble figuring out a scenario where someone could have access to all the file properties above and not also have access to the database.

I think there was an issue where this was discussed before but I can't seem to find it now.

As much as I think this issue is awesome, we should be accurate with the categorization; incomplete test coverage is not a critical bug. If it were, we'd be in trouble :)

And we do test the interactive installer already (doesn't the testbot do it for us every time it tests a patch?)... what's nice about this issue, though, is that it allows us to test as many installation options as we want to, whereas the testbot only runs a single basic one.

By the way, see my comment at #630446-18: Allow SimpleTest to test the non-interactive installer for an idea on how we might be able to combine these two issues together (because testing the interactive and non-interactive installer should ideally not be different from the perspective of the person writing the test even if it's different underneath) but for now I think we should keep them separate since this issue is inherently a lot more difficult than that one.

At least one of these two issues is a critical task, because we consistently have critical regressions due to the installer being broken. Rather than playing tug-of-status I'll leave it to others to decide how to combine them. :)

Fixing silly mistake in the SimpleTestTest...

I think that the conditions below are not quite equivalent and the explicit 403 preferable. The second condition will pass if $GLOBALS['databases'] is not empty - regardless of the result of drupal_valid_test_ua(). The first condition ensures that if the simpletest user agent is set you must also have a valid token.

+++ b/core/includes/install.core.incundefined
@@ -279,6 +273,13 @@ function install_begin_request(&$install_state) {
+  // The user agent header is used to pass a database prefix in the request when
+  // running tests. If prefix is set, ensure that we have a valid token.
+  if (isset($_SERVER['HTTP_USER_AGENT']) && strpos($_SERVER['HTTP_USER_AGENT'], "simpletest") !== FALSE && !drupal_valid_test_ua()) {
+    header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden');
+    exit;
+  }

@@ -427,7 +428,17 @@ function install_begin_request(&$install_state) {
     // Do not install over a configured settings.php.
-    if (!empty($GLOBALS['databases'])) {
+    if (!empty($GLOBALS['databases']) && !drupal_valid_test_ua()) {
+      throw new Exception(install_already_done_error());
+    }

However, the essential question is: Is drupal_valid_test_ua() code enough?

I think that the conditions below are not quite equivalent and the explicit 403 preferable. The second condition will pass if $GLOBALS['databases'] is not empty - regardless of the result of drupal_valid_test_ua().

I think you mean "empty" rather than "not empty"? My thought was that if $GLOBALS['databases'] is empty then it doesn't matter because anyone can already use install.php in that case (regardless of Simpletest or not). However, with all the Simpletest-specific settings.php swapping going on now perhaps that is not guaranteed.... and also the explicit 403 check is a lot easier to understand when reading the code, so yeah, I agree, let's just keep both.

But, the hash salt is a tricky one to guess.

It's not just about guessing it; if you manage to get read access to a site's code repository, then you can find the hash salt directly in settings.php. Whereas for other Drupal tokens you would need access to the code repository and also would need to get your hands on a database backup (with the private key) in order to reconstruct the token.

I think @pwolanin would be a good reviewer here; as far as I remember he was responsible for adding the Simpletest check to the installer originally.

Tagging for security review.

I'm unsure about the logic here:

+  }
+  if (!$test_prefix) {
+    $settings['drupal_hash_salt'] = (object) array(

Looks like you don't need the if() again, but it could be part of the else{} above?

So, I'm wondering if we can lock this down further so basically this test will fail unless there is specific setup in advance? Maybe an extra flag or key in settings.php that's not populated in a default install?

We are removing a blanket prohibition on the interactive install and opening a significant potential attack surface.

Hmm, ok - that was apparent from the patch - would be good to explain why that setting is protective.

Fixing a immensely minor nit :)

But I want this patch in - so removing a possible objection :)

1. +++ b/core/includes/install.core.incundefined
   @@ -432,7 +436,7 @@ function install_begin_request(&$install_state) {
        // Do not install over a configured settings.php.
   -    if (!empty($GLOBALS['databases'])) {
   +    if (!empty($GLOBALS['databases']) && !drupal_valid_test_ua()) {
          throw new Exception(install_already_done_error());
        }
   

   I hope we're being really defensive against a bug with our later code installing over the main settings.php. :) Is there we could be more explicit here or later about what we do when it is a test request?

2. +++ b/core/includes/install.core.incundefined
   @@ -1072,8 +1080,11 @@ function install_settings_form_validate($form, &$form_state) {
      // TODO: remove when PIFR will be updated to use 'db_prefix' instead of
   -  // 'prefix' in the database settings form.
   -  $database['prefix'] = $database['db_prefix'];
   +  // 'prefix' in the database settings form with the non-interactive
   +  // installer.
   +  if (!$test_prefix) {
   +    $database['prefix'] = $database['db_prefix'];
   

   This comment is really confusing now and should be updated and clarified.

3. +++ b/core/includes/install.core.incundefined
   @@ -1133,14 +1144,34 @@ function install_settings_form_submit($form, &$form_state) {
   +  $database = $form_state['storage']['database'];
   ...
   +  else {
   +    // Because of the test specific code above, this assignment here must be
   +    // kept simple and must not call any functions to avoid creating a tested
   +    // and a non-tested code path.
   +    $settings['databases']['default']['default'] = (object) array(
   +      'value'    => $database,
   +      'required' => TRUE,
   

   It took me like 5 minutes of staring at this before I figured out what it was on about. This is basically about the $database assignment above, to ensure that the same values are used for both paths? I guess? Can we clarify, and add a comment above $database as well if that's important?

4. +++ b/core/includes/install.core.incundefined
   @@ -1133,14 +1144,34 @@ function install_settings_form_submit($form, &$form_state) {
   +  if ($test_prefix = drupal_valid_test_ua()) {
   +    // This is test specific code, however it is very small and contained.
   +    foreach ($form_state['storage']['database'] as $k => $v) {
   +      if ($k != 'password') {
   +        $settings['databases']['default']['default'][$k] = (object) array(
   +          'value'    => $v,
   +          'required' => TRUE,
   +        );
   +      }
   

   A comment explaining what we're doing and why would be more valuable than defending this hunk being where it doesn't seem to belong. Maybe it should be factored out into a separate function? In any case, what are we sorting out of the database submission here and why? Seems like code smell.

5. +++ b/core/modules/system/lib/Drupal/system/Tests/InstallerTest.phpundefined
   @@ -0,0 +1,144 @@
   +    // Temporary fix so that when running from run-tests.sh we don't get an
   +    // empty current path which would indicate we're on the home page.
   +    $path = current_path();
   +    if (empty($path)) {
   +      _current_path('run-tests');
   +    }
   

   Er. How temporary is this temporary fix? Are we going to fix it here or in a followup? Could you explain a little more?

   My guess is: Normally, an empty path is considered to be the front page. During installation, no path is set because we're running install.php, not index.php. But why does this matter to simpletest, and why is it different from the upgrade path tests? I checked in UpgradePathTestBase and there is nothing like this.

6. +++ b/core/modules/system/lib/Drupal/system/Tests/InstallerTest.phpundefined
   @@ -0,0 +1,144 @@
   +  protected function refreshVariables() {
   +    if (!empty($this->setup)) {
   +      parent::refreshVariables();
   +    }
   +  }
   

   Needs a docblock. Why do we need to override this?

7. +++ b/core/includes/install.core.incundefined
   @@ -1133,14 +1144,34 @@ function install_settings_form_submit($form, &$form_state) {
   +    // Tests must not override the hash salt so that the user agent can be
   +    // verified.
   +    $settings['drupal_hash_salt'] = (object) array(
   +      'value'    => drupal_hash_base64(drupal_random_bytes(55)),
   +      'required' => TRUE,
   +    );
   

   Uh... isn't this the else hunk, where we're not in a test? The comment doesn't make any sense to me here.

Minor things I'll clean up:

1. +++ b/core/modules/system/lib/Drupal/system/Tests/InstallerTest.phpundefined
   @@ -0,0 +1,144 @@
   +    // We re-using a CURL connection here. If that connection still has certain
   +    // options set, it might change the GET into a POST. Make sure we clear out
   +    // previous options.
   

   The first sentence no verb. :)

2. +++ b/core/modules/system/lib/Drupal/system/Tests/InstallerTest.phpundefined
   @@ -0,0 +1,144 @@
   +   * This override is necessary because the parent drupalGet() calls t() which
   +   * is not working in early install.
   

   So, there's a similar, clearer comment on UpgradePathTestBase, which I'll use here. But my question is, why the heck are they calling t()? We already don't translate assertion messages. I'll file a separate issue for this.

3. +++ b/core/includes/install.core.incundefined
   @@ -280,6 +273,17 @@ function install_begin_request(&$install_state) {
   +  // If the hash salt leaks, it becomes possible to forge a valid testing user
   +  // agent, install a new Drupal and take over the oirginal site. To avoid
   

   "Original" is misspelled. Also, a comma is missing.

4. +++ b/core/modules/system/lib/Drupal/system/Tests/InstallerTest.phpundefined
   @@ -0,0 +1,144 @@
   +class InstallerTest extends WebTestBase {
   ...
   +  function testInstaller() {
   +    $this->drupalGet('user');
   

   These need docblocks.

5. +++ b/core/modules/system/lib/Drupal/system/Tests/InstallerTest.phpundefined
   @@ -0,0 +1,144 @@
   +    // When running tests through the Simpletest UI (vs. on the command line),
   +    // Simpletest's batch conflicts with the installer's batch. Batch API does
   +    // not support the concept of nested batches (in which the nested is not
   +    // progressive), so we need to temporarily pretend there was no batch.
   +    // Backup the currently running Simpletest batch.
   

   Nitpicks: "SimpleTest" should be either CamelCase or lowercase, and "Back up" should be two words when used as a verb.

6. +++ b/core/includes/install.core.incundefined
   @@ -280,6 +273,17 @@ function install_begin_request(&$install_state) {
   +  // also a special test specific settings.php overriding conf_path().
   

   test-specific, overriding the.

Back to our regularly scheduled wizard.

OK then let's do this: aside from a valid token you need to be able to write the filesystem along with creating a new dir which we somewhat presumed not even "leaky" upload scripts do.

I think this makes sense. Although is it possible one of these directories (and the associated settings.php file) is left on the filesystem due to an aborted test run, and the attacker could find and reuse that? Maybe there's no way they could reinstall in that case though - I'm not sure - in which case it would be no different from the situation we have now.

-  elseif (isset($_SERVER['HTTP_USER_AGENT']) && strpos($_SERVER['HTTP_USER_AGENT'], "simpletest") !== FALSE) {
-    header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden');
-    exit;
-  }
....
+  if ($install_state['interactive'] && drupal_valid_test_ua() && !settings()->get('simpletest_conf_path')) {
+    header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden');
+    exit;
+  }

It took me a while to figure why it was OK for the new code to let through a request with an invalid drupal_valid_test_ua(). It's probably OK, but in the spirit of making the code easier to understand (and also reducing the attack surface) I think it would be better if the installer rejected invalid ones outright as soon as possible. So basically, keeping both code blocks above around (rather than deleting the first), but adding !drupal_valid_test_ua() to the if() statement in the first (similar to earlier patches). Yes, I do realize this is pretty much a 180 degree turnaround from my earlier opinion in this issue...

So basically, keeping both code blocks above around (rather than deleting the first), but adding !drupal_valid_test_ua() to the if() statement in the first (similar to earlier patches).

Or combining the two code blocks, I guess, since you probably have to move the first one later in order to add drupal_valid_test_ua() to it. So something like this:

  if ($install_state['interactive'] && isset($_SERVER['HTTP_USER_AGENT']) && strpos($_SERVER['HTTP_USER_AGENT'], "simpletest") !== FALSE && (!drupal_valid_test_ua() || !settings()->get('simpletest_conf_path'))) {
    header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden');
    exit;
  }

Right, that's why I eventually concluded it was OK to remove, but in the name of defense in depth (and easier to understand code) I think it still makes a lot of sense to keep.

Basically, if someone is trying to access install.php with a forged simpletest user agent header, we know they're up to serious no good... so why not reject the request as early as we possibly can?

#32: 1961938_32.patch queued for re-testing.

Rerolled patch against latest dev.

Changing status to needs review.

I just spent a day wrestling with getting D8 running, and finally got it working after resolving two issues:

1 - Parts of it were exceeding the 30 second execution time. I don't know if there's much that can be done about this. Is calling set_time_limit() is kosher?

2 - Guzzle was failing because the curl extension wasn't enabled. Should the script check to see if this and other extensions are enabled?

I realized after posting that this issue was about writhing automated tests for the installer, not manual testing the installer for usability. I will try to find a better issue for my concerns. Sorry.

I was learning how to re-roll a patch this morning at DrupalCon Portland. Just out of curiosity, what was wrong with my patch in #40. Any ideas?

+++ b/core/includes/install.core.incundefined
@@ -266,13 +266,6 @@ function install_begin_request(&$install_state) {
-  // The user agent header is used to pass a database prefix in the request when
-  // running tests. However, for security reasons, it is imperative that no
-  // installation be permitted using such a prefix.
-  elseif (isset($_SERVER['HTTP_USER_AGENT']) && strpos($_SERVER['HTTP_USER_AGENT'], "simpletest") !== FALSE) {
-    header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden');
-    exit;

This is the only part I'm very unsure of. Why is this no longer needed? Obviously an in-code comment isn't necessary, but if you could say why in the issue I'd appreciate it.

+++ b/core/modules/system/lib/Drupal/system/Tests/InstallerTest.phpundefined
@@ -0,0 +1,156 @@
+    // When running from run-tests.sh we don't get an mpty current path which

empty is misspelled.

+++ b/core/modules/system/lib/Drupal/system/Tests/InstallerTest.phpundefined
@@ -0,0 +1,156 @@
+   * During setup(), drupalPost costs refreshVariables which tries to read

is "costs" the right word?

Otherwise this is actually very sensible and readable when you really look at it. There are some great changes in here. Thanks for keeping this going chx.

EDIT: My question was answered above, see #36

+++ b/core/includes/errors.incundefined
@@ -136,7 +136,9 @@ function _drupal_decode_exception($exception) {
+  unset($decode['backtrace']);

So why do we remove the backtrace, doesn't it have valid information in there? I guess this is the same problem as https://drupal.org/node/1158322#comment-7189458

+++ b/core/includes/install.core.incundefined
@@ -1080,6 +1084,10 @@ function install_settings_form($form, &$form_state, &$install_state) {
+  if ($test_prefix = drupal_valid_test_ua()) {
+    $database['prefix'] = $test_prefix;
+    $database['password'] = $GLOBALS['databases']['default']['default']['password'];
+  }

Some comment would be nice.

+++ b/core/modules/system/lib/Drupal/system/Tests/InstallerTest.phpundefined
@@ -0,0 +1,156 @@
+    $this->drupalGet('core/install.php?langcode=en&profile=minimal');

drupalGet has parameters for query so let's use it.

It is important that we can test our installer.

I looked at this for core gates doc stuff, and didn't see anything blocking.
These are great comments in this patch.

I noticed these tiny things, so just did them.

1.

+++ b/core/includes/install.core.incundefined
@@ -1086,9 +1096,11 @@ function install_settings_form_validate($form, &$form_state) {
+  // TODO: PIFR uses 'db_prefix' instead of 'prefix'. Remove this when it gets

https://drupal.org/node/1354#todo

not blocking, since this line is changing, might as well use the standard @todo.

2.

+++ b/core/modules/system/lib/Drupal/system/Tests/InstallerTest.phpundefined
@@ -0,0 +1,156 @@
+  protected function setUp() {

https://drupal.org/node/325974

public function setUp() {

3.

+++ b/core/modules/system/lib/Drupal/system/Tests/InstallerTest.phpundefined
@@ -0,0 +1,156 @@
+    // changed, since Drupal\Core\Utility\CacheArray implementations attempt to

https://drupal.org/node/1354#classes

"If you use a namespace in documentation, always make sure it is a fully-qualified namespace (beginning with a backslash)."

+++ b/core/modules/system/lib/Drupal/system/Tests/InstallerTest.phpundefined
@@ -23,6 +23,9 @@ public static function getInfo() {
+  /**
+   * {@inheritdoc}
+   */
   protected function setUp() {

https://drupal.org/node/325974

"There is no PHPDoc on this function since it is an inherited method."

---

Maybe we want to update the testing standards doc to use [@inheritdoc}? But I think we dont start doing it here. #2007766: {@inheritdoc} in tests on setUp and getInfo

I left the setUp protected, if it really should be, then an issue needs to be opened to update the standard.
(see also similar conversation in the past: #1757566-49: Convert user account e-mail templates to configuration system. maybe can be fixed as part of #1869794: Update tests coding standards doc and make consistant with 1354 where appropriate).
----
note interdiff is to #54 @dawehner's, so can skip mine in #55.
----
Anyway since we are only finding these small things here, looks good to me.

should be rtbc when the bot comes back.

Took a look through this and I think it's reasonable. Committed/pushed to 8.x.