From a4186018c264ccab9bfe6104c9cd941034cb6c43 Mon Sep 17 00:00:00 2001 From: "Matthew M. Emma" Date: Sat, 8 Aug 2026 06:39:02 -0700 Subject: [PATCH] Stop the settings sanitizer corrupting the stored client secret MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit register_setting() attaches Settings::sanitize() to the sanitize_option_{$option} filter, and WordPress runs that filter on every update_option() for the option — not just on the settings form submit. That includes this plugin's own internal writes in Settings::set() and Settings::migrate_legacy(). Because the sanitizer encrypted the client secret unconditionally, each of those writes re-encrypted the already-encrypted value, one layer at a time. After a couple of saves the secret no longer decrypted, and the site lost its Procore connection with no indication why. Measured in a live install: after the admin screen registered the setting, a single migrate_legacy() write produced a triple-encrypted value. Encryption is now idempotent at that boundary. Encryption::is_encrypted() recognises cipher text by its prefix, and sanitize() passes such a value through untouched while still encrypting genuine plaintext from the form. Two regression tests cover it, and both were confirmed to fail against the previous behaviour before the fix was applied: - sanitize() fed its own output repeatedly still yields the same plaintext - a settings write followed by three internal writes leaves the secret usable This was only reachable with the setting registered, which is every real admin request but was not the case in the earlier integration run — that run never called register_setting(), which is why it passed. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 37 ++++++++++++++++++++++++++++ languages/procore-connect.pot | 6 ++--- procore-connect.php | 4 ++-- readme.txt | 12 +++++++++- src/Admin/Settings.php | 18 +++++++++++--- src/Support/Encryption.php | 17 +++++++++++++ tests/unit/SettingsTest.php | 45 +++++++++++++++++++++++++++++++++++ 7 files changed, 130 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03b0d3d..8c50f38 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,42 @@ All notable changes to Procore Connect are documented here. The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [2.0.2] — 2026-08-08 + +### Fixed + +- **Saved credentials could be corrupted beyond recovery.** `register_setting()` + attaches the settings sanitizer to `sanitize_option_{$option}`, and WordPress runs + that filter on *every* `update_option()` for the option — including this plugin's own + internal writes in `Settings::set()` and `Settings::migrate_legacy()`. The sanitizer + encrypted the client secret unconditionally, so each of those writes re-encrypted the + stored cipher text, one layer at a time. After a couple of saves the secret could no + longer be decrypted and the site silently lost its Procore connection, with no way to + recover the value short of re-entering it. + + Encryption is now idempotent at that boundary: a value that is already cipher text is + passed through untouched. Added `Encryption::is_encrypted()` and two regression tests + that fail against the previous behaviour. + + This was found by running the plugin in a real WordPress install with the setting + registered, which is the only configuration where the faulty path is reachable — the + earlier integration run never called `register_setting()`, so it missed it. + +### Added + +- 18 tests covering both OAuth grants and the token store: token-endpoint host, token + reuse, CSRF `state` rejection and replay, refresh-token rotation, the reconnect path + when a refresh is rejected, encryption at rest, expiry margin, environment isolation + and lock exclusivity. +- `.wordpress-org/` banner and icon assets for a WordPress.org submission, excluded from + the release ZIP. + +### Removed + +- The `== Screenshots ==` section of `readme.txt`, which declared six files that were + never produced and would have rendered as six broken slots on the plugin page. + `.wordpress-org/README.md` records what to capture from a real Procore-connected site. + ## [2.0.1] — 2026-08-08 Four defects found by running the plugin inside a real WordPress install @@ -152,6 +188,7 @@ every install was non-functional regardless of configuration. - Initial release. +[2.0.2]: https://github.com/ibuilder/ProcoreWP/releases/tag/v2.0.2 [2.0.1]: https://github.com/ibuilder/ProcoreWP/releases/tag/v2.0.1 [2.0.0]: https://github.com/ibuilder/ProcoreWP/releases/tag/v2.0.0 [1.0.0]: https://github.com/ibuilder/ProcoreWP/releases/tag/v1.0.0 diff --git a/languages/procore-connect.pot b/languages/procore-connect.pot index 077639d..08a729c 100644 --- a/languages/procore-connect.pot +++ b/languages/procore-connect.pot @@ -575,7 +575,7 @@ msgstr "" msgid "No project was available to test against." msgstr "" -#: src/Frontend/Shortcodes/AbstractShortcode.php:392 +#: src/Frontend/Shortcodes/AbstractShortcode.php:395 msgid "No records were found." msgstr "" @@ -719,7 +719,7 @@ msgid "Procore Connect" msgstr "" #. translators: %s: error message from the Procore API. -#: src/Frontend/Shortcodes/AbstractShortcode.php:359 +#: src/Frontend/Shortcodes/AbstractShortcode.php:362 msgid "Procore Connect (visible to administrators only): %s" msgstr "" @@ -817,7 +817,7 @@ msgstr "" msgid "Project detail" msgstr "" -#: src/Frontend/Shortcodes/AbstractShortcode.php:366 +#: src/Frontend/Shortcodes/AbstractShortcode.php:369 #: src/Rest/Controller.php:209 msgid "Project information is temporarily unavailable." msgstr "" diff --git a/procore-connect.php b/procore-connect.php index dbdcd40..b1a163c 100644 --- a/procore-connect.php +++ b/procore-connect.php @@ -3,7 +3,7 @@ * Plugin Name: Procore Connect * Plugin URI: https://github.com/ibuilder/ProcoreWP * Description: Connect WordPress to the Procore construction management platform. Display projects, teams, drawings, RFIs and more with shortcodes, blocks and a cached REST proxy. - * Version: 2.0.1 + * Version: 2.0.2 * Requires at least: 6.5 * Requires PHP: 7.4 * Author: ibuilder @@ -22,7 +22,7 @@ defined( 'ABSPATH' ) || exit; -const VERSION = '2.0.1'; +const VERSION = '2.0.2'; define( 'PROCORE_CONNECT_VERSION', VERSION ); define( 'PROCORE_CONNECT_FILE', __FILE__ ); diff --git a/readme.txt b/readme.txt index e5729af..7a8eafb 100644 --- a/readme.txt +++ b/readme.txt @@ -4,7 +4,7 @@ Tags: procore, construction, project management, shortcode, api Requires at least: 6.5 Tested up to: 7.0 Requires PHP: 7.4 -Stable tag: 2.0.1 +Stable tag: 2.0.2 License: GPLv2 or later License URI: https://www.gnu.org/licenses/gpl-2.0.html @@ -127,6 +127,13 @@ Yes. Caching goes through the transient API, so a persistent object cache such a == Changelog == += 2.0.2 = + +* **Fixed a credential-corrupting bug.** WordPress runs a registered setting's sanitize callback on every update to that option, including the plugin's own internal writes. Because the sanitizer encrypted the client secret unconditionally, each write re-encrypted the stored value until it could no longer be decrypted and the site silently lost its Procore connection. Encryption is now idempotent. If your connection stopped working after saving settings, re-enter the Client Secret once on 2.0.2 and it will stay valid. +* Added 18 tests covering both OAuth grants, CSRF state handling and refresh-token rotation. +* Added WordPress.org banner and icon assets. +* Removed a Screenshots section that referenced files which did not exist. + = 2.0.1 = Fixes found by running the plugin inside a real WordPress install. @@ -167,6 +174,9 @@ A complete rewrite. See the upgrade notice below before updating. == Upgrade Notice == += 2.0.2 = +Important fix: repeated settings saves could re-encrypt the stored Client Secret until it became unrecoverable, silently breaking the Procore connection. Update, then re-enter your Client Secret once if the connection had stopped working. + = 2.0.1 = Bug fixes from real-world integration testing: pagination on single-record endpoints, the REST proxy ignoring the default company, translations loading too early, and WP-CLI subcommand names. No action required. diff --git a/src/Admin/Settings.php b/src/Admin/Settings.php index d5a0f1b..7c47e55 100644 --- a/src/Admin/Settings.php +++ b/src/Admin/Settings.php @@ -221,12 +221,24 @@ public static function sanitize( $input ): array { $clean['client_id'] = sanitize_text_field( (string) ( $input['client_id'] ?? '' ) ); - // An empty secret field means "leave the stored secret alone"; the form - // renders a masked placeholder rather than the real value. + /* + * An empty secret field means "leave the stored secret alone"; the form + * renders a masked placeholder rather than the real value. + * + * A value that is already cipher text is passed through untouched. + * register_setting() attaches this sanitizer to + * sanitize_option_{$option}, which WordPress runs on *every* + * update_option() for the option — including this plugin's own internal + * writes in set() and migrate_legacy(). Encrypting unconditionally would + * re-encrypt the stored value on each save, one layer at a time, until + * the credential could no longer be recovered. + */ $submitted_secret = trim( (string) ( $input['client_secret'] ?? '' ) ); if ( '' !== $submitted_secret ) { - $clean['client_secret'] = Encryption::encrypt( $submitted_secret ); + $clean['client_secret'] = Encryption::is_encrypted( $submitted_secret ) + ? $submitted_secret + : Encryption::encrypt( $submitted_secret ); } if ( ! empty( $input['clear_client_secret'] ) ) { diff --git a/src/Support/Encryption.php b/src/Support/Encryption.php index 0629981..d0b99ac 100644 --- a/src/Support/Encryption.php +++ b/src/Support/Encryption.php @@ -52,6 +52,23 @@ public static function is_strong(): bool { && in_array( self::CIPHER, (array) openssl_get_cipher_methods(), true ); } + /** + * Whether a value has already been through `encrypt()`. + * + * Callers use this to stay idempotent. `register_setting()` attaches the + * settings sanitizer to `sanitize_option_{$option}`, and WordPress runs + * that filter on *every* `update_option()` for the option — including the + * plugin's own internal writes. Encrypting unconditionally would therefore + * re-encrypt the stored cipher text on each save until the credential could + * no longer be recovered. + * + * @param string $value Candidate value. + * @return bool True when the value is already cipher text. + */ + public static function is_encrypted( string $value ): bool { + return 0 === strpos( $value, self::PREFIX ) || 0 === strpos( $value, self::FALLBACK_PREFIX ); + } + /** * Encrypt a value for storage. * diff --git a/tests/unit/SettingsTest.php b/tests/unit/SettingsTest.php index 6365731..c6c0434 100644 --- a/tests/unit/SettingsTest.php +++ b/tests/unit/SettingsTest.php @@ -68,6 +68,51 @@ public function test_encrypts_the_client_secret(): void { $this->assertSame( 'super-secret-value', Encryption::decrypt( $clean['client_secret'] ) ); } + /** + * Sanitizing must be idempotent, so a secret survives repeated saves. + * + * `register_setting()` attaches this sanitizer to `sanitize_option_{$option}`, + * and WordPress runs that on every `update_option()` for the option — + * including the plugin's own writes in `set()` and `migrate_legacy()`. + * Encrypting unconditionally re-encrypted the stored cipher text one layer + * per save until the credential was unrecoverable. + * + * @return void + */ + public function test_sanitize_does_not_re_encrypt_an_encrypted_secret(): void { + $once = Settings::sanitize( array( 'client_secret' => 'super-secret-value' ) ); + + $this->assertSame( 'super-secret-value', Encryption::decrypt( $once['client_secret'] ) ); + + // Feed the sanitizer its own output, as update_option() does. + $twice = Settings::sanitize( $once ); + $thrice = Settings::sanitize( $twice ); + + $this->assertSame( $once['client_secret'], $twice['client_secret'] ); + $this->assertSame( 'super-secret-value', Encryption::decrypt( $twice['client_secret'] ) ); + $this->assertSame( 'super-secret-value', Encryption::decrypt( $thrice['client_secret'] ) ); + } + + /** + * Writing settings programmatically must not corrupt the stored secret. + * + * @return void + */ + public function test_set_preserves_the_secret_through_the_sanitizer(): void { + $stored = Settings::sanitize( array( 'client_secret' => 'keep-me' ) ); + update_option( Settings::OPTION, $stored ); + Settings::flush(); + + // Simulate WordPress re-running the sanitizer on an internal write. + for ( $i = 0; $i < 3; $i++ ) { + $round = Settings::sanitize( Settings::all() ); + update_option( Settings::OPTION, $round ); + Settings::flush(); + } + + $this->assertSame( 'keep-me', Settings::client_secret() ); + } + /** * An empty secret field must leave the stored secret untouched, because the * form renders a mask rather than the real value.