Repository navigation
Key Encryption: Restore API keys on every site on network deactivation and before uninstall - #1118
maheshbohara wants to merge 10 commits into
Conversation
Every site in a network has its own master key, and the encryption provider keeps the first one it loads for the rest of the request. Secrets_Bridge::reset_provider() drops it so code that loops over sites reads and writes each site's secrets with that site's key.
…ivation The deactivation routine ignored the $network_wide argument and decrypted the current site only. Every other site with the experiment enabled was left with an empty connector option and its key locked in the secrets store. See WordPress#1117.
The connector API keys belong to the Connectors screen, not to this plugin. Uninstall deleted the encrypted copies on every site, which lost the key for good on any site the deactivation routine had not run for. Keys are left untouched when a site opts out with the wpai_remove_data_on_uninstall filter, as before. See WordPress#1117.
Network deactivation now restores plaintext keys on every site, so network activation has to schedule the re-encryption on every site too. Before, only the current site was flagged. See WordPress#1117.
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1118 +/- ##
=============================================
+ Coverage 82.11% 82.17% +0.06%
- Complexity 3394 3417 +23
=============================================
Files 136 137 +1
Lines 13178 13249 +71
=============================================
+ Hits 10821 10888 +67
- Misses 2357 2361 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
WordPress does not always pass a boolean to the activation and deactivation hooks. A null made the new bool parameter throw a TypeError and stopped the plugin from activating. See WordPress#1117.
…vation Activation and deactivation each had their own loop over the network's sites, and neither could run on single site, so the coverage job never reached them. Key_Encryption::for_each_site() now holds the loop and runs the same code for one site or for all of them. A callback that throws is skipped on single site too, so a key that cannot be decrypted no longer blocks deactivation. Uninstall no longer checks for wp_get_connectors(), which always exists on the supported WordPress versions, and has a test for a key that cannot be decrypted. See WordPress#1117.
| try { | ||
| $callback(); | ||
| } catch ( Throwable $e ) { | ||
| // A site whose secrets cannot be read must not keep the other sites |
There was a problem hiding this comment.
This is a behavior change compared to previous, where an error would stop the deactivation process. Seems we should consider at least showing an error message if an error happens since that means decryption failed and API keys are no longer accessible, so alerting the user would be nice
There was a problem hiding this comment.
@dkotter Good catch, that was not intended. Deactivation now stops again when a key cannot be decrypted, with a message that names the sites and connectors instead of an uncaught exception.
|
|
||
| // The API keys belong to the Connectors screen, not to this plugin, so they | ||
| // are put back before the encrypted copies are deleted with the options. | ||
| self::restore_encrypted_keys(); |
There was a problem hiding this comment.
Should this be moved before the above filter? The purpose of that filter is to allow sites to opt out of data removal, though technically this isn't removing data, it's restoring data. So seems like it may be best if this always runs, regardless of the result of that filter
There was a problem hiding this comment.
@dkotter Agreed, moved. Keys are now restored before the filter is checked, so a site that keeps the plugin's data gets its keys back too. I updated the filter's docblock and the test.
… decrypted A key that could not be decrypted was skipped without a word, and the plugin was deactivated anyway. It also kept the keys after it from being restored, although nothing was wrong with them. decrypt_all() now restores every key it can and reports the ones it could not. Deactivation still goes through every site, then stops with a message that names the sites and connectors concerned. The plugin stays active, so the keys that were restored are flagged to be encrypted again. See WordPress#1117.
A site that opted out with the wpai_remove_data_on_uninstall filter kept its keys encrypted after the plugin was deleted, with nothing left to decrypt them. Restoring a key is not removing data, so it now happens before the filter is checked. See WordPress#1117.
|
@maheshbohara a merge conflict to resolve, please and thanks! |
|
@jeffpaul Resolved the merge conflict. Thanks for the heads-up! |
3 participants
What?
Closes #1117
On multisite, restores every site's API keys when the plugin is network-deactivated with Key Encryption enabled, and stops uninstall from deleting keys it has not restored.
Why?
Deactivation::deactivation_callback()ignored the$network_wideargument and decrypted the current site only. Every other site with the experiment enabled was left with an emptyconnectors_*_api_keyoption and its key locked in_secret_ai/*.Uninstall::run()then deleted those rows on every site, so the keys were lost for good. Steps and database state are in #1117.How?
Key_Encryption::for_each_site(): runs a callback on the current site, or on every site when the routine runs for the whole network. Each site has its own_secrets_master_key, and the provider caches the first one it loads for the rest of the request, so the cache is dropped around each call. A callback that throws does not stop the other sites. What it threw is returned, keyed by site ID.deactivation_callback()accepts$network_wideand decrypts, throughfor_each_site(), each site that has the experiment enabled.decrypt_all()now restores every key it can and then throws aKey_Decryption_Exceptionthat lists the connectors it could not decrypt. If any site reports one, deactivation stops with awp_die()message that names the sites and connectors, as it stopped ondevelopwith an uncaught exception. The plugin stays active, so the keys that were restored are flagged to be encrypted again on the next request.activation_callback()accepts$network_wideand flags every site for re-encryption. Without this, a network deactivate and reactivate would leave the other sites' keys in plaintext while the experiment is still shown as enabled.wpai_remove_data_on_uninstallfilter is checked, so a site that keeps the plugin's data gets its keys back too. It covers sites the deactivation routine never ran for. A key that cannot be decrypted is skipped there, because stopping would abort the deletion halfway.helpers.phpis loaded there because the plugin is not bootstrapped during uninstall andSecrets_Bridgeneedsget_ai_connectors().Secrets_Bridge::reset_provider(): drops the cached master key. It callsSecrets_Manager::reset(), which the vendored SDK documents as "for testing only". I did not find another way to drop the cached key without changing the vendored code. I am happy to do this differently if you prefer.?bool $network_wide, because WordPress does not always pass a boolean.Two things to know:
developand is not changed here. I have a small follow-up that makes such a key read as not set, and I can add it to this PR or open a separate one.Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus
Used for: Investigation, implementation, tests, manual testing, and PR wording. I reviewed the changes and the test results, and I take responsibility for the contribution.
Testing Instructions
developit is back to "Set up".wp option get connectors_ai_openai_api_key --url=<site 2>prints the key, and there is no_secret_ai/openai_api_keyrow._secret_ai/openai_api_keyrows are back.connectors_ai_openai_api_key.To test the uninstall change on its own, deactivate with
wp plugin deactivate ai --network --skip-pluginsso the deactivation routine does not run, then delete the plugin from Network Admin > Plugins. Both sites keep their key.To test the message, damage one stored key on the second site, for example with
wp db query "UPDATE wp_2_options SET option_value='x' WHERE option_name='_secret_ai/openai_api_key'", then click Network Deactivate. The deactivation stops with a page that names the site and the connector, the plugin stays active, and the other keys on the network are restored and then encrypted again.To test the opt-out case, add
add_filter( 'wpai_remove_data_on_uninstall', '__return_false' );in a must-use plugin and repeat the uninstall steps above. The sites keep the plugin's data and get their keys back.I ran these steps on WordPress 7.1.2 multisite (wp-env, PHP 8.3) with dummy keys. Core checks a key against the provider when it is saved, so I answered that one request locally with a
pre_http_requestfilter. A real key does not need that.Automated:
ActivationTest,DeactivationTest,UninstallTestandKey_EncryptionTest.npm run test:phpandnpm run test:php:multisitepass (1786 tests each).composer lint, PHPStan,npm run typecheckandnpm run lint:jspass.get_sites()andrestore_current_blog(), which only run on multisite and are covered by the multisite jobs, the fallback for an error other than aKey_Decryption_Exceptionin the deactivation message, and theABSPATHguard of the new exception file.Changelog Entry