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.
Files: 
CommentFileSizeAuthor
#23 212284-23-install-security-check.patch822 bytesalexanderpas
PASSED: [[SimpleTest]]: [MySQL] 36,943 pass(es).
[ View ]
#20 212284-install-security-check.patch842 bytesalexanderpas
PASSED: [[SimpleTest]]: [MySQL] 33,801 pass(es).
[ View ]
#19 212284-19.install-security-check.patch842 bytesdww
PASSED: [[SimpleTest]]: [MySQL] 33,802 pass(es).
[ View ]
#12 212284-install-security-check.patch1.12 KBDamien Tournoud
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 212284-install-security-check.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#10 212284-install-security-check-d7.patch1.12 KBDamien Tournoud
#10 212284-install-security-check-d8.patch1.12 KBDamien Tournoud

Comments

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.

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.

Fair enough, thanks.

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.

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

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

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.

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

Status:Active» Needs review
StatusFileSize
new1.12 KB
new1.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.

Bumping this.

StatusFileSize
new1.12 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 212284-install-security-check.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

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.

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.

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

I'm fine with just removing this.

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

#12: 212284-install-security-check.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new842 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,802 pass(es).
[ View ]

Rerolled for new directory layout.

StatusFileSize
new842 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,801 pass(es).
[ View ]

have a re-roll

edit: ninja'ed

Status:Needs review» Reviewed & tested by the community

Looks good. Hits the target and tests are green.

Title:security check in 'configure' stage not compatible with overiding variables: site_name and site_mailsecurity 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.

StatusFileSize
new822 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,943 pass(es).
[ View ]

this should do it.

Status:Needs review» Reviewed & tested by the community

Hooray for removing old cruft.

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!

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