Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#54800 closed defect (bug) (fixed)

Nonce creation causes DB access errors when initializing multisite networks

Reported by: schlessera's profile schlessera Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.0 Priority: normal
Severity: normal Version: 5.0
Component: Script Loader Keywords: has-patch has-testing-info fixed-major
Focuses: Cc:

Description

The default script assets to be enqueued via the script loader use nonces for the assets that integrate with the WP REST API.

The nonce creation through wp_create_nonce() fails with a database error when it is called on a site that has not fully been initialized yet, as the table where the nonce is supposed to be stored does not yet exist.

WordPress database error Table 'wp_cli_test.wp_sitemeta' doesn't exist for query SELECT meta_value FROM
wp_sitemeta WHERE meta_key = 'nonce_key' AND site_id = 1 made by
include('phar:///home/alain/bin/wp/php/boot-phar.php'),
include('phar:///home/alain/bin/wp/vendor/wp-cli/wp-cli/php/wp-cli.php'),
WP_CLI\bootstrap, WP_CLI\Bootstrap\LaunchRunner->process, WP_CLI\Runner->start, WP_CLI\Runner->load_wordpress,
require('wp-settings.php'), do_action('init'), WP_Hook->do_action, WP_Hook->apply_filters,
register_block_core_file, register_block_type_from_metadata, register_block_script_handle, wp_register_script,
wp_scripts, WP_Scripts->__construct, WP_Scripts->init, do_action_ref_array('wp_default_scripts'),
WP_Hook->do_action, WP_Hook->apply_filters, wp_default_scripts, wp_create_nonce, wp_hash, wp_salt,
get_site_option, get_network_option

There is already some safeguarding in place around these nonces, but it is not enough, as it specifically excludes the case of a multisite setup.

The ! is_multisite() condition should be removed from these safeguards to ensure the error does never surface.

This is a follow-up ticket to #54634.

Change History (24)

This ticket was mentioned in PR #2147 on WordPress/wordpress-develop by schlessera.


3 years ago
#1

  • Keywords has-patch added

This PR adapts the safeguards around the wp_create_nonce() calls in the wp-includes/script-loader.php file to ensure they are never called during a WordPress installation process.

The calls were already safeguarded but only on non-multisite setups, which has turned out to be not enough.

Trac ticket: #54800

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


3 years ago

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


3 years ago

#4 @schlessera
3 years ago

The root cause here was introduced with the Gutenberg 10.5 merge: [50824]

The files that added block registration caused scripts to be enqueued unconditionally. This in turn triggers the wp_default_scripts hook and function, which includes REST API assets that require a nonce.

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

#5 @johnbillion
3 years ago

  • Version changed from trunk to 5.8

This ticket was mentioned in Slack in #cli by schlessera. View the logs.


3 years ago

#7 @hellofromTonya
3 years ago

  • Keywords reporter-feedback added

[50824] has been identified as the root cause, though this changeset was introduced in 5.8, not in 5.9.

I'm hesitant to move backport the patch to 5.9 without understanding root cause introduced in 5.9 and testing more broadly.

@schlessera some questions to help with decision making:

  • Is this error happening on 5.8 also? (should be if the changeset is when the issue was introduced into Core)
  • Is this error on all PHP versions?
  • With this error, does the upgrade or site fail? (severity)

#8 @hellofromTonya
3 years ago

  • Keywords needs-testing-info added

Also, @schlessera can you provide the step-by-step instructions on how to reproduce the reported errors? This information helps testers create Test Reports to confirm original error and fix resolution. Thank you!

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


3 years ago

#10 @audrasjb
3 years ago

As per today's bug scrub, let's keep this ticket in the milestone until WP 5.9 RC3 tomorrow.

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


3 years ago

peterwilsoncc commented on PR #2147:


3 years ago
#12

These changes look good to me and prevent the issue when installing. To test via WP-CLI I was running the commands:

 wp db --yes reset
 wp core multisite-install --title='wpdevms' --admin_user=admin --admin_email=admin@example.com --admin_password=password --url=http://wp-dev.local/

During the install process, define( 'MULTISITE', true ) is included in my wp-config file. No salts are defined in code, causing WP to fall back to the defaults.

