Problem/Motivation

The early installer is full of quirks and has little test coverage.

Proposed resolution

Use the overrides introduced in #1949724: Allow simpletest child sites to additionally load a test-specific settings.php to allow testing anonymous and configless updates to go for testing.

Remaining tasks

The site configuration form needs to be filled out and the ability to login asserted. Profile and language is hardwired ATM.

User interface changes

None.

API changes

None.

Both are included in this patch but I expect both to get in within a reasonable timeframe. Meanwhile we can finetune this.

Followups

The following issues need test coverage once this is fixed:

Files: 
CommentFileSizeAuthor
#56 1961938_56.patch13.57 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 57,687 pass(es).
[ View ]
#56 interdiff-54da_wehner-56.txt1.55 KBYesCT
#55 1961938_54.patch13.61 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#55 interdiff-53-54.txt1.63 KBYesCT
#54 drupal-1961938-54.patch13.6 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#54 interdiff.txt1.39 KBdawehner
#53 1961938_53.patch13.61 KBchx
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#53 interdiff.txt1.23 KBchx
#52 1961938_51.patch13.49 KBchx
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#52 interdiff.txt1.02 KBchx
#49 1961938_49.patch13.49 KBchx
PASSED: [[SimpleTest]]: [MySQL] 55,824 pass(es).
[ View ]
#47 interdiff.txt417 byteschx
#45 1961938_45.patch13.63 KBchx
FAILED: [[SimpleTest]]: [MySQL] 55,728 pass(es), 0 fail(s), and 3 exception(s).
[ View ]
#45 diffdiff.txt1.75 KBchx
#40 1961938_40.patch13.7 KBattheshow
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#32 1961938_32.patch13.66 KBchx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1961938_32.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#32 interdiff.txt4.96 KBchx
#31 simpletest-installer-1961938-31.patch13.06 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 54,355 pass(es).
[ View ]
#31 installer-interdiff.txt4.11 KBxjm
#29 28-29-interdiff.txt763 bytesalexpott
#29 1961938_29.patch12.76 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 54,526 pass(es).
[ View ]
#28 1961938_28.patch12.76 KBchx
PASSED: [[SimpleTest]]: [MySQL] 54,545 pass(es).
[ View ]
#28 interdiff.txt1021 byteschx
#24 1961938_24.patch12.55 KBchx
PASSED: [[SimpleTest]]: [MySQL] 54,449 pass(es).
[ View ]
#24 interdiff.txt506 byteschx
#22 1961938_22.patch12.49 KBchx
PASSED: [[SimpleTest]]: [MySQL] 54,124 pass(es).
[ View ]
#22 interdiff.txt1.1 KBchx
#20 1961938_20.patch13.73 KBchx
PASSED: [[SimpleTest]]: [MySQL] 54,214 pass(es).
[ View ]
#20 interdiff.txt1.21 KBchx
#19 1961938_19.patch13.43 KBchx
PASSED: [[SimpleTest]]: [MySQL] 54,106 pass(es).
[ View ]
#19 interdiff.txt326 byteschx
#17 1961938_17.patch13.22 KBchx
FAILED: [[SimpleTest]]: [MySQL] 31,916 pass(es), 13,016 fail(s), and 1,646 exception(s).
[ View ]
#16 1961938_16.patch18.92 KBchx
PASSED: [[SimpleTest]]: [MySQL] 54,046 pass(es).
[ View ]
#16 interdiff.txt3.74 KBchx
#14 1961938_14.patch19.66 KBchx
PASSED: [[SimpleTest]]: [MySQL] 54,072 pass(es).
[ View ]
#14 interdiff.txt678 byteschx
#13 8-13-interdiff.txt719 bytesalexpott
#13 1961938_13.patch18.92 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 54,092 pass(es).
[ View ]
#8 5-8-interdiff.txt3.61 KBalexpott
#8 1961938_8.patch18.92 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 54,176 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#5 1961938_5.patch18.05 KBchx
PASSED: [[SimpleTest]]: [MySQL] 54,249 pass(es).
[ View ]
#3 1961938_3.patch30.97 KBchx
FAILED: [[SimpleTest]]: [MySQL] 54,094 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#1 1961938_1-do-not-test.patch11.97 KBchx
installertest.patch30.06 KBchx
FAILED: [[SimpleTest]]: [MySQL] 54,088 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

