Repository navigation
Conversation
Copies src/ from the Secrets API feature plugin at v0.2.0 (https://github.com/ericmann/secrets-api), unchanged: the public functions, WP_Secret and WP_Secret_Version, the provider, store, and keyring interfaces with their shipped and fail-closed implementations, the cipher and key manager, and the Site Health tests. Nothing loads these files yet; the next commit wires them in.
- wp-settings.php loads the API next to application passwords, and loads the secrets.php drop-in just before must-use plugins, once every interface a drop-in implements exists. - wp_load_secrets_dropin() in load.php marks the drop-in broken when it throws or sets a provider, store, or keyring global of the wrong type, so the API fails closed instead of falling back to the default. - class-wp-site-health.php loads the Secrets API's Site Health tests, so every path that loads WP_Site_Health gets them. - populate_roles_720() grants manage_secrets to administrators, and upgrade_720() does the same for existing sites (db version 61900; the committer should set the real revision). - map_meta_cap() lists manage_network_secrets with the other network capabilities, which only super admins hold.
Ports the feature plugin's tests for everything under src/ into tests/phpunit/tests/secrets/, with its shared helpers (assertions trait, mock store and keyring, provider and keyring conformance suites) in tests/phpunit/includes/secrets/. The plugin-only tests (prototype upgrade path, WP-CLI, architecture, no-op gate) stay behind. wp-tests-config-sample.php ships placeholder salts, which the config keyring refuses by design, so the test bootstrap defines a fixed WP_SECRETS_KEY in the main PHPUnit process. Process-isolated tests set or unset the key themselves; the four that exercise the LOGGED_IN salt fallback skip when the salts are placeholders. Also registers manage_secrets and manage_network_secrets in the capability inventory test, and adds tests for populate_roles_720(), the upgrade_720() gate, and manage_network_secrets on multisite.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
PHP 8.5 deprecates ReflectionMethod::setAccessible() and ReflectionProperty::setAccessible(), which have had no effect since 8.1, so the four calls in the Secrets API tests errored on the PHP 8.5 jobs. They now use the same PHP_VERSION_ID < 80100 guard as the rest of the suite.
This was referenced Sep 28, 2026
Follows the layout abilities-api/ and ai-client/ use: the classes and interfaces sit in their own folder, and the functions file stays at wp-includes/secrets.php. The 13 files are renamed without edits, and wp-settings.php requires them from the new paths in the same order. See #66187.
Member
|
I ran PHPStan rule level 10 on the changes in this PR and there were quite a few opportunities for improvement. For the changes in Errors in
|
westonruter
reviewed
Oct 7, 2026
westonruter
reviewed
Oct 7, 2026
westonruter
reviewed
Oct 7, 2026
westonruter
reviewed
Oct 7, 2026
mindctrl
reviewed
Oct 7, 2026
Author
I appreciate the callouts sir! I need to get this resolved on the feature plugin as well to keep them in sync, so I'll start working on resolving this all ASAP. |
…n PHPStan PHPStan reads both constants from wp-config-sample.php, where they hold the placeholder phrase, and so treats any comparison against that phrase as always true. Their values differ on every install, like AUTH_SALT, which is already listed. The Secrets API refuses to derive a key from the placeholder salts, and its tests and the test bootstrap check for them. See #66187.
Addresses the PHPStan level 10 report on the pull request: 85 findings in src/ and 400 in the tests. In src/, most fixes are docblocks that say what an array holds, with @phpstan-return shapes where a list entry or a record slot has fixed keys. The code changes: - Decrypt results are checked with is_string() rather than against false, since sodium_compat's signatures allow more than string|false. - The cached root key is only served once it is known to be set. - The config keyring decodes a canonical base64 key once, instead of validating it and then decoding it again. - Demoting a slot keeps `created` only when it is an integer and stores `needs_rotation` as a boolean. - Listing skips a stored option name that is not a string. - The request-cached store, key manager, and provider are narrowed with instanceof rather than compared against null. - wp_secrets_memzero(), wp_load_secrets_dropin(), populate_roles_720(), and upgrade_720() declare a void return. - The two "documented in" references for wp_secret_changed name the file that documents the hook. Docblocks no longer put a blank line between @PARAM and @return. In the tests, values are narrowed with assertions before use, helpers and data providers are typed, and test methods declare a void return. A test that passes a wrong type on purpose, or pins a constant or an interface, carries an `@phpstan-ignore <identifier> (reason)` comment. The error-code test is now one case per constant. This is the feature plugin's src/ and tests at 0.2.2. See #66187.
westonruter
reviewed
Oct 7, 2026
westonruter
reviewed
Oct 7, 2026
westonruter
reviewed
Oct 7, 2026
Core does not pass 'default' to __() and its relatives for its own strings. The 101 calls in the Secrets API now leave the argument off, as the rest of core does. The strings are unchanged. See #66187.
…r firing it The provider fires wp_secret_changed, because only the provider has the prior fingerprint without another read. A drop-in provider that left the call out would silence a site's audit log, and nothing checked for that. The hook stays with the provider, and the requirement is now enforced: - WP_Secrets_Provider states it on delete() and retire_previous() as well as set(): fire once the change has succeeded, and nothing otherwise. - WP_Secrets_Provider_Conformance checks a write, an overridden action, a delete, a retired version, a network-scope change, and a refused write. It asks for every argument, so a provider that passes too few fails on the count. wp_secret_changed also gains a seventh argument, `bool $network`. A network secret and a site secret can share a name, and a listener could not tell which one had changed. A site-scope change belongs to the current site. This is the feature plugin's src/ and tests at 0.2.3. See #66187.
audrasjb
previously requested changes
Oct 8, 2026
audrasjb
left a comment
audrasjb
left a comment
Contributor
There was a problem hiding this comment.
First pass, some change requests :)
… hold Parameters and return values carry PHP 7.4 type declarations: the public functions, the three interfaces and their implementations, and the internal classes. A return that needs a union type, such as WP_Secret|null|WP_Error, stays in the docblock. The runtime checks the declarations replace are removed, with the tests that exercised them. A plaintext value stays untyped and checked: as `string`, PHP would coerce false to an empty secret and report success. The two Site Health filter callbacks stay untyped, since they receive whatever an earlier callback returned. Static types are narrowed where that holds: versions and slots, scopes, the protection boundary, Site Health statuses, and counts. A secret's name stays `string`, because the functions exist to validate one. This is the feature plugin's src/ and tests at 0.3.0. See #66187.
Comments used `--` as a dash. They now use an en dash, and the two messages that had one are reworded. Ten docblock continuation lines were indented one column too far. See #66187.
Trunk moved to PHPStan 2.3.0, which no longer reports method.impossibleType on the assertion in the memzero test. With reportUnmatchedIgnoredErrors on, the leftover ignore was itself an error at level 10. See #66187.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the Secrets API for 7.2: encrypted, versioned storage for credentials, with a
secrets.phpdrop-in for hosts. The ticket has the full description and the list of additions beyond the proposal. The documentation is at secretsapi.dev.What is in the patch
src/wp-includes/secrets.php, 13 class and interface files insrc/wp-includes/secrets/, and the Site Health tests insrc/wp-admin/includes/secrets-site-health.php. These are the feature plugin'ssrc/at v0.3.0, byte for byte.wp-settings.phploads the API, then loadswp-content/secrets.phpthrough a newwp_load_secrets_dropin()inload.php, just before must-use plugins.WP_Errorand never falls back to the default provider.class-wp-site-health.phploads the secrets Site Health tests.populate_roles_720()andupgrade_720()grantmanage_secretsto administrators.manage_network_secretsgoes with the network capabilities inmap_meta_cap().$wp_db_versionis a placeholder, 61900, which needs the real revision on commit.tests/phpunit/tests/secrets/, with helpers intests/phpunit/includes/secrets/, plus the capability inventory.WP_SECRETS_KEYwhenwp-tests-config.phphas the placeholder salts, which the default keyring refuses.tests/phpstan/base.neonaddsLOGGED_IN_KEYandLOGGED_IN_SALTtodynamicConstantNames, next toAUTH_SALT. Without it PHPStan reads the placeholder fromwp-config-sample.phpand treats the placeholder checks as always true.Changes from review
The patch was opened at plugin v0.2.0. Since then:
src/wp-includes/secrets/, followingabilities-api/andai-client/, per comment 5 on the ticket. The functions file stays atsrc/wp-includes/secrets.php.// @phpstan-ignore <identifier> (reason).WP_Secret|null|WP_Error, stays in the docblock. A plaintext value stays untyped and checked, becausestring $valuewould let PHP coercefalseinto an empty secret. See ADR 0013.string.wp_secret_changedpassesbool $networkas a seventh argument. The provider still fires the hook, and the provider conformance suite now tests that it does. See ADR 0012.--.Testing
npm run test:php -- --group secretspasses on single site and multisite.tests/phpunit/tests/user/capabilities.phppasses.vendor/bin/phpstan analysepasses with core's configuration, and the Secrets API files and tests pass at level 10.Trac ticket: https://core.trac.wordpress.org/ticket/66187
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5.5, Claude Sonnet 5, Claude Fable 5.1
Used for: Most of the code and tests, in the feature plugin and in this port: the API, the core wiring, the test port, and review passes. I directed the design, and the decisions are recorded as ADRs at secretsapi.dev.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.