Secrets API: A failed read of a secret's record reads as a missing secret

[ericmann]

Follow-up to [64337] (#66187). Related: #66413.

WP_Secrets_Option_Store::get() reads a record with get_option( $name, null ) (or get_site_option()). Those return the default both when the option doesn't exist and when the query fails. So a failed read, such as a killed query, a lost connection or a replica that is behind, looks like "this secret does not exist":

• wp_get_secret() returns null instead of WP_Error. That breaks the three-state contract, which reserves null for a secret that doesn't exist. A caller that sees null may ask for the credential again or create a new one.
• wp_set_secret() treats the secret as new. It returns true and writes a record with no previous version, so a rotation in progress loses the value it depends on.

#66413 is the same problem for the root key.

Reproduced on trunk, single site. The first SELECT of the record fails once, through a query filter:

before: current two, previous one
wp_get_secret() during a failed read: NULL
wp_set_secret( three ) during a failed read: true
after: current three, previous NULL

Possible fix: when get_option() returns the default, check against the database the way #66413's patch does for the root key. Return secret_store_unavailable if that read fails too. The write path already treats secret_store_unavailable from the store as fatal.

AI assistance: Claude Code (Claude Opus 5.5) for finding this and writing the reproduction. I ran it and reviewed the results.

WP_Secrets_Option_Store::get() read a record with get_option( $name, null ) (or get_site_option()). Those return the default both when the option does not exist and when their query failed, so a failed read looked like "this secret does not exist":

• wp_get_secret() returned null instead of WP_Error, which breaks the three-state contract WP_Secrets_Store::get() documents (the record if it exists, null if it does not, WP_Error if it could not be determined which). A caller that sees null may ask for the credential again or create a new one.
• wp_set_secret() treated the secret as new. It returned true and wrote a record with no previous version, so a rotation in progress lost the value it depends on.

When the options API reports the record as missing, get() now reads the row directly to tell absence from a failed read, and returns secret_store_unavailable if that read fails too. WP_Secrets_Libsodium_Provider::read_prior_record() already treats that code from the store as fatal, so the write aborts before anything is written.

Two notes on the implementation:

• Failure comes from wpdb::query()'s return value, not wpdb::$last_error. last_error cannot carry this: when the server has gone away and check_connection() fails, wpdb::query() returns false after flush() has cleared the error string and without setting a new one, so a lost connection — the case this ticket is really about — would still have read as a missing secret. wpdb::query() returns false on every failed read and a row count on every successful one.
• The helper returns false for "no row", so a row that exists while holding no value is reported as a malformed record rather than as absence. wp_options.option_value is NOT NULL, but wp_sitemeta.meta_value is nullable, so this is reachable for network-scope secrets.

This is the same shape as the root-key fix on #66413 (PR #15186), kept local to the option store. It deliberately does not prune the cache of options that do not exist, because get() does the direct read on every null, so a stale "missing" entry cannot make the store report absence.

Testing

--group 66424, 6 new tests:

• Tests_Secrets_WPSecretsOptionStore: test_get_returns_an_error_when_the_record_read_fails, ..._when_a_network_record_read_fails, ..._when_the_read_never_reaches_the_database, test_get_returns_an_error_for_a_network_row_with_no_value (ms-required)
• Tests_Secrets_ThreeStateContract: test_a_failed_record_read_is_wp_error_not_null, test_a_failed_record_read_does_not_let_a_write_report_success

Each fails on trunk. The write test is the clearest: on trunk wp_set_secret() returns true during a failed read.

• --group 66424: 6 tests, 21 assertions on multisite; 5 tests, 18 assertions on single site.
• --group secrets: 392 tests single site, 393 multisite, no failures.
• --group option: 446 tests, no failures.
• PHPCS clean on the three changed files. PHPStan clean on the changed source file.

Query counts for the store, measured with $wpdb->num_queries: an absent secret costs one extra read (2 instead of 1 on the first call in a request, 1 instead of 0 on a repeat, since the direct read does not use the notoptions cache). A stored secret is unchanged: 1 uncached, 0 cached.

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Writing the tests and the patch, and running the checks above. I reviewed every line and checked the results myself.

[dhrupo]

Patch and tests are in PR #15301.

One note on how the failure is detected, then four adjacent items I left out of the patch.

Detection. My first attempt used $wpdb->last_error to tell a failed read from an absent record. That misses the case this ticket is really about: when the server has gone away and check_connection() fails, wpdb::query() returns false after flush() has already cleared the error string, and without setting a new one, so last_error is empty and the secret still reads as missing. The patch takes failure from the return value of wpdb::query() instead, which is false on every failed read and a row count on every successful one. wpdb::$ready being false, and a query filter emptying the SQL, also return before flush(), so last_error can be stale in the other direction too. There is a regression test for this: it fails with the last_error check and passes with the result check.

read_stored_record() also returns false for "no row", so a row that exists while holding no value reports secret_record_malformed rather than absence. wp_options.option_value is NOT NULL, but wp_sitemeta.meta_value is nullable, so that is reachable for network-scope secrets.

Four things in the same class that the patch does not touch:

1. WP_Secrets_Option_Store::delete() keeps the same ambiguity: on a failed read delete_option() returns false, the get_option( $option_name, null ) existence check returns null, so $still_exists is false and delete() returns true. The public path is not affected once this patch lands, because WP_Secrets_Libsodium_Provider::delete() calls read_prior_record() first and now gets secret_store_unavailable, and the wp_secret_changed 'deleted' action is guarded by is_array( $existing ) so it does not fire either way. So this is the store's own contract being looser than the interface says, not a user-facing bug.
2. list_names() has an unreachable failure guard. wpdb::get_col() initialises $new_array = array() and always returns an array, including when the query failed, so ! is_array( $option_names ) is never true. A failed listing returns array(): wp_list_secrets() shows no secrets, and the Site Health check that deliberately keeps undecryptable records visible sees nothing.
3. A failed read leaves the record marked in the notoptions cache and the patch does not prune it. get() stays correct, because it does the direct read on every null, but get_option() keeps short-circuiting for the rest of the request, so update_option() on the next write routes through add_option() instead of the update path and skips the update_option_* and updated_option actions. The patch on #66413 prunes its own entry; if both land as they are, core ends up with two near-identical helpers for that, which is why I left it alone here.
4. The direct read bypasses the Options API filter chain. pre_option_{$option}, option_{$option} and default_option_{$option} can all legitimately produce null, and the SELECT ignores them, so a filtered or drop-in options backend that leaves a stale wp_options row behind could see get() resurrect a retired record.

2 is a two-line fix in the same file, and 1 is small too. 3 and 4 are design calls about how this store relates to the Options API, so they are yours to make. Happy to patch any of them, here or on separate tickets, whichever you prefer.

AI assistance: Claude Code (Claude Opus 5) for the patch, the tests and this comment. I reviewed every line, ran the tests, and checked each claim against core myself.

get_option() and get_site_option() return the default both when an option does not exist and when their query fails, and they cache the option as missing either way. WP_Secrets_Option_Store::get() read that as an absent secret, so during a failed read:

• wp_get_secret() returned null for a stored secret.
• wp_set_secret() treated the secret as new, returned true, and wrote a record with no previous version.

When the Options API returns the default, get() now checks the database directly:

• If the row exists, it returns the record and clears the cached "missing" entry.
• If there is no row, it returns null.
• If the read fails too, it returns secret_store_unavailable, which the write path already treats as fatal.

Failure is taken from the return value of wpdb::query(), because wpdb::$last_error is not set on every failure path.

delete() now uses the same check, so it no longer reports success when it cannot tell whether the record still exists.

Relationship to #15301

#15301 was opened on this ticket first and has the same core approach. This PR differs in two ways:

1. It clears the cached "missing" entry. #15301 leaves that entry in place on purpose. On multisite, the next update_site_option() then goes through add_network_option(), which inserts a second wp_sitemeta row, and later requests read the old one. I ran the same scenario against both: store one, then two, fail one read, then write three to a network secret, then flush the cache and read it back.

| | wp_set_network_secret() | Rows | Same request | After cache flush |
|---|---|---|---|---|
| #15301 (1ef7cc819c) | true | 2 | three | two |
| This PR | true | 1 | three | three |

2. It also fixes delete(), which the Trac comment on #15301 lists as left out of that patch.

The handling of a row with an empty or NULL value comes from that same comment and PR, and I had missed that case. If the committer prefers #15301, the cache cleanup and the test that flushes the cache could move there instead.

Known limits

• Replica lag is not fixed. The ticket lists "a replica that is behind", but that query succeeds with no rows, so it still looks like a missing secret.
• One extra query per miss. Every read of a missing secret now reaches the database, even with a persistent object cache, because the cached "missing" entry can't be trusted.
• The direct read skips Options API filters. It only runs after the Options API returned the default.
• The cache cleanup overlaps with #15186. That PR has a similar helper for the root key, so a shared one may make sense once both land.
• list_names() is not changed. wpdb::get_col() always returns an array, so its error check can't fire and a failed query looks like an empty list.

Testing

The new tests make the record's SELECT fail through a query filter and run in both scopes, plus one multisite-only test. To run them:

vendor/bin/phpunit --group 66424
vendor/bin/phpunit -c tests/phpunit/multisite.xml --group 66424

• The initial tests failed on trunk before the fix. Removing the cache cleanup makes tests fail again.
• --group secrets has no failures: 399 tests on single site and 400 on multisite, on PHP 8.2 and MySQL 8.4.
• PHPCS is clean on both changed files. PHPStan is clean on the changed source file.

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5.5
Used for: Reproducing the bug, writing the patch and tests, running the checks and the comparison above, and drafting this description.

[ericmann]

Thanks @dhrupo and @MicahelE, two patches in two days.

Both take the same approach: when the Options API returns the default, read the row directly and take failure from wpdb::query()'s return value. I'd like to go with ​PR 15329 as the base, for the reason @MicahelE gives. A failed read leaves the record in notoptions, and on multisite the next update_site_option() then goes through add_network_option(). That function skips its existence check for an option in notoptions and inserts a second wp_sitemeta row, which has no unique key. I checked that path in option.php on trunk. Rotating a network secret after one failed read would leave two records, and later requests can read the old one. So the cache entry has to be cleared, not only bypassed.

Props should go to both of you. @dhrupo found the last_error problem and the empty meta_value case, and both are in 15329.

On the four items from comment:2:

1. delete(): fixed in 15329.
2. list_names(): please fold it into 15329. It's the same bug: a failed listing reads as "no secrets", and the Site Health test that keeps undecryptable records visible then sees nothing. Taking failure from $wpdb->query(), as get() now does, would fix it.
3. Pruning notoptions: needed, as above. 15186 (#66413) has a similar helper for the root key. Whichever lands second can use the other's, or a committer can merge them on commit. I don't think that should hold either up.
4. Bypassing the Options API filters: I'd accept this and say so in the docblock. The direct read only runs after the Options API has returned the default. A host that keeps options somewhere else should replace the store through secrets.php, not filter the default store.

Replica lag is out of scope here. That read succeeds with no rows, and telling it apart is the database layer's job.

@dhrupo, could we close 15301 in favor of 15329? I'll review 15329 on GitHub.

Closing in favour of #15329, as agreed on the ticket: https://core.trac.wordpress.org/ticket/66424#comment:4

[dhrupo]

Agreed, thanks @ericmann and @MicahelE. The notoptions point is right: I left that entry in place to avoid a second helper next to the one in PR 15186, and missed that add_network_option() skips its existence check for it. I've closed PR 15301 in favour of PR 15329.