@schlessera I've merged an up to date version of trunk in to this PR to get the tests running on Core in the current state.

#13 @peterwilsoncc
3 years ago

Testing this with @costdev today, we came to the conclusion these database errors were added later and during the WP 5.9 cycle.

The linked pull request appears to fix the error.

--- Reproduction notes ---

The easiest method to reproduce is via WP CLI.

wp-config.php

  • Ensure the file includes define( 'MULTISITE', true );
  • Ensure the file does not include any of the *_KEY or *_SALT definitions
  • Ensure debug logging is enabled

Run the following commands via WP CLI (changing the host name as appropriate)

wp db --yes reset
wp core multisite-install --title='wpdevms' --admin_user=admin --admin_email=admin@example.com --admin_password=password --url=http://wp-dev.local/

--- Without the patch applied ---

Multiple errors are logged during the installation process as the wp_create_nonce() is called. Each error is similar to the log entry below but the meta_key value can vary:

WordPress database error Table 'wordpress.wp_sitemeta' doesn't exist for query SELECT meta_value FROM wp_sitemeta WHERE meta_key = 'nonce_key' AND site_id = 1 made by include('phar:///usr/local/src/wp-cli/bin/wp/php/boot-phar.php'), include('phar:///usr/local/src/wp-cli/bin/wp/vendor/wp-cli/wp-cli/php/wp-cli.php'), WP_CLI\bootstrap, WP_CLI\Bootstrap\LaunchRunner->process, WP_CLI\Runner->start, WP_CLI\Runner->load_wordpress, require('wp-settings.php'), do_action('init'), WP_Hook->do_action, WP_Hook->apply_filters, register_block_core_file, register_block_type_from_metadata, register_block_script_handle, wp_register_script, wp_scripts, WP_Scripts->__construct, WP_Scripts->init, do_action_ref_array('wp_default_scripts'), WP_Hook->do_action, WP_Hook->apply_filters, wp_default_scripts, wp_create_nonce, wp_hash, wp_salt, get_site_option, get_network_option

--- With the patch applied ---

No nonce related errors are thrown during the installation process.

#14 @hellofromTonya
3 years ago

  • Keywords has-testing-info added; reporter-feedback needs-testing-info removed
  • Milestone changed from 5.9 to 5.9.1
  • Priority changed from high to normal
  • Severity changed from major to normal
  • Version changed from 5.8 to 5.0

Adding a few notes from #core slack discussions:

@peterwilsoncc identified [51501] as the possible source that introduced the nonce table errors in 5.9

I ended up running bisect twice as it doesn’t quite make sense to me as to why [51501] would be introducing the nonce table erorrs.

Following up the earlier notes for the database errors thrown during multisite installs (reported in #54801 and #54800). The errors during wp core multisite-install are new for 5.9 so it’s preferred to focus on those.

Multiple errors when attempting to set the same via set_site_transient()

These errors appear to be coming from to wp_is_block_theme() calls within the following locations:

  • create_initial_post_types()
  • _add_default_theme_supports()
  • wp_enable_block_templates()

The nonce errors as reported due to the conditions is_installing() && ! is_multisite() when setting up the default scripts, this appears to have been added during the commit I mentioned above.

Knowns:

  • [51501] highlighted the problem during wp_installing() as 3 new functions eventually call get_theme_roots() which then calls get_transient() for single site or get_site_transient() for multisite. get_transient() has checks for wp_installing() whereas get_site_transient() does not.
    • Peter opened #54849 to address this problem possibly in 5.9.1.
  • Root cause of the errors can be resolved in #54849.
  • Reporting of the errors (this ticket) is due to what @schlessera identified and fixed with PR 2147.
  • The code for this fix is from 5.0 (see #45065 and changeset [44114]).

Next steps:

  • Lowering the severity level and priority as the reporting of the errors does not cause the upgrade or site to fail.
  • What about backporting to 5.9? Thinking this needs to move to 5.9.1 to go with #54849. Why?
    • The reporting of the errors were not introduced during RC or beta and are not blockers for a final release.
    • The severity of does not seem to be a blocker for 5.9 final release.
    • The root cause resolution (see #54849) is in 5.9.1 milestone.

I welcome discussion on whether to backport to 5.9 during RC or push this to 5.9.1. Please note, if backport doesn't happen in time for RC3 today, backporting would require another RC release.

#15 @peterwilsoncc
3 years ago

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

In 52602:

Script loader: Prevent DB errors during Multisite install.

Prevent the script loader from attempting to create nonces during the installation process for Multisite configurations.

Prior to this fix, multiple "Table does not exist" errors were thrown during installation if MULTISITE was defined in the wp-config.php file but the salt constants were not defined. Without the salts defined in PHP, WP was attempting to use the database fallbacks prior to table creation.

Props schlessera, johnbillion, hellofromTonya, audrasjb, costdev.
Fixes #54800.

#16 @peterwilsoncc
3 years ago

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

Reopening for merging in to the 5.9 branch for 5.9.1.

Please do not merge to the 5.9 branch prior to the release of WordPress 5.9.

#18 @SergeyBiryukov
3 years ago

comment:21:ticket:39047 has some context for adding the ! is_multisite() checks removed in [52602].

Some history here:

  • [39684] added a wp_installing() && ! is_multisite() check for the wp-api nonce.
  • [44114] used the same check in a new wp_default_packages_inline_scripts() function.
  • [51525] used the same check for the user-profile nonce.

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


3 years ago

#20 follow-up: @peterwilsoncc
3 years ago

@SergeyBiryukov Are you noting the history for information purposes or are you after some changes prior to merging in to the 5.9 branch? If the latter, are you able to be more specific?

#21 in reply to: ↑ 20 ; follow-up: @SergeyBiryukov
3 years ago

Replying to peterwilsoncc:

Are you noting the history for information purposes or are you after some changes prior to merging in to the 5.9 branch?

I think I wanted to see if comment:21:ticket:39047 is still relevant, but didn't get around to it, so I guess it's just for information purposes at this point :)

#22 in reply to: ↑ 21 @dd32
3 years ago

Replying to SergeyBiryukov:

Replying to peterwilsoncc:

Are you noting the history for information purposes or are you after some changes prior to merging in to the 5.9 branch?

I think I wanted to see if comment:21:ticket:39047 is still relevant, but didn't get around to it, so I guess it's just for information purposes at this point :)

I think it's still somewhat valid, but I think leaving [52602] in place for trunk is a good idea. I don't see this as being something worth back-porting to 5.9.x though, it's not a regression or major bug.

There's three cases here:

  • Single Site: wp_installing() truthful, actually installing, nonces unavailable.
  • Multisite Site creation: wp_installing() truthful, but nonces available due to Site being installed. With [52602] in place, any pages where a site is being created, the nonces which are no longer being generated here are now unavailable, even though they can be generated. Does that cause a problem? Unsure.
  • Multisite Network Creation: wp_installing() truthful, but nonces unavailable due to Network not being created yet.

Somewhat importantly, that last one is not exposed via the WordPress UI's as far as I know? so it was never really accounted for.

I guess in part, wp_installing() is really being overloaded for multiple different things, so doesn't always make total sense.

Perhaps the fix actually needs to be made lower in the stack, when wp_installing() functions that trigger DB access should check the table exists before accessing it?

Last edited 2 years ago by dd32 (previous) (diff)

#23 @peterwilsoncc
3 years ago

  • Milestone changed from 5.9.1 to 6.0
  • Resolution set to fixed
  • Status changed from reopened to closed

I agree with Dion's comment above:

I think it's still somewhat valid, but I think leaving [52602] in place for trunk is a good idea. I don't see this as being something worth back-porting to 5.9.x though, it's not a regression or major bug.

Closing as fixed and changing the milestone to 6.0

#24 @azaozz
2 years ago

Somewhat importantly, that last one is not exposed via the WordPress UI's as far as I know?

Yea, think there is no way to install a multisite network directly. The only way is to install a single site first, then upgrade it (afaik).

This bug seems specific to WP CLI. Maybe worth another look there if installing a multisite network directly doesn't interfere somewhere else.

Last edited 2 years ago by azaozz (previous) (diff)
Note: See TracTickets for help on using tickets.