setting

$conf = array(
  'anonymous'   => 'Visitor',
  'site_name'   => 'Drupal 6 Testing Site',
  'site_mail'   => 'mail@example.com',

in ./sites/default/settings.php will give the notice: Drupal already installed during installation.
You won't be able to set an administrator account and when visiting your site, you won't be able to login.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

agentrickard’s picture

Is this an installer problem or a documentation problem?

I think documentation. Since the installer (re)writes settings.php, you should not be editing that file by hand before running install.php.

alexanderpas’s picture

it is NOT a documentation problem.

installer doesn't write to settings.php when $db_url is already pointing to a EMPTY database.

think about re-installations after dropping all tables in your drupal database.
also, think about multisite installations.

agentrickard’s picture

Fair enough, thanks.

dvessel’s picture

Priority: Critical » Normal
Status: Active » Postponed (maintainer needs more info)

How is this critical? This is how Drupal 5 and earlier behaves. You drop the db, then scrap your settings.php and start over.

theborg’s picture

Version: 6.0-rc2 » 6.x-dev
Status: Postponed (maintainer needs more info) » Active

The security check in the 'configure' stage of the installation is not compatible with the possibility of overiding variables.

  if ($task == 'configure') {
    if (variable_get('site_name', FALSE) || variable_get('site_mail', FALSE)) {
      // Site already configured: This should never happen, means re-running
      // the installer, possibly by an attacker after the 'install_task' variable
      // got accidentally blown somewhere. Stop it now.
      install_already_done_error();
    }
    $form = drupal_get_form('install_configure_form', $url);

    if (!variable_get('site_name', FALSE) && !variable_get('site_mail', FALSE)) {
alexanderpas’s picture

Title: setting Variable overrides makes installing properly impossible! » security check in 'configure' stage not compatible with overiding variables: site_name and site_mail
Priority: Normal » Critical

more detailed:

The security check in the 'configure' stage of the installation is not compatible with the possibility of overiding variables: site_name and site_mail

putting this back to critical because it'll BREAK the installation since it'll leave you with the placeholder-for-uid-1 in the database without you being able to set an administrator account for yourself.

remember, you can reproduce this behavior just by setting: site_name or site_mail in settings.php

note that site_name is actually used as an example in settings.php

catch’s picture

Status: Active » Closed (won't fix)

It's trivial to comment these out before you reinstall. If you're editing this part of settings.php, then you know how to do this. So won't fixing this.

I've reinstalled drupal dozens of times with db information in the settings.php, that works fine, and would be critical if broken, this is not the same thing at all.

Damien Tournoud’s picture

Version: 6.x-dev » 8.x-dev
Priority: Critical » Normal
Status: Closed (won't fix) » Active

Let's reopen.

This "security check" feels really dumb to me. It makes it impossible to have more complex installation profiles and makes it impossible to reinstall a site that has its site name configured in settings.php (which is a very good idea, because it makes the proper site name appears in the maintenance page when the database is not available).

Damien Tournoud’s picture

Status: Active » Needs review
FileSize
1.12 KB
1.12 KB

I suggest we just remove the check, which is just babysitting broken installations. If the installation is missing a variable, it is very likely it is missing plenty others. There is nothing we can to do protect users messing up with the database against themselves.

Damien Tournoud’s picture

Bumping this.

Damien Tournoud’s picture

dww’s picture

Status: Needs review » Reviewed & tested by the community

This stupidity keeps nailing me, too. DamZ is right. This is a pointless check that actively harms people trying to do the right thing. Tested manually and it works to (re)install when you already have site_name defined in settings.php.

dww’s picture

p.s. I also checked and this is *not* the only time we throw that exception, so we still want to keep the install_already_done_error() function around for the other valid cases when we hit that condition and want to abort the installation.

chx’s picture

given the impossibility to test this (no, Virginia, we are NOT changing settings.php for a test) I agree with it.

catch’s picture

I'm fine with just removing this.

Issue is not tagged for backport, but looks like it should be?

catch’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 212284-install-security-check.patch, failed testing.

dww’s picture

Status: Needs work » Needs review
FileSize
842 bytes

Rerolled for new directory layout.

alexanderpas’s picture

have a re-roll

edit: ninja'ed

agentrickard’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Hits the target and tests are green.

catch’s picture

Title: security check in 'configure' stage not compatible with overiding variables: site_name and site_mail » security check in 'configure' stage not compatible with overriding variables: site_name and site_mail
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs backport to D7

Committed/pushed to 8.x.

Looks backportable to me.

alexanderpas’s picture

this should do it.

agentrickard’s picture

Status: Needs review » Reviewed & tested by the community

Hooray for removing old cruft.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I guess given that this has 3-4 members of the security team saying +1, the security check is ok to remove. I'm still not quite sure how people are actively hitting this bug in the field, though. Hrm.

Anyway, committed and pushed to 7.x. Thanks!

dww’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

@webchick: To reproduce:

- Get a clean copy of core (prior to the fix, of course)
- Put $conf['site_name'] = 'local D7 test site'; in your settings.php (perhaps along with your DB credentials to save time).
- Visit install.php or try to run drush site-install

I don't think that's a crazy edge-case. It's pretty common to want to specify site_name in settings.php. Once you do that, you can no longer reinstall the site (e.g. rebuilding a site from a .make file and features or something).

Anyway, yay this is gone from D7 and D8, thanks! Is it worth back-porting this all the way to D6? The same basic bug is there, but the code in D6 lives directly in install.php so this needs to be ported if we want it. I'm in favor, but I don't want to spend energy if it's going to be shot down. Thoughts?

Thanks,
-Derek

Status: Patch (to be ported) » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.