#62815 closed task (blessed) (fixed)
Explicitly require the `hash` extension
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.8 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Security | Keywords: | has-patch has-unit-tests |
Focuses: | Cc: |
Description
In #60638 the Gravatar hashes have been switched from sha1 to sha256. In #21022 sha384 will be used for pre-hashing user passwords. The hash
extension is required in order for either of these extensions to be available for use by hash()
or hash_hmac()
.
In the discussion on https://github.com/WordPress/wordpress-develop/pull/8097, Dion observed that one single site out of all those tracked on 6.1+ had hash
disabled while otherwise being compatible.
As a result, let's add hash
to the list of extensions that are required to run WordPress 6.8 so we can ship safer Gravatar hashes and safer password hashing, and correspondingly remove some compatibility code.
Change History (20)
This ticket was mentioned in PR #8138 on WordPress/wordpress-develop by @johnbillion.
5 weeks ago
#1
- Keywords has-patch has-unit-tests added; needs-patch removed
@johnbillion commented on PR #8138:
5 weeks ago
#2
@ayeshrajans commented on PR #8138:
5 weeks ago
#3
PHP 7.4 is the first version to properly bundle the hash extension, so prior to PHP 7.4, it is technically possible to compile PHP without the hash extension.
@peterwilsoncc commented on PR #8138:
5 weeks ago
#4
@johnbillion In it's current form, the error will only show the first extension that is missing. If an install is missing both required extensions, I think it would be helpful to include that in the WP_Error. Does something like this work?
// Add a warning when required PHP extensions are missing.
$required_php_extensions = array( 'json', 'hash' );
$missing_php_extensions = array();
foreach ( $required_php_extensions as $required_php_extension ) {
if ( ! extension_loaded( $required_php_extension ) ) {
$missing_php_extensions[] = $required_php_extension;
}
}
if ( ! empty( $missing_php_extensions ) ) {
$php_extension_error = new WP_Error();
foreach ( $missing_php_extensions as $missing_php_extension ) {
$php_extension_error->add(
"php_not_compatible_{$missing_php_extension}",
sprintf(
/* translators: 1: WordPress version number, 2: The PHP extension name needed. */
__( 'The update cannot be installed because WordPress %1$s requires the %2$s PHP extension.' ),
$wp_version,
strtoupper( $missing_php_extension )
)
);
}
return $php_extension_error;
}
@johnbillion commented on PR #8138:
5 weeks ago
#5
@Ayesh Yep that's correct. Dion ran the numbers in #8097 and saw almost zero affected sites with hash
disabled, hence this proposal. I've asked him to run the numbers again to double check!
@johnbillion commented on PR #8138:
5 weeks ago
#6
@peterwilsoncc Good point, I'll take a look
@johnbillion commented on PR #8138:
5 weeks ago
#7
So we've a catch-22 situation here, the code in update_core()
cannot introduce any new logic to prevent an update from occurring because its code won't yet be in place during the update. The update_core()
function specifically reads $required_php_version
and $required_mysql_version
from wp-includes/version.php
in the update package for the PHP and MySQL version checks.
For future-proofing we could introduce a $required_php_extensions
global in this file, but it wouldn't be able to take effect during any update from a version earlier than 6.8.
@peterwilsoncc commented on PR #8138:
4 weeks ago
#8
@johnbillion It could be done on the API by only presenting the 6.8 update if the hash extension is available. The main consideration there would be how to handle two situations:
- updating from a version prior to 6.1
- updates from sites running a plugin to remove passing extension data to wordpress.org
Each of these will remain a problem, even if a two step approach is taken (prep in 6.8, implement in 6.9) so the API approach will be needed in some form regardless.
I'm sure patches to meta are welcome but I was unable to find where to create a PR. in the meta repo. It's possible the API is in the private systems repo.
#10
@
2 weeks ago
Meta ticket: https://meta.trac.wordpress.org/ticket/7900
This ticket was mentioned in PR #8252 on WordPress/wordpress-develop by @johnbillion.
2 weeks ago
#11
This introduces a new workflow which tests the upgrade process from the three latest major versions to the current branch version. The current branch is built and zipped up, and the zip is used by WP-CLI for the update.
This uses a smaller set of matrix values than the "Upgrade Tests" workflow. I think this is sufficient for now, it can always be expanded at a later date if necessary.
Workflow runs can be seen here: https://github.com/WordPress/wordpress-develop/actions/workflows/upgrade-develop-testing.yml
Trac ticket: https://core.trac.wordpress.org/ticket/62815
@audrasjb commented on PR #8252:
2 weeks ago
#12
Thanks for the ping @johnbillion. While I'm not the best person to checkout this proposal, I just wanted to ask a question about the "a new workflow which tests the upgrade process from the three latest major versions to the current branch" part: if this proposal is accepted, does it mean that we'll have to manually bump the related versions on each major release? This is not a blocker, but definitely something that will need some specific documentation on the major release handbook page :)
@johnbillion commented on PR #8252:
2 weeks ago
#13
@audrasjb Yes that's correct. We already have that in several other workflows, eg the performance tests, old branch tests, and upgrade tests. I'll make a note to get the major release handbook page udpated.
@johnbillion commented on PR #8252:
13 days ago
#14
we already generate a ZIP of the code in the current branch during the build test workflow
Yeah, if you take a look at the first few commits in this branch you'll see that I was initially trying to use that build, but it requires using the workflow_run
trigger in order to make use of the build artifact once the workflow completes, and that trigger can't be tested in a PR because it only functions when the workflow with the trigger exists in the default branch. I tried workflow_dispatch
but similarly this can't be tested in a PR from a fork. So I just went with another build.
I think this could be accomplished with some conditional checks, but we'd likely need to add a 3rd-party action like changed-files to know when to run each job
I understand your point but I'm not overly keen on adding further third party actions. Is a bunch of conditionals and an extra third party action better than an additional workflow? I don't think there's much between them.
@joemcgill commented on PR #8252:
7 days ago
#17
Thanks, @johnbillion. I think this is a good idea and things are looking good to me. Just to confirm one of my assumptions here, in addition to needing to bump the WP versions in the matrix passed for the reusable upgrade testing workflow, we'll also need to make sure the matrix stays up to date with the supported PHP and DB versions as that support changes over time, correct?
I also notice that the current matrix doesn't include any object cache tests. Is that intentional or do you think there would be some value in including at least one object cache test in the matrix?
@johnbillion commented on PR #8252:
7 days ago
#18
@joemcgill That's correct. We have that case already with the other upgrade tests. It's unfortunate, and perhaps going forward we could populate the matrix from .version-support-php.json
and .version-support-mysql.json
, but that would be better proposed in a separate PR I think.
Good spot on the object caching. Looking at the Installation Tests workflow there's a matrix entry for memcached
but it's not actually used. The Upgrade Tests workflow doesn't include any handling for memcached.
Adding testing with and without memcached would require a change to the way these installation and upgrade workflow files work because they all use PHP directly on the host runner instead of using the containers. I'll open a follow-up ticket.
@johnbillion commented on PR #8252:
6 days ago
#19
Committed in https://core.trac.wordpress.org/changeset/59815
@dd32 I would like to ask you to double check those stats you pulled about the
hash
extension. It's great that only a single site on 6.1+ has json but not hash, but it's also a suspiciously low number. Is there any chance it may not be accurate? Better to be safe than sorry. Thanks!