Comments

chx’s picture

StatusFileSize
new11.97 KB

This is what the patch will be once those two are in. It might be easier to review.

Status:Needs review» Needs work

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

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new30.97 KB
FAILED: [[SimpleTest]]: [MySQL] 54,094 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1961938_3.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new18.05 KB
PASSED: [[SimpleTest]]: [MySQL] 54,249 pass(es).
[ View ]

I forgot to inherit the password during install form submit and system module install. One line change really, just hard to see because meanwhile webchick committed the override patch (huzzah!)

xjm’s picture

xjm’s picture

Issue tags:+security

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;
-  }

.

xjm’s picture

Issue summary:View changes

Updated issue summary.

alexpott’s picture

Category:task» bug
StatusFileSize
new18.92 KB
FAILED: [[SimpleTest]]: [MySQL] 54,176 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new3.61 KB

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)

Status:Needs review» Needs work

The last submitted patch, 1961938_8.patch, failed testing.

David_Rothstein’s picture

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):

<?php
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.

David_Rothstein’s picture

Category:bug» task
Priority:Critical» Major

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.

xjm’s picture

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. :)

alexpott’s picture

StatusFileSize
new18.92 KB
PASSED: [[SimpleTest]]: [MySQL] 54,092 pass(es).
[ View ]
new719 bytes

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?

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new678 bytes
new19.66 KB
PASSED: [[SimpleTest]]: [MySQL] 54,072 pass(es).
[ View ]

drupal_valid_test_ua is pretty secure, yes. The fileXtime provides some security -- you can have a somewhat good guess and only need to try, say, less than a million variants. Not a lot by today's standards. But, the hash salt is a tricky one to guess. Let's make sure it's there.

David_Rothstein’s picture

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.

chx’s picture

StatusFileSize
new3.74 KB
new18.92 KB
PASSED: [[SimpleTest]]: [MySQL] 54,046 pass(es).
[ View ]

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. One hunk of interdiff is just keeping up with the now-RTBC installer error report patch. This patch will be ready the moment that gets in.

chx’s picture

StatusFileSize
new13.22 KB
FAILED: [[SimpleTest]]: [MySQL] 31,916 pass(es), 13,016 fail(s), and 1,646 exception(s).
[ View ]

Keeping up with HEAD. This should be ready.

Status:Needs review» Needs work

The last submitted patch, 1961938_17.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new326 bytes
new13.43 KB
PASSED: [[SimpleTest]]: [MySQL] 54,106 pass(es).
[ View ]

*blush*

chx’s picture

StatusFileSize
new1.21 KB
new13.73 KB
PASSED: [[SimpleTest]]: [MySQL] 54,214 pass(es).
[ View ]

alexpott drawn my attention to drupal_get_has_salt() -- surely the commit of that broke my patch cos I didnt change a thing between #16 and #17 in bootstrap.inc. So let's use it.

xjm’s picture

Issue tags:+needs security review

Tagging for security review.

chx’s picture

StatusFileSize
new1.1 KB
new12.49 KB
PASSED: [[SimpleTest]]: [MySQL] 54,124 pass(es).
[ View ]

Actually? drupal_get_hash_salt() never returns empty so we can remove all the bootstrap.inc changes! I asked pwolanin to review this per David Rothstein's request, hopefully he can RTBC it.

chx’s picture

Issue summary:View changes

Updated issue summary.

pwolanin’s picture

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?

chx’s picture

StatusFileSize
new506 bytes
new12.55 KB
PASSED: [[SimpleTest]]: [MySQL] 54,449 pass(es).
[ View ]

I am glad if that's all the problems you can find with this patch :)

pwolanin’s picture

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.

chx’s picture

> Maybe an extra flag or key in settings.php that's not populated in a default install?

Yes, that's done.

<?php
 
if ($install_state['interactive'] && drupal_valid_test_ua() && !settings()->get('simpletest_conf_path')) {
   
header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden');
    exit;
  }
?>
pwolanin’s picture

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

