diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 571f8fc..022fb29 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -100,6 +100,12 @@ jobs: matrix: php: ['7.4', '8.0', '8.3', '8.5'] wp: ['latest', 'trunk'] + # The declared floor, `Requires at least` in secrets-api.php. src/ relies on + # core's polyfills (array_all() arrived in 6.8), so the floor gets a leg of + # its own rather than being assumed from latest. + include: + - php: '7.4' + wp: '6.8' services: mysql: image: mysql:8.0 diff --git a/README.md b/README.md index 48a1b47..d77e493 100644 --- a/README.md +++ b/README.md @@ -3,7 +3,7 @@ Feature plugin for the [Secrets API proposed for WordPress 7.2][proposal]. Encrypted, versioned credential storage with pluggable storage and keyring back ends. -This exists so contributors can *run* the API instead of reading about it, and so sites on 6.6 +This exists so contributors can *run* the API instead of reading about it, and so sites on 6.8 and later get something usable before the core patch lands. Everything under `src/` is written to be copied verbatim into `wordpress-develop` — same paths, same coding style, same `default` text domain. @@ -105,7 +105,7 @@ future screen needs are in scope; the screen is not. ## Requirements - PHP 7.4+ (core's floor — `src/` contains no PHP 8 syntax) -- WordPress 6.6+ +- WordPress 6.8+ (core polyfills `array_all()` from 6.8, and `src/` uses it) - libsodium, via the extension or core's bundled `sodium_compat` ## Coexisting with the Displace prototype diff --git a/docs/reference/classes.md b/docs/reference/classes.md index bec5bbc..d9b2cb6 100644 --- a/docs/reference/classes.md +++ b/docs/reference/classes.md @@ -1767,6 +1767,15 @@ stopped firing once a host installed a provider would be unreliable in exactly the situation it matters most, so WP_Secrets_Provider_Conformance checks it. A write that fails fires nothing. +Writing the value a secret already holds is not a rotation. A settings form +that saves every field on every submit does this routinely, so a provider +that keeps a previous version must not discard it for an unchanged write; +doing so would end a rotation's grace period early, with every consumer +still on the old credential losing access. WP_Secrets_Provider_Conformance +checks this. Treating the write as a no-op that fires nothing is preferred, +but a provider that cannot tell the value is unchanged without an additional +read may write it and report it. + ```php public function set( string $name, $value, bool $network = false, bool $needs_rotation = false, ?string $action = null ) ``` diff --git a/docs/spec/extension-points.md b/docs/spec/extension-points.md index 325aabc..4241766 100644 --- a/docs/spec/extension-points.md +++ b/docs/spec/extension-points.md @@ -56,7 +56,7 @@ class Tests_My_Platform_Provider extends WP_Secrets_Provider_Conformance { It checks that a name never set reads as `null`; that `PREVIOUS` with no previous value is an absence rather than an error; that deleting or retiring something absent succeeds; that a stored value round-trips and a deleted one reads as absent; that fingerprints stay stable for the same -value; that listings never contain a plaintext and respect a name prefix; that the label, +value; that writing an unchanged value keeps the previous version; that listings never contain a plaintext and respect a name prefix; that the label, boundary, and writability declarations are well-formed; and that a provider declaring itself read-only really does refuse writes with `secret_provider_read_only`. Where the contract allows variation it adapts: a read-only provider is never asked to round-trip a value, and the skipped diff --git a/docs/spec/rotation.md b/docs/spec/rotation.md index cb609a7..228b550 100644 --- a/docs/spec/rotation.md +++ b/docs/spec/rotation.md @@ -24,7 +24,9 @@ the new value into `current`, and moves the old `current` to `previous` through which decrypts under the `current` binding and re-encrypts under `previous`. If the outgoing slot cannot be decrypted, it is dropped and the write proceeds, so a corrupted record never blocks its own repair. The `wp_secret_changed` action fires with `updated`. During the drain, `wp_get_secret( -$name, WP_Secret_Version::PREVIOUS )` returns the old value. +$name, WP_Secret_Version::PREVIOUS )` returns the old value. Saving the value `current` already +holds is not a rotation: nothing is written and nothing fires, so a settings form that re-saves an +unchanged key does not end the drain early. See [versioning.md](versioning.md). **Retiring the previous value.** `wp_retire_secret_version()` and `wp_retire_network_secret_version()` in `src/wp-includes/secrets.php` call the provider's diff --git a/docs/spec/scope.md b/docs/spec/scope.md index 4dfc7e9..6c37473 100644 --- a/docs/spec/scope.md +++ b/docs/spec/scope.md @@ -15,7 +15,7 @@ scope", and "Timeline" in the [proposal][proposal]. ## As built -**The plugin.** `secrets-api.php` declares version `0.3.0`, `Requires at least: 6.6`, and +**The plugin.** `secrets-api.php` declares version `0.3.0`, `Requires at least: 6.8`, and `Requires PHP: 7.4`. **Core-bound versus plugin-only.** Everything under `src/` is written to be copied into @@ -38,7 +38,8 @@ It also brings the tests for everything in `src/`. **Standing down.** `wp_secrets_api_bootstrap()` in `secrets-api.php` loads nothing and shows an admin notice when `$wp_version` is at least `WP_SECRETS_API_CORE_VERSION` (`7.2`, overridable) -**and** `wp_get_secret()` already exists. If the symbol exists but the version test fails, the +**and** `wp_get_secret()` already exists. A beta, RC, or trunk build counts as its release, so +`7.2-beta1-64606-src` passes the version test. If the symbol exists but the version test fails, the plugin refuses to load and shows a conflict notice instead of deferring to an unknown implementation. `tests/phpunit/test-secrets-noop-gate.php` covers the gate. diff --git a/docs/spec/versioning.md b/docs/spec/versioning.md index 2ce0364..02c7816 100644 --- a/docs/spec/versioning.md +++ b/docs/spec/versioning.md @@ -28,6 +28,13 @@ checks the same set before any cryptographic operation. and demotes the outgoing `current` to `previous`. Whatever was already in `previous` is not carried forward, so a third write discards the oldest value. +**An unchanged write is not a rotation.** Writing the value `current` already holds, as a settings +form does on every save, leaves the record untouched and does not fire `wp_secret_changed`, so +`previous` survives. The one exception is a write that raises `needs_rotation` on a value not yet +flagged, such as `wp_import_option_as_secret()` on an option holding the current value. It +records the flag and carries `previous` over as stored. The `WP_Secrets_Provider::set()` contract +requires any provider that keeps a previous version to keep it through an unchanged write. + **Slot is part of the ciphertext binding.** The slot name is one of the fields in the AAD `WP_Secrets_Cipher::build_aad()` computes, so a slot's ciphertext is bound to its position. Demotion is therefore a decrypt-and-re-encrypt in diff --git a/examples/README.md b/examples/README.md index c65f479..f29fe67 100644 --- a/examples/README.md +++ b/examples/README.md @@ -84,7 +84,8 @@ class Tests_My_Platform_Provider extends WP_Secrets_Provider_Conformance { ``` It covers what `implements WP_Secrets_Provider` can't: absence reported as `null`, deleting -something absent succeeding, fingerprints staying stable for the same value, listings never +something absent succeeding, fingerprints staying stable for the same value, an unchanged write +keeping the previous version, listings never containing a plaintext, and a read-only declaration actually being honoured. See [`../docs/spec/extension-points.md`](../docs/spec/extension-points.md). diff --git a/examples/aws-secrets-manager/secrets.php b/examples/aws-secrets-manager/secrets.php index 56e4311..3da89cb 100644 --- a/examples/aws-secrets-manager/secrets.php +++ b/examples/aws-secrets-manager/secrets.php @@ -138,6 +138,14 @@ public function get( string $name, string $version, bool $network = false ) { public function set( string $name, $value, bool $network = false, bool $needs_rotation = false, ?string $action = null ) { $aws_name = $this->aws_name( $name, $network ); + // PutSecretValue makes a new version even for the value AWSCURRENT + // already holds, which would move AWSPREVIOUS onto that same value and + // lose the real previous one. An unchanged write is skipped instead – + // see WP_Secrets_Provider::set(). + if ( $this->holds_current_value( $aws_name, $value ) ) { + return true; + } + // PutSecretValue rotates AWSCURRENT -> AWSPREVIOUS for us, which is the // whole two-slot model handled server-side. $result = $this->call( @@ -278,6 +286,32 @@ public function list_secrets( string $name_prefix = '', bool $network = false ) return $entries; } + /** + * Whether AWSCURRENT already holds this value. A failed read is not an + * answer, so it reports false and leaves the write to proceed and report + * its own error. + * + * @param string $aws_name Name in Secrets Manager. + * @param mixed $value Plaintext about to be written. + * + * @return bool + */ + private function holds_current_value( $aws_name, $value ) { + $response = $this->call( + 'GetSecretValue', + array( + 'SecretId' => $aws_name, + 'VersionStage' => 'AWSCURRENT', + ) + ); + + return ! is_wp_error( $response ) + && isset( $response['SecretString'] ) + && is_string( $response['SecretString'] ) + && is_string( $value ) + && hash_equals( $response['SecretString'], $value ); + } + /** * Whether a ListSecrets entry has a version labelled AWSPREVIOUS. * diff --git a/examples/vault-provider/secrets.php b/examples/vault-provider/secrets.php index f84a7e7..9bf043a 100644 --- a/examples/vault-provider/secrets.php +++ b/examples/vault-provider/secrets.php @@ -169,6 +169,42 @@ public function set( string $name, $value, bool $network = false, bool $needs_ro $created = ( null === $meta ); + // Writing the value Vault already holds as current would add version + // N+1 and push the real previous value out, so it is skipped – see + // WP_Secrets_Provider::set(). Raising the rotation flag on that value + // is still recorded, as a metadata write with no new version. + if ( ! $created && $this->holds_current_value( $vault_path, $value ) ) { + if ( ! $needs_rotation || $this->flag_is_set( $meta ) ) { + return true; + } + + $flag = $this->write_flag( $vault_path, true, $meta ); + + if ( is_wp_error( $flag ) ) { + return new WP_Error( + WP_SECRETS_ERROR_STORE_UNAVAILABLE, + sprintf( + 'Vault refused to record the rotation flag: %s', + $flag->get_error_message() + ) + ); + } + + /** This action is documented in WP_Secrets_Libsodium_Provider::set(). */ + do_action( + 'wp_secret_changed', + $name, + null !== $action ? $action : 'updated', + get_current_user_id(), + time(), + '', + '', + $network + ); + + return true; + } + if ( $created ) { $result = $this->request( 'POST', @@ -599,6 +635,26 @@ private function flag_is_set( $meta ) { && '1' === $meta['custom_metadata'][ self::ROTATION_FLAG ]; } + /** + * Whether the current version already holds this value. A failed read is + * not an answer, so it reports false and leaves the write to proceed and + * report its own error. + * + * @param string $vault_path Path under the mount. + * @param mixed $value Plaintext about to be written. + * + * @return bool + */ + private function holds_current_value( $vault_path, $value ) { + $data = $this->request( 'GET', $this->url( 'data', $vault_path ) ); + + return is_array( $data ) + && isset( $data['data']['value'] ) + && is_string( $data['data']['value'] ) + && is_string( $value ) + && hash_equals( $data['data']['value'], $value ); + } + /** * Writes the rotation flag. Vault replaces custom_metadata wholesale on * every POST, so this merges the flag into the custom_metadata already diff --git a/examples/vault-provider/tests/test-vault-provider.php b/examples/vault-provider/tests/test-vault-provider.php index 72c076d..0f688c3 100644 --- a/examples/vault-provider/tests/test-vault-provider.php +++ b/examples/vault-provider/tests/test-vault-provider.php @@ -185,6 +185,64 @@ public function test_only_two_versions_are_kept_in_vault_itself() { $this->assertSame( 200, $this->server->read_version( $path, 3 ) ); } + public function test_an_unchanged_write_adds_no_version_and_fires_nothing() { + $path = 'wp/site/1/acme/key'; + + $this->provider->set( 'acme/key', 'v1' ); + $this->provider->set( 'acme/key', 'v2' ); + + $fired = array(); + add_action( + 'wp_secret_changed', + static function ( $name, $action ) use ( &$fired ) { + $fired[] = $action; + }, + 10, + 2 + ); + + $this->assertTrue( $this->provider->set( 'acme/key', 'v2' ) ); + + $this->assertSame( 2, $this->server->metadata( $path )['current_version'] ); + $this->assertSame( 'v1', $this->provider->get( 'acme/key', WP_Secret_Version::PREVIOUS )->reveal() ); + $this->assertSame( array(), $fired ); + } + + public function test_flagging_an_unchanged_value_writes_the_flag_without_a_new_version() { + $path = 'wp/site/1/acme/key'; + + $this->provider->set( 'acme/key', 'v1' ); + $this->provider->set( 'acme/key', 'v2' ); + + $fired = array(); + add_action( + 'wp_secret_changed', + static function ( $name, $action ) use ( &$fired ) { + $fired[] = $action; + }, + 10, + 2 + ); + + $this->assertTrue( $this->provider->set( 'acme/key', 'v2', false, true, 'imported' ) ); + + $meta = $this->server->metadata( $path ); + + $this->assertSame( 2, $meta['current_version'] ); + $this->assertSame( '1', $meta['custom_metadata']['needs_rotation'] ); + $this->assertSame( 'v1', $this->provider->get( 'acme/key', WP_Secret_Version::PREVIOUS )->reveal() ); + $this->assertSame( array( 'imported' ), $fired ); + } + + public function test_an_unchanged_write_keeps_the_rotation_flag() { + $path = 'wp/site/1/acme/key'; + + $this->provider->set( 'acme/key', 'v1', false, true ); + $this->provider->set( 'acme/key', 'v1' ); + + $this->assertSame( '1', $this->server->metadata( $path )['custom_metadata']['needs_rotation'] ); + } + public function test_previous_is_strictly_n_minus_1_even_when_older_versions_survive() { $path = 'wp/site/1/acme/key'; diff --git a/phpstan.neon.dist b/phpstan.neon.dist index 9cec3db..5b9a440 100644 --- a/phpstan.neon.dist +++ b/phpstan.neon.dist @@ -24,6 +24,7 @@ parameters: scanFiles: - vendor/php-stubs/wp-cli-stubs/wp-cli-stubs.php - vendor/php-stubs/wp-cli-stubs/wp-cli-commands-stubs.php + - tests/phpstan/compat-stubs.php bootstrapFiles: - tests/phpstan-bootstrap.php treatPhpDocTypesAsCertain: false @@ -37,3 +38,12 @@ parameters: paths: - src/wp-includes/secrets/class-wp-secrets-option-store.php - plugin/class-secrets-api-legacy-reader.php + # r64674's no-op check reads $existing['current'] behind $is_unchanged, which is + # only true when $has_current is. PHPStan narrows $existing through $has_current + # but not through $is_unchanged. src/ stays as core committed it, so the one + # line is ignored here rather than rewritten. + - + message: '#^Cannot access offset ''needs_rotation'' on mixed\.$#' + identifier: offsetAccess.nonOffsetAccessible + path: src/wp-includes/secrets/class-wp-secrets-libsodium-provider.php + count: 1 diff --git a/secrets-api.php b/secrets-api.php index 47053f0..8e985ee 100644 --- a/secrets-api.php +++ b/secrets-api.php @@ -4,7 +4,7 @@ * Plugin URI: https://github.com/ericmann/secrets-api * Description: Feature plugin for the WordPress Secrets API proposed for 7.2. Encrypted, versioned credential storage with pluggable storage and keyring back ends. * Version: 0.3.0 - * Requires at least: 6.6 + * Requires at least: 6.8 * Requires PHP: 7.4 * License: GPL-2.0-or-later * License URI: https://www.gnu.org/licenses/gpl-2.0.html @@ -63,8 +63,12 @@ function wp_secrets_api_bootstrap(): void { $symbol_taken = function_exists( 'wp_get_secret' ); + // A beta, RC, or trunk build such as '7.2-beta1-64606-src' counts as its release: + // version_compare() alone ranks every pre-release below '7.2'. + $release = explode( '-', $wp_version )[0]; + // Core ships the API. Load nothing; the plugin is redundant. - if ( version_compare( $wp_version, WP_SECRETS_API_CORE_VERSION, '>=' ) && $symbol_taken ) { + if ( version_compare( $release, WP_SECRETS_API_CORE_VERSION, '>=' ) && $symbol_taken ) { add_action( 'admin_notices', 'wp_secrets_api_notice_superseded' ); return; diff --git a/src/wp-admin/includes/secrets-site-health.php b/src/wp-admin/includes/secrets-site-health.php index 65ffe28..eef44e9 100644 --- a/src/wp-admin/includes/secrets-site-health.php +++ b/src/wp-admin/includes/secrets-site-health.php @@ -85,7 +85,7 @@ function wp_secrets_site_health_result( string $test, string $label, string $sta function wp_secrets_site_health_test_key_source(): array { $key_source = _wp_secrets_get_key_manager()->get_keyring()->get_key_source(); - if ( false !== strpos( $key_source, 'broken' ) ) { + if ( str_contains( $key_source, 'broken' ) ) { return wp_secrets_site_health_result( 'secrets_api_key_source', __( 'Secrets API key source is broken' ), @@ -97,7 +97,7 @@ function wp_secrets_site_health_test_key_source(): array { ); } - if ( false !== strpos( $key_source, 'legacy interpretation' ) ) { + if ( str_contains( $key_source, 'legacy interpretation' ) ) { return wp_secrets_site_health_result( 'secrets_api_key_source', __( 'Secrets API key should be regenerated' ), @@ -109,7 +109,7 @@ function wp_secrets_site_health_test_key_source(): array { ); } - if ( false !== strpos( $key_source, 'LOGGED_IN_KEY' ) ) { + if ( str_contains( $key_source, 'LOGGED_IN_KEY' ) ) { return wp_secrets_site_health_result( 'secrets_api_key_source', __( 'Secrets API is using a fallback key' ), diff --git a/src/wp-includes/secrets/class-wp-secrets-config-key-provider.php b/src/wp-includes/secrets/class-wp-secrets-config-key-provider.php index 7fbd208..c99adc9 100644 --- a/src/wp-includes/secrets/class-wp-secrets-config-key-provider.php +++ b/src/wp-includes/secrets/class-wp-secrets-config-key-provider.php @@ -261,13 +261,10 @@ private function get_site_key() { * @phpstan-assert-if-true non-empty-string $logged_in_salt */ private function are_usable_salt_values( $logged_in_key, $logged_in_salt ): bool { - foreach ( array( $logged_in_key, $logged_in_salt ) as $value ) { - if ( ! is_string( $value ) || '' === $value || self::KNOWN_PLACEHOLDER === $value ) { - return false; - } - } - - return true; + return array_all( + array( $logged_in_key, $logged_in_salt ), + fn( $value ) => is_string( $value ) && '' !== $value && self::KNOWN_PLACEHOLDER !== $value + ); } /** diff --git a/src/wp-includes/secrets/class-wp-secrets-libsodium-provider.php b/src/wp-includes/secrets/class-wp-secrets-libsodium-provider.php index d7dc205..dc360a1 100644 --- a/src/wp-includes/secrets/class-wp-secrets-libsodium-provider.php +++ b/src/wp-includes/secrets/class-wp-secrets-libsodium-provider.php @@ -320,7 +320,36 @@ public function set( string $name, $value, bool $network = false, bool $needs_ro return $master_key; } - $cipher = new WP_Secrets_Cipher(); + $cipher = new WP_Secrets_Cipher(); + $has_current = is_array( $existing ) && isset( $existing['current'] ) && is_array( $existing['current'] ); + $is_unchanged = false; + + if ( $has_current ) { + $current_value = $cipher->decrypt_value( $master_key, $scope, $site_id, $name, WP_Secret_Version::CURRENT, $existing['current'] ); + $is_unchanged = is_string( $current_value ) && hash_equals( $current_value, $value ); + + if ( is_string( $current_value ) ) { + wp_secrets_memzero( $current_value ); + } + } + + /* + * A write that would change nothing stored is skipped, and reports nothing + * to wp_secret_changed: a settings form that saves an unchanged value on + * every submit has not changed the secret. Rewriting the record would also + * reset the current slot's 'created' time and clear a 'needs_rotation' flag + * on a value that was never rotated. + * + * The exception is a write that raises 'needs_rotation', such as importing + * an option that holds the value already stored. That value has still sat + * in plaintext, so the flag is recorded, and handled as a rewrite below. + */ + if ( $is_unchanged && ( ! $needs_rotation || ! empty( $existing['current']['needs_rotation'] ) ) ) { + wp_secrets_memzero( $master_key ); + + return true; + } + $new_slot = $cipher->encrypt_value( $master_key, $scope, $site_id, $name, WP_Secret_Version::CURRENT, $value ); if ( is_wp_error( $new_slot ) ) { @@ -355,12 +384,24 @@ public function set( string $name, $value, bool $network = false, bool $needs_ro * than failing this write. The value was already unreadable before this * call; refusing to let an operator set a new one over it would repeat the * corrupted-record-blocks-everything failure this API is built to avoid. + * + * Saving the current value again, as a settings form does on every save, is + * not a rotation: demoting it would replace the previous value that a + * rotation in progress still depends on. The existing previous slot is kept + * instead, and can be carried over as stored because it is already bound to + * WP_Secret_Version::PREVIOUS. */ - if ( is_array( $existing ) && isset( $existing['current'] ) && is_array( $existing['current'] ) ) { - $demoted = $this->demote_slot( $cipher, $master_key, $scope, $site_id, $name, $existing['current'] ); - - if ( ! is_wp_error( $demoted ) ) { - $record['previous'] = $demoted; + if ( $has_current ) { + if ( $is_unchanged ) { + if ( isset( $existing['previous'] ) && is_array( $existing['previous'] ) ) { + $record['previous'] = $existing['previous']; + } + } else { + $demoted = $this->demote_slot( $cipher, $master_key, $scope, $site_id, $name, $existing['current'] ); + + if ( ! is_wp_error( $demoted ) ) { + $record['previous'] = $demoted; + } } } @@ -650,7 +691,7 @@ public function list_secrets( string $name_prefix = '', bool $network = false ) $entries = array(); foreach ( $names as $name ) { - if ( '' !== $name_prefix && 0 !== strpos( $name, $name_prefix . '/' ) ) { + if ( '' !== $name_prefix && ! str_starts_with( $name, $name_prefix . '/' ) ) { continue; } diff --git a/src/wp-includes/secrets/interface-wp-secrets-provider.php b/src/wp-includes/secrets/interface-wp-secrets-provider.php index bd11a88..d1916aa 100644 --- a/src/wp-includes/secrets/interface-wp-secrets-provider.php +++ b/src/wp-includes/secrets/interface-wp-secrets-provider.php @@ -112,6 +112,15 @@ public function get( string $name, string $version, bool $network = false ); * the situation it matters most, so WP_Secrets_Provider_Conformance checks it. * A write that fails fires nothing. * + * Writing the value a secret already holds is not a rotation. A settings form + * that saves every field on every submit does this routinely, so a provider + * that keeps a previous version must not discard it for an unchanged write; + * doing so would end a rotation's grace period early, with every consumer + * still on the old credential losing access. WP_Secrets_Provider_Conformance + * checks this. Treating the write as a no-op that fires nothing is preferred, + * but a provider that cannot tell the value is unchanged without an additional + * read may write it and report it. + * * @since 7.2.0 * * @param string $name The secret's name. diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 265c467..c28808c 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -45,12 +45,21 @@ function _secrets_api_manually_load_plugin() { require $_tests_dir . '/includes/bootstrap.php'; -require_once __DIR__ . '/includes/trait-wp-secrets-assertions.php'; -require_once __DIR__ . '/includes/class-mock-store.php'; -require_once __DIR__ . '/includes/class-mock-keyring.php'; +/* + * A test library from a WordPress that ships the Secrets API (trunk since r64337) + * already loaded core's copies of these from its own bootstrap. Loading the + * plugin's as well would redeclare them. + */ +if ( ! file_exists( $_tests_dir . '/includes/secrets/trait-wp-secrets-assertions.php' ) ) { + require_once __DIR__ . '/includes/trait-wp-secrets-assertions.php'; + require_once __DIR__ . '/includes/class-mock-store.php'; + require_once __DIR__ . '/includes/class-mock-keyring.php'; + require_once __DIR__ . '/includes/class-wp-secrets-provider-conformance.php'; + require_once __DIR__ . '/includes/class-wp-secrets-keyring-conformance.php'; +} + require_once __DIR__ . '/includes/class-legacy-fixture-writer.php'; -require_once __DIR__ . '/includes/class-wp-secrets-provider-conformance.php'; -require_once __DIR__ . '/includes/class-wp-secrets-keyring-conformance.php'; +require_once __DIR__ . '/includes/trait-secrets-api-plugin-layer-tests.php'; /* * The migrator loads under WP-CLI only, which the mock above satisfies. The diff --git a/tests/includes/class-mock-keyring.php b/tests/includes/class-mock-keyring.php index 159f0b1..1272cad 100644 --- a/tests/includes/class-mock-keyring.php +++ b/tests/includes/class-mock-keyring.php @@ -61,7 +61,7 @@ public function unwrap( string $wrapped ) { return new WP_Error( WP_SECRETS_ERROR_KEY_UNAVAILABLE, 'Mock_Keyring: unwrap() configured to fail.' ); } - if ( 0 !== strpos( $wrapped, self::MARKER ) ) { + if ( ! str_starts_with( $wrapped, self::MARKER ) ) { return new WP_Error( WP_SECRETS_ERROR_KEY_UNAVAILABLE, 'Mock_Keyring: not a value this keyring wrapped.' ); } diff --git a/tests/includes/class-mock-store.php b/tests/includes/class-mock-store.php index bef4fca..784a1db 100644 --- a/tests/includes/class-mock-store.php +++ b/tests/includes/class-mock-store.php @@ -40,7 +40,7 @@ public function get( string $name, bool $network = false ) { $key = $this->key( $name, $network ); - return array_key_exists( $key, $this->records ) ? $this->records[ $key ] : null; + return $this->records[ $key ] ?? null; } public function set( string $name, array $record, bool $network = false ) { @@ -74,7 +74,7 @@ public function list_names( bool $network = false ) { $names = array(); foreach ( array_keys( $this->records ) as $key ) { - if ( 0 === strpos( $key, $prefix ) ) { + if ( str_starts_with( $key, $prefix ) ) { $names[] = substr( $key, strlen( $prefix ) ); } } diff --git a/tests/includes/class-wp-secrets-provider-conformance.php b/tests/includes/class-wp-secrets-provider-conformance.php index 1a18062..98feab7 100644 --- a/tests/includes/class-wp-secrets-provider-conformance.php +++ b/tests/includes/class-wp-secrets-provider-conformance.php @@ -236,6 +236,39 @@ public function test_a_fingerprint_is_stable_for_the_same_value(): void { $this->assertSame( $first, $second ); } + /** + * A settings form saves every field on every submit, so writing the value a + * secret already holds is routine. A provider that treats it as a rotation + * discards the previous version while consumers may still be using it. + * + * Compared by fingerprint rather than by value, so a provider that withholds + * values is checked too. Only asked of a provider that keeps a previous + * version; one with no history has nothing to lose. + * + * @ticket 66353 + */ + public function test_writing_an_unchanged_value_keeps_the_previous_version(): void { + $provider = $this->provider(); + $this->require_writable( $provider ); + + $name = $this->conformance_name(); + $provider->set( $name, 'first-value' ); + $provider->set( $name, 'second-value' ); + + $previous = $provider->get( $name, WP_Secret_Version::PREVIOUS ); + + if ( ! $previous instanceof WP_Secret ) { + $this->markTestSkipped( 'Provider keeps no previous version; there is nothing to keep.' ); + } + + $this->assertNotWPError( $provider->set( $name, 'second-value' ) ); + + $kept = $provider->get( $name, WP_Secret_Version::PREVIOUS ); + + $this->assertInstanceOf( 'WP_Secret', $kept, 'Writing an unchanged value must not discard the previous version.' ); + $this->assertSame( $previous->fingerprint(), $kept->fingerprint(), 'Writing an unchanged value must not replace the previous version.' ); + } + public function test_a_deleted_secret_reads_as_absent(): void { $provider = $this->provider(); $this->require_writable( $provider ); diff --git a/tests/includes/trait-secrets-api-plugin-layer-tests.php b/tests/includes/trait-secrets-api-plugin-layer-tests.php new file mode 100644 index 0000000..d64ac3a --- /dev/null +++ b/tests/includes/trait-secrets-api-plugin-layer-tests.php @@ -0,0 +1,40 @@ +getFileName(); + + return false !== $declared_in && realpath( $declared_in ) === realpath( ABSPATH . WPINC . '/secrets.php' ); + } +} diff --git a/tests/phpstan/compat-stubs.php b/tests/phpstan/compat-stubs.php new file mode 100644 index 0000000..a760bb4 --- /dev/null +++ b/tests/phpstan/compat-stubs.php @@ -0,0 +1,45 @@ +write_secret( 'api_key', 'sk_legacy_value' ); diff --git a/tests/phpunit/test-secrets-api-migrator.php b/tests/phpunit/test-secrets-api-migrator.php index 3a9e649..afda05e 100644 --- a/tests/phpunit/test-secrets-api-migrator.php +++ b/tests/phpunit/test-secrets-api-migrator.php @@ -7,6 +7,8 @@ */ class Tests_Secrets_ApiMigrator extends WP_UnitTestCase { + use Secrets_API_Plugin_Layer_Tests; + private function migrator() { return new Secrets_API_Migrator(); } diff --git a/tests/phpunit/test-secrets-api-prototype-fallback-store.php b/tests/phpunit/test-secrets-api-prototype-fallback-store.php index 9e6f6c7..e2cfb8d 100644 --- a/tests/phpunit/test-secrets-api-prototype-fallback-store.php +++ b/tests/phpunit/test-secrets-api-prototype-fallback-store.php @@ -15,6 +15,8 @@ */ class Tests_Secrets_ApiPrototypeFallbackStore extends WP_UnitTestCase { + use Secrets_API_Plugin_Layer_Tests; + private function store() { return new Secrets_API_Prototype_Fallback_Store( new WP_Secrets_Option_Store() ); } diff --git a/tests/phpunit/test-secrets-noop-gate.php b/tests/phpunit/test-secrets-noop-gate.php index d184fa1..c0682a3 100644 --- a/tests/phpunit/test-secrets-noop-gate.php +++ b/tests/phpunit/test-secrets-noop-gate.php @@ -65,6 +65,28 @@ public function test_a_wordpress_newer_than_the_core_version_also_no_ops() { $this->assertSame( 10, has_action( 'admin_notices', 'wp_secrets_api_notice_superseded' ) ); } + /** + * Betas, RCs, and trunk report a pre-release version, which version_compare() + * ranks below the release itself. A 7.2 beta that ships the API is core, not a + * conflict. + */ + public function test_a_prerelease_of_the_core_version_also_no_ops() { + $GLOBALS['wp_version'] = WP_SECRETS_API_CORE_VERSION . '-beta1-64606-src'; + + wp_secrets_api_bootstrap(); + + $this->assertSame( 10, has_action( 'admin_notices', 'wp_secrets_api_notice_superseded' ) ); + $this->assertFalse( has_action( 'admin_notices', 'wp_secrets_api_notice_conflict' ) ); + } + + public function test_a_prerelease_of_an_earlier_version_is_still_a_conflict() { + $GLOBALS['wp_version'] = '7.1-RC1'; + + wp_secrets_api_bootstrap(); + + $this->assertSame( 10, has_action( 'admin_notices', 'wp_secrets_api_notice_conflict' ) ); + } + // – something else claimed the symbol ------------------------------------- /** diff --git a/tests/phpunit/test-secrets-version-slots.php b/tests/phpunit/test-secrets-version-slots.php index 274853b..b4262ab 100644 --- a/tests/phpunit/test-secrets-version-slots.php +++ b/tests/phpunit/test-secrets-version-slots.php @@ -57,6 +57,86 @@ public function test_third_write_discards_the_oldest_value(): void { $this->assertNeverContainsPlaintext( 'value-a', get_option( '_wp_secret_myplugin/api-key' ) ); } + /** + * Saving the current value again, as a settings form does, must not discard + * the previous value a rotation still depends on. + * + * @ticket 66353 + */ + public function test_rewriting_the_current_value_keeps_the_previous_slot(): void { + wp_set_secret( 'myplugin/api-key', 'first-value' ); + wp_set_secret( 'myplugin/api-key', 'second-value' ); + wp_set_secret( 'myplugin/api-key', 'second-value' ); + + $this->assertRecordSlotDecryptsTo( 'myplugin/api-key', WP_Secret_Version::PREVIOUS, 'first-value' ); + $this->assertRecordSlotDecryptsTo( 'myplugin/api-key', WP_Secret_Version::CURRENT, 'second-value' ); + } + + /** + * @ticket 66353 + */ + public function test_rewriting_the_current_value_leaves_the_stored_record_untouched(): void { + wp_set_secret( 'myplugin/api-key', 'first-value' ); + wp_set_secret( 'myplugin/api-key', 'second-value' ); + $before = get_option( '_wp_secret_myplugin/api-key' ); + + $this->assertTrue( wp_set_secret( 'myplugin/api-key', 'second-value' ) ); + + $this->assertSame( $before, get_option( '_wp_secret_myplugin/api-key' ) ); + } + + /** + * @ticket 66353 + */ + public function test_rewriting_the_current_value_does_not_fire_the_change_hook(): void { + wp_set_secret( 'myplugin/api-key', 'value' ); + + $fired = false; + add_action( + 'wp_secret_changed', + function () use ( &$fired ) { + $fired = true; + } + ); + + wp_set_secret( 'myplugin/api-key', 'value' ); + + $this->assertFalse( $fired ); + } + + /** + * Re-saving an imported value through a settings form has not rotated it. + * + * @ticket 66353 + */ + public function test_rewriting_the_current_value_keeps_its_needs_rotation_flag(): void { + update_option( 'my_plugin_api_key', 'imported-value' ); + wp_import_option_as_secret( 'my_plugin_api_key', 'myplugin/api-key' ); + + wp_set_secret( 'myplugin/api-key', 'imported-value' ); + + $this->assertTrue( $this->stored_slot( WP_Secret_Version::CURRENT )['needs_rotation'] ); + } + + /** + * Importing an option that holds the value already stored still means that + * value sat in plaintext, so it is flagged, without discarding the previous + * version. + * + * @ticket 66353 + */ + public function test_importing_the_current_value_flags_it_and_keeps_the_previous_slot(): void { + wp_set_secret( 'myplugin/api-key', 'first-value' ); + wp_set_secret( 'myplugin/api-key', 'second-value' ); + update_option( 'my_plugin_api_key', 'second-value' ); + + $this->assertTrue( wp_import_option_as_secret( 'my_plugin_api_key', 'myplugin/api-key' ) ); + + $this->assertTrue( $this->stored_slot( WP_Secret_Version::CURRENT )['needs_rotation'] ); + $this->assertRecordSlotDecryptsTo( 'myplugin/api-key', WP_Secret_Version::PREVIOUS, 'first-value' ); + $this->assertRecordSlotDecryptsTo( 'myplugin/api-key', WP_Secret_Version::CURRENT, 'second-value' ); + } + public function test_previous_preserves_its_original_fingerprint(): void { wp_set_secret( 'myplugin/api-key', 'first-value' ); $original = wp_get_secret( 'myplugin/api-key' ); diff --git a/tests/phpunit/test-wp-cli-secret-command.php b/tests/phpunit/test-wp-cli-secret-command.php index 3bb07f8..cb37455 100644 --- a/tests/phpunit/test-wp-cli-secret-command.php +++ b/tests/phpunit/test-wp-cli-secret-command.php @@ -7,6 +7,8 @@ */ class Tests_Secrets_WPCLISecretCommand extends WP_UnitTestCase { + use Secrets_API_Plugin_Layer_Tests; + public function set_up() { parent::set_up(); diff --git a/tests/phpunit/test-wp-cli-secret-network-command.php b/tests/phpunit/test-wp-cli-secret-network-command.php index ba7264b..f086ec2 100644 --- a/tests/phpunit/test-wp-cli-secret-network-command.php +++ b/tests/phpunit/test-wp-cli-secret-network-command.php @@ -6,6 +6,8 @@ */ class Tests_Secrets_WPCLISecretNetworkCommand extends WP_UnitTestCase { + use Secrets_API_Plugin_Layer_Tests; + public function set_up() { parent::set_up();