WordPress.org

Make WordPress Core

Opened 9 months ago

Closed 7 months ago

#39047 closed defect (bug) (fixed)

Installer tries to create nonce before options table exists

Reported by: nullvariable Owned by: rmccue
Milestone: 4.7.1 Priority: normal
Severity: normal Version: 4.7
Component: Upgrade/Install Keywords: has-patch commit fixed-major
Focuses: rest-api Cc:

Description

When creating a new install via wp-admin/install.php the following errors occur:

WordPress database error: [Table '....wp_options' doesn't exist]
INSERT INTO wp_options (option_name, option_value, autoload) ....

WordPress database error: [Table '....wp_options' doesn't exist]
INSERT INTO wp_options (option_name, option_value, autoload) ....

This error occurs with the following conditions:

  • wp-config.php created by user instead of installer
  • all salts and keys in wp-config are identical
  • database has not been installed

Attachments (5)

39047.patch (1.3 KB) - added by nullvariable 9 months ago.
patch
39047-2.patch (1.3 KB) - added by nullvariable 8 months ago.
updated patch to remove extra parentheses
39047.diff (1.1 KB) - added by pento 8 months ago.
39047.2.diff (658 bytes) - added by rmccue 7 months ago.
39047-2.patch, but using the function instead of the constant
39047.3.diff (682 bytes) - added by rmccue 7 months ago.
Add multisite check

Download all attachments as: .zip

Change History (33)

#1 @nullvariable
9 months ago

Looks like this is caused by localizing stuff for the api.

wp-includes/script-loader.php line 510 creates a nonce:

'nonce'         => wp_create_nonce( 'wp_rest' ),

@nullvariable
9 months ago

patch

#2 @nullvariable
9 months ago

I added a check for WP_INSTALLING when the nonce is created to skip generating one during install

#3 @nullvariable
9 months ago

to be more specific on the error conditions, this only seems to occur if you leave the 'put your unique phrase here' string in your wp-config (or have duplicate keys/salts). pluggable.php's wp_salt function will try to generate unique keys and salts for you but fails because the database hasn't been created yet.

it may be that wp_salt should be updated to check for WP_INSTALLING also

This ticket was mentioned in Slack in #core by helen. View the logs.


9 months ago

#5 follow-up: @pento
9 months ago

  • Milestone changed from Awaiting Review to 4.7.1

I'm not sure the best way to fix this - checking WP_INSTALLING limits the future possibility of using the REST API during installation, which I'm not wild about.

This is pretty edge-casey, let's investigate it further for a point release.

#6 @nullvariable
9 months ago

@pento this fix wouldn't prevent the API from working during install, just prevents it from creating a nonce during install.

Edit: more specifically the patch I attached modifies the script localization to exclude the nonce if WP_INSTALLING

Last edited 9 months ago by nullvariable (previous) (diff)

#7 in reply to: ↑ 5 @rmccue
8 months ago

  • Focuses rest-api added

Replying to pento:

I'm not sure the best way to fix this - checking WP_INSTALLING limits the future possibility of using the REST API during installation, which I'm not wild about.

The nonce is only used for authentication checks in any case, which doesn't make sense if you don't actually have any users to authenticate against. If we add things in the installer in the future, those will need some other way of authenticating.

The way the patch handles this makes sense to me (although, no need for parens around the constant).

@nullvariable
8 months ago

updated patch to remove extra parentheses

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


8 months ago

#9 @Presskopp
8 months ago

  • Keywords has-patch added

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


8 months ago

#11 follow-up: @pento
8 months ago

@nullvariable: I haven't been able to reproduce this error.

Could you please upload a copy of the wp-config.php you're using that causes this?

Have the database user and database name defined in your wp-config.php been created in MySQL?

Are there any other tables in that database using the same table prefix as defined in your wp-config.php?

#12 in reply to: ↑ 11 @nullvariable
8 months ago

Replying to pento:

@nullvariable: I haven't been able to reproduce this error.

Could you please upload a copy of the wp-config.php you're using that causes this?

<?php
define('WP_HOME', 'http://wordpress.dev');
define('WP_SITEURL', 'http://wordpress.dev');
define('DB_NAME', 'wordpress');
define('DB_USER', 'root');
define('DB_PASSWORD', 'root');
define('DB_HOST', 'db');
define('DB_CHARSET', 'utf8');
define('DB_COLLATE', '');
define('AUTH_KEY',         'put your unique phrase here');
define('SECURE_AUTH_KEY',  'put your unique phrase here');
define('LOGGED_IN_KEY',    'put your unique phrase here');
define('NONCE_KEY',        'put your unique phrase here');
define('AUTH_SALT',        'put your unique phrase here');
define('SECURE_AUTH_SALT', 'put your unique phrase here');
define('LOGGED_IN_SALT',   'put your unique phrase here');
define('NONCE_SALT',       'put your unique phrase here');
$table_prefix  = 'wp_';
define('WP_DEBUG', true);

Have the database user and database name defined in your wp-config.php been created in MySQL?

Yes

Are there any other tables in that database using the same table prefix as defined in your wp-config.php?

no, brand new database, no tables.

@pento
8 months ago

#13 @pento
8 months ago

Thanks @nullvariable, I was able to reproduce it with that. :-) Could you try out 39047.diff? Instead of removing the nonce through the entire installation process, it only happens while the tables don't exist (which is a reasonable proxy for determining when the REST API is able to be used, too).

#14 @nullvariable
8 months ago

@pento that solves it in my testing. Agreed on testing to see if the table exists, should solve the problem with the least impact.

#15 follow-up: @ocean90
8 months ago

39047.diff will produce a PHP fatal error if SCRIPT_DEBUG is false because wp_installing() isn't available in load-scripts.php.

#16 in reply to: ↑ 15 @dd32
8 months ago

Replying to ocean90:

39047.diff will produce a PHP fatal error if SCRIPT_DEBUG is false because wp_installing() isn't available in load-scripts.php.

Yeah, the conditional needs a did_action( 'init' ) && prepended to it at a minimum.

I don't like the direct SQL here, or anything which triggers a SQL. Can this scenario be handled by checking that the underlying values that nonces rely upon is already setup maybe?

#17 @ocean90
8 months ago

#39440 was marked as a duplicate.

#18 follow-up: @rmccue
7 months ago

The underlying reason for the query is that wp_salt() attempts to save the salt to the DB when it's default ("put your unique phrase here"), but the table does not yet exist. The error doesn't occur when the salts are set manually/in your wp-config.php, since it doesn't need to write to the DB.

Detecting this isn't super easy, as you can't know whether the salt will be loaded from a constant or the DB beforehand. In addition, changing that function is a bit tough, because it's pluggable.

I remain convinced that what I said still holds:

The nonce is only used for authentication checks in any case, which doesn't make sense if you don't actually have any users to authenticate against. If we add things in the installer in the future, those will need some other way of authenticating.

I'd say we still use wp_installing().

Replying to ocean90:

39047.diff​ will produce a PHP fatal error if SCRIPT_DEBUG is false because wp_installing() isn't available in load-scripts.php.

load-scripts.php isn't used during installation, as wp_print_scripts() is called directly rather than via print_footer_scripts. (This actually might already fail from calling wp_create_nonce, which isn't stubbed...; will investigate.)

#19 in reply to: ↑ 18 @rmccue
7 months ago

Replying to rmccue:

load-scripts.php isn't used during installation, as wp_print_scripts() is called directly rather than via print_footer_scripts. (This actually might already fail from calling wp_create_nonce, which isn't stubbed...; will investigate.)

As it turns out, the did_action( 'init' ) && already handles this, so it's perfectly safe to call wp_installing() (and wp_create_nonce()) inside the localisation.

@rmccue
7 months ago

39047-2.patch, but using the function instead of the constant

#20 @rachelbaker
7 months ago

@ocean90 @pento @dd32 Any objections to Ryan's approach in 39047.2.diff?

#21 @dd32
7 months ago

Using wp_installing() seems okay, but it should be paired with a multisite check I believe - installing is truthful during a new site creation, but the salts are network wide IIRC.

wp_installing && ! is_multisite() would be correct I think.

Last edited 7 months ago by dd32 (previous) (diff)

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 months ago

This ticket was mentioned in Slack in #core by rmccue. View the logs.


7 months ago

@rmccue
7 months ago

Add multisite check

#24 @rmccue
7 months ago

Updated patch to include the check.

@pento Any final thoughts before this goes in? Speak now, or forever hold your peace. :)

#25 @pento
7 months ago

  • Keywords commit added

Get to da choppa.

#26 @rmccue
7 months ago

  • Owner set to rmccue
  • Resolution set to fixed
  • Status changed from new to closed

In 39684:

Upgrade/Install: Avoid creating nonce during installation.

When installing and using database-saved salts, wp_create_nonce() causes database errors as wp_salt() attempts to insert into the not-yet-created options table. Since authentication isn't available during installation, we can safely skip creating a nonce.

Props nullvariable, pento, dd32.
Fixes #39047.

#27 @rmccue
7 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#28 @dd32
7 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 39697:

Upgrade/Install: Avoid creating nonce during installation.

When installing and using database-saved salts, wp_create_nonce() causes database errors as wp_salt() attempts to insert into the not-yet-created options table. Since authentication isn't available during installation, we can safely skip creating a nonce.

Props nullvariable, pento, dd32, rmccue.
Merges [39684] to the 4.7 branch.
Fixes #39047.

Note: See TracTickets for help on using tickets.