chx’s picture

StatusFileSize
new1021 bytes
new12.76 KB
PASSED: [[SimpleTest]]: [MySQL] 54,545 pass(es).
[ View ]
alexpott’s picture

StatusFileSize
new12.76 KB
PASSED: [[SimpleTest]]: [MySQL] 54,526 pass(es).
[ View ]
new763 bytes

Fixing a immensely minor nit :)

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

xjm’s picture

Assigned:chx» xjm
Status:Needs review» Needs work
  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.

xjm’s picture

Assigned:xjm» chx
Status:Needs work» Needs review
StatusFileSize
new4.11 KB
new13.06 KB
PASSED: [[SimpleTest]]: [MySQL] 54,355 pass(es).
[ View ]

Back to our regularly scheduled wizard.

chx’s picture

StatusFileSize
new4.96 KB
new13.66 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1961938_32.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
David_Rothstein’s picture

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...

David_Rothstein’s picture

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:

<?php
 
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;
  }
?>
chx’s picture

There is no point in filtering for simpletest* as the db prefix is only used when the hash is valid, that's why I deleted the old code: it made no sense.

David_Rothstein’s picture

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?

chx’s picture

A forged simpletest agent will behave the same as visiting install.php and terminate with install already done exception. There's no point in adding code to stop it early; it stops early enough. It makes it easier to test for install already done exceptions, in fact, if we don't.

benjifisher’s picture

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

Status:Needs review» Needs work
Issue tags:+security, +needs security review

The last submitted patch, 1961938_32.patch, failed testing.

attheshow’s picture

StatusFileSize
new13.7 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

Rerolled patch against latest dev.

attheshow’s picture

Status:Needs work» Needs review

Changing status to needs review.

alieffring’s picture

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?

chx’s picture

1. this test is not particularly different to any other, that should not be necessary

2. All tests would fail if cURL is disabled. I thought simpletest had curl as a hook requirement. That's a separate issue if it doesn't.

andrew.lieffring’s picture

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.

chx’s picture

StatusFileSize
new1.75 KB
new13.63 KB
FAILED: [[SimpleTest]]: [MySQL] 55,728 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

Rerolled. As these things go, the diffdiff is fairly readable.

attheshow’s picture

Status:Needs review» Needs work

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?

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new417 bytes

Thanks for rerolling the patch! The problem is that 'value'    => drupal_hash_base64(drupal_random_bytes(55)), is now obsolete and Crypt::randomStringHashed(55) is used now instead. This caused the patch to not apply in the first place but during the manual re-apply you copy-pasted from the old one without checking what / why failed and moved. This is an unusally tricky issue as far as rerolls go, it's much more typical to have an irrelevant bit of context information to change.

Sorry for confusing you with only adding a diff between #45 and #32. Here's an interdiff for #40-#45.

Status:Needs review» Needs work

The last submitted patch, 1961938_45.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new13.49 KB
PASSED: [[SimpleTest]]: [MySQL] 55,824 pass(es).
[ View ]
tim.plunkett’s picture

+++ 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

dawehner’s picture

+++ 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.

chx’s picture

StatusFileSize
new1.02 KB
new13.49 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

See #37, other two addressed.

chx’s picture

StatusFileSize
new1.23 KB
new13.61 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

I have no clue what is happening there but strtr bails on the backtrace; it's not used; so let's nuke it. There might be a PHP bug hiding there; it might be the serialization issue; I do not know but since it's not used, it's no problem.

Actually, that core/install.php rather needs base_url, much like getUpdatePhp() in lib/Drupal/system/Tests/Upgrade/UpgradePathTestBase.php

dawehner’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new1.39 KB
new13.6 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

It is important that we can test our installer.

YesCT’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new1.63 KB
new13.61 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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)."

YesCT’s picture

StatusFileSize
new1.55 KB
new13.57 KB
PASSED: [[SimpleTest]]: [MySQL] 57,687 pass(es).
[ View ]
+++ 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.

YesCT’s picture

Status:Needs review» Reviewed & tested by the community

should be rtbc when the bot comes back.

catch’s picture

Status:Reviewed & tested by the community» Fixed

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

